fix(nvidia): collapse duplicate MIG candidates in topology scheduling - #2751
Conversation
Topology scheduling collects every fitting candidate before choosing an allocation, and Fit re-examines a MIG card once per free instance slot. A card therefore contributes one interchangeable candidate per slot, so an eight-card node with seven free slots each builds a 56-entry pool for a request that only distinct physical cards should satisfy. Pair scores are defined between physical cards and a card has no score against itself, so the extra entries can never improve a combination. They do two kinds of damage. On a node that publishes no pair scores every combination ties at zero, so the first one generated wins, and that is several slots of the same GPU while the other cards sit idle. On any node the pool inflates the search: generateCombinations enumerates C(56,5) rather than C(8,5), which took ~28s in a unit test. Score one candidate per physical card whenever the node has at least as many distinct cards as the request. Keep the full pool otherwise, so several MIG instances on one card can still satisfy a request that no set of distinct cards could. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughNVIDIA topology scheduling now deduplicates MIG candidates by physical GPU UUID when enough distinct cards are available. New tests cover spreading, score selection, single-card ranking, fallback packing, and candidate ordering. ChangesMIG topology selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change prevents duplicate MIG candidates from concentrating allocations on one physical GPU and removes the associated scheduling slowdown; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, Eshiv-Pandey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
NvidiaGPUDevices.Fit()re-examines a MIG card once per free MIG placement (if dev.Mode == "mig" { i++ }). In topology mode nothing short-circuits the collection loop, so each pass appends another candidate entry. An 8-card node with 7 free slices each produces 56 candidate entries describing only 8 physical cards — seven identical structs per card.That inflated pool causes two defects:
All devices land on one card.
generateCombinationswalks the slice in order, so the first combination emitted is N copies of the same card.computeBestCombinationreplaces the best only ontotalScore > bestScore, so when no pair scores are published (every score0) that first same-card combination wins the tie. A card has no pair score against itself, so a same-card set can never lose on score either. A 5-GPU request on an 8-card node is satisfied by 5 slices of one card, leaving 7 cards idle.Combinatorial blow-up. C(56,5) = 3,819,816 combinations are enumerate instead of C(8,5) = 56, per node, on the scheduling hot path.
Measured on 8 MIG cards x 7 free slices with the topology policy and no published pair scores: 2 GPUs -> 1 card in 1.15ms, 3 -> 1 card in 14.3ms, 4 -> 1 card in 237.7ms, 5 -> 1 card in 11.19s. The same 4-GPU request without the topology policy takes 53.8us and correctly picks 4 cards.
This PR collapses the candidate pool by physical card UUID before scoring, but only when the node has at least as many distinct cards as the pod requested. The duplicate entries are identical structs and pair scores are per physical
card, so they carry no information the scorer can use - dropping them cannot change which set of cards is best. The concrete MIG placement is still chosen later by
selectMigCandidate()inAddResourceUsage(), so nothing is lost.The
len(distinct) >= originReqguard preserves packing multiple MIG instances onto one card when the node has no alternative - behaviour PR #2724 shows with hardware evidence is wanted.After the fix all requests select the expected number of distinct cards, and the 5-GPU case drops from 11.19s to under 1ms.
Non-MIG nodes are unaffected: without MIG there are no duplicates, so the helper returns the slice unchanged and every path is byte-identical. The `== originReq exact-match branch is deliberately untouched - that pool is the only option, so
there is nothing to choose between.
Which issue(s) this PR fixes:
Fixes #2750
Special notes for your reviewer:
pkg/scheduler/score.gorejects the node beforeFit()runs, so it is unreachable unless fix(scheduler): allow multiple MIG instances on one GPU #2724 changes that gate.computeWorstSingleCarddereferencesdeviceScoreMap[dev1.UUID].Scoreswith no nil check. Happy to file separately.Does this PR introduce a user-facing change?: