fix(hygon,metax): guard int32 narrowing in GenerateResourceRequests - #2388
Merged
hami-robot[bot] merged 3 commits intoAug 14, 2026
Merged
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:
📝 WalkthroughWalkthroughHygon and Metax resource request generation now rejects negative or int32-overflowing counts, memory values, scaled memory values, and core values. Regression tests cover invalid inputs and memory-factor overflow. ChangesResource range validation
Estimated code review effort: 3 (Moderate) | ~20 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.
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/metax/sdevice.go`:
- Around line 262-266: Update the memory validation in the device request
handling around MemoryFactor so the unscaled value v is checked against
int64(math.MaxInt32)/int64(MemoryFactor) before calculating mem, preventing
overflowing products from wrapping into an apparently valid int32 range.
Preserve rejection of negative or otherwise out-of-range requests, and add a
regression test covering an int64-overflowing product.
🪄 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: 7dceaeac-6df5-427e-be54-b155a55fec7a
📒 Files selected for processing (4)
pkg/device/hygon/device.gopkg/device/hygon/device_test.gopkg/device/metax/sdevice.gopkg/device/metax/sdevice_test.go
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/device/hygon/device_test.go (1)
619-658: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd lower-bound regression cases for both backends.
The new tables cover values above
math.MaxInt32, but the implementations also reject non-positive counts and negative memory or core values.
pkg/device/hygon/device_test.go#L619-L658: add zero and negativehygon.com/dcunumcases and a negativehygon.com/dcumemcase.pkg/device/metax/sdevice_test.go#L468-L490: add zero or negativemetax-tech.com/sgpu, negativemetax-tech.com/vcore, and negative unitlessmetax-tech.com/vmemorycases.Based on the supplied backend implementation snippets, these lower-bound branches are not exercised in the new tables.
🤖 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/hygon/device_test.go` around lines 619 - 658, The out-of-range regression tables only cover upper bounds; add lower-bound cases for both backends. In pkg/device/hygon/device_test.go lines 619-658, extend Test_GenerateResourceRequests_OutOfRangeValues with zero and negative hynog.com/dcunum values plus a negative hynog.com/dcumem case, expecting an empty request. In pkg/device/metax/sdevice_test.go lines 468-490, add zero or negative metax-tech.com/sgpu, negative metax-tech.com/vcore, and negative unitless metax-tech.com/vmemory cases, likewise expecting the existing empty-request result.
🤖 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/hygon/device_test.go`:
- Around line 619-658: The out-of-range regression tables only cover upper
bounds; add lower-bound cases for both backends. In
pkg/device/hygon/device_test.go lines 619-658, extend
Test_GenerateResourceRequests_OutOfRangeValues with zero and negative
hynog.com/dcunum values plus a negative hynog.com/dcumem case, expecting an
empty request. In pkg/device/metax/sdevice_test.go lines 468-490, add zero or
negative metax-tech.com/sgpu, negative metax-tech.com/vcore, and negative
unitless metax-tech.com/vmemory cases, likewise expecting the existing
empty-request result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a79f8eaf-dc6f-4424-a1e3-c657e8eacf81
📒 Files selected for processing (3)
pkg/device/hygon/device_test.gopkg/device/metax/sdevice.gopkg/device/metax/sdevice_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/device/metax/sdevice.go
archlitchi
reviewed
Aug 6, 2026
Eshiv-Pandey
force-pushed
the
fix/int32-overflow-hygon-metax-sgpu
branch
from
August 6, 2026 10:30
754ae25 to
10279bf
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. |
GenerateResourceRequests in the hygon and metax-sgpu backends converts
user-controlled int64 resource limits (count, memory, cores) into int32
fields on ContainerDeviceRequest (Nums, Memreq, Coresreq) without bounds
checking. A limit exceeding math.MaxInt32 silently wraps, producing an
incorrect small or negative value that flows into Fit().
For example, hygon.com/dcumem: 16Gi yields AsInt64() = 17179869184. After
applying MemoryFactor, int32(result) wraps to 0, causing the scheduler to
treat the device as having sufficient memory when it does not.
Add range guards that return empty ContainerDeviceRequest{} (the existing
reject sentinel) before any int32() narrowing, matching the overflow
protection already in enflame, iluvatar, mthreads, and amd backends.
Add test coverage for pre-factor overflow (16Gi direct) and post-factor
overflow (3000000 * 1024 > MaxInt32) in hygon, and out-of-range memory
in metax-sgpu.
AI assistance disclosure: I used an AI tool for codebase navigation and review support.
Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
The int32 range check ran after `mem = v * int64(MemoryFactor)`, so the multiplication could itself overflow int64 and wrap to a small in-range value that slipped past the check. Validate `v` against `math.MaxInt32 / MemoryFactor` before multiplying in the no-unit branch. Also add regression tests for the reachable overflow guards in both backends (device count and core out-of-range, negative core), raising patch coverage of the new validation paths. AI assistance disclosure: I used an AI tool for codebase navigation and review support. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
Address review feedback: drop the explanatory comments on the pre-factor overflow guard in GenerateResourceRequests. The check is self-explanatory and the surrounding backends keep these guards uncommented. AI assistance disclosure: I used an AI tool for codebase navigation and review support. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
Eshiv-Pandey
force-pushed
the
fix/int32-overflow-hygon-metax-sgpu
branch
from
August 6, 2026 11:00
10279bf to
d987f40
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. |
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/hygon/device.go`:
- Around line 192-202: Update the DCU resource quantity conversions for memory
and core requests to check the success flag returned by AsInt64() before using
the values. Reject non-integer, decimal, and negative quantities instead of
allowing memnums or corenum to retain their defaults, applying the same
validation to the later conversion around the core handling logic. Preserve the
existing range and overflow checks after successful conversion.
🪄 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: 33b0e4d5-2abc-41f4-a991-69716e7bcf39
📒 Files selected for processing (4)
pkg/device/hygon/device.gopkg/device/hygon/device_test.gopkg/device/metax/sdevice.gopkg/device/metax/sdevice_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/device/metax/sdevice_test.go
- pkg/device/hygon/device_test.go
- pkg/device/metax/sdevice.go
Closed
Contributor
|
/lgtm |
This was referenced Aug 11, 2026
ascend: GenerateResourceRequests narrows user-controlled values to int32 without bounds checks
#2600
Closed
Contributor
|
[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 |
Contributor
|
/lgtm |
hami-robot Bot
pushed a commit
that referenced
this pull request
Aug 18, 2026
* fix(ascend): guard int32 narrowing in GenerateResourceRequests
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 #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>
* fix(ascend): reject negative memory quantities before AsInt64
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>
* test(ascend): shorten verbose comments in device_test.go
Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
* fix(ascend): add regression test for negative whole memory request
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>
---------
Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
This was referenced Aug 18, 2026
nvidia: GenerateResourceRequests narrows user-controlled values to int32 without bounds checks
#2718
Closed
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:
GenerateResourceRequestsin the hygon and metax-sgpu device backends converts user-controlledint64resource limits (device count, memory, cores) intoint32fields onContainerDeviceRequest(Nums,Memreq,Coresreq) without bounds checking. A resource limit exceedingmath.MaxInt32silently wraps to an incorrect small or negative value that flows intoFit(), causing the scheduler to incorrectly assess device availability and potentially schedule pods onto overcommitted devices.This PR adds range guards that return an empty
ContainerDeviceRequest{}(the existing rejection sentinel) before anyint32()narrowing, matching the overflow protection already implemented in the enflame, iluvatar, mthreads, and amd backends.Which issue(s) this PR fixes:
Fixes #2383
Special notes for your reviewer:
MemoryFactormultiplication).16GiexceedsMaxInt32directly) and post-factor overflow (raw value3000000fits in int32, but3000000 * 1024overflows), plus an out-of-range memory case for metax-sgpu.Does this PR introduce a user-facing change?:
AI assistance disclosure: I used an AI tool for codebase navigation and review support.
Summary by CodeRabbit