From 65a03e76bfda0e4379fd9971153ff52a443978e9 Mon Sep 17 00:00:00 2001
From: lixiang <lixiang.cdc@gmail.com>
Date: Fri, 19 Jun 2020 11:48:23 +0800
Subject: [PATCH] Add sorting to RemovePodsViolatingInterPodAntiAffinity

---
 README.md                                     |  2 +-
 .../strategies/pod_antiaffinity.go            |  2 +
 .../strategies/pod_antiaffinity_test.go       | 45 ++++++++++++++-----
 3 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/README.md b/README.md
index 32bced6a3..e7d26ee92 100644
--- a/README.md
+++ b/README.md
@@ -238,7 +238,7 @@ 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.
-* In `LowNodeUtilization`, pods are evicted by their priority from low to high, and if they have same priority,
+* In `LowNodeUtilization` and `RemovePodsViolatingInterPodAntiAffinity`, 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.
diff --git a/pkg/descheduler/strategies/pod_antiaffinity.go b/pkg/descheduler/strategies/pod_antiaffinity.go
index 15c96b36a..30990929e 100644
--- a/pkg/descheduler/strategies/pod_antiaffinity.go
+++ b/pkg/descheduler/strategies/pod_antiaffinity.go
@@ -37,6 +37,8 @@ func RemovePodsViolatingInterPodAntiAffinity(ctx context.Context, client clients
 		if err != nil {
 			return
 		}
+		// sort the evictable Pods based on priority, if there are multiple pods with same priority, they are sorted based on QoS tiers.
+		podutil.SortPodsBasedOnPriorityLowToHigh(pods)
 		totalPods := len(pods)
 		for i := 0; i < totalPods; i++ {
 			if checkPodsWithAntiAffinityExist(pods[i], pods) {
diff --git a/pkg/descheduler/strategies/pod_antiaffinity_test.go b/pkg/descheduler/strategies/pod_antiaffinity_test.go
index 37fdf4617..d0d0a4990 100644
--- a/pkg/descheduler/strategies/pod_antiaffinity_test.go
+++ b/pkg/descheduler/strategies/pod_antiaffinity_test.go
@@ -37,16 +37,33 @@ func TestPodAntiAffinity(t *testing.T) {
 	p2 := test.BuildTestPod("p2", 100, 0, node.Name, nil)
 	p3 := test.BuildTestPod("p3", 100, 0, node.Name, nil)
 	p4 := test.BuildTestPod("p4", 100, 0, node.Name, nil)
+	p5 := test.BuildTestPod("p5", 100, 0, node.Name, nil)
+	p6 := test.BuildTestPod("p6", 100, 0, node.Name, nil)
+	p7 := test.BuildTestPod("p7", 100, 0, node.Name, nil)
 	p2.Labels = map[string]string{"foo": "bar"}
-	p1.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
-	p2.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
-	p3.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
-	p4.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
+	p5.Labels = map[string]string{"foo": "bar"}
+	p6.Labels = map[string]string{"foo": "bar"}
+	p7.Labels = map[string]string{"foo1": "bar1"}
+	test.SetNormalOwnerRef(p1)
+	test.SetNormalOwnerRef(p2)
+	test.SetNormalOwnerRef(p3)
+	test.SetNormalOwnerRef(p4)
+	test.SetNormalOwnerRef(p5)
+	test.SetNormalOwnerRef(p6)
+	test.SetNormalOwnerRef(p7)
 
 	// set pod anti affinity
-	setPodAntiAffinity(p1)
-	setPodAntiAffinity(p3)
-	setPodAntiAffinity(p4)
+	setPodAntiAffinity(p1, "foo", "bar")
+	setPodAntiAffinity(p3, "foo", "bar")
+	setPodAntiAffinity(p4, "foo", "bar")
+	setPodAntiAffinity(p5, "foo1", "bar1")
+	setPodAntiAffinity(p6, "foo1", "bar1")
+	setPodAntiAffinity(p7, "foo", "bar")
+
+	// set pod priority
+	test.SetPodPriority(p5, 100)
+	test.SetPodPriority(p6, 50)
+	test.SetPodPriority(p7, 0)
 
 	tests := []struct {
 		description             string
@@ -66,13 +83,19 @@ func TestPodAntiAffinity(t *testing.T) {
 			pods:                    []v1.Pod{*p1, *p2, *p3, *p4},
 			expectedEvictedPodCount: 3,
 		},
+		{
+			description:             "Evict only 1 pod after sorting",
+			maxPodsToEvict:          0,
+			pods:                    []v1.Pod{*p5, *p6, *p7},
+			expectedEvictedPodCount: 1,
+		},
 	}
 
 	for _, test := range tests {
 		// create fake client
 		fakeClient := &fake.Clientset{}
 		fakeClient.Fake.AddReactor("list", "pods", func(action core.Action) (bool, runtime.Object, error) {
-			return true, &v1.PodList{Items: []v1.Pod{*p1, *p2, *p3, *p4}}, nil
+			return true, &v1.PodList{Items: test.pods}, nil
 		})
 		fakeClient.Fake.AddReactor("get", "nodes", func(action core.Action) (bool, runtime.Object, error) {
 			return true, node, nil
@@ -95,7 +118,7 @@ func TestPodAntiAffinity(t *testing.T) {
 	}
 }
 
-func setPodAntiAffinity(inputPod *v1.Pod) {
+func setPodAntiAffinity(inputPod *v1.Pod, labelKey, labelValue string) {
 	inputPod.Spec.Affinity = &v1.Affinity{
 		PodAntiAffinity: &v1.PodAntiAffinity{
 			RequiredDuringSchedulingIgnoredDuringExecution: []v1.PodAffinityTerm{
@@ -103,9 +126,9 @@ func setPodAntiAffinity(inputPod *v1.Pod) {
 					LabelSelector: &metav1.LabelSelector{
 						MatchExpressions: []metav1.LabelSelectorRequirement{
 							{
-								Key:      "foo",
+								Key:      labelKey,
 								Operator: metav1.LabelSelectorOpIn,
-								Values:   []string{"bar"},
+								Values:   []string{labelValue},
 							},
 						},
 					},