autoscaler: fix premature end of binpacking

When PermissionToAddNode gets called without actually adding a new node, the
node counter in the thresholdBasedEstimationLimiter gets out of sync with the
actual number of new nodes. This can happen when the "is the last node empty"
check triggers.

The solution used here is to rearrange the checks so that PermissionToAddNode
is followed by adding a new node. Alternatively, it might also be possible
to pass the current number of nodes as parameter.

cluster-autoscaler/estimator/binpacking_estimator.go

@@ -106,19 +106,23 @@ func (e *BinpackingNodeEstimator) Estimate(
 		}
 
 		if !found {
-			// Stop binpacking if we reach the limit of nodes we can add.
-			// We return the result of the binpacking that we already performed.
-			if !e.limiter.PermissionToAddNode() {
-				break
-			}
-
 			// If the last node we've added is empty and the pod couldn't schedule on it, it wouldn't be able to schedule
 			// on a new node either. There is no point adding more nodes to snapshot in such case, especially because of
 			// performance cost each extra node adds to future FitsAnyNodeMatching calls.
 			if lastNodeName != "" && !newNodesWithPods[lastNodeName] {
 				continue
 			}
 
+			// Stop binpacking if we reach the limit of nodes we can add.
+			// We return the result of the binpacking that we already performed.
+			//
+			// The thresholdBasedEstimationLimiter implementation assumes that for
+			// each call that returns true, one node gets added. Therefore this
+			// must be the last check right before really adding a node.
+			if !e.limiter.PermissionToAddNode() {
+				break
+			}
+
 			// Add new node
 			newNodeName, err := e.addNewNodeToSnapshot(nodeTemplate, newNodeNameIndex)
 			if err != nil {