rbd: make pool optional in rbd sc if topologyconstraints are present

if rbd storage class is created with topologyconstraintspools replicated pool was still mandatory, making the pool optional if the topologyconstraintspools is requested Closes: ceph#4380 Signed-off-by: parth-gr <[email protected]>
red-hat-storage · Mar 22, 2024 · 063319f · 063319f
1 parent 5224d58
commit 063319f
Show file tree

Hide file tree

Showing 8 changed files with 42 additions and 16 deletions.
diff --git a/charts/ceph-csi-rbd/templates/storageclass.yaml b/charts/ceph-csi-rbd/templates/storageclass.yaml
@@ -16,8 +16,10 @@ metadata:
 provisioner: {{ .Values.driverName }}
 parameters:
   clusterID: {{ .Values.storageClass.clusterID }}
-  pool: {{ .Values.storageClass.pool }}
   imageFeatures: {{ .Values.storageClass.imageFeatures }}
+{{- if .Values.storageClass.pool }}  
+  pool: {{ .Values.storageClass.pool }}
+{{- end }}  
 {{- if .Values.storageClass.tryOtherMounters }}
   tryOtherMounters: {{ .Values.storageClass.tryOtherMounters | quote}}
 {{- end }}

diff --git a/charts/ceph-csi-rbd/values.yaml b/charts/ceph-csi-rbd/values.yaml
@@ -343,6 +343,7 @@ storageClass:
   dataPool: ""
 
   # (required) Ceph pool into which the RBD image shall be created
+  # (optional) if topologyConstrainedPools is provided
   # eg: pool: replicapool
   pool: replicapool
 

