From fb9c2f89be85f4c02e7d265bfc06472ff9d3e4e1 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sun, 23 Aug 2026 13:28:07 -0700 Subject: [PATCH] fix: budget the drain against the container's own healthcheck MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rolling deploy marks a container unhealthy, waits for the runtime to notice, and only then stops it — so the proxy has stopped routing before the container goes away. The wait was budgeted as retries × the cadence Onebox polls `docker inspect` at, while the flip it waits for happens at retries × the interval the container's own healthcheck runs at. Those are different jobs: one is a local query, the other a probe inside the container. They agreed only when health.interval was authored. With the shorthand `health: /path` no interval was written into the generated healthcheck at all, so the runtime applied its own 30s default while Onebox budgeted against its 2s poll cadence: a flip needing 90s against a 10s budget, timing out on every replica of every deploy, and stopping each container while the proxy might still be routing to it — precisely what the budget exists to prevent. The budget now comes from the draining container itself, read back with the same inspect that already checks whether its check is drain-guarded. A healthcheck is baked in at creation, so the spec being deployed describes the containers being started, never the ones being drained. Budgeting from the spec would leave the failure intact for one more deploy after any change to the probe timing — including a change to Onebox's own default, which no operator asked for and which would otherwise strand every replica of the first deploy after an upgrade. Alongside that, the generated healthcheck stops leaving anything to the runtime's defaults. Interval, retries and start period are all written down, so what Onebox reasons about and what the container does cannot drift apart. An unset interval becomes 5s rather than the runtime's 30s: a probe every five seconds costs twelve requests a minute per container and lets a drained one leave rotation in fifteen, which keeps a rolling deploy from being dominated by waiting for the flip. Writing down a fast interval is only safe with a grace period to match. Before, the runtime's 30s interval gave a booting container roughly a minute and a half before a failed probe could count against it; a 5s interval with no start period would call a slow-starting application unhealthy while it is still coming up, and that verdict is visible to dependency conditions, restart watchers and alerting long before the rollout would notice. The start period is therefore written too, at 30s, which is the delay the first probe already had. The runtime leaves it at the first success, so a fast application pays none of it. The readiness budget stretches to cover one full flip cycle when the probe timing needs it. 120s is a figure chosen for ordinary timings, and with `interval: 3m` the newcomer's first probe lands after the budget expires — the rollout would remove a container that was never given the chance to answer. An authored `within` is still taken at its word. Retries gains an upper bound. It multiplies the probe interval to give the budget, and a count large enough to overflow that arithmetic yields a negative budget: one that expires immediately, which is the original failure reached by another route. Validating that retries × interval fits the budget is deliberately not added. The budget is (retries + 2) × interval against a flip of at most (retries + 1) × interval, so it covers the flip by construction. Every generated runtime that declares a healthcheck changes, because it now carries three keys it previously left to the runtime. Frozen corpus verdicts are regenerated accordingly. Closes #109 --- docs/onebox.run-v1.schema.json | 26 +- internal/app/generate.go | 54 +++- internal/app/health_timing_test.go | 238 ++++++++++++++++++ internal/app/runtime.go | 105 ++++++-- internal/app/runtime_test.go | 10 +- internal/app/testdata/contract-verdicts.json | 24 +- internal/app/types.go | 12 +- internal/app/validate.go | 50 +++- internal/engine/deploy_test.go | 4 +- internal/engine/drain_budget_test.go | 195 ++++++++++++++ internal/engine/plan.go | 8 +- internal/engine/plan_drain_test.go | 59 +++++ internal/engine/roll.go | 72 +++++- internal/engine/roll_test.go | 4 +- site/public/onebox.run-v1.schema.json | 26 +- .../docs/reference/fields/top-level.mdx | 8 +- .../docs/reference/fields/workloads.mdx | 12 +- 17 files changed, 807 insertions(+), 100 deletions(-) create mode 100644 internal/app/health_timing_test.go create mode 100644 internal/engine/drain_budget_test.go create mode 100644 internal/engine/plan_drain_test.go diff --git a/docs/onebox.run-v1.schema.json b/docs/onebox.run-v1.schema.json index 3c4c8da5..67202552 100644 --- a/docs/onebox.run-v1.schema.json +++ b/docs/onebox.run-v1.schema.json @@ -979,7 +979,8 @@ "type": "string" }, "interval": { - "description": "Delay between container health probes. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "default": "5s", + "description": "Delay between container health probes, at most 7d. Always written into the generated healthcheck, so the rollout's drain budget is computed from the value the container actually runs with. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "2s" ], @@ -996,14 +997,16 @@ "type": "integer" }, "retries": { - "description": "Consecutive failed probes before the container is unhealthy.", + "default": 3, + "description": "Consecutive failed probes before the container is unhealthy. A draining container leaves rotation after this many probes, so it sets how long a rolling deploy waits for each replica.", "examples": [ 3 ], "type": "integer" }, "start_period": { - "description": "Startup grace period before failed probes count. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "default": "30s", + "description": "Startup grace period before failed probes count, at most 7d. Always written into the generated healthcheck, so writing down a fast probe interval does not call a booting container unhealthy. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "5s" ], @@ -1016,7 +1019,7 @@ "type": "boolean" }, "within": { - "description": "Maximum time a rollout waits for readiness. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "description": "Maximum time a rollout waits for readiness, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "120s" ], @@ -1957,7 +1960,7 @@ }, "properties": { "grace": { - "description": "Maximum graceful-shutdown time before forced termination. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "description": "Maximum graceful-shutdown time before forced termination, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "30s" ], @@ -1971,7 +1974,7 @@ "type": "string" }, "wait": { - "description": "Time allowed for the proxy to stop routing before shutdown begins. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "description": "Time allowed for the proxy to stop routing before shutdown begins, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "10s" ], @@ -2080,7 +2083,8 @@ "type": "string" }, "interval": { - "description": "Delay between container health probes. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "default": "5s", + "description": "Delay between container health probes, at most 7d. Always written into the generated healthcheck, so the rollout's drain budget is computed from the value the container actually runs with. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "2s" ], @@ -2097,14 +2101,16 @@ "type": "integer" }, "retries": { - "description": "Consecutive failed probes before the container is unhealthy.", + "default": 3, + "description": "Consecutive failed probes before the container is unhealthy. A draining container leaves rotation after this many probes, so it sets how long a rolling deploy waits for each replica.", "examples": [ 3 ], "type": "integer" }, "start_period": { - "description": "Startup grace period before failed probes count. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "default": "30s", + "description": "Startup grace period before failed probes count, at most 7d. Always written into the generated healthcheck, so writing down a fast probe interval does not call a booting container unhealthy. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "5s" ], @@ -2117,7 +2123,7 @@ "type": "boolean" }, "within": { - "description": "Maximum time a rollout waits for readiness. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "description": "Maximum time a rollout waits for readiness, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "120s" ], diff --git a/internal/app/generate.go b/internal/app/generate.go index 698bd04b..565e179e 100644 --- a/internal/app/generate.go +++ b/internal/app/generate.go @@ -5,6 +5,7 @@ import ( "encoding/hex" "fmt" "strings" + "time" "gopkg.in/yaml.v3" ) @@ -396,9 +397,7 @@ func (p *Spec) renderWorkload(n Names, name string, w Workload, releaseID string svc["logging"] = lg } } - if w.Drain != nil && w.Drain.Grace != "" { - svc["stop_grace_period"] = w.Drain.Grace - } + applyStopGrace(svc, w) if w.Resources != nil { if w.Resources.Memory != "" { svc["mem_limit"] = w.Resources.Memory @@ -603,17 +602,24 @@ func healthcheck(h *Health) map[string]any { default: return nil } - out := map[string]any{"test": test} - if h.Interval != "" { - out["interval"] = h.Interval - } - if h.StartPeriod != "" { - out["start_period"] = h.StartPeriod - } - if h.Retries > 0 { - out["retries"] = h.Retries + // Interval, retries and the start period are always written, never left to + // the runtime's default. The rollout budgets the drain against retries × interval, and a + // budget computed from a number the container was not created with is a + // budget that expires while the container is still healthy — which stops it + // while the proxy may still be routing to it. Emitting them is what makes + // the model and the runtime the same. + // + // Every duration goes through the model rather than being echoed as + // authored: `14d` is a duration Onebox accepts and Compose cannot parse, so + // echoing it validates cleanly and then fails the deploy at the compose + // step with `time: unknown unit "d"`. + workload := Workload{Health: h} + return map[string]any{ + "test": test, + "interval": composeDuration(h.Interval, workload.HealthInterval()), + "retries": workload.HealthRetries(), + "start_period": composeDuration(h.StartPeriod, workload.HealthStartPeriod()), } - return out } func composeCondition(c string) string { @@ -959,3 +965,25 @@ func composePullPolicy(declared string) string { } return "" } + +// applyStopGrace writes the authored stop grace, normalised. Compose parses +// durations with its own grammar, which has no day unit: echoing `1d` as +// authored validates here and then fails the deploy with `time: unknown unit +// "d"` — for a value Onebox told the author was fine. +func applyStopGrace(svc map[string]any, w Workload) { + if w.Drain == nil || w.Drain.Grace == "" { + return + } + svc["stop_grace_period"] = composeDuration(w.Drain.Grace, 0) +} + +// composeDuration renders a duration in units Compose can parse. The authored +// value wins whenever it parses — including an explicit zero, which is a real +// instruction to the runtime and not the absence of one — and the fallback is +// used only when nothing was written. +func composeDuration(authored string, fallback time.Duration) string { + if d, ok := ParseDuration(authored); ok { + return d.String() + } + return fallback.String() +} diff --git a/internal/app/health_timing_test.go b/internal/app/health_timing_test.go new file mode 100644 index 00000000..c08bd0e3 --- /dev/null +++ b/internal/app/health_timing_test.go @@ -0,0 +1,238 @@ +package app + +import ( + "strings" + "testing" + "time" +) + +// The interval Docker probes at and the cadence Onebox polls `docker inspect` +// at are different things. Conflating them made the drain budget model a flip +// that took 3 x 30s against a budget of 5 x 2s. +func TestHealthIntervalIsTheProbeCadenceNotThePollCadence(t *testing.T) { + shorthand := Workload{Health: &Health{HTTP: "/healthz"}} + if got := shorthand.HealthInterval(); got != 5*time.Second { + t.Fatalf("default health interval = %v, want 5s", got) + } + _, poll := shorthand.ReadyTiming() + if poll != 2*time.Second { + t.Fatalf("poll cadence = %v, want 2s — it is a local inspect, not a container probe", poll) + } + + authored := Workload{Health: &Health{HTTP: "/healthz", Interval: "30s"}} + if got := authored.HealthInterval(); got != 30*time.Second { + t.Fatalf("authored health interval = %v, want 30s", got) + } +} + +// Every value the drain budget is computed from must also be the value the +// container was created with, or the budget models a flip that cannot happen. +func TestHealthcheckAlwaysCarriesTheIntervalAndRetriesTheBudgetAssumes(t *testing.T) { + workload := Workload{Health: &Health{HTTP: "/healthz", Port: 8080}} + check := healthcheck(workload.Health) + if check == nil { + t.Fatal("no healthcheck generated") + } + if got, want := check["interval"], workload.HealthInterval().String(); got != want { + t.Fatalf("healthcheck interval = %v, want %v", got, want) + } + if got, want := check["retries"], workload.HealthRetries(); got != want { + t.Fatalf("healthcheck retries = %v, want %v", got, want) + } +} + +// The authored duration is normalised, not echoed: `interval: 90s` renders as +// 1m30s. Same instruction to the runtime, different spelling. +func TestAuthoredHealthTimingReachesTheHealthcheck(t *testing.T) { + workload := Workload{Health: &Health{HTTP: "/healthz", Port: 8080, Interval: "7s", Retries: 4}} + check := healthcheck(workload.Health) + if got := check["interval"]; got != "7s" { + t.Fatalf("healthcheck interval = %v, want the authored 7s", got) + } + if got := check["retries"]; got != 4 { + t.Fatalf("healthcheck retries = %v, want the authored 4", got) + } +} + +// Before the interval was written down, a shorthand healthcheck inherited the +// runtime's 30s probe interval, which gave a booting container roughly a +// minute and a half before it could be called unhealthy. Writing a 5s interval +// without also writing a start period would take that away and mark a +// slow-starting app unhealthy while it is still coming up — visible to +// dependency conditions, restart watchers, and alerting. +func TestHealthcheckGivesABootingContainerAStartPeriod(t *testing.T) { + check := healthcheck(&Health{HTTP: "/healthz", Port: 8080}) + if got := check["start_period"]; got != defaultHealthStartPeriod.String() { + t.Fatalf("start_period = %v, want %v", got, defaultHealthStartPeriod) + } +} + +// Onebox's duration grammar accepts days; Compose's does not know the unit at +// all. Every duration written into the healthcheck is therefore normalised to +// units Compose can parse, or the deploy dies at the compose step with +// `time: unknown unit "d"` — for a value that validated cleanly. +func TestAuthoredHealthDurationsAreNormalisedForCompose(t *testing.T) { + check := healthcheck(&Health{HTTP: "/healthz", Port: 8080, StartPeriod: "90s", Interval: "7d"}) + if got := check["start_period"]; got != "1m30s" { + t.Fatalf("start_period = %v, want the normalised 1m30s", got) + } + if got := check["interval"]; got != "168h0m0s" { + t.Fatalf("interval = %v, want the normalised 168h0m0s", got) + } + for _, field := range []string{"interval", "start_period"} { + if strings.ContainsAny(check[field].(string), "dwy") { + t.Fatalf("%s = %v carries a unit Compose cannot parse", field, check[field]) + } + } +} + +// The readiness budget has to cover at least one full flip cycle, or a rollout +// gives up before the container's own healthcheck could have reported anything. +func TestReadyBudgetCoversAtLeastOneFlipCycle(t *testing.T) { + slow := Workload{Health: &Health{HTTP: "/healthz", Interval: "3m"}} + within, _ := slow.ReadyTiming() + cycle := time.Duration(slow.HealthRetries()+1)*slow.HealthInterval() + slow.HealthStartPeriod() + if within < cycle { + t.Fatalf("within = %v, want at least one flip cycle (%v)", within, cycle) + } + + ordinary := Workload{Health: &Health{HTTP: "/healthz"}} + if within, _ := ordinary.ReadyTiming(); within != 120*time.Second { + t.Fatalf("within = %v, want the 120s default for ordinary timings", within) + } + + authored := Workload{Health: &Health{HTTP: "/healthz", Interval: "3m", Within: "45s"}} + if within, _ := authored.ReadyTiming(); within != 45*time.Second { + t.Fatalf("within = %v, want the authored 45s — an explicit budget is not second-guessed", within) + } +} + +// A retries count large enough to overflow the drain budget's arithmetic turns +// it negative, which expires instantly — the failure the budget exists to +// prevent, reached by a route validation could have closed. +func TestAbsurdRetriesIsRejected(t *testing.T) { + _, err := LoadBytes([]byte("api_version: onebox.run/v1\napp: ledger\n"+ + "environments: {production: {server: root@10.0.0.1}}\n"+ + "image: nginx\ndomain: d.example.com\nport: 8080\n"+ + "health: {http: /healthz, retries: 100000000000}\n"), "ob.yml") + if err == nil { + t.Fatal("a retries count that overflows the drain budget was accepted") + } + if !strings.Contains(err.Error(), "retries") { + t.Fatalf("error does not name retries: %v", err) + } +} + +// Bounding retries alone does not close the overflow: the readiness budget +// multiplies it by the interval, so an absurd interval wraps the arithmetic +// just as well. A wrapped budget is an effectively infinite one, and a +// crash-looping newcomer that should abort the rollout hangs instead. +func TestAbsurdHealthDurationsAreRejected(t *testing.T) { + for name, health := range map[string]string{ + "interval": "{http: /healthz, interval: 100000d}", + "start_period": "{http: /healthz, start_period: 100000d}", + "within": "{http: /healthz, within: 100000d}", + } { + t.Run(name, func(t *testing.T) { + _, err := LoadBytes([]byte("api_version: onebox.run/v1\napp: ledger\n"+ + "environments: {production: {server: root@10.0.0.1}}\n"+ + "image: nginx\ndomain: d.example.com\nport: 8080\n"+ + "health: "+health+"\n"), "ob.yml") + if err == nil { + t.Fatalf("health %s was accepted", health) + } + if !strings.Contains(err.Error(), name) { + t.Fatalf("error does not name %s: %v", name, err) + } + }) + } +} + +// The budget stays inside int64 nanoseconds at every accepted extreme. +func TestReadyBudgetStaysPositiveAtTheAcceptedExtremes(t *testing.T) { + extreme := Workload{ + Health: &Health{ + HTTP: "/healthz", Interval: maxLifecycleDuration.String(), + StartPeriod: maxLifecycleDuration.String(), Retries: maxHealthRetries, + }, + // An authored wait, so the drain assertion below is about the + // arithmetic rather than about the zero returned when none was written. + Drain: &Drain{Wait: maxLifecycleDuration.String()}, + } + within, _ := extreme.ReadyTiming() + if within <= 0 { + t.Fatalf("within = %v — the budget overflowed", within) + } + if drain := extreme.DrainWait(); drain <= 0 { + t.Fatalf("drain wait = %v — the budget overflowed", drain) + } +} + +// `start_period: 0s` means "no grace" to the runtime, and an author who writes +// it means it. Routing emission through a defaulted accessor turned it into +// the 30s default with no warning. +func TestAuthoredZeroStartPeriodIsKept(t *testing.T) { + check := healthcheck(&Health{HTTP: "/healthz", Port: 8080, StartPeriod: "0s"}) + if got := check["start_period"]; got != "0s" { + t.Fatalf("start_period = %v, want the authored 0s", got) + } +} + +// Every duration Compose will parse has to be normalised, not only the ones in +// the healthcheck. stop_grace_period is authored with the same grammar. +func TestStopGracePeriodIsNormalisedForCompose(t *testing.T) { + workload := Workload{Role: RoleApplication, Drain: &Drain{Grace: "1d"}} + svc := map[string]any{} + applyStopGrace(svc, workload) + if got := svc["stop_grace_period"]; got != "24h0m0s" { + t.Fatalf("stop_grace_period = %v, want the normalised 24h0m0s", got) + } +} + +// A day count large enough to overflow int64 nanoseconds must not parse as a +// valid duration: it wraps to a negative — or worse, to a plausible positive — +// and slips past every bound expressed as `d > limit`. +func TestParseDurationRejectsOverflowingDayCounts(t *testing.T) { + for _, raw := range []string{"1000000d", "213504d", "9223372036854775807d"} { + if d, ok := ParseDuration(raw); ok { + t.Fatalf("ParseDuration(%q) = %v, want rejection", raw, d) + } + } + if d, ok := ParseDuration("14d"); !ok || d != 14*24*time.Hour { + t.Fatalf("ParseDuration(\"14d\") = %v, %v — an ordinary day count must still parse", d, ok) + } +} + +// A day count that wraps int64 must be rejected by validation too, not merely +// by the parser: the two together are what make the bound mean something. +func TestOverflowingDayCountIsRejectedAtLoad(t *testing.T) { + _, err := LoadBytes([]byte("api_version: onebox.run/v1\napp: ledger\n"+ + "environments: {production: {server: root@10.0.0.1}}\n"+ + "image: nginx\ndomain: d.example.com\nport: 8080\n"+ + "health: {http: /healthz, interval: 1000000d}\n"), "ob.yml") + if err == nil { + t.Fatal("an interval that overflows int64 nanoseconds was accepted") + } + if !strings.Contains(err.Error(), "interval") { + t.Fatalf("error does not name interval: %v", err) + } +} + +func TestAbsurdDrainDurationsAreRejected(t *testing.T) { + for name, drain := range map[string]string{ + "wait": "{wait: 100000d}", + "grace": "{grace: 100000d}", + } { + t.Run(name, func(t *testing.T) { + _, err := LoadBytes([]byte("api_version: onebox.run/v1\napp: ledger\n"+ + "environments: {production: {server: root@10.0.0.1}}\n"+ + "workloads: {web: {image: nginx, domain: d.example.com, port: 8080, drain: "+drain+"}}\n"), "ob.yml") + if err == nil { + t.Fatalf("drain %s was accepted", drain) + } + if !strings.Contains(err.Error(), name) { + t.Fatalf("error does not name %s: %v", name, err) + } + }) + } +} diff --git a/internal/app/runtime.go b/internal/app/runtime.go index 6ab076c0..4f13a41d 100644 --- a/internal/app/runtime.go +++ b/internal/app/runtime.go @@ -63,42 +63,104 @@ func (w Workload) StopGraceSeconds() int { // HealthRetries is the consecutive-failure count before the runtime flips a // container unhealthy. It governs how fast a draining container is dropped by -// the proxy — the flip takes Retries × Interval — so the drain budget is +// the proxy — the flip takes Retries × HealthInterval — so the drain budget is // derived from it rather than guessed alongside it. func (w Workload) HealthRetries() int { if w.Health != nil && w.Health.Retries > 0 { return w.Health.Retries } - return 3 + return defaultHealthRetries } -// ReadyTiming is the health gate's overall budget and its poll cadence. -func (w Workload) ReadyTiming() (within, interval time.Duration) { - within, interval = 120*time.Second, 2*time.Second - if w.Health == nil { - return within, interval +// HealthInterval is the delay between the container's own health probes: the +// cadence the *runtime* runs the check at, not the cadence Onebox polls +// `docker inspect` at. The two are different jobs — one is a probe inside the +// container, the other a local query — and treating them as one number made +// the drain budget model a flip that could not happen in the time allowed. +// +// Both this and HealthRetries are written into the generated healthcheck, so +// the values a budget is computed from are the values the container was +// actually created with. Leaving either to the runtime's own default means +// budgeting against a number Onebox never chose and cannot see. +func (w Workload) HealthInterval() time.Duration { + if w.Health != nil { + if d, ok := ParseDuration(w.Health.Interval); ok && d > 0 { + return d + } + } + return defaultHealthInterval +} + +// HealthStartPeriod is the grace a container gets before a failed probe counts +// against it. It is written into every generated healthcheck for the same +// reason the interval is: a five-second probe with no grace would call a +// perfectly healthy application unhealthy while it is still starting, and that +// verdict is visible to dependency conditions, restart watchers and alerting +// long before the rollout would notice it. +func (w Workload) HealthStartPeriod() time.Duration { + if w.Health != nil { + if d, ok := ParseDuration(w.Health.StartPeriod); ok && d > 0 { + return d + } } - if d, ok := ParseDuration(w.Health.Within); ok && d > 0 { - within = d + return defaultHealthStartPeriod +} + +const ( + // A probe every five seconds costs twelve requests a minute per container + // and lets a drained container leave rotation in fifteen — fast enough + // that a rolling deploy is not dominated by waiting for the flip, cheap + // enough to run against every replica forever. + defaultHealthInterval = 5 * time.Second + defaultHealthRetries = 3 + // Thirty seconds is what a shorthand healthcheck effectively had before the + // interval was written down: the runtime's own 30s interval meant the first + // probe did not land until then. Keeping that as the grace means writing + // the interval down costs a booting container nothing. The runtime leaves + // the start period at the first success, so a fast application pays none of + // it. + defaultHealthStartPeriod = 30 * time.Second +) + +// ReadyTiming is the health gate's overall budget and the cadence Onebox polls +// `docker inspect` at while waiting. The poll cadence is deliberately not the +// container's probe interval: inspecting locally is cheap, so a slow probe +// interval should not also make Onebox notice the result slowly. +func (w Workload) ReadyTiming() (within, interval time.Duration) { + interval = 2 * time.Second + if w.Health != nil { + if d, ok := ParseDuration(w.Health.Within); ok && d > 0 { + return d, interval + } } - if d, ok := ParseDuration(w.Health.Interval); ok && d > 0 { - interval = d + // The default budget stretches to cover one full flip cycle when the probe + // timing is slow enough to need it. A rollout that gives up before the + // container's healthcheck could have reported anything is not measuring the + // application, and 120s is a figure chosen for ordinary probe timings, not + // a statement that a three-minute interval should fail. + within = 120 * time.Second + if cycle := time.Duration(w.HealthRetries()+1)*w.HealthInterval() + w.HealthStartPeriod(); cycle > within { + within = cycle } return within, interval } // DrainWait is how long to leave a container marked unhealthy before stopping -// it, so the proxy has time to notice and stop sending it traffic. Without an -// explicit value it is derived from the health timing, which is the only way -// the two cannot drift apart. +// it, so the proxy has time to notice and stop sending it traffic. +// +// Only an authored wait counts. The derived value this used to fall back to +// was unreachable — every caller checks `drain.wait` was written before asking +// — so it existed only to be printed by the plan, promising a pause no deploy +// ever took. Deriving one for real would add a sleep to every deploy that +// names a drain signal, which is a change to make deliberately, not by way of +// a fallback nothing calls. func (w Workload) DrainWait() time.Duration { if w.Drain != nil && w.Drain.Wait != "" { if d, ok := ParseDuration(w.Drain.Wait); ok { return d } } - _, interval := w.ReadyTiming() - return time.Duration(w.HealthRetries()) * interval + return 0 } // DrainSignal is the signal sent to begin a graceful stop. @@ -288,6 +350,13 @@ func ParseDuration(s string) (time.Duration, bool) { return 0, false } if days, err := strconv.Atoi(strings.TrimSuffix(s, "d")); err == nil && strings.HasSuffix(s, "d") { + // A day count that cannot be held in nanoseconds is not a long + // duration, it is a wrapped one: it comes back negative, or — worse — + // as a plausible positive that slips past every bound written as + // `d > limit`. Refusing it is the only reading that cannot mislead. + if days > maxDurationDays || days < -maxDurationDays { + return 0, false + } return time.Duration(days) * 24 * time.Hour, true } d, err := time.ParseDuration(s) @@ -389,3 +458,7 @@ func ParsePostgresDuration(value string) (time.Duration, bool) { } return 0, false } + +// maxDurationDays is the largest whole-day count that fits in int64 +// nanoseconds: math.MaxInt64 / (24h in ns). +const maxDurationDays = 106751 diff --git a/internal/app/runtime_test.go b/internal/app/runtime_test.go index 48ebd801..e7b856b9 100644 --- a/internal/app/runtime_test.go +++ b/internal/app/runtime_test.go @@ -47,10 +47,14 @@ func TestReleaseOrderHonoursExplicitAndKeepsOmissions(t *testing.T) { } } -func TestDrainWaitDerivesFromHealthTiming(t *testing.T) { +// Only an authored wait is a wait. The derived value this used to return was +// unreachable — roll, recreate and the plan all check `drain.wait` was written +// before asking for it — so it could only ever be printed by a plan describing +// a pause the deploy would not take. +func TestDrainWaitCountsOnlyAnAuthoredWait(t *testing.T) { w := Workload{Role: RoleApplication, Health: &Health{Interval: "1s", Retries: 4}} - if got := w.DrainWait(); got != 4*time.Second { - t.Errorf("drain wait = %v, want 4s (retries × interval)", got) + if got := w.DrainWait(); got != 0 { + t.Errorf("drain wait = %v, want 0 without an authored drain.wait", got) } w.Drain = &Drain{Wait: "9s"} if got := w.DrainWait(); got != 9*time.Second { diff --git a/internal/app/testdata/contract-verdicts.json b/internal/app/testdata/contract-verdicts.json index b6692893..101cd526 100644 --- a/internal/app/testdata/contract-verdicts.json +++ b/internal/app/testdata/contract-verdicts.json @@ -537,12 +537,12 @@ { "case": "corpus/authentik.yml", "loads": true, - "digest": "3cd927a4bf78330bba4642d740c9ece86964a30218d6d50689c2e23370aae6b2 postgres=dc4f8448b8b82b4a redis=d2660eeb4faa49fe" + "digest": "20ab797fa4ec9e6a14282b5358a902c104db3de3a16f803df540fa7cab9466cc postgres=dc4f8448b8b82b4a redis=d2660eeb4faa49fe" }, { "case": "corpus/ext-authentik-managed.yml", "loads": true, - "digest": "f402d2e878a34a9e496401c7533442934d3e37c720eb137459bc87aa4e090ad6 postgres=dc4f8448b8b82b4a redis=d2660eeb4faa49fe" + "digest": "f3bb559d2c91eb464f809ab583f4985d4df4d85407d0f58a34d27b7d41adc0ef postgres=dc4f8448b8b82b4a redis=d2660eeb4faa49fe" }, { "case": "corpus/ext-authentik.yml", @@ -557,7 +557,7 @@ { "case": "corpus/ext-gitea.yml", "loads": true, - "digest": "aa26f69004bf9a169e005a24be991a67f22287ec7d99428aa15d39d41a6e6a6d postgres=e70cc45c347098f9" + "digest": "fce80729763d1fc0ba227569c135409bdeb777b6381aaf1f38ab36e57a581a9c postgres=e70cc45c347098f9" }, { "case": "corpus/ext-immich-sourced.yml", @@ -572,7 +572,7 @@ { "case": "corpus/ext-n8n.yml", "loads": true, - "digest": "2c553da86bf863420b725d26c37aa25fac407adf77b3d4502de883c75cf14fcb postgres=809549d286e2dbdc redis=86933b446609e6d8" + "digest": "dd7fc16843e836e74ad4f09bdbfb4360183e75b8fcf725647edbc361f7d4579f postgres=809549d286e2dbdc redis=86933b446609e6d8" }, { "case": "corpus/ext-paperless.yml", @@ -582,12 +582,12 @@ { "case": "corpus/ext-plausible.yml", "loads": true, - "digest": "cab9f37156b86ba788b71f110ccdd8ae7276607336d3fafcb06d9b5255e7a380 events=1e798590e3a5dda4 postgres=b1fac70440c33545" + "digest": "da52a65aaae0f8283f24980f50050e052205a327c8c170c1625447b4798036c7 events=1e798590e3a5dda4 postgres=b1fac70440c33545" }, { "case": "corpus/ext-umami.yml", "loads": true, - "digest": "79b4c4a3d6a6ef07bca7bc0b14d9bc16c44bd2f111754909961067523497f6c9 postgres=36c6c38ba304b445" + "digest": "ef5db4cb343d6dd50e16d720410978d5c65ddc6ad4330578ea1a4177deefb9f7 postgres=36c6c38ba304b445" }, { "case": "corpus/ghost.yml", @@ -597,7 +597,7 @@ { "case": "corpus/gitea.yml", "loads": true, - "digest": "0c7b84f5b5d850e4489f27c40ae17176db81f94016a76ff65e683513e1109d30" + "digest": "43701aca44d733368efe26965e76ed9fec5a194494d3fb8c2a2369ef04e4c056" }, { "case": "corpus/goal.yml", @@ -607,7 +607,7 @@ { "case": "corpus/immich.yml", "loads": true, - "digest": "f7e7a345ebcfa542e54f85ce35f4e9773a3c6a0532b88f5e5a807dbccb93e4c7" + "digest": "a5d556aead79066fbba27d5aa50ef2366d4c6dd49c87e7faf35e3972319df514" }, { "case": "corpus/monk.yml", @@ -617,12 +617,12 @@ { "case": "corpus/n8n.yml", "loads": true, - "digest": "044dcabe64d885d4eafa025cb187deff9a03886d582429c979de4839599bf186" + "digest": "95ac57f61a47bc455b9afd2497dbd20826374b72b772d6ecf988f29871df9314" }, { "case": "corpus/paperless.yml", "loads": true, - "digest": "069aa81ec83b61bcdced44bcd677e38c8f182889cce91d34a161781fb9c3dc77" + "digest": "a197f0d4fbbe20f86de9887363bb88c88d815c4a23edd2860315c6b3bbf49abc" }, { "case": "corpus/penpot.yml", @@ -642,12 +642,12 @@ { "case": "corpus/rocketchat.yml", "loads": true, - "digest": "06ef5ded6f0afc5abef7e7ff29f778100b976e6393450418a75350683dad44cc mongodb=eaca06e5d1b88e4b" + "digest": "fb7d986bcf290c22a8965cc7ff69c64742ee596964280d4a4c764c27daf5ab41 mongodb=eaca06e5d1b88e4b" }, { "case": "corpus/umami.yml", "loads": true, - "digest": "f2e6a77d9bb6123eafc531a3cb6969db42cb5ab8d89e3549d06d742b272f4c42" + "digest": "6382188f4685b42b53d190ba22f8bb2a6bf1385658051f02da7993940c941dca" }, { "case": "corpus/uptime-kuma.yml", diff --git a/internal/app/types.go b/internal/app/types.go index f3026bea..dc3d671a 100644 --- a/internal/app/types.go +++ b/internal/app/types.go @@ -209,16 +209,16 @@ type Health struct { Exec any `json:"exec,omitempty" description:"Health command as a shell string or direct argument list."` TCP bool `json:"tcp,omitempty" description:"Probe the configured port by opening a TCP connection." default:"false"` Port int `json:"port,omitempty" description:"Container port probed by HTTP or TCP health checks." example:"8080"` - Interval string `json:"interval,omitempty" description:"Delay between container health probes." example:"2s"` - StartPeriod string `json:"start_period,omitempty" description:"Startup grace period before failed probes count." example:"5s"` - Within string `json:"within,omitempty" description:"Maximum time a rollout waits for readiness." example:"120s"` - Retries int `json:"retries,omitempty" description:"Consecutive failed probes before the container is unhealthy." example:"3"` + Interval string `json:"interval,omitempty" description:"Delay between container health probes, at most 7d. Always written into the generated healthcheck, so the rollout's drain budget is computed from the value the container actually runs with." default:"5s" example:"2s"` + StartPeriod string `json:"start_period,omitempty" description:"Startup grace period before failed probes count, at most 7d. Always written into the generated healthcheck, so writing down a fast probe interval does not call a booting container unhealthy." default:"30s" example:"5s"` + Within string `json:"within,omitempty" description:"Maximum time a rollout waits for readiness, at most 7d." example:"120s"` + Retries int `json:"retries,omitempty" description:"Consecutive failed probes before the container is unhealthy. A draining container leaves rotation after this many probes, so it sets how long a rolling deploy waits for each replica." default:"3" example:"3"` } type Drain struct { Signal string `json:"signal" description:"Signal sent to begin graceful shutdown." default:"TERM"` - Wait string `json:"wait,omitempty" description:"Time allowed for the proxy to stop routing before shutdown begins." example:"10s"` - Grace string `json:"grace,omitempty" description:"Maximum graceful-shutdown time before forced termination." example:"30s"` + Wait string `json:"wait,omitempty" description:"Time allowed for the proxy to stop routing before shutdown begins, at most 7d." example:"10s"` + Grace string `json:"grace,omitempty" description:"Maximum graceful-shutdown time before forced termination, at most 7d." example:"30s"` } type Logging struct { diff --git a/internal/app/validate.go b/internal/app/validate.go index f2299ef9..63122265 100644 --- a/internal/app/validate.go +++ b/internal/app/validate.go @@ -2,6 +2,7 @@ package app import ( "strings" + "time" obtarget "github.com/labstack/onebox/internal/target" ) @@ -291,7 +292,7 @@ func validateWorkload(w Workload, path string) error { return err } for field, value := range map[string]string{"wait": w.Drain.Wait, "grace": w.Drain.Grace} { - if err := gDur.checkOptional(path+".drain."+field, value); err != nil { + if err := checkLifecycleDuration(path+".drain."+field, field, value); err != nil { return err } } @@ -444,13 +445,22 @@ func validateHealth(h *Health, path string) error { for field, value := range map[string]string{ "interval": h.Interval, "start_period": h.StartPeriod, "within": h.Within, } { - if err := gDur.checkOptional(path+"."+field, value); err != nil { + if err := checkLifecycleDuration(path+"."+field, field, value); err != nil { return err } } + // Bounded above as well as below. Retries multiplies the probe interval to + // give the rollout's drain budget, and a count large enough to overflow + // that arithmetic yields a negative budget — one that expires immediately, + // stopping a container the proxy may still be routing to. No real + // healthcheck needs a four-figure count, so the bound costs nothing. if h.Retries < 0 { return errf("project_invalid", path+".retries", "", "must not be negative") } + if h.Retries > maxHealthRetries { + return errf("project_invalid", path+".retries", "", + "must not exceed %d — retries multiplies the probe interval to give the rollout's drain budget", maxHealthRetries) + } return nil } @@ -635,3 +645,39 @@ func validateAddress(kind, path, host, user string, port int) error { } return nil } + +// checkLifecycleDuration holds a deploy-lifecycle duration to the grammar and +// to a ceiling. The ceiling matters because these values are multiplied and +// summed to give budgets a rollout waits on: a duration in the thousands of +// days wraps that arithmetic, and every one of them is a duration the deploy +// would sit and sleep through if it did not. +func checkLifecycleDuration(path, field, value string) error { + if err := gDur.checkOptional(path, value); err != nil { + return err + } + if value == "" { + return nil + } + // The grammar matches the shape; parsing decides whether the value is + // representable. A day count beyond int64 nanoseconds satisfies the first + // and fails the second, and treating that as "no bound to apply" would let + // the very value the ceiling exists for pass unchecked. + d, ok := ParseDuration(value) + if !ok { + return errf("project_invalid", path, "", "%s %q is too large to represent", field, value) + } + if d > maxLifecycleDuration { + return errf("project_invalid", path, "", "%s must not exceed %s", field, maxLifecycleDuration) + } + return nil +} + +const ( + // maxHealthRetries and maxLifecycleDuration keep retries × interval far + // inside int64 nanoseconds while staying orders of magnitude above any real + // healthcheck: a week between probes is already far past the point where a + // health check is measuring anything, and a week of drain wait is a deploy + // nobody is waiting for. + maxHealthRetries = 1000 + maxLifecycleDuration = 7 * 24 * time.Hour +) diff --git a/internal/engine/deploy_test.go b/internal/engine/deploy_test.go index 98431222..619aa8fb 100644 --- a/internal/engine/deploy_test.go +++ b/internal/engine/deploy_test.go @@ -39,8 +39,8 @@ func seedStagedApplicationManifest(f *transport.Fake, releaseID string) { func happyFake() *transport.Fake { f := &transport.Fake{} f.Dynamic = func(cmd string) (transport.Result, bool) { - if strings.Contains(cmd, "Config.Healthcheck.Test") { - return transport.Result{Stdout: guardedHealthcheck + "\n"}, true + if strings.Contains(cmd, "Config.Healthcheck") { + return transport.Result{Stdout: `{"Test":` + guardedHealthcheck + `,"Interval":5000000000,"Retries":3}` + "\n"}, true } // server roll state, derived from history so the loop converges: NEW1 // appears after a scale, OLD1 disappears once removed, names track renames. diff --git a/internal/engine/drain_budget_test.go b/internal/engine/drain_budget_test.go new file mode 100644 index 00000000..4473a85f --- /dev/null +++ b/internal/engine/drain_budget_test.go @@ -0,0 +1,195 @@ +package engine + +import ( + "bytes" + "context" + "fmt" + "strings" + "sync" + "testing" + "time" + + "github.com/labstack/onebox/internal/app" + "github.com/labstack/onebox/internal/transport" +) + +// virtualClock advances only when the engine sleeps, so a test can model a +// container that takes real seconds to flip without spending them. +type virtualClock struct { + mu sync.Mutex + elapsed time.Duration +} + +func (c *virtualClock) now() time.Time { + c.mu.Lock() + defer c.mu.Unlock() + return time.Date(2026, 8, 23, 0, 0, 0, 0, time.UTC).Add(c.elapsed) +} + +func (c *virtualClock) sleep(d time.Duration) { + c.mu.Lock() + defer c.mu.Unlock() + c.elapsed += d +} + +func (c *virtualClock) since(mark time.Duration) time.Duration { + c.mu.Lock() + defer c.mu.Unlock() + return c.elapsed - mark +} + +// flippingFake models what Docker actually does: a drained container keeps +// reporting healthy until `retries` consecutive probes have failed, one probe +// every `interval`. The old fake flipped the instant it was drained, which is +// why a budget too short for the real flip never failed a test. +func flippingFake(clock *virtualClock, flipAfter time.Duration, baked string) *transport.Fake { + f := &transport.Fake{} + var drainedAt sync.Map + lastField := func(s string) string { + fields := strings.Fields(s) + if len(fields) == 0 { + return "" + } + return fields[len(fields)-1] + } + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "Config.Healthcheck") { + return transport.Result{Stdout: baked + "\n"}, true + } + removed := map[string]bool{} + scale := 0 + for _, c := range f.Commands { + if strings.Contains(c, "--scale web=") { + scale++ + } + if i := strings.Index(c, "docker rm "); i >= 0 { + removed[strings.Fields(strings.TrimPrefix(c[i+len("docker rm "):], "-f "))[0]] = true + } + if i := strings.Index(c, "docker exec "); i >= 0 && strings.Contains(c, "touch") { + id := strings.Fields(c[i+len("docker exec "):])[0] + clock.mu.Lock() + elapsed := clock.elapsed + clock.mu.Unlock() + drainedAt.LoadOrStore(id, elapsed) + } + } + var news []string + for k := 1; k <= scale; k++ { + if id := fmt.Sprintf("NEW%d", k); !removed[id] { + news = append(news, id) + } + } + var olds []string + if !removed["OLD1"] { + olds = append(olds, "OLD1") + } + switch { + case strings.Contains(cmd, "docker ps -q") && strings.Contains(cmd, "ob.release="): + return transport.Result{Stdout: strings.Join(news, "\n") + "\n"}, true + case strings.Contains(cmd, "docker ps -q") && strings.Contains(cmd, "service='web'"): + return transport.Result{Stdout: strings.Join(append(append([]string{}, olds...), news...), "\n") + "\n"}, true + case strings.Contains(cmd, "{{.Name}}"): + return transport.Result{Stdout: "/web\n"}, true + case strings.Contains(cmd, "State.Health"): + id := lastField(cmd) + if strings.HasPrefix(id, "NEW") { + return transport.Result{Stdout: "healthy\n"}, true + } + mark, ok := drainedAt.Load(id) + if !ok { + return transport.Result{Stdout: "healthy\n"}, true + } + if clock.since(mark.(time.Duration)) < flipAfter { + return transport.Result{Stdout: "healthy\n"}, true + } + return transport.Result{Stdout: "unhealthy\n"}, true + } + return transport.Result{}, false + } + return f +} + +// The budget exists so a container is never stopped while the proxy may still +// be routing to it. With the shorthand `health: /path`, the container flips +// after retries × the generated interval; a budget derived from anything else +// expires first and stops it anyway — the exact outcome the budget prevents. +func TestDrainBudgetCoversTheFlipTheGeneratedHealthcheckProduces(t *testing.T) { + clock := &virtualClock{} + config := testConfig() + workload := config.Workloads["web"] + workload.Health = &app.Health{HTTP: "/healthz", Port: 8080} + workload.Drain = nil + config.Workloads["web"] = workload + + // Worst case, not best: a real probe cycle is not aligned to the moment the + // drain file appears, so the flip can take up to one extra interval. A + // budget of exactly retries x interval would pass a best-case test and + // still strand containers in the field. + flip := time.Duration(workload.HealthRetries()+1)*workload.HealthInterval() - time.Millisecond + fake := flippingFake(clock, flip, bakedHealthcheckJSON("5s", 3)) + out := &bytes.Buffer{} + e := New(config, testProject(t), fake, Options{Out: out, Sleep: clock.sleep, Now: clock.now}) + if err := e.RollRole(context.Background(), "web", "/var/lib/ob/sample/releases/R1/compose.yaml"); err != nil { + t.Fatalf("roll: %v", err) + } + assertDrained(t, out.String()) + if strings.Contains(out.String(), "never reported unhealthy") { + t.Fatalf("the drain budget expired before the container could flip:\n%s", out.String()) + } +} + +// assertDrained fails a test that would otherwise pass vacuously: a rollout +// that decides the health check cannot be drain-guarded skips the wait +// entirely, so it never warns no matter how wrong the budget is. +func assertDrained(t *testing.T, output string) { + t.Helper() + if strings.Contains(output, "cannot be drain-guarded") { + t.Fatalf("the rollout skipped the drain wait, so the budget was never exercised:\n%s", output) + } +} + +// bakedHealthcheckJSON is what `docker inspect .Config.Healthcheck` returns for +// a container: durations in nanoseconds, and an omitted key meaning "the +// runtime's own default" rather than zero. +func bakedHealthcheckJSON(interval string, retries int) string { + fields := `"Test":` + guardedHealthcheck + if interval != "" { + d, err := time.ParseDuration(interval) + if err != nil { + panic(err) + } + fields += fmt.Sprintf(`,"Interval":%d`, d.Nanoseconds()) + } + if retries > 0 { + fields += fmt.Sprintf(`,"Retries":%d`, retries) + } + return "{" + fields + "}" +} + +// The containers a deploy drains were created by the PREVIOUS deploy, so they +// carry the previous healthcheck. Budgeting from the spec being deployed means +// the first rollout after any change to the probe timing — including an upgrade +// that changes Onebox's own default — budgets for a flip the running container +// cannot perform. That is the reported failure, one release later. +func TestDrainBudgetCoversAContainerBakedBeforeTheChange(t *testing.T) { + clock := &virtualClock{} + config := testConfig() + workload := config.Workloads["web"] + workload.Health = &app.Health{HTTP: "/healthz", Port: 8080} + workload.Drain = nil + config.Workloads["web"] = workload + + // No Interval and no Retries: exactly what Onebox emitted before this fix, + // which Docker runs at its own 30s default. + const dockerDefaultInterval = 30 * time.Second + fake := flippingFake(clock, 4*dockerDefaultInterval-time.Millisecond, bakedHealthcheckJSON("", 0)) + out := &bytes.Buffer{} + e := New(config, testProject(t), fake, Options{Out: out, Sleep: clock.sleep, Now: clock.now}) + if err := e.RollRole(context.Background(), "web", "/var/lib/ob/sample/releases/R1/compose.yaml"); err != nil { + t.Fatalf("roll: %v", err) + } + assertDrained(t, out.String()) + if strings.Contains(out.String(), "never reported unhealthy") { + t.Fatalf("the budget ignored the healthcheck the draining container actually carries:\n%s", out.String()) + } +} diff --git a/internal/engine/plan.go b/internal/engine/plan.go index ce33b455..34576ea5 100644 --- a/internal/engine/plan.go +++ b/internal/engine/plan.go @@ -514,7 +514,13 @@ func (e *Engine) Describe(remoteCompose string) []string { if n := role.Count(); n > 1 { scale = fmt.Sprintf(" --scale %s=%d", svc, n) } - if wait := role.DrainWait(); role.Drain != nil && role.DrainSignal() != "TERM" && wait > 0 { + // Gated exactly as recreate gates it — an authored `drain.wait`, + // nothing else. recreate signals every container and then sleeps, + // whatever the signal is, so excluding the default TERM here hid a + // kill and a pause the deploy certainly takes. A plan that shows a + // step execution skips, or hides one it takes, is a plan nobody can + // check against. + if wait := role.DrainWait(); role.Drain != nil && role.Drain.Wait != "" && wait > 0 { out = append(out, fmt.Sprintf(" docker kill --signal=%s ; wait %s", role.DrainSignal(), svc, wait), ) diff --git a/internal/engine/plan_drain_test.go b/internal/engine/plan_drain_test.go new file mode 100644 index 00000000..f738347a --- /dev/null +++ b/internal/engine/plan_drain_test.go @@ -0,0 +1,59 @@ +package engine + +import ( + "io" + "strings" + "testing" + + "github.com/labstack/onebox/internal/app" + "github.com/labstack/onebox/internal/transport" +) + +// A plan is read as a promise about what the deploy will do. Recreate only +// signals and waits when `drain.wait` was authored, so a plan that prints the +// step for a workload that never set one describes a deploy nobody gets. +func TestPlanPromisesADrainWaitOnlyWhenTheDeployTakesOne(t *testing.T) { + config := testConfig() + workload := config.Workloads["web"] + workload.Strategy = "recreate" + workload.Health = nil + workload.Drain = &app.Drain{Signal: "USR1"} + config.Workloads["web"] = workload + + // Scoped to this workload: the fixture's worker authors a drain wait of its + // own, and that line is correct. + lines := strings.Join(newPlanEngine(t, config).Describe("/var/lib/ob/sample/releases/R1/compose.yaml"), "\n") + if strings.Contains(lines, "") { + t.Fatalf("the plan promises a drain step the deploy does not take:\n%s", lines) + } + + withWait := workload + withWait.Drain = &app.Drain{Signal: "USR1", Wait: "12s"} + config.Workloads["web"] = withWait + lines = strings.Join(newPlanEngine(t, config).Describe("/var/lib/ob/sample/releases/R1/compose.yaml"), "\n") + if !strings.Contains(lines, "--signal=USR1 ; wait 12s") { + t.Fatalf("the plan omits the drain step the deploy does take:\n%s", lines) + } +} + +func newPlanEngine(t *testing.T, config *app.Resolved) *Engine { + t.Helper() + return New(config, testProject(t), &transport.Fake{}, Options{Out: io.Discard, Sleep: noSleep}) +} + +// recreate sends the drain signal for every authored wait, TERM included, and +// then sleeps. A plan that shows the step only for a non-default signal hides +// a kill and a pause the deploy will certainly take. +func TestPlanShowsTheDrainStepForTheDefaultSignalToo(t *testing.T) { + config := testConfig() + workload := config.Workloads["web"] + workload.Strategy = "recreate" + workload.Health = nil + workload.Drain = &app.Drain{Wait: "15s"} // no signal: TERM + config.Workloads["web"] = workload + + lines := strings.Join(newPlanEngine(t, config).Describe("/var/lib/ob/sample/releases/R1/compose.yaml"), "\n") + if !strings.Contains(lines, "--signal=TERM ; wait 15s") { + t.Fatalf("the plan hides the drain step recreate will take:\n%s", lines) + } +} diff --git a/internal/engine/roll.go b/internal/engine/roll.go index 74ef2e53..bc34bcd9 100644 --- a/internal/engine/roll.go +++ b/internal/engine/roll.go @@ -2,6 +2,7 @@ package engine import ( "context" + "encoding/json" "errors" "fmt" "path" @@ -165,7 +166,7 @@ func (e *Engine) retireContainer(ctx context.Context, role app.Workload, id stri // check in an image with no shell — can never flip, so waiting for it would // burn the whole budget and then stop a container the proxy is still using. // Saying so is better than a warning that reads like a fault. - guarded, err := e.drainGuarded(ctx, id) + guarded, probeEvery, probeRetries, err := e.bakedHealthcheck(ctx, id) if err != nil { return err } @@ -178,12 +179,19 @@ func (e *Engine) retireContainer(ctx context.Context, role app.Workload, id stri if err := e.mutateChecked(ctx, "mark container "+id+" draining", "docker exec "+id+" touch "+app.DrainFile); err != nil { return err } - // Budget the drain wait off the ACTUAL flip cost (retries × interval) plus - // two poll-intervals of slack, so a raised ready.retries can't make the flip - // exceed the budget — that would time out and SIGTERM a container the proxy - // may still be routing to. At the default 3 retries this is 5*pollEvery, the - // prior value. - drainBudget := time.Duration(role.HealthRetries()+2) * pollEvery + // Budget the drain wait off the ACTUAL flip cost — this container's own + // retries × its own probe interval — plus two probes of slack, so raising + // retries cannot make the flip exceed the budget. Timing out here SIGTERMs + // a container the proxy may still be routing to, which is what the budget + // exists to prevent. + // + // Both numbers come from the container, not from the spec: the spec + // describes what is being started, while this is what is being drained. Nor + // is either the poll cadence, which is only how often Onebox runs `docker + // inspect` — a local query. Budgeting a container-side flip against a local + // query's cadence is what made the budget expire before the flip could + // happen at all. + drainBudget := time.Duration(probeRetries+2) * probeEvery if err := e.waitHealth(ctx, id, "unhealthy", drainBudget, pollEvery); err != nil { e.warnf("container never reported unhealthy (%v); proceeding after buffer", err) } @@ -212,18 +220,56 @@ func (e *Engine) stopAndRemove(ctx context.Context, role app.Workload, id string return e.mutateChecked(ctx, "remove container "+id, "docker rm "+id) } -// drainGuarded reports whether a container's health check reads the drain file. -func (e *Engine) drainGuarded(ctx context.Context, id string) (bool, error) { - res, err := e.T.Run(ctx, "docker inspect -f '{{json .Config.Healthcheck.Test}}' "+id) +// bakedHealthcheck is the healthcheck a running container was CREATED with, +// which is the only one that governs how it behaves now. A healthcheck is baked +// in at creation, so the spec being deployed describes the containers being +// started, never the ones being drained: reading it from the container is what +// makes the drain budget survive a change to the probe timing — including a +// change to Onebox's own default, which no operator asked for and would +// otherwise strand every replica of the first deploy after an upgrade. +// +// Omitted fields mean the runtime's defaults, not zero. +func (e *Engine) bakedHealthcheck(ctx context.Context, id string) (guarded bool, interval time.Duration, retries int, err error) { + res, err := e.T.Run(ctx, "docker inspect -f '{{json .Config.Healthcheck}}' "+id) if err != nil { - return false, err + return false, 0, 0, err } if res.ExitCode != 0 { - return false, nil + return false, dockerDefaultHealthInterval, dockerDefaultHealthRetries, nil + } + guarded = strings.Contains(res.Stdout, app.DrainFile) + var baked struct { + Interval int64 `json:"Interval"` + Retries int `json:"Retries"` + } + interval, retries = dockerDefaultHealthInterval, dockerDefaultHealthRetries + if err := json.Unmarshal([]byte(strings.TrimSpace(res.Stdout)), &baked); err == nil { + // Clamped, because these come from a container rather than from a + // validated project: one created by an older runner can carry values + // that were never bounded, and a budget wrapped negative by them + // expires instantly — stopping a container the proxy may still be + // routing to, which is the failure this budget exists to prevent. + if baked.Interval > 0 && time.Duration(baked.Interval) <= maxBakedHealthInterval { + interval = time.Duration(baked.Interval) + } + if baked.Retries > 0 && baked.Retries <= maxBakedHealthRetries { + retries = baked.Retries + } } - return strings.Contains(res.Stdout, app.DrainFile), nil + return guarded, interval, retries, nil } +// The runtime's own defaults, applied when a healthcheck omits the field. They +// are the values a container created by an older Onebox is running with. +const ( + dockerDefaultHealthInterval = 30 * time.Second + dockerDefaultHealthRetries = 3 + // Ceilings for what a container reports, matching what the project file is + // allowed to declare. + maxBakedHealthInterval = 7 * 24 * time.Hour + maxBakedHealthRetries = 1000 +) + // reslot gives each new-release container a clean, stable slot name: the plain // --1..--N for every replica count. // A slot still held by an old container counts as taken, so names never clash; diff --git a/internal/engine/roll_test.go b/internal/engine/roll_test.go index b96dfc34..203937cb 100644 --- a/internal/engine/roll_test.go +++ b/internal/engine/roll_test.go @@ -29,8 +29,8 @@ func replicaFake(desired int, oldIDs []string, oldNames map[string]string, resum f.Dynamic = func(cmd string) (transport.Result, bool) { // Every generated shell-form check carries the drain guard; a rollout // probes for it before poisoning health. - if strings.Contains(cmd, "Config.Healthcheck.Test") { - return transport.Result{Stdout: guardedHealthcheck + "\n"}, true + if strings.Contains(cmd, "Config.Healthcheck") { + return transport.Result{Stdout: `{"Test":` + guardedHealthcheck + `,"Interval":5000000000,"Retries":3}` + "\n"}, true } scale := 0 removed := map[string]bool{} diff --git a/site/public/onebox.run-v1.schema.json b/site/public/onebox.run-v1.schema.json index 3c4c8da5..67202552 100644 --- a/site/public/onebox.run-v1.schema.json +++ b/site/public/onebox.run-v1.schema.json @@ -979,7 +979,8 @@ "type": "string" }, "interval": { - "description": "Delay between container health probes. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "default": "5s", + "description": "Delay between container health probes, at most 7d. Always written into the generated healthcheck, so the rollout's drain budget is computed from the value the container actually runs with. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "2s" ], @@ -996,14 +997,16 @@ "type": "integer" }, "retries": { - "description": "Consecutive failed probes before the container is unhealthy.", + "default": 3, + "description": "Consecutive failed probes before the container is unhealthy. A draining container leaves rotation after this many probes, so it sets how long a rolling deploy waits for each replica.", "examples": [ 3 ], "type": "integer" }, "start_period": { - "description": "Startup grace period before failed probes count. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "default": "30s", + "description": "Startup grace period before failed probes count, at most 7d. Always written into the generated healthcheck, so writing down a fast probe interval does not call a booting container unhealthy. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "5s" ], @@ -1016,7 +1019,7 @@ "type": "boolean" }, "within": { - "description": "Maximum time a rollout waits for readiness. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "description": "Maximum time a rollout waits for readiness, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "120s" ], @@ -1957,7 +1960,7 @@ }, "properties": { "grace": { - "description": "Maximum graceful-shutdown time before forced termination. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "description": "Maximum graceful-shutdown time before forced termination, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "30s" ], @@ -1971,7 +1974,7 @@ "type": "string" }, "wait": { - "description": "Time allowed for the proxy to stop routing before shutdown begins. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "description": "Time allowed for the proxy to stop routing before shutdown begins, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "10s" ], @@ -2080,7 +2083,8 @@ "type": "string" }, "interval": { - "description": "Delay between container health probes. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "default": "5s", + "description": "Delay between container health probes, at most 7d. Always written into the generated healthcheck, so the rollout's drain budget is computed from the value the container actually runs with. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "2s" ], @@ -2097,14 +2101,16 @@ "type": "integer" }, "retries": { - "description": "Consecutive failed probes before the container is unhealthy.", + "default": 3, + "description": "Consecutive failed probes before the container is unhealthy. A draining container leaves rotation after this many probes, so it sets how long a rolling deploy waits for each replica.", "examples": [ 3 ], "type": "integer" }, "start_period": { - "description": "Startup grace period before failed probes count. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "default": "30s", + "description": "Startup grace period before failed probes count, at most 7d. Always written into the generated healthcheck, so writing down a fast probe interval does not call a booting container unhealthy. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "5s" ], @@ -2117,7 +2123,7 @@ "type": "boolean" }, "within": { - "description": "Maximum time a rollout waits for readiness. Expects a duration such as 30s, 5m, 1h30m or 14d.", + "description": "Maximum time a rollout waits for readiness, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d.", "examples": [ "120s" ], diff --git a/site/src/content/docs/reference/fields/top-level.mdx b/site/src/content/docs/reference/fields/top-level.mdx index 67f666b6..9b16d730 100644 --- a/site/src/content/docs/reference/fields/top-level.mdx +++ b/site/src/content/docs/reference/fields/top-level.mdx @@ -37,12 +37,12 @@ cannot drift from what `ob validate` accepts. | `health` | object | — | Readiness check used to gate rolling replacement. Also accepts an HTTP health path. | | `health.exec` | — | — | Health command as a shell string or direct argument list. | | `health.http` | string | — | HTTP path probed inside the container. Expects a path beginning with /. | -| `health.interval` | string | — | Delay between container health probes. Expects a duration such as 30s, 5m, 1h30m or 14d. | +| `health.interval` | string | `5s` | Delay between container health probes, at most 7d. Always written into the generated healthcheck, so the rollout's drain budget is computed from the value the container actually runs with. Expects a duration such as 30s, 5m, 1h30m or 14d. | | `health.port` | integer | — | Container port probed by HTTP or TCP health checks. | -| `health.retries` | integer | — | Consecutive failed probes before the container is unhealthy. | -| `health.start_period` | string | — | Startup grace period before failed probes count. Expects a duration such as 30s, 5m, 1h30m or 14d. | +| `health.retries` | integer | `3` | Consecutive failed probes before the container is unhealthy. A draining container leaves rotation after this many probes, so it sets how long a rolling deploy waits for each replica. | +| `health.start_period` | string | `30s` | Startup grace period before failed probes count, at most 7d. Always written into the generated healthcheck, so writing down a fast probe interval does not call a booting container unhealthy. Expects a duration such as 30s, 5m, 1h30m or 14d. | | `health.tcp` | boolean | `false` | Probe the configured port by opening a TCP connection. | -| `health.within` | string | — | Maximum time a rollout waits for readiness. Expects a duration such as 30s, 5m, 1h30m or 14d. | +| `health.within` | string | — | Maximum time a rollout waits for readiness, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d. | | `image` | object | — | Container image source, written as a reference string or an object. Also accepts an image reference. | | `image.pull` | `always` · `missing` · `never` | `missing` | When to fetch the image from the registry: missing fetches only what the host does not already hold, always fetches every release, never fetches at all and fails on a missing image. | | `image.reference` | string | — | Complete container image reference, optionally tagged or digest-pinned. Expects a registry reference such as nginx:1.27 or ghcr.io/acme/app@sha256:…. | diff --git a/site/src/content/docs/reference/fields/workloads.mdx b/site/src/content/docs/reference/fields/workloads.mdx index f7039573..ebce28eb 100644 --- a/site/src/content/docs/reference/fields/workloads.mdx +++ b/site/src/content/docs/reference/fields/workloads.mdx @@ -35,9 +35,9 @@ cannot drift from what `ob validate` accepts. | `.data_effect` | `none` · `migration` · `destructive` · `unknown` | — | Job data impact used by rollback and abort gates. | | `.domain` | string | — | Domain shorthand for one HTTPS route; requires port and cannot be combined with routes. | | `.drain` | object | — | Signal and timing used to remove a container from traffic before stopping it. | -| `.drain.grace` | string | — | Maximum graceful-shutdown time before forced termination. Expects a duration such as 30s, 5m, 1h30m or 14d. | +| `.drain.grace` | string | — | Maximum graceful-shutdown time before forced termination, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d. | | `.drain.signal` | string | `TERM` | Signal sent to begin graceful shutdown. Expects a signal name such as TERM or QUIT. | -| `.drain.wait` | string | — | Time allowed for the proxy to stop routing before shutdown begins. Expects a duration such as 30s, 5m, 1h30m or 14d. | +| `.drain.wait` | string | — | Time allowed for the proxy to stop routing before shutdown begins, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d. | | `.entrypoint` | list | — | Container entrypoint as a string or argument list. Also accepts an entrypoint or argument list. | | `.env` | map | — | Literal container environment values. Managed-service credential variables cannot be overridden. | | `.env_files` | list | — | Workload-specific ordered environment-file list. Replaces broader defaults when present. | @@ -47,12 +47,12 @@ cannot drift from what `ob validate` accepts. | `.health` | object | — | Readiness check used to gate rolling replacement. Also accepts an HTTP health path. | | `.health.exec` | — | — | Health command as a shell string or direct argument list. | | `.health.http` | string | — | HTTP path probed inside the container. Expects a path beginning with /. | -| `.health.interval` | string | — | Delay between container health probes. Expects a duration such as 30s, 5m, 1h30m or 14d. | +| `.health.interval` | string | `5s` | Delay between container health probes, at most 7d. Always written into the generated healthcheck, so the rollout's drain budget is computed from the value the container actually runs with. Expects a duration such as 30s, 5m, 1h30m or 14d. | | `.health.port` | integer | — | Container port probed by HTTP or TCP health checks. | -| `.health.retries` | integer | — | Consecutive failed probes before the container is unhealthy. | -| `.health.start_period` | string | — | Startup grace period before failed probes count. Expects a duration such as 30s, 5m, 1h30m or 14d. | +| `.health.retries` | integer | `3` | Consecutive failed probes before the container is unhealthy. A draining container leaves rotation after this many probes, so it sets how long a rolling deploy waits for each replica. | +| `.health.start_period` | string | `30s` | Startup grace period before failed probes count, at most 7d. Always written into the generated healthcheck, so writing down a fast probe interval does not call a booting container unhealthy. Expects a duration such as 30s, 5m, 1h30m or 14d. | | `.health.tcp` | boolean | `false` | Probe the configured port by opening a TCP connection. | -| `.health.within` | string | — | Maximum time a rollout waits for readiness. Expects a duration such as 30s, 5m, 1h30m or 14d. | +| `.health.within` | string | — | Maximum time a rollout waits for readiness, at most 7d. Expects a duration such as 30s, 5m, 1h30m or 14d. | | `.hostname` | string | — | Hostname assigned inside the workload container. | | `.image` | object | — | Container image source, written as a reference string or an object. Also accepts an image reference. | | `.image.pull` | `always` · `missing` · `never` | `missing` | When to fetch the image from the registry: missing fetches only what the host does not already hold, always fetches every release, never fetches at all and fails on a missing image. |