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/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 | 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/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/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)) +} 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 { 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) +} 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()) + } + }) + } +}