diff --git a/e2e/rbd.go b/e2e/rbd.go
@@ -3043,19 +3043,21 @@ var _ = Describe("RBD", func() {
 				}
 
 				By("ensuring created PV has its CSI journal in the CSI journal specific pool")
-				err = checkPVCCSIJournalInPool(f, pvc, "replicapool")
+				err = checkPVCCSIJournalInPool(f, pvc, rbdTopologyPool)
 				if err != nil {
 					framework.Failf("failed to check csi journal in pool: %v", err)
 				}
 
-				err = deleteJournalInfoInPool(f, pvc, "replicapool")
+				err = deleteJournalInfoInPool(f, pvc, rbdTopologyPool)
 				if err != nil {
 					framework.Failf("failed to delete omap data: %v", err)
 				}
+
 				err = deletePVCAndApp("", f, pvc, app)
 				if err != nil {
 					framework.Failf("failed to delete PVC and application: %v", err)
 				}
+
 				validateRBDImageCount(f, 0, defaultRBDPool)
 				validateOmapCount(f, 0, rbdType, defaultRBDPool, volumesType)
 
@@ -3092,7 +3094,7 @@ var _ = Describe("RBD", func() {
 						framework.Failf("failed to check data pool for image: %v", err)
 					}
 
-					err = deleteJournalInfoInPool(f, pvc, "replicapool")
+					err = deleteJournalInfoInPool(f, pvc, rbdTopologyPool)
 					if err != nil {
 						framework.Failf("failed to delete omap data: %v", err)
 					}

diff --git a/e2e/rbd_helper.go b/e2e/rbd_helper.go
@@ -126,7 +126,10 @@ func createRBDStorageClass(
 	if name != "" {
 		sc.Name = name
 	}
-	sc.Parameters["pool"] = defaultRBDPool
+	// add pool only if topologyConstrainedPools is not present
+	if _, ok := parameters["topologyConstrainedPools"]; !ok {
+		sc.Parameters["pool"] = defaultRBDPool
+	}
 	sc.Parameters["csi.storage.k8s.io/provisioner-secret-namespace"] = cephCSINamespace
 	sc.Parameters["csi.storage.k8s.io/provisioner-secret-name"] = rbdProvisionerSecretName
 

diff --git a/examples/rbd/storageclass.yaml b/examples/rbd/storageclass.yaml
@@ -26,6 +26,7 @@ parameters:
    # dataPool: <ec-data-pool>
 
    # (required) Ceph pool into which the RBD image shall be created
+   # (optional) If the topologyConstrainedPools is provided
    # eg: pool: rbdpool
    pool: <rbd-pool-name>
 

diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go
@@ -76,12 +76,19 @@ func (cs *ControllerServer) validateVolumeReq(ctx context.Context, req *csi.Crea
 	}
 	options := req.GetParameters()
 	if value, ok := options["clusterID"]; !ok || value == "" {
-		return status.Error(codes.InvalidArgument, "missing or empty cluster ID to provision volume from")
+		return status.Error(codes.InvalidArgument, "empty cluster ID to provision volume from")
 	}
-	if value, ok := options["pool"]; !ok || value == "" {
+	poolValue, poolOK := options["pool"]
+	topologyConstrainedPoolsValue, topologyOK := options["topologyConstrainedPools"]
+	if !poolOK {
+		if topologyOK && topologyConstrainedPoolsValue == "" {
+			return status.Error(codes.InvalidArgument, "empty pool name or topologyConstrainedPools to provision volume")
+		} else if !topologyOK {
+			return status.Error(codes.InvalidArgument, "missing or empty pool name to provision volume from")
+		}
+	} else if poolValue == "" {
 		return status.Error(codes.InvalidArgument, "missing or empty pool name to provision volume from")
 	}
-
 	if value, ok := options["dataPool"]; ok && value == "" {
 		return status.Error(codes.InvalidArgument, "empty datapool name to provision volume from")
 	}
@@ -244,6 +251,7 @@ func buildCreateVolumeResponse(req *csi.CreateVolumeRequest, rbdVol *rbdVolume)
 		VolumeContext: volumeContext,
 		ContentSource: req.GetVolumeContentSource(),
 	}
+
 	if rbdVol.Topology != nil {
 		volume.AccessibleTopology = []*csi.Topology{
 			{
@@ -341,6 +349,11 @@ func (cs *ControllerServer) CreateVolume(
 		return nil, err
 	}
 
+	err = updateTopologyConstraints(rbdVol, rbdSnap)
+	if err != nil {
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+
 	found, err := rbdVol.Exists(ctx, parentVol)
 	if err != nil {
 		return nil, getGRPCErrorForCreateVolume(err)
@@ -358,7 +371,7 @@ func (cs *ControllerServer) CreateVolume(
 		return nil, err
 	}
 
-	err = reserveVol(ctx, rbdVol, rbdSnap, cr)
+	err = reserveVol(ctx, rbdVol, cr)
 	if err != nil {
 		return nil, status.Error(codes.Internal, err.Error())
 	}

diff --git a/internal/rbd/rbd_journal.go b/internal/rbd/rbd_journal.go
@@ -76,6 +76,10 @@ func validateRbdVol(rbdVol *rbdVolume) error {
 		return err
 	}
 
+	if err = validateNonEmptyField(rbdVol.JournalPool, "JournalPool", "rbdVolume"); err != nil {
+		return err
+	}
+
 	if err = validateNonEmptyField(rbdVol.ClusterID, "ClusterID", "rbdVolume"); err != nil {
 		return err
 	}
@@ -433,6 +437,7 @@ func updateTopologyConstraints(rbdVol *rbdVolume, rbdSnap *rbdSnapshot) error {
 		if rbdVol.Topology != nil {
 			rbdVol.Pool = poolName
 			rbdVol.DataPool = dataPoolName
+			rbdVol.JournalPool = poolName
 		}
 
 		return nil
@@ -446,21 +451,17 @@ func updateTopologyConstraints(rbdVol *rbdVolume, rbdSnap *rbdSnapshot) error {
 		rbdVol.Pool = poolName
 		rbdVol.DataPool = dataPoolName
 		rbdVol.Topology = topology
+		rbdVol.JournalPool = poolName
 	}
 
 	return nil
 }
 
 // reserveVol is a helper routine to request a rbdVolume name reservation and generate the
 // volume ID for the generated name.
-func reserveVol(ctx context.Context, rbdVol *rbdVolume, rbdSnap *rbdSnapshot, cr *util.Credentials) error {
+func reserveVol(ctx context.Context, rbdVol *rbdVolume, cr *util.Credentials) error {
 	var err error
 
-	err = updateTopologyConstraints(rbdVol, rbdSnap)
-	if err != nil {
-		return err
-	}
-
 	journalPoolID, imagePoolID, err := util.GetPoolIDs(ctx, rbdVol.Monitors, rbdVol.JournalPool, rbdVol.Pool, cr)
 	if err != nil {
 		return err

diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go
@@ -1270,9 +1270,12 @@ func genVolFromVolumeOptions(
 	)
 
 	rbdVol := &rbdVolume{}
+
 	rbdVol.Pool, ok = volOptions["pool"]
 	if !ok {
-		return nil, errors.New("missing required parameter pool")
+		if _, ok = volOptions["topologyConstrainedPools"]; !ok {
+			return nil, errors.New("empty pool name or topologyConstrainedPools to provision volume")
+		}
 	}
 
 	rbdVol.DataPool = volOptions["dataPool"]