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

[migration phase 1] Make scheduler cache, volume binder and listers available when registering default plugins #83694

Merged
merged 1 commit into from Oct 11, 2019

Conversation

@ahg-g
Copy link
Member

commented Oct 9, 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 feature

What this PR does / why we need it:

Plumped handlers necessary to instantiate some predicates/priorities. This is necessary during phase 1 of framework migration.

Which issue(s) this PR fixes:
Fixes #83687

NONE
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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 requested review from draveness and Huang-Wei Oct 9, 2019
@ahg-g ahg-g force-pushed the ahg-g:ahg-adv-config branch from faee910 to d6b4061 Oct 9, 2019
@ahg-g ahg-g force-pushed the ahg-g:ahg-adv-config branch from d6b4061 to 908b273 Oct 9, 2019
)

// DefaultRegistryArgs arguments needed to create default plugin factories.
type DefaultRegistryArgs struct {

This comment has been minimized.

Copy link
@hex108

hex108 Oct 10, 2019

Member

I prefer to name it to RegistryArgs, because when declaring a DefaultRegistryArgs variable does not really give you a default registry args by default.

This comment has been minimized.

Copy link
@ahg-g

ahg-g Oct 10, 2019

Author Member

This is args for default registry, not default RegistryArgs, but I see how this can be interpreted the other way, I changed it as you suggested.

time.Duration(options.bindTimeoutSeconds)*time.Second)

if options.frameworkRegistry == nil {
options.frameworkRegistry = frameworkplugins.NewDefaultRegistry(&frameworkplugins.DefaultRegistryArgs{

This comment has been minimized.

Copy link
@hex108

hex108 Oct 10, 2019

Member

If we create a new function(e.g. NewRegistryArgs), we could call it from cmd/kube-scheduler/app/server.go, then we do not need to disable out-of-tree framework plugins.

This comment has been minimized.

Copy link
@ahg-g

ahg-g Oct 10, 2019

Author Member

that will require us to define global variables for the cache and volume binder, which is a pain to deal with. The framework is changing so fast right now there is no point in actually supporting out-of-tree plugins before beta.

This comment has been minimized.

Copy link
@hex108

hex108 Oct 10, 2019

Member

that will require us to define global variables for the cache and volume binder

I could not get your point why we need to define global variables for them, we could still declare them as local variables and use them as following. Is there any problem?

	schedulerCache := internalcache.New(30*time.Second, stopCh)
	volumeBinder := volumebinder.NewVolumeBinder(client, nodeInformer, pvcInformer, pvInformer, storageClassInformer

The framework is changing so fast right now there is no point in actually supporting out-of-tree plugins before beta.

We are trying this feature, not sure whether others are using it now. Even it is alpha, we could get feedback from users. If it is not a must, I think it might be better to keep it.

This comment has been minimized.

Copy link
@ahg-g

ahg-g Oct 10, 2019

Author Member

that will require us to define global variables for the cache and volume binder

I could not get your point why we need to define global variables for them, we could still declare them as local variables and use them as following. Is there any problem?

	schedulerCache := internalcache.New(30*time.Second, stopCh)
	volumeBinder := volumebinder.NewVolumeBinder(client, nodeInformer, pvcInformer, pvInformer, storageClassInformer

schedulerCache and volumeBinder are shared handlers, we can't instantiate them multiple times, they need to be created once.

The framework is changing so fast right now there is no point in actually supporting out-of-tree plugins before beta.

We are trying this feature, not sure whether others are using it now. Even it is alpha, we could get feedback from users. If it is not a must, I think it might be better to keep it.

It is going to be tricky to keep this, and it is very premature to say we support out-of-tree plugins at this stage because we are changing the framework very frequently as we discover its limitation during the migration. If you are testing this feature, I think you can still carry a small patch to enable this back in your dev environment. Is that reasonable?

@ahg-g ahg-g changed the title Disabled out-of-tree framework plugins [migration phase 1] Disabled out-of-tree framework plugins Oct 10, 2019
if len(registryOptions) != 0 {
return errors.New("out-of-tree framework plugins are not yet supported")
}
Comment for lines 162  – 166

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Oct 10, 2019

Member

Instead of totally disabling it, does a warning giving the limitations sound better? If so, does it resolve @hex108 's concern?

volumeBinder := volumebinder.NewVolumeBinder(client, nodeInformer, pvcInformer, pvInformer, storageClassInformer,
time.Duration(options.bindTimeoutSeconds)*time.Second)

if options.frameworkRegistry == nil {

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Oct 10, 2019

Member

If external users register something into frameworkRegistry, i.e. it's not nil, IMO it's still possible for the internal scheduler to register the frameworkplugins.RegistryArgs below? Are you concerned with some side effects? If so, we can warn users.

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Oct 10, 2019

Member

I mean we can still give a bare frameworkRegistry to end users to register custom things at cmd/.../app.go, and then inject the necessary fields here (to pass to legacy predicates/priorities).

This comment has been minimized.

Copy link
@ahg-g

ahg-g Oct 10, 2019

Author Member

I think I know how to do this. We can add an option to scheduler.New to pass a "outOfTreeRegistry", which collects out-of-tree plugins that we append to the default registry, I will send a patch to show you how it looks, thanks for brainstorming.

@ahg-g ahg-g changed the title [migration phase 1] Disabled out-of-tree framework plugins [migration phase 1] Make schedulercache, volume binder available when registering default plugins Oct 10, 2019
@ahg-g ahg-g changed the title [migration phase 1] Make schedulercache, volume binder available when registering default plugins [migration phase 1] Make scheduler cache, volume binder and listers available when registering default plugins Oct 10, 2019
@ahg-g ahg-g force-pushed the ahg-g:ahg-adv-config branch 2 times, most recently from dd314d7 to a8ce4ac Oct 10, 2019
…actories for default plugins
@ahg-g ahg-g force-pushed the ahg-g:ahg-adv-config branch from a8ce4ac to 456df97 Oct 10, 2019
@Huang-Wei

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

/lgtm

Thanks, @ahg-g !

@Huang-Wei

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

/retest

@hex108

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

It is great!

The release note also needs to be changed. :)

@k8s-ci-robot k8s-ci-robot merged commit a5e6ac0 into kubernetes:master Oct 11, 2019
16 checks passed
16 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-kind-ipv6 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-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.17 milestone Oct 11, 2019
ohsewon added a commit to ohsewon/kubernetes that referenced this pull request Oct 16, 2019
[migration phase 1] Make scheduler cache, volume binder and listers available when registering default plugins
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.