fix(csi): verify VMI Ready after kubevirt-csi Publish to surface stuck-PVC failures - #2659
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 introduces a defensive wrapper around the CSI ControllerPublishVolume operation to address silent failures during hotplug volume attachment. By verifying the VMI volume status after the upstream driver reports success, the system can now correctly identify and surface underlying provisioning issues (such as stuck PVCs) to the CSI layer, preventing downstream errors in the tenant kubelet. 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
|
|
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 selected for processing (1)
📝 WalkthroughWalkthroughThe controller imports KubeVirt API types and adds publishHotplugVolume, which delegates publishing to the upstream controller, then fetches the VM and verifies the target volume exists and reached VolumeReady in vmi.Status.VolumeStatus. ControllerPublishVolume routes non‑NFS requests through this verification path. ChangesHotplug Volume Readiness Verification
Sequence DiagramsequenceDiagram
participant Client
participant ControllerPublishVolume
participant publishHotplugVolume
participant UpstreamController
participant KubeClient
participant VMIStatus as vmi.Status.VolumeStatus
Client->>ControllerPublishVolume: ControllerPublishVolume (non‑NFS)
ControllerPublishVolume->>publishHotplugVolume: route request
publishHotplugVolume->>UpstreamController: ControllerPublishVolume (delegate)
UpstreamController-->>publishHotplugVolume: publish response
publishHotplugVolume->>KubeClient: Get VMI by NodeId
KubeClient-->>publishHotplugVolume: VMI object
publishHotplugVolume->>VMIStatus: verify volume presence & VolumeReady
VMIStatus-->>publishHotplugVolume: status result
publishHotplugVolume-->>ControllerPublishVolume: Unavailable or Success
ControllerPublishVolume-->>Client: final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
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 |
There was a problem hiding this comment.
Code Review
This pull request introduces a publishHotplugVolume method to the KubeVirt CSI driver to verify volume readiness in the VirtualMachineInstance (VMI) status after a publish operation. This ensures that the volume is actually ready before reporting success to the external-attacher. A critical bug was identified where the code incorrectly attempts to access VolumeStatus on a VirtualMachine object instead of a VirtualMachineInstance, which would lead to a compilation or runtime error. The reviewer suggested using GetVirtualMachineInstance and improved error handling for cases where the VMI is not found or the API call fails.
| vmi, getErr := w.virtClient.GetVirtualMachine(ctx, vmNamespace, vmName) | ||
| if getErr != nil { | ||
| klog.Warningf("Cannot verify VMI %s/%s readiness after publish of %s: %v", vmNamespace, vmName, dvName, getErr) | ||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
The code currently calls GetVirtualMachine, but VolumeStatus is a field of VirtualMachineInstanceStatus, not VirtualMachineStatus. Accessing vmi.Status.VolumeStatus on a VirtualMachine object will likely result in a compilation error or incorrect behavior as the types are distinct in the KubeVirt API.
Additionally, if the VMI is not found (e.g., the VM is powered off), the check should be skipped to maintain compatibility with upstream behavior where "attached" simply means "present in VM spec". For other errors (like API timeouts), it is safer to return codes.Unavailable to trigger a retry rather than silently succeeding, which would bypass the intended safety check.
vmi, getErr := w.virtClient.GetVirtualMachineInstance(ctx, vmNamespace, vmName)
if getErr != nil {
if errors.IsNotFound(getErr) {
return resp, nil
}
return nil, status.Errorf(codes.Unavailable, "failed to get VMI %s/%s to verify readiness: %v", vmNamespace, vmName, getErr)
}There was a problem hiding this comment.
The type-error claim is incorrect: the upstream kubevirt.io/csi-driver client defines GetVirtualMachine as returning *kubevirtv1.VirtualMachineInstance (see pkg/kubevirt/client.go in the vendored module), despite the misleading method name. Accessing vmi.Status.VolumeStatus on the result is type-correct, and go build/go vet pass. I've added a one-line comment at the call site so the next reader doesn't have to look this up.
The error-handling point is valid and overlaps with coderabbitai's thread. Addressed in cbe4b86: parse failures now return codes.Internal and VMI get failures return codes.Unavailable, so we never silently report Publish success when the readiness check couldn't run.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go`:
- Around line 258-267: The current fallback branches return the successful
response (resp, nil) when cache.SplitMetaNamespaceKey(req.GetNodeId()) or
w.virtClient.GetVirtualMachine(...) fail, which can falsely mark
VolumeAttachment.Attached=true; change both branches to return a gRPC error
instead of resp, nil (e.g. return status.Errorf(...) with appropriate codes like
codes.InvalidArgument or codes.Internal and include the node ID/vmi info) so
that NodePublishVolume does not report success when VolumeReady proof couldn't
run; update the branches that check parseErr and getErr (and any callers that
rely on resp) accordingly to avoid setting VolumeReady/Attachment on transient
parse/lookup failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8890d1a0-80d6-41f4-9b78-168adc29412f
📒 Files selected for processing (1)
packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go
Upstream ControllerPublishVolume has a fast path that returns success
based only on the volume's presence in VM.spec.template.spec.volumes
(via EnsureVolumeAvailableVM). If a prior Publish wrote that entry but
the volume never became Ready in VMI status - e.g. the infra PVC is
stuck ClaimPending because LINSTOR cannot autoplace - the retry hits
the fast path and reports success. external-attacher marks the
VolumeAttachment Attached=true, but QEMU was never sent device_add, so
the tenant kubelet fails NodeStageVolume with:
rpc error: code = Unknown desc = couldn't find device by serial id
Wrap upstream Publish for hotplug volumes (non-NFS) with a post-call
check that the volume reached VolumeReady in VMI.Status.VolumeStatus.
When it has not, the RPC returns codes.Unavailable carrying the actual
reason (e.g. "PVC is in phase ClaimPending"), so external-attacher
keeps retrying and the underlying provisioning failure is visible at
the CSI layer rather than as a confusing "device not found by serial
id" on the tenant kubelet.
This is a wrapper-side workaround for an upstream defect in
kubevirt.io/csi-driver; the structural fix belongs upstream (tighten
the fast path to require VMI Ready) and will be filed separately.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…nnot run The fallback branches in publishHotplugVolume silently returned the upstream success response when the node ID failed to parse or the VMI get call returned an error. external-attacher then set Attached=true without VolumeReady proof, which recreates the false-positive attach this wrapper exists to prevent. Fail loudly instead: - parseErr -> codes.Internal: external-attacher is expected to provide a well-formed "namespace/name" node ID per the CSI contract; if it does not, surface the contract violation rather than mask it. - getErr -> codes.Unavailable: this is the same code returned for the not-Ready and not-present cases just below, so the retry behavior on the attacher side stays consistent. By the time we reach this branch the upstream Publish call already succeeded, which means it just looked the VMI up; a transient get error here is a race or API hiccup and a retry is safe. Also add a one-line comment on GetVirtualMachine: in the upstream kubevirt.io/csi-driver client this method returns a VirtualMachineInstance despite its name, so accessing vmi.Status.VolumeStatus on the result is type-correct. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
88a770f to
cbe4b86
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — wrapper now invariably verifies VMI.Status.VolumeStatus.Phase == VolumeReady after upstream's Publish returns success, so the EnsureVolumeAvailableVM fast-path false-positive no longer propagates to external-attacher. The two follow-up commits address bot feedback cleanly: gRPC error codes (Internal for malformed node ID, Unavailable for VMI fetch/not-Ready/not-present) restore the wrapper's invariant that VolumeAttachment.Attached=true requires observed VolumeReady.
Business context: Upstream's EnsureVolumeAvailableVM declares a volume attached based only on its presence in VM.spec.template.spec.volumes, so a Publish retry after a 2-minute readiness timeout reports success without QEMU actually receiving device_add; the tenant kubelet then fails NodeStageVolume with couldn't find device by serial id instead of seeing a retriable CSI attach error.
|
Successfully created backport PR for |
What this PR does
Stops
ControllerPublishVolumefrom silently reporting success when the volume is in VM spec but the hotplug attach never actually completed.Root cause
Upstream's fast path (
EnsureVolumeAvailableVM) checks onlyvm.Spec.Template.Spec.Volumes. If a prior Publish wrote that entry but timed out waiting for VMI Ready (e.g. the infra PVC is stuckClaimPendingbecause LINSTOR cannot autoplace), the next Publish short-circuits to success. external-attacher setsAttached=true, but QEMU was never sentdevice_add. The tenant kubelet's NodeStageVolume then fails with:Observed in production. Sequence from kubevirt-csi-driver controller logs:
VMI volumeStatus during that window:
phase=Pending message="PVC is in phase ClaimPending". VolumeAttachment then goesAttached=true; tenant kubelet surfaces "couldn't find device by serial id".Fix
After upstream Publish returns success for a hotplug volume, the wrapper re-reads VMI status and checks
Phase == VolumeReadyfor this DataVolume. If it is not Ready (or not present), the RPC returnscodes.Unavailablewith the upstream reason, so external-attacher keeps retrying instead of declaring the volume attached. The underlying provisioning failure surfaces at the CSI layer instead of becoming a confusing kubelet-side "device not found by serial id".This is a wrapper-side workaround for a defect in upstream
kubevirt.io/csi-driver; the structural fix belongs upstream (tighten the fast path to require VMI Ready) and will be filed separately. The wrapper guard is safe to keep as defense-in-depth even after the upstream fix lands.Relates to #2634.
Release note
Summary by CodeRabbit