-
Notifications
You must be signed in to change notification settings - Fork 15.1k
Document the new NodeInclusionPolicyInPodTopologySpread feature gate #35044
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
Document the new NodeInclusionPolicyInPodTopologySpread feature gate #35044
Conversation
👷 Deploy Preview for kubernetes-io-vnext-staging processing.
|
chrisnegus
left a comment
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.
/lgtm
|
This doesn't seem to be ready for review. |
89c8cd9 to
c20b438
Compare
c20b438 to
b2683dd
Compare
|
Document is ready to review now. cc @alculquicondor |
b2683dd to
fbd534d
Compare
|
Hey there 👋 @kerthcet, 1.25 Release Docs Shadow here! Thanks for filing this placeholder. As a reminder, final docs PRs are due August 9th. Take a look at Documenting a release for additional information for the docs requirement for the release. Thanks again! |
content/en/docs/concepts/scheduling-eviction/topology-spread-constraints.md
Outdated
Show resolved
Hide resolved
content/en/docs/concepts/scheduling-eviction/topology-spread-constraints.md
Outdated
Show resolved
Hide resolved
content/en/docs/concepts/scheduling-eviction/topology-spread-constraints.md
Outdated
Show resolved
Hide resolved
| that match this label selector are counted to determine the | ||
| number of Pods in their corresponding topology domain. | ||
| See [Label Selectors](/docs/concepts/overview/working-with-objects/labels/#label-selectors) | ||
| for more details. |
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.
We have improved our API docs a lot to include definitions of "topology domain", "eligible domain", and others. For example: https://github.com/kubernetes/api/blob/c8f06018bfc8e3c8213c53ec87b7fa4e2b9121ab/core/v1/types.go#L3396
Could you add those in this doc?
Maybe we could add the definitions before talking about the field themselves.
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.
It's also OK to link to https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#scheduling (ie, in Markdown it'd be [anchor text](/docs/reference/kubernetes-api/workload-resources/pod-v1/#scheduling)) if that's useful. No need to repeat the API reference word-for-word.
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, updated
content/en/docs/reference/command-line-tools-reference/feature-gates.md
Outdated
Show resolved
Hide resolved
| cluster. This could lead to a problem in autoscaled clusters, when a node pool (or | ||
| node group) is scaled to zero nodes, and you're expecting the cluster to scale up, | ||
| because, in this case, those topology domains won't be considered until there is | ||
| at least one node in them. |
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.
Please retain the line break here (triggered by two spaces at the end of the line).
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.
Should we? I found other paragraphs don't have this. It was auto formatted.
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 use an actual empty line?
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.
If you'd like to insist this change, at least make it a separate commit (or even separate PR). It's not part of documenting NodeInclusionPolicyInPodTopologySpread.
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.
hold for #35966
/hold
content/en/docs/reference/command-line-tools-reference/feature-gates.md
Outdated
Show resolved
Hide resolved
| - `NetworkPolicyStatus`: Enable the `status` subresource for NetworkPolicy objects. | ||
| - `NodeInclusionPolicyInPodTopologySpread`: Enable using `nodeAffinityPolicy` and `nodeTaintsPolicy` | ||
| in Pod [topology spread constraints](/docs/concepts/workloads/pods/pod-topology-spread-constraints/) | ||
| in Pod [topology spread constraints](/docs/concepts/scheduling-eviction/topology-spread-constraints/) |
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 Pod [topology spread constraints](/docs/concepts/scheduling-eviction/topology-spread-constraints/) | |
| in [Pod topology spread constraints](/docs/concepts/scheduling-eviction/topology-spread-constraints/) |
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.
updated.
alculquicondor
left a comment
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 forgot to send my review last week
content/en/docs/concepts/scheduling-eviction/topology-spread-constraints.md
Outdated
Show resolved
Hide resolved
| We consider each <key, value> as a "bucket", and try to put balanced number | ||
| of pods into each bucket. | ||
| We define a domain as a particular instance of a topology. |
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 just realized we were saying the same thing twice.
| We consider each <key, value> as a "bucket", and try to put balanced number | |
| of pods into each bucket. | |
| We define a domain as a particular instance of a topology. | |
| We call each instance of a topology (in other words, a <key, value> pair) a domain. The scheduler will try to put a balanced number of pods into each domain. |
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.
You can fix the API too, but we can leave that for 1.26.
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.
Updated as advised in another PR #35966, this one only focus on the new feature introduced.
| cluster. This could lead to a problem in autoscaled clusters, when a node pool (or | ||
| node group) is scaled to zero nodes, and you're expecting the cluster to scale up, | ||
| because, in this case, those topology domains won't be considered until there is | ||
| at least one node in them. |
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 use an actual empty line?
|
I can lgtm after rebase |
Signed-off-by: kerthcet <[email protected]>
4a66046 to
3b0b5b9
Compare
|
Rebased. |
|
/hold cancel |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tengqm 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 |
|
/lgtm |
|
LGTM label has been added. Git tree hash: ae161d09cb8f224bc723ba5f1763063b69d79572
|
Signed-off-by: kerthcet [email protected]
Ref: kubernetes/enhancements#3094
/sig scheduling