feat(device-plugin): support CDI-only NVIDIA accelerators (GB10) - #2087
feat(device-plugin): support CDI-only NVIDIA accelerators (GB10)#2087tittuvarghese wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tittuvarghese 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 @tittuvarghese! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CDI-only NVIDIA device discovery for accelerators unavailable through NVML, including CDI resource managers, plugin registration, Helm configuration, CLI validation, external CDI handling, tests, and GB10 documentation. ChangesCDI discovery support
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PluginFactory
participant CDIResourceManagers
participant NvidiaDevicePlugin
participant DeviceRegistration
PluginFactory->>PluginFactory: resolve strategy as cdi
PluginFactory->>CDIResourceManagers: create CDI resource managers
PluginFactory->>NvidiaDevicePlugin: enable cdiDiscovery
NvidiaDevicePlugin->>DeviceRegistration: build devices from CDI entries
DeviceRegistration-->>NvidiaDevicePlugin: return registered devices
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 support for the NVIDIA GB10 (Grace-Blackwell iGPU) and other CDI-only accelerators that cannot be enumerated via NVML. It adds a new cdi device discovery strategy that reads node CDI specs directly to discover GPUs, bypassing NVML. It also adds configuration options for specifying pre-configured device memory and type, and updates the Helm charts, documentation, and tests accordingly. One high-severity issue was found in pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go where calling d.GetUUID() with a single-value assignment will cause a compilation error, as it returns two values. It is recommended to use d.ID directly instead.
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.
bc14794 to
a3bfca2
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)
295-296: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSkip MIG handling when CDI discovery is enabled.
The comment on line 265 states that MIG handling should be skipped entirely when in CDI discovery mode. However, the code currently enters the MIG handling block whenever
operatingMode == "mig". BecausedeviceNamesis empty in this mode,deviceSupportMigremainstrue, and the plugin attempts to executenvidia-mig-parted(which relies on NVML). This results in a failed execution and an unnecessary error log before the plugin eventually falls back to a non-MIG configuration.Update the condition to explicitly skip MIG handling when
cdiDiscoveryis enabled.🐛 Proposed fix
migApplied := false - if plugin.operatingMode == "mig" { + if plugin.operatingMode == "mig" && !plugin.cdiDiscovery { deviceSupportMig := true🤖 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 295 - 296, Update the MIG handling condition near migApplied so it only executes when operatingMode is "mig" and cdiDiscovery is disabled. Preserve the existing MIG logic for non-CDI MIG mode and skip it entirely when CDI discovery is enabled.
🧹 Nitpick comments (1)
docs/nvidia-gb10-support.md (1)
34-35: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAlign the CDI path documentation with the mounted host path
docs/nvidia-gb10-support.mdsays the plugin scans/etc/cdiand/var/run/cdi, but the DaemonSet only mounts/var/run/cdi. Update the docs to match the mount, or add the missing/etc/cdimount if both paths are intended.🤖 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 `@docs/nvidia-gb10-support.md` around lines 34 - 35, Update the NVIDIA GPU Operator/CDI setup documentation in nvidia-gb10-support.md to reference only the mounted /var/run/cdi path, unless the DaemonSet is intentionally changed to mount /etc/cdi as well. Ensure the documented plugin scan paths and generated CDI spec location match the actual DaemonSet mount configuration.Source: MCP tools
🤖 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/nvidia-gb10-support.md`:
- Around line 71-73: Update the `preConfiguredDeviceMemory` documentation to
clarify that `122566` represents the DGX Spark’s reported usable GPU memory
after system reservation, not a direct 120 GiB-to-MiB conversion, and cite the
authoritative source for that value.
---
Outside diff comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go`:
- Around line 295-296: Update the MIG handling condition near migApplied so it
only executes when operatingMode is "mig" and cdiDiscovery is disabled. Preserve
the existing MIG logic for non-CDI MIG mode and skip it entirely when CDI
discovery is enabled.
---
Nitpick comments:
In `@docs/nvidia-gb10-support.md`:
- Around line 34-35: Update the NVIDIA GPU Operator/CDI setup documentation in
nvidia-gb10-support.md to reference only the mounted /var/run/cdi path, unless
the DaemonSet is intentionally changed to mount /etc/cdi as well. Ensure the
documented plugin scan paths and generated CDI spec location match the actual
DaemonSet mount configuration.
🪄 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: 9954d92b-d79b-4347-9e38-8e7a698770f4
📒 Files selected for processing (16)
charts/hami/templates/device-plugin/daemonsetnvidia.yamlcharts/hami/templates/scheduler/device-configmap.yamlcharts/hami/values.yamlcmd/device-plugin/nvidia/main.gocmd/device-plugin/nvidia/plugin-manager.godocs/nvidia-gb10-support.mdpkg/device-plugin/nvidiadevice/nvinternal/cdi/cdi.gopkg/device-plugin/nvidiadevice/nvinternal/cdi/external.gopkg/device-plugin/nvidiadevice/nvinternal/cdi/external_test.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/factory.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/register.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/server.gopkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_devices.gopkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_devices_test.gopkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_manager.gopkg/device/nvidia/device.go
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-plugin/nvidiadevice/nvinternal/plugin/server.go (1)
295-296: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip MIG handling when CDI discovery is active.
The comment on line 264 explicitly states that MIG handling should be skipped entirely for CDI-only accelerators. However, because
deviceNamesis empty in CDI mode, the loop overdeviceNames(line 298) does zero iterations, leavingdeviceSupportMigastrue. This inadvertently allows the plugin to executenvidia-mig-parted export, violating the intent to skip MIG handling.Modify the condition to explicitly bypass the MIG configuration block when
cdiDiscoveryis true.🐛 Proposed fix
migApplied := false - if plugin.operatingMode == "mig" { + if plugin.operatingMode == "mig" && !plugin.cdiDiscovery { deviceSupportMig := true🤖 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 295 - 296, Update the MIG configuration condition in the plugin server to require both MIG operating mode and !cdiDiscovery, so CDI-only accelerators bypass the entire MIG handling block. Preserve the existing MIG behavior when CDI discovery is inactive.
🧹 Nitpick comments (1)
docs/nvidia-gb10-support.md (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
To improve syntax highlighting and resolve markdownlint warnings (MD040), specify a language for this code block (e.g.,
textorlog).♻️ Proposed fix
-``` +```text🤖 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 `@docs/nvidia-gb10-support.md` at line 22, Specify a language identifier on the fenced code block in the NVIDIA GB10 support documentation, using an appropriate value such as text or log, while leaving the block’s contents unchanged.Source: Linters/SAST tools
🤖 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-plugin/nvidiadevice/nvinternal/plugin/server.go`:
- Around line 295-296: Update the MIG configuration condition in the plugin
server to require both MIG operating mode and !cdiDiscovery, so CDI-only
accelerators bypass the entire MIG handling block. Preserve the existing MIG
behavior when CDI discovery is inactive.
---
Nitpick comments:
In `@docs/nvidia-gb10-support.md`:
- Line 22: Specify a language identifier on the fenced code block in the NVIDIA
GB10 support documentation, using an appropriate value such as text or log,
while leaving the block’s contents unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ab66088-3c74-4984-915a-47da23bfb95c
📒 Files selected for processing (16)
charts/hami/templates/device-plugin/daemonsetnvidia.yamlcharts/hami/templates/scheduler/device-configmap.yamlcharts/hami/values.yamlcmd/device-plugin/nvidia/main.gocmd/device-plugin/nvidia/plugin-manager.godocs/nvidia-gb10-support.mdpkg/device-plugin/nvidiadevice/nvinternal/cdi/cdi.gopkg/device-plugin/nvidiadevice/nvinternal/cdi/external.gopkg/device-plugin/nvidiadevice/nvinternal/cdi/external_test.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/factory.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/register.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/server.gopkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_devices.gopkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_devices_test.gopkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_manager.gopkg/device/nvidia/device.go
🚧 Files skipped from review as they are similar to previous changes (11)
- cmd/device-plugin/nvidia/plugin-manager.go
- pkg/device-plugin/nvidiadevice/nvinternal/cdi/external.go
- cmd/device-plugin/nvidia/main.go
- pkg/device-plugin/nvidiadevice/nvinternal/cdi/cdi.go
- charts/hami/templates/device-plugin/daemonsetnvidia.yaml
- pkg/device-plugin/nvidiadevice/nvinternal/cdi/external_test.go
- pkg/device/nvidia/device.go
- charts/hami/templates/scheduler/device-configmap.yaml
- charts/hami/values.yaml
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/factory.go
- pkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_manager.go
a3bfca2 to
d99e1f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/nvidia-gb10-support.md`:
- Line 22: Update the diagnostic log code fence in the documentation to specify
the text language, using the existing fenced block as the target.
- Line 102: Update the preConfiguredDeviceType bullet in the documentation to
begin with “It can be overridden,” making the sentence grammatically complete
while preserving the existing meaning and per-node preconfigureddevicetype
reference.
- Around line 76-97: Correct the memory conversion example in the documentation
so the sample Kubernetes capacity, preConfiguredDeviceMemory value, and derived
MiB explanation are consistent. Update the 122566 value or the displayed
calculation to reflect that 125506464Ki divided by 1024 is approximately
122565MiB, or explicitly identify any intentional policy rounding/adjustment.
🪄 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: 11481b5f-c1c0-43f0-a762-ae95cd3e0bef
📒 Files selected for processing (1)
docs/nvidia-gb10-support.md
d99e1f2 to
63cabd3
Compare
|
if you wouldn't mind doing merge commits instead of force pushing, that would make it a bit clearer what (if anything has changed). |
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-plugin/nvidiadevice/nvinternal/plugin/register.go (1)
277-278:⚠️ Potential issue | 🟠 MajorBypass topology score calculation when using CDI discovery.
As noted in a previous review,
nvidia.CalculateGPUScoreinvokes NVML. On a CDI-only node (e.g., GB10), NVML is unavailable or explicitly bypassed, meaning this call will fail or panic ifENABLE_TOPOLOGY_SCOREistrue.Update the condition to ensure the score calculation is skipped when
cdiDiscoveryis active.🛠️ Proposed fix
- if os.Getenv("ENABLE_TOPOLOGY_SCORE") == "true" { + if os.Getenv("ENABLE_TOPOLOGY_SCORE") == "true" && !plugin.cdiDiscovery {🤖 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/register.go` around lines 277 - 278, Update the topology-score condition surrounding nvidia.CalculateGPUScore to also require that cdiDiscovery is inactive. Preserve the existing ENABLE_TOPOLOGY_SCORE behavior for non-CDI discovery and leave the score handling unchanged once calculation is permitted.
🤖 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-plugin/nvidiadevice/nvinternal/plugin/register.go`:
- Around line 277-278: Update the topology-score condition surrounding
nvidia.CalculateGPUScore to also require that cdiDiscovery is inactive. Preserve
the existing ENABLE_TOPOLOGY_SCORE behavior for non-CDI discovery and leave the
score handling unchanged once calculation is permitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa9151a3-9a3e-4552-a6e9-18efab73cbb6
📒 Files selected for processing (16)
charts/hami/templates/device-plugin/daemonsetnvidia.yamlcharts/hami/templates/scheduler/device-configmap.yamlcharts/hami/values.yamlcmd/device-plugin/nvidia/main.gocmd/device-plugin/nvidia/plugin-manager.godocs/nvidia-gb10-support.mdpkg/device-plugin/nvidiadevice/nvinternal/cdi/cdi.gopkg/device-plugin/nvidiadevice/nvinternal/cdi/external.gopkg/device-plugin/nvidiadevice/nvinternal/cdi/external_test.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/factory.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/register.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/server.gopkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_devices.gopkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_devices_test.gopkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_manager.gopkg/device/nvidia/device.go
🚧 Files skipped from review as they are similar to previous changes (14)
- cmd/device-plugin/nvidia/plugin-manager.go
- charts/hami/templates/scheduler/device-configmap.yaml
- charts/hami/templates/device-plugin/daemonsetnvidia.yaml
- pkg/device-plugin/nvidiadevice/nvinternal/cdi/external.go
- charts/hami/values.yaml
- pkg/device-plugin/nvidiadevice/nvinternal/cdi/external_test.go
- pkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_devices_test.go
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/factory.go
- cmd/device-plugin/nvidia/main.go
- pkg/device/nvidia/device.go
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
- pkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_manager.go
- pkg/device-plugin/nvidiadevice/nvinternal/cdi/cdi.go
- pkg/device-plugin/nvidiadevice/nvinternal/rm/cdi_devices.go
a37b72f to
541edb4
Compare
Guard the MIG operating-mode block with !cdiDiscovery so nvidia-mig-parted no longer runs on CDI-only accelerators (e.g. GB10), matching the documented behavior that MIG handling is skipped for CDI-only nodes. Signed-off-by: tittu.varghese <tittu.varghese@npci.org.in>
5391d4f to
0ab307a
Compare
DSFans2014
left a comment
DSFans2014
left a comment
There was a problem hiding this comment.
@tittuvarghese Does this issue exist with k8s-device-plugin?
@DSFans2014 it exists in K8-device-plugin as well, but they patched by tolerating the missing memory details. But i feel our approach is more cleaner and valid for truly CDI only node. Sources:
|
|
@tittuvarghese I have reviewed all the issues and repos above and none of them prove that The GB10 not enumerable via NVML which is the root problem of this pr. We must confirm that it dose exist before further discussion. |
|
agree with @FouoF, pls share raw nvml or nvidia-smi output from a real gb10 node proving the device is not enumerable, |
|
closing bcs of inactivity of pr owner. it can be re-opened if needed |
5 participants
Problem
The NVIDIA GB10 (Grace-Blackwell iGPU, as in the DGX Spark) is exposed to the node only via CDI — the NVIDIA Container Toolkit / GPU Operator generates
/var/run/cdi/k8s.device-plugin.nvidia.com-gpu.json, and the device is not enumerable via NVML inside the device-plugin container.HAMi's NVIDIA device plugin discovers devices only through NVML (or tegra). On a GB10 node
info.ResolvePlatform()returnsunknown,getResourceManagers()hits its default branch, and the plugin crashes at start-up:The node therefore never registers the GPU with HAMi.
This is distinct from #1637 (handling
GetMemoryInfo→NOT_SUPPORTEDfor unified-memory GPUs): that fix runs inregister.goafter NVML discovery has already succeeded, so it is never reached on a CDI-only node — the crash happens earlier, in discovery.Change
Add a
cdidevice-discovery strategy that discovers GPUs from the node's externally-managed CDI specs, without NVML:NewCDIResourceManagers/buildCDIDeviceMapenumerate GPU devices from the CDI specs (vendork8s.device-plugin.nvidia.com, classgpu; theallmeta-device is excluded), keying each device by its CDI device name so the existingcdi-annotations/cdi-criallocation path builds a matching qualified device name for injection.cdicase, and auto-fall back to it fromautowhen no NVML/tegra platform is detected but a CDI device-list strategy is active and CDI specs are present.QualifiedName()still works for injection.DeviceInfofrom configuration (preConfiguredDeviceMemory/ newpreConfiguredDeviceType) and skip NVML-based enumeration, device counting and MIG handling.--device-discovery-strategy=cdiand allow CDI device-list strategies without NVML when using CDI discovery.devicePlugin.deviceDiscoveryStrategyanddevicePlugin.preConfiguredDeviceType.docs/nvidia-gb10-support.md.Recommended GB10 configuration
preConfiguredDeviceMemoryis a scheduling policy value (MiB), not a hardware readout — on a unified-memory GPU the memory is shared with the OS/CPU. The122566example is this DGX Spark's total system memory reported by Kubernetes (.status.capacity.memory = 125506464Ki, i.e.125506464 / 1024 ≈ 122566 MiB ≈ 119.7 GiB), not a direct GiB conversion (120 GiB would be 122880 MiB). Set it at or below the node's total unified memory, leaving OS headroom.The node also needs the
gpu=onlabel (device-plugin DaemonSet selector), and — unrelated to this change but required for the plugin to start on busy nodes — a sufficiently highfs.inotify.max_user_instances(the default 128 can be exhausted, causingcouldn't initialize inotify: too many open files). See the doc for details.Testing
go build ./...,go vet, andgolangci-lint(v2.12.2) — clean (0 issues) on all changed packages.buildCDIDeviceMap,listCDIGPUDevices,HasCDISpecs,NewCDIResourceManagers) and the external CDI handler; fullpkg/device-plugin/...andpkg/device/nvidia/...suites pass with--race.helm lint/helm templateverified (default render is unchanged / backward-compatible).invalid device discovery strategy) on the node.cdiVersion 0.7.0, kindk8s.device-plugin.nvidia.com/gpu, with a GPU-UUID device entry — matching the discovery vendor/class and theQualifiedNameused for injection.nvidia.com/gpu.product=NVIDIA-GB10(the defaultpreConfiguredDeviceType) and ~120 GiB unified memory.AI assistance disclosure
Per CONTRIBUTING.md: this contribution (code and docs) was developed with AI assistance under human supervision. I reviewed the generated code, and diagnosed the failure mode and validated the fix end-to-end against a real GB10 (DGX Spark) node as described in Testing.
Summary by CodeRabbit
deviceDiscoveryStrategyandpreConfiguredDeviceTypeconfiguration, with CDI-mode defaults toNVIDIA-GB10(when unset), including scheduler/device-plugin integration.