diff --git a/api/v1alpha1/bootcnodepool_types.go b/api/v1alpha1/bootcnodepool_types.go index 66991df..e750ff3 100644 --- a/api/v1alpha1/bootcnodepool_types.go +++ b/api/v1alpha1/bootcnodepool_types.go @@ -30,7 +30,9 @@ const ( PoolPaused string = "Paused" ) -// PoolDegraded condition reasons. +// PoolDegraded condition reasons. Precedence ordering (which reason +// takes priority when multiple apply) is defined by +// poolDegradedPrecedence in the controller package. const ( // PoolNodeConflict means a node's labels match multiple pool selectors. PoolNodeConflict string = "NodeConflict" diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index 999c062..9f37a7a 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -327,12 +327,7 @@ func (r *BootcNodePoolReconciler) resolveTargetDigest(ctx context.Context, pool digest, err := r.TagResolver.Resolve(ctx, pool.Spec.Image.Ref) if err != nil { log.Error(err, "Failed to resolve tag", "ref", pool.Spec.Image.Ref) - apimeta.SetStatusCondition(&pool.Status.Conditions, metav1.Condition{ - Type: bootcv1alpha1.PoolDegraded, - Status: metav1.ConditionTrue, - Reason: bootcv1alpha1.PoolTagResolutionError, - Message: err.Error(), - }) + setPoolDegraded(pool, bootcv1alpha1.PoolTagResolutionError, err.Error()) } else { if pool.Status.TargetDigest != digest { log.Info("Resolved tag to new digest", "ref", pool.Spec.Image.Ref, "digest", digest) @@ -374,12 +369,7 @@ func isInvalidSpecError(err error) bool { // setInvalidSpecCondition sets Degraded/InvalidSpec on the pool and // returns (Result, nil) so Reconcile stops without requeueing. func (r *BootcNodePoolReconciler) setInvalidSpecCondition(ctx context.Context, pool *bootcv1alpha1.BootcNodePool, specErr error) (ctrl.Result, error) { - apimeta.SetStatusCondition(&pool.Status.Conditions, metav1.Condition{ - Type: bootcv1alpha1.PoolDegraded, - Status: metav1.ConditionTrue, - Reason: bootcv1alpha1.PoolInvalidSpec, - Message: specErr.Error(), - }) + setPoolDegraded(pool, bootcv1alpha1.PoolInvalidSpec, specErr.Error()) if err := r.Status().Update(ctx, pool); err != nil { return ctrl.Result{}, fmt.Errorf("updating pool status: %w", err) } @@ -460,11 +450,16 @@ func (r *BootcNodePoolReconciler) syncMembership(ctx context.Context, pool *boot } // Set or clear the conflict condition based on what we found. - var conflictingPools []string - for name := range conflicting { - conflictingPools = append(conflictingPools, name) + if len(conflicting) > 0 { + var conflictingPools []string + for name := range conflicting { + conflictingPools = append(conflictingPools, name) + } + // Sort so the message is stable across reconciles. + setPoolDegraded(pool, bootcv1alpha1.PoolNodeConflict, + fmt.Sprintf("Node selector overlaps with pool(s): %s", + strings.Join(slices.Sorted(slices.Values(conflictingPools)), ", "))) } - syncConflictCondition(pool, conflictingPools) return ownedSet, nil } @@ -644,18 +639,39 @@ func (r *BootcNodePoolReconciler) restoreCordonState(ctx context.Context, bn *bo return nil } -// syncConflictCondition sets or clears the Degraded condition with -// reason NodeConflict on the pool. It only mutates the in-memory -// object; the caller is responsible for writing status. -func syncConflictCondition(pool *bootcv1alpha1.BootcNodePool, conflictingPools []string) { - if len(conflictingPools) > 0 { - apimeta.SetStatusCondition(&pool.Status.Conditions, metav1.Condition{ - Type: bootcv1alpha1.PoolDegraded, - Status: metav1.ConditionTrue, - Reason: bootcv1alpha1.PoolNodeConflict, - // Sort so the message is stable across reconciles. - Message: fmt.Sprintf("Node selector overlaps with pool(s): %s", - strings.Join(slices.Sorted(slices.Values(conflictingPools)), ", ")), - }) +// poolDegradedPrecedence defines the priority ordering of PoolDegraded +// condition reasons. When multiple degraded reasons apply in a single +// reconcile, the highest-precedence reason wins. setPoolDegraded uses this to +// avoid overwriting a higher-precedence reason with a lower one. +// +// InvalidSpec and TagResolutionError cause early returns in Reconcile and +// don't coexist with the others in practice, but are included for +// completeness. +var poolDegradedPrecedence = map[string]int{ + bootcv1alpha1.PoolHealthy: 0, + bootcv1alpha1.PoolNodeDegraded: 1, + bootcv1alpha1.PoolNodeConflict: 2, + bootcv1alpha1.PoolRolloutHalted: 3, + bootcv1alpha1.PoolInvalidSpec: 4, + bootcv1alpha1.PoolTagResolutionError: 5, +} + +// setPoolDegraded sets the Degraded condition on the pool, but only if the new +// reason has higher precedence than the current one. This prevents e.g. +// RolloutHalted from overwriting NodeConflict. Unknown reasons get precedence +// 0 (map zero-value), so they silently no-op. Keep poolDegradedPrecedence in +// sync with the API constants. +func setPoolDegraded(pool *bootcv1alpha1.BootcNodePool, reason, message string) { + current := apimeta.FindStatusCondition(pool.Status.Conditions, bootcv1alpha1.PoolDegraded) + if current != nil && current.Status == metav1.ConditionTrue { + if poolDegradedPrecedence[reason] <= poolDegradedPrecedence[current.Reason] { + return + } } + apimeta.SetStatusCondition(&pool.Status.Conditions, metav1.Condition{ + Type: bootcv1alpha1.PoolDegraded, + Status: metav1.ConditionTrue, + Reason: reason, + Message: message, + }) } diff --git a/internal/controller/rollout.go b/internal/controller/rollout.go index a7090b3..27eea00 100644 --- a/internal/controller/rollout.go +++ b/internal/controller/rollout.go @@ -68,10 +68,13 @@ func (r *BootcNodePoolReconciler) driveRollout(ctx context.Context, pool *bootcv rs := buildRolloutState(log, ownedBootcNodes) - // Flag degraded nodes at the pool level. NodeConflict (set during - // membership sync) takes priority, so we only set NodeDegraded if - // the pool isn't already degraded for another reason. - syncNodeDegradedCondition(pool, rs.degraded) + // Flag degraded nodes at the pool level. + if len(rs.degraded) > 0 { + names := nodeNames(rs.degraded) + slices.Sort(names) + setPoolDegraded(pool, bootcv1alpha1.PoolNodeDegraded, + fmt.Sprintf("Degraded nodes: %s", strings.Join(names, ", "))) + } // Free reboot slots for nodes that have successfully rebooted into // the desired image. This runs before computing available slots so @@ -88,7 +91,13 @@ func (r *BootcNodePoolReconciler) driveRollout(ctx context.Context, pool *bootcv rolloutHalted := len(unhealthy) >= unhealthySlotHaltThreshold if rolloutHalted { - syncRolloutHaltedCondition(pool, unhealthy) + details := make([]string, len(unhealthy)) + for i, u := range unhealthy { + details[i] = u.name + ": " + u.reason + } + slices.Sort(details) + setPoolDegraded(pool, bootcv1alpha1.PoolRolloutHalted, + fmt.Sprintf("Rollout halted: 2+ unhealthy nodes in reboot slots (%s)", strings.Join(details, ", "))) log.Info("Rollout halted: 2+ unhealthy nodes in reboot slots", "unhealthyInSlots", len(unhealthy)) @@ -393,29 +402,6 @@ func buildRolloutState(log logr.Logger, ownedBootcNodes map[string]*bootcv1alpha return rs } -// syncNodeDegradedCondition sets Degraded/NodeDegraded on the pool if any -// nodes are degraded. It respects priority: if the pool is already degraded -// for another reason (e.g. NodeConflict), it does not overwrite it. -// XXX(jl): Or... should this be a new condition type so it can be surfaced in -// parallel? Let's see how this approach feels and iterate. -func syncNodeDegradedCondition(pool *bootcv1alpha1.BootcNodePool, degraded []*bootcv1alpha1.BootcNode) { - if len(degraded) == 0 { - return - } - if apimeta.IsStatusConditionTrue(pool.Status.Conditions, bootcv1alpha1.PoolDegraded) { - return - } - names := nodeNames(degraded) - // Sort so the message is stable across reconciles. - slices.Sort(names) - apimeta.SetStatusCondition(&pool.Status.Conditions, metav1.Condition{ - Type: bootcv1alpha1.PoolDegraded, - Status: metav1.ConditionTrue, - Reason: bootcv1alpha1.PoolNodeDegraded, - Message: fmt.Sprintf("Degraded nodes: %s", strings.Join(names, ", ")), - }) -} - // unhealthySlot identifies a node in a reboot slot that is unhealthy. type unhealthySlot struct { name string @@ -452,24 +438,6 @@ func findUnhealthySlots(rs *rolloutState, targetDigest string) []unhealthySlot { return result } -// syncRolloutHaltedCondition sets Degraded/RolloutHalted on the pool. It -// implicitly takes priority over NodeDegraded (which checks for existing -// daemon-driven degraded status) by running later than -// syncNodeDegradedCondition. -func syncRolloutHaltedCondition(pool *bootcv1alpha1.BootcNodePool, unhealthy []unhealthySlot) { - details := make([]string, len(unhealthy)) - for i, u := range unhealthy { - details[i] = u.name + ": " + u.reason - } - slices.Sort(details) - apimeta.SetStatusCondition(&pool.Status.Conditions, metav1.Condition{ - Type: bootcv1alpha1.PoolDegraded, - Status: metav1.ConditionTrue, - Reason: bootcv1alpha1.PoolRolloutHalted, - Message: fmt.Sprintf("Rollout halted: 2+ unhealthy nodes in reboot slots (%s)", strings.Join(details, ", ")), - }) -} - // resolveMaxUnavailable computes the effective maxUnavailable value from the // pool's rollout spec. Defaults to 1 when unset. A value of 0 is allowed and // means no reboot slots are available (effectively paused). Returns an