Remove validations for batch attach (#3622)

Author: skogta

pkg/apis/cnsoperator/cnsnodevmbatchattachment/v1alpha1/cnsnodebatchvmattachment_types.go

@@ -29,6 +29,9 @@ const (
 	IndependentPersistent DiskMode = "independent_persistent"
 	// Changes are immediately and permanently written to the virtual disk.
 	Persistent DiskMode = "persistent"
+	// Changes to virtual disk are made to a redo log and discarded at power off.
+	// It is not affected by snapshots.
+	IndependentNonPersistent = "independent_nonpersistent"
 )
 
 // The sharing mode of the virtual disk.

pkg/csi/service/wcp/controller.go

@@ -516,16 +516,6 @@ func (c *controller) createBlockVolume(ctx context.Context, req *csi.CreateVolum
 			"failed to get vCenter from Manager. Error: %v", err)
 	}
 
-	if commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx,
-		common.SharedDiskFss) && isSharedRawBlockRequest(ctx, req.VolumeCapabilities) {
-		log.Infof("Volume request is for shared RWX volume. Validatig if policy is compatible for VMFS datastores.")
-		err := validateStoragePolicyForVmfs(ctx, vc, storagePolicyID)
-		if err != nil {
-			log.Errorf("failed validation for policy %s", storagePolicyID)
-			return nil, csifault.CSIInternalFault, err
-		}
-	}
-
 	// Fetch the accessibility requirements from the request.
 	topologyRequirement = req.GetAccessibilityRequirements()
 	filterSuspendedDatastores := commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.CnsMgrSuspendCreateVolume)

pkg/csi/service/wcp/controller_helper.go

@@ -50,12 +50,6 @@ import (
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/syncer/k8scloudoperator"
 )
 
-var (
-	vmfsNamespace         = "com.vmware.storage.volumeallocation"
-	vmfsNamespaceEztValue = "Fully initialized"
-	vmfsClusteredVmdk     = "Clustered"
-)
-
 // validateCreateBlockReqParam is a helper function used to validate the parameter
 // name received in the CreateVolume request for block volumes on WCP CSI driver.
 // Returns true if the parameter name is valid, false otherwise.
@@ -92,64 +86,6 @@ func validateCreateFileReqParam(paramName, value string) bool {
 		paramName == common.AttributeIsLinkedCloneKey
 }
 
-// verifyStoragePolicyForVmfsUtil goes through each rule in the policy to
-// find out if it is fully intialized and has clustered VMDK enabled for VMFS datastores.
-// This check is required for RWX shared block volumes as for VMFS, the policy must be EZT
-// and clustered.
-func verifyStoragePolicyForVmfsUtil(ctx context.Context,
-	spbmPolicyContentList []vsphere.SpbmPolicyContent,
-	storagePolicyId string) error {
-	log := logger.GetLogger(ctx)
-
-	isEzt := false
-	isClustered := false
-	isVmfsNamespace := false
-
-	for _, polictContent := range spbmPolicyContentList {
-		for _, profile := range polictContent.Profiles {
-			for _, rule := range profile.Rules {
-				if rule.Ns == vmfsNamespace {
-					isVmfsNamespace = true
-					switch rule.Value {
-					case vmfsNamespaceEztValue:
-						isEzt = true
-					case vmfsClusteredVmdk:
-						isClustered = true
-					}
-				}
-			}
-		}
-	}
-
-	// If any policy is for VMFS and it is not EZT or clustered, then send back error.
-	if isVmfsNamespace && (!isEzt || !isClustered) {
-		msg := fmt.Sprintf("Policy %s is for VMFS datastores. "+
-			"It must be fully initialized and must have clustered VMDK enabled for "+
-			"RWX block volumes", storagePolicyId)
-		err := errors.New(msg)
-		log.Errorf(msg)
-		return err
-	}
-
-	log.Debugf("Policy %s validated correctly", storagePolicyId)
-	return nil
-}
-
-// validateStoragePolicyForVmfs checks whether the policy is compatible for VMFS datastores.
-// This function is only called for RWX shared block volumes.
-func validateStoragePolicyForVmfs(ctx context.Context,
-	vc *vsphere.VirtualCenter, storagePolicyId string) error {
-	log := logger.GetLogger(ctx)
-
-	spbmPolicyContentList, err := vc.PbmRetrieveContent(ctx, []string{storagePolicyId})
-	if err != nil {
-		log.Errorf("failed to retrieve policy %s", storagePolicyId)
-		return err
-	}
-
-	return verifyStoragePolicyForVmfsUtil(ctx, spbmPolicyContentList, storagePolicyId)
-}
-
 // validateWCPCreateVolumeRequest is the helper function to validate
 // CreateVolumeRequest for WCP CSI driver.
 // Function returns error if validation fails otherwise returns nil.

pkg/csi/service/wcp/controller_helper_test.go

@@ -11,7 +11,6 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
 	k8stesting "k8s.io/client-go/testing"
-	cnsvsphere "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/vsphere"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/unittestcommon"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common/commonco"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/logger"
@@ -133,95 +132,6 @@ func TestGetPodVMUUID(t *testing.T) {
 	})
 }
 
-func TestVerifyStoragePolicyForVmfsUtil(t *testing.T) {
-	ctx := context.TODO()
-	policyId := "policy-123"
-
-	tests := []struct {
-		name        string
-		input       []cnsvsphere.SpbmPolicyContent
-		expectError bool
-	}{
-		{
-			name: "Valid VMFS policy with EZT and Clustered VMDK",
-			input: []cnsvsphere.SpbmPolicyContent{
-				{
-					Profiles: []cnsvsphere.SpbmPolicySubProfile{
-						{
-							Rules: []cnsvsphere.SpbmPolicyRule{
-								{Ns: vmfsNamespace, Value: vmfsNamespaceEztValue},
-								{Ns: vmfsNamespace, Value: vmfsClusteredVmdk},
-							},
-						},
-					},
-				},
-			},
-			expectError: false,
-		},
-		{
-			name: "VMFS policy missing EZT",
-			input: []cnsvsphere.SpbmPolicyContent{
-				{
-					Profiles: []cnsvsphere.SpbmPolicySubProfile{
-						{
-							Rules: []cnsvsphere.SpbmPolicyRule{
-								{Ns: vmfsNamespace, Value: vmfsClusteredVmdk},
-							},
-						},
-					},
-				},
-			},
-			expectError: true,
-		},
-		{
-			name: "VMFS policy missing Clustered VMDK",
-			input: []cnsvsphere.SpbmPolicyContent{
-				{
-					Profiles: []cnsvsphere.SpbmPolicySubProfile{
-						{
-							Rules: []cnsvsphere.SpbmPolicyRule{
-								{Ns: vmfsNamespace, Value: vmfsNamespaceEztValue},
-							},
-						},
-					},
-				},
-			},
-			expectError: true,
-		},
-		{
-			name: "Non-VMFS policy should pass",
-			input: []cnsvsphere.SpbmPolicyContent{
-				{
-					Profiles: []cnsvsphere.SpbmPolicySubProfile{
-						{
-							Rules: []cnsvsphere.SpbmPolicyRule{
-								{Ns: "non-vmfs", Value: "some-value"},
-							},
-						},
-					},
-				},
-			},
-			expectError: false,
-		},
-		{
-			name:        "Empty policy list should pass",
-			input:       []cnsvsphere.SpbmPolicyContent{},
-			expectError: false,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			err := verifyStoragePolicyForVmfsUtil(ctx, tt.input, policyId)
-			if tt.expectError {
-				assert.Error(t, err)
-			} else {
-				assert.NoError(t, err)
-			}
-		})
-	}
-}
-
 func newMockPod(name, namespace, nodeName string, volumes []string,
 	annotations map[string]string, phase v1.PodPhase) *v1.Pod {
 	vols := make([]v1.Volume, len(volumes))

pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go

@@ -23,7 +23,6 @@ import (
 	"slices"
 
 	vimtypes "github.com/vmware/govmomi/vim25/types"
-	v1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	v1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsnodevmbatchattachment/v1alpha1"
 	volumes "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume"
@@ -361,82 +360,11 @@ func constructBatchAttachRequest(ctx context.Context,
 			ControllerKey: volume.PersistentVolumeClaim.ControllerKey,
 			UnitNumber:    volume.PersistentVolumeClaim.UnitNumber,
 		}
-		// Validate each attach request before proceeding.
-		updateBatchAttachRequest, err := validateBatchAttachRequest(ctx,
-			currentBatchAttachRequest, instance.Namespace, pvcName)
-		if err != nil {
-			log.Errorf("failed to validate attach request for PVC %s in namespace %s. Err: %s",
-				pvcName, instance.Namespace, err)
-			return pvcsInSpec, volumeIdsInSpec, batchAttachRequest, err
-		}
-		batchAttachRequest = append(batchAttachRequest, updateBatchAttachRequest)
+		batchAttachRequest = append(batchAttachRequest, currentBatchAttachRequest)
 	}
 	return pvcsInSpec, volumeIdsInSpec, batchAttachRequest, nil
 }
 
