kubelet: update ASW when CSIDriver attachment requirement changes

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

The problem

When a Kubernetes CSIDriver object changes its spec.attachRequired field from true to false, the kubelet correctly updates its desired state to reflect that the volume no longer needs controller attachment. However, the actual state of the world (ASW) in the kubelet still marks the volume as "attachable." As a result, the kubelet continues reporting the volume in Node.Status.VolumesInUse, and the attach/detach controller does not clean up the now-unnecessary VolumeAttachment object.

Why it's non-trivial

A naive fix might update only the desired state, but the actual state must also be updated to prevent dangling VolumeAttachment objects and incorrect status reporting. Both in-memory states must be kept in sync, and the volume's presence in ASW must be preserved while only its attachability flag is changed. This requires careful coordination and testing to ensure correctness during dynamic transitions.

The approach

1. Add a method to update attachability in ASW:

A new method is added to the ActualStateOfWorld interface and implementation to update the attachability of an existing volume without removing it:

+	// MarkVolumeAttachability updates attachability for an existing volume.
+	MarkVolumeAttachability(volumeName v1.UniqueVolumeName, attachable bool)

Implementation:

+func (asw *actualStateOfWorld) MarkVolumeAttachability(volumeName v1.UniqueVolumeName, attachable bool) {
+	asw.Lock()
+	defer asw.Unlock()
+
+	volumeObj, volumeExists := asw.attachedVolumes[volumeName]
+	if !volumeExists {
+		return
+	}
+
+	if attachable {
+		volumeObj.pluginIsAttachable = volumeAttachabilityTrue
+	} else {
+		volumeObj.pluginIsAttachable = volumeAttachabilityFalse
+	}
+	asw.attachedVolumes[volumeName] = volumeObj
+}

Annotation:
This method safely updates the pluginIsAttachable field for a volume in ASW, mapping the boolean input to the correct internal enum type. It does nothing if the volume is not present.

2. Update both desired and actual state when attachability changes:

In the populator, both states are updated when a volume transitions from attachable to non-attachable:

-					dswp.desiredStateOfWorld.MarkVolumeAttachability(volumeToMount.VolumeName, false)
+					dswp.desiredStateOfWorld.MarkVolumeAttachability(volumeToMount.VolumeName, false)
+					dswp.actualStateOfWorld.MarkVolumeAttachability(volumeToMount.VolumeName, false)

Annotation:
Previously, only the desired state was updated. Now, the actual state is also updated, ensuring the kubelet's view of the volume is consistent.

3. Add and update unit tests:

A new test verifies that updating attachability preserves the volume in ASW and updates its attachability:

+func TestMarkVolumeAttachability(t *testing.T) {
+	// ... setup omitted ...
+	if err := asw.MarkVolumeAsAttached(logger, volumeName, volumeSpec, "", "fake/device/path"); err != nil {
+		t.Fatalf("MarkVolumeAsAttached failed: %v", err)
+	}
+
+	asw.MarkVolumeAttachability(volumeName, false)
+
+	attachedVolume, found := asw.GetAttachedVolume(volumeName)
+	if !found {
+		t.Fatalf("Expected volume %q to remain in ASW", volumeName)
+	}
+	if attachedVolume.PluginIsAttachable {
+		t.Fatalf("Expected volume %q to be non-attachable", volumeName)
+	}
+}

Annotation:
This test ensures that after calling MarkVolumeAttachability, the volume remains present but is now marked non-attachable.

4. Strengthen e2e test to avoid races:

The e2e test now waits for the VolumeAttachment to be attached and the pod to be running before changing the CSIDriver:

+			ginkgo.By("Wait for VolumeAttachment to become attached")
+			err = e2evolume.WaitForVolumeAttached(ctx, attachmentName, m.cs, csiVolumeAttachmentTimeout)
+			framework.ExpectNoError(err, "VolumeAttachment %q did not become attached: %v", attachmentName, err)
+
+			ginkgo.By("Wait for pod volume mount to complete")
+			err = e2epod.WaitForPodNameRunningInNamespace(ctx, m.cs, pod.Name, pod.Namespace)
+			framework.ExpectNoError(err, "Pod did not start with AttachRequired=true: %v", err)

Annotation:
This ensures the attachability transition is tested only after the initial state is fully established, preventing race conditions.

From the review thread

[review] 249043822: nit: actually we don't need the if-else, volumeObj.pluginIsAttachable = attachable

[review] Rajalakshmi-Girish: Thanks for the suggestion. But unfortunately, volumeObj.pluginIsAttachable = attachable won't compile directly since pluginIsAttachable is of type volumeAttachability (a string alias with three states: True/False/Uncertain) while attachable is a bool.

The if-else is needed to map between the two types.

[review] 249043822: okay

Takeaways

• When updating the semantics of a resource, update all relevant in-memory state representations, not just the desired state.
• When mapping between types (e.g., bool to enum), ensure type safety and explicit conversion to avoid subtle bugs.
• Strengthen tests to cover state transitions and prevent race conditions by waiting for system state to stabilize before making assertions.