Integrate vendor VFIO vGPUs into the instance lifecycle - #321
Conversation
199b1d1 to
a93d009
Compare
d893fce to
de4742a
Compare
de4742a to
18e047f
Compare
|
Trimmed over-engineering after an architectural review against the parent task (host kernel 6.8 / Ubuntu 24.04 support). Most of the removed weight came from earlier review-round additions rather than the original design: |
7ef1f8d to
bf21162
Compare
bf21162 to
83c53ea
Compare
83c53ea to
79eef07
Compare
ab2b0e8 to
19b9602
Compare
d92201d to
f177335
Compare
f177335 to
1d875f2
Compare
1d875f2 to
23fb2f5
Compare
A vGPU release during delete routinely fails when a GPU-busy VMM's kernel-side VFIO teardown outlives the force-kill wait. Delete's log-and-continue contract then deleted the metadata, stranding the VF until the next server restart and silently shrinking host GPU capacity. Hand the failed assignment to a bounded background retry (30s interval, 20 attempts) that re-runs the full release path — claim scan and destroy guards included — off the request path. The in-memory queue dedupes by device path; a restart abandons it and startup reconciliation sweeps the VF as before.
The two delete-continues tests triggered the new orphan retry with the default 30s delay, leaving a goroutine running ~10 minutes past the test. Use a millisecond delay and drain the queue before returning.
A SIGKILL delivered to QEMU while the NVIDIA vGPU plugin is still initializing the VF wedges it near-deterministically: the guest driver loops on RmInitAdapter timeouts with no host-side signal, and only an SR-IOV cycle of the parent GPU recovers it. Voluntary QEMU exits run their VFIO teardown and are safe, as are hard kills after init. Start-failure cleanup and the force-kill fallback for initializing vGPU instances now send SIGTERM and wait a bounded grace before SIGKILL, and a hard kill inside the init window logs the affected device path. Clean creates, graceful stops, and running-instance deletes are unchanged.
shutdownHypervisor (stop) could still SIGKILL an initializing vGPU QEMU directly: on QMP connect failure, on graceful-quit timeout, and when the hypervisor lacks graceful shutdown, bypassing the grace killHypervisor applies. Extract the SIGTERM-then-SIGKILL escalation into terminateThenKill and use it at all four force-kill sites.
Observed mid-init VFIO teardown completes in 1-2s, so 5s keeps 2-3x margin while halving the worst-case delay for a SIGTERM-ignoring process.
Running reports true ~4s before the guest driver finishes initializing and no host-side signal observes that boundary, so gating the SIGTERM grace on StateInitializing left a window where a failed stop or delete could SIGKILL QEMU mid-driver-init and wedge the VF. Post-init the SIGTERM is proven harmless and costs the grace only when the process ignores it.
A concurrent instance deletion between the claim scan's metadata listing and load turned ErrNotFound into a host-wide fail-closed release error, even though a vanished record cannot be a live claimant; skip it. Stop on a delete-only retention stub released its VF while leaving GPURetainedForCleanup set, so the stub's start/fork/snapshot errors kept claiming an assignment that no longer existed. Retention stubs now release only through delete, as documented.
The two rollback paths carried near-identical retention-stub literals that would drift as fields are added. Build both from one helper, use nowUTC like the rest of the file, and drop the starter guard that is dead since create fails on a nil starter long before the vGPU block.
Replace the listMetadataFilesWithStatErrors(bool) mode flag with listMetadataFiles / listMetadataFilesStrict so call sites say which failure semantics they rely on.
ListInstancesForReconcile failed hard when an instance was deleted between the metadata listing and its load. The startup call runs before the API serves, but the grace-period retry fires while deletes are in flight; one racing delete errored the whole list, which zeroed the retry and left vendor VFIO reconciliation disabled until the next restart. Skip ErrNotFound like the release claim scan does: a vanished record cannot claim a VF.
Giving up on an orphaned release leaves the VF allocated while /resources still advertises it, until startup reconciliation or manual remediation. That was visible only as a log line; count it so capacity leaks can alert.
Start, fork, and snapshot each carried a verbatim copy of the rejection error and its rationale; the stub fill in create's cleanup closure duplicated retainedVGPUFromCreateError field-for-field. One error value and one device-to-stub helper replace the copies.
The create and start handlers carried near-identical 20-line blocks deriving the vgpu_cleanup_pending message and inner error detail, differing only in the verb and release guidance.
- Retry the vendor VFIO reconcile sweep with a bounded delay when the startup instance listing fails, instead of disabling orphan recovery until the next process restart. One pending retry at a time. - Schedule the in-process orphaned-release retry when a rollback's retention record cannot be saved (create and start), instead of leaking the VF until restart. The retry scans claims without a self-exclusion because a restarted instance may hold the same VF. - Reject snapshot restore into a vGPU retention stub, matching start, fork, and snapshot. - Give passthrough PCI instances the same SIGTERM grace as vGPU instances on stop/delete, matching the QEMU-side vfioTermGraceFor. - Render the vgpu_cleanup_pending API detail from the error itself instead of duplicating its prose; use the manager clock in the claim scan; collapse the create-rollback retention branch. - Move ReconcileVGPUs off the Manager interface to a startup type assertion and unexport listInstancesForReconcile and hypervisorMayBeAlive.
sjmiller609
left a comment
There was a problem hiding this comment.
-
lib/instances/vgpu_orphan.go:24-27— path-only deduplication can discard cleanup for a newer assignment reusing the VF, leaving it orphaned until restart. -
lib/instances/vgpu_{orphan,reconcile,retention}.go— a periodic fail-closed reconciler could delete vgpu_orphan.go, CAS/timer retries, and stopped-instance release special cases while simplifying retention fallbacks. Also that doesn't have to run when we don't have gpus
Record the owning instance and assignment time for each vendor VFIO VF so a reconcile sweep can run while instances are being created: recently assigned VFs get a grace period before they are eligible (mirroring orphanedMdevGracePeriod), and owned VFs are destroyed with their recorded owner ID instead of failing the ownership check.
Run the fail-closed vGPU reconcile once at startup and every minute after, skipping hosts without a vGPU framework. Each pass retries releases for assignments whose owner is no longer live (re-verified under the instance lock) and then sweeps device-level leftovers with no live metadata claim. This deletes the per-path orphan retry goroutines - whose path-keyed dedup could drop cleanup for a newer assignment reusing the same VF - the CAS/timer retry in ReconcileVGPUs, the stopped-instance release special case in StopInstance, the retention fallbacks that scheduled background retries, and the orphan-abandoned metric.
yummybomb
left a comment
There was a problem hiding this comment.
found one blocker in the current head. the periodic reconciler can classify a live mdev assignment as stale when the post-boot PID save failed, then remove it from the running VM. please add the nil-PID/live-socket regression alongside the fix.
ci is also red on linux and darwin because the API tests still assert startup reconcile after the user-facing guidance changed to periodic vGPU reconcile (instances_test.go:107 and :1041).
non-blocking: the five-minute assignment safety window is duplicated in lib/instances/vgpu.go and lib/devices/vendor_vfio_linux.go; one shared source would keep these policies from drifting. the retention/reconciler split is otherwise much clearer than the per-path retry machinery.
| if storedVGPUDevicePath(stored) == "" { | ||
| continue | ||
| } | ||
| livePID := stored.HypervisorPID != nil && hypervisorMayBeAlive(stored.HypervisorProcessIdentity, stored.SocketPath) |
There was a problem hiding this comment.
blocker: this gate makes a live socket owner invisible whenever its PID was not persisted. both create and start intentionally treat the post-boot metadata save as non-fatal, so disk can contain an old GPUAssignedAt plus a valid SocketPath and a nil HypervisorPID while the VM is still running. after five minutes this path calls releaseStaleVGPUAssignment; for mdev, releaseStoredVGPU skips the live-claim scan and DestroyMdev writes the sysfs remove file without an in-use check, so the reconciler can tear the mdev out of that live VM. please resolve socket ownership even with an empty stored identity (hypervisorMayBeAlive already supports that), make vgpuAssignmentLiveness honor that result without requiring a non-nil stored PID, and repeat the check under the instance lock. a regression test should use an old nil-PID assignment with a live socket owner.
An assignment could lose its persisted hypervisor PID while its VM stays alive - a post-boot metadata save failure or a hypeman crash before the save. The liveness checks gated the socket-ownership scan on a non-nil PID, so once the startup grace expired the reconciler considered such an assignment stale and could remove an mdev out from under the live VM (DestroyMdev has no in-use guard). Run the socket-ownership scan unconditionally: a live VMM always holds its control-socket listener, and a missing listener still resolves to not-alive, so genuinely stopped instances are released as before. The claim scan in releaseStoredVGPUExcluding gets the same treatment.
The five-minute fresh-assignment protection existed three times: the instances startup grace, the mdev orphan grace, and the vendor VFIO sweep grace. Define it once in lib/devices and alias the instances constant to it.
Summary
Top layer of the vendor VFIO vGPU stack (
generalize-vgpu-device#322 ←hypervisor-liveness#363 ←vendor-vfio-backend#364 ← this). The backend itself is in #364; this layer wires it into the instance lifecycle and adds the release guards that make reusable-VF assignments safe:ListInstancesForReconcile, which fails on any unreadable metadata instead of silently skipping it. Recent assignments receive bounded startup protection when their PID is absent or stale; when the inventory is unavailable, vendor VFIO reconciliation is skipped entirely while mdev reconciliation still runs.vgpu_cleanup_pendingwith retained/unretained guidance; the retention record keeps identity fields (name, image, hypervisor) so it lists as a recognizable, deletable instance. The instance API exposesgpu.device_path, the assignment identity on vendor VFIO hosts wheremdev_uuidis empty.Testing
go build ./...,go vet ./...cleango test -racepasses forlib/instancestargeted suites,lib/devices,lib/resources,lib/builds(TestCreateInstanceWithNetworkneeds image pulls + iptables and fails in this environment on the unmodified base as well)Note
High Risk
Changes GPU assignment persistence, release ordering, and hypervisor teardown on production-critical paths; incorrect reconcile or claim logic could drop live vGPUs or leak VFs.
Overview
Vendor VFIO vGPUs are wired through create/start/stop/delete instead of being blocked at the device layer. Assignments get
GPUAssignedAt, persisted metadata before boot, and retention stubs (GPURetainedForCleanup) when create/start rollback cannot release the VF—those stubs are delete-only and block start, fork, and snapshots until cleared.Orphan cleanup moves from a one-shot mdev reconcile at API startup to a periodic vGPU reconciler (initial pass + ~1 minute). It releases stale instance-held assignments with a 5-minute grace for booting VMs, builds a protected device set from strict metadata listing (vendor VFIO sweep skipped if listing fails), and sweeps device-level leftovers. Vendor VFIO release is fail-closed against live claimants via socket/PID liveness checks.
Create/start API errors map
VGPUCleanupPendingErrortovgpu_cleanup_pendingwith retained vs reconcile guidance and inner error codes. Instance responses addgpu.device_path.QEMU and instance teardown send SIGTERM before SIGKILL for VFIO/vGPU workloads (
VFIOTermGrace) to reduce VF wedge risk; hypervisor kill paths use the same pattern with a wedge warning on forced kill.Reviewed by Cursor Bugbot for commit bfe2816. Bugbot is set up for automated code reviews on this repo. Configure here.