From 30c972e49e94c2335bd748c5d5209961def90988 Mon Sep 17 00:00:00 2001 From: Lucas Severo Alves Date: Wed, 15 Jun 2022 11:30:37 +0200 Subject: [PATCH] change namespaceTopologySpreadConstraints from map to slice --- .../strategies/topologyspreadconstraint.go | 16 ++++----- .../topologyspreadconstraint_test.go | 36 ++++++++++--------- 2 files changed, 28 insertions(+), 24 deletions(-) diff --git a/pkg/descheduler/strategies/topologyspreadconstraint.go b/pkg/descheduler/strategies/topologyspreadconstraint.go index 2bdf67e9a..d586daa0f 100644 --- a/pkg/descheduler/strategies/topologyspreadconstraint.go +++ b/pkg/descheduler/strategies/topologyspreadconstraint.go @@ -105,7 +105,7 @@ 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 @@ -113,12 +113,12 @@ func RemovePodsViolatingTopologySpreadConstraint( continue } // Need to check v1.TopologySpreadConstraint deepEquality because - // v1.TopologySpreadConstraint.LabelSelector is a pointer - // and assigning new constrains would always add more keys + // v1.TopologySpreadConstraint has pointer fields + // and we don't need to go over duplicated constraints later on if hasIdenticalConstraints(constraint, namespaceTopologySpreadConstraints) { continue } - namespaceTopologySpreadConstraints[constraint] = struct{}{} + namespaceTopologySpreadConstraints = append(namespaceTopologySpreadConstraints, constraint) } } if len(namespaceTopologySpreadConstraints) == 0 { @@ -126,7 +126,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) @@ -190,9 +190,9 @@ 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 { +// 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 } diff --git a/pkg/descheduler/strategies/topologyspreadconstraint_test.go b/pkg/descheduler/strategies/topologyspreadconstraint_test.go index a934261c0..5a0b96e01 100644 --- a/pkg/descheduler/strategies/topologyspreadconstraint_test.go +++ b/pkg/descheduler/strategies/topologyspreadconstraint_test.go @@ -1043,47 +1043,51 @@ func getDefaultTopologyConstraints(maxSkew int32) []v1.TopologySpreadConstraint } func TestCheckIdenticalConstraints(t *testing.T) { - newConstrainSame := v1.TopologySpreadConstraint{ + newConstraintSame := v1.TopologySpreadConstraint{ MaxSkew: 2, TopologyKey: "zone", WhenUnsatisfiable: v1.DoNotSchedule, LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, } - newConstrainDifferent := v1.TopologySpreadConstraint{ + newConstraintDifferent := 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{}{} + 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 map[v1.TopologySpreadConstraint]struct{} + namespaceTopologySpreadConstraints []v1.TopologySpreadConstraint newConstraint v1.TopologySpreadConstraint expectedResult bool }{ { name: "new constraint is identical", namespaceTopologySpreadConstraints: namespaceTopologySpreadConstraint, - newConstraint: newConstrainSame, expectedResult: true}, + newConstraint: newConstraintSame, + expectedResult: true, + }, { name: "new constraint is different", namespaceTopologySpreadConstraints: namespaceTopologySpreadConstraint, - newConstraint: newConstrainDifferent, expectedResult: false, + newConstraint: newConstraintDifferent, + 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) + 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) } }) }