From 4548723dea2f9d104cde9fc55c9730537ae801c6 Mon Sep 17 00:00:00 2001 From: googs1025 Date: Thu, 20 Mar 2025 19:13:46 +0800 Subject: [PATCH 1/3] fix(removepodsviolatingtopologyspreadconstraint): fix removepodsviolatingtopologyspreadconstraint plugin sort logic Signed-off-by: googs1025 --- .../topologyspreadconstraint.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go index c66b71556..1adf3d731 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go @@ -423,10 +423,10 @@ func sortDomains(constraintTopologyPairs map[topologyPair][]*v1.Pod, isEvictable // if true and both and non-evictable, order doesn't matter return !(isEvictable(list[i]) && !isEvictable(list[j])) } - // if both pods have selectors/affinity, compare them by their priority if hasSelectorOrAffinity(*list[i]) == hasSelectorOrAffinity(*list[j]) { - comparePodsByPriority(list[i], list[j]) + // Sort by priority in ascending order (lower priority Pods first) + return !comparePodsByPriority(list[i], list[j]) } return hasSelectorOrAffinity(*list[i]) && !hasSelectorOrAffinity(*list[j]) }) From 59d1d5d1b97f349e0fd873f324678cb42aadc19c Mon Sep 17 00:00:00 2001 From: googs1025 Date: Wed, 12 Feb 2025 15:45:19 +0800 Subject: [PATCH 2/3] making isEvictable and hasSelectorOrAffinity invoked only once Signed-off-by: googs1025 --- .../topologyspreadconstraint.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go index 1adf3d731..cc460106c 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go @@ -417,18 +417,23 @@ func sortDomains(constraintTopologyPairs map[topologyPair][]*v1.Pod, isEvictable // followed by the highest priority pods with affinity or nodeSelector sort.Slice(list, func(i, j int) bool { // any non-evictable pods should be considered last (ie, first in the list) - if !isEvictable(list[i]) || !isEvictable(list[j]) { + evictableI := isEvictable(list[i]) + evictableJ := isEvictable(list[j]) + + if !evictableI || !evictableJ { // false - i is the only non-evictable, so return true to put it first // true - j is non-evictable, so return false to put j before i // if true and both and non-evictable, order doesn't matter - return !(isEvictable(list[i]) && !isEvictable(list[j])) + return !(evictableI && !evictableJ) } + hasSelectorOrAffinityI := hasSelectorOrAffinity(*list[i]) + hasSelectorOrAffinityJ := hasSelectorOrAffinity(*list[j]) // if both pods have selectors/affinity, compare them by their priority - if hasSelectorOrAffinity(*list[i]) == hasSelectorOrAffinity(*list[j]) { + if hasSelectorOrAffinityI == hasSelectorOrAffinityJ { // Sort by priority in ascending order (lower priority Pods first) return !comparePodsByPriority(list[i], list[j]) } - return hasSelectorOrAffinity(*list[i]) && !hasSelectorOrAffinity(*list[j]) + return hasSelectorOrAffinityI && !hasSelectorOrAffinityJ }) sortedTopologies = append(sortedTopologies, topology{pair: pair, pods: list}) } From b4b203cc606f53f3dd5be66b65800f692399f07c Mon Sep 17 00:00:00 2001 From: googs1025 Date: Fri, 28 Mar 2025 20:58:54 +0800 Subject: [PATCH 3/3] chore: add unit test for sortDomains func --- .../topologyspreadconstraint_test.go | 226 ++++++++++++++++++ 1 file changed, 226 insertions(+) diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go index 8bd4e88b8..0355f5df3 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go @@ -3,6 +3,7 @@ package removepodsviolatingtopologyspreadconstraint import ( "context" "fmt" + "reflect" "sort" "testing" @@ -1476,6 +1477,231 @@ func TestTopologySpreadConstraint(t *testing.T) { } } +func TestSortDomains(t *testing.T) { + tests := []struct { + name string + constraintTopology map[topologyPair][]*v1.Pod + want []topology + }{ + { + name: "empty input", + constraintTopology: map[topologyPair][]*v1.Pod{}, + want: []topology{}, + }, + { + name: "single domain with mixed pods", + constraintTopology: map[topologyPair][]*v1.Pod{ + {"zone", "a"}: { + { + ObjectMeta: metav1.ObjectMeta{ + Name: "non-evictable-pod", + Annotations: map[string]string{"evictable": "false"}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "evictable-with-affinity", + Annotations: map[string]string{"evictable": "true", "hasSelectorOrAffinity": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](10), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "evictable-high-priority", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](15), + }, + }, + }, + }, + want: []topology{ + {pair: topologyPair{"zone", "a"}, pods: []*v1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "non-evictable-pod", + Annotations: map[string]string{"evictable": "false"}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "evictable-with-affinity", + Annotations: map[string]string{"evictable": "true", "hasSelectorOrAffinity": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](10), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "evictable-high-priority", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](15), + }, + }, + }}, + }, + }, + { + name: "multiple domains with different priorities and selectors", + constraintTopology: map[topologyPair][]*v1.Pod{ + {"zone", "a"}: { + { + ObjectMeta: metav1.ObjectMeta{ + Name: "high-priority-affinity", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](20), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "low-priority-no-affinity", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](5), + }, + }, + }, + {"zone", "b"}: { + { + ObjectMeta: metav1.ObjectMeta{ + Name: "medium-priority-affinity", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](15), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "non-evictable-pod", + Annotations: map[string]string{"evictable": "false"}, + }, + }, + }, + }, + want: []topology{ + {pair: topologyPair{"zone", "a"}, pods: []*v1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "low-priority-no-affinity", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](5), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "high-priority-affinity", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](20), + }, + }, + }}, + {pair: topologyPair{"zone", "b"}, pods: []*v1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "non-evictable-pod", + Annotations: map[string]string{"evictable": "false"}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "medium-priority-affinity", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](15), + }, + }, + }}, + }, + }, + { + name: "domain with pods having different selector/affinity", + constraintTopology: map[topologyPair][]*v1.Pod{ + {"zone", "a"}: { + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pod-with-affinity", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Affinity: &v1.Affinity{ + NodeAffinity: &v1.NodeAffinity{}, + }, + Priority: utilptr.To[int32](10), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pod-no-affinity", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](15), + }, + }, + }, + }, + want: []topology{ + {pair: topologyPair{"zone", "a"}, pods: []*v1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pod-with-affinity", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Affinity: &v1.Affinity{ + NodeAffinity: &v1.NodeAffinity{}, + }, + Priority: utilptr.To[int32](10), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pod-no-affinity", + Annotations: map[string]string{"evictable": "true"}, + }, + Spec: v1.PodSpec{ + Priority: utilptr.To[int32](15), + }, + }, + }}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockIsEvictable := func(pod *v1.Pod) bool { + if val, exists := pod.Annotations["evictable"]; exists { + return val == "true" + } + return false + } + got := sortDomains(tt.constraintTopology, mockIsEvictable) + sort.Slice(got, func(i, j int) bool { + return got[i].pair.value < got[j].pair.value + }) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("sortDomains() = %v, want %v", got, tt.want) + } + }) + } +} + type testPodList struct { count int node string