From 85a28774829741dcfc5024bfff3ab2370e2defd0 Mon Sep 17 00:00:00 2001
From: jgonzalez <jgonzalez@atlassian.com>
Date: Wed, 2 Oct 2019 12:12:35 +1000
Subject: [PATCH] #168: Pass through tainted and untained nodes to edge case
 scale up Tests

---
 pkg/controller/controller.go                  |  8 ++-
 .../controller_scale_node_group_test.go       | 59 +++++++++++++++++++
 2 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index f200f8e3..aa7a397b 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -281,9 +281,11 @@ func (c *Controller) scaleNodeGroup(nodegroup string, nodeGroup *NodeGroupState)
 	if len(untaintedNodes) < nodeGroup.Opts.MinNodes {
 		log.WithField("nodegroup", nodegroup).Warn("There are less untainted nodes than the minimum")
 		result, err := c.ScaleUp(scaleOpts{
-			nodes:      allNodes,
-			nodesDelta: nodeGroup.Opts.MinNodes - len(untaintedNodes),
-			nodeGroup:  nodeGroup,
+			nodes:          allNodes,
+			nodesDelta:     nodeGroup.Opts.MinNodes - len(untaintedNodes),
+			nodeGroup:      nodeGroup,
+			taintedNodes:   taintedNodes,
+			untaintedNodes: untaintedNodes,
 		})
 		if err != nil {
 			log.WithField("nodegroup", nodegroup).Error(err)
diff --git a/pkg/controller/controller_scale_node_group_test.go b/pkg/controller/controller_scale_node_group_test.go
index 57bbe18d..c71ccd8f 100644
--- a/pkg/controller/controller_scale_node_group_test.go
+++ b/pkg/controller/controller_scale_node_group_test.go
@@ -63,6 +63,65 @@ func buildTestClient(nodes []*v1.Node, pods []*v1.Pod, nodeGroups []NodeGroupOpt
 	return client, opts
 }
 
+// Test the edge case where the min nodes gets changed to above the current number of untainted nodes
+// the controller should untaint all tainted nodes to get above the new min ASG size instead of bringing up new nodes first
+func TestUntaintNodeGroupMinNodes(t *testing.T) {
+	t.Run("10 minNodes, 10 tainted, 0 untainted - scale up by untainting", func(t *testing.T) {
+		nodeGroupName := "default"
+		minNodes := 10
+		maxNodes := 20
+		nodeGroups := []NodeGroupOptions{{
+			Name:                    nodeGroupName,
+			MinNodes:                minNodes,
+			MaxNodes:                maxNodes,
+			ScaleUpThresholdPercent: 100,
+		}}
+
+		nodes := test.BuildTestNodes(10, test.NodeOpts{
+			CPU:     1000,
+			Mem:     1000,
+			Tainted: true,
+		})
+
+		client, opts := buildTestClient(nodes, buildTestPods(10, 1000, 1000), nodeGroups, ListerOptions{})
+
+		// For these test cases we only use 1 node group/cloud provider node group
+		nodeGroupSize := 1
+
+		// Create a test (mock) cloud provider
+		testCloudProvider := test.NewCloudProvider(nodeGroupSize)
+		testNodeGroup := test.NewNodeGroup(
+			nodeGroupName,
+			int64(minNodes),
+			int64(maxNodes),
+			int64(len(nodes)),
+		)
+		testCloudProvider.RegisterNodeGroup(testNodeGroup)
+
+		// Create a node group state with the mapping of node groups to the cloud providers node groups
+		nodeGroupsState := BuildNodeGroupsState(nodeGroupsStateOpts{
+			nodeGroups: nodeGroups,
+			client:     *client,
+		})
+
+		controller := &Controller{
+			Client:        client,
+			Opts:          opts,
+			stopChan:      nil,
+			nodeGroups:    nodeGroupsState,
+			cloudProvider: testCloudProvider,
+		}
+
+		_, err := controller.scaleNodeGroup(nodeGroupName, nodeGroupsState[nodeGroupName])
+		assert.NoError(t, err)
+
+		untainted, tainted, _ := controller.filterNodes(nodeGroupsState[nodeGroupName], nodes)
+		// Ensure that the tainted nodes where untainted
+		assert.Equal(t, minNodes, len(untainted))
+		assert.Equal(t, 0, len(tainted))
+	})
+}
+
 //Test if when the cluster has nodes = MaxNodes but some of these nodes are tainted
 // it will untaint them before trying to scale up the cloud provider
 func TestUntaintNodeGroupMaxNodes(t *testing.T) {