Make StatefulSet restart pods with phase Succeeded #120398

aleksandra-malinowska · 2023-09-04T09:06:04Z

What type of PR is this?

/kind bug
/sig apps

What this PR does / why we need it:

After the changes in #115331, pod phase determination changed. It is now possible for StatefulSet pods to get in Succeeded pod phase. This can happen when a pod is evicted or a node is stopped and the pod container exists with 0. StatefulSet should restart such pods. It is not possible for a StatefulSet pod to ever truly complete, as validation enforces restartPolicy=Always.

Which issue(s) this PR fixes:

Part of #118310

Does this PR introduce a user-facing change?

Fixes an issue where StatefulSet might not restart a pod after eviction or node failure.

/cc @mimowo

k8s-ci-robot · 2023-09-04T09:06:14Z

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.

mimowo

I would also like to have an integration test, if feasible.

mimowo · 2023-09-04T09:31:07Z

pkg/controller/statefulset/stateful_set_control.go

+	//
+	// Also note that it is impossible for StatefulSet pods to ever truly complete,
+	// as validation allows only restartPolicy=Always.
+	if isFinished(replicas[i]) {
 		ssc.recorder.Eventf(set, v1.EventTypeWarning, "RecreatingFailedPod",


I think it is better to issue a dedicated event RecreatingSucceededPod. This is what we do for daemon set in the analogous case: https://github.com/kubernetes/kubernetes/pull/117073/files

I'm not sure about this level of granularity in event type tbh. Maybe we could have event like RecreatingPod (both here and elsewhere - happy to fix), and add the more detailed reason only in description?

Or:
event type - what happened
reason/message - why it happened

kind of like scheduler only has generic event type for failed scheduling, but gives detailed information in the reason. WDYT?

Potentially, but I'm concerned about backwards compatibility as this would mean renaming an existing event type. I would suggest decoupling this to a dedicated discussion / PR. @alculquicondor wdyt?

If I would do it all over again, I would call it: RecreatingTerminatedPod. But since it says Failed, I prefer we keep the existing one and add a new one for Succeeded, like we did for DaemonSet.

Based on discussion on sig-apps slack:

we need to be careful with renaming events of type warning (so, RecreateFailedPod stays as it is),

just recreating a pod should be emitted as a normal event, not warning.

@alculquicondor DaemonSet events are already different - I might be missing something, but I haven't found any 'recreate' events, only FailedDaemonPod and SucceededDaemonPod informing of deleting the pod. So I'm not sure if we need to name the new event RecreateSucceededPod, as there's already little consistency. I think we can use RecreateTerminatedPod here, and it'll be much clearer to the user.

What I mean about Daemonset is that there used to be only one event FailedDaemonPod and we added SucceededDaemonPod. So for StatefulSet we should do something similar and add RecreateSucceededPod. A normal event makes sense.

mimowo · 2023-09-04T09:31:24Z

pkg/controller/statefulset/stateful_set_control_test.go

+		t.Error(err)
+	}
+	if isCreated(pods[0]) {
+		t.Error("StatefulSet did not recreate failed Pod")


Suggested change

t.Error("StatefulSet did not recreate failed Pod")

t.Error("StatefulSet did not recreate succeeded Pod")

alculquicondor · 2023-09-05T14:13:37Z

pkg/controller/statefulset/stateful_set_control.go

+	// Also note that it is impossible for StatefulSet pods to ever truly complete,
+	// as validation allows only restartPolicy=Always.


I would remove this last paragraph. It is confusing, as we can see that it will actually complete in case of VM preemption or node pressure.

alculquicondor · 2023-09-07T16:30:55Z

pkg/controller/statefulset/stateful_set_control_test.go

+	if err != nil {
+		t.Error(err)
+	}
+	pods[0].Status.Phase = v1.PodSucceeded


Is this the only difference with RecreatesFailedPod? Can we deduplicate by adding a function that returns a function?

let's leave it for a follow up, given the cherry-pick deadline.

alculquicondor

/lgtm
/approve
/assign @soltysh @janetkuo

k8s-ci-robot · 2023-09-08T15:58:28Z

LGTM label has been added.

Git tree hash: a0b3260f4d222dff23d857bb72b8bff0c446eb00

aleksandra-malinowska · 2023-09-12T12:47:24Z

/retest

janetkuo

/approve

k8s-ci-robot · 2023-09-13T18:24:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, aleksandra-malinowska, janetkuo

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~~ [janetkuo]

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

alculquicondor · 2023-09-13T19:12:10Z

/retest
#119600

k8s-ci-robot requested a review from mimowo September 4, 2023 09:06

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. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 4, 2023

mimowo reviewed Sep 4, 2023

View reviewed changes

aleksandra-malinowska force-pushed the sts-restart-always branch from ffce67e to 452989e Compare September 4, 2023 10:13

mimowo mentioned this pull request Sep 4, 2023

Pod phase changed when container exits with 0 in 1.27 #118310

Closed

alculquicondor reviewed Sep 5, 2023

View reviewed changes

alculquicondor reviewed Sep 7, 2023

View reviewed changes

adilGhaffarDev mentioned this pull request Sep 8, 2023

[StatefulSet] fix statefulSet partition rollingUpdate wrong compute replicas. #119759

Closed

Make StatefulSet restart pods with phase Succeeded

d7264d0

aleksandra-malinowska force-pushed the sts-restart-always branch from 452989e to d7264d0 Compare September 8, 2023 15:47

alculquicondor reviewed Sep 8, 2023

View reviewed changes

k8s-ci-robot assigned janetkuo, soltysh and alculquicondor Sep 8, 2023

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

janetkuo approved these changes Sep 13, 2023

View reviewed changes

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

k8s-ci-robot merged commit 3eca0a5 into kubernetes:master Sep 13, 2023
14 checks passed

k8s-ci-robot added this to the v1.29 milestone Sep 13, 2023

This was referenced Sep 15, 2023

[Flake] StatefulSet Basic StatefulSet functionality #120700

Closed

[Flaky] [sig-apps] [StatefulSetBasic] should perform rolling updates and roll backs of template modifications [Conformance] #120650

Closed

mimowo mentioned this pull request Sep 18, 2023

Fixing CurrentReplicas and CurrentRevision in completeRollingUpdate #120731

Merged

aleksandra-malinowska mentioned this pull request Sep 19, 2023

Revert "Make StatefulSet restart pods with phase Succeeded" #120755

Merged

comminutus mentioned this pull request Oct 13, 2023

StatefulSet instances exit with exit code 0 when the GracefulNodeShutdown feature gate is used mariadb-operator/mariadb-operator#253

Closed

aleksandra-malinowska mentioned this pull request Oct 20, 2023

Resubmit "Make StatefulSet restart pods with phase Succeeded" #121389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make StatefulSet restart pods with phase Succeeded #120398

Make StatefulSet restart pods with phase Succeeded #120398

aleksandra-malinowska commented Sep 4, 2023 •

edited

k8s-ci-robot commented Sep 4, 2023

mimowo left a comment

mimowo Sep 4, 2023

aleksandra-malinowska Sep 4, 2023

mimowo Sep 4, 2023

alculquicondor Sep 5, 2023

aleksandra-malinowska Sep 7, 2023

alculquicondor Sep 7, 2023

mimowo Sep 4, 2023

alculquicondor Sep 5, 2023

alculquicondor Sep 7, 2023

alculquicondor Sep 8, 2023

alculquicondor left a comment

k8s-ci-robot commented Sep 8, 2023

aleksandra-malinowska commented Sep 12, 2023

janetkuo left a comment

k8s-ci-robot commented Sep 13, 2023

alculquicondor commented Sep 13, 2023

	t.Error("StatefulSet did not recreate failed Pod")
	t.Error("StatefulSet did not recreate succeeded Pod")

		// Also note that it is impossible for StatefulSet pods to ever truly complete,
		// as validation allows only restartPolicy=Always.

Make StatefulSet restart pods with phase Succeeded #120398

Make StatefulSet restart pods with phase Succeeded #120398

Conversation

aleksandra-malinowska commented Sep 4, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Sep 4, 2023

mimowo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 8, 2023

aleksandra-malinowska commented Sep 12, 2023

janetkuo left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 13, 2023

alculquicondor commented Sep 13, 2023

aleksandra-malinowska commented Sep 4, 2023 •

edited