refactor(MIG): support MIG instance dynamic allocation/deallocation - #2378
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR replaces legacy MIG geometry and ChangesDynamic MIG allocation
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related PRs
Suggested reviewers: 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 |
github-actions
Bot
added
kind/enhancement
c9e65ab to
ffa8ab3
Compare
04ff0b5 to
e004b39
Compare
e004b39 to
5c04cbd
Compare
5c04cbd to
5786b1a
Compare
|
As this feature correctness depends on NVML driver behavior. UT can not cover it very well. So I added an e2e test script instead. I think we can ignore the codecov check. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/scheduler/scheduler.go (1)
662-711: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed for unconsumed MIG allocation reservations.
Lines 687-706 consume one reservation per cached
ContainerDevicebut discard remaining entries inallocationsByGPU.If
hami.io/vgpu-mig-allocationscontains an extra valid reservation for a parent GPU, scheduler state omits its placement. The scheduler can then allocate capacity that the runtime reservation already occupies.After processing the Pod, account for every remaining reservation or mark the matching MIG device unhealthy. Add a test where the annotation has more reservations than
PodDevicesentries.🤖 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/scheduler/scheduler.go` around lines 662 - 711, Ensure the resource-rebuild logic around allocationsByGPU accounts for every unconsumed MIG reservation after processing each Pod. For any remaining reservation associated with a parent GPU, add its placement to the matching device’s MigAllocationsInUse when possible, or mark that MIG device unhealthy to fail closed; do not silently discard leftovers. Add a test covering an annotation with more reservations than cached ContainerDevice entries.
🧹 Nitpick comments (10)
cmd/scheduler/metrics.go (1)
195-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for allocations that are not runtime-ready.
The current test only verifies
RuntimeReady: true. Add aRuntimeReady: falseallocation and assert thathami_node_gpu_mig_instance_infodoes not expose it. This protects the readiness filter from regression.🤖 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 `@cmd/scheduler/metrics.go` around lines 195 - 228, Extend the test covering MIG instance metrics to include an allocation with RuntimeReady set to false, alongside the existing ready allocation. Assert that hami_node_gpu_mig_instance_info excludes the non-ready allocation while retaining the ready allocation’s metric, covering the filter in the MIG allocation iteration.cmd/vGPUmonitor/metrics.go (1)
515-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a collector-level MIG metric test.
metrics_mig_test.goonly testsmigAllocationContainerName. Add a test that supplies pod annotations and verifies the emittedhami_mig_device_infolabels. Include an allocation without complete runtime identity and assert that collection skips it.🤖 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 `@cmd/vGPUmonitor/metrics.go` around lines 515 - 545, Add a collector-level test covering the MIG allocation loop that provides pod annotations with valid and incomplete allocations, then verifies the emitted hami_mig_device_info metric labels for the valid allocation. Assert that an allocation missing MigUUID, GPUInstanceID, or ComputeInstanceID is skipped, using the existing metrics collection entry point and test helpers in metrics_mig_test.go.pkg/device-plugin/nvidiadevice/nvinternal/plugin/migmgr.go (2)
591-609: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReturn an error for an unknown GI profile ID instead of defaulting to 1 slice.
profileIDToCIProfileIDreturnsCOMPUTE_INSTANCE_PROFILE_1_SLICEfor any unrecognized input. Callers then query the wrong compute-instance profile and silently skip real compute instances. Return a second boolean result, and let callers reject unknown profiles.🤖 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/migmgr.go` around lines 591 - 609, Update profileIDToCIProfileID to return the mapped compute-instance profile plus a boolean indicating whether giProfileID was recognized; remove the default 1-slice fallback and mark unknown inputs false. Update every caller to handle the boolean and reject or propagate unknown GI profiles before querying compute-instance profiles.
531-566: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused MIG placement helpers.
pickFreePlacement,placementCandidates,giProfileIDToSliceKey,chooseFreePlacement, andpreferHighPlacementare only defined or referenced from tests in Go files. Remove them frommigmgr.goso placement selection has one authority.🤖 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/migmgr.go` around lines 531 - 566, Remove the unused placement helpers pickFreePlacement, placementCandidates, giProfileIDToSliceKey, chooseFreePlacement, and preferHighPlacement from migmgr.go, along with any imports used only by them. Leave the active placement-selection implementation unchanged so it has a single authority.pkg/device/nvidia/device.go (1)
752-762: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the comment to the new profile/placement model.
The comment refers to "the device's current template" and to resharding. This PR removed MIG templates and geometry. Describe the condition as "no allowed MIG profile has a free placement on this device" so the comment matches
CustomFilterRule.🤖 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/nvidia/device.go` around lines 752 - 762, The comment in the dev.Mode == MigMode branch should reflect the profile/placement model: replace references to the device’s current template, topology, and resharding with wording that explains no allowed MIG profile has a free placement on the device. Keep the behavior and logging unchanged.pkg/device/nvidia/device_test.go (1)
1226-1236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the MIG failure path of
AddResourceUsage.This test now covers only the non-MIG path.
pkg/device/nvidia/mig_capability_test.gocovers the successful MIG path. No test covers the new error return"MIG profile and placement allocation failed". Add a case withMode: MigModeand no free placement, and assert thatAddResourceUsagereturns an error and leavesn.Used,n.Usedmem, andn.Usedcoresunchanged.🤖 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/nvidia/device_test.go` around lines 1226 - 1236, Extend TestDevices_AddResourceUsage with a MIG-mode case using Mode: MigMode and no available placement, then assert AddResourceUsage returns the “MIG profile and placement allocation failed” error. Verify the usage fields Used, Usedmem, and Usedcores remain unchanged after the failure.pkg/device-plugin/nvidiadevice/nvinternal/plugin/mig_startup.go (2)
141-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify the MIG-mode branch condition.
Line 148 tests
ret == nvml.ERROR_NOT_SUPPORTEDand thenret != nvml.SUCCESS. The second test already covers the first. Also confirm thatGetMaxMigDeviceCountreturning a count larger than the number of live MIG devices is expected here; the loop relies onGetMigDeviceHandleByIndexreturning a non-SUCCESScode for empty slots and continues silently in that case.🤖 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/mig_startup.go` around lines 141 - 153, Simplify the MIG-mode condition in the loop around GetMigMode by removing the redundant ret == nvml.ERROR_NOT_SUPPORTED check and retaining the general ret != nvml.SUCCESS or disabled-mode handling. Verify that the count from GetMaxMigDeviceCount is intentionally used as the loop bound, with GetMigDeviceHandleByIndex failures for empty slots continuing silently.
116-126: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInitialize NVML once for MIG UUID lookups.
GetContainerDeviceStrArraycallsgpuUUIDToIndex(reservation.GPUUUID)for every MIG reservation on Allocate, and active-pod/runtime-info paths also call it. NVML reference-counts initialization, while the current helpers each callnvml.Init()withoutnvml.Shutdown. Move a singlenvml.Init()into plugin startup and make these helpers assume NVML is initialized.🤖 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/mig_startup.go` around lines 116 - 126, Initialize NVML once during plugin startup, before any MIG UUID lookup paths can run, and handle startup failure consistently. Update gpuUUIDToIndex and related lookup helpers to assume NVML is already initialized by removing their per-call nvml.Init invocation; keep UUID handle and index error handling unchanged.pkg/device/nvidia/mig_capability_test.go (1)
51-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case with a non-empty
toAllocate.Both cases pass
nilfortoAllocate, so the queued-request branch ofCustomFilterRuleis untested. Add a case that passes container devices already selected for this GPU and for a different GPU. That case pins the device-identity behavior discussed inpkg/device/nvidia/device.goLines 581-593. Add a case where the request exceeds every profile, to cover thefalsereturn ofselectMigCandidate.🤖 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/nvidia/mig_capability_test.go` around lines 51 - 67, Extend TestCustomFilterUsesReportedPlacementCapacity with non-empty toAllocate cases containing devices selected for the same GPU and for a different GPU, verifying only matching device identities affect CustomFilterRule’s queued-request path. Also add a request whose memory exceeds every MIG profile and assert CustomFilterRule returns false through selectMigCandidate.pkg/device/nvidia/mig_allocations.go (1)
48-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent skip can produce a partial annotation.
EncodeMigAllocationsskips any device whoseCustomInfolacks a valid profile or placement, and still returnstrueif at least one device produced an entry. The device plugin then rejects the Pod at Allocate time, becauseGetContainerDeviceStrArrayrequires one reservation per requested device (pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go, Line 305). Add a log line for each skipped MIG device so the cause is visible in the scheduler logs.🤖 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/nvidia/mig_allocations.go` around lines 48 - 77, Update EncodeMigAllocations to log each MIG device skipped because CustomInfo is missing or its profile/placement is invalid, including the device identifiers and skip reason, while preserving the existing filtering and return behavior.
🤖 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 `@hack/hami-mig-e2e.sh`:
- Around line 35-44: Scope the MIG test to one explicitly selected target node:
add a preflight topology check if single-node operation is required, bind Pods
created by create_pod to that node, and run mig_count/profile_count against its
GPU. Update the device-plugin restart logic to delete or restart only the plugin
Pod on the target node rather than rolling out the DaemonSet cluster-wide.
- Around line 26-33: Register the defined cleanup function with an EXIT trap
immediately after its definition so it runs on every script exit path, including
failed assertions. Preserve the existing cleanup behavior and avoid relying
solely on the final explicit cleanup call.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/mig_startup_test.go`:
- Around line 13-21: Reorder the imports in the test so standard-library
packages remain first, k8s.io dependencies form the external group, and the
github.com/Project-HAMi/HAMi/pkg/device/nvidia import is placed in its own
trailing local group. Apply the repository’s goimports grouping convention
without changing code behavior.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/migmgr.go`:
- Around line 236-271: Update destroyAllMigInstances to enumerate and destroy
every compute-instance profile available for each gi, rather than only the
single profile returned by profileIDToCIProfileID(giProfileID). Complete all
successful GetComputeInstances results and propagate Destroy errors before
calling gi.Destroy(), preserving the existing GPU-instance destruction order.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go`:
- Around line 186-194: The MIG registration flow in the isMigMode branch must
handle an empty result from discoverMigProfiles before appending the device: log
a clear warning identifying the model and missing profiles, or skip registration
of the zero-capacity device. Preserve the existing Count calculation when
profiles are discovered.
- Around line 203-236: Update the core-percentage calculation in the MIG profile
discovery flow to use the device’s physical SM count from
GetAttributes().MultiprocessorCount, matching Devcore = 100. Retain
fullGPUMultiprocessors as a fallback only when the device attribute value is
unavailable, and use the selected device capacity when computing profile.Core.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go`:
- Around line 307-335: Move NewMigInstanceManager initialization for
plugin.migMgr to the plugin setup path before plugin.Serve and plugin.Register,
ensuring Allocate requests always see an initialized manager. Remove the later
assignment from the MIG startup block, leaving only GPU reset, annotation
adoption, and reconciliation work there; preserve non-MIG behavior and ensure
initialization is not concurrently written after serving begins.
- Around line 283-296: Update the deviceSupportMig initialization and validation
in the MIG capability check so an empty deviceNames result is treated as
unsupported. Ensure MIG is enabled only when at least one device name exists and
every name matches an entry in plugin.schedulerConfig.MigProfileAllowlist, while
preserving the existing unsupported behavior for unmatched models.
- Around line 794-800: In the refresh path within the request allocation flow,
update the Pod fetch around current so Get assigns to a temporary Pod variable
first; check the error before replacing current, and pass the still-valid
current Pod to PodAllocationFailed on failure. Only update current after a
successful fetch.
- Around line 362-394: Update annotateMigRuntimeInfo to patch annotations with
optimistic concurrency using the Pod’s current resourceVersion, ensuring
concurrent updates to hami.io/vgpu-mig-allocations cannot be overwritten; use
the Kubernetes patch options precondition rather than the unconditional
PatchPodAnnotations path, while preserving the existing runtime-field updates.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go`:
- Around line 311-326: Update deviceAllocateFromAnnotation to track MIG UUIDs
returned by EnsureAllocation and release all already-created allocations through
nv.migMgr.Release when a later reservation or allocation step fails. Ensure
cleanup occurs before returning the error, including failures from reservation
validation, gpuUUIDToIndex, or EnsureAllocation, while preserving the existing
successful output behavior.
In `@pkg/device/nvidia/device.go`:
- Around line 619-624: The MIG selection path must use the same effective memory
value as accounting: update Fit and its CustomFilterRule invocation to resolve
percentage-based requests into the derived memory amount before filtering, or
otherwise pass that effective value instead of the unmodified request.Memreq.
Ensure the value used to select the MIG profile matches ContainerDevice.Usedmem
consumed by AddResourceUsage.
- Around line 581-593: In CustomFilterRule, filter toAllocate entries by
devusage.ID before calling selectMigCandidate, so only allocations queued for
the GPU currently being evaluated reserve MIG placements. Preserve the existing
occupied-placement accumulation and final request check, while excluding entries
belonging to other GPU UUIDs and preventing duplicate reservations when the same
device is revisited.
---
Outside diff comments:
In `@pkg/scheduler/scheduler.go`:
- Around line 662-711: Ensure the resource-rebuild logic around allocationsByGPU
accounts for every unconsumed MIG reservation after processing each Pod. For any
remaining reservation associated with a parent GPU, add its placement to the
matching device’s MigAllocationsInUse when possible, or mark that MIG device
unhealthy to fail closed; do not silently discard leftovers. Add a test covering
an annotation with more reservations than cached ContainerDevice entries.
---
Nitpick comments:
In `@cmd/scheduler/metrics.go`:
- Around line 195-228: Extend the test covering MIG instance metrics to include
an allocation with RuntimeReady set to false, alongside the existing ready
allocation. Assert that hami_node_gpu_mig_instance_info excludes the non-ready
allocation while retaining the ready allocation’s metric, covering the filter in
the MIG allocation iteration.
In `@cmd/vGPUmonitor/metrics.go`:
- Around line 515-545: Add a collector-level test covering the MIG allocation
loop that provides pod annotations with valid and incomplete allocations, then
verifies the emitted hami_mig_device_info metric labels for the valid
allocation. Assert that an allocation missing MigUUID, GPUInstanceID, or
ComputeInstanceID is skipped, using the existing metrics collection entry point
and test helpers in metrics_mig_test.go.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/mig_startup.go`:
- Around line 141-153: Simplify the MIG-mode condition in the loop around
GetMigMode by removing the redundant ret == nvml.ERROR_NOT_SUPPORTED check and
retaining the general ret != nvml.SUCCESS or disabled-mode handling. Verify that
the count from GetMaxMigDeviceCount is intentionally used as the loop bound,
with GetMigDeviceHandleByIndex failures for empty slots continuing silently.
- Around line 116-126: Initialize NVML once during plugin startup, before any
MIG UUID lookup paths can run, and handle startup failure consistently. Update
gpuUUIDToIndex and related lookup helpers to assume NVML is already initialized
by removing their per-call nvml.Init invocation; keep UUID handle and index
error handling unchanged.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/migmgr.go`:
- Around line 591-609: Update profileIDToCIProfileID to return the mapped
compute-instance profile plus a boolean indicating whether giProfileID was
recognized; remove the default 1-slice fallback and mark unknown inputs false.
Update every caller to handle the boolean and reject or propagate unknown GI
profiles before querying compute-instance profiles.
- Around line 531-566: Remove the unused placement helpers pickFreePlacement,
placementCandidates, giProfileIDToSliceKey, chooseFreePlacement, and
preferHighPlacement from migmgr.go, along with any imports used only by them.
Leave the active placement-selection implementation unchanged so it has a single
authority.
In `@pkg/device/nvidia/device_test.go`:
- Around line 1226-1236: Extend TestDevices_AddResourceUsage with a MIG-mode
case using Mode: MigMode and no available placement, then assert
AddResourceUsage returns the “MIG profile and placement allocation failed”
error. Verify the usage fields Used, Usedmem, and Usedcores remain unchanged
after the failure.
In `@pkg/device/nvidia/device.go`:
- Around line 752-762: The comment in the dev.Mode == MigMode branch should
reflect the profile/placement model: replace references to the device’s current
template, topology, and resharding with wording that explains no allowed MIG
profile has a free placement on the device. Keep the behavior and logging
unchanged.
In `@pkg/device/nvidia/mig_allocations.go`:
- Around line 48-77: Update EncodeMigAllocations to log each MIG device skipped
because CustomInfo is missing or its profile/placement is invalid, including the
device identifiers and skip reason, while preserving the existing filtering and
return behavior.
In `@pkg/device/nvidia/mig_capability_test.go`:
- Around line 51-67: Extend TestCustomFilterUsesReportedPlacementCapacity with
non-empty toAllocate cases containing devices selected for the same GPU and for
a different GPU, verifying only matching device identities affect
CustomFilterRule’s queued-request path. Also add a request whose memory exceeds
every MIG profile and assert CustomFilterRule returns false through
selectMigCandidate.
🪄 Autofix
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: 9f19bdbd-92bf-48fd-bba5-317cf834ffa0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
charts/hami/templates/scheduler/device-configmap.yamlcmd/scheduler/metrics.gocmd/scheduler/metrics_test.gocmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_mig_test.godocker/Dockerfiledocker/Dockerfile.hamimasterdocker/Dockerfile.no-core-testdocker/Dockerfile.withlibdocs/develop/mig-dynamic-deallocate.mdgo.modhack/hami-mig-e2e.shpkg/device-plugin/nvidiadevice/nvinternal/plugin/mig_startup.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/mig_startup_test.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/migmgr.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/migmgr_placement_test.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/register.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/register_test.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/server.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/util.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/util_test.gopkg/device/common/common.gopkg/device/devices.gopkg/device/devices_test.gopkg/device/nvidia/device.gopkg/device/nvidia/device_test.gopkg/device/nvidia/mig_allocations.gopkg/device/nvidia/mig_capability_test.gopkg/device/nvidia/mig_profiles.gopkg/device/nvidia/mig_profiles_test.gopkg/device/nvidia/mig_topology.gopkg/device/nvidia/mig_topology_test.gopkg/device/pod_test.gopkg/device/pods.gopkg/scheduler/mig_allocation_test.gopkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.gopkg/scheduler/score_test.gopkg/scheduler/webhook_test.go
💤 Files with no reviewable changes (6)
- docker/Dockerfile.hamimaster
- docker/Dockerfile
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/register_test.go
- docker/Dockerfile.withlib
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/util_test.go
- pkg/scheduler/scheduler_test.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)
313-325: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winComplete MIG startup before plugin registration.
Startserves and registers before collecting active MIG allocations, destroying idle instances, and adopting annotations. AnAllocatecall can then callEnsureAllocationand create a GI/CI after the snapshot whileResetIdleGPUsis still deleting instances on GPUs not found in that snapshot. Do registration after reset and annotation adoption complete, or protect startup work and allocation under a common gate.🤖 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 313 - 325, Update the plugin.Start startup sequence so MIG allocation detection, ResetIdleGPUs, and annotation adoption finish before serving or registering the plugin. Ensure Allocate/EnsureAllocation cannot run concurrently with this initialization, either by moving registration after startup work or guarding both paths with a shared initialization gate.
🤖 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 313-325: Update the plugin.Start startup sequence so MIG
allocation detection, ResetIdleGPUs, and annotation adoption finish before
serving or registering the plugin. Ensure Allocate/EnsureAllocation cannot run
concurrently with this initialization, either by moving registration after
startup work or guarding both paths with a shared initialization gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02450ca8-a892-4675-8863-bdbf737649e7
📒 Files selected for processing (8)
hack/hami-mig-e2e.shpkg/device-plugin/nvidiadevice/nvinternal/plugin/mig_startup_test.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/migmgr.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/register.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/server.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/util.gopkg/device/nvidia/device.gopkg/device/nvidia/mig_capability_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/mig_startup_test.go
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go
- pkg/device/nvidia/device.go
Signed-off-by: Jifei Wang <jifei.wang@dynamia.ai>
Signed-off-by: Jifei Wang <jifei.wang@dynamia.ai>
Signed-off-by: Jifei Wang <jifei.wang@dynamia.ai>
6142cd9 to
9c5464e
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, FouoF The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
NVIDIA MIG manager and current dynamic MIG can only apply by node scope, whether it is the former or latter. So changing MIG instances requires drain and cordon the node.
Which issue(s) this PR fixes:
HAMi RoadMap v2.10 Flexible MIG
Special notes for your reviewer:
Added e2e script and already passed on A100-40G * 1 instance.
Known limits:
Does this PR introduce a user-facing change?:
Only introduce internal API changes, dynamic MIG can use as usual.
MIG related metrics label changed for better match DCGM style.
Summary by CodeRabbit
New Features
Documentation
Testing