Repository: kubernetes-sigs/descheduler
Pull request #1541 (2024-11-11 15:24:42)

Update nodes sorting function to respect available resources
So `sortNodesByUsage` can be invoked over any list of resources

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please ask for approval from [ingvagabund](https://github.com/ingvagabund). For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fdescheduler).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/descheduler/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["ingvagabund"]} -->

Patch:
diff --git a/pkg/framework/plugins/nodeutilization/nodeutilization.go b/pkg/framework/plugins/nodeutilization/nodeutilization.go
index b3a352aad..7b052aa0c 100644
--- a/pkg/framework/plugins/nodeutilization/nodeutilization.go
+++ b/pkg/framework/plugins/nodeutilization/nodeutilization.go
@@ -368,8 +368,22 @@ func evictPods(
 // sortNodesByUsage sorts nodes based on usage according to the given plugin.
 func sortNodesByUsage(nodes []NodeInfo, ascending bool) {
 	sort.Slice(nodes, func(i, j int) bool {
-		ti := nodes[i].usage[v1.ResourceMemory].Value() + nodes[i].usage[v1.ResourceCPU].MilliValue() + nodes[i].usage[v1.ResourcePods].Value()
-		tj := nodes[j].usage[v1.ResourceMemory].Value() + nodes[j].usage[v1.ResourceCPU].MilliValue() + nodes[j].usage[v1.ResourcePods].Value()
+		ti := resource.NewQuantity(0, resource.DecimalSI).Value()
+		tj := resource.NewQuantity(0, resource.DecimalSI).Value()
+		for resourceName := range nodes[i].usage {
+			if resourceName == v1.ResourceCPU {
+				ti += nodes[i].usage[resourceName].MilliValue()
+			} else {
+				ti += nodes[i].usage[resourceName].Value()
+			}
+		}
+		for resourceName := range nodes[j].usage {
+			if resourceName == v1.ResourceCPU {
+				tj += nodes[j].usage[resourceName].MilliValue()
+			} else {
+				tj += nodes[j].usage[resourceName].Value()
+			}
+		}
 
 		// extended resources
 		for name := range nodes[i].usage {


Test patch:
diff --git a/pkg/framework/plugins/nodeutilization/nodeutilization_test.go b/pkg/framework/plugins/nodeutilization/nodeutilization_test.go
index 0107d8af3..1703fd492 100644
--- a/pkg/framework/plugins/nodeutilization/nodeutilization_test.go
+++ b/pkg/framework/plugins/nodeutilization/nodeutilization_test.go
@@ -25,59 +25,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-var (
-	lowPriority      = int32(0)
-	highPriority     = int32(10000)
-	extendedResource = v1.ResourceName("example.com/foo")
-	testNode1        = NodeInfo{
-		NodeUsage: NodeUsage{
-			node: &v1.Node{
-				Status: v1.NodeStatus{
-					Capacity: v1.ResourceList{
-						v1.ResourceCPU:    *resource.NewMilliQuantity(2000, resource.DecimalSI),
-						v1.ResourceMemory: *resource.NewQuantity(3977868*1024, resource.BinarySI),
-						v1.ResourcePods:   *resource.NewQuantity(29, resource.BinarySI),
-					},
-					Allocatable: v1.ResourceList{
-						v1.ResourceCPU:    *resource.NewMilliQuantity(1930, resource.DecimalSI),
-						v1.ResourceMemory: *resource.NewQuantity(3287692*1024, resource.BinarySI),
-						v1.ResourcePods:   *resource.NewQuantity(29, resource.BinarySI),
-					},
-				},
-				ObjectMeta: metav1.ObjectMeta{Name: "node1"},
-			},
-			usage: map[v1.ResourceName]*resource.Quantity{
-				v1.ResourceCPU:    resource.NewMilliQuantity(1730, resource.DecimalSI),
-				v1.ResourceMemory: resource.NewQuantity(3038982964, resource.BinarySI),
-				v1.ResourcePods:   resource.NewQuantity(25, resource.BinarySI),
-			},
-		},
-	}
-	testNode2 = NodeInfo{
-		NodeUsage: NodeUsage{
-			node: &v1.Node{
-				Status: v1.NodeStatus{
-					Capacity: v1.ResourceList{
-						v1.ResourceCPU:    *resource.NewMilliQuantity(2000, resource.DecimalSI),
-						v1.ResourceMemory: *resource.NewQuantity(3977868*1024, resource.BinarySI),
-						v1.ResourcePods:   *resource.NewQuantity(29, resource.BinarySI),
-					},
-					Allocatable: v1.ResourceList{
-						v1.ResourceCPU:    *resource.NewMilliQuantity(1930, resource.DecimalSI),
-						v1.ResourceMemory: *resource.NewQuantity(3287692*1024, resource.BinarySI),
-						v1.ResourcePods:   *resource.NewQuantity(29, resource.BinarySI),
-					},
-				},
-				ObjectMeta: metav1.ObjectMeta{Name: "node2"},
-			},
-			usage: map[v1.ResourceName]*resource.Quantity{
-				v1.ResourceCPU:    resource.NewMilliQuantity(1220, resource.DecimalSI),
-				v1.ResourceMemory: resource.NewQuantity(3038982964, resource.BinarySI),
-				v1.ResourcePods:   resource.NewQuantity(11, resource.BinarySI),
-			},
-		},
-	}
-	testNode3 = NodeInfo{
+func BuildTestNodeInfo(name string, apply func(*NodeInfo)) *NodeInfo {
+	nodeInfo := &NodeInfo{
 		NodeUsage: NodeUsage{
 			node: &v1.Node{
 				Status: v1.NodeStatus{
@@ -92,15 +41,18 @@ var (
 						v1.ResourcePods:   *resource.NewQuantity(29, resource.BinarySI),
 					},
 				},
-				ObjectMeta: metav1.ObjectMeta{Name: "node3"},
-			},
-			usage: map[v1.ResourceName]*resource.Quantity{
-				v1.ResourceCPU:    resource.NewMilliQuantity(1530, resource.DecimalSI),
-				v1.ResourceMemory: resource.NewQuantity(5038982964, resource.BinarySI),
-				v1.ResourcePods:   resource.NewQuantity(20, resource.BinarySI),
+				ObjectMeta: metav1.ObjectMeta{Name: name},
 			},
 		},
 	}
+	apply(nodeInfo)
+	return nodeInfo
+}
+
+var (
+	lowPriority      = int32(0)
+	highPriority     = int32(10000)
+	extendedResource = v1.ResourceName("example.com/foo")
 )
 
 func TestResourceUsagePercentages(t *testing.T) {
@@ -141,26 +93,81 @@ func TestResourceUsagePercentages(t *testing.T) {
 	t.Logf("resourceUsagePercentage: %#v\n", resourceUsagePercentage)
 }
 
-func TestSortNodesByUsageDescendingOrder(t *testing.T) {
-	nodeList := []NodeInfo{testNode1, testNode2, testNode3}
-	expectedNodeList := []NodeInfo{testNode3, testNode1, testNode2} // testNode3 has the highest usage
-	sortNodesByUsage(nodeList, false)                               // ascending=false, sort nodes in descending order
-
-	for i := 0; i < len(expectedNodeList); i++ {
-		if nodeList[i].NodeUsage.node.Name != expectedNodeList[i].NodeUsage.node.Name {
-			t.Errorf("Expected %v, got %v", expectedNodeList[i].NodeUsage.node.Name, nodeList[i].NodeUsage.node.Name)
-		}
+func TestSortNodesByUsage(t *testing.T) {
+	tests := []struct {
+		name                  string
+		nodeInfoList          []NodeInfo
+		expectedNodeInfoNames []string
+	}{
+		{
+			name: "cpu memory pods",
+			nodeInfoList: []NodeInfo{
+				*BuildTestNodeInfo("node1", func(nodeInfo *NodeInfo) {
+					nodeInfo.usage = map[v1.ResourceName]*resource.Quantity{
+						v1.ResourceCPU:    resource.NewMilliQuantity(1730, resource.DecimalSI),
+						v1.ResourceMemory: resource.NewQuantity(3038982964, resource.BinarySI),
+						v1.ResourcePods:   resource.NewQuantity(25, resource.BinarySI),
+					}
+				}),
+				*BuildTestNodeInfo("node2", func(nodeInfo *NodeInfo) {
+					nodeInfo.usage = map[v1.ResourceName]*resource.Quantity{
+						v1.ResourceCPU:    resource.NewMilliQuantity(1220, resource.DecimalSI),
+						v1.ResourceMemory: resource.NewQuantity(3038982964, resource.BinarySI),
+						v1.ResourcePods:   resource.NewQuantity(11, resource.BinarySI),
+					}
+				}),
+				*BuildTestNodeInfo("node3", func(nodeInfo *NodeInfo) {
+					nodeInfo.usage = map[v1.ResourceName]*resource.Quantity{
+						v1.ResourceCPU:    resource.NewMilliQuantity(1530, resource.DecimalSI),
+						v1.ResourceMemory: resource.NewQuantity(5038982964, resource.BinarySI),
+						v1.ResourcePods:   resource.NewQuantity(20, resource.BinarySI),
+					}
+				}),
+			},
+			expectedNodeInfoNames: []string{"node3", "node1", "node2"},
+		},
+		{
+			name: "memory",
+			nodeInfoList: []NodeInfo{
+				*BuildTestNodeInfo("node1", func(nodeInfo *NodeInfo) {
+					nodeInfo.usage = map[v1.ResourceName]*resource.Quantity{
+						v1.ResourceMemory: resource.NewQuantity(3038982964, resource.BinarySI),
+					}
+				}),
+				*BuildTestNodeInfo("node2", func(nodeInfo *NodeInfo) {
+					nodeInfo.usage = map[v1.ResourceName]*resource.Quantity{
+						v1.ResourceMemory: resource.NewQuantity(2038982964, resource.BinarySI),
+					}
+				}),
+				*BuildTestNodeInfo("node3", func(nodeInfo *NodeInfo) {
+					nodeInfo.usage = map[v1.ResourceName]*resource.Quantity{
+						v1.ResourceMemory: resource.NewQuantity(5038982964, resource.BinarySI),
+					}
+				}),
+			},
+			expectedNodeInfoNames: []string{"node3", "node1", "node2"},
+		},
 	}
-}
 
-func TestSortNodesByUsageAscendingOrder(t *testing.T) {
-	nodeList := []NodeInfo{testNode1, testNode2, testNode3}
-	expectedNodeList := []NodeInfo{testNode2, testNode1, testNode3}
-	sortNodesByUsage(nodeList, true) // ascending=true, sort nodes in ascending order
+	for _, tc := range tests {
+		t.Run(tc.name+" descending", func(t *testing.T) {
+			sortNodesByUsage(tc.nodeInfoList, false) // ascending=false, sort nodes in descending order
 
-	for i := 0; i < len(expectedNodeList); i++ {
-		if nodeList[i].NodeUsage.node.Name != expectedNodeList[i].NodeUsage.node.Name {
-			t.Errorf("Expected %v, got %v", expectedNodeList[i].NodeUsage.node.Name, nodeList[i].NodeUsage.node.Name)
-		}
+			for i := 0; i < len(tc.nodeInfoList); i++ {
+				if tc.nodeInfoList[i].NodeUsage.node.Name != tc.expectedNodeInfoNames[i] {
+					t.Errorf("Expected %v, got %v", tc.expectedNodeInfoNames[i], tc.nodeInfoList[i].NodeUsage.node.Name)
+				}
+			}
+		})
+		t.Run(tc.name+" ascending", func(t *testing.T) {
+			sortNodesByUsage(tc.nodeInfoList, true) // ascending=true, sort nodes in ascending order
+
+			size := len(tc.nodeInfoList)
+			for i := 0; i < size; i++ {
+				if tc.nodeInfoList[i].NodeUsage.node.Name != tc.expectedNodeInfoNames[size-i-1] {
+					t.Errorf("Expected %v, got %v", tc.expectedNodeInfoNames[size-i-1], tc.nodeInfoList[i].NodeUsage.node.Name)
+				}
+			}
+		})
 	}
 }