findOffendingNodes returns usable maps of nodes rather than lists of …

…strings
atlassian-labs · Jul 22, 2024 · 01b396e · 01b396e
1 parent 11a4767
commit 01b396e
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 34 deletions.
diff --git a/pkg/controller/cyclenoderequest/transitioner/transitions.go b/pkg/controller/cyclenoderequest/transitioner/transitions.go
@@ -111,17 +111,33 @@ func (t *CycleNodeRequestTransitioner) transitionPending() (reconcile.Result, er
 		nodesNotInCPNodeGroup, nodesNotInKube := findOffendingNodes(kubeNodes, nodeGroupInstances)
 
 		if len(nodesNotInCPNodeGroup) > 0 {
+			providerIDs := make([]string, 0)
+
+			for providerID := range nodesNotInCPNodeGroup {
+				providerIDs = append(providerIDs,
+					fmt.Sprintf("id %q", providerID),
+				)
+			}
+
 			offendingNodesInfo += "nodes not in node group: "
-			offendingNodesInfo += strings.Join(nodesNotInCPNodeGroup, ",")
+			offendingNodesInfo += strings.Join(providerIDs, ",")
 		}
 
 		if len(nodesNotInKube) > 0 {
 			if offendingNodesInfo != "" {
 				offendingNodesInfo += ";"
 			}
 
+			providerIDs := make([]string, 0)
+
+			for providerID, node := range nodesNotInKube {
+				providerIDs = append(providerIDs,
+					fmt.Sprintf("id %q in %q", providerID, node.NodeGroupName()),
+				)
+			}
+
 			offendingNodesInfo += "nodes not inside cluster: "
-			offendingNodesInfo += strings.Join(nodesNotInKube, ",")
+			offendingNodesInfo += strings.Join(providerIDs, ",")
 		}
 
 		t.rm.LogEvent(t.cycleNodeRequest, "NodeCountMismatch",

diff --git a/pkg/controller/cyclenoderequest/transitioner/util.go b/pkg/controller/cyclenoderequest/transitioner/util.go
@@ -253,26 +253,19 @@ func (t *CycleNodeRequestTransitioner) checkIfTransitioning(numNodesToCycle, num
 
 // findOffendingNodes finds the offending nodes information which cause number of nodes mismatch between
 // cloud provider node group and nodes inside kubernetes cluster using label selector
-func findOffendingNodes(kubeNodes map[string]corev1.Node, cloudProviderNodes map[string]cloudprovider.Instance) ([]string, []string) {
-	var nodesNotInCPNodeGroup []string
-	var nodesNotInKube []string
+func findOffendingNodes(kubeNodes map[string]corev1.Node, cloudProviderNodes map[string]cloudprovider.Instance) (map[string]corev1.Node, map[string]cloudprovider.Instance) {
+	var nodesNotInCPNodeGroup = make(map[string]corev1.Node)
+	var nodesNotInKube = make(map[string]cloudprovider.Instance)
 
 	for _, kubeNode := range kubeNodes {
 		if _, ok := cloudProviderNodes[kubeNode.Spec.ProviderID]; !ok {
-			nodesNotInCPNodeGroup = append(nodesNotInCPNodeGroup,
-				fmt.Sprintf("id %q", kubeNode.Spec.ProviderID),
-			)
+			nodesNotInCPNodeGroup[kubeNode.Spec.ProviderID] = kubeNode
 		}
 	}
 
-	for cpNode := range cloudProviderNodes {
-		if _, ok := kubeNodes[cpNode]; !ok {
-			nodesNotInKube = append(nodesNotInKube,
-				fmt.Sprintf("id %q in %q",
-					cpNode,
-					cloudProviderNodes[cpNode].NodeGroupName(),
-				),
-			)
+	for providerID, cpNode := range cloudProviderNodes {
+		if _, ok := kubeNodes[providerID]; !ok {
+			nodesNotInKube[providerID] = cpNode
 		}
 	}
 

diff --git a/pkg/controller/cyclenoderequest/transitioner/util_test.go b/pkg/controller/cyclenoderequest/transitioner/util_test.go
@@ -1,6 +1,7 @@
 package transitioner
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/atlassian-labs/cyclops/pkg/cloudprovider"
@@ -47,8 +48,8 @@ func TestFindOffendingNodes(t *testing.T) {
 		name                   string
 		knodes                 map[string]corev1.Node
 		cnodes                 map[string]cloudprovider.Instance
-		expectNotInCPNodeGroup []string
-		expectNotInKube        []string
+		expectNotInCPNodeGroup map[string]corev1.Node
+		expectNotInKube        map[string]cloudprovider.Instance
 	}{
 		{
 			"kube nodes match cloud provider nodes",
@@ -62,8 +63,8 @@ func TestFindOffendingNodes(t *testing.T) {
 				dummyInstanceB.providerID: &dummyInstanceB,
 				dummyInstanceC.providerID: &dummyInstanceC,
 			},
-			[]string{},
-			[]string{},
+			make(map[string]corev1.Node),
+			make(map[string]cloudprovider.Instance),
 		},
 		{
 			"more nodes in kube than cloud provider",
@@ -76,8 +77,10 @@ func TestFindOffendingNodes(t *testing.T) {
 				dummyInstanceA.providerID: &dummyInstanceA,
 				dummyInstanceB.providerID: &dummyInstanceB,
 			},
-			[]string{"id \"aws:///us-east-1c/i-cbcdefghijk\""},
-			[]string{},
+			map[string]corev1.Node{
+				dummyInstanceC.providerID: buildNode(dummyInstanceC),
+			},
+			make(map[string]cloudprovider.Instance),
 		},
 		{
 			"more nodes in cloud provider than kube",
@@ -90,18 +93,23 @@ func TestFindOffendingNodes(t *testing.T) {
 				dummyInstanceB.providerID: &dummyInstanceB,
 				dummyInstanceC.providerID: &dummyInstanceC,
 			},
-			[]string{},
-			[]string{"id \"aws:///us-east-1c/i-cbcdefghijk\" in \"GroupC\""},
+			make(map[string]corev1.Node),
+			map[string]cloudprovider.Instance{
+				dummyInstanceC.providerID: &dummyInstanceC,
+			},
 		},
 		{
 			"no nodes in cloud provider",
 			map[string]corev1.Node{
 				dummyInstanceA.providerID: buildNode(dummyInstanceA),
 				dummyInstanceB.providerID: buildNode(dummyInstanceB),
 			},
-			map[string]cloudprovider.Instance{},
-			[]string{"id \"aws:///us-east-1a/i-abcdefghijk\"", "id \"aws:///us-east-1b/i-bbcdefghijk\""},
-			[]string{},
+			make(map[string]cloudprovider.Instance),
+			map[string]corev1.Node{
+				dummyInstanceA.providerID: buildNode(dummyInstanceA),
+				dummyInstanceB.providerID: buildNode(dummyInstanceB),
+			},
+			make(map[string]cloudprovider.Instance),
 		},
 		{
 			"no nodes in kube",
@@ -110,23 +118,26 @@ func TestFindOffendingNodes(t *testing.T) {
 				dummyInstanceA.providerID: &dummyInstanceA,
 				dummyInstanceB.providerID: &dummyInstanceB,
 			},
-			[]string{},
-			[]string{"id \"aws:///us-east-1a/i-abcdefghijk\" in \"GroupA\"", "id \"aws:///us-east-1b/i-bbcdefghijk\" in \"GroupB\""},
+			make(map[string]corev1.Node),
+			map[string]cloudprovider.Instance{
+				dummyInstanceA.providerID: &dummyInstanceA,
+				dummyInstanceB.providerID: &dummyInstanceB,
+			},
 		},
 		{
 			"both cloud provider and kube nodes are empty",
 			make(map[string]corev1.Node),
-			map[string]cloudprovider.Instance{},
-			[]string{},
-			[]string{},
+			make(map[string]cloudprovider.Instance),
+			make(map[string]corev1.Node),
+			make(map[string]cloudprovider.Instance),
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			nodesNotInCPNodeGroup, nodesNotInKube := findOffendingNodes(test.knodes, test.cnodes)
-			assert.ElementsMatch(t, test.expectNotInCPNodeGroup, nodesNotInCPNodeGroup)
-			assert.ElementsMatch(t, test.expectNotInKube, nodesNotInKube)
+			assert.Equal(t, true, reflect.DeepEqual(test.expectNotInCPNodeGroup, nodesNotInCPNodeGroup))
+			assert.Equal(t, true, reflect.DeepEqual(test.expectNotInKube, nodesNotInKube))
 		})
 	}
 }