From fd5b298c5085dec3b4892bb767a9cc7e9090f2f2 Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Wed, 26 Feb 2020 16:17:34 -0500 Subject: [PATCH] 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()).