feat(monitor): add DeviceProcessCount metric to expose tracked GPU process count - #2505
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Nakshatra480 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe NVIDIA monitor now counts active processes for each device and exports the count as a container-device Prometheus gauge. The v0 and v1 implementations bound process-slot access and check process status and memory usage. ChangesDevice process metrics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ContainerCollector
participant UsageInfo
participant NVIDIASpec
participant Prometheus
ContainerCollector->>UsageInfo: Request DeviceProcessCount(device index)
UsageInfo->>NVIDIASpec: Count bounded active process slots
NVIDIASpec-->>UsageInfo: Return device process count
UsageInfo-->>ContainerCollector: Return count
ContainerCollector->>Prometheus: Emit process-count gauge
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (2)
pkg/monitor/nvidia/v1/spec_test.go (1)
1392-1470: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the combined memory footprint.
Add cases where
totalis zero andcontextSizeormoduleSizeis nonzero. The current cases do not detect the total-only predicate.🤖 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/monitor/nvidia/v1/spec_test.go` around lines 1392 - 1470, Extend TestSpec_DeviceProcessCount with cases where deviceMemory.total is zero but contextSize or moduleSize is nonzero, covering each field and the expected process-count behavior. Keep the existing active-process and device-selection coverage unchanged, and ensure the assertions detect whether DeviceProcessCount uses the combined memory footprint rather than total alone.pkg/monitor/nvidia/v0/spec_test.go (1)
492-570: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the combined memory footprint.
Add cases where
totalis zero andcontextSizeormoduleSizeis nonzero. The current cases do not detect the total-only predicate.🤖 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/monitor/nvidia/v0/spec_test.go` around lines 492 - 570, Extend TestSpec_DeviceProcessCount with cases where a process has zero deviceMemory.total but a nonzero contextSize or moduleSize on the selected device, and assert it is counted. Keep the existing total-memory cases and vary the fields independently so the test verifies the combined memory-footprint predicate rather than total alone.
🤖 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/monitor/nvidia/v0/spec.go`:
- Around line 165-173: Update Spec.DeviceProcessCount in
pkg/monitor/nvidia/v0/spec.go:165-173 and pkg/monitor/nvidia/v1/spec.go:179-187
to count active slots when total, contextSize, or moduleSize is nonzero. Extend
the existing tests in pkg/monitor/nvidia/v0/spec_test.go:492-570 and
pkg/monitor/nvidia/v1/spec_test.go:1392-1470 with context-only and module-only
cases, preserving the current total-memory coverage.
---
Nitpick comments:
In `@pkg/monitor/nvidia/v0/spec_test.go`:
- Around line 492-570: Extend TestSpec_DeviceProcessCount with cases where a
process has zero deviceMemory.total but a nonzero contextSize or moduleSize on
the selected device, and assert it is counted. Keep the existing total-memory
cases and vary the fields independently so the test verifies the combined
memory-footprint predicate rather than total alone.
In `@pkg/monitor/nvidia/v1/spec_test.go`:
- Around line 1392-1470: Extend TestSpec_DeviceProcessCount with cases where
deviceMemory.total is zero but contextSize or moduleSize is nonzero, covering
each field and the expected process-count behavior. Keep the existing
active-process and device-selection coverage unchanged, and ensure the
assertions detect whether DeviceProcessCount uses the combined memory footprint
rather than total alone.
🪄 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: 84ab2744-9703-4da6-a50d-f8c3f87d548a
📒 Files selected for processing (8)
cmd/vGPUmonitor/feedback_test.gocmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_container_test.gopkg/monitor/nvidia/cudevshr.gopkg/monitor/nvidia/v0/spec.gopkg/monitor/nvidia/v0/spec_test.gopkg/monitor/nvidia/v1/spec.gopkg/monitor/nvidia/v1/spec_test.go
…ocess count Add hami_container_device_process_count, a per-device Prometheus gauge that reports the number of process slots in a container's shared-memory region that have status != 0 and a nonzero memory footprint on the queried device. When a child process or a process reached through SSH bypasses libvgpu's LD_PRELOAD hook it never gets a shared-memory slot allocated. The count therefore drops below the real number of CUDA processes in the container, giving operators a direct Prometheus signal that GPU memory isolation has broken down before a memory overrun occurs. Changes: - Add DeviceProcessCount(idx int) int to the UsageInfo interface. - Implement in pkg/monitor/nvidia/v0/spec.go and v1/spec.go. Counts process slots where status != 0 and the sum of total, contextSize, and moduleSize for device idx is nonzero. - Add unit tests to v0/spec_test.go and v1/spec_test.go covering: no active processes, one active process, process active on a different device only, dead slot within procnum, and corrupt procnum clamped without panic. - Add DeviceProcessCount to the stubInfo in feedback_test.go. - Register ctrDeviceProcessCountDesc and emit the metric in collectContainerMetrics alongside the other per-device gauges. - Update metrics_container_test.go expected metric counts. Signed-off-by: Nakshatra Sharma <nakshatra.sharma3012@gmail.com>
b639414 to
a9b22c3
Compare
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Thanks for the metrics proposal. DeviceProcessCount is presented as a count of tracked GPU processes, but the implementation counts slots with any non-zero memory, context, or module field. It cannot distinguish the untracked-process case described in the PR and has no independent process inventory against which the value is validated. That would publish a misleading metric contract, so we are closing the current PR. A future proposal should first define the exact population and prove it against a real process source. |
Thank you for the detailed feedback, your point about not having an independent process source to verify the slot count against is a valid one. I will submit a proposal which cross-references either |
2 participants
Summary
Add
hami_container_device_process_count, a per-device Prometheus gaugethat reports the number of process slots in a container's shared-memory
region that have
status != 0and a nonzero memory footprint on thequeried virtual device.
Why this matters: When a child process or a process reached through SSH
bypasses libvgpu's
LD_PRELOADhook it never gets a shared-memory slotallocated. The count therefore drops below the real number of CUDA processes
in the container. Operators and alert rules can use this metric to detect
GPU memory isolation failures early before a memory overrun occurs by
comparing
hami_container_device_process_countagainst the expected numberof tracked GPU processes.
Changes
pkg/monitor/nvidia/cudevshr.goDeviceProcessCount(idx int) inttoUsageInfointerfacepkg/monitor/nvidia/v0/spec.gostatus != 0andtotal+contextSize+moduleSize > 0for deviceidxpkg/monitor/nvidia/v1/spec.gopkg/monitor/nvidia/v0/spec_test.gopkg/monitor/nvidia/v1/spec_test.gocmd/vGPUmonitor/feedback_test.gocmd/vGPUmonitor/metrics.goctrDeviceProcessCountDesc, emit incollectContainerMetricscmd/vGPUmonitor/metrics_container_test.goExample output
hami_container_device_process_count{container="worker",device_uuid="GPU-abc123...", namespace="team-a",pod="trainer-0",vdevice_index="0"} 2If a child process escapes slot tracking, this drops to 1 while the real
GPU process count remains 2, the isolation failure becomes immediately
observable in Grafana.
Which issue(s) this PR fixes:
Part of #2126 (LFX observability hardening)
AI Disclosure:
AI assistance was used for code inspection and draft formatting; all logic,
test cases, and verification were manually reviewed and validated.
Summary by CodeRabbit
New Features
Bug Fixes
Tests