From fbf11df729cd6f456a2c94c1fea795ebb971fc0d Mon Sep 17 00:00:00 2001 From: Amir Alavi Date: Thu, 10 Jul 2025 17:26:19 -0400 Subject: [PATCH 1/2] 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 --- .../topologyspreadconstraint.go | 9 ++ .../topologyspreadconstraint_test.go | 104 +++++++++++++----- 2 files changed, 86 insertions(+), 27 deletions(-) diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go index e95a1b7f6..d62a15ebb 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go +++ b/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 diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go index bf5033458..c3b1763f6 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go +++ b/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{ - { - MaxSkew: maxSkew, - TopologyKey: "zone", - WhenUnsatisfiable: v1.DoNotSchedule, - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - MatchLabelKeys: []string{appsv1.DefaultDeploymentUniqueLabelKey}, - }, + 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"}}, } + + for _, edit := range edits { + edit(&constraint) + } + + return []v1.TopologySpreadConstraint{constraint} } func TestCheckIdenticalConstraints(t *testing.T) { From eadfe4a5461898623a2132e8686fc030b3fb2eb8 Mon Sep 17 00:00:00 2001 From: Amir Alavi Date: Thu, 10 Jul 2025 17:27:58 -0400 Subject: [PATCH 2/2] fix: topologyspreadconstraint plugin to not add PodNodeAffinity unless the inclusion policy is honor --- .../topologyspreadconstraint.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go index d62a15ebb..0544d5551 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go @@ -539,16 +539,16 @@ func newTopologySpreadConstraint(constraint v1.TopologySpreadConstraint, pod *v1 MaxSkew: constraint.MaxSkew, TopologyKey: constraint.TopologyKey, Selector: selector, - NodeAffinityPolicy: v1.NodeInclusionPolicyHonor, // If NodeAffinityPolicy is nil, we treat NodeAffinityPolicy as "Honor". - NodeTaintsPolicy: v1.NodeInclusionPolicyIgnore, // If NodeTaintsPolicy is nil, we treat NodeTaintsPolicy as "Ignore". - PodNodeAffinity: nodeaffinity.GetRequiredNodeAffinity(pod), - PodTolerations: pod.Spec.Tolerations, + NodeAffinityPolicy: utilptr.Deref(constraint.NodeAffinityPolicy, v1.NodeInclusionPolicyHonor), // If NodeAffinityPolicy is nil, we treat NodeAffinityPolicy as "Honor". + NodeTaintsPolicy: utilptr.Deref(constraint.NodeTaintsPolicy, v1.NodeInclusionPolicyIgnore), // If NodeTaintsPolicy is nil, we treat NodeTaintsPolicy as "Ignore". } - if constraint.NodeAffinityPolicy != nil { - tsc.NodeAffinityPolicy = *constraint.NodeAffinityPolicy + + if tsc.NodeAffinityPolicy == v1.NodeInclusionPolicyHonor { + tsc.PodNodeAffinity = nodeaffinity.GetRequiredNodeAffinity(pod) } - if constraint.NodeTaintsPolicy != nil { - tsc.NodeTaintsPolicy = *constraint.NodeTaintsPolicy + + if tsc.NodeTaintsPolicy == v1.NodeInclusionPolicyHonor { + tsc.PodTolerations = pod.Spec.Tolerations } return tsc, nil