Add StatefulSet pod index as pod label #119232

danielvegamyhre · 2023-07-11T18:08:58Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implementing StatefulSet changes for KEP-4017.

Reviewer note: please only look at commit 42302f3 and beyond, since this branch is based off the branch where I implemented the corresponding changes for indexed jobs.

Which issue(s) this PR fixes:

Part of kubernetes/enhancements#4017

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Yes, the pod index of a StatefulSet pod will now be added as a pod a label, statefulset.kubernetes.io/pod-index.

StatefulSet pods now have the pod index set as a pod label `statefulset.kubernetes.io/pod-index`.

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

[KEP](https://github.com/kubernetes/enhancements/pull/4019)

k8s-ci-robot · 2023-07-11T18:09:08Z

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.

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/test-infra repository.

danielvegamyhre · 2023-07-11T18:20:33Z

Reviewer note: please only look at commit 42302f3 and beyond, since this branch is based off the branch where I implemented the corresponding changes for indexed jobs.

k8s-triage-robot · 2023-07-11T19:25:49Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

ahg-g · 2023-07-11T22:05:17Z

/assign @soltysh

ahg-g · 2023-07-11T22:05:38Z

/label api-review

ahg-g · 2023-07-13T15:43:06Z

staging/src/k8s.io/api/apps/v1/types.go

@@ -29,6 +29,7 @@ const (
 	DeprecatedRollbackTo           = "deprecated.deployment.rollback.to"
 	DeprecatedTemplateGeneration   = "deprecated.daemonset.template.generation"
 	StatefulSetPodNameLabel        = "statefulset.kubernetes.io/pod-name"
+	StatefulSetPodIndexLabel       = "statefulset.kubernetes.io/pod-index"


The agreement is to use "statefulset.kubernetes.io/pod-index", can we please change this?

@soltysh what do you suggest we do with the existing pod name label? add a corresponding one using the key apps.kubernetes.io/pod-name and start the deprecation process (I bet it will be painful to deprecate since it is widely used)?

Updated to use apps prefix instead of statefulset.

danielvegamyhre · 2023-07-16T04:11:37Z

pkg/controller/statefulset/stateful_set_control_test.go

@@ -227,6 +228,28 @@ func CreatesPods(t *testing.T, set *apps.StatefulSet, invariants invariantFunc)
 	if set.Status.UpdatedReplicas != 3 {
 		t.Error("Failed to set UpdatedReplicas correctly")
 	}
+	// Check all pods have correct pod index label.
+	if feature.DefaultFeatureGate.Enabled(features.PodIndexLabel) {


Reviewer note: I thought about adding a podIndexLabelEnabled bool as a test case parameter, but that would then require changing the function signature of all 11 test functions (e.g. CreatePods, ScalesUp, ScalesDown, etc.) to add this parameter. This seemed like I would be shoe-horning it in and making the test code uglier, so I opted to just check if the feature gate itself is enabled in the test code, even though I don't see this pattern used much elsewhere in the codebase.

Since the code freeze is in 2 days I also created a PR with an alternative testing implementation #119355 which does refactor the unit test code to allow for toggling the feature gate on/off based on the test case and includes test cases with the feature gate enabled/disabled, so that we can move forward quickly with whichever approach is prefered.

Let me know if there is ok or if there is a different/preferred pattern to do this.

cc @soltysh on above comment

ahg-g

/lgtm

pkg/controller/statefulset/stateful_set_control_test.go

k8s-ci-robot · 2023-07-16T18:01:48Z

LGTM label has been added.

Git tree hash: 8a0005fe54efda88bccc50f6ffc25f707bd31769

danielvegamyhre · 2023-07-17T04:27:55Z

/retest

staging/src/k8s.io/api/apps/v1/types.go

liggitt · 2023-07-17T16:44:37Z

/lgtm
/approve

ahg-g · 2023-07-17T16:44:48Z

can you squash pls

k8s-ci-robot · 2023-07-17T16:44:49Z

LGTM label has been added.

Git tree hash: e36cc60fe740580b588f4dbd1785fc2a1da29017

k8s-ci-robot · 2023-07-17T16:45:08Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~pkg/controller/statefulset/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]

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

ahg-g · 2023-07-17T17:00:58Z

can you squash pls

not necessary, merge-method-squash label was applied

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 11, 2023

k8s-ci-robot requested review from dchen1107 and deads2k July 11, 2023 18:09

danielvegamyhre force-pushed the sts branch from 42302f3 to 1d3c12d Compare July 11, 2023 20:52

npolshakova mentioned this pull request Jul 11, 2023

Add Pod Index Label for StatefulSets and Indexed Jobs kubernetes/enhancements#4017

Open

11 tasks

k8s-ci-robot assigned soltysh Jul 11, 2023

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jul 11, 2023

ahg-g reviewed Jul 13, 2023

View reviewed changes

add statefulset pod index as pod label

40058b0

danielvegamyhre force-pushed the sts branch from 1d3c12d to 40058b0 Compare July 16, 2023 04:02

danielvegamyhre commented Jul 16, 2023

View reviewed changes

change statefulset pod index label name

f852967

danielvegamyhre force-pushed the sts branch from 9d95900 to f852967 Compare July 16, 2023 15:47

danielvegamyhre mentioned this pull request Jul 16, 2023

Alternative kep-4017 statefulset implementation #119355

Closed

ahg-g reviewed Jul 16, 2023

View reviewed changes

pkg/controller/statefulset/stateful_set_control_test.go Show resolved Hide resolved

k8s-ci-robot assigned ahg-g Jul 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2023

check 3 pods

62c315e

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2023

k8s-ci-robot requested review from ahg-g and soltysh July 16, 2023 22:59

liggitt reviewed Jul 17, 2023

View reviewed changes

staging/src/k8s.io/api/apps/v1/types.go Outdated Show resolved Hide resolved

change label variable name

e2df80d

liggitt added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 17, 2023

liggitt self-assigned this Jul 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2023

liggitt added this to API review completed, 1.28 in API Reviews Jul 17, 2023

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

liggitt added this to the v1.28 milestone Jul 17, 2023

k8s-ci-robot merged commit 7698fe7 into kubernetes:master Jul 17, 2023
12 checks passed

tcdowney mentioned this pull request Jul 27, 2023

Set CF_INSTANCE_INDEX based on statefulset.kubernetes.io/pod-index label cloudfoundry/korifi#2732

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add StatefulSet pod index as pod label #119232

Add StatefulSet pod index as pod label #119232

danielvegamyhre commented Jul 11, 2023 •

edited

k8s-ci-robot commented Jul 11, 2023

danielvegamyhre commented Jul 11, 2023

k8s-triage-robot commented Jul 11, 2023

ahg-g commented Jul 11, 2023

ahg-g commented Jul 11, 2023

ahg-g Jul 13, 2023

danielvegamyhre Jul 16, 2023

danielvegamyhre Jul 16, 2023 •

edited

danielvegamyhre Jul 16, 2023

ahg-g left a comment

k8s-ci-robot commented Jul 16, 2023

danielvegamyhre commented Jul 17, 2023

liggitt commented Jul 17, 2023

ahg-g commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

ahg-g commented Jul 17, 2023

Add StatefulSet pod index as pod label #119232

Add StatefulSet pod index as pod label #119232

Conversation

danielvegamyhre commented Jul 11, 2023 • 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.:

k8s-ci-robot commented Jul 11, 2023

danielvegamyhre commented Jul 11, 2023

k8s-triage-robot commented Jul 11, 2023

ahg-g commented Jul 11, 2023

ahg-g commented Jul 11, 2023

ahg-g Jul 13, 2023

Choose a reason for hiding this comment

danielvegamyhre Jul 16, 2023

Choose a reason for hiding this comment

danielvegamyhre Jul 16, 2023 • edited

Choose a reason for hiding this comment

danielvegamyhre Jul 16, 2023

Choose a reason for hiding this comment

ahg-g left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 16, 2023

danielvegamyhre commented Jul 17, 2023

liggitt commented Jul 17, 2023

ahg-g commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

ahg-g commented Jul 17, 2023

danielvegamyhre commented Jul 11, 2023 •

edited

danielvegamyhre Jul 16, 2023 •

edited