fix(device): reject int32-overflowing memory requests in iluvatar and mthreads - #2285
Conversation
… mthreads GenerateResourceRequests multiplied the requested memory by a per-backend factor (x256 for iluvatar, x512 for mthreads) and narrowed the result to int32. A Gi-scale quantity like 16Gi wraps past math.MaxInt32 to Memreq: 0, which passes the Totalmem-Usedmem < memreq check vacuously and lets the pod schedule onto a full device (oversubscription / OOM). Guard the memory value against the int32 range and reject the request when it overflows, mirroring the device-count guard already present in mthreads (Project-HAMi#2134) and the enflame int32 bounding in Project-HAMi#2145 / Project-HAMi#2190. Adds a regression case to each backend's Test_GenerateResourceRequests. Fixes Project-HAMi#2284 Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
|
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:
📝 WalkthroughWalkthroughIluvatar, Mthreads, Cambricon, and NVIDIA now reject invalid, negative, and overflowing memory requests before ChangesDevice memory 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/iluvatar/device.go`:
- Around line 220-224: Validate memnums before converting or multiplying in both
backends: in pkg/device/iluvatar/device.go at lines 220-224, reject values above
int64(math.MaxInt32)/256 before the int(memnums)*256 calculation; in
pkg/device/mthreads/device.go at lines 225-229, apply the corresponding
int64(math.MaxInt32)/512 check, then divide the validated product by n before
assigning Memreq. Preserve the existing rejection behavior and container
context.
🪄 Autofix (Beta)
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: 957c20e8-c92f-47b0-acf6-87ebcde17d5b
📒 Files selected for processing (4)
pkg/device/iluvatar/device.gopkg/device/iluvatar/device_test.gopkg/device/mthreads/device.gopkg/device/mthreads/device_test.go
Move the int32 range check ahead of the int(memnums)*factor multiplication so an extreme AsInt64 value cannot overflow the intermediate product before it is checked. Bounds memnums against int64(math.MaxInt32)/factor, matching the bound-before-conversion approach in enflame (Project-HAMi#2145). Addresses CodeRabbit review feedback; behavior and tests unchanged. Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.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.
🚀 New features to boost your workflow:
|
|
Cambricon has the same overflow class and is tracked separately in #2278, with no open PR yet. Given you already have the pattern worked out here (negative/overflow guard before the int32 narrowing), would you be open to extending this PR to cover cambricon too, or opening a follow-up PR for it? |
Cambricon's GenerateResourceRequests has the same int32 overflow as iluvatar and mthreads: memory is multiplied by 256 and narrowed to int32, so a Gi-scale request wraps to Memreq: 0 and passes the memory fit check vacuously. Apply the same bound-before-multiply guard and add a regression case. Requested by a reviewer on the PR; also closes Project-HAMi#2278. Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
|
thanks — extended it to cambricon in bf214f2, same guard (bound before the ×256) plus a regression case, and linked #2278 so it closes on merge. one thing I noticed while doing this: nvidia's GenerateResourceRequests has the same int32(memnum) narrowing. for nvidia it's only reachable via a misconfigured Gi value or a large MemoryFactor since it doesn't multiply by a fixed unit, so it's a weaker case — happy to fold it in here too or leave it out to keep this focused. your call. |
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/cambricon/device.go`:
- Around line 255-259: The Cambricon memory-request validation must reject
decimal-backed quantities instead of leaving memnum at zero when
Quantity.AsInt64() fails. In pkg/device/cambricon/device.go at lines 255-259,
handle AsInt64 failure using Sign()/CmpInt64() and return the existing empty
ContainerDeviceRequest for invalid or out-of-range values; in
pkg/device/cambricon/device_test.go at lines 290-304, add regression coverage
for a decimal resource such as 16.0Gi and verify it is rejected.
🪄 Autofix (Beta)
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: ecfb6d04-2f63-447b-8b56-b28848488f2b
📒 Files selected for processing (2)
pkg/device/cambricon/device.gopkg/device/cambricon/device_test.go
|
cc @archlitchi wdyt, fold nvidia's same int32 narrowing into this PR too, or keep it separate? |
i think we should narrow them into this PR too |
|
please resolve these conflicts and resolve the code-rabbit comment |
Resolve conflicts in cambricon/iluvatar/mthreads GenerateResourceRequests: master moved the hardcoded memory factor to the new MemoryFactor const; keep that and re-apply the overflow guard on top. Per review on Project-HAMi#2285: - extend the guard to nvidia's GenerateResourceRequests (int32(memnum)) - reject decimal-form quantities: AsInt64 returns false for values like 16.0Gi, which previously fell through and left the request at zero - add decimal-form regression cases for all four backends Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/device/nvidia/device_test.go (1)
2016-2039: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the scaled overflow boundary.
Lines 2016-2039 use
MemoryFactor: 1. They only test a value that already exceedsint32before scaling. Add aMemoryFactor: 2case with1Gi, which fits before scaling but exceedsint32after scaling. Expect an emptyContainerDeviceRequest. This protects the bound-before-multiply check at line 549.🤖 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/nvidia/device_test.go` around lines 2016 - 2039, Extend the relevant device request test cases to cover scaled overflow by adding a MemoryFactor: 2 scenario with a 1Gi memory request, which fits before scaling but must be rejected after multiplication. Assert an empty device.ContainerDeviceRequest and target the test setup exercising the bound-before-multiply logic.
🤖 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/nvidia/device_test.go`:
- Around line 2016-2039: Extend the relevant device request test cases to cover
scaled overflow by adding a MemoryFactor: 2 scenario with a 1Gi memory request,
which fits before scaling but must be rejected after multiplication. Assert an
empty device.ContainerDeviceRequest and target the test setup exercising the
bound-before-multiply logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8947ca5d-9888-41f2-a298-43fe49bbb477
📒 Files selected for processing (8)
pkg/device/cambricon/device.gopkg/device/cambricon/device_test.gopkg/device/iluvatar/device.gopkg/device/iluvatar/device_test.gopkg/device/mthreads/device.gopkg/device/mthreads/device_test.gopkg/device/nvidia/device.gopkg/device/nvidia/device_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/device/iluvatar/device_test.go
- pkg/device/cambricon/device_test.go
- pkg/device/mthreads/device.go
- pkg/device/cambricon/device.go
- pkg/device/iluvatar/device.go
- pkg/device/mthreads/device_test.go
|
pushed the changes. nvidia's in now, the guard bounds the value before the int32(memnum) narrowing there too. resolved the conflicts against master (kept the new MemoryFactor const and put the overflow check back on top of it). for the coderabbit comment: the guard used to sit inside the AsInt64 ok-branch, so a decimal quantity like 16.0Gi (where AsInt64 returns false) slipped past it and left the request at 0. it rejects on !parsed or out-of-range now. added tests for both 16Gi and 16.0Gi across the four backends. one thing on the red Unit test check, that's not coming from this PR. master itself is failing the scheduler quota tests right now because my #2313 and #2347 conflict once they're both on master (each passed CI on its own base). fix is up in #2408, it's test-only. once that merges i'll re-merge master here and this should go green. |
The test case names already state what each case covers, and the rest of these tables carry no comments. Addresses review feedback. Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
|
You can view the relevant rule here. |
|
@mesutoezdil , actually I write and give it to ai every time I write to polish and make the content shorter , isn't that allowed the whole thinking and the idea is mine . if its not allowed I'll write. it in my own words this time .! |
…rflow Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
|
@archlitchi , resolved the conflicts . |
|
@veyron-kairo I think we should not only reject the memory requests over int32, but also remind the users that they may used invalid unit. As your example shown, this condition happens on the user input like 16Gi not an integer over int32. So I suggest update the error message to include "Memory unit is treated as MB not Byte". |
|
title only says iluvatar/mthreads, but nvidia and cambricon are in scope too, fix the title |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, veyron-kairo 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 |
4 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
GenerateResourceRequestsin the nvidia, cambricon, iluvatar, and mthreadsbackends multiplies the requested device memory by a per-backend factor and
narrows the result to
int32. A normal Gi-scale quantity like16Giwraps pastmath.MaxInt32toMemreq: 0, which passes theTotalmem - Usedmem < memreqcheck in
Fitvacuously, so the pod is scheduled onto an already-full device —oversubscription and OOM.
This bounds the memory value before the multiply and rejects the request on
overflow instead of silently truncating to zero. It also rejects decimal-form
quantities (
AsInt64()returns false for values like16.0Gi, which previouslyfell through and left the request at zero). Mirrors the device-count guard in
mthreads (#2134) and the enflame int32 bounding in #2145 / #2190.
Which issue(s) this PR fixes:
Fixes #2284
Fixes #2278
Special notes for your reviewer:
Covers nvidia, cambricon, iluvatar, and mthreads (nvidia added per @archlitchi's
review). Each backend has regression cases for both an int32-overflowing integer
(
16Gi) and a decimal-form quantity (16.0Gi), both of which are now rejectedrather than truncated. Merged current master to resolve conflicts with the new
MemoryFactorconst.make verifyand the device unit tests pass locally.AI assistance disclosure:
I used AI assistance (Claude Code) to help locate the overflow across backends,
resolve the merge, and draft the tests. I reviewed and verified the change,
confirmed the before/after behavior with the regression tests, ran
make verifylocally, and take responsibility for it.
Does this PR introduce a user-facing change?:
NONE
Summary by CodeRabbit
Bug Fixes
Tests