From 1284855f495f5d8bbeb36fa6c5c5898e0bdf70e1 Mon Sep 17 00:00:00 2001 From: alankar_verma Date: Wed, 19 Jul 2023 07:04:55 -0400 Subject: [PATCH 1/5] Support or volume limits feature --- go.mod | 4 +- go.sum | 4 ++ helm/csi-powerstore/templates/node.yaml | 2 + helm/csi-powerstore/values.yaml | 7 ++++ pkg/common/envvars.go | 6 +++ pkg/common/k8sutils/k8sutils.go | 56 +++++++++++++++++++++++++ pkg/node/base.go | 13 ++++++ pkg/node/node.go | 51 ++++++++++++++++++++++ 8 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 pkg/common/k8sutils/k8sutils.go diff --git a/go.mod b/go.mod index 211ee660..ee427c4b 100644 --- a/go.mod +++ b/go.mod @@ -35,6 +35,7 @@ require ( golang.org/x/net v0.10.0 google.golang.org/grpc v1.55.0 gopkg.in/yaml.v3 v3.0.1 + k8s.io/client-go v0.26.1 ) require ( @@ -53,10 +54,12 @@ require ( github.com/go-openapi/jsonreference v0.20.0 // indirect github.com/go-openapi/swag v0.19.14 // indirect github.com/gogo/protobuf v1.3.2 // indirect + github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/google/gnostic v0.5.7-v3refs // indirect github.com/google/go-cmp v0.5.9 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/hashicorp/hcl v1.0.0 // indirect + github.com/imdario/mergo v0.3.6 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect github.com/magiconair/properties v1.8.6 // indirect @@ -104,7 +107,6 @@ require ( gopkg.in/yaml.v2 v2.4.0 // indirect k8s.io/api v0.26.1 // indirect k8s.io/apimachinery v0.26.1 // indirect - k8s.io/client-go v0.26.1 // indirect k8s.io/component-base v0.24.1 // indirect k8s.io/klog/v2 v2.80.1 // indirect k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280 // indirect diff --git a/go.sum b/go.sum index 4fe5ca2d..b6dfd391 100644 --- a/go.sum +++ b/go.sum @@ -159,6 +159,7 @@ github.com/envoyproxy/go-control-plane v0.9.9-0.20210217033140-668b12f5399d/go.m github.com/envoyproxy/go-control-plane v0.9.9-0.20210512163311-63b5d3c536b0/go.mod h1:hliV/p42l8fGbc6Y9bQ70uLwIvmJyVE5k4iMKlh8wCQ= github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= github.com/evanphx/json-patch v4.11.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= +github.com/evanphx/json-patch v4.12.0+incompatible h1:4onqiflcdA9EOZ4RxV643DvftH5pOlLGNtQ5lPWQu84= github.com/evanphx/json-patch v4.12.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4= github.com/felixge/httpsnoop v1.0.1/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U= @@ -217,6 +218,7 @@ github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef/go.mod h1:cIg4er github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/groupcache v0.0.0-20191227052852-215e87163ea7/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= +github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da h1:oI5xCqsCo564l8iNU+DwB5epxmsaqB+rhGL0m5jtYqE= github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= github.com/golang/mock v1.2.0/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= @@ -330,6 +332,8 @@ github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpO github.com/ianlancetaylor/demangle v0.0.0-20181102032728-5e5cf60278f6/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc= github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc= github.com/imdario/mergo v0.3.5/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA= +github.com/imdario/mergo v0.3.6 h1:xTNEAn+kxVO7dTZGu0CegyqKZmoWFI0rF8UxjlB2d28= +github.com/imdario/mergo v0.3.6/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA= github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8= github.com/jarcoal/httpmock v1.2.0 h1:gSvTxxFR/MEMfsGrvRbdfpRUMBStovlSRLw0Ep1bwwc= github.com/jonboulle/clockwork v0.1.0/go.mod h1:Ii8DK3G1RaLaWxj9trq07+26W01tbo22gdxWY5EU2bo= diff --git a/helm/csi-powerstore/templates/node.yaml b/helm/csi-powerstore/templates/node.yaml index eb0ce7f0..8236466e 100644 --- a/helm/csi-powerstore/templates/node.yaml +++ b/helm/csi-powerstore/templates/node.yaml @@ -193,6 +193,8 @@ spec: value: {{ .Values.node.nodeNamePrefix }} - name: X_CSI_POWERSTORE_NODE_ID_PATH value: /node-id + - name: X_CSI_POWERSTORE_MAX_VOLUMES_PER_NODE + value: "{{ .Values.maxPowerstoreVolumesPerNode }}" - name: X_CSI_POWERSTORE_NODE_CHROOT_PATH value: /noderoot - name: X_CSI_POWERSTORE_TMP_DIR diff --git a/helm/csi-powerstore/values.yaml b/helm/csi-powerstore/values.yaml index 0d4686b1..0250f7b4 100644 --- a/helm/csi-powerstore/values.yaml +++ b/helm/csi-powerstore/values.yaml @@ -53,6 +53,13 @@ externalAccess: # Default value: None imagePullPolicy: IfNotPresent +# maxPowerstoreVolumesPerNode: Specify default value for maximum number of volumes that controller can publish to the node. +# If value is zero CO SHALL decide how many volumes of this type can be published by the controller to the node. +# This limit is applicable to all the nodes in the cluster for which node label 'max-powerstore-volumes-per-node' is not set. +# Allowed values: n, where n >= 0 +# Default value: 0 +maxPowerstoreVolumesPerNode: 0 + # nfsAcls: enables setting permissions on NFS mount directory # This value acts as default value for NFS ACL (nfsAcls), if not specified for an array config in secret # Permissions can be specified in two formats: diff --git a/pkg/common/envvars.go b/pkg/common/envvars.go index 7b8f471d..f556b48b 100644 --- a/pkg/common/envvars.go +++ b/pkg/common/envvars.go @@ -30,10 +30,16 @@ const ( // node name EnvKubeNodeName = "X_CSI_POWERSTORE_KUBE_NODE_NAME" + //EnvKubeConfigPath indicates kubernetes configuration path that has to be used by CSI Driver + EnvKubeConfigPath = "KUBECONFIG" + // EnvNodeNamePrefix is the name of the environment variable which stores prefix which will be // used when registering node on PowerStore array EnvNodeNamePrefix = "X_CSI_POWERSTORE_NODE_NAME_PREFIX" + // EnvMaxVolumesPerNode specifies maximum number of volumes that controller can publish to the node + EnvMaxVolumesPerNode = "X_CSI_POWERSTORE_MAX_VOLUMES_PER_NODE" + // EnvNodeChrootPath is the name of the environment variable which store path to chroot where // to execute iSCSI commands EnvNodeChrootPath = "X_CSI_POWERSTORE_NODE_CHROOT_PATH" diff --git a/pkg/common/k8sutils/k8sutils.go b/pkg/common/k8sutils/k8sutils.go new file mode 100644 index 00000000..b1082e51 --- /dev/null +++ b/pkg/common/k8sutils/k8sutils.go @@ -0,0 +1,56 @@ +package k8sutils + +/* + Copyright (c) 2020-2022 Dell Inc, or its subsidiaries. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +import ( + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/clientcmd" +) + +type leaderElection interface { + Run() error + WithNamespace(namespace string) +} + +// CreateKubeClientSet - Returns kubeclient set +func CreateKubeClientSet(kubeconfig string) (*kubernetes.Clientset, error) { + var clientset *kubernetes.Clientset + if kubeconfig != "" { + // use the current context in kubeconfig + config, err := clientcmd.BuildConfigFromFlags("", kubeconfig) + if err != nil { + return nil, err + } + // create the clientset + clientset, err = kubernetes.NewForConfig(config) + if err != nil { + return nil, err + } + } else { + config, err := rest.InClusterConfig() + if err != nil { + return nil, err + } + // creates the clientset + clientset, err = kubernetes.NewForConfig(config) + if err != nil { + return nil, err + } + } + return clientset, nil +} diff --git a/pkg/node/base.go b/pkg/node/base.go index d034bf5a..fdc61b3d 100644 --- a/pkg/node/base.go +++ b/pkg/node/base.go @@ -87,6 +87,10 @@ func getNodeOptions() Opts { opts.NodeIDFilePath = path } + if kubeConfigPath, ok := csictx.LookupEnv(ctx, common.EnvKubeConfigPath); ok { + opts.KubeConfigPath = kubeConfigPath + } + if prefix, ok := csictx.LookupEnv(ctx, common.EnvNodeNamePrefix); ok { opts.NodeNamePrefix = prefix } @@ -107,6 +111,15 @@ func getNodeOptions() Opts { opts.NodeChrootPath = defaultNodeChrootPath } + if maxVolumesPerNodeStr, ok := csictx.LookupEnv(ctx, common.EnvMaxVolumesPerNode); ok { + maxVolumesPerNode, err := strconv.ParseInt(maxVolumesPerNodeStr, 10, 64) + if err != nil { + opts.MaxVolumesPerNode = 0 + } else { + opts.MaxVolumesPerNode = maxVolumesPerNode + } + } + if tmpDir, ok := csictx.LookupEnv(ctx, common.EnvTmpDir); ok { opts.TmpDir = tmpDir } diff --git a/pkg/node/node.go b/pkg/node/node.go index eb89a069..bbf9e83b 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -33,11 +33,13 @@ import ( "strings" "github.com/dell/gonvme" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/container-storage-interface/spec/lib/go/csi" "github.com/dell/csi-powerstore/v2/pkg/array" "github.com/dell/csi-powerstore/v2/pkg/common" "github.com/dell/csi-powerstore/v2/pkg/common/fs" + "github.com/dell/csi-powerstore/v2/pkg/common/k8sutils" "github.com/dell/csi-powerstore/v2/pkg/controller" "github.com/dell/gobrick" csictx "github.com/dell/gocsi/context" @@ -55,8 +57,10 @@ type Opts struct { NodeIDFilePath string NodeNamePrefix string NodeChrootPath string + MaxVolumesPerNode int64 FCPortsFilterFilePath string KubeNodeName string + KubeConfigPath string CHAPUsername string CHAPPassword string TmpDir string @@ -1027,6 +1031,24 @@ func (s *Service) NodeGetCapabilities(context context.Context, request *csi.Node }, nil } +// GetNodeLabels returns labels present in the node +func (s *Service) GetNodeLabels(ctx context.Context) (map[string]string, error) { + k8sclientset, err := k8sutils.CreateKubeClientSet(s.opts.KubeConfigPath) + if err != nil { + log.Errorf("init client failed: '%s'", err.Error()) + return nil, err + } + // access the API to fetch node object + node, err := k8sclientset.CoreV1().Nodes().Get(ctx, s.opts.KubeNodeName, v1.GetOptions{}) + if err != nil { + log.Errorf("getting node details failed: '%s'", err.Error()) + return nil, err + } + log.Debugf("Node labels: %v\n", node.Labels) + + return node.Labels, nil +} + // NodeGetInfo returns id of the node and topology constraints func (s *Service) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoRequest) (*csi.NodeGetInfoResponse, error) { // Create the topology keys @@ -1206,6 +1228,35 @@ func (s *Service) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoRequest) } } } + + // Check for node label 'max-powerstore-volumes-per-node'. If present set 'maxVolumesPerNode' to this value. + // If node label is not present, set 'maxVolumesPerNode' to default value i.e., 0 + var maxVolumesPerNode int64 = 0 + + labels, err := s.GetNodeLabels(ctx) + if err != nil { + log.Error("failed to get Node Labels with error", err.Error()) + return nil, err + } + + if val, ok := labels["max-powerstore-volumes-per-node"]; ok { + maxVols, err := strconv.ParseInt(val, 10, 64) + if err != nil { + return nil, fmt.Errorf("invalid value '%s' specified for 'max-powerstore-volumes-per-node' node label", val) + } + if maxVols > 0 { + maxVolumesPerNode = maxVols + } + log.Infof("node label 'max-powerstore-volumes-per-node' is available and is set to value '%d'", maxVolumesPerNode) + } else { + if s.opts.MaxVolumesPerNode > 0 { + maxVolumesPerNode = s.opts.MaxVolumesPerNode + } + log.Infof("node label 'max-powerstore-volumes-per-node' is not available. Using default volume limit '%v'", maxVolumesPerNode) + } + + resp.MaxVolumesPerNode = maxVolumesPerNode + return resp, nil } From e7e70ee6a6b749b66a74ced72429f7aa7fbfb840 Mon Sep 17 00:00:00 2001 From: alankar_verma Date: Wed, 19 Jul 2023 07:21:57 -0400 Subject: [PATCH 2/5] Support for volume limits feature --- go.mod | 3 +-- go.sum | 2 -- pkg/common/k8sutils/k8sutils.go | 5 ----- 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/go.mod b/go.mod index ee427c4b..8c6f60ff 100644 --- a/go.mod +++ b/go.mod @@ -35,6 +35,7 @@ require ( golang.org/x/net v0.10.0 google.golang.org/grpc v1.55.0 gopkg.in/yaml.v3 v3.0.1 + k8s.io/apimachinery v0.26.1 k8s.io/client-go v0.26.1 ) @@ -54,7 +55,6 @@ require ( github.com/go-openapi/jsonreference v0.20.0 // indirect github.com/go-openapi/swag v0.19.14 // indirect github.com/gogo/protobuf v1.3.2 // indirect - github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/google/gnostic v0.5.7-v3refs // indirect github.com/google/go-cmp v0.5.9 // indirect github.com/google/gofuzz v1.2.0 // indirect @@ -106,7 +106,6 @@ require ( gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect k8s.io/api v0.26.1 // indirect - k8s.io/apimachinery v0.26.1 // indirect k8s.io/component-base v0.24.1 // indirect k8s.io/klog/v2 v2.80.1 // indirect k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280 // indirect diff --git a/go.sum b/go.sum index b6dfd391..68995844 100644 --- a/go.sum +++ b/go.sum @@ -159,7 +159,6 @@ github.com/envoyproxy/go-control-plane v0.9.9-0.20210217033140-668b12f5399d/go.m github.com/envoyproxy/go-control-plane v0.9.9-0.20210512163311-63b5d3c536b0/go.mod h1:hliV/p42l8fGbc6Y9bQ70uLwIvmJyVE5k4iMKlh8wCQ= github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= github.com/evanphx/json-patch v4.11.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= -github.com/evanphx/json-patch v4.12.0+incompatible h1:4onqiflcdA9EOZ4RxV643DvftH5pOlLGNtQ5lPWQu84= github.com/evanphx/json-patch v4.12.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4= github.com/felixge/httpsnoop v1.0.1/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U= @@ -218,7 +217,6 @@ github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef/go.mod h1:cIg4er github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/groupcache v0.0.0-20191227052852-215e87163ea7/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= -github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da h1:oI5xCqsCo564l8iNU+DwB5epxmsaqB+rhGL0m5jtYqE= github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= github.com/golang/mock v1.2.0/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= diff --git a/pkg/common/k8sutils/k8sutils.go b/pkg/common/k8sutils/k8sutils.go index b1082e51..912e074c 100644 --- a/pkg/common/k8sutils/k8sutils.go +++ b/pkg/common/k8sutils/k8sutils.go @@ -22,11 +22,6 @@ import ( "k8s.io/client-go/tools/clientcmd" ) -type leaderElection interface { - Run() error - WithNamespace(namespace string) -} - // CreateKubeClientSet - Returns kubeclient set func CreateKubeClientSet(kubeconfig string) (*kubernetes.Clientset, error) { var clientset *kubernetes.Clientset From 4738a6f6ca6be3df76dd447e195b9f06292a0647 Mon Sep 17 00:00:00 2001 From: alankar_verma Date: Mon, 24 Jul 2023 06:48:55 -0400 Subject: [PATCH 3/5] Volume limits support for csi-powerstore --- mocks/NodeLabelsRetriever.go | 152 +++++++++++++++++++++++++++ pkg/common/k8sutils/k8sutils.go | 72 +++++++++++-- pkg/node/node.go | 28 ++--- pkg/node/node_test.go | 175 ++++++++++++++++++++++++++++++-- 4 files changed, 386 insertions(+), 41 deletions(-) create mode 100644 mocks/NodeLabelsRetriever.go diff --git a/mocks/NodeLabelsRetriever.go b/mocks/NodeLabelsRetriever.go new file mode 100644 index 00000000..6f367573 --- /dev/null +++ b/mocks/NodeLabelsRetriever.go @@ -0,0 +1,152 @@ +/* + Copyright (c) 2021-2023 Dell Inc, or its subsidiaries. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +// Code generated by mockery. DO NOT EDIT. + +package mocks + +import ( + context "context" + + kubernetes "k8s.io/client-go/kubernetes" + + mock "github.com/stretchr/testify/mock" + + rest "k8s.io/client-go/rest" +) + +// NodeLabelsRetrieverInterface is an autogenerated mock type for the NodeLabelsRetrieverInterface type +type NodeLabelsRetrieverInterface struct { + mock.Mock +} + +// BuildConfigFromFlags provides a mock function with given fields: masterUrl, kubeconfig +func (_m *NodeLabelsRetrieverInterface) BuildConfigFromFlags(masterUrl string, kubeconfig string) (*rest.Config, error) { + ret := _m.Called(masterUrl, kubeconfig) + + var r0 *rest.Config + var r1 error + if rf, ok := ret.Get(0).(func(string, string) (*rest.Config, error)); ok { + return rf(masterUrl, kubeconfig) + } + if rf, ok := ret.Get(0).(func(string, string) *rest.Config); ok { + r0 = rf(masterUrl, kubeconfig) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*rest.Config) + } + } + + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(masterUrl, kubeconfig) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetNodeLabels provides a mock function with given fields: k8sclientset, ctx, kubeNodeName +func (_m *NodeLabelsRetrieverInterface) GetNodeLabels(k8sclientset *kubernetes.Clientset, ctx context.Context, kubeNodeName string) (map[string]string, error) { + ret := _m.Called(k8sclientset, ctx, kubeNodeName) + + var r0 map[string]string + var r1 error + if rf, ok := ret.Get(0).(func(*kubernetes.Clientset, context.Context, string) (map[string]string, error)); ok { + return rf(k8sclientset, ctx, kubeNodeName) + } + if rf, ok := ret.Get(0).(func(*kubernetes.Clientset, context.Context, string) map[string]string); ok { + r0 = rf(k8sclientset, ctx, kubeNodeName) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(map[string]string) + } + } + + if rf, ok := ret.Get(1).(func(*kubernetes.Clientset, context.Context, string) error); ok { + r1 = rf(k8sclientset, ctx, kubeNodeName) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// InClusterConfig provides a mock function with given fields: +func (_m *NodeLabelsRetrieverInterface) InClusterConfig() (*rest.Config, error) { + ret := _m.Called() + + var r0 *rest.Config + var r1 error + if rf, ok := ret.Get(0).(func() (*rest.Config, error)); ok { + return rf() + } + if rf, ok := ret.Get(0).(func() *rest.Config); ok { + r0 = rf() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*rest.Config) + } + } + + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// NewForConfig provides a mock function with given fields: config +func (_m *NodeLabelsRetrieverInterface) NewForConfig(config *rest.Config) (*kubernetes.Clientset, error) { + ret := _m.Called(config) + + var r0 *kubernetes.Clientset + var r1 error + if rf, ok := ret.Get(0).(func(*rest.Config) (*kubernetes.Clientset, error)); ok { + return rf(config) + } + if rf, ok := ret.Get(0).(func(*rest.Config) *kubernetes.Clientset); ok { + r0 = rf(config) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*kubernetes.Clientset) + } + } + + if rf, ok := ret.Get(1).(func(*rest.Config) error); ok { + r1 = rf(config) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// NewNodeLabelsRetrieverInterface creates a new instance of NodeLabelsRetrieverInterface. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewNodeLabelsRetrieverInterface(t interface { + mock.TestingT + Cleanup(func()) +}) *NodeLabelsRetrieverInterface { + mock := &NodeLabelsRetrieverInterface{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/common/k8sutils/k8sutils.go b/pkg/common/k8sutils/k8sutils.go index 912e074c..0f36a716 100644 --- a/pkg/common/k8sutils/k8sutils.go +++ b/pkg/common/k8sutils/k8sutils.go @@ -1,7 +1,5 @@ -package k8sutils - /* - Copyright (c) 2020-2022 Dell Inc, or its subsidiaries. + Copyright (c) 2021-2023 Dell Inc, or its subsidiaries. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -16,36 +14,92 @@ package k8sutils limitations under the License. */ +package k8sutils + import ( + "context" + + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" ) -// CreateKubeClientSet - Returns kubeclient set +// NodeLabelsRetrieverInterface defines the methods for retrieving Kubernetes Node Labels +type NodeLabelsRetrieverInterface interface { + BuildConfigFromFlags(masterUrl, kubeconfig string) (*rest.Config, error) + InClusterConfig() (*rest.Config, error) + NewForConfig(config *rest.Config) (*kubernetes.Clientset, error) + GetNodeLabels(k8sclientset *kubernetes.Clientset, ctx context.Context, kubeNodeName string) (map[string]string, error) +} + +// NodeLabelsRetrieverImpl provided the implementation for NodeLabelsRetrieverInterface +type NodeLabelsRetrieverImpl struct{} + +var NodeLabelsRetriever NodeLabelsRetrieverInterface + +func init() { + NodeLabelsRetriever = new(NodeLabelsRetrieverImpl) +} + +func (svc *NodeLabelsRetrieverImpl) BuildConfigFromFlags(masterUrl, kubeconfig string) (*rest.Config, error) { + return clientcmd.BuildConfigFromFlags(masterUrl, kubeconfig) +} + +func (svc *NodeLabelsRetrieverImpl) InClusterConfig() (*rest.Config, error) { + return rest.InClusterConfig() +} + +func (svc *NodeLabelsRetrieverImpl) NewForConfig(config *rest.Config) (*kubernetes.Clientset, error) { + return kubernetes.NewForConfig(config) +} + +func (svc *NodeLabelsRetrieverImpl) GetNodeLabels(k8sclientset *kubernetes.Clientset, ctx context.Context, kubeNodeName string) (map[string]string, error) { + if k8sclientset != nil { + node, err := k8sclientset.CoreV1().Nodes().Get(ctx, kubeNodeName, v1.GetOptions{}) + if err != nil { + return nil, err + } + + return node.Labels, nil + } + + return nil, nil +} + +// CreateKubeClientSet creates and returns kubeclient set func CreateKubeClientSet(kubeconfig string) (*kubernetes.Clientset, error) { var clientset *kubernetes.Clientset if kubeconfig != "" { - // use the current context in kubeconfig - config, err := clientcmd.BuildConfigFromFlags("", kubeconfig) + config, err := NodeLabelsRetriever.BuildConfigFromFlags("", kubeconfig) if err != nil { return nil, err } // create the clientset - clientset, err = kubernetes.NewForConfig(config) + clientset, err = NodeLabelsRetriever.NewForConfig(config) if err != nil { return nil, err } } else { - config, err := rest.InClusterConfig() + config, err := NodeLabelsRetriever.InClusterConfig() if err != nil { return nil, err } // creates the clientset - clientset, err = kubernetes.NewForConfig(config) + clientset, err = NodeLabelsRetriever.NewForConfig(config) if err != nil { return nil, err } } return clientset, nil } + +// GetNodeLabels returns labels present in the k8s node +func GetNodeLabels(ctx context.Context, kubeConfigPath string, kubeNodeName string) (map[string]string, error) { + k8sclientset, err := CreateKubeClientSet(kubeConfigPath) + if err != nil { + return nil, err + } + + return NodeLabelsRetriever.GetNodeLabels(k8sclientset, ctx, kubeNodeName) +} diff --git a/pkg/node/node.go b/pkg/node/node.go index bbf9e83b..3307fe41 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -33,7 +33,6 @@ import ( "strings" "github.com/dell/gonvme" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/container-storage-interface/spec/lib/go/csi" "github.com/dell/csi-powerstore/v2/pkg/array" @@ -93,6 +92,10 @@ type Service struct { array.Locker } +const ( + maxPowerstoreVolumesPerNodeLabel = "max-powerstore-volumes-per-node" +) + // Init initializes node service by parsing environmental variables, connecting it as a host. // Will init ISCSIConnector, FcConnector and ControllerService if they are nil. func (s *Service) Init() error { @@ -1031,24 +1034,6 @@ func (s *Service) NodeGetCapabilities(context context.Context, request *csi.Node }, nil } -// GetNodeLabels returns labels present in the node -func (s *Service) GetNodeLabels(ctx context.Context) (map[string]string, error) { - k8sclientset, err := k8sutils.CreateKubeClientSet(s.opts.KubeConfigPath) - if err != nil { - log.Errorf("init client failed: '%s'", err.Error()) - return nil, err - } - // access the API to fetch node object - node, err := k8sclientset.CoreV1().Nodes().Get(ctx, s.opts.KubeNodeName, v1.GetOptions{}) - if err != nil { - log.Errorf("getting node details failed: '%s'", err.Error()) - return nil, err - } - log.Debugf("Node labels: %v\n", node.Labels) - - return node.Labels, nil -} - // NodeGetInfo returns id of the node and topology constraints func (s *Service) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoRequest) (*csi.NodeGetInfoResponse, error) { // Create the topology keys @@ -1233,13 +1218,13 @@ func (s *Service) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoRequest) // If node label is not present, set 'maxVolumesPerNode' to default value i.e., 0 var maxVolumesPerNode int64 = 0 - labels, err := s.GetNodeLabels(ctx) + labels, err := k8sutils.GetNodeLabels(ctx, s.opts.KubeConfigPath, s.opts.KubeNodeName) if err != nil { log.Error("failed to get Node Labels with error", err.Error()) return nil, err } - if val, ok := labels["max-powerstore-volumes-per-node"]; ok { + if val, ok := labels[maxPowerstoreVolumesPerNodeLabel]; ok { maxVols, err := strconv.ParseInt(val, 10, 64) if err != nil { return nil, fmt.Errorf("invalid value '%s' specified for 'max-powerstore-volumes-per-node' node label", val) @@ -1256,7 +1241,6 @@ func (s *Service) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoRequest) } resp.MaxVolumesPerNode = maxVolumesPerNode - return resp, nil } diff --git a/pkg/node/node_test.go b/pkg/node/node_test.go index aefac046..748df099 100644 --- a/pkg/node/node_test.go +++ b/pkg/node/node_test.go @@ -32,6 +32,7 @@ import ( "github.com/dell/csi-powerstore/v2/mocks" "github.com/dell/csi-powerstore/v2/pkg/array" "github.com/dell/csi-powerstore/v2/pkg/common" + "github.com/dell/csi-powerstore/v2/pkg/common/k8sutils" "github.com/dell/csi-powerstore/v2/pkg/controller" "github.com/dell/gobrick" csictx "github.com/dell/gocsi/context" @@ -48,16 +49,17 @@ import ( ) var ( - iscsiConnectorMock *mocks.ISCSIConnector - nvmeConnectorMock *mocks.NVMEConnector - fcConnectorMock *mocks.FcConnector - utilMock *mocks.UtilInterface - fsMock *mocks.FsInterface - nodeSvc *Service - clientMock *gopowerstoremock.Client - ctrlMock *mocks.ControllerInterface - iscsiLibMock *goiscsi.MockISCSI - nvmeLibMock *gonvme.MockNVMe + iscsiConnectorMock *mocks.ISCSIConnector + nvmeConnectorMock *mocks.NVMEConnector + fcConnectorMock *mocks.FcConnector + utilMock *mocks.UtilInterface + fsMock *mocks.FsInterface + nodeSvc *Service + clientMock *gopowerstoremock.Client + ctrlMock *mocks.ControllerInterface + iscsiLibMock *goiscsi.MockISCSI + nvmeLibMock *gonvme.MockNVMe + nodeLabelsRetrieverMock *mocks.NodeLabelsRetrieverInterface ) const ( @@ -190,6 +192,8 @@ func setVariables() { clientMock = new(gopowerstoremock.Client) iscsiLibMock = goiscsi.NewMockISCSI(nil) nvmeLibMock = gonvme.NewMockNVMe(nil) + nodeLabelsRetrieverMock = new(mocks.NodeLabelsRetrieverInterface) + k8sutils.NodeLabelsRetriever = nodeLabelsRetrieverMock arrays := getTestArrays() @@ -221,6 +225,13 @@ func setVariables() { nodeSvc.SetDefaultArray(arrays[firstValidIP]) } +func setDefaultNodeLabelsRetrieverMock() { + nodeLabelsRetrieverMock.On("BuildConfigFromFlags", mock.Anything, mock.Anything).Return(nil, nil) + nodeLabelsRetrieverMock.On("GetNodeLabels", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil) + nodeLabelsRetrieverMock.On("InClusterConfig", mock.Anything).Return(nil, nil) + nodeLabelsRetrieverMock.On("NewForConfig", mock.Anything).Return(nil, nil) +} + var _ = Describe("CSINodeService", func() { BeforeEach(func() { setVariables() @@ -3127,6 +3138,8 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) Expect(res).To(Equal(&csi.NodeGetInfoResponse{ @@ -3138,6 +3151,112 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, + })) + }) + }) + + When("node label max-powerstore-volumes-per-node is set and retrieved successfully", func() { + It("should return correct MaxVolumesPerNode in response", func() { + clientMock.On("GetStorageISCSITargetAddresses", mock.Anything). + Return([]gopowerstore.IPPoolAddress{ + { + Address: "192.168.1.1", + IPPort: gopowerstore.IPPortInstance{TargetIqn: "iqn"}, + }, + { + Address: "192.168.1.2", + IPPort: gopowerstore.IPPortInstance{TargetIqn: "iqn2"}, + }, + }, nil) + conn, _ := net.Dial("udp", "127.0.0.1:80") + fsMock.On("NetDial", mock.Anything).Return( + conn, + nil, + ) + nodeLabelsRetrieverMock.On("BuildConfigFromFlags", mock.Anything, mock.Anything).Return(nil, nil) + nodeLabelsRetrieverMock.On("GetNodeLabels", mock.Anything, mock.Anything, mock.Anything).Return(map[string]string{"max-powerstore-volumes-per-node": "2"}, nil) + nodeLabelsRetrieverMock.On("InClusterConfig", mock.Anything).Return(nil, nil) + nodeLabelsRetrieverMock.On("NewForConfig", mock.Anything).Return(nil, nil) + + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) + Expect(err).To(BeNil()) + Expect(res).To(Equal(&csi.NodeGetInfoResponse{ + NodeId: nodeSvc.nodeID, + AccessibleTopology: &csi.Topology{ + Segments: map[string]string{ + common.Name + "/" + firstValidIP + "-nfs": "true", + common.Name + "/" + firstValidIP + "-iscsi": "true", + common.Name + "/" + secondValidIP + "-nfs": "true", + }, + }, + MaxVolumesPerNode: 2, + })) + }) + }) + + When("there is some issue while retrieving node labels", func() { + It("should return proper error", func() { + clientMock.On("GetStorageISCSITargetAddresses", mock.Anything). + Return([]gopowerstore.IPPoolAddress{ + { + Address: "192.168.1.1", + IPPort: gopowerstore.IPPortInstance{TargetIqn: "iqn"}, + }, + { + Address: "192.168.1.2", + IPPort: gopowerstore.IPPortInstance{TargetIqn: "iqn2"}, + }, + }, nil) + conn, _ := net.Dial("udp", "127.0.0.1:80") + fsMock.On("NetDial", mock.Anything).Return( + conn, + nil, + ) + nodeLabelsRetrieverMock.On("BuildConfigFromFlags", mock.Anything, mock.Anything).Return(nil, nil) + nodeLabelsRetrieverMock.On("GetNodeLabels", mock.Anything, mock.Anything, mock.Anything).Return(map[string]string{"max-powerstore-volumes-per-node": "2"}, nil) + nodeLabelsRetrieverMock.On("InClusterConfig", mock.Anything).Return(nil, errors.New("Unable to create kubeclientset")) + nodeLabelsRetrieverMock.On("NewForConfig", mock.Anything).Return(nil, nil) + + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) + Expect(err).To(Equal(errors.New("Unable to create kubeclientset"))) + Expect(res).To(BeNil()) + }) + }) + + When("MaxVolumesPerNode is set via environment variable at the time of installation", func() { + It("should return correct MaxVolumesPerNode in response", func() { + clientMock.On("GetStorageISCSITargetAddresses", mock.Anything). + Return([]gopowerstore.IPPoolAddress{ + { + Address: "192.168.1.1", + IPPort: gopowerstore.IPPortInstance{TargetIqn: "iqn"}, + }, + { + Address: "192.168.1.2", + IPPort: gopowerstore.IPPortInstance{TargetIqn: "iqn2"}, + }, + }, nil) + conn, _ := net.Dial("udp", "127.0.0.1:80") + fsMock.On("NetDial", mock.Anything).Return( + conn, + nil, + ) + setDefaultNodeLabelsRetrieverMock() + nodeSvc.opts.MaxVolumesPerNode = 2 + + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) + Expect(err).To(BeNil()) + Expect(res).To(Equal(&csi.NodeGetInfoResponse{ + NodeId: nodeSvc.nodeID, + AccessibleTopology: &csi.Topology{ + Segments: map[string]string{ + common.Name + "/" + firstValidIP + "-nfs": "true", + common.Name + "/" + firstValidIP + "-iscsi": "true", + common.Name + "/" + secondValidIP + "-nfs": "true", + }, + }, + MaxVolumesPerNode: 2, })) }) }) @@ -3160,6 +3279,8 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) Expect(res).To(Equal(&csi.NodeGetInfoResponse{ @@ -3170,6 +3291,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) }) @@ -3184,6 +3306,8 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) @@ -3195,6 +3319,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) }) @@ -3216,6 +3341,8 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) Expect(res).To(Equal(&csi.NodeGetInfoResponse{ @@ -3226,6 +3353,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) gonvme.GONVMEMock.InduceDiscoveryError = false }) @@ -3263,6 +3391,7 @@ var _ = Describe("CSINodeService", func() { }}, Name: "host-name", }, nil) + setDefaultNodeLabelsRetrieverMock() res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) @@ -3275,6 +3404,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) @@ -3313,6 +3443,7 @@ var _ = Describe("CSINodeService", func() { }}, Name: "host-name", }, nil) + setDefaultNodeLabelsRetrieverMock() res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) @@ -3325,6 +3456,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) @@ -3363,6 +3495,7 @@ var _ = Describe("CSINodeService", func() { }}, Name: "host-name", }, nil) + setDefaultNodeLabelsRetrieverMock() res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) @@ -3374,6 +3507,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) }) @@ -3390,6 +3524,8 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) Expect(res).To(Equal(&csi.NodeGetInfoResponse{ @@ -3400,6 +3536,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) }) @@ -3418,6 +3555,8 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) Expect(res).To(Equal(&csi.NodeGetInfoResponse{ @@ -3428,6 +3567,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) }) @@ -3454,6 +3594,7 @@ var _ = Describe("CSINodeService", func() { }}, Name: "host-name", }, nil) + setDefaultNodeLabelsRetrieverMock() res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) @@ -3465,6 +3606,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) }) @@ -3492,6 +3634,7 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) @@ -3504,6 +3647,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) @@ -3528,6 +3672,7 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) @@ -3539,6 +3684,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) }) @@ -3560,6 +3706,8 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) Expect(res).To(Equal(&csi.NodeGetInfoResponse{ @@ -3571,6 +3719,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) @@ -3592,6 +3741,8 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) Expect(res).To(Equal(&csi.NodeGetInfoResponse{ @@ -3602,6 +3753,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) gonvme.GONVMEMock.InduceDiscoveryError = false }) @@ -3619,6 +3771,8 @@ var _ = Describe("CSINodeService", func() { conn, nil, ) + setDefaultNodeLabelsRetrieverMock() + res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) Expect(err).To(BeNil()) Expect(res).To(Equal(&csi.NodeGetInfoResponse{ @@ -3629,6 +3783,7 @@ var _ = Describe("CSINodeService", func() { common.Name + "/" + secondValidIP + "-nfs": "true", }, }, + MaxVolumesPerNode: 0, })) }) }) From d714c3fac64abdcf710b2be6c8238bc59c2a98dd Mon Sep 17 00:00:00 2001 From: alankar_verma Date: Mon, 24 Jul 2023 07:23:07 -0400 Subject: [PATCH 4/5] Volume limits support for csi-powerstore - fixed one UT test case --- pkg/node/node_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/node/node_test.go b/pkg/node/node_test.go index 748df099..7071da67 100644 --- a/pkg/node/node_test.go +++ b/pkg/node/node_test.go @@ -3214,7 +3214,7 @@ var _ = Describe("CSINodeService", func() { nil, ) nodeLabelsRetrieverMock.On("BuildConfigFromFlags", mock.Anything, mock.Anything).Return(nil, nil) - nodeLabelsRetrieverMock.On("GetNodeLabels", mock.Anything, mock.Anything, mock.Anything).Return(map[string]string{"max-powerstore-volumes-per-node": "2"}, nil) + nodeLabelsRetrieverMock.On("GetNodeLabels", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil) nodeLabelsRetrieverMock.On("InClusterConfig", mock.Anything).Return(nil, errors.New("Unable to create kubeclientset")) nodeLabelsRetrieverMock.On("NewForConfig", mock.Anything).Return(nil, nil) From a849ce3a6140b94d48c91484be3157fa47fab1ee Mon Sep 17 00:00:00 2001 From: alankar_verma Date: Tue, 25 Jul 2023 08:16:32 -0400 Subject: [PATCH 5/5] Volume limits support for csi-powerstore - fixed liniting issues and incorporated review comments --- mocks/NodeLabelsRetriever.go | 18 +++++++-------- pkg/common/k8sutils/k8sutils.go | 19 ++++++++++------ pkg/node/base.go | 1 + pkg/node/node.go | 40 +++++++++++++++++---------------- pkg/node/node_test.go | 14 ++++++++++-- 5 files changed, 55 insertions(+), 37 deletions(-) diff --git a/mocks/NodeLabelsRetriever.go b/mocks/NodeLabelsRetriever.go index 6f367573..4dca0b74 100644 --- a/mocks/NodeLabelsRetriever.go +++ b/mocks/NodeLabelsRetriever.go @@ -1,5 +1,5 @@ /* - Copyright (c) 2021-2023 Dell Inc, or its subsidiaries. + Copyright (c) 2023 Dell Inc, or its subsidiaries. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -33,17 +33,17 @@ type NodeLabelsRetrieverInterface struct { mock.Mock } -// BuildConfigFromFlags provides a mock function with given fields: masterUrl, kubeconfig -func (_m *NodeLabelsRetrieverInterface) BuildConfigFromFlags(masterUrl string, kubeconfig string) (*rest.Config, error) { - ret := _m.Called(masterUrl, kubeconfig) +// BuildConfigFromFlags provides a mock function with given fields: masterURL, kubeconfig +func (_m *NodeLabelsRetrieverInterface) BuildConfigFromFlags(masterURL string, kubeconfig string) (*rest.Config, error) { + ret := _m.Called(masterURL, kubeconfig) var r0 *rest.Config var r1 error if rf, ok := ret.Get(0).(func(string, string) (*rest.Config, error)); ok { - return rf(masterUrl, kubeconfig) + return rf(masterURL, kubeconfig) } if rf, ok := ret.Get(0).(func(string, string) *rest.Config); ok { - r0 = rf(masterUrl, kubeconfig) + r0 = rf(masterURL, kubeconfig) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*rest.Config) @@ -51,7 +51,7 @@ func (_m *NodeLabelsRetrieverInterface) BuildConfigFromFlags(masterUrl string, k } if rf, ok := ret.Get(1).(func(string, string) error); ok { - r1 = rf(masterUrl, kubeconfig) + r1 = rf(masterURL, kubeconfig) } else { r1 = ret.Error(1) } @@ -59,8 +59,8 @@ func (_m *NodeLabelsRetrieverInterface) BuildConfigFromFlags(masterUrl string, k return r0, r1 } -// GetNodeLabels provides a mock function with given fields: k8sclientset, ctx, kubeNodeName -func (_m *NodeLabelsRetrieverInterface) GetNodeLabels(k8sclientset *kubernetes.Clientset, ctx context.Context, kubeNodeName string) (map[string]string, error) { +// GetNodeLabels provides a mock function with given fields: ctx, k8sclientset, kubeNodeName +func (_m *NodeLabelsRetrieverInterface) GetNodeLabels(ctx context.Context, k8sclientset *kubernetes.Clientset, kubeNodeName string) (map[string]string, error) { ret := _m.Called(k8sclientset, ctx, kubeNodeName) var r0 map[string]string diff --git a/pkg/common/k8sutils/k8sutils.go b/pkg/common/k8sutils/k8sutils.go index 0f36a716..8f52f4a8 100644 --- a/pkg/common/k8sutils/k8sutils.go +++ b/pkg/common/k8sutils/k8sutils.go @@ -1,5 +1,5 @@ /* - Copyright (c) 2021-2023 Dell Inc, or its subsidiaries. + Copyright (c) 2023 Dell Inc, or its subsidiaries. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -27,34 +27,39 @@ import ( // NodeLabelsRetrieverInterface defines the methods for retrieving Kubernetes Node Labels type NodeLabelsRetrieverInterface interface { - BuildConfigFromFlags(masterUrl, kubeconfig string) (*rest.Config, error) + BuildConfigFromFlags(masterURL, kubeconfig string) (*rest.Config, error) InClusterConfig() (*rest.Config, error) NewForConfig(config *rest.Config) (*kubernetes.Clientset, error) - GetNodeLabels(k8sclientset *kubernetes.Clientset, ctx context.Context, kubeNodeName string) (map[string]string, error) + GetNodeLabels(ctx context.Context, k8sclientset *kubernetes.Clientset, kubeNodeName string) (map[string]string, error) } // NodeLabelsRetrieverImpl provided the implementation for NodeLabelsRetrieverInterface type NodeLabelsRetrieverImpl struct{} +// NodeLabelsRetriever is the actual instance of NodeLabelsRetrieverInterface which is used to retrieve the node labels var NodeLabelsRetriever NodeLabelsRetrieverInterface func init() { NodeLabelsRetriever = new(NodeLabelsRetrieverImpl) } -func (svc *NodeLabelsRetrieverImpl) BuildConfigFromFlags(masterUrl, kubeconfig string) (*rest.Config, error) { - return clientcmd.BuildConfigFromFlags(masterUrl, kubeconfig) +// BuildConfigFromFlags is a method for building kubernetes client config +func (svc *NodeLabelsRetrieverImpl) BuildConfigFromFlags(masterURL, kubeconfig string) (*rest.Config, error) { + return clientcmd.BuildConfigFromFlags(masterURL, kubeconfig) } +// InClusterConfig returns a config object which uses the service account kubernetes gives to pods func (svc *NodeLabelsRetrieverImpl) InClusterConfig() (*rest.Config, error) { return rest.InClusterConfig() } +// NewForConfig creates a new Clientset for the given config func (svc *NodeLabelsRetrieverImpl) NewForConfig(config *rest.Config) (*kubernetes.Clientset, error) { return kubernetes.NewForConfig(config) } -func (svc *NodeLabelsRetrieverImpl) GetNodeLabels(k8sclientset *kubernetes.Clientset, ctx context.Context, kubeNodeName string) (map[string]string, error) { +// GetNodeLabels retrieves the kubernetes node object and returns its labels +func (svc *NodeLabelsRetrieverImpl) GetNodeLabels(ctx context.Context, k8sclientset *kubernetes.Clientset, kubeNodeName string) (map[string]string, error) { if k8sclientset != nil { node, err := k8sclientset.CoreV1().Nodes().Get(ctx, kubeNodeName, v1.GetOptions{}) if err != nil { @@ -101,5 +106,5 @@ func GetNodeLabels(ctx context.Context, kubeConfigPath string, kubeNodeName stri return nil, err } - return NodeLabelsRetriever.GetNodeLabels(k8sclientset, ctx, kubeNodeName) + return NodeLabelsRetriever.GetNodeLabels(ctx, k8sclientset, kubeNodeName) } diff --git a/pkg/node/base.go b/pkg/node/base.go index fdc61b3d..a171185f 100644 --- a/pkg/node/base.go +++ b/pkg/node/base.go @@ -114,6 +114,7 @@ func getNodeOptions() Opts { if maxVolumesPerNodeStr, ok := csictx.LookupEnv(ctx, common.EnvMaxVolumesPerNode); ok { maxVolumesPerNode, err := strconv.ParseInt(maxVolumesPerNodeStr, 10, 64) if err != nil { + log.Warn("error while parsing the value of maxPowerstoreVolumesPerNode, using default value 0") opts.MaxVolumesPerNode = 0 } else { opts.MaxVolumesPerNode = maxVolumesPerNode diff --git a/pkg/node/node.go b/pkg/node/node.go index 3307fe41..fab77152 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -1214,33 +1214,35 @@ func (s *Service) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoRequest) } } - // Check for node label 'max-powerstore-volumes-per-node'. If present set 'maxVolumesPerNode' to this value. - // If node label is not present, set 'maxVolumesPerNode' to default value i.e., 0 var maxVolumesPerNode int64 = 0 + // Setting maxVolumesPerNode using the value of field maxPowerstoreVolumesPerNode specified in values.yaml + if s.opts.MaxVolumesPerNode > 0 { + maxVolumesPerNode = s.opts.MaxVolumesPerNode + } + + // Check for node label 'max-powerstore-volumes-per-node'. If present set 'maxVolumesPerNode' to this value. labels, err := k8sutils.GetNodeLabels(ctx, s.opts.KubeConfigPath, s.opts.KubeNodeName) if err != nil { - log.Error("failed to get Node Labels with error", err.Error()) - return nil, err - } + log.Warnf("failed to get Node Labels with error: %s", err.Error()) + } else if labels != nil { + if val, ok := labels[maxPowerstoreVolumesPerNodeLabel]; ok { + maxVols, err := strconv.ParseInt(val, 10, 64) + if err != nil { + log.Warnf("invalid value '%s' specified for 'max-powerstore-volumes-per-node' node label", val) + } else if maxVols > 0 { + maxVolumesPerNode = maxVols + log.Infof("node label 'max-powerstore-volumes-per-node' is available and is set to value '%d'", maxVolumesPerNode) + } - if val, ok := labels[maxPowerstoreVolumesPerNodeLabel]; ok { - maxVols, err := strconv.ParseInt(val, 10, 64) - if err != nil { - return nil, fmt.Errorf("invalid value '%s' specified for 'max-powerstore-volumes-per-node' node label", val) } - if maxVols > 0 { - maxVolumesPerNode = maxVols - } - log.Infof("node label 'max-powerstore-volumes-per-node' is available and is set to value '%d'", maxVolumesPerNode) - } else { - if s.opts.MaxVolumesPerNode > 0 { - maxVolumesPerNode = s.opts.MaxVolumesPerNode - } - log.Infof("node label 'max-powerstore-volumes-per-node' is not available. Using default volume limit '%v'", maxVolumesPerNode) } - resp.MaxVolumesPerNode = maxVolumesPerNode + if maxVolumesPerNode >= 0 { + resp.MaxVolumesPerNode = maxVolumesPerNode + log.Infof("Setting MaxVolumesPerNode to '%d'", maxVolumesPerNode) + } + return resp, nil } diff --git a/pkg/node/node_test.go b/pkg/node/node_test.go index 7071da67..3e1ca112 100644 --- a/pkg/node/node_test.go +++ b/pkg/node/node_test.go @@ -3219,8 +3219,18 @@ var _ = Describe("CSINodeService", func() { nodeLabelsRetrieverMock.On("NewForConfig", mock.Anything).Return(nil, nil) res, err := nodeSvc.NodeGetInfo(context.Background(), &csi.NodeGetInfoRequest{}) - Expect(err).To(Equal(errors.New("Unable to create kubeclientset"))) - Expect(res).To(BeNil()) + Expect(err).To(BeNil()) + Expect(res).To(Equal(&csi.NodeGetInfoResponse{ + NodeId: nodeSvc.nodeID, + AccessibleTopology: &csi.Topology{ + Segments: map[string]string{ + common.Name + "/" + firstValidIP + "-nfs": "true", + common.Name + "/" + firstValidIP + "-iscsi": "true", + common.Name + "/" + secondValidIP + "-nfs": "true", + }, + }, + MaxVolumesPerNode: 0, + })) }) })