From 063319f6e516be12398adc0fa57adf00584b54e0 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 --- .../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 | 21 +++++++++++++++---- internal/rbd/rbd_journal.go | 13 ++++++------ internal/rbd/rbd_util.go | 5 ++++- 8 files changed, 42 insertions(+), 16 deletions(-) diff --git a/charts/ceph-csi-rbd/templates/storageclass.yaml b/charts/ceph-csi-rbd/templates/storageclass.yaml index 459a6ea31..188362fc0 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 20e39cd8d..0a8195e74 100644 --- 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 index e4c320c26..c8b71817b 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 3c448dd12..cc56f759a 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 ca7fc9a72..601a6696a 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 9e924710a..75197c8bb 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, 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 index 303d62f8f..f7c2f86c7 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 @@ -453,14 +459,9 @@ func updateTopologyConstraints(rbdVol *rbdVolume, rbdSnap *rbdSnapshot) error { // 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 index 401711a9b..b318d0f62 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"]