Change the implementation design of matchLabelKeys in PodTopologySpread to be aligned with PodAffinity by mochizuki875 · Pull Request #129874 · kubernetes/kubernetes

mochizuki875 · 2025-01-29T08:40:26Z

What type of PR is this?

/kind feature
/sig scheduling

What this PR does / why we need it:

As described in #129480, the design of matchLabelKeys is different between podAffinity and topologySpreadConstraint, which might confuse users.
This PR makes topologySpread's matchLabelKeys behave the same as podAffinity.

Which issue(s) this PR fixes:

Fixes #129480

Special notes for your reviewer:

I plan to include a description in the documentation stating updating the labels is not supported.

~~kubernetes/website#49602~~
kubernetes/website#49607

Does this PR introduce a user-facing change?

Kubernetes api-server now merges selectors built from matchLabelKeys into the labelSelector of topologySpreadConstraints, 
aligning Pod Topology Spread with the approach used by Inter-Pod Affinity.

To avoid breaking existing pods that use matchLabelKeys, the current scheduler behavior will be preserved until it is removed in v1.34. 
Therefore, do not upgrade your scheduler directly from v1.32 to v1.34. 
Instead, upgrade step-by-step (from v1.32 to v1.33, then to v1.34), 
ensuring that any pods created at v1.32 with matchLabelKeys are either removed or already scheduled by the time you reach v1.34.

If you maintain controllers that previously relied on matchLabelKeys (for instance, to simulate scheduling), 
you likely no longer need to handle matchLabelKeys directly. Instead, you can just rely on the labelSelector field going forward.

Additionally, a new feature gate `MatchLabelKeysInPodTopologySpreadSelectorMerge`, which is enabled by default, has been 
added to control this behavior.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling/3243-respect-pod-topology-spread-after-rolling-upgrades

k8s-ci-robot · 2025-01-29T08:40:35Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

mochizuki875 · 2025-01-29T08:41:38Z

/cc @sanposhiho

sanposhiho · 2025-01-29T08:43:43Z

/cc @macsko @dom4ha

For the first path

mochizuki875 · 2025-01-30T07:06:55Z

I have submitted #129900 and will restart this PR once it is merged.

sanposhiho · 2025-01-30T07:08:44Z

This PR isn't blocked by #129900, right?

mochizuki875 · 2025-01-30T07:19:13Z

This PR isn't blocked by #129900, right?

In this PR, validateMatchLabelKeysAndMismatchLabelKeys() is used to validate PodTopologySpread's matchLabelKeys, and the fixed result in #129900 is expected in the unit-test.
So without #129900, I think some test cases will fail.

kubernetes/pkg/apis/core/validation/validation_test.go

Line 22743 in 39e23ed

    
           wantFieldErrors: field.ErrorList{field.Invalid(fieldPathMatchLabelKeys.Index(0), "foo", "exists in both matchLabelKeys and labelSelector")},

sanposhiho · 2025-01-30T07:25:03Z

I mean, you can implement a unit test case with the previous behavior in this PR, and

if this PR is merged before #129900, #129900 would have a failing unit test (that is added by this PR) and you can modify the test case in #129900.
on the contrary, if #129900 is merged before this PR, your new test case here would start to fail in this PR and then you can modify here.

mochizuki875 · 2025-01-31T02:37:20Z

@sanposhiho @macsko
Thank you for your comment and I've addressed them, including this comment.
Please check again.

sanposhiho

Looking good overall

sanposhiho · 2025-01-31T03:06:41Z

/release-note-edit

Kubernetes api-server now merges selectors built from matchLabelKeys into the labelSelector of topologySpreadConstraints, 
aligning Pod Topology Spread with the approach used by Inter-Pod Affinity.

To avoid breaking existing pods that use matchLabelKeys, the current scheduler behavior will be preserved until it is removed in v1.34. 
Therefore, do not upgrade your scheduler directly from v1.32 to v1.34. 
Instead, upgrade step-by-step (from v1.32 to v1.33, then to v1.34), 
ensuring that any pods created at v1.32 with matchLabelKeys are either removed or already scheduled by the time you reach v1.34.

