Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Automated cherry pick of #105046: Skip check for all topology labels when using system default #106607

Merged
merged 1 commit into from
Nov 23, 2021
Merged

Automated cherry pick of #105046: Skip check for all topology labels when using system default #106607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/scheduler/framework/plugins/podtopologyspread/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ var systemDefaultConstraints = []v1.TopologySpreadConstraint{

// PodTopologySpread is a plugin that ensures pod's topologySpreadConstraints is satisfied.
type PodTopologySpread struct {
systemDefaulted bool
defaultConstraints []v1.TopologySpreadConstraint
sharedLister framework.SharedLister
services corelisters.ServiceLister
Expand Down Expand Up @@ -92,6 +93,7 @@ func New(plArgs runtime.Object, h framework.Handle) (framework.Plugin, error) {
}
if args.DefaultingType == config.SystemDefaulting {
pl.defaultConstraints = systemDefaultConstraints
pl.systemDefaulted = true
}
if len(pl.defaultConstraints) != 0 {
if h.SharedInformerFactory() == nil {
Expand Down
12 changes: 8 additions & 4 deletions pkg/scheduler/framework/plugins/podtopologyspread/scoring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (s *preScoreState) Clone() framework.StateData {
// 1) s.TopologyPairToPodCounts: keyed with both eligible topology pair and node names.
// 2) s.IgnoredNodes: the set of nodes that shouldn't be scored.
// 3) s.TopologyNormalizingWeight: The weight to be given to each constraint based on the number of values in a topology.
func (pl *PodTopologySpread) initPreScoreState(s *preScoreState, pod *v1.Pod, filteredNodes []*v1.Node) error {
func (pl *PodTopologySpread) initPreScoreState(s *preScoreState, pod *v1.Pod, filteredNodes []*v1.Node, requireAllTopologies bool) error {
var err error
if len(pod.Spec.TopologySpreadConstraints) > 0 {
s.Constraints, err = filterTopologySpreadConstraints(pod.Spec.TopologySpreadConstraints, v1.ScheduleAnyway)
Expand All @@ -75,7 +75,7 @@ func (pl *PodTopologySpread) initPreScoreState(s *preScoreState, pod *v1.Pod, fi
}
topoSize := make([]int, len(s.Constraints))
for _, node := range filteredNodes {
if !nodeLabelsMatchSpreadConstraints(node.Labels, s.Constraints) {
if requireAllTopologies && !nodeLabelsMatchSpreadConstraints(node.Labels, s.Constraints) {
// Nodes which don't have all required topologyKeys present are ignored
// when scoring later.
s.IgnoredNodes.Insert(node.Name)
Expand Down Expand Up @@ -126,7 +126,11 @@ func (pl *PodTopologySpread) PreScore(
IgnoredNodes: sets.NewString(),
TopologyPairToPodCounts: make(map[topologyPair]*int64),
}
err = pl.initPreScoreState(state, pod, filteredNodes)
// Only require that nodes have all the topology labels if using
// non-system-default spreading rules. This allows nodes that don't have a
// zone label to still have hostname spreading.
requireAllTopologies := len(pod.Spec.TopologySpreadConstraints) > 0 || !pl.systemDefaulted
err = pl.initPreScoreState(state, pod, filteredNodes, requireAllTopologies)
if err != nil {
return framework.AsStatus(fmt.Errorf("calculating preScoreState: %w", err))
}
Expand All @@ -146,7 +150,7 @@ 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) {
(requireAllTopologies && !nodeLabelsMatchSpreadConstraints(node.Labels, state.Constraints)) {
return
}

Expand Down
58 changes: 54 additions & 4 deletions pkg/scheduler/framework/plugins/podtopologyspread/scoring_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ func TestPreScoreStateEmptyNodes(t *testing.T) {
},
nodes: []*v1.Node{
st.MakeNode().Name("node-a").Label(v1.LabelHostname, "node-a").Label(v1.LabelTopologyZone, "mars").Obj(),
st.MakeNode().Name("node-b").Label(v1.LabelHostname, "node-b").Label(v1.LabelTopologyZone, "mars").Obj(),
// Nodes with no zone are not excluded. They are considered a separate zone.
st.MakeNode().Name("node-c").Label(v1.LabelHostname, "node-c").Obj(),
st.MakeNode().Name("node-d").Label(v1.LabelHostname, "node-d").Obj(),
},
objs: []runtime.Object{
&appsv1.ReplicaSet{Spec: appsv1.ReplicaSetSpec{Selector: st.MakeLabelSelector().Exists("foo").Obj()}},
Expand All @@ -142,8 +146,9 @@ func TestPreScoreStateEmptyNodes(t *testing.T) {
IgnoredNodes: sets.NewString(),
TopologyPairToPodCounts: map[topologyPair]*int64{
{key: v1.LabelTopologyZone, value: "mars"}: pointer.Int64Ptr(0),
{key: v1.LabelTopologyZone, value: ""}: pointer.Int64Ptr(0),
},
TopologyNormalizingWeight: []float64{topologyNormalizingWeight(1), topologyNormalizingWeight(1)},
TopologyNormalizingWeight: []float64{topologyNormalizingWeight(4), topologyNormalizingWeight(2)},
},
},
{
Expand Down Expand Up @@ -275,6 +280,7 @@ func TestPodTopologySpreadScore(t *testing.T) {
existingPods []*v1.Pod
nodes []*v1.Node
failedNodes []*v1.Node // nodes + failedNodes = all nodes
objs []runtime.Object
want framework.NodeScoreList
}{
// Explanation on the Legend:
Expand Down Expand Up @@ -425,6 +431,40 @@ func TestPodTopologySpreadScore(t *testing.T) {
{Name: "node-d", Score: 100},
},
},
{
name: "system defaulting, nodes don't have zone, pods match service",
pod: st.MakePod().Name("p").Label("foo", "").Obj(),
existingPods: []*v1.Pod{
// matching pods spread as 4/3/2/1.
st.MakePod().Name("p-a1").Node("node-a").Label("foo", "").Obj(),
st.MakePod().Name("p-a2").Node("node-a").Label("foo", "").Obj(),
st.MakePod().Name("p-a3").Node("node-a").Label("foo", "").Obj(),
st.MakePod().Name("p-a4").Node("node-a").Label("foo", "").Obj(),
st.MakePod().Name("p-b1").Node("node-b").Label("foo", "").Obj(),
st.MakePod().Name("p-b2").Node("node-b").Label("foo", "").Obj(),
st.MakePod().Name("p-b3").Node("node-b").Label("foo", "").Obj(),
st.MakePod().Name("p-c1").Node("node-c").Label("foo", "").Obj(),
st.MakePod().Name("p-c2").Node("node-c").Label("foo", "").Obj(),
st.MakePod().Name("p-d1").Node("node-d").Label("foo", "").Obj(),
},
nodes: []*v1.Node{
st.MakeNode().Name("node-a").Label(v1.LabelHostname, "node-a").Obj(),
st.MakeNode().Name("node-b").Label(v1.LabelHostname, "node-b").Obj(),
st.MakeNode().Name("node-c").Label(v1.LabelHostname, "node-c").Obj(),
st.MakeNode().Name("node-d").Label(v1.LabelHostname, "node-d").Obj(),
},
failedNodes: []*v1.Node{},
objs: []runtime.Object{
&v1.Service{Spec: v1.ServiceSpec{Selector: map[string]string{"foo": ""}}},
},
want: []framework.NodeScore{
// Same scores as if we were using one spreading constraint.
{Name: "node-a", Score: 33},
{Name: "node-b", Score: 55},
{Name: "node-c", Score: 77},
{Name: "node-d", Score: 100},
},
},
{
// matching pods spread as 4/2/1/~3~ (node4 is not a candidate)
name: "one constraint on node, 3 out of 4 nodes are candidates",
Expand Down Expand Up @@ -684,11 +724,21 @@ func TestPodTopologySpreadScore(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)
allNodes := append([]*v1.Node{}, tt.nodes...)
allNodes = append(allNodes, tt.failedNodes...)
state := framework.NewCycleState()
snapshot := cache.NewSnapshot(tt.existingPods, allNodes)
p := &PodTopologySpread{sharedLister: snapshot}
p := &PodTopologySpread{
sharedLister: snapshot,
defaultConstraints: systemDefaultConstraints,
systemDefaulted: true,
}
informerFactory := informers.NewSharedInformerFactory(fake.NewSimpleClientset(tt.objs...), 0)
p.setListers(informerFactory)
informerFactory.Start(ctx.Done())
informerFactory.WaitForCacheSync(ctx.Done())

status := p.PreScore(context.Background(), state, tt.pod, tt.nodes)
if !status.IsSuccess() {
Expand All @@ -698,14 +748,14 @@ func TestPodTopologySpreadScore(t *testing.T) {
var gotList framework.NodeScoreList
for _, n := range tt.nodes {
nodeName := n.Name
score, status := p.Score(context.Background(), state, tt.pod, nodeName)
score, status := p.Score(ctx, state, tt.pod, nodeName)
if !status.IsSuccess() {
t.Errorf("unexpected error: %v", status)
}
gotList = append(gotList, framework.NodeScore{Name: nodeName, Score: score})
}

status = p.NormalizeScore(context.Background(), state, tt.pod, gotList)
status = p.NormalizeScore(ctx, state, tt.pod, gotList)
if !status.IsSuccess() {
t.Errorf("unexpected error: %v", status)
}
Expand Down