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? |
| - `readinessFailureDelay`: 0 seconds - Optional delay before failing readiness after drain starts | ||
|
|
||
| Configure `readinessFailureDelay` when failing readiness immediately would leave | ||
| the node without ready local endpoints before upstream load balancers have |
There was a problem hiding this comment.
can this section be rephrased so it highlights that this field may need to be set only when CNIs eagerly rm endpoints even when terminating ? relates to cilium/cilium#40969 (comment)
There was a problem hiding this comment.
Hmmmm my observation was that i had trouble with non-ready and terminating endpoints, not with terminating endpoints - the pod will still be terminating just not become non-ready - and this is being addressed in the issue.
It sounds like it is the reasonable behavior to not route traffic to a non-ready pod, no matter what.
I may figure out in more detail what you mean, and i'll think extensively about it during the weekend, but i can't yet clearly see when this ought/ought not to be configured. If you have a sharp description on when already, it could help me out a lot!
There was a problem hiding this comment.
Key findings
- Cilium 1.18.3 introduced a fix (loadbalancer: Keep backends around that are terminating and not serving cilium/cilium#40969) for ensuring existing connections traffic arrive, and I've experienced issues with Cilium 1.18.7 related to new connections traffic
- The envoy gateway controller (not the envoy gateway helm chart itself) creates k8s Service resources with
type: LoadBalancerandexternalTrafficPolicy: Localby default - A provisioned LoadBalancer for the Service resource will likely not itself be pod aware, and route to nodes. A controller will make it route to relevant nodes, but it may be slow to update its node targets, and slower than terminating pods become non-ready.
- The readiness probe is by default configured to be probed every 5 seconds with a failure threshold of 1, so a terminating pod will become non-ready in 0-5 seconds.
- A new connection arriving to a node, having only non-ready terminating pods, will fail to route to a pod.
My conclusions
Feature relevance
There is relevance for this feature whenever a provisioned Load Balancer routes to nodes, which I think it often will be, unless the readiness probe is adjusted to fail slower than the node targets are updated by the load balancer's controller.
I think something like this is an essential feature!
Related but off topic thinking
- I doubt having the configured k8s health probe fail at all is desirable to signal something, and assume its an accepted side consequence of wanting to fail the readiness probe to signal something and using the same endpoint.
- I'm not aware when it makes sense to fail the readiness probe to signal something, but for GKE's load balancers, its not relevant. They will route traffic elsewhere just by observing the only pods on a node are terminating - ready or not.
There was a problem hiding this comment.
I really struggle! Should the feature instead be a toggle of if the healthcheck/fail call should be made at all?
When is there value to calling healthcheck/fail as part of termination and make k8s health and readiness probes fail? When is there value in delaying that rather than toggling doing it or not?
I only know a value to not do it at all (solves my issue fully), and think it could be a better default even!
There was a problem hiding this comment.
@arkodg I transitioned this into draft, as I now question if a delay or toggle should be developed, and struggled so hard with docs on when to configure a specific delay - I however think it needs your or other maintainers input on this.
There was a problem hiding this comment.
hey LGTM with the way it currently reads
| // If unspecified, defaults to 0 seconds. | ||
| // | ||
| // +optional | ||
| ReadinessFailureDelay *gwapiv1.Duration `json:"readinessFailureDelay,omitempty"` |
There was a problem hiding this comment.
can we call this HealthCheckFailureDelay ? since it also impacts https://gateway.envoyproxy.io/docs/api/extension_types/#healthchecksettings
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".
| if err != nil { | ||
| return nil | ||
| } | ||
| command = append(command, fmt.Sprintf("--health-check-failure-delay=%.0fs", d.Seconds())) |
There was a problem hiding this comment.
When healthCheckFailureDelay is configured with a valid subsecond value such as 400ms, formatting d.Seconds() with %.0f produces --health-check-failure-delay=0s. Shutdown consequently takes the immediate-failure branch, so the configured delay is lost entirely. The generated CRD explicitly accepts ms durations, so preserve the original duration or use d.String() rather than rounding to whole seconds.
Useful? React with 👍 / 👎.
ac96af4 to
4f879fc
Compare
4f879fc to
118e833
Compare
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