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 writing out of tree custom scheduler plugins #78162

Merged
merged 4 commits into from Jul 16, 2019

Conversation

@hex108
Copy link
Member

commented May 21, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
Add support for writing out of tree custom scheduler plugins

Which issue(s) this PR fixes:

Fixes #

Partially fix #78093

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add support for writing out of tree custom scheduler plugins.

/sig scheduling

@hex108

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

Users could write scheduler with plugins in the same way as https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-scheduler/scheduler.go, and just need register their own plugins.

func main() {
        //  ==> Register plugins here
        registry := app.GetSchedulerRegistry()
        registry.Register("example-plugin", ExamplePlugin)
 
	command := app.NewSchedulerCommand()
	if err := command.Execute(); err != nil {
		fmt.Fprintf(os.Stderr, "%v\n", err)
		os.Exit(1)
	}
}
@ahg-g

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Users could write scheduler with plugins in the same way as https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-scheduler/scheduler.go, and just need register their own plugins.

func main() {
        //  ==> Register plugins here
        registry := app.GetSchedulerRegistry()
        registry.Register("example-plugin", ExamplePlugin)
 
	command := app.NewSchedulerCommand()
	if err := command.Execute(); err != nil {
		fmt.Fprintf(os.Stderr, "%v\n", err)
		os.Exit(1)
	}
}

This design feels a little like an anti-pattern. I worry about the global variable being introduced, especially that it is accompanied with a method that basically makes it accessible from anywhere. How about we add a WithPlugin option to NewSchedulerCommand.

The above example would look like this:

func main() {
	command := app.NewSchedulerCommand(
                  app.WithPlugin("example-plugin1", ExamplePlugin1),
                  app.WithPlugin("example-plugin2", ExamplePlugin2))
	if err := command.Execute(); err != nil {
		fmt.Fprintf(os.Stderr, "%v\n", err)
		os.Exit(1)
	}
}

WithPlugin function would look something like this:

func WithPlugin(name string, factory PluginFactory) {
         return func(registry framework.Registry) {
		registry.Register(name, factory)
	}
}
@hex108

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

@ahg-g Thanks for the suggestion! There is no option for app.NewXXXCommand of other components(e.g. kube-apiserver), I am no sure it is OK to add it for scheduler. If others do not object it, I'll add it later.

If it is inappropriate to introduce a global variable, we could just export a function like func RegisterSchedulerRegistry(name, plugin) error.

@ahg-g

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@ahg-g Thanks for the suggestion! There is no option for app.NewXXXCommand of other components(e.g. kube-apiserver), I am no sure it is OK to add it for scheduler. If others do not object it, I'll add it later.

I am not sure if other commands allow out of tree customization the way we want to support it here, and so looking at them may not be that insightful, but sure using the with pattern might be an overkill.

If it is inappropriate to introduce a global variable, we could just export a function like func RegisterSchedulerRegistry(name, plugin) error.

That is certainly better than returning a global variable. Nit: can you please name it RegisterFrameworkPlugin.

I am guessing you will want to use a struct to encapsulate the state and remove the global variable? and the struct will have NewSchedulerCommand, Run and RegisterFrameworkPlugin as member functions?

@hex108 hex108 force-pushed the hex108:registry branch from cd8b68a to f1a3027 May 28, 2019

@k8s-ci-robot k8s-ci-robot added size/S and removed size/XS labels May 28, 2019

@hex108

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

I am guessing you will want to use a struct to encapsulate the state and remove the global variable? and the struct will have NewSchedulerCommand, Run and RegisterFrameworkPlugin as member functions?

It seems enough to export RegisterFrameworkPlugin, it might make it complex to add a struct for them. I added a new commit for it. @ahg-g

@ahg-g

This comment has been minimized.

Copy link
Member

commented May 28, 2019

I am guessing you will want to use a struct to encapsulate the state and remove the global variable? and the struct will have NewSchedulerCommand, Run and RegisterFrameworkPlugin as member functions?

It seems enough to export RegisterFrameworkPlugin, it might make it complex to add a struct for them. I added a new commit for it. @ahg-g

I don't think it is complex, but perhaps this is enough if it is supposed to be a singleton.

@hex108 hex108 force-pushed the hex108:registry branch from f1a3027 to ada2236 May 28, 2019

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

Closed

REQUEST: New membership for ahg-g #848

6 of 6 tasks complete
@hex108

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

/retest

1 similar comment
@hex108

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

/retest

@hex108

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

@hex108: GitHub didn't allow me to assign the following users: baasbank.

Note that only kubernetes members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @baasbank @misterikkit

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.

@hex108

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

@hex108 hex108 force-pushed the hex108:registry branch from ada2236 to 3270fd3 Jun 24, 2019

@hex108

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

@misterikkit I learned a lot from the link. Thanks! Updated the code, PTAL
cc @baasbank @ahg-g

@hex108

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@hex108 hex108 force-pushed the hex108:registry branch from 036b6dd to 107e989 Jun 25, 2019

@hex108

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

/retest

1 similar comment
@hex108

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

/retest

@ahg-g

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

/lgtm

@bsalamat
Copy link
Member

left a comment

It is not obvious to me that this is the best way to enable out of tree plugins. The approach we choose here will be used by custom schedulers and changing it will break them. So, in a way the interface introduced here will be a part of our API that cannot be changed easily in the future. For that reason, I would like to see a small proposal showing a few possible approaches with pros and cons of each. Then, we can decide which one to take.

@hex108

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

It is not obvious to me that this is the best way to enable out of tree plugins. The approach we choose here will be used by custom schedulers and changing it will break them. So, in a way the interface introduced here will be a part of our API that cannot be changed easily in the future. For that reason, I would like to see a small proposal showing a few possible approaches with pros and cons of each. Then, we can decide which one to take.

Thanks Bobby for the suggestion! I will write a doc for it. In my plan, I’d like to support both ways in #78093 for users. Users could choose one of them.

@hex108

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@bsalamat
Copy link
Member

left a comment

Thanks, @hex108 for writing the doc and exploring other options. I think this solution makes sense. I just have minor comment about naming.

// NewSchedulerCommand creates a *cobra.Command object with default parameters
func NewSchedulerCommand() *cobra.Command {
// PluginOption is an function type used for dealing with scheduler registry.
type PluginOption func(framework.Registry) error

This comment has been minimized.

Copy link
@bsalamat

bsalamat Jul 10, 2019

Member

PluginOption is a bit confusing. I don't have a great suggestion for the name. Does PluginRegisterer make more sense?

This comment has been minimized.

Copy link
@misterikkit

misterikkit Jul 10, 2019

Contributor

The only exported things in this package are NewSchedulerCommand and Run. I suggest that the option type be called Option.

This comment has been minimized.

Copy link
@hex108

hex108 Jul 11, 2019

Author Member

I am not good at naming. From my perspective, Option is most general that it could deal with everything to framework.Registry, PluginOption represents that it deals with registry plugin related things, PluginRegisterer represents that it registries plugin to framework.Registry, that is what WithPlugin is doing now.

Scheduler also has Option. If we'll add some thing more WithXXX for framework.Registry, it might be better to use Option, otherwise either one is OK for me(considering it will be arguments for some functions(e.g. Run, runCommand), Option is a little confusing from its name, PluginRegisterer or PluginOption might be better).

This comment has been minimized.

Copy link
@hex108

hex108 Jul 11, 2019

Author Member

Scheduler also has Option. If we'll add some thing more WithXXX for framework.Registry, it might be better to use Option

Because of this reason, I renamed PluginOption to Option in the latest commit. Does it make sense for you? PTAL @bsalamat @misterikkit

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 11, 2019

type Option func(framework.Registry) error

// NewSchedulerCommand creates a *cobra.Command object with default parameters and registryOptions
func NewSchedulerCommand(registryOptions ...Option) *cobra.Command {

This comment has been minimized.

Copy link
@hex108

hex108 Jul 11, 2019

Author Member

There is an imported package named options, so I use registryOptions instead of options for the argument name, and it seems more clear for its purpose.

// NewSchedulerCommand creates a *cobra.Command object with default parameters
func NewSchedulerCommand() *cobra.Command {
// Option configures a framework.Registry.
type Option func(framework.Registry) error

This comment has been minimized.

Copy link
@bsalamat

bsalamat Jul 12, 2019

Member

Option is not a great name for a function. It should start with a verb or has an action in the name. Something like pluginAdder.

This comment has been minimized.

Copy link
@misterikkit

misterikkit Jul 12, 2019

Contributor

we are not naming a function, we are naming a type. It's a fairly well accepted pattern in golang at this point. https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

This comment has been minimized.

Copy link
@bsalamat

bsalamat Jul 16, 2019

Member

alright. I guess I should accept it since it is an accepted pattern. 🙂

This comment has been minimized.

Copy link
@misterikkit

misterikkit Jul 16, 2019

Contributor

I wish "an accepted pattern" wasn't the reason you approved. It's always good to question whether established patterns are serving us. In this specific case, I think it makes more sense from outside the package. If you would read the godoc, you would see,

func New(a, b int, x, y string, opts... Option) {/*...*/}

func WithFooCount(i int) Option {/*...*/}
func WithBarName(name string) Option {/*...*/}

If you have that API, it actually doesn't matter to me what the Option type looks like. It just so happens that in golang, the func-based implementation is pretty terse.

@bsalamat
Copy link
Member

left a comment

/lgtm
/approve

Thanks, @hex108!

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 16, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, hex108

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

@hex108

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

/retest

3 similar comments
@hex108

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

/retest

@hex108

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

/retest

@hex108

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit c30f024 into kubernetes:master Jul 16, 2019

23 checks passed

cla/linuxfoundation hex108 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
@hex108

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

Thanks all!

// WithPlugin creates an Option based on plugin name and factory.
func WithPlugin(name string, factory framework.PluginFactory) Option {
return func(registry framework.Registry) error {
return registry.Register(name, factory)

This comment has been minimized.

Copy link
@tedyu

tedyu Jul 16, 2019

Contributor

I wonder if there is any validation that can be done for the registration.

This comment has been minimized.

Copy link
@hex108

hex108 Jul 16, 2019

Author Member

What kind of validation?

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.