-
Notifications
You must be signed in to change notification settings - Fork 14.9k
Update the description of PodTopologySpread matchLabelKeys implementation change since v1.34 #51119
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
base: dev-1.34
Are you sure you want to change the base?
Conversation
👷 Deploy Preview for kubernetes-io-vnext-staging processing.
|
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
content/en/docs/concepts/scheduling-eviction/topology-spread-constraints.md
Outdated
Show resolved
Hide resolved
LGTM label has been added. Git tree hash: 3198ef123f30040fafb95be0d9458712dcd744c0
|
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
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 for docs
/lgtm |
LGTM label has been added. Git tree hash: e0ce9afeec7c3ac1f968ddee7ba4662e2a0e9c81
|
/sig scheduling |
@kubernetes/sig-scheduling-leads PTAL and advise on tech accuracy. |
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.
two nits
fromVersion: "1.34" | ||
--- | ||
Enable merging of selectors built from `matchLabelKeys` into `labelSelector` of | ||
[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.
This feature gate can be enabled when matchLabelKeys
feature is enabled with the MatchLabelKeysInPodTopologySpread
feature flag.
@@ -162,6 +169,11 @@ your cluster. Those fields are: | |||
{{< note >}} | |||
The `matchLabelKeys` field is a beta-level field and enabled by default in 1.27. You can disable it by disabling the | |||
`MatchLabelKeysInPodTopologySpread` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/). | |||
|
|||
Before v1.34, kube-scheduler just internally handled `matchLabelKeys` before the calculation of scheduling results. |
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: kube-scheduler didn't handle it before the calculation, it did handle it during the calculation.
Before v1.34, kube-scheduler just internally handled `matchLabelKeys` before the calculation of scheduling results. | |
Before v1.34, kube-scheduler just internally handled `matchLabelKeys` and calculating scheduling results. |
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.
@sanposhiho
That's absolutely true.
BTW, is "calculated" more appropriate than "calculating" in terms of grammar?
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.
yup
New changes are detected. LGTM label has been removed. |
@@ -170,7 +170,7 @@ your cluster. Those fields are: | |||
The `matchLabelKeys` field is a beta-level field and enabled by default in 1.27. You can disable it by disabling the | |||
`MatchLabelKeysInPodTopologySpread` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/). | |||
|
|||
Before v1.34, kube-scheduler just internally handled `matchLabelKeys` before the calculation of scheduling results. | |||
Before v1.34, kube-scheduler just internally handled `matchLabelKeys` and calculated scheduling results. |
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.
Why this change?
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.
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.
So .... there used to be a strict order but starting from 1.34, the ordering has been relaxed?
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 terms of the function of matchLabelKeys, which is a list of pod label keys that pod spreading is calculated based on, the strength of constraints hasn't changed between v1.33 and v1.34.
In the case of PodAffinity
's matchLabelKeys
, key-value labels corresponding to matchLabelKeys
are explicitly merged into LabelSelector
by apiserver, and scheduler handles them.
On the other hand, in the case of TopologySpreadConstraints
prior to v1.34, matchLabelKeys
are directly handled by scheduler and key-value labels corresponding to matchLabelKeys
are not merged into LabelSelector
.
From v1.34, key-value labels are merged into LabelSelector
as PodAffinity
(as described here), but the function of matchLabelKeys
does not change or become stricter.
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.
@mochizuki875 Do you mean that the user perceivable behavior has not changed? In other words, users don't need to care about this level of implementation 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.
@tengqm
Hmm, the behavior itself(scheduling calculation logic of matchLabelKeys
) has not change.
So I think users don't need to care about it so much.
It only changes how it appears to the user; whether labelSelectors
built from matchLabelKeys
are visible in podSpec
(e.g. The result of kubectl get pod xxx -o yaml
).
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 clarification, @mochizuki875. Can we then focus on the user-perceivable changes rather than forcing the users to understand the implementation details?
If users don't need to care about it, maybe we can remove all the relevant texts to avoid confusion?
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.
What's "users" you are saying? I mean, if it's end users who just uses K8s cluster that whoever provisioned, then they shouldn't have to know about this detail. But, if you mean "users" equals cluster admins, then they might want to know it. Because they might observe some unintended behavior from this change (of course, hopefully not though). Note that we introduced this feature flag for those cases.
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.
Okay. Then I'd suggest that we focus on the user-perceivable (behavior) changes.
Changing from
Before v1.34, kube-scheduler just internally handled
matchLabelKeys
before the calculation of scheduling results.
to
Before v1.34, kube-scheduler just internally handled
matchLabelKeys
and calculated scheduling results.
doesn't help clarify what the users need/want to know.
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.
@tengqm
If we focus on behavior as perceived by end users, I think we should only refer to whether labelSelectors
built from matchLabelKeys
are visible in podSpec
or not.
So how about this:
Before v1.34,
matchLabelKeys
was handled implicitly.
Since v1.34, key-value labels corresponding tomatchLabelKeys
are explicitly merged intolabelSelector
.
da6f099
to
525fb22
Compare
525fb22
to
4a09bb7
Compare
2167fb7
to
8ad8c10
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/remove-language bn fr id ja pt zh |
Description
This PR is a follow-up to #49607 and #50122, resubmitted for the v1.34 release because the code change missed v1.33.
related: