Fix startup probe worker termination for sidecar containers #133072

AadiDev005 · 2025-07-19T04:29:55Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fixes a bug where startup probe workers terminate incorrectly for sidecar containers with restartPolicy=Always when the pod has restartPolicy=Never. This causes main containers to remain stuck in "Initializing" state even after sidecar containers successfully restart.

Problem Description:

Pod with restartPolicy: Never
Sidecar container with restartPolicy: Always and startup probe
Sidecar fails initially, then restarts successfully
Bug: Main container never starts (stuck in Initializing)
Root cause: Probe worker terminates based only on pod restart policy

Solution:
Minimal fix adds container-level restart policy check only for init containers to the probe worker continuation logic, allowing workers to continue for sidecar containers while preserving existing behavior for regular containers.

Which issue(s) this PR is related to:

Fixes #132826

Special notes for your reviewer:

Minimal change: Single line addition with proper null safety checks and init container validation
No breaking changes: Existing behavior preserved for regular containers
Scoped to init containers only: Fix only applies to restartable init containers (sidecars)
Comprehensive testing: Unit tests cover both sidecar and regular container scenarios
E2E test included: Validates end-to-end functionality

Does this PR introduce a user-facing change?

Fixed a startup probe race condition that caused main containers to remain stuck in "Initializing" state when sidecar containers with startup probes failed initially but succeeded on restart in pods with restartPolicy=Never.

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

k8s-ci-robot · 2025-07-19T04:30:05Z

Hi @AadiDev005. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

gjkim42 · 2025-07-19T06:00:55Z

/cc
/priority important-soon
/ok-to-test

gjkim42

It looks mostly good at first glance 👍

left some comments. let's see if the e2e passes.

gjkim42 · 2025-07-19T06:09:59Z

test/e2e_node/container_lifecycle_test.go

@@ -2126,6 +2126,133 @@ var _ = SIGDescribe(feature.SidecarContainers, "Containers Lifecycle", func() {
 	addAfterEachForCleaningUpPods(f)
 	f.NamespacePodSecurityLevel = admissionapi.LevelPrivileged

+	ginkgo.When("A sidecar container with startup probe fails initially", func() {


nit:
Can we move this test to the bottom of "SidecarContainers Container Lifecycle" block, since this is a very edge case?

gjkim42 · 2025-07-19T06:12:32Z

test/e2e_node/container_lifecycle_test.go

@@ -2126,6 +2126,133 @@ var _ = SIGDescribe(feature.SidecarContainers, "Containers Lifecycle", func() {
 	addAfterEachForCleaningUpPods(f)
 	f.NamespacePodSecurityLevel = admissionapi.LevelPrivileged

+	ginkgo.When("A sidecar container with startup probe fails initially", func() {
+		ginkgo.It("should continue probing and allow main container to start after sidecar recovers [NodeConformance]", func(ctx context.Context) {


Can we remove the [NodeConformance] label here?
I think we don't need it for this edge case, and it is deprecated as well.

gjkim42 · 2025-07-19T06:16:23Z

pkg/kubelet/prober/worker.go

@@ -253,7 +253,9 @@ func (w *worker) doProbe(ctx context.Context) (keepGoing bool) {
 		}
 		// Abort if the container will not be restarted.
 		return c.State.Terminated == nil ||
-			w.pod.Spec.RestartPolicy != v1.RestartPolicyNever
+			w.pod.Spec.RestartPolicy != v1.RestartPolicyNever ||
+			(w.container.RestartPolicy != nil && *w.container.RestartPolicy == v1.ContainerRestartPolicyAlways)


Can we check this only if the container is an init container?
The usage of the container restart policy for a regular container is not defined yet.

gjkim42 · 2025-08-03T11:29:49Z

/test pull-kubernetes-node-kubelet-serial-containerd-sidecar-containers
/test pull-kubernetes-node-e2e-containerd-features
/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-features
/test pull-kubernetes-cos-cgroupv1-containerd-node-e2e-features

gjkim42 · 2025-08-03T11:32:20Z

Thank you.
/lgtm

k8s-ci-robot · 2025-08-03T11:32:27Z

LGTM label has been added.

Git tree hash: f45acaaa0c792d94f849ef3ab006a26ee7d9b01b

gjkim42

/lgtm
/assign @SergeyKanzhelev
Could you take a look at this?

This fixes the bug where a startup probe of a restartable init container may not be executed if the restartable init container restarts.

gjkim42 · 2025-08-03T12:05:48Z

/retest

gjkim42 · 2025-08-04T12:43:02Z

/retest

AadiDev005 · 2025-08-04T12:59:22Z

Hi @gjkim42 / @SergeyKanzhelev,

The required pull-kubernetes-unit job is failing in the scheduler’s dynamic-resources tests:

pkg/scheduler/framework/plugins/dynamicresources
└─ TestPlugin/extended-resource-name-with-resources … FAIL
    expected ResourceClaim != nil

My patch only touches pkg/kubelet/prober/*, so I’m not sure whether this is:

A side-effect of my branch being a bit behind master, or
Something I need to fix directly.

Before I start rebasing and regenerating code, could you please confirm the expected next step?
Happy to follow any guidance.

Thanks!

gjkim42 · 2025-08-04T13:09:21Z

#133302
^
I think the test seems flaky.
(and it succeeds this time, fortunately)

(FYI: We cannot merge a PR unless all required tests succeed, so we have to retest them if it looks like a flaky test issue.)

AadiDev005 · 2025-08-04T13:44:32Z

@Random-Liu @tallclair @SergeyKanzhelev
All required tests are green and the fix is tagged priority/important-soon.
Could one of you please:
/milestone v1.34
/approve

k8s-ci-robot · 2025-08-04T13:44:34Z

@AadiDev005: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Milestone Maintainers Team and have them propose you as an additional delegate for this responsibility.

In response to this:

@Random-Liu @tallclair @SergeyKanzhelev
All required tests are green and the fix is tagged priority/important-soon.
Could one of you please:
/milestone v1.34
/approve

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.

k8s-ci-robot · 2025-08-04T13:45:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AadiDev005
Once this PR has been reviewed and has the lgtm label, please ask for approval from sergeykanzhelev. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/kubelet/OWNERS
test/e2e_node/OWNERS

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

gjkim42 · 2025-08-04T14:47:40Z

The code freeze is already in effect and this doesn't look like a urgent fix, so let's merge this for the next cycle.

https://www.kubernetes.dev/resources/release/

toVersus · 2025-08-05T05:23:23Z

pkg/kubelet/prober/worker_test.go

+// newTestWorkerWithInitContainer creates a test worker with an init container setup
+func newTestWorkerWithInitContainer(m *manager, probeType probeType, probe v1.Probe) *worker {


nit: It might be better to clarify that this is specifically about restartable init container.

Suggested change

// newTestWorkerWithInitContainer creates a test worker with an init container setup

func newTestWorkerWithInitContainer(m *manager, probeType probeType, probe v1.Probe) *worker {

// newTestWorkerWithRestartableInitContainer creates a test worker with an init container setup

func newTestWorkerWithRestartableInitContainer(m *manager, probeType probeType, probe v1.Probe) *worker {

toVersus · 2025-08-05T05:27:54Z

pkg/kubelet/prober/worker_test.go

+		Image: "registry.k8s.io/pause:3.9",
+	}}
+
+	return &worker{


Can we reuse the existing newWorker() here? That way, we won't need getInitialResult() anymore.

Something like this:

diff --git a/pkg/kubelet/prober/worker_test.go b/pkg/kubelet/prober/worker_test.go index 1f6104179b1..d8ef9ae7462 100644 --- a/pkg/kubelet/prober/worker_test.go +++ b/pkg/kubelet/prober/worker_test.go @@ -38,7 +38,7 @@ func init() { } // newTestWorkerWithInitContainer creates a test worker with an init container setup -func newTestWorkerWithInitContainer(m *manager, probeType probeType, probe v1.Probe) *worker { +func newTestWorkerWithInitContainer(m *manager, probeType probeType) *worker { pod := getTestPod() // Set up init container with restart policy @@ -50,19 +50,10 @@ func newTestWorkerWithInitContainer(m *manager, probeType probeType, probe v1.Pr // Move container to init containers and add a regular container pod.Spec.InitContainers = []v1.Container{initContainer} pod.Spec.Containers = []v1.Container{{ - Name: "main-container", - Image: "registry.k8s.io/pause:3.9", + Name: "main-container", }} - return &worker{ - pod: pod, - container: initContainer, - probeType: probeType, - spec: &probe, - probeManager: m, - resultsManager: resultsManager(m, probeType), - initialValue: getInitialResult(probeType), - } + return newWorker(m, probeType, pod, initContainer) }

toVersus · 2025-08-05T05:28:58Z

pkg/kubelet/prober/worker_test.go

+	pod.Spec.InitContainers = []v1.Container{initContainer}
+	pod.Spec.Containers = []v1.Container{{
+		Name:  "main-container",
+		Image: "registry.k8s.io/pause:3.9",


nit: Since the image field isn't used in the test, it can be removed.

Suggested change

Image: "registry.k8s.io/pause:3.9",

toVersus · 2025-08-05T05:31:28Z

pkg/kubelet/prober/worker.go

@@ -253,7 +263,8 @@ func (w *worker) doProbe(ctx context.Context) (keepGoing bool) {
 		}
 		// Abort if the container will not be restarted.
 		return c.State.Terminated == nil ||
-			w.pod.Spec.RestartPolicy != v1.RestartPolicyNever
+			w.pod.Spec.RestartPolicy != v1.RestartPolicyNever ||
+			(w.isInitContainer() && w.container.RestartPolicy != nil && *w.container.RestartPolicy == v1.ContainerRestartPolicyAlways)


nit: I don't feel strongly about this, but for readability, it might be worth giving this condition a name.

diff --git a/pkg/kubelet/prober/worker.go b/pkg/kubelet/prober/worker.go index e71fd1b5687..4948cdb4236 100644 --- a/pkg/kubelet/prober/worker.go +++ b/pkg/kubelet/prober/worker.go @@ -261,10 +261,14 @@ func (w *worker) doProbe(ctx context.Context) (keepGoing bool) { if !w.containerID.IsEmpty() { w.resultsManager.Set(w.containerID, results.Failure, w.pod) } + + isRestartableInitContainer := w.isInitContainer() && + w.container.RestartPolicy != nil && *w.container.RestartPolicy == v1.ContainerRestartPolicyAlways + // Abort if the container will not be restarted. return c.State.Terminated == nil || w.pod.Spec.RestartPolicy != v1.RestartPolicyNever || - (w.isInitContainer() && w.container.RestartPolicy != nil && *w.container.RestartPolicy == v1.ContainerRestartPolicyAlways) + isRestartableInitContainer } // Graceful shutdown of the pod.

toVersus · 2025-08-05T05:32:12Z

pkg/kubelet/prober/worker_test.go

@@ -530,6 +572,72 @@ func TestResultRunOnStartupCheckFailure(t *testing.T) {
 	}
 }

+func TestDoProbe_TerminatedRestartableInitContainerWithRestartPolicyNone(t *testing.T) {


nit:

Suggested change

func TestDoProbe_TerminatedRestartableInitContainerWithRestartPolicyNone(t *testing.T) {

func TestDoProbe_TerminatedRestartableInitContainerWithRestartPolicyNever(t *testing.T) {

toVersus · 2025-08-05T05:35:16Z

pkg/kubelet/prober/worker_test.go

+	// Test: Terminated restartable init container with restartPolicy=Always should continue probing
+	// even when pod has restartPolicy=Never
+	if !w.doProbe(ctx) {
+		t.Error("Expected to continue probing for terminated restartable init container with restart policy Always")


nit:

Suggested change

t.Error("Expected to continue probing for terminated restartable init container with restart policy Always")

t.Error("Expected to continue probing for terminated restartable init container with pod restart policy Never")

toVersus · 2025-08-05T05:35:51Z

pkg/kubelet/prober/worker_test.go

+	}
+
+	// Verify result is set to Failure for terminated container
+	expectResult(t, w, results.Failure, "terminated restartable init container with restart policy Always")


nit:

Suggested change

expectResult(t, w, results.Failure, "terminated restartable init container with restart policy Always")

expectResult(t, w, results.Failure, "restartable init container with pod restart policy Never")

Fixes a bug where startup probe workers terminate incorrectly for sidecar containers with restartPolicy=Always when the pod has restartPolicy=Never, causing main containers to remain stuck in "Initializing" state. Changes: - Add container-level restart policy check for init containers only - Extract complex boolean logic to named variable for readability - Refactor test helper to use existing newWorker() function - Fix terminology: RestartPolicyNone → RestartPolicyNever - Add comprehensive unit and e2e tests for both scenarios

k8s-ci-robot · 2025-08-05T06:42:32Z

New changes are detected. LGTM label has been removed.

AadiDev005 · 2025-08-05T06:46:53Z

Hey @toVersus, thanks so much for the thorough review! 🙏

I've implemented all your suggestions and squashed everything into a clean single commit:

• Renamed the function to be clearer about restartable init containers
• Switched to using the existing newWorker() (great catch on the code duplication!)
• Cleaned up the unused image field and helper function
• Fixed the terminology from None to Never to match Kubernetes conventions
• Made that complex boolean condition much more readable with a proper variable name

Really appreciate you taking the time to make the code cleaner and more maintainable. The refactoring suggestions especially make a lot of sense - no need to reinvent the wheel when newWorker() already does what we need!
😊

@gjkim42 - can u take a look for a moment

gjkim42 · 2025-08-05T06:55:55Z

/test pull-kubernetes-node-e2e-containerd-features
/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-features
/test pull-kubernetes-cos-cgroupv1-containerd-node-e2e-features

k8s-ci-robot · 2025-08-05T07:09:52Z

@AadiDev005: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-unit-windows-master	`0a26f03`	link	false	`/test pull-kubernetes-unit-windows-master`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

AadiDev005 · 2025-08-08T07:17:54Z

The Windows unit test failure in pull-kubernetes-unit-windows-master is unrelated to the sidecar startup probe fix, which only touches Linux kubelet probe logic in pkg/kubelet/prober/*. With 15/16 successful checks including all Linux-specific node tests, the sidecar fix is working correctly.

@SergeyKanzhelev @gjkim42 All code review feedback addressed and Linux tests green. Ready for approval when the freeze lifts?

k8s-ci-robot added area/kubelet area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 19, 2025

github-project-automation bot added this to SIG Node CI/Test Board Jul 19, 2025

github-project-automation bot moved this to Triage in SIG Node CI/Test Board Jul 19, 2025

github-project-automation bot added this to SIG Node: code and documentation PRs Jul 19, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs Jul 19, 2025

k8s-ci-robot requested review from Random-Liu and tallclair July 19, 2025 04:30

k8s-ci-robot requested a review from gjkim42 July 19, 2025 06:00

AadiDev005 force-pushed the fix-sidecar-startup-probe branch from 2e2983c to 29f796b Compare July 19, 2025 06:26

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jul 19, 2025

gjkim42 reviewed Jul 19, 2025

View reviewed changes

AadiDev005 force-pushed the fix-sidecar-startup-probe branch from fc077e0 to bc4856a Compare August 3, 2025 11:24

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Aug 3, 2025

AadiDev005 requested a review from gjkim42 August 3, 2025 11:28

k8s-ci-robot assigned gjkim42 Aug 3, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2025

gjkim42 reviewed Aug 3, 2025

View reviewed changes

k8s-ci-robot assigned SergeyKanzhelev Aug 3, 2025

toVersus reviewed Aug 5, 2025

View reviewed changes

AadiDev005 force-pushed the fix-sidecar-startup-probe branch from bc4856a to 0a26f03 Compare August 5, 2025 06:42

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2025

k8s-ci-robot requested review from gjkim42 and SergeyKanzhelev August 5, 2025 06:42

AadiDev005 requested a review from toVersus August 5, 2025 06:47

		// newTestWorkerWithInitContainer creates a test worker with an init container setup
		func newTestWorkerWithInitContainer(m manager, probeType probeType, probe v1.Probe) worker {

	func TestDoProbe_TerminatedRestartableInitContainerWithRestartPolicyNone(t *testing.T) {
	func TestDoProbe_TerminatedRestartableInitContainerWithRestartPolicyNever(t *testing.T) {

	t.Error("Expected to continue probing for terminated restartable init container with restart policy Always")
	t.Error("Expected to continue probing for terminated restartable init container with pod restart policy Never")

	expectResult(t, w, results.Failure, "terminated restartable init container with restart policy Always")
	expectResult(t, w, results.Failure, "restartable init container with pod restart policy Never")

Fix startup probe worker termination for sidecar containers #133072

Are you sure you want to change the base?

Fix startup probe worker termination for sidecar containers #133072

Conversation

AadiDev005 commented Jul 19, 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 is related to:

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 Jul 19, 2025

Uh oh!

gjkim42 commented Jul 19, 2025

Uh oh!

gjkim42 left a comment

Choose a reason for hiding this comment

Uh oh!

gjkim42 Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gjkim42 commented Aug 3, 2025

Uh oh!

gjkim42 commented Aug 3, 2025

Uh oh!

k8s-ci-robot commented Aug 3, 2025

Uh oh!

gjkim42 left a comment

Choose a reason for hiding this comment

Uh oh!

gjkim42 commented Aug 3, 2025

Uh oh!

gjkim42 commented Aug 4, 2025

Uh oh!

AadiDev005 commented Aug 4, 2025

Uh oh!

gjkim42 commented Aug 4, 2025

Uh oh!

AadiDev005 commented Aug 4, 2025

Uh oh!

k8s-ci-robot commented Aug 4, 2025

Uh oh!

k8s-ci-robot commented Aug 4, 2025

Uh oh!

gjkim42 commented Aug 4, 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!

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!

k8s-ci-robot commented Aug 5, 2025

Uh oh!

AadiDev005 commented Aug 5, 2025

Uh oh!

gjkim42 commented Aug 5, 2025

Uh oh!

k8s-ci-robot commented Aug 5, 2025

Uh oh!

AadiDev005 commented Aug 8, 2025

Uh oh!

Uh oh!

AadiDev005 commented Jul 19, 2025 •

edited

Loading

gjkim42 Jul 19, 2025 •

edited

Loading

gjkim42 commented Aug 4, 2025 •

edited

Loading