From c4e373c72fbf23e1fdb969c5fee656f80b464f8c Mon Sep 17 00:00:00 2001 From: Praveen M Date: Mon, 30 Oct 2023 13:53:12 +0530 Subject: [PATCH] deploy: support for read affinity options per cluster Implemented the capability to include read affinity options for individual clusters within the ceph-csi-config ConfigMap. This allows users to configure the crush location for each cluster separately. The read affinity options specified in the ConfigMap will supersede those provided via command line arguments. Signed-off-by: Praveen M --- PendingReleaseNotes.md | 5 + charts/ceph-csi-rbd/values.yaml | 5 + deploy/csi-config-map-sample.yaml | 13 +++ docs/deploy-rbd.md | 8 +- e2e/configmap.go | 10 ++ internal/rbd/driver/driver.go | 25 +++-- internal/rbd/nodeserver.go | 36 ++---- internal/rbd/nodeserver_test.go | 176 +++++++++++++++++++++--------- internal/rbd/rbd_attach.go | 10 +- internal/util/crushlocation.go | 13 +-- internal/util/csiconfig.go | 23 ++++ internal/util/csiconfig_test.go | 113 +++++++++++++++++++ 12 files changed, 338 insertions(+), 99 deletions(-) diff --git a/PendingReleaseNotes.md b/PendingReleaseNotes.md index 8a11ae3c8..3ee8d31d7 100644 --- a/PendingReleaseNotes.md +++ b/PendingReleaseNotes.md @@ -5,3 +5,8 @@ - Removed the deprecated grpc metrics flag's in [PR](https://github.com/ceph/ceph-csi/pull/4225) ## Features + +RBD + +- Support for configuring read affinity for individuals cluster within the ceph-csi-config + ConfigMap in [PR](https://github.com/ceph/ceph-csi/pull/4165) diff --git a/charts/ceph-csi-rbd/values.yaml b/charts/ceph-csi-rbd/values.yaml index 0d43b1671..64b74e2ab 100644 --- a/charts/ceph-csi-rbd/values.yaml +++ b/charts/ceph-csi-rbd/values.yaml @@ -27,6 +27,11 @@ serviceAccounts: # - "" # rbd: # netNamespaceFilePath: "{{ .kubeletDir }}/plugins/{{ .driverName }}/net" +# readAffinity: +# enabled: true +# crushLocationLabels: +# - topology.kubernetes.io/region +# - topology.kubernetes.io/zone csiConfig: [] # Configuration details of clusterID,PoolID and FscID mapping diff --git a/deploy/csi-config-map-sample.yaml b/deploy/csi-config-map-sample.yaml index b48e834a5..7f8653a58 100644 --- a/deploy/csi-config-map-sample.yaml +++ b/deploy/csi-config-map-sample.yaml @@ -32,6 +32,10 @@ kind: ConfigMap # path for the Ceph cluster identified by the , This will be used # by the RBD CSI plugin to execute the rbd map/unmap in the # network namespace specified by the "rbd.netNamespaceFilePath". +# The "readAffinity" fields are used to enable read affinity and pass the crush +# location map for the Ceph cluster identified by the cluster , +# enabling this will add +# "read_from_replica=localize,crush_location=" to the map option. # If a CSI plugin is using more than one Ceph cluster, repeat the section for # each such cluster in use. # NOTE: Changes to the configmap is automatically updated in the running pods, @@ -66,6 +70,15 @@ data: } "nfs": { "netNamespaceFilePath": "/plugins/nfs.csi.ceph.com/net", + }, + "readAffinity": { + "enabled": "false", + "crushLocationLabels": [ + "", + "" + ... + "" + ] } } ] diff --git a/docs/deploy-rbd.md b/docs/deploy-rbd.md index 9f22c1315..de76d5a1a 100644 --- a/docs/deploy-rbd.md +++ b/docs/deploy-rbd.md @@ -47,7 +47,7 @@ make image-cephcsi | `--maxsnapshotsonimage` | `450` | Maximum number of snapshots allowed on rbd image without flattening | | `--setmetadata` | `false` | Set metadata on volume | | `--enable-read-affinity` | `false` | enable read affinity | -| `--crush-location-labels`| _empty_ | Kubernetes node labels that determine the CRUSH location the node belongs to, separated by ',' | +| `--crush-location-labels`| _empty_ | Kubernetes node labels that determine the CRUSH location the node belongs to, separated by ','.
`Note: These labels will be replaced if crush location labels are defined in the ceph-csi-config ConfigMap for the specific cluster.` | **Available volume parameters:** @@ -222,6 +222,12 @@ If enabled, this option will be added to all RBD volumes mapped by Ceph CSI. Well known labels can be found [here](https://kubernetes.io/docs/reference/labels-annotations-taints/). +Read affinity can be configured for individual clusters within the +`ceph-csi-config` ConfigMap. This allows configuring the crush location labels +for each ceph cluster separately. The crush location labels specified in the +ConfigMap will supersede those provided via command line argument +`--crush-location-labels`. + >Note: Label values will have all its dots `"."` normalized with dashes `"-"` in order for it to work with ceph CRUSH map. diff --git a/e2e/configmap.go b/e2e/configmap.go index 6eb366065..a52626af7 100644 --- a/e2e/configmap.go +++ b/e2e/configmap.go @@ -63,6 +63,16 @@ func createConfigMap(pluginPath string, c kubernetes.Interface, f *framework.Fra }{ RadosNamespace: radosNamespace, }, + ReadAffinity: struct { + Enabled bool `json:"enabled"` + CrushLocationLabels []string `json:"crushLocationLabels"` + }{ + Enabled: true, + CrushLocationLabels: []string{ + crushLocationRegionLabel, + crushLocationZoneLabel, + }, + }, }} if upgradeTesting { subvolumegroup = "csi" diff --git a/internal/rbd/driver/driver.go b/internal/rbd/driver/driver.go index 728249f84..eda9f03be 100644 --- a/internal/rbd/driver/driver.go +++ b/internal/rbd/driver/driver.go @@ -26,6 +26,7 @@ import ( csicommon "github.com/ceph/ceph-csi/internal/csi-common" "github.com/ceph/ceph-csi/internal/rbd" "github.com/ceph/ceph-csi/internal/util" + "github.com/ceph/ceph-csi/internal/util/k8s" "github.com/ceph/ceph-csi/internal/util/log" "github.com/container-storage-interface/spec/lib/go/csi" @@ -68,14 +69,14 @@ func NewControllerServer(d *csicommon.CSIDriver) *rbd.ControllerServer { func NewNodeServer( d *csicommon.CSIDriver, t string, - topology map[string]string, - crushLocationMap map[string]string, + nodeLabels, topology, crushLocationMap map[string]string, ) (*rbd.NodeServer, error) { ns := rbd.NodeServer{ - DefaultNodeServer: csicommon.NewDefaultNodeServer(d, t, topology), - VolumeLocks: util.NewVolumeLocks(), + DefaultNodeServer: csicommon.NewDefaultNodeServer(d, t, topology), + VolumeLocks: util.NewVolumeLocks(), + NodeLabels: nodeLabels, + CLIReadAffinityMapOptions: util.ConstructReadAffinityMapOption(crushLocationMap), } - ns.SetReadAffinityMapOptions(crushLocationMap) return &ns, nil } @@ -87,8 +88,8 @@ func NewNodeServer( // setupCSIAddonsServer(). func (r *Driver) Run(conf *util.Config) { var ( - err error - topology, crushLocationMap map[string]string + err error + nodeLabels, topology, crushLocationMap map[string]string ) // update clone soft and hard limit rbd.SetGlobalInt("rbdHardMaxCloneDepth", conf.RbdHardMaxCloneDepth) @@ -125,13 +126,17 @@ func (r *Driver) Run(conf *util.Config) { }) } - if conf.EnableReadAffinity { - crushLocationMap, err = util.GetCrushLocationMap(conf.CrushLocationLabels, conf.NodeID) + if k8s.RunsOnKubernetes() { + nodeLabels, err = k8s.GetNodeLabels(conf.NodeID) if err != nil { log.FatalLogMsg(err.Error()) } } + if conf.EnableReadAffinity { + crushLocationMap = util.GetCrushLocationMap(conf.CrushLocationLabels, nodeLabels) + } + // Create GRPC servers r.ids = NewIdentityServer(r.cd) @@ -140,7 +145,7 @@ func (r *Driver) Run(conf *util.Config) { if err != nil { log.FatalLogMsg(err.Error()) } - r.ns, err = NewNodeServer(r.cd, conf.Vtype, topology, crushLocationMap) + r.ns, err = NewNodeServer(r.cd, conf.Vtype, nodeLabels, topology, crushLocationMap) if err != nil { log.FatalLogMsg("failed to start node server, err %v\n", err) } diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go index 1009fd25f..d7961e2bb 100644 --- a/internal/rbd/nodeserver.go +++ b/internal/rbd/nodeserver.go @@ -45,8 +45,10 @@ type NodeServer struct { // A map storing all volumes with ongoing operations so that additional operations // for that same volume (as defined by VolumeID) return an Aborted error VolumeLocks *util.VolumeLocks - // readAffinityMapOptions contains map options to enable read affinity. - readAffinityMapOptions string + // NodeLabels stores the node labels + NodeLabels map[string]string + // CLIReadAffinityMapOptions contains map options passed through command line to enable read affinity. + CLIReadAffinityMapOptions string } // stageTransaction struct represents the state a transaction was when it either completed @@ -258,11 +260,10 @@ func (ns *NodeServer) populateRbdVol( rv.Mounter = rbdNbdMounter } - err = getMapOptions(req, rv) + err = ns.getMapOptions(req, rv) if err != nil { return nil, err } - ns.appendReadAffinityMapOptions(rv) rv.VolID = volID @@ -280,14 +281,14 @@ func (ns *NodeServer) populateRbdVol( // appendReadAffinityMapOptions appends readAffinityMapOptions to mapOptions // if mounter is rbdDefaultMounter and readAffinityMapOptions is not empty. -func (ns NodeServer) appendReadAffinityMapOptions(rv *rbdVolume) { +func (rv *rbdVolume) appendReadAffinityMapOptions(readAffinityMapOptions string) { switch { - case ns.readAffinityMapOptions == "" || rv.Mounter != rbdDefaultMounter: + case readAffinityMapOptions == "" || rv.Mounter != rbdDefaultMounter: return case rv.MapOptions != "": - rv.MapOptions += "," + ns.readAffinityMapOptions + rv.MapOptions += "," + readAffinityMapOptions default: - rv.MapOptions = ns.readAffinityMapOptions + rv.MapOptions = readAffinityMapOptions } } @@ -1378,22 +1379,3 @@ func getDeviceSize(ctx context.Context, devicePath string) (uint64, error) { return size, nil } - -func (ns *NodeServer) SetReadAffinityMapOptions(crushLocationMap map[string]string) { - if len(crushLocationMap) == 0 { - return - } - - var b strings.Builder - b.WriteString("read_from_replica=localize,crush_location=") - first := true - for key, val := range crushLocationMap { - if first { - b.WriteString(fmt.Sprintf("%s:%s", key, val)) - first = false - } else { - b.WriteString(fmt.Sprintf("|%s:%s", key, val)) - } - } - ns.readAffinityMapOptions = b.String() -} diff --git a/internal/rbd/nodeserver_test.go b/internal/rbd/nodeserver_test.go index 822232c6f..bffc114dc 100644 --- a/internal/rbd/nodeserver_test.go +++ b/internal/rbd/nodeserver_test.go @@ -18,8 +18,12 @@ package rbd import ( "context" + "encoding/json" + "os" "testing" + "github.com/ceph/ceph-csi/internal/util" + "github.com/container-storage-interface/spec/lib/go/csi" "github.com/stretchr/testify/assert" ) @@ -107,53 +111,6 @@ func TestParseBoolOption(t *testing.T) { } } -func TestNodeServer_SetReadAffinityMapOptions(t *testing.T) { - t.Parallel() - tests := []struct { - name string - crushLocationmap map[string]string - wantAny []string - }{ - { - name: "nil crushLocationmap", - crushLocationmap: nil, - wantAny: []string{""}, - }, - { - name: "empty crushLocationmap", - crushLocationmap: map[string]string{}, - wantAny: []string{""}, - }, - { - name: "single entry in crushLocationmap", - crushLocationmap: map[string]string{ - "region": "east", - }, - wantAny: []string{"read_from_replica=localize,crush_location=region:east"}, - }, - { - name: "multiple entries in crushLocationmap", - crushLocationmap: map[string]string{ - "region": "east", - "zone": "east-1", - }, - wantAny: []string{ - "read_from_replica=localize,crush_location=region:east|zone:east-1", - "read_from_replica=localize,crush_location=zone:east-1|region:east", - }, - }, - } - for _, tt := range tests { - currentTT := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - ns := &NodeServer{} - ns.SetReadAffinityMapOptions(currentTT.crushLocationmap) - assert.Contains(t, currentTT.wantAny, ns.readAffinityMapOptions) - }) - } -} - func TestNodeServer_appendReadAffinityMapOptions(t *testing.T) { t.Parallel() type input struct { @@ -236,11 +193,128 @@ func TestNodeServer_appendReadAffinityMapOptions(t *testing.T) { MapOptions: currentTT.args.mapOptions, Mounter: currentTT.args.mounter, } - ns := &NodeServer{ - readAffinityMapOptions: currentTT.args.readAffinityMapOptions, - } - ns.appendReadAffinityMapOptions(rv) + rv.appendReadAffinityMapOptions(currentTT.args.readAffinityMapOptions) assert.Equal(t, currentTT.want, rv.MapOptions) }) } } + +func TestReadAffinity_GetReadAffinityMapOptions(t *testing.T) { + t.Parallel() + + nodeLabels := map[string]string{ + "topology.kubernetes.io/zone": "east-1", + "topology.kubernetes.io/region": "east", + } + + csiConfig := []util.ClusterInfo{ + { + ClusterID: "cluster-1", + ReadAffinity: struct { + Enabled bool `json:"enabled"` + CrushLocationLabels []string `json:"crushLocationLabels"` + }{ + Enabled: true, + CrushLocationLabels: []string{ + "topology.kubernetes.io/region", + }, + }, + }, + { + ClusterID: "cluster-2", + ReadAffinity: struct { + Enabled bool `json:"enabled"` + CrushLocationLabels []string `json:"crushLocationLabels"` + }{ + Enabled: false, + CrushLocationLabels: []string{ + "topology.kubernetes.io/region", + }, + }, + }, + { + ClusterID: "cluster-3", + ReadAffinity: struct { + Enabled bool `json:"enabled"` + CrushLocationLabels []string `json:"crushLocationLabels"` + }{ + Enabled: true, + CrushLocationLabels: []string{}, + }, + }, + { + ClusterID: "cluster-4", + }, + } + + csiConfigFileContent, err := json.Marshal(csiConfig) + if err != nil { + t.Errorf("failed to marshal csi config info %v", err) + } + tmpConfPath := util.CsiConfigFile + err = os.Mkdir("/etc/ceph-csi-config", 0o600) + if err != nil { + t.Errorf("failed to create directory %s: %v", "/etc/ceph-csi-config", err) + } + err = os.WriteFile(tmpConfPath, csiConfigFileContent, 0o600) + if err != nil { + t.Errorf("failed to write %s file content: %v", util.CsiConfigFile, err) + } + + tests := []struct { + name string + clusterID string + CLICrushLocationLabels string + want string + }{ + { + name: "Enabled in cluster-1 and Enabled in CLI", + clusterID: "cluster-1", + CLICrushLocationLabels: "topology.kubernetes.io/region", + want: "read_from_replica=localize,crush_location=region:east", + }, + { + name: "Disabled in cluster-2 and Enabled in CLI", + clusterID: "cluster-2", + CLICrushLocationLabels: "topology.kubernetes.io/zone", + want: "", + }, + { + name: "Enabled in cluster-3 with empty crush labels and Enabled in CLI", + clusterID: "cluster-3", + CLICrushLocationLabels: "topology.kubernetes.io/zone", + want: "read_from_replica=localize,crush_location=zone:east-1", + }, + { + name: "Enabled in cluster-3 with empty crush labels and Disabled in CLI", + clusterID: "cluster-3", + CLICrushLocationLabels: "", + want: "", + }, + { + name: "Absent in cluster-4 and Enabled in CLI", + clusterID: "cluster-4", + CLICrushLocationLabels: "topology.kubernetes.io/zone", + want: "", + }, + } + + for _, tt := range tests { + tc := tt + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + crushLocationMap := util.GetCrushLocationMap(tc.CLICrushLocationLabels, nodeLabels) + ns := &NodeServer{ + CLIReadAffinityMapOptions: util.ConstructReadAffinityMapOption(crushLocationMap), + } + readAffinityMapOptions, err := util.GetReadAffinityMapOptions( + tc.clusterID, ns.CLIReadAffinityMapOptions, nodeLabels, + ) + if err != nil { + assert.Fail(t, err.Error()) + } + + assert.Equal(t, tc.want, readAffinityMapOptions) + }) + } +} diff --git a/internal/rbd/rbd_attach.go b/internal/rbd/rbd_attach.go index c8d4701e2..37d121d79 100644 --- a/internal/rbd/rbd_attach.go +++ b/internal/rbd/rbd_attach.go @@ -295,7 +295,7 @@ func parseMapOptions(mapOptions string) (string, string, error) { // getMapOptions is a wrapper func, calls parse map/unmap funcs and feeds the // rbdVolume object. -func getMapOptions(req *csi.NodeStageVolumeRequest, rv *rbdVolume) error { +func (ns *NodeServer) getMapOptions(req *csi.NodeStageVolumeRequest, rv *rbdVolume) error { krbdMapOptions, nbdMapOptions, err := parseMapOptions(req.GetVolumeContext()["mapOptions"]) if err != nil { return err @@ -312,6 +312,14 @@ func getMapOptions(req *csi.NodeStageVolumeRequest, rv *rbdVolume) error { rv.UnmapOptions = nbdUnmapOptions } + readAffinityMapOptions, err := util.GetReadAffinityMapOptions( + rv.ClusterID, ns.CLIReadAffinityMapOptions, ns.NodeLabels, + ) + if err != nil { + return err + } + rv.appendReadAffinityMapOptions(readAffinityMapOptions) + return nil } diff --git a/internal/util/crushlocation.go b/internal/util/crushlocation.go index 5f3751c33..bc68bf4e6 100644 --- a/internal/util/crushlocation.go +++ b/internal/util/crushlocation.go @@ -23,20 +23,15 @@ import ( ) // GetCrushLocationMap returns the crush location map, determined from -// the crush location labels and their values from the CO system. +// the crush location labels and their values from the node labels passed in arg. // Expects crushLocationLabels in arg to be in the format "[prefix/],[prefix/],...",. // Returns map of crush location types with its array of associated values. -func GetCrushLocationMap(crushLocationLabels, nodeName string) (map[string]string, error) { +func GetCrushLocationMap(crushLocationLabels string, nodeLabels map[string]string) map[string]string { if crushLocationLabels == "" { - return nil, nil + return nil } - nodeLabels, err := k8sGetNodeLabels(nodeName) - if err != nil { - return nil, err - } - - return getCrushLocationMap(crushLocationLabels, nodeLabels), nil + return getCrushLocationMap(crushLocationLabels, nodeLabels) } // getCrushLocationMap returns the crush location map, determined from diff --git a/internal/util/csiconfig.go b/internal/util/csiconfig.go index b39ce241d..abacab329 100644 --- a/internal/util/csiconfig.go +++ b/internal/util/csiconfig.go @@ -62,6 +62,11 @@ type ClusterInfo struct { // symlink filepath for the network namespace where we need to execute commands. NetNamespaceFilePath string `json:"netNamespaceFilePath"` } `json:"nfs"` + // Read affinity map options + ReadAffinity struct { + Enabled bool `json:"enabled"` + CrushLocationLabels []string `json:"crushLocationLabels"` + } `json:"readAffinity"` } // Expected JSON structure in the passed in config file is, @@ -203,3 +208,21 @@ func GetNFSNetNamespaceFilePath(pathToConfig, clusterID string) (string, error) return cluster.NFS.NetNamespaceFilePath, nil } + +// GetCrushLocationLabels returns the `readAffinity.enabled` and `readAffinity.crushLocationLabels` +// values from the CSI config for the given `clusterID`. If `readAffinity.enabled` is set to true +// it returns `true` and `crushLocationLabels`, else returns `false` and an empty string. +func GetCrushLocationLabels(pathToConfig, clusterID string) (bool, string, error) { + cluster, err := readClusterInfo(pathToConfig, clusterID) + if err != nil { + return false, "", err + } + + if !cluster.ReadAffinity.Enabled { + return false, "", nil + } + + crushLocationLabels := strings.Join(cluster.ReadAffinity.CrushLocationLabels, ",") + + return true, crushLocationLabels, nil +} diff --git a/internal/util/csiconfig_test.go b/internal/util/csiconfig_test.go index 66b5c927d..40e1b4d5e 100644 --- a/internal/util/csiconfig_test.go +++ b/internal/util/csiconfig_test.go @@ -365,3 +365,116 @@ func TestGetNFSNetNamespaceFilePath(t *testing.T) { }) } } + +func TestGetReadAffinityOptions(t *testing.T) { + t.Parallel() + tests := []struct { + name string + clusterID string + want struct { + enabled bool + labels string + } + }{ + { + name: "ReadAffinity enabled set to true for cluster-1", + clusterID: "cluster-1", + want: struct { + enabled bool + labels string + }{true, "topology.kubernetes.io/region,topology.kubernetes.io/zone,topology.io/rack"}, + }, + { + name: "ReadAffinity enabled set to true for cluster-2", + clusterID: "cluster-2", + want: struct { + enabled bool + labels string + }{true, "topology.kubernetes.io/region"}, + }, + { + name: "ReadAffinity enabled set to false for cluster-3", + clusterID: "cluster-3", + want: struct { + enabled bool + labels string + }{false, ""}, + }, + { + name: "ReadAffinity option not set in cluster-4", + clusterID: "cluster-4", + want: struct { + enabled bool + labels string + }{false, ""}, + }, + } + + csiConfig := []ClusterInfo{ + { + ClusterID: "cluster-1", + ReadAffinity: struct { + Enabled bool `json:"enabled"` + CrushLocationLabels []string `json:"crushLocationLabels"` + }{ + Enabled: true, + CrushLocationLabels: []string{ + "topology.kubernetes.io/region", + "topology.kubernetes.io/zone", + "topology.io/rack", + }, + }, + }, + { + ClusterID: "cluster-2", + ReadAffinity: struct { + Enabled bool `json:"enabled"` + CrushLocationLabels []string `json:"crushLocationLabels"` + }{ + Enabled: true, + CrushLocationLabels: []string{ + "topology.kubernetes.io/region", + }, + }, + }, + { + ClusterID: "cluster-3", + ReadAffinity: struct { + Enabled bool `json:"enabled"` + CrushLocationLabels []string `json:"crushLocationLabels"` + }{ + Enabled: false, + CrushLocationLabels: []string{ + "topology.io/rack", + }, + }, + }, + { + ClusterID: "cluster-4", + }, + } + csiConfigFileContent, err := json.Marshal(csiConfig) + if err != nil { + t.Errorf("failed to marshal csi config info %v", err) + } + tmpConfPath := t.TempDir() + "/ceph-csi.json" + err = os.WriteFile(tmpConfPath, csiConfigFileContent, 0o600) + if err != nil { + t.Errorf("failed to write %s file content: %v", CsiConfigFile, err) + } + for _, tt := range tests { + tc := tt + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + enabled, labels, err := GetCrushLocationLabels(tmpConfPath, tc.clusterID) + if err != nil { + t.Errorf("GetCrushLocationLabels() error = %v", err) + + return + } + if enabled != tc.want.enabled || labels != tc.want.labels { + t.Errorf("GetCrushLocationLabels() = {%v %v} want %v", enabled, labels, tc.want) + } + }) + } +}