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 support for multiple scheduling profiles #88285

Merged

Conversation

@alculquicondor
Copy link
Member

alculquicondor commented Feb 18, 2020

What type of PR is this?

/kind feature

What this PR does / why we need it:

kube-scheduler can run with more than one profile. Given a pod, the profile is selected by using .spec.SchedulerName.
Profiles should have different scheduler names. They should have the same queue sort plugin configuration.

Which issue(s) this PR fixes:

Part of #85737, kubernetes/enhancements#1451

Special notes for your reviewer:

This PR builds on top of #88087, so you can review from the commit titled: Support multiple scheduling profiles in a single scheduler

For convenience, I split this PR in 2 commits, which will be squashed after the review.

  • implementation and tests for new package.
  • fixes for existing tests.

A follow up PR will add unit and integration tests to exercise the multiple profiles.

Does this PR introduce a user-facing change?:

kube-scheduler can run more than one scheduling profile. Given a pod, the profile is selected by using its `.spec.SchedulerName`.

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

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-scheduling/20200114-multi-scheduling-profiles.md
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 19, 2020

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

pkg/scheduler/factory_test.go Outdated Show resolved Hide resolved
pkg/scheduler/factory_test.go Outdated Show resolved Hide resolved
pkg/scheduler/factory_test.go Outdated Show resolved Hide resolved
pkg/scheduler/scheduler_test.go Outdated Show resolved Hide resolved
@@ -591,7 +598,7 @@ func TestSchedulerFailedSchedulingReasons(t *testing.T) {

// queuedPodStore: pods queued before processing.
// scache: scheduler cache that might contain assumed pods.
func setupTestScheduler(queuedPodStore *clientcache.FIFO, scache internalcache.Cache, informerFactory informers.SharedInformerFactory, recorder events.EventRecorder, fakeVolumeBinder *volumebinder.VolumeBinder, fns ...st.RegisterPluginFunc) (*Scheduler, chan *v1.Binding, chan error) {
func setupTestScheduler(queuedPodStore *clientcache.FIFO, scache internalcache.Cache, informerFactory informers.SharedInformerFactory, broadcaster events.EventBroadcaster, fakeVolumeBinder *volumebinder.VolumeBinder, fns ...st.RegisterPluginFunc) (*Scheduler, chan *v1.Binding, chan error) {

This comment has been minimized.

Copy link
@ahg-g

ahg-g Feb 19, 2020

Member

Instead of passing it as an argument, I would create the EventBroadcaster inside the function and return it (since a couple of tests require it).

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Feb 19, 2020

Author Member

It seems that there are the same amount of tests that use it than tests that don't.
I prefer to keep it as is for now given that the PR is quite big already.

pkg/scheduler/factory.go Outdated Show resolved Hide resolved
pkg/scheduler/profile/profile.go Outdated Show resolved Hide resolved
@alculquicondor alculquicondor force-pushed the alculquicondor:multiprofiles-runtime branch from f7464c4 to 0b55fe5 Feb 19, 2020
@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 19, 2020

/retest

@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 19, 2020

/assign @Huang-Wei for a second pass

It looks like the api-review is almost done

Copy link
Member

Huang-Wei left a comment

Some comments.

pkg/scheduler/core/generic_scheduler.go Outdated Show resolved Hide resolved
cmd/kube-scheduler/app/server.go Show resolved Hide resolved
cmd/kube-scheduler/app/server.go Outdated Show resolved Hide resolved
prof := &c.profiles[i]
plugins := &schedulerapi.Plugins{}
plugins.Append(defaultPlugins)
plugins.Apply(prof.Plugins)
prof.Plugins = plugins
Comment on lines +184 to +187

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Feb 20, 2020

Member

nit: these 5 lines can be extracted into a function to avoid duplication (with L304~L308)

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Feb 20, 2020

Author Member

I wouldn't mind too much. They are slightly different and createFromConfig should go away soon when we remove policy support.

pkg/scheduler/core/generic_scheduler.go Outdated Show resolved Hide resolved
pkg/scheduler/scheduler.go Show resolved Hide resolved
@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 21, 2020

/priority important-soon

/hold for #88087

@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 25, 2020

/hold cancel

@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 25, 2020

/assign @msau42
for test

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 25, 2020

@alculquicondor: GitHub didn't allow me to assign the following users: for, test.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @msau42 for test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Member

ahg-g left a comment

looks good to me.

@@ -215,7 +197,9 @@ func WithPodMaxBackoffSeconds(podMaxBackoffSeconds int64) Option {
}

var defaultSchedulerOptions = schedulerOptions{
schedulerName: v1.DefaultSchedulerName,
profiles: []schedulerapi.KubeSchedulerProfile{
{SchedulerName: v1.DefaultSchedulerName},

This comment has been minimized.

Copy link
@ahg-g

ahg-g Feb 25, 2020

Member

Add a comment clarifying that the default provider plugins is used by default

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Feb 25, 2020

Author Member

Done

Signed-off-by: Aldo Culquicondor <acondor@google.com>
@alculquicondor alculquicondor force-pushed the alculquicondor:multiprofiles-runtime branch from de4522c to c048858 Feb 25, 2020
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Feb 25, 2020

/lgtm

@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 25, 2020

/retest

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Feb 25, 2020

/approve
for test

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 25, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, msau42

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

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 25, 2020

/retest

1 similar comment
@alculquicondor

This comment has been minimized.

Copy link
Member Author

alculquicondor commented Feb 25, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit f7c37d3 into kubernetes:master Feb 26, 2020
17 checks passed
17 checks passed
cla/linuxfoundation alculquicondor authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ipv6-parallel 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 Feb 26, 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

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