Merge pull request kubernetes#5118 from x13n/scaledown

Extract scale down eligibility checking to a separate object

Author: k8s-ci-robot

cluster-autoscaler/core/scaledown/actuation/actuator.go

@@ -221,7 +221,7 @@ func (a *Actuator) scaleDownNodeToReport(node *apiv1.Node, drain bool) (*status.
 	if err != nil {
 		return nil, err
 	}
-	utilInfo, err := utilization.Calculate(node, nodeInfo, a.ctx.IgnoreDaemonSetsUtilization, a.ctx.IgnoreMirrorPodsUtilization, a.ctx.CloudProvider.GPULabel(), time.Now())
+	utilInfo, err := utilization.Calculate(nodeInfo, a.ctx.IgnoreDaemonSetsUtilization, a.ctx.IgnoreMirrorPodsUtilization, a.ctx.CloudProvider.GPULabel(), time.Now())
 	if err != nil {
 		return nil, err
 	}

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

@@ -0,0 +1,181 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package eligibility
+
+import (
+	"reflect"
+	"time"
+
+	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
+	"k8s.io/autoscaler/cluster-autoscaler/context"
+	"k8s.io/autoscaler/cluster-autoscaler/core/scaledown/actuation"
+	"k8s.io/autoscaler/cluster-autoscaler/core/scaledown/unremovable"
+	"k8s.io/autoscaler/cluster-autoscaler/simulator"
+	"k8s.io/autoscaler/cluster-autoscaler/simulator/utilization"
+	"k8s.io/autoscaler/cluster-autoscaler/utils/gpu"
+	"k8s.io/autoscaler/cluster-autoscaler/utils/klogx"
+
+	apiv1 "k8s.io/api/core/v1"
+	klog "k8s.io/klog/v2"
+	schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework"
+)
+
+const (
+	// ScaleDownDisabledKey is the name of annotation marking node as not eligible for scale down.
+	ScaleDownDisabledKey = "cluster-autoscaler.kubernetes.io/scale-down-disabled"
+)
+
+// Checker is responsible for deciding which nodes pass the criteria for scale down.
+type Checker struct {
+	thresholdGetter utilizationThresholdGetter
+}
+
+type utilizationThresholdGetter interface {
+	// GetScaleDownUtilizationThreshold returns ScaleDownUtilizationThreshold value that should be used for a given NodeGroup.
+	GetScaleDownUtilizationThreshold(context *context.AutoscalingContext, nodeGroup cloudprovider.NodeGroup) (float64, error)
+	// GetScaleDownGpuUtilizationThreshold returns ScaleDownGpuUtilizationThreshold value that should be used for a given NodeGroup.
+	GetScaleDownGpuUtilizationThreshold(context *context.AutoscalingContext, nodeGroup cloudprovider.NodeGroup) (float64, error)
+}
+
+// NewChecker creates a new Checker object.
+func NewChecker(thresholdGetter utilizationThresholdGetter) *Checker {
+	return &Checker{
+		thresholdGetter: thresholdGetter,
+	}
+}
+
+// FilterOutUnremovable accepts a list of nodes that are candidates for
+// scale down and filters out nodes that cannot be removed, along with node
+// utilization info.
+// TODO(x13n): Node utilization could actually be calculated independently for
+// all nodes and just used here. Next refactor...
+func (c *Checker) FilterOutUnremovable(context *context.AutoscalingContext, scaleDownCandidates []*apiv1.Node, timestamp time.Time, unremovableNodes *unremovable.Nodes) ([]string, map[string]utilization.Info) {
+	unremovableNodes.Update(context.ClusterSnapshot.NodeInfos(), timestamp)
+
+	skipped := 0
+	utilizationMap := make(map[string]utilization.Info)
+	currentlyUnneededNodeNames := make([]string, 0, len(scaleDownCandidates))
+	utilLogsQuota := klogx.NewLoggingQuota(20)
+
+	for _, node := range scaleDownCandidates {
+		nodeInfo, err := context.ClusterSnapshot.NodeInfos().Get(node.Name)
+		if err != nil {
+			klog.Errorf("Can't retrieve scale-down candidate %s from snapshot, err: %v", node.Name, err)
+			unremovableNodes.AddReason(node, simulator.UnexpectedError)
+			continue
+		}
+
+		// Skip nodes that were recently checked.
+		if unremovableNodes.IsRecent(node.Name) {
+			unremovableNodes.AddReason(node, simulator.RecentlyUnremovable)
+			skipped++
+			continue
+		}
+
+		reason, utilInfo := c.unremovableReasonAndNodeUtilization(context, timestamp, nodeInfo, utilLogsQuota)
+		if utilInfo != nil {
+			utilizationMap[node.Name] = *utilInfo
+		}
+		if reason != simulator.NoReason {
+			unremovableNodes.AddReason(node, reason)
+			continue
+		}
+
+		currentlyUnneededNodeNames = append(currentlyUnneededNodeNames, node.Name)
+	}
+
+	klogx.V(4).Over(utilLogsQuota).Infof("Skipped logging utilization for %d other nodes", -utilLogsQuota.Left())
+	if skipped > 0 {
+		klog.V(1).Infof("Scale-down calculation: ignoring %v nodes unremovable in the last %v", skipped, context.AutoscalingOptions.UnremovableNodeRecheckTimeout)
+	}
+	return currentlyUnneededNodeNames, utilizationMap
+}
+
+func (c *Checker) unremovableReasonAndNodeUtilization(context *context.AutoscalingContext, timestamp time.Time, nodeInfo *schedulerframework.NodeInfo, utilLogsQuota *klogx.Quota) (simulator.UnremovableReason, *utilization.Info) {
+	node := nodeInfo.Node()
+
+	// Skip nodes marked to be deleted, if they were marked recently.
+	// Old-time marked nodes are again eligible for deletion - something went wrong with them
+	// and they have not been deleted.
+	if actuation.IsNodeBeingDeleted(node, timestamp) {
+		klog.V(1).Infof("Skipping %s from delete consideration - the node is currently being deleted", node.Name)
+		return simulator.CurrentlyBeingDeleted, nil
+	}
+
+	// Skip nodes marked with no scale down annotation
+	if HasNoScaleDownAnnotation(node) {
+		klog.V(1).Infof("Skipping %s from delete consideration - the node is marked as no scale down", node.Name)
+		return simulator.ScaleDownDisabledAnnotation, nil
+	}
+
+	utilInfo, err := utilization.Calculate(nodeInfo, context.IgnoreDaemonSetsUtilization, context.IgnoreMirrorPodsUtilization, context.CloudProvider.GPULabel(), timestamp)
+	if err != nil {
+		klog.Warningf("Failed to calculate utilization for %s: %v", node.Name, err)
+	}
+
+	nodeGroup, err := context.CloudProvider.NodeGroupForNode(node)
+	if err != nil {
+		klog.Warning("Node group not found for node %v: %v", node.Name, err)
+		return simulator.UnexpectedError, nil
+	}
+	if nodeGroup == nil || reflect.ValueOf(nodeGroup).IsNil() {
+		// We should never get here as non-autoscaled nodes should not be included in scaleDownCandidates list
+		// (and the default PreFilteringScaleDownNodeProcessor would indeed filter them out).
+		klog.Warningf("Skipped %s from delete consideration - the node is not autoscaled", node.Name)
+		return simulator.NotAutoscaled, nil
+	}
+
+	underutilized, err := c.isNodeBelowUtilizationThreshold(context, node, nodeGroup, utilInfo)
+	if err != nil {
+		klog.Warningf("Failed to check utilization thresholds for %s: %v", node.Name, err)
+		return simulator.UnexpectedError, nil
+	}
+	if !underutilized {
+		klog.V(4).Infof("Node %s is not suitable for removal - %s utilization too big (%f)", node.Name, utilInfo.ResourceName, utilInfo.Utilization)
+		return simulator.NotUnderutilized, &utilInfo
+	}
+
+	klogx.V(4).UpTo(utilLogsQuota).Infof("Node %s - %s utilization %f", node.Name, utilInfo.ResourceName, utilInfo.Utilization)
+
+	return simulator.NoReason, &utilInfo
+}
+
+// isNodeBelowUtilizationThreshold determines if a given node utilization is below threshold.
+func (c *Checker) isNodeBelowUtilizationThreshold(context *context.AutoscalingContext, node *apiv1.Node, nodeGroup cloudprovider.NodeGroup, utilInfo utilization.Info) (bool, error) {
+	var threshold float64
+	var err error
+	if gpu.NodeHasGpu(context.CloudProvider.GPULabel(), node) {
+		threshold, err = c.thresholdGetter.GetScaleDownGpuUtilizationThreshold(context, nodeGroup)
+		if err != nil {
+			return false, err
+		}
+	} else {
+		threshold, err = c.thresholdGetter.GetScaleDownUtilizationThreshold(context, nodeGroup)
+		if err != nil {
+			return false, err
+		}
+	}
+	if utilInfo.Utilization >= threshold {
+		return false, nil
+	}
+	return true, nil
+}
+
+// HasNoScaleDownAnnotation checks whether the node has an annotation blocking it from being scaled down.
+func HasNoScaleDownAnnotation(node *apiv1.Node) bool {
+	return node.Annotations[ScaleDownDisabledKey] == "true"
+}

cluster-autoscaler/core/scaledown/eligibility/eligibility_test.go

@@ -0,0 +1,137 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package eligibility
+
+import (
+	"strconv"
+	"testing"
+	"time"
+
+	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
+	testprovider "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/test"
+	"k8s.io/autoscaler/cluster-autoscaler/config"
+	"k8s.io/autoscaler/cluster-autoscaler/context"
+	"k8s.io/autoscaler/cluster-autoscaler/core/scaledown/unremovable"
+	. "k8s.io/autoscaler/cluster-autoscaler/core/test"
+	"k8s.io/autoscaler/cluster-autoscaler/simulator"
+	"k8s.io/autoscaler/cluster-autoscaler/utils/deletetaint"
+	. "k8s.io/autoscaler/cluster-autoscaler/utils/test"
+
+	"github.com/stretchr/testify/assert"
+	apiv1 "k8s.io/api/core/v1"
+	"k8s.io/client-go/kubernetes/fake"
+)
+
+func TestFilterOutUnremovable(t *testing.T) {
+	now := time.Now()
+
+	regularNode := BuildTestNode("regular", 1000, 10)
+	SetNodeReadyState(regularNode, true, time.Time{})
+
+	justDeletedNode := BuildTestNode("justDeleted", 1000, 10)
+	justDeletedNode.Spec.Taints = []apiv1.Taint{{Key: deletetaint.ToBeDeletedTaint, Value: strconv.FormatInt(now.Unix()-30, 10)}}
+	SetNodeReadyState(justDeletedNode, true, time.Time{})
+
+	tooOldDeletedNode := BuildTestNode("tooOldDeleted", 1000, 10)
+	tooOldDeletedNode.Spec.Taints = []apiv1.Taint{{Key: deletetaint.ToBeDeletedTaint, Value: strconv.FormatInt(now.Unix()-301, 10)}}
+	SetNodeReadyState(tooOldDeletedNode, true, time.Time{})
+
+	noScaleDownNode := BuildTestNode("noScaleDown", 1000, 10)
+	noScaleDownNode.Annotations = map[string]string{ScaleDownDisabledKey: "true"}
+	SetNodeReadyState(noScaleDownNode, true, time.Time{})
+
+	bigPod := BuildTestPod("bigPod", 600, 0)
+	bigPod.Spec.NodeName = "regular"
+
+	smallPod := BuildTestPod("smallPod", 100, 0)
+	smallPod.Spec.NodeName = "regular"
+
+	testCases := []struct {
+		desc  string
+		nodes []*apiv1.Node
+		pods  []*apiv1.Pod
+		want  []string
+	}{
+		{
+			desc:  "regular node stays",
+			nodes: []*apiv1.Node{regularNode},
+			want:  []string{"regular"},
+		},
+		{
+			desc:  "recently deleted node is filtered out",
+			nodes: []*apiv1.Node{regularNode, justDeletedNode},
+			want:  []string{"regular"},
+		},
+		{
+			desc:  "deleted long time ago stays",
+			nodes: []*apiv1.Node{regularNode, tooOldDeletedNode},
+			want:  []string{"regular", "tooOldDeleted"},
+		},
+		{
+			desc:  "marked no scale down is filtered out",
+			nodes: []*apiv1.Node{noScaleDownNode, regularNode},
+			want:  []string{"regular"},
+		},
+		{
+			desc:  "highly utilized node is filtered out",
+			nodes: []*apiv1.Node{regularNode},
+			pods:  []*apiv1.Pod{bigPod},
+			want:  []string{},
+		},
+		{
+			desc:  "underutilized node stays",
+			nodes: []*apiv1.Node{regularNode},
+			pods:  []*apiv1.Pod{smallPod},
+			want:  []string{"regular"},
+		},
+	}
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.desc, func(t *testing.T) {
+			t.Parallel()
+			c := NewChecker(&staticThresholdGetter{0.5})
+			options := config.AutoscalingOptions{
+				UnremovableNodeRecheckTimeout: 5 * time.Minute,
+			}
+			provider := testprovider.NewTestCloudProvider(nil, nil)
+			provider.AddNodeGroup("ng1", 1, 10, 2)
+			for _, n := range tc.nodes {
+				provider.AddNode("ng1", n)
+			}
+			context, err := NewScaleTestAutoscalingContext(options, &fake.Clientset{}, nil, provider, nil, nil)
+			simulator.InitializeClusterSnapshotOrDie(t, context.ClusterSnapshot, tc.nodes, tc.pods)
+			if err != nil {
+				t.Fatalf("Could not create autoscaling context: %v", err)
+			}
+			unremovableNodes := unremovable.NewNodes()
+			got, _ := c.FilterOutUnremovable(&context, tc.nodes, now, unremovableNodes)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
+type staticThresholdGetter struct {
+	threshold float64
+}
+
+func (s *staticThresholdGetter) GetScaleDownUtilizationThreshold(_ *context.AutoscalingContext, _ cloudprovider.NodeGroup) (float64, error) {
+	return s.threshold, nil
+}
+
+func (s *staticThresholdGetter) GetScaleDownGpuUtilizationThreshold(_ *context.AutoscalingContext, _ cloudprovider.NodeGroup) (float64, error) {
+	return s.threshold, nil
+}