Change break condition for lowUtilization

1.Set default CPU/Mem of targetThresholds to 100
2.Stop evicting pods if any resource ran out
2.Change testcase and readme

README.md

@@ -91,7 +91,8 @@ Currently, pods request resource requirements are considered for computing node
 There is another configurable threshold, `targetThresholds`, that is used to compute those potential nodes
 from where pods could be evicted. Any node, between the thresholds, `thresholds` and `targetThresholds` is
 considered appropriately utilized and is not considered for eviction. The threshold, `targetThresholds`,
-can be configured for cpu, memory, and number of pods too in terms of percentage.
+can be configured for cpu, memory, and number of pods too in terms of percentage. Notice that `pods` must be
+configured and if `cpu` or `memory` not configured, they'll be set to default value `100`.
 
 These thresholds, `thresholds` and `targetThresholds`, could be tuned as per your cluster requirements.
 Here is an example of a policy for this strategy:

pkg/descheduler/strategies/lownodeutilization.go

@@ -59,6 +59,13 @@ func LowNodeUtilization(ctx context.Context, client clientset.Interface, strateg
 	if !validateTargetThresholds(targetThresholds) {
 		return
 	}
+	// check if CPU/Mem not set in targetThresholds and set it to 100
+	if _, ok := targetThresholds[v1.ResourceCPU]; !ok {
+		targetThresholds[v1.ResourceCPU] = 100
+	}
+	if _, ok := targetThresholds[v1.ResourceMemory]; !ok {
+		targetThresholds[v1.ResourceMemory] = 100
+	}
 
 	npm := createNodePodsMap(ctx, client, nodes)
 	lowNodes, targetNodes := classifyNodes(npm, thresholds, targetThresholds, evictLocalStoragePods)
@@ -187,16 +194,13 @@ func evictPodsFromTargetNodes(
 		totalPods += ((float64(podsPercentage) * float64(nodeCapacity.Pods().Value())) / 100)
 
 		// totalCPU capacity to be moved
-		if _, ok := targetThresholds[v1.ResourceCPU]; ok {
-			cpuPercentage := targetThresholds[v1.ResourceCPU] - node.usage[v1.ResourceCPU]
-			totalCPU += ((float64(cpuPercentage) * float64(nodeCapacity.Cpu().MilliValue())) / 100)
-		}
+		// CPU and Mem in targetThresholds have already validated in LowNodeUtilization
+		cpuPercentage := targetThresholds[v1.ResourceCPU] - node.usage[v1.ResourceCPU]
+		totalCPU += ((float64(cpuPercentage) * float64(nodeCapacity.Cpu().MilliValue())) / 100)
 
 		// totalMem capacity to be moved
-		if _, ok := targetThresholds[v1.ResourceMemory]; ok {
-			memPercentage := targetThresholds[v1.ResourceMemory] - node.usage[v1.ResourceMemory]
-			totalMem += ((float64(memPercentage) * float64(nodeCapacity.Memory().Value())) / 100)
-		}
+		memPercentage := targetThresholds[v1.ResourceMemory] - node.usage[v1.ResourceMemory]
+		totalMem += ((float64(memPercentage) * float64(nodeCapacity.Memory().Value())) / 100)
 	}
 
 	klog.V(1).Infof("Total capacity to be moved: CPU:%v, Mem:%v, Pods:%v", totalCPU, totalMem, totalPods)
@@ -249,7 +253,8 @@ func evictPods(
 	taintsOfLowNodes map[string][]v1.Taint,
 	podEvictor *evictions.PodEvictor,
 	node *v1.Node) {
-	if IsNodeAboveTargetUtilization(nodeUsage, targetThresholds) && (*totalPods > 0 || *totalCPU > 0 || *totalMem > 0) {
+	// stop if node utilization drops below target threshold or any of required capacity (cpu, memory, pods) is moved
+	if IsNodeAboveTargetUtilization(nodeUsage, targetThresholds) && *totalPods > 0 && *totalCPU > 0 && *totalMem > 0 {
 		onePodPercentage := api.Percentage((float64(1) * 100) / float64(nodeCapacity.Pods().Value()))
 		for _, pod := range inputPods {
 			if !utils.PodToleratesTaints(pod, taintsOfLowNodes) {
@@ -280,8 +285,8 @@ func evictPods(
 				nodeUsage[v1.ResourceMemory] -= api.Percentage(float64(mUsage) / float64(nodeCapacity.Memory().Value()) * 100)
 
 				klog.V(3).Infof("updated node usage: %#v", nodeUsage)
-				// check if node utilization drops below target threshold or required capacity (cpu, memory, pods) is moved
-				if !IsNodeAboveTargetUtilization(nodeUsage, targetThresholds) || (*totalPods <= 0 && *totalCPU <= 0 && *totalMem <= 0) {
+				// check if node utilization drops below target threshold or any required capacity (cpu, memory, pods) is moved
+				if !IsNodeAboveTargetUtilization(nodeUsage, targetThresholds) || *totalPods <= 0 || *totalCPU <= 0 || *totalMem <= 0 {
 					break
 				}
 			}

pkg/descheduler/strategies/lownodeutilization_test.go

@@ -80,8 +80,11 @@ func TestLowNodeUtilization(t *testing.T) {
 		thresholds, targetThresholds api.ResourceThresholds
 		nodes                        map[string]*v1.Node
 		pods                         map[string]*v1.PodList
-		expectedPodsEvicted          int
-		evictedPods                  []string
+		// TODO: divide expectedPodsEvicted into two params like other tests
+		// expectedPodsEvicted should be the result num of pods that this testCase expected but now it represents both
+		// MaxNoOfPodsToEvictPerNode and the test's expected result
+		expectedPodsEvicted int
+		evictedPods         []string
 	}{
 		{
 			name: "without priorities",
@@ -141,6 +144,65 @@ func TestLowNodeUtilization(t *testing.T) {
 			},
 			expectedPodsEvicted: 3,
 		},
+		{
+			name: "without priorities stop when cpu capacity is depleted",
+			thresholds: api.ResourceThresholds{
+				v1.ResourceCPU:  30,
+				v1.ResourcePods: 30,
+			},
+			targetThresholds: api.ResourceThresholds{
+				v1.ResourceCPU:  50,
+				v1.ResourcePods: 50,
+			},
+			nodes: map[string]*v1.Node{
+				n1NodeName: test.BuildTestNode(n1NodeName, 4000, 3000, 9, nil),
+				n2NodeName: test.BuildTestNode(n2NodeName, 4000, 3000, 10, nil),
+				n3NodeName: test.BuildTestNode(n3NodeName, 4000, 3000, 10, setNodeUnschedulable),
+			},
+			pods: map[string]*v1.PodList{
+				n1NodeName: {
+					Items: []v1.Pod{
+						*test.BuildTestPod("p1", 400, 300, n1NodeName, setRSOwnerRef),
+						*test.BuildTestPod("p2", 400, 300, n1NodeName, setRSOwnerRef),
+						*test.BuildTestPod("p3", 400, 300, n1NodeName, setRSOwnerRef),
+						*test.BuildTestPod("p4", 400, 300, n1NodeName, setRSOwnerRef),
+						*test.BuildTestPod("p5", 400, 300, n1NodeName, setRSOwnerRef),
+						// These won't be evicted.
+						*test.BuildTestPod("p6", 400, 300, n1NodeName, setDSOwnerRef),
+						*test.BuildTestPod("p7", 400, 300, n1NodeName, func(pod *v1.Pod) {
+							// A pod with local storage.
+							setNormalOwnerRef(pod)
+							pod.Spec.Volumes = []v1.Volume{
+								{
+									Name: "sample",
+									VolumeSource: v1.VolumeSource{
+										HostPath: &v1.HostPathVolumeSource{Path: "somePath"},
+										EmptyDir: &v1.EmptyDirVolumeSource{
+											SizeLimit: resource.NewQuantity(int64(10), resource.BinarySI)},
+									},
+								},
+							}
+							// A Mirror Pod.
+							pod.Annotations = test.GetMirrorPodAnnotation()
+						}),
+						*test.BuildTestPod("p8", 400, 300, n1NodeName, func(pod *v1.Pod) {
+							// A Critical Pod.
+							pod.Namespace = "kube-system"
+							priority := utils.SystemCriticalPriority
+							pod.Spec.Priority = &priority
+						}),
+					},
+				},
+				n2NodeName: {
+					Items: []v1.Pod{
+						*test.BuildTestPod("p9", 400, 2100, n1NodeName, setRSOwnerRef),
+					},
+				},
+				n3NodeName: {},
+			},
+			// 4 pods available for eviction based on v1.ResourcePods, only 3 pods can be evicted before cpu is depleted
+			expectedPodsEvicted: 3,
+		},
 		{
 			name: "with priorities",
 			thresholds: api.ResourceThresholds{
@@ -346,11 +408,6 @@ func TestLowNodeUtilization(t *testing.T) {
 				nodes = append(nodes, node)
 			}
 
-			npm := createNodePodsMap(ctx, fakeClient, nodes)
-			lowNodes, targetNodes := classifyNodes(npm, test.thresholds, test.targetThresholds, false)
-			if len(lowNodes) != 1 {
-				t.Errorf("After ignoring unschedulable nodes, expected only one node to be under utilized.")
-			}
 			podEvictor := evictions.NewPodEvictor(
 				fakeClient,
 				"v1",
@@ -359,7 +416,17 @@ func TestLowNodeUtilization(t *testing.T) {
 				nodes,
 			)
 
-			evictPodsFromTargetNodes(ctx, targetNodes, lowNodes, test.targetThresholds, false, podEvictor)
+			strategy := api.DeschedulerStrategy{
+				Enabled: true,
+				Params: api.StrategyParameters{
+					NodeResourceUtilizationThresholds: &api.NodeResourceUtilizationThresholds{
+						Thresholds: test.thresholds,
+						TargetThresholds: test.targetThresholds,
+					},
+				},
+			}
+			LowNodeUtilization(ctx, fakeClient, strategy, nodes, false, podEvictor)
+
 			podsEvicted := podEvictor.TotalEvicted()
 			if test.expectedPodsEvicted != podsEvicted {
 				t.Errorf("Expected %#v pods to be evicted but %#v got evicted", test.expectedPodsEvicted, podsEvicted)