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
Scheduler simplified MultiPoint plugin config #105611
Conversation
@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 The 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. |
/hold |
233ae2a
to
51822ce
Compare
51822ce
to
92ca7d7
Compare
92ca7d7
to
7a3d217
Compare
3e88e24
to
00b8fa9
Compare
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 |
@damemi can you please close the comments that have already been addressed so we can focus on the ones that are still actually open? |
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. |
a517726
to
907e900
Compare
I have one final comment: #105611 (comment) otherwise this lgtm |
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) |
7a20b5a
to
33b8100
Compare
/lgtm |
33b8100
to
420c530
Compare
#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. |
#105611 (comment) resolves my concern. /lgtm |
@liggitt we converged :) |
/approve for config API bits |
[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 |
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig scheduling