fix(cambricon): enforce ResourceQuota for percentage/whole-card memory requests - #2536
Conversation
…y requests Fit() resolved memreq from either an explicit Memreq or, when that was zero, from MemPercentagereq against the candidate device's Totalmem - but only ever checked the result against device capacity. The namespace ResourceQuota was never consulted for that path, so a pod that requests memory by percentage (or omits the memory field, which defaults to MemPercentagereq=100, i.e. a whole card) is admitted and its usage charged even when it exceeds the quota. Every other pod in the namespace is then denied against a quota that one pod already silently blew through. NVIDIA already avoids this by calling FitQuota from inside its own Fit(), once the real card's Totalmem is known and the percentage can be resolved to an absolute value. This mirrors that same pattern for Cambricon: resolve memreq first, then check it against the quota before falling through to the existing capacity checks. Fixes Project-HAMi#2468 Signed-off-by: Aditya Raut <araut7798@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)
📝 WalkthroughWalkthroughCambricon ChangesCambricon quota enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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❌ Patch coverage is
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:
|
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adity1raut, FouoF 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 this PR does
Fixes a ResourceQuota bypass for Cambricon when memory is requested by percentage, including the implicit whole-card default a pod gets when it omits the memory field entirely.
Why
Fit()inpkg/device/cambricon/device.goresolvesmemreqfrom either an explicitMemreq, or, when that's zero, fromMemPercentagereqagainst the candidate device'sTotalmem. Either way, the only thing that value was ever checked against was device capacity (dev.Totalmem-dev.Usedmem < memreq). The namespaceResourceQuotawas never consulted on this path.Concretely: a pod that requests
cambricon.com/mlu: 1with novmemoryfield getsMemreq=0,MemPercentagereq=100fromGenerateResourceRequests- that's the normal way to ask for a whole card.Fitresolves that against the real card and hands out the full amount, and the quota manager still records it as used. If the namespace has a tight quota, that one pod consumes it entirely without ever being checked against it, and every subsequent pod in the namespace is denied - including small, well-formed ones that respect the quota.fitResourceQuotain the webhook can't fix this on its own, because it sumsreq.Memreq * req.Numsacross containers before a node/card is even chosen, andMemreqis 0 for a percentage-based request - the real value only exists once the specific card'sTotalmemis known, i.e. insideFit.NVIDIA already handles this correctly: it has a local
fitQuotahelper that resolves the pod's hypothetical total usage and callsFitQuotafrom inside its ownFit(), right aftermemreqis resolved. Cambricon didn't have the equivalent.What changed
fitQuotahelper topkg/device/cambricon/device.go, structurally identical to NVIDIA's: it folds the candidate allocation into whatever's already tentatively allocated for the pod (respectingCollapseInitContainerUsagefor init-container peak usage), then callsdevice.GetLocalCache().FitQuota(...).Fit()right aftermemreqis resolved (mirroring where NVIDIA places its own call), before the existing capacity checks. A quota miss now incrementsResourceQuotaNotFitand moves on to the next candidate device, same as every other rejection reason in this function.TestDevices_Fit_ResourceQuotaWholeCardRequestinpkg/device/cambricon/device_test.go, which seeds a namespace quota viaQuotaManager.AddQuotaand asserts a whole-card (MemPercentagereq=100) request that exceeds it is now denied withResourceQuotaNotFit. I checked this test does fail against the pre-fix code (reverted theFit()change locally and reran it - it comes backfit=true, quota silently ignored).This only touches the Cambricon backend. The same admission-time gap likely exists for other non-NVIDIA backends that expose a percentage-based memory resource, but I didn't want to bundle a multi-vendor change into one PR - happy to open follow-ups per backend if that's useful.
Verification
go build ./...go test ./pkg/device/cambricon/... ./pkg/device/... -short --race -count=1(all pass, including the new regression test)go vet ./pkg/device/cambricon/...golangci-lint run ./pkg/device/cambricon/...(0 issues)hack/verify-license.sh,hack/verify-import-aliases.sh(pass)fitQuotait mirrors), so I validated it with the unit test above rather than on real MLU hardware.Fixes #2468
AI assistance disclosure
I ran into this while back in the ResourceQuota test fixtures I touched in #2432, and used Claude Code to help pin down the exact call sites and draft the fix by mirroring NVIDIA's existing
fitQuota, plus the regression test. I read through the resulting diff, verified the root cause by reverting theFit()change and confirming the new test fails without it, and take responsibility for the correctness of both the fix and the test.Summary by CodeRabbit