From a8a23c4e6cc946c17f16d5e535502b631c23025c Mon Sep 17 00:00:00 2001 From: Zack Zlotnik Date: Thu, 29 Oct 2026 20:27:47 +0000 Subject: [PATCH 1/5] karpenter: adds internal NodeClaim representation Add an internal Karpenter integration layer based on unstructured.Unstructured. Keep Karpenter out of the operator's Go dependencies and scheme registration so clusters without the NodeClaim CRD continue to work normally. Add tests for metadata initialization, status parsing, error handling, condition evaluation, and deep-copy behavior. Assisted-by: AI Signed-off-by: Zack Zlotnik --- internal/karpenter/nodeclaim.go | 119 +++++++++++++++++++++++++++ internal/karpenter/nodeclaim_test.go | 100 ++++++++++++++++++++++ 2 files changed, 219 insertions(+) create mode 100644 internal/karpenter/nodeclaim.go create mode 100644 internal/karpenter/nodeclaim_test.go diff --git a/internal/karpenter/nodeclaim.go b/internal/karpenter/nodeclaim.go new file mode 100644 index 00000000..f29b2e07 --- /dev/null +++ b/internal/karpenter/nodeclaim.go @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package karpenter provides types and constants for working with Karpenter +// NodeClaim objects without importing the Karpenter API module. Using +// unstructured keeps Karpenter optional, avoids adding a Karpenter API +// dependency or scheme registration to bootc-operator, and lets clusters +// without the NodeClaim CRD continue to work normally. The tradeoff is that +// the small set of NodeClaim fields used by the controller, such as +// status.nodeName and status.conditions, is checked at runtime rather than +// being enforced by Go types. +// +// See docs/src/operations/karpenter.md for the integration design. +package karpenter + +import ( + "fmt" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" +) + +// Karpenter API identity constants. +const ( + Group = "karpenter.sh" + NodeClaimAPIVersion = Group + "/v1" + NodeClaimKind = "NodeClaim" +) + +// Annotation keys managed by bootc-operator on Nodes and NodeClaims to +// coordinate with Karpenter lifecycle operations. +const ( + DoNotRepairAnnotationKey = "karpenter.sh/do-not-repair" + DoNotDisruptAnnotationKey = "karpenter.sh/do-not-disrupt" +) + +// Taint keys and effects used by Karpenter during node lifecycle operations. +const ( + RebootingTaintKey = "karpenter.sh/rebooting" + DisruptedTaintKey = "karpenter.sh/disrupted" + RebootingTaintEffect = corev1.TaintEffectNoSchedule +) + +// NodeClaim status condition types that indicate an active lifecycle operation +// incompatible with a bootc-operator update. +const ( + ConditionRebooting = "Rebooting" + ConditionDisruptionReason = "DisruptionReason" + ConditionInstanceTerminating = "InstanceTerminating" +) + +// NodeClaim is the controller's representation of a Karpenter NodeClaim. +// Embedding Unstructured keeps Karpenter optional while keeping all +// Karpenter-specific field access behind this type. If Karpenter API types are +// imported in the future, this is the only type callers need to change. +type NodeClaim struct { + unstructured.Unstructured +} + +var ( + _ metav1.Object = (*NodeClaim)(nil) + _ runtime.Object = (*NodeClaim)(nil) +) + +// NewNodeClaim returns a NodeClaim with APIVersion and Kind pre-set. +func NewNodeClaim() *NodeClaim { + c := &NodeClaim{} + c.SetAPIVersion(NodeClaimAPIVersion) + c.SetKind(NodeClaimKind) + return c +} + +// DeepCopy preserves the wrapper type when a NodeClaim is copied for a patch. +func (c *NodeClaim) DeepCopy() *NodeClaim { + if c == nil { + return nil + } + return &NodeClaim{Unstructured: *c.Unstructured.DeepCopy()} +} + +// DeepCopyObject preserves the wrapper type for runtime.Object callers. +func (c *NodeClaim) DeepCopyObject() runtime.Object { + return c.DeepCopy() +} + +// NodeName returns the name of the Kubernetes Node represented by this claim. +// An empty node name is valid when the field is present. It returns an error +// when status.nodeName is missing or has the wrong type. +func (c *NodeClaim) NodeName() (string, error) { + nodeName, found, err := unstructured.NestedString(c.Object, "status", "nodeName") + if err != nil { + return "", err + } + if !found { + return "", fmt.Errorf("status.nodeName not found") + } + return nodeName, nil +} + +// ConditionIsTrue reports whether the specified NodeClaim status condition is +// currently True. +func (c *NodeClaim) ConditionIsTrue(conditionType string) (bool, error) { + conditions, _, err := unstructured.NestedSlice(c.Object, "status", "conditions") + if err != nil { + return false, err + } + + for _, item := range conditions { + condition, ok := item.(map[string]interface{}) + if !ok || condition["status"] != "True" { + continue + } + if condition["type"] == conditionType { + return true, nil + } + } + return false, nil +} diff --git a/internal/karpenter/nodeclaim_test.go b/internal/karpenter/nodeclaim_test.go new file mode 100644 index 00000000..130c4ab9 --- /dev/null +++ b/internal/karpenter/nodeclaim_test.go @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: Apache-2.0 + +package karpenter_test + +import ( + "testing" + + . "github.com/onsi/gomega" + + karpenter "github.com/bootc-dev/bootc-operator/internal/karpenter" +) + +func TestNodeClaim(t *testing.T) { + g := NewWithT(t) + claim := karpenter.NewNodeClaim() + claim.Object["metadata"] = map[string]interface{}{"name": "claim"} + claim.Object["status"] = map[string]interface{}{ + "nodeName": "node", + "conditions": []interface{}{ + map[string]interface{}{"type": karpenter.ConditionDisruptionReason, "status": "False"}, + map[string]interface{}{"type": karpenter.ConditionRebooting, "status": "True"}, + }, + } + + g.Expect(claim.GetAPIVersion()).To(Equal(karpenter.NodeClaimAPIVersion)) + g.Expect(claim.GetKind()).To(Equal(karpenter.NodeClaimKind)) + nodeName, err := claim.NodeName() + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(nodeName).To(Equal("node")) + + active, err := claim.ConditionIsTrue(karpenter.ConditionRebooting) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(active).To(BeTrue()) + active, err = claim.ConditionIsTrue(karpenter.ConditionDisruptionReason) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(active).To(BeFalse()) +} + +func TestNodeClaimDeepCopy(t *testing.T) { + g := NewWithT(t) + claim := karpenter.NewNodeClaim() + claim.SetName("claim") + claim.SetAnnotations(map[string]string{"example.com/annotation": "original"}) + + copied := claim.DeepCopy() + copied.SetAnnotations(map[string]string{"example.com/annotation": "copy"}) + + g.Expect(copied).NotTo(BeIdenticalTo(claim)) + g.Expect(copied.GetName()).To(Equal(claim.GetName())) + g.Expect(claim.GetAnnotations()).To(HaveKeyWithValue("example.com/annotation", "original")) + g.Expect(copied.GetAnnotations()).To(HaveKeyWithValue("example.com/annotation", "copy")) + g.Expect(copied.DeepCopyObject()).To(BeAssignableToTypeOf(&karpenter.NodeClaim{})) +} + +func TestNodeClaimStatusErrors(t *testing.T) { + tests := []struct { + name string + status map[string]interface{} + wantNodeName string + wantNodeNameErr bool + wantConditionErr bool + }{ + {name: "missing node name", status: map[string]interface{}{}, wantNodeNameErr: true}, + {name: "empty node name", status: map[string]interface{}{"nodeName": ""}}, + { + name: "invalid node name", + status: map[string]interface{}{"nodeName": 42}, + wantNodeNameErr: true, + }, + { + name: "invalid conditions", + status: map[string]interface{}{"nodeName": "node", "conditions": "invalid"}, + wantNodeName: "node", + wantConditionErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + claim := karpenter.NewNodeClaim() + claim.Object["status"] = tt.status + + nodeName, err := claim.NodeName() + g.Expect(nodeName).To(Equal(tt.wantNodeName)) + if tt.wantNodeNameErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + + _, err = claim.ConditionIsTrue(karpenter.ConditionRebooting) + if tt.wantConditionErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +} From e80fd42d491f695b7c3560a03db71a8cf017f1f9 Mon Sep 17 00:00:00 2001 From: Zack Zlotnik Date: Thu, 29 Oct 2026 20:28:29 +0000 Subject: [PATCH 2/5] rollout: add Karpenter and startup taint coordination Detect NodeClaim and node-level disruption signals before assigning reboot slots or starting drains, and defer updates while Karpenter is rebooting, disrupting, or terminating a node. Temporarily apply do-not-repair and do-not-disrupt protection to Karpenter-managed Nodes and NodeClaims during update slots. Track ownership on BootcNodes and remove only annotations added by the operator when the slot is released. Add support for bootc.dev/os-not-ready startup taints. Remove it when the on-disk image matches the BootcNode desired image and the taint was present when the node joined the cluster. Assisted-by: AI Signed-off-by: Zack Zlotnik --- api/v1alpha1/constants.go | 47 +++ config/rbac/role.yaml | 9 + .../controller/bootcnodepool_controller.go | 315 ++++++++++++++++-- internal/controller/karpenter.go | 263 +++++++++++++++ internal/controller/rollout.go | 304 +++++++++++++++-- 5 files changed, 874 insertions(+), 64 deletions(-) create mode 100644 internal/controller/karpenter.go diff --git a/api/v1alpha1/constants.go b/api/v1alpha1/constants.go index d3d3338c..9e4d9034 100644 --- a/api/v1alpha1/constants.go +++ b/api/v1alpha1/constants.go @@ -30,4 +30,51 @@ 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 karpenter.sh/do-not-disrupt to the K8s Node during + // slot assignment. It has the same ownership and cleanup semantics as + // AnnotationOwnsKarpenterDoNotRepair. + AnnotationOwnsKarpenterDoNotDisrupt = "bootc.dev/owns-karpenter-do-not-disrupt" + + // AnnotationOwnsKarpenterNodeClaimDoNotRepair records that the controller + // added do-not-repair to the associated Karpenter NodeClaim. It follows the + // same ownership and cleanup semantics as AnnotationOwnsKarpenterDoNotRepair. + AnnotationOwnsKarpenterNodeClaimDoNotRepair = "bootc.dev/owns-karpenter-nodeclaim-do-not-repair" + + // AnnotationOwnsKarpenterNodeClaimDoNotDisrupt records that the controller + // added do-not-disrupt to the associated Karpenter NodeClaim. It follows the + // same ownership and cleanup semantics as AnnotationOwnsKarpenterDoNotRepair. + AnnotationOwnsKarpenterNodeClaimDoNotDisrupt = "bootc.dev/owns-karpenter-nodeclaim-do-not-disrupt" + + // TaintKeyOsNotReady is a well-known taint key that cluster administrators + // can apply to Karpenter NodePools as a startupTaint to gate workload + // scheduling until bootc-operator has applied and verified the pool's target + // OS image. The full taint is "bootc.dev/os-not-ready:NoSchedule". + // + // When a node joins with this taint, bootc-operator records ownership via + // AnnotationOwnsOsNotReadyTaint and removes the taint once the node is + // UpToDate and Ready. If the node is already running the target image when + // it joins, the taint is removed without requiring a reboot slot. + // + // This taint MUST be declared as a startupTaint in the Karpenter NodePool. + // If it is configured only as a regular taint, Karpenter will treat nodes + // with the taint as unable to satisfy pending pods and will provision + // additional nodes in a loop. + TaintKeyOsNotReady = "bootc.dev/os-not-ready" + + // AnnotationOwnsOsNotReadyTaint is set on a BootcNode to record that the + // controller found and owns the bootc.dev/os-not-ready:NoSchedule taint on + // the K8s Node at join time. When present, the controller removes the taint + // once the node is UpToDate and Ready. The controller never removes a taint + // it did not record ownership of. + AnnotationOwnsOsNotReadyTaint = "bootc.dev/owns-os-not-ready-taint" ) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index c25de146..28818c10 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -49,6 +49,15 @@ rules: - daemonsets verbs: - get +- apiGroups: + - karpenter.sh + resources: + - nodeclaims + verbs: + - get + - list + - patch + - watch - apiGroups: - node.bootc.dev resources: diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index 7d334476..4eb71a84 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -20,6 +20,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/events" @@ -35,6 +36,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/source" bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + karpenter "github.com/bootc-dev/bootc-operator/internal/karpenter" ) // drainStatus tracks an in-progress drain goroutine for a single node. @@ -89,6 +91,7 @@ type BootcNodePoolReconciler struct { // +kubebuilder:rbac:groups=apps,resources=daemonsets,verbs=get // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch +// +kubebuilder:rbac:groups=karpenter.sh,resources=nodeclaims,verbs=get;list;watch;patch // SetupWithManager sets up the controller with the Manager. func (r *BootcNodePoolReconciler) SetupWithManager(mgr ctrl.Manager) error { @@ -96,14 +99,67 @@ func (r *BootcNodePoolReconciler) SetupWithManager(mgr ctrl.Manager) error { r.drainCh = make(chan event.GenericEvent, 100) r.drains = make(map[string]*drainStatus) - return ctrl.NewControllerManagedBy(mgr). + // ctrlBuilder avoids shadowing the imported "sigs.k8s.io/controller-runtime/pkg/builder" package. + ctrlBuilder := ctrl.NewControllerManagedBy(mgr). For(&bootcv1alpha1.BootcNodePool{}). Owns(&bootcv1alpha1.BootcNode{}). Watches(&corev1.Node{}, handler.EnqueueRequestsFromMapFunc(r.mapNodeToPoolRequests), builder.WithPredicates(nodePredicates())). Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.mapSecretToPoolRequests), builder.WithPredicates(secretTypePredicate())). - WatchesRawSource(source.Channel(r.drainCh, &handler.EnqueueRequestForObject{})). - Named("bootcnodepool"). - Complete(r) + WatchesRawSource(source.Channel(r.drainCh, &handler.EnqueueRequestForObject{})) + + // Karpenter is optional. Only register the NodeClaim watch when its CRD is + // discoverable, so the operator remains usable on ordinary clusters. + if _, err := mgr.GetRESTMapper().RESTMapping( + schema.GroupKind{Group: karpenter.Group, Kind: karpenter.NodeClaimKind}, + "v1", + ); err == nil { + ctrlBuilder = ctrlBuilder.Watches( + karpenter.NewNodeClaim(), + handler.EnqueueRequestsFromMapFunc(r.mapNodeClaimToPoolRequests), + ) + } + + return ctrlBuilder.Named("bootcnodepool").Complete(r) +} + +// mapNodeClaimToPoolRequests maps a Karpenter NodeClaim status update to the +// BootcNodePool owning the corresponding Node. +func (r *BootcNodePoolReconciler) mapNodeClaimToPoolRequests( + ctx context.Context, + obj client.Object, +) []reconcile.Request { + log := logf.FromContext(ctx) + claim, ok := obj.(*karpenter.NodeClaim) + if !ok { + return nil + } + nodeName, err := claim.NodeName() + if err != nil { + log.Error(err, "Failed to read NodeClaim node name", "claim", claim.GetName()) + return nil + } + if nodeName == "" { + log.Info( + "NodeClaim has an empty node name; skipping event", + "claim", + claim.GetName(), + "warning", + true, + ) + return nil + } + var bn bootcv1alpha1.BootcNode + if err := r.Get(ctx, types.NamespacedName{Name: nodeName}, &bn); err != nil { + if !apierrors.IsNotFound(err) { + log.Error(err, "Failed to get BootcNode in NodeClaim mapper", "node", nodeName) + } + return nil + } + owner := metav1.GetControllerOf(&bn) + if owner == nil || owner.Kind != "BootcNodePool" { + return nil + } + return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: owner.Name}}} } // mapNodeToPoolRequests maps a Node event to the BootcNodePool(s) that should @@ -213,8 +269,8 @@ func nodeSelectorMatchesNode(sel *metav1.LabelSelector, node *corev1.Node) (bool } // nodePredicates returns predicates that filter Node events to only those -// relevant to pool membership: label changes, Ready condition changes, and -// spec.unschedulable changes. +// relevant to pool membership or reboot coordination: label changes, Ready +// condition changes, spec.unschedulable changes, and managed taint changes. func nodePredicates() predicate.Predicate { return predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { @@ -231,7 +287,9 @@ func nodePredicates() predicate.Predicate { } return nodeLabelsChanged(oldNode, newNode) || nodeReadyConditionChanged(oldNode, newNode) || - nodeUnschedulableChanged(oldNode, newNode) + nodeUnschedulableChanged(oldNode, newNode) || + karpenterRebootTaintChanged(oldNode, newNode) || + osNotReadyTaintChanged(oldNode, newNode) }, GenericFunc: func(e event.GenericEvent) bool { return true @@ -260,6 +318,24 @@ func nodeUnschedulableChanged(oldNode, newNode *corev1.Node) bool { return oldNode.Spec.Unschedulable != newNode.Spec.Unschedulable } +func karpenterRebootTaintChanged(oldNode, newNode *corev1.Node) bool { + return hasKarpenterRebootTaint(oldNode) != hasKarpenterRebootTaint(newNode) +} + +func osNotReadyTaintChanged(oldNode, newNode *corev1.Node) bool { + return nodeHasOsNotReadyTaint(oldNode) != nodeHasOsNotReadyTaint(newNode) +} + +func hasKarpenterRebootTaint(node *corev1.Node) bool { + for _, taint := range node.Spec.Taints { + if taint.Key == karpenter.RebootingTaintKey && + taint.Effect == karpenter.RebootingTaintEffect { + return true + } + } + return false +} + // secretTypePredicate returns a predicate that accepts only Secrets of // type kubernetes.io/dockerconfigjson. func secretTypePredicate() predicate.Predicate { @@ -396,10 +472,7 @@ func (r *BootcNodePoolReconciler) Reconcile( // Carry a time-based requeue (e.g. reboot timeout) unless something // sooner is already scheduled. - if rs.requeueAfter > 0 && - (resolveResult.RequeueAfter == 0 || rs.requeueAfter < resolveResult.RequeueAfter) { - resolveResult.RequeueAfter = rs.requeueAfter - } + resolveResult.RequeueAfter = earlierRequeue(resolveResult.RequeueAfter, rs.requeueAfter) return complete(resolveResult) } @@ -809,6 +882,18 @@ func (r *BootcNodePoolReconciler) createBootcNode( bn.Spec.RebootPolicy = effectiveRebootPolicy(pool) + // Record ownership of the os-not-ready startup taint if the node carries + // it. This must be done at join time so that nodes already running the + // target image (which never go through a reboot slot) still have their + // taint removed via the up-to-date path in driveRollout. + if nodeHasOsNotReadyTaint(node) { + metav1.SetMetaDataAnnotation( + &bn.ObjectMeta, + bootcv1alpha1.AnnotationOwnsOsNotReadyTaint, + "true", + ) + } + // Set ownerReference so the BootcNode is cleaned up if the pool is // deleted and so the Owns() watch routes BootcNode events to this pool. if err := controllerutil.SetControllerReference(pool, bn, r.Scheme); err != nil { @@ -827,6 +912,17 @@ func (r *BootcNodePoolReconciler) createBootcNode( return bn, nil } +// nodeHasOsNotReadyTaint reports whether the K8s Node carries the well-known +// bootc.dev/os-not-ready:NoSchedule startup taint. +func nodeHasOsNotReadyTaint(node *corev1.Node) bool { + for _, t := range node.Spec.Taints { + if t.Key == bootcv1alpha1.TaintKeyOsNotReady && t.Effect == corev1.TaintEffectNoSchedule { + return true + } + } + return false +} + // ensureManagedLabel adds or removes the bootc.dev/managed label on a Node. func (r *BootcNodePoolReconciler) ensureManagedLabel( ctx context.Context, @@ -879,19 +975,26 @@ func (r *BootcNodePoolReconciler) removeBootcNode( // Try to clean up the node (label + cordon state) before deleting // the BootcNode. The node may have been deleted from the cluster. var node corev1.Node - 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.ensureManagedLabel(ctx, &node, false); err != nil { - return fmt.Errorf("removing managed label: %w", err) - } - } - } else if !apierrors.IsNotFound(err) { + err := r.Get(ctx, types.NamespacedName{Name: bn.Name}, &node) + if err != nil && !apierrors.IsNotFound(err) { return fmt.Errorf("fetching node %s: %w", bn.Name, err) } + // No point in cleaning up the Node object if it's going away anyway... + if err == nil && node.DeletionTimestamp == nil { + // A node leaving the pool has not necessarily been verified against + // the pool image. Preserve the startup taint in that case; removing + // it here could expose an unverified OS image to workloads. The + // taint can be removed by the controller after verification or by an + // administrator when retiring the pool. + cleanupBN := bn.DeepCopy() + delete(cleanupBN.Annotations, bootcv1alpha1.AnnotationOwnsOsNotReadyTaint) + if err := r.restoreNodeState(ctx, cleanupBN, &node, nil); 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) + } + } if err := r.Delete(ctx, bn); err != nil && !apierrors.IsNotFound(err) { return fmt.Errorf("deleting BootcNode: %w", err) @@ -900,23 +1003,171 @@ 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, + prefetchedClaim *karpenter.NodeClaim, ) error { - if bn.Annotations[bootcv1alpha1.AnnotationWasCordoned] != "false" { + log := logf.FromContext(ctx) + modified := node.DeepCopy() + changed := false + + if removeOwnedAnnotation( + bn, + modified, + bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair, + karpenter.DoNotRepairAnnotationKey, + ) { + log.Info("Removing karpenter.sh/do-not-repair annotation from node", "node", node.Name) + changed = true + } + + if removeOwnedAnnotation( + bn, + modified, + bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt, + karpenter.DoNotDisruptAnnotationKey, + ) { + log.Info("Removing karpenter.sh/do-not-disrupt annotation from node", "node", node.Name) + changed = true + } + + if wasCordoned, ok := bn.Annotations[bootcv1alpha1.AnnotationWasCordoned]; ok && + wasCordoned == "false" { + log.Info("Uncordoning node", "node", node.Name) + modified.Spec.Unschedulable = false + changed = true + } + + if metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationOwnsOsNotReadyTaint) { + if removedTaint := removeOsNotReadyTaint(modified); removedTaint { + log.Info("Removing bootc.dev/os-not-ready taint from node", "node", node.Name) + changed = true + } + } + + if changed { + if err := r.Patch(ctx, modified, client.StrategicMergeFrom(node)); err != nil { + return fmt.Errorf("restoring node state: %w", err) + } + *node = *modified + } + + // Only fetch the NodeClaim when we own at least one of its annotations, + // to avoid a spurious API call for non-Karpenter nodes. + ownsNodeClaimRepair := metav1.HasAnnotation( + bn.ObjectMeta, + bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotRepair, + ) + ownsNodeClaimDisrupt := metav1.HasAnnotation( + bn.ObjectMeta, + bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotDisrupt, + ) + + // If we don't own either of these annotations, then there is nothing to do. + if !ownsNodeClaimRepair && !ownsNodeClaimDisrupt { 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) + + var claim *karpenter.NodeClaim + if prefetchedClaim != nil { + claim = prefetchedClaim + } else { + var err error + claim, _, err = r.getKarpenterNodeClaim(ctx, node) + if err != nil { + return fmt.Errorf("getting Karpenter NodeClaim: %w", err) + } } - *node = *modified + + // If the NodeClaim is absent (deleted or CRD missing), skip the patch. + // The node may have been forcefully replaced by Karpenter; there is nothing + // to clean up on a NodeClaim that no longer exists. + if claim == nil { + return nil + } + + claimModified := claim.DeepCopy() + claimChanged := ownsNodeClaimRepair && removeOwnedAnnotation( + bn, + claimModified, + bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotRepair, + karpenter.DoNotRepairAnnotationKey, + ) + + if ownsNodeClaimDisrupt && removeOwnedAnnotation( + bn, + claimModified, + bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotDisrupt, + karpenter.DoNotDisruptAnnotationKey, + ) { + claimChanged = true + } + + if claimChanged { + if err := r.Patch( + ctx, + &claimModified.Unstructured, + client.MergeFrom(&claim.Unstructured), + ); err != nil { + return fmt.Errorf("restoring Karpenter NodeClaim annotations: %w", err) + } + } + + return nil +} + +// removeOsNotReadyTaint removes the bootc.dev/os-not-ready:NoSchedule taint +// from modified in place and returns true if the taint was present and removed. +func removeOsNotReadyTaint(node *corev1.Node) bool { + taints := node.Spec.Taints + for i, t := range taints { + if t.Key == bootcv1alpha1.TaintKeyOsNotReady && t.Effect == corev1.TaintEffectNoSchedule { + node.Spec.Taints = append(taints[:i], taints[i+1:]...) + return true + } + } + return false +} + +// removeOsNotReadyTaintAndAnnotation removes the bootc.dev/os-not-ready taint +// from the K8s Node and clears the ownership annotation from the BootcNode. +// It is called for nodes that are UpToDate and Ready but never held a reboot +// slot (the taint removal was not handled by restoreNodeState in that case). +func (r *BootcNodePoolReconciler) removeOsNotReadyTaintAndAnnotation( + ctx context.Context, + bn *bootcv1alpha1.BootcNode, + node *corev1.Node, +) error { + log := logf.FromContext(ctx) + + modifiedNode := node.DeepCopy() + if removedTaint := removeOsNotReadyTaint(modifiedNode); removedTaint { + log.Info( + "Removing bootc.dev/os-not-ready taint from already-up-to-date node", + "node", + node.Name, + ) + if err := r.Patch(ctx, modifiedNode, client.StrategicMergeFrom(node)); err != nil { + return fmt.Errorf("removing os-not-ready taint from node: %w", err) + } + *node = *modifiedNode + } + + modifiedBN := bn.DeepCopy() + delete(modifiedBN.Annotations, bootcv1alpha1.AnnotationOwnsOsNotReadyTaint) + if err := r.Patch(ctx, modifiedBN, client.MergeFrom(bn)); err != nil { + return fmt.Errorf("clearing os-not-ready taint ownership annotation: %w", err) + } + *bn = *modifiedBN + return nil } diff --git a/internal/controller/karpenter.go b/internal/controller/karpenter.go new file mode 100644 index 00000000..0217b217 --- /dev/null +++ b/internal/controller/karpenter.go @@ -0,0 +1,263 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +// karpenter.go contains all Karpenter-specific controller logic: constants, +// types, and methods that coordinate bootc-operator node updates with Karpenter +// lifecycle management. The integration is designed to be optional — clusters +// without the Karpenter NodeClaim CRD continue to reconcile normally. +// +// See docs/src/operations/karpenter.md for a full description of the +// integration design, annotation semantics, and known limitations. + +import ( + "context" + "maps" + "time" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + karpenter "github.com/bootc-dev/bootc-operator/internal/karpenter" +) + +// karpenterMissingClaimRequeue is how long the controller waits before retrying when a +// Karpenter-owned node's NodeClaim is unexpectedly absent. The NodeClaim may +// be mid-deletion; waiting avoids treating a node under Karpenter termination +// as safe to update. +const karpenterMissingClaimRequeue = 30 * time.Second + +// karpenterNodeClaimResult is the outcome of a NodeClaim lookup. +type karpenterNodeClaimResult int + +const ( + // karpenterClaimFound means the NodeClaim exists and was returned. + karpenterClaimFound karpenterNodeClaimResult = iota + // karpenterClaimAbsent means the node has a NodeClaim owner reference but + // the object returned 404. The NodeClaim may be mid-deletion; callers + // should treat this as unsafe and defer. + karpenterClaimAbsent + // karpenterClaimCRDMissing means the Karpenter NodeClaim CRD is not + // installed on this cluster. Treat the node as non-Karpenter. + karpenterClaimCRDMissing + // karpenterClaimNoRef means the node has no Karpenter NodeClaim owner + // reference at all. + karpenterClaimNoRef + // karpenterClaimError means the API call failed for a transient or + // unexpected reason. The caller should propagate the accompanying error. + karpenterClaimError +) + +// isNodeOwnedByKarpenterNodeClaim reports whether the K8s Node has an owner +// reference pointing to a Karpenter NodeClaim (karpenter.sh/v1). +func isNodeOwnedByKarpenterNodeClaim(node *corev1.Node) bool { + for _, ref := range node.OwnerReferences { + if ref.APIVersion == karpenter.NodeClaimAPIVersion && ref.Kind == karpenter.NodeClaimKind { + return true + } + } + return false +} + +// getKarpenterNodeClaim returns the NodeClaim owning node, if one is available. +// The unstructured type keeps Karpenter optional; clusters without its CRD +// continue to reconcile normally. +// +// The second return value distinguishes five cases: +// - karpenterClaimFound: claim is non-nil, everything is normal. +// - karpenterClaimAbsent: the node has a NodeClaim owner ref but the object +// returned 404. This is unsafe — the NodeClaim may be mid-deletion. +// - karpenterClaimCRDMissing: the Karpenter CRD is not installed; treat the +// node as non-Karpenter. +// - karpenterClaimNoRef: no Karpenter owner reference on this node. +// - karpenterClaimError: a transient or unexpected API error; the +// accompanying error value is non-nil and should be propagated. +func (r *BootcNodePoolReconciler) getKarpenterNodeClaim( + ctx context.Context, + node *corev1.Node, +) (*karpenter.NodeClaim, karpenterNodeClaimResult, error) { + for _, ref := range node.OwnerReferences { + if ref.APIVersion != karpenter.NodeClaimAPIVersion || ref.Kind != karpenter.NodeClaimKind { + continue + } + claim := karpenter.NewNodeClaim() + err := r.Get(ctx, types.NamespacedName{Name: ref.Name}, claim) + switch { + case err == nil: + return claim, karpenterClaimFound, nil + case apimeta.IsNoMatchError(err): + // CRD not installed — Karpenter is not present on this cluster. + return nil, karpenterClaimCRDMissing, nil + case apierrors.IsNotFound(err): + // Owner ref points to a NodeClaim that no longer exists. + // This is unsafe: Karpenter may be replacing or terminating + // the node right now. + return nil, karpenterClaimAbsent, nil + default: + return nil, karpenterClaimError, err + } + } + return nil, karpenterClaimNoRef, nil +} + +// karpenterDisruptionStatus is the result of isKarpenterDisruptionActive. +type karpenterDisruptionStatus struct { + // active is true when Karpenter owns a lifecycle operation that conflicts + // with a bootc-operator update. claimMissing implies active. + active bool + + // claimMissing distinguishes an unexpectedly absent NodeClaim from other + // active disruptions. It always implies active; callers should check it + // before active and arrange a requeue rather than proceeding immediately. + claimMissing bool + + // claim is the fetched NodeClaim. It is only set when active is false and + // the node is Karpenter-owned. Callers can reuse it to avoid a second API + // round-trip (e.g. assignRebootSlot). + claim *karpenter.NodeClaim +} + +// isKarpenterDisruptionActive reports whether Karpenter currently owns any +// lifecycle operation that would conflict with a bootc-operator update. It +// checks for: +// +// - Node or NodeClaim deletion in progress (DeletionTimestamp set) +// - Karpenter reboot taint on the Node +// - Karpenter disrupted taint on the Node (voluntary disruption committed) +// - NodeClaim status conditions: Rebooting, DisruptionReason, or +// InstanceTerminating set to True +// +// When the NodeClaim owner reference is present but the object is absent +// (mid-deletion), Active and ClaimMissing are both true. Callers should +// arrange a requeue rather than continuing immediately. +// +// When Active is false and a NodeClaim was successfully fetched, Claim is +// non-nil. Callers that need the NodeClaim (e.g. assignRebootSlot) can reuse +// it to avoid a second API round-trip. +func (r *BootcNodePoolReconciler) isKarpenterDisruptionActive( + ctx context.Context, + node *corev1.Node, +) (karpenterDisruptionStatus, error) { + // A deleting Node is already being terminated — always defer. + if node.DeletionTimestamp != nil { + return karpenterDisruptionStatus{active: true}, nil + } + + // Fast path: check Node taints before making any API calls. + for _, taint := range node.Spec.Taints { + if taint.Effect != karpenter.RebootingTaintEffect { + continue + } + if taint.Key == karpenter.RebootingTaintKey || taint.Key == karpenter.DisruptedTaintKey { + return karpenterDisruptionStatus{active: true}, nil + } + } + + claim, result, err := r.getKarpenterNodeClaim(ctx, node) + if err != nil { + return karpenterDisruptionStatus{}, err + } + switch result { + case karpenterClaimNoRef, karpenterClaimCRDMissing: + // Not a Karpenter node, or Karpenter not installed. + return karpenterDisruptionStatus{}, nil + case karpenterClaimAbsent: + // NodeClaim owner ref exists but object is missing. This is unsafe: + // Karpenter may be mid-termination. Signal the caller to requeue. + return karpenterDisruptionStatus{active: true, claimMissing: true}, nil + } + + // The NodeClaim exists. Check for a pending deletion first. + if claim.GetDeletionTimestamp() != nil { + return karpenterDisruptionStatus{active: true}, nil + } + + // Check NodeClaim status conditions. + for _, conditionType := range []string{ + karpenter.ConditionRebooting, + karpenter.ConditionDisruptionReason, + karpenter.ConditionInstanceTerminating, + } { + active, err := claim.ConditionIsTrue(conditionType) + if err != nil { + return karpenterDisruptionStatus{}, err + } + if active { + return karpenterDisruptionStatus{active: true}, nil + } + } + return karpenterDisruptionStatus{claim: claim}, nil +} + +// annotationsEqual reports whether two annotation maps are identical. +// It treats a nil map and an empty map as equal, unlike maps.Equal which +// returns false when one argument is nil and the other is a non-nil empty map. +// Kubernetes can return either (a freshly created object with no annotations +// has nil; one that had all annotations deleted may have an empty map), so +// this distinction matters for correct annotation comparison. +func annotationsEqual(a, b map[string]string) bool { + if len(a) != len(b) { + return false + } + return len(a) == 0 || maps.Equal(a, b) +} + +// ensureOwnedAnnotation sets targetKey on target and records ownership on +// ownerKey when target does not already have the requested value. It does not +// rewrite the owner when ownership is already recorded. +func ensureOwnedAnnotation( + owner *bootcv1alpha1.BootcNode, + target metav1.Object, + ownerKey, targetKey, value string, +) bool { + if target.GetAnnotations()[targetKey] == value { + return false + } + + if _, owned := owner.GetAnnotations()[ownerKey]; !owned { + ownerAnnotations := maps.Clone(owner.GetAnnotations()) + if ownerAnnotations == nil { + ownerAnnotations = map[string]string{} + } + ownerAnnotations[ownerKey] = "true" + owner.SetAnnotations(ownerAnnotations) + } + + targetAnnotations := maps.Clone(target.GetAnnotations()) + if targetAnnotations == nil { + targetAnnotations = map[string]string{} + } + targetAnnotations[targetKey] = value + target.SetAnnotations(targetAnnotations) + return true +} + +// removeOwnedAnnotation removes targetKey from target when owner has recorded +// that it owns the annotation (via ownerKey). It does not remove ownerKey from owner; callers are +// responsible for clearing all ownership annotations together (e.g. in +// freeRebootSlot). This intentional asymmetry with ensureOwnedAnnotation keeps +// the ownership cleanup atomic at the call site rather than scattered across +// individual annotation removals. +// +// The map index on a nil annotations map is safe in Go and returns ("", false). +func removeOwnedAnnotation( + owner *bootcv1alpha1.BootcNode, + target metav1.Object, + ownerKey, targetKey string, +) bool { + if _, ok := owner.GetAnnotations()[ownerKey]; !ok { + return false + } + + targetAnnotations := maps.Clone(target.GetAnnotations()) + if _, ok := targetAnnotations[targetKey]; !ok { + return false + } + delete(targetAnnotations, targetKey) + target.SetAnnotations(targetAnnotations) + return true +} diff --git a/internal/controller/rollout.go b/internal/controller/rollout.go index f1c3d069..74f4257c 100644 --- a/internal/controller/rollout.go +++ b/internal/controller/rollout.go @@ -23,6 +23,7 @@ import ( bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" "github.com/bootc-dev/bootc-operator/internal/image" + karpenter "github.com/bootc-dev/bootc-operator/internal/karpenter" ) // Hardcode to 2 for now; might make this configurable later, or dynamically @@ -68,6 +69,12 @@ func (rs *rolloutState) nodeCount() int { len(rs.rebooting) + len(rs.degraded) + len(rs.unclassified) } +// setRequeueAfter schedules a requeue after d unless something sooner is +// already set. +func (rs *rolloutState) setRequeueAfter(d time.Duration) { + rs.requeueAfter = earlierRequeue(rs.requeueAfter, d) +} + // driveRollout is the main function that advances the rollout state machine. func (r *BootcNodePoolReconciler) driveRollout( ctx context.Context, @@ -163,18 +170,59 @@ func (r *BootcNodePoolReconciler) driveRollout( ) // Assign reboot slots to candidates. + // assignRebootSlot patches the BootcNode, so the resulting object event + // triggers the next reconciliation. No timed requeue is needed here. for _, bn := range candidates { var node corev1.Node if err := r.Get(ctx, types.NamespacedName{Name: bn.Name}, &node); err != nil { return nil, fmt.Errorf("fetching node %s: %w", bn.Name, err) } - if err := r.assignRebootSlot(ctx, bn, &node); err != nil { + // isKarpenterDisruptionActive fetches the NodeClaim as part of its + // disruption checks. When disruption is not active, it returns the + // claim so assignRebootSlot can reuse it without a second API call. + kds, err := r.isKarpenterDisruptionActive(ctx, &node) + if err != nil { + return nil, fmt.Errorf("checking Karpenter disruption for %s: %w", bn.Name, err) + } + if kds.claimMissing { + log.Info( + "Deferring reboot slot: Karpenter NodeClaim is unexpectedly absent, will retry", + "node", + bn.Name, + ) + rs.setRequeueAfter(karpenterMissingClaimRequeue) + continue + } + if kds.active { + log.Info("Deferring reboot slot: Karpenter disruption is active", "node", bn.Name) + continue + } + if err := r.assignRebootSlot(ctx, bn, &node, kds.claim); err != nil { return nil, fmt.Errorf("assigning reboot slot to %s: %w", bn.Name, err) } + // Periodic requeue fallback: now that a slot is assigned to a + // Karpenter-owned node, ensure we poll for NodeClaim status changes + // even when the NodeClaim watch is not registered. + if isNodeOwnedByKarpenterNodeClaim(&node) { + rs.setRequeueAfter(karpenterMissingClaimRequeue) + } + } + + // Periodic requeue fallback for already-slotted Karpenter-owned nodes: + // the annotation written by assignRebootSlot persists across reconciles, + // so we can detect Karpenter ownership without an extra API call. + for _, bn := range rs.staged { + if metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair) { + rs.setRequeueAfter(karpenterMissingClaimRequeue) + break + } } // Start drains for all slotted Staged nodes that haven't already been // approved for reboot (which implies they've already been drained). + // ensureDrain starts an asynchronous drain; completion sends an explicit + // event through drainCh. requeueAfter is therefore reserved for timeout + // and polling fallbacks, rather than normal drain progress. for _, bn := range rs.staged { if !metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) { continue @@ -182,66 +230,229 @@ func (r *BootcNodePoolReconciler) driveRollout( if bn.Spec.DesiredImageState == bootcv1alpha1.DesiredImageStateBooted { continue } + var node corev1.Node + if err := r.Get(ctx, types.NamespacedName{Name: bn.Name}, &node); err != nil { + return nil, fmt.Errorf("fetching node %s: %w", bn.Name, err) + } + kds, err := r.isKarpenterDisruptionActive(ctx, &node) + if err != nil { + return nil, fmt.Errorf("checking Karpenter disruption for %s: %w", bn.Name, err) + } + if kds.claimMissing { + log.Info("Deferring drain: Karpenter NodeClaim is unexpectedly absent, will retry", + "node", bn.Name) + rs.setRequeueAfter(karpenterMissingClaimRequeue) + continue + } + if kds.active { + log.Info("Deferring drain: Karpenter disruption is active", "node", bn.Name) + continue + } r.ensureDrain(ctx, pool, bn) } 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. +// 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. +// +// Note: these annotations are best-effort coordination signals, not a +// distributed lock. Karpenter can commit a disruption or reboot between the +// isKarpenterDisruptionActive check and the start of the drain. +// +// All operations are idempotent. +// +// prefetchedClaim may be the NodeClaim already fetched by a preceding +// isKarpenterDisruptionActive call. When non-nil and the node is +// Karpenter-owned, it is used directly to avoid a redundant API round-trip. func (r *BootcNodePoolReconciler) assignRebootSlot( ctx context.Context, bn *bootcv1alpha1.BootcNode, node *corev1.Node, + prefetchedClaim *karpenter.NodeClaim, ) error { log := logf.FromContext(ctx) - // Set annotations on the BootcNode 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{} + karpenterOwned := isNodeOwnedByKarpenterNodeClaim(node) + + // Resolve the NodeClaim. Use the caller-supplied prefetched claim when + // available to avoid a second API round-trip. If no claim was pre-fetched + // (e.g. the node is not Karpenter-owned or the call site doesn't supply + // one), fall back to fetching it here. + // + // claim may be nil even when karpenterOwned is true: + // - The NodeClaim CRD is not installed (Karpenter is not present on this + // cluster, so the API server returns a no-match error). + // - The NodeClaim is absent (karpenterClaimAbsent) — callers must have + // already deferred via isKarpenterDisruptionActive before reaching here. + // In both cases we skip NodeClaim annotation management and proceed with + // only Node-level fencing. + var claim *karpenter.NodeClaim + if karpenterOwned { + if prefetchedClaim != nil { + claim = prefetchedClaim + } else { + var claimResult karpenterNodeClaimResult + var err error + claim, claimResult, err = r.getKarpenterNodeClaim(ctx, node) + if err != nil { + return fmt.Errorf("getting NodeClaim: %w", err) + } + if claimResult == karpenterClaimAbsent { + return fmt.Errorf("NodeClaim for node %s disappeared unexpectedly", node.Name) + } } - modifiedBN.Annotations[bootcv1alpha1.AnnotationInRebootSlot] = "" - // Record whether the node was already cordoned before us. + } + // Build the desired BootcNode annotation state and patch only if anything + // changed. We apply every annotation the slot requires unconditionally and + // let the comparison at the end decide whether an API call is needed. This + // avoids a separate upfront guard expression that would duplicate the same + // per-annotation conditions. + modifiedBN := bn.DeepCopy() + modifiedNode := node.DeepCopy() + var modifiedClaim *karpenter.NodeClaim + if claim != nil { + modifiedClaim = claim.DeepCopy() + } + + // Mark the node as occupying a reboot slot. + metav1.SetMetaDataAnnotation(&modifiedBN.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot, "") + + // Record the prior cordon state so restoreNodeState can leave the node as + // it was found (i.e. not uncordon a node that was already cordoned). Only + // set this on first assignment — if the slot annotation is already present + // we must not overwrite the original recorded value. + if !metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) { + wasCordoned := "false" + if node.Spec.Unschedulable { - modifiedBN.Annotations[bootcv1alpha1.AnnotationWasCordoned] = "true" + wasCordoned = "true" + } + + metav1.SetMetaDataAnnotation( + &modifiedBN.ObjectMeta, + bootcv1alpha1.AnnotationWasCordoned, + wasCordoned, + ) + } + + // For Karpenter-managed nodes, record ownership of the do-not-repair and + // do-not-disrupt annotations on the Node and NodeClaim. We take ownership + // when the annotation is absent or not already set to "true". Karpenter + // only honors the literal value "true"; any other value (e.g. "false") is + // ineffective, so we overwrite it and track ownership so we clean it up. + if karpenterOwned { + ensureOwnedAnnotation( + modifiedBN, + modifiedNode, + bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair, + karpenter.DoNotRepairAnnotationKey, + "true", + ) + ensureOwnedAnnotation( + modifiedBN, + modifiedNode, + bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt, + karpenter.DoNotDisruptAnnotationKey, + "true", + ) + + if claim != nil { + ensureOwnedAnnotation( + modifiedBN, + modifiedClaim, + bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotRepair, + karpenter.DoNotRepairAnnotationKey, + "true", + ) + ensureOwnedAnnotation( + modifiedBN, + modifiedClaim, + bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotDisrupt, + karpenter.DoNotDisruptAnnotationKey, + "true", + ) + } + } + + if !annotationsEqual(bn.Annotations, modifiedBN.Annotations) { + if !metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) { + log.Info("Assigning reboot slot", "node", bn.Name) } else { - modifiedBN.Annotations[bootcv1alpha1.AnnotationWasCordoned] = "false" + log.Info("Updating reboot slot annotations", "node", bn.Name) } 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. + // Apply the Karpenter fence to the NodeClaim after recording ownership on + // the BootcNode. This ordering prevents a successful NodeClaim patch from + // becoming orphaned if the durable BootcNode update fails. + // + // NodeClaim annotations are a supplemental signal, not a distributed lock. + // Karpenter can already have selected or queued a disruption by the time + // these annotations are written; the Node-level checks (taint, deletion + // timestamp, disruption conditions) are the authoritative guard. + if claim != nil { + if !annotationsEqual(claim.GetAnnotations(), modifiedClaim.GetAnnotations()) { + if err := r.Patch( + ctx, + &modifiedClaim.Unstructured, + client.MergeFrom(&claim.Unstructured), + ); err != nil { + return fmt.Errorf("patching Karpenter NodeClaim: %w", err) + } + } + } + + // Cordon the K8s Node and set the Node-level Karpenter annotations. + // Skip the patch entirely when nothing needs to change. if !node.Spec.Unschedulable { log.Info("Cordoning node", "node", node.Name) - modifiedNode := node.DeepCopy() modifiedNode.Spec.Unschedulable = true + } + + if modifiedNode.Spec.Unschedulable != node.Spec.Unschedulable || + !annotationsEqual(node.Annotations, modifiedNode.Annotations) { if err := r.Patch(ctx, modifiedNode, client.StrategicMergeFrom(node)); err != nil { - return fmt.Errorf("cordoning node: %w", err) + return fmt.Errorf("patching node: %w", err) + } + if karpenterOwned { + if node.Annotations[karpenter.DoNotRepairAnnotationKey] != "true" { + log.Info("Set karpenter.sh/do-not-repair on node", "node", node.Name) + } + if node.Annotations[karpenter.DoNotDisruptAnnotationKey] != "true" { + log.Info("Set karpenter.sh/do-not-disrupt on node", "node", node.Name) + } } - *node = *modifiedNode } return nil } -// freeCompletedSlots releases reboot slots for upToDate nodes that are Ready. -// It decrements rs.occupiedSlots for each freed slot so the caller's available -// slot count is accurate. +// freeCompletedSlots releases reboot slots for upToDate nodes that are Ready, +// and removes the os-not-ready startup taint from nodes that are UpToDate and +// Ready but never held a reboot slot (e.g. nodes whose booted image already +// matched the target when they joined the pool). func (r *BootcNodePoolReconciler) freeCompletedSlots(ctx context.Context, rs *rolloutState) error { log := logf.FromContext(ctx) for _, bn := range rs.upToDate { - if !metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) { + inSlot := metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) + ownsTaint := metav1.HasAnnotation( + bn.ObjectMeta, + bootcv1alpha1.AnnotationOwnsOsNotReadyTaint, + ) + + if !inSlot && !ownsTaint { continue } @@ -251,15 +462,37 @@ func (r *BootcNodePoolReconciler) freeCompletedSlots(ctx context.Context, rs *ro } if nodeReadyStatus(&node) != corev1.ConditionTrue { - log.V(1).Info("Node rebooted but not yet Ready, holding reboot slot", "node", bn.Name) + if inSlot { + log.V(1). + Info("Node rebooted but not yet Ready, holding reboot slot", "node", bn.Name) + } continue } - log.Info("Freeing reboot slot: node healthy and Ready", "node", bn.Name) - if err := r.freeRebootSlot(ctx, bn, &node); err != nil { - return fmt.Errorf("freeing reboot slot for %s: %w", bn.Name, err) + if inSlot { + log.Info("Freeing reboot slot: node healthy and Ready", "node", bn.Name) + // Reuse the NodeClaim fetched by the disruption check when the + // node is Karpenter-owned. This avoids fetching it again while + // restoring NodeClaim annotations. + kds, err := r.isKarpenterDisruptionActive(ctx, &node) + if err != nil { + return fmt.Errorf("checking Karpenter disruption for %s: %w", bn.Name, err) + } + if err := r.freeRebootSlot(ctx, bn, &node, kds.claim); err != nil { + return fmt.Errorf("freeing reboot slot for %s: %w", bn.Name, err) + } + rs.occupiedSlots-- + continue + } + + if ownsTaint { + // Node was already running the target image when it joined; it + // never needed a reboot slot. Remove only the startup taint and + // clear the ownership annotation. + if err := r.removeOsNotReadyTaintAndAnnotation(ctx, bn, &node); err != nil { + return fmt.Errorf("removing os-not-ready taint for %s: %w", bn.Name, err) + } } - rs.occupiedSlots-- } return nil @@ -271,13 +504,20 @@ func (r *BootcNodePoolReconciler) freeRebootSlot( ctx context.Context, bn *bootcv1alpha1.BootcNode, node *corev1.Node, + prefetchedClaim *karpenter.NodeClaim, ) error { - if err := r.restoreCordonState(ctx, bn, node); err != nil { + if err := r.restoreNodeState(ctx, bn, node, prefetchedClaim); err != nil { return err } - // Remove both reboot slot annotations from the BootcNode. + // Remove all reboot slot annotations from the BootcNode, including the + // Karpenter and os-not-ready ownership trackers if present. modified := bn.DeepCopy() + delete(modified.Annotations, bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair) + delete(modified.Annotations, bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt) + delete(modified.Annotations, bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotRepair) + delete(modified.Annotations, bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotDisrupt) + delete(modified.Annotations, bootcv1alpha1.AnnotationOwnsOsNotReadyTaint) delete(modified.Annotations, bootcv1alpha1.AnnotationWasCordoned) delete(modified.Annotations, bootcv1alpha1.AnnotationInRebootSlot) if err := r.Patch(ctx, modified, client.MergeFrom(bn)); err != nil { From 400956077a025de2746d82b5e7db6378fb73a230 Mon Sep 17 00:00:00 2001 From: Zack Zlotnik Date: Thu, 29 Oct 2026 20:28:33 +0000 Subject: [PATCH 3/5] test: cover Karpenter rollout coordination Add unit and envtest coverage for Karpenter-aware rollout behavior. Assisted-by: AI Signed-off-by: Zack Zlotnik --- internal/controller/karpenter_test.go | 1316 +++++++++++++++++ .../rollout_karpenter_envtest_test.go | 257 ++++ 2 files changed, 1573 insertions(+) create mode 100644 internal/controller/karpenter_test.go create mode 100644 internal/controller/rollout_karpenter_envtest_test.go diff --git a/internal/controller/karpenter_test.go b/internal/controller/karpenter_test.go new file mode 100644 index 00000000..769c6a5a --- /dev/null +++ b/internal/controller/karpenter_test.go @@ -0,0 +1,1316 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +// karpenter_test.go contains unit tests for the Karpenter integration logic in +// karpenter.go and the Karpenter-specific paths in rollout.go and +// bootcnodepool_controller.go. Tests that require a live API server (envtest) +// live in rollout_karpenter_envtest_test.go. + +import ( + "context" + "testing" + + . "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/types" + "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" + karpenter "github.com/bootc-dev/bootc-operator/internal/karpenter" + testutil "github.com/bootc-dev/bootc-operator/test/util" +) + +// newTestKarpenterNodeClaim builds a karpenter.NodeClaim pre-seeded with the +// given name, annotations, and status map. Nil arguments are ignored. +func newTestKarpenterNodeClaim( + name string, + annotations map[string]string, + status map[string]interface{}, +) *karpenter.NodeClaim { + claim := karpenter.NewNodeClaim() + claim.SetName(name) + if annotations != nil { + claim.SetAnnotations(annotations) + } + if status != nil { + claim.Object["status"] = status + } + return claim +} + +// 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. +// +// karpenter.NodeClaim embeds unstructured.Unstructured, which the fake client +// handles without a registered GVK — objects are stored and retrieved by the +// GVK embedded in the object itself. If karpenter.NodeClaim is ever changed to +// a typed (non-unstructured) struct it would need to be added to the scheme. +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, Scheme: sc} + return r, counting +} + +func TestKarpenterRebootActive(t *testing.T) { + tests := []struct { + name string + taints []corev1.Taint + conditions []interface{} + }{ + { + name: "NodeClaim rebooting condition", + conditions: []interface{}{ + map[string]interface{}{"type": "Rebooting", "status": "True"}, + }, + }, + { + name: "Node rebooting taint", + taints: []corev1.Taint{{ + Key: karpenter.RebootingTaintKey, + Effect: karpenter.RebootingTaintEffect, + }}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + claim := newTestKarpenterNodeClaim( + "claim", + nil, + map[string]interface{}{"conditions": tt.conditions}, + ) + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: "node", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + Name: "claim", + UID: types.UID("claim-uid"), + }}, + }, Spec: corev1.NodeSpec{Taints: tt.taints}} + + r, _ := newFakeReconciler(node, claim) + kds, err := r.isKarpenterDisruptionActive(context.Background(), node) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(kds.active).To(BeTrue()) + }) + } +} + +// 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{ + karpenter.DoNotRepairAnnotationKey: "true", + karpenter.DoNotDisruptAnnotationKey: "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{ + karpenter.DoNotRepairAnnotationKey: "true", + karpenter.DoNotDisruptAnnotationKey: "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{ + karpenter.DoNotRepairAnnotationKey: "true", + karpenter.DoNotDisruptAnnotationKey: "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{ + karpenter.DoNotRepairAnnotationKey: "true", + karpenter.DoNotDisruptAnnotationKey: "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, nil) + 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(karpenter.DoNotRepairAnnotationKey)) + } else { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenter.DoNotRepairAnnotationKey)) + } + if tc.wantDisruptPresent { + g.Expect(finalNode.Annotations).To(HaveKey(karpenter.DoNotDisruptAnnotationKey)) + } else { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenter.DoNotDisruptAnnotationKey)) + } + }) + } +} + +// 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: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + 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{ + karpenter.DoNotRepairAnnotationKey: "true", + karpenter.DoNotDisruptAnnotationKey: "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}, + } + + objects := []client.Object{bn, k8sNode} + + if tc.karpenterOwned { + k8sNode.OwnerReferences = []metav1.OwnerReference{karpenterOwnerRef} + + claim := newTestKarpenterNodeClaim(karpenterOwnerRef.Name, nil, nil) + objects = append(objects, claim) + } + + r, counting := newFakeReconciler(objects...) + + err := r.assignRebootSlot(ctx, bn, k8sNode, nil) + 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(karpenter.DoNotRepairAnnotationKey)) + } else { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenter.DoNotRepairAnnotationKey)) + } + if tc.wantDisruptPresent { + g.Expect(finalNode.Annotations).To(HaveKey(karpenter.DoNotDisruptAnnotationKey)) + } else { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenter.DoNotDisruptAnnotationKey)) + } + + // 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)) + }) + } +} + +// TestKarpenterNodeClaimAnnotationLifecycle verifies that assignRebootSlot +// applies do-not-repair and do-not-disrupt to the NodeClaim when absent, and +// that freeRebootSlot removes only annotations the operator owns. +func TestKarpenterNodeClaimAnnotationLifecycle(t *testing.T) { + type annotationPair struct { + claimKey string + ownershipAnnotation string + } + annotations := []annotationPair{ + { + karpenter.DoNotDisruptAnnotationKey, + bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotDisrupt, + }, + { + karpenter.DoNotRepairAnnotationKey, + bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotRepair, + }, + } + + tests := []struct { + name string + preExistingAnnotation bool + wantDuring bool + wantOwned bool + wantAfter bool + }{ + { + name: "operator-owned annotation is applied and removed", + wantDuring: true, + wantOwned: true, + wantAfter: false, + }, + { + name: "pre-existing annotation is preserved", + preExistingAnnotation: true, + wantDuring: true, + wantOwned: false, + wantAfter: true, + }, + } + + for _, ann := range annotations { + for _, tc := range tests { + t.Run(ann.claimKey+"/"+tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + var claim *karpenter.NodeClaim + if tc.preExistingAnnotation { + claim = newTestKarpenterNodeClaim( + "test-claim", + map[string]string{ann.claimKey: "true"}, + nil, + ) + } else { + claim = newTestKarpenterNodeClaim("test-claim", nil, nil) + } + + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + Name: "test-claim", + UID: types.UID("test-claim-uid"), + }}, + }} + bn := testutil.NewNode("test-node", testImageDigestRefA) + + r, _ := newFakeReconciler(bn, node, claim) + g.Expect(r.assignRebootSlot(ctx, bn, node, nil)).To(Succeed()) + + duringClaim := newTestKarpenterNodeClaim("test-claim", nil, nil) + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-claim"}, duringClaim)). + To(Succeed()) + if tc.wantDuring { + g.Expect(duringClaim.GetAnnotations()). + To(HaveKeyWithValue(ann.claimKey, "true")) + } else { + g.Expect(duringClaim.GetAnnotations()).NotTo(HaveKey(ann.claimKey)) + } + + var duringBN bootcv1alpha1.BootcNode + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &duringBN)).To(Succeed()) + if tc.wantOwned { + g.Expect(duringBN.Annotations).To(HaveKey(ann.ownershipAnnotation)) + } else { + g.Expect(duringBN.Annotations).NotTo(HaveKey(ann.ownershipAnnotation)) + } + + g.Expect(r.freeRebootSlot(ctx, &duringBN, node, nil)).To(Succeed()) + + afterClaim := newTestKarpenterNodeClaim("test-claim", nil, nil) + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-claim"}, afterClaim)). + To(Succeed()) + if tc.wantAfter { + g.Expect(afterClaim.GetAnnotations()).To(HaveKeyWithValue(ann.claimKey, "true")) + } else { + g.Expect(afterClaim.GetAnnotations()).NotTo(HaveKey(ann.claimKey)) + } + }) + } + } +} + +// TestFreeRebootSlotNodeClaimGone verifies that freeRebootSlot succeeds cleanly +// when the NodeClaim no longer exists by the time the slot is freed. +// +// The scenario unfolds as follows: +// +// 1. The operator assigns the node a reboot slot and, because the node is +// Karpenter-managed, sets do-not-repair / do-not-disrupt annotations on +// both the corev1.Node and its backing NodeClaim to prevent Karpenter from +// interfering while the OS update is in progress. It records ownership of +// those annotations via AnnotationOwnsKarpenterNodeClaimDoNotRepair and +// AnnotationOwnsKarpenterNodeClaimDoNotDisrupt on the BootcNode. +// +// 2. The node reboots and comes back running the new image. The reconciler +// calls freeRebootSlot → restoreNodeState, which uses the NodeClaim lookup +// to remove the annotations it added. +// +// 3. Between step 1 and step 2 Karpenter forcefully replaced the node via a +// mechanism that the annotations do not block — for example a spot-instance +// interruption, node expiration (expireAfter), or manual NodeClaim +// deletion. (do-not-repair and do-not-disrupt only suppress Node Auto +// Repair and voluntary disruption respectively; they cannot prevent these +// forceful termination paths.) Karpenter deletes the NodeClaim and +// provisions a new one. The original NodeClaim therefore returns a 404 +// (IsNotFound) from the API server. +// +// getKarpenterNodeClaim treats a 404 (or no-match) as a nil return, so +// restoreNodeState skips the NodeClaim patch and returns nil. This test +// confirms the operator does not surface that 404 as an error, and that it +// still clears all slot/ownership annotations and uncordons the node, leaving +// state clean for the next reconcile cycle. +func TestFreeRebootSlotNodeClaimGone(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // Build a BootcNode with both NodeClaim ownership annotations set. + bn := testutil.NewNode("test-node", testImageDigestRefA) + bn.Annotations = map[string]string{ + bootcv1alpha1.AnnotationInRebootSlot: "", + bootcv1alpha1.AnnotationWasCordoned: "false", + bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotRepair: "true", + bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotDisrupt: "true", + } + + // Node has an owner reference pointing to a NodeClaim that no longer exists. + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + Name: "gone-claim", + UID: types.UID("gone-uid"), + }}, + }, + Spec: corev1.NodeSpec{Unschedulable: true}, + } + + // The NodeClaim is intentionally absent from the fake client. + r, _ := newFakeReconciler(bn, k8sNode) + g.Expect(r.freeRebootSlot(ctx, bn, k8sNode, nil)).To(Succeed()) + + // Cordon should be restored (was-cordoned=false). + var finalNode corev1.Node + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)).To(Succeed()) + g.Expect(finalNode.Spec.Unschedulable).To(BeFalse(), "node should be uncordoned") + + // All slot annotations should be cleared. + var finalBN bootcv1alpha1.BootcNode + g.Expect(r.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.AnnotationOwnsKarpenterNodeClaimDoNotRepair)) + g.Expect(finalBN.Annotations). + NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotDisrupt)) +} + +// TestDriveRolloutDefersWhenKarpenterRebootActive verifies that driveRollout +// skips slot assignment and drain for a node whose NodeClaim has +// Rebooting=True, leaving the BootcNode without an in-reboot-slot annotation. +func TestDriveRolloutDefersWhenKarpenterRebootActive(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // A staged BootcNode ready for a reboot slot. + bn := testutil.NewNode("staged-node", testImageDigestRefA, + testutil.WithBootedDigest(testDigestB), + testutil.WithNodeCondition( + bootcv1alpha1.NodeIdle, + metav1.ConditionFalse, + bootcv1alpha1.NodeReasonStaged, + ), + ) + + // The corresponding K8s Node is owned by a NodeClaim that is currently rebooting. + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "staged-node", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + Name: "active-claim", + UID: types.UID("active-claim-uid"), + }}, + }, + } + + // NodeClaim with Rebooting=True. + claim := newTestKarpenterNodeClaim("active-claim", nil, map[string]interface{}{ + "nodeName": "staged-node", + "conditions": []interface{}{ + map[string]interface{}{"type": "Rebooting", "status": "True"}, + }, + }) + + pool := testutil.NewPool("test-pool", testImageDigestRefA, testutil.WithWorkerSelector()) + pool.Status.TargetDigest = testDigestA + + r, _ := newFakeReconciler(bn, k8sNode, claim) + ownedNodes := map[string]*bootcv1alpha1.BootcNode{"staged-node": bn} + + rs, err := r.driveRollout(ctx, pool, ownedNodes) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rs).NotTo(BeNil()) + + // The node must not have been assigned a reboot slot. + var finalBN bootcv1alpha1.BootcNode + g.Expect(r.Get(ctx, client.ObjectKey{Name: "staged-node"}, &finalBN)).To(Succeed()) + g.Expect(finalBN.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationInRebootSlot), + "slot must not be assigned while Karpenter reboot is active") +} + +// 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{ + karpenter.DoNotRepairAnnotationKey: "true", + karpenter.DoNotDisruptAnnotationKey: "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{ + karpenter.DoNotRepairAnnotationKey: "true", + karpenter.DoNotDisruptAnnotationKey: "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, nil) + 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(karpenter.DoNotRepairAnnotationKey)) + } + if tc.ownsDisrupt { + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenter.DoNotDisruptAnnotationKey)) + } + + // 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)) + }) + } +} + +// TestIsKarpenterDisruptionActive verifies that isKarpenterDisruptionActive detects +// all relevant disruption signals: +// - Node taint karpenter.sh/rebooting:NoSchedule +// - Node taint karpenter.sh/disrupted:NoSchedule +// - Node DeletionTimestamp set +// - NodeClaim DeletionTimestamp set +// - NodeClaim condition DisruptionReason=True +// - NodeClaim condition InstanceTerminating=True +// - NodeClaim missing but owner ref present → claimMissing=true +func TestIsKarpenterDisruptionActive(t *testing.T) { + now := metav1.Now() + + tests := []struct { + name string + // node setup + nodeTaints []corev1.Taint + nodeDeletionTS *metav1.Time + nodeKarpenterOwned bool + // claim setup + claimConditions []interface{} + claimDeletionTS *metav1.Time + claimAbsent bool // NodeClaim owner ref present but object 404 + // expected + wantActive bool + wantClaimMissing bool + }{ + { + name: "idle node, no owner ref: inactive", + wantActive: false, + }, + { + name: "rebooting taint: active", + nodeKarpenterOwned: true, + nodeTaints: []corev1.Taint{{ + Key: karpenter.RebootingTaintKey, + Effect: karpenter.RebootingTaintEffect, + }}, + wantActive: true, + }, + { + name: "disrupted taint: active", + nodeKarpenterOwned: true, + nodeTaints: []corev1.Taint{{ + Key: karpenter.DisruptedTaintKey, + Effect: karpenter.RebootingTaintEffect, + }}, + wantActive: true, + }, + { + name: "node deletion timestamp: active", + nodeKarpenterOwned: true, + nodeDeletionTS: &now, + wantActive: true, + }, + { + name: "NodeClaim deletion timestamp: active", + nodeKarpenterOwned: true, + claimDeletionTS: &now, + wantActive: true, + }, + { + name: "NodeClaim Rebooting=True condition: active", + nodeKarpenterOwned: true, + claimConditions: []interface{}{ + map[string]interface{}{"type": "Rebooting", "status": "True"}, + }, + wantActive: true, + }, + { + name: "NodeClaim DisruptionReason=True condition: active", + nodeKarpenterOwned: true, + claimConditions: []interface{}{ + map[string]interface{}{"type": "DisruptionReason", "status": "True"}, + }, + wantActive: true, + }, + { + name: "NodeClaim InstanceTerminating=True condition: active", + nodeKarpenterOwned: true, + claimConditions: []interface{}{ + map[string]interface{}{"type": "InstanceTerminating", "status": "True"}, + }, + wantActive: true, + }, + { + name: "NodeClaim condition status False: not active", + nodeKarpenterOwned: true, + claimConditions: []interface{}{ + map[string]interface{}{"type": "DisruptionReason", "status": "False"}, + }, + wantActive: false, + }, + { + name: "NodeClaim absent but owner ref present: claimMissing=true", + nodeKarpenterOwned: true, + claimAbsent: true, + wantActive: true, + wantClaimMissing: true, + }, + { + name: "idle Karpenter node, no conditions: not active", + nodeKarpenterOwned: true, + wantActive: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + // Kubernetes requires a finalizer for DeletionTimestamp to be set. + Finalizers: func() []string { + if tc.nodeDeletionTS != nil { + return []string{"test/finalizer"} + } + return nil + }(), + DeletionTimestamp: tc.nodeDeletionTS, + }, + Spec: corev1.NodeSpec{Taints: tc.nodeTaints}, + } + + var objs []client.Object + objs = append(objs, node) + + if tc.nodeKarpenterOwned && !tc.claimAbsent { + claim := newTestKarpenterNodeClaim("test-claim", nil, nil) + if len(tc.claimConditions) > 0 { + claim.Object["status"] = map[string]interface{}{ + "conditions": tc.claimConditions, + } + } + if tc.claimDeletionTS != nil { + claim.SetDeletionTimestamp(tc.claimDeletionTS) + claim.SetFinalizers([]string{"test/finalizer"}) + } + objs = append(objs, claim) + } + + if tc.nodeKarpenterOwned { + node.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + Name: "test-claim", + UID: types.UID("test-claim-uid"), + }} + } + + r, _ := newFakeReconciler(objs...) + kds, err := r.isKarpenterDisruptionActive(ctx, node) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(kds.active).To(Equal(tc.wantActive), "active") + g.Expect(kds.claimMissing).To(Equal(tc.wantClaimMissing), "claimMissing") + }) + } +} + +// TestAssignRebootSlotAnnotationValueCheck verifies that assignRebootSlot +// takes ownership of and overwrites a Karpenter annotation that already exists +// but has a value other than "true" (e.g. "false"). Karpenter only honors the +// literal value "true"; any other value leaves the node unprotected. +func TestAssignRebootSlotAnnotationValueCheck(t *testing.T) { + tests := []struct { + name string + existingValue string // value already set on the Node annotation + wantOwned bool // does the BootcNode get an ownership annotation? + }{ + { + name: "annotation absent: operator takes ownership", + wantOwned: true, + }, + { + name: "annotation already true: operator does not take ownership", + existingValue: "true", + wantOwned: false, + }, + { + name: "annotation is false: operator overwrites and takes ownership", + existingValue: "false", + wantOwned: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + nodeAnnotations := map[string]string{} + if tc.existingValue != "" { + nodeAnnotations[karpenter.DoNotDisruptAnnotationKey] = tc.existingValue + nodeAnnotations[karpenter.DoNotRepairAnnotationKey] = tc.existingValue + } + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Annotations: nodeAnnotations, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + Name: "test-claim", + UID: types.UID("test-claim-uid"), + }}, + }, + } + claim := newTestKarpenterNodeClaim("test-claim", nil, nil) + bn := testutil.NewNode("test-node", testImageDigestRefA) + + r, _ := newFakeReconciler(bn, k8sNode, claim) + g.Expect(r.assignRebootSlot(ctx, bn, k8sNode, nil)).To(Succeed()) + + // The Node annotation must always be "true" after assignment. + var finalNode corev1.Node + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)).To(Succeed()) + g.Expect(finalNode.Annotations[karpenter.DoNotDisruptAnnotationKey]).To(Equal("true")) + g.Expect(finalNode.Annotations[karpenter.DoNotRepairAnnotationKey]).To(Equal("true")) + + // Ownership annotation on BootcNode. + var finalBN bootcv1alpha1.BootcNode + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalBN)).To(Succeed()) + if tc.wantOwned { + g.Expect(finalBN.Annotations). + To(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt)) + g.Expect(finalBN.Annotations). + To(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair)) + } else { + g.Expect(finalBN.Annotations). + NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt)) + g.Expect(finalBN.Annotations). + NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair)) + } + }) + } +} + +// TestDriveRolloutDoesNotProtectStaging verifies that driveRollout does not +// apply Karpenter protection until a reboot slot is assigned. +func TestDriveRolloutDoesNotProtectStaging(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // A staging BootcNode (image pull in progress, not yet staged). + bn := testutil.NewNode("staging-node", testImageDigestRefA, + testutil.WithBootedDigest(testDigestB), + testutil.WithNodeCondition( + bootcv1alpha1.NodeIdle, + metav1.ConditionFalse, + bootcv1alpha1.NodeReasonStaging, + ), + ) + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "staging-node", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + Name: "staging-claim", + UID: types.UID("staging-claim-uid"), + }}, + }, + } + claim := newTestKarpenterNodeClaim( + "staging-claim", + nil, + map[string]interface{}{"nodeName": "staging-node"}, + ) + + pool := testutil.NewPool("test-pool", testImageDigestRefA, testutil.WithWorkerSelector()) + pool.Status.TargetDigest = testDigestA + + r, _ := newFakeReconciler(bn, k8sNode, claim) + ownedNodes := map[string]*bootcv1alpha1.BootcNode{"staging-node": bn} + + _, err := r.driveRollout(ctx, pool, ownedNodes) + g.Expect(err).NotTo(HaveOccurred()) + + // A staging node must not have Karpenter protection applied. + var finalNode corev1.Node + g.Expect(r.Get(ctx, client.ObjectKey{Name: "staging-node"}, &finalNode)).To(Succeed()) + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenter.DoNotRepairAnnotationKey), + "do-not-repair must not be set during staging") + g.Expect(finalNode.Annotations).NotTo(HaveKey(karpenter.DoNotDisruptAnnotationKey), + "do-not-disrupt must not be set during staging") + + finalClaim := newTestKarpenterNodeClaim("staging-claim", nil, nil) + g.Expect(r.Get(ctx, client.ObjectKey{Name: "staging-claim"}, finalClaim)).To(Succeed()) + g.Expect(finalClaim.GetAnnotations()).NotTo(HaveKey(karpenter.DoNotRepairAnnotationKey), + "NodeClaim do-not-repair must not be set during staging") + g.Expect(finalClaim.GetAnnotations()).NotTo(HaveKey(karpenter.DoNotDisruptAnnotationKey), + "NodeClaim do-not-disrupt must not be set during staging") + + // The node must not have been assigned a reboot slot (it's still staging). + var finalBN bootcv1alpha1.BootcNode + g.Expect(r.Get(ctx, client.ObjectKey{Name: "staging-node"}, &finalBN)).To(Succeed()) + g.Expect(finalBN.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationInRebootSlot), + "slot must not be assigned during staging") + g.Expect(finalBN.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotRepair)) + g.Expect(finalBN.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterDoNotDisrupt)) + g.Expect(finalBN.Annotations). + NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotRepair)) + g.Expect(finalBN.Annotations). + NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsKarpenterNodeClaimDoNotDisrupt)) +} + +// TestDriveRolloutDefersWhenKarpenterDisruptionActive verifies that driveRollout +// defers slot assignment when Karpenter has committed a disruption (not just a +// reboot). +func TestDriveRolloutDefersWhenKarpenterDisruptionActive(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // A staged BootcNode ready for a reboot slot. + bn := testutil.NewNode("staged-node", testImageDigestRefA, + testutil.WithBootedDigest(testDigestB), + testutil.WithNodeCondition( + bootcv1alpha1.NodeIdle, + metav1.ConditionFalse, + bootcv1alpha1.NodeReasonStaged, + ), + ) + + // The Node has the disrupted taint (Karpenter voluntary disruption committed). + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "staged-node", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + Name: "active-claim", + UID: types.UID("active-claim-uid"), + }}, + }, + Spec: corev1.NodeSpec{ + Taints: []corev1.Taint{{ + Key: karpenter.DisruptedTaintKey, + Effect: karpenter.RebootingTaintEffect, + }}, + }, + } + claim := newTestKarpenterNodeClaim( + "active-claim", + nil, + map[string]interface{}{"nodeName": "staged-node"}, + ) + + pool := testutil.NewPool("test-pool", testImageDigestRefA, testutil.WithWorkerSelector()) + pool.Status.TargetDigest = testDigestA + + r, _ := newFakeReconciler(bn, k8sNode, claim) + ownedNodes := map[string]*bootcv1alpha1.BootcNode{"staged-node": bn} + + rs, err := r.driveRollout(ctx, pool, ownedNodes) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rs).NotTo(BeNil()) + + // The node must not have been assigned a reboot slot. + var finalBN bootcv1alpha1.BootcNode + g.Expect(r.Get(ctx, client.ObjectKey{Name: "staged-node"}, &finalBN)).To(Succeed()) + g.Expect(finalBN.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationInRebootSlot), + "slot must not be assigned while Karpenter disruption is active") +} + +// TestDriveRolloutDefersWhenNodeClaimAbsent verifies that driveRollout defers +// slot assignment when the Karpenter NodeClaim owner reference is present but +// the NodeClaim object is missing (404). +func TestDriveRolloutDefersWhenNodeClaimAbsent(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + bn := testutil.NewNode("staged-node", testImageDigestRefA, + testutil.WithBootedDigest(testDigestB), + testutil.WithNodeCondition( + bootcv1alpha1.NodeIdle, + metav1.ConditionFalse, + bootcv1alpha1.NodeReasonStaged, + ), + ) + + // The Node has a Karpenter owner reference, but the NodeClaim does not exist. + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "staged-node", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + Name: "missing-claim", + UID: types.UID("missing-claim-uid"), + }}, + }, + } + // The NodeClaim is intentionally absent from the fake client. + + pool := testutil.NewPool("test-pool", testImageDigestRefA, testutil.WithWorkerSelector()) + pool.Status.TargetDigest = testDigestA + + r, _ := newFakeReconciler(bn, k8sNode) + ownedNodes := map[string]*bootcv1alpha1.BootcNode{"staged-node": bn} + + rs, err := r.driveRollout(ctx, pool, ownedNodes) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rs).NotTo(BeNil()) + + // Slot must not have been assigned — the absent NodeClaim is unsafe. + var finalBN bootcv1alpha1.BootcNode + g.Expect(r.Get(ctx, client.ObjectKey{Name: "staged-node"}, &finalBN)).To(Succeed()) + g.Expect(finalBN.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationInRebootSlot), + "slot must not be assigned when NodeClaim owner is missing") + + // A requeue should be scheduled to retry. + g.Expect(rs.requeueAfter).To(BeNumerically(">", 0), + "requeueAfter must be set to retry after missing NodeClaim") +} + +// TestDriveRolloutKarpenterRequeueFallback verifies that driveRollout sets a +// periodic requeue when Karpenter-owned nodes are active (staging or staged), +// to ensure NodeClaim status changes are eventually observed even if the +// NodeClaim watch is not registered. +func TestDriveRolloutKarpenterRequeueFallback(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // A staged Karpenter-owned node. It has no protection annotations until the + // reboot slot is assigned. + bn := testutil.NewNode("staged-node", testImageDigestRefA, + testutil.WithBootedDigest(testDigestB), + testutil.WithNodeCondition( + bootcv1alpha1.NodeIdle, + metav1.ConditionFalse, + bootcv1alpha1.NodeReasonStaged, + ), + ) + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "staged-node", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + Name: "test-claim", + UID: types.UID("test-claim-uid"), + }}, + }, + } + claim := newTestKarpenterNodeClaim( + "test-claim", + nil, + map[string]interface{}{"nodeName": "staged-node"}, + ) + + pool := testutil.NewPool("test-pool", testImageDigestRefA, testutil.WithWorkerSelector()) + pool.Status.TargetDigest = testDigestA + + r, _ := newFakeReconciler(bn, k8sNode, claim) + ownedNodes := map[string]*bootcv1alpha1.BootcNode{"staged-node": bn} + + rs, err := r.driveRollout(ctx, pool, ownedNodes) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rs).NotTo(BeNil()) + + // requeueAfter must be set to the fallback interval. + g.Expect(rs.requeueAfter).To(Equal(karpenterMissingClaimRequeue), + "requeueAfter must be set to the Karpenter fallback requeue interval") +} + +func TestOwnedAnnotationHelpers(t *testing.T) { + g := NewWithT(t) + owner := &bootcv1alpha1.BootcNode{} + target := &corev1.Node{} + const ( + ownerKey = "operator.example.com/owns-annotation" + targetKey = "example.com/annotation" + ) + + target.SetAnnotations(map[string]string{targetKey: "false"}) + g.Expect(ensureOwnedAnnotation(owner, target, ownerKey, targetKey, "true")).To(BeTrue()) + g.Expect(owner.GetAnnotations()).To(HaveKeyWithValue(ownerKey, "true")) + g.Expect(target.GetAnnotations()).To(HaveKeyWithValue(targetKey, "true")) + + // Repeating the operation does not change either object. + g.Expect(ensureOwnedAnnotation(owner, target, ownerKey, targetKey, "true")).To(BeFalse()) + + g.Expect(removeOwnedAnnotation(owner, target, ownerKey, targetKey)).To(BeTrue()) + g.Expect(target.GetAnnotations()).NotTo(HaveKey(targetKey)) + g.Expect(removeOwnedAnnotation(owner, target, ownerKey, targetKey)).To(BeFalse()) +} + +func TestRemoveOwnedAnnotationRequiresOwnership(t *testing.T) { + g := NewWithT(t) + owner := &bootcv1alpha1.BootcNode{} + target := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + "example.com/annotation": "true", + }}} + + g.Expect(removeOwnedAnnotation( + owner, + target, + "operator.example.com/owns-annotation", + "example.com/annotation", + )).To(BeFalse()) + g.Expect(target.GetAnnotations()).To(HaveKey("example.com/annotation")) +} + +func TestRemoveOwnedAnnotationRemovesExternallyChangedValue(t *testing.T) { + g := NewWithT(t) + owner := &bootcv1alpha1.BootcNode{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + "operator.example.com/owns-annotation": "true", + }}} + target := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + "example.com/annotation": "external-value", + }}} + + g.Expect(removeOwnedAnnotation( + owner, + target, + "operator.example.com/owns-annotation", + "example.com/annotation", + )).To(BeTrue()) + g.Expect(target.GetAnnotations()).NotTo(HaveKey("example.com/annotation")) +} diff --git a/internal/controller/rollout_karpenter_envtest_test.go b/internal/controller/rollout_karpenter_envtest_test.go new file mode 100644 index 00000000..b181179b --- /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. + +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" + karpenter "github.com/bootc-dev/bootc-operator/internal/karpenter" + 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{ + karpenter.DoNotRepairAnnotationKey: "true", + karpenter.DoNotDisruptAnnotationKey: "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{ + karpenter.DoNotRepairAnnotationKey: "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: karpenter.NodeClaimAPIVersion, + Kind: karpenter.NodeClaimKind, + 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(karpenter.DoNotRepairAnnotationKey)) + if wantRepair { + repair = HaveKey(karpenter.DoNotRepairAnnotationKey) + } + disrupt := Not(HaveKey(karpenter.DoNotDisruptAnnotationKey)) + if wantDisrupt { + disrupt = HaveKey(karpenter.DoNotDisruptAnnotationKey) + } + return And(repair, disrupt) +} From c311a16a36997f4dd6d4fc1ab059dd38cdd9c14c Mon Sep 17 00:00:00 2001 From: Zack Zlotnik Date: Thu, 29 Oct 2026 20:28:34 +0000 Subject: [PATCH 4/5] test: cover OS initialization taint lifecycle Add coverage for the bootc.dev/os-not-ready startup taint lifecycle. Assisted-by: AI Signed-off-by: Zack Zlotnik --- .../controller/os_not_ready_taint_test.go | 503 ++++++++++++++++++ 1 file changed, 503 insertions(+) create mode 100644 internal/controller/os_not_ready_taint_test.go diff --git a/internal/controller/os_not_ready_taint_test.go b/internal/controller/os_not_ready_taint_test.go new file mode 100644 index 00000000..dc8abe0a --- /dev/null +++ b/internal/controller/os_not_ready_taint_test.go @@ -0,0 +1,503 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +// os_not_ready_taint_test.go contains unit tests for the bootc.dev/os-not-ready +// startup taint feature: ownership recording at join time, taint removal via the +// reboot-slot path (restoreNodeState), and taint removal via the already-up-to-date +// path (removeOsNotReadyTaintAndAnnotation / freeCompletedSlots). + +import ( + "context" + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + testutil "github.com/bootc-dev/bootc-operator/test/util" +) + +// osNotReadyTaint returns the well-known bootc.dev/os-not-ready:NoSchedule taint. +func osNotReadyTaint() corev1.Taint { + return corev1.Taint{ + Key: bootcv1alpha1.TaintKeyOsNotReady, + Effect: corev1.TaintEffectNoSchedule, + } +} + +// TestNodeHasOsNotReadyTaint verifies the taint detection helper. +func TestNodeHasOsNotReadyTaint(t *testing.T) { + tests := []struct { + name string + taints []corev1.Taint + want bool + }{ + { + name: "taint present: detected", + taints: []corev1.Taint{osNotReadyTaint()}, + want: true, + }, + { + name: "no taints: not detected", + taints: nil, + want: false, + }, + { + name: "different key same effect: not detected", + taints: []corev1.Taint{{Key: "other.taint/key", Effect: corev1.TaintEffectNoSchedule}}, + want: false, + }, + { + name: "correct key wrong effect: not detected", + taints: []corev1.Taint{ + {Key: bootcv1alpha1.TaintKeyOsNotReady, Effect: corev1.TaintEffectNoExecute}, + }, + want: false, + }, + { + name: "taint among others: detected", + taints: []corev1.Taint{ + {Key: "other/taint", Effect: corev1.TaintEffectNoSchedule}, + osNotReadyTaint(), + }, + want: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + node := &corev1.Node{Spec: corev1.NodeSpec{Taints: tc.taints}} + NewWithT(t).Expect(nodeHasOsNotReadyTaint(node)).To(Equal(tc.want)) + }) + } +} + +func TestNodePredicatesDetectOsNotReadyTaintChanges(t *testing.T) { + g := NewWithT(t) + oldNode := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "test-node"}} + newNode := oldNode.DeepCopy() + newNode.Spec.Taints = []corev1.Taint{osNotReadyTaint()} + + g.Expect(nodePredicates().Update(event.UpdateEvent{ + ObjectOld: oldNode, + ObjectNew: newNode, + })).To(BeTrue()) + g.Expect(nodePredicates().Update(event.UpdateEvent{ + ObjectOld: newNode, + ObjectNew: oldNode, + })).To(BeTrue()) +} + +// TestCreateBootcNodeRecordsOsNotReadyTaintOwnership verifies that createBootcNode +// sets AnnotationOwnsOsNotReadyTaint on the BootcNode when the K8s Node carries +// the startup taint, and does not set it when the taint is absent. +func TestCreateBootcNodeRecordsOsNotReadyTaintOwnership(t *testing.T) { + tests := []struct { + name string + hasTaint bool + wantOwned bool + }{ + { + name: "node has startup taint: ownership recorded", + hasTaint: true, + wantOwned: true, + }, + { + name: "node has no startup taint: ownership not recorded", + hasTaint: false, + wantOwned: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + pool := testutil.NewPool( + "test-pool", + testImageDigestRefA, + testutil.WithWorkerSelector(), + ) + pool.Status.TargetDigest = testDigestA + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "test-node"}, + } + if tc.hasTaint { + k8sNode.Spec.Taints = []corev1.Taint{osNotReadyTaint()} + } + + // newFakeReconciler builds a reconciler with scheme already set up. + r, _ := newFakeReconciler(k8sNode) + + bn, err := r.createBootcNode(ctx, pool, k8sNode) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(bn).NotTo(BeNil()) + + if tc.wantOwned { + g.Expect(bn.Annotations).To(HaveKey(bootcv1alpha1.AnnotationOwnsOsNotReadyTaint)) + } else { + g.Expect(bn.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsOsNotReadyTaint)) + } + }) + } +} + +// TestRestoreNodeStateRemovesOsNotReadyTaint verifies that restoreNodeState +// removes the os-not-ready taint when the BootcNode carries the ownership +// annotation, and leaves it alone when the annotation is absent. +func TestRestoreNodeStateRemovesOsNotReadyTaint(t *testing.T) { + tests := []struct { + name string + ownsTaint bool + taintOnNode bool + + wantTaintPresent bool + wantNodePatches int + }{ + { + name: "owns taint and taint is on node: taint removed in same patch as uncordon", + ownsTaint: true, + taintOnNode: true, + wantTaintPresent: false, + wantNodePatches: 1, + }, + { + name: "owns taint but taint already gone: no node patch", + ownsTaint: true, + taintOnNode: false, + wantTaintPresent: false, + wantNodePatches: 0, + }, + { + name: "does not own taint: taint left in place, no patch", + ownsTaint: false, + taintOnNode: true, + wantTaintPresent: true, + wantNodePatches: 0, + }, + { + name: "neither taint nor ownership: no-op", + ownsTaint: false, + taintOnNode: false, + wantTaintPresent: false, + wantNodePatches: 0, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + bnAnnotations := map[string]string{} + if tc.ownsTaint { + bnAnnotations[bootcv1alpha1.AnnotationOwnsOsNotReadyTaint] = "true" + } + bn := testutil.NewNode("test-node", testImageDigestRefA) + bn.Annotations = bnAnnotations + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "test-node"}, + } + if tc.taintOnNode { + k8sNode.Spec.Taints = []corev1.Taint{osNotReadyTaint()} + } + + r, counting := newFakeReconciler(bn, k8sNode) + g.Expect(r.restoreNodeState(ctx, bn, k8sNode, nil)).To(Succeed()) + + g.Expect(counting.nodePatches).To(Equal(tc.wantNodePatches), "node patch count") + + var finalNode corev1.Node + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)).To(Succeed()) + hasTaint := nodeHasOsNotReadyTaint(&finalNode) + g.Expect(hasTaint).To(Equal(tc.wantTaintPresent), "taint presence") + }) + } +} + +// TestRestoreNodeStateRemovesOsNotReadyTaintWithUncordon verifies that when both +// the os-not-ready taint removal and an uncordon are needed, they are combined +// into a single Node patch. +func TestRestoreNodeStateRemovesOsNotReadyTaintWithUncordon(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + bn := testutil.NewNode("test-node", testImageDigestRefA) + bn.Annotations = map[string]string{ + bootcv1alpha1.AnnotationOwnsOsNotReadyTaint: "true", + bootcv1alpha1.AnnotationWasCordoned: "false", + } + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "test-node"}, + Spec: corev1.NodeSpec{ + Unschedulable: true, + Taints: []corev1.Taint{osNotReadyTaint()}, + }, + } + + r, counting := newFakeReconciler(bn, k8sNode) + g.Expect(r.restoreNodeState(ctx, bn, k8sNode, nil)).To(Succeed()) + + // Taint removal + uncordon must be a single patch. + g.Expect(counting.nodePatches).To(Equal(1), "taint removal and uncordon should be one patch") + + var finalNode corev1.Node + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)).To(Succeed()) + g.Expect(nodeHasOsNotReadyTaint(&finalNode)).To(BeFalse(), "taint should be removed") + g.Expect(finalNode.Spec.Unschedulable).To(BeFalse(), "node should be uncordoned") +} + +func TestRemoveBootcNodePreservesUnverifiedOsNotReadyTaint(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + bn := testutil.NewNode("test-node", testImageDigestRefA) + bn.Annotations = map[string]string{ + bootcv1alpha1.AnnotationOwnsOsNotReadyTaint: "true", + } + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "test-node"}, + Spec: corev1.NodeSpec{Taints: []corev1.Taint{osNotReadyTaint()}}, + } + + r, _ := newFakeReconciler(bn, k8sNode) + g.Expect(r.removeBootcNode(ctx, bn)).To(Succeed()) + + var finalNode corev1.Node + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)).To(Succeed()) + g.Expect(nodeHasOsNotReadyTaint(&finalNode)).To(BeTrue()) + var finalBN bootcv1alpha1.BootcNode + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalBN)). + To(MatchError(apierrors.IsNotFound, "IsNotFound")) +} + +// TestFreeRebootSlotClearsOsNotReadyOwnershipAnnotation verifies that +// freeRebootSlot removes the AnnotationOwnsOsNotReadyTaint ownership +// annotation from the BootcNode as part of slot cleanup, in addition to +// removing the taint from the Node via restoreNodeState. +func TestFreeRebootSlotClearsOsNotReadyOwnershipAnnotation(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + bn := testutil.NewNode("test-node", testImageDigestRefA) + bn.Annotations = map[string]string{ + bootcv1alpha1.AnnotationInRebootSlot: "", + bootcv1alpha1.AnnotationWasCordoned: "false", + bootcv1alpha1.AnnotationOwnsOsNotReadyTaint: "true", + } + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "test-node"}, + Spec: corev1.NodeSpec{ + Unschedulable: true, + Taints: []corev1.Taint{osNotReadyTaint()}, + }, + } + + r, _ := newFakeReconciler(bn, k8sNode) + g.Expect(r.freeRebootSlot(ctx, bn, k8sNode, nil)).To(Succeed()) + + // Taint must be gone from the Node. + var finalNode corev1.Node + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)).To(Succeed()) + g.Expect(nodeHasOsNotReadyTaint(&finalNode)).To(BeFalse(), "taint should be removed") + + // All slot and ownership annotations must be gone from the BootcNode. + var finalBN bootcv1alpha1.BootcNode + g.Expect(r.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.AnnotationOwnsOsNotReadyTaint)) +} + +// TestFreeCompletedSlotsRemovesTaintForAlreadyUpToDateNode verifies the +// already-up-to-date path: a node that was running the target image when it +// joined (never held a reboot slot) has its startup taint removed and its +// ownership annotation cleared once it is UpToDate and Ready. +func TestFreeCompletedSlotsRemovesTaintForAlreadyUpToDateNode(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // BootcNode is UpToDate (booted digest matches desired), owns the taint, + // but has never held a reboot slot. + bn := testutil.NewNode( + "test-node", + testImageDigestRefA, + testutil.WithBootedDigest(testDigestA), + testutil.WithNodeCondition( + bootcv1alpha1.NodeIdle, + metav1.ConditionTrue, + bootcv1alpha1.NodeReasonIdle, + ), + testutil.WithNodeAnnotation(bootcv1alpha1.AnnotationOwnsOsNotReadyTaint, "true"), + ) + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "test-node"}, + Spec: corev1.NodeSpec{ + Taints: []corev1.Taint{osNotReadyTaint()}, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + }, + }, + } + + r, _ := newFakeReconciler(bn, k8sNode) + + rs := &rolloutState{upToDate: []*bootcv1alpha1.BootcNode{bn}} + g.Expect(r.freeCompletedSlots(ctx, rs)).To(Succeed()) + + // Taint must be removed from the Node. + var finalNode corev1.Node + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)).To(Succeed()) + g.Expect(nodeHasOsNotReadyTaint(&finalNode)).To(BeFalse(), "taint should be removed") + + // Ownership annotation must be cleared from the BootcNode. + var finalBN bootcv1alpha1.BootcNode + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalBN)).To(Succeed()) + g.Expect(finalBN.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsOsNotReadyTaint)) + + // The node was never in a reboot slot; slot count must be unchanged. + g.Expect(rs.occupiedSlots).To(Equal(0)) +} + +// TestFreeCompletedSlotsHoldsTaintWhenNotReady verifies that the taint is not +// removed for an UpToDate node that is not yet Ready, even if the ownership +// annotation is present. +func TestFreeCompletedSlotsHoldsTaintWhenNotReady(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + bn := testutil.NewNode( + "test-node", + testImageDigestRefA, + testutil.WithBootedDigest(testDigestA), + testutil.WithNodeCondition( + bootcv1alpha1.NodeIdle, + metav1.ConditionTrue, + bootcv1alpha1.NodeReasonIdle, + ), + testutil.WithNodeAnnotation(bootcv1alpha1.AnnotationOwnsOsNotReadyTaint, "true"), + ) + + // Node is not Ready. + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "test-node"}, + Spec: corev1.NodeSpec{Taints: []corev1.Taint{osNotReadyTaint()}}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionFalse}, + }, + }, + } + + r, counting := newFakeReconciler(bn, k8sNode) + rs := &rolloutState{upToDate: []*bootcv1alpha1.BootcNode{bn}} + g.Expect(r.freeCompletedSlots(ctx, rs)).To(Succeed()) + + // No patches should have been issued. + g.Expect(counting.nodePatches).To(Equal(0), "taint must not be removed when node is not Ready") + + // Taint must still be present. + var finalNode corev1.Node + g.Expect(r.Get(ctx, client.ObjectKey{Name: "test-node"}, &finalNode)).To(Succeed()) + g.Expect(nodeHasOsNotReadyTaint(&finalNode)). + To(BeTrue(), "taint should be preserved until Ready") +} + +// TestFreeCompletedSlotsNoOpWhenNoTaintOwnership verifies that freeCompletedSlots +// skips UpToDate nodes that carry neither a reboot slot nor the taint ownership +// annotation, issuing no API calls. +func TestFreeCompletedSlotsNoOpWhenNoTaintOwnership(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // UpToDate node with no slot and no taint ownership. + bn := testutil.NewNode( + "test-node", + testImageDigestRefA, + testutil.WithBootedDigest(testDigestA), + testutil.WithNodeCondition( + bootcv1alpha1.NodeIdle, + metav1.ConditionTrue, + bootcv1alpha1.NodeReasonIdle, + ), + ) + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "test-node"}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + }, + }, + } + + r, counting := newFakeReconciler(bn, k8sNode) + rs := &rolloutState{upToDate: []*bootcv1alpha1.BootcNode{bn}} + g.Expect(r.freeCompletedSlots(ctx, rs)).To(Succeed()) + + g.Expect(counting.nodePatches).To(Equal(0), "no patches should be issued") +} + +// TestDriveRolloutRemovesTaintForAlreadyUpToDateNode is an integration-style +// unit test verifying that driveRollout removes the startup taint for a node +// that is UpToDate and Ready on its first reconcile (never needed a reboot slot). +func TestDriveRolloutRemovesTaintForAlreadyUpToDateNode(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // BootcNode is UpToDate (booted digest matches pool target), owns the taint. + bn := testutil.NewNode( + "up-to-date-node", + testImageDigestRefA, + testutil.WithBootedDigest(testDigestA), + testutil.WithNodeCondition( + bootcv1alpha1.NodeIdle, + metav1.ConditionTrue, + bootcv1alpha1.NodeReasonIdle, + ), + testutil.WithNodeAnnotation(bootcv1alpha1.AnnotationOwnsOsNotReadyTaint, "true"), + ) + + k8sNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "up-to-date-node"}, + Spec: corev1.NodeSpec{Taints: []corev1.Taint{osNotReadyTaint()}}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + }, + }, + } + + pool := testutil.NewPool("test-pool", testImageDigestRefA, testutil.WithWorkerSelector()) + pool.Status.TargetDigest = testDigestA + + r, _ := newFakeReconciler(bn, k8sNode) + ownedNodes := map[string]*bootcv1alpha1.BootcNode{"up-to-date-node": bn} + + _, err := r.driveRollout(ctx, pool, ownedNodes) + g.Expect(err).NotTo(HaveOccurred()) + + // Taint must be gone from the Node. + var finalNode corev1.Node + g.Expect(r.Get(ctx, client.ObjectKey{Name: "up-to-date-node"}, &finalNode)).To(Succeed()) + g.Expect(nodeHasOsNotReadyTaint(&finalNode)).To(BeFalse(), + "startup taint must be removed for a node that is already up-to-date") + + // Ownership annotation must be cleared from the BootcNode. + var finalBN bootcv1alpha1.BootcNode + g.Expect(r.Get(ctx, client.ObjectKey{Name: "up-to-date-node"}, &finalBN)).To(Succeed()) + g.Expect(finalBN.Annotations).NotTo(HaveKey(bootcv1alpha1.AnnotationOwnsOsNotReadyTaint)) +} From 52cf2cf434d2cb056b891531d43e871811138ef0 Mon Sep 17 00:00:00 2001 From: Zack Zlotnik Date: Thu, 29 Oct 2026 20:28:37 +0000 Subject: [PATCH 5/5] docs: document Karpenter coordination Document Karpenter integration and OS initialization taint behavior. Assisted-by: AI Signed-off-by: Zack Zlotnik --- docs/src/SUMMARY.md | 2 + docs/src/operations/karpenter.md | 297 ++++++++++++++++++++++ docs/src/operations/os-not-ready-taint.md | 65 +++++ 3 files changed, 364 insertions(+) create mode 100644 docs/src/operations/karpenter.md create mode 100644 docs/src/operations/os-not-ready-taint.md diff --git a/docs/src/SUMMARY.md b/docs/src/SUMMARY.md index fbfaf433..1eb8249d 100644 --- a/docs/src/SUMMARY.md +++ b/docs/src/SUMMARY.md @@ -8,3 +8,5 @@ - [Helm Chart](helm.md) - [Operations](operations/index.md) - [Managing a pool](operations/pool.md) + - [OS initialization taint](operations/os-not-ready-taint.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..8414ea12 --- /dev/null +++ b/docs/src/operations/karpenter.md @@ -0,0 +1,297 @@ +# Karpenter integration + +bootc-operator is aware of [Karpenter](https://karpenter.sh/) and coordinates +with it during node updates to prevent unnecessary node replacement, +voluntary disruption, and concurrent reboot handling. + +## 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 +when assigning a reboot slot, and keeps them through node reboot and health +verification. Where the associated `NodeClaim` exists, it also sets the +annotations there. This is best-effort coordination, not a distributed lock. + +## How it works + +Karpenter protection is applied when a Karpenter-owned node is assigned a +reboot slot. To do this, bootc-operator sets `do-not-repair` and +`do-not-disrupt` on the Node and, where available, the associated NodeClaim. +This protection is not applied during the staging process. This protection +persists through drain, reboot, and post-boot health verification. + +Before assigning a reboot slot or starting a drain, the controller checks +whether Karpenter has already committed any lifecycle operation that would +conflict with the bootc update. It checks: + +- Node taints `karpenter.sh/rebooting:NoSchedule` and + `karpenter.sh/disrupted:NoSchedule` +- Node `DeletionTimestamp` (node is being terminated) +- NodeClaim `DeletionTimestamp` (NodeClaim is being deleted) +- NodeClaim `status.conditions`: `Rebooting=True`, `DisruptionReason=True`, + or `InstanceTerminating=True` + +If any of these signals is active, bootc-operator defers and retries. It +will never remove a Karpenter taint. + +If the Karpenter NodeClaim owner reference is present but the NodeClaim object +does not exist, bootc-operator treats this as unsafe (the NodeClaim may be +mid-deletion) and defers reboot-slot assignment and drain with a short requeue +rather than proceeding. No Karpenter annotations are applied until a reboot +slot is assigned. + +If none of those signals is active, the controller takes the following steps: + +1. For each annotation (`do-not-repair`, `do-not-disrupt`): if the annotation + is **not** already set to `"true"`, the controller sets it (value `"true"`) + on the Node and, where available, the NodeClaim. It records ownership + separately on the `BootcNode` object: + - `bootc.dev/owns-karpenter-do-not-repair: "true"` + - `bootc.dev/owns-karpenter-do-not-disrupt: "true"` + - `bootc.dev/owns-karpenter-nodeclaim-do-not-repair: "true"` + - `bootc.dev/owns-karpenter-nodeclaim-do-not-disrupt: "true"` +2. The controller proceeds through the normal update flow: drain, reboot, and + image verification. +3. When the slot is freed (node is Ready and running the new image), the + controller removes only annotations for which it recorded ownership and + that are still present. It never removes annotations it did not add. If an + external actor changes an annotation that bootc-operator added, the + operator still removes that annotation when the slot is freed. + +## NodeClaim annotations are supplemental + +NodeClaim annotations (`do-not-repair`, `do-not-disrupt`) are a supplemental +signal, not a distributed lock. By the time bootc-operator writes them, +Karpenter may already have selected or queued a disruption for the registered +Node. The authoritative disruption checks are the Node deletion timestamp, +Node-level taints, NodeClaim deletion timestamp, and NodeClaim status +conditions — the controller checks these before both slot assignment and +drain. + +## NodeClaim watch registration + +When the controller starts, it checks whether the Karpenter `NodeClaim` API is +available. If it is, the controller registers a watch for NodeClaim events and +uses changes to the associated NodeClaim to requeue its owning +`BootcNodePool`. This allows changes such as Karpenter starting or completing +a reboot to be observed promptly. + +The watch is registered only during startup. If Karpenter is installed after +bootc-operator has started, the operator does not add the watch dynamically; +restart the bootc-operator controller after installing Karpenter. As a fallback, +the controller schedules a periodic requeue (every 30 seconds) while +Karpenter-owned nodes are in the staged or slot-assignment portion of the +update, so that missed NodeClaim events do not stall progress indefinitely. + +## Pre-existing annotations + +If a Karpenter annotation is already present on the Node with a value of +`"true"` before protection is applied — set by an external actor, another +controller, or a manual operation — bootc-operator leaves it alone: + +- It does **not** set the corresponding tracking annotation on the `BootcNode`. +- It does **not** remove the annotation when the slot is freed. + +If the annotation is present but set to any value other than `"true"` (for +example `"false"`), bootc-operator overwrites it with `"true"` and records +ownership. This is required because Karpenter only honors the literal value +`"true"`; any other value leaves the node unprotected. + +If an annotation owned by bootc-operator is changed by another actor while the +slot is active, bootc-operator removes that annotation when the slot is freed; +the ownership marker on the `BootcNode` remains authoritative for cleanup. + +The two annotations are tracked independently. If only one is already set to +`"true"`, the operator owns (and later removes) only the other. + +## Temporary overprovisioning during node drains + +During a bootc-operator update, the following sequence can cause temporary +overprovisioning: + +1. bootc-operator evicts workloads from the node being updated. +2. The evicted workloads become pending. +3. Karpenter provisions new capacity to satisfy the pending pods. +4. The original node reboots into the new image and rejoins the cluster. +5. The newly provisioned capacity may become excess and be consolidated later. + +This is expected behavior. The `do-not-disrupt` annotation suppresses +Karpenter's voluntary disruption of the node being updated, but it does not +suppress Karpenter provisioning new capacity for pending workloads. Karpenter +provisioning is driven by pending pod demand rather than disruption eligibility. + +## Post-rollout consolidation and administrator policy + +bootc-operator's protection is intentionally limited to the update lifecycle. +When a node is Ready and the new image has been verified, the operator frees its +reboot slot and removes only the Karpenter annotations that it added. If the +workload has moved to newly provisioned capacity and the original node is now +empty, Karpenter may then consolidate that node with an `Empty` disruption. This +is normal Karpenter behavior after the bootc update has completed; it is not an +OS update failure. + +Cluster administrators who need a node to remain in service after the rollout +must configure that steady-state policy in Karpenter. bootc-operator should not +permanently disable consolidation by default, because doing so can leave unused +capacity running indefinitely. + +This is an intentional separation of responsibilities. bootc-operator protects +only the Karpenter-owned nodes that are participating in its update lifecycle; +it does not apply protection to every node in the `BootcNodePool`. + +| Responsibility | bootc-operator | Cluster administrator / Karpenter | +|---|---|---| +| Protect a node during slot assignment, drain, reboot, and health verification | Applies temporary node and NodeClaim annotations and checks for conflicting Karpenter operations | — | +| Provision capacity for pending workloads | — | Karpenter provisions nodes according to its NodePool and workload requirements | +| Consolidate or drift unrelated nodes | — | Karpenter manages nodes according to the configured disruption policy | +| Retain a node after the rollout becomes complete | Removes only annotations it owns; does not freeze unrelated nodes | Configure `do-not-disrupt`, disruption budgets, or `consolidateAfter` as needed | +| Protect an entire Karpenter pool during maintenance | — | Configure an explicit Karpenter-wide maintenance policy | +| Handle expiration, interruption, manual deletion, or other forceful termination | Detects some conflicts and defers its own update when possible | Configure and operate the applicable Karpenter and infrastructure policies | + +If an entire pool must be held stable, configure that as an explicit Karpenter +maintenance policy rather than relying on bootc-operator to freeze unrelated +nodes. + +Options include: + +1. **Keep a node protected explicitly.** Set + `karpenter.sh/do-not-disrupt: "true"` on the Node and its NodeClaim. An + annotation that was already present before bootc-operator starts protecting + the node is treated as externally owned and is not removed when the rollout + completes. This blocks voluntary disruption such as consolidation and drift, + but not expiration, interruption, manual deletion, or other forceful + termination. +2. **Temporarily block disruption for the whole Karpenter NodePool.** During a + rollout, an administrator can set a zero disruption budget: + + ```yaml + spec: + disruption: + budgets: + - nodes: "0" + ``` + + Restore the normal budget after the rollout. This protects all nodes in the + pool, not just the node being updated. +3. **Delay consolidation.** Configure a suitable `consolidateAfter` interval so + that a rebooted node has time to become schedulable and receive workloads + before Karpenter considers it empty. This reduces the race but does not + guarantee that the node will be retained. + +All three options are administrator-owned policy. The first is appropriate for +selected nodes that must remain stable; the second is appropriate when an +entire pool must be held stable during a controlled maintenance window; and the +third is appropriate when consolidation should remain enabled but be delayed. + +## OS initialization startup taint + +The lifecycle-manager-neutral behavior and taint lifecycle are documented in +[OS initialization taint](os-not-ready-taint.md). With Karpenter, configure +the taint as a `startupTaint` on the `NodePool`. + +Add `bootc.dev/os-not-ready` to the `startupTaints` list in the Karpenter +`NodePool`: + +```yaml +apiVersion: karpenter.sh/v1 +kind: NodePool +metadata: + name: default +spec: + template: + spec: + startupTaints: + - key: bootc.dev/os-not-ready + effect: NoSchedule +``` + +No changes to the `BootcNodePool` spec are required. bootc-operator activates +the feature automatically when it detects the taint on a newly joined node. + +### `startupTaints` vs `taints` + +This taint **must** be declared under `startupTaints`, not `taints`, in the +Karpenter `NodePool`. + +Karpenter uses `startupTaints` to mark nodes that require external +initialization before workloads are scheduled. It factors the taint into its +bin-packing decisions and does not treat nodes with the taint as unable to +satisfy pending pods. + +If the taint is instead declared under `taints` (permanent taints), Karpenter +will observe that pending pods cannot be scheduled on nodes carrying the taint +and will provision additional nodes in a loop — each of which will also carry +the taint, perpetuating the loop. + +## 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. + +## Residual race and coordination guarantees + +The annotations and disruption checks are not an authoritative mutex. Karpenter +does not currently acknowledge `do-not-repair`, expose a shared Lease, or +coordinate its reboot commit with bootc-operator's slot assignment. Karpenter +can therefore commit a disruption immediately before or after bootc-operator's +last check. Setting `do-not-repair` does not cancel a disruption that Karpenter +has already committed. + +The integration narrows this window by: + +- Checking all disruption signals (reboot taint, disrupted taint, deletion + timestamp, and NodeClaim conditions) before both slot assignment and drain. +- Applying protection when the reboot slot is assigned, before draining. + +Full serialization requires Karpenter support for a shared ownership protocol +or an admission/provider mechanism that prevents a competing operation from +being committed. + +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 | +| `karpenter.sh/do-not-repair` | NodeClaim | bootc-operator (or external) | Best-effort suppression of Karpenter repair selection before an operator reboot | +| `karpenter.sh/do-not-disrupt` | NodeClaim | bootc-operator (or external) | Best-effort suppression of Karpenter voluntary disruption before an operator reboot | +| `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 | +| `bootc.dev/owns-karpenter-nodeclaim-do-not-repair` | BootcNode | bootc-operator | Tracks ownership of NodeClaim `do-not-repair` | +| `bootc.dev/owns-karpenter-nodeclaim-do-not-disrupt` | BootcNode | bootc-operator | Tracks ownership of NodeClaim `do-not-disrupt` | diff --git a/docs/src/operations/os-not-ready-taint.md b/docs/src/operations/os-not-ready-taint.md new file mode 100644 index 00000000..4564e111 --- /dev/null +++ b/docs/src/operations/os-not-ready-taint.md @@ -0,0 +1,65 @@ +# OS initialization taint + +`bootc.dev/os-not-ready:NoSchedule` prevents normal workloads from being +scheduled onto a newly created node until bootc-operator has verified that the +node is running the target OS image and is Ready. + +Use this taint with a node lifecycle manager that can apply it before the node +accepts normal workloads. Karpenter is one example; the taint is not specific +to Karpenter. + +## Why use it + +A lifecycle manager may create a node from an image that predates the target +OS image configured in a `BootcNodePool`. Without the taint, workloads can be +scheduled before bootc-operator stages, reboots, and verifies the target OS. + +Apply this exact taint to new nodes: + +```yaml +key: bootc.dev/os-not-ready +effect: NoSchedule +``` + +No changes to the `BootcNodePool` spec are required. + +## Lifecycle + +When bootc-operator creates a `BootcNode` for a newly joined node that carries +the taint, it records responsibility for removing it with the +`bootc.dev/owns-os-not-ready-taint: "true"` annotation on the `BootcNode`. +It then follows the normal update lifecycle. + +The operator removes the taint only after the node is both: + +- UpToDate, meaning its booted image matches the pool target digest; and +- Ready. + +If the node already runs the target image when it joins the pool, the operator +removes the taint as soon as those two conditions are confirmed. The node does +not need to occupy a reboot slot first. + +The operator does not add this taint itself. It records responsibility only +when the `BootcNode` is created. Therefore, a taint added later to an existing +managed node is not removed by bootc-operator. + +If a node becomes Degraded before it reaches the target image, the taint stays +in place. If a node leaves the `BootcNodePool` before verification completes, +the operator also leaves the taint in place; an administrator must decide how +to recover or retire that node. + +## Lifecycle-manager requirements + +The lifecycle manager must apply the taint before normal workload scheduling +can begin, and it must not remove the taint itself before bootc-operator has +verified the target OS. Ensure that any system workloads required for node +initialization tolerate the taint as appropriate. + +For Karpenter-specific configuration and its `startupTaints` requirement, see +[Karpenter integration](karpenter.md#os-initialization-startup-taint). + +## Annotation reference + +| Annotation | Object | Managed by | Purpose | +|---|---|---|---| +| `bootc.dev/owns-os-not-ready-taint` | BootcNode | bootc-operator | Records responsibility for removing the observed `bootc.dev/os-not-ready:NoSchedule` taint; cleared when the taint is removed |