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.

Sign up for GitHub

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

WIP: Add FeatureGate toggling implementation change since v1.33 #50122

Closed
wants to merge 1 commit into from
Closed

WIP: Add FeatureGate toggling implementation change since v1.33 #50122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions content/en/docs/concepts/scheduling-eviction/topology-spread-constraints.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,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.33, kube-scheduler just internally handled `matchLabelKeys` before the calculation of scheduling results.
Since v1.33, merging selectors built from `matchLabelKeys` into `labelSelector` is enabled by default.
You can disable it and revert to the previous behavior by disabling the `MatchLabelKeysInPodTopologySpreadSelectorMerge`
[feature gate](/docs/reference/command-line-tools-reference/feature-gates/) of kube-apiserver.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clarification means we only need to toggle the gate on the API server, right?
That is nice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tengqm

This clarification means we only need to toggle the gate on the API server, right?

Yes, that's right.

{{< /note >}}

- **nodeAffinityPolicy** indicates how we will treat Pod's nodeAffinity/nodeSelector
Expand Down
14 changes: 14 additions & 0 deletions ...tools-reference/feature-gates/MatchLabelKeysInPodTopologySpreadSelectorMerge.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
title: MatchLabelKeysInPodTopologySpreadSelectorMerge
content_type: feature_gate
_build:
list: never
render: false

stages:
- stage: beta
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this kind of shit as gate features, can we start from "Alpha"?
We as developers should avoid changing user land whenever possible.
If we have to do it, to fix a serious bug, we just fix it.
Why are we introducing a feature gate for such a change?

Please think from users' perspective. I, as a user, have no idea why this change is supposed to be disabled, or enabled. Even I do understand the corner case and its impact on my workloads, I have no idea which components this feature affects. Then I have to restart all my control-plane pods and all kubelets on all nodes. Is this a scenario we the development community is happy with?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tengqm
Thank you for your comment.
This feature is an implementation design change of KEP-3243.
We consider the risk to be low and the FeatureGate of KEP-3243 MatchLabelKeysInPodTopologySpread has been true since v1.27.
Therefore in our discussion, we decided to add the new FeatureGate MatchLabelKeysInPodTopologySpreadSelectorMerge to control whether this change enable or disable, and start at the same level as MatchLabelKeysInPodTopologySpread(beta).

Copy link
Contributor

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.
I may sound too harsh for a change that has been thoroughly discussed, but please don't take it personal. I've been watching feature gate changes for many years. One thing that concerns me is that some developers are abusing feature gates for things that they are not sure about. They are doing this without thinking from user's perspective. For example, when we are introducing a new behavior change to a particular component, say kube-scheduler, we may add a configuration option to the scheduler config file, or a command line flag. Feature gates are heavy. Given that we cannot tell which gate is relevant to which component, the safest way to make a change is to restart all control-plane components, and all node agents including kubelet and kube-proxy. For a large scale cluster, this process is not trivial. We develop features or propose bug fixes to make people's lives easier, not harder. We provide clear documentation for them to know why there is such a gate and what are the pros and cons for enabling/disabling it. Kubernetes are used in many production environments, we are not supposed to treat changes like this as developer-facing experiments. By saying this, I'm talking about a lot of shit gates we encountered lately.

Copy link
Member Author

@mochizuki875 mochizuki875 Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tengqm
OK, I've added additional description about the FeatureGate in latest commit.
Do you think there is anything else we should include?

In addition, the code change(#129874) may not meet the v1.33 code freeze.
Therefore I'll revert docs change(#49607) and close this PR.
After that, I'll submit new PR merging them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worry. Let's put this on hold. We'll only lift the hold when the upstream change is merged.
/hold

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tengqm
Thank you.
Since the code change(#129874) has not meet the v1.33 code freeze, I've submitted revert PR(#50164) which remove the description of this feature from dev-1.33 branch.

For the next step, which option do you think is better? Alternatively, do you have any other suggestions?

  1. Keep this featuregate PR and resubmit new PR including the description of this feature(#49607).
  2. Merge the description of this feature(#49607) into this PR and retitle
  3. Close this featuregate PR and resubmit new PR merging the description of this feature(#49607)

Anyway, I think I need to change the target branch dev-v1.33 to dev-1.34(when created).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I'd suggest we close this one and resubmit a new one when the upstream code change is ready.

defaultValue: true
fromVersion: "1.33"
---
Enable merging of selectors built from `matchLabelKeys` into `labelSelector` of
[Pod topology spread constraints](/docs/concepts/scheduling-eviction/topology-spread-constraints/).