diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go index c66b71556..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]) { - comparePodsByPriority(list[i], 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}) } 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