Merge pull request kubernetes#2893 from aleksandra-malinowska/delta-snapshot-24

Minor cleanups in scale-down

Author: k8s-ci-robot

cluster-autoscaler/core/scale_down.go

@@ -465,7 +465,7 @@ func (sd *ScaleDown) UpdateUnneededNodes(
 		klog.V(4).Infof("Node %s - %s utilization %f", node.Name, utilInfo.ResourceName, utilInfo.Utilization)
 		utilizationMap[node.Name] = utilInfo
 
-		if !sd.isNodeBelowUtilzationThreshold(node, utilInfo) {
+		if !sd.isNodeBelowUtilizationThreshold(node, utilInfo) {
 			klog.V(4).Infof("Node %s is not suitable for removal - %s utilization too big (%f)", node.Name, utilInfo.ResourceName, utilInfo.Utilization)
 			sd.addUnremovableNodeReason(node, simulator.NotUnderutilized)
 			continue
@@ -600,8 +600,8 @@ func (sd *ScaleDown) UpdateUnneededNodes(
 	return nil
 }
 
-// isNodeBelowUtilzationThreshold determintes if a given node utilization is blow threshold.
-func (sd *ScaleDown) isNodeBelowUtilzationThreshold(node *apiv1.Node, utilInfo simulator.UtilizationInfo) bool {
+// isNodeBelowUtilizationThreshold determines if a given node utilization is below threshold.
+func (sd *ScaleDown) isNodeBelowUtilizationThreshold(node *apiv1.Node, utilInfo simulator.UtilizationInfo) bool {
 	if gpu.NodeHasGpu(sd.context.CloudProvider.GPULabel(), node) {
 		if utilInfo.Utilization >= sd.context.ScaleDownGpuUtilizationThreshold {
 			return false
@@ -736,7 +736,10 @@ func (sd *ScaleDown) SoftTaintUnneededNodes(allNodes []*apiv1.Node) (errors []er
 
 // TryToScaleDown tries to scale down the cluster. It returns a result inside a ScaleDownStatus indicating if any node was
 // removed and error if such occurred.
-func (sd *ScaleDown) TryToScaleDown(pdbs []*policyv1.PodDisruptionBudget, currentTime time.Time) (*status.ScaleDownStatus, errors.AutoscalerError) {
+func (sd *ScaleDown) TryToScaleDown(
+	currentTime time.Time,
+	pdbs []*policyv1.PodDisruptionBudget,
+) (*status.ScaleDownStatus, errors.AutoscalerError) {
 
 	scaleDownStatus := &status.ScaleDownStatus{NodeDeleteResults: sd.nodeDeletionTracker.GetAndClearNodeDeleteResults()}
 	nodeDeletionDuration := time.Duration(0)

cluster-autoscaler/core/scale_down_test.go

@@ -977,7 +977,7 @@ func TestScaleDown(t *testing.T) {
 	simulator.InitializeClusterSnapshotOrDie(t, context.ClusterSnapshot, nodes, []*apiv1.Pod{p1, p2})
 	autoscalererr = scaleDown.UpdateUnneededNodes(nodes, nodes, time.Now().Add(-5*time.Minute), nil)
 	assert.NoError(t, autoscalererr)
-	scaleDownStatus, err := scaleDown.TryToScaleDown(nil, time.Now())
+	scaleDownStatus, err := scaleDown.TryToScaleDown(time.Now(), nil)
 	waitForDeleteToFinish(t, scaleDown)
 	assert.NoError(t, err)
 	assert.Equal(t, status.ScaleDownNodeDeleteStarted, scaleDownStatus.Result)
@@ -1231,7 +1231,7 @@ func simpleScaleDownEmpty(t *testing.T, config *scaleTestConfig) {
 	simulator.InitializeClusterSnapshotOrDie(t, context.ClusterSnapshot, nodes, []*apiv1.Pod{})
 	autoscalererr = scaleDown.UpdateUnneededNodes(nodes, nodes, time.Now().Add(-5*time.Minute), nil)
 	assert.NoError(t, autoscalererr)
-	scaleDownStatus, err := scaleDown.TryToScaleDown(nil, time.Now())
+	scaleDownStatus, err := scaleDown.TryToScaleDown(time.Now(), nil)
 	assert.False(t, scaleDown.nodeDeletionTracker.IsNonEmptyNodeDeleteInProgress())
 
 	assert.NoError(t, err)
@@ -1317,7 +1317,7 @@ func TestNoScaleDownUnready(t *testing.T) {
 	simulator.InitializeClusterSnapshotOrDie(t, context.ClusterSnapshot, nodes, []*apiv1.Pod{p2})
 	autoscalererr = scaleDown.UpdateUnneededNodes(nodes, nodes, time.Now().Add(-5*time.Minute), nil)
 	assert.NoError(t, autoscalererr)
-	scaleDownStatus, err := scaleDown.TryToScaleDown(nil, time.Now())
+	scaleDownStatus, err := scaleDown.TryToScaleDown(time.Now(), nil)
 	waitForDeleteToFinish(t, scaleDown)
 
 	assert.NoError(t, err)
@@ -1340,7 +1340,7 @@ func TestNoScaleDownUnready(t *testing.T) {
 	simulator.InitializeClusterSnapshotOrDie(t, context.ClusterSnapshot, nodes, []*apiv1.Pod{p2})
 	autoscalererr = scaleDown.UpdateUnneededNodes(nodes, nodes, time.Now().Add(-2*time.Hour), nil)
 	assert.NoError(t, autoscalererr)
-	scaleDownStatus, err = scaleDown.TryToScaleDown(nil, time.Now())
+	scaleDownStatus, err = scaleDown.TryToScaleDown(time.Now(), nil)
 	waitForDeleteToFinish(t, scaleDown)
 
 	assert.NoError(t, err)
@@ -1427,7 +1427,7 @@ func TestScaleDownNoMove(t *testing.T) {
 	simulator.InitializeClusterSnapshotOrDie(t, context.ClusterSnapshot, nodes, []*apiv1.Pod{p1, p2})
 	autoscalererr = scaleDown.UpdateUnneededNodes(nodes, nodes, time.Now().Add(-5*time.Minute), nil)
 	assert.NoError(t, autoscalererr)
-	scaleDownStatus, err := scaleDown.TryToScaleDown(nil, time.Now())
+	scaleDownStatus, err := scaleDown.TryToScaleDown(time.Now(), nil)
 	waitForDeleteToFinish(t, scaleDown)
 
 	assert.NoError(t, err)

cluster-autoscaler/core/static_autoscaler.go

@@ -505,7 +505,7 @@ func (a *StaticAutoscaler) RunOnce(currentTime time.Time) errors.AutoscalerError
 
 			scaleDownStart := time.Now()
 			metrics.UpdateLastTime(metrics.ScaleDown, scaleDownStart)
-			scaleDownStatus, typedErr := scaleDown.TryToScaleDown(pdbs, currentTime)
+			scaleDownStatus, typedErr := scaleDown.TryToScaleDown(currentTime, pdbs)
 			metrics.UpdateDurationFromStart(metrics.ScaleDown, scaleDownStart)
 
 			scaleDownStatus.RemovedNodeGroups = removedNodeGroups

cluster-autoscaler/simulator/cluster.go

@@ -278,14 +278,14 @@ func calculateUtilizationOfResource(node *apiv1.Node, nodeInfo *schedulernodeinf
 }
 
 func findPlaceFor(removedNode string, pods []*apiv1.Pod, nodes map[string]bool,
-	clusterSnaphost ClusterSnapshot, predicateChecker PredicateChecker, oldHints map[string]string, newHints map[string]string, usageTracker *UsageTracker,
+	clusterSnapshot ClusterSnapshot, predicateChecker PredicateChecker, oldHints map[string]string, newHints map[string]string, usageTracker *UsageTracker,
 	timestamp time.Time) error {
 
-	if err := clusterSnaphost.Fork(); err != nil {
+	if err := clusterSnapshot.Fork(); err != nil {
 		return err
 	}
 	defer func() {
-		err := clusterSnaphost.Revert()
+		err := clusterSnapshot.Revert()
 		if err != nil {
 			klog.Fatalf("Got error when calling ClusterSnapshot.Revert(); %v", err)
 		}
@@ -298,13 +298,13 @@ func findPlaceFor(removedNode string, pods []*apiv1.Pod, nodes map[string]bool,
 	loggingQuota := glogx.PodsLoggingQuota()
 
 	tryNodeForPod := func(nodename string, pod *apiv1.Pod) bool {
-		if err := predicateChecker.CheckPredicates(clusterSnaphost, pod, nodename); err != nil {
+		if err := predicateChecker.CheckPredicates(clusterSnapshot, pod, nodename); err != nil {
 			glogx.V(4).UpTo(loggingQuota).Infof("Evaluation %s for %s/%s -> %v", nodename, pod.Namespace, pod.Name, err.VerboseMessage())
 			return false
 		}
 
 		klog.V(4).Infof("Pod %s/%s can be moved to %s", pod.Namespace, pod.Name, nodename)
-		if err := clusterSnaphost.AddPod(pod, nodename); err != nil {
+		if err := clusterSnapshot.AddPod(pod, nodename); err != nil {
 			klog.Errorf("Simulating scheduling of %s/%s to %s return error; %v", pod.Namespace, pod.Name, nodename, err)
 			return false
 		}
@@ -314,9 +314,9 @@ func findPlaceFor(removedNode string, pods []*apiv1.Pod, nodes map[string]bool,
 
 	pods = tpu.ClearTPURequests(pods)
 
-	// remove pods from clusterSnaphot first
+	// remove pods from clusterSnapshot first
 	for _, pod := range pods {
-		if err := clusterSnaphost.RemovePod(pod.Namespace, pod.Name, removedNode); err != nil {
+		if err := clusterSnapshot.RemovePod(pod.Namespace, pod.Name, removedNode); err != nil {
 			// just log error
 			klog.Errorf("Simulating removal of %s/%s return error; %v", pod.Namespace, pod.Name, err)
 		}
@@ -329,17 +329,18 @@ func findPlaceFor(removedNode string, pods []*apiv1.Pod, nodes map[string]bool,
 
 		foundPlace := false
 		targetNode := ""
+
 		loggingQuota.Reset()
 
 		klog.V(5).Infof("Looking for place for %s/%s", pod.Namespace, pod.Name)
 
-		hintedNode, hasHint := oldHints[podKey(pod)]
-		if hasHint {
+		if hintedNode, hasHint := oldHints[podKey(pod)]; hasHint {
 			if hintedNode != removedNode && tryNodeForPod(hintedNode, pod) {
 				foundPlace = true
 				targetNode = hintedNode
 			}
 		}
+
 		if !foundPlace {
 			for nodeName := range nodes {
 				if nodeName == removedNode {