From 0c093f5dd0f00286822f4d5c09de4869b1a70044 Mon Sep 17 00:00:00 2001 From: Zack Zlotnik Date: Fri, 9 Oct 2026 10:29:51 -0400 Subject: [PATCH] makes bootc-operator aware of karpenter If a cluster contains both bootc-operator and Karpenter, it is possible that Karpenter could reap cluster nodes while a bootc-operator driven update is in progress. To prevent that, we detect whether the Node object is owned by a Karpenter NodeClaim object. And in that situation, we apply both the Karpenter Do Not Repair and Do Not Disrupt annotations to each node when it is cordoned for an update and we remove it after the update is completed. Signed-off-by: Zack Zlotnik Assisted-by: AI --- api/v1alpha1/constants.go | 16 + docs/src/SUMMARY.md | 1 + docs/src/operations/karpenter.md | 97 ++++ .../controller/bootcnodepool_controller.go | 42 +- internal/controller/rollout.go | 136 +++++- .../rollout_karpenter_envtest_test.go | 257 ++++++++++ internal/controller/rollout_test.go | 459 ++++++++++++++++++ 7 files changed, 977 insertions(+), 31 deletions(-) create mode 100644 docs/src/operations/karpenter.md create mode 100644 internal/controller/rollout_karpenter_envtest_test.go 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)) + }) + } +}