From 5cc9e68127873ca692eacf40ce23d7688ed891c2 Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Wed, 23 Jul 2025 10:31:14 +0200 Subject: [PATCH 1/3] Drop assignment if default test values --- .../defaultevictor/defaultevictor_test.go | 131 ++++-------------- 1 file changed, 29 insertions(+), 102 deletions(-) diff --git a/pkg/framework/plugins/defaultevictor/defaultevictor_test.go b/pkg/framework/plugins/defaultevictor/defaultevictor_test.go index e176e39a3..781c50f0e 100644 --- a/pkg/framework/plugins/defaultevictor/defaultevictor_test.go +++ b/pkg/framework/plugins/defaultevictor/defaultevictor_test.go @@ -90,10 +90,7 @@ func TestDefaultEvictorPreEvictionFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: false, + nodeFit: true, }, { description: "Pod with correct tolerations running on normal node, all other nodes tainted", pods: []*v1.Pod{ @@ -128,10 +125,8 @@ func TestDefaultEvictorPreEvictionFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: true, + nodeFit: true, + result: true, }, { description: "Pod with incorrect node selector", pods: []*v1.Pod{ @@ -154,10 +149,7 @@ func TestDefaultEvictorPreEvictionFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: false, + nodeFit: true, }, { description: "Pod with correct node selector", pods: []*v1.Pod{ @@ -180,10 +172,8 @@ func TestDefaultEvictorPreEvictionFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: true, + nodeFit: true, + result: true, }, { description: "Pod with correct node selector, but only available node doesn't have enough CPU", pods: []*v1.Pod{ @@ -206,10 +196,7 @@ func TestDefaultEvictorPreEvictionFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: false, + nodeFit: true, }, { description: "Pod with correct node selector, and one node has enough memory", pods: []*v1.Pod{ @@ -242,10 +229,8 @@ func TestDefaultEvictorPreEvictionFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: true, + nodeFit: true, + result: true, }, { description: "Pod with correct node selector, but both nodes don't have enough memory", pods: []*v1.Pod{ @@ -278,10 +263,7 @@ func TestDefaultEvictorPreEvictionFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: false, + nodeFit: true, }, { description: "Pod with incorrect node selector, but nodefit false, should still be evicted", pods: []*v1.Pod{ @@ -304,10 +286,7 @@ func TestDefaultEvictorPreEvictionFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: false, - result: true, + result: true, }, } @@ -349,13 +328,10 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.Status.Phase = v1.PodFailed }), }, - evictFailedBarePods: false, - result: false, }, { description: "Normal pod eviction with no ownerRefs and evictFailedBarePods enabled", pods: []*v1.Pod{test.BuildTestPod("bare_pod", 400, 0, n1.Name, nil)}, evictFailedBarePods: true, - result: false, }, { description: "Failed pod eviction with no ownerRefs", pods: []*v1.Pod{ @@ -372,9 +348,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: true, + result: true, }, { description: "Normal pod eviction with normal ownerRefs and descheduler.alpha.kubernetes.io/evict annotation", pods: []*v1.Pod{ @@ -383,9 +357,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: true, + result: true, }, { description: "Normal pod eviction with replicaSet ownerRefs", pods: []*v1.Pod{ @@ -393,9 +365,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: true, + result: true, }, { description: "Normal pod eviction with replicaSet ownerRefs and descheduler.alpha.kubernetes.io/evict annotation", pods: []*v1.Pod{ @@ -404,9 +374,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.ObjectMeta.OwnerReferences = test.GetReplicaSetOwnerRefList() }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: true, + result: true, }, { description: "Normal pod eviction with statefulSet ownerRefs", pods: []*v1.Pod{ @@ -414,9 +382,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: true, + result: true, }, { description: "Normal pod eviction with statefulSet ownerRefs and descheduler.alpha.kubernetes.io/evict annotation", pods: []*v1.Pod{ @@ -425,9 +391,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.ObjectMeta.OwnerReferences = test.GetStatefulSetOwnerRefList() }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: true, + result: true, }, { description: "Pod not evicted because it is bound to a PV and evictLocalStoragePods = false", pods: []*v1.Pod{ @@ -446,9 +410,6 @@ func TestDefaultEvictorFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: false, }, { description: "Pod is evicted because it is bound to a PV and evictLocalStoragePods = true", pods: []*v1.Pod{ @@ -467,9 +428,8 @@ func TestDefaultEvictorFilter(t *testing.T) { } }), }, - evictLocalStoragePods: true, - evictSystemCriticalPods: false, - result: true, + evictLocalStoragePods: true, + result: true, }, { description: "Pod is evicted because it is bound to a PV and evictLocalStoragePods = false, but it has scheduler.alpha.kubernetes.io/evict annotation", pods: []*v1.Pod{ @@ -489,9 +449,7 @@ func TestDefaultEvictorFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: true, + result: true, }, { description: "Pod not evicted because it is part of a daemonSet", pods: []*v1.Pod{ @@ -500,9 +458,6 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.ObjectMeta.OwnerReferences = test.GetDaemonSetOwnerRefList() }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: false, }, { description: "Pod is evicted because it is part of a daemonSet, but it has scheduler.alpha.kubernetes.io/evict annotation", pods: []*v1.Pod{ @@ -511,9 +466,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.ObjectMeta.OwnerReferences = test.GetDaemonSetOwnerRefList() }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: true, + result: true, }, { description: "Pod not evicted because it is a mirror poddsa", pods: []*v1.Pod{ @@ -522,9 +475,6 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.Annotations = test.GetMirrorPodAnnotation() }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: false, }, { description: "Pod is evicted because it is a mirror pod, but it has scheduler.alpha.kubernetes.io/evict annotation", pods: []*v1.Pod{ @@ -534,9 +484,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.Annotations["descheduler.alpha.kubernetes.io/evict"] = "true" }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: true, + result: true, }, { description: "Pod not evicted because it has system critical priority", pods: []*v1.Pod{ @@ -546,9 +494,6 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.Spec.Priority = &priority }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: false, }, { description: "Pod is evicted because it has system critical priority, but it has scheduler.alpha.kubernetes.io/evict annotation", pods: []*v1.Pod{ @@ -561,9 +506,7 @@ func TestDefaultEvictorFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - result: true, + result: true, }, { description: "Pod not evicted because it has a priority higher than the configured priority threshold", pods: []*v1.Pod{ @@ -572,10 +515,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.Spec.Priority = &highPriority }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - priorityThreshold: &lowPriority, - result: false, + priorityThreshold: &lowPriority, }, { description: "Pod is evicted because it has a priority higher than the configured priority threshold, but it has scheduler.alpha.kubernetes.io/evict annotation", pods: []*v1.Pod{ @@ -585,10 +525,8 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.Spec.Priority = &highPriority }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - priorityThreshold: &lowPriority, - result: true, + priorityThreshold: &lowPriority, + result: true, }, { description: "Pod is evicted because it has system critical priority, but evictSystemCriticalPods = true", pods: []*v1.Pod{ @@ -598,7 +536,6 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.Spec.Priority = &priority }), }, - evictLocalStoragePods: false, evictSystemCriticalPods: true, result: true, }, { @@ -611,7 +548,6 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.Spec.Priority = &priority }), }, - evictLocalStoragePods: false, evictSystemCriticalPods: true, result: true, }, { @@ -622,7 +558,6 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.Spec.Priority = &highPriority }), }, - evictLocalStoragePods: false, evictSystemCriticalPods: true, priorityThreshold: &lowPriority, result: true, @@ -635,7 +570,6 @@ func TestDefaultEvictorFilter(t *testing.T) { pod.Spec.Priority = &highPriority }), }, - evictLocalStoragePods: false, evictSystemCriticalPods: true, priorityThreshold: &lowPriority, result: true, @@ -666,10 +600,8 @@ func TestDefaultEvictorFilter(t *testing.T) { } }), }, - evictLocalStoragePods: false, - evictSystemCriticalPods: false, - nodeFit: true, - result: true, + nodeFit: true, + result: true, }, { description: "minReplicas of 2, owner with 2 replicas, evicts", pods: []*v1.Pod{ @@ -697,7 +629,6 @@ func TestDefaultEvictorFilter(t *testing.T) { }), }, minReplicas: 3, - result: false, }, { description: "minReplicas of 2, multiple owners, no eviction", pods: []*v1.Pod{ @@ -721,7 +652,6 @@ func TestDefaultEvictorFilter(t *testing.T) { }), }, minPodAge: &minPodAge, - result: false, }, { description: "minPodAge of 50, pod created 60 minutes ago, evicts", pods: []*v1.Pod{ @@ -754,7 +684,6 @@ func TestDefaultEvictorFilter(t *testing.T) { }), }, ignorePodsWithoutPDB: true, - result: false, }, { description: "ignorePodsWithoutPDB, pod with PDBs, evicts", pods: []*v1.Pod{ @@ -785,7 +714,6 @@ func TestDefaultEvictorFilter(t *testing.T) { }), }, ignorePvcPods: true, - result: false, }, { description: "ignorePvcPods is not set, pod with PVC, evicts", pods: []*v1.Pod{ @@ -800,8 +728,7 @@ func TestDefaultEvictorFilter(t *testing.T) { } }), }, - ignorePvcPods: false, - result: true, + result: true, }, } From 6e9d8891c51164902a952d87629ab89500ea0e5d Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Thu, 24 Jul 2025 12:06:32 +0200 Subject: [PATCH 2/3] defaultevictor_test.go: replace descheduler.alpha.kubernetes.io/evict literal with evictPodAnnotationKey const --- .../defaultevictor/defaultevictor_test.go | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/framework/plugins/defaultevictor/defaultevictor_test.go b/pkg/framework/plugins/defaultevictor/defaultevictor_test.go index 781c50f0e..11fadf7fe 100644 --- a/pkg/framework/plugins/defaultevictor/defaultevictor_test.go +++ b/pkg/framework/plugins/defaultevictor/defaultevictor_test.go @@ -350,10 +350,10 @@ func TestDefaultEvictorFilter(t *testing.T) { }, result: true, }, { - description: "Normal pod eviction with normal ownerRefs and descheduler.alpha.kubernetes.io/evict annotation", + description: "Normal pod eviction with normal ownerRefs and " + evictPodAnnotationKey + " annotation", pods: []*v1.Pod{ test.BuildTestPod("p2", 400, 0, n1.Name, func(pod *v1.Pod) { - pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"} + pod.Annotations = map[string]string{evictPodAnnotationKey: "true"} pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() }), }, @@ -367,10 +367,10 @@ func TestDefaultEvictorFilter(t *testing.T) { }, result: true, }, { - description: "Normal pod eviction with replicaSet ownerRefs and descheduler.alpha.kubernetes.io/evict annotation", + description: "Normal pod eviction with replicaSet ownerRefs and " + evictPodAnnotationKey + " annotation", pods: []*v1.Pod{ test.BuildTestPod("p4", 400, 0, n1.Name, func(pod *v1.Pod) { - pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"} + pod.Annotations = map[string]string{evictPodAnnotationKey: "true"} pod.ObjectMeta.OwnerReferences = test.GetReplicaSetOwnerRefList() }), }, @@ -384,10 +384,10 @@ func TestDefaultEvictorFilter(t *testing.T) { }, result: true, }, { - description: "Normal pod eviction with statefulSet ownerRefs and descheduler.alpha.kubernetes.io/evict annotation", + description: "Normal pod eviction with statefulSet ownerRefs and " + evictPodAnnotationKey + " annotation", pods: []*v1.Pod{ test.BuildTestPod("p19", 400, 0, n1.Name, func(pod *v1.Pod) { - pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"} + pod.Annotations = map[string]string{evictPodAnnotationKey: "true"} pod.ObjectMeta.OwnerReferences = test.GetStatefulSetOwnerRefList() }), }, @@ -434,7 +434,7 @@ func TestDefaultEvictorFilter(t *testing.T) { description: "Pod is evicted because it is bound to a PV and evictLocalStoragePods = false, but it has scheduler.alpha.kubernetes.io/evict annotation", pods: []*v1.Pod{ test.BuildTestPod("p7", 400, 0, n1.Name, func(pod *v1.Pod) { - pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"} + pod.Annotations = map[string]string{evictPodAnnotationKey: "true"} pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() pod.Spec.Volumes = []v1.Volume{ { @@ -462,7 +462,7 @@ func TestDefaultEvictorFilter(t *testing.T) { description: "Pod is evicted because it is part of a daemonSet, but it has scheduler.alpha.kubernetes.io/evict annotation", pods: []*v1.Pod{ test.BuildTestPod("p9", 400, 0, n1.Name, func(pod *v1.Pod) { - pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"} + pod.Annotations = map[string]string{evictPodAnnotationKey: "true"} pod.ObjectMeta.OwnerReferences = test.GetDaemonSetOwnerRefList() }), }, @@ -481,7 +481,7 @@ func TestDefaultEvictorFilter(t *testing.T) { test.BuildTestPod("p11", 400, 0, n1.Name, func(pod *v1.Pod) { pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() pod.Annotations = test.GetMirrorPodAnnotation() - pod.Annotations["descheduler.alpha.kubernetes.io/evict"] = "true" + pod.Annotations[evictPodAnnotationKey] = "true" }), }, result: true, @@ -502,7 +502,7 @@ func TestDefaultEvictorFilter(t *testing.T) { priority := utils.SystemCriticalPriority pod.Spec.Priority = &priority pod.Annotations = map[string]string{ - "descheduler.alpha.kubernetes.io/evict": "true", + evictPodAnnotationKey: "true", } }), }, @@ -521,7 +521,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pods: []*v1.Pod{ test.BuildTestPod("p15", 400, 0, n1.Name, func(pod *v1.Pod) { pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() - pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"} + pod.Annotations = map[string]string{evictPodAnnotationKey: "true"} pod.Spec.Priority = &highPriority }), }, @@ -543,7 +543,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pods: []*v1.Pod{ test.BuildTestPod("p16", 400, 0, n1.Name, func(pod *v1.Pod) { pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() - pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"} + pod.Annotations = map[string]string{evictPodAnnotationKey: "true"} priority := utils.SystemCriticalPriority pod.Spec.Priority = &priority }), @@ -566,7 +566,7 @@ func TestDefaultEvictorFilter(t *testing.T) { pods: []*v1.Pod{ test.BuildTestPod("p17", 400, 0, n1.Name, func(pod *v1.Pod) { pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList() - pod.Annotations = map[string]string{"descheduler.alpha.kubernetes.io/evict": "true"} + pod.Annotations = map[string]string{evictPodAnnotationKey: "true"} pod.Spec.Priority = &highPriority }), }, From d0548b75d7fb3fc39a789065b8f556a72f7fd4a9 Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Thu, 24 Jul 2025 12:09:25 +0200 Subject: [PATCH 3/3] TestSortPodsBasedOnPriorityLowToHigh: check the whole sorted list of pods --- pkg/descheduler/pod/pods_test.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/descheduler/pod/pods_test.go b/pkg/descheduler/pod/pods_test.go index c9198ff2f..c890f8aaf 100644 --- a/pkg/descheduler/pod/pods_test.go +++ b/pkg/descheduler/pod/pods_test.go @@ -117,6 +117,14 @@ func TestListPodsOnANode(t *testing.T) { } } +func getPodListNames(pods []*v1.Pod) []string { + names := []string{} + for _, pod := range pods { + names = append(names, pod.Name) + } + return names +} + func TestSortPodsBasedOnPriorityLowToHigh(t *testing.T) { n1 := test.BuildTestNode("n1", 4000, 3000, 9, nil) @@ -150,10 +158,11 @@ func TestSortPodsBasedOnPriorityLowToHigh(t *testing.T) { p6.Spec.Priority = nil podList := []*v1.Pod{p4, p3, p2, p1, p6, p5} + expectedPodList := []*v1.Pod{p5, p6, p1, p2, p3, p4} 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]) + if !reflect.DeepEqual(getPodListNames(podList), getPodListNames(expectedPodList)) { + t.Errorf("Pods were sorted in an unexpected order: %v, expected %v", getPodListNames(podList), getPodListNames(expectedPodList)) } }