Merge pull request #6165 from pohly/binpacking-fix

autoscaler: fix premature end of binpacking

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 {