Add deployment.minReadySeconds/progressDeadlineSeconds values field to gateway-helm chart - #9919
Add deployment.minReadySeconds/progressDeadlineSeconds values field to gateway-helm chart#9919lboynton wants to merge 1 commit into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de76e860f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| {{- with .Values.deployment.minReadySeconds }} | ||
| minReadySeconds: {{ . }} |
There was a problem hiding this comment.
When deployment.minReadySeconds is set to 600 or more, the rendered Deployment provides no corresponding progressDeadlineSeconds, so Kubernetes defaults that field to 600 and rejects the resource because the progress deadline must be greater than minReadySeconds. This makes plausible bake periods such as ten minutes fail during installation or upgrade; expose a configurable progress deadline or validate/restrict this value before rendering.
Useful? React with 👍 / 👎.
| # proxies (and anything else relying on this controller's xDS/SDS streams) a bake period to | ||
| # reconnect and resync against a stable subset of replicas before the next one is cycled, | ||
| # bounding how many proxies can be disrupted at once by a single controller rollout. | ||
| minReadySeconds: 0 |
There was a problem hiding this comment.
This introduces a user-facing Helm capability, but the commit does not add the release-notes/current/new_features/<pr-number>-<slug>.md fragment required by release-notes/current/README.md:3-22. Without the fragment, the new option will be omitted when the next release notes are compiled.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9919 +/- ##
==========================================
- Coverage 81.29% 81.27% -0.02%
==========================================
Files 263 263
Lines 40870 40870
==========================================
- Hits 33224 33219 -5
- Misses 7646 7651 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The chart already exposes deployment.envoyGateway.strategy for tuning maxSurge/maxUnavailable during a rolling update, but there's no way to add a bake period between replacement steps. With the low replica counts typical for this controller, a rollout can replace every replica within seconds of each other, leaving no time for proxies maintaining long-lived xDS/SDS subscriptions against this controller to reconnect and resync before the next replica cycles too. minReadySeconds is the standard Kubernetes Deployment field for this; expose it as a chart value, defaulting to 0 (the Kubernetes default, so no behaviour change unless set). Also expose deployment.progressDeadlineSeconds alongside it: Kubernetes requires progressDeadlineSeconds to exceed minReadySeconds and defaults it to 600, so a minReadySeconds of 600 or more would otherwise make the Deployment fail to apply with no way to raise the deadline to match. Adds the release-notes fragment required by release-notes/current/README.md. Signed-off-by: Lee Boynton <lee.boynton@truelayer.com>
de76e86 to
df3f30b
Compare
lboynton
changed the title
1 participant
What this does: exposes the standard Kubernetes
Deployment.spec.minReadySecondsfield as a chart value (deployment.minReadySeconds, defaulting to0, matching the Kubernetes default — no behaviour change unless set).Why: the chart already lets you tune
deployment.envoyGateway.strategy(maxSurge/maxUnavailable), which controls how many replicas can be mid-rollout at once, but there's no way to add a bake period between replacement steps. On a rolling update withmaxUnavailable: 0, maxSurge: 1(the effective default at low replica counts), each new pod is considered available the instant its readiness probe passes, and the controller immediately proceeds to replace the next replica. On a 3-replica deployment this means a full rollout can cycle every replica within seconds — fine for the controller's own availability, but it doesn't leave time for downstream consumers of its xDS/SDS streams (Envoy proxy pods maintaining long-lived gRPC subscriptions against this controller) to actually reconnect and resync before the next replica goes down too. We hit exactly this: a full-fleet restart within ~13 seconds forced effectively every connected proxy through a reconnect-and-resubscribe cycle inside one tight window, rather than spreading that load out one replica at a time.minReadySecondsis the standard Kubernetes lever for this — a mandatory pause after a new pod becomes Ready before the rollout controller treats it as available and proceeds to the next replacement. It's already supported natively byDeployment; this chart just doesn't expose it as a value yet, so it can't be set without a post-render patch.Scope: minimal — one new value, one new conditional block in the existing deployment template, following the same
{{- with ... }}pattern already used forstrategyimmediately above it. No default behaviour change (0is the Kubernetes default).Testing: rendered the chart locally with
deployment.minReadySeconds: 90set and confirmedspec.minReadySeconds: 90appears on the renderedDeployment, with the field omitted entirely (as before) when left at the default0/unset. RegeneratedREADME.md/api.mdviamake helm-readme-gen.gateway-helm.