fix(device-plugin): guard Allocate() against empty DevicesIds to prevent process-crashing panic

[AyushSrivastava1818]

Summary

In pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go, Allocate() iterates over reqs.ContainerRequests and accesses req.DevicesIds[0] without checking if DevicesIds is empty. If a request with an empty DevicesIds slice is received, Go panics with runtime error: index out of range [0]. Because the gRPC server has no panic-recovery interceptor, this crashes the entire nvidia-device-plugin process — affecting every pod on the node waiting on GPU allocation, not just the one triggering the bad request.

This PR adds a guard at the top of the ContainerRequests loop:

• Calls PodAllocationFailed(nodename, current, NodeLockNvidia) to clean up node locks and pod status annotations.
• Returns nil, fmt.Errorf("invalid allocation request: container request %d has no DevicesIds", idx) for a clean gRPC error instead of a crash.

Fixes #2405

Changes

• pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go: add len(req.DevicesIds) == 0 check before req.DevicesIds[0] is accessed.
• pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go: add table-driven unit test TestAllocate_EmptyDevicesIdsAndRegression covering:
  1. Empty DevicesIds → non-nil error, no panic, PodAllocationFailed invoked (asserted directly, not just the error return).
  2. Existing non-MIG allocation path still succeeds unchanged.
  3. Existing MIG allocation path still succeeds unchanged.

Test Matrix & Verification Status

• Local (Windows): go test and make verify cannot compile pkg/device-plugin/nvidiadevice/nvinternal/plugin locally due to pre-existing CGo / Linux-only dependencies (go-nvml, nvidia-container-toolkit, opencontainers/cgroups using syscall.Mmap / unix.Mkdev). This occurs identically on an unmodified master checkout.
• CI: full compilation, go test, and make verify will run on Linux runners.

AI Assistance Disclosure

I used AI assistance (Claude) to identify this panic risk, implement the guard, and draft the regression test. I reviewed the diff, ran the test suite where locally possible, and take full responsibility for the final change.

Summary by CodeRabbit

• Bug Fixes

  • Prevented allocation requests with no device IDs from causing failures.
  • Added clear validation errors identifying the affected request.
  • Ensured allocation locks are released when validation fails.
  • Preserved successful allocation behavior for standard and MIG devices.

• Tests

  • Added regression coverage for empty device requests, failure handling, and successful standard and MIG allocations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3b9e1ae2-7de9-484a-888a-b099a25ff968

📥 Commits

Reviewing files that changed from the base of the PR and between 257b666 and 0ce8d38.

📒 Files selected for processing (1)

• pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go

---

📝 Walkthrough

Walkthrough

Allocate now rejects empty device-ID requests before indexing, releases the node allocation lock, and returns a validation error. Regression tests cover empty, non-MIG, and MIG allocation paths, including callback behavior.

Changes

Allocation validation

Layer / File(s)	Summary
Validate requests and test allocation outcomes pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go, pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go	Allocate handles empty DevicesIds lists without indexing. Tests verify the error, lock cleanup, failure handling, and successful MIG and non-MIG allocations.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

• device-plugin: Allocate() panics with index-out-of-range when a ContainerRequest has empty DevicesIds #2405: Tracks the Allocate() panic caused by empty DevicesIds input, which this change validates and tests.
• device-plugin: Allocate() panics with index-out-of-range when a ContainerRequest has empty DevicesIds #2414: Describes the same empty-DevicesIds validation fix in Allocate().

Possibly related PRs

• Project-HAMi/HAMi#2262: Also updates Allocate failure handling to release the node allocation lock.
• Project-HAMi/HAMi#2293: Also adds Allocate failure cleanup and regression coverage.
• Project-HAMi/HAMi#2307: Also modifies NVIDIA allocation failure handling and tests failure paths.

Suggested reviewers: archlitchi

Poem

A rabbit checked the device list,
Found no IDs hiding in the mist.
The lock was freed, the error clear,
MIG and non-MIG paths still cheer.
Tests now guard the burrow door.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the guard added to Allocate() to prevent a panic from empty DevicesIds.
Linked Issues check	✅ Passed	The implementation validates empty DevicesIds before indexing, releases the lock, returns an error, and adds regression coverage for issue #2405.
Out of Scope Changes check	✅ Passed	All changes directly support issue #2405 by fixing Allocate() validation and adding focused regression tests.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go (1)

1443-1455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the allocation contract for each table case.

The successful cases pass when Allocate returns an empty ContainerResponses slice. Assert the expected response count and allocated device identifiers for both MIG and non-MIG cases.

The empty request case only checks "has no DevicesIds". Assert "container request 0" so the test preserves the indexed validation-error contract.

🤖 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_test.go` around lines
1443 - 1455, The Allocate table tests currently verify only response presence
and errors, not allocation contents. Extend the success assertions around
plugin.Allocate to check the expected ContainerResponses count and allocated
device identifiers for both MIG and non-MIG cases, using the table case
expectations. Strengthen the empty-request error assertion to require the
indexed “container request 0” text in addition to “has no DevicesIds”.

🤖 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.

Nitpick comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go`:
- Around line 1443-1455: The Allocate table tests currently verify only response
presence and errors, not allocation contents. Extend the success assertions
around plugin.Allocate to check the expected ContainerResponses count and
allocated device identifiers for both MIG and non-MIG cases, using the table
case expectations. Strengthen the empty-request error assertion to require the
indexed “container request 0” text in addition to “has no DevicesIds”.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3c02bb31-743e-4288-a68d-ab1c1fffed84

📥 Commits

Reviewing files that changed from the base of the PR and between 90b2ede and fb90b55.

📒 Files selected for processing (2)

• pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
• pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
unittests	64.13% <100.00%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ce-plugin/nvidiadevice/nvinternal/plugin/server.go	43.15% <100.00%> (+1.00%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[archlitchi]

please resolve this conflicts

[archlitchi]

/assign

[hami-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AyushSrivastava1818
Once this PR has been reviewed and has the lgtm label, please ask for approval from archlitchi. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[AyushSrivastava1818]

@archlitchi resolved the conflicts take a look whenever you are free!

[FouoF]

Thanks for considering defensive handling here. No supported kubelet path or production reproduction reaching an Allocate request with an empty DevicesIds list has been shown; the supplied reproduction requires sending a malformed gRPC request directly. The current PR adds a large test harness for a very small guard and is already conflicting with master. The maintenance cost is not justified by the available evidence, so we are closing this PR. A new change would need a real kubelet/runtime reproduction or a confirmed protocol requirement.