Volume limits support for csi-powerstore - fixed liniting issues and …

…incorporated review comments

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)
 }

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

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
 }
 

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,
+				}))
 			})
 		})