fix(preempt): keep nominated gang tasks eligible and retry stranded gang placements - #19
Conversation
…ang placements A task whose nominatedNodeName passes predicates was rejected by taskEligibleToPreempt before it could join its gang's preemption transaction, so multi-worker gangs above the victims' priority stalled in a saturated pool with the statement discarded every cycle. Predicates ignore resource fit, so a passing nominated node says nothing about whether the task can run there. Drop that bail-out and let allocate own the nominated-node fast path. Also retry a job's preemption transaction, excluding the nodes the abandoned attempt pipelined onto, when placing tasks one at a time strands later siblings (e.g. topology pod affinity into a domain with no other preemptable node). Bounded by the new gangPlacementRetries argument (default 2).
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
ReviewI traced the whole latch end-to-end and ran ablations against the two new cases. The diagnosis is right and the Statement bookkeeping is sound. Two things I'd fix before merge: the retry test is only a ~40%-green guard, and the 1. Root cause confirmed — and it's a permanent latch, not a transient oneThe chain is exactly as described, and it's worth writing down because it explains why the pod never recovers:
So the deeper bug is (5): a discarded preemption publishes a nomination that nothing can retract. This PR neutralises the harm, which is the right call for a hotfix, but consider a follow-up that clears 2. Change (1) — eligibility: correct, and it converges on upstream kube-schedulerThe removed branch came from upstream volcano One consequence that's easy to miss reading the diff and is worth a line in the description: the only semantic delta is the "predicates pass" case, and it now falls through to the terminating-victim loop, which master skipped via the early Please file this upstream (volcano-sh/volcano) or at least leave a 3. Change (2) — retry loop: bookkeeping is safe, blast radius is well scopedThings I verified rather than assumed:
The residual cost case is a large gang: 63 of 64 workers pipeline, the 64th finds no capacity, and we now throw away all 63 and re-run the full pass twice with those 63 nodes excluded — near-certain to fail, at the price of 2 extra Minor, but real: after a fully-failed retry sequence, the nomination published to the API (§1) comes from the last attempt — i.e. the node chosen after the good ones were excluded. So for gangs that never succeed, the retry actively degrades the hint that 4.
|
|
(continued — my previous comment was cut off mid-section 5) 4.
|
7. The two new cases: one is a solid guard, one is a coin flipI ablated each piece of the fix and ran
Case 1 is a proper regression test. Case 2 is not: with retries fully disabled it still goes green 40% of the time, and the node-exclusion mechanism it is named after is caught only 20% of the time. The nondeterminism is Verified fix — make the sampling exhaustive for this case, which also matches what Delphi actually runs ( MinCandidateNodesPercentageKey: 100,
MinCandidateNodesAbsoluteKey: 100, // >= len(Nodes)
MaxCandidateNodesAbsoluteKey: 100,With that, over 15 runs each: baseline 15/15 pass, Two more test gaps:
8. Nits
Bottom line: ship (1) — minimal, matches upstream kube-scheduler, properly tested. For (2) I'd want the retry case made deterministic and |
- Drop the ssn.NodeList override on retries: allocate's per-node verdicts on full nodes are always resource failures (Unschedulable), never affinity verdicts, so nothing needs to bypass FilterOutUnschedulableAndUnresolvable. - Remove excluded nodes before PredicateNodes samples candidates instead of after it truncates. - Hold predicate helpers per job and replace the job's helper on retry, so attempt-specific cache entries do not leak into later preemption work. - Run PrePredicateFn before taskEligibleToPreempt: predicating the nominated node needs the task's cycle state, which allocate does not always leave. - Clear the tasks' LastTransaction before each retry so the published scheduling reason and nominated node reflect the final attempt only. - Tests: exhaustive candidate sampling for retry cases, whole-gang pipelined assertions, nominated-node case under normal preemption, allocate-first draining-node case matching the production shape.
…quence Clear LastTransaction on the job's pending tasks when retries are exhausted, so a nomination from the last attempt (made with the better nodes excluded) is not published as the scheduling hint. A single failed attempt keeps upstream behaviour.
|
Thanks — all of §2–§7 addressed in 13e374e and 038e250 (replying top-level since the review comments are not inline threads). §1 latch chain — agreed on all six steps. Clearing §2 — added an explicit "diverges from upstream" note on the removed bail-out. Also moved §3 stale nomination — §4 §5 — helpers are now held per job ( §6 — excluded nodes are removed from §7 — retry cases run with min/max candidates = §8 — |
|
CI: |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Prio-0 multi-worker
training_taskgangs (reserved-only, no spillover) stalled for 20+ minutes on Delphi while the reserved B200 pool was 100% allocated by lower-priority gangs, even though preempting them was allowed. Two compounding causes inpreempt, both only reachable by gangs above their victims' priority in a saturated pool:1. Nominated node bail-out (upstream,
e9040d33).taskEligibleToPreemptreturned an error whenPod.Status.NominatedNodeNamewas set and the node passed predicates:Predicates ignore resource fit, so this fires whenever a task was pipelined + un-pipelined in a previous cycle (allocate/preempt found one node, the gang did not reach
minAvailable, the statement was discarded, but the nominatedNodeName stuck —Statement.DiscardrecordsLastTransactionand the cache only ever setsNominatedNodeName, never clears it). From then on worker-0 is skipped every cycle, worker-1 alone can never satisfy the gang, and the transaction is discarded forever. Observed sequence onakbmmrhwvjgn4tgfwlrb-fjkyifby-0:Fix: drop that branch (marked as an intentional divergence from upstream). The nominated-node fast path is allocate's job (
allocate.goalready checksFutureIdleon the nominated node); preempt keeps the task eligible and letspipelineOnFittingNode/ candidate search decide. TheUnschedulableAndUnresolvableand terminating-victim checks are unchanged.PrePredicateFnnow runs before the eligibility check, since predicating the nominated node reads the task's cycle state and allocate does not always leave it behind (e.g. task skipped because the queue was overused).2. Stranded gang placement. Tasks are placed one at a time, so the node chosen for worker-0 can leave worker-1 with zero candidates — here
ip-10-82-77-191sat in a 3-node capacity reservation whose other two nodes ran prio-0 jobs, so the EFA pod affinity (karpenter.k8s.aws/capacity-reservation-id) gave worker-1 nowhere to go. A single pass discards and repeats the same choice next session.Fix:
preemptForJobretries the job's transaction, excluding the nodes the abandoned attempt pipelined onto, up togangPlacementRetriestimes (new action arg, default 2,0disables):PredicateNodessamples/truncates it.Statement.Evictonly mutates session state; the API call happens inCommit).Execute's cross-job loop body moved intopreemptForJob/preemptJobTasks; the same-job loop is unchanged apart from sharingpendingPreemptorTasksand the per-job helper.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Heron review addressed (see comments):
allNodes = ssn.NodeListbypass dropped — allocate checks resource fit before predicates, so on a full node it recordsInsufficient cpu(Unschedulable), never a sibling-affinity U&U verdict, so there was nothing to bypass.New cases:
TestPreemptandTestTopologyAwarePreempt:task nominated to a node that passes predicates still preempts for its gang— runsallocatethenpreempt(so the predicate cycle state exists, as in production). With the old bail-out restored it fails with 0 evictions and logs the exact production error.TestTopologyAwarePreempt:retry gang placement when the first task lands in a domain its siblings cannot join— zone-a node holds the cheapest victim but is alone in its zone; the retry moves the gang to zone b. Deterministically fails withgangPlacementRetries: 0, without node exclusion, or without the fresh predicate helper.TestTopologyAwarePreempt:retry gang placement when the first task fits a draining node its siblings cannot join— the production shape: allocate pipelines worker-0 onto a node with releasing resources, strands worker-1, discards; preempt's first attempt repeats it for free; the retry moves the gang.Retry cases predicate every node (as Delphi does with
minCandidateNodesPercentage: 100) and every gang case assertsExpectTaskStatusNums: {Pipelined: 2}.Verified:
gofmt,go vet,go test ./pkg/scheduler/actions/... ./pkg/scheduler/framework/... ./pkg/scheduler/util/...; preempt package green at-count=10.Not a
training_taskregression: the generated pod spec (priority class, reserved-only affinity, EFA pod affinity, resources) is identical to gangs that scheduled fine earlier the same day.Does this PR introduce a user-facing change?
Link to Devin session: https://app.devin.ai/sessions/9618e4fbaeb345ba9c161ec18bfdec6c
Open in Devin Desktop: https://app.devin.ai/desktop/session/9618e4fbaeb345ba9c161ec18bfdec6c?variant=devin
Requested by: @pfernandes21