diff --git a/pkg/operator/staticpod/controller/installer/installer_controller.go b/pkg/operator/staticpod/controller/installer/installer_controller.go index 80d0265a8d..e240983d03 100644 --- a/pkg/operator/staticpod/controller/installer/installer_controller.go +++ b/pkg/operator/staticpod/controller/installer/installer_controller.go @@ -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" @@ -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 + + // 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" @@ -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 @@ -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 @@ -322,11 +351,44 @@ 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") } @@ -334,6 +396,9 @@ func nodeToStartRevisionWith(ctx context.Context, getStaticPodStateFn staticPodS // 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 } @@ -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 @@ -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 @@ -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 @@ -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 } @@ -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 + } + } + // 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 } @@ -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) + } + + 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 } diff --git a/pkg/operator/staticpod/controller/installer/installer_controller_test.go b/pkg/operator/staticpod/controller/installer/installer_controller_test.go index 3b18e46e13..7ba814ee0f 100644 --- a/pkg/operator/staticpod/controller/installer/installer_controller_test.go +++ b/pkg/operator/staticpod/controller/installer/installer_controller_test.go @@ -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") } diff --git a/pkg/operator/staticpod/controller/installer/installer_notready_test.go b/pkg/operator/staticpod/controller/installer/installer_notready_test.go new file mode 100644 index 0000000000..5c628a0fae --- /dev/null +++ b/pkg/operator/staticpod/controller/installer/installer_notready_test.go @@ -0,0 +1,232 @@ +package installer + +import ( + "context" + "fmt" + "strings" + "testing" + "time" + + operatorv1 "github.com/openshift/api/operator/v1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/client-go/tools/cache" + clocktesting "k8s.io/utils/clock/testing" +) + +func TestNodeToStartRevisionWith_SkipsNotReadyNodes(t *testing.T) { + now := time.Date(2026, 6, 26, 12, 0, 0, 0, time.UTC) + + fakeGetStaticPodState := func(ctx context.Context, nodeName string) (staticPodState, string, string, []string, time.Time, error) { + switch nodeName { + case "master-0": + return staticPodStateReady, "5", "", nil, now, nil + case "master-1": + return staticPodStatePending, "4", "", nil, now, nil + case "master-2": + return staticPodStatePending, "3", "", nil, now, nil + default: + return staticPodStatePending, "1", "", nil, now, nil + } + } + + nodes := []operatorv1.NodeStatus{ + {NodeName: "master-0", CurrentRevision: 5}, + {NodeName: "master-1", CurrentRevision: 4}, + {NodeName: "master-2", CurrentRevision: 3}, + } + + tests := []struct { + name string + notReadyNodes map[string]bool + expectedNodeIndex int + expectedContains string + noReadinessCheckFn bool + }{ + { + name: "without readiness check, picks oldest not-ready (master-2)", + notReadyNodes: nil, + expectedNodeIndex: 2, + noReadinessCheckFn: true, + }, + { + name: "with readiness check, all ready, still picks oldest not-ready (master-2)", + notReadyNodes: map[string]bool{}, + expectedNodeIndex: 2, + }, + { + name: "master-2 is NotReady, picks master-1 instead", + notReadyNodes: map[string]bool{"master-2": true}, + expectedNodeIndex: 1, + }, + { + name: "both master-1 and master-2 NotReady, picks master-0 (latest revision)", + notReadyNodes: map[string]bool{"master-1": true, "master-2": true}, + expectedNodeIndex: 0, + expectedContains: "is the oldest", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var isNotReady func(string) bool + if !tt.noReadinessCheckFn { + isNotReady = func(nodeName string) bool { + return tt.notReadyNodes[nodeName] + } + } + + idx, reason, err := nodeToStartRevisionWith(context.TODO(), fakeGetStaticPodState, nodes, isNotReady) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if idx != tt.expectedNodeIndex { + t.Errorf("expected node index %d, got %d (reason: %s)", tt.expectedNodeIndex, idx, reason) + } + if tt.expectedContains != "" && !strings.Contains(reason, tt.expectedContains) { + t.Errorf("expected reason to contain %q, got %q", tt.expectedContains, reason) + } + }) + } +} + +func TestIsNodeNotReadyForTooLong(t *testing.T) { + now := time.Date(2026, 6, 26, 12, 0, 0, 0, time.UTC) + fakeClock := clocktesting.NewFakeClock(now) + + tests := []struct { + name string + node *corev1.Node + expected bool + }{ + { + name: "node is Ready", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "master-0"}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + LastTransitionTime: metav1.NewTime(now.Add(-1 * time.Hour)), + }, + }, + }, + }, + expected: false, + }, + { + name: "node NotReady for 5 minutes (below threshold)", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "master-1"}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionFalse, + LastTransitionTime: metav1.NewTime(now.Add(-5 * time.Minute)), + }, + }, + }, + }, + expected: false, + }, + { + name: "node NotReady for 15 minutes (above threshold)", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "master-2"}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionFalse, + LastTransitionTime: metav1.NewTime(now.Add(-15 * time.Minute)), + }, + }, + }, + }, + expected: true, + }, + { + name: "node NotReady with Unknown status for 20 minutes", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "master-3"}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionUnknown, + LastTransitionTime: metav1.NewTime(now.Add(-20 * time.Minute)), + }, + }, + }, + }, + expected: true, + }, + { + name: "node with no NodeReady condition", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "master-4"}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{}, + }, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + if err := indexer.Add(tt.node); err != nil { + t.Fatal(err) + } + nodeLister := &fakeNodeLister{indexer: indexer} + + c := &InstallerController{ + nodeLister: nodeLister, + clock: fakeClock, + } + + result := c.isNodeNotReadyForTooLong(tt.node.Name) + if result != tt.expected { + t.Errorf("expected %v, got %v for node %s", tt.expected, result, tt.node.Name) + } + }) + } +} + +func TestIsNodeNotReadyForTooLong_NilLister(t *testing.T) { + c := &InstallerController{ + nodeLister: nil, + } + if c.isNodeNotReadyForTooLong("any-node") { + t.Error("expected false when nodeLister is nil") + } +} + +type fakeNodeLister struct { + indexer cache.Indexer +} + +func (f *fakeNodeLister) List(selector labels.Selector) ([]*corev1.Node, error) { + objs := f.indexer.List() + nodes := make([]*corev1.Node, 0, len(objs)) + for _, obj := range objs { + nodes = append(nodes, obj.(*corev1.Node)) + } + return nodes, nil +} + +func (f *fakeNodeLister) Get(name string) (*corev1.Node, error) { + obj, exists, err := f.indexer.GetByKey(name) + if err != nil { + return nil, err + } + if !exists { + return nil, fmt.Errorf("node %q not found", name) + } + return obj.(*corev1.Node), nil +} diff --git a/pkg/operator/staticpod/controllers.go b/pkg/operator/staticpod/controllers.go index 62722c66c3..b5702b8d7f 100644 --- a/pkg/operator/staticpod/controllers.go +++ b/pkg/operator/staticpod/controllers.go @@ -275,7 +275,7 @@ func (b *staticPodOperatorControllerBuilder) ToControllers() (manager.Controller } if len(b.installCommand) > 0 { - manager.WithController(installer.NewInstallerController( + installerCtrl := installer.NewInstallerController( b.operandName, b.operandNamespace, b.staticPodName, @@ -296,7 +296,14 @@ func (b *staticPodOperatorControllerBuilder) ToControllers() (manager.Controller b.installerPodMutationFunc, ).WithMinReadyDuration( b.minReadyDuration, - ), 1) + ) + if clusterInformers != nil && b.revisionControllerPrecondition == nil { + installerCtrl = installerCtrl.WithNodeLister( + clusterInformers.Core().V1().Nodes().Lister(), + clusterInformers.Core().V1().Nodes().Informer(), + ) + } + manager.WithController(installerCtrl, 1) manager.WithController(installerstate.NewInstallerStateController( b.operandName,