fix: topologyspreadconstraint to prefer evictable before sorting domains

Sort pods that are above ideal avg based on the criteria that they fit on other nodes that are below avg

Signed-off-by: Amir Alavi <amiralavi7@gmail.com>

pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go

@@ -356,9 +356,17 @@ func (d *RemovePodsViolatingTopologySpreadConstraint) balanceDomains(
 			continue
 		}
 
+		// prefer pods above the ideal average to be those that fit on nodes that are below ideal avg (need more pods)
+		sort.SliceStable(sortedDomains[j].pods, func(a, b int) bool {
+			canEvictA := node.PodFitsAnyOtherNode(getPodsAssignedToNode, sortedDomains[j].pods[a], nodesBelowIdealAvg)
+			canEvictB := node.PodFitsAnyOtherNode(getPodsAssignedToNode, sortedDomains[j].pods[b], nodesBelowIdealAvg)
+			return !canEvictA && canEvictB
+		})
+
 		// remove pods from the higher topology and add them to the list of pods to be evicted
 		// also (just for tracking), add them to the list of pods in the lower topology
 		aboveToEvict := sortedDomains[j].pods[len(sortedDomains[j].pods)-movePods:]
+
 		for k := range aboveToEvict {
 			// PodFitsAnyOtherNode excludes the current node because, for the sake of domain balancing only, we care about if there is any other
 			// place it could theoretically fit.
@@ -429,6 +437,7 @@ func sortDomains(constraintTopologyPairs map[topologyPair][]*v1.Pod, isEvictable
 				// if true and both and non-evictable, order doesn't matter
 				return !(evictableI && !evictableJ)
 			}
+
 			hasSelectorOrAffinityI := hasSelectorOrAffinity(*list[i])
 			hasSelectorOrAffinityJ := hasSelectorOrAffinity(*list[j])
 			// if both pods have selectors/affinity, compare them by their priority

pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go

@@ -1399,6 +1399,63 @@ func TestTopologySpreadConstraint(t *testing.T) {
 			args:                 RemovePodsViolatingTopologySpreadConstraintArgs{},
 			nodeFit:              true,
 		},
+		{
+			name: "3 domains, sizes [2, 1, 0] with selectors, maxSkew=1, nodeTaintsPolicy=Ignore, should move 1 for [1, 1, 1]",
+			nodes: []*v1.Node{
+				test.BuildTestNode("A1", 1000, 2000, 9, func(n *v1.Node) {
+					n.Labels["zone"] = "zoneA"
+					n.Labels[v1.LabelArchStable] = "arm64"
+				}),
+				test.BuildTestNode("B1", 1000, 2000, 9, func(n *v1.Node) {
+					n.Labels["zone"] = "zoneB"
+					n.Labels[v1.LabelArchStable] = "arm64"
+				}),
+				test.BuildTestNode("C1", 1000, 2000, 9, func(n *v1.Node) {
+					n.Labels["zone"] = "zoneC"
+					n.Labels[v1.LabelArchStable] = "arm64"
+				}),
+			},
+			pods: createTestPods([]testPodList{
+				{
+					count:  1,
+					node:   "A1",
+					labels: map[string]string{"foo": "bar"},
+					constraints: getDefaultTopologyConstraints(1, func(constraint *v1.TopologySpreadConstraint) {
+						constraint.NodeAffinityPolicy = utilptr.To(v1.NodeInclusionPolicyIgnore)
+					}),
+					nodeSelector: map[string]string{
+						v1.LabelArchStable: "arm64",
+					},
+				},
+				{
+					count:  1,
+					node:   "A1",
+					labels: map[string]string{"foo": "bar"},
+					constraints: getDefaultTopologyConstraints(1, func(constraint *v1.TopologySpreadConstraint) {
+						constraint.NodeAffinityPolicy = utilptr.To(v1.NodeInclusionPolicyIgnore)
+					}),
+					nodeSelector: map[string]string{
+						"zone":             "zoneA",
+						v1.LabelArchStable: "arm64",
+					},
+				},
+				{
+					count:  1,
+					node:   "B1",
+					labels: map[string]string{"foo": "bar"},
+					constraints: getDefaultTopologyConstraints(1, func(constraint *v1.TopologySpreadConstraint) {
+						constraint.NodeAffinityPolicy = utilptr.To(v1.NodeInclusionPolicyIgnore)
+					}),
+					nodeSelector: map[string]string{
+						v1.LabelArchStable: "arm64",
+					},
+				},
+			}),
+			expectedEvictedCount: 1,
+			expectedEvictedPods:  []string{"pod-0"},
+			namespaces:           []string{"ns1"},
+			args:                 RemovePodsViolatingTopologySpreadConstraintArgs{},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -1747,38 +1804,31 @@ func getLabelSelector(key string, values []string, operator metav1.LabelSelector
 	}
 }
 
-func getDefaultTopologyConstraints(maxSkew int32) []v1.TopologySpreadConstraint {
-	return []v1.TopologySpreadConstraint{
-		{
-			MaxSkew:           maxSkew,
-			TopologyKey:       "zone",
-			WhenUnsatisfiable: v1.DoNotSchedule,
-			LabelSelector:     &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
-		},
-	}
-}
-
 func getDefaultNodeTopologyConstraints(maxSkew int32) []v1.TopologySpreadConstraint {
-	return []v1.TopologySpreadConstraint{
-		{
-			MaxSkew:           maxSkew,
-			TopologyKey:       "node",
-			WhenUnsatisfiable: v1.DoNotSchedule,
-			LabelSelector:     &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
-		},
-	}
+	return getDefaultTopologyConstraints(maxSkew, func(constraint *v1.TopologySpreadConstraint) {
+		constraint.TopologyKey = "node"
+	})
 }
 
 func getDefaultTopologyConstraintsWithPodTemplateHashMatch(maxSkew int32) []v1.TopologySpreadConstraint {
-	return []v1.TopologySpreadConstraint{
-		{
+	return getDefaultTopologyConstraints(maxSkew, func(constraint *v1.TopologySpreadConstraint) {
+		constraint.MatchLabelKeys = []string{appsv1.DefaultDeploymentUniqueLabelKey}
+	})
+}
+
+func getDefaultTopologyConstraints(maxSkew int32, edits ...func(*v1.TopologySpreadConstraint)) []v1.TopologySpreadConstraint {
+	constraint := v1.TopologySpreadConstraint{
 		MaxSkew:           maxSkew,
 		TopologyKey:       "zone",
 		WhenUnsatisfiable: v1.DoNotSchedule,
 		LabelSelector:     &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
-			MatchLabelKeys:    []string{appsv1.DefaultDeploymentUniqueLabelKey},
-		},
 	}
+
+	for _, edit := range edits {
+		edit(&constraint)
+	}
+
+	return []v1.TopologySpreadConstraint{constraint}
 }
 
 func TestCheckIdenticalConstraints(t *testing.T) {