Skip to content

feat(rescheduling): gpuFragmentation drains multi-pod nodes via simulated whole-node repack - #16

Merged
pfernandes21 merged 4 commits into
exa/masterfrom
devin/1787863624-repack-multi-pod
Aug 28, 2026
Merged

feat(rescheduling): gpuFragmentation drains multi-pod nodes via simulated whole-node repack#16
pfernandes21 merged 4 commits into
exa/masterfrom
devin/1787863624-repack-multi-pod

Conversation

@pfernandes21

@pfernandes21 pfernandes21 commented Aug 27, 2026

Copy link
Copy Markdown

What type of PR is this?

/kind feature

What this PR does / why we need it:

The gpuFragmentation strategy only repacked nodes whose entire GPU usage was a single pod, and only onto a strictly fuller node. Multi-pod partial nodes (e.g. two nodes each running 1-2 low-priority 1-GPU pods) never consolidated, leaving a steady stranded-GPU residue.

The planner now considers all pods on a node and drains whole nodes:

  • movableSoleGpuTaskmovableGpuTasks: every GPU task on the node must be movable (same per-task eligibility checks: running, opt-out label, do-not-disrupt, priority ceiling, controller-owned, single-member PodGroup, unspent eviction cap). One immovable GPU pod disqualifies the node — evicting the others would churn without freeing it.
  • findDestinationsimulateDrain: first-fit-decreasing placement of the whole victim set over cloned idle capacity, so two victims can't both claim the same free GPU. All-or-nothing: no partial drains. The simulation covers resources; predicates run against live node state, so placed-pod-dependent constraints (anti-affinity, topology spread) are not co-simulated — a drain can leave replacements Pending in that case rather than misplacing them.
  • Destinations may now be equally full, so two equally-empty nodes consolidate (the case that unlocks the a3-highgpu-4g pair). Fullness is compared as a fraction of allocatable, so mixed-size pools compare fairly. Fuller destinations are still tried first; less-utilized sources drain first, and equal-utilization sources tie-break toward the node whose victims have lower priority (then fewer victims, then name) — space saved stays the primary goal, but between two equally packable workloads the cheaper-to-disrupt one moves.
  • To keep the replacement from landing back on the drained source when it ties with the destination under binpack, the source is stamped with exa.ai/repack-drain-source and a node-order penalty steers GPU replacements away from it for the cooldown window (a penalty, not a veto — it can still be used if nothing else fits). Destinations get the pool-cooldown stamp (exa.ai/repack-last-eviction) too, so the anti-thrash clock survives Karpenter deleting the emptied source; only the source gets the penalized annotation.
  • Still at most one source node per pool per pass, behind the pool cooldown. If the emptiest drainable node's move set exceeds the remaining victim budget, the pool is skipped for this pass rather than silently draining a fuller node.
  • maxVictims caps total evictions per pass with node move sets taken atomically; default raised 1 → 8. Note the practical per-node max on an 8-GPU node is 7 one-GPU victims — a fully used node is never a source.
  • Probe construction and PrePredicateFn are memoized per victim; PredicateFn still runs per candidate destination.
before: evict 1 pod iff it is the node's sole GPU pod && some strictly fuller node fits it
after:  evict all GPU pods on the least-utilized drainable node (ties -> lower-priority victims)
        iff FFD-sim places every one onto >=-as-utilized pool nodes within remaining victim budget

Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:

Execution is per drain set: the source (and destinations) are stamped once before any eviction, and a source-stamp failure aborts the whole set — no evictions without a recorded cooldown. Per-victim PodGroup stamping can still partially fail; that victim is skipped and the remainder drains on a later pass under the already-recorded cooldown.

Does this PR introduce a user-facing change?

The gpuFragmentation rescheduling strategy can now drain nodes running multiple GPU pods, consolidating them onto equally-or-more-utilized (by fraction of allocatable) nodes in the same pool via a simulated first-fit-decreasing repack; equally-utilized candidates drain the node running lower-priority pods first. Recently drained nodes are de-prioritized for GPU replacements during the cooldown window. maxVictims now caps total evictions per pass (default 8) with per-node move sets applied atomically.

Link to Devin session: https://app.devin.ai/sessions/9f5f310b988c4566869471b54b1154dd
Open in Devin Desktop: https://app.devin.ai/desktop/session/9f5f310b988c4566869471b54b1154dd?variant=devin
Requested by: @pfernandes21

