feat: add healthCheckFailureDelay to shutdown config - #9363
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| 1. Kubernetes sends SIGTERM to the pod | ||
| 2. Shutdown manager fails health checks via `/healthcheck/fail` | ||
| - This causes Kubernetes readiness probes to fail | ||
| - External load balancers and services stop routing new traffic to the pod |
There was a problem hiding this comment.
They stop routing traffic, but it can take many seconds - for example for an load balancers like provided for GCP for a service of type: LoadBalancer.
![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: 7b759222e4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
30c704f to
77d11ae
Compare
77d11ae to
8c81595
Compare
|
@zirain could you approve the CI to run? |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (25.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #9363 +/- ##
==========================================
- Coverage 76.04% 76.04% -0.01%
==========================================
Files 259 259
Lines 43258 43278 +20
==========================================
+ Hits 32896 32911 +15
- Misses 8174 8180 +6
+ Partials 2188 2187 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
hey @consideRatio does this PR fix the issue on GKE ? could you test this for http/1.1 and h2 with a moderate rps |
|
Thank you for investing time to this @arkodg!!
Yes fully, I'm very happy with the outcome! I'm using this (pre-rebase) branch build currently, making my service successfully respond to all requests even when spamming requests (http/1.1) via new and existing connection while a node with just one envoy pod is terminating (the previous failure mode).
I aim to do this during the coming weekend. I'm currently vacationing away from my computer. To clarify, there isnt tooling or docs to run such test in the project already right? |
8c81595 to
e6be2a9
Compare
consideRatio
changed the title
f5c2235 to
300a264
Compare
Allow Envoy listener drain to start immediately while delaying `/healthcheck/fail` during pod termination. This helps deployments that need the terminating pod to remain a ready local endpoint while upstream load balancers stop sending traffic to the node. The default remains 0s, preserving the existing behavior where `/healthcheck/fail` starts listener drain immediately. Signed-off-by: Erik Sundell <erik@sundellopensource.se>
300a264 to
89d8e2e
Compare
any update on this ? |
I didnt have a http 2 workload, and ran out of time to spend =\ it is http 1.1 tested before and the functionality triggered as expected etc, but didnt have a existing http 2 workload I'll rebuild an image on the current main and reverify every keeps working as expected with a after http 1.1 load and such, thr new config name, and now that its based on an updated main branch |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89d8e2ef71
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4f879fc to
118e833
Compare
|
@arkodg okay retested with latest changes on main branch, with a envoy gateway 1.8.3 helm chart with image built based on this PR branch, where i provided http 1.1 load. Everything behaves as expected! It will be significant effort for me to do http 2 testing as well, but I think this PR doesn't really change something related to http 1.1 / http 2 differences, right? |
Signed-off-by: Erik Sundell <erik@sundellopensource.se>
118e833 to
e4c8dde
Compare
|
(force-pushed to fix DCO) |
2 participants
What
Adds
healthCheckFailureDelaytoShutdownConfig.When configured, the shutdown manager starts Envoy listener drain immediately with
/drain_listeners?graceful&skip_exit, then delays/healthcheck/failuntil the configured duration has elapsed. The default remains0s, preserving the existing behavior where/healthcheck/failstarts listener drain immediately.This is useful for environments where failing health checks (and thereby readiness) immediately can leave a node without ready local endpoints before upstream load balancers have stopped sending traffic to it.
In practice, I experienced the need for this on GKE with Cilium / (they call it Dataplane v2), with a envoy gateway helm chart deployed as Gateway API controller. The envoy gateway Service of
type: LoadBalancerhadexternalTrafficPolicy: Localby default, and the GCP provided LoadBalancer for the Service resource takes many seconds to realize that no non-terminating pods on the node are available - so it kept sending traffic to the node. The node receiving the traffic then didn't pass it forward to other pods on other node (because ofexternalTrafficPolicy: Local), and since the pod on the node wasn't just terminating, but non-ready, new connection was refused.Validation
I did a e2e test in the GKE cluster where I observed issues before, and confirmed that the issue was resolved using an image built from this PR branch.
go test ./internal/cmd/envoy ./internal/infrastructure/kubernetes/proxy ./api/v1alpha1/validation git diff --check