Repository navigation
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: wbpcode <wbphub@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
535408f to
ad2e2ed
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10295 +/- ##
==========================================
+ Coverage 82.19% 82.21% +0.01%
==========================================
Files 277 277
Lines 44014 44027 +13
==========================================
+ Hits 36178 36196 +18
+ Misses 7835 7830 -5
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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: d5ec7a23f3
ℹ️ 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".
| // Defaults to immediate when unset. | ||
| return "immediate" |
There was a problem hiding this comment.
When this setting is supplied through the file-loaded EnvoyGateway.envoyProxy defaults rather than an EnvoyProxy CRD, the kubebuilder enum marker is never enforced and validateEnvoyProxySpec does not validate Shutdown; an unsupported value such as Graduel therefore reaches this catch-all and is emitted as --drain-strategy immediate. That configuration passes startup validation but silently defeats the requested gradual drain, so validate the enum in the non-CRD path or return an error here rather than treating every non-Gradual value as the unset default.
Useful? React with 👍 / 👎.
|
thanks for adding this !
|
I agree to set the But all these may needn't to be part of this PR because they could be relatively independent changes? |
without the changes in the shutdown code logic, will TCP listeners gracefully drain or immediately with |
gracefully, I can create related PRs also anyway. :) |
I see why. The EG use the Then, yeah, if the Good catch. Thanks. |
Signed-off-by: wbpcode <wbphub@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a1ec08c2a
ℹ️ 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".
| MaxConnectionsToAcceptPerSocketEvent: maxAcceptPerSocketEvent, | ||
| // Drain on listener modification/removal and on the drain sequence started by the shutdown | ||
| // manager, but not on /healthcheck/fail, which would bypass the configured drain strategy. | ||
| DrainType: listenerv3.Listener_MODIFY_ONLY, |
There was a problem hiding this comment.
During a Kubernetes rolling upgrade from a previous Envoy Gateway release (or with a pinned older shutdownManager.image), the old pods still run the prior Shutdown path, which at the default zero healthCheckFailureDelay only posts /healthcheck/fail. As soon as the new control plane sends this MODIFY_ONLY listener to those pods, that endpoint explicitly no longer starts their listener drain, so long-lived connections can remain open until the old manager hits drainTimeout and the rollout kills them; preserve the old manager's drain behavior during version skew or gate this listener mode until every sidecar is upgraded.
Useful? React with 👍 / 👎.
Signed-off-by: wbpcode <wbphub@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 313b85ea71
ℹ️ 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".
| MaxConnectionsToAcceptPerSocketEvent: maxAcceptPerSocketEvent, | ||
| // Drain on listener modification/removal and on the drain sequence started by the shutdown | ||
| // manager, but not on /healthcheck/fail, which would bypass the configured drain strategy. | ||
| DrainType: listenerv3.Listener_MODIFY_ONLY, |
There was a problem hiding this comment.
When existing proxies receive the first LDS snapshot from the upgraded control plane, unconditionally serializing MODIFY_ONLY changes every application listener even if drainStrategy is unset. As the adjacent comment notes, Envoy drains listeners on modification/removal, and those existing pods were launched with --drain-strategy immediate, so this update starts draining connections across the whole fleet at once rather than through the Deployment's rolling termination. Gate this xDS change on an opted-in strategy/version or stage the migration so default upgrades do not trigger a fleet-wide connection drain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yeah. Control plane upgrading will result in a draining of listener because we changed the MODIFY_ONLY changes.
🤔
There was a problem hiding this comment.
Now, EG will only change the drain_type when the drain-strategy is set to gradual.
Signed-off-by: wbpcode <wbphub@gmail.com>
|
/retest |
What this PR does / why we need it:
Envoy Gateway always starts Envoy with
--drain-strategy immediate, and the flag cannot be overridden throughextraArgsbecause Envoy rejects a duplicated flag.This adds an optional
drainStrategyfield (GradualorImmediate) toEnvoyProxy.spec.shutdown, which maps to Envoy's--drain-strategy. When unset, it defaults toImmediate, so existing behavior is unchanged. No+kubebuilder:defaultis set, so the default is not persisted into stored objects.Which issue(s) this PR fixes:
Fixes #
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.