fix:volumeattachment could not be cleanup while controller-manager restart after volumeattachment created because of csidriver created delay

Project: kubernetes/kubernetes | Language: Go | Concepts: refactoring, error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/kubernetes/kubernetes/pull/141158

The problem

When the Kubernetes controller-manager restarts, it may not be able to clean up VolumeAttachment objects if the corresponding CSIDriver object was created with a delay. This leads to stuck attachments and potential resource leaks, especially for CSI volumes that are not immediately recognized as detachable.

Why it's non-trivial

A naive fix might just retry attachment or detachment, but the root cause is that the controller logic relies on plugin detection that fails if the CSIDriver isn't yet present. The logic for determining if a volume is detachable is duplicated and not robust to the CSI driver's late appearance. Properly handling this requires both code deduplication and more resilient plugin detection.

The approach

1. Deduplicate and move detachable plugin logic:
The logic to determine if a volume is detachable (especially for CSI plugins) was duplicated and not easily reusable. The PR moves this logic into a shared utility function.

// pkg/volume/util/util.go
+// FindDetachablePluginBySpec is a variant of VolumePluginMgr.FindAttachablePluginByName() function.
+// The difference is that it bypass the CanAttach() check for CSI plugin, i.e. it assumes all CSI plugin supports detach.
+// The intention here is that a CSI plugin volume can end up in an Uncertain state,  so that a detach
+// operation will help it to detach no matter it actually has the ability to attach/detach.
+func FindDetachablePluginBySpec(spec *volume.Spec, pm *volume.VolumePluginMgr) (volume.AttachableVolumePlugin, error) {
+	volumePlugin, err := pm.FindPluginBySpec(spec)
+	if err != nil {
+		return nil, err
+	}
+	if attachableVolumePlugin, ok := volumePlugin.(volume.AttachableVolumePlugin); ok {
+		if attachableVolumePlugin.GetPluginName() == "kubernetes.io/csi" {
+			return attachableVolumePlugin, nil
+		}
+		if canAttach, err := attachableVolumePlugin.CanAttach(spec); err != nil {
+			return nil, err
+		} else if canAttach {
+			return attachableVolumePlugin, nil
+		}
+	}
+	return nil, nil
+}

This function centralizes the logic for finding a detachable plugin, specifically allowing CSI plugins to be treated as detachable even if their driver hasn't fully registered yet.

2. Refactor controller and operation generator to use new utility:
Both the attach-detach controller and the operation generator are updated to use the new shared function.

-			plugin, err = adc.volumePluginMgr.FindAttachablePluginBySpec(volumeSpec)
+			plugin, err = volumeutil.FindDetachablePluginBySpec(volumeSpec, &adc.volumePluginMgr)

-		attachableVolumePlugin, err = findDetachablePluginBySpec(volumeToDetach.VolumeSpec, og.volumePluginMgr)
+		attachableVolumePlugin, err = util.FindDetachablePluginBySpec(volumeToDetach.VolumeSpec, og.volumePluginMgr)

This ensures consistent logic is used everywhere, reducing the risk of subtle bugs and making future maintenance easier.

3. Expand and improve testing for CSI plugin scenarios:
The test suite is enhanced to cover the case where a CSI driver is present but not immediately available, ensuring the new logic is exercised.

+		{ // pod is scheduled, volume changes from attachable to non-attachable, attach status:false, verify volume is marked as uncertain
+			testName:               "Scheduled Pod with non-attachable PV",
+			volName:                "csi-driver1^vol-handle1",
+			podNodeName:            "mynode-1",
+			pvName:                 "pv1",
+			vaName:                 "va1",
+			vaNodeName:             "mynode-1",
+			vaAttachStatus:         false,
+			csiMigration:           false,
+			csiPlugin:              true,
+			expectedASWAttachState: cache.AttachStateUncertain,
+		},

This test ensures that the controller marks the state as "uncertain" when the plugin is not immediately recognized, matching the intended behavior.

4. Add helpers for CSI driver and PV creation in tests:
To support the new test, helpers for creating fake CSI drivers and PVs are added.

+func NewCSIDriver(uid, name string, attachable bool) *storagev1.CSIDriver { ... }
+func NewCSIPV(pvName string) *v1.PersistentVolume { ... }

These helpers make it easy to simulate the presence or absence of CSI drivers in the test environment.

From the review thread

This function is same as - https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util/operationexecutor/operation_generator.go#L2214

I wonder if we can reuse this code, so as in future this intention is clearer in both the places.

Done

Takeaways

• Centralize shared logic to avoid duplication and subtle inconsistencies.
• Handle plugin or dependency delays gracefully, especially in distributed systems where registration order is not guaranteed.
• Expand tests to cover edge cases introduced by asynchronous or delayed resource creation.