From d36ebf143ad3325c9422edf1cb96dcabc76c6971 Mon Sep 17 00:00:00 2001 From: parth-gr Date: Mon, 26 Feb 2024 19:29:58 +0530 Subject: [PATCH] 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: https://github.com/ceph/ceph-csi/issues/4380 Signed-off-by: parth-gr Signed-off-by: parth-gr --- .../ceph-csi-rbd/templates/storageclass.yaml | 4 +++- charts/ceph-csi-rbd/values.yaml | 1 + e2e/rbd.go | 8 +++++--- e2e/rbd_helper.go | 5 ++++- examples/rbd/storageclass.yaml | 1 + internal/rbd/controllerserver.go | 19 ++++++++++++++++--- internal/rbd/rbd_journal.go | 11 ++++++----- internal/rbd/rbd_util.go | 5 ++++- 8 files changed, 40 insertions(+), 14 deletions(-) diff --git a/charts/ceph-csi-rbd/templates/storageclass.yaml b/charts/ceph-csi-rbd/templates/storageclass.yaml index 459a6ea312ac..188362fc0503 100644 --- 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 index 7dc4d019ab17..1f071a20afec 100644 --- a/charts/ceph-csi-rbd/values.yaml +++ b/charts/ceph-csi-rbd/values.yaml @@ -321,6 +321,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 index e4c320c26d60..c8b71817b367 100644 --- 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 index 3c448dd12ada..cc56f759a3e4 100644 --- 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 index ca7fc9a7247a..601a6696af4c 100644 --- a/examples/rbd/storageclass.yaml +++ b/examples/rbd/storageclass.yaml @@ -26,6 +26,7 @@ parameters: # dataPool: # (required) Ceph pool into which the RBD image shall be created + # (optional) If the topologyConstrainedPools is provided # eg: pool: rbdpool pool: diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go index e3796b6611d2..e2d5d63a6eff 100644 --- 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, err + } + found, err := rbdVol.Exists(ctx, parentVol) if err != nil { return nil, getGRPCErrorForCreateVolume(err) diff --git a/internal/rbd/rbd_journal.go b/internal/rbd/rbd_journal.go index 55c6a033ebfd..079c1f407c04 100644 --- 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,6 +451,7 @@ func updateTopologyConstraints(rbdVol *rbdVolume, rbdSnap *rbdSnapshot) error { rbdVol.Pool = poolName rbdVol.DataPool = dataPoolName rbdVol.Topology = topology + rbdVol.JournalPool = poolName } return nil @@ -456,11 +462,6 @@ func updateTopologyConstraints(rbdVol *rbdVolume, rbdSnap *rbdSnapshot) error { func reserveVol(ctx context.Context, rbdVol *rbdVolume, rbdSnap *rbdSnapshot, 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 index 08f9f759889a..7485e0e7fed8 100644 --- 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"]