feat: enforce vGPU memory isolation for child and ssh processes - #2576
feat: enforce vGPU memory isolation for child and ssh processes#2576MuhammadBilal561 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MuhammadBilal561 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 |
|
Welcome @MuhammadBilal561! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
📝 WalkthroughWalkthroughChangesOverride environment allocation
libvgpu reference update
ListAndWatch test cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NVIDIADevicePlugin
participant buildOverrideEnv
participant ContainerCache
participant ContainerRuntime
NVIDIADevicePlugin->>buildOverrideEnv: Filter HAMi environment variables
buildOverrideEnv-->>NVIDIADevicePlugin: Return sorted overrideEnv content
NVIDIADevicePlugin->>ContainerCache: Write overrideEnv
NVIDIADevicePlugin->>ContainerRuntime: Mount file read-only at /overrideEnv
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 |
Signed-off-by: MuhammadBilal561 <bilalrehan2006@gmail.com>
4560817 to
b026538
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go (1)
892-912: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftStore
overrideEnvoutside the writable cache mount.
overrideEnvHostPathis insidecacheFileHostDirectory. The container also receives this directory as a read-write mount athostHookPath/vgpu. A process can modifyhostHookPath/vgpu/overrideEnvand change the constraints that libvgpu reads for later processes.Write
overrideEnvin a separate host directory that is not exposed through a writable container mount. Keep that directory non-writable to the container. Add lifecycle cleanup for the separate file.🤖 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-plugin/nvidiadevice/nvinternal/plugin/server.go` around lines 892 - 912, Update the overrideEnv handling near buildOverrideEnv and the response.Mounts setup so the file is written under a separate host directory, outside cacheFileHostDirectory and any writable container mount. Mount only that separate file read-only at /overrideEnv, ensure the containing directory is not exposed writable, and add lifecycle cleanup for the generated file and directory.
🤖 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-plugin/nvidiadevice/nvinternal/plugin/server.go`:
- Around line 894-897: Update the override-file handling around os.WriteFile to
create the file unconditionally, including when overrideEnvContent is empty. If
creation fails, call PodAllocationFailed, return the write error immediately,
and do not add the mount; only proceed to mount the successfully created file.
---
Outside diff comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go`:
- Around line 892-912: Update the overrideEnv handling near buildOverrideEnv and
the response.Mounts setup so the file is written under a separate host
directory, outside cacheFileHostDirectory and any writable container mount.
Mount only that separate file read-only at /overrideEnv, ensure the containing
directory is not exposed writable, and add lifecycle cleanup for the generated
file and directory.
🪄 Autofix
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: f4a499f1-7e15-424d-9fd2-be2d71a1b06d
📒 Files selected for processing (3)
libvgpupkg/device-plugin/nvidiadevice/nvinternal/plugin/server.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go
💤 Files with no reviewable changes (1)
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go
| if len(overrideEnvContent) > 0 { | ||
| if err := os.WriteFile(overrideEnvHostPath, []byte(overrideEnvContent), 0644); err != nil { | ||
| klog.ErrorS(err, "failed to write overrideEnv for container", "path", overrideEnvHostPath) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail allocation when the override file cannot be created.
If os.WriteFile fails, this code still adds a mount whose host source may not exist. Kubelet can then fail the container mount after Allocate reports success. Continuing also removes the memory-isolation guarantee for child processes.
Create the file unconditionally, including an empty file when needed. If creation fails, call PodAllocationFailed and return the error before adding the mount.
Proposed fix
- if len(overrideEnvContent) > 0 {
- if err := os.WriteFile(overrideEnvHostPath, []byte(overrideEnvContent), 0644); err != nil {
- klog.ErrorS(err, "failed to write overrideEnv for container", "path", overrideEnvHostPath)
- }
+ if err := os.WriteFile(overrideEnvHostPath, []byte(overrideEnvContent), 0644); err != nil {
+ PodAllocationFailed(nodename, current, NodeLockNvidia)
+ return nil, fmt.Errorf("write overrideEnv for container: %w", err)
}Also applies to: 910-912
🤖 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-plugin/nvidiadevice/nvinternal/plugin/server.go` around lines 894
- 897, Update the override-file handling around os.WriteFile to create the file
unconditionally, including when overrideEnvContent is empty. If creation fails,
call PodAllocationFailed, return the write error immediately, and do not add the
mount; only proceed to mount the successfully created file.
|
This is being closed because it does not comply with the contribution guidelines. |
2 participants
/kind feature
What this PR does / why we need it:
Enforces vGPU memory isolation across child and SSH processes by injecting
overrideEnv. This guarantees that memory ceilings and core allocation limits are strictly maintained across process boundaries spawned within the container environment.Which issue(s) this PR fixes:
Special notes for your reviewer:
Includes updates to
libvgpufor process-level memory enforcement andpkg/device plugin logic to handleoverrideEnvinjections.Does this PR introduce a user-facing change?:
NONE
Summary by CodeRabbit
New Features
Bug Fixes
Chores