From 921a302f55fb7d2de0b5fc7aa6831c67cae4e9fb Mon Sep 17 00:00:00 2001 From: Praveen M Date: Thu, 14 Mar 2024 17:15:01 +0530 Subject: [PATCH 1/3] rbd: remove topologyConstrainedPools parameter This commit removes the `topologyConstrainedPools` parameter from PV volumeAttributes as it is not required. Signed-off-by: Praveen M --- internal/rbd/controllerserver.go | 3 +-- internal/rbd/rbd_attach.go | 3 +-- internal/rbd/rbd_util.go | 11 ----------- internal/util/topology.go | 5 ++++- internal/util/util.go | 32 ++++++++++++++++++++++++++++++++ 5 files changed, 38 insertions(+), 16 deletions(-) diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go index 29e4af59933..e82f1e894ef 100644 --- a/internal/rbd/controllerserver.go +++ b/internal/rbd/controllerserver.go @@ -240,8 +240,7 @@ func (cs *ControllerServer) parseVolCreateRequest( } func buildCreateVolumeResponse(req *csi.CreateVolumeRequest, rbdVol *rbdVolume) *csi.CreateVolumeResponse { - // remove kubernetes csi prefixed parameters. - volumeContext := k8s.RemoveCSIPrefixedParameters(req.GetParameters()) + volumeContext := util.GetVolumeContext(req.GetParameters()) volumeContext["pool"] = rbdVol.Pool volumeContext["journalPool"] = rbdVol.JournalPool volumeContext["imageName"] = rbdVol.RbdImageName diff --git a/internal/rbd/rbd_attach.go b/internal/rbd/rbd_attach.go index 54025eface8..ee17a7c7e9f 100644 --- a/internal/rbd/rbd_attach.go +++ b/internal/rbd/rbd_attach.go @@ -353,7 +353,6 @@ func attachRBDImage(ctx context.Context, volOptions *rbdVolume, device string, c } err = waitForrbdImage(ctx, backoff, volOptions) - if err != nil { return "", err } @@ -364,7 +363,7 @@ func attachRBDImage(ctx context.Context, volOptions *rbdVolume, device string, c } func appendNbdDeviceTypeAndOptions(cmdArgs []string, userOptions, cookie string) []string { - isUnmap := CheckSliceContains(cmdArgs, "unmap") + isUnmap := util.CheckSliceContains(cmdArgs, "unmap") if !isUnmap { if !strings.Contains(userOptions, useNbdNetlink) { cmdArgs = append(cmdArgs, "--"+useNbdNetlink) diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go index c50ebedd2c9..ae0ea389f60 100644 --- a/internal/rbd/rbd_util.go +++ b/internal/rbd/rbd_util.go @@ -2071,17 +2071,6 @@ func getCephClientLogFileName(id, logDir, prefix string) string { return fmt.Sprintf("%s/%s-%s.log", logDir, prefix, id) } -// CheckSliceContains checks the slice for string. -func CheckSliceContains(options []string, opt string) bool { - for _, o := range options { - if o == opt { - return true - } - } - - return false -} - // strategicActionOnLogFile act on log file based on cephLogStrategy. func strategicActionOnLogFile(ctx context.Context, logStrategy, logFile string) { var err error diff --git a/internal/util/topology.go b/internal/util/topology.go index 1f08ca6ff04..e24a05aca02 100644 --- a/internal/util/topology.go +++ b/internal/util/topology.go @@ -30,6 +30,9 @@ import ( const ( keySeparator rune = '/' labelSeparator string = "," + + // topologyPoolsParam is the parameter name used to pass topology constrained pools. + topologyPoolsParam = "topologyConstrainedPools" ) // GetTopologyFromDomainLabels returns the CSI topology map, determined from @@ -129,7 +132,7 @@ func GetTopologyFromRequest( var topologyPools []TopologyConstrainedPool // check if parameters have pool configuration pertaining to topology - topologyPoolsStr := req.GetParameters()["topologyConstrainedPools"] + topologyPoolsStr := req.GetParameters()[topologyPoolsParam] if topologyPoolsStr == "" { return nil, nil, nil } diff --git a/internal/util/util.go b/internal/util/util.go index 76607f9db49..83a470e251e 100644 --- a/internal/util/util.go +++ b/internal/util/util.go @@ -27,6 +27,7 @@ import ( "strings" "time" + "github.com/ceph/ceph-csi/internal/util/k8s" "github.com/ceph/ceph-csi/internal/util/log" "golang.org/x/sys/unix" @@ -381,3 +382,34 @@ func CallStack() string { return string(stack) } + +// CheckSliceContains checks the slice for string. +func CheckSliceContains(options []string, opt string) bool { + for _, o := range options { + if o == opt { + return true + } + } + + return false +} + +// GetVolumeContext filters out parameters that are not required in volume context. +func GetVolumeContext(parameters map[string]string) map[string]string { + volumeContext := map[string]string{} + + // parameters that are not required in the volume context + notRequiredParams := []string{ + topologyPoolsParam, + } + for k, v := range parameters { + if !CheckSliceContains(notRequiredParams, k) { + volumeContext[k] = v + } + } + + // remove kubernetes csi prefixed parameters. + volumeContext = k8s.RemoveCSIPrefixedParameters(volumeContext) + + return volumeContext +} From 8d318d20f3b55d349e2c80ee92d770acb22ff00f Mon Sep 17 00:00:00 2001 From: Praveen M Date: Wed, 27 Mar 2024 16:48:56 +0530 Subject: [PATCH 2/3] cephfs: refactor code for improved reusability Signed-off-by: Praveen M --- internal/cephfs/controllerserver.go | 64 ++++++++++++----------------- 1 file changed, 27 insertions(+), 37 deletions(-) diff --git a/internal/cephfs/controllerserver.go b/internal/cephfs/controllerserver.go index e6de82ea541..28e3837e2be 100644 --- a/internal/cephfs/controllerserver.go +++ b/internal/cephfs/controllerserver.go @@ -256,6 +256,31 @@ func checkValidCreateVolumeRequest( return nil } +func buildCreateVolumeResponse( + req *csi.CreateVolumeRequest, + volOptions *store.VolumeOptions, + vID *store.VolumeIdentifier, +) *csi.CreateVolumeResponse { + volumeContext := util.GetVolumeContext(req.GetParameters()) + volumeContext["subvolumeName"] = vID.FsSubvolName + volumeContext["subvolumePath"] = volOptions.RootPath + volume := &csi.Volume{ + VolumeId: vID.VolumeID, + CapacityBytes: volOptions.Size, + ContentSource: req.GetVolumeContentSource(), + VolumeContext: volumeContext, + } + if volOptions.Topology != nil { + volume.AccessibleTopology = []*csi.Topology{ + { + Segments: volOptions.Topology, + }, + } + } + + return &csi.CreateVolumeResponse{Volume: volume} +} + // CreateVolume creates a reservation and the volume in backend, if it is not already present. // //nolint:gocognit,gocyclo,nestif,cyclop // TODO: reduce complexity @@ -376,25 +401,7 @@ func (cs *ControllerServer) CreateVolume( } } - // remove kubernetes csi prefixed parameters. - volumeContext := k8s.RemoveCSIPrefixedParameters(req.GetParameters()) - volumeContext["subvolumeName"] = vID.FsSubvolName - volumeContext["subvolumePath"] = volOptions.RootPath - volume := &csi.Volume{ - VolumeId: vID.VolumeID, - CapacityBytes: volOptions.Size, - ContentSource: req.GetVolumeContentSource(), - VolumeContext: volumeContext, - } - if volOptions.Topology != nil { - volume.AccessibleTopology = []*csi.Topology{ - { - Segments: volOptions.Topology, - }, - } - } - - return &csi.CreateVolumeResponse{Volume: volume}, nil + return buildCreateVolumeResponse(req, volOptions, vID), nil } // Reservation @@ -467,25 +474,8 @@ func (cs *ControllerServer) CreateVolume( log.DebugLog(ctx, "cephfs: successfully created backing volume named %s for request name %s", vID.FsSubvolName, requestName) - // remove kubernetes csi prefixed parameters. - volumeContext := k8s.RemoveCSIPrefixedParameters(req.GetParameters()) - volumeContext["subvolumeName"] = vID.FsSubvolName - volumeContext["subvolumePath"] = volOptions.RootPath - volume := &csi.Volume{ - VolumeId: vID.VolumeID, - CapacityBytes: volOptions.Size, - ContentSource: req.GetVolumeContentSource(), - VolumeContext: volumeContext, - } - if volOptions.Topology != nil { - volume.AccessibleTopology = []*csi.Topology{ - { - Segments: volOptions.Topology, - }, - } - } - return &csi.CreateVolumeResponse{Volume: volume}, nil + return buildCreateVolumeResponse(req, volOptions, vID), nil } // DeleteVolume deletes the volume in backend and its reservation. From da9ef002f5771adeb8bfc5ab762a44a6e88f757d Mon Sep 17 00:00:00 2001 From: Praveen M Date: Mon, 1 Apr 2024 09:02:41 +0530 Subject: [PATCH 3/3] cleanup: use slices package This commit replaces the user implemented function `CheckSliceContains()` with `slices.Contains()` function introduced in Go 1.21. Signed-off-by: Praveen M --- internal/rbd/rbd_attach.go | 3 ++- internal/util/util.go | 13 +------------ 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/internal/rbd/rbd_attach.go b/internal/rbd/rbd_attach.go index ee17a7c7e9f..fbac9a3f767 100644 --- a/internal/rbd/rbd_attach.go +++ b/internal/rbd/rbd_attach.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "os" + "slices" "strings" "time" @@ -363,7 +364,7 @@ func attachRBDImage(ctx context.Context, volOptions *rbdVolume, device string, c } func appendNbdDeviceTypeAndOptions(cmdArgs []string, userOptions, cookie string) []string { - isUnmap := util.CheckSliceContains(cmdArgs, "unmap") + isUnmap := slices.Contains(cmdArgs, "unmap") if !isUnmap { if !strings.Contains(userOptions, useNbdNetlink) { cmdArgs = append(cmdArgs, "--"+useNbdNetlink) diff --git a/internal/util/util.go b/internal/util/util.go index 83a470e251e..65909a2c106 100644 --- a/internal/util/util.go +++ b/internal/util/util.go @@ -383,17 +383,6 @@ func CallStack() string { return string(stack) } -// CheckSliceContains checks the slice for string. -func CheckSliceContains(options []string, opt string) bool { - for _, o := range options { - if o == opt { - return true - } - } - - return false -} - // GetVolumeContext filters out parameters that are not required in volume context. func GetVolumeContext(parameters map[string]string) map[string]string { volumeContext := map[string]string{} @@ -403,7 +392,7 @@ func GetVolumeContext(parameters map[string]string) map[string]string { topologyPoolsParam, } for k, v := range parameters { - if !CheckSliceContains(notRequiredParams, k) { + if !slices.Contains(notRequiredParams, k) { volumeContext[k] = v } }