From 023a2f2a4719d17edc5850cf1bc7c89a2b814e6d Mon Sep 17 00:00:00 2001
From: Sebastiaan Tammer <sebastiaantammer@gmail.com>
Date: Sat, 25 May 2019 11:18:11 +0200
Subject: [PATCH] Replaced UID with Namespace for duplicate check, added tests
 (+ cleanup)

---
 pkg/descheduler/strategies/duplicates.go      |  4 +-
 pkg/descheduler/strategies/duplicates_test.go | 51 ++++++++++++++-----
 test/test_utils.go                            |  2 +-
 3 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/pkg/descheduler/strategies/duplicates.go b/pkg/descheduler/strategies/duplicates.go
index 821571827..b0333e206 100644
--- a/pkg/descheduler/strategies/duplicates.go
+++ b/pkg/descheduler/strategies/duplicates.go
@@ -88,8 +88,8 @@ func FindDuplicatePods(pods []*v1.Pod) DuplicatePodsMap {
 	for _, pod := range pods {
 		ownerRefList := podutil.OwnerRef(pod)
 		for _, ownerRef := range ownerRefList {
-			// Kind and Namespace are not unique enough, which is why we use UID as well.
-			s := strings.Join([]string{ownerRef.Kind, ownerRef.Name, string(ownerRef.UID)}, "/")
+			// Namespace/Kind/Name should be unique for the cluster.
+			s := strings.Join([]string{pod.ObjectMeta.Namespace, ownerRef.Kind, ownerRef.Name}, "/")
 			dpm[s] = append(dpm[s], pod)
 		}
 	}
diff --git a/pkg/descheduler/strategies/duplicates_test.go b/pkg/descheduler/strategies/duplicates_test.go
index a35838dc9..b53d1da5b 100644
--- a/pkg/descheduler/strategies/duplicates_test.go
+++ b/pkg/descheduler/strategies/duplicates_test.go
@@ -31,26 +31,43 @@ import (
 func TestFindDuplicatePods(t *testing.T) {
 	node := test.BuildTestNode("n1", 2000, 3000, 10)
 	p1 := test.BuildTestPod("p1", 100, 0, node.Name)
+	p1.Namespace = "dev"
 	p2 := test.BuildTestPod("p2", 100, 0, node.Name)
+	p2.Namespace = "dev"
 	p3 := test.BuildTestPod("p3", 100, 0, node.Name)
+	p3.Namespace = "dev"
 	p4 := test.BuildTestPod("p4", 100, 0, node.Name)
 	p5 := test.BuildTestPod("p5", 100, 0, node.Name)
 	p6 := test.BuildTestPod("p6", 100, 0, node.Name)
 	p7 := test.BuildTestPod("p7", 100, 0, node.Name)
+	p7.Namespace = "kube-system"
 	p8 := test.BuildTestPod("p8", 100, 0, node.Name)
+	p8.Namespace = "test"
 	p9 := test.BuildTestPod("p9", 100, 0, node.Name)
+	p9.Namespace = "test"
+	p10 := test.BuildTestPod("p10", 100, 0, node.Name)
+	p10.Namespace = "test"
 
-	// All the following pods expect for one will be evicted.
-	p1.ObjectMeta.OwnerReferences = test.GetReplicaSetOwnerRefList()
-	p2.ObjectMeta.OwnerReferences = test.GetReplicaSetOwnerRefList()
-	p3.ObjectMeta.OwnerReferences = test.GetReplicaSetOwnerRefList()
-	p8.ObjectMeta.OwnerReferences = test.GetReplicaSetOwnerRefList()
-	p9.ObjectMeta.OwnerReferences = test.GetReplicaSetOwnerRefList()
+	// ### Evictable Pods ###
 
-	// The following 4 pods won't get evicted.
-	// A daemonset.
+	// Three Pods in the "default" Namespace, bound to same ReplicaSet. 2 should be evicted.
+	ownerRef1 := test.GetReplicaSetOwnerRefList()
+	p1.ObjectMeta.OwnerReferences = ownerRef1
+	p2.ObjectMeta.OwnerReferences = ownerRef1
+	p3.ObjectMeta.OwnerReferences = ownerRef1
+
+	// Three Pods in the "test" Namespace, bound to same ReplicaSet. 2 should be evicted.
+	ownerRef2 := test.GetReplicaSetOwnerRefList()
+	p8.ObjectMeta.OwnerReferences = ownerRef2
+	p9.ObjectMeta.OwnerReferences = ownerRef2
+	p10.ObjectMeta.OwnerReferences = ownerRef2
+
+	// ### Non-evictable Pods ###
+
+	// A DaemonSet.
 	p4.ObjectMeta.OwnerReferences = test.GetDaemonSetOwnerRefList()
-	// A pod with local storage.
+
+	// A Pod with local storage.
 	p5.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
 	p5.Spec.Volumes = []v1.Volume{
 		{
@@ -62,24 +79,30 @@ func TestFindDuplicatePods(t *testing.T) {
 			},
 		},
 	}
+
 	// A Mirror Pod.
 	p6.Annotations = test.GetMirrorPodAnnotation()
+
 	// A Critical Pod.
-	p7.Namespace = "kube-system"
 	p7.Annotations = test.GetCriticalPodAnnotation()
-	expectedEvictedPodCount := 2
+
+	// Setup the 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, *p5, *p6, *p7, *p8, *p9}}, nil
+		return true, &v1.PodList{Items: []v1.Pod{*p1, *p2, *p3, *p4, *p5, *p6, *p7, *p8, *p9, *p10}}, nil
 	})
 	fakeClient.Fake.AddReactor("get", "nodes", func(action core.Action) (bool, runtime.Object, error) {
 		return true, node, nil
 	})
+
+	expectedEvictedPodCount := 4
 	npe := nodePodEvictedCount{}
 	npe[node] = 0
-	podsEvicted := deleteDuplicatePods(fakeClient, "v1", []*v1.Node{node}, false, npe, 2, false)
+
+	// Start evictions.
+	podsEvicted := deleteDuplicatePods(fakeClient, "v1", []*v1.Node{node}, false, npe, 10, false)
 	if podsEvicted != expectedEvictedPodCount {
-		t.Errorf("Unexpected no of pods evicted")
+		t.Error("Unexpected number of pods evicted.\nExpected:\t", expectedEvictedPodCount, "\nActual:\t\t", podsEvicted)
 	}
 
 }
diff --git a/test/test_utils.go b/test/test_utils.go
index 136213aa3..926242a61 100644
--- a/test/test_utils.go
+++ b/test/test_utils.go
@@ -73,7 +73,7 @@ func GetNormalPodOwnerRefList() []metav1.OwnerReference {
 // GetReplicaSetOwnerRefList returns the ownerRef needed for replicaset pod.
 func GetReplicaSetOwnerRefList() []metav1.OwnerReference {
 	ownerRefList := make([]metav1.OwnerReference, 0)
-	ownerRefList = append(ownerRefList, metav1.OwnerReference{Kind: "ReplicaSet", APIVersion: "v1"})
+	ownerRefList = append(ownerRefList, metav1.OwnerReference{Kind: "ReplicaSet", APIVersion: "v1", Name: "replicateset-1"})
 	return ownerRefList
 }