diff --git a/pkg/descheduler/strategies/topologyspreadconstraint.go b/pkg/descheduler/strategies/topologyspreadconstraint.go index 2f0fda87a..01c174797 100644 --- a/pkg/descheduler/strategies/topologyspreadconstraint.go +++ b/pkg/descheduler/strategies/topologyspreadconstraint.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "math" + "reflect" "sort" v1 "k8s.io/api/core/v1" @@ -106,14 +107,20 @@ func RemovePodsViolatingTopologySpreadConstraint( } // ...where there is a topology constraint - namespaceTopologySpreadConstraints := make(map[v1.TopologySpreadConstraint]struct{}) + namespaceTopologySpreadConstraints := []v1.TopologySpreadConstraint{} for _, pod := range namespacePods.Items { for _, constraint := range pod.Spec.TopologySpreadConstraints { // Ignore soft topology constraints if they are not included if constraint.WhenUnsatisfiable == v1.ScheduleAnyway && (strategy.Params == nil || !strategy.Params.IncludeSoftConstraints) { continue } - namespaceTopologySpreadConstraints[constraint] = struct{}{} + // Need to check v1.TopologySpreadConstraint deepEquality because + // v1.TopologySpreadConstraint has pointer fields + // and we don't need to go over duplicated constraints later on + if hasIdenticalConstraints(constraint, namespaceTopologySpreadConstraints) { + continue + } + namespaceTopologySpreadConstraints = append(namespaceTopologySpreadConstraints, constraint) } } if len(namespaceTopologySpreadConstraints) == 0 { @@ -121,7 +128,7 @@ func RemovePodsViolatingTopologySpreadConstraint( } // 2. for each topologySpreadConstraint in that namespace - for constraint := range namespaceTopologySpreadConstraints { + for _, constraint := range namespaceTopologySpreadConstraints { constraintTopologies := make(map[topologyPair][]*v1.Pod) // pre-populate the topologyPair map with all the topologies available from the nodeMap // (we can't just build it from existing pods' nodes because a topology may have 0 pods) @@ -185,6 +192,16 @@ func RemovePodsViolatingTopologySpreadConstraint( } } +// hasIdenticalConstraints checks if we already had an identical TopologySpreadConstraint in namespaceTopologySpreadConstraints slice +func hasIdenticalConstraints(newConstraint v1.TopologySpreadConstraint, namespaceTopologySpreadConstraints []v1.TopologySpreadConstraint) bool { + for _, constraint := range namespaceTopologySpreadConstraints { + if reflect.DeepEqual(newConstraint, constraint) { + return true + } + } + return false +} + // topologyIsBalanced checks if any domains in the topology differ by more than the MaxSkew // this is called before any sorting or other calculations and is used to skip topologies that don't need to be balanced func topologyIsBalanced(topology map[topologyPair][]*v1.Pod, constraint v1.TopologySpreadConstraint) bool { diff --git a/pkg/descheduler/strategies/topologyspreadconstraint_test.go b/pkg/descheduler/strategies/topologyspreadconstraint_test.go index 22a863651..cd4fd4bac 100644 --- a/pkg/descheduler/strategies/topologyspreadconstraint_test.go +++ b/pkg/descheduler/strategies/topologyspreadconstraint_test.go @@ -971,3 +971,54 @@ func getDefaultTopologyConstraints(maxSkew int32) []v1.TopologySpreadConstraint }, } } + +func TestCheckIdenticalConstraints(t *testing.T) { + newConstraintSame := v1.TopologySpreadConstraint{ + MaxSkew: 2, + TopologyKey: "zone", + WhenUnsatisfiable: v1.DoNotSchedule, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + } + newConstraintDifferent := v1.TopologySpreadConstraint{ + MaxSkew: 3, + TopologyKey: "node", + WhenUnsatisfiable: v1.DoNotSchedule, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + } + namespaceTopologySpreadConstraint := []v1.TopologySpreadConstraint{} + namespaceTopologySpreadConstraint = []v1.TopologySpreadConstraint{ + { + MaxSkew: 2, + TopologyKey: "zone", + WhenUnsatisfiable: v1.DoNotSchedule, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + }, + } + testCases := []struct { + name string + namespaceTopologySpreadConstraints []v1.TopologySpreadConstraint + newConstraint v1.TopologySpreadConstraint + expectedResult bool + }{ + { + name: "new constraint is identical", + namespaceTopologySpreadConstraints: namespaceTopologySpreadConstraint, + newConstraint: newConstraintSame, + expectedResult: true, + }, + { + name: "new constraint is different", + namespaceTopologySpreadConstraints: namespaceTopologySpreadConstraint, + newConstraint: newConstraintDifferent, + expectedResult: false, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + isIdentical := hasIdenticalConstraints(tc.newConstraint, tc.namespaceTopologySpreadConstraints) + if isIdentical != tc.expectedResult { + t.Errorf("Test error for description: %s. Expected result %v, got %v", tc.name, tc.expectedResult, isIdentical) + } + }) + } +}