OCPBUGS-101813: only block vSphere machine deletion for VMDK-backed v… - #1528
OCPBUGS-101813: only block vSphere machine deletion for VMDK-backed v…#1528jcpowermac wants to merge 1 commit into
Conversation
…olumes The nodeHasVolumesAttached() check was volume-type-agnostic, blocking machine deletion for any attached volume. This caused indefinite blocking when non-VMDK volumes (NFS, iSCSI, etc.) were attached, since DaemonSet pods with these volumes remain Running and never get evicted. The VMDK data loss risk only applies to vSphere-backed volumes. This change filters by VolumeAttachment.Spec.Attacher to only block for: - csi.vsphere.vmware.com (vSphere CSI) - kubernetes.io/vsphere-volume (vSphere in-tree) Non-vSphere volumes no longer block deletion. Conservative behavior: if VolumeAttachment lookup fails, treat as vSphere-backed and block. Includes unit tests for volume type filtering and e2e test for NFS volume deletion scenario.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/valid-reference
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-101813, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughChangesThe vSphere reconciler now checks each attached volume’s vSphere volume-aware machine deletion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant KubernetesAPI
participant vCenter
Reconciler->>KubernetesAPI: Look up VolumeAttachment objects
KubernetesAPI-->>Reconciler: Return attachers or lookup errors
Reconciler->>Reconciler: Classify attached volumes
Reconciler->>vCenter: Delete VM if no vSphere volume remains
Reconciler-->>Reconciler: Requeue if a vSphere volume remains
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/test ? |
|
/test e2e-vsphere-ovn-techpreview-serial |
|
/test unit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@pkg/controller/vsphere/reconciler_test.go`:
- Around line 2598-2771: Extend the volumeTypeFilteringTestCases table to cover
nodeHasVolumesAttached conservative fallbacks: add a case with a node-attached
volume but no matching VolumeAttachment object, and a case with a matching
attachment whose Attacher is empty or unrecognized. Configure both cases to
expect deletion blocking with the existing attached-volumes requeue error, and
retain the established machine/node setup and reconcile assertions.
In `@pkg/controller/vsphere/reconciler.go`:
- Around line 581-587: Update the VolumeAttachment attacher classification
around the switch in the reconciler so only explicitly recognized safe
non-vSphere attachers remain in nonVSphereVolumes; route unknown or empty
Attacher values into unknownVolumes, alongside failed lookups. Ensure the
existing unknownVolumes handling and logging are exercised, preserving vSphere
attachers in vsphereVolumes.
- Around line 567-587: The VolumeAttachment lookup in the node volume
classification loop must use the production metadata name for CSI attachments
rather than treating AttachedVolume.Name as the object name. Update the logic
around apiReader.Get and the surrounding reconciliation method to derive the
csi-<sha256(volumeHandle+driver+nodeName)> name, or list and correlate
VolumeAttachment objects by node and attached volume, so non-vSphere CSI volumes
are classified by their real attacher. Update the related tests to use
production-style hashed VolumeAttachment names.
In `@test/e2e/vsphere/machines.go`:
- Around line 282-354: The test must attach an NFS volume to the created
machine’s node before deletion. Extend the flow around testNSName,
nfsStorageClassName, and the machine readiness check to create a PVC and
mounting Pod or DaemonSet using that storage class, ensure it schedules onto
machine.Name’s node, and wait for the corresponding VolumeAttachment or
node.Status.VolumesAttached entry before deleting the machine. Keep the existing
deletion and cluster-size assertions unchanged.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c530423b-96ea-445c-9067-22760792a36c
📒 Files selected for processing (4)
ocpbugs-101813.mdpkg/controller/vsphere/reconciler.gopkg/controller/vsphere/reconciler_test.gotest/e2e/vsphere/machines.go
| volumeTypeFilteringTestCases := []struct { | ||
| name string | ||
| machine func(t *testing.T, simServerHost string) *machinev1.Machine | ||
| node func(t *testing.T) *corev1.Node | ||
| volumeAttachments []runtimeclient.Object | ||
| attachDisks bool | ||
| secondReconcileError string | ||
| }{ | ||
| { | ||
| name: "NFS volumes attached, deletion proceeds", | ||
| machine: func(t *testing.T, simServerHost string) *machinev1.Machine { | ||
| return getMachineWithStatus(t, machinev1.MachineStatus{ | ||
| NodeRef: &corev1.ObjectReference{ | ||
| Name: nodeName, | ||
| }, | ||
| }, simServerHost) | ||
| }, | ||
| node: func(t *testing.T) *corev1.Node { | ||
| node := getNodeWithConditions([]corev1.NodeCondition{ | ||
| { | ||
| Type: corev1.NodeReady, | ||
| Status: corev1.ConditionUnknown, | ||
| }, | ||
| }) | ||
| node.Status.VolumesAttached = []corev1.AttachedVolume{ | ||
| { | ||
| Name: "pvc-nfs-123", | ||
| DevicePath: "/dev/sda", | ||
| }, | ||
| } | ||
| return node | ||
| }, | ||
| volumeAttachments: []runtimeclient.Object{ | ||
| &storagev1.VolumeAttachment{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "pvc-nfs-123", | ||
| }, | ||
| Spec: storagev1.VolumeAttachmentSpec{ | ||
| Attacher: "nfs.csi.k8s.io", | ||
| NodeName: nodeName, | ||
| }, | ||
| }, | ||
| }, | ||
| attachDisks: false, | ||
| secondReconcileError: "destroying vm in progress, requeuing", | ||
| }, | ||
| { | ||
| name: "vSphere CSI volumes attached, deletion blocked", | ||
| machine: func(t *testing.T, simServerHost string) *machinev1.Machine { | ||
| return getMachineWithStatus(t, machinev1.MachineStatus{ | ||
| NodeRef: &corev1.ObjectReference{ | ||
| Name: nodeName, | ||
| }, | ||
| }, simServerHost) | ||
| }, | ||
| node: func(t *testing.T) *corev1.Node { | ||
| node := getNodeWithConditions([]corev1.NodeCondition{ | ||
| { | ||
| Type: corev1.NodeReady, | ||
| Status: corev1.ConditionUnknown, | ||
| }, | ||
| }) | ||
| node.Status.VolumesAttached = []corev1.AttachedVolume{ | ||
| { | ||
| Name: "pvc-vsphere-456", | ||
| DevicePath: "/dev/sdb", | ||
| }, | ||
| } | ||
| return node | ||
| }, | ||
| volumeAttachments: []runtimeclient.Object{ | ||
| &storagev1.VolumeAttachment{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "pvc-vsphere-456", | ||
| }, | ||
| Spec: storagev1.VolumeAttachmentSpec{ | ||
| Attacher: VSphereCSIDriverName, | ||
| NodeName: nodeName, | ||
| }, | ||
| }, | ||
| }, | ||
| attachDisks: true, | ||
| secondReconcileError: "node somenodename has attached volumes, requeuing", | ||
| }, | ||
| { | ||
| name: "Mixed volumes (NFS + vSphere), deletion blocked", | ||
| machine: func(t *testing.T, simServerHost string) *machinev1.Machine { | ||
| return getMachineWithStatus(t, machinev1.MachineStatus{ | ||
| NodeRef: &corev1.ObjectReference{ | ||
| Name: nodeName, | ||
| }, | ||
| }, simServerHost) | ||
| }, | ||
| node: func(t *testing.T) *corev1.Node { | ||
| node := getNodeWithConditions([]corev1.NodeCondition{ | ||
| { | ||
| Type: corev1.NodeReady, | ||
| Status: corev1.ConditionUnknown, | ||
| }, | ||
| }) | ||
| node.Status.VolumesAttached = []corev1.AttachedVolume{ | ||
| { | ||
| Name: "pvc-nfs-123", | ||
| DevicePath: "/dev/sda", | ||
| }, | ||
| { | ||
| Name: "pvc-vsphere-456", | ||
| DevicePath: "/dev/sdb", | ||
| }, | ||
| } | ||
| return node | ||
| }, | ||
| volumeAttachments: []runtimeclient.Object{ | ||
| &storagev1.VolumeAttachment{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "pvc-nfs-123", | ||
| }, | ||
| Spec: storagev1.VolumeAttachmentSpec{ | ||
| Attacher: "nfs.csi.k8s.io", | ||
| NodeName: nodeName, | ||
| }, | ||
| }, | ||
| &storagev1.VolumeAttachment{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "pvc-vsphere-456", | ||
| }, | ||
| Spec: storagev1.VolumeAttachmentSpec{ | ||
| Attacher: VSphereCSIDriverName, | ||
| NodeName: nodeName, | ||
| }, | ||
| }, | ||
| }, | ||
| attachDisks: true, | ||
| secondReconcileError: "node somenodename has attached volumes, requeuing", | ||
| }, | ||
| { | ||
| name: "Non-vSphere attacher, deletion proceeds", | ||
| machine: func(t *testing.T, simServerHost string) *machinev1.Machine { | ||
| return getMachineWithStatus(t, machinev1.MachineStatus{ | ||
| NodeRef: &corev1.ObjectReference{ | ||
| Name: nodeName, | ||
| }, | ||
| }, simServerHost) | ||
| }, | ||
| node: func(t *testing.T) *corev1.Node { | ||
| node := getNodeWithConditions([]corev1.NodeCondition{ | ||
| { | ||
| Type: corev1.NodeReady, | ||
| Status: corev1.ConditionUnknown, | ||
| }, | ||
| }) | ||
| node.Status.VolumesAttached = []corev1.AttachedVolume{ | ||
| { | ||
| Name: "pvc-iscsi-789", | ||
| DevicePath: "/dev/sdc", | ||
| }, | ||
| } | ||
| return node | ||
| }, | ||
| volumeAttachments: []runtimeclient.Object{ | ||
| &storagev1.VolumeAttachment{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "pvc-iscsi-789", | ||
| }, | ||
| Spec: storagev1.VolumeAttachmentSpec{ | ||
| Attacher: "iscsi.csi.k8s.io", | ||
| NodeName: nodeName, | ||
| }, | ||
| }, | ||
| }, | ||
| attachDisks: false, | ||
| secondReconcileError: "destroying vm in progress, requeuing", | ||
| }, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add coverage for the "VolumeAttachment lookup fails" and "unknown attacher" scenarios.
The four new cases cover NFS-only, vSphere CSI-only, mixed, and a named non-vSphere attacher (iSCSI). None of them cover the "VolumeAttachment not found" or "attacher value is empty/unrecognized" paths that nodeHasVolumesAttached explicitly handles as conservative-block cases. Since this conservative fallback is the safety net for the whole feature, add a case where node.Status.VolumesAttached references a volume with no matching VolumeAttachment object, and assert that deletion is still blocked.
🤖 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/controller/vsphere/reconciler_test.go` around lines 2598 - 2771, Extend
the volumeTypeFilteringTestCases table to cover nodeHasVolumesAttached
conservative fallbacks: add a case with a node-attached volume but no matching
VolumeAttachment object, and a case with a matching attachment whose Attacher is
empty or unrecognized. Configure both cases to expect deletion blocking with the
existing attached-volumes requeue error, and retain the established machine/node
setup and reconcile assertions.
| for _, vol := range node.Status.VolumesAttached { | ||
| volName := string(vol.Name) | ||
| va := &storagev1.VolumeAttachment{} | ||
| if err := r.apiReader.Get(ctx, apimachinerytypes.NamespacedName{Name: volName}, va); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| klog.Warningf("Machine %s: VolumeAttachment %s not found, conservatively treating as vSphere-backed", machineName, volName) | ||
| vsphereVolumes = append(vsphereVolumes, volName) | ||
| continue | ||
| } | ||
| klog.Warningf("Machine %s: failed to get VolumeAttachment %s: %v, conservatively treating as vSphere-backed", machineName, volName, err) | ||
| vsphereVolumes = append(vsphereVolumes, volName) | ||
| continue | ||
| } | ||
|
|
||
| switch va.Spec.Attacher { | ||
| case VSphereCSIDriverName, VSphereInTreePluginName: | ||
| vsphereVolumes = append(vsphereVolumes, volName) | ||
| default: | ||
| nonVSphereVolumes = append(nonVSphereVolumes, fmt.Sprintf("%s (attacher: %s)", volName, va.Spec.Attacher)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does the VolumeAttachment object name match AttachedVolume.Name in node.status.volumesAttached, or is it a separate sha256-based name?
💡 Result:
The VolumeAttachment object name does not match the name field in node.status.volumesAttached [1]. They serve different purposes and follow different naming conventions [1][2]. The node.status.volumesAttached field contains a list of volumes currently attached to the node, where the Name field is a unique identifier (often referred to as a UniqueVolumeName) [3][2]. For CSI drivers, this name is typically a string concatenation of the driver name and the volume handle (e.g., driver-name/volume-handle) [4][2]. This field is intended to provide a simplified view of the volumes attached to a node for state tracking [2][5]. In contrast, a VolumeAttachment object is a dedicated API resource used to manage the attachment lifecycle for CSI volumes [6][1]. To ensure these objects are unique and deterministic, Kubernetes generates their names using a SHA-256 hash [1]. Specifically, the name is typically constructed as csi-<sha256(volumeName, csiDriverName, nodeName)> [1]. Because of this difference in naming, you cannot directly map a node.status.volumesAttached entry to a specific VolumeAttachment object by name [1]. If you need to find the VolumeAttachment for a specific volume on a node, you must either list all VolumeAttachment objects and filter them by their spec (matching on nodeName and source) or reconstruct the expected name using the same SHA-256 hashing logic [1].
Citations:
- 1: https://sklar.rocks/k8s-volumeattachment-names/
- 2: Attach/detach controller does not track "attaching" volumes persistently kubernetes/kubernetes#44858
- 3: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/volume/attachdetach/attach_detach_controller.go
- 4: The status of asw in attachDetachController is inconsistent with the actual node status kubernetes/kubernetes#122413
- 5: Check if volume is attached before deleting it kubernetes/kubernetes#84226
- 6: https://kubernetes.io/docs/reference/kubernetes-api/storage/volume-attachment-v1/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target implementation ---'
sed -n '520,610p' pkg/controller/vsphere/reconciler.go
printf '%s\n' '--- relevant test fixture ---'
sed -n '2580,2670p' pkg/controller/vsphere/reconciler_test.go
printf '%s\n' '--- VolumeAttachment and attached-volume references ---'
rg -n -C 3 'VolumesAttached|VolumeAttachment|AttachedVolume|VolumeAttachmentName' pkg/controller/vsphere
printf '%s\n' '--- module and Kubernetes dependency versions ---'
rg -n 'k8s.io/(api|apimachinery|client-go)|controller-runtime' go.modRepository: openshift/machine-api-operator
Length of output: 19553
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import hashlib
# Deterministic probe of the naming distinction described in the review.
driver = "pd.csi.storage.gke.io"
handle = "projects/p/zones/z/disks/example"
node = "worker-1"
attached_name = f"kubernetes.io/csi/{driver}^{handle}"
candidate_inputs = [
attached_name,
f"{handle}{driver}{node}",
f"{driver}{handle}{node}",
]
print("AttachedVolume.Name:", attached_name)
for value in candidate_inputs:
print("sha256 candidate:", value, "=>", "csi-" + hashlib.sha256(value.encode()).hexdigest())
print("names_equal:", attached_name == "csi-" + hashlib.sha256((handle + driver + node).encode()).hexdigest())
PYRepository: openshift/machine-api-operator
Length of output: 748
🌐 Web query:
Kubernetes VolumeAttachment metadata.name generation sha256 volumeName attacher nodeName and VolumeAttachmentSpec fields source persistentVolumeName nodeName
💡 Result:
In Kubernetes, the name of a VolumeAttachment object is programmatically generated by the attach-detach controller or the CSI driver to ensure uniqueness and deterministic mapping between a volume and a node [1][2]. The generation logic follows a specific pattern using the SHA256 hash of three key components [1][3][2]: 1. The volume handle (the unique identifier for the volume, as defined in the CSI specification) [1][3]. 2. The CSI driver name (the plugin performing the attachment) [1][3]. 3. The target node name [1][3]. The name is constructed by concatenating these three strings, calculating their SHA256 sum, and prefixing the result with "csi-" [1][3][2]. In Go, this is implemented as follows [2]: func getAttachmentName(volName, csiDriverName, nodeName string) string { result:= sha256.Sum256([]byte(fmt.Sprintf("%s%s%s", volName, csiDriverName, nodeName))) return fmt.Sprintf("csi-%x", result) } Regarding the VolumeAttachmentSpec fields: - attacher: This field specifies the name of the volume driver that must handle the attachment request [4][5]. - nodeName: This field identifies the specific node to which the volume should be attached [4][5]. - source: This field defines the volume source to be attached [4][5]. For standard persistent volumes, this includes the persistentVolumeName, which identifies the specific PersistentVolume object being attached [2][6][5]. These fields are populated by the Kubernetes system as part of the attach-detach process [7][4]. VolumeAttachment objects are non-namespaced resources [4].
Citations:
- 1: https://sklar.rocks/k8s-volumeattachment-names/
- 2: https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/csi/csi_attacher.go
- 3: https://dev.to/sklarsa/how-are-kubernetes-volumeattachments-named-5o8
- 4: https://github.com/kubernetes/kubernetes/blob/63b36867/staging/src/k8s.io/api/storage/v1/generated.proto
- 5: https://docs.okd.io/latest/rest_api/storage_apis/volumeattachment-storage-k8s-io-v1.html
- 6: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/volume/attachdetach/attach_detach_controller.go
- 7: https://kubernetes.io/docs/reference/kubernetes-api/storage/volume-attachment-v1/
🌐 Web query:
Kubernetes node.status.volumesAttached AttachedVolume.Name CSI format VolumeAttachment correlation
💡 Result:
In Kubernetes, the node.status.volumesAttached field contains a list of AttachedVolume objects [1]. For CSI volumes, the AttachedVolume.Name uses a specific format that identifies the driver and the volume instance, typically formatted as kubernetes.io/csi/<driver-name>^<volume-handle> [2]. Correlation with VolumeAttachment objects is achieved through a deterministic naming convention [3]. When the Kubernetes attach-detach controller manages a CSI volume, it generates a VolumeAttachment object with a unique name derived from a SHA-256 hash of the volume handle, the CSI driver name, and the target node name [3][4]. Key details of this relationship: 1. AttachedVolume.Name format: The name field in node.status.volumesAttached for CSI volumes follows the pattern kubernetes.io/csi/<driver-name>^<volume-handle>. This uniquely identifies the volume managed by a specific CSI driver on that node [2]. 2. VolumeAttachment Correlation: The VolumeAttachment object name is calculated using the following logic [3]: csi-<hex-encoded-sha256(volumeHandle + csiDriverName + nodeName)> The Kubernetes controller uses this deterministic name to locate the corresponding VolumeAttachment resource in the API server [3][4]. By checking the existence and the status.attached field of the VolumeAttachment object with this derived name, the system verifies whether the volume is successfully attached to the node [3][4]. 3. Controller Logic: The attach-detach-controller uses these internal identifiers to synchronize the state between the node's reported status and the actual state of the cluster [5][6]. If a VolumeAttachment object for a specific volume is not found or indicates an attachment error, the controller updates the node's status or triggers corrective actions [2][4]. This architecture separates the node-level status (which tracks what is currently attached for kubelet usage) from the cluster-level VolumeAttachment resource (which coordinates the asynchronous attachment process with external CSI drivers) [7][8].
Citations:
- 1: https://kubernetes.io/docs/reference/kubernetes-api/core/node-v1/
- 2: The status of asw in attachDetachController is inconsistent with the actual node status kubernetes/kubernetes#122413
- 3: https://dev.to/sklarsa/how-are-kubernetes-volumeattachments-named-5o8
- 4: https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/csi/csi_attacher.go
- 5: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/volume/attachdetach/attach_detach_controller.go
- 6: https://github.com/kubernetes/kubernetes/blob/63b36867/pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go
- 7: https://kubernetes.io/docs/reference/kubernetes-api/storage/volume-attachment-v1/
- 8: https://kubernetes.io/docs/reference/kubernetes-api/storage/csi-driver-v1/
Use the real VolumeAttachment name for CSI volumes.
AttachedVolume.Name uses kubernetes.io/csi/<driver>^<volumeHandle>, but VolumeAttachment.metadata.name uses csi-<sha256(volumeHandle+driver+nodeName)>. The current lookup misses real objects and classifies non-vSphere CSI volumes, including NFS volumes, as vSphere-backed. Derive the hashed name or list VolumeAttachment objects for the node and correlate them with attached volumes. Update the tests to use production-style names.
🤖 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/controller/vsphere/reconciler.go` around lines 567 - 587, The
VolumeAttachment lookup in the node volume classification loop must use the
production metadata name for CSI attachments rather than treating
AttachedVolume.Name as the object name. Update the logic around apiReader.Get
and the surrounding reconciliation method to derive the
csi-<sha256(volumeHandle+driver+nodeName)> name, or list and correlate
VolumeAttachment objects by node and attached volume, so non-vSphere CSI volumes
are classified by their real attacher. Update the related tests to use
production-style hashed VolumeAttachment names.
| switch va.Spec.Attacher { | ||
| case VSphereCSIDriverName, VSphereInTreePluginName: | ||
| vsphereVolumes = append(vsphereVolumes, volName) | ||
| default: | ||
| nonVSphereVolumes = append(nonVSphereVolumes, fmt.Sprintf("%s (attacher: %s)", volName, va.Spec.Attacher)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Switch statement never treats a truly unknown attacher as blocking, contradicting the stated design.
The switch has only two branches:
switch va.Spec.Attacher {
case VSphereCSIDriverName, VSphereInTreePluginName:
vsphereVolumes = append(vsphereVolumes, volName)
default:
nonVSphereVolumes = append(nonVSphereVolumes, fmt.Sprintf("%s (attacher: %s)", volName, va.Spec.Attacher))
}Any attacher that is not vSphere CSI or in-tree, including a genuinely unrecognized or empty Attacher value, falls into default and is treated as safe to ignore. unknownVolumes is declared and logged at Line 595-597, but nothing ever appends to it, so that branch is dead code.
ocpbugs-101813.md (Line 49) states the intended behavior: "Conservative error handling: if VolumeAttachment lookup fails or attacher is unknown, treat as potentially risky and block." The implementation does not do this for an unknown attacher, only for a failed lookup. Add an explicit case (or an allow-list of known-safe non-vSphere attachers) so any attacher not recognized as safe is treated the same as vsphereVolumes.
🛡️ Proposed fix to treat unrecognized attachers conservatively
+ knownSafeAttachers := map[string]bool{
+ "nfs.csi.k8s.io": true,
+ "csi.nfs.io": true,
+ "iscsi.csi.k8s.io": true,
+ }
switch va.Spec.Attacher {
case VSphereCSIDriverName, VSphereInTreePluginName:
vsphereVolumes = append(vsphereVolumes, volName)
- default:
+ case "":
+ unknownVolumes = append(unknownVolumes, volName)
+ vsphereVolumes = append(vsphereVolumes, volName)
+ default:
+ if !knownSafeAttachers[va.Spec.Attacher] {
+ unknownVolumes = append(unknownVolumes, fmt.Sprintf("%s (attacher: %s)", volName, va.Spec.Attacher))
+ vsphereVolumes = append(vsphereVolumes, volName)
+ break
+ }
nonVSphereVolumes = append(nonVSphereVolumes, fmt.Sprintf("%s (attacher: %s)", volName, va.Spec.Attacher))
}Also applies to: 595-597
🤖 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/controller/vsphere/reconciler.go` around lines 581 - 587, Update the
VolumeAttachment attacher classification around the switch in the reconciler so
only explicitly recognized safe non-vSphere attachers remain in
nonVSphereVolumes; route unknown or empty Attacher values into unknownVolumes,
alongside failed lookups. Ensure the existing unknownVolumes handling and
logging are exercised, preserving vSphere attachers in vsphereVolumes.
| It("complete deletion with NFS volumes attached [apigroup:machine.openshift.io][Serial][Suite:openshift/conformance/serial]", func() { | ||
| machineName := "machine-nfs-volume-test" | ||
|
|
||
| By("checking for the openshift machine api operator") | ||
| util.SkipUnlessMachineAPIOperator(dc, c.CoreV1().Namespaces()) | ||
|
|
||
| By("checking for NFS CSI driver") | ||
| storageClasses, err := c.StorageV1().StorageClasses().List(ctx, metav1.ListOptions{}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| var nfsStorageClassName string | ||
| for _, sc := range storageClasses.Items { | ||
| if sc.Provisioner == "nfs.csi.k8s.io" || sc.Provisioner == "csi.nfs.io" || sc.Provisioner == "openshift-storage.noobaa.io" { | ||
| nfsStorageClassName = sc.Name | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if nfsStorageClassName == "" { | ||
| Skip("No NFS CSI storage class found, skipping test") | ||
| } | ||
|
|
||
| By(fmt.Sprintf("using NFS storage class: %s", nfsStorageClassName)) | ||
|
|
||
| By("checking initial cluster size") | ||
| nodeList, err := c.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| initialNumberOfNodes := len(nodeList.Items) | ||
|
|
||
| By("creating test namespace") | ||
| testNSName := "nfs-volume-test-" + machineName | ||
| _, err = c.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNSName}}, metav1.CreateOptions{}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| defer c.CoreV1().Namespaces().Delete(ctx, testNSName, metav1.DeleteOptions{}) | ||
|
|
||
| By("creating machine") | ||
| provider := getProviderFromMachineSet(cfg) | ||
| provider.DataDisks = []v1beta1.VSphereDisk{} | ||
| provRawData, err := vsphere.RawExtensionFromProviderSpec(provider) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| machine, err := util.CreateMachine(ctx, cfg, mc, machineName, machineRole, provRawData) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("waiting for machine to become ready") | ||
| Eventually(func() (string, error) { | ||
| ms, err := mc.Machines(util.MachineAPINamespace).Get(ctx, machine.Name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if ms.Status.Phase == nil { | ||
| return "", nil | ||
| } | ||
| return *(ms.Status.Phase), nil | ||
| }, machineReadyTimeout).Should(BeEquivalentTo("Running")) | ||
|
|
||
| By("deleting the machine") | ||
| err = mc.Machines(util.MachineAPINamespace).Delete(ctx, machine.Name, metav1.DeleteOptions{}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("verifying machine deletion completes (non-VMDK volumes should not block)") | ||
| Eventually(func() bool { | ||
| _, err := mc.Machines(util.MachineAPINamespace).Get(ctx, machine.Name, metav1.GetOptions{}) | ||
| return err != nil | ||
| }, 10*time.Minute, 5*time.Second).Should(BeTrue(), "machine should be deleted even with NFS volumes attached") | ||
|
|
||
| By(fmt.Sprintf("waiting for cluster to get back to original size: %d nodes", initialNumberOfNodes)) | ||
| Eventually(func() bool { | ||
| nodeList, err := c.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| return len(nodeList.Items) == initialNumberOfNodes | ||
| }, 10*time.Minute, 5*time.Second).Should(BeTrue()) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Test does not actually attach an NFS volume to the machine's node.
The test discovers an NFS-compatible storage class and creates a namespace (testNSName), but testNSName is never used afterward: no PersistentVolumeClaim, Pod, or DaemonSet is created in it, and nfsStorageClassName is never referenced after the Skip check. As a result, the created machine's node never gets an entry in node.Status.VolumesAttached, so nodeHasVolumesAttached returns false immediately at the len(node.Status.VolumesAttached) == 0 check in pkg/controller/vsphere/reconciler.go, and the new volume-filtering code path added by this PR is never exercised.
As written, this test is equivalent to the existing "create machines with data disks" test without data disks: it only verifies plain machine deletion, not that non-vSphere volumes fail to block deletion. Create a PVC using nfsStorageClassName and a pod (or DaemonSet) that mounts it and gets scheduled onto the machine's node, and wait for the corresponding VolumeAttachment to exist, before deleting the machine.
🤖 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 `@test/e2e/vsphere/machines.go` around lines 282 - 354, The test must attach an
NFS volume to the created machine’s node before deletion. Extend the flow around
testNSName, nfsStorageClassName, and the machine readiness check to create a PVC
and mounting Pod or DaemonSet using that storage class, ensure it schedules onto
machine.Name’s node, and wait for the corresponding VolumeAttachment or
node.Status.VolumesAttached entry before deleting the machine. Keep the existing
deletion and cluster-size assertions unchanged.
|
@jcpowermac: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
…olumes
The nodeHasVolumesAttached() check was volume-type-agnostic, blocking machine deletion for any attached volume. This caused indefinite blocking when non-VMDK volumes (NFS, iSCSI, etc.) were attached, since DaemonSet pods with these volumes remain Running and never get evicted.
The VMDK data loss risk only applies to vSphere-backed volumes. This change filters by VolumeAttachment.Spec.Attacher to only block for:
Non-vSphere volumes no longer block deletion. Conservative behavior: if VolumeAttachment lookup fails, treat as vSphere-backed and block.
Includes unit tests for volume type filtering and e2e test for NFS volume deletion scenario.
Summary by CodeRabbit
Bug Fixes
Tests