diff --git a/pkg/descheduler/strategies/topologyspreadconstraint.go b/pkg/descheduler/strategies/topologyspreadconstraint.go index 035896bbc..2bdf67e9a 100644 --- a/pkg/descheduler/strategies/topologyspreadconstraint.go +++ b/pkg/descheduler/strategies/topologyspreadconstraint.go @@ -19,6 +19,7 @@ package strategies import ( "context" "math" + "reflect" "sort" v1 "k8s.io/api/core/v1" @@ -111,6 +112,12 @@ func RemovePodsViolatingTopologySpreadConstraint( if constraint.WhenUnsatisfiable == v1.ScheduleAnyway && (strategy.Params == nil || !strategy.Params.IncludeSoftConstraints) { continue } + // Need to check v1.TopologySpreadConstraint deepEquality because + // v1.TopologySpreadConstraint.LabelSelector is a pointer + // and assigning new constrains would always add more keys + if hasIdenticalConstraints(constraint, namespaceTopologySpreadConstraints) { + continue + } namespaceTopologySpreadConstraints[constraint] = struct{}{} } } @@ -183,6 +190,16 @@ func RemovePodsViolatingTopologySpreadConstraint( } } +// hasIdenticalConstraints checks if already had an identical TopologySpreadConstraint in namespaceTopologySpreadConstraints map +func hasIdenticalConstraints(newConstraint v1.TopologySpreadConstraint, namespaceTopologySpreadConstraints map[v1.TopologySpreadConstraint]struct{}) 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 f2b8ac8fa..a934261c0 100644 --- a/pkg/descheduler/strategies/topologyspreadconstraint_test.go +++ b/pkg/descheduler/strategies/topologyspreadconstraint_test.go @@ -1041,3 +1041,50 @@ func getDefaultTopologyConstraints(maxSkew int32) []v1.TopologySpreadConstraint }, } } + +func TestCheckIdenticalConstraints(t *testing.T) { + newConstrainSame := v1.TopologySpreadConstraint{ + MaxSkew: 2, + TopologyKey: "zone", + WhenUnsatisfiable: v1.DoNotSchedule, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + } + newConstrainDifferent := v1.TopologySpreadConstraint{ + MaxSkew: 3, + TopologyKey: "node", + WhenUnsatisfiable: v1.DoNotSchedule, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + } + namespaceTopologySpreadConstraint := make(map[v1.TopologySpreadConstraint]struct{}) + namespaceTopologySpreadConstraint[v1.TopologySpreadConstraint{ + MaxSkew: 2, + TopologyKey: "zone", + WhenUnsatisfiable: v1.DoNotSchedule, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + }] = struct{}{} + testCases := []struct { + name string + namespaceTopologySpreadConstraints map[v1.TopologySpreadConstraint]struct{} + newConstraint v1.TopologySpreadConstraint + expectedResult bool + }{ + { + name: "new constraint is identical", + namespaceTopologySpreadConstraints: namespaceTopologySpreadConstraint, + newConstraint: newConstrainSame, expectedResult: true}, + { + name: "new constraint is different", + namespaceTopologySpreadConstraints: namespaceTopologySpreadConstraint, + newConstraint: newConstrainDifferent, expectedResult: false, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + isIdentitcal := hasIdenticalConstraints(tc.newConstraint, tc.namespaceTopologySpreadConstraints) + if isIdentitcal != tc.expectedResult { + t.Errorf("Test error for description: %s. Expected result %v, got %v", tc.name, tc.expectedResult, isIdentitcal) + } + }) + } +}