fix(nvidia): stamp flattened UsageList index in MIG UUID suffix - #2513
fix(nvidia): stamp flattened UsageList index in MIG UUID suffix#2513Eshiv-Pandey wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Eshiv-Pandey The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNVIDIA MIG allocation now records the flattened ChangesMIG UUID indexing
Estimated code review effort: 2 (Simple) | ~10 minutes 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 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
AddResourceUsage stamps the MIG slot position into the container UUID suffix, but the two allocation branches disagreed on what that position means. The fresh-template branch stamped the geometry item index while reserving the slot at the flattened UsageList offset, whereas the reuse branch stamped the flattened offset. ExtractMigTemplatesFromUUID, and therefore getNodesUsage, reads the suffix as a flattened offset into UsageList. When an earlier item in the geometry has Count > 1 the two diverge, so getNodesUsage marks a different slot in use than the one actually reserved: the allocated slot appears free while an unallocated one appears taken. Stamp usageListIdx in the fresh-template branch so both branches and the reader agree. Also correct TestDevices_AddResourceUsage, whose Count > 1 case pinned the buggy suffix (dev-3[0-1]) while its assertion helper silently re-derived the flattened offset, masking the mismatch. It now expects the flattened suffix (dev-3[0-2]) and reads the UsageList index directly. Adds TestAddResourceUsage_MigFreshTemplateStampsFlattenedIndex as a focused regression. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
757c440 to
3579148
Compare
|
#2378 have refactored dynamic MIG, please check is this bug still exists on it. |
| // and getNodesUsage read back), not the geometry item index. | ||
| if !strings.Contains(ctr.UUID, "[") { | ||
| ctr.UUID = ctr.UUID + "[" + fmt.Sprint(tidx) + "-" + fmt.Sprint(idx) + "]" | ||
| ctr.UUID = ctr.UUID + "[" + fmt.Sprint(tidx) + "-" + fmt.Sprint(usageListIdx) + "]" |
There was a problem hiding this comment.
usagelistidx and tidx are gone in master now. #2378 rewrote this function. pls check vs new code.
There was a problem hiding this comment.
i just checked the bug is gone now after the refactor. closing this. thanks for the review!
What this PR does
AddResourceUsagerecords which MIG slot a container got by appending a[templateIdx-position]suffix to the device UUID. The two allocation branches disagreed on whatpositionmeans:UsageListoffset (usageListIdx) but stamped the geometry item index (idx) into the UUID.ExtractMigTemplatesFromUUID, and thereforegetNodesUsage, reads thesuffix as a flattened offset intoUsageList. When an earlier item in the geometry hasCount > 1, the geometry index and the flattened offset diverge, sogetNodesUsagemarks a different slot in use than the one that was actually reserved. The allocated slot appears free while an unallocated one appears taken, which can lead to double-booking a MIG instance.The fix
Stamp
usageListIdxin the fresh-template branch so both branches and the reader agree on the meaning of the suffix.Why it was not caught earlier
The shipped A100-40GB geometry is
1g.5gb x1, 2g.10gb x3, where the geometry index and flattened offset of the first fitting slot coincide, so the bug is masked. It surfaces on geometries where an earlier item hasCount > 1, such as the2g.10gb x3, 1g.5gb x1example in docs/develop/dynamic-mig.md.Testing
Added
TestAddResourceUsage_MigFreshTemplateStampsFlattenedIndex, which builds a1g.5gb x2, 2g.10gb x1geometry (flattenedUsageListindex 2 = geometry index 1) and asserts the stamped suffix round-trips throughExtractMigTemplatesFromUUIDto the slot actually reserved. The test fails without the fix (stamps1, expects2) and passes with it.Also corrected the existing
TestDevices_AddResourceUsage: itsCount > 1case pinned the buggy suffix (dev-3[0-1]) while its assertion helper silently re-derived the flattened offset, masking the mismatch. It now expects the flattened suffix (dev-3[0-2]) and reads theUsageListindex directly. Full unit suite run with-race.AI assistance disclosure
This change was prepared with AI assistance (Claude Opus 4.8). All code and tests were reviewed by me.
Summary by CodeRabbit
Bug Fixes
Tests