fix(scheduler): scope anti-thrash preemption cap per requester - #92
Merged
Conversation
Issue #79: a high-priority pod could be silently starved forever behind a small pool of lower-priority pods once that pool's shared anti-thrash budget (isAntiThrashed, >3 preemptions/5min) was exhausted by an EARLIER, unrelated pod's retries. The cap was keyed by victim name alone, so any pending pod's preemption activity against a victim counted against every future pod's ability to preempt that same victim -- even one that never caused any thrash itself. ADR 005's anti-thrash mitigation targets a single pod flip-flopping with the same victim (misconfigured priority causing repeat preempt/fail/restart/preempt cycles), not cross-pod budget sharing. Scope isAntiThrashed/recordPreemption to the (victim, requester) pair so a fresh requester is never blocked by another pod's thrashing, while the original single-requester flip-flop protection (TestSchedule_AntiThrash) is unchanged. Also names the anti-thrash cap explicitly in both Pending Reason messages when it is the actual blocker, instead of a generic 'no preemption candidates' or 'evicting N candidates' that reads as a plain availability shortfall. Adds TestSchedule_AntiThrashStarvesUnrelatedRequester (T5.1), red under the prior victim-only cap, green after this fix.
TestSchedule_PreemptionFairness_Issue79ExactScenario drives the same Schedule/Preempt/AddPod cycle the reconciler runs in production: a 'high' priority pod preempts a pool of 4 'normal' priority pods (matching priorityClassName: high vs the default, and the reported > 3-candidate shape) that restart immediately each time. Confirms both halves of the T5.2 fix hold together: - the same pod retrying a 5th time against the same victims is still correctly capped, with the Pending reason naming the anti-thrash cap explicitly - a different, unrelated high-priority pod queued moments later is not starved by the first pod's exhausted budget
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #79: a
priorityClassName: highpod could staypendingindefinitely behind a small pool of same-priority
normalpods, with amisleading
evicting N candidate(s)event that read as "nothing more canbe preempted" instead of naming the real blocker. The anti-thrash
preemption cap in
internal/scheduler/scheduler.gowas keyed only byvictim pod name, so one pending pod's retries could exhaust a victim's
preemption budget and then silently starve every other, unrelated
high-priority pod that later needed to preempt that same victim, for up
to the 5-minute anti-thrash window, even though evicting the same victim
again would have satisfied the new pod. This scopes the cap to the
(victim, requester)pair instead, and makes the event message name thecap explicitly when it is the actual blocker.
Design decision (T5.2) -- flagged for review
docs/plan.md's T5.2 explicitly required a real design call here, not amechanical patch, and instructed stopping to ask before picking a
resolution.
AskUserQuestionwas not available in this dispatchedteammate's toolset (functionally equivalent to hook-blocked -- no live
human to confirm with). Per the task's explicit fallback instruction, the
most conservative fix compatible with the existing anti-thrash regression
test (
TestSchedule_AntiThrash) was chosen without live confirmation.This specific call should get a human look before merge.
The bug:
isAntiThrashed/recordPreemptiontracked "has this podbeen preempted more than 3 times in 5 minutes" per victim only -- with no
notion of who did the preempting. Any pending pod's successful
preemption of a victim counted toward the same shared budget. On a
resource-constrained single-GPU node with few low-priority pods (the
issue's exact shape: 15 same-priority CI-runner/service pods), a burst of
legitimate preemption activity from a stream of different pods sharing
that small victim pool can exhaust it, after which any subsequent
high-priority pod finds zero preemption candidates and sits
pendingforup to 5 minutes -- a denial-of-service on scheduling that has nothing to
do with resource availability.
The fix: scope the cap to the
(victim, requester)pair. A pod thatitself keeps failing and re-triggering against the same victim is still
capped after 3 preemptions in 5 minutes -- ADR 005's original
misconfigured-priority flip-flop protection is unchanged, and
TestSchedule_AntiThrashstill passes with its original intent intact(same requester name reused across both halves of that test so the
assertion is still exercising window expiry, not pair-scoping). A
different, unrelated pod is no longer blocked by another pod's
thrashing.
Alternatives considered and rejected:
rather than fixing it, and weakens thrash protection for the exact
misconfigured-priority flip-flop scenario ADR 005 names as the reason
the cap exists.
when no other candidates exist. Rejected because it directly breaks
TestSchedule_AntiThrash's existing invariant (single victim, singlerequester, already thrashed, sole candidate -- must stay
Pending);this is precisely the flip-flop case the cap protects against.
Changes
internal/scheduler/scheduler.go:isAntiThrashedandrecordPreemptionnow take arequesterparameter and key the preemption record by
(victim, requester)viaa new
preemptionKeyhelper, instead of by victim name alone.antiThrashMaxPreemptions(3) andantiThrashWindow(5m)as named constants (previously inline magic numbers repeated in two
places).
Pendingreasons (no preemption candidates/preemption insufficient) now report how many lower-priority podswere excluded specifically by the anti-thrash cap, and say so
explicitly, whenever that -- not raw resource availability -- is the
actual blocker.
internal/scheduler/scheduler_test.go:TestSchedule_AntiThrash's tworecordPreemptioncalls tothe new signature, both attributed to the same requester so the test
still asserts the flip-flop/window-expiry behavior it always did.
TestSchedule_AntiThrashStarvesUnrelatedRequester(T5.1): redunder the prior victim-only cap (a fresh requester was blocked by an
unrelated pod's thrash history), green after the fix.
TestSchedule_PreemptionFairness_Issue79ExactScenario(T5.3):end-to-end regression matching the issue's exact shape --
priorityClassName: high(100) vsnormal(1000, default), 4lower-priority victims restarting immediately after each eviction,
driven through the same
Schedule/RemovePod/AddPodcycle thereconciler runs. Confirms the same pod is still capped on its 5th
retry (with the cap named in the reason) while a different,
unrelated high-priority pod scheduled moments later is not starved
by it.
Testing
All run from the branch worktree, full repo:
go build ./...-- clean.go vet ./...-- clean.staticcheck ./...-- clean, no findings.go test ./... -race -timeout 120s -count=1-- all 13 packages pass(
api,bus,cron,executor,gpu,housekeeper,lifecycle,manifest,metrics,reconciler,scheduler,state,watcher;cmd/sparkhas no tests). No regressions in the pre-existinginternal/schedulersuite, includingTestSchedule_AntiThrashand theother preemption tests (
TestSchedule_PreemptLowPriority,TestSchedule_MultipleVictimsNeeded,TestSchedule_VictimSelectionPrefersRecentlyStarted,TestPreemptionCount).TestSchedule_AntiThrashStarvesUnrelatedRequesterfailed against the pre-fix code (
action=2/Pending) before the fixlanded, confirming it's a genuine red-then-green test, not a green
test written after the fact.
Not yet done (belongs to the coordinator's centralized DGX pass per the
dispatch instructions for this task): live verification on the DGX with a
manifest reproducing the reported scenario, and closing issue #79. A
concrete 4+-pod repro recipe (manifests, expected before/after event
text, cleanup) is included in this task's handoff report to the
coordinator.
docs/plan.md acceptance criteria (E5, issue #79)
TestSchedule_AntiThrashStarvesUnrelatedRequestershows a high-priority pod staying
Pendingforever under the pre-fixcode despite 4 eligible lower-priority victims, confirming both the
starvation and the misleading
no preemption candidatesmessage. Met.TestSchedule_AntiThrash(the pre-existing anti-thrash/flip-flop test)passes unchanged in intent; T5.1's test goes green. Met, with the
design decision above flagged for review as instructed.
4+ candidates, high-priority pod):
TestSchedule_PreemptionFairness_Issue79ExactScenario. Met.go vet ./... && staticcheck ./... && go test ./... -race -timeout 120s -count=1: all exit 0. Met.Linked issues
Fixes #79 (preemption cap silently starves a high-priority pod behind
more than 3 lower-priority pods, with a misleading
evicting N candidate(s)event message that doesn't disclose the cap).