Skip to content

Fix: OCPBUGS-93148: installer skip NotReady nodes during static pod revision rollout #2337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dpateriya wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix: OCPBUGS-93148: installer skip NotReady nodes during static pod revision rollout #2337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 142 additions & 3 deletions pkg/operator/staticpod/controller/installer/installer_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/client-go/informers"
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
corev1listers "k8s.io/client-go/listers/core/v1"
"k8s.io/klog/v2"
"k8s.io/utils/clock"
"k8s.io/utils/ptr"
Expand All @@ -45,6 +46,16 @@ const (

revisionLabel = "revision"

// 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

@tjungblu tjungblu Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@dpateriya dpateriya Jul 4, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tjungblu tjungblu Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dpateriya dpateriya Jul 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dpateriya dpateriya Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dpateriya dpateriya Jul 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes have been made.

You can verify the same.


// installerPodStuckOnNotReadyNodeTimeout is the maximum duration an installer
// pod can remain in a non-terminal phase on a NotReady node before being
// force-failed so that the rollout can proceed on healthy nodes.
installerPodStuckOnNotReadyNodeTimeout = 15 * time.Minute

nodeStatusOperandFailedReason = "OperandFailed"
nodeStatusInstalledFailedReason = "InstallerFailed"
nodeStatusOperandFailedFallbackReason = "OperandFailedFallback"
Expand Down Expand Up @@ -97,6 +108,11 @@ type InstallerController struct {
eventRecorder events.Recorder
now func() time.Time // for test plumbing

// nodeLister provides access to node objects for checking the NodeReady condition.
// When set, the installer deprioritizes nodes that have been NotReady longer than
// nodeNotReadyThreshold and times out installer pods stuck on such nodes.
nodeLister corev1listers.NodeLister

// installerPodImageFn returns the image name for the installer pod
installerPodImageFn func() string
// ownerRefsFn sets the ownerrefs on the pruner pod
Expand Down Expand Up @@ -133,6 +149,19 @@ func (c *InstallerController) WithMinReadyDuration(minReadyDuration time.Duratio
return c
}

// WithNodeLister enables NodeReady-aware rollout ordering. Nodes that have been
// NotReady for longer than nodeNotReadyThreshold are deprioritized in the rollout
// ring, and installer pods stuck on such nodes are timed out. This prevents a
// single failed master from blocking revision rollout (e.g., cert rotation) on
// healthy masters. The nodeInformer triggers controller resync on node status changes.
func (c *InstallerController) WithNodeLister(nodeLister corev1listers.NodeLister, nodeInformer factory.Informer) *InstallerController {
c.nodeLister = nodeLister
if nodeInformer != nil {
c.factory.WithInformers(nodeInformer)
}
return c
}

func (c *InstallerController) WithCerts(certDir string, certConfigMaps, certSecrets []UnrevisionedResource) *InstallerController {
c.certDir = certDir
c.certConfigMaps = certConfigMaps
Expand Down Expand Up @@ -322,18 +351,54 @@ func (c *InstallerController) getStaticPodState(ctx context.Context, nodeName st

type staticPodStateFunc func(ctx context.Context, nodeName string) (state staticPodState, revision, reason string, errors []string, ts time.Time, err error)

// isNodeNotReadyForTooLong returns true if the named node has been in a
// non-Ready state for longer than nodeNotReadyThreshold. Returns false when
// the node lister is not configured, the node cannot be found, or the node
// has been NotReady for less than the threshold.
func (c *InstallerController) isNodeNotReadyForTooLong(nodeName string) bool {
if c.nodeLister == nil {
return false
}
node, err := c.nodeLister.Get(nodeName)
if err != nil {
klog.V(4).Infof("Cannot get node %s to check readiness, assuming ready: %v", nodeName, err)
return false
}
for _, cond := range node.Status.Conditions {
if cond.Type == corev1.NodeReady {
if cond.Status != corev1.ConditionTrue {
notReadyDuration := c.clock.Now().Sub(cond.LastTransitionTime.Time)
if notReadyDuration > nodeNotReadyThreshold {
klog.Infof("Node %s has been NotReady for %v (threshold %v), deprioritizing for rollout",
nodeName, notReadyDuration.Round(time.Second), nodeNotReadyThreshold)
return true
}
}
return false
}
}
// No NodeReady condition found — the node may be freshly bootstrapping;
// do not penalize it.
return false
}

// nodeToStartRevisionWith returns a node index i and guarantees for every node < i that it is
// - not updating
// - ready
// - at the revision claimed in CurrentRevision.
func nodeToStartRevisionWith(ctx context.Context, getStaticPodStateFn staticPodStateFunc, nodes []operatorv1.NodeStatus) (int, string, error) {
// isNodeNotReadyTooLong, when non-nil, is used to skip nodes that have been Kubernetes-NotReady
// for too long, so that healthy nodes are served first during revision rollout.
func nodeToStartRevisionWith(ctx context.Context, getStaticPodStateFn staticPodStateFunc, nodes []operatorv1.NodeStatus, isNodeNotReadyTooLong func(string) bool) (int, string, error) {
if len(nodes) == 0 {
return 0, "", fmt.Errorf("nodes array cannot be empty")
}

// find upgrading node as this will be the first to start new revision (to minimize number of down nodes)
for i := range nodes {
if nodes[i].TargetRevision != 0 {
if isNodeNotReadyTooLong != nil && isNodeNotReadyTooLong(nodes[i].NodeName) {
continue
}
reason := fmt.Sprintf("node %s is progressing towards %d", nodes[i].NodeName, nodes[i].TargetRevision)
return i, reason, nil
}
Expand All @@ -346,16 +411,24 @@ func nodeToStartRevisionWith(ctx context.Context, getStaticPodStateFn staticPodS
}
for i := range nodes {
if nodes[i].LastFailedRevision > mostCurrent {
if isNodeNotReadyTooLong != nil && isNodeNotReadyTooLong(nodes[i].NodeName) {
continue
}
reason := fmt.Sprintf("node %s is progressing with failed revisions", nodes[i].NodeName)
return i, reason, nil
}
}

// otherwise try to find a node that is not ready. Take the oldest one.
// Skip nodes whose Kubernetes NodeReady condition has been false for too long,
// as they are likely down due to hardware failure and would block the entire ring.
oldestNotReadyRevisionNode := -1
oldestNotReadyRevision := math.MaxInt32
for i := range nodes {
currNodeState := &nodes[i]
if isNodeNotReadyTooLong != nil && isNodeNotReadyTooLong(currNodeState.NodeName) {
continue
}
state, runningRevision, _, _, _, err := getStaticPodStateFn(ctx, currNodeState.NodeName)
if err != nil && apierrors.IsNotFound(err) {
return i, fmt.Sprintf("node %s static pod not found", currNodeState.NodeName), nil
Expand Down Expand Up @@ -383,6 +456,9 @@ func nodeToStartRevisionWith(ctx context.Context, getStaticPodStateFn staticPodS
oldestPodRevision := math.MaxInt32
for i := range nodes {
currNodeState := &nodes[i]
if isNodeNotReadyTooLong != nil && isNodeNotReadyTooLong(currNodeState.NodeName) {
continue
}
_, runningRevision, _, _, _, err := getStaticPodStateFn(ctx, currNodeState.NodeName)
if err != nil && apierrors.IsNotFound(err) {
return i, fmt.Sprintf("node %s static pod not found", currNodeState.NodeName), nil
Expand Down Expand Up @@ -410,6 +486,9 @@ func nodeToStartRevisionWith(ctx context.Context, getStaticPodStateFn staticPodS
oldestCurrentRevision := int32(math.MaxInt32)
for i := range nodes {
currNodeState := &nodes[i]
if isNodeNotReadyTooLong != nil && isNodeNotReadyTooLong(currNodeState.NodeName) {
continue
}
if currNodeState.CurrentRevision < oldestCurrentRevision {
oldestCurrentRevisionNode = i
oldestCurrentRevision = currNodeState.CurrentRevision
Expand All @@ -420,7 +499,8 @@ func nodeToStartRevisionWith(ctx context.Context, getStaticPodStateFn staticPodS
return oldestCurrentRevisionNode, reason, nil
}

reason := fmt.Sprintf("node %s of revision %d is no worse than any other node, but comes first", nodes[0].NodeName, oldestCurrentRevision)
// All nodes are either NotReady or at the latest revision — pick the first available
reason := fmt.Sprintf("all ready nodes are at the latest revision, node %s comes first", nodes[0].NodeName)
return 0, reason, nil
}

Expand Down Expand Up @@ -470,8 +550,37 @@ func (c *InstallerController) manageInstallationPods(ctx context.Context, operat
return false, 0, nil, nil, nil
}

// If a NotReady-for-too-long node still holds a non-zero targetRevision (e.g. it went
// NotReady after being targeted), clear it now so the "only 1 non-zero targetRevision"
// API constraint doesn't block healthy nodes from being targeted.
for i := range operatorStatus.NodeStatuses {
ns := &operatorStatus.NodeStatuses[i]
if ns.TargetRevision != 0 && c.isNodeNotReadyForTooLong(ns.NodeName) {
klog.Infof("Clearing stale targetRevision %d on NotReady node %s to unblock rollout on healthy nodes", ns.TargetRevision, ns.NodeName)
installerPodName := getInstallerPodName(ns)
if err := c.podsGetter.Pods(c.targetNamespace).Delete(ctx, installerPodName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
return false, 0, nil, nil, fmt.Errorf("deleting stuck installer pod %s/%s on NotReady node: %w", c.targetNamespace, installerPodName, err)
}
ret := &operatorv1.NodeStatus{
NodeName: ns.NodeName,
CurrentRevision: ns.CurrentRevision,
TargetRevision: 0,
LastFailedRevision: ns.TargetRevision,
LastFailedCount: ns.LastFailedCount + 1,
LastFallbackCount: ns.LastFallbackCount,
LastFailedReason: nodeStatusInstalledFailedReason,
LastFailedRevisionErrors: []string{
fmt.Sprintf("node %s is NotReady, clearing targetRevision to unblock healthy node rollout", ns.NodeName),
},
}
now := metav1.NewTime(c.clock.Now())
ret.LastFailedTime = &now
return false, 0, ret, nil, nil
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// start with node which is in worst state (instead of terminating healthy pods first)
startNode, nodeChoiceReason, err := nodeToStartRevisionWith(ctx, c.getStaticPodState, operatorStatus.NodeStatuses)
startNode, nodeChoiceReason, err := nodeToStartRevisionWith(ctx, c.getStaticPodState, operatorStatus.NodeStatuses, c.isNodeNotReadyForTooLong)
if err != nil {
return true, 0, nil, nil, err
}
Expand Down Expand Up @@ -872,6 +981,36 @@ func (c *InstallerController) newNodeStateForInstallInProgress(ctx context.Conte
return ret, true, fmt.Sprintf("installer pod failed: %v", strings.Join(errors, "\n")), nil

default:
// If the installer pod is stuck on a NotReady node beyond the timeout, fail it
// so the rollout can proceed on healthy nodes.
if c.isNodeNotReadyForTooLong(currNodeState.NodeName) {
now := c.clock.Now()
podAge := now.Sub(installerPod.CreationTimestamp.Time)
if podAge > installerPodStuckOnNotReadyNodeTimeout {
klog.Warningf("Installer pod %s/%s stuck for %v on NotReady node %s, marking as failed",
installerPod.Namespace, installerPod.Name, podAge.Round(time.Second), currNodeState.NodeName)
c.eventRecorder.Warningf("InstallerPodStuckOnNotReadyNode",
"Installer pod %s on node %s in %s phase for %v (node NotReady), treating as failed to unblock rollout",
installerPod.Name, currNodeState.NodeName, installerPod.Status.Phase, podAge.Round(time.Second))

if err := c.podsGetter.Pods(c.targetNamespace).Delete(ctx, installerPod.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
return nil, false, "", fmt.Errorf("deleting stuck installer pod %s/%s on NotReady node: %w", installerPod.Namespace, installerPod.Name, err)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

ret := deepCopyNodeStatusWithoutOldFailedState(currNodeState)
ret.LastFailedRevision = currNodeState.TargetRevision
ret.TargetRevision = 0
metaNow := metav1.NewTime(now)
ret.LastFailedTime = &metaNow
ret.LastFailedCount++
ret.LastFailedReason = nodeStatusInstalledFailedReason
ret.LastFailedRevisionErrors = []string{
fmt.Sprintf("installer pod stuck on NotReady node %s for %v", currNodeState.NodeName, podAge.Round(time.Second)),
}
return ret, true, fmt.Sprintf("installer pod stuck on NotReady node %s", currNodeState.NodeName), nil
}
}

if len(installerPod.Status.Message) > 0 {
return currNodeState, false, fmt.Sprintf("installer is not finished: %s", installerPod.Status.Message), nil
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/operator/staticpod/controller/installer/installer_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2276,7 +2276,7 @@ func TestNodeToStartRevisionWith(t *testing.T) {
}
return staticPodStatePending, "", "", nil, time.Now(), errors.NewNotFound(schema.GroupResource{Resource: "pods"}, nodeName)
}
i, _, err := nodeToStartRevisionWith(context.TODO(), fakeGetStaticPodState, test.nodes)
i, _, err := nodeToStartRevisionWith(context.TODO(), fakeGetStaticPodState, test.nodes, nil)
if err == nil && test.expectedErr {
t.Fatalf("expected error, got none")
}
Expand Down
Loading