…ated whole-node repack

Assisted-by: Devin:claude-opus-4.6
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@exa-heron-staging

Copy link
Copy Markdown

Review of the whole-node drain planner. Read the planner against pkg/scheduler/{api,framework,cache} and the binpack plugin, and exercised the hypotheses with throwaway tests in pkg/scheduler/plugins/rescheduling (numbers below are measured, not estimated). The FFD-over-cloned-idle core is right — Idle.Clone() + Sub genuinely fixes double-booking, Sub's assert can't fire because LessEqual is checked first with the same api.Zero default (resource_info.go#L283-L286), the refactor preserves returning the session-side task rather than the node-local clone, and all-or-nothing on an immovable GPU pod is the right call. The concerns are around the parts of the invariant that live outside the planner.


1. Allowing equally-full destinations removes the only thing steering the replacement away from the node you just drained

This is the change I'd want resolved before this runs non-dry-run.

An evicted task stays in node.Used for the whole eviction window — AddTask with Releasing status does ni.Releasing.Add and ni.Used.Add (node_info.go#L453-L458), and Idle was already decremented at allocation time. BinPackingScore scores on exactly task.Resreq / node.Allocatable / node.Used (binpack.go#L206-L246); the useFutureOccupancy branch doesn't kick in here because the 1-GPU replacement still fits the source's Idle.

Repro of the exact motivating case (two equally-empty 4-GPU nodes, one 1-GPU pod each), stepping the planned victim to Releasing:

plan: pod-a: node-a -> node-b
node-a used=1000 idle=3000 releasing=1000
node-b used=1000 idle=3000 releasing=0
binpack inputs gpu: node-a (used=1000, alloc=4000)  node-b (used=1000, alloc=4000)
cpu:                node-a used=4000 alloc=64000  ;  node-b used=4000 alloc=64000

Identical inputs → identical score → util.SelectBestNodeAndScore returns bestNodes[rand.Intn(len(bestNodes))] (scheduler_helper.go#L190-L205). ReplicaSet treats a pod with a DeletionTimestamp as not-active and creates the replacement immediately, so the replacement is scored while the source is still holding the releasing GPU.

Under the old strictly-fuller rule the destination always outscored the source and this couldn't happen. The deterministic source ordering (L245-L255) makes the planner non-swapping, but it says nothing about where the replacement lands; the pool cooldown only rate-limits the coin flip to one per 30 min, it doesn't make it converge.

To be precise about what I did and didn't verify: the tie and the random tie-break are confirmed in code; whether it actually bounces depends on the rest of your nodeorder stack (weights, other plugins) and on the delete-vs-reschedule race, neither of which I can see from this repo. So I'd treat "does the replacement land on the destination?" as the thing the dry-run rollout has to answer, and it can't answer it today — the dry-run only logs the plan, not the outcome. Options, roughly in order of how much I'd trust them:

  • Make the drained node unattractive for the cooldown window. The exa.ai/repack-last-eviction stamp already exists; a PredicateFn/NodeOrderFn registered by the same plugin that filters or penalises nodes stamped within cooldownSeconds closes the loop directly.
  • Require the destination to be strictly fuller after the move set (dest.Used + Σ placed > source.Used), which is automatically true for real multi-pod consolidations and only excludes the exact 1-for-1 tie.
  • Cordon/taint the source for the drain window.

2. "All-or-nothing" is a planning property only; execution is per-victim and unrollbackable

stampGpuFragmentationMove runs per plan and stamps before eviction (L131-L146), and the actual delete is fire-and-forget in a detached goroutine that only resyncTasks on failure (cache.go#L908-L915). So any of {stamp error, ssn.Evict error, delete rejected, scheduler restart mid-set} yields a partially drained node: pods churned, node not freed, pool then parked on a 30-minute cooldown. The PR notes this, but the exposure went from 1 pod to maxVictims, and two details make it worse than "the remainder drains on a later pass":

  • The node stamp is written once per victim. Same Node object, 8 sequential Nodes().Patch + 8 PodGroups().Patch calls with context.TODO() and no timeout, inline in the scheduling cycle (L463-L479). Stamping the node once per drain set and aborting the whole set if that fails would make the execution match the planner's contract, instead of evicting a prefix.
  • A stamped-but-not-actually-deleted pod is poisoned forever. movableTask rejects any PodGroup with exa.ai/repack-evictions != "0" (L356-L358) and nothing ever resets it. Successful evictions are self-cleaning (new pod → new PodGroup), failed ones are not. Under all-or-nothing, one poisoned pod now permanently disqualifies its entire node, and you're generating 8x the evictions per pass for that residue to accumulate from.

3. Nothing gates this on PDBs, and the default maxVictims is now 8

The default evictor calls Pods().Delete() directly rather than the eviction subresource (cache.go#L288-L295), and the pdb plugin's victim fn is additive in ssn.VictimTasks — the tier loop unions victim sets and returns the first non-empty one, it never filters another plugin's victims (session_plugins.go#L593-L617). So maxVictims is the only thing between this strategy and N simultaneous pod deletions. Raising the default 1 → 8 in the same PR that also introduces multi-pod drains multiplies blast radius twice over; I'd land the planner with maxVictims at 2-3 and raise it once dry-run data (and #1) back it up.

4. The stated justification for maxVictims: 8 isn't reachable

default raised 1 → 8 so a fully fragmented 8-GPU node (e.g. B200 running 8x 1-GPU pods) can drain

movableGpuTasks returns nil when used >= node.Allocatable (L294), so a node running 8×1-GPU pods on 8 GPUs is never a source. That guard is correct — a full node has no stranded GPUs — but it caps the victim set at allocatable - 1, i.e. 7 on an 8-GPU node. Verified: 8×1GPU → 0 plans; 7×1GPU → 7 plans. The release note and PR body should say 7 (or the default should be justified some other way).

5. Global maxVictims + alphabetical pool order starves later pools

L207 breaks the pool loop on the global budget and L258 skips a source whose set doesn't fit the remainder. Repro with default conf, aaa-pool holding a 7-victim drain and zzz-pool an independent 2-victim drain with an unspent cooldown:

maxVictims=8: plans=7 by pool=map[aaa-pool:7]

zzz-pool gets nothing, purely because of sort.Strings(poolNames). Since the strategy is already "at most one source node per pool per pass", a per-pool (or per-drain) cap would be both more predictable and closer to what the release note describes.

Related: the continue at L258-L260 means that under budget pressure the planner silently falls through to a fuller source (sources are sorted used-ascending), so the emptiest-first intent is quietly dropped exactly when the budget is tight. break would at least keep the policy honest.

6. The FFD simulates resources but not predicates

Cloned idle stops two victims claiming the same GPU, but predicate is still evaluated against live node state, so any predicate whose result depends on the pods already on the node — pod anti-affinity with topologyKey: kubernetes.io/hostname, topologySpreadConstraints, some device-plugin/extended-resource checks — will pass for every victim independently and then only admit one of them. Repro with a predicate modelling "at most one repack victim per node":

plans=2
  victim-a -> dest
  victim-b -> dest

Real-world effect: the drain executes, the node frees, and one replacement goes Pending or triggers a new Karpenter node. Doesn't invalidate the design, but "provably fit … all-or-nothing" reads stronger than what's actually proven, so I'd at least say so in the doc comment on simulateDrain. Cheap partial mitigation: once a victim is placed on a candidate, skip that candidate for other victims sharing an owner reference.

7. The cooldown clock lives on the node the drain is designed to delete

The pool cooldown is read from exa.ai/repack-last-eviction across pool members (L218-L232) and only ever written to the source (L466-L469). A drain that works empties the source, Karpenter reaps it, and the pool's only cooldown record leaves with it — so the anti-thrash window disappears precisely in the success case and the next pass can immediately drain another node. Pre-existing, but it matters more now that a pass can move 8 pods. Stamping the destinations too (they survive by construction) or a pool-scoped lease would fix it.

8. Predicate cost now scales with victims × candidates × sources, and every call deep-copies the pod

The closure calls probeTask(task)pod.DeepCopy() + api.NewTaskInfo — and Session.PrePredicateFn on every invocation (L119-L128), and simulateDrain is retried per candidate source until one succeeds. Measured on a synthetic 30-node pool with 4 victims/node: 870 predicate invocations when nothing places, 960 when victims partially place. That's ~1k pod deep-copies + full predicate stacks per pass in a 30-node pool, growing as O(nodes² × victims). Building the probe once per victim (and running PrePredicateFn once per victim) is a few lines for a maxVictims-fold saving.


Nits

  • sort.Slice(members, …) at L211-L217 is dead ordering now — sources is fully re-sorted at L245-L255 and simulateDrain re-sorts candidates at L416-L422.
  • gain is the source node's total usage copied onto every plan and logged per victim (L133, L142). From dry-run logs you can't distinguish one atomic 7-pod drain from 7 independent moves, which is awkward given dry-run is the validation mechanism. One node-level line — draining <node>: N pods, frees G GPUs -> [dests] — would make the rollout much easier to read.
  • Fullness is compared in absolute GPUs, not as a fraction of allocatable. A single karpenter.sh/nodepool routinely spans instance types; with a 64-GPU node at used=2 and a 4-GPU node at used=2 the planner drains the 64-GPU node onto the 4-GPU one (het: b0: big -> small), decided purely by the name tie-break. Which node survives probably shouldn't come down to strings.Compare.
  • simulateDrain returns a non-nil empty slice for an empty victim set, which the moves == nil check at L262 would read as success. Unreachable today because L240 filters empty sets, but it's load-bearing at a distance.
  • maxVictimPriority uses max, so between a node with one -1 pod and a node with four -2 pods, the four-pod node drains first: same GPU gain, 4x the churn and 4x the budget. Worth folding victim count into the tie-break.
  • maxVictimPriority's i == 0 || guard is correct for all-negative sets but reads like a bug; min := int32(math.MinInt32) or an explicit empty-slice contract would be clearer.

…fractional fullness

Address review findings: stamp source+destinations once per drain set and
abort the set when the cooldown cannot be recorded; penalize recently
drained sources via a node-order fn (separate drain-source annotation so
destinations are not steered away from); compare node fullness as a
fraction of allocatable for mixed-size pools; break instead of falling
through to a fuller source under budget pressure; memoize probe and
PrePredicate per victim; log drains at node level; document the
predicate-simulation limitation and the 7-victim max on 8-GPU nodes.

Assisted-by: Devin:claude
@devin-ai-integration

Copy link
Copy Markdown

Re: Heron review (comment 5446343722) — addressed in a015c06, point by point:

  1. Equally-full destination bounce — added gpuFragmentationNodeOrderFn, registered on every session open (not just rescheduling passes, since replacements schedule in the sessions after the drain). It penalizes nodes carrying a fresh exa.ai/repack-drain-source stamp by -100 for GPU-requesting pods — enough to break a binpack tie without vetoing the node. The penalty annotation is separate from the pool-cooldown annotation so destinations (which now also get the cooldown stamp) are not steered away from.

  2. Planning vs execution atomicity — stamping is now once per drain set (stampGpuFragmentationNodes): source stamp failure aborts the whole set before any eviction; destinations are stamped with the pool-cooldown annotation so the anti-thrash clock survives Karpenter reaping the emptied source (dest stamp failure is logged, non-fatal — the source clock already holds the pool). Per-victim PodGroup stamping remains per-victim; failure skips only that victim under the already-recorded cooldown.

  3. PDB / direct deletion + maxVictims=8 default — acknowledged; deletion path unchanged in this PR. The default stays 8 per the operator's explicit request (fragmented nodes here are 8-GPU B200s), dry-run remains the default, and only priority < 0 single-member-PodGroup controller-owned pods are ever eligible. Escalated the default and per-pool-vs-global budget question to the requester.

  4. 7 not 8 — PR description, release note, and the MaxVictims doc now state the 7-victim practical max on an 8-GPU node (fully-used nodes are never sources).

  5. Budget fall-through — now breaks the pool instead of falling through to a fuller source, preserving emptiest-first semantics. Budget stays global for now (see 3).

  6. Predicate simulation limitation — documented on simulateDrain and in the PR description; no co-simulation of placed pods in this PR.

  7. Cooldown lifetime — covered by destination stamping (see 2).

  8. Predicate cost — probe + PrePredicateFn memoized once per victim (probes/preFailed maps); PredicateFn still per candidate destination.

  9. Nits — dead members sort removed; dry-run and execution logs are one line per drain set (source, pool, pod count, GPUs freed, destinations); fullness compares used/allocatable fractions (test covers the 1/8-vs-1/4 mixed-size case); simulateDrain rejects an empty victim set; source tie-break adds victim count after max priority; maxVictimPriority is only called with non-empty victim slices from the planner.

@pfernandes21
pfernandes21 merged commit 917406b into exa/master Aug 28, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant