fix stateful set pod recreation and event spam #123809

atiratree · 2024-03-07T21:50:25Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

do not emit events when pod reaches terminal phase
do not try to recreate pod until the old pod has been removed from
etcd storage

Which issue(s) this PR fixes:

Fixes #122709

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Emition of RecreatingFailedPod and RecreatingTerminatedPod events has been removed from stateful set lifecycle.

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

atiratree · 2024-03-07T21:57:43Z

pkg/controller/statefulset/stateful_set_control.go

 	// Note that pods with phase Succeeded will also trigger this event. This is
 	// because final pod phase of evicted or otherwise forcibly stopped pods
 	// (e.g. terminated on node reboot) is determined by the exit code of the
 	// container, not by the reason for pod termination. We should restart the pod
 	// regardless of the exit code.
 	if isFailed(replicas[i]) || isSucceeded(replicas[i]) {
-		if isFailed(replicas[i]) {
-			ssc.recorder.Eventf(set, v1.EventTypeWarning, "RecreatingFailedPod",


I do not see a use case behind this event. It is here only for tracking of the terminal phases.

Without this event we will still get the following events:

default 5s Normal Killing pod/nginx-roll-2 Stopping container nginx default 4s Normal SuccessfulCreate statefulset/nginx-roll create Pod nginx-roll-2 in StatefulSet nginx-roll successful

I suppose we could move the event generation into the updatePod handler if we want to be backwards compatible. Thoughts?

Move or change here to logging seems acceptable.

atiratree · 2024-03-07T21:58:14Z

pkg/controller/statefulset/stateful_set.go

@@ -235,6 +235,9 @@ func (ssc *StatefulSetController) updatePod(logger klog.Logger, old, cur interfa
 			return
 		}
 		logger.V(4).Info("Pod objectMeta updated", "pod", klog.KObj(curPod), "oldObjectMeta", oldPod.ObjectMeta, "newObjectMeta", curPod.ObjectMeta)
+		if oldPod.Status.Phase != curPod.Status.Phase && podutil.IsPodPhaseTerminal(curPod.Status.Phase) {
+			logger.V(4).Info("StatefulSet Pod reached terminal phase", "pod", klog.KObj(curPod), "statefulSet", klog.KObj(set), "podPhase", curPod.Status.Phase)


I am adding this logging in case somebody would like to debug these.

Nit, why not just add that pod phase to the above line?

yes, that might be even better for debugging

atiratree · 2024-03-07T22:00:39Z

pkg/controller/statefulset/stateful_set_control.go

-			updateRevision.Name,
-			replicaOrd)
+		// New pod should be generated on the next sync after the current pod is removed from etcd.
+		return true, nil


We should try not to create the pods until the old one has been removed from etcd. Otherwise the kcm logs are getting spammed with:

stateful_set.go:430] error syncing StatefulSet default/nginx-roll, requeuing: object is being deleted: pods "nginx-roll-2" already exists, the server was not able to generate a unique name for the object

atiratree · 2024-03-07T23:14:46Z

/triage accepted
/priority important-soon
/assign @soltysh

atiratree · 2024-03-11T18:59:23Z

test/e2e/framework/statefulset/rest.go

+	// - try at least 3 times if there are 0 replicas
+	// - assume that each replica can potentially generate 3 conflicts when statefulset status is updated
+	//   e.g. sudden pod changes between (Running, Ready, Available, Terminating, Deleted)


E2E's were failing without this fix; https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/123809/pull-kubernetes-conformance-kind-ga-only-parallel/1766049762357809152

Kubernetes e2e suite: [It] [sig-apps] StatefulSet Basic StatefulSet functionality [StatefulSetBasic] Scaling should happen in predictable order and halt if any stateful pod is unhealthy [Slow] [Conformance] expand_less 1m56s { failed [FAILED] too many retries draining statefulset "ss" In [It] at: k8s.io/kubernetes/test/e2e/framework/statefulset/rest.go:265 @ 03/08/24 10:57:02.069 }

I am not sure if this is suppose to test conflicts? If not, this could be changed to a timeout based update.

To me this looks like helper written before the backoff was introduced. How about re-writing this with wiat.ExponentialBackoff with initial duration 2s, factor 1.5 and 2 steps. wdyt?

+1, RetryOnConflict might be even better for this. Also I based the backoff on DefaultRetry and increased the number of steps a bit

soltysh

Some minor nits, but overall this looks reasonable.

soltysh · 2024-03-15T12:40:32Z

pkg/controller/statefulset/stateful_set.go

@@ -235,6 +235,9 @@ func (ssc *StatefulSetController) updatePod(logger klog.Logger, old, cur interfa
 			return
 		}
 		logger.V(4).Info("Pod objectMeta updated", "pod", klog.KObj(curPod), "oldObjectMeta", oldPod.ObjectMeta, "newObjectMeta", curPod.ObjectMeta)
+		if oldPod.Status.Phase != curPod.Status.Phase && podutil.IsPodPhaseTerminal(curPod.Status.Phase) {
+			logger.V(4).Info("StatefulSet Pod reached terminal phase", "pod", klog.KObj(curPod), "statefulSet", klog.KObj(set), "podPhase", curPod.Status.Phase)


Nit, why not just add that pod phase to the above line?

soltysh · 2024-03-15T12:41:24Z

pkg/controller/statefulset/stateful_set_control.go

 	// Note that pods with phase Succeeded will also trigger this event. This is
 	// because final pod phase of evicted or otherwise forcibly stopped pods
 	// (e.g. terminated on node reboot) is determined by the exit code of the
 	// container, not by the reason for pod termination. We should restart the pod
 	// regardless of the exit code.
 	if isFailed(replicas[i]) || isSucceeded(replicas[i]) {
-		if isFailed(replicas[i]) {
-			ssc.recorder.Eventf(set, v1.EventTypeWarning, "RecreatingFailedPod",


Move or change here to logging seems acceptable.

soltysh · 2024-03-15T12:50:40Z

test/e2e/framework/statefulset/rest.go

+	// - try at least 3 times if there are 0 replicas
+	// - assume that each replica can potentially generate 3 conflicts when statefulset status is updated
+	//   e.g. sudden pod changes between (Running, Ready, Available, Terminating, Deleted)


To me this looks like helper written before the backoff was introduced. How about re-writing this with wiat.ExponentialBackoff with initial duration 2s, factor 1.5 and 2 steps. wdyt?

- do not emit events when pod reaches terminal phase - do not try to recreate pod until the old pod has been removed from etcd storage

statefulset controller does less requests per sync now and thus can reconcile status faster, thus resulting in a higher chance for conflicts

soltysh

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

k8s-ci-robot · 2024-03-15T16:11:41Z

LGTM label has been added.

Git tree hash: 7aa2668daacc5ea3b06841104fe670296e1a2ed9

k8s-ci-robot · 2024-03-15T16:11:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, soltysh

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~~ [soltysh]
~~test/e2e/framework/OWNERS~~ [soltysh]

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

atiratree · 2024-03-15T17:37:26Z

unrelated
/test pull-kubernetes-e2e-gce

k8s-ci-robot requested review from krmayankk and mortent March 7, 2024 21:50

k8s-ci-robot added 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 Mar 7, 2024

atiratree commented Mar 7, 2024

View reviewed changes

atiratree force-pushed the fix-sts-events branch 2 times, most recently from 2a42314 to fe811c5 Compare March 7, 2024 23:03

k8s-ci-robot assigned soltysh Mar 7, 2024

atiratree force-pushed the fix-sts-events branch 3 times, most recently from a9728e2 to f952189 Compare March 11, 2024 16:09

k8s-ci-robot added area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 11, 2024

atiratree force-pushed the fix-sts-events branch 4 times, most recently from 9497927 to 4fcd99f Compare March 11, 2024 18:58

atiratree commented Mar 11, 2024

View reviewed changes

soltysh reviewed Mar 15, 2024

View reviewed changes

atiratree added 2 commits March 15, 2024 15:18

fix pods tracking and internal error checking in statefulset tests

54f0b33

fix stateful set pod recreation and event spam

8c874c3

- do not emit events when pod reaches terminal phase - do not try to recreate pod until the old pod has been removed from etcd storage

atiratree force-pushed the fix-sts-events branch 4 times, most recently from c3dbcd7 to b904810 Compare March 15, 2024 16:07

fix conflict race in statefulset rest update

9d9a2d1

statefulset controller does less requests per sync now and thus can reconcile status faster, thus resulting in a higher chance for conflicts

atiratree force-pushed the fix-sts-events branch from b904810 to 9d9a2d1 Compare March 15, 2024 16:10

soltysh approved these changes Mar 15, 2024

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 Mar 15, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2024

k8s-ci-robot merged commit 85d55b6 into kubernetes:master Apr 18, 2024
15 checks passed

k8s-ci-robot added this to the v1.31 milestone Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix stateful set pod recreation and event spam #123809

fix stateful set pod recreation and event spam #123809

atiratree commented Mar 7, 2024

atiratree Mar 7, 2024

atiratree Mar 11, 2024

soltysh Mar 15, 2024

atiratree Mar 7, 2024

soltysh Mar 15, 2024

atiratree Mar 15, 2024

atiratree Mar 7, 2024

atiratree commented Mar 7, 2024

atiratree Mar 11, 2024

soltysh Mar 15, 2024

atiratree Mar 15, 2024

soltysh left a comment

soltysh Mar 15, 2024

soltysh Mar 15, 2024

soltysh Mar 15, 2024

soltysh left a comment

k8s-ci-robot commented Mar 15, 2024

k8s-ci-robot commented Mar 15, 2024

atiratree commented Mar 15, 2024

fix stateful set pod recreation and event spam #123809

fix stateful set pod recreation and event spam #123809

Conversation

atiratree commented Mar 7, 2024

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

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

atiratree commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh 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

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 15, 2024

k8s-ci-robot commented Mar 15, 2024

atiratree commented Mar 15, 2024