fix(enflame): bound profile values before int32 conversion in Fit - #2190
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:
📝 WalkthroughWalkthroughEnflame profile sizing now validates int32-safe bounds before conversion. Regression coverage verifies that oversized slice and memory values, plus boundary-sized profiles, are rejected without selecting devices. ChangesEnflame profile validation
Estimated code review effort: 2 (Simple) | ~10 minutes 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/enflame/device_test.go`:
- Around line 442-447: Update the regression test around Fit to use a safe
Memreq value and a MemPercentagereq that selects the oversized profile through
the intended selection path, allowing execution to reach the overflow checks in
selectProfileByRequest. Add assertions verifying the slice-specific overflow
behavior rather than only invoking Fit and ignoring its results.
In `@pkg/device/enflame/device.go`:
- Around line 394-406: Update the profile-size handling around requiredSlice and
the drsSlice construction to consistently enforce the supported slice bound: use
the bounded requiredSlice value when populating drsSlice, or reject out-of-range
profile.Size values before allocation. Ensure oversized positive values cannot
pass validation or be emitted through the original profile.Size.
🪄 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: edb6e2e3-6ba0-4b37-b73c-35edd0838088
📒 Files selected for processing (2)
pkg/device/enflame/device.gopkg/device/enflame/device_test.go
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:
|
7372890 to
bb43ede
Compare
profile.Size, MemoryGB and CorePercent come from strconv.Atoi on a profile name, so a name like 4294967299g.4294967299gb wraps to 3 slices and 3072 MiB and Fit accepts it. Reject a slice count or memory size that would overflow, and clamp the core percent on the int value rather than after the conversion, so every int32 cast in Fit has both bounds established beforehand. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
bb43ede to
4dd0511
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/device/enflame/device.go (1)
407-415: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard the derived core percentage before narrowing to
int32.
collectDRSProfilesderivesCorePercentwithint(math.Ceil(float64(size)*100/float64(maxSlice))). Go makes out-of-range float-to-integer conversion implementation-dependent, so a calculated ratio larger than the platformintrange may bypass thisMaxInt32check and then become invalid when assigned toUsedcores. Reject the profile before conversion when the double-valued ratio is outside the intendedint32range, e.g. usingfloat64(maxSlice)in anMaxInt32/0bound check, and keep this0/MaxInt32guard here.🤖 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/enflame/device.go` around lines 407 - 415, Guard the derived CorePercent in collectDRSProfiles before converting the calculated ratio to int, rejecting ratios at or below 0 or above math.MaxInt32 using the float64 size/maxSlice calculation. Preserve the existing profile.CorePercent MaxInt32/0 validation in the surrounding device logic, then safely narrow profile.CorePercent to int32 for Usedcores.Source: Linters/SAST tools
♻️ Duplicate comments (1)
pkg/device/enflame/device_test.go (1)
446-469: 🎯 Functional Correctness | 🟠 MajorMake the regression cases reach the overflow checks.
MemPercentagereq: 101selects direct mode, whereselectProfileByRequestonly accepts a profile whose size equalsrequest.Memreq(3). None of these profiles matches, so every call returns before the new validation inFit; the assertions currently pass without testing slice, memory, or core overflow handling.Use a non-direct request mode, such as
MemPercentagereq: 100, while keepingMemreq: 3.🤖 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/enflame/device_test.go` around lines 446 - 469, Update the regression requests in the affected Fit test cases to use non-direct mode by changing MemPercentagereq from 101 to 100 while preserving Memreq as 3. Keep the existing overflow-specific profiles and assertions unchanged so each case reaches Fit’s slice, memory, and core overflow validation.
🤖 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.
Outside diff comments:
In `@pkg/device/enflame/device.go`:
- Around line 407-415: Guard the derived CorePercent in collectDRSProfiles
before converting the calculated ratio to int, rejecting ratios at or below 0 or
above math.MaxInt32 using the float64 size/maxSlice calculation. Preserve the
existing profile.CorePercent MaxInt32/0 validation in the surrounding device
logic, then safely narrow profile.CorePercent to int32 for Usedcores.
---
Duplicate comments:
In `@pkg/device/enflame/device_test.go`:
- Around line 446-469: Update the regression requests in the affected Fit test
cases to use non-direct mode by changing MemPercentagereq from 101 to 100 while
preserving Memreq as 3. Keep the existing overflow-specific profiles and
assertions unchanged so each case reaches Fit’s slice, memory, and core overflow
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8afc6d34-1c7d-4969-9aee-e29db32739a8
📒 Files selected for processing (2)
pkg/device/enflame/device.gopkg/device/enflame/device_test.go
The existing case tripped the slice bound first, so the memory and core percent rejections were never exercised. Add a profile that clears the slice bound but overflows on memory, and one that pairs a maxSlice of 1 with a max int32 slice count to push the derived core percent past int32. Drop the non-positive core percent clamp with it: collectDRSProfiles skips non-positive sizes and derives CorePercent as ceil(size*100/maxSlice) with maxSlice >= 1, so the value is always at least 1 by the time Fit sees it. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
ba64855 to
607cf27
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FouoF, mesutoezdil 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 |
… mthreads (#2285) * fix(device): reject int32-overflowing memory requests in iluvatar and 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 (#2134) and the enflame int32 bounding in #2145 / #2190. Adds a regression case to each backend's Test_GenerateResourceRequests. Fixes #2284 Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com> * fix(device): validate memory request before the multiply 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 (#2145). Addresses CodeRabbit review feedback; behavior and tests unchanged. Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com> * fix(device): extend memory overflow guard to cambricon 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 #2278. Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com> * test(device): drop explanatory comments from the overflow test cases 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> * test(nvidia): check 1Gi is rejected when MemoryFactor doubles it Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com> --------- Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com> Co-authored-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
3 participants
/kind bug
Fitinpkg/device/enflame/device.goconverts three profile fields toint32without an upper bound. All three trace back tostrconv.AtoiinparseProfile, so a profile name such as4294967299g.4294967299gbwraps to 3 slices and 3072 MiB and the profile is accepted. The bogusdrsSlicealso reaches the container annotation. This is CodeQL alertgo/incorrect-integer-conversionatpkg/device/enflame/device.go:394. #2145 addedclampToInt32and applied it elsewhere in the file but left these three sites.