fix(device): re-check ResourceQuota in Fit() for non-NVIDIA backends - #2377
fix(device): re-check ResourceQuota in Fit() for non-NVIDIA backends#2377Lakshya77089 wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDevice fitting now validates pod-level memory and core quotas across accelerator backends. Shared quota accounting includes temporary and allocated device usage. Tests cover quota limits, scaling factors, unrestricted namespaces, nil allocations, and topology selection. ChangesResource quota enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DeviceFit
participant FitQuotaWithPodDevices
participant LocalQuotaManager
DeviceFit->>FitQuotaWithPodDevices: submit requested resources and selected devices
FitQuotaWithPodDevices->>LocalQuotaManager: evaluate combined quota usage
LocalQuotaManager-->>FitQuotaWithPodDevices: return quota fit result
FitQuotaWithPodDevices-->>DeviceFit: accept or reject candidate
Possibly related issues
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.
Actionable comments posted: 2
🤖 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/ascend/device.go`:
- Around line 515-520: Move the FitQuotaWithPodDevices call out of the
provisional topology-selection loop, so candidate cards are not quota-checked or
charged before computeBestCombination selects originReq. After the best topology
combination is chosen, validate that selected combination with
FitQuotaWithPodDevices and apply ResourceQuotaNotFit handling there, preserving
the existing quota accounting for the final devices only.
In `@pkg/device/quota.go`:
- Around line 101-117: Make the quota check-and-use operation atomic across the
scheduling flow rooted at FitQuotaWithPodDevices: serialize concurrent checks
and subsequent usage updates for the same namespace and device, or reserve quota
atomically once device allocation is finalized. Ensure the lock or reservation
spans FitQuota through the later AddUsage call so concurrent Fit operations
cannot overlap quota usage.
🪄 Autofix
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: 0a4cc570-2f4b-4977-b26a-f1c8dc6d2411
📒 Files selected for processing (9)
pkg/device/ascend/device.gopkg/device/cambricon/device.gopkg/device/cambricon/device_test.gopkg/device/hygon/device.gopkg/device/iluvatar/device.gopkg/device/mthreads/device.gopkg/device/nvidia/device.gopkg/device/quota.gopkg/device/quota_test.go
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:
|
557f315 to
a038cac
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. |
a038cac to
2e13cb2
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. |
|
please resolve these conflicts |
2e13cb2 to
c0ea46b
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. |
| qm := device.NewQuotaManager() | ||
| t.Cleanup(func() { delete(qm.Quotas, "ascend-topo") }) | ||
|
|
||
| // Room for the two cards actually requested (32768) but not for all four |
|
/assign |
c0ea46b to
d36803a
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Lakshya77089 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 |
|
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. |
|
please sync with master to pass the UT |
d36803a to
8f2e568
Compare
|
Thanks for the review — both done. Removed the comments inside method bodies, 25 lines across While rebasing I also had to update the quota fixtures in my tests. #2313 changed Rebased onto c5e99b6 and CI is green. |
7b23487 to
92f6754
Compare
|
@archlitchi gentle ping on this one when you have a moment. The in-body comments you asked about are removed, and I've rebased onto current master — CI is green. |
| } | ||
| } | ||
| } | ||
| return GetLocalCache().FitQuota(ns, mem, memoryFactor, core, deviceName) |
There was a problem hiding this comment.
@Lakshya77089 I think we shouldn't remove the log message in the previouse code.
Usage is only recorded at Filter time, so pods created together all pass admission against the same figure. nvidia re-checked in Fit(); no other backend did, so a burst could schedule past a namespace limit. Move nvidia's fitQuota helper into pkg/device as FitQuotaWithPodDevices and call it from ascend, cambricon, hygon, iluvatar and mthreads, which share nvidia's Fit() shape. The other backends have a different shape and are left for a follow-up. Refs Project-HAMi#2363 Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com>
92f6754 to
64736f0
Compare
|
@Shouren good catch — that was an accidental drop when the helper moved out of the nvidia package, not intentional. Restored it in |
|
You can view the relevant rule here. |
4 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Follow-up to #2347, from @mesutoezdil's review there. #2347 made the admission
webhook enforce
ResourceQuotafor every backend. Admission alone is not enough.Namespace usage is only recorded by
QuotaManager.AddUsageat Filter time, notat admission. So a burst of pods created together all reach the webhook before
any of them has been scheduled, all read the same value of
Used, and all pass.NVIDIA caught this on the way through
Fit()— by the time the second pod isscored, the first one's usage has been recorded. No other backend had that
second gate, so the whole burst schedules and the namespace ends up over its
limit.
Reproduces as a five-replica Deployment against a quota sized for two pods: on
nvidia.com/gpumemthe extra replicas are denied, oncambricon.com/mlu.smlu.vmemoryall five schedule.fitQuotawas the only implementation and it lived inpkg/device/nvidia,hardcoded to
NvidiaGPUDeviceandMemoryFactor. Both of those are nowper-backend values after #2347, so the helper moves to
pkg/deviceasFitQuotaWithPodDevicesand takes them as arguments. NVIDIA keeps itsone-line wrapper and behaves exactly as before.
The five backends that share NVIDIA's
Fit()shape — ascend, cambricon, hygon,iluvatar, mthreads — call it at the same point NVIDIA always has: after
memreqis resolved, before the per-card capacity checks.
The helper counts cards already picked for this pod, both the ones in
tmpDevsand the ones in
allocated, so a multi-card request is weighed as a wholerather than waved through one card at a time.
Tests
TestFitQuotaWithPodDevicesandTestFitQuotaWithPodDevicesNilAllocatedinpkg/device/quota_test.gocover the accumulation acrosstmpDevsandallocated, cores exceeding on their own, the memory factor raising the limit,and a namespace with no quota.
TestDevices_FitResourceQuotainpkg/device/cambricon/device_test.gois theone that matters — it drives the real
Fit()and asserts a card with plenty offree memory is still refused when the namespace is out of quota, with
ResourceQuotaNotFitin the reason. It fails on master and passes here.Which issue(s) this PR fixes:
Refs #2363
Deliberately
Refsrather thanFixes, because #2363 also covers the backendsbelow.
Special notes for your reviewer:
Scope. amd, awsneuron, enflame, kunlun vdevice and metax sgpu also expose
quota-capable resources, but their
Fit()is shaped differently — no sharedmemreqresolution block to hang the check on. Wiring them up means readingeach allocation loop on its own terms, and I would rather not bury five
one-line insertions and five bespoke ones in the same diff. #2363 stays open
for them.
The bigger question from #2363 is still open and I have not tried to answer it
here: reserving quota at admission instead of at Filter would close the window
for every backend at once, rather than adding a gate per backend. That is a
larger change and wants your input before anyone writes it. This PR is the
incremental step, and the shared helper is needed either way.
One behaviour note: a pod that previously scheduled while its namespace was
over quota will now be refused on these five backends, with
ResourceQuotaNotFitin the filter reason. That is the fix working, but it isa visible change for anyone who had been relying on the quota not being
enforced.
Not validated on real hardware. The change is in the scheduler extender's Fit
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, the shared helper, and the tests. Flagging the extent up front per
CONTRIBUTING.
Summary by CodeRabbit