-
Notifications
You must be signed in to change notification settings - Fork 7
Implement rollout halting and degraded node surfacing #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
fdf2123
5029fc5
ddd0c0c
94e484c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,20 +59,47 @@ 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) | ||
|
|
||
| // Free reboot slots for nodes that have successfully rebooted into | ||
| // the desired image. This runs before computing available slots so | ||
| // that freed capacity is immediately usable for new candidates. | ||
| if err := r.freeCompletedSlots(ctx, rs); err != nil { | ||
| return fmt.Errorf("freeing completed slots: %w", err) | ||
| } | ||
|
|
||
| // Check for unhealthy nodes on the target digest in reboot slots. If | ||
| // 2+ are unhealthy, halt the rollout. Note that freeCompletedSlots() | ||
| // already removed Ready nodes, so any upToDate node still in a slot is | ||
| // implied to be NotReady. | ||
| unhealthy := findUnhealthySlots(rs, pool.Status.TargetDigest) | ||
|
|
||
| // Hardcode to 2 for now; might make this configurable later, or | ||
| // dynamically adjusted. The rationale is that 1 unhealthy node might | ||
| // be a one-off. But 2 unhealthy nodes becomes a pattern that might | ||
| // indicate a bad image. Now, this _probably_ should scale with number | ||
| // of nodes somehow. E.g. a 500 node cluster could tolerate more | ||
| // unhealthy nodes. Just starting conservative here and we can adjust. | ||
| rolloutHalted := len(unhealthy) >= 2 | ||
| if rolloutHalted { | ||
| syncRolloutHaltedCondition(pool, unhealthy) | ||
| log.Info("Rollout halted: 2+ unhealthy nodes in reboot slots", | ||
| "unhealthyInSlots", len(unhealthy)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to return earlier here? Or do we want to drain eventual node even if the pool is unhealthy?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a bit confusing that we are halting the rollout but the function continues |
||
| } | ||
|
|
||
| maxUnavail, err := resolveMaxUnavailable(pool, rs.nodeCount()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| avail := max(0, maxUnavail-rs.occupiedSlots) | ||
| candidates := selectDrainCandidates(rs.staged, avail) | ||
| availableSlots := 0 | ||
| if !rolloutHalted { | ||
| availableSlots = max(0, maxUnavail-rs.occupiedSlots) | ||
| } | ||
| candidates := selectDrainCandidates(rs.staged, availableSlots) | ||
|
|
||
| log.V(1).Info("Rollout state", | ||
| "upToDate", len(rs.upToDate), | ||
|
|
@@ -84,7 +111,7 @@ func (r *BootcNodePoolReconciler) driveRollout(ctx context.Context, pool *bootcv | |
| "unclassified", nodeNames(rs.unclassified), | ||
| "occupiedSlots", rs.occupiedSlots, | ||
| "maxUnavailable", maxUnavail, | ||
| "availableSlots", avail, | ||
| "availableSlots", availableSlots, | ||
| "candidates", nodeNames(candidates), | ||
| ) | ||
|
|
||
|
|
@@ -358,6 +385,86 @@ 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 := make([]string, len(degraded)) | ||
| for i, bn := range degraded { | ||
| names[i] = bn.Name | ||
| } | ||
| // 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 | ||
| reason string // "Degraded" or "NotReady" | ||
| } | ||
|
|
||
| // findUnhealthySlots returns nodes occupying reboot slots that are unhealthy. | ||
| // Two categories qualify: | ||
| // - Degraded nodes (marked by the daemon itself) that booted the target | ||
| // digest (the image itself may be bad). | ||
| // - UpToDate nodes still holding a slot after freeCompletedSlots (not | ||
| // Ready). I.e. the daemon doesn't see anything wrong, but there's something | ||
| // wrong preventing the kubelet from working correctly. | ||
| func findUnhealthySlots(rs *rolloutState, targetDigest string) []unhealthySlot { | ||
| var result []unhealthySlot | ||
| for _, bn := range rs.degraded { | ||
| if !metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) { | ||
| continue | ||
| } | ||
| // Note we count nil Booted as unhealthy here; this really | ||
| // shouldn't happen because clearly the daemon came up at least | ||
| // once in this node's history to be able to get to a reboot | ||
| // slot. And so Booted should always be set here. | ||
| if bn.Status.Booted == nil || bn.Status.Booted.ImageDigest == targetDigest { | ||
| result = append(result, unhealthySlot{name: bn.Name, reason: "Degraded"}) | ||
| } | ||
| } | ||
| for _, bn := range rs.upToDate { | ||
| if metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) { | ||
| // Still has annotation after freeCompletedSlots → not Ready. | ||
| result = append(result, unhealthySlot{name: bn.Name, reason: "NotReady"}) | ||
| } | ||
| } | ||
| 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create an issue to eventually extend the BootcPoolNode API?