fix(nvidia): validate gpumem-percentage range to prevent silent unschedulability - #1997
Conversation
…edulability When a pod requests an out-of-range memory percentage (e.g. nvidia.com/gpumem-percentage > 100), the scheduler computed a memory request larger than any card's total memory, leaving the pod Pending forever with a misleading CardInsufficientMemory reason. A value of exactly 101 collided with the internal 'percentage not set' sentinel. - Reject percentages outside [0, 100] at admission time in MutateAdmission with a human-readable error message. Init containers are validated as well: they are scheduled via Resourcereqs but never passed to MutateAdmission by the webhook. - Clamp out-of-range percentages to 100 in GenerateResourceRequests as defense-in-depth for pods that bypass the admission webhook, mirroring the existing Coresreq > 100 clamp in Fit. Fixes Project-HAMi#1781 Signed-off-by: Saiyam Pathak <saiyam911@gmail.com>
📝 WalkthroughWalkthroughValidates NVIDIA memory-percentage values during admission and clamps out-of-range percentages to 100 during resource request generation. Tests cover accepted, rejected, and clamped inputs. ChangesMemory Percentage Validation and Clamping
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces memory percentage validation for GPU devices in MutateAdmission (for both standard and init containers) and clamps out-of-range memory percentage requests to 100 in GenerateResourceRequests. Unit tests have been added to verify these validation and clamping behaviors. The reviewer suggested adding nil checks for the container and pod parameters in MutateAdmission to prevent potential nil pointer dereferences.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/nvidia/device.go (1)
566-580: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBounds check happens after truncating int64→int32, defeating the clamp on overflow.
mempnum = int32(mempnums)truncates the rawint64value before the range check on Line 575. Ifmempnumsis a value that overflowsint32(e.g.,2^32 + 50), the truncated result can land inside[0, 100]and silently bypass the clamp — exactly the kind of malformed/bypassing input this defense-in-depth code is meant to catch. Since this resource has no upper bound enforced by the API server, a pod bypassing the webhook could supply such a value.Validate against the original
int64before casting toint32.🛡️ Proposed fix to validate before truncating
if ok { mempnums, ok := mem.AsInt64() if ok { - mempnum = int32(mempnums) - if mempnum < 0 || mempnum > 100 { - klog.ErrorS(nil, "memory percentage request out of range, clamping to 100", "container", ctr.Name, "requested", mempnum) - mempnum = 100 + if mempnums < 0 || mempnums > 100 { + klog.ErrorS(nil, "memory percentage request out of range, clamping to 100", "container", ctr.Name, "requested", mempnums) + mempnum = 100 + } else { + mempnum = int32(mempnums) } } }🤖 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.go` around lines 566 - 580, The memory percentage clamp in the NVIDIA device resource parsing is checking after the int64-to-int32 cast, so overflowed values can bypass validation. In the `pkg/device/nvidia/device.go` logic around `mempnum`, validate `mempnums` as an int64 first against the 0–100 range, log and clamp if it is out of range, and only then assign it to `mempnum` as int32. Keep the fix inside the existing `resourceMemPercentage` parsing block so the `klog.ErrorS` path still reports the container name and requested value.
🧹 Nitpick comments (1)
pkg/device/nvidia/device_test.go (1)
1818-1853: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding an int32-overflow test case.
Given the truncation issue flagged in
pkg/device/nvidia/device.go(Lines 566-580), consider adding a case here with a value like4294967397(2^32 + 101) to lock in the fix and prevent regression once the bounds check is moved before the cast.🤖 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 1818 - 1853, Add an int32-overflow regression case in the GPU memory percentage table in device_test.go, alongside the existing gpumem-percentage clamping tests. Use a value beyond int32 range (for example 2^32+101) so the test exercises the truncation path in the device parsing logic and verifies the request still clamps to 100. Keep it aligned with the parsing behavior in the Nvidia device request code so the fix in device.go is protected from future regressions.
🤖 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/nvidia/device.go`:
- Around line 566-580: The memory percentage clamp in the NVIDIA device resource
parsing is checking after the int64-to-int32 cast, so overflowed values can
bypass validation. In the `pkg/device/nvidia/device.go` logic around `mempnum`,
validate `mempnums` as an int64 first against the 0–100 range, log and clamp if
it is out of range, and only then assign it to `mempnum` as int32. Keep the fix
inside the existing `resourceMemPercentage` parsing block so the `klog.ErrorS`
path still reports the container name and requested value.
---
Nitpick comments:
In `@pkg/device/nvidia/device_test.go`:
- Around line 1818-1853: Add an int32-overflow regression case in the GPU memory
percentage table in device_test.go, alongside the existing gpumem-percentage
clamping tests. Use a value beyond int32 range (for example 2^32+101) so the
test exercises the truncation path in the device parsing logic and verifies the
request still clamps to 100. Keep it aligned with the parsing behavior in the
Nvidia device request code so the fix in device.go is protected from future
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9519d16-25c6-4066-8961-21a2b7240690
📒 Files selected for processing (2)
pkg/device/nvidia/device.gopkg/device/nvidia/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:
|
…low wraparound Values beyond int32 range (e.g. 2^32+50) wrapped to an in-range number after the cast and bypassed the clamp. Validate the raw int64 value first, then cast. Addresses review feedback on Project-HAMi#1997. Signed-off-by: Saiyam Pathak <saiyam911@gmail.com>
…review Init containers need more careful design at the admission layer; they remain protected by the out-of-range clamp in GenerateResourceRequests, which covers the scheduling path. Addresses review feedback from archlitchi on Project-HAMi#1997. Signed-off-by: Saiyam Pathak <saiyam911@gmail.com>
|
@archlitchi Thanks for the review — removed the init-container loop in d5a1fe3. Regular containers are still validated at admission, and init containers remain covered by the out-of-range clamp in GenerateResourceRequests on the scheduling path, so they can't produce permanently-Pending pods. Happy to explore a more holistic init-container admission design in a follow-up if there's interest. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, saiyam1814 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:
nvidia.com/gpumem-percentageis never range-validated. A pod requesting a value > 100 passes admission, but the scheduler then computesmemreq = Totalmem * pct / 100— larger than any card — so the pod stays Pending forever with a misleadingCardInsufficientMemoryreason. A value of exactly101silently collides with the internal "percentage not set" sentinel inGenerateResourceRequests.This PR:
[0, 100]at admission time inMutateAdmissionwith a clear error message (same pattern as Ascend's validation).100inGenerateResourceRequests(validated on the raw int64 before the int32 cast to avoid wraparound), mirroring the existingCoresreq > 100clamp inFit. This also covers init containers and pods that bypass the webhook — admission-time validation for init containers was dropped per review, as it needs more careful design.Valid
0–100requests are unchanged; only invalid specs (negative, 101, >100) change behavior — from silent forever-Pending to an immediate, actionable rejection.Which issue(s) this PR fixes:
Fixes #1781
Special notes for your reviewer:
The scenario as originally reported (missing
gpumemon v2.6.14) is largely resolved on current releases: whole-card default exists since v2.3.2 and human-readable filter reasons landed in v2.7.0 (#1097). This PR closes the remaining gap that still reproduces the silent-unschedulability on master: out-of-range percentage values.Known pre-existing limitation (not introduced here): pods bypassing the webhook don't get the admission-time exclusive-core mutation from
defaultExclusiveCoreIfNeeded; the clamp keeps scheduling sane for them regardless.Does this PR introduce a user-facing change?: