Match vGPU allocation by pod identity and device IDs#139
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nishitnshah 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 @nishitnshah! It looks like this is your first PR to Project-HAMi/volcano-vgpu-device-plugin 🎉 |
📝 WalkthroughWalkthroughThis PR adds pod identity annotations and exact device matching for pending pod selection, then wires Allocate to use the new matcher for MIG and non-MIG paths. It also adds tests for matching, ambiguity, and metadata mismatch cases. ChangesDevice identity matching for pending pod allocation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
nishitnshah
force-pushed
the
nishitnshah/test-pending-pod-matching
branch
from
012d256 to
b08dc74
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: 2
🧹 Nitpick comments (2)
pkg/util/util.go (1)
300-307: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify rationale for self-referential identity check.
podIdentityAnnotationsMatchcompares a pod's identity annotations to its ownName/Namespace/UID. This is presumably a defense against stale/incorrectly-copied Volcano annotations (per the referenced volcano-sh/volcano#5570 assumption), but as written it reads as tautological. A short comment explaining the staleness scenario it guards against would help future maintainers.🤖 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/util/util.go` around lines 300 - 307, Add a brief comment in podIdentityAnnotationsMatch explaining that the check is intentionally self-referential to detect stale or incorrectly copied identity annotations, not a tautology; mention that the pod’s AssignedPodNameAnnotations, AssignedPodNamespaceAnnotations, and AssignedPodUIDAnnotations must match the pod’s current Name, Namespace, and UID to guard against reused Volcano annotations.pkg/util/util_test.go (1)
1-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood repro/regression coverage; consider adding a case for mismatched pod identity annotations.
All test pods built via
podWithAllocatedDevicehave identity annotations that match their own Name/Namespace/UID, sopodIdentityAnnotationsMatchreturningfalseis never directly exercised. A test with a pod whoseAssignedPodNameAnnotations/AssignedPodUIDAnnotationsdeliberately don't match its own identity would close this gap and guard the new matcher's core safety property.🤖 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/util/util_test.go` around lines 1 - 123, Add a regression test in util_test.go that exercises pod identity annotation mismatch so podIdentityAnnotationsMatch false is covered. Reuse podWithAllocatedDevice, then deliberately mutate AssignedPodNameAnnotations or AssignedPodUIDAnnotations on one pod so it no longer matches its Name/Namespace/UID, and assert GetMatchingDeviceRequest rejects it with the expected not-found style error. Keep the test near the existing GetMatchingDeviceRequest cases so the matcher behavior is validated alongside the current ambiguity and stale-annotation coverage.
🤖 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/plugin/server.go`:
- Around line 394-402: The non-MIG allocation path in server.go is shadowing the
outer current pod variable, so PodAllocationTrySuccess later sees nil for
non-MIG requests. In the GetMatchingPendingDeviceRequest block, replace the
short declaration with assignment to the already-declared
current/currentCtr/devreq/err variables so the outer current is populated, and
verify the same current value is then used consistently in the allocation loop
and the final PodAllocationTrySuccess call.
In `@pkg/util/util.go`:
- Around line 260-294: GetMatchingDeviceRequest currently aborts the entire
search when a pod’s decoded device index is out of range, which prevents valid
matches from being returned. Update the loop in GetMatchingDeviceRequest so that
the container index check for pod.Spec.Containers skips the malformed candidate
and continues scanning other pods/entries instead of returning an error
immediately. Keep the existing matchCount, matchedPod, matchedContainer, and
matchedDevices logic intact so only truly valid matches are considered, and
preserve the final ambiguous/not found errors based on the accumulated matches.
---
Nitpick comments:
In `@pkg/util/util_test.go`:
- Around line 1-123: Add a regression test in util_test.go that exercises pod
identity annotation mismatch so podIdentityAnnotationsMatch false is covered.
Reuse podWithAllocatedDevice, then deliberately mutate
AssignedPodNameAnnotations or AssignedPodUIDAnnotations on one pod so it no
longer matches its Name/Namespace/UID, and assert GetMatchingDeviceRequest
rejects it with the expected not-found style error. Keep the test near the
existing GetMatchingDeviceRequest cases so the matcher behavior is validated
alongside the current ambiguity and stale-annotation coverage.
In `@pkg/util/util.go`:
- Around line 300-307: Add a brief comment in podIdentityAnnotationsMatch
explaining that the check is intentionally self-referential to detect stale or
incorrectly copied identity annotations, not a tautology; mention that the pod’s
AssignedPodNameAnnotations, AssignedPodNamespaceAnnotations, and
AssignedPodUIDAnnotations must match the pod’s current Name, Namespace, and UID
to guard against reused Volcano annotations.
🪄 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: e9702367-513d-4c7a-a888-be66d25cbd46
📒 Files selected for processing (4)
pkg/plugin/server.gopkg/util/types.gopkg/util/util.gopkg/util/util_test.go
nishitnshah
force-pushed
the
nishitnshah/test-pending-pod-matching
branch
from
b08dc74 to
28b3eff
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/util/util_test.go (1)
45-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd multiset device-ID coverage.
All matching tests here (Lines 45-126) request exactly one device UUID per container. The PR objective explicitly calls out matching against an "exact device UUID multiset," but no test exercises a container with multiple devices, verifies order-independent matching, or asserts rejection when the requested IDs are a strict subset/superset of the container's device set. Given this multiset comparison is central to the new matcher's correctness, consider adding at least one test with 2+ devices per container to cover this path.
🧪 Example additional test case
func TestGetMatchingDeviceRequestMatchesMultipleDeviceUUIDs(t *testing.T) { const nodeName = "gpu-node-1" pod := podWithAllocatedDevices("multi-pod", "default", "multi-uid", nodeName, "100", "worker", []string{"GPU-1", "GPU-2"}) // Requested in different order than stored; should still match. _, container, devices, err := GetMatchingDeviceRequest(NvidiaGPUDevice, nodeName, []string{"GPU-2", "GPU-1"}, []v1.Pod{*pod}) if err != nil { t.Fatalf("expected match, got error: %v", err) } if container.Name != "worker" || len(devices) != 2 { t.Fatalf("expected 2 matched devices, got %#v", devices) } // Requesting a strict subset should not match. if _, _, _, err := GetMatchingDeviceRequest(NvidiaGPUDevice, nodeName, []string{"GPU-1"}, []v1.Pod{*pod}); err == nil { t.Fatal("expected no match for partial device ID subset") } }🤖 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/util/util_test.go` around lines 45 - 126, The current tests around GetMatchingDeviceRequest only cover single-device cases, so add coverage for exact multiset matching with multiple device UUIDs. Extend the test set near TestGetMatchingDeviceRequestUsesPodIdentityAndDeviceID to include a pod with 2+ assigned devices and verify order-independent matching in GetMatchingDeviceRequest, plus rejection for strict subset/superset requests. Reuse helpers like podWithAllocatedDevice or add a multi-device variant so the matcher’s exact multiset behavior is exercised.
🤖 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/util/util_test.go`:
- Around line 45-126: The current tests around GetMatchingDeviceRequest only
cover single-device cases, so add coverage for exact multiset matching with
multiple device UUIDs. Extend the test set near
TestGetMatchingDeviceRequestUsesPodIdentityAndDeviceID to include a pod with 2+
assigned devices and verify order-independent matching in
GetMatchingDeviceRequest, plus rejection for strict subset/superset requests.
Reuse helpers like podWithAllocatedDevice or add a multi-device variant so the
matcher’s exact multiset behavior is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 39096f9c-668a-4f04-b425-8a9cd70cef65
📒 Files selected for processing (4)
pkg/plugin/server.gopkg/util/types.gopkg/util/util.gopkg/util/util_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/util/types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/plugin/server.go
- pkg/util/util.go
Use Volcano pod identity annotations and kubelet-requested device IDs to select the pending vGPU pod/container for non-MIG allocations. This prevents the oldest-pod heuristic from mounting another pod's /tmp/vgpu HostPath when multiple same-node allocations are pending. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Nishit Shah <nish511@gmail.com>
nishitnshah
force-pushed
the
nishitnshah/test-pending-pod-matching
branch
from
28b3eff to
9842103
Compare
1 participant
Summary
Use Volcano pod identity annotations plus kubelet-requested device IDs to select the pending vGPU pod/container for non-MIG allocations. This replaces the oldest-pod heuristic in the HAMi vGPU allocation path, preventing the device plugin from building
/tmp/vgpu/containers/<uid>_<container>from another pending pod when multiple same-node vGPU allocations overlap.Addresses issue #138.
This PR assumes the pod identity annotations added by volcano-sh/volcano#5570 are present:
volcano.sh/vgpu-pod-namevolcano.sh/vgpu-pod-namespacevolcano.sh/vgpu-pod-uidThat Volcano PR should merge before this one.
Testing
Master-compatible repro against current heuristic:
Branch validation: