Respect PodTopologySpread after rolling upgrades #111441

denkensk · 2022-07-26T15:34:01Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

PodTopologySpread is widely used in production environments, especially in service type workloads which employ Deployments. However, currently it has a limitation that manifests during rolling updates which causes the deployment to end up out of balance (98215, 105661,k8s-pod-topology spread is not respected after rollout).

The root cause is that PodTopologySpread constraints allow defining a key-value label selector, which applies to all pods in a Deployment irrespective of their owning ReplicaSet. As a result, when a new revision is rolled out, spreading will apply across pods from both the old and new ReplicaSets, and so by the time the new ReplicaSet is completely rolled out and the old one is rolled back, the actual spreading we are left with may not match expectations because the deleted pods from the older ReplicaSet will cause skewed distribution for the remaining pods.

Which issue(s) this PR fixes:

Fixes #98215
Fixes #105661

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Respect PodTopologySpread after rolling upgrades

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

[KEP]: Respect PodTopologySpread after rolling upgrades enhancements#3243)

denkensk · 2022-07-26T15:34:37Z

/sig scheduling

denkensk · 2022-07-26T17:29:20Z

/assign @ahg-g

pkg/api/pod/util_test.go

pkg/api/pod/util.go

pkg/api/pod/util_test.go

pkg/apis/core/validation/validation_test.go

chendave · 2022-07-29T10:09:42Z

pkg/apis/core/validation/validation_test.go

+					MatchLabelKeys:    []string{"/simple"},
+				},
+			},
+			wantFieldErrors: []*field.Error{field.Invalid(fieldPathMatchLabelKeys, "/simple", "prefix part must be non-empty")},


prefix part must be non-empty this error msg is hard to understand at the first glance until I checked the source which split the string by '/'.

you may want to add another test string, such as an empty string, I am fine with this as is though.

pkg/features/kube_features.go

ahg-g · 2022-07-29T15:22:27Z

looks good to me

@liggitt this is ready.

liggitt

API bits look good, just a few doc/lint/message nits

I didn't review the pkg/scheduler and test changes, will defer to scheduling reviewers/approvers for those changes

api/api-rules/violation_exceptions.list

pkg/apis/core/types.go

liggitt · 2022-07-29T15:45:00Z

pkg/apis/core/validation/validation.go

