fix(ascend): reject over-capacity memory requests instead of defaulting to whole card - #2543
fix(ascend): reject over-capacity memory requests instead of defaulting to whole card#2543Neal006 wants to merge 3 commits into
Conversation
…ng to whole card trimMemory returns 0 when a request exceeds every template and the card capacity. GenerateResourceRequests dropped that zero, and the whole-card default a few lines below reads a zero as "no memory requested", so an unsatisfiable request became a request for 100% of a card and scheduled. MutateAdmission already rejects this on the limits path, but it returns early when the device count is absent from limits, so a pod declaring resources under requests only never reaches that check. Carry the requested value through when trimMemory returns 0 so Fit rejects it, clamped to MaxInt32 so the int32 narrowing cannot wrap it back to zero. Signed-off-by: Neal006 <nealdaftary0405@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Neal006 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 |
|
Welcome @Neal006! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAscend resource generation now preserves oversized memory requests with an ChangesAscend memory request handling
Estimated code review effort: 2 (Simple) | ~10 minutes 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.
🧹 Nitpick comments (1)
pkg/device/ascend/device_test.go (1)
1214-1231: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression cases for the int32 cap and the
Limitspath.The new case proves that
65536is preserved, but that value fits inint32, so it does not test the newmath.MaxInt32clamp. It also exercises onlyRequests, while the PR objective requires the same behavior forLimits. Add oneLimits-only over-capacity case and one request abovemath.MaxInt32that expectsMemreq == math.MaxInt32.Example int32-boundary case
+ { + name: "resourcememoryname above int32 max is capped", + args: corev1.Container{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + "huawei.com/Ascend910A": resource.MustParse("1"), + "huawei.com/Ascend910A-memory": resource.MustParse("2147483648"), + }, + }, + }, + want: device.ContainerDeviceRequest{ + Nums: int32(1), + Type: "Ascend910A", + Memreq: int32(2147483647), + MemPercentagereq: int32(0), + Coresreq: int32(0), + }, + },🤖 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/ascend/device_test.go` around lines 1214 - 1231, Add regression coverage in the device request test cases around the existing resourcememoryname scenario: add a Limits-only case with an over-capacity memory request, and add a Requests case whose memory value exceeds math.MaxInt32 and expects Memreq to equal math.MaxInt32. Import or reference math.MaxInt32 consistently with the test package’s conventions.
🤖 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/ascend/device_test.go`:
- Around line 1214-1231: Add regression coverage in the device request test
cases around the existing resourcememoryname scenario: add a Limits-only case
with an over-capacity memory request, and add a Requests case whose memory value
exceeds math.MaxInt32 and expects Memreq to equal math.MaxInt32. Import or
reference math.MaxInt32 consistently with the test package’s conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b451805e-a267-4996-8479-44857cb24f52
📒 Files selected for processing (2)
pkg/device/ascend/device.gopkg/device/ascend/device_test.go
| // request. Carry the requested value through so Fit | ||
| // rejects it, rather than letting the zero fall | ||
| // through to the whole-card default below. | ||
| m = min(memnums, math.MaxInt32) |
There was a problem hiding this comment.
no test covers this maxint32 clamp path. can u add a case with a request above int32 max?
There was a problem hiding this comment.
Covered in 9432a48. The case requests 2147483648 and expects Memreq to equal math.MaxInt32.
The first regression case used 65536, which fits in int32 and so never reached the clamp. Add a request above int32 max that expects MaxInt32, and a limits-only case so both resource shapes are covered. Signed-off-by: Neal006 <nealdaftary0405@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
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:
|
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
left two small questions inline. checked the fix against fit and trimMemory, logic looks correct.
| @@ -339,6 +340,13 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.Conta | |||
| memnum = int(memnums) | |||
| } else { | |||
| m, _ := dev.trimMemory(memnums) | |||
There was a problem hiding this comment.
this only fixes the scheduler side. pod still passes admission, then fails later at fit. is that fine, or should mutateadmission also fall back to requests, like #2532 asks?
There was a problem hiding this comment.
Yes, that is deliberate. Failing at Fit is the correct outcome either way, since the pod cannot run on any card. What this PR removes is the silent success, where the pod scheduled and was accounted as 100% of a card.
MutateAdmission reading requests as well is the other half of #2532, but it changes behaviour for every requests-only Ascend pod, not just the over-capacity ones. That is a wider blast radius than this fix and it deserves its own review. I would rather keep this PR to the scheduler path and send the admission change separately.
Happy to fold it in here instead if a maintainer prefers one PR.
| // request. Carry the requested value through so Fit | ||
| // rejects it, rather than letting the zero fall | ||
| // through to the whole-card default below. | ||
| m = min(memnums, math.MaxInt32) |
There was a problem hiding this comment.
test does not set memoryfactor above 1. does the clamp still force a reject after the factor scaling runs?
There was a problem hiding this comment.
Good catch, added a case in cc01a6f.
The factor is applied at line 324, before trimMemory runs, so trimMemory and the clamp both see the already scaled value. A larger factor pushes the request further over capacity, never under it, so the reject holds.
The new case in Test_GenerateResourceRequestsFactor uses factor 1000 against the shared 128 MiB request. That scales to 128000, which exceeds the 32768 MemoryCapacity, and the test asserts Memreq stays 128000 with MemPercentagereq 0. Before this fix that same input produced Memreq 0 and MemPercentagereq 100, which Fit reads as the whole card.
memoryFactor scaling runs before trimMemory, so the clamp sees the already scaled value. Add a case where the factor is what pushes the request past MemoryCapacity, to pin that the scaled value is carried through instead of falling back to the whole-card default. Signed-off-by: Neal006 <nealdaftary0405@gmail.com>
|
This is being closed because it does not comply with the contribution guidelines. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
trimMemoryreturns0when an Ascend memory request exceeds every template and the card capacity.GenerateResourceRequestsdropped that zero, and the whole-card default below it reads a zero as "no memory requested", so an unsatisfiable request became a request for 100% of a card and scheduled.MutateAdmissionalready rejects this on the limits path, but it returns early when the device count is absent fromlimits, so a pod declaring resources underrequestsonly never reaches that check.This carries the requested value through when
trimMemoryreturns0, soFitrejects it. The value is clamped toMaxInt32because theint32narrowing at the return would otherwise wrap a large quantity back to zero and reintroduce the same bug.Which issue(s) this PR fixes:
Fixes #2532
Special notes for your reviewer:
#2532 offers a second approach: make
MutateAdmissionfall back torequestsso the existing rejection covers both shapes. That closes the bypass at its root, but it changes admission behaviour for every Ascend pod, so I took the narrower change here. Happy to switch to it or add it alongside if you prefer.Verified with a regression case added to the existing
Test_GenerateResourceRequeststable. Allpkg/device/...packages pass. I could not run--raceor buildpkg/device/nvidialocally because both need Linux, so CI covers those.Per CONTRIBUTING.md, I used AI assistance while investigating and preparing this change. I verified the behaviour myself and can explain it.
Does this PR introduce a user-facing change?:
An Ascend memory request larger than the device capacity is now rejected instead of being silently scheduled as a whole-card request.
Summary by CodeRabbit