diff --git a/api/v1alpha1/bootcnodepool_types.go b/api/v1alpha1/bootcnodepool_types.go index 1818307..66991df 100644 --- a/api/v1alpha1/bootcnodepool_types.go +++ b/api/v1alpha1/bootcnodepool_types.go @@ -40,6 +40,11 @@ const ( // affected nodes and their issues. PoolNodeDegraded string = "NodeDegraded" + // PoolRolloutHalted means the controller stopped assigning new + // reboot slots because 2 or more nodes in reboot slots are + // unhealthy (Degraded or not Ready). + PoolRolloutHalted string = "RolloutHalted" + // PoolInvalidSpec means the pool's spec contains invalid values that // the controller cannot process (e.g. an image ref that is not a // digest ref, or a malformed nodeSelector). diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index bd5c3b3..2a6271b 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -130,15 +130,16 @@ status: Pool conditions and their reasons: -| Condition | Status | Reason | Meaning | -|-----------|--------|-------------------|---------------------------------------------------| -| UpToDate | True | AllUpdated | All nodes are running targetDigest | -| UpToDate | False | RolloutInProgress | Nodes are actively being updated | -| UpToDate | False | Paused | Updates pending but pool is paused | -| Degraded | True | NodeConflict | Node selector overlaps with another pool | -| Degraded | True | NodeDegraded | At least one node has errors or isn't converging | -| Degraded | True | InvalidSpec | Pool spec contains invalid values | -| Degraded | False | Healthy | No issues | +| Condition | Status | Reason | Meaning | +|-----------|--------|-------------------|-----------------------------------------------------| +| UpToDate | True | AllUpdated | All nodes are running targetDigest | +| UpToDate | False | RolloutInProgress | Nodes are actively being updated | +| UpToDate | False | Paused | Updates pending but pool is paused | +| Degraded | True | NodeConflict | Node selector overlaps with another pool | +| Degraded | True | NodeDegraded | At least one node has errors or isn't converging | +| Degraded | True | RolloutHalted | 2+ unhealthy nodes in reboot slots; rollout stopped | +| Degraded | True | InvalidSpec | Pool spec contains invalid values | +| Degraded | False | Healthy | No issues | The `UpToDate` condition is determined by the controller by comparing `spec.desiredImage` vs `status.booted.imageDigest` across all nodes in the pool. diff --git a/internal/controller/rollout.go b/internal/controller/rollout.go index 6e41a7c..5c52082 100644 --- a/internal/controller/rollout.go +++ b/internal/controller/rollout.go @@ -59,6 +59,11 @@ func (r *BootcNodePoolReconciler) driveRollout(ctx context.Context, pool *bootcv rs := buildRolloutState(log, ownedBootcNodes) + // Flag degraded nodes at the pool level. NodeConflict (set during + // membership sync) takes priority, so we only set NodeDegraded if + // the pool isn't already degraded for another reason. + syncNodeDegradedCondition(pool, rs.degraded) + // Free reboot slots for nodes that have successfully rebooted into // the desired image. This runs before computing available slots so // that freed capacity is immediately usable for new candidates. @@ -66,13 +71,35 @@ func (r *BootcNodePoolReconciler) driveRollout(ctx context.Context, pool *bootcv return fmt.Errorf("freeing completed slots: %w", err) } + // Check for unhealthy nodes on the target digest in reboot slots. If + // 2+ are unhealthy, halt the rollout. Note that freeCompletedSlots() + // already removed Ready nodes, so any upToDate node still in a slot is + // implied to be NotReady. + unhealthy := findUnhealthySlots(rs, pool.Status.TargetDigest) + + // Hardcode to 2 for now; might make this configurable later, or + // dynamically adjusted. The rationale is that 1 unhealthy node might + // be a one-off. But 2 unhealthy nodes becomes a pattern that might + // indicate a bad image. Now, this _probably_ should scale with number + // of nodes somehow. E.g. a 500 node cluster could tolerate more + // unhealthy nodes. Just starting conservative here and we can adjust. + rolloutHalted := len(unhealthy) >= 2 + if rolloutHalted { + syncRolloutHaltedCondition(pool, unhealthy) + log.Info("Rollout halted: 2+ unhealthy nodes in reboot slots", + "unhealthyInSlots", len(unhealthy)) + } + maxUnavail, err := resolveMaxUnavailable(pool, rs.nodeCount()) if err != nil { return err } - avail := max(0, maxUnavail-rs.occupiedSlots) - candidates := selectDrainCandidates(rs.staged, avail) + availableSlots := 0 + if !rolloutHalted { + availableSlots = max(0, maxUnavail-rs.occupiedSlots) + } + candidates := selectDrainCandidates(rs.staged, availableSlots) log.V(1).Info("Rollout state", "upToDate", len(rs.upToDate), @@ -84,7 +111,7 @@ func (r *BootcNodePoolReconciler) driveRollout(ctx context.Context, pool *bootcv "unclassified", nodeNames(rs.unclassified), "occupiedSlots", rs.occupiedSlots, "maxUnavailable", maxUnavail, - "availableSlots", avail, + "availableSlots", availableSlots, "candidates", nodeNames(candidates), ) @@ -358,6 +385,86 @@ func buildRolloutState(log logr.Logger, ownedBootcNodes map[string]*bootcv1alpha return rs } +// syncNodeDegradedCondition sets Degraded/NodeDegraded on the pool if any +// nodes are degraded. It respects priority: if the pool is already degraded +// for another reason (e.g. NodeConflict), it does not overwrite it. +// XXX(jl): Or... should this be a new condition type so it can be surfaced in +// parallel? Let's see how this approach feels and iterate. +func syncNodeDegradedCondition(pool *bootcv1alpha1.BootcNodePool, degraded []*bootcv1alpha1.BootcNode) { + if len(degraded) == 0 { + return + } + if apimeta.IsStatusConditionTrue(pool.Status.Conditions, bootcv1alpha1.PoolDegraded) { + return + } + names := make([]string, len(degraded)) + for i, bn := range degraded { + names[i] = bn.Name + } + // Sort so the message is stable across reconciles. + slices.Sort(names) + apimeta.SetStatusCondition(&pool.Status.Conditions, metav1.Condition{ + Type: bootcv1alpha1.PoolDegraded, + Status: metav1.ConditionTrue, + Reason: bootcv1alpha1.PoolNodeDegraded, + Message: fmt.Sprintf("Degraded nodes: %s", strings.Join(names, ", ")), + }) +} + +// unhealthySlot identifies a node in a reboot slot that is unhealthy. +type unhealthySlot struct { + name string + reason string // "Degraded" or "NotReady" +} + +// findUnhealthySlots returns nodes occupying reboot slots that are unhealthy. +// Two categories qualify: +// - Degraded nodes (marked by the daemon itself) that booted the target +// digest (the image itself may be bad). +// - UpToDate nodes still holding a slot after freeCompletedSlots (not +// Ready). I.e. the daemon doesn't see anything wrong, but there's something +// wrong preventing the kubelet from working correctly. +func findUnhealthySlots(rs *rolloutState, targetDigest string) []unhealthySlot { + var result []unhealthySlot + for _, bn := range rs.degraded { + if !metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) { + continue + } + // Note we count nil Booted as unhealthy here; this really + // shouldn't happen because clearly the daemon came up at least + // once in this node's history to be able to get to a reboot + // slot. And so Booted should always be set here. + if bn.Status.Booted == nil || bn.Status.Booted.ImageDigest == targetDigest { + result = append(result, unhealthySlot{name: bn.Name, reason: "Degraded"}) + } + } + for _, bn := range rs.upToDate { + if metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) { + // Still has annotation after freeCompletedSlots → not Ready. + result = append(result, unhealthySlot{name: bn.Name, reason: "NotReady"}) + } + } + return result +} + +// syncRolloutHaltedCondition sets Degraded/RolloutHalted on the pool. It +// implicitly takes priority over NodeDegraded (which checks for existing +// daemon-driven degraded status) by running later than +// syncNodeDegradedCondition. +func syncRolloutHaltedCondition(pool *bootcv1alpha1.BootcNodePool, unhealthy []unhealthySlot) { + details := make([]string, len(unhealthy)) + for i, u := range unhealthy { + details[i] = u.name + ": " + u.reason + } + slices.Sort(details) + apimeta.SetStatusCondition(&pool.Status.Conditions, metav1.Condition{ + Type: bootcv1alpha1.PoolDegraded, + Status: metav1.ConditionTrue, + Reason: bootcv1alpha1.PoolRolloutHalted, + Message: fmt.Sprintf("Rollout halted: 2+ unhealthy nodes in reboot slots (%s)", strings.Join(details, ", ")), + }) +} + // resolveMaxUnavailable computes the effective maxUnavailable value from the // pool's rollout spec. Defaults to 1 when unset. A value of 0 is allowed and // means no reboot slots are available (effectively paused). Returns an diff --git a/internal/controller/rollout_envtest_test.go b/internal/controller/rollout_envtest_test.go index 17b53f6..7df9766 100644 --- a/internal/controller/rollout_envtest_test.go +++ b/internal/controller/rollout_envtest_test.go @@ -130,6 +130,206 @@ func TestSimpleRollout(t *testing.T) { } } +// TestDegradedNodeSetsPoolCondition verifies that when a daemon reports +// Degraded=True on a BootcNode, the pool is marked Degraded/NodeDegraded, +// and the rollout continues on non-degraded nodes. +func TestDegradedNodeSetsPoolCondition(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + const poolName = "degraded-pool" + + // Create 2 worker nodes. + nodeNames := []string{"degraded-w1", "degraded-w2"} + for _, name := range nodeNames { + name := name + node := testutil.NewK8sNode(name, testutil.WorkerLabels()) + g.Expect(k8sClient.Create(ctx, node)).To(Succeed()) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, node) + }) + } + + // Create pool targeting digest B with maxUnavailable: 1. + pool := testutil.NewPool(poolName, testImageDigestRefB, + testutil.WithWorkerSelector(), + testutil.WithMaxUnavailable(intstr.FromInt32(1)), + ) + g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, pool) + }) + + // Wait for BootcNodes to be created. + for _, name := range nodeNames { + name := name + g.Eventually(func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: name}, &bootcv1alpha1.BootcNode{}) + }).Should(Succeed()) + } + + // Simulate w1 as degraded (staging failed) and w2 as staged. + simulateDaemonDegraded(g, ctx, "degraded-w1", testDigestA) + simulateDaemonStatus(g, ctx, "degraded-w2", testDigestA, bootcv1alpha1.NodeReasonStaged) + + // Verify pool is Degraded/NodeDegraded mentioning w1. + g.Eventually(func(g Gomega) { + var p bootcv1alpha1.BootcNodePool + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: poolName}, &p)).To(Succeed()) + g.Expect(p.Status.Conditions).To(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolDegraded), + HaveField("Status", metav1.ConditionTrue), + HaveField("Reason", bootcv1alpha1.PoolNodeDegraded), + HaveField("Message", ContainSubstring("degraded-w1")), + ))) + }).Should(Succeed()) + + // Verify rollout continues on the non-degraded node: w2 should get + // a reboot slot despite w1 being degraded. + g.Eventually(func(g Gomega) { + var bn bootcv1alpha1.BootcNode + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: "degraded-w2"}, &bn)).To(Succeed()) + g.Expect(bn.Annotations).To(HaveKey(bootcv1alpha1.AnnotationInRebootSlot), + "non-degraded node should get a reboot slot") + }).Should(Succeed()) +} + +// TestUnhealthyNodesHaltRollout verifies that when 2+ nodes in reboot slots +// are unhealthy, the controller stops assigning new slots and sets +// Degraded/RolloutHalted on the pool. It also verifies recovery: when +// unhealthy nodes are fixed, the rollout resumes. +func TestUnhealthyNodesHaltRollout(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + const poolName = "halt-pool" + + // Create 4 worker nodes. + nodeNames := []string{"halt-w1", "halt-w2", "halt-w3", "halt-w4"} + for _, name := range nodeNames { + name := name + node := testutil.NewK8sNode(name, testutil.WorkerLabels()) + g.Expect(k8sClient.Create(ctx, node)).To(Succeed()) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, node) + }) + } + + // Create pool targeting digest B with maxUnavailable: 3. + pool := testutil.NewPool(poolName, testImageDigestRefB, + testutil.WithWorkerSelector(), + testutil.WithMaxUnavailable(intstr.FromInt32(3)), + ) + g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, pool) + }) + + // Wait for BootcNodes to be created. + for _, name := range nodeNames { + name := name + g.Eventually(func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: name}, &bootcv1alpha1.BootcNode{}) + }).Should(Succeed()) + } + + // Simulate all 4 nodes as Staged (booted old image, staged new one). + for _, name := range nodeNames { + simulateDaemonStatus(g, ctx, name, testDigestA, bootcv1alpha1.NodeReasonStaged) + } + + // With maxUnavailable: 3, the first 3 nodes (alphabetical) should get + // reboot slots. Wait for w1, w2, w3 to get slots. + for _, name := range nodeNames[:3] { + name := name + g.Eventually(func(g Gomega) { + var bn bootcv1alpha1.BootcNode + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: name}, &bn)).To(Succeed()) + g.Expect(bn.Annotations).To(HaveKey(bootcv1alpha1.AnnotationInRebootSlot), + "node %s should have reboot slot", name) + g.Expect(bn.Spec.DesiredImageState).To(Equal(bootcv1alpha1.DesiredImageStateBooted), + "node %s should have desiredImageState Booted", name) + }).Should(Succeed()) + } + + // w4 should not have a slot (all 3 slots are occupied). + var bn4 bootcv1alpha1.BootcNode + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: "halt-w4"}, &bn4)).To(Succeed()) + g.Expect(bn4.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationInRebootSlot), + "w4 should not have a slot yet") + + // Simulate w1 as degraded (booted the target digest and Ready but daemon says it's bad). + simulateDaemonDegraded(g, ctx, "halt-w1", testDigestB) + setNodeReady(g, ctx, "halt-w1") + + // Simulate w2 as upToDate but not Ready (booted target, node not Ready). + simulateDaemonStatus(g, ctx, "halt-w2", testDigestB, bootcv1alpha1.NodeReasonIdle) + // Note: we do NOT call setNodeReady for w2, so it stays not Ready. + + // Simulate w3 as having successfully completed (booted target, Idle, Ready). + simulateDaemonStatus(g, ctx, "halt-w3", testDigestB, bootcv1alpha1.NodeReasonIdle) + setNodeReady(g, ctx, "halt-w3") + + // Wait for pool to be Degraded/RolloutHalted. + g.Eventually(func(g Gomega) { + var p bootcv1alpha1.BootcNodePool + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: poolName}, &p)).To(Succeed()) + g.Expect(p.Status.Conditions).To(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolDegraded), + HaveField("Status", metav1.ConditionTrue), + HaveField("Reason", bootcv1alpha1.PoolRolloutHalted), + HaveField("Message", ContainSubstring("halt-w1")), + HaveField("Message", ContainSubstring("halt-w2")), + ))) + }).Should(Succeed()) + + // w3's slot should be freed (it's healthy and Ready), but w4 should + // still not get a slot because the rollout is halted. + g.Eventually(func(g Gomega) { + var bn3 bootcv1alpha1.BootcNode + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: "halt-w3"}, &bn3)).To(Succeed()) + g.Expect(bn3.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationInRebootSlot), + "w3 slot should be freed") + }).Should(Succeed()) + + g.Consistently(func(g Gomega) { + var bn bootcv1alpha1.BootcNode + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: "halt-w4"}, &bn)).To(Succeed()) + g.Expect(bn.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationInRebootSlot), + "w4 should not get a slot while rollout is halted") + }, "2s", pollInterval).Should(Succeed()) + + // Fix w1: clear degraded, report as booted on target and Idle. + simulateDaemonStatus(g, ctx, "halt-w1", testDigestB, bootcv1alpha1.NodeReasonIdle) + setNodeReady(g, ctx, "halt-w1") + + // Fix w2: set node Ready. + setNodeReady(g, ctx, "halt-w2") + + // Rollout should resume: w4 should now get a slot. + g.Eventually(func(g Gomega) { + var bn bootcv1alpha1.BootcNode + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: "halt-w4"}, &bn)).To(Succeed()) + g.Expect(bn.Annotations).To(HaveKey(bootcv1alpha1.AnnotationInRebootSlot), + "w4 should get a slot after rollout resumes") + }).Should(Succeed()) + + // Pool should no longer be Degraded/RolloutHalted. + g.Eventually(func(g Gomega) { + var p bootcv1alpha1.BootcNodePool + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: poolName}, &p)).To(Succeed()) + g.Expect(p.Status.Conditions).To(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolDegraded), + HaveField("Status", metav1.ConditionFalse), + HaveField("Reason", bootcv1alpha1.PoolHealthy), + ))) + }).Should(Succeed()) +} + // simulateDaemonStatus writes BootcNode status as if the daemon had // reported the given booted digest and Idle condition reason. func simulateDaemonStatus(g Gomega, ctx context.Context, nodeName, bootedDigest, idleReason string) { @@ -157,6 +357,35 @@ func simulateDaemonStatus(g Gomega, ctx context.Context, nodeName, bootedDigest, g.Expect(k8sClient.Status().Update(ctx, &bn)).To(Succeed()) } +// simulateDaemonDegraded writes BootcNode status as if the daemon had +// reported the given booted digest with Degraded=True (e.g. staging failed). +func simulateDaemonDegraded(g Gomega, ctx context.Context, nodeName, bootedDigest string) { + var bn bootcv1alpha1.BootcNode + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &bn)).To(Succeed()) + + bn.Status.Booted = &bootcv1alpha1.ImageInfo{ + Image: "quay.io/example/myos@" + bootedDigest, + ImageDigest: bootedDigest, + } + bn.Status.Conditions = []metav1.Condition{ + { + Type: bootcv1alpha1.NodeIdle, + Status: metav1.ConditionFalse, + Reason: bootcv1alpha1.NodeReasonStaging, + LastTransitionTime: metav1.Now(), + }, + { + Type: bootcv1alpha1.NodeDegraded, + Status: metav1.ConditionTrue, + Reason: bootcv1alpha1.NodeReasonError, + Message: "simulated staging failure", + LastTransitionTime: metav1.Now(), + }, + } + + g.Expect(k8sClient.Status().Update(ctx, &bn)).To(Succeed()) +} + // setNodeReady sets the Ready condition on a K8s Node to True. In // envtest there is no kubelet, so this simulates the node becoming // healthy after a reboot. @@ -164,19 +393,21 @@ func setNodeReady(g Gomega, ctx context.Context, nodeName string) { var node corev1.Node g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &node)).To(Succeed()) + modified := node.DeepCopy() + // Replace any existing Ready condition, preserving other conditions. var filtered []corev1.NodeCondition - for _, c := range node.Status.Conditions { + for _, c := range modified.Status.Conditions { if c.Type != corev1.NodeReady { filtered = append(filtered, c) } } - node.Status.Conditions = append(filtered, corev1.NodeCondition{ + modified.Status.Conditions = append(filtered, corev1.NodeCondition{ Type: corev1.NodeReady, Status: corev1.ConditionTrue, LastHeartbeatTime: metav1.Now(), LastTransitionTime: metav1.Now(), }) - g.Expect(k8sClient.Status().Update(ctx, &node)).To(Succeed()) + g.Expect(k8sClient.Status().Patch(ctx, modified, client.MergeFrom(&node))).To(Succeed()) }