fix: skip faulty devices instead of panicking during registration - #2231
fix: skip faulty devices instead of panicking during registration#2231shellyco-code wants to merge 2 commits into
Conversation
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shellyco-code 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 |
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
pls add a unit test covering the skip path.
no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice
|
@mesutoezdil getAPIDevices() directly calls nvml.Init(), which panics in CI (no GPU) so we can't test the skip path unless we refactor the plugin to use a mockable nvml.Interface. Should I include that refactor in this PR, or skip the test? |
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
📝 WalkthroughWalkthroughNVIDIA device registration now logs NVML query failures, skips affected devices, and continues discovery. It also warns when all discovered devices are skipped and the device list is empty. ChangesNVIDIA device discovery
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
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 (1)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go (1)
111-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the device UUID in skip logs.
The
GetIndex()andGetName()error logs include onlyret. When multiple GPUs fail, operators cannot identify the skipped device. IncludeUUIDin both messages.Suggested log context
- klog.Errorln("nvml get index error ret=", ret) + klog.Errorln("nvml get index error uuid=", UUID, "ret=", ret) - klog.Error("nvml get name error ret=", ret) + klog.Error("nvml get name error uuid=", UUID, "ret=", ret)Also applies to: 139-142
🤖 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 111 - 114, Update the error logs in the device registration flow around GetIndex() and GetName() to include the corresponding device UUID alongside ret, while preserving the existing continue behavior for both failures.
🤖 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/register.go`:
- Around line 109-114: Replace the panic in getAPIDevices() when nvml.Init()
fails with error propagation, then have RegisterInAnnotation() return that error
and update WatchAndRegister() to handle it by retrying while preserving the last
valid annotation. Add coverage for NVML initialization failure and individual
device failure paths.
---
Nitpick comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go`:
- Around line 111-114: Update the error logs in the device registration flow
around GetIndex() and GetName() to include the corresponding device UUID
alongside ret, while preserving the existing continue behavior for both
failures.
🪄 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: 40ea26ec-aa02-41fc-9fc5-e3d5a320c0a7
📒 Files selected for processing (1)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go
| continue | ||
| } | ||
| idx, ret := ndev.GetIndex() | ||
| if ret != nvml.SUCCESS { | ||
| klog.Errorln("nvml get index error ret=", ret) | ||
| panic(0) | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'nvml\.Init\(\)|panic\(0\)|getAPIDevices|RegisterInAnnotation' \
pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go \
pkg/device-plugin/nvidiadevice/nvinternal/plugin/register_test.goRepository: Project-HAMi/HAMi
Length of output: 4931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== register.go relevant sections ==="
sed -n '89,200p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go | cat -n | sed 's/^/register.go /'
sed -n '250,285p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go | cat -n | sed 's/^/register.go /'
echo "=== occurrences of nvidia.Init() / nvml.Init() / getAPIDevices / WatchAndRegister / RegisterInAnnotation ==="
rg -n -C 3 'nvml\.Init\(|\.Init\(\)|getAPIDevices|WatchAndRegister|RegisterInAnnotation|initInterface|Nvml' pkg/device-plugin/nvidiadevice/nvinternal/pkg pkg/device-plugin/nvidiadevice/nvinternal/plugin -g '*.go'Repository: Project-HAMi/HAMi
Length of output: 27427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== RegisterInAnnotation body ==="
sed -n '197,245p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go | cat -n | sed 's/^/register.go /'
echo "=== factory init handling ==="
sed -n '90,130p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/factory.go | cat -n | sed 's/^/factory.go /'
echo "=== Read-only structural check: getAPIDevices returns pointer and RegisterInAnnotation propagates no error from getAPIDevices ==="
python3 - <<'PY'
from pathlib import Path
p = Path('pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go')
s = p.read_text()
print('signature:', 'func (plugin *NvidiaDevicePlugin) getAPIDevices() *[]*device.DeviceInfo {' in s)
print('getAPIDevices returns &res:', '&res' in s[s.index('func (plugin *NvidiaDevicePlugin) getAPIDevices'):s.index('// RegisterInAnnotation')])
check = 'devices := plugin.getAPIDevices()'
idx = s.index(check)
block = s[idx:idx+400]
print('RegisterInAnnotation calls getAPIDevices:', True)
print('RegisterInAnnotation immediately accesses result:', '*devices' in block or 'len(*devices)' in block)
print('RegisterInAnnotation has explicit error from getAPIDevices:', any(tok in block for tok in [':= getAPIDevices', '= getAPIDevices', 'return nil']))
print('current RegisterInAnnotation return:', 'return true, err' in block or 'return false, err' in block)
PYRepository: Project-HAMi/HAMi
Length of output: 5559
Handle nvml.Init() failures from getAPIDevices() without panicking.
getAPIDevices() still calls panic(0) when nvml.Init() fails, which crashes WatchAndRegister and can skip the node annotation path. Return this error through RegisterInAnnotation() and call it from WatchAndRegister() so WatchAndRegister() retries without replacing the last valid annotation. Add tests for NVML initialization and per-device failure paths.
🤖 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
109 - 114, Replace the panic in getAPIDevices() when nvml.Init() fails with
error propagation, then have RegisterInAnnotation() return that error and update
WatchAndRegister() to handle it by retrying while preserving the last valid
annotation. Add coverage for NVML initialization failure and individual device
failure paths.
|
explained in #2233 |
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it: This PR fixes a critical reliability issue where the HAMi device plugin crashes via panic(0) if a single GPU fails NVML queries during the buildDeviceMap registration loop (e.g. if the driver or hardware is in a bad state).
Instead of panicking and crashing the entire plugin, the loop now logs the error via klog.Errorln and safely uses continue to skip the faulty device, allowing the plugin to successfully register the remaining healthy GPUs on the node.
Which issue(s) this PR fixes: Fixes #2233
Special notes for your reviewer:
This replaces all panic(0) calls with continue in the registration loop.
I have added DCO sign-off (Signed-off-by) to the commit.
Does this PR introduce a user-facing change?:
Note :This PR was created with the assistance of an AI coding agent.
release-note
Fix: the device plugin will no longer crash entirely if a single GPU is faulty or fails NVML queries during initialization.
Summary by CodeRabbit