Skip to content

fix(nvidia): treat gpumem-percentage of 0 as unset - #2156

Merged
mesutoezdil merged 1 commit into
Project-HAMi:masterfrom
Wangmin362:fix/gpumem-percentage-zero
Aug 4, 2026
Merged

fix(nvidia): treat gpumem-percentage of 0 as unset#2156
mesutoezdil merged 1 commit into
Project-HAMi:masterfrom
Wangmin362:fix/gpumem-percentage-zero

Conversation

@Wangmin362

@Wangmin362 Wangmin362 commented Jul 28, 2026

Copy link
Copy Markdown
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

nvidia.com/gpumem-percentage: 0 is accepted today, so the scheduler computes memreq = Totalmem * 0 / 100 = 0: the pod fits any card however full it is, is booked with 0 memory, and the device plugin injects CUDA_DEVICE_MEMORY_LIMIT_0=0m, which hami-core reads as "no limit". The container can then use the whole card while the scheduler still thinks it is free.

nvidia.com/gpumem: 0 is fine because it leaves the percentage unset and falls through to the defaultMemory / whole-card branch below. This makes a non-positive percentage take the same path.

Which issue(s) this PR fixes:
NONE

Special notes for your reviewer:

Clamping to 100 instead would also stop the 0-memory booking, but it skips that defaultMemory branch, and unlike the 101 sentinel, 100 is not excluded in ComputeScore, so scoring would count a whole card.

On a V100-32GB node, a pod with gpumem-percentage: 0 got 0m and allocated 8 GiB while still booked as 0; a second pod asking for gpumem: 30000 on the same card was then admitted and died with cuMemoryAllocate failed res=2. I also ran a before/after A/B on an A100-80GB node with two shadow schedulers differing only in this change: an empty card is now booked as 81920 instead of 0, with 80000/81920 already booked the same request goes Pending with CardInsufficientMemory, and with defaultMemory: 512 it is booked as 512 and the container gets 512m instead of 0m.

The tests added for a zero percentage fail on master and pass here. This PR was written primarily by Claude Code, and I reviewed and tested it.

Does this PR introduce a user-facing change?:

A nvidia.com/gpumem-percentage of 0 is now treated as unset: the container gets the whole card, or defaultMemory when one is configured, instead of running with no GPU memory limit.

Summary by CodeRabbit

  • Bug Fixes
    • NVIDIA GPU memory requests with zero or negative percentages now use the default full-card allocation.
    • Percentage values above 100 remain capped at 100, while positive values are honored.
    • Explicit GPU memory requests continue to take precedence over percentage settings.
    • Resource accounting now correctly reserves full GPU memory for zero-percentage requests, preventing allocation when insufficient memory remains.

@hami-robot hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Jul 28, 2026
@hami-robot
hami-robot Bot requested review from lengrongfu and wawa0210 July 28, 2026 06:14
@hami-robot hami-robot Bot added the size/L label Jul 28, 2026
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

NVIDIA resource request generation now preserves the unset sentinel for zero and negative gpumem-percentage values. Tests verify whole-card fallback, explicit gpumem precedence, default memory behavior, and fit accounting.

Changes

NVIDIA memory percentage fallback

Layer / File(s) Summary
Preserve non-positive percentage fallback and accounting
pkg/device/nvidia/device.go, pkg/device/nvidia/device_test.go
GenerateResourceRequests keeps zero and negative percentages at sentinel 101. Tests verify whole-card allocation, explicit gpumem precedence, default memory behavior, and insufficient-memory failures.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: wawa0210, lengrongfu

Poem

A rabbit sees zero in the GPU queue,
Keeps the unset mark steady and true.
Whole-card memory fills the request,
Explicit memory takes precedence best.
Tests watch each allocation hop.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: treating a zero NVIDIA GPU memory percentage as unset.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 60.87% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/device/nvidia/device.go 96.96% <100.00%> (+0.02%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread pkg/device/nvidia/device.go Outdated
Comment thread pkg/device/nvidia/device_test.go Outdated
@ouyangluwei163

Copy link
Copy Markdown
Contributor

Could we handle nvidia.com/gpumem-percentage: 0 the same way nvidia.com/gpumem: 0 is already handled, entirely inside Generate Resource Requests — instead of rejecting it at admission?
https://github.com/Project-HAMi/HAMi/blob/master/pkg/device/nvidia/device.go#L557-L577

A gpumem-percentage of 0 made the scheduler book 0 memory, so the pod fit any
card and the device plugin injected CUDA_DEVICE_MEMORY_LIMIT=0m, which hami-core
reads as no limit. Handle it like nvidia.com/gpumem: 0: fall back to
defaultMemory, or to the whole card when no default is configured.

Signed-off-by: wangmin <wangmin@riseunion.io>
@Wangmin362
Wangmin362 force-pushed the fix/gpumem-percentage-zero branch from 7a5be78 to ab79651 Compare August 4, 2026 08:29
@Wangmin362 Wangmin362 changed the title fix(nvidia): reject gpumem-percentage of 0 fix(nvidia): treat gpumem-percentage of 0 as unset Aug 4, 2026
@Wangmin362

Copy link
Copy Markdown
Member Author

Could we handle nvidia.com/gpumem-percentage: 0 the same way nvidia.com/gpumem: 0 is already handled, entirely inside Generate Resource Requests — instead of rejecting it at admission? https://github.com/Project-HAMi/HAMi/blob/master/pkg/device/nvidia/device.go#L557-L577

done, handled entirely in GenerateResourceRequests now. thank you

@mesutoezdil

Copy link
Copy Markdown
Contributor

/lgtm

@archlitchi archlitchi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@hami-robot

hami-robot Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, Wangmin362

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hami-robot hami-robot Bot added the approved label Aug 4, 2026
@mesutoezdil
mesutoezdil merged commit 4f8a82f into Project-HAMi:master Aug 4, 2026
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants