cephfs: remove subvolumegroup creation

PendingReleaseNotes.md

@@ -4,6 +4,9 @@
 
 - Removed the deprecated grpc metrics flag's in [PR](https://github.com/ceph/ceph-csi/pull/4225)
 
+- Support for pre-creation of cephFS subvolumegroup before creating subvolume
+  is removed in [PR](https://github.com/ceph/ceph-csi/pull/4195)
+
 ## Features
 
 RBD

deploy/csi-config-map-sample.yaml

@@ -20,6 +20,7 @@ kind: ConfigMap
 # NOTE: Make sure you don't add radosNamespace option to a currently in use
 # configuration as it will cause issues.
 # The field "cephFS.subvolumeGroup" is optional and defaults to "csi".
+# NOTE: The given subvolumeGroup must already exist in the filesystem.
 # The "cephFS.netNamespaceFilePath" fields are the various network namespace
 # path for the Ceph cluster identified by the <cluster-id>, This will be used
 # by the CephFS CSI plugin to execute the mount -t in the

docs/deploy-cephfs.md

@@ -239,3 +239,9 @@ However, not all KMS are supported in order to be compatible with
 [fscrypt](https://github.com/google/fscrypt). In general KMS that
 either store secrets to use directly (Vault), or allow access to the
 plain password (Kubernetes Secrets) work.
+
+## CephFS PVC Provisioning
+
+Requires subvolumegroup to be created before provisioning the PVC.
+If the subvolumegroup provided in `ceph-csi-config` ConfigMap is missing
+in the ceph cluster, the PVC creation will fail and will stay in `Pending` state.

e2e/cephfs.go

@@ -215,6 +215,11 @@ var _ = Describe(cephfsType, func() {
 		if err != nil {
 			framework.Failf("timeout waiting for deployment update %s/%s: %v", cephCSINamespace, cephFSDeploymentName, err)
 		}
+
+		err = createSubvolumegroup(f, fileSystemName, subvolumegroup)
+		if err != nil {
+			framework.Failf("%v", err)
+		}
 	})
 
 	AfterEach(func() {
@@ -254,10 +259,15 @@ var _ = Describe(cephfsType, func() {
 		}
 		deleteVault()
 
+		err = deleteSubvolumegroup(f, fileSystemName, subvolumegroup)
+		if err != nil {
+			framework.Failf("%v", err)
+		}
+
 		if deployCephFS {
 			deleteCephfsPlugin()
 			if cephCSINamespace != defaultNs {
-				err := deleteNamespace(c, cephCSINamespace)
+				err = deleteNamespace(c, cephCSINamespace)
 				if err != nil {
 					framework.Failf("failed to delete namespace %s: %v", cephCSINamespace, err)
 				}
@@ -939,14 +949,24 @@ var _ = Describe(cephfsType, func() {
 				}
 
 				// re-define configmap with information of multiple clusters.
+				subvolgrp1 := "subvolgrp1"
+				subvolgrp2 := "subvolgrp2"
 				clusterInfo := map[string]map[string]string{}
 				clusterID1 := "clusterID-1"
 				clusterID2 := "clusterID-2"
 				clusterInfo[clusterID1] = map[string]string{}
-				clusterInfo[clusterID1]["subvolumeGroup"] = "subvolgrp1"
+				clusterInfo[clusterID1]["subvolumeGroup"] = subvolgrp1
 				clusterInfo[clusterID2] = map[string]string{}
-				clusterInfo[clusterID2]["subvolumeGroup"] = "subvolgrp2"
+				clusterInfo[clusterID2]["subvolumeGroup"] = subvolgrp2
 
+				err = createSubvolumegroup(f, fileSystemName, subvolgrp1)
+				if err != nil {
+					framework.Failf("%v", err)
+				}
+				err = createSubvolumegroup(f, fileSystemName, subvolgrp2)
+				if err != nil {
+					framework.Failf("%v", err)
+				}
 				err = createCustomConfigMap(f.ClientSet, cephFSDirPath, clusterInfo)
 				if err != nil {
 					framework.Failf("failed to create configmap: %v", err)
@@ -967,7 +987,7 @@ var _ = Describe(cephfsType, func() {
 					framework.Failf("failed to delete storageclass: %v", err)
 				}
 				// verify subvolume group creation.
-				err = validateSubvolumegroup(f, "subvolgrp1")
+				err = validateSubvolumegroup(f, subvolgrp1)
 				if err != nil {
 					framework.Failf("failed to validate subvolume group: %v", err)
 				}
@@ -987,10 +1007,18 @@ var _ = Describe(cephfsType, func() {
 				if err != nil {
 					framework.Failf("failed to delete storageclass: %v", err)
 				}
-				err = validateSubvolumegroup(f, "subvolgrp2")
+				err = validateSubvolumegroup(f, subvolgrp2)
 				if err != nil {
 					framework.Failf("failed to validate subvolume group: %v", err)
 				}
+				err = deleteSubvolumegroup(f, fileSystemName, subvolgrp1)
+				if err != nil {
+					framework.Failf("%v", err)
+				}
+				err = deleteSubvolumegroup(f, fileSystemName, subvolgrp2)
+				if err != nil {
+					framework.Failf("%v", err)
+				}
 				err = deleteConfigMap(cephFSDirPath)
 				if err != nil {
 					framework.Failf("failed to delete configmap: %v", err)

e2e/nfs.go

@@ -276,6 +276,11 @@ var _ = Describe("nfs", func() {
 		if err != nil {
 			framework.Failf("failed to create node secret: %v", err)
 		}
+
+		err = createSubvolumegroup(f, fileSystemName, subvolumegroup)
+		if err != nil {
+			framework.Failf("%v", err)
+		}
 	})
 
 	AfterEach(func() {
@@ -313,10 +318,15 @@ var _ = Describe("nfs", func() {
 		if err != nil {
 			framework.Failf("failed to delete storageclass: %v", err)
 		}
+		err = deleteSubvolumegroup(f, fileSystemName, subvolumegroup)
+		if err != nil {
+			framework.Failf("%v", err)
+		}
+
 		if deployNFS {
 			deleteNFSPlugin()
 			if cephCSINamespace != defaultNs {
-				err := deleteNamespace(c, cephCSINamespace)
+				err = deleteNamespace(c, cephCSINamespace)
 				if err != nil {
 					framework.Failf("failed to delete namespace %s: %v", cephCSINamespace, err)
 				}

e2e/utils.go

@@ -1853,3 +1853,31 @@ func checkExports(f *framework.Framework, clusterID, clientString string) bool {
 
 	return true
 }
+
+// createSubvolumegroup creates a subvolumegroup.
+func createSubvolumegroup(f *framework.Framework, fileSystemName, subvolumegroup string) error {
+	cmd := fmt.Sprintf("ceph fs subvolumegroup create %s %s", fileSystemName, subvolumegroup)
+	_, stdErr, err := execCommandInToolBoxPod(f, cmd, rookNamespace)
+	if err != nil {
+		return fmt.Errorf("failed to exec command in toolbox: %w", err)
+	}
+	if stdErr != "" {
+		return fmt.Errorf("failed to create subvolumegroup %s : %v", subvolumegroup, stdErr)
+	}
+
+	return nil
+}
+
+// deleteSubvolumegroup creates a subvolumegroup.
+func deleteSubvolumegroup(f *framework.Framework, fileSystemName, subvolumegroup string) error {
+	cmd := fmt.Sprintf("ceph fs subvolumegroup rm %s %s", fileSystemName, subvolumegroup)
+	_, stdErr, err := execCommandInToolBoxPod(f, cmd, rookNamespace)
+	if err != nil {
+		return fmt.Errorf("failed to exec command in toolbox: %w", err)
+	}
+	if stdErr != "" {
+		return fmt.Errorf("failed to remove subvolumegroup %s : %v", subvolumegroup, stdErr)
+	}
+
+	return nil
+}

internal/cephfs/core/metadata.go

@@ -51,32 +51,6 @@ func (s *subVolumeClient) isUnsupportedSubVolMetadata(err error) bool {
 	return true
 }
 
-// isSubVolumeGroupCreated returns true if subvolume group is created.
-func (s *subVolumeClient) isSubVolumeGroupCreated() bool {
-	newLocalClusterState(s.clusterID)
-	clusterAdditionalInfo[s.clusterID].subVolumeGroupsRWMutex.RLock()
-	defer clusterAdditionalInfo[s.clusterID].subVolumeGroupsRWMutex.RUnlock()
-
-	if clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated == nil {
-		return false
-	}
-
-	return clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated[s.SubvolumeGroup]
-}
-
-// updateSubVolumeGroupCreated updates subvolume group created map.
-// If the map is nil, it creates a new map and updates it.
-func (s *subVolumeClient) updateSubVolumeGroupCreated(state bool) {
-	clusterAdditionalInfo[s.clusterID].subVolumeGroupsRWMutex.Lock()
-	defer clusterAdditionalInfo[s.clusterID].subVolumeGroupsRWMutex.Unlock()
-
-	if clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated == nil {
-		clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated = make(map[string]bool)
-	}
-
-	clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated[s.SubvolumeGroup] = state
-}
-
 // setMetadata sets custom metadata on the subvolume in a volume as a
 // key-value pair.
 func (s *subVolumeClient) setMetadata(key, value string) error {

internal/cephfs/core/volume.go

@@ -210,14 +210,6 @@ type localClusterState struct {
 	resizeState                 operationState
 	subVolMetadataState         operationState
 	subVolSnapshotMetadataState operationState
-	// A cluster can have multiple filesystem for that we need to have a map of
-	// subvolumegroups to check filesystem is created nor not.
-	// set true once a subvolumegroup is created
-	// for corresponding filesystem in a cluster.
-	subVolumeGroupsCreated map[string]bool
-	// subVolumeGroupsRWMutex is used to protect subVolumeGroupsCreated map
-	//  against concurrent writes while allowing multiple readers.
-	subVolumeGroupsRWMutex sync.RWMutex
 }
 
 func newLocalClusterState(clusterID string) {
@@ -241,24 +233,6 @@ func (s *subVolumeClient) CreateVolume(ctx context.Context) error {
 		return err
 	}
 
-	// create subvolumegroup if not already created for the cluster.
-	if !s.isSubVolumeGroupCreated() {
-		opts := fsAdmin.SubVolumeGroupOptions{}
-		err = ca.CreateSubVolumeGroup(s.FsName, s.SubvolumeGroup, &opts)
-		if err != nil {
-			log.ErrorLog(
-				ctx,
-				"failed to create subvolume group %s, for the vol %s: %s",
-				s.SubvolumeGroup,
-				s.VolID,
-				err)
-
-			return err
-		}
-		log.DebugLog(ctx, "cephfs: created subvolume group %s", s.SubvolumeGroup)
-		s.updateSubVolumeGroupCreated(true)
-	}
-
 	opts := fsAdmin.SubVolumeOptions{
 		Size: fsAdmin.ByteCount(s.Size),
 	}
@@ -271,12 +245,6 @@ func (s *subVolumeClient) CreateVolume(ctx context.Context) error {
 	if err != nil {
 		log.ErrorLog(ctx, "failed to create subvolume %s in fs %s: %s", s.VolID, s.FsName, err)
 
-		if errors.Is(err, rados.ErrNotFound) {
-			// Reset the subVolumeGroupsCreated so that we can try again to create the
-			// subvolumegroup in next request if the error is Not Found.
-			s.updateSubVolumeGroupCreated(false)
-		}
-
 		return err
 	}
 

scripts/rook.sh

@@ -5,6 +5,7 @@ ROOK_DEPLOY_TIMEOUT=${ROOK_DEPLOY_TIMEOUT:-300}
 ROOK_URL="https://raw.githubusercontent.com/rook/rook/${ROOK_VERSION}/deploy/examples"
 ROOK_BLOCK_POOL_NAME=${ROOK_BLOCK_POOL_NAME:-"newrbdpool"}
 ROOK_BLOCK_EC_POOL_NAME=${ROOK_BLOCK_EC_POOL_NAME:-"ec-pool"}
+ROOK_SUBVOLUMEGROUP_NAME=${ROOK_SUBVOLUMEGROUP_NAME:-"csi"}
 
 SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd)"
 # shellcheck disable=SC1091
@@ -53,11 +54,14 @@ function deploy_rook() {
 		cat "${TEMP_DIR}"/cluster-test.yaml
 		kubectl_retry create -f "${TEMP_DIR}/cluster-test.yaml"
 	fi
+
 	rm -rf "${TEMP_DIR}"
 
 	kubectl_retry create -f "${ROOK_URL}/toolbox.yaml"
 	kubectl_retry create -f "${ROOK_URL}/filesystem-test.yaml"
 	kubectl_retry create -f "${ROOK_URL}/pool-test.yaml"
+
+	create_or_delete_subvolumegroup "create"
 
 	# Check if CephCluster is empty
 	if ! kubectl_retry -n rook-ceph get cephclusters -oyaml | grep 'items: \[\]' &>/dev/null; then
@@ -79,6 +83,7 @@ function deploy_rook() {
 }
 
 function teardown_rook() {
+	create_or_delete_subvolumegroup "delete"
 	kubectl delete -f "${ROOK_URL}/pool-test.yaml"
 	kubectl delete -f "${ROOK_URL}/filesystem-test.yaml"
 	kubectl delete -f "${ROOK_URL}/toolbox.yaml"
@@ -88,6 +93,21 @@ function teardown_rook() {
 	kubectl delete -f "${ROOK_URL}/crds.yaml"
 }
 
+# TODO: to be removed once issue is closed - https://github.com/rook/rook/issues/13040
+function create_or_delete_subvolumegroup() {
+	local action="$1"
+	curl -o "subvolumegroup.yaml" "${ROOK_URL}/subvolumegroup.yaml"
+	sed -i "s|name:.*|name: $ROOK_SUBVOLUMEGROUP_NAME|g" subvolumegroup.yaml
+
+	if [ "$action" == "create" ]; then
+		kubectl_retry create -f subvolumegroup.yaml
+	else
+		kubectl delete -f subvolumegroup.yaml
+	fi
+
+	rm -f "subvolumegroup.yaml"
+}
+
 function create_block_pool() {
 	curl -o newpool.yaml "${ROOK_URL}/pool-test.yaml"
 	sed -i "s/replicapool/$ROOK_BLOCK_POOL_NAME/g" newpool.yaml