Merge pull request #11386 from sbueringer/pr-add-fg-volume-detach-attachments

🌱 Add feature gate to consider VolumeAttachments when waiting for volume detach

Author: k8s-ci-robot

config/manager/manager.yaml

@@ -24,7 +24,7 @@ spec:
             - "--diagnostics-address=${CAPI_DIAGNOSTICS_ADDRESS:=:8443}"
             - "--insecure-diagnostics=${CAPI_INSECURE_DIAGNOSTICS:=false}"
             - "--use-deprecated-infra-machine-naming=${CAPI_USE_DEPRECATED_INFRA_MACHINE_NAMING:=false}"
-            - "--feature-gates=MachinePool=${EXP_MACHINE_POOL:=true},ClusterResourceSet=${EXP_CLUSTER_RESOURCE_SET:=true},ClusterTopology=${CLUSTER_TOPOLOGY:=false},RuntimeSDK=${EXP_RUNTIME_SDK:=false},MachineSetPreflightChecks=${EXP_MACHINE_SET_PREFLIGHT_CHECKS:=true}"
+            - "--feature-gates=MachinePool=${EXP_MACHINE_POOL:=true},ClusterResourceSet=${EXP_CLUSTER_RESOURCE_SET:=true},ClusterTopology=${CLUSTER_TOPOLOGY:=false},RuntimeSDK=${EXP_RUNTIME_SDK:=false},MachineSetPreflightChecks=${EXP_MACHINE_SET_PREFLIGHT_CHECKS:=true},MachineWaitForVolumeDetachConsiderVolumeAttachments=${EXP_MACHINE_WAITFORVOLUMEDETACH_CONSIDER_VOLUMEATTACHMENTS:=true}"
           image: controller:latest
           name: manager
           env:

docs/book/src/tasks/experimental-features/experimental-features.md

@@ -3,6 +3,18 @@
 Cluster API now ships with a new experimental package that lives under the `exp/` directory. This is a
 temporary location for features which will be moved to their permanent locations after graduation. Users can experiment with these features by enabling them using feature gates.
 
+Currently Cluster API has the following experimental features:
+* `ClusterResourceSet` (env var: `EXP_CLUSTER_RESOURCE_SET`): [ClusterResourceSet](./cluster-resource-set.md)
+* `MachinePool` (env var: `EXP_MACHINE_POOL`): [MachinePools](./machine-pools.md)
+* `MachineSetPreflightChecks` (env var: `EXP_MACHINE_SET_PREFLIGHT_CHECKS`): [MachineSetPreflightChecks](./machineset-preflight-checks.md)
+* `MachineWaitForVolumeDetachConsiderVolumeAttachments` (env var: `EXP_MACHINE_WAITFORVOLUMEDETACH_CONSIDER_VOLUMEATTACHMENTS`):
+  * During Machine drain the Machine controller waits for volumes to be detached. Per default, the controller considers
+    `Nodes.status.volumesAttached` and `VolumesAttachments`. This feature flag allows to opt-out from considering `VolumeAttachments`.
+    The feature gate was added to allow to opt-out in case unforeseen issues occur with `VolumeAttachments`.
+* `ClusterTopology` (env var: `CLUSTER_TOPOLOGY`): [ClusterClass](./cluster-class/index.md)
+* `RuntimeSDK` (env var: `EXP_RUNTIME_SDK`): [RuntimeSDK](./runtime-sdk/index.md)
+* `KubeadmBootstrapFormatIgnition` (env var: `EXP_KUBEADM_BOOTSTRAP_FORMAT_IGNITION`): [Ignition](./ignition.md)
+
 ## Enabling Experimental Features for Management Clusters Started with clusterctl
 
 Users can enable/disable features by setting OS environment variables before running `clusterctl init`, e.g.:

feature/feature.go

