fix(quota): honor an explicit zero limit in ResourceQuota - #2313
Merged
archlitchi merged 2 commits intoAug 6, 2026
Conversation
FitQuota gated on Limit != 0 to decide whether a quota was configured, so setting limits.<gpu-resource>: "0" in a ResourceQuota (used to block all GPU usage in a namespace) was indistinguishable from "no limit", and every pod was admitted. Add a LimitSet flag to Quota that AddQuota sets and DelQuota clears, and gate FitQuota on LimitSet instead of Limit != 0, so an explicit zero is honored as a hard block. Entries auto-created by usage tracking keep LimitSet false and are unaffected. This revives the fix from Project-HAMi#1904 by @iasthc, which was closed by the activity policy without review. Adds a regression test that fails on the pre-fix code and passes after. Fixes Project-HAMi#2312 Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.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 (1)
📝 WalkthroughWalkthroughThe quota model now records whether limits were explicitly configured. Enforcement rejects usage against explicitly configured zero memory or core limits, while unset limits remain unenforced. Tests cover the new zero-memory behavior. ChangesQuota limit semantics
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
pkg/device/quota_test.go (1)
163-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression cases for the other
LimitSetstates.This test covers explicit zero memory only.
FitQuotahas a separate core branch, and usage-created entries intentionally haveLimit=0withLimitSet=false. Add cases for explicit zero core rejection and usage-only admission. Also assertLimitSetis true afterAddQuotaand false afterDelQuotainTestAddQuotaAndDelQuotaat Lines 245-273.This follows the stated objective to enforce explicit zero limits while preserving usage-only entries.
🤖 Prompt for 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. In `@pkg/device/quota_test.go` around lines 163 - 182, Extend the quota tests with regression cases for explicit zero core rejection and usage-only admission, covering the separate FitQuota branches while preserving entries with Limit=0 and LimitSet=false. In TestFitQuotaExplicitZeroBlocks, add the core resource scenario; add a usage-only case that admits valid usage without a configured limit. In TestAddQuotaAndDelQuota, assert LimitSet is true after AddQuota and false after DelQuota.
🤖 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.
Nitpick comments:
In `@pkg/device/quota_test.go`:
- Around line 163-182: Extend the quota tests with regression cases for explicit
zero core rejection and usage-only admission, covering the separate FitQuota
branches while preserving entries with Limit=0 and LimitSet=false. In
TestFitQuotaExplicitZeroBlocks, add the core resource scenario; add a usage-only
case that admits valid usage without a configured limit. In
TestAddQuotaAndDelQuota, assert LimitSet is true after AddQuota and false after
DelQuota.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d28cb95f-a6ed-443e-b839-e7d1f0cdce3d
📒 Files selected for processing (2)
pkg/device/quota.gopkg/device/quota_test.go
Contributor
|
pkg/scheduler/webhook_test.go:272-273 also builds Quota{} directly and needs LimitSet: true, that's why TestFitResourceQuota fails in CI. |
TestFitResourceQuota builds device.Quota values directly instead of going through AddQuota, so LimitSet defaulted to false and FitQuota read the configured limits as "not set" — the two cases that expect a denial were admitted instead. Set LimitSet on the fixture to match what AddQuota produces. Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
Contributor
Author
|
Good catch, thanks . that's the one I missed. TestFitResourceQuota builds the Quota values by hand instead of going through AddQuota, so LimitSet stayed false and FitQuota read both limits as unconfigured, which is why the two denial cases got admitted. |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
This was referenced Aug 4, 2026
Closed
Member
|
Have you tested on your environment? |
Member
|
/assign |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, veyron-kairo 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 |
adity1raut
added a commit
to adity1raut/HAMi
that referenced
this pull request
Aug 7, 2026
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>
adity1raut
added a commit
to adity1raut/HAMi
that referenced
this pull request
Aug 7, 2026
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>
adity1raut
added a commit
to adity1raut/HAMi
that referenced
this pull request
Aug 7, 2026
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>
hami-robot Bot
pushed a commit
that referenced
this pull request
Aug 7, 2026
#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>
hami-robot Bot
pushed a commit
that referenced
this pull request
Aug 7, 2026
* test(plugin): raise register.go coverage from 15% to 25% Add tests for GetNumaNode's two deterministic error branches (PCI info lookup failure, and the numa_node sysfs read failure that always occurs since the domain prefix is stripped before building the path) and a test that pins down getAPIDevices' existing panic-on-no-driver behavior via recover(). getAPIDevices, RegisterInAnnotation, and most of WatchAndRegister's active branch remain uncovered: getAPIDevices calls the real nvml.Init() directly (not an injectable interface), which fails and panics on any machine without an NVIDIA driver - this one included - so exercising their actual logic isn't possible without either real GPU hardware or refactoring nvml access behind an interface, which is out of scope for this test-only change. Signed-off-by: Aditya Raut <araut7798@gmail.com> * fix(plugin): stop getAPIDevices crashing when NVML init fails nvml.Shutdown() was deferred before checking whether nvml.Init() succeeded. On a host with no NVML library at all, Init fails gracefully, but the deferred Shutdown() still runs during the panic(0) unwind and crashes the whole process with a dynamic symbol lookup error, since the library was never loaded. Defer Shutdown only after a successful Init. Also route Init through a package-level nvmlInit var so TestGetAPIDevices_PanicsOnNVMLInitFailure can simulate an init failure directly instead of depending on the test host's driver state, and fix the numa_node mock/comment to use NVML's real 8-digit-domain BusId format per review feedback. Signed-off-by: Aditya Raut <araut7798@gmail.com> * test(plugin): cover getAPIDevices success path for codecov gap The only existing getAPIDevices test forces nvmlInit to fail, so the defer nvml.Shutdown() moved after the success check in 5a1cd13 was never exercised, leaving codecov/patch failing on that line. Add a success-path test that stubs both nvmlInit and nvml.Shutdown (both are reassignable package vars) so it never touches the real, unloaded NVML library on this host. Signed-off-by: Aditya Raut <araut7798@gmail.com> * test(plugin): raise register.go coverage Signed-off-by: Aditya Raut <araut7798@gmail.com> * refactor(plugin): trim comments to one sentence per review feedback archlitchi asked for unnecessary comments to be removed from register.go and register_test.go, with method/test doc comments trimmed to one sentence describing what the code does. Also fixes a comment above defer nvml.Shutdown() that was missing its lead-in clause ("succeeded: calling it after a failed Init crashes the process...") and read as a sentence fragment. Signed-off-by: Aditya Raut <araut7798@gmail.com> * fix(scheduler): set LimitSet in ResourceQuota test fixtures #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> --------- Signed-off-by: Aditya Raut <araut7798@gmail.com>
adity1raut
added a commit
to adity1raut/HAMi
that referenced
this pull request
Aug 7, 2026
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>
hami-robot Bot
pushed a commit
that referenced
this pull request
Aug 7, 2026
* fix(device,plugin): stop MIG usage corruption and fd leak
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>
* fix(device): don't bump n.Used on failed MIG allocation
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>
* fix(scheduler): set LimitSet in ResourceQuota test fixtures
#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>
* refactor(device): trim comments to one sentence per review feedback
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>
---------
Signed-off-by: Aditya Raut <araut7798@gmail.com>
hami-robot Bot
pushed a commit
that referenced
this pull request
Aug 7, 2026
…ice (#2395) * fix(amd): don't reject devices when nouse-gputype annotation is empty checkAMDType only guarded the in-use annotation against an empty value; the no-use annotation had no such guard. If amd.com/nouse-gputype was present but set to an empty string, strings.Split returned a single empty-string element, and strings.Contains(cardType, "") is always true in Go, so every AMD card was treated as excluded and no AMD device could be scheduled. Apply the same "ok && non-empty" guard used for the in-use annotation to the no-use annotation as well, matching the shared device.CheckType helper used by other vendor backends. Signed-off-by: Aditya Raut <araut7798@gmail.com> * test(amd): add regression test for checkAMDType empty annotation handling Signed-off-by: Aditya Raut <araut7798@gmail.com> * fix(amd): skip empty comma-separated members in gputype annotations strings.Split on a trailing/leading/double comma (e.g. "MI250,") leaves an empty member, and strings.Contains(cardType, "") is always true, so that member matched every card. This let a nouse-gputype annotation like "MI250," exclude all cards instead of just MI250, and let a use-gputype annotation like "MI300," match cards it shouldn't. Trim and skip empty members before matching, in both branches. Also add whitespace-only and trailing/leading-comma regression cases to TestCheckAMDType, per review feedback on PR #2395. Signed-off-by: Aditya Raut <araut7798@gmail.com> * chore: retrigger CI (unrelated flaky pkg/scheduler test) Signed-off-by: Aditya Raut <araut7798@gmail.com> * fix(scheduler): stop leaked informers in tests to fix CI flakiness Several tests call informerFactory.Start(s.stopCh) but never close s.stopCh, so the shared informer's event-processing goroutine keeps running after the test finishes. Test_Filter and Test_ResourceQuota also wire that informer's UpdateFunc to s.onUpdatePod, so a pod update processed during the test can still be in flight on that leaked goroutine once later tests start, racing their unsynchronized writes to global state (e.g. device.SupportDevices in Test_onAddPod_BadDeviceAnnotation) against DecodePodDevices' reads. Reproduced with the race detector via the same command CI's hack/unit-test.sh runs (go test $(go list ./pkg/... ./cmd/...) -short --race -count=1); confirmed unrelated to the pkg/device/amd change in this PR since it reproduces without it. Add t.Cleanup to close each stopCh (and to remove the "TEST" entry added to device.SupportDevices) so no test outlives its own goroutines. Signed-off-by: Aditya Raut <araut7798@gmail.com> * fix(scheduler): set LimitSet in ResourceQuota test fixtures #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> --------- Signed-off-by: Aditya Raut <araut7798@gmail.com>
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
3 participants
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.
What type of PR is this?
/kind bug
What this PR does / why we need it:
FitQuotausedLimit != 0to decide whether a quota was configured. As aresult, setting
limits.nvidia.com/gpumem: "0"(orgpucores: "0") in aResourceQuota — which an operator would use to block all GPU usage in a
namespace — was silently treated as "no limit", and every pod was admitted
instead of being denied.
This adds a
LimitSetflag toQuotato distinguish an explicitly configuredlimit (including an explicit
0) from an entry auto-created by usage tracking.AddQuotasets it,DelQuotaclears it, andFitQuotanow gates onLimitSetinstead of
Limit != 0, so an explicit zero is honored as a hard block.Usage-tracking entries keep
LimitSetfalse and behave exactly as before.Which issue(s) this PR fixes:
Fixes #2312
Special notes for your reviewer:
This revives the fix from #1904 by @iasthc, which was closed by the activity
policy without review rather than on merit — full credit to them for the
original approach. I've added a regression test (
TestFitQuotaExplicitZeroBlocks)that admits a request under a
"0"quota on the pre-fix code and blocks it after,and updated
TestFitQuotaso its manually-built quotas carryLimitSet.make verifyand thepkg/deviceunit tests pass locally.AI assistance disclosure:
I used AI assistance (Claude Code) while confirming the root cause .
wrote the regression test by myself . I reviewed and verified the change, ran
make verifylocally,and take responsibility for it.
Does this PR introduce a user-facing change?:
NONE
Summary by CodeRabbit
Summary by CodeRabbit