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. |