fix(csi): detach orphan hot-plug volumes when VMI outlives its VM - #2866
Conversation
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Upstream treats a missing parent VM as detach success, so when the VMI outlives its VM during node reprovisioning the hot-plug pod keeps the infra device exclusively attached to the source host and subsequent attaches fail. Re-check VMI volume status after upstream unpublish and detach from the VMI directly. Backport of kubevirt/csi-driver#184. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWrappedControllerService adds post-unpublish verification for hotplug volumes via a new ChangesHotplug Volume Unpublish Verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where hot-plug volumes remain exclusively attached to the source host during node reprovisioning when a VirtualMachineInstance outlives its parent VirtualMachine. By adding a verification step in the controller wrapper to check VMI volume status after the upstream unpublish process, the driver now correctly cleans up orphaned volumes, preventing subsequent attachment failures. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper method unpublishHotplugVolume in the KubeVirt CSI driver controller to handle the detachment of hotplug volumes directly from the Virtual Machine Instance (VMI) if they remain attached after upstream unpublishing. It also updates the kubevirt.io/csi-driver dependency. A critical issue was identified in the namespace lookup within unpublishHotplugVolume: the code incorrectly queries the VMI in the infrastructure namespace (w.infraNamespace) instead of the tenant namespace parsed from the node ID, which would cause the lookup to fail and silently bypass the detachment logic.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| _, vmName, err := cache.SplitMetaNamespaceKey(req.GetNodeId()) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to parse node ID %q: %v", req.GetNodeId(), err) | ||
| } | ||
|
|
||
| // GetVirtualMachine on this client returns a VirtualMachineInstance, not a VM. | ||
| vmi, err := w.virtClient.GetVirtualMachine(ctx, w.infraNamespace, vmName) | ||
| if err != nil { | ||
| if errors.IsNotFound(err) { | ||
| return &csi.ControllerUnpublishVolumeResponse{}, nil | ||
| } | ||
| return nil, status.Errorf(codes.Unavailable, | ||
| "cannot verify VMI %s/%s after unpublish of %s: %v", w.infraNamespace, vmName, dvName, err) | ||
| } | ||
|
|
||
| stillAttached := false | ||
| for _, vs := range vmi.Status.VolumeStatus { | ||
| if vs.HotplugVolume != nil && vs.Name == dvName { | ||
| stillAttached = true | ||
| break | ||
| } | ||
| } | ||
| if !stillAttached { | ||
| return &csi.ControllerUnpublishVolumeResponse{}, nil | ||
| } | ||
|
|
||
| klog.Infof("Volume %s still reported by VMI %s/%s after unpublish – detaching from the VMI", dvName, w.infraNamespace, vmName) | ||
| if err := w.virtClient.RemoveVolumeFromVMI(ctx, w.infraNamespace, vmName, &kubevirtv1.RemoveVolumeOptions{Name: dvName}); err != nil { | ||
| return nil, status.Errorf(codes.Unavailable, | ||
| "failed to remove volume %s from VMI %s/%s: %v", dvName, w.infraNamespace, vmName, err) | ||
| } | ||
| if err := w.virtClient.EnsureVolumeRemoved(ctx, w.infraNamespace, vmName, dvName, 2*time.Minute); err != nil { | ||
| return nil, status.Errorf(codes.Unavailable, | ||
| "volume %s failed to be removed from VMI %s/%s in time: %v", dvName, w.infraNamespace, vmName, err) | ||
| } | ||
|
|
||
| klog.V(3).Infof("Successfully unpublished volume %s from VMI %s/%s", dvName, w.infraNamespace, vmName) | ||
| return &csi.ControllerUnpublishVolumeResponse{}, nil |
There was a problem hiding this comment.
In unpublishHotplugVolume, the code parses the namespace and name of the VM/VMI from req.GetNodeId(), but discards the namespace (_) and instead uses w.infraNamespace to look up and modify the VMI.
In Cozystack, the VMI runs in the tenant namespace (which is vmNamespace parsed from the node ID), whereas w.infraNamespace is the namespace of the infrastructure cluster where the CSI driver and storage resources are located. Looking up the VMI in w.infraNamespace will result in a NotFound error, which is caught and silently ignored (returning success). This completely bypasses the orphan hot-plug volume detachment logic.
We should use vmNamespace instead of w.infraNamespace for all VMI operations in this function, matching the behavior in publishHotplugVolume.
vmNamespace, vmName, err := cache.SplitMetaNamespaceKey(req.GetNodeId())
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse node ID %q: %v", req.GetNodeId(), err)
}
// GetVirtualMachine on this client returns a VirtualMachineInstance, not a VM.
vmi, err := w.virtClient.GetVirtualMachine(ctx, vmNamespace, vmName)
if err != nil {
if errors.IsNotFound(err) {
return &csi.ControllerUnpublishVolumeResponse{}, nil
}
return nil, status.Errorf(codes.Unavailable,
"cannot verify VMI %s/%s after unpublish of %s: %v", vmNamespace, vmName, dvName, err)
}
stillAttached := false
for _, vs := range vmi.Status.VolumeStatus {
if vs.HotplugVolume != nil && vs.Name == dvName {
stillAttached = true
break
}
}
if !stillAttached {
return &csi.ControllerUnpublishVolumeResponse{}, nil
}
klog.Infof("Volume %s still reported by VMI %s/%s after unpublish – detaching from the VMI", dvName, vmNamespace, vmName)
if err := w.virtClient.RemoveVolumeFromVMI(ctx, vmNamespace, vmName, &kubevirtv1.RemoveVolumeOptions{Name: dvName}); err != nil {
return nil, status.Errorf(codes.Unavailable,
"failed to remove volume %s from VMI %s/%s: %v", dvName, vmNamespace, vmName, err)
}
if err := w.virtClient.EnsureVolumeRemoved(ctx, vmNamespace, vmName, dvName, 2*time.Minute); err != nil {
return nil, status.Errorf(codes.Unavailable,
"volume %s failed to be removed from VMI %s/%s in time: %v", dvName, vmNamespace, vmName, err)
}
klog.V(3).Infof("Successfully unpublished volume %s from VMI %s/%s", dvName, vmNamespace, vmName)
return &csi.ControllerUnpublishVolumeResponse{}, nil|
Successfully created backport PR for |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.5
git worktree add -d .worktree/backport-2866-to-release-1.5 origin/release-1.5
cd .worktree/backport-2866-to-release-1.5
git switch --create backport-2866-to-release-1.5
git cherry-pick -x 6b4d7adb2c9ba96a80a34b44680d2769a7cc10f7 68136f9292f8f6a877ea0ce137ed97caf81f2a4a |
What this PR does
During node reprovisioning in tenant Kubernetes clusters, the VMI (and its hot-plug pod) can outlive the parent VirtualMachine. The upstream kubevirt-csi-driver treats a missing VM as "detach succeeded", so the hot-plug pod stays alive and keeps the infra storage device exclusively attached to the source host — every subsequent attach of that volume fails, on DRBD with
failed to set source device readwrite. This was hit repeatedly in production tenant clusters (see kubevirt/csi-driver#182).This PR:
kubevirt.io/csi-drivermodule to the latest upstream commit;VMI.status.volumeStatusand, if the hot-plug is still reported there (e.g. because the parent VM is already gone), detaches it from the VMI directly via theremovevolumesubresource and waits until it disappears.The backport is self-contained in the wrapper and can be dropped once kubevirt/csi-driver#184 merges upstream and the module pin is bumped past it.
Credit to mattia-eleuteri for the root-cause analysis and the upstream fix.
Screenshots
Not a UI change.
Release note
Summary by CodeRabbit