-
Notifications
You must be signed in to change notification settings - Fork 39.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
feat: forbid to set matchLabelKeys when labelSelector isn’t set in topologySpreadConstraints #116535
Conversation
@denkensk it's better to have a release note since this change may break someone's PodTopologySpread with MatchLabelKeys. |
pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go
Outdated
Show resolved
Hide resolved
/triage accepted (Probably, we want to merge this before code-freeze since the matchLabelKeys in TopologySpread is gonna be beta and get enabled by default in v1.27) |
/assign @alculquicondor |
Add in notes to reviewer: It's ok to change the behavior because the feature is graduating to beta in 1.27 |
Added |
release note is out-of-date /hold |
/lgtm cancel Please also update the API field comment to state the validation rule |
Updated. @alculquicondor |
pkg/apis/core/types.go
Outdated
@@ -5972,8 +5972,12 @@ type TopologySpreadConstraint struct { | |||
// spreading will be calculated. The keys are used to lookup values from the | |||
// incoming pod labels, those key-value labels are ANDed with labelSelector | |||
// to select the group of existing pods over which spreading will be calculated | |||
// for the incoming pod. Keys that don't exist in the incoming pod labels will | |||
// for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. | |||
// MatchLabelKeys is forbidden to set when LabelSelector isn't set. |
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.
// MatchLabelKeys is forbidden to set when LabelSelector isn't set. | |
// MatchLabelKeys cannot be set when LabelSelector isn't set. |
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.
Done
Signed-off-by: Alex Wang <wangqingcan1990@gmail.com>
/lgtm |
LGTM label has been added. Git tree hash: ec0d545c81ea1c762568452afdf4507f8cbf8168
|
This is a bug fix, so it qualifies to be merged before test freeze. |
/approve @alculquicondor has lgtm and can cancel hold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, denkensk, 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 |
FYI: The website needs updates. The example doesn't have a node-selector https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/#spread-constraint-definition /hold cancel |
Update it in kubernetes/website#40008 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
When users set
lableSelector
in topologySpreadConstraints as "null", it representslabels.Nothing()
, so the key in matchLabelKeys needs to be ignored.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
It's ok to change the behavior because the feature is graduating to beta in 1.27
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: