Skip to content

OCPBUGS-101813: only block vSphere machine deletion for VMDK-backed v… #1528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
jcpowermac wants to merge 1 commit into
base: main
Choose a base branch
from
+601 −6
Draft

OCPBUGS-101813: only block vSphere machine deletion for VMDK-backed v… #1528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions ocpbugs-101813.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@


# OCPBUGS-101813 — vSphere machine controller nodeHasVolumesAttached() blocks deletion indefinitely with no timeout for non-VMDK volumes
| Field | Value |
|---|---|
| **Project** | OpenShift Bugs (OCPBUGS) |
| **Issue Type** | Bug |
| **Status** | New |
| **Priority** | Normal |
| **Reporter** | ship-help-jira |
| **Assignee** | — |
| **Components** | Cloud Compute / Machine API Providers |
| **Affects Versions** | 4.17 |
| **Security Level** | Red Hat Employee |
| **Created** | 2026-08-03 |

---
## Summary
The vSphere machine actuator's `delete()` method in `pkg/controller/vsphere/reconciler.go` calls `nodeHasVolumesAttached()` which checks `node.Status.VolumesAttached`. If any volumes are reported as attached, the reconciler requeues indefinitely with no timeout or escape hatch. This creates an unrecoverable state when non-VMDK volumes (e.g. NFS via NetApp/Trident CSI) are attached, because the VMDK data loss risk that motivates the check does not apply to these volume types, yet the check is volume-type-agnostic.
## Details
The `nodeHasVolumesAttached()` function (lines 537–553 of `reconciler.go`) simply returns `len(node.Status.VolumesAttached) != 0`. When true, the reconciler attempts `deleteUnevictedPods()`, which only deletes pods already in `Terminating` state on unreachable nodes. DaemonSet pods are excluded from standard node drain and are never evicted, so they remain in `Running` state — invisible to `deleteUnevictedPods()`. If those DaemonSet pods have NFS volume mounts, the `VolumeAttachment` objects persist, `VolumesAttached` remains non-empty, and the machine stays in `Deleting` indefinitely.
The built-in recovery (`deleteUnevictedPods`) logs `Deleted 0 pods` on every reconcile cycle because the DaemonSet pods are not in `Terminating` state.
## Expected Behavior
The machine controller should distinguish between VMDK-backed volumes (where data loss is a real risk) and non-VMDK volumes (NFS, iSCSI, etc.) where the vSphere `Destroy_Task` data loss concern does not apply.
## Actual Behavior
Machine remains stuck in `Deleting` state indefinitely. Manual intervention is required (applying the `node.kubernetes.io/out-of-service` taint, or manually deleting `VolumeAttachment` objects) to unblock.

---

## Fix Plan

**Approach:** Modify `nodeHasVolumesAttached()` to only block deletion for vSphere-backed volumes.

### Implementation

**File: `pkg/controller/vsphere/reconciler.go`**

1. Add constants for vSphere attacher names:
- `VSphereCSIDriverName = "csi.vsphere.vmware.com"`
- `VSphereInTreePluginName = "kubernetes.io/vsphere-volume"`

2. Rewrite `nodeHasVolumesAttached()` to:
- Iterate over `node.Status.VolumesAttached`
- For each volume, fetch the corresponding `VolumeAttachment` by name (`AttachedVolume.Name` = VolumeAttachment name)
- Check `VolumeAttachment.Spec.Attacher`:
- If vSphere CSI or in-tree → volume is VMDK-backed → block deletion
- If non-vSphere (NFS, iSCSI, etc.) → skip, no data loss risk
- Return `true` only if vSphere-backed volumes are found
- Conservative error handling: if VolumeAttachment lookup fails or attacher is unknown, treat as potentially risky and block
- Log all volumes being checked and which ones are blocking deletion

### Unit Tests

**File: `pkg/controller/vsphere/reconciler_test.go`**

Add test cases:
- NFS volumes attached → deletion proceeds (no block)
- vSphere CSI volumes attached → deletion blocked
- Mixed volumes (NFS + vSphere) → deletion blocked
- Non-vSphere attacher (iSCSI, etc.) → deletion proceeds (no block)

### E2E Tests

