Move sortPodsBasedOnPriority to podutil

README.md

@@ -238,7 +238,8 @@ When the descheduler decides to evict pods from a node, it employs the following
 never evicted because these pods won't be recreated.
 * Pods associated with DaemonSets are never evicted.
 * Pods with local storage are never evicted.
-* Best efforts pods are evicted before burstable and guaranteed pods.
+* In `LowNodeUtilization`, pods are evicted by their priority from low to high, and if they have same priority,
+best effort pods are evicted before burstable and guaranteed pods.
 * All types of pods with the annotation descheduler.alpha.kubernetes.io/evict are evicted. This
   annotation is used to override checks which prevent eviction and users can select which pod is evicted.
   Users should know how and if the pod will be recreated.

pkg/descheduler/pod/pods.go

@@ -23,6 +23,7 @@ import (
 	"k8s.io/apimachinery/pkg/fields"
 	clientset "k8s.io/client-go/kubernetes"
 	"sigs.k8s.io/descheduler/pkg/utils"
+	"sort"
 )
 
 // ListPodsOnANode lists all of the pods on a node
@@ -68,3 +69,27 @@ func IsBurstablePod(pod *v1.Pod) bool {
 func IsGuaranteedPod(pod *v1.Pod) bool {
 	return utils.GetPodQOS(pod) == v1.PodQOSGuaranteed
 }
+
+// SortPodsBasedOnPriorityLowToHigh sorts pods based on their priorities from low to high.
+// If pods have same priorities, they will be sorted by QoS in the following order:
+// BestEffort, Burstable, Guaranteed
+func SortPodsBasedOnPriorityLowToHigh(pods []*v1.Pod) {
+	sort.Slice(pods, func(i, j int) bool {
+		if pods[i].Spec.Priority == nil && pods[j].Spec.Priority != nil {
+			return true
+		}
+		if pods[j].Spec.Priority == nil && pods[i].Spec.Priority != nil {
+			return false
+		}
+		if (pods[j].Spec.Priority == nil && pods[i].Spec.Priority == nil) || (*pods[i].Spec.Priority == *pods[j].Spec.Priority) {
+			if IsBestEffortPod(pods[i]) {
+				return true
+			}
+			if IsBurstablePod(pods[i]) && IsGuaranteedPod(pods[j]) {
+				return true
+			}
+			return false
+		}
+		return *pods[i].Spec.Priority < *pods[j].Spec.Priority
+	})
+}

pkg/descheduler/pod/pods_test.go

@@ -19,6 +19,7 @@ package pod
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"strings"
 	"testing"
 
@@ -29,6 +30,11 @@ import (
 	"sigs.k8s.io/descheduler/test"
 )
 
+var (
+	lowPriority  = int32(0)
+	highPriority = int32(10000)
+)
+
 func TestListPodsOnANode(t *testing.T) {
 	testCases := []struct {
 		name             string
@@ -67,3 +73,43 @@ func TestListPodsOnANode(t *testing.T) {
 		}
 	}
 }
+
+func TestSortPodsBasedOnPriorityLowToHigh(t *testing.T) {
+	n1 := test.BuildTestNode("n1", 4000, 3000, 9, nil)
+
+	p1 := test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) {
+		test.SetPodPriority(pod, lowPriority)
+	})
+
+	// BestEffort
+	p2 := test.BuildTestPod("p2", 400, 0, n1.Name, func(pod *v1.Pod) {
+		test.SetPodPriority(pod, highPriority)
+		test.MakeBestEffortPod(pod)
+	})
+
+	// Burstable
+	p3 := test.BuildTestPod("p3", 400, 0, n1.Name, func(pod *v1.Pod) {
+		test.SetPodPriority(pod, highPriority)
+		test.MakeBurstablePod(pod)
+	})
+
+	// Guaranteed
+	p4 := test.BuildTestPod("p4", 400, 100, n1.Name, func(pod *v1.Pod) {
+		test.SetPodPriority(pod, highPriority)
+		test.MakeGuaranteedPod(pod)
+	})
+
+	// Best effort with nil priorities.
+	p5 := test.BuildTestPod("p5", 400, 100, n1.Name, test.MakeBestEffortPod)
+	p5.Spec.Priority = nil
+
+	p6 := test.BuildTestPod("p6", 400, 100, n1.Name, test.MakeGuaranteedPod)
+	p6.Spec.Priority = nil
+
+	podList := []*v1.Pod{p4, p3, p2, p1, p6, p5}
+
+	SortPodsBasedOnPriorityLowToHigh(podList)
+	if !reflect.DeepEqual(podList[len(podList)-1], p4) {
+		t.Errorf("Expected last pod in sorted list to be %v which of highest priority and guaranteed but got %v", p4, podList[len(podList)-1])
+	}
+}

pkg/descheduler/strategies/lownodeutilization.go

@@ -247,7 +247,7 @@ func evictPodsFromTargetNodes(
 			evictablePods = append(append(burstablePods, bestEffortPods...), guaranteedPods...)
 
 			// sort the evictable Pods based on priority. This also sorts them based on QoS. If there are multiple pods with same priority, they are sorted based on QoS tiers.
-			sortPodsBasedOnPriority(evictablePods)
+			podutil.SortPodsBasedOnPriorityLowToHigh(evictablePods)
 			evictPods(ctx, evictablePods, targetThresholds, nodeCapacity, node.usage, &totalPods, &totalCPU, &totalMem, taintsOfLowNodes, podEvictor, node.node)
 		} else {
 			// TODO: Remove this when we support only priority.
@@ -338,28 +338,6 @@ func sortNodesByUsage(nodes []NodeUsageMap) {
 	})
 }
 
-// sortPodsBasedOnPriority sorts pods based on priority and if their priorities are equal, they are sorted based on QoS tiers.
-func sortPodsBasedOnPriority(evictablePods []*v1.Pod) {
-	sort.Slice(evictablePods, func(i, j int) bool {
-		if evictablePods[i].Spec.Priority == nil && evictablePods[j].Spec.Priority != nil {
-			return true
-		}
-		if evictablePods[j].Spec.Priority == nil && evictablePods[i].Spec.Priority != nil {
-			return false
-		}
-		if (evictablePods[j].Spec.Priority == nil && evictablePods[i].Spec.Priority == nil) || (*evictablePods[i].Spec.Priority == *evictablePods[j].Spec.Priority) {
-			if podutil.IsBestEffortPod(evictablePods[i]) {
-				return true
-			}
-			if podutil.IsBurstablePod(evictablePods[i]) && podutil.IsGuaranteedPod(evictablePods[j]) {
-				return true
-			}
-			return false
-		}
-		return *evictablePods[i].Spec.Priority < *evictablePods[j].Spec.Priority
-	})
-}
-
 // createNodePodsMap returns nodepodsmap with evictable pods on node.
 func createNodePodsMap(ctx context.Context, client clientset.Interface, nodes []*v1.Node) NodePodsMap {
 	npm := NodePodsMap{}

pkg/descheduler/strategies/lownodeutilization_test.go

@@ -22,8 +22,6 @@ import (
 	"strings"
 	"testing"
 
-	"reflect"
-
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/api/policy/v1beta1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -471,46 +469,6 @@ func TestLowNodeUtilization(t *testing.T) {
 	}
 }
 
-func TestSortPodsByPriority(t *testing.T) {
-	n1 := test.BuildTestNode("n1", 4000, 3000, 9, nil)
-
-	p1 := test.BuildTestPod("p1", 400, 0, n1.Name, func(pod *v1.Pod) {
-		test.SetPodPriority(pod, lowPriority)
-	})
-
-	// BestEffort
-	p2 := test.BuildTestPod("p2", 400, 0, n1.Name, func(pod *v1.Pod) {
-		test.SetPodPriority(pod, highPriority)
-		test.MakeBestEffortPod(pod)
-	})
-
-	// Burstable
-	p3 := test.BuildTestPod("p3", 400, 0, n1.Name, func(pod *v1.Pod) {
-		test.SetPodPriority(pod, highPriority)
-		test.MakeBurstablePod(pod)
-	})
-
-	// Guaranteed
-	p4 := test.BuildTestPod("p4", 400, 100, n1.Name, func(pod *v1.Pod) {
-		test.SetPodPriority(pod, highPriority)
-		test.MakeGuaranteedPod(pod)
-	})
-
-	// Best effort with nil priorities.
-	p5 := test.BuildTestPod("p5", 400, 100, n1.Name, test.MakeBestEffortPod)
-	p5.Spec.Priority = nil
-
-	p6 := test.BuildTestPod("p6", 400, 100, n1.Name, test.MakeGuaranteedPod)
-	p6.Spec.Priority = nil
-
-	podList := []*v1.Pod{p4, p3, p2, p1, p6, p5}
-
-	sortPodsBasedOnPriority(podList)
-	if !reflect.DeepEqual(podList[len(podList)-1], p4) {
-		t.Errorf("Expected last pod in sorted list to be %v which of highest priority and guaranteed but got %v", p4, podList[len(podList)-1])
-	}
-}
-
 func TestValidateStrategyConfig(t *testing.T) {
 	tests := []struct {
 		name             string