Merge pull request #869 from knelasevero/existing-contraints-backport-1.23

Existing contraints fix backport 1.23

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 {

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)
+			}
+		})
+	}
+}