Do not remove healthy nodes from partially failing zero-or-max-scaling node pool scale-ups

Author: adamoldak

cluster-autoscaler/config/autoscaling_options.go

@@ -52,6 +52,8 @@ type NodeGroupAutoscalingOptions struct {
 	MaxNodeProvisionTime time.Duration
 	// ZeroOrMaxNodeScaling means that a node group should be scaled up to maximum size or down to zero nodes all at once instead of one-by-one.
 	ZeroOrMaxNodeScaling bool
+	// AllowNonAtomicScaleUpToMax indicates that partially failing scale-ups of ZeroOrMaxNodeScaling node groups should not be cancelled
+	AllowNonAtomicScaleUpToMax bool
 	// IgnoreDaemonSetsUtilization sets if daemonsets utilization should be considered during node scale-down
 	IgnoreDaemonSetsUtilization bool
 }

cluster-autoscaler/core/static_autoscaler.go

@@ -801,6 +801,10 @@ func (a *StaticAutoscaler) removeOldUnregisteredNodes(allUnregisteredNodes []clu
 			continue
 		}
 
+		if len(nodesToDelete) == 0 {
+			continue
+		}
+
 		if a.ForceDeleteLongUnregisteredNodes {
 			err = nodeGroup.ForceDeleteNodes(nodesToDelete)
 			if err == cloudprovider.ErrNotImplemented {
@@ -880,12 +884,14 @@ func (a *StaticAutoscaler) deleteCreatedNodesWithErrors() {
 		if nodeGroup == nil {
 			err = fmt.Errorf("node group %s not found", nodeGroupId)
 		} else if nodesToDelete, err = overrideNodesToDeleteForZeroOrMax(a.NodeGroupDefaults, nodeGroup, nodesToDelete); err == nil {
-			err = nodeGroup.DeleteNodes(nodesToDelete)
+			if len(nodesToDelete) > 0 {
+				err = nodeGroup.DeleteNodes(nodesToDelete)
+			}
 		}
 
 		if err != nil {
 			klog.Warningf("Error while trying to delete nodes from %v: %v", nodeGroupId, err)
-		} else {
+		} else if len(nodesToDelete) > 0 {
 			deletedAny = true
 			a.clusterStateRegistry.InvalidateNodeInstancesCacheEntry(nodeGroup)
 		}
@@ -898,21 +904,29 @@ func (a *StaticAutoscaler) deleteCreatedNodesWithErrors() {
 }
 
 // overrideNodesToDeleteForZeroOrMax returns a list of nodes to delete, taking into account that
-// node deletion for a "ZeroOrMaxNodeScaling" node group is atomic and should delete all nodes.
+// node deletion for a "ZeroOrMaxNodeScaling" should either keep or remove all the nodes.
 // For a non-"ZeroOrMaxNodeScaling" node group it returns the unchanged list of nodes to delete.
 func overrideNodesToDeleteForZeroOrMax(defaults config.NodeGroupAutoscalingOptions, nodeGroup cloudprovider.NodeGroup, nodesToDelete []*apiv1.Node) ([]*apiv1.Node, error) {
 	opts, err := nodeGroup.GetOptions(defaults)
 	if err != nil && err != cloudprovider.ErrNotImplemented {
 		return []*apiv1.Node{}, fmt.Errorf("Failed to get node group options for %s: %s", nodeGroup.Id(), err)
 	}
 	// If a scale-up of "ZeroOrMaxNodeScaling" node group failed, the cleanup
-	// should stick to the all-or-nothing principle. Deleting all nodes.
+	// node deletion for a "ZeroOrMaxNodeScaling" node group is atomic and should delete all nodes or none.
 	if opts != nil && opts.ZeroOrMaxNodeScaling {
 		instances, err := nodeGroup.Nodes()
 		if err != nil {
 			return []*apiv1.Node{}, fmt.Errorf("Failed to fill in nodes to delete from group %s based on ZeroOrMaxNodeScaling option: %s", nodeGroup.Id(), err)
 		}
-		return instancesToFakeNodes(instances), nil
+
+		// Remove all nodes in case when either:
+		// 1. All nodes are failing
+		// 2. AllowNonAtomicScaleUpToMax is false which means we want to atomically remove partially failed node groups
+		if len(instances) == len(nodesToDelete) || !opts.AllowNonAtomicScaleUpToMax {
+			// Remove all nodes
+			return instancesToFakeNodes(instances), nil
+		}
+		return []*apiv1.Node{}, nil
 	}
 	// No override needed.
 	return nodesToDelete, nil