Fix/multi device quota enforcement - #2218
Conversation
…not only NVIDIA Remove the NVIDIA-only guard in fitResourceQuota() that skipped quota enforcement for every non-NVIDIA device backend (Ascend, Cambricon, Hygon, etc.). MemoryFactor defaults to 1 for non-NVIDIA devices. Add mock device test helper and three new Ascend quota enforcement test cases including exceeded-quota, within-quota, and memory-only-no-count scenarios. Fixes Project-HAMi#2157 Signed-off-by: Manmath Hatte <manmathcode@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: manmathbh 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a gap in the scheduler admission webhook’s namespace ResourceQuota enforcement so that quota checks apply to non-NVIDIA accelerator backends (e.g., Ascend/Cambricon/Hygon) instead of being hard-skipped for all devices except NVIDIA. This aligns fitResourceQuota() behavior with the already device-agnostic QuotaManager.FitQuota() logic.
Changes:
- Removed the NVIDIA-only guard in
fitResourceQuota()and appliedFitQuota()checks across all registered device backends. - Preserved NVIDIA’s existing
nvidia.MemoryFactorbehavior while defaulting other backends tomemoryFactor=1. - Added webhook unit tests with a stub (mock) Ascend device to verify over-quota/within-quota and a “memory-only without count” edge case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/scheduler/webhook.go | Removes NVIDIA-only quota enforcement and applies quota checks to all registered device backends, keeping NVIDIA memoryFactor behavior. |
| pkg/scheduler/webhook_test.go | Adds a mock Ascend device and test cases to validate quota enforcement for a non-NVIDIA backend. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ascendCountName := "huawei.com/Ascend910B" | ||
| ascendMemName := "huawei.com/Ascend910B-memory" | ||
| device.DevicesMap["Ascend910B"] = &mockDevices{ | ||
| resourceNames: device.ResourceNames{ | ||
| ResourceCountName: ascendCountName, | ||
| ResourceMemoryName: ascendMemName, | ||
| }, | ||
| } |
📝 WalkthroughWalkthroughThe webhook quota-fitting loop now checks all registered device types, using NVIDIA’s memory factor only for NVIDIA devices. Tests add Ascend device registration, quota data, and coverage for exceeded, within-quota, and memory-only requests. ChangesCross-device quota enforcement
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 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/scheduler/webhook_test.go`:
- Around line 302-320: Update the test setup around the Ascend910B device and
default quota entry to save their existing global values before overwriting
them, then restore those values in defer cleanup; only delete each key when no
prior value existed. Ensure both device.DevicesMap and qm.Quotas retain their
original state for subsequent tests.
🪄 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: 9c1e58a6-bd7b-42b9-b828-ab578ebf539b
📒 Files selected for processing (2)
pkg/scheduler/webhook.gopkg/scheduler/webhook_test.go
| device.DevicesMap["Ascend910B"] = &mockDevices{ | ||
| resourceNames: device.ResourceNames{ | ||
| ResourceCountName: ascendCountName, | ||
| ResourceMemoryName: ascendMemName, | ||
| }, | ||
| } | ||
| defer func() { | ||
| delete(device.DevicesMap, "Ascend910B") | ||
| }() | ||
|
|
||
| qm := device.NewQuotaManager() | ||
| ns := "default" | ||
| memName := "nvidia.com/gpumem" | ||
| coreName := "nvidia.com/gpucores" | ||
| nvidiaMemName := "nvidia.com/gpumem" | ||
| nvidiaCoreName := "nvidia.com/gpucores" | ||
|
|
||
| qm.Quotas[ns] = &device.DeviceQuota{ | ||
| memName: &device.Quota{Used: 1000, Limit: 2000}, | ||
| coreName: &device.Quota{Used: 200, Limit: 400}, | ||
| nvidiaMemName: &device.Quota{Used: 1000, Limit: 2000}, | ||
| nvidiaCoreName: &device.Quota{Used: 200, Limit: 400}, | ||
| ascendMemName: &device.Quota{Used: 0, Limit: 1000}, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the global test state instead of deleting it.
Line 302 can overwrite an existing Ascend910B device, but the defer always deletes it; Line 317 also leaves the singleton quota entry for default behind. Save and restore both prior values so later tests cannot become order-dependent.
Proposed fix
+ oldAscend, hadAscend := device.DevicesMap["Ascend910B"]
+ oldQuota, hadQuota := qm.Quotas[ns]
device.DevicesMap["Ascend910B"] = &mockDevices{
resourceNames: device.ResourceNames{
ResourceCountName: ascendCountName,
ResourceMemoryName: ascendMemName,
},
}
defer func() {
- delete(device.DevicesMap, "Ascend910B")
+ if hadAscend {
+ device.DevicesMap["Ascend910B"] = oldAscend
+ } else {
+ delete(device.DevicesMap, "Ascend910B")
+ }
+ if hadQuota {
+ qm.Quotas[ns] = oldQuota
+ } else {
+ delete(qm.Quotas, ns)
+ }
}()🤖 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/scheduler/webhook_test.go` around lines 302 - 320, Update the test setup
around the Ascend910B device and default quota entry to save their existing
global values before overwriting them, then restore those values in defer
cleanup; only delete each key when no prior value existed. Ensure both
device.DevicesMap and qm.Quotas retain their original state for subsequent
tests.
|
the gap is real, but hardcoding memoryfactor=1 for non-nvidia devices is wrong.. quota usage is accumulated from factor-scaled values (ascend/hygon multiply by their configured memoryfactor in generateresourcerequests), the factor must come from each backend's config. |
DSFans2014
left a comment
DSFans2014
left a comment
There was a problem hiding this comment.
@manmathbh thank you for your contribution. it seems duplicated with #2172
|
Didn't see #2172 was open for this. Closing, thanks for the review. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
fitResourceQuota had a hard skip for every device except NVIDIA. Pods
requesting Ascend, Cambricon, Hygon or any other backend could blow right
past namespace ResourceQuota at admission time.
Removed the guard. Non-NVIDIA devices use memoryFactor=1, NVIDIA keeps
nvidia.MemoryFactor as before.
Which issue(s) this PR fixes:
Fixes #2157
Special notes for your reviewer:
FitQuota in pkg/device/quota.go was already device-agnostic — call it with
an Ascend device name and it returns the right answer. The gap was purely
in fitResourceQuota skipping the call for non-NVIDIA.
Test setup registers a stub Ascend device with a 1000mb quota (0 used) and
verifies deny/allow for over/under-quota requests plus a no-count-resource
edge case.
Does this PR introduce a user-facing change?:
Pods requesting non-NVIDIA accelerator memory or core resources are now
checked against namespace ResourceQuota at admission time, same as NVIDIA.
Previously they were silently excluded.
Summary by CodeRabbit