From 0e46803e778d3681474c3db9b173a538cb4b942e Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Wed, 26 Feb 2020 14:25:28 -0500 Subject: [PATCH 1/2] Test PodTopologySpread.PreFilter instead of internal pre-processing. Signed-off-by: Aldo Culquicondor --- .../plugins/podtopologyspread/common.go | 17 +- .../plugins/podtopologyspread/filtering.go | 144 +++---- .../podtopologyspread/filtering_test.go | 393 ++++++++++-------- 3 files changed, 299 insertions(+), 255 deletions(-) diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/common.go b/pkg/scheduler/framework/plugins/podtopologyspread/common.go index 88c05078aea..97bc9006472 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/common.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/common.go @@ -29,16 +29,17 @@ type topologyPair struct { // topologySpreadConstraint is an internal version for v1.TopologySpreadConstraint // and where the selector is parsed. +// Fields are exported for comparison during testing. type topologySpreadConstraint struct { - maxSkew int32 - topologyKey string - selector labels.Selector + MaxSkew int32 + TopologyKey string + Selector labels.Selector } -// nodeLabelsMatchSpreadConstraints checks if ALL topology keys in spread constraints are present in node labels. +// nodeLabelsMatchSpreadConstraints checks if ALL topology keys in spread Constraints are present in node labels. func nodeLabelsMatchSpreadConstraints(nodeLabels map[string]string, constraints []topologySpreadConstraint) bool { for _, c := range constraints { - if _, ok := nodeLabels[c.topologyKey]; !ok { + if _, ok := nodeLabels[c.TopologyKey]; !ok { return false } } @@ -54,9 +55,9 @@ func filterTopologySpreadConstraints(constraints []v1.TopologySpreadConstraint, return nil, err } result = append(result, topologySpreadConstraint{ - maxSkew: c.MaxSkew, - topologyKey: c.TopologyKey, - selector: selector, + MaxSkew: c.MaxSkew, + TopologyKey: c.TopologyKey, + Selector: selector, }) } } diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go b/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go index 87d4d9603f3..36505b00c91 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go @@ -29,26 +29,26 @@ import ( pluginhelper "k8s.io/kubernetes/pkg/scheduler/framework/plugins/helper" framework "k8s.io/kubernetes/pkg/scheduler/framework/v1alpha1" "k8s.io/kubernetes/pkg/scheduler/nodeinfo" - schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo" ) const preFilterStateKey = "PreFilter" + Name // preFilterState computed at PreFilter and used at Filter. -// It combines tpKeyToCriticalPaths and tpPairToMatchNum to represent: +// It combines TpKeyToCriticalPaths and TpPairToMatchNum to represent: // (1) critical paths where the least pods are matched on each spread constraint. // (2) number of pods matched on each spread constraint. // A nil preFilterState denotes it's not set at all (in PreFilter phase); // An empty preFilterState object denotes it's a legit state and is set in PreFilter phase. +// Fields are exported for comparison during testing. type preFilterState struct { - constraints []topologySpreadConstraint + Constraints []topologySpreadConstraint // We record 2 critical paths instead of all critical paths here. - // criticalPaths[0].matchNum always holds the minimum matching number. - // criticalPaths[1].matchNum is always greater or equal to criticalPaths[0].matchNum, but + // criticalPaths[0].MatchNum always holds the minimum matching number. + // criticalPaths[1].MatchNum is always greater or equal to criticalPaths[0].MatchNum, but // it's not guaranteed to be the 2nd minimum match number. - tpKeyToCriticalPaths map[string]*criticalPaths - // tpPairToMatchNum is keyed with topologyPair, and valued with the number of matching pods. - tpPairToMatchNum map[topologyPair]int32 + TpKeyToCriticalPaths map[string]*criticalPaths + // TpPairToMatchNum is keyed with topologyPair, and valued with the number of matching pods. + TpPairToMatchNum map[topologyPair]int32 } // Clone makes a copy of the given state. @@ -58,66 +58,65 @@ func (s *preFilterState) Clone() framework.StateData { return nil } copy := preFilterState{ - // constraints are shared because they don't change. - constraints: s.constraints, - tpKeyToCriticalPaths: make(map[string]*criticalPaths, len(s.tpKeyToCriticalPaths)), - tpPairToMatchNum: make(map[topologyPair]int32, len(s.tpPairToMatchNum)), + // Constraints are shared because they don't change. + Constraints: s.Constraints, + TpKeyToCriticalPaths: make(map[string]*criticalPaths, len(s.TpKeyToCriticalPaths)), + TpPairToMatchNum: make(map[topologyPair]int32, len(s.TpPairToMatchNum)), } - for tpKey, paths := range s.tpKeyToCriticalPaths { - copy.tpKeyToCriticalPaths[tpKey] = &criticalPaths{paths[0], paths[1]} + for tpKey, paths := range s.TpKeyToCriticalPaths { + copy.TpKeyToCriticalPaths[tpKey] = &criticalPaths{paths[0], paths[1]} } - for tpPair, matchNum := range s.tpPairToMatchNum { + for tpPair, matchNum := range s.TpPairToMatchNum { copyPair := topologyPair{key: tpPair.key, value: tpPair.value} - copy.tpPairToMatchNum[copyPair] = matchNum + copy.TpPairToMatchNum[copyPair] = matchNum } return © } -type criticalPath struct { - // topologyValue denotes the topology value mapping to topology key. - topologyValue string - // matchNum denotes the number of matching pods. - matchNum int32 -} - // CAVEAT: the reason that `[2]criticalPath` can work is based on the implementation of current // preemption algorithm, in particular the following 2 facts: // Fact 1: we only preempt pods on the same node, instead of pods on multiple nodes. // Fact 2: each node is evaluated on a separate copy of the preFilterState during its preemption cycle. // If we plan to turn to a more complex algorithm like "arbitrary pods on multiple nodes", this // structure needs to be revisited. -type criticalPaths [2]criticalPath +// Fields are exported for comparison during testing. +type criticalPaths [2]struct { + // TopologyValue denotes the topology value mapping to topology key. + TopologyValue string + // MatchNum denotes the number of matching pods. + MatchNum int32 +} func newCriticalPaths() *criticalPaths { - return &criticalPaths{{matchNum: math.MaxInt32}, {matchNum: math.MaxInt32}} + return &criticalPaths{{MatchNum: math.MaxInt32}, {MatchNum: math.MaxInt32}} } func (paths *criticalPaths) update(tpVal string, num int32) { // first verify if `tpVal` exists or not i := -1 - if tpVal == paths[0].topologyValue { + if tpVal == paths[0].TopologyValue { i = 0 - } else if tpVal == paths[1].topologyValue { + } else if tpVal == paths[1].TopologyValue { i = 1 } if i >= 0 { // `tpVal` exists - paths[i].matchNum = num - if paths[0].matchNum > paths[1].matchNum { + paths[i].MatchNum = num + if paths[0].MatchNum > paths[1].MatchNum { // swap paths[0] and paths[1] paths[0], paths[1] = paths[1], paths[0] } } else { // `tpVal` doesn't exist - if num < paths[0].matchNum { + if num < paths[0].MatchNum { // update paths[1] with paths[0] paths[1] = paths[0] // update paths[0] - paths[0].topologyValue, paths[0].matchNum = tpVal, num - } else if num < paths[1].matchNum { + paths[0].TopologyValue, paths[0].MatchNum = tpVal, num + } else if num < paths[1].MatchNum { // update paths[1] - paths[1].topologyValue, paths[1].matchNum = tpVal, num + paths[1].TopologyValue, paths[1].MatchNum = tpVal, num } } } @@ -126,40 +125,31 @@ func (s *preFilterState) updateWithPod(updatedPod, preemptorPod *v1.Pod, node *v if s == nil || updatedPod.Namespace != preemptorPod.Namespace || node == nil { return } - if !nodeLabelsMatchSpreadConstraints(node.Labels, s.constraints) { + if !nodeLabelsMatchSpreadConstraints(node.Labels, s.Constraints) { return } podLabelSet := labels.Set(updatedPod.Labels) - for _, constraint := range s.constraints { - if !constraint.selector.Matches(podLabelSet) { + for _, constraint := range s.Constraints { + if !constraint.Selector.Matches(podLabelSet) { continue } - k, v := constraint.topologyKey, node.Labels[constraint.topologyKey] + k, v := constraint.TopologyKey, node.Labels[constraint.TopologyKey] pair := topologyPair{key: k, value: v} - s.tpPairToMatchNum[pair] = s.tpPairToMatchNum[pair] + delta + s.TpPairToMatchNum[pair] = s.TpPairToMatchNum[pair] + delta - s.tpKeyToCriticalPaths[k].update(v, s.tpPairToMatchNum[pair]) + s.TpKeyToCriticalPaths[k].update(v, s.TpPairToMatchNum[pair]) } } // PreFilter invoked at the prefilter extension point. func (pl *PodTopologySpread) PreFilter(ctx context.Context, cycleState *framework.CycleState, pod *v1.Pod) *framework.Status { - var s *preFilterState - var allNodes []*nodeinfo.NodeInfo - var err error - - if allNodes, err = pl.sharedLister.NodeInfos().List(); err != nil { - return framework.NewStatus(framework.Error, fmt.Sprintf("failed to list NodeInfos: %v", err)) + s, err := pl.calPreFilterState(pod) + if err != nil { + return framework.NewStatus(framework.Error, err.Error()) } - - if s, err = calPreFilterState(pod, allNodes); err != nil { - return framework.NewStatus(framework.Error, fmt.Sprintf("Error calculating preFilterState of PodTopologySpread Plugin: %v", err)) - } - cycleState.Write(preFilterStateKey, s) - return nil } @@ -206,9 +196,13 @@ func getPreFilterState(cycleState *framework.CycleState) (*preFilterState, error } // calPreFilterState computes preFilterState describing how pods are spread on topologies. -func calPreFilterState(pod *v1.Pod, allNodes []*schedulernodeinfo.NodeInfo) (*preFilterState, error) { +func (pl *PodTopologySpread) calPreFilterState(pod *v1.Pod) (*preFilterState, error) { + allNodes, err := pl.sharedLister.NodeInfos().List() + if err != nil { + return nil, fmt.Errorf("listing NodeInfos: %v", err) + } // We have feature gating in APIServer to strip the spec - // so don't need to re-check feature gate, just check length of constraints. + // so don't need to re-check feature gate, just check length of Constraints. constraints, err := filterTopologySpreadConstraints(pod.Spec.TopologySpreadConstraints, v1.DoNotSchedule) if err != nil { return nil, err @@ -222,13 +216,13 @@ func calPreFilterState(pod *v1.Pod, allNodes []*schedulernodeinfo.NodeInfo) (*pr // TODO(Huang-Wei): It might be possible to use "make(map[topologyPair]*int32)". // In that case, need to consider how to init each tpPairToCount[pair] in an atomic fashion. s := preFilterState{ - constraints: constraints, - tpKeyToCriticalPaths: make(map[string]*criticalPaths, len(constraints)), - tpPairToMatchNum: make(map[topologyPair]int32), + Constraints: constraints, + TpKeyToCriticalPaths: make(map[string]*criticalPaths, len(constraints)), + TpPairToMatchNum: make(map[topologyPair]int32), } addTopologyPairMatchNum := func(pair topologyPair, num int32) { lock.Lock() - s.tpPairToMatchNum[pair] += num + s.TpPairToMatchNum[pair] += num lock.Unlock() } @@ -245,7 +239,7 @@ func calPreFilterState(pod *v1.Pod, allNodes []*schedulernodeinfo.NodeInfo) (*pr return } - // Ensure current node's labels contains all topologyKeys in 'constraints'. + // Ensure current node's labels contains all topologyKeys in 'Constraints'. if !nodeLabelsMatchSpreadConstraints(node.Labels, constraints) { return } @@ -257,11 +251,11 @@ func calPreFilterState(pod *v1.Pod, allNodes []*schedulernodeinfo.NodeInfo) (*pr if existingPod.DeletionTimestamp != nil || existingPod.Namespace != pod.Namespace { continue } - if constraint.selector.Matches(labels.Set(existingPod.Labels)) { + if constraint.Selector.Matches(labels.Set(existingPod.Labels)) { matchTotal++ } } - pair := topologyPair{key: constraint.topologyKey, value: node.Labels[constraint.topologyKey]} + pair := topologyPair{key: constraint.TopologyKey, value: node.Labels[constraint.TopologyKey]} addTopologyPairMatchNum(pair, matchTotal) } } @@ -269,11 +263,11 @@ func calPreFilterState(pod *v1.Pod, allNodes []*schedulernodeinfo.NodeInfo) (*pr // calculate min match for each topology pair for i := 0; i < len(constraints); i++ { - key := constraints[i].topologyKey - s.tpKeyToCriticalPaths[key] = newCriticalPaths() + key := constraints[i].TopologyKey + s.TpKeyToCriticalPaths[key] = newCriticalPaths() } - for pair, num := range s.tpPairToMatchNum { - s.tpKeyToCriticalPaths[pair.key].update(pair.value, num) + for pair, num := range s.TpPairToMatchNum { + s.TpKeyToCriticalPaths[pair.key].update(pair.value, num) } return &s, nil @@ -292,38 +286,38 @@ func (pl *PodTopologySpread) Filter(ctx context.Context, cycleState *framework.C } // However, "empty" preFilterState is legit which tolerates every toSchedule Pod. - if len(s.tpPairToMatchNum) == 0 || len(s.constraints) == 0 { + if len(s.TpPairToMatchNum) == 0 || len(s.Constraints) == 0 { return nil } podLabelSet := labels.Set(pod.Labels) - for _, c := range s.constraints { - tpKey := c.topologyKey - tpVal, ok := node.Labels[c.topologyKey] + for _, c := range s.Constraints { + tpKey := c.TopologyKey + tpVal, ok := node.Labels[c.TopologyKey] if !ok { klog.V(5).Infof("node '%s' doesn't have required label '%s'", node.Name, tpKey) return framework.NewStatus(framework.Unschedulable, ErrReasonConstraintsNotMatch) } selfMatchNum := int32(0) - if c.selector.Matches(podLabelSet) { + if c.Selector.Matches(podLabelSet) { selfMatchNum = 1 } pair := topologyPair{key: tpKey, value: tpVal} - paths, ok := s.tpKeyToCriticalPaths[tpKey] + paths, ok := s.TpKeyToCriticalPaths[tpKey] if !ok { // error which should not happen - klog.Errorf("internal error: get paths from key %q of %#v", tpKey, s.tpKeyToCriticalPaths) + klog.Errorf("internal error: get paths from key %q of %#v", tpKey, s.TpKeyToCriticalPaths) continue } // judging criteria: // 'existing matching num' + 'if self-match (1 or 0)' - 'global min matching num' <= 'maxSkew' - minMatchNum := paths[0].matchNum - matchNum := s.tpPairToMatchNum[pair] + minMatchNum := paths[0].MatchNum + matchNum := s.TpPairToMatchNum[pair] skew := matchNum + selfMatchNum - minMatchNum - if skew > c.maxSkew { - klog.V(5).Infof("node '%s' failed spreadConstraint[%s]: matchNum(%d) + selfMatchNum(%d) - minMatchNum(%d) > maxSkew(%d)", node.Name, tpKey, matchNum, selfMatchNum, minMatchNum, c.maxSkew) + if skew > c.MaxSkew { + klog.V(5).Infof("node '%s' failed spreadConstraint[%s]: MatchNum(%d) + selfMatchNum(%d) - minMatchNum(%d) > maxSkew(%d)", node.Name, tpKey, matchNum, selfMatchNum, minMatchNum, c.MaxSkew) return framework.NewStatus(framework.Unschedulable, ErrReasonConstraintsNotMatch) } } diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go b/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go index fe489459747..f4a59370ac6 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go @@ -21,6 +21,7 @@ import ( "reflect" "testing" + "github.com/google/go-cmp/cmp" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -35,7 +36,24 @@ var ( softSpread = v1.ScheduleAnyway ) -func TestCalPreFilterState(t *testing.T) { +func (s preFilterState) Equal(o preFilterState) bool { + type internal preFilterState + type internalCP criticalPaths + return cmp.Equal(internal(s), internal(o), + cmp.Comparer(func(s1 labels.Selector, s2 labels.Selector) bool { + return reflect.DeepEqual(s1, s2) + }), + cmp.Transformer("sort", func(p criticalPaths) internalCP { + if p[0].MatchNum == p[1].MatchNum && p[0].TopologyValue > p[1].TopologyValue { + // Swap TopologyValue to make them sorted alphabetically. + p[0].TopologyValue, p[1].TopologyValue = p[1].TopologyValue, p[0].TopologyValue + } + return internalCP(p) + }), + ) +} + +func TestPreFilterState(t *testing.T) { fooSelector := st.MakeLabelSelector().Exists("foo").Obj() barSelector := st.MakeLabelSelector().Exists("bar").Obj() tests := []struct { @@ -57,17 +75,17 @@ func TestCalPreFilterState(t *testing.T) { st.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ { - maxSkew: 5, - topologyKey: "zone", - selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Label("foo", "bar").Obj()), + MaxSkew: 5, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Label("foo", "bar").Obj()), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 0}, {"zone2", 0}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 0, {key: "zone", value: "zone2"}: 0, }, @@ -92,17 +110,17 @@ func TestCalPreFilterState(t *testing.T) { st.MakePod().Name("p-y2").Node("node-y").Label("foo", "").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ { - maxSkew: 1, - topologyKey: "zone", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone2", 2}, {"zone1", 3}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 3, {key: "zone", value: "zone2"}: 2, }, @@ -129,17 +147,17 @@ func TestCalPreFilterState(t *testing.T) { st.MakePod().Name("p-y2").Node("node-y").Label("foo", "").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ { - maxSkew: 1, - topologyKey: "zone", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone3", 0}, {"zone2", 2}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 3, {key: "zone", value: "zone2"}: 2, {key: "zone", value: "zone3"}: 0, @@ -165,17 +183,17 @@ func TestCalPreFilterState(t *testing.T) { st.MakePod().Name("p-y2").Node("node-y").Label("foo", "").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ { - maxSkew: 1, - topologyKey: "zone", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone2", 1}, {"zone1", 2}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 2, {key: "zone", value: "zone2"}: 1, }, @@ -203,23 +221,23 @@ func TestCalPreFilterState(t *testing.T) { st.MakePod().Name("p-y4").Node("node-y").Label("foo", "").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ { - maxSkew: 1, - topologyKey: "zone", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, { - maxSkew: 1, - topologyKey: "node", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 3}, {"zone2", 4}}, "node": {{"node-x", 0}, {"node-b", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 3, {key: "zone", value: "zone2"}: 4, {key: "node", value: "node-a"}: 2, @@ -252,23 +270,23 @@ func TestCalPreFilterState(t *testing.T) { st.MakePod().Name("p-y4").Node("node-y").Label("foo", "").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ { - maxSkew: 1, - topologyKey: "zone", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, { - maxSkew: 1, - topologyKey: "node", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 3}, {"zone2", 4}}, "node": {{"node-b", 1}, {"node-a", 2}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 3, {key: "zone", value: "zone2"}: 4, {key: "node", value: "node-a"}: 2, @@ -293,23 +311,23 @@ func TestCalPreFilterState(t *testing.T) { st.MakePod().Name("p-b").Node("node-b").Label("bar", "").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ { - maxSkew: 1, - topologyKey: "zone", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, { - maxSkew: 1, - topologyKey: "node", - selector: mustConvertLabelSelectorAsSelector(t, barSelector), + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, barSelector), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone2", 0}, {"zone1", 1}}, "node": {{"node-a", 0}, {"node-y", 0}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 1, {key: "zone", value: "zone2"}: 0, {key: "node", value: "node-a"}: 0, @@ -339,23 +357,23 @@ func TestCalPreFilterState(t *testing.T) { st.MakePod().Name("p-y4").Node("node-y").Label("foo", "").Label("bar", "").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ { - maxSkew: 1, - topologyKey: "zone", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, { - maxSkew: 1, - topologyKey: "node", - selector: mustConvertLabelSelectorAsSelector(t, barSelector), + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, barSelector), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 3}, {"zone2", 4}}, "node": {{"node-b", 0}, {"node-a", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 3, {key: "zone", value: "zone2"}: 4, {key: "node", value: "node-a"}: 1, @@ -387,23 +405,23 @@ func TestCalPreFilterState(t *testing.T) { st.MakePod().Name("p-y4").Node("node-y").Label("foo", "").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ { - maxSkew: 1, - topologyKey: "zone", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, { - maxSkew: 1, - topologyKey: "node", - selector: mustConvertLabelSelectorAsSelector(t, fooSelector), + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, fooSelector), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 3}, {"zone2", 4}}, "node": {{"node-b", 1}, {"node-a", 2}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 3, {key: "zone", value: "zone2"}: 4, {key: "node", value: "node-a"}: 2, @@ -415,12 +433,19 @@ func TestCalPreFilterState(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := cache.NewSnapshot(tt.existingPods, tt.nodes) - l, _ := s.NodeInfos().List() - got, _ := calPreFilterState(tt.pod, l) - got.sortCriticalPaths() - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("calPreFilterState() = %#v, want %#v", *got, *tt.want) + pl := PodTopologySpread{ + sharedLister: cache.NewSnapshot(tt.existingPods, tt.nodes), + } + cs := framework.NewCycleState() + if s := pl.PreFilter(context.Background(), cs, tt.pod); !s.IsSuccess() { + t.Fatal(s.AsError()) + } + got, err := getPreFilterState(cs) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(got, tt.want); diff != "" { + t.Errorf("PodTopologySpread#PreFilter() returned diff (-want,+got):\n%s", diff) } }) } @@ -428,12 +453,12 @@ func TestCalPreFilterState(t *testing.T) { func TestPreFilterStateAddPod(t *testing.T) { nodeConstraint := topologySpreadConstraint{ - maxSkew: 1, - topologyKey: "node", - selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("foo").Obj()), + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("foo").Obj()), } zoneConstraint := nodeConstraint - zoneConstraint.topologyKey = "zone" + zoneConstraint.TopologyKey = "zone" tests := []struct { name string preemptor *v1.Pod @@ -456,11 +481,11 @@ func TestPreFilterStateAddPod(t *testing.T) { st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{nodeConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{nodeConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "node": {{"node-b", 0}, {"node-a", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "node", value: "node-a"}: 1, {key: "node", value: "node-b"}: 0, }, @@ -481,11 +506,11 @@ func TestPreFilterStateAddPod(t *testing.T) { st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{nodeConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{nodeConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "node": {{"node-a", 1}, {"node-b", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "node", value: "node-a"}: 1, {key: "node", value: "node-b"}: 1, }, @@ -506,11 +531,11 @@ func TestPreFilterStateAddPod(t *testing.T) { st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{nodeConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{nodeConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "node": {{"node-a", 0}, {"node-b", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "node", value: "node-a"}: 0, {key: "node", value: "node-b"}: 1, }, @@ -531,11 +556,11 @@ func TestPreFilterStateAddPod(t *testing.T) { st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{nodeConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{nodeConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "node": {{"node-a", 0}, {"node-b", 2}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "node", value: "node-a"}: 0, {key: "node", value: "node-b"}: 2, }, @@ -555,12 +580,12 @@ func TestPreFilterStateAddPod(t *testing.T) { st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{zoneConstraint, nodeConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{zoneConstraint, nodeConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone2", 0}, {"zone1", 1}}, "node": {{"node-x", 0}, {"node-a", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 1, {key: "zone", value: "zone2"}: 0, {key: "node", value: "node-a"}: 1, @@ -584,12 +609,12 @@ func TestPreFilterStateAddPod(t *testing.T) { st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{zoneConstraint, nodeConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{zoneConstraint, nodeConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 1}, {"zone2", 1}}, "node": {{"node-a", 1}, {"node-x", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 1, {key: "zone", value: "zone2"}: 1, {key: "node", value: "node-a"}: 1, @@ -616,12 +641,12 @@ func TestPreFilterStateAddPod(t *testing.T) { st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{zoneConstraint, nodeConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{zoneConstraint, nodeConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone2", 1}, {"zone1", 3}}, "node": {{"node-a", 1}, {"node-x", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 3, {key: "zone", value: "zone2"}: 1, {key: "node", value: "node-a"}: 1, @@ -631,7 +656,7 @@ func TestPreFilterStateAddPod(t *testing.T) { }, }, { - name: "constraints hold different labelSelectors, node a impacts topologyKeyToMinPodsMap on zone", + name: "Constraints hold different labelSelectors, node a impacts topologyKeyToMinPodsMap on zone", preemptor: st.MakePod().Name("p").Label("foo", "").Label("bar", ""). SpreadConstraint(1, "zone", hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", hardSpread, st.MakeLabelSelector().Exists("bar").Obj()). @@ -649,19 +674,19 @@ func TestPreFilterStateAddPod(t *testing.T) { st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ zoneConstraint, { - maxSkew: 1, - topologyKey: "node", - selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("bar").Obj()), + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("bar").Obj()), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone2", 1}, {"zone1", 2}}, "node": {{"node-a", 0}, {"node-b", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 2, {key: "zone", value: "zone2"}: 1, {key: "node", value: "node-a"}: 0, @@ -671,7 +696,7 @@ func TestPreFilterStateAddPod(t *testing.T) { }, }, { - name: "constraints hold different labelSelectors, node a impacts topologyKeyToMinPodsMap on both zone and node", + name: "Constraints hold different labelSelectors, node a impacts topologyKeyToMinPodsMap on both zone and node", preemptor: st.MakePod().Name("p").Label("foo", "").Label("bar", ""). SpreadConstraint(1, "zone", hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", hardSpread, st.MakeLabelSelector().Exists("bar").Obj()). @@ -689,19 +714,19 @@ func TestPreFilterStateAddPod(t *testing.T) { st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(), }, want: &preFilterState{ - constraints: []topologySpreadConstraint{ + Constraints: []topologySpreadConstraint{ zoneConstraint, { - maxSkew: 1, - topologyKey: "node", - selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("bar").Obj()), + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("bar").Obj()), }, }, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 1}, {"zone2", 1}}, "node": {{"node-a", 1}, {"node-b", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 1, {key: "zone", value: "zone2"}: 1, {key: "node", value: "node-a"}: 1, @@ -713,13 +738,28 @@ func TestPreFilterStateAddPod(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := cache.NewSnapshot(tt.existingPods, tt.nodes) - l, _ := s.NodeInfos().List() - state, _ := calPreFilterState(tt.preemptor, l) - state.updateWithPod(tt.addedPod, tt.preemptor, tt.nodes[tt.nodeIdx], 1) - state.sortCriticalPaths() - if !reflect.DeepEqual(state, tt.want) { - t.Errorf("preFilterState#AddPod() = %v, want %v", state, tt.want) + snapshot := cache.NewSnapshot(tt.existingPods, tt.nodes) + pl := PodTopologySpread{ + sharedLister: snapshot, + } + cs := framework.NewCycleState() + ctx := context.Background() + if s := pl.PreFilter(ctx, cs, tt.preemptor); !s.IsSuccess() { + t.Fatal(s.AsError()) + } + nodeInfo, err := snapshot.Get(tt.nodes[tt.nodeIdx].Name) + if err != nil { + t.Fatal(err) + } + if s := pl.AddPod(ctx, cs, tt.preemptor, tt.addedPod, nodeInfo); !s.IsSuccess() { + t.Fatal(s.AsError()) + } + state, err := getPreFilterState(cs) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(state, tt.want); diff != "" { + t.Errorf("PodTopologySpread.AddPod() returned diff (-want,+got):\n%s", diff) } }) } @@ -727,12 +767,12 @@ func TestPreFilterStateAddPod(t *testing.T) { func TestPreFilterStateRemovePod(t *testing.T) { nodeConstraint := topologySpreadConstraint{ - maxSkew: 1, - topologyKey: "node", - selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("foo").Obj()), + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("foo").Obj()), } zoneConstraint := nodeConstraint - zoneConstraint.topologyKey = "zone" + zoneConstraint.TopologyKey = "zone" tests := []struct { name string preemptor *v1.Pod // preemptor pod @@ -763,11 +803,11 @@ func TestPreFilterStateRemovePod(t *testing.T) { deletedPodIdx: 0, // remove pod "p-a1" nodeIdx: 0, // node-a want: &preFilterState{ - constraints: []topologySpreadConstraint{zoneConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{zoneConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 1}, {"zone2", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 1, {key: "zone", value: "zone2"}: 1, }, @@ -793,11 +833,11 @@ func TestPreFilterStateRemovePod(t *testing.T) { deletedPodIdx: 0, // remove pod "p-a1" nodeIdx: 0, // node-a want: &preFilterState{ - constraints: []topologySpreadConstraint{zoneConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{zoneConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 1}, {"zone2", 2}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 1, {key: "zone", value: "zone2"}: 2, }, @@ -824,11 +864,11 @@ func TestPreFilterStateRemovePod(t *testing.T) { deletedPodIdx: 0, // remove pod "p-a0" nodeIdx: 0, // node-a want: &preFilterState{ - constraints: []topologySpreadConstraint{zoneConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{zoneConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 2}, {"zone2", 2}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 2, {key: "zone", value: "zone2"}: 2, }, @@ -855,11 +895,11 @@ func TestPreFilterStateRemovePod(t *testing.T) { deletedPod: st.MakePod().Name("p-a0").Node("node-a").Label("bar", "").Obj(), nodeIdx: 0, // node-a want: &preFilterState{ - constraints: []topologySpreadConstraint{zoneConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{zoneConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone1", 2}, {"zone2", 2}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 2, {key: "zone", value: "zone2"}: 2, }, @@ -886,12 +926,12 @@ func TestPreFilterStateRemovePod(t *testing.T) { deletedPodIdx: 3, // remove pod "p-x1" nodeIdx: 2, // node-x want: &preFilterState{ - constraints: []topologySpreadConstraint{zoneConstraint, nodeConstraint}, - tpKeyToCriticalPaths: map[string]*criticalPaths{ + Constraints: []topologySpreadConstraint{zoneConstraint, nodeConstraint}, + TpKeyToCriticalPaths: map[string]*criticalPaths{ "zone": {{"zone2", 1}, {"zone1", 3}}, "node": {{"node-b", 1}, {"node-x", 1}}, }, - tpPairToMatchNum: map[topologyPair]int32{ + TpPairToMatchNum: map[topologyPair]int32{ {key: "zone", value: "zone1"}: 3, {key: "zone", value: "zone2"}: 1, {key: "node", value: "node-a"}: 2, @@ -903,20 +943,36 @@ func TestPreFilterStateRemovePod(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := cache.NewSnapshot(tt.existingPods, tt.nodes) - l, _ := s.NodeInfos().List() - state, _ := calPreFilterState(tt.preemptor, l) + snapshot := cache.NewSnapshot(tt.existingPods, tt.nodes) + pl := PodTopologySpread{ + sharedLister: snapshot, + } + cs := framework.NewCycleState() + ctx := context.Background() + s := pl.PreFilter(ctx, cs, tt.preemptor) + if !s.IsSuccess() { + t.Fatal(s.AsError()) + } - var deletedPod *v1.Pod + deletedPod := tt.deletedPod if tt.deletedPodIdx < len(tt.existingPods) && tt.deletedPodIdx >= 0 { deletedPod = tt.existingPods[tt.deletedPodIdx] - } else { - deletedPod = tt.deletedPod } - state.updateWithPod(deletedPod, tt.preemptor, tt.nodes[tt.nodeIdx], -1) - state.sortCriticalPaths() - if !reflect.DeepEqual(state, tt.want) { - t.Errorf("preFilterState#RemovePod() = %v, want %v", state, tt.want) + + nodeInfo, err := snapshot.Get(tt.nodes[tt.nodeIdx].Name) + if err != nil { + t.Fatal(err) + } + if s := pl.RemovePod(ctx, cs, tt.preemptor, deletedPod, nodeInfo); !s.IsSuccess() { + t.Fatal(s.AsError()) + } + + state, err := getPreFilterState(cs) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(state, tt.want); diff != "" { + t.Errorf("PodTopologySpread.RemovePod() returned diff (-want,+got):\n%s", diff) } }) } @@ -949,7 +1005,7 @@ func BenchmarkTestCalPreFilterState(b *testing.B) { filteredNodesNum: 500, }, { - name: "1000nodes/two-constraints-zone-node", + name: "1000nodes/two-Constraints-zone-node", pod: st.MakePod().Name("p").Label("foo", "").Label("bar", ""). SpreadConstraint(1, v1.LabelZoneFailureDomain, hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, v1.LabelHostname, hardSpread, st.MakeLabelSelector().Exists("bar").Obj()). @@ -962,27 +1018,20 @@ func BenchmarkTestCalPreFilterState(b *testing.B) { for _, tt := range tests { b.Run(tt.name, func(b *testing.B) { existingPods, allNodes, _ := st.MakeNodesAndPodsForEvenPodsSpread(tt.pod.Labels, tt.existingPodsNum, tt.allNodesNum, tt.filteredNodesNum) - s := cache.NewSnapshot(existingPods, allNodes) - l, _ := s.NodeInfos().List() + pl := PodTopologySpread{ + sharedLister: cache.NewSnapshot(existingPods, allNodes), + } b.ResetTimer() for i := 0; i < b.N; i++ { - calPreFilterState(tt.pod, l) + s := pl.PreFilter(context.Background(), framework.NewCycleState(), tt.pod) + if !s.IsSuccess() { + b.Fatal(s.AsError()) + } } }) } } -// sortCriticalPaths is only served for testing purpose. -func (s *preFilterState) sortCriticalPaths() { - for _, paths := range s.tpKeyToCriticalPaths { - // If two paths both hold minimum matching number, and topologyValue is unordered. - if paths[0].matchNum == paths[1].matchNum && paths[0].topologyValue > paths[1].topologyValue { - // Swap topologyValue to make them sorted alphabetically. - paths[0].topologyValue, paths[1].topologyValue = paths[1].topologyValue, paths[0].topologyValue - } - } -} - func mustConvertLabelSelectorAsSelector(t *testing.T, ls *metav1.LabelSelector) labels.Selector { t.Helper() s, err := metav1.LabelSelectorAsSelector(ls) @@ -1278,7 +1327,7 @@ func TestMultipleConstraints(t *testing.T) { // 1. to fulfil "zone" constraint, incoming pod can be placed on any zone (hence any node) // 2. to fulfil "node" constraint, incoming pod can be placed on node-x // intersection of (1) and (2) returns node-x - name: "two constraints on zone and node, spreads = [3/3, 2/1/0/3]", + name: "two Constraints on zone and node, spreads = [3/3, 2/1/0/3]", pod: st.MakePod().Name("p").Label("foo", ""). SpreadConstraint(1, "zone", hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). @@ -1308,7 +1357,7 @@ func TestMultipleConstraints(t *testing.T) { // 1. to fulfil "zone" constraint, incoming pod can be placed on zone1 (node-a or node-b) // 2. to fulfil "node" constraint, incoming pod can be placed on node-x // intersection of (1) and (2) returns no node - name: "two constraints on zone and node, spreads = [3/4, 2/1/0/4]", + name: "two Constraints on zone and node, spreads = [3/4, 2/1/0/4]", pod: st.MakePod().Name("p").Label("foo", ""). SpreadConstraint(1, "zone", hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). @@ -1339,7 +1388,7 @@ func TestMultipleConstraints(t *testing.T) { // 1. to fulfil "zone" constraint, incoming pod can be placed on zone2 (node-x or node-y) // 2. to fulfil "node" constraint, incoming pod can be placed on node-b or node-x // intersection of (1) and (2) returns node-x - name: "constraints hold different labelSelectors, spreads = [1/0, 1/0/0/1]", + name: "Constraints hold different labelSelectors, spreads = [1/0, 1/0/0/1]", pod: st.MakePod().Name("p").Label("foo", "").Label("bar", ""). SpreadConstraint(1, "zone", hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", hardSpread, st.MakeLabelSelector().Exists("bar").Obj()). @@ -1365,7 +1414,7 @@ func TestMultipleConstraints(t *testing.T) { // 1. to fulfil "zone" constraint, incoming pod can be placed on zone2 (node-x or node-y) // 2. to fulfil "node" constraint, incoming pod can be placed on node-a or node-b // intersection of (1) and (2) returns no node - name: "constraints hold different labelSelectors, spreads = [1/0, 0/0/1/1]", + name: "Constraints hold different labelSelectors, spreads = [1/0, 0/0/1/1]", pod: st.MakePod().Name("p").Label("foo", "").Label("bar", ""). SpreadConstraint(1, "zone", hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", hardSpread, st.MakeLabelSelector().Exists("bar").Obj()). @@ -1392,7 +1441,7 @@ func TestMultipleConstraints(t *testing.T) { // 1. to fulfil "zone" constraint, incoming pod can be placed on zone1 (node-a or node-b) // 2. to fulfil "node" constraint, incoming pod can be placed on node-b or node-x // intersection of (1) and (2) returns node-b - name: "constraints hold different labelSelectors, spreads = [2/3, 1/0/0/1]", + name: "Constraints hold different labelSelectors, spreads = [2/3, 1/0/0/1]", pod: st.MakePod().Name("p").Label("foo", "").Label("bar", ""). SpreadConstraint(1, "zone", hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", hardSpread, st.MakeLabelSelector().Exists("bar").Obj()). @@ -1421,7 +1470,7 @@ func TestMultipleConstraints(t *testing.T) { // 1. pod doesn't match itself on "zone" constraint, so it can be put onto any zone // 2. to fulfil "node" constraint, incoming pod can be placed on node-a or node-b // intersection of (1) and (2) returns node-a and node-b - name: "constraints hold different labelSelectors but pod doesn't match itself on 'zone' constraint", + name: "Constraints hold different labelSelectors but pod doesn't match itself on 'zone' constraint", pod: st.MakePod().Name("p").Label("bar", ""). SpreadConstraint(1, "zone", hardSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", hardSpread, st.MakeLabelSelector().Exists("bar").Obj()). From fd5b298c5085dec3b4892bb767a9cc7e9090f2f2 Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Wed, 26 Feb 2020 16:17:34 -0500 Subject: [PATCH 2/2] Test PodTopologySpread.PreScore instead of internal pre-processing. Signed-off-by: Aldo Culquicondor --- .../framework/plugins/podtopologyspread/BUILD | 2 + .../plugins/podtopologyspread/scoring.go | 67 +++++------ .../plugins/podtopologyspread/scoring_test.go | 111 ++++++++++++------ 3 files changed, 112 insertions(+), 68 deletions(-) diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/BUILD b/pkg/scheduler/framework/plugins/podtopologyspread/BUILD index 3a3a789b950..e65cfbaddf5 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/BUILD +++ b/pkg/scheduler/framework/plugins/podtopologyspread/BUILD @@ -41,6 +41,8 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", + "//vendor/k8s.io/utils/pointer:go_default_library", ], ) diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/scoring.go b/pkg/scheduler/framework/plugins/podtopologyspread/scoring.go index 6b928093292..929d2b077ba 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/scoring.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/scoring.go @@ -34,12 +34,13 @@ import ( const preScoreStateKey = "PreScore" + Name // preScoreState computed at PreScore and used at Score. +// Fields are exported for comparison during testing. type preScoreState struct { - constraints []topologySpreadConstraint - // nodeNameSet is a string set holding all node names which have all constraints[*].topologyKey present. - nodeNameSet sets.String - // topologyPairToPodCounts is keyed with topologyPair, and valued with the number of matching pods. - topologyPairToPodCounts map[topologyPair]*int64 + Constraints []topologySpreadConstraint + // NodeNameSet is a string set holding all node names which have all Constraints[*].topologyKey present. + NodeNameSet sets.String + // TopologyPairToPodCounts is keyed with topologyPair, and valued with the number of matching pods. + TopologyPairToPodCounts map[topologyPair]*int64 } // Clone implements the mandatory Clone interface. We don't really copy the data since @@ -50,8 +51,8 @@ func (s *preScoreState) Clone() framework.StateData { // initialize iterates "filteredNodes" to filter out the nodes which don't have required topologyKey(s), // and initialize two maps: -// 1) s.topologyPairToPodCounts: keyed with both eligible topology pair and node names. -// 2) s.nodeNameSet: keyed with node name, and valued with a *int64 pointer for eligible node only. +// 1) s.TopologyPairToPodCounts: keyed with both eligible topology pair and node names. +// 2) s.NodeNameSet: keyed with node name, and valued with a *int64 pointer for eligible node only. func (s *preScoreState) initialize(pod *v1.Pod, filteredNodes []*v1.Node) error { constraints, err := filterTopologySpreadConstraints(pod.Spec.TopologySpreadConstraints, v1.ScheduleAnyway) if err != nil { @@ -60,20 +61,20 @@ func (s *preScoreState) initialize(pod *v1.Pod, filteredNodes []*v1.Node) error if constraints == nil { return nil } - s.constraints = constraints + s.Constraints = constraints for _, node := range filteredNodes { - if !nodeLabelsMatchSpreadConstraints(node.Labels, s.constraints) { + if !nodeLabelsMatchSpreadConstraints(node.Labels, s.Constraints) { continue } - for _, constraint := range s.constraints { - pair := topologyPair{key: constraint.topologyKey, value: node.Labels[constraint.topologyKey]} - if s.topologyPairToPodCounts[pair] == nil { - s.topologyPairToPodCounts[pair] = new(int64) + for _, constraint := range s.Constraints { + pair := topologyPair{key: constraint.TopologyKey, value: node.Labels[constraint.TopologyKey]} + if s.TopologyPairToPodCounts[pair] == nil { + s.TopologyPairToPodCounts[pair] = new(int64) } } - s.nodeNameSet.Insert(node.Name) + s.NodeNameSet.Insert(node.Name) // For those nodes which don't have all required topologyKeys present, it's intentional to leave - // their entries absent in nodeNameSet, so that we're able to score them to 0 afterwards. + // their entries absent in NodeNameSet, so that we're able to score them to 0 afterwards. } return nil } @@ -96,16 +97,16 @@ func (pl *PodTopologySpread) PreScore( } state := &preScoreState{ - nodeNameSet: sets.String{}, - topologyPairToPodCounts: make(map[topologyPair]*int64), + NodeNameSet: sets.String{}, + TopologyPairToPodCounts: make(map[topologyPair]*int64), } err = state.initialize(pod, filteredNodes) if err != nil { return framework.NewStatus(framework.Error, fmt.Sprintf("error when calculating preScoreState: %v", err)) } - // return if incoming pod doesn't have soft topology spread constraints. - if state.constraints == nil { + // return if incoming pod doesn't have soft topology spread Constraints. + if state.Constraints == nil { cycleState.Write(preScoreStateKey, state) return nil } @@ -119,15 +120,15 @@ func (pl *PodTopologySpread) PreScore( // (1) `node` should satisfy incoming pod's NodeSelector/NodeAffinity // (2) All topologyKeys need to be present in `node` if !pluginhelper.PodMatchesNodeSelectorAndAffinityTerms(pod, node) || - !nodeLabelsMatchSpreadConstraints(node.Labels, state.constraints) { + !nodeLabelsMatchSpreadConstraints(node.Labels, state.Constraints) { return } - for _, c := range state.constraints { - pair := topologyPair{key: c.topologyKey, value: node.Labels[c.topologyKey]} + for _, c := range state.Constraints { + pair := topologyPair{key: c.TopologyKey, value: node.Labels[c.TopologyKey]} // If current topology pair is not associated with any candidate node, // continue to avoid unnecessary calculation. - if state.topologyPairToPodCounts[pair] == nil { + if state.TopologyPairToPodCounts[pair] == nil { continue } @@ -138,11 +139,11 @@ func (pl *PodTopologySpread) PreScore( if existingPod.DeletionTimestamp != nil || existingPod.Namespace != pod.Namespace { continue } - if c.selector.Matches(labels.Set(existingPod.Labels)) { + if c.Selector.Matches(labels.Set(existingPod.Labels)) { matchSum++ } } - atomic.AddInt64(state.topologyPairToPodCounts[pair], matchSum) + atomic.AddInt64(state.TopologyPairToPodCounts[pair], matchSum) } } workqueue.ParallelizeUntil(ctx, 16, len(allNodes), processAllNode) @@ -167,17 +168,17 @@ func (pl *PodTopologySpread) Score(ctx context.Context, cycleState *framework.Cy } // Return if the node is not qualified. - if _, ok := s.nodeNameSet[node.Name]; !ok { + if _, ok := s.NodeNameSet[node.Name]; !ok { return 0, nil } // For each present , current node gets a credit of . // And we sum up and return it as this node's score. var score int64 - for _, c := range s.constraints { - if tpVal, ok := node.Labels[c.topologyKey]; ok { - pair := topologyPair{key: c.topologyKey, value: tpVal} - matchSum := *s.topologyPairToPodCounts[pair] + for _, c := range s.Constraints { + if tpVal, ok := node.Labels[c.TopologyKey]; ok { + pair := topologyPair{key: c.TopologyKey, value: tpVal} + matchSum := *s.TopologyPairToPodCounts[pair] score += matchSum } } @@ -198,8 +199,8 @@ func (pl *PodTopologySpread) NormalizeScore(ctx context.Context, cycleState *fra var minScore int64 = math.MaxInt64 var total int64 for _, score := range scores { - // it's mandatory to check if is present in m.nodeNameSet - if _, ok := s.nodeNameSet[score.Name]; !ok { + // it's mandatory to check if is present in m.NodeNameSet + if _, ok := s.NodeNameSet[score.Name]; !ok { continue } total += score.Score @@ -228,7 +229,7 @@ func (pl *PodTopologySpread) NormalizeScore(ctx context.Context, cycleState *fra continue } - if _, ok := s.nodeNameSet[node.Name]; !ok { + if _, ok := s.NodeNameSet[node.Name]; !ok { scores[i].Score = 0 continue } diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/scoring_test.go b/pkg/scheduler/framework/plugins/podtopologyspread/scoring_test.go index ba8c21fe941..4cca9cde0e0 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/scoring_test.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/scoring_test.go @@ -21,20 +21,31 @@ import ( "reflect" "testing" + "github.com/google/go-cmp/cmp" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/sets" framework "k8s.io/kubernetes/pkg/scheduler/framework/v1alpha1" "k8s.io/kubernetes/pkg/scheduler/internal/cache" st "k8s.io/kubernetes/pkg/scheduler/testing" + "k8s.io/utils/pointer" ) -func TestPreScoreStateInitialize(t *testing.T) { +func (s preScoreState) Equal(o preScoreState) bool { + type internal preScoreState + return cmp.Equal(internal(s), internal(o), + cmp.Comparer(func(s1 labels.Selector, s2 labels.Selector) bool { + return reflect.DeepEqual(s1, s2) + }), + ) +} + +func TestPreScoreStateEmptyNodes(t *testing.T) { tests := []struct { - name string - pod *v1.Pod - nodes []*v1.Node - wantNodeNameSet sets.String - wantTopologyPairMap map[topologyPair]*int64 + name string + pod *v1.Pod + nodes []*v1.Node + want *preScoreState }{ { name: "normal case", @@ -47,13 +58,27 @@ func TestPreScoreStateInitialize(t *testing.T) { st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(), st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(), }, - wantNodeNameSet: sets.NewString("node-a", "node-b", "node-x"), - wantTopologyPairMap: map[topologyPair]*int64{ - {key: "zone", value: "zone1"}: new(int64), - {key: "zone", value: "zone2"}: new(int64), - {key: "node", value: "node-a"}: new(int64), - {key: "node", value: "node-b"}: new(int64), - {key: "node", value: "node-x"}: new(int64), + want: &preScoreState{ + Constraints: []topologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("foo").Obj()), + }, + { + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("foo").Obj()), + }, + }, + NodeNameSet: sets.NewString("node-a", "node-b", "node-x"), + TopologyPairToPodCounts: map[topologyPair]*int64{ + {key: "zone", value: "zone1"}: pointer.Int64Ptr(0), + {key: "zone", value: "zone2"}: pointer.Int64Ptr(0), + {key: "node", value: "node-a"}: pointer.Int64Ptr(0), + {key: "node", value: "node-b"}: pointer.Int64Ptr(0), + {key: "node", value: "node-x"}: pointer.Int64Ptr(0), + }, }, }, { @@ -67,28 +92,44 @@ func TestPreScoreStateInitialize(t *testing.T) { st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(), st.MakeNode().Name("node-x").Label("node", "node-x").Obj(), }, - wantNodeNameSet: sets.NewString("node-a", "node-b"), - wantTopologyPairMap: map[topologyPair]*int64{ - {key: "zone", value: "zone1"}: new(int64), - {key: "node", value: "node-a"}: new(int64), - {key: "node", value: "node-b"}: new(int64), + want: &preScoreState{ + Constraints: []topologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("foo").Obj()), + }, + { + MaxSkew: 1, + TopologyKey: "node", + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("bar").Obj()), + }, + }, + NodeNameSet: sets.NewString("node-a", "node-b"), + TopologyPairToPodCounts: map[topologyPair]*int64{ + {key: "zone", value: "zone1"}: pointer.Int64Ptr(0), + {key: "node", value: "node-a"}: pointer.Int64Ptr(0), + {key: "node", value: "node-b"}: pointer.Int64Ptr(0), + }, }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := &preScoreState{ - nodeNameSet: sets.String{}, - topologyPairToPodCounts: make(map[topologyPair]*int64), + pl := PodTopologySpread{ + sharedLister: cache.NewSnapshot(nil, tt.nodes), } - if err := s.initialize(tt.pod, tt.nodes); err != nil { + cs := framework.NewCycleState() + if s := pl.PreScore(context.Background(), cs, tt.pod, tt.nodes); !s.IsSuccess() { + t.Fatal(s.AsError()) + } + + got, err := getPreScoreState(cs) + if err != nil { t.Fatal(err) } - if !reflect.DeepEqual(s.nodeNameSet, tt.wantNodeNameSet) { - t.Errorf("initilize().nodeNameSet = %#v, want %#v", s.nodeNameSet, tt.wantNodeNameSet) - } - if !reflect.DeepEqual(s.topologyPairToPodCounts, tt.wantTopologyPairMap) { - t.Errorf("initilize().topologyPairToPodCounts = %#v, want %#v", s.topologyPairToPodCounts, tt.wantTopologyPairMap) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("PodTopologySpread#PreScore() returned (-want, +got):\n%s", diff) } }) } @@ -300,7 +341,7 @@ func TestPodTopologySpreadScore(t *testing.T) { // matching pods spread as 2/~1~/2/~4~, total = 2+3 + 2+6 = 13 (zone and node should be both summed up) // after reversing, it's 8/5 // so scores = 800/8, 500/8 - name: "two constraints on zone and node, 2 out of 4 nodes are candidates", + name: "two Constraints on zone and node, 2 out of 4 nodes are candidates", pod: st.MakePod().Name("p").Label("foo", ""). SpreadConstraint(1, "zone", softSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", softSpread, st.MakeLabelSelector().Exists("foo").Obj()). @@ -330,7 +371,7 @@ func TestPodTopologySpreadScore(t *testing.T) { }, }, { - // If constraints hold different labelSelectors, it's a little complex. + // If Constraints hold different labelSelectors, it's a little complex. // +----------------------+------------------------+ // | zone1 | zone2 | // +----------------------+------------------------+ @@ -343,7 +384,7 @@ func TestPodTopologySpreadScore(t *testing.T) { // sum them up gets: 2/3/1/2, and total number is 8. // after reversing, it's 6/5/7/6 // so scores = 600/7, 500/7, 700/7, 600/7 - name: "two constraints on zone and node, with different labelSelectors", + name: "two Constraints on zone and node, with different labelSelectors", pod: st.MakePod().Name("p").Label("foo", "").Label("bar", ""). SpreadConstraint(1, "zone", softSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", softSpread, st.MakeLabelSelector().Exists("bar").Obj()). @@ -374,7 +415,7 @@ func TestPodTopologySpreadScore(t *testing.T) { // sum them up gets: 0/1/2/3, and total number is 6. // after reversing, it's 6/5/4/3. // so scores = 600/6, 500/6, 400/6, 300/6 - name: "two constraints on zone and node, with different labelSelectors, some nodes have 0 pods", + name: "two Constraints on zone and node, with different labelSelectors, some nodes have 0 pods", pod: st.MakePod().Name("p").Label("foo", "").Label("bar", ""). SpreadConstraint(1, "zone", softSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", softSpread, st.MakeLabelSelector().Exists("bar").Obj()). @@ -404,7 +445,7 @@ func TestPodTopologySpreadScore(t *testing.T) { // sum them up gets: 2/3/1, and total number is 6. // after reversing, it's 4/3/5 // so scores = 400/5, 300/5, 500/5 - name: "two constraints on zone and node, with different labelSelectors, 3 out of 4 nodes are candidates", + name: "two Constraints on zone and node, with different labelSelectors, 3 out of 4 nodes are candidates", pod: st.MakePod().Name("p").Label("foo", "").Label("bar", ""). SpreadConstraint(1, "zone", softSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, "node", softSpread, st.MakeLabelSelector().Exists("bar").Obj()). @@ -495,8 +536,8 @@ func TestPodTopologySpreadScore(t *testing.T) { if !status.IsSuccess() { t.Errorf("unexpected error: %v", status) } - if !reflect.DeepEqual(tt.want, gotList) { - t.Errorf("expected:\n\t%+v,\ngot:\n\t%+v", tt.want, gotList) + if diff := cmp.Diff(tt.want, gotList); diff != "" { + t.Errorf("unexpected scores (-want,+got):\n%s", diff) } }) } @@ -529,7 +570,7 @@ func BenchmarkTestPodTopologySpreadScore(b *testing.B) { filteredNodesNum: 500, }, { - name: "1000nodes/two-constraints-zone-node", + name: "1000nodes/two-Constraints-zone-node", pod: st.MakePod().Name("p").Label("foo", "").Label("bar", ""). SpreadConstraint(1, v1.LabelZoneFailureDomain, softSpread, st.MakeLabelSelector().Exists("foo").Obj()). SpreadConstraint(1, v1.LabelHostname, softSpread, st.MakeLabelSelector().Exists("bar").Obj()).