-// validateBatchAttachRequest ensures that the right combination for the input request.
-// This is the validation criteria:
-// RWX accessMode -> ControllerKey and UnitNumber are required, DiskMode must be IndependentPersistent.
-// RWO accessMode -> DiskMode must not be IndependentPersistent, SharingMode must not be SharingMultiWriter.
-func validateBatchAttachRequest(ctx context.Context,
-	batchAttachRequest volumes.BatchAttachRequest, namespace string, pvcName string) (volumes.BatchAttachRequest, error) {
-	log := logger.GetLogger(ctx)
-
-	log.Infof("Verifying if PVC %s has correct input parameters for batch attach", pvcName)
-
-	// Get PVC object from informer cache
-	pvc, err := commonco.ContainerOrchestratorUtility.GetPvcObjectByName(ctx, pvcName, namespace)
-	if err != nil {
-		log.Errorf("failed to get PVC object for PVC %s. Err: %s", pvcName, err)
-		return batchAttachRequest, err
-	}
-
-	for _, accessMode := range pvc.Spec.AccessModes {
-		// RWX accessMode -> ControllerKey and UnitNumber are required, DiskMode must be IndependentPersistent.
-		if accessMode == v1.ReadWriteMany || accessMode == v1.ReadOnlyMany {
-			if batchAttachRequest.DiskMode == "" {
-				batchAttachRequest.DiskMode = string(v1alpha1.IndependentPersistent)
-			}
-			if batchAttachRequest.DiskMode != string(v1alpha1.IndependentPersistent) {
-				return batchAttachRequest, fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
-					"DiskMode cannot be %s", pvcName, namespace, accessMode, batchAttachRequest.DiskMode)
-			}
-			if batchAttachRequest.ControllerKey == "" {
-				return batchAttachRequest, fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
-					"ControllerKey cannot be empty", pvcName, namespace, accessMode)
-			}
-			if batchAttachRequest.UnitNumber == "" {
-				return batchAttachRequest, fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
-					" UnitNumber cannot be empty", pvcName, namespace, accessMode)
-			}
-			if batchAttachRequest.SharingMode == "" {
-				return batchAttachRequest, fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
-					" SharingMode cannot be empty", pvcName, namespace, accessMode)
-			}
-		}
-
-		// RWO accessMode -> DiskMode, SharingMode and ControllerNumber and Unit Number must be empty.
-		if accessMode == v1.ReadWriteOnce {
-			if batchAttachRequest.DiskMode != "" {
-				return batchAttachRequest, fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
-					"DiskMode cannot be %s", pvcName, namespace, accessMode, batchAttachRequest.DiskMode)
-			}
-			if batchAttachRequest.SharingMode != "" {
-				return batchAttachRequest, fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
-					"SharingMode cannot be %s", pvcName, namespace, accessMode, batchAttachRequest.SharingMode)
-			}
-			if batchAttachRequest.ControllerKey != "" || batchAttachRequest.UnitNumber != "" {
-				return batchAttachRequest, fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
-					"ControllerNumber and UnitNumber must not be provided with RWO accessMode", pvcName, namespace, accessMode)
-			}
-		}
-	}
-
-	log.Infof("Validated request for PVC %s in namespace %s", pvcName, namespace)
-
-	return batchAttachRequest, nil
-}
-
 // getVmObject find the VM object on vCenter.
 // If VM retrieval from vCenter fails with NotFound error,
 // then it is not considered an error because VM CR is probably being deleted.

pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_test.go

@@ -34,7 +34,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
-	volumes "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume"
 	cnsvsphere "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/vsphere"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/config"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/unittestcommon"
@@ -367,83 +366,6 @@ func TestReconcileWithoutDeletionTimestampWhenAttachFails(t *testing.T) {
 	})
 }
 
-func TestValidateBatchAttachRequestWithRwoPvc(t *testing.T) {
-
-	t.Run("TestValidateBatchAttachRequestWithRwoPvc", func(t *testing.T) {
-
-		batchAttachRequest := volumes.BatchAttachRequest{
-			DiskMode: "IndependentPersistent",
-		}
-
-		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
-		_, err := validateBatchAttachRequest(context.TODO(), batchAttachRequest, testNamespace, "pvc-1")
-		expectedEr := fmt.Errorf("incorrect input for PVC pvc-1 in namespace test-ns with accessMode ReadWriteOnce. " +
-			"DiskMode cannot be IndependentPersistent")
-		assert.EqualError(t, expectedEr, err.Error())
-
-		batchAttachRequest = volumes.BatchAttachRequest{
-			SharingMode: "sharingMultiWriter",
-		}
-
-		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
-		_, err = validateBatchAttachRequest(context.TODO(), batchAttachRequest, testNamespace, "pvc-1")
-		expectedEr = fmt.Errorf("incorrect input for PVC pvc-1 in namespace test-ns with accessMode ReadWriteOnce. " +
-			"SharingMode cannot be sharingMultiWriter")
-		assert.EqualError(t, expectedEr, err.Error())
-
-		batchAttachRequest = volumes.BatchAttachRequest{
-			SharingMode: "",
-		}
-
-		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
-		_, err = validateBatchAttachRequest(context.TODO(), batchAttachRequest, testNamespace, "pvc-1")
-		assert.NoError(t, err)
-
-	})
-}
-
-func TestValidateBatchAttachRequestWithRwxPvc(t *testing.T) {
-
-	t.Run("TestValidateBatchAttachRequestWithRwxPvc", func(t *testing.T) {
-
-		batchAttachRequest := volumes.BatchAttachRequest{
-			DiskMode:      "persistent",
-			ControllerKey: "12345",
-			UnitNumber:    "9",
-		}
-
-		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
-		_, err := validateBatchAttachRequest(context.TODO(), batchAttachRequest, testNamespace, "pvc-rwx")
-		expectedErr := fmt.Errorf("incorrect input for PVC pvc-rwx in namespace test-ns with accessMode ReadWriteMany. " +
-			"DiskMode cannot be persistent")
-		assert.EqualError(t, expectedErr, err.Error())
-
-		batchAttachRequest = volumes.BatchAttachRequest{
-			ControllerKey: "",
-			UnitNumber:    "12",
-			DiskMode:      "independent_persistent",
-		}
-
-		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
-		_, err = validateBatchAttachRequest(context.TODO(), batchAttachRequest, testNamespace, "pvc-rwx")
-		expectedErr = fmt.Errorf("incorrect input for PVC pvc-rwx in namespace test-ns with accessMode ReadWriteMany. " +
-			"ControllerKey cannot be empty")
-		assert.EqualError(t, expectedErr, err.Error())
-
-		batchAttachRequest = volumes.BatchAttachRequest{
-			ControllerKey: "1001",
-			UnitNumber:    "12",
-			DiskMode:      "",
-			SharingMode:   "None",
-		}
-
-		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
-		attacheReq, err := validateBatchAttachRequest(context.TODO(), batchAttachRequest, testNamespace, "pvc-rwx")
-		assert.NoError(t, err)
-		assert.Equal(t, "independent_persistent", attacheReq.DiskMode)
-	})
-}
-
 func MockGetVMFromVcenter(ctx context.Context, nodeUUID string,
 	configInfo config.ConfigurationInfo) (*cnsvsphere.VirtualMachine, error) {
 	var vm *cnsvsphere.VirtualMachine