**File: `test/e2e/vsphere/machines.go`** (or appropriate existing e2e test file)

Add e2e test scenario (requires NFS CSI driver like NetApp/Trident):
- Create a Machine with an NFS-backed PVC mounted (via DaemonSet or similar)
- Trigger machine deletion
- Verify machine completes deletion without getting stuck, even with NFS `VolumeAttachment` objects still present
- Confirm VM is destroyed in vSphere

Note: This test requires an NFS CSI driver installed in the cluster. If unavailable, can be validated manually or via integration tests.

59 changes: 53 additions & 6 deletions pkg/controller/vsphere/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/vmware/govmomi/vim25/types"

corev1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
apimachinerytypes "k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -55,6 +56,10 @@ const (
// Not all controllers support up to 30, but the maximum is 30.
// xref: https://docs.vmware.com/en/VMware-vSphere/8.0/vsphere-vm-administration/GUID-5872D173-A076-42FE-8D0B-9DB0EB0E7362.html#:~:text=If%20you%20add%20a%20hard,values%20from%200%20to%2014.
maxUnitNumber = 30
// VSphereCSIDriverName is the CSI driver name for vSphere volumes.
VSphereCSIDriverName = "csi.vsphere.vmware.com"
// VSphereInTreePluginName is the in-tree plugin name for vSphere volumes.
VSphereInTreePluginName = "kubernetes.io/vsphere-volume"
)

// These are the guestinfo variables used by Ignition.
Expand Down Expand Up @@ -534,11 +539,11 @@ func (r *Reconciler) delete() error {
return fmt.Errorf("destroying vm in progress, requeuing")
}

// nodeHasVolumesAttached returns true if node status still have volumes attached
// pod deletion and volume detach happen asynchronously, so pod could be deleted before volume detached from the node
// this could cause issue for some storage provisioner, for example, vsphere-volume this is problematic
// because if the node is deleted before detach success, then the underline VMDK will be deleted together with the Machine
// so after node draining we need to check if all volumes are detached before deleting the node.
// nodeHasVolumesAttached returns true if node status still has vSphere-backed volumes attached.
// Pod deletion and volume detach happen asynchronously, so pod could be deleted before volume detached from the node.
// This is problematic for vSphere volumes because if the node is deleted before detach succeeds,
// the underlying VMDK will be deleted together with the Machine.
// Non-vSphere volumes (NFS, iSCSI, etc.) do not have this risk since vSphere Destroy_Task does not affect them.
func (r *Reconciler) nodeHasVolumesAttached(ctx context.Context, nodeName string, machineName string) (bool, error) {
node := &corev1.Node{}
if err := r.apiReader.Get(ctx, apimachinerytypes.NamespacedName{Name: nodeName}, node); err != nil {
Expand All @@ -549,7 +554,49 @@ func (r *Reconciler) nodeHasVolumesAttached(ctx context.Context, nodeName string
return true, err
}

return len(node.Status.VolumesAttached) != 0, nil
if len(node.Status.VolumesAttached) == 0 {
return false, nil
}

klog.V(3).Infof("Machine %s: checking %d attached volumes on node %s for vSphere-backed volumes", machineName, len(node.Status.VolumesAttached), nodeName)

var vsphereVolumes []string
var nonVSphereVolumes []string
var unknownVolumes []string

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))
}
}
Comment on lines +567 to +587

@coderabbitai coderabbitai Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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:

🏁 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.mod

Repository: 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())
PY

Repository: 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:

🌐 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:

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.

Comment on lines +581 to +587

@coderabbitai coderabbitai Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.


if len(vsphereVolumes) > 0 {
klog.Warningf("Machine %s: vSphere-backed volumes still attached on node %s: %v", machineName, nodeName, vsphereVolumes)
}
if len(nonVSphereVolumes) > 0 {
klog.V(3).Infof("Machine %s: non-vSphere volumes attached (safe to ignore): %v", machineName, nonVSphereVolumes)
}
if len(unknownVolumes) > 0 {
klog.Warningf("Machine %s: volumes with unknown type attached: %v", machineName, unknownVolumes)
}

return len(vsphereVolumes) > 0, nil
}

// reconcileMachineWithCloudState reconcile machineSpec and status with the latest cloud state
Expand Down
Loading