From 7fcc7b6e1a3879d02a3735e67dc16564c28e75be Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Wed, 15 Jul 2026 12:30:09 -0400 Subject: [PATCH] controller: add precedence-aware setPoolDegraded helper It's annoying right now how there's unclear precedence rules on the PoolDegraded reasons. Rather than trying to encode this in the imperative businesss logic, let's add a clear precedence map and helper where the source of truth for this stuff lives. Of course, the larger issue there is that it's even possible to have multiple degraded reasons coexist, which suggests we should actually split out into more condition types. Otherwise, sysadmins have to fix one, and then wait for the next reconcile, fix the next issue, etc... But at the same time... fully splitting them out would be quite a lot, and I think in practice it'll be quite rare to have a pool with more than e.g. 2 categories of bad things going on. I also think once we start emitting Events, that will help a lot with this visibility issue. So the API complexity of multiple condition types might not be worth it. Assisted-by: Pi (Claude Opus 4.6) Signed-off-by: Jonathan Lebon --- api/v1alpha1/bootcnodepool_types.go | 4 +- .../controller/bootcnodepool_controller.go | 74 +++++++++++-------- internal/controller/rollout.go | 60 ++++----------- 3 files changed, 62 insertions(+), 76 deletions(-) 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