Skip to content

refactor: replace NVML util panics with error returns #2234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shellyco-code wants to merge 1 commit into from
Closed

refactor: replace NVML util panics with error returns #2234

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 24 additions & 22 deletions pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,29 +121,25 @@ var eraseNextDeviceTypeFromAnnotation = func(dtype string, p corev1.Pod) error {
return util.PatchPodAnnotations(&p, newannos)
}

func GetIndexAndTypeFromUUID(uuid string) (string, int) {
func GetIndexAndTypeFromUUID(uuid string) (string, int, error) {
defer nvml.Shutdown()
if nvret := nvml.Init(); nvret != nvml.SUCCESS {
klog.Errorln("nvml Init err: ", nvret)
panic(0)
return "", 0, fmt.Errorf("nvml Init err: %v", nvret)
}
originuuid := strings.Split(uuid, "[")[0]
ndev, ret := nvml.DeviceGetHandleByUUID(originuuid)
if ret != nvml.SUCCESS {
klog.Error("nvml get handlebyuuid error ret=", ret)
panic(0)
return "", 0, fmt.Errorf("nvml get handlebyuuid error ret=%v", ret)
}
Model, ret := ndev.GetName()
if ret != nvml.SUCCESS {
klog.Error("nvml get name error ret=", ret)
panic(0)
return "", 0, fmt.Errorf("nvml get name error ret=%v", ret)
}
index, ret := ndev.GetIndex()
if ret != nvml.SUCCESS {
klog.Error("nvml get index error ret=", ret)
panic(0)
return "", 0, fmt.Errorf("nvml get index error ret=%v", ret)
}
return Model, index
return Model, index, nil
}

func GetMigUUIDFromSmiOutput(output string, uuid string, idx int) string {
Expand Down Expand Up @@ -178,17 +174,15 @@ func GetMigUUIDFromSmiOutput(output string, uuid string, idx int) string {
return ""
}

func GetMigUUIDFromIndex(uuid string, idx int) string {
func GetMigUUIDFromIndex(uuid string, idx int) (string, error) {
defer nvml.Shutdown()
if nvret := nvml.Init(); nvret != nvml.SUCCESS {
klog.Errorln("nvml Init err: ", nvret)
panic(0)
return "", fmt.Errorf("nvml Init err: %v", nvret)
}
originuuid := strings.Split(uuid, "[")[0]
ndev, ret := nvml.DeviceGetHandleByUUID(originuuid)
if ret != nvml.SUCCESS {
klog.Error(`nvml get device uuid error ret=`, ret)
panic(0)
return "", fmt.Errorf("nvml get device uuid error ret=%v", ret)
}
migdev, ret := nvml.DeviceGetMigDeviceHandleByIndex(ndev, idx)
if ret != nvml.SUCCESS {
Expand All @@ -199,18 +193,17 @@ func GetMigUUIDFromIndex(uuid string, idx int) string {
cmd.Stderr = &stderr
err := cmd.Run()
if err != nil {
klog.Fatalf("nvidia-smi -L failed with %s\n", err)
return "", fmt.Errorf("nvidia-smi -L failed with %s", err)
}
outStr := stdout.String()
uuid := GetMigUUIDFromSmiOutput(outStr, originuuid, idx)
return uuid
return uuid, nil
Comment on lines 198 to +200

@coderabbitai coderabbitai Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Return an error when the fallback does not find a MIG UUID.

GetMigUUIDFromSmiOutput returns "" when the requested MIG device is absent from the output. Lines 198-200 currently report that result as success. Line 505 then appends an empty device identifier.

Return a non-nil error when migUUID == "" so the existing error path logs and skips the device.

Proposed fix 
-			uuid := GetMigUUIDFromSmiOutput(outStr, originuuid, idx)
-			return uuid, nil
+			migUUID := GetMigUUIDFromSmiOutput(outStr, originuuid, idx)
+			if migUUID == "" {
+				return "", fmt.Errorf("nvidia-smi -L returned no MIG UUID for GPU %q at index %d", originuuid, idx)
+			}
+			return migUUID, nil
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
outStr := stdout.String()
uuid := GetMigUUIDFromSmiOutput(outStr, originuuid, idx)
return uuid
return uuid, nil
outStr := stdout.String()
migUUID := GetMigUUIDFromSmiOutput(outStr, originuuid, idx)
if migUUID == "" {
return "", fmt.Errorf("nvidia-smi -L returned no MIG UUID for GPU %q at index %d", originuuid, idx)
}
return migUUID, nil
🤖 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/util.go` around lines 198 -
200, Update the fallback path around GetMigUUIDFromSmiOutput to return a non-nil
error when the resolved MIG UUID is empty, instead of returning empty success.
Preserve the existing successful return for non-empty UUIDs so callers use their
current error path to log and skip the device.

}
res, ret := migdev.GetUUID()
if ret != nvml.SUCCESS {
klog.Error(`nvml get mig uuid error ret=`, ret)
panic(0)
return "", fmt.Errorf("nvml get mig uuid error ret=%v", ret)
}
return res
return res, nil
}

func GetMigGpuInstanceIdFromIndex(uuid string, idx int) (int, error) {
Expand Down Expand Up @@ -465,7 +458,11 @@ func (nv *NvidiaDevicePlugin) GetContainerDeviceStrArray(c device.ContainerDevic
if !strings.Contains(val.UUID, "[") {
tmp = append(tmp, val.UUID)
} else {
devtype, devindex := GetIndexAndTypeFromUUID(val.UUID)
devtype, devindex, err := GetIndexAndTypeFromUUID(val.UUID)
if err != nil {
klog.Errorf("failed to get index and type from UUID %s: %v", val.UUID, err)
continue
}
Comment on lines +461 to +465

@coderabbitai coderabbitai Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Fail allocation when conversion drops a requested device.

These continue statements shorten tmp, but GetContainerDeviceStrArray cannot report that loss. Allocate passes this result to getAllocateResponse, then removes the pending allocation annotation and marks the allocation successful after a nil response error. It does not compare the converted-device count with devreq.

Return an error from GetContainerDeviceStrArray when any requested device cannot convert. Propagate that error through Allocate. Keep device discovery-time skipping separate from allocation-time failure handling.

Also applies to: 500-505

🤖 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/util.go` around lines 461 -
465, Update GetContainerDeviceStrArray so conversion failures from
GetIndexAndTypeFromUUID return an error instead of continuing with a shortened
device list; preserve skipping behavior only for device discovery paths.
Propagate this error through Allocate before calling getAllocateResponse,
ensuring failed conversion does not remove the pending allocation annotation or
mark the allocation successful.

position, needsreset = nv.GenerateMigTemplate(devtype, devindex, val)
if needsreset {
nv.ApplyMigTemplate()
Expand Down Expand Up @@ -500,7 +497,12 @@ func (nv *NvidiaDevicePlugin) GetContainerDeviceStrArray(c device.ContainerDevic
}
}
}
tmp = append(tmp, GetMigUUIDFromIndex(val.UUID, position))
migUUID, err := GetMigUUIDFromIndex(val.UUID, position)
if err != nil {
klog.Errorf("failed to get mig uuid for %s: %v", val.UUID, err)
continue
}
tmp = append(tmp, migUUID)
}
}
klog.V(3).Infoln("mig current=", nv.migCurrent, ":", needsreset, "position=", position, "uuid lists", tmp)
Expand Down
Loading