@@ -62,6 +62,12 @@ const (
 	// alpha: v1.5
 	// beta: v1.9
 	MachineSetPreflightChecks featuregate.Feature = "MachineSetPreflightChecks"
+
+	// MachineWaitForVolumeDetachConsiderVolumeAttachments is a feature gate that controls if the Machine controller
+	// also considers VolumeAttachments in addition to Nodes.status.volumesAttached when waiting for volumes to be detached.
+	//
+	// beta: v1.9
+	MachineWaitForVolumeDetachConsiderVolumeAttachments featuregate.Feature = "MachineWaitForVolumeDetachConsiderVolumeAttachments"
 )
 
 func init() {
@@ -72,9 +78,10 @@ func init() {
 // To add a new feature, define a key for it above and add it here.
 var defaultClusterAPIFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
 	// Every feature should be initiated here:
-	ClusterResourceSet:             {Default: true, PreRelease: featuregate.Beta},
-	MachinePool:                    {Default: true, PreRelease: featuregate.Beta},
-	MachineSetPreflightChecks:      {Default: true, PreRelease: featuregate.Beta},
+	ClusterResourceSet:        {Default: true, PreRelease: featuregate.Beta},
+	MachinePool:               {Default: true, PreRelease: featuregate.Beta},
+	MachineSetPreflightChecks: {Default: true, PreRelease: featuregate.Beta},
+	MachineWaitForVolumeDetachConsiderVolumeAttachments: {Default: true, PreRelease: featuregate.Beta},
 	ClusterTopology:                {Default: false, PreRelease: featuregate.Alpha},
 	KubeadmBootstrapFormatIgnition: {Default: false, PreRelease: featuregate.Alpha},
 	RuntimeSDK:                     {Default: false, PreRelease: featuregate.Alpha},

internal/controllers/machine/machine_controller.go

@@ -49,6 +49,7 @@ import (
 	"sigs.k8s.io/cluster-api/controllers/clustercache"
 	"sigs.k8s.io/cluster-api/controllers/external"
 	"sigs.k8s.io/cluster-api/controllers/noderefutil"
+	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/controllers/machine/drain"
 	"sigs.k8s.io/cluster-api/internal/util/cache"
 	"sigs.k8s.io/cluster-api/internal/util/ssa"
@@ -1125,17 +1126,19 @@ func getAttachedVolumeInformation(ctx context.Context, remoteClient client.Clien
 		attachedVolumeName.Insert(string(attachedVolume.Name))
 	}
 
-	volumeAttachments, err := getVolumeAttachmentForNode(ctx, remoteClient, node.GetName())
-	if err != nil {
-		return nil, nil, errors.Wrap(err, "failed to list VolumeAttachments")
-	}
+	if feature.Gates.Enabled(feature.MachineWaitForVolumeDetachConsiderVolumeAttachments) {
+		volumeAttachments, err := getVolumeAttachmentForNode(ctx, remoteClient, node.GetName())
+		if err != nil {
+			return nil, nil, errors.Wrap(err, "failed to list VolumeAttachments")
+		}
 
-	for _, va := range volumeAttachments {
-		// Return an error if a VolumeAttachments does not refer a PersistentVolume.
-		if va.Spec.Source.PersistentVolumeName == nil {
-			return nil, nil, errors.Errorf("spec.source.persistentVolumeName for VolumeAttachment %s is not set", va.GetName())
+		for _, va := range volumeAttachments {
+			// Return an error if a VolumeAttachments does not refer a PersistentVolume.
+			if va.Spec.Source.PersistentVolumeName == nil {
+				return nil, nil, errors.Errorf("spec.source.persistentVolumeName for VolumeAttachment %s is not set", va.GetName())
+			}
+			attachedPVNames.Insert(*va.Spec.Source.PersistentVolumeName)
 		}
-		attachedPVNames.Insert(*va.Spec.Source.PersistentVolumeName)
 	}
 
 	return attachedVolumeName, attachedPVNames, nil

internal/controllers/machine/machine_controller_test.go

@@ -30,6 +30,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/client-go/tools/record"
+	utilfeature "k8s.io/component-base/featuregate/testing"
 	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache/informertest"
@@ -44,6 +45,7 @@ import (
 	"sigs.k8s.io/cluster-api/controllers/clustercache"
 	"sigs.k8s.io/cluster-api/controllers/external"
 	externalfake "sigs.k8s.io/cluster-api/controllers/external/fake"
+	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/util/cache"
 	"sigs.k8s.io/cluster-api/internal/util/ssa"
 	"sigs.k8s.io/cluster-api/util"
@@ -2007,6 +2009,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) {
 		name                    string
 		node                    *corev1.Node
 		remoteObjects           []client.Object
+		featureGateDisabled     bool
 		expected                ctrl.Result
 		expectedDeletingReason  string
 		expectedDeletingMessage string
@@ -2158,6 +2161,28 @@ func TestShouldWaitForNodeVolumes(t *testing.T) {
 			expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z)
 * PersistentVolumeClaims: default/test-pvc`,
 		},
+		{
+			name: "Node has volumes attached according to volumeattachments (but ignored because feature gate is disabled)",
+			node: &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: nodeName,
+				},
+				Status: corev1.NodeStatus{
+					Conditions: []corev1.NodeCondition{
+						{
+							Type:   corev1.NodeReady,
+							Status: corev1.ConditionTrue,
+						},
+					},
+				},
+			},
+			remoteObjects: []client.Object{
+				volumeAttachment,
+				persistentVolume,
+			},
+			featureGateDisabled: true,
+			expected:            ctrl.Result{},
+		},
 		{
 			name: "Node has volumes attached according to volumeattachments but without a pv",
 			node: &corev1.Node{
@@ -2339,6 +2364,10 @@ func TestShouldWaitForNodeVolumes(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
+			if tt.featureGateDisabled {
+				utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachineWaitForVolumeDetachConsiderVolumeAttachments, false)
+			}
+
 			fakeClient := fake.NewClientBuilder().WithObjects(testCluster).Build()
 
 			var remoteObjects []client.Object