SREP-4577: Fix webhook drain deadlock with preStop hook and failurePolicy: Ignore#717
SREP-4577: Fix webhook drain deadlock with preStop hook and failurePolicy: Ignore#717dustman9000 wants to merge 3 commits intoopenshift:mainfrom
Conversation
The addon-operator-webhooks pod gets stuck in Terminating during MCO node drains, blocking the drain for 1+ hour and causing cascading alerts (ControlPlaneNodeUnschedulableSRE, ClusterMonitoringErrorBudgetBurnSRE). The root cause is a race condition: when the pod is evicted, it stops serving but remains registered as a webhook backend. With failurePolicy: Fail, API calls hang waiting for the dying pod. Adding a 5s preStop sleep gives the endpoints controller time to remove the pod from the Service before it stops serving, breaking the deadlock. Observed twice in 2 hours on rosaprod.gxp9.p1 (af-south-1). Both times required force-deleting the stuck pod to unblock the drain. Ref: https://redhat.atlassian.net/browse/SREP-4577
openshift-ci-robot
added
the
jira/valid-reference
|
@dustman9000: This pull request references SREP-4577 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdded a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server as API Server
participant Webhook as Webhook Server
participant Kubelet
participant Pod as Webhook Pod
Client->>API_Server: submit object (create/update)
API_Server->>Webhook: call validating webhook
alt Old (failurePolicy: Fail)
Webhook--x API_Server: timeout/error
API_Server->>Client: reject request (admission failed)
else New (failurePolicy: Ignore)
Webhook--x API_Server: timeout/error
API_Server->>Client: accept request (proceed despite webhook failure)
end
Note over Kubelet,Pod: Pod termination sequence
Kubelet->>Pod: SIGTERM
Pod->>Pod: preStop hook (sleep 5)
Pod-->>Kubelet: container exits after preStop completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dustman9000 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
With failurePolicy: Fail, the API server hangs when the webhook pod is being evicted during node drains. The dying pod cannot serve requests but is still registered as a backend, creating a deadlock that blocks MCO drains for 1+ hour. Changing to failurePolicy: Ignore means the API server skips validation during the brief window when the webhook is unavailable, rather than blocking the entire node drain. The second replica continues serving, so most requests still get validated. Combined with the preStop sleep from the previous commit, this provides defense-in-depth: the preStop gives time for endpoint deregistration, and failurePolicy: Ignore prevents hangs if that race is still hit. Ref: https://redhat.atlassian.net/browse/SREP-4577
dustman9000
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/templates/csv-template.yaml`:
- Line 56: The CSV webhook configuration currently sets failurePolicy: Ignore in
config/templates/csv-template.yaml which makes the webhook fail-open; either
explicitly document that this tradeoff is intentional in the manifest (add a
clear comment near failurePolicy explaining why failing open is acceptable and
list compensating controls) or change the webhook failurePolicy to Fail to
enforce validation on webhook unavailability; locate the webhook spec
(failurePolicy field in the CSV/webhook configuration) and either replace Ignore
with Fail or add the explicit justification and link to compensating controls
(replicas, preStop, monitoring/alerting) in the template.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ccee0ca4-d87c-43db-8c1c-ea0afcd92857
📒 Files selected for processing (5)
bundle/manifests/addon-operator.clusterserviceversion.yamlconfig/templates/csv-template.yamldeploy-extras/development/webhook/validatingwebhookconfig.yamldeploy_pko/ValidatingWebhookConfiguration-addons.yamlhack/webhookdefinition.yaml
✅ Files skipped from review due to trivial changes (2)
- hack/webhookdefinition.yaml
- deploy_pko/ValidatingWebhookConfiguration-addons.yaml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #717 +/- ##
=======================================
Coverage 59.24% 59.24%
=======================================
Files 62 62
Lines 4125 4125
=======================================
Hits 2444 2444
Misses 1532 1532
Partials 149 149 🚀 New features to boost your workflow:
|
|
@dustman9000: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Hey Dustin - thanks for the submission! Are you sure that this PR actually fixes the node drain issue you've described? |
4 participants
Summary
Problem
The addon-operator-webhooks pod gets stuck in Terminating during MCO node drains, blocking the drain for 1+ hour. This causes cascading alerts (ControlPlaneNodeUnschedulableSRE, ClusterMonitoringErrorBudgetBurnSRE) and requires SRE force-deletion to resolve.
Observed twice in 2 hours on the same cluster (rosaprod.gxp9.p1, af-south-1), on two different infra nodes.
Root Cause
Race condition during eviction: the pod stops serving requests but remains registered as a webhook backend. With failurePolicy: Fail, API calls to the dying pod hang rather than failing open. This prevents clean termination and blocks the MCO drain indefinitely.
Fix
Two changes that provide defense-in-depth:
preStop sleep (5s): Gives the endpoints controller time to remove the pod from the Service before it stops serving. Standard Kubernetes pattern for webhook pods.
failurePolicy: Ignore: If the webhook pod is unavailable during drain, the API server skips validation instead of hanging. The second replica continues serving most requests, so the validation gap is minimal (a few seconds during node drain).
Jira: https://redhat.atlassian.net/browse/SREP-4577
Test plan
Summary by CodeRabbit