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

prefilter extension point implementation for the scheduler #78005

Merged
merged 1 commit into from Jun 14, 2019

Conversation

@ahg-g
Copy link
Member

commented May 17, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
Implements the prefilter extension point in the scheduler

Which issue(s) this PR fixes:

Fixes #77998

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add Pre-filter extension point to the scheduling framework.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Hi @ahg-g. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@@ -454,6 +454,21 @@ func (sched *Scheduler) scheduleOne() {
// Synchronously attempt to find a fit for the pod.
start := time.Now()
pluginContext := framework.NewPluginContext()

// Run "prefilter" plugins.
prefilterStatus := fwk.RunPrefilterPlugins(pluginContext, pod)

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 17, 2019

Member

I don't think we should call Prefilter here. Prefilter, Filter, PostFilter, Scoring, and Normalize Scoring should be invoked by genericScheduler. We should pass pluginContext to genericScheduler and let it run these plugins. We should also start thinking about providing some of the object listers via FrameworkHandle so that plugins can use them.

This comment has been minimized.

Copy link
@ahg-g

ahg-g May 18, 2019

Author Member

Can you elaborate why this should be called from generic scheduler?

We should also start thinking about providing some of the object listers
via FrameworkHandle so that plugins can use them.

Should this be in a separate PR?

This comment has been minimized.

Copy link
@ahg-g

ahg-g May 18, 2019

Author Member

ok, taking a closer look at the code I think it make sense to move prefilter inside generic scheduler since all the other extensions points you mention can only exist there, and it make sense to put prefilter in the same place as filter and post filter.

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 18, 2019

Member

Should this be in a separate PR?

Yes, absolutely. In fact, it does not make sense to put those changes in this PR. The other PR should probably be postponed until we start converting predicate/priority functions.

ok, taking a closer look at the code I think it make sense to move prefilter inside generic scheduler since all the other extensions points you mention can only exist there, and it make sense to put prefilter in the same place as filter and post filter.

That's right. That's exactly the reason.

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 18, 2019

Member

One more thing that I would like to point out for the record (and does not apply to this PR, but is related to our discussion above) is that we should think about removing genericScheduler and replacing it with the Framework.

This comment has been minimized.

Copy link
@ahg-g

ahg-g May 21, 2019

Author Member

Done moving to genericScheduler. PTAL.

I agree with removing genericScheduler.

@ahg-g ahg-g force-pushed the ahg-g:ahg-perfilter branch 2 times, most recently from fc1bc53 to f9eeedd May 21, 2019

@ahg-g ahg-g force-pushed the ahg-g:ahg-perfilter branch from f9eeedd to b7a6e46 May 23, 2019

@misterikkit

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

/ok-to-test

@ahg-g ahg-g force-pushed the ahg-g:ahg-perfilter branch from b7a6e46 to e9d1308 May 24, 2019

@ahg-g

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

/retest

@ahg-g ahg-g force-pushed the ahg-g:ahg-perfilter branch from e9d1308 to 29435eb May 24, 2019

@ahg-g

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

/retest

1 similar comment
@ahg-g

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

/retest

@ahg-g ahg-g referenced this pull request May 28, 2019

Closed

REQUEST: New membership for ahg-g #848

6 of 6 tasks complete
pc *PluginContext, pod *v1.Pod) *Status {
for _, pl := range f.prefilterPlugins {
status := pl.Prefilter(pc, pod)
if !status.IsSuccess() {

This comment has been minimized.

Copy link
@YoubingLi

YoubingLi May 29, 2019

Contributor

Please handle "status == nil" case.

This comment has been minimized.

Copy link
@ahg-g
@YoubingLi

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@ahg-g @bsalamat

All extension points share the same plugin context.

Should we allow to set different plugin context in each extension point?

@ahg-g ahg-g force-pushed the ahg-g:ahg-perfilter branch from cf186f1 to fe790d8 May 29, 2019

@ahg-g

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

@ahg-g @bsalamat

All extension points share the same plugin context.

Should we allow to set different plugin context in each extension point?

Can you explain why we need to do that? The purpose of PluginContext is to share state across plugins.

Plugin private data should be stored in the plugin struct, see example here: https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/plugins/examples/stateful/stateful.go#L32

@ahg-g

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

/retest pull-kubernetes-dependencies

@ahg-g

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

/retest pull-kubernetes-verify

@ahg-g

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

/retest

@@ -201,6 +201,12 @@ func (g *genericScheduler) Schedule(pod *v1.Pod, nodeLister algorithm.NodeLister
return result, err
}

// Run "prefilter" plugins.

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 30, 2019

Member

Please move this block to line 192, right after the podPassesBasicChecks block. In fact, podPassesBasicChecks should be eventually converted to a default prefilter plugin.

This comment has been minimized.

Copy link
@ahg-g

ahg-g May 30, 2019

Author Member

Done.

@ahg-g ahg-g force-pushed the ahg-g:ahg-perfilter branch from 590be90 to f87eecd May 30, 2019

@bsalamat
Copy link
Member

left a comment

Looks good. Thanks, @ahg-g! Please go ahead and squash commits.

@ahg-g ahg-g force-pushed the ahg-g:ahg-perfilter branch from f87eecd to a61a437 Jun 10, 2019

@bsalamat
Copy link
Member

left a comment

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, bsalamat

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

@k8s-ci-robot k8s-ci-robot merged commit d8695d0 into kubernetes:master Jun 14, 2019

23 checks passed

cla/linuxfoundation ahg-g authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
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-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
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
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.