@@ -6490,6 +6490,7 @@ func validateTopologySpreadConstraints(constraints []core.TopologySpreadConstrai
 		if err := validateNodeInclusionPolicy(subFldPath.Child("nodeTaintsPolicy"), constraint.NodeTaintsPolicy); err != nil {
 			allErrs = append(allErrs, err)
 		}
+		allErrs = append(allErrs, validateMatchLabelKeys(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...)
 	}


pre-existing, but I noticed validateTopologySpreadConstraints doesn't seem to validate constraint.LabelSelector at all... @ahg-g, can you open a separate issue to track that (need to make sure the scheduler is robust against completely invalid selectors, and consider how to fix this validation in the least disruptive way possible)

Some update on this comments, the validation is actually done when the labels.Selector is built. e.g.

kubernetes/pkg/scheduler/framework/plugins/podtopologyspread/common.go

Line 94 in 891cbed

selector, err := metav1.LabelSelectorAsSelector(c.LabelSelector)

And it is validated here,

kubernetes/staging/src/k8s.io/apimachinery/pkg/labels/selector.go

Lines 191 to 194 in 891cbed

for i := range vals {

if err := validateLabelValue(key, vals[i], valuePath.Index(i)); err != nil {

allErrs = append(allErrs, err)

}

I think the original comment is still valid and the the validation mentioned above would mitigate the problem as the invalid label value won't be accepted at all.

that happens at time of use, not time of API write, which is not ideal (since the user has no idea the object they created is not working like they want it to)

pr sent: #112121

pkg/apis/core/validation/validation.go

ahg-g · 2022-07-29T16:48:17Z

/milestone v1.25

liggitt · 2022-07-30T03:58:34Z

/approve
API change lgtm

/hold looks like commits need squashing
will let @ahg-g lgtm once that is done

k8s-ci-robot · 2022-07-30T03:58:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~api/OWNERS~~ [liggitt]
~~pkg/api/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [liggitt]
~~pkg/generated/openapi/OWNERS~~ [liggitt]
~~pkg/scheduler/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]
~~test/integration/scheduler/OWNERS~~ [liggitt]

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

Signed-off-by: Alex Wang <wangqingcan1990@gmail.com>

ahg-g · 2022-07-30T07:17:19Z

/lgtm
/hold cancel

sftim · 2022-08-01T18:08:16Z

@denkensk, could we also link to the relevant docs?

See #111194 for an example of a detailed release note that includes a link to docs.

fedebongio · 2022-08-02T20:05:12Z

/triage accepted

k8s-ci-robot requested review from caesarxuchao and chendave July 26, 2022 15:35

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

denkensk mentioned this pull request Jul 26, 2022

Respect PodTopologySpread after rolling upgrades kubernetes/enhancements#3243

Open

8 tasks

denkensk force-pushed the respect-topology branch from 6b1008e to 4283ef2 Compare July 26, 2022 17:25

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 26, 2022

denkensk force-pushed the respect-topology branch from 4283ef2 to 87a2954 Compare July 26, 2022 17:25

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 26, 2022

denkensk force-pushed the respect-topology branch from 87a2954 to bdfffb7 Compare July 26, 2022 17:28

k8s-ci-robot assigned ahg-g Jul 26, 2022

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 28, 2022

liggitt moved this from Assigned to In progress in API Reviews Jul 28, 2022

liggitt moved this from In progress to Changes requested in API Reviews Jul 28, 2022

chendave reviewed Jul 29, 2022

View reviewed changes

liggitt moved this from Changes requested to Assigned in API Reviews Jul 29, 2022

liggitt moved this from Assigned to In progress in API Reviews Jul 29, 2022

liggitt reviewed Jul 29, 2022

View reviewed changes

liggitt moved this from In progress to Changes requested in API Reviews Jul 29, 2022

k8s-ci-robot added this to the v1.25 milestone Jul 29, 2022

liggitt moved this from Changes requested to API review completed, 1.25 in API Reviews Jul 30, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2022

denkensk added 3 commits July 30, 2022 13:21

api defination for MatchLabelKeys in TopologySpreadConstraint

e6c2bf8

Signed-off-by: Alex Wang <wangqingcan1990@gmail.com>

implementation for MatchLabelKeys in TopologySpreadConstraint

f4bc904

Signed-off-by: Alex Wang <wangqingcan1990@gmail.com>

code generated by script for MatchLabelKeys in TopologySpreadConstraint

86a2a85

Signed-off-by: Alex Wang <wangqingcan1990@gmail.com>

denkensk force-pushed the respect-topology branch from 6cdf9bd to 86a2a85 Compare July 30, 2022 05:25

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 30, 2022

k8s-ci-robot merged commit 3902a53 into kubernetes:master Jul 30, 2022

denkensk deleted the respect-topology branch July 31, 2022 08:06

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 2, 2022

a7i mentioned this pull request Aug 18, 2022

Use pod-template-hash label in addition to topology spread constraint matchLabels kubernetes-sigs/descheduler#917

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect PodTopologySpread after rolling upgrades #111441

Respect PodTopologySpread after rolling upgrades #111441

denkensk commented Jul 26, 2022 •

edited

denkensk commented Jul 26, 2022

denkensk commented Jul 26, 2022

chendave Jul 29, 2022

ahg-g commented Jul 29, 2022

liggitt left a comment •

edited

liggitt Jul 29, 2022

chendave Aug 29, 2022

liggitt Aug 29, 2022

chendave Aug 30, 2022

ahg-g commented Jul 29, 2022

liggitt commented Jul 30, 2022

k8s-ci-robot commented Jul 30, 2022

ahg-g commented Jul 30, 2022

sftim commented Aug 1, 2022

fedebongio commented Aug 2, 2022

	for i := range vals {
	if err := validateLabelValue(key, vals[i], valuePath.Index(i)); err != nil {
	allErrs = append(allErrs, err)
	}

Respect PodTopologySpread after rolling upgrades #111441

Respect PodTopologySpread after rolling upgrades #111441

Conversation

denkensk commented Jul 26, 2022 • edited

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.:

denkensk commented Jul 26, 2022

denkensk commented Jul 26, 2022

chendave Jul 29, 2022

Choose a reason for hiding this comment

ahg-g commented Jul 29, 2022

liggitt left a comment • edited

Choose a reason for hiding this comment

liggitt Jul 29, 2022

Choose a reason for hiding this comment

chendave Aug 29, 2022

Choose a reason for hiding this comment

liggitt Aug 29, 2022

Choose a reason for hiding this comment

chendave Aug 30, 2022

Choose a reason for hiding this comment

ahg-g commented Jul 29, 2022

liggitt commented Jul 30, 2022

k8s-ci-robot commented Jul 30, 2022

ahg-g commented Jul 30, 2022

sftim commented Aug 1, 2022

fedebongio commented Aug 2, 2022

denkensk commented Jul 26, 2022 •

edited

liggitt left a comment •

edited