diff --git a/api/v1alpha1/constants.go b/api/v1alpha1/constants.go index d3d3338c..1c6e4879 100644 --- a/api/v1alpha1/constants.go +++ b/api/v1alpha1/constants.go @@ -30,4 +30,20 @@ const ( // state for which the controller considered emitting an event. It is // observability bookkeeping only and is never used to drive reconciliation. AnnotationLastObservedState = "bootc.dev/last-observed-state" + + // AnnotationOwnsKarpenterDoNotRepair is set on a BootcNode to record that + // the controller added the karpenter.sh/do-not-repair annotation to the K8s + // Node during slot assignment. When present, the controller removes the + // karpenter.sh/do-not-repair annotation from the Node when the slot is freed. + // It is not set when the annotation was already present before slot assignment + // (externally set), so the controller never removes an annotation it did not add. + AnnotationOwnsKarpenterDoNotRepair = "bootc.dev/owns-karpenter-do-not-repair" + + // AnnotationOwnsKarpenterDoNotDisrupt is set on a BootcNode to record that + // the controller added the karpenter.sh/do-not-disrupt annotation to the K8s + // Node during slot assignment. When present, the controller removes the + // karpenter.sh/do-not-disrupt annotation from the Node when the slot is freed. + // It is not set when the annotation was already present before slot assignment + // (externally set), so the controller never removes an annotation it did not add. + AnnotationOwnsKarpenterDoNotDisrupt = "bootc.dev/owns-karpenter-do-not-disrupt" ) diff --git a/docs/src/SUMMARY.md b/docs/src/SUMMARY.md index fbfaf433..d411f43b 100644 --- a/docs/src/SUMMARY.md +++ b/docs/src/SUMMARY.md @@ -8,3 +8,4 @@ - [Helm Chart](helm.md) - [Operations](operations/index.md) - [Managing a pool](operations/pool.md) + - [Karpenter integration](operations/karpenter.md) diff --git a/docs/src/operations/karpenter.md b/docs/src/operations/karpenter.md new file mode 100644 index 00000000..990b0445 --- /dev/null +++ b/docs/src/operations/karpenter.md @@ -0,0 +1,97 @@ +# Karpenter integration + +bootc-operator is aware of [Karpenter](https://karpenter.sh/) and coordinates +with it during node updates to prevent unnecessary node replacement or +voluntary disruption. + +## Why this matters + +Karpenter monitors nodes for health and may decide to replace or disrupt a +node that appears unhealthy or underutilized — for example, one that is +cordoned, being drained, or not yet Ready after a reboot. All of those +conditions arise naturally during a bootc-operator update window, so without +coordination Karpenter could race with the operator and terminate a node that +is in the middle of a legitimate OS update. + +Karpenter respects two node-level annotations: + +- `karpenter.sh/do-not-repair: "true"` — prevents Node Auto Repair from + replacing a node that has unhealthy conditions. +- `karpenter.sh/do-not-disrupt: "true"` — prevents voluntary disruption + actions (consolidation, drift) from selecting this node. + +> **Note:** `karpenter.sh/do-not-repair` was introduced in Karpenter v1.15.0. +> That is the minimum required version for full bootc-operator Karpenter integration. + +bootc-operator sets both annotations automatically on Karpenter-managed nodes +for the duration of the reboot slot. + +## How it works + +When the controller assigns a reboot slot to a node it checks whether the K8s +Node object has an owner reference pointing to a Karpenter `NodeClaim` +(`apiVersion: karpenter.sh/v1`, `kind: NodeClaim`). If it does, the +controller takes the following steps: + +1. For each annotation (`do-not-repair`, `do-not-disrupt`): if the annotation + is **not** already present on the Node, the controller adds it (value + `"true"`) and records ownership by setting the corresponding tracking + annotation on the `BootcNode` object: + - `bootc.dev/owns-karpenter-do-not-repair: "true"` + - `bootc.dev/owns-karpenter-do-not-disrupt: "true"` +2. The node proceeds through the normal update flow: drain, reboot, image + verification. +3. When the slot is freed (node is Ready and running the new image), the + controller checks the tracking annotations on the `BootcNode`. For each + annotation it owns, it removes the corresponding annotation from the Node + and clears the tracking annotation from the `BootcNode`. Both removals are + issued as a single Node patch. + +## Pre-existing annotations + +If either Karpenter annotation is already on the Node before slot assignment — +set by an external actor, another controller, or a manual operation — +bootc-operator leaves it completely alone: + +- It does **not** overwrite or change the annotation on assignment. +- It does **not** set the corresponding tracking annotation on the `BootcNode`. +- It does **not** remove the annotation when the slot is freed. + +The two annotations are tracked independently. If only one is pre-existing, the +operator owns (and later removes) only the other. + +This ensures bootc-operator never removes an annotation it did not add. + +## Limitations + +`karpenter.sh/do-not-disrupt` only blocks voluntary disruption methods +(consolidation and drift). It does not protect a node against forceful +termination triggered by expiration (`expireAfter`), interruption (spot +instance reclaim), Node Auto Repair, or manual deletion. If any of those +events occur while a node is in a bootc-operator reboot slot, Karpenter may +still terminate it. + +`karpenter.sh/do-not-repair` similarly has no effect on voluntary disruption +or expiration — it only suppresses Node Auto Repair. + +If a node is terminated by expiration, interruption, or any other forceful +method while a bootc-operator update is in progress, the termination is caused +by Karpenter acting on its own policy — not by the bootc-operator update. The +update simply happened to be in flight at the time. + +Refer to the [Karpenter disruption documentation](https://karpenter.sh/docs/concepts/disruption/) +for a full breakdown of which controls apply to which disruption methods. + +## Non-Karpenter nodes + +Nodes without a `NodeClaim` owner reference are unaffected. The feature +activates only when the owner reference check matches. + +## Annotation reference + +| Annotation | Object | Managed by | Purpose | +|---|---|---|---| +| `karpenter.sh/do-not-repair` | K8s Node | bootc-operator (or external) | Prevents Karpenter Node Auto Repair from replacing this node during an update | +| `karpenter.sh/do-not-disrupt` | K8s Node | bootc-operator (or external) | Prevents Karpenter voluntary disruption (consolidation, drift) during an update | +| `bootc.dev/owns-karpenter-do-not-repair` | BootcNode | bootc-operator | Tracks that the operator added `do-not-repair`; cleared when the annotation is removed on slot free | +| `bootc.dev/owns-karpenter-do-not-disrupt` | BootcNode | bootc-operator | Tracks that the operator added `do-not-disrupt`; cleared when the annotation is removed on slot free | diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index 787e654b..3eeff914 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -859,8 +859,8 @@ func (r *BootcNodePoolReconciler) removeBootcNode( if err := r.Get(ctx, types.NamespacedName{Name: bn.Name}, &node); err == nil { // No point in cleaning up the Node object if it's going away anyway... if node.DeletionTimestamp == nil { - if err := r.restoreCordonState(ctx, bn, &node); err != nil { - return fmt.Errorf("restoring cordon state: %w", err) + if err := r.restoreNodeState(ctx, bn, &node); err != nil { + return fmt.Errorf("restoring node state: %w", err) } if err := r.ensureManagedLabel(ctx, &node, false); err != nil { return fmt.Errorf("removing managed label: %w", err) @@ -877,21 +877,43 @@ func (r *BootcNodePoolReconciler) removeBootcNode( return nil } -// restoreCordonState uncordons the Node if the BootcNode's was-cordoned -// annotation indicates the operator cordoned it. If the annotation is absent -// or "true" (node was already cordoned before us), this is a no-op. -func (r *BootcNodePoolReconciler) restoreCordonState( +// restoreNodeState removes any Karpenter annotations the controller owns and +// restores the node's cordon state, combining both into a single patch when +// needed. Karpenter annotation ownership is tracked via BootcNode annotations +// rather than inspecting the Node directly, so an external actor removing them +// doesn't cause spurious patches. The cordon is only removed if the was-cordoned +// annotation records that the operator was the one who cordoned it. +func (r *BootcNodePoolReconciler) restoreNodeState( ctx context.Context, bn *bootcv1alpha1.BootcNode, node *corev1.Node, ) error { - if bn.Annotations[bootcv1alpha1.AnnotationWasCordoned] != "false" { + log := logf.FromContext(ctx) + modified := node.DeepCopy() + changed := false + + if metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair) { + log.Info("Removing karpenter.sh/do-not-repair from node", "node", node.Name) + delete(modified.Annotations, karpenterDoNotRepairAnnotationKey) + changed = true + } + if metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt) { + log.Info("Removing karpenter.sh/do-not-disrupt from node", "node", node.Name) + delete(modified.Annotations, karpenterDoNotDisruptAnnotationKey) + changed = true + } + if metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationWasCordoned) && + bn.Annotations[bootcv1alpha1.AnnotationWasCordoned] == "false" { + log.Info("Uncordoning node", "node", node.Name) + modified.Spec.Unschedulable = false + changed = true + } + + if !changed { return nil } - modified := node.DeepCopy() - modified.Spec.Unschedulable = false if err := r.Patch(ctx, modified, client.StrategicMergeFrom(node)); err != nil { - return fmt.Errorf("uncordoning node: %w", err) + return fmt.Errorf("restoring node state: %w", err) } *node = *modified return nil diff --git a/internal/controller/rollout.go b/internal/controller/rollout.go index f1c3d069..45af54bb 100644 --- a/internal/controller/rollout.go +++ b/internal/controller/rollout.go @@ -34,6 +34,18 @@ import ( // https://github.com/bootc-dev/bootc-operator/issues/99. const unhealthySlotHaltThreshold = 2 +// Karpenter integration constants. The controller checks for a NodeClaim owner +// reference and manages karpenter.sh/do-not-repair and +// karpenter.sh/do-not-disrupt to prevent Karpenter from replacing or +// voluntarily disrupting a node while it is being updated. +// See docs/src/operations/karpenter.md. +const ( + karpenterDoNotRepairAnnotationKey = "karpenter.sh/do-not-repair" + karpenterDoNotDisruptAnnotationKey = "karpenter.sh/do-not-disrupt" + karpenterNodeClaimAPIVersion = "karpenter.sh/v1" + karpenterNodeClaimKind = "NodeClaim" +) + // rolloutState holds the classified BootcNodes for a single reconcile // pass. type rolloutState struct { @@ -188,11 +200,26 @@ func (r *BootcNodePoolReconciler) driveRollout( return rs, nil } -// assignRebootSlot marks a BootcNode as occupying a reboot slot and -// cordons the corresponding K8s Node. It sets the in-reboot-slot -// annotation on the BootcNode, records prior cordon state in the -// was-cordoned annotation, and cordons the node. All operations are -// idempotent. +// nodeIsOwnedByKarpenterNodeClaim reports whether the K8s Node has an owner +// reference pointing to a Karpenter NodeClaim (karpenter.sh/v1). +func nodeIsOwnedByKarpenterNodeClaim(node *corev1.Node) bool { + for _, ref := range node.OwnerReferences { + if ref.APIVersion == karpenterNodeClaimAPIVersion && ref.Kind == karpenterNodeClaimKind { + return true + } + } + return false +} + +// assignRebootSlot marks a BootcNode as occupying a reboot slot and cordons +// the corresponding K8s Node. It sets the in-reboot-slot annotation on the +// BootcNode and records the prior cordon state in the was-cordoned annotation. +// For nodes owned by a Karpenter NodeClaim, it also sets the +// karpenter.sh/do-not-repair and karpenter.sh/do-not-disrupt annotations on +// the K8s Node to prevent Karpenter from replacing or disrupting the node +// during the update. Ownership of those annotations is tracked on the BootcNode +// so that restoreNodeState can remove only the ones the controller added. +// All operations are idempotent. func (r *BootcNodePoolReconciler) assignRebootSlot( ctx context.Context, bn *bootcv1alpha1.BootcNode, @@ -200,35 +227,99 @@ func (r *BootcNodePoolReconciler) assignRebootSlot( ) error { log := logf.FromContext(ctx) - // Set annotations on the BootcNode if not already present. + // Determine Karpenter intent once so both patch blocks stay consistent. + // shouldOwn* is true when the node is Karpenter-managed and the respective + // annotation is not already present (set by an external actor). In that + // case, the controller adds the annotation and records ownership so it can + // be cleaned up when the slot is freed. + karpenterOwned := nodeIsOwnedByKarpenterNodeClaim(node) + shouldOwnDoNotRepair := karpenterOwned && + !metav1.HasAnnotation(node.ObjectMeta, karpenterDoNotRepairAnnotationKey) + shouldOwnDoNotDisrupt := karpenterOwned && + !metav1.HasAnnotation(node.ObjectMeta, karpenterDoNotDisruptAnnotationKey) + + // Patch the BootcNode with slot annotations if not already present. if !metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) { log.Info("Assigning reboot slot", "node", bn.Name) modifiedBN := bn.DeepCopy() - if modifiedBN.Annotations == nil { - modifiedBN.Annotations = map[string]string{} - } - modifiedBN.Annotations[bootcv1alpha1.AnnotationInRebootSlot] = "" + + metav1.SetMetaDataAnnotation( + &modifiedBN.ObjectMeta, + bootcv1alpha1.AnnotationInRebootSlot, + "", + ) + // Record whether the node was already cordoned before us. if node.Spec.Unschedulable { - modifiedBN.Annotations[bootcv1alpha1.AnnotationWasCordoned] = "true" + metav1.SetMetaDataAnnotation( + &modifiedBN.ObjectMeta, + bootcv1alpha1.AnnotationWasCordoned, + "true", + ) } else { - modifiedBN.Annotations[bootcv1alpha1.AnnotationWasCordoned] = "false" + metav1.SetMetaDataAnnotation( + &modifiedBN.ObjectMeta, + bootcv1alpha1.AnnotationWasCordoned, + "false", + ) + } + // Track Karpenter annotation ownership so freeRebootSlot knows + // whether to remove the annotations from the Node. + if shouldOwnDoNotRepair { + metav1.SetMetaDataAnnotation( + &modifiedBN.ObjectMeta, + bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair, + "true", + ) + } + if shouldOwnDoNotDisrupt { + metav1.SetMetaDataAnnotation( + &modifiedBN.ObjectMeta, + bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt, + "true", + ) } if err := r.Patch(ctx, modifiedBN, client.MergeFrom(bn)); err != nil { return fmt.Errorf("annotating BootcNode: %w", err) } - *bn = *modifiedBN } - // Cordon the K8s Node if not already cordoned. + // If the node is cordoned and we do not own either of the Karpenter + // annotations, there is nothing for to do. + if node.Spec.Unschedulable && !shouldOwnDoNotRepair && !shouldOwnDoNotDisrupt { + return nil + } + + // Cordon the K8s node and set the appropriate annotations. All operations + // are idempotent and are combined into a single API call. + modifiedNode := node.DeepCopy() if !node.Spec.Unschedulable { log.Info("Cordoning node", "node", node.Name) - modifiedNode := node.DeepCopy() modifiedNode.Spec.Unschedulable = true - if err := r.Patch(ctx, modifiedNode, client.StrategicMergeFrom(node)); err != nil { - return fmt.Errorf("cordoning node: %w", err) - } - *node = *modifiedNode + } + + // If we own the do-not-repair annotation, set it now. + if shouldOwnDoNotRepair { + log.Info("Setting karpenter.sh/do-not-repair on node", "node", node.Name) + metav1.SetMetaDataAnnotation( + &modifiedNode.ObjectMeta, + karpenterDoNotRepairAnnotationKey, + "true", + ) + } + + // If we own the do-not-disrupt annotation, set it now. + if shouldOwnDoNotDisrupt { + log.Info("Setting karpenter.sh/do-not-disrupt on node", "node", node.Name) + metav1.SetMetaDataAnnotation( + &modifiedNode.ObjectMeta, + karpenterDoNotDisruptAnnotationKey, + "true", + ) + } + + if err := r.Patch(ctx, modifiedNode, client.StrategicMergeFrom(node)); err != nil { + return fmt.Errorf("patching node: %w", err) } return nil @@ -272,12 +363,15 @@ func (r *BootcNodePoolReconciler) freeRebootSlot( bn *bootcv1alpha1.BootcNode, node *corev1.Node, ) error { - if err := r.restoreCordonState(ctx, bn, node); err != nil { + if err := r.restoreNodeState(ctx, bn, node); err != nil { return err } - // Remove both reboot slot annotations from the BootcNode. + // Remove all reboot slot annotations from the BootcNode, including the + // Karpenter ownership trackers if present. modified := bn.DeepCopy() + delete(modified.Annotations, bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair) + delete(modified.Annotations, bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt) delete(modified.Annotations, bootcv1alpha1.AnnotationWasCordoned) delete(modified.Annotations, bootcv1alpha1.AnnotationInRebootSlot) if err := r.Patch(ctx, modified, client.MergeFrom(bn)); err != nil { diff --git a/internal/controller/rollout_karpenter_envtest_test.go b/internal/controller/rollout_karpenter_envtest_test.go new file mode 100644 index 00000000..91c25755 --- /dev/null +++ b/internal/controller/rollout_karpenter_envtest_test.go @@ -0,0 +1,257 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +// TestKarpenterAnnotations is a table-driven integration test that covers +// Karpenter NodeClaim annotation management across all distinct scenarios. Each +// row exercises a different combination of node ownership, pre-existing +// annotations, and cordon state. The focused single-scenario predecessors of +// this test are preserved in rollout_karpenter_focused_envtest_test.go. + +import ( + "context" + "fmt" + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" + "sigs.k8s.io/controller-runtime/pkg/client" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + testutil "github.com/bootc-dev/bootc-operator/test/util" +) + +func TestKarpenterAnnotations(t *testing.T) { + tests := []struct { + // name is the subtest name and is used to derive unique resource names. + name string + + // Node setup. + nodeClaim bool // whether to attach a NodeClaim owner reference + preExistingAnnotations map[string]string // annotations to set on the Node before the rollout + preCordon bool // whether the Node starts cordoned + + // Expected BootcNode ownership annotations while the reboot slot is held. + wantOwnsRepair bool // AnnotationOwnsKarpenterDoNotRepair should be present + wantOwnsDisrupt bool // AnnotationOwnsKarpenterDoNotDisrupt should be present + + // Expected Node annotations while the reboot slot is held. + wantRepairDuring bool // karpenter.sh/do-not-repair should be present + wantDisruptDuring bool // karpenter.sh/do-not-disrupt should be present + + // Expected Node annotations after the slot is freed. + wantRepairAfter bool // karpenter.sh/do-not-repair should still be present + wantDisruptAfter bool // karpenter.sh/do-not-disrupt should still be present + + // Expected Node cordon state after the slot is freed. + wantCordonedAfter bool + }{ + // A plain NodeClaim-owned node with no pre-existing Karpenter state. + // The operator adds both annotations on slot assignment, records + // ownership, and removes them when the slot is freed. + { + name: "nodeclaim owned, no pre-existing annotations", + nodeClaim: true, + wantOwnsRepair: true, + wantOwnsDisrupt: true, + wantRepairDuring: true, + wantDisruptDuring: true, + wantRepairAfter: false, + wantDisruptAfter: false, + wantCordonedAfter: false, + }, + // A node with both Karpenter annotations already set by an external + // actor and no NodeClaim owner reference. The operator must not claim + // ownership of either annotation and must leave both untouched + // throughout, including after the slot is freed. + { + name: "no nodeclaim, both annotations pre-existing", + nodeClaim: false, + preExistingAnnotations: map[string]string{ + karpenterDoNotRepairAnnotationKey: "true", + karpenterDoNotDisruptAnnotationKey: "true", + }, + wantOwnsRepair: false, + wantOwnsDisrupt: false, + wantRepairDuring: true, // preserved as-is + wantDisruptDuring: true, // preserved as-is + wantRepairAfter: true, // untouched by operator + wantDisruptAfter: true, // untouched by operator + wantCordonedAfter: false, + }, + // A NodeClaim-owned node that was already cordoned before the rollout. + // The operator adds both Karpenter annotations and removes them on slot + // free, but the pre-existing cordon must be preserved throughout. + { + name: "nodeclaim owned, pre-cordoned", + nodeClaim: true, + preCordon: true, + wantOwnsRepair: true, + wantOwnsDisrupt: true, + wantRepairDuring: true, + wantDisruptDuring: true, + wantRepairAfter: false, + wantDisruptAfter: false, + wantCordonedAfter: true, // pre-existing cordon preserved + }, + // A NodeClaim-owned node where only do-not-repair is pre-existing. + // The operator must not claim do-not-repair (already set externally) + // but must add, own, and later remove do-not-disrupt independently. + // This verifies that per-annotation ownership tracking is applied + // independently rather than as an all-or-nothing decision. + { + name: "nodeclaim owned, only do-not-repair pre-existing", + nodeClaim: true, + preExistingAnnotations: map[string]string{ + karpenterDoNotRepairAnnotationKey: "true", + }, + wantOwnsRepair: false, // pre-existing; operator does not claim it + wantOwnsDisrupt: true, // not pre-existing; operator adds and owns it + wantRepairDuring: true, // preserved from pre-existing + wantDisruptDuring: true, // added by operator + wantRepairAfter: true, // not owned; left in place + wantDisruptAfter: false, // owned; removed by operator + wantCordonedAfter: false, + }, + } + + for i, tc := range tests { + tc := tc + // Derive unique, collision-free resource names from the subtest index. + nodeName := fmt.Sprintf("karp-ann-%d-node", i) + poolName := fmt.Sprintf("karp-ann-%d-pool", i) + + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + // Build and create the K8s Node. + node := testutil.NewK8sNode(nodeName, testutil.WorkerLabels()) + if tc.nodeClaim { + node.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: karpenterNodeClaimAPIVersion, + Kind: karpenterNodeClaimKind, + Name: nodeName + "-claim", + UID: types.UID(nodeName + "-claim-uid"), + }} + } + if len(tc.preExistingAnnotations) > 0 { + node.Annotations = tc.preExistingAnnotations + } + if tc.preCordon { + node.Spec.Unschedulable = true + } + g.Expect(k8sClient.Create(ctx, node)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, node) }) + + // Create pool and wait for BootcNode. + 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) }) + + g.Eventually(func() error { + return k8sClient.Get( + ctx, + client.ObjectKey{Name: nodeName}, + &bootcv1alpha1.BootcNode{}, + ) + }).Should(Succeed()) + + simulateDaemonStatus(g, ctx, nodeName, testDigestA, bootcv1alpha1.NodeReasonStaged) + + // Wait for slot assignment and drain completion. + g.Eventually(func() (*bootcv1alpha1.BootcNode, error) { + var bn bootcv1alpha1.BootcNode + err := k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &bn) + return &bn, err + }).Should(And( + HaveField("Annotations", HaveKey(bootcv1alpha1.AnnotationInRebootSlot)), + HaveField("Spec.DesiredImageState", Equal(bootcv1alpha1.DesiredImageStateBooted)), + ), "BootcNode should have reboot slot and desiredImageState Booted") + + // Assert ownership annotations on BootcNode. + g.Eventually(func() (map[string]string, error) { + var bn bootcv1alpha1.BootcNode + err := k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &bn) + return bn.Annotations, err + }).Should(matchOwnership(tc.wantOwnsRepair, tc.wantOwnsDisrupt), + "BootcNode ownership annotations during slot") + + // Assert Karpenter annotations on Node. + g.Eventually(func() (map[string]string, error) { + var n corev1.Node + err := k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &n) + return n.Annotations, err + }).Should(matchKarpenterAnnotations(tc.wantRepairDuring, tc.wantDisruptDuring), + "Node Karpenter annotations during slot") + + // Simulate successful reboot and node becoming Ready. + simulateDaemonStatus(g, ctx, nodeName, testDigestB, bootcv1alpha1.NodeReasonIdle) + setNodeReady(g, ctx, nodeName) + + // Wait for slot to be freed. + g.Eventually(func() (map[string]string, error) { + var bn bootcv1alpha1.BootcNode + err := k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &bn) + return bn.Annotations, err + }).Should(And( + Not(HaveKey(bootcv1alpha1.AnnotationInRebootSlot)), + Not(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair)), + Not(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt)), + ), "BootcNode should have no slot or ownership annotations after slot free") + + // Assert final Node annotation state. + g.Eventually(func() (map[string]string, error) { + var n corev1.Node + err := k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &n) + return n.Annotations, err + }).Should(matchKarpenterAnnotations(tc.wantRepairAfter, tc.wantDisruptAfter), + "Node Karpenter annotations after slot free") + + // Assert final cordon state. + g.Eventually(func() (bool, error) { + var n corev1.Node + err := k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &n) + return n.Spec.Unschedulable, err + }).Should(Equal(tc.wantCordonedAfter), "Node cordon state after slot free") + }) + } +} + +// matchOwnership returns a Gomega matcher for the BootcNode annotation map +// that asserts the presence or absence of the two Karpenter ownership +// annotations independently. +func matchOwnership(wantRepair, wantDisrupt bool) OmegaMatcher { + repair := Not(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair)) + if wantRepair { + repair = HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair) + } + disrupt := Not(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt)) + if wantDisrupt { + disrupt = HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt) + } + return And(repair, disrupt) +} + +// matchKarpenterAnnotations returns a Gomega matcher for the Node annotation +// map that asserts the presence or absence of the two Karpenter annotations +// independently. +func matchKarpenterAnnotations(wantRepair, wantDisrupt bool) OmegaMatcher { + repair := Not(HaveKey(karpenterDoNotRepairAnnotationKey)) + if wantRepair { + repair = HaveKey(karpenterDoNotRepairAnnotationKey) + } + disrupt := Not(HaveKey(karpenterDoNotDisruptAnnotationKey)) + if wantDisrupt { + disrupt = HaveKey(karpenterDoNotDisruptAnnotationKey) + } + return And(repair, disrupt) +} diff --git a/internal/controller/rollout_test.go b/internal/controller/rollout_test.go index 18ffd244..8b31bff3 100644 --- a/internal/controller/rollout_test.go +++ b/internal/controller/rollout_test.go @@ -3,18 +3,62 @@ package controller import ( + "context" "testing" "time" "github.com/go-logr/logr" . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" testutil "github.com/bootc-dev/bootc-operator/test/util" ) +// patchCountingClient wraps a client.Client and counts Patch calls to the +// Node object (by GVK). Used to assert that freeRebootSlot doesn't +// double-patch the Node. +type patchCountingClient struct { + client.Client + nodePatches int +} + +func (c *patchCountingClient) Patch( + ctx context.Context, + obj client.Object, + patch client.Patch, + opts ...client.PatchOption, +) error { + if _, ok := obj.(*corev1.Node); ok { + c.nodePatches++ + } + return c.Client.Patch(ctx, obj, patch, opts...) +} + +// newFakeReconciler builds a BootcNodePoolReconciler backed by a fake +// client pre-seeded with the given objects. It also returns the counting +// wrapper so callers can assert the number of Node patches. +func newFakeReconciler(objs ...client.Object) (*BootcNodePoolReconciler, *patchCountingClient) { + sc := runtime.NewScheme() + _ = scheme.AddToScheme(sc) + _ = bootcv1alpha1.AddToScheme(sc) + + base := fake.NewClientBuilder(). + WithScheme(sc). + WithObjects(objs...). + Build() + + counting := &patchCountingClient{Client: base} + r := &BootcNodePoolReconciler{Client: counting} + return r, counting +} + func TestBuildRolloutState(t *testing.T) { const ( desiredImage = testImageDigestRefA @@ -522,3 +566,418 @@ func TestFindUnhealthySlots(t *testing.T) { g.Expect(findUnhealthySlots(rs, targetDigest)).To(BeEmpty()) }) } + +// TestRestoreNodeState verifies that restoreNodeState removes owned Karpenter +// annotations and restores cordon state in a single patch, covering all +// combinations of ownership and cordon state. +func TestRestoreNodeState(t *testing.T) { + tests := []struct { + name string + ownsRepair bool + ownsDisrupt bool + wasCordoned string // "true", "false", or "" (absent) + nodeCordoned bool + nodeAnnotations map[string]string + + wantNodePatches int + wantCordoned bool + wantRepairPresent bool + wantDisruptPresent bool + }{ + { + name: "owns both annotations, operator cordoned: single patch removes all", + ownsRepair: true, + ownsDisrupt: true, + wasCordoned: "false", + nodeCordoned: true, + nodeAnnotations: map[string]string{ + karpenterDoNotRepairAnnotationKey: "true", + karpenterDoNotDisruptAnnotationKey: "true", + }, + wantNodePatches: 1, + wantCordoned: false, + wantRepairPresent: false, + wantDisruptPresent: false, + }, + { + name: "owns both annotations, pre-cordoned: single patch, cordon preserved", + ownsRepair: true, + ownsDisrupt: true, + wasCordoned: "true", + nodeCordoned: true, + nodeAnnotations: map[string]string{ + karpenterDoNotRepairAnnotationKey: "true", + karpenterDoNotDisruptAnnotationKey: "true", + }, + wantNodePatches: 1, + wantCordoned: true, + wantRepairPresent: false, + wantDisruptPresent: false, + }, + { + name: "owns only do-not-disrupt: removes only that one", + ownsRepair: false, + ownsDisrupt: true, + wasCordoned: "true", + nodeCordoned: true, + nodeAnnotations: map[string]string{ + karpenterDoNotRepairAnnotationKey: "true", + karpenterDoNotDisruptAnnotationKey: "true", + }, + wantNodePatches: 1, + wantCordoned: true, + wantRepairPresent: true, + wantDisruptPresent: false, + }, + { + name: "no karpenter ownership, operator cordoned: single uncordon patch", + ownsRepair: false, + ownsDisrupt: false, + wasCordoned: "false", + nodeCordoned: true, + wantNodePatches: 1, + wantCordoned: false, + }, + { + name: "no karpenter ownership, pre-cordoned: no patch needed", + ownsRepair: false, + ownsDisrupt: false, + wasCordoned: "true", + nodeCordoned: true, + wantNodePatches: 0, + wantCordoned: true, + }, + { + name: "nothing to do: zero patches", + ownsRepair: false, + ownsDisrupt: false, + wasCordoned: "", + nodeCordoned: false, + wantNodePatches: 0, + wantCordoned: false, + }, + { + // Karpenter annotations present on the node but set by an external + // actor, not the operator. The operator must not touch them. + name: "annotations present but not owned: zero patches, annotations preserved", + ownsRepair: false, + ownsDisrupt: false, + wasCordoned: "false", + nodeCordoned: true, + nodeAnnotations: map[string]string{ + karpenterDoNotRepairAnnotationKey: "true", + karpenterDoNotDisruptAnnotationKey: "true", + }, + wantNodePatches: 1, // uncordon only + wantCordoned: false, + wantRepairPresent: true, + wantDisruptPresent: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + bnAnnotations := map[string]string{} + if tc.wasCordoned != "" { + bnAnnotations[bootcv1alpha1.AnnotationWasCordoned] = tc.wasCordoned + } + if tc.ownsRepair { + bnAnnotations[bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair] = "true" + } + if tc.ownsDisrupt { + bnAnnotations[bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt] = "true" + } + bn := testutil.NewNode("test-node", testImageDigestRefA) + bn.Annotations = bnAnnotations + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Annotations: tc.nodeAnnotations, + }, + Spec: corev1.NodeSpec{Unschedulable: tc.nodeCordoned}, + } + + r, counting := newFakeReconciler(bn, k8sNode) + + err := r.restoreNodeState(ctx, bn, k8sNode) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(counting.nodePatches).To(Equal(tc.wantNodePatches), "node patch count") + + var finalNode corev1.Node + g.Expect(counting.Client.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)). + To(Succeed()) + g.Expect(finalNode.Spec.Unschedulable).To(Equal(tc.wantCordoned), "cordon state") + if tc.wantRepairPresent { + g.Expect(finalNode.Annotations).To(HaveKey(karpenterDoNotRepairAnnotationKey)) + } else { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenterDoNotRepairAnnotationKey)) + } + if tc.wantDisruptPresent { + g.Expect(finalNode.Annotations).To(HaveKey(karpenterDoNotDisruptAnnotationKey)) + } else { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenterDoNotDisruptAnnotationKey)) + } + }) + } +} + +// TestAssignRebootSlotNodePatchCount verifies that assignRebootSlot issues at +// most one patch to the corev1.Node regardless of Karpenter ownership and +// prior cordon state. +func TestAssignRebootSlotNodePatchCount(t *testing.T) { + karpenterOwnerRef := metav1.OwnerReference{ + APIVersion: karpenterNodeClaimAPIVersion, + Kind: karpenterNodeClaimKind, + Name: "test-claim", + UID: "test-claim-uid", + } + + tests := []struct { + name string + karpenterOwned bool + preExistAnnotations map[string]string // annotations already on the Node + preCordoned bool + + wantNodePatches int + wantCordoned bool + wantRepairPresent bool + wantDisruptPresent bool + }{ + { + name: "karpenter owned, not cordoned: one patch for cordon + both annotations", + karpenterOwned: true, + wantNodePatches: 1, + wantCordoned: true, + wantRepairPresent: true, + wantDisruptPresent: true, + }, + { + name: "karpenter owned, pre-cordoned: one patch for annotations only", + karpenterOwned: true, + preCordoned: true, + wantNodePatches: 1, + wantCordoned: true, + wantRepairPresent: true, + wantDisruptPresent: true, + }, + { + name: "karpenter owned, both annotations pre-existing: zero node patches", + karpenterOwned: true, + preCordoned: true, + preExistAnnotations: map[string]string{ + karpenterDoNotRepairAnnotationKey: "true", + karpenterDoNotDisruptAnnotationKey: "true", + }, + wantNodePatches: 0, + wantCordoned: true, + wantRepairPresent: true, + wantDisruptPresent: true, + }, + { + name: "non-karpenter, not cordoned: one patch for cordon only", + karpenterOwned: false, + wantNodePatches: 1, + wantCordoned: true, + }, + { + name: "non-karpenter, pre-cordoned: zero node patches", + karpenterOwned: false, + preCordoned: true, + wantNodePatches: 0, + wantCordoned: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + bn := testutil.NewNode("test-node", testImageDigestRefA) + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Annotations: tc.preExistAnnotations, + }, + Spec: corev1.NodeSpec{Unschedulable: tc.preCordoned}, + } + if tc.karpenterOwned { + k8sNode.OwnerReferences = []metav1.OwnerReference{karpenterOwnerRef} + } + + r, counting := newFakeReconciler(bn, k8sNode) + + err := r.assignRebootSlot(ctx, bn, k8sNode) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(counting.nodePatches).To(Equal(tc.wantNodePatches), "node patch count") + + var finalNode corev1.Node + g.Expect(counting.Client.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)). + To(Succeed()) + g.Expect(finalNode.Spec.Unschedulable).To(Equal(tc.wantCordoned), "cordon state") + if tc.wantRepairPresent { + g.Expect(finalNode.Annotations).To(HaveKey(karpenterDoNotRepairAnnotationKey)) + } else { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenterDoNotRepairAnnotationKey)) + } + if tc.wantDisruptPresent { + g.Expect(finalNode.Annotations).To(HaveKey(karpenterDoNotDisruptAnnotationKey)) + } else { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenterDoNotDisruptAnnotationKey)) + } + + // The BootcNode must always have the slot annotation. + var finalBN bootcv1alpha1.BootcNode + g.Expect(counting.Client.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalBN)). + To(Succeed()) + g.Expect(finalBN.Annotations).To(HaveKey(bootcv1alpha1.AnnotationInRebootSlot)) + }) + } +} + +// TestFreeRebootSlotNodePatchCount verifies that freeRebootSlot issues at most +// one patch to the corev1.Node regardless of which combination of Karpenter +// annotations and cordon state are in play. +func TestFreeRebootSlotNodePatchCount(t *testing.T) { + tests := []struct { + name string + + // BootcNode annotation state + ownsRepair bool + ownsDisrupt bool + wasCordoned string // "true", "false", or "" (absent) + + // Node state + nodeCordoned bool + nodeAnnotations map[string]string + + // Expected outcomes + wantNodePatches int + wantCordoned bool + wantRepairPresent bool + wantDisruptPresent bool + }{ + { + // Karpenter annotations owned + operator cordoned: remove both + // annotations and uncordon in a single patch. + name: "karpenter owned, operator cordoned: one patch", + ownsRepair: true, + ownsDisrupt: true, + wasCordoned: "false", + nodeCordoned: true, + nodeAnnotations: map[string]string{ + karpenterDoNotRepairAnnotationKey: "true", + karpenterDoNotDisruptAnnotationKey: "true", + }, + wantNodePatches: 1, + wantCordoned: false, + wantRepairPresent: false, + wantDisruptPresent: false, + }, + { + // Karpenter annotations owned + node was pre-cordoned: remove + // annotations but leave the cordon, still a single patch. + name: "karpenter owned, pre-cordoned: one patch, cordon preserved", + ownsRepair: true, + ownsDisrupt: true, + wasCordoned: "true", + nodeCordoned: true, + nodeAnnotations: map[string]string{ + karpenterDoNotRepairAnnotationKey: "true", + karpenterDoNotDisruptAnnotationKey: "true", + }, + wantNodePatches: 1, + wantCordoned: true, + wantRepairPresent: false, + wantDisruptPresent: false, + }, + { + // No Karpenter ownership + operator cordoned: single uncordon patch. + name: "no karpenter, operator cordoned: one patch", + ownsRepair: false, + ownsDisrupt: false, + wasCordoned: "false", + nodeCordoned: true, + wantNodePatches: 1, + wantCordoned: false, + }, + { + // No Karpenter ownership + node was pre-cordoned: no node patch needed. + name: "no karpenter, pre-cordoned: zero patches", + ownsRepair: false, + ownsDisrupt: false, + wasCordoned: "true", + nodeCordoned: true, + wantNodePatches: 0, + wantCordoned: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // Build the BootcNode with the appropriate slot annotations. + bnAnnotations := map[string]string{ + bootcv1alpha1.AnnotationInRebootSlot: "", + } + if tc.wasCordoned != "" { + bnAnnotations[bootcv1alpha1.AnnotationWasCordoned] = tc.wasCordoned + } + if tc.ownsRepair { + bnAnnotations[bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair] = "true" + } + if tc.ownsDisrupt { + bnAnnotations[bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt] = "true" + } + bn := testutil.NewNode("test-node", testImageDigestRefA) + bn.Annotations = bnAnnotations + + // Build the corev1.Node. + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Annotations: tc.nodeAnnotations, + }, + Spec: corev1.NodeSpec{Unschedulable: tc.nodeCordoned}, + } + + r, counting := newFakeReconciler(bn, k8sNode) + + err := r.freeRebootSlot(ctx, bn, k8sNode) + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(counting.nodePatches).To(Equal(tc.wantNodePatches), + "unexpected number of Node patches") + + // Verify the final node state from the fake store. + var finalNode corev1.Node + g.Expect(counting.Client.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)). + To(Succeed()) + g.Expect(finalNode.Spec.Unschedulable).To(Equal(tc.wantCordoned), "cordon state") + if tc.ownsRepair { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenterDoNotRepairAnnotationKey)) + } + if tc.ownsDisrupt { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenterDoNotDisruptAnnotationKey)) + } + + // Verify slot annotations are cleared from the BootcNode. + var finalBN bootcv1alpha1.BootcNode + g.Expect(counting.Client.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalBN)). + To(Succeed()) + g.Expect(finalBN.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationInRebootSlot)) + g.Expect(finalBN.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationWasCordoned)) + g.Expect(finalBN.Annotations). + NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair)) + g.Expect(finalBN.Annotations). + NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt)) + }) + } +}