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.

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

Add default constraints to PodTopologySpread plugin #88671

Merged

Conversation

@alculquicondor
Copy link
Member

alculquicondor commented Feb 28, 2020

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds ability to set default constraints for the PodTopologySpread plugin. Pod's that don't define any constraints in pod.spec.topologySpreadConstraints get constraints deduced from the provided default.
Label selectors are deduced from the membership of the pod to Services, Replication Controllers, Replica Sets or Stateful Sets, similar to DefaultPodTopologySpread plugin.

With this PR, k8s is not setting any defaults.

However, I updated the benchmark that compares PodTopologySpread against DefaultPodTopologySpread to use the new feature. This is what I got:

pkg: k8s.io/kubernetes/pkg/scheduler/framework/plugins/defaultpodtopologyspread
BenchmarkTestSelectorSpreadPriority/100nodes-56         	    6190	    192691 ns/op
BenchmarkTestSelectorSpreadPriority/1000nodes-56        	     796	   1434913 ns/op

pkg: k8s.io/kubernetes/pkg/scheduler/framework/plugins/podtopologyspread
BenchmarkTestDefaultEvenPodsSpreadPriority/100nodes-56         	    1984	    581228 ns/op
BenchmarkTestDefaultEvenPodsSpreadPriority/1000nodes-56        	     320	   3809369 ns/op

Which gives a difference of 3x for 100 nodes and 2.7x for 1000 nodes. So we maintain the same complexity but with different constant #84936. I don't think we are ready to add the k8s default this release.

Which issue(s) this PR fixes:

Part of #80639

Special notes for your reviewer:

Note that the changes only affect PreFilter and PreScore endpoints, at which point, we store the processed constraints and proceed (as before) with Filter and Score using them.

Does this PR introduce a user-facing change?:

DefaultConstraints can be specified for the PodTopologySpread plugin in the component config

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-scheduling/20190926-default-even-pod-spreading.md
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 28, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@alculquicondor alculquicondor force-pushed the alculquicondor:feat/default-spreading branch from 3ae3df7 to 3ad8bdd Feb 28, 2020
@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 28, 2020

/hold for reviews

@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 28, 2020

/assign @Huang-Wei

@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 28, 2020

/retest

Copy link
Member

Huang-Wei left a comment

/lgtm

Thanks @alculquicondor .

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 29, 2020
Copy link
Member

ahg-g left a comment

default_pod_topology_spread skips scoring if podTopologySpread is set (Lines 77 and 106), pls add a todo/comment that this doesn't address the case where a default PodTolopologySpread constraint is set.


b.ResetTimer()

for i := 0; i < b.N; i++ {
var gotList framework.NodeScoreList
status := p.PreScore(context.Background(), state, pod, filteredNodes)
status := p.PreScore(ctx, state, pod, filteredNodes)

This comment has been minimized.

Copy link
@ahg-g

ahg-g Mar 1, 2020

Member

we should also call NormalizeScore at the end, also for the default_pod_topology_spread.

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Mar 2, 2020

Author Member

It's already here but it wasn't in the default_pod_topology_spread. This means that we have only a 3x difference, instead of 5x.

Copy link
Member Author

alculquicondor left a comment

default_pod_topology_spread skips scoring if podTopologySpread is set (Lines 77 and 106), pls add a todo/comment that this doesn't address the case where a default PodTolopologySpread constraint is set.

Added a comment. PodTopologySpread with a default and DefaultPodTopologySpread aren't supposed to co-exist. I'll make sure to include this in the documentation.

@k8s-ci-robot k8s-ci-robot added size/XXL and removed lgtm size/XL labels Mar 2, 2020
@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Mar 2, 2020

/retest

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Mar 2, 2020

looks good, feel free to squash.

And update benchmark for even pod spreading to use default constraints

Signed-off-by: Aldo Culquicondor <acondor@google.com>
@alculquicondor alculquicondor force-pushed the alculquicondor:feat/default-spreading branch from 2991549 to 73ad385 Mar 2, 2020
@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Mar 2, 2020

Rebased :)

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Mar 2, 2020

/lgtm

@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Mar 2, 2020

/hold cancel

@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Mar 2, 2020

/priority important-soon

@k8s-ci-robot k8s-ci-robot merged commit 1b4b155 into kubernetes:master Mar 2, 2020
16 checks passed
16 checks passed
cla/linuxfoundation alculquicondor authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-e2e-kind-ipv6 Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.