[disruption]: pipelined disruption budgets - #51
Conversation
Add an opt-in PIPELINED_DISRUPTION_BUDGETS mode that splits a NodePool's disruption budget into two gates. Starting a command only counts nodes that are marked and waiting on replacements, so booting replacements no longer block new commands. Before the queue deletes a command's candidates it checks a second gate, allowed minus nodes that are already terminating or NotReady, so the budget bounds how many nodes are draining at once. Budget waits poll every 10s and are not counted toward the command timeout. New gauges karpenter_nodepools_nodes_pending_replacement and karpenter_nodepools_nodes_terminating split the existing consuming-budgets gauge; karpenter_voluntary_disruption_queue_termination_waits_total counts termination-gate waits. Assisted-by: Claude Code:claude-marshmallow-eap
![exa-heron-staging[bot]](https://avatars.githubusercontent.com/in/3694514?s=60&v=4){kind=small}
There was a problem hiding this comment.
Heron review
global-review-orchestrator-guardian
Verdict: ✅ Approve · 2 findings · head 7bbf8b91e7 · openai/gpt-5.6-luna · 4m 22s · session
| Severity | Rule | Location | Finding |
|---|---|---|---|
high-confidence-regression |
pkg/controllers/disruption/helpers.go:448 |
Initialized replacement commands still consume the pipeline start budget | |
unsafe-side-effect |
pkg/controllers/disruption/queue.go:308-321 |
Termination-budget retry can deadlock after a partial multi-node delete |
Last updated 2026-08-24 03:31 UTC · reply @heron dismiss <reason> to dismiss on its comment thread
| switch { | ||
| case nodeutils.GetCondition(node.Node, corev1.NodeReady).Status != corev1.ConditionTrue || node.Deleted(): | ||
| c.terminating++ | ||
| case node.MarkedForDeletion(): |
There was a problem hiding this comment.
Initialized replacement commands still consume the pipeline start budget
high-confidence-regression · confidence 0.98
countBudgetConsumers classifies every initialized, ready node with node.MarkedForDeletion() as pending, and BuildDisruptionBudgetMapping subtracts that count when pipelining is enabled (consuming = c.pending at line 366). However, StartCommand marks candidates for deletion immediately after launching replacements (queue.go:425), while replacement readiness is tracked only in the command (cmd.Replacements[i].Initialized = true, queue.go:266); the cluster state is not changed when a replacement becomes initialized. Consequently a command whose replacement is ready but waiting at the new termination gate remains counted as a pending replacement, so it blocks admission of further commands until its candidate is deleted. This defeats the PR's advertised decoupling of replacement boot time from disruption throughput (and makes the pending-replacement metric inaccurate). Track replacement readiness when calculating the start-stage budget (or otherwise release the command's pending reservation once its replacements initialize), while retaining the termination-stage reservation.
Suggested fix:
Make the start-stage budget use actual queue command/replacement readiness rather than treating all cluster MarkedForDeletion nodes as pending, or update the state used by the budget mapping when a command's replacements become initialized.
Heron review global-review-orchestrator-guardian · fingerprint 30eaaf249de1 · reply @heron dismiss <reason> to dismiss
| if c.Deleted() { | ||
| continue | ||
| } | ||
| needed[c.NodePool.Name]++ | ||
| } | ||
| if len(needed) == 0 { | ||
| return nil | ||
| } | ||
| terminationBudget, err := BuildTerminationBudgetMapping(ctx, q.cluster, q.clock, q.kubeClient, q.cloudProvider, cmd.Reason()) | ||
| if err != nil { | ||
| return fmt.Errorf("building termination budgets, %w", err) | ||
| } | ||
| for nodePool, n := range needed { | ||
| if available := terminationBudget[nodePool]; available < n { |
There was a problem hiding this comment.
Termination-budget retry can deadlock after a partial multi-node delete
unsafe-side-effect · confidence 0.96
The new termination gate attempts to exclude candidates deleted by a prior attempt with c.Deleted() (lines 308-310), but candidates are built from cluster.DeepCopyNodes() in GetCandidatesWithTotals and are retained as those deep-copied StateNodes in the command. StartCommand only marks the live cluster state for deletion (queue.go:425); a successful API delete does not update the command's copied candidate, so c.Deleted() remains false on a retry. If a multi-candidate command deletes one NodeClaim successfully and another delete fails, the retry still sets needed to the full original candidate set while BuildTerminationBudgetMapping sees the first NodeClaim as terminating and subtracts it from availability. For a pool budget of 2 this becomes needed=2, available=1, so the command waits forever (even after the deleted node disappears, the remaining budget is at most 1). This changes the existing partial-delete retry path from retrying the failed candidate to an unrecoverable stuck command. Record/refetch successful deletions and exclude them from needed before rechecking the gate.
Suggested fix:
Maintain per-candidate deletion success in the Command or refresh candidate NodeClaims from the API/cluster before constructing needed; only candidates that still require deletion should be compared with the termination budget.
Heron review global-review-orchestrator-guardian · fingerprint 540848bf6ce0 · reply @heron dismiss <reason> to dismiss
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if IsTerminationBudgetError(err) { | ||
| return | ||
| } | ||
| if q.clock.Since(cmd.CreationTimestamp) > retryDuration { | ||
| err = NewUnrecoverableError(serrors.Wrap(fmt.Errorf("command reached timeout, %w", err), "duration", q.clock.Since(cmd.CreationTimestamp))) | ||
| } |
There was a problem hiding this comment.
🔴 Successful pipelined disruption reported as a timeout failure
When a command waits past retryDuration at the termination gate and then deletes its candidates, waitOrTerminate returns nil, but the deferred timeout check still wraps that nil into an UnrecoverableError; the new guard only exempts TerminationBudgetError, not eventual success. Reconcile then runs the failure branch (queue.go), untainting the draining candidates, clearing their disruption condition, counting a queue failure, and skipping the realized-savings metrics. Pipelined waits are unbounded while retryDuration caps at one hour, so this hits any command delayed at the gate.
| if IsTerminationBudgetError(err) { | |
| return | |
| } | |
| if q.clock.Since(cmd.CreationTimestamp) > retryDuration { | |
| err = NewUnrecoverableError(serrors.Wrap(fmt.Errorf("command reached timeout, %w", err), "duration", q.clock.Since(cmd.CreationTimestamp))) | |
| } | |
| if IsTerminationBudgetError(err) { | |
| return | |
| } | |
| if err != nil && q.clock.Since(cmd.CreationTimestamp) > retryDuration { | |
| err = NewUnrecoverableError(serrors.Wrap(fmt.Errorf("command reached timeout, %w", err), "duration", q.clock.Since(cmd.CreationTimestamp))) | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
PIPELINED_DISRUPTION_BUDGETS(--pipelined-disruption-budgets) mode, default off.BuildDisruptionBudgetMapping):allowed − nodes marked for deletion and waiting on replacements. Draining nodes no longer count here when pipelined.BuildTerminationBudgetMapping, checked in the disruption queue once a command's replacements are Initialized, right before its candidates are deleted):allowed − nodes terminating or NotReady.karpenter_nodepools_nodes_pending_replacementandkarpenter_nodepools_nodes_terminatingsplit the existingkarpenter_nodepools_nodes_consuming_budgets; new counterkarpenter_voluntary_disruption_queue_termination_waits_total{nodepool,reason}.Test plan
go test ./pkg/controllers/disruption/(full suite, 361 specs) and./pkg/operator/options/queue_termination_waits_total/nodes_terminatingagainst the NodePool budget