From 89ac90ba95f447324997c5ca9407ecebb49c2722 Mon Sep 17 00:00:00 2001 From: Aly Nathoo Date: Mon, 2 Dec 2024 16:11:38 -0500 Subject: [PATCH 1/8] Adding GetMatchingNodes as a function to find Nodes that have labels that match label and label value. --- controllers/csm_controller.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/controllers/csm_controller.go b/controllers/csm_controller.go index 997a516c..e549190c 100644 --- a/controllers/csm_controller.go +++ b/controllers/csm_controller.go @@ -277,6 +277,14 @@ func (r *ContainerStorageModuleReconciler) Reconcile(_ context.Context, req ctrl return ctrl.Result{}, err } + nodeList, err := GetMatchingNodes(ctx, r, "topology.kubernetes.io/zone", "US-EAST") + if err != nil { + log.Errorw("Failed to retrieve list of nodes for label", + "topology.kubernetes.io/zone") + } + log.Infow("nodeList with labels", "csm", nodeList) + + // perform prechecks err = r.PreChecks(ctx, csm, *operatorConfig) if err != nil { @@ -1517,3 +1525,13 @@ func (r *ContainerStorageModuleReconciler) GetUpdateCount() int32 { func (r *ContainerStorageModuleReconciler) GetK8sClient() kubernetes.Interface { return r.K8sClient } + +func GetMatchingNodes(ctx context.Context, r *ContainerStorageModuleReconciler, labelKey string, labelValue string) (*corev1.NodeList, error) { + nodeList := &corev1.NodeList{} + opts := []client.ListOption{ + client.MatchingLabels{labelKey: labelValue}, + } + err := r.List(ctx, nodeList, opts...) + + return nodeList, err +} From b32b393e4f13db528556ce12c5f1240dce6fa793 Mon Sep 17 00:00:00 2001 From: Aly Nathoo Date: Tue, 3 Dec 2024 17:06:05 -0500 Subject: [PATCH 2/8] minor refactor. --- controllers/csm_controller.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/controllers/csm_controller.go b/controllers/csm_controller.go index e549190c..12c4930c 100644 --- a/controllers/csm_controller.go +++ b/controllers/csm_controller.go @@ -277,14 +277,13 @@ func (r *ContainerStorageModuleReconciler) Reconcile(_ context.Context, req ctrl return ctrl.Result{}, err } - nodeList, err := GetMatchingNodes(ctx, r, "topology.kubernetes.io/zone", "US-EAST") + nodeList, err := r.GetMatchingNodes(ctx, "topology.kubernetes.io/zone", "US-EAST") if err != nil { log.Errorw("Failed to retrieve list of nodes for label", "topology.kubernetes.io/zone") } log.Infow("nodeList with labels", "csm", nodeList) - // perform prechecks err = r.PreChecks(ctx, csm, *operatorConfig) if err != nil { @@ -1526,7 +1525,7 @@ func (r *ContainerStorageModuleReconciler) GetK8sClient() kubernetes.Interface { return r.K8sClient } -func GetMatchingNodes(ctx context.Context, r *ContainerStorageModuleReconciler, labelKey string, labelValue string) (*corev1.NodeList, error) { +func (r *ContainerStorageModuleReconciler) GetMatchingNodes(ctx context.Context, labelKey string, labelValue string) (*corev1.NodeList, error) { nodeList := &corev1.NodeList{} opts := []client.ListOption{ client.MatchingLabels{labelKey: labelValue}, From ed3da5cd1ebdf7562f86316a9e2227f3296aaee2 Mon Sep 17 00:00:00 2001 From: Aly Nathoo Date: Wed, 4 Dec 2024 17:38:28 -0500 Subject: [PATCH 3/8] Adding basic Unit test. --- controllers/csm_controller_test.go | 55 ++++++++++++++++++++++++++++++ tests/shared/crclient/client.go | 11 ++++++ 2 files changed, 66 insertions(+) diff --git a/controllers/csm_controller_test.go b/controllers/csm_controller_test.go index 91e11165..c5882c88 100644 --- a/controllers/csm_controller_test.go +++ b/controllers/csm_controller_test.go @@ -2382,3 +2382,58 @@ func (suite *CSMControllerTestSuite) makeFakeRevProxyCSM(name string, ns string, err = suite.fakeClient.Create(ctx, &csm) assert.Nil(suite.T(), err) } + +func (suite *CSMControllerTestSuite) TestGetNodeLabels() { + // TBD: crclient/client.go needs to be augmented to filter on labels during + // the List return for a viable thorough test. Since this functionality is + // missing, this test is quite elementary as a result. + + csm := shared.MakeCSM(csmName, suite.namespace, configVersion) + csm.Spec.Driver.CSIDriverType = csmv1.PowerScale + csm.Spec.Driver.Common.Image = "image" + csm.Annotations[configVersionKey] = configVersion + + sec := shared.MakeSecret(csmName+"-creds", suite.namespace, configVersion) + err := suite.fakeClient.Create(ctx, sec) + if err != nil { + panic(err) + } + + csm.ObjectMeta.Finalizers = []string{CSMFinalizerName} + err = suite.fakeClient.Create(ctx, &csm) + if err != nil { + panic(err) + } + + reconciler := suite.createReconciler() + + // create node object, add to fakeclient, reconcile.GetMatchingNodes + node := shared.MakeNode("node1", suite.namespace) + node.Labels["topology.kubernetes.io/zone"] = "US-EAST" + + err = suite.fakeClient.Create(ctx, &node) + assert.Nil(suite.T(), err) + + nodeList := &corev1.NodeList{} + err = suite.fakeClient.List(ctx, nodeList, nil) + assert.Nil(suite.T(), err) + + nodeListMatching, err := reconciler.GetMatchingNodes(ctx, "topology.kubernetes.io/zone", "US-EAST") + ctrl.Log.Info("node list response (1)", "number of nodes is: ", len(nodeListMatching.Items)) + + // Check the len to be 1 else fail + if len(nodeListMatching.Items) != 1 { + ctrl.Log.Error(err, "Unexpected length on node list.","length",len(nodeListMatching.Items)) + panic(err) + } + + for _, node := range nodeListMatching.Items { + ctrl.Log.Info("Matching node item", "name ", node.ObjectMeta.GetName()) + } + if node.ObjectMeta.GetName() != "node1" { + ctrl.Log.Error(err, "Unmatched label on node.") + panic(err) + } + assert.Nil(suite.T(), err) + +} diff --git a/tests/shared/crclient/client.go b/tests/shared/crclient/client.go index a6d1d0f4..3ecf2979 100644 --- a/tests/shared/crclient/client.go +++ b/tests/shared/crclient/client.go @@ -106,6 +106,8 @@ func (f Client) List(ctx context.Context, list client.ObjectList, _ ...client.Li switch l := list.(type) { case *corev1.PodList: return f.listPodList(l) + case *corev1.NodeList: + return f.listNodeList(l) case *appsv1.DeploymentList: return f.listDeploymentList(ctx, &appsv1.DeploymentList{}) default: @@ -122,6 +124,15 @@ func (f Client) listPodList(list *corev1.PodList) error { return nil } +func (f Client) listNodeList(list *corev1.NodeList) error { + for k, v := range f.Objects { + if k.Kind == "Node" { + list.Items = append(list.Items, *v.(*corev1.Node)) + } + } + return nil +} + func (f Client) listDeploymentList(_ context.Context, list *appsv1.DeploymentList) error { for k, v := range f.Objects { if k.Kind == "Deployment" { From 5c777e284565836bc451fc263be84c1a9e086b5a Mon Sep 17 00:00:00 2001 From: Aly Nathoo Date: Wed, 4 Dec 2024 18:15:47 -0500 Subject: [PATCH 4/8] Adding changes to common.go. --- tests/shared/common.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/shared/common.go b/tests/shared/common.go index 2855cd45..fc48d6ae 100644 --- a/tests/shared/common.go +++ b/tests/shared/common.go @@ -195,6 +195,19 @@ func MakePod(name, ns string) corev1.Pod { return podObj } +// MakeNode returns a node object +func MakeNode(name, ns string) corev1.Node { + nodeObj := corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + Labels: map[string]string{}, + }, + } + + return nodeObj +} + // MakeReverseProxyModule returns a csireverseproxy object func MakeReverseProxyModule(_ string) csmv1.Module { revproxy := csmv1.Module{ From 084f250f0e418e3642423cc7c1d7dd27a978146d Mon Sep 17 00:00:00 2001 From: Aly Nathoo Date: Wed, 4 Dec 2024 19:58:46 -0500 Subject: [PATCH 5/8] fixing golangci-lint errors. --- controllers/csm_controller.go | 2 +- controllers/csm_controller_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/controllers/csm_controller.go b/controllers/csm_controller.go index 12c4930c..500b5a72 100644 --- a/controllers/csm_controller.go +++ b/controllers/csm_controller.go @@ -280,7 +280,7 @@ func (r *ContainerStorageModuleReconciler) Reconcile(_ context.Context, req ctrl nodeList, err := r.GetMatchingNodes(ctx, "topology.kubernetes.io/zone", "US-EAST") if err != nil { log.Errorw("Failed to retrieve list of nodes for label", - "topology.kubernetes.io/zone") + "topology.kubernetes.io/zone") } log.Infow("nodeList with labels", "csm", nodeList) diff --git a/controllers/csm_controller_test.go b/controllers/csm_controller_test.go index c5882c88..1a8a1f28 100644 --- a/controllers/csm_controller_test.go +++ b/controllers/csm_controller_test.go @@ -2385,7 +2385,7 @@ func (suite *CSMControllerTestSuite) makeFakeRevProxyCSM(name string, ns string, func (suite *CSMControllerTestSuite) TestGetNodeLabels() { // TBD: crclient/client.go needs to be augmented to filter on labels during - // the List return for a viable thorough test. Since this functionality is + // the List return for a viable thorough test. Since this functionality is // missing, this test is quite elementary as a result. csm := shared.MakeCSM(csmName, suite.namespace, configVersion) @@ -2423,7 +2423,7 @@ func (suite *CSMControllerTestSuite) TestGetNodeLabels() { // Check the len to be 1 else fail if len(nodeListMatching.Items) != 1 { - ctrl.Log.Error(err, "Unexpected length on node list.","length",len(nodeListMatching.Items)) + ctrl.Log.Error(err, "Unexpected length on node list.", "length", len(nodeListMatching.Items)) panic(err) } From befa5badb49e9af7a43282506364061877e506dc Mon Sep 17 00:00:00 2001 From: Aly Nathoo Date: Thu, 5 Dec 2024 09:09:24 -0500 Subject: [PATCH 6/8] Removing node list code from Reconciler as not needed. --- controllers/csm_controller.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/controllers/csm_controller.go b/controllers/csm_controller.go index 500b5a72..05a1054d 100644 --- a/controllers/csm_controller.go +++ b/controllers/csm_controller.go @@ -277,13 +277,6 @@ func (r *ContainerStorageModuleReconciler) Reconcile(_ context.Context, req ctrl return ctrl.Result{}, err } - nodeList, err := r.GetMatchingNodes(ctx, "topology.kubernetes.io/zone", "US-EAST") - if err != nil { - log.Errorw("Failed to retrieve list of nodes for label", - "topology.kubernetes.io/zone") - } - log.Infow("nodeList with labels", "csm", nodeList) - // perform prechecks err = r.PreChecks(ctx, csm, *operatorConfig) if err != nil { From 4e989b42f5a2236e91a94e622e0a330774b7e641 Mon Sep 17 00:00:00 2001 From: Aly Nathoo Date: Thu, 5 Dec 2024 10:01:22 -0500 Subject: [PATCH 7/8] Attempting to fix gofumpt error. --- controllers/csm_controller_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controllers/csm_controller_test.go b/controllers/csm_controller_test.go index 1a8a1f28..3b80ecbe 100644 --- a/controllers/csm_controller_test.go +++ b/controllers/csm_controller_test.go @@ -2435,5 +2435,4 @@ func (suite *CSMControllerTestSuite) TestGetNodeLabels() { panic(err) } assert.Nil(suite.T(), err) - } From b463656d4659397b86f8ee516149bf512796756c Mon Sep 17 00:00:00 2001 From: Aly Nathoo Date: Thu, 5 Dec 2024 16:33:21 -0500 Subject: [PATCH 8/8] Address type assertion possible panic in test code. Signed-off-by: Aly Nathoo --- tests/shared/crclient/client.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/shared/crclient/client.go b/tests/shared/crclient/client.go index 3ecf2979..ee6be344 100644 --- a/tests/shared/crclient/client.go +++ b/tests/shared/crclient/client.go @@ -127,7 +127,10 @@ func (f Client) listPodList(list *corev1.PodList) error { func (f Client) listNodeList(list *corev1.NodeList) error { for k, v := range f.Objects { if k.Kind == "Node" { - list.Items = append(list.Items, *v.(*corev1.Node)) + node, ok := v.(*corev1.Node) + if ok { + list.Items = append(list.Items, *node) + } } } return nil