Fix RemovalSimulation for parallel scale down

Author: yaroslava-serdiuk

cluster-autoscaler/core/scaledown/planner/planner.go

@@ -239,7 +239,6 @@ func (p *Planner) injectPods(pods []*apiv1.Pod) error {
 
 // categorizeNodes determines, for each node, whether it can be eventually
 // removed or if there are reasons preventing that.
-// TODO: Track remaining PDB budget.
 func (p *Planner) categorizeNodes(podDestinations map[string]bool, scaleDownCandidates []*apiv1.Node) {
 	unremovableTimeout := p.latestUpdate.Add(p.context.AutoscalingOptions.UnremovableNodeRecheckTimeout)
 	unremovableCount := 0
@@ -251,26 +250,27 @@ func (p *Planner) categorizeNodes(podDestinations map[string]bool, scaleDownCand
 	}
 	p.nodeUtilizationMap = utilizationMap
 	timer := time.NewTimer(p.context.ScaleDownSimulationTimeout)
+RemovalSimulation:
 	for i, node := range currentlyUnneededNodeNames {
 		select {
 		case <-timer.C:
 			klog.Warningf("%d out of %d nodes skipped in scale down simulation due to timeout.", len(currentlyUnneededNodeNames)-i, len(currentlyUnneededNodeNames))
-			break
+			break RemovalSimulation
 		default:
-		}
-		removable, unremovable := p.rs.SimulateNodeRemoval(node, podDestinations, p.latestUpdate, p.context.RemainingPdbTracker.GetPdbs())
-		if removable != nil {
-			_, inParallel, _ := p.context.RemainingPdbTracker.CanRemovePods(removable.PodsToReschedule)
-			if !inParallel {
-				removable.IsRisky = true
+			removable, unremovable := p.rs.SimulateNodeRemoval(node, podDestinations, p.latestUpdate, p.context.RemainingPdbTracker.GetPdbs())
+			if removable != nil {
+				_, inParallel, _ := p.context.RemainingPdbTracker.CanRemovePods(removable.PodsToReschedule)
+				if !inParallel {
+					removable.IsRisky = true
+				}
+				delete(podDestinations, removable.Node.Name)
+				p.context.RemainingPdbTracker.RemovePods(removable.PodsToReschedule)
+				removableList = append(removableList, *removable)
+			}
+			if unremovable != nil {
+				unremovableCount += 1
+				p.unremovableNodes.AddTimeout(unremovable, unremovableTimeout)
 			}
-			delete(podDestinations, removable.Node.Name)
-			p.context.RemainingPdbTracker.RemovePods(removable.PodsToReschedule)
-			removableList = append(removableList, *removable)
-		}
-		if unremovable != nil {
-			unremovableCount += 1
-			p.unremovableNodes.AddTimeout(unremovable, unremovableTimeout)
 		}
 	}
 	p.unneededNodes.Update(removableList, p.latestUpdate)

cluster-autoscaler/core/scaledown/planner/planner_test.go

@@ -451,7 +451,8 @@ func TestUpdateClusterState(t *testing.T) {
 			assert.NoError(t, p.UpdateClusterState(tc.nodes, tc.nodes, tc.actuationStatus, time.Now()))
 			wantUnneeded := asMap(tc.wantUnneeded)
 			for _, n := range tc.nodes {
-				assert.Equal(t, wantUnneeded[n.Name], p.unneededNodes.Contains(n.Name), n.Name)
+				assert.Equal(t, wantUnneeded[n.Name], p.unneededNodes.Contains(n.Name), []string{n.Name, "unneeded"})
+				assert.Equal(t, !wantUnneeded[n.Name], p.unremovableNodes.Contains(n.Name), []string{n.Name, "unremovable"})
 			}
 		})
 	}

cluster-autoscaler/core/scaledown/unremovable/nodes.go

@@ -67,6 +67,12 @@ func (n *Nodes) Update(nodeInfos NodeInfoGetter, timestamp time.Time) {
 	n.ttls = newTTLs
 }
 
+// Contains returns true iff a given node is unremovable.
+func (n *Nodes) Contains(nodeName string) bool {
+	_, found := n.reasons[nodeName]
+	return found
+}
+
 // Add adds an unremovable node.
 func (n *Nodes) Add(node *simulator.UnremovableNode) {
 	n.reasons[node.Node.Name] = node

cluster-autoscaler/core/scaledown/unremovable/nodes_test.go

@@ -25,6 +25,7 @@ import (
 
 	apiv1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	. "k8s.io/autoscaler/cluster-autoscaler/utils/test"
 	schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework"
 )
 
@@ -80,6 +81,21 @@ func TestUpdate(t *testing.T) {
 	}
 }
 
+func TestContains(t *testing.T) {
+	n := NewNodes()
+	nodes := []string{"n1", "n2", "n3"}
+
+	n.Add(makeUnremovableNode(nodes[0]))
+	n.AddTimeout(makeUnremovableNode(nodes[1]), time.Now())
+	n.AddReason(BuildTestNode(nodes[2], 0, 0), simulator.UnremovableReason(1))
+
+	for _, node := range nodes {
+		if !n.Contains(node) {
+			t.Errorf("n.Contains(%s) return false, want true", node)
+		}
+	}
+}
+
 type fakeNodeInfoGetter struct {
 	names map[string]bool
 }

cluster-autoscaler/main.go

@@ -215,7 +215,7 @@ var (
 	skipNodesWithLocalStorage          = flag.Bool("skip-nodes-with-local-storage", true, "If true cluster autoscaler will never delete nodes with pods with local storage, e.g. EmptyDir or HostPath")
 	minReplicaCount                    = flag.Int("min-replica-count", 0, "Minimum number or replicas that a replica set or replication controller should have to allow their pods deletion in scale down")
 	nodeDeleteDelayAfterTaint          = flag.Duration("node-delete-delay-after-taint", 5*time.Second, "How long to wait before deleting a node after tainting it")
-	scaleDownSimulationTimeout         = flag.Duration("scale-down-simulation-timeout", 5*time.Minute, "How long should we run scale down simulation.")
+	scaleDownSimulationTimeout         = flag.Duration("scale-down-simulation-timeout", 30*time.Second, "How long should we run scale down simulation.")
 	parallelDrain                      = flag.Bool("parallel-drain", false, "Whether to allow parallel drain of nodes.")
 	maxCapacityMemoryDifferenceRatio   = flag.Float64("memory-difference-ratio", config.DefaultMaxCapacityMemoryDifferenceRatio, "Maximum difference in memory capacity between two similar node groups to be considered for balancing. Value is a ratio of the smaller node group's memory capacity.")
 	maxFreeDifferenceRatio             = flag.Float64("max-free-difference-ratio", config.DefaultMaxFreeDifferenceRatio, "Maximum difference in free resources between two similar node groups to be considered for balancing. Value is a ratio of the smaller node group's free resource.")