fix(ascend): guard int32 narrowing in GenerateResourceRequests - #2601
Conversation
GenerateResourceRequests narrowed user-controlled int64 limits (device
count, memory, cores) into the int32 fields of ContainerDeviceRequest
without bounds checking. A value over math.MaxInt32 silently wrapped to a
small or negative number that flowed into Fit(), letting the scheduler
place pods onto overcommitted NPUs. A byte-suffixed memory quantity such
as 16Gi wrapped to 0 on the soft-partitioning path, and MemPercentagereq
then defaulted to 0 as well, so the fit check could never reject it.
Guard every narrowing point before the int32 cast, returning the existing
empty ContainerDeviceRequest{} rejection sentinel, matching the pattern
from Project-HAMi#2388 (hygon, metax-sgpu). Memory guards cover both the pre-factor
value and the value after MemoryFactor scaling, on the soft-partitioning
and trimMemory paths alike. The memory error messages state that Ascend
memory is treated as MB not Byte, since a request like 16Gi is almost
always a wrong-unit mistake.
Signed-off-by: Eshiv Pandey <eshivpandey18@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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAscend resource request generation now validates device, memory, and core values against nonnegative ChangesAscend resource validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change is localized to rejecting oversized or invalid Ascend resource requests before narrowing, with no actionable merge-blocking risk remaining beyond normal checks and review. 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: 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/device/ascend/device.go`:
- Around line 331-334: Update the memory validation around mem.AsInt64() to
reject negative quantities using mem.Sign() < 0 before converting or evaluating
memnums, ensuring values such as -1m do not produce a valid 100% request.
Preserve the existing out-of-range handling for non-negative quantities and add
a regression case covering -1m.
🪄 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: 2807a772-85d2-44e3-a0e7-94ee4ab7efc6
📒 Files selected for processing (2)
pkg/device/ascend/device.gopkg/device/ascend/device_test.go
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
A negative fractional quantity such as -1m returns ok=false from AsInt64, so it bypassed the range guard and silently defaulted to a 100% memory request. Check mem.Sign() before AsInt64 to reject all negative quantities, and drop the now-redundant memnums < 0 branch. Add a -1m regression case and shorten the explanatory comments. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
The -1m test only exercised the fractional negative path where AsInt64 returns ok=false. A plain whole -100 passes AsInt64 (ok=true), so the old int32 overflow guard alone would never catch it. This pins the Sign() < 0 check to the whole-number case, directly covering the case the reviewer asked about. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, Eshiv-Pandey 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 type of PR is this?
/kind bug
What this PR does / why we need it:
GenerateResourceRequestsin the ascend backend converts user-controlledint64resource limits (device count, memory, cores) into theint32fields ofContainerDeviceRequest(Nums,Memreq,Coresreq) without bounds checking. A value exceedingmath.MaxInt32silently wraps to a small or negative number that flows intoFit(). A memory request that wraps to 0 makes theFit()check (Totalmem - Usedmem < memreq) always pass, so the scheduler can place a pod onto an already-full NPU, causing oversubscription and OOM for other workloads on the same card.This PR adds range guards that return the existing empty
ContainerDeviceRequest{}rejection sentinel before everyint32()narrowing, matching the pattern from #2388 (hygon, metax-sgpu). Memory is guarded both before and after theMemoryFactormultiplication, covering the soft-partitioning path (raw value) and thetrimMemorypath.Which issue(s) this PR fixes:
Fixes #2600
Special notes for your reviewer:
MemoryFactorscaling), and cores.16Giinstead of an integer number of MB). The memory error messages state that Ascend memory is treated as MB not Byte, and that16384should be requested for 16 GB.trimMemoryreturns 0), which is belowmath.MaxInt32and out of scope here.16Gion both the soft-partitioning and trim paths), post-factor overflow afterMemoryFactor, oversized device count, and negative and oversized core requests. Each was confirmed to fail before the guard and pass after.Does this PR introduce a user-facing change?:
AI assistance disclosure: I used AI assistance (Claude Code) to help audit the backends, implement the guards, and draft the regression tests. I reviewed the change and verified the new tests fail without the guards and pass with them.
Summary by CodeRabbit
Bug Fixes
Tests