If you maintain controllers that previously relied on matchLabelKeys (for instance, to simulate scheduling), 
you likely no longer need to handle matchLabelKeys directly. Instead, you can just rely on the labelSelector field going forward.

mochizuki875 · 2025-01-31T05:29:00Z

Special notes for your reviewer:
I plan to include a description in the documentation stating updating the labels is not supported.

I've submitted PR.
~~kubernetes/website#49602~~
kubernetes/website#49607

…ad to be aligned with PodAffinity

deads2k · 2025-05-06T18:14:26Z

+			switch {
+			case opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && opts.OldPodViolatesMatchLabelKeysValidation:
+				allErrs = append(allErrs, ValidateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)
+			case opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && !opts.OldPodViolatesMatchLabelKeysValidation:
+				allErrs = append(allErrs, ValidateMatchLabelKeysAndMismatchLabelKeys(subFldPath, constraint.MatchLabelKeys, nil, constraint.LabelSelector)...)
+			case !opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && opts.OldPodViolatesLegacyMatchLabelKeysValidation:
+				allErrs = append(allErrs, ValidateMatchLabelKeysAndMismatchLabelKeys(subFldPath, constraint.MatchLabelKeys, nil, constraint.LabelSelector)...)
+			case !opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && !opts.OldPodViolatesLegacyMatchLabelKeysValidation:
+				allErrs = append(allErrs, ValidateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)
+			default:
+				allErrs = append(allErrs, ValidateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)
+			}


For my future sanity it will help to add some comments. Please review carefully to see if I've gotten it correct.

Suggested change

switch {

case opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && opts.OldPodViolatesMatchLabelKeysValidation:

allErrs = append(allErrs, ValidateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)

case opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && !opts.OldPodViolatesMatchLabelKeysValidation:

allErrs = append(allErrs, ValidateMatchLabelKeysAndMismatchLabelKeys(subFldPath, constraint.MatchLabelKeys, nil, constraint.LabelSelector)...)

case !opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && opts.OldPodViolatesLegacyMatchLabelKeysValidation:

allErrs = append(allErrs, ValidateMatchLabelKeysAndMismatchLabelKeys(subFldPath, constraint.MatchLabelKeys, nil, constraint.LabelSelector)...)

case !opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && !opts.OldPodViolatesLegacyMatchLabelKeysValidation:

allErrs = append(allErrs, ValidateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)

default:

allErrs = append(allErrs, ValidateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)

}

// legacyValidationFunction is ValidateMatchLabelKeysInTopologySpread

// preferredValidationFunction is ValidateMatchLabelKeysAndMismatchLabelKeys

// OldPodViolatesMatchLabelKeysValidation==true means ValidateMatchLabelKeysAndMismatchLabelKeys failed, which means preferredValidation failed

// OldPodViolatesLegacyMatchLabelKeysValidation==true means ValidateMatchLabelKeysInTopologySpread failed

switch {

case opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && opts.OldPodViolatesMatchLabelKeysValidation:

// this case means that we want to use preferred validation, but the old pod doesn't pass new validation, so it must continue using legacyValidationFunction. This is because we don't allow the fields to change.

allErrs = append(allErrs, ValidateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)

case opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && !opts.OldPodViolatesMatchLabelKeysValidation:

// this case means we want to use the preferred validation and the old pod worked with new validation, so we will continue requiring new the preferred validation to pass.

allErrs = append(allErrs, ValidateMatchLabelKeysAndMismatchLabelKeys(subFldPath, constraint.MatchLabelKeys, nil, constraint.LabelSelector)...)

case !opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && opts.OldPodViolatesLegacyMatchLabelKeysValidation:

// this is the case where we want legacy validation and the old pod does not pass legacy validation. In this case we use the preferred (new) validation instead so that updates to other fields can happen. This allows us to enable the featuregate, then disable the featuregate and still be able to update the pod.

allErrs = append(allErrs, ValidateMatchLabelKeysAndMismatchLabelKeys(subFldPath, constraint.MatchLabelKeys, nil, constraint.LabelSelector)...)

case !opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && !opts.OldPodViolatesLegacyMatchLabelKeysValidation:

// this is the case where we want legacy validation and the old pod passes legacy validation. In this case we use legacy validation

allErrs = append(allErrs, ValidateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)

default:

// if we fall through, then we use legacy validation because that's what we did prior to the new gate.

allErrs = append(allErrs, ValidateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)

}

Thank you for your suggestion!
I've addressed.

deads2k · 2025-05-07T18:45:48Z

 	},

+	MatchLabelKeysInPodTopologySpreadSelectorMerge: {
+		{Version: version.MustParse("1.34"), Default: true, PreRelease: featuregate.Beta},


Noting for the record that this is adjustment to a feature already in beta, so going directly to beta and enabled by default is acceptable. We are gating this so that if this adjustment causes negative consequences for a cluster, a cluster-admin can use the feature as it was before instead of being trapped.

deads2k · 2025-05-07T18:55:57Z

/label tide/merge-method-squash
/lgtm
/approve

k8s-ci-robot · 2025-05-07T18:56:04Z

LGTM label has been added.

Details

Git tree hash: 72c970d8209fa5e7a64f0a266162a6c570cf646d

k8s-ci-robot · 2025-05-07T18:56:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, mochizuki875, sanposhiho

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/api/OWNERS~~ [deads2k]
~~pkg/apis/OWNERS~~ [deads2k]
~~pkg/features/OWNERS~~ [deads2k]
~~pkg/registry/core/pod/OWNERS~~ [deads2k]
~~test/compatibility_lifecycle/reference/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sanposhiho

I intended to review the PR after kubernetes/enhancements#5205 is merged though. Anyway..

sanposhiho · 2025-05-08T16:01:10Z

 			allErrs = append(allErrs, err)
 		}
-		allErrs = append(allErrs, validateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)
+		// legacyValidationFunction is ValidateMatchLabelKeysInTopologySpread


Wondering, in the first place though, do we need to have a validation on the update path? because all the related fields are immutable, right? So, instead of having those complexity, can we just skip validation on the update path?

Considering the comment , I assumed all paths, including update path, must be validated.

we need to validate with either old or new validation, so branches that don't end with validation are a problem and the code currently in the PR appears to have multiple ways to end up not validating at all.

@deads2k, do you have any comments on this?

I don't get that. What's the point of having the validation for the update path when you don't allow the field to be updated in the first place.

I didn't notice the immutability check. We do normally re-validate everything anyway, but if we enforce immutability I agree we could simplify.

Thanks, so I'll review it and submit a PR.

sanposhiho · 2025-05-08T16:15:45Z

+			default:
+				// If we fall through, then we use the legacyValidationFunction because that's what we did prior to the featuregate(MatchLabelKeysInPodTopologySpreadSelectorMerge).
+				allErrs = append(allErrs, ValidateMatchLabelKeysInTopologySpread(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)


Can we reach here?

Just like the else block right after this, it is to make it clear that validation is always be performed.
However, maybe it could have been combined with the case just above(case !opts.AllowMatchLabelKeysInPodTopologySpreadSelectorMerge && !opts.OldPodViolatesLegacyMatchLabelKeysValidation)...

The following else can be reached when opts.AllowMatchLabelKeysInPodTopologySpread is false, which is fine.
But, this default shouldn't be reached (iiuc), and we should remove it or we should, at least, log errors, not just returning something. (I do prefer removing it.)

As you say, the default block is indeed unreachable.
So I'll submit a PR which remove it.

I've submit #132608.

…ad to be aligned with PodAffinity (kubernetes#129874) * Change the implementation design of matchLabelKeys in PodTopologySpread to be aligned with PodAffinity * fix1

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jan 29, 2025

k8s-ci-robot requested review from sjenning and sttts January 29, 2025 08:40

k8s-ci-robot requested a review from sanposhiho January 29, 2025 08:41

k8s-ci-robot requested review from dom4ha and macsko January 29, 2025 08:43

sanposhiho reviewed Jan 29, 2025

View reviewed changes

Comment thread pkg/apis/core/validation/validation.go Outdated

macsko reviewed Jan 29, 2025

View reviewed changes

Comment thread pkg/registry/core/pod/strategy.go Outdated

Comment thread pkg/registry/core/pod/strategy.go Outdated

Comment thread pkg/registry/core/pod/strategy_test.go Outdated

mochizuki875 mentioned this pull request Jan 30, 2025

Fix the error message of duplicate keys existing in both matchLabelKeys and labelSelector #129900

Closed

mochizuki875 force-pushed the matchlabelkeys-of-podtopologyspread branch from 92a93ae to 16ef4ec Compare January 31, 2025 02:33

sanposhiho reviewed Jan 31, 2025

View reviewed changes

Comment thread pkg/apis/core/validation/validation_test.go Outdated

mochizuki875 mentioned this pull request Jan 31, 2025

Add deprecation notices related to PodAffinity's matchLabelKeys/mismatchLabelKeys kubernetes/website#49602

Merged

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 3, 2025

mochizuki875 force-pushed the matchlabelkeys-of-podtopologyspread branch 2 times, most recently from 8dc05f8 to a33269c Compare May 3, 2025 15:51

Change the implementation design of matchLabelKeys in PodTopologySpre…

1d563f9

…ad to be aligned with PodAffinity

mochizuki875 force-pushed the matchlabelkeys-of-podtopologyspread branch from a33269c to 1d563f9 Compare May 3, 2025 15:52

hashim21223445 approved these changes May 4, 2025

View reviewed changes

deads2k reviewed May 6, 2025

View reviewed changes

Comment thread pkg/apis/core/validation/validation.go

fix1

9854158

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 7, 2025

deads2k reviewed May 7, 2025

View reviewed changes

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 7, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2025

k8s-ci-robot merged commit a309701 into kubernetes:master May 7, 2025
16 checks passed

k8s-ci-robot added this to the v1.34 milestone May 7, 2025

github-project-automation Bot moved this from PRs Waiting on Author to Done in SIG Node CI/Test Board May 7, 2025

github-project-automation Bot moved this from In Progress to Done in SIG Apps May 7, 2025

github-project-automation Bot moved this from Waiting on Author to Done in SIG Node: code and documentation PRs May 7, 2025

sanposhiho reviewed May 8, 2025

View reviewed changes

mochizuki875 mentioned this pull request May 30, 2025

Update the description of PodTopologySpread matchLabelKeys implementation change since v1.34 kubernetes/website#51119

Merged

mochizuki875 mentioned this pull request Jun 29, 2025

Remove unreachable block from validateTopologySpreadConstraints #132608

Closed

jmdeal mentioned this pull request Sep 3, 2025

fix: avoid hash collisions with duplicate match expressions kubernetes-sigs/karpenter#2479

Merged

llavaud mentioned this pull request Mar 11, 2026

Syncer fails to create pods on host when topologySpreadConstraints uses matchLabelKeys (k8s 1.34+ API server mutation conflict) loft-sh/vcluster#3668

Open

timebertt mentioned this pull request Mar 13, 2026

Error syncing pods with spec.topologySpreadConstraints[].matchLabelKeys on k8s 1.34+ loft-sh/vcluster#3684

Closed

Conversation

mochizuki875 commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Jan 29, 2025

Uh oh!

mochizuki875 commented Jan 29, 2025

Uh oh!

sanposhiho commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mochizuki875 commented Jan 30, 2025

Uh oh!

sanposhiho commented Jan 30, 2025

Uh oh!

mochizuki875 commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanposhiho commented Jan 30, 2025

Uh oh!

mochizuki875 commented Jan 31, 2025

Uh oh!

sanposhiho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sanposhiho commented Jan 31, 2025

Uh oh!

mochizuki875 commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented May 7, 2025

Uh oh!

k8s-ci-robot commented May 7, 2025

Uh oh!

k8s-ci-robot commented May 7, 2025

Uh oh!

Uh oh!

sanposhiho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

mochizuki875 commented Jan 29, 2025 •

edited

Loading

sanposhiho commented Jan 29, 2025 •

edited

Loading

mochizuki875 commented Jan 30, 2025 •

edited

Loading

mochizuki875 commented Jan 31, 2025 •

edited

Loading