Fix: OCPBUGS-93148: installer skip NotReady nodes during static pod revision rollout - #2337
Fix: OCPBUGS-93148: installer skip NotReady nodes during static pod revision rollout#2337dpateriya wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise 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 (3)
WalkthroughThe installer controller now uses NodeLister-backed readiness-age checks to skip long-NotReady nodes during rollout ordering and to delete installer pods stuck on those nodes past a timeout. Wiring and tests were updated to cover the new behavior. ChangesInstaller NodeReady-aware rollout
Estimated code review effort: 4 (Complex) | ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dpateriya 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/staticpod/controller/installer/installer_controller.go`:
- Around line 966-968: The stuck installer pod cleanup path in the installer
controller is swallowing real Delete failures, which lets the code continue as
if the pod was removed. Update the cleanup logic around
c.podsGetter.Pods(...).Delete in the installer controller so that non-NotFound
errors are returned to the caller instead of only logging a warning, and ensure
the code that advances LastFailed* or otherwise mutates node state only runs
after deletion succeeds. Keep the node state unchanged when deletion fails, so
the retry logic in the installer controller can retry cleanup before proceeding.
In `@pkg/operator/staticpod/controller/installer/installer_notready_test.go`:
- Around line 87-89: The fallback check in installer_notready_test.go is too
weak because it only asserts that reason is non-empty instead of verifying the
expected substring. Update the test branch in the relevant loop to use
strings.Contains against tt.expectedContains when validating reason, and add the
strings import so the assertion actually fails if the message regresses. Use the
existing tt.expectedContains and reason variables in the test helper.
🪄 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: Enterprise
Run ID: 8689a70f-b28d-4eb9-9d89-1dd7b10164ea
📒 Files selected for processing (4)
pkg/operator/staticpod/controller/installer/installer_controller.gopkg/operator/staticpod/controller/installer/installer_controller_test.gopkg/operator/staticpod/controller/installer/installer_notready_test.gopkg/operator/staticpod/controllers.go
dpateriya
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dpateriya: This pull request references Jira Issue OCPBUGS-93148, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci-robot
added
the
jira/invalid-bug
5c603f8 to
9dae900
Compare
openshift-ci-robot
added
jira/valid-bug
|
@dpateriya: This pull request references Jira Issue OCPBUGS-93148, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
Aside from the technicalities involved in the static pod machinery that makes this approach infeasible, the cert rotation has a 9 month (!) buffer for you to fix your node: are you telling me that there is a node that was not ready for 9 consecutive months? |
|
Hi @tjungblu, it is not about the etcd certificate. It is about the aggregator-client, aggregator-client-signer, and csr-signer certificates, which have a shorter validity period compared to etcd certs. When these certificates rotate, the new cert is written to etcd and a new revision is triggered. The installer controller then targets the NotReady node first because nodeToStartRevisionWith prioritizes nodes whose static pod is not reporting Ready — which includes nodes that are Kubernetes-NotReady (since kubelet is down, the mirror pod status is stale/absent). The customer had a master node in NotReady state for 12 days due to hardware failure. They were aware of this and the fix (hardware replacement) was estimated to take another 2-3 weeks. The cluster was expected to continue operating with 2 healthy masters during this period. Due to the aggregator-client and aggregator-client-signer certificate rotation triggering a new revision that got stuck on the NotReady node, the customer was unable to login to the OCP cluster via console and CLI (401 Unauthorized for all users). |
|
Also, this fix preserves the one-at-a-time rollout invariant; it only changes which node is picked first in the ring. |
|
@tjungblu, can you please do the needful once you have time? |
@dpateriya please check but I think these certificates are unrevisioned. https://github.com/openshift/cluster-kube-apiserver-operator/blob/main/pkg/operator/starter.go#L675C1-L675C29 Unrevisioned resources are delivered by a sidecar and don't go through the revision/installer mechanism. The static pods reload them dynamically from disk without restart. If that the case then it could be that something else broke the cluster. |
|
@p0lyn0mial , I did confirm the same from the must-gather report. While the leaf aggregator-client cert is indeed delivered by cert-syncer (unrevisioned), the aggregator-client-ca ConfigMap IS a revisioned resource (visible in CertConfigMapNamePrefixes). When the signer rotated, it updated this CA bundle, triggering revision 301. The InstallerController selected the NotReady node first for revision 301, got stuck, and never proceeded to the two healthy nodes. The healthy nodes remained on revision 299 with the old CA trust bundle, so they could not validate the newly rotated leaf certs delivered by cert-syncer — resulting in x509: certificate has expired errors and authentication failures. Once the NotReady node was removed (unblocking the operator), the revision rolled out to the healthy nodes and login was immediately restored — confirming the revision contained the critical aggregator-client-ca update. The fix ensures the InstallerController deprioritizes NotReady nodes so that healthy nodes receive the new revision first, preventing this class of outage. |
@dpateriya please double check but it seems that have a look at: https://github.com/openshift/cluster-kube-apiserver-operator/blob/main/pkg/operator/starter.go#L661 |
|
@p0lyn0mial you are correct, However, the must-gather evidence clearly shows that revision 301 was created (likely triggered by a revisioned resource such as kube-apiserver-cert-syncer-kubeconfig, sa-token-signing-certs, or bound-sa-token-signing-certs) and got stuck on the NotReady node: The InstallerController selected the NotReady node first for revision 301. The two healthy nodes with targetRevision: 0 were never started. Once the NotReady node was removed and the revision completed on the healthy nodes, login was immediately restored. Regardless of which specific revisioned resource triggered revision 301, the core issue is that any new revision gets stuck when the InstallerController targets a NotReady node first, blocking healthy nodes from progressing. This fix ensures healthy nodes are prioritized so the cluster remains functional even with a prolonged NotReady master. I'd value your perspective on whether deprioritizing NotReady nodes in |
are you sure, Claude? because in the support case this is not the case, the customer wanted to keep the not-ready node around at all cost. The procedure applied was:
But it is not clear which of those restarts were actually fixing the issue. Maybe just restarting oauth-apiserver would've fixed it. They have experienced login issues and they're using IDP. I think somebody from auth should take a look at this case first and analyse the must-gather before we start plumbing the static pod machinery here. |
|
@tjungblu, claude was not on the call with the customer. I was on the call with the customer. When I said I removed the master node, it meant that the master role (node-role.kubernetes.io/master=) from the not ready master node was removed, and then the installercontroller targeted the rest of available 2 master nodes. Manual restart of oauth-openshift pods, openshift-apiserver pods, openshift-oauth-apiserver pods and even static pods like KCM, KAS were restarted. The static pods were restarted by moving their pod yaml from /etc/kubernetes/manifests to /home/core and then again moved back to the original location. All these were the efforts made during the remote call. But this does not change the fact that the installer controller prioritizes the not ready master node first. |
| // nodeNotReadyThreshold is how long a node must be Kubernetes-NotReady before | ||
| // the installer controller deprioritizes it in rollout ordering. This prevents | ||
| // a prolonged hardware failure from blocking cert rotation on healthy masters. | ||
| nodeNotReadyThreshold = 10 * time.Minute |
There was a problem hiding this comment.
a bare metal node can take about an hour to reboot, so the node will be not ready for 50 minutes. Will this cause etcd downtime when a static pod revision rolls out?
There was a problem hiding this comment.
AFAIK, etcd can suffer a loss/unavailability of one master node.
So even if a bare-metal node takes hours to reboot, this will have no downtime for etcd.
Also, the KAS revision rollout does not trigger etcd pod rollout.
Taking your feedback into consideration, we can increase the nodeNotReadyThreshold to 60 minutes to avoid unnecessary skipping during normal reboots for bare-metal nodes while still catching prolonged outages (the incident involved 12+ days NotReady).
Please confirm.
There was a problem hiding this comment.
You understand that the code is shared between all control plane operators, right?
So even if a bare-metal node takes hours to reboot, this will have no downtime for etcd.
of course it does, because your code doesn't stop the rollout.
There was a problem hiding this comment.
Agree, so a bare-metal node reboot + etcd revision rollout on the other master node will briefly trigger quorum loss and a cluster outage.
We can exclude etcd without adding any new methods or interfaces. The etcd-operator is the only consumer that sets WithRevisionControllerPrecondition (for quorum safety). KAS, KCM, and scheduler don't use it.
So the operators like etcd, which are quorum-sensitive, will not get the NotReady skip and all others like KAS, KCM, and scheduler will do.
There was a problem hiding this comment.
One condition change in controllers.go line 300:
// Before:
if clusterInformers != nil {
// After:
if clusterInformers != nil && b.revisionControllerPrecondition == nil {
I think this would help then.
There was a problem hiding this comment.
Changes have been made.
You can verify the same.
9dae900 to
51b392e
Compare
|
@dpateriya: This pull request references Jira Issue OCPBUGS-93148, which is valid. 3 validation(s) were run on this bug
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/staticpod/controller/installer/installer_controller.go (1)
983-998: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a single time source in this block.
podAgeis computed fromc.clock.Now()(Line 983) whileLastFailedTimeis set fromc.now()(Line 998). These are two independently injectable clocks (clockandnow), so they can diverge under test and are needlessly inconsistent. Pick one time source here for the failure record.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/staticpod/controller/installer/installer_controller.go` around lines 983 - 998, The stuck-installer-pod handling block uses two different time sources, which makes the failure record inconsistent and can diverge in tests. In the installer_controller.go logic around the installer pod timeout handling, use a single source for both podAge and the LastFailedTime assignment, ideally the same clock already used by c.clock.Now(), and keep the rest of the failure path in sync with that choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/staticpod/controller/installer/installer_controller.go`:
- Around line 553-576: The stale targetRevision cleanup in
installer_controller.go should also delete the stuck installer pod before
returning. In the loop over operatorStatus.NodeStatuses inside the staticpod
installer controller logic, update the branch that clears a NotReady node’s
targetRevision so it first invokes the existing pod cleanup path used for
timeout-based cleanup, then returns the updated NodeStatus. This ensures the
installer pod is removed even when the early return short-circuits the later
cleanup logic.
---
Nitpick comments:
In `@pkg/operator/staticpod/controller/installer/installer_controller.go`:
- Around line 983-998: The stuck-installer-pod handling block uses two different
time sources, which makes the failure record inconsistent and can diverge in
tests. In the installer_controller.go logic around the installer pod timeout
handling, use a single source for both podAge and the LastFailedTime assignment,
ideally the same clock already used by c.clock.Now(), and keep the rest of the
failure path in sync with that choice.
🪄 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: Enterprise
Run ID: 24579fe3-09de-4474-8053-ad71f5300475
📒 Files selected for processing (4)
pkg/operator/staticpod/controller/installer/installer_controller.gopkg/operator/staticpod/controller/installer/installer_controller_test.gopkg/operator/staticpod/controller/installer/installer_notready_test.gopkg/operator/staticpod/controllers.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/staticpod/controllers.go
- pkg/operator/staticpod/controller/installer/installer_controller_test.go
- pkg/operator/staticpod/controller/installer/installer_notready_test.go
A node that has been Kubernetes-NotReady for more than 10 minutes is now deprioritized in the installer controller's rollout ring. Additionally, installer pods stuck on such nodes for more than 15 minutes are force-failed so the rollout proceeds on healthy masters. The fix covers all node selection paths in nodeToStartRevisionWith: - TargetRevision (in-progress) loop - LastFailedRevision loop - Not-ready static pod loop - Wrong-revision loop - Oldest-revision loop A node informer is wired into the controller factory so that changes in node readiness trigger controller resync. This prevents a single hardware-failed master from blocking certificate rotation (and other revision-driven updates) across the entire cluster, which previously caused authentication outages. Bug: https://redhat.atlassian.net/browse/OCPBUGS-93148 Co-authored-by: Cursor <cursoragent@cursor.com>
51b392e to
9d4c867
Compare
|
@dpateriya: 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. |
|
Hi Team, can someone please re-review this? |
4 participants
Summary
Problem
When a master node enters a prolonged NotReady state (e.g., hardware failure), the InstallerController's
nodeToStartRevisionWithfunction selects it first (because its static pod isn't reporting Ready). This blocks the entire revision rollout ring, preventing healthy masters from receiving updated configurations.This is critical during certificate rotation: new certificates are written to etcd but operators cannot roll out new revisions to disk. Static pods on healthy masters continue using old (expired) certificate files, causing cascading authentication failures (401 Unauthorized for all users).
Root Cause
The InstallerController had no awareness of the Kubernetes
NodeReadycondition. It only checked static pod readiness (via mirror pod status), which is unreportable when a node is down. Combined withtolerations: [{operator: Exists}]on installer pods, installations were attempted on unreachable nodes indefinitely.Fix Design
Layer 1: Deprioritization in
nodeToStartRevisionWithAll selection loops (TargetRevision, LastFailedRevision, not-ready static pod, wrong-revision, oldest-revision) now skip nodes where
isNodeNotReadyForTooLongreturns true.Layer 2: Timeout in
newNodeStateForInstallInProgressIf an installer pod has been in a non-terminal phase on a NotReady node for more than 15 minutes, it is deleted and the installation is marked as failed. The normal back-off retry mechanism then applies.
Backward Compatibility:
WithNodeListeris opt-in; whennodeListeris nil, all checks return false (legacy behavior preserved)nilfor the new parameterTest Plan
isNodeNotReadyForTooLong(Ready, below threshold, above threshold, Unknown status, no condition, nil lister)nodeToStartRevisionWithwith NotReady skip (single NotReady, multiple NotReady, all NotReady fallback)installer_controller_test.gotests pass unchangedgo build ./...passesReferences
Made with Cursor
Summary by CodeRabbit