fix: validate gpuMemoryFactor to avoid kubelet gRPC rejection (Fixes … - #2191
fix: validate gpuMemoryFactor to avoid kubelet gRPC rejection (Fixes …#2191Aayush-207 wants to merge 2 commits into
Conversation
…roject-HAMi#2187) Signed-off-by: Aayush-207 <aayushraut2006@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Aayush-207 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds ChangesMemory factor safeguards
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InitNvidiaDevice
participant ValidateMemoryFactor
participant Devices.GetPluginDevices
participant ListAndWatch
participant Kubelet
InitNvidiaDevice->>ValidateMemoryFactor: validate memoryFactor
ListAndWatch->>Devices.GetPluginDevices: generate device entries
Devices.GetPluginDevices-->>ListAndWatch: device list and entry count
ListAndWatch->>Kubelet: send device list
Kubelet-->>ListAndWatch: success or ResourceExhausted error
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 3
🧹 Nitpick comments (1)
pkg/device/nvidia/device_test.go (1)
2898-2918: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the promised log behavior.
These tests only verify local arithmetic or state; deleting the warnings would still pass. Capture the logger output (or inject a logger) and assert warnings for over-limit/invalid configurations and no warning for safe ones.
pkg/device/nvidia/device_test.go#L2898-L2918: assertValidateMemoryFactorwarning behavior.pkg/device/nvidia/device_test.go#L2925-L2944: assert the non-positive-factor normalization warning.pkg/device-plugin/nvidiadevice/nvinternal/rm/devices_test.go#L187-L200: assert the over-limitGetPluginDeviceswarning.🤖 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 2898 - 2918, Update ValidateMemoryFactor tests in pkg/device/nvidia/device_test.go:2898-2918 to capture logger output and assert warnings for over-limit and invalid configurations, with no warning for safe inputs. Also update pkg/device/nvidia/device_test.go:2925-2944 to assert the non-positive-factor normalization warning, and pkg/device-plugin/nvidiadevice/nvinternal/rm/devices_test.go:187-200 to assert the over-limit warning from GetPluginDevices.
🤖 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 `@docs/gpu-memory-factor.md`:
- Line 37: Add language identifiers to every fenced code block in
docs/gpu-memory-factor.md at lines 37, 70, 99, 105, 111, 117, 128, 149, and 187;
use text for formulas and log samples, and retain yaml for YAML content.
In `@pkg/device/nvidia/device.go`:
- Around line 237-241: Remove or revise the no-op startup validation claim
around ValidateMemoryFactor in pkg/device/nvidia/device.go:237-241, unless real
GPU memory and count can be supplied at initialization. Update
docs/gpu-memory-factor.md:55-64 to state that entry-count validation occurs at
runtime inside GetPluginDevices when startup validation is not implemented.
- Around line 210-212: Correct the memory-factor guidance to use
ceil(totalMemMiB / floor(60000 / gpuCount)) rather than ceil(totalMemMiB *
gpuCount / 60000), handling gpuCount > 60000 explicitly. Apply this consistently
in pkg/device/nvidia/device.go lines 210-212,
pkg/device-plugin/nvidiadevice/nvinternal/rm/devices.go lines 232-236,
pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go lines 465-468,
charts/hami/templates/scheduler/device-configmap.yaml lines 25-33, and both
documented formula/instruction sections at docs/gpu-memory-factor.md lines 68-75
and 144-153.
---
Nitpick comments:
In `@pkg/device/nvidia/device_test.go`:
- Around line 2898-2918: Update ValidateMemoryFactor tests in
pkg/device/nvidia/device_test.go:2898-2918 to capture logger output and assert
warnings for over-limit and invalid configurations, with no warning for safe
inputs. Also update pkg/device/nvidia/device_test.go:2925-2944 to assert the
non-positive-factor normalization warning, and
pkg/device-plugin/nvidiadevice/nvinternal/rm/devices_test.go:187-200 to assert
the over-limit warning from GetPluginDevices.
🪄 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: 4c8188bd-30ad-403a-a1c6-f6a8b801fee2
📒 Files selected for processing (7)
charts/hami/templates/scheduler/device-configmap.yamldocs/gpu-memory-factor.mdpkg/device-plugin/nvidiadevice/nvinternal/plugin/server.gopkg/device-plugin/nvidiadevice/nvinternal/rm/devices.gopkg/device-plugin/nvidiadevice/nvinternal/rm/devices_test.gopkg/device/nvidia/device.gopkg/device/nvidia/device_test.go
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
this doesnt fix #2187. the bug is in volcano-vgpu-device-plugin, not here. |
2 participants
…#2187)
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes Issue #2187 by adding validation and logging for gpuMemoryFactor in the volcano‑vgpu‑device‑plugin. Previously, setting gpuMemoryFactor to 1 or 2 on large GPUs (e.g., A800) caused kubelet to reject the ListAndWatch response due to exceeding the gRPC size limit (~60k entries), resulting in volcano.sh/vgpu-memory showing 0 or stale values.
The fix ensures:
Validation at startup to warn if (totalMemMiB * gpuCount / factor) > 60000.
Explicit logging when kubelet rejects the response, instead of failing silently.
Documentation updates to explain the requirements for configuring gpuMemoryFactor.
Unit tests to confirm validation and logging behavior.
Which issue(s) this PR fixes:
Fixes #2187
Special notes for your reviewer:
Tested on A800 with HAMi v2.9, Kubernetes v1.35, Volcano v1.15.
At factor 1, res length= exceeded ~81k entries and was rejected; at factor 4+, values were accepted.
Does this PR introduce a user-facing change?:
Yes. Users will now see clear warnings and logs when gpuMemoryFactor is set too low, and documentation clarifies the configuration requirements.
Summary by CodeRabbit
New Features
memoryFactorsizing, including a computed minimum safe factor.memoryFactorby defaulting to1safely.Bug Fixes
Documentation
memoryFactordocumentation page with formulas, examples, and troubleshooting steps.Tests