fix(scheduler): enforce ResourceQuota for every device backend - #2347
Merged
hami-robot[bot] merged 2 commits intoAug 5, 2026
Merged
Conversation
Contributor
|
Welcome @Lakshya77089! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
📝 WalkthroughWalkthroughThis PR adds ChangesMemoryFactor propagation and generalized quota enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Pod
participant Webhook as fitResourceQuota
participant Backend as Device Backend
participant QuotaManager
Pod->>Webhook: submit pod specification
Webhook->>Backend: GenerateResourceRequests(container)
Backend-->>Webhook: memory, core, MemoryFactor
Webhook->>QuotaManager: FitQuota(namespace, memory, core, MemoryFactor)
QuotaManager-->>Webhook: allow or deny
Webhook-->>Pod: admission decision
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 |
Lakshya77089
force-pushed
the
fix/multi-vendor-resource-quota
branch
from
August 4, 2026 09:28
645bc5b to
5cb42f4
Compare
cambricon, iluvatar, mthreads and metax each multiply the vmemory value from the pod spec by a fixed number to get the MiB accounted internally. The numbers were bare literals repeated between GenerateResourceRequests and GetNodeDevices. Give each backend a named MemoryFactor constant so the scale is stated once. No behaviour change. Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com>
fitResourceQuota skipped every backend except NVIDIA, so a namespace ResourceQuota on cambricon, ascend, hygon or any other vendor's memory and core resources was accepted by the apiserver and then ignored. Units are why this is not simply a wider loop. Backends scale the pod's memory value before recording it as used, by a configured memoryFactor for nvidia, ascend and hygon and by a fixed 256, 512 or 1024 elsewhere, while a ResourceQuota limit is unscaled. Each backend now reports its scale through ResourceNames.MemoryFactor, and the webhook calls the backend's GenerateResourceRequests rather than parsing the container spec, so admission and the scheduler work from the same numbers. Fixes Project-HAMi#2157 Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com>
Lakshya77089
force-pushed
the
fix/multi-vendor-resource-quota
branch
from
August 4, 2026 09:34
5cb42f4 to
e9454b6
Compare
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
|
/lgtm |
Contributor
|
fit() path is still nvidia only for quota, non-nvidia has a race window between admission and scheduling. pls open a followup issue instead of letting fixes #2157 close it fully. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, Lakshya77089 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 |
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:
fitResourceQuotain the admission webhook only ever looked at NVIDIA:So a namespace
ResourceQuotaoncambricon.com/mlu.smlu.vmemory,huawei.com/Ascend910B-memory,hygon.com/dcumemor any other vendor resourcewas accepted by the apiserver and then ignored. Pods were admitted no matter how
much of that quota the namespace had already used.
QuotaManageritself isdevice agnostic and works fine when called directly, so the gap is entirely in
the wiring.
Two commits:
refactor(device): name the vmemory scaling constants— no behaviour change,pulled out so the second diff is readable.
fix(scheduler): enforce ResourceQuota for every device backend— the fix.The unit problem, and why the loop could not just be widened
This is where the earlier attempt in #2218 came unstuck, and @mesutoezdil's
review there is what shaped this patch. Several backends scale the memory value
from the pod spec before recording it as used:
memoryFactorRecorded usage is in scaled units. A
ResourceQuotalimit is written inunscaled ones. Compare them directly and a pod asking for 50 units of MLU
vmemory against a 100 unit limit is measured as 12800 against 100 and denied,
even though it is well inside quota. Hardcoding
memoryFactor = 1fornon-NVIDIA backends, which is what #2218 did, produces exactly that.
So each backend now reports its own scale through a new
ResourceNames.MemoryFactor, andFitQuotaraises the limit by it — the samething it already did for NVIDIA. Backends that do not scale leave it zero.
The webhook also stops re-reading the container spec by hand and asks the
backend instead:
That is the same call the scheduler makes on the Fit path, so admission and the
scheduler now work from identical numbers, including per-backend defaults and
Ascend's template rounding. It also drops about 25 lines of duplicated
limits/requests parsing.
Tests
TestFitResourceQuotaNonNvidia,TestFitResourceQuotaCountsEveryDeviceandTestFitResourceQuotaAscendMemoryFactorinpkg/scheduler/webhook_test.go.They cover deny and allow for MLU memory, MLU cores, DCU memory with
memoryFactor: 2, Ascend withmemoryFactor: 4, multi-device requests, and anamespace with no quota. All five deny cases fail on master and pass with this
change.
TestFitResourceQuotaAscendMemoryFactorasserts the backend actually registeredbefore running, because the existing
TestFitResourceQuota/request_ascendcasepasses vacuously today — Ascend is behind
--enable-ascendand was never inDevicesMapfor that test.Which issue(s) this PR fixes:
Fixes #2157
Special notes for your reviewer:
Two behaviour changes worth a second look:
defaultMemorywhen a pod asks fornvidia.com/gpuwithoutnvidia.com/gpumem. The old code only counted memorythe pod named explicitly, which under-counted against what the scheduler
actually records. Default config has
defaultMemory: 0, so most deploymentssee no difference.
100) now have those cores counted. Same reasoning: that is what ends up in
Usedcores.Deliberately left out, happy to add if you would rather see them here:
FitQuotaon the schedulerFit()path is still NVIDIA only. This PR fixesadmission. Doing
Fit()properly means touching every backend's allocationloop and I did not want to bury the webhook fix in that. Tracked separately in
Quota is only re-checked in Fit() for NVIDIA, leaving a race window for every other backend #2363, which also covers the admission-to-scheduling race @mesutoezdil raised
below.
vmemoryvalue as Gi but a suffixed one as bytes. The 1024factor is right for the bare form, which is what a
ResourceQuotauses. Aquota written as
16Giwould still be wrong, but that isAddQuota'shandling of suffixed quantities and predates this change.
Not validated on real hardware. The change is confined to the scheduler
extender's admission path and is covered by unit tests, which CONTRIBUTING
allows for scheduler-scoped changes.
Does this PR introduce a user-facing change?:
AI assistance disclosure: this change was developed with Claude Code — codebase
exploration, working through the per-backend unit scaling, the tests, and this
description. Flagging the extent up front per CONTRIBUTING.