Merge pull request #280 from atlassian/aqasimi/fix-scale-down

fix scale down logic

Author: aqasimi

pkg/controller/controller.go

@@ -253,8 +253,7 @@ func (c *Controller) scaleNodeGroup(nodegroup string, nodeGroup *NodeGroupState)
 	metrics.NodeGroupNodesForceTainted.WithLabelValues(nodegroup).Set(float64(len(forceTaintedNodes)))
 	metrics.NodeGroupPods.WithLabelValues(nodegroup).Set(float64(len(pods)))
 
-	// We want to be really simple right now so we don't do anything if we are outside the range of allowed nodes
-	// We assume it is a config error or something bad has gone wrong in the cluster
+	// We dont need to handle the case where node count <  minimum, but we do handle the case where node count > maximum
 
 	if len(allNodes) == 0 && len(pods) == 0 {
 		log.WithField("nodegroup", nodegroup).Info("no pods requests and remain 0 node for node group")
@@ -270,15 +269,6 @@ func (c *Controller) scaleNodeGroup(nodegroup string, nodeGroup *NodeGroupState)
 		)
 		return 0, err
 	}
-	if len(allNodes) > nodeGroup.Opts.MaxNodes {
-		err = errors.New("node count larger than the maximum")
-		log.WithField("nodegroup", nodegroup).Warningf(
-			"Node count of %v larger than maximum of %v",
-			len(allNodes),
-			nodeGroup.Opts.MaxNodes,
-		)
-		return 0, err
-	}
 
 	// update the map of node to nodeinfo
 	// for working out which pods are on which nodes
@@ -343,7 +333,7 @@ func (c *Controller) scaleNodeGroup(nodegroup string, nodeGroup *NodeGroupState)
 	}
 
 	// Metrics
-	log.WithField("nodegroup", nodegroup).Infof("cpu: %v, memory: %v", cpuPercent, memPercent)
+	log.WithField("nodegroup", nodegroup).Infof("cpu: %.2f%%, memory: %.2f%%", cpuPercent, memPercent)
 
 	// on the case that we're scaling up from 0, emit 0 as the metrics to keep metrics sane
 	if cpuPercent == math.MaxFloat64 || memPercent == math.MaxFloat64 {
@@ -407,6 +397,16 @@ func (c *Controller) scaleNodeGroup(nodegroup string, nodeGroup *NodeGroupState)
 		nodesDelta = int(math.Max(float64(nodesDelta), 1))
 	}
 
+	// Check if desired was set to higher than max
+	if len(untaintedNodes) > nodeGroup.Opts.MaxNodes {
+		excessNodes := len(untaintedNodes) - nodeGroup.Opts.MaxNodes
+		log.WithField("nodegroup", nodegroup).
+			Infof("Node count %v exceeds maximum %v. Forcing scale down of %v nodes",
+				len(untaintedNodes), nodeGroup.Opts.MaxNodes, excessNodes)
+		// Scale down at least the excess amount
+		nodesDelta = int(math.Min(float64(nodesDelta), float64(-excessNodes)))
+	}
+
 	log.WithField("nodegroup", nodegroup).Debugf("Delta: %v", nodesDelta)
 
 	scaleOptions := scaleOpts{

pkg/controller/controller_scale_node_group_test.go

@@ -324,7 +324,7 @@ func TestScaleNodeGroup(t *testing.T) {
 			nil,
 		},
 		{
-			"node count less than the minimum",
+			"node count less than the minimum, should return error",
 			args{
 				nodeArgs{1, 0, 0},
 				buildTestPods(0, 0, 0),
@@ -338,21 +338,6 @@ func TestScaleNodeGroup(t *testing.T) {
 			0,
 			errors.New("node count less than the minimum"),
 		},
-		{
-			"node count larger than the maximum",
-			args{
-				nodeArgs{10, 0, 0},
-				buildTestPods(0, 0, 0),
-				NodeGroupOptions{
-					Name:                   "default",
-					CloudProviderGroupName: "default",
-					MaxNodes:               5,
-				},
-				ListerOptions{},
-			},
-			0,
-			errors.New("node count larger than the maximum"),
-		},
 		{
 			"node and pod usage/requests",
 			args{
@@ -1061,6 +1046,211 @@ func TestScaleNodeGroup_MultipleRuns(t *testing.T) {
 	}
 }
 
+// TestScaleDesiredNodesExceedsMaxNodes tests the scenario where ASG desired is set higher than MaxNodes.
+// The controller should trigger a scale-down to bring nodes back within the MaxNodes limit.
+func TestScaleDesiredNodesExceedsMaxNodes(t *testing.T) {
+	type args struct {
+		nodes            []*v1.Node
+		pods             []*v1.Pod
+		nodeGroupOptions NodeGroupOptions
+		listerOptions    ListerOptions
+	}
+
+	tests := []struct {
+		name              string
+		args              args
+		expectedNodeDelta int
+		err               error
+	}{
+		{
+			"12 nodes exceeds max of 10, should scale down by 2",
+			args{
+				buildTestNodes(12, 2000, 8000),
+				buildTestPods(30, 500, 1000), // 62% CPU utilization, normally no scaling
+				NodeGroupOptions{
+					Name:                               "default",
+					CloudProviderGroupName:             "default",
+					MinNodes:                           5,
+					MaxNodes:                           10,
+					TaintLowerCapacityThresholdPercent: 40,
+					TaintUpperCapacityThresholdPercent: 50,
+					ScaleUpThresholdPercent:            80,
+				},
+				ListerOptions{},
+			},
+			-2,
+			nil,
+		},
+		{
+			"15 nodes exceeds max of 10, should scale down by 5",
+			args{
+				buildTestNodes(15, 2000, 8000),
+				buildTestPods(30, 500, 1000), // 62% CPU utilization, normally no scaling
+				NodeGroupOptions{
+					Name:                               "default",
+					CloudProviderGroupName:             "default",
+					MinNodes:                           5,
+					MaxNodes:                           10,
+					TaintLowerCapacityThresholdPercent: 40,
+					TaintUpperCapacityThresholdPercent: 50,
+					ScaleUpThresholdPercent:            80,
+				},
+				ListerOptions{},
+			},
+			-5,
+			nil,
+		},
+		{
+			"11 nodes exceeds max of 10, should scale down by 1",
+			args{
+				buildTestNodes(11, 2000, 8000),
+				buildTestPods(40, 500, 1000), // 90% CPU utilization, would normally scale up
+				NodeGroupOptions{
+					Name:                               "default",
+					CloudProviderGroupName:             "default",
+					MinNodes:                           5,
+					MaxNodes:                           10,
+					TaintLowerCapacityThresholdPercent: 40,
+					TaintUpperCapacityThresholdPercent: 50,
+					ScaleUpThresholdPercent:            80,
+				},
+				ListerOptions{},
+			},
+			-1, // Even with high utilization, should force scale down 1 node
+			nil,
+		},
+		{
+			"10 nodes equals max, no scale down",
+			args{
+				buildTestNodes(10, 2000, 8000),
+				buildTestPods(26, 500, 1000), // 65% CPU utilization, normally no scaling
+				NodeGroupOptions{
+					Name:                               "default",
+					CloudProviderGroupName:             "default",
+					MinNodes:                           5,
+					MaxNodes:                           10,
+					TaintLowerCapacityThresholdPercent: 40,
+					TaintUpperCapacityThresholdPercent: 50,
+					ScaleUpThresholdPercent:            80,
+				},
+				ListerOptions{},
+			},
+			0,
+			nil,
+		},
+		{
+			"14 nodes, 12 untainted exceed max of 10, scale down by FastNodeRemovalRate below MaxNodes",
+			args{
+				func() []*v1.Node {
+					// 10 untainted + 2 tainted = 12 total nodes
+					untainted := test.BuildTestNodes(12, test.NodeOpts{
+						CPU: 2000,
+						Mem: 8000,
+					})
+					tainted := test.BuildTestNodes(2, test.NodeOpts{
+						CPU:     2000,
+						Mem:     8000,
+						Tainted: true,
+					})
+					return append(untainted, tainted...)  
+				}(),
+				buildTestPods(10, 500, 1000), // 20% CPU utilization, should scale down by FastNodeRemovalRate
+				NodeGroupOptions{
+					Name:                               "default",
+					CloudProviderGroupName:             "default",
+					MinNodes:                           5,
+					MaxNodes:                           10,
+					TaintLowerCapacityThresholdPercent: 40,
+					TaintUpperCapacityThresholdPercent: 50,
+					FastNodeRemovalRate:                4,
+				},
+				ListerOptions{},
+			},
+			-4,
+			nil,
+		},
+		{
+			"12 nodes, 8 untainted dont exceed max of 10, no scale down",
+			args{
+				func() []*v1.Node {
+					// 8 untainted + 4 tainted = 12 total nodes
+					untainted := test.BuildTestNodes(8, test.NodeOpts{
+						CPU: 2000,
+						Mem: 8000,
+					})
+					tainted := test.BuildTestNodes(4, test.NodeOpts{
+						CPU:     2000,
+						Mem:     8000,
+						Tainted: true,
+					})
+					return append(untainted, tainted...)
+				}(),
+				buildTestPods(20, 500, 1000),   // 62% CPU utilization, normally no scaling
+				NodeGroupOptions{
+					Name:                               "default",
+					CloudProviderGroupName:             "default",
+					MinNodes:                           5,
+					MaxNodes:                           10,
+					ScaleUpThresholdPercent:            70,
+					TaintLowerCapacityThresholdPercent: 40,
+					TaintUpperCapacityThresholdPercent: 50,
+				},
+				ListerOptions{},
+			},
+			0,
+			nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			nodeGroups := []NodeGroupOptions{tt.args.nodeGroupOptions}
+			ngName := tt.args.nodeGroupOptions.Name
+			client, opts, err := buildTestClient(tt.args.nodes, tt.args.pods, nodeGroups, tt.args.listerOptions)
+			require.NoError(t, err)
+
+			// 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(
+				tt.args.nodeGroupOptions.CloudProviderGroupName,
+				tt.args.nodeGroupOptions.Name,
+				int64(tt.args.nodeGroupOptions.MinNodes),
+				int64(tt.args.nodeGroupOptions.MaxNodes),
+				int64(len(tt.args.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,
+			}
+
+			nodesDelta, err := controller.scaleNodeGroup(ngName, nodeGroupsState[ngName])
+
+			// Ensure there were no errors
+			if tt.err == nil {
+				require.NoError(t, err)
+			} else {
+				require.EqualError(t, tt.err, err.Error())
+			}
+
+			assert.Equal(t, tt.expectedNodeDelta, nodesDelta)
+		})
+	}
+}
+
 func TestScaleNodeGroupNodeMaxAge(t *testing.T) {
 	buildNode := func(creation stdtime.Time, tainted bool) *v1.Node {
 		return test.BuildTestNode(test.NodeOpts{