-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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 docs on scheduler MultiPoint config #30442
Add docs on scheduler MultiPoint config #30442
Conversation
👷 Deploy Preview for kubernetes-io-vnext-staging processing. 🔨 Explore the source changes: f7f336c 🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/61a65ee8ae674f0007fa2756 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @alculquicondor @ahg-g @Huang-Wei
ptal, and let me know any other edge cases I should cover
/assign @nate-double-u |
``` | ||
|
||
Specific extension points can be excluded from `MultiPoint` expansion using | ||
the `disabled` fields. This works with disabling default plugins, out-of-tree plugins, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should it be field instead?
the `disabled` fields. This works with disabling default plugins, out-of-tree plugins, | |
the `disabled` field. This works with disabling default plugins, out-of-tree plugins, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should avoid talking about out-of-tree plugins in official k8s documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't disagree, but since this is different than how previous extension points treated out-of-tree plugins and this does apply to them, should those aspects be noted somewhere? Maybe part of the scheduling framework docs?
@@ -255,7 +255,183 @@ the same configuration parameters (if applicable). This is because the scheduler | |||
only has one pending pods queue. | |||
{{< /note >}} | |||
|
|||
## {{% heading "whatsnext" %}} | |||
### MultiPoint config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd write:
### MultiPoint config | |
## Plugins that apply to all extension points {#multipoint} |
The fragment identifier will then be multipoint
.
Also, under “Extension points” (line 47), I'd mention and hyperlink to this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made this multiple extension points
rather than all
, think that's clearer
/sig scheduling |
If this is a new field that doesn't work in older Kubernetes, we should also document that detail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the write-up! LGTM except for some nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments inline:
``` | ||
|
||
Specific extension points can be excluded from `MultiPoint` expansion using | ||
the `disabled` fields. This works with disabling default plugins, out-of-tree plugins, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should avoid talking about out-of-tree plugins in official k8s documentation.
profiles: | ||
- schedulerName: multipoint-scheduler | ||
plugins: | ||
score: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multiPoint? Order only really matters for Filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was trying to show a couple things here, that we can reorder and re-weight the plugins. Order technically only matters for Filter, true, but I think this still gets both of those points across
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
* Disables `DefaultPlugin1`, but only for `Filter` | ||
* Reorders `DefaultPlugin2` to run first in `Score` (even before the custom plugins) | ||
|
||
Without `MultiPoint` configuration, the above snippet would equate to this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe explicitly disable *
in multiPoint
to make it clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this section I'm actually showing what the equivalent in a pre-v1beta3 config would be. I tried updating the wording here to make that clearer. I could still add the disable: *
to multiPoint
if you think that would help too
d99496d
to
4c1c01e
Compare
|
||
In older versions of the config, without `multiPoint`, the above snippet would equate to this: | ||
```yaml | ||
apiVersion: kubescheduler.config.k8s.io/v1beta3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this either should be v1beta2 or just add the disable *
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
went with v1beta2
and pointed that out. multiPoint.disabled: *
would mean all of the other default plugins need to be explicitly enabled which isn't exactly the comparison I was trying to draw here
profiles: | ||
- schedulerName: multipoint-scheduler | ||
plugins: | ||
score: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
@@ -341,7 +341,7 @@ extension point in the future, the `multiPoint` config will automatically enable | |||
for the new extension. | |||
|
|||
Specific extension points can be excluded from `MultiPoint` expansion using | |||
the `disabled` field. This works with disabling default plugins, out-of-tree plugins, | |||
the `disabled` field. This works with disabling default plugins, non-default plugins, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's say:
using the disabled
field for the extension point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to add any extra notes in #30404?
IIUC, people using v1beta2 could just swap the version without needing extra changes, right?
@@ -78,6 +78,8 @@ extension points: | |||
least one bind plugin is required. | |||
1. `postBind`: This is an informational extension point that is called after | |||
a Pod has been bound. | |||
1. `multiPoint`: This is a config-only field that allows plugins to be enabled for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enabled or disabled
618450e
to
d1b0a46
Compare
Right, an existing v1beta2 config will work with v1beta3 without any changes |
/lgtm /sig scheduling |
LGTM label has been added. Git tree hash: 99fd7c5079bc28eedd04ff2c0b2e6d721f4969fb
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good from a docs standpoint. I have a couple suggestions inline to simplify or clarify a couple sentences. I don't think these are strictly necessary for merge though.
/lgtm
/cc @jlbutler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I didn't scroll down far enough on the deploy preview.
The "What's next" heading needs to be fixed:
d1b0a46
to
e880cd2
Compare
Updated and squashed, will need another lgtm when anyone has the chance |
e880cd2
to
f7f336c
Compare
/lgtm |
LGTM label has been added. Git tree hash: b2573535f5bab19c8998ebb0b5eb577eacf1b067
|
looks like final editorial and tech comments have been addressed, thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlbutler 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 |
This adds documentation for the scheduler's new
MultiPoint
config field, along with examples and comparisons to the old way of configuring the scheduler.Enhancement issue: kubernetes/enhancements#2891
Work PR: kubernetes/kubernetes#105611