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 scheduling framework configuration #77501

Merged
merged 2 commits into from May 18, 2019

Conversation

@JieJhih
Copy link
Member

commented May 6, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:
Add scheduling framework configuration
Which issue(s) this PR fixes:

Fixes #77365

Ref. kubernetes/enhancements#624

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add configuration options for the scheduling framework and its plugins.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Hi @JieJhih. 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.

@JieJhih

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

/assign @bsalamat

@draveness

This comment has been minimized.

Copy link
Member

commented May 6, 2019

/ok-to-test
/priority important-soon
/kind feature

@misterikkit
Copy link
Contributor

left a comment

Thanks for the implementation. 😄

@@ -86,6 +87,8 @@ type KubeSchedulerConfiguration struct {
// Value must be non-negative integer. The value zero indicates no waiting.
// If this value is nil, the default value will be used.
BindTimeoutSeconds *int64
Plugins Plugins

This comment has been minimized.

Copy link
@misterikkit

misterikkit May 6, 2019

Contributor

These will need docstrings. Feel free to reach out to me if you're not sure what to write.

Name string
}

type ScorePlugin struct {

This comment has been minimized.

Copy link
@misterikkit

misterikkit May 6, 2019

Contributor

@bsalamat we went back and forth over whether to make this a separate struct. Are you happy with this implementation? I don't recall the details of that conversation.

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 6, 2019

Member

I prefer to have a single type for all plugins. The framework will ignore Weight where it doesn't make sense to have a weight.

This comment has been minimized.

Copy link
@JieJhih

JieJhih May 7, 2019

Author Member

Agree. Weight it doesn't seem to work in framework now.
I have unified all plugins type. PTAL.

// TODO: 'nil' should be replaced by plugin config.
p, err := factory(nil, f)
// use configuration to decide which plugins to instantiate.
pc := pluginConfig(name, args)

This comment has been minimized.

Copy link
@misterikkit

misterikkit May 6, 2019

Contributor

What if we put the plugin configs into a map so the value can be looked up directly for each factory?

This comment has been minimized.

Copy link
@JieJhih

JieJhih May 7, 2019

Author Member

Updated. PTAL.
Thanks!!

@bsalamat
Copy link
Member

left a comment

We need some tests as well.

Name string
}

type ScorePlugin struct {

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 6, 2019

Member

I prefer to have a single type for all plugins. The framework will ignore Weight where it doesn't make sense to have a weight.

@@ -38,7 +39,7 @@ type framework struct {
var _ = Framework(&framework{})

// NewFramework initializes plugins given the configuration and the registry.
func NewFramework(r Registry, _ *runtime.Unknown) (Framework, error) {
func NewFramework(r Registry, args []config.PluginConfig) (Framework, error) {

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 6, 2019

Member

We need to pass both Plugins and PluginConfig to NewFramewok and change the initialization for loop to build only those Plugins whose names appear in Plugins.

This comment has been minimized.

Copy link
@JieJhih

JieJhih May 7, 2019

Author Member

Updated. PTAL.
Thanks!!

@@ -112,3 +114,13 @@ func (f *framework) RunReservePlugins(
func (f *framework) NodeInfoSnapshot() *cache.NodeInfoSnapshot {
return f.nodeInfoSnapshot
}

func pluginConfig(name string, args []config.PluginConfig) *runtime.Unknown {

This comment has been minimized.

Copy link
@wgliang

wgliang May 7, 2019

Member

Why not args use the map structure?

type Plugins struct {
// EnabledPlugins specifies plugins that should be enabled in addition to default plugins.
EnabledPlugins *PluginSet
// DisabledPlugins specifies default plugins that should be disabled.

This comment has been minimized.

Copy link
@liggitt

liggitt May 17, 2019

Member

need to clearly describe how to disable all default plugins. enumerate all default plugins in the disabled list (if so, how do they guard against new default plugins on upgrade)? include one with name: * in the disabled list?

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 17, 2019

Member

Another great point. We should support "disable all", although disabling plugins is not implemented yet. We still should have a plan for it and document it in the API.

@liggitt

This comment has been minimized.

Copy link
Member

commented May 17, 2019

config looks much cleaner, thanks. one question about nesting structure and a couple doc suggestions , but this looks fine for alpha config

edit: the ordering question at https://github.com/kubernetes/kubernetes/pull/77501/files#r285259640 needs to be resolved, since that could affect the shape of the options

@liggitt liggitt moved this from In progress to Completed, 1.15 in API Reviews May 17, 2019

@liggitt liggitt self-assigned this May 17, 2019

// a particular extension point are the only ones enabled. If an extension point is
// omitted from the config, then the default set of plugins is used for that extension point.
type Plugins struct {
// EnabledPlugins specifies plugins that should be enabled in addition to default plugins.

This comment has been minimized.

Copy link
@liggitt

liggitt May 17, 2019

Member

in addition to default plugins

if the order in these enabled slices is significant, but there can also be default plugins, in what order are the default plugins and the ordered plugins in this list combined?

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 17, 2019

Member

Great point. We should probably go with "defaults first" then "enabled ones". If that is not desired, one can disable defaults and re-enable everything (defaults and custom ones) in a different order.

@liggitt liggitt moved this from Completed, 1.15 to Changes requested in API Reviews May 17, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 17, 2019

@bsalamat
Copy link
Member

left a comment

Thanks again, @JieJhih!

reserve:
enabled:
- name: foo
- name: bar

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 17, 2019

Member

Could you also add disabled plugins here for testing purposes?

This comment has been minimized.

Copy link
@JieJhih

JieJhih May 17, 2019

Author Member

Updated. PTAL.
Thanks!!

@@ -141,11 +142,18 @@ type KubeSchedulerLeaderElectionConfiguration struct {
LockObjectName string
}

// Plugins include multiple extension points. When specified, the list of plugins for
// Plugins include enabled plugins and disabled plugins, when enabled, the list of plugins for

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 17, 2019

Member

Please update the doc string as suggested above and add:

Enabled plugins are called in the order specified here, after default plugins. If they need to be invoked before default plugins, default plugins must be disabled and re-enabled here in desired order.

Unreserve *PluginSet
}

// PluginSet has an array of plugins for each extension point.

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 17, 2019

Member

PluginSet specifies enabled and disabled plugins for an extension point.

This comment has been minimized.

Copy link
@JieJhih

JieJhih May 17, 2019

Author Member

Updated. PTAL.
Thanks!!

// PluginSet has an array of plugins for each extension point.
// If an array is empty, missing, or nil, default plugins at that extension point will be used.
type PluginSet struct {
// Enabled specifies plugins that should be enabled in addition to default plugins.

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 17, 2019

Member

Please add:
These are called after default plugins and in the same order specified here.

This comment has been minimized.

Copy link
@tedyu

tedyu May 18, 2019

Contributor

Since order is significant, I wonder if the name PluginSet conveys this requirement.

type PluginSet struct {
// Enabled specifies plugins that should be enabled in addition to default plugins.
Enabled []Plugin
// Disabled specifies default plugins that should be disabled.

This comment has been minimized.

Copy link
@bsalamat

bsalamat May 17, 2019

Member

Please add
When all default plugins need to be disabled, an array containing only one "*" should be provided.

@bsalamat
Copy link
Member

left a comment

/lgtm

Thanks a lot, @JieJhih!

@k8s-ci-robot k8s-ci-robot added the lgtm label May 17, 2019

add scheduling framework configuration
update bazel build

fix get plugin config method

initialize only needed plugins

fix unit test

fix import duplicate package

update bazel

add docstrings

add weight field to plugin

add plugin to v1alpha1

add plugins at appropriate extension points

remove todo statement

fix import package file path

set plugin json schema

add plugin unit test to option

initial plugin in test integration

initialize only needed plugins

update bazel

rename func

change plugins needed logic

remove v1 alias

change the comment

fix alias shorter

remove blank line

change docstrings

fix map bool to struct

add some docstrings

add unreserve plugin

fix docstrings

move variable inside the for loop

make if else statement cleaner

remove plugin config from reserve plugin unit test

add plugin config and reduce unnecessary options for unit test

update bazel

fix race condition

fix permit plugin integration

change plugins to be pointer

change weight to int32

fix package alias

initial queue sort plugin

rename unreserve plugin

redesign plugin struct

update docstrings

check queue sort plugin amount

fix error message

fix condition

change plugin struct

add disabled plugin for unit test

fix docstrings

handle nil plugin set

@JieJhih JieJhih force-pushed the JieJhih:scheduling/plugin branch from 4e659d4 to 2cd5fc5 May 17, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 17, 2019

@bsalamat

This comment has been minimized.

Copy link
Member

commented May 17, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 17, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 17, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented May 17, 2019

This config looks workable. Would be good to make sure there are unit tests covering the scenarios where someone wants to disable a default plugin, or disable all and specify their own order, or include a default plugin in an explicit order

/approve
for config changes

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, JieJhih, liggitt

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

@bsalamat

This comment has been minimized.

Copy link
Member

commented May 18, 2019

@liggitt The logic to disable plugins are not added yet. It will be added later as a separate PR. In fact, we haven't added the logic to define default plugins which is the first step before adding the logic to disable them. When we add that logic later, we will definitely add tests to cover it.

@bsalamat
Copy link
Member

left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 18, 2019

@fejta-bot

This comment has been minimized.

Copy link

commented May 18, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit df4f033 into kubernetes:master May 18, 2019

20 checks passed

cla/linuxfoundation JieJhih 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-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-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
// PluginSet has an array of plugins for each extension point.
// If an array is empty, missing, or nil, default plugins at that extension point will be used.
type PluginSet struct {
// Enabled specifies plugins that should be enabled in addition to default plugins.

This comment has been minimized.

Copy link
@tedyu

tedyu May 18, 2019

Contributor

Since order is significant, I wonder if the name PluginSet conveys this requirement.

}

// find the config args of a plugin
pc := pluginConfig[name]

This comment has been minimized.

Copy link
@tedyu

tedyu May 18, 2019

Contributor

Should we check that the plugin has corresponding config ?

This comment has been minimized.

Copy link
@misterikkit

misterikkit May 20, 2019

Contributor

For now, I think it's okay to pass nil config into plugin factories. I'm not sure it would help to pass in an "empty" runtime.Unknown object.

@JieJhih JieJhih referenced this pull request May 19, 2019

Closed

REQUEST: New membership for JieJhih #833

6 of 6 tasks complete

@liggitt liggitt moved this from Changes requested to Completed, 1.15 in API Reviews May 20, 2019

@draveness draveness referenced this pull request Jun 3, 2019

Open

Scheduling Framework #624

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.