From e5d9756ebef44df461358ac2d3ded25903afaa62 Mon Sep 17 00:00:00 2001
From: Mike Dame <mdame@redhat.com>
Date: Wed, 9 Sep 2020 09:22:04 -0400
Subject: [PATCH] Move IsEvictable check in PodAntiAffinity

While non-evictable pods should never be evicted, they should still be
considered when calculating PodAntiAffinity violations. For example, you
may have an evictable pod that should not be running next to a system-critical
static pod. We currently filter IsEvictable before checking for Affinity violations,
so this case would not be caught.
---
 pkg/descheduler/strategies/pod_antiaffinity.go      |  3 +--
 pkg/descheduler/strategies/pod_antiaffinity_test.go | 12 ++++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/pkg/descheduler/strategies/pod_antiaffinity.go b/pkg/descheduler/strategies/pod_antiaffinity.go
index 473437bcd..f8e741d4b 100644
--- a/pkg/descheduler/strategies/pod_antiaffinity.go
+++ b/pkg/descheduler/strategies/pod_antiaffinity.go
@@ -74,7 +74,6 @@ func RemovePodsViolatingInterPodAntiAffinity(ctx context.Context, client clients
 			ctx,
 			client,
 			node,
-			podutil.WithFilter(evictable.IsEvictable),
 			podutil.WithNamespaces(includedNamespaces),
 			podutil.WithoutNamespaces(excludedNamespaces),
 		)
@@ -85,7 +84,7 @@ func RemovePodsViolatingInterPodAntiAffinity(ctx context.Context, client clients
 		podutil.SortPodsBasedOnPriorityLowToHigh(pods)
 		totalPods := len(pods)
 		for i := 0; i < totalPods; i++ {
-			if checkPodsWithAntiAffinityExist(pods[i], pods) {
+			if checkPodsWithAntiAffinityExist(pods[i], pods) && evictable.IsEvictable(pods[i]) {
 				success, err := podEvictor.EvictPod(ctx, pods[i], node, "InterPodAntiAffinity")
 				if err != nil {
 					klog.ErrorS(err, "Error evicting pod")
diff --git a/pkg/descheduler/strategies/pod_antiaffinity_test.go b/pkg/descheduler/strategies/pod_antiaffinity_test.go
index 6bbf84506..60c8a0537 100644
--- a/pkg/descheduler/strategies/pod_antiaffinity_test.go
+++ b/pkg/descheduler/strategies/pod_antiaffinity_test.go
@@ -27,6 +27,7 @@ import (
 	core "k8s.io/client-go/testing"
 	"sigs.k8s.io/descheduler/pkg/api"
 	"sigs.k8s.io/descheduler/pkg/descheduler/evictions"
+	"sigs.k8s.io/descheduler/pkg/utils"
 	"sigs.k8s.io/descheduler/test"
 )
 
@@ -40,10 +41,15 @@ func TestPodAntiAffinity(t *testing.T) {
 	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)
+	criticalPriority := utils.SystemCriticalPriority
+	nonEvictablePod := test.BuildTestPod("non-evict", 100, 0, node.Name, func(pod *v1.Pod) {
+		pod.Spec.Priority = &criticalPriority
+	})
 	p2.Labels = map[string]string{"foo": "bar"}
 	p5.Labels = map[string]string{"foo": "bar"}
 	p6.Labels = map[string]string{"foo": "bar"}
 	p7.Labels = map[string]string{"foo1": "bar1"}
+	nonEvictablePod.Labels = map[string]string{"foo": "bar"}
 	test.SetNormalOwnerRef(p1)
 	test.SetNormalOwnerRef(p2)
 	test.SetNormalOwnerRef(p3)
@@ -89,6 +95,12 @@ func TestPodAntiAffinity(t *testing.T) {
 			pods:                    []v1.Pod{*p5, *p6, *p7},
 			expectedEvictedPodCount: 1,
 		},
+		{
+			description:             "Evicts pod that conflicts with critical pod (but does not evict critical pod)",
+			maxPodsToEvictPerNode:   1,
+			pods:                    []v1.Pod{*p1, *nonEvictablePod},
+			expectedEvictedPodCount: 1,
+		},
 	}
 
 	for _, test := range tests {