Change batch attach validation to not allow DiskMode and SharingMode for RWX volumes (#3550)

Author: skogta

pkg/common/cns-lib/volume/manager.go

@@ -3585,7 +3585,7 @@ func (m *defaultManager) BatchAttachVolumes(ctx context.Context,
 		var errMsg string
 		if len(volumesThatFailedToAttach) != 0 {
 			errMsg = strings.Join(volumesThatFailedToAttach, ",")
-			overallError = errors.New(errMsg)
+			overallError = errors.New("failed to attach volumes: " + errMsg)
 			return batchAttachResult, csifault.CSIBatchAttachFault, overallError
 		}
 		log.Infof("BatchAttach: all volumes attached successfully with opID %s", taskInfo.ActivationId)

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

@@ -362,13 +362,14 @@ func constructBatchAttachRequest(ctx context.Context,
 			UnitNumber:    volume.PersistentVolumeClaim.UnitNumber,
 		}
 		// Validate each attach request before proceeding.
-		err := validateBatchAttachRequest(ctx, currentBatchAttachRequest, instance.Namespace, pvcName)
+		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, currentBatchAttachRequest)
+		batchAttachRequest = append(batchAttachRequest, updateBatchAttachRequest)
 	}
 	return pvcsInSpec, volumeIdsInSpec, batchAttachRequest, nil
 }
@@ -378,7 +379,7 @@ func constructBatchAttachRequest(ctx context.Context,
 // 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) error {
+	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)
@@ -387,46 +388,53 @@ func validateBatchAttachRequest(ctx context.Context,
 	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 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 fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
+				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 fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
+				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 fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
+				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 must NOT be IndependentPersistent, SharingMode must not be SharingMultiWriter.
+		// RWO accessMode -> DiskMode, SharingMode and ControllerNumber and Unit Number must be empty.
 		if accessMode == v1.ReadWriteOnce {
-			if batchAttachRequest.DiskMode != string(v1alpha1.Persistent) && batchAttachRequest.DiskMode != "" {
-				return fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
+			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 != string(v1alpha1.SharingNone) && batchAttachRequest.SharingMode != "" {
-				return fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
+			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 fmt.Errorf("incorrect input for PVC %s in namespace %s with accessMode %s. "+
+			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 nil
+	return batchAttachRequest, nil
 }
 
 // getVmObject find the VM object on vCenter.

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

@@ -376,7 +376,7 @@ func TestValidateBatchAttachRequestWithRwoPvc(t *testing.T) {
 		}
 
 		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
-		err := validateBatchAttachRequest(context.TODO(), batchAttachRequest, testNamespace, "pvc-1")
+		_, 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())
@@ -386,10 +386,19 @@ func TestValidateBatchAttachRequestWithRwoPvc(t *testing.T) {
 		}
 
 		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
-		err = validateBatchAttachRequest(context.TODO(), batchAttachRequest, testNamespace, "pvc-1")
+		_, 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)
+
 	})
 }
 
@@ -404,7 +413,7 @@ func TestValidateBatchAttachRequestWithRwxPvc(t *testing.T) {
 		}
 
 		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
-		err := validateBatchAttachRequest(context.TODO(), batchAttachRequest, testNamespace, "pvc-rwx")
+		_, 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())
@@ -416,10 +425,22 @@ func TestValidateBatchAttachRequestWithRwxPvc(t *testing.T) {
 		}
 
 		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
-		err = validateBatchAttachRequest(context.TODO(), batchAttachRequest, testNamespace, "pvc-rwx")
+		_, 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)
 	})
 }