fix(device,plugin): stop MIG usage corruption and fd leak - #2245
Conversation
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe changes propagate lock close errors, preserve MIG allocation state after failed template matching, and apply resolved memory requests during filtering. Webhook quota fixtures now mark configured limits explicitly. ChangesLock file handling
MIG allocation and filtering
Quota test fixtures
Estimated code review effort: 2 (Simple) | ~15 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/device/nvidia/device.go`:
- Around line 703-705: Move the n.Used increment in AddResourceUsage until after
MIG allocation succeeds, so failed no-fit allocations leave usage unchanged. In
pkg/device/nvidia/device.go lines 703-705, update the allocation flow
accordingly; in pkg/device/nvidia/device_test.go lines 2539-2543, assert that
usage.Used remains 0 after the failed allocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 525199dc-ba9c-4325-b209-c3be339fdccd
📒 Files selected for processing (3)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/lock.gopkg/device/nvidia/device.gopkg/device/nvidia/device_test.go
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:
|
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice
33d0905 to
ecf7eac
Compare
|
@archlitchi PTAL This Pr ready to Merged |
|
/assign |
Two independent bugs found while auditing MIG resource accounting:
1. pkg/device/nvidia/device.go: CustomFilterRule was checking MIG
template/slot sizes against the raw, unresolved request.Memreq.
For percentage-based memory requests (MemPercentagereq set,
Memreq == 0) this comparison is always trivially true, so the
filter would admit a device even when no MIG slot is actually
big enough. AddResourceUsage's "fresh template" branch then had
no guard for that case either: if no template fit, it fell
through silently and still incremented Usedmem/Usedcores,
corrupting the node's accounting. Fixed by resolving the actual
memory request before calling CustomFilterRule, and by adding
the same found-guard the sibling ("reuse existing slot") branch
already had.
2. pkg/device-plugin/nvidiadevice/nvinternal/plugin/lock.go: the
*os.File returned by os.Create in createMigApplyLock was never
closed, leaking one fd per MIG apply on every ApplyMigTemplate
call.
Added regression tests for both accounting-corruption paths and
the percentage-based MIG filter check.
Signed-off-by: Aditya Raut <araut7798@gmail.com>
AddResourceUsage incremented n.Used before checking whether a MIG template/slot actually fit the request, in both the fresh-template and reuse-slot branches. On a no-fit error, n.Used stayed bumped even though nothing was allocated, corrupting device usage accounting. Signed-off-by: Aditya Raut <araut7798@gmail.com>
Project-HAMi#2313 added a LimitSet flag that FitQuota now gates on instead of Limit != 0, and updated TestFitResourceQuota's fixture accordingly, but missed three sibling tests in the same file that also build device.Quota directly: TestFitResourceQuotaNonNvidia, TestFitResourceQuotaCountsEveryDevice, and TestFitResourceQuotaAscendMemoryFactor. Their fixtures defaulted to LimitSet: false, so FitQuota treated the configured limits as unset and admitted every pod, failing the denial assertions in all three tests on current master. Set LimitSet: true on the five affected fixture entries to match what AddQuota produces, mirroring the fix already applied to TestFitResourceQuota. Signed-off-by: Aditya Raut <araut7798@gmail.com>
archlitchi asked for the comments around the CustomFilterRule fix and its test in device.go/device_test.go to be trimmed to one sentence. Signed-off-by: Aditya Raut <araut7798@gmail.com>
ecf7eac to
16d1b3c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adity1raut, archlitchi 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:
Two independent bugs found while auditing MIG resource accounting in
pkg/device/nvidia/device.goandpkg/device-plugin/nvidiadevice/nvinternal/plugin/lock.go:Silent MIG usage-accounting corruption for percentage-based memory requests.
CustomFilterRulechecked MIG template/slot sizes against the raw, unresolvedrequest.Memreq. For percentage-based memory requests (MemPercentagereqset,Memreq == 0), that comparison is always trivially true, so the filter could admit a device even when no MIG slot is actually big enough for the real (percentage-resolved) request.AddResourceUsage's "fresh template" branch had no guard for the case where no template fits — unlike its sibling ("reuse existing slot") branch, which already returns an error in that case. So if no template fit, the function silently fell through and still incrementedUsedmem/Usedcores, corrupting the node's device usage accounting and leavingContainerDevice.UUIDwithout a valid MIG index suffix.Fix: resolve the actual memory request before calling
CustomFilterRule(instead of passing the still-zeroMemreq), and add the samefound-guard toAddResourceUsage's fresh-template branch that the sibling branch already has.File descriptor leak in
createMigApplyLock.os.Create(file)returns an open*os.Filethat was discarded without calling.Close(). This function runs on everyApplyMigTemplate()call (viaDisableOtherNVMLOperation), so every MIG reconfiguration on a node leaked one fd.Fix: close the file handle after creating it.
Added regression tests:
TestAddResourceUsage_MigResetNoFit— verifiesAddResourceUsagenow returns an error (and doesn't touch usage counters) when no MIG template fits during a fresh template selection.TestFit_MigPercentageRequestRejectsUndersizedTemplate— verifiesFit()now correctly rejects a MIG device whose only template is too small for a percentage-based memory request (this previously passed incorrectly).Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Both fixes are minimal and mirror existing patterns already in the file (the
found-guard mirrors the sibling branch inAddResourceUsage; the fd close follows normalos.Createusage elsewhere in the codebase).Does this PR introduce a user-facing change?:
AI Assistance Disclosure: I used Geminy to help investigate the codebase for bugs, diagnose the root cause of both issues, and draft the fix and regression tests. I reviewed, understood, and verified all AI-generated changes (build, tests, lint) before submitting.
Summary by CodeRabbit
Bug Fixes
Tests