Repository navigation
fix(csi): route RWX Block volumes to upstream hotplug detach - #2658
Conversation
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 improves the CSI driver's volume handling logic to correctly identify and process RWX Block volumes. Previously, these volumes were incorrectly categorized as NFS, leading to improper cleanup during unpublish operations and incorrect expansion configurations. By refining the volume identification criteria, the driver now ensures proper lifecycle management for live-migration disks, preventing potential attachment conflicts. 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
|
github-actions
Bot
added
area/uncategorized
📝 WalkthroughWalkthroughThe PR adds an isNFSVolume(pvc) helper that classifies PVCs as NFS only when they are RWX and Filesystem (not Block), and updates ControllerUnpublishVolume and ControllerExpandVolume to use this predicate for cleanup and node-expansion decisions. ChangesNFS Volume Classification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refines the identification of NFS volumes within the KubeVirt CSI driver by introducing the isNFSVolume function. This helper ensures that RWX Block volumes, used for live migration, are correctly handled via the upstream hotplug removal path rather than being treated as NFS, which prevents issues with stale VM-spec entries. Feedback suggests making the isNFSVolume check more robust by explicitly verifying the Filesystem volume mode to accommodate potential future Kubernetes updates.
| func isNFSVolume(pvc *corev1.PersistentVolumeClaim) bool { | ||
| if !hasRWXAccessMode(pvc) { | ||
| return false | ||
| } | ||
| if pvc.Spec.VolumeMode != nil && *pvc.Spec.VolumeMode == corev1.PersistentVolumeBlock { | ||
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
The current implementation of isNFSVolume treats any non-Block RWX volume as an NFS volume. While Kubernetes currently only supports Filesystem and Block modes, it's more robust to explicitly check for Filesystem (or nil, which defaults to Filesystem) to ensure that any future volume modes aren't accidentally routed to the NFS path.
func isNFSVolume(pvc *corev1.PersistentVolumeClaim) bool {
if !hasRWXAccessMode(pvc) {
return false
}
return pvc.Spec.VolumeMode == nil || *pvc.Spec.VolumeMode == corev1.PersistentVolumeFilesystem
}There was a problem hiding this comment.
Good call — switched isNFSVolume to the explicit Filesystem-only form in c0db805. The predicate now returns true only when VolumeMode is nil (defaults to Filesystem) or Filesystem, so any future VolumeMode value won't accidentally fall through to the NFS path.
dosubot
Bot
added
area/kubernetes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go (1)
374-383:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid early success on missing PVC; it can skip required hotplug detach.
If PVC lookup returns NotFound (Line 377), the method exits before the upstream
ControllerUnpublishVolumecall (Line 383). For RWX Block, this can still leave stale VM spec volume entries when PVC is already gone, which is exactly the attach-blocking failure mode this PR is fixing.Proposed fix
pvc, err := w.infraClient.CoreV1().PersistentVolumeClaims(w.infraNamespace).Get(ctx, dvName, metav1.GetOptions{}) if err != nil { if errors.IsNotFound(err) { - return &csi.ControllerUnpublishVolumeResponse{}, nil + // PVC may already be deleted, but VM-spec hotplug cleanup may still be required. + resp, upErr := w.ControllerService.ControllerUnpublishVolume(ctx, req) + if upErr != nil { + return nil, upErr + } + return resp, nil } return nil, err } if !isNFSVolume(pvc) { return w.ControllerService.ControllerUnpublishVolume(ctx, req) }🤖 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 `@packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go` around lines 374 - 383, The code currently returns early with success when the PVC Get returns NotFound, skipping the upstream cleanup; change this so a NotFound error does not short-circuit the flow: when infraClient.CoreV1().PersistentVolumeClaims(...).Get returns a NotFound, do not return immediately—mark pvc as missing (or set a pvcNotFound flag) and fall through to the existing logic that decides whether to call w.ControllerService.ControllerUnpublishVolume; specifically ensure the branch around isNFSVolume(pvc) and the call to w.ControllerService.ControllerUnpublishVolume(ctx, req) still executes for the cases that need hotplug detach (e.g., RWX Block) even if pvc is missing, while preserving the original behavior for NFS volumes.
🤖 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 `@packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go`:
- Around line 374-383: The code currently returns early with success when the
PVC Get returns NotFound, skipping the upstream cleanup; change this so a
NotFound error does not short-circuit the flow: when
infraClient.CoreV1().PersistentVolumeClaims(...).Get returns a NotFound, do not
return immediately—mark pvc as missing (or set a pvcNotFound flag) and fall
through to the existing logic that decides whether to call
w.ControllerService.ControllerUnpublishVolume; specifically ensure the branch
around isNFSVolume(pvc) and the call to
w.ControllerService.ControllerUnpublishVolume(ctx, req) still executes for the
cases that need hotplug detach (e.g., RWX Block) even if pvc is missing, while
preserving the original behavior for NFS volumes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31b46753-31cc-4e5f-9da5-4da2d8d7f9c0
📒 Files selected for processing (1)
packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go
WrappedControllerService.ControllerUnpublishVolume decided between NFS cleanup and upstream hotplug detach using only hasRWXAccessMode(pvc). RWX Block volumes (KubeVirt live-migration disks) match that check and were routed to the NFS path, which only deletes a CiliumNetworkPolicy. For a block volume that CNP does not exist, so removeCNPOwnerReference returns successfully without doing anything, and the function reports success without ever calling upstream's hotplug detach. The DataVolume entry stays in VM.spec.template.spec.volumes; when the same PVC later attaches to a different VM, linstor-csi's anti-split-brain check sees pods from two different VMs referencing the same RWX block PVC and rejects the attach permanently. Introduce isNFSVolume(pvc) requiring both ReadWriteMany access mode and VolumeMode=Filesystem (always true for PVCs provisioned via our NFS CreateVolume path). RWX Block now delegates to upstream, which removes the entry from VM spec via RemoveVolumeFromVM. The same predicate replaces the access-mode check in ControllerExpandVolume - RWX Block volumes still need in-VM disk expansion. Fixes #2634 Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Address review feedback from gemini-code-assist on packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go:562: classify a PVC as NFS only when its VolumeMode is Filesystem (or nil, which defaults to Filesystem), instead of "anything not Block". Future VolumeMode values will no longer be silently routed to the NFS path. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
2f57b1c to
c0db805
Compare
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)
packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go (1)
374-383:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't ack unpublish when PVC lookup misses.
If the PVC is already gone, this still returns success before the upstream hotplug detach runs. That reintroduces the stale
VM.spec.template.spec.volumescase from issue#2634on any path where publish updated the VM spec but the PVC/DataVolume lookup no longer succeeds. Please fall back tow.ControllerService.ControllerUnpublishVolumehere instead of returning an empty success response.Suggested fix
pvc, err := w.infraClient.CoreV1().PersistentVolumeClaims(w.infraNamespace).Get(ctx, dvName, metav1.GetOptions{}) if err != nil { if errors.IsNotFound(err) { - return &csi.ControllerUnpublishVolumeResponse{}, nil + return w.ControllerService.ControllerUnpublishVolume(ctx, req) } return nil, err }🤖 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 `@packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go` around lines 374 - 383, The current PVC Get error handling returns an empty success when the PVC is not found, which hides failures and reintroduces stale VM spec issues; change the branch that handles errors.IsNotFound(err) to delegate to the upstream unpublish implementation by calling and returning w.ControllerService.ControllerUnpublishVolume(ctx, req) instead of returning &csi.ControllerUnpublishVolumeResponse{} so that the normal unpublish path runs; ensure this change is applied in the code that does pvc, err := w.infraClient.CoreV1().PersistentVolumeClaims(...).Get(...) and keep the existing non-NFS check that calls w.ControllerService.ControllerUnpublishVolume(ctx, req) for non-NFS volumes.
🤖 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 `@packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go`:
- Around line 374-383: The current PVC Get error handling returns an empty
success when the PVC is not found, which hides failures and reintroduces stale
VM spec issues; change the branch that handles errors.IsNotFound(err) to
delegate to the upstream unpublish implementation by calling and returning
w.ControllerService.ControllerUnpublishVolume(ctx, req) instead of returning
&csi.ControllerUnpublishVolumeResponse{} so that the normal unpublish path runs;
ensure this change is applied in the code that does pvc, err :=
w.infraClient.CoreV1().PersistentVolumeClaims(...).Get(...) and keep the
existing non-NFS check that calls
w.ControllerService.ControllerUnpublishVolume(ctx, req) for non-NFS volumes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00de61da-f4a5-4ff2-be8f-e932c1a0d00a
📒 Files selected for processing (1)
packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — routing predicate now correctly distinguishes our RWX-Filesystem NFS volumes from upstream RWX-Block hotplug volumes; the latter delegates to upstream ControllerUnpublishVolume/ControllerExpandVolume so VM-spec entries get cleaned up and Block disks keep their node-side expansion path. Gemini's VolumeMode==Filesystem tightening landed in c0db805.
Business context: RWX Block volumes for KubeVirt live-migration were silently routed through the NFS cleanup branch (hasRWXAccessMode-only predicate), leaving the DataVolume entry in VM.spec.template.spec.volumes after unpublish; on the next attach to a different VM, the linstor-csi anti-split-brain check then permanently blocks the operation.
myasnikovdaniil
added
the
kind/backport
|
Successfully created backport PR for |
Aleksei Sviridkin (lexfrei)
removed
the
area/uncategorized
2 participants
What this PR does
Fixes routing in
WrappedControllerServiceso RWX Block volumes (KubeVirt live-migration disks) are no longer treated as NFS during unpublish and expand.Root cause
ControllerUnpublishVolumeselected the NFS cleanup branch based onhasRWXAccessMode(pvc)alone. RWX Block matched and went down the NFS path — which just deletes aCiliumNetworkPolicy. For a block volume that CNP does not exist, soremoveCNPOwnerReferencereturns successfully without doing anything, and the function reports success without ever calling upstream's hotplug detach. The DataVolume entry stays inVM.spec.template.spec.volumespermanently. When the same PVC later attaches to a different VM, linstor-csi's anti-split-brain check sees pods from two different VMs referencing the same RWX block PVC and blocks the attach with:ControllerPublishVolumewas unaffected because it routes on the explicitnfsVolumeKeyfromVolumeContext.ControllerUnpublishVolumedoes not receiveVolumeContextand has to infer from the PVC, and the previous inference was incomplete.Fix
isNFSVolume(pvc)now requiresReadWriteManyaccess mode andVolumeMode == Filesystem(always set by our NFSCreateVolumepath). RWX Block correctly delegates to upstreamControllerUnpublishVolume, which removes the entry from VM spec viaRemoveVolumeFromVM. The same predicate replaces the previous access-mode check inControllerExpandVolume, which was also clearingNodeExpansionRequiredfor RWX Block — block volumes still need in-VM disk expansion.Fixes #2634.
Release note
Summary by CodeRabbit
Bug Fixes
Refactor