Honor start_period grace before the readiness rollout deadline#183
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Readiness health checks now accept an optional
start_periodgrace window. Some containers legitimately fail readiness for their whole boot because they build their app after starting (e.g.bun run buildbehind Caddy). Without a grace, a shortRING_ROLLOUT_DEADLINEfails a perfectly healthy slow-building app the instant its build outlasts the deadline.With
start_period, the rollout deadline only starts counting after the grace window. The effective budget becomesstart_period + RING_ROLLOUT_DEADLINE. Forcommandchecks on Docker, the value is also forwarded to the nativeHEALTHCHECK start_period.readiness: false. Same duration syntax asinterval/timeout.Bug caught by e2e
The
applyclient re-declares its ownHealthCheckstruct to parse YAML, and it was missing the new field — sostart_periodwas silently dropped before ever reaching the API, leaving the feature inert. Cargo tests build the deployment in memory and never exercise the apply → API → DB round-trip, so they passed while the feature did nothing. The e2e (which drives the real binary) surfaced it: the storedhealth_checksJSON had nostart_period, and a deployment withstart_period: 40sstill failed at the bare 10s deadline. Fixed by adding the field to the apply-side struct.Tests
tests/e2e/docker/t37_readiness_start_period.sh(new): with a never-ready probe and a short deadline, proves a no-grace deployment fails at the deadline, astart_period: 40sdeployment survives well past it, and the deferred safety valve still fires once the budget elapses.t22_readiness_rolling_update.shstill green (no regression on the readiness gate).