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

Scheduler simplified MultiPoint plugin config #105611

Merged

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Oct 11, 2021

What type of PR is this?

/kind feature

What this PR does / why we need it:

This adds a MultiPoint scheduler config field. This field is to be used by administrators to simplify the necessary config for most basic scheduler setups.

Which issue(s) this PR fixes:

Fixes #102303

Special notes for your reviewer:

Does this PR introduce a user-facing change?

KubeSchedulerConfiguration provides a new field `MultiPoint` which will register a plugin for all valid extension points

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


/sig scheduling

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 11, 2021
@k8s-ci-robot
Copy link
Contributor

@damemi: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 11, 2021
@damemi
Copy link
Contributor Author

damemi commented Oct 11, 2021

/hold
this is in progress, just opening the PR now in case anyone wants an early look

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Oct 11, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 11, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2021
@damemi damemi force-pushed the simplified-multipoint-extension branch from 233ae2a to 51822ce Compare October 21, 2021 13:27
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2021
@damemi damemi force-pushed the simplified-multipoint-extension branch from 51822ce to 92ca7d7 Compare October 21, 2021 14:09
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 21, 2021
@damemi damemi force-pushed the simplified-multipoint-extension branch from 92ca7d7 to 7a3d217 Compare October 21, 2021 17:46
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 21, 2021
@damemi damemi force-pushed the simplified-multipoint-extension branch 2 times, most recently from 3e88e24 to 00b8fa9 Compare October 21, 2021 20:29
@liggitt liggitt moved this from In progress to Changes requested in API Reviews Nov 16, 2021
@liggitt
Copy link
Member

liggitt commented Nov 16, 2021

It looks like there’s still gaps (or at least concerns, based on @Huang-Wei’s comments) about some of the config merging bits… those need to be agreed on and demonstrated in tests to make sure the combinations are understood and correct

@ahg-g
Copy link
Member

ahg-g commented Nov 16, 2021

@damemi can you please close the comments that have already been addressed so we can focus on the ones that are still actually open?

@damemi
Copy link
Contributor Author

damemi commented Nov 16, 2021

Pushed the small changes as well as re-adding the logic to handle disabled plugins in the default registry. We don't have any of these right now, but the logic is there with a test case in case we add any in the future.

Also went through and resolved as many conversations as I could as @liggitt requested.

@damemi damemi force-pushed the simplified-multipoint-extension branch from a517726 to 907e900 Compare November 16, 2021 16:26
@ahg-g
Copy link
Member

ahg-g commented Nov 16, 2021

I have one final comment: #105611 (comment)

otherwise this lgtm

@damemi
Copy link
Contributor Author

damemi commented Nov 16, 2021

Got to the last comments. If we want to do a final round, should I squash this now? Don't want to preemptively squash to mess anyone up

@ahg-g
Copy link
Member

ahg-g commented Nov 16, 2021

Got to the last comments. If we want to do a final round, should I squash this now? Don't want to preemptively squash to mess anyone up

there is still this one open/not addressed: #105611 (comment)

@damemi damemi force-pushed the simplified-multipoint-extension branch from 7a20b5a to 33b8100 Compare November 16, 2021 19:16
@ahg-g
Copy link
Member

ahg-g commented Nov 16, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2021
@damemi damemi force-pushed the simplified-multipoint-extension branch from 33b8100 to 420c530 Compare November 16, 2021 19:56
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2021
@Huang-Wei
Copy link
Member

#105611 (comment) was folded. So re-comment here, I'd like to see subtest "disable and enable MultiPoint plugin by name" in pkg/scheduler/framework/runtime/framework_test.go to show the wantPlugins as:

			wantPlugins: &config.Plugins{
				QueueSort: config.PluginSet{Enabled: []config.Plugin{{Name: queueSortPlugin}}},
				PreScore: config.PluginSet{Enabled: []config.Plugin{
					{Name: scorePlugin1},
				}},
				Bind: config.PluginSet{Enabled: []config.Plugin{{Name: bindPlugin}}},
			},

@ahg-g
Copy link
Member

ahg-g commented Nov 16, 2021

#105611 (comment) was folded. So re-comment here, I'd like to see subtest "disable and enable MultiPoint plugin by name" in pkg/scheduler/framework/runtime/framework_test.go to show the wantPlugins as:

			wantPlugins: &config.Plugins{
				QueueSort: config.PluginSet{Enabled: []config.Plugin{{Name: queueSortPlugin}}},
				PreScore: config.PluginSet{Enabled: []config.Plugin{
					{Name: scorePlugin1},
				}},
				Bind: config.PluginSet{Enabled: []config.Plugin{{Name: bindPlugin}}},
			},

The current test is correct. The disabled set of multipoint plugins has no meaning at the point when trying to expand multipoint enabled plugins, this is what "disable and enable MultiPoint plugin by name" is showing.

@Huang-Wei
Copy link
Member

#105611 (comment) resolves my concern.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2021
@ahg-g
Copy link
Member

ahg-g commented Nov 16, 2021

@liggitt we converged :)

@liggitt liggitt moved this from Changes requested to In progress in API Reviews Nov 16, 2021
@liggitt
Copy link
Member

liggitt commented Nov 17, 2021

/approve

for config API bits

@liggitt liggitt moved this from In progress to API review completed, 1.23 in API Reviews Nov 17, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2021
@k8s-ci-robot k8s-ci-robot merged commit c0b5ed7 into kubernetes:master Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Status: API review completed, 1.23
Development

Successfully merging this pull request may close these issues.

Scheduler: Simplify Plugin Configuration in ComponentConfig
7 participants