e2e node: Test probes during pod termination #127863

hshiina · 2024-10-04T18:07:04Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

These test cases look to intend to verify probe behaviors while a pod is being terminated:

kubernetes/test/e2e_node/container_lifecycle_test.go

Line 756 in 7ab8a2b

ginkgo.When("a pod is terminating because its liveness probe fails", func() {

In SyncTerminatingPod(), liveness probes are stopped before killing a pod while readiness probes are stopped after killing the pod:

kubernetes/pkg/kubelet/kubelet.go

Lines 2024 to 2038 in c9a61af

    
           kl.probeManager.StopLivenessAndStartup(pod) 
        
           p := kubecontainer.ConvertPodStatusToRunningPod(kl.getRuntime().Type(), podStatus) 
        
           if err := kl.killPod(ctx, pod, p, gracePeriod); err != nil { 
        
           	kl.recorder.Eventf(pod, v1.EventTypeWarning, events.FailedToKillPod, "error killing pod: %v", err) 
        
           	// there was an error killing the pod, so we return that error directly 
        
           	utilruntime.HandleError(err) 
        
           	return err 
        
           } 
        
           // Once the containers are stopped, we can stop probing for liveness and readiness. 
        
           // TODO: once a pod is terminal, certain probes (liveness exec) could be stopped immediately after 
        
           //   the detection of a container shutdown or (for readiness) after the first failure. Tracked as 
        
           //   https://github.com/kubernetes/kubernetes/issues/107894 although may not be worth optimizing. 
        
           kl.probeManager.RemovePod(pod)

The test cases are trying to test this code path with a failing liveness probe. However, when a liveness probe fails, the container is stopped at SyncPod() before the pod gets into termination phase even if RestartPolicy is Never:

kubernetes/pkg/kubelet/kubelet.go

Line 1963 in c9a61af

    
           result := kl.containerRuntime.SyncPod(sctx, pod, podStatus, pullSecrets, kl.backOff)

kubernetes/pkg/kubelet/kuberuntime/kuberuntime_manager.go

Lines 996 to 999 in c9a61af

    
           } else if liveness, found := m.livenessManager.Get(containerStatus.ID); found && liveness == proberesults.Failure { 
        
           	// If the container failed the liveness probe, we should kill it. 
        
           	message = fmt.Sprintf("Container %s failed liveness probe", container.Name) 
        
           	reason = reasonLivenessProbe

So, both liveness probes and readiness probes can run while the preStop hook is running. The test case to see if the liveness probe is stopped passes just because the init container gets into CrashLoopBackoff. If the init container doesn’t get into CrashLoopBackoff, this test fails (#127312).

In order to run the preStop hook in SyncTerminatingPod(), this PR adds test cases to terminate a pod with ActiveDeadlineSeconds.

After this is fixed, there remains a problem. Due to #124648, all probes are stopped before killing a pod if pod termination is triggered by kubelet such as active deadline and eviction. We need to fix this issue.

Alternatively, we can test this code path with deleting a pod. This PR also adds test cases to delete a pod with probes.

By the way, liveness probes for init containers are not stopped before killing the pod when the pod is terminated by deletion because StopLivenessAndStartup() stops only probes of regular containers:

kubernetes/pkg/kubelet/prober/prober_manager.go

Line 232 in 5ecd83f

for _, c := range pod.Spec.Containers {

Which issue(s) this PR fixes:

Fixes

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2024-10-04T18:07:14Z

Hi @hshiina. 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.

k8s-ci-robot · 2024-10-04T18:07:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hshiina
Once this PR has been reviewed and has the lgtm label, please assign mrunalp for approval. For more information see the Kubernetes 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:

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

hshiina · 2024-10-04T18:31:21Z

/remove-kind bug
/kind cleanup

test/e2e_node/container_lifecycle_test.go

matthyx · 2024-10-06T10:22:10Z

/ok-to-test

In order to test probes during pod termination, this fix uses ActiveDeadlineSeconds to trigger the pod termination. A failing liveness probe is not available for this purpose because a container is stopped before pod termination.

In order to test probes during pod termination, this fix add test cases to delete a pod with probes to trigger the pod termination.

hshiina · 2024-10-07T14:38:41Z

/remove-kind flake

AnishShah · 2024-10-09T17:16:03Z

/assign @AnishShah

bart0sh · 2024-10-13T16:44:25Z

/triage accepted
/priority important-longterm

k8s-ci-robot · 2024-10-13T16:44:29Z

@bart0sh: The label(s) priority/imprtant-longterm cannot be applied, because the repository doesn't have them.

In response to this:

/triage accepted
/priority imprtant-longterm

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.

bart0sh · 2024-10-13T16:45:03Z

/priority important-longterm

HirazawaUi

This PR adds two new triggering conditions to the existing test cases. However, running them takes too much time. For the "ActiveDeadlineSeconds expires" condition, we even need 110+ seconds to complete a single test case. I believe this is unnecessary in practice. You should try to minimize the waiting time to make the tests run faster :)

HirazawaUi · 2025-01-25T14:49:20Z

test/e2e_node/container_lifecycle_test.go

+							Name:  regular1,
+							Image: imageutils.GetE2EImage(imageutils.BusyBox),
+							Command: ExecCommand(regular1, execCommand{
+								Delay:              100,


Does this mean the pod will stay in the Running state for 100 seconds? This test takes way too long.

No, the pod termination is triggered in ActiveDeadlineSeconds(10s).

HirazawaUi · 2025-01-25T14:49:41Z

test/e2e_node/container_lifecycle_test.go

+			framework.ExpectNoError(err)
+
+			// Sleep expecting thet the preStop wiil complete and the container will get into TerminationSeconds window.
+			time.Sleep(25 * time.Second)


I think waiting for 25 seconds here is unnecessary. We can synchronize the reduction of the required time through TerminationSeconds and the PreStop hook, so that a single test case can run at a faster speed.

Though I don't remember exactly why I chose these values, I might have tried to make this test pass with EventedPLEG against #124704 that causes delays at deletion.

hshiina

I will revise this PR with shortening duration after #129355 or #127865 is merged.

/hold

hshiina · 2025-01-26T14:37:19Z

test/e2e_node/container_lifecycle_test.go

+							Name:  regular1,
+							Image: imageutils.GetE2EImage(imageutils.BusyBox),
+							Command: ExecCommand(regular1, execCommand{
+								Delay:              100,


No, the pod termination is triggered in ActiveDeadlineSeconds(10s).

hshiina · 2025-01-26T14:37:27Z

test/e2e_node/container_lifecycle_test.go

+			framework.ExpectNoError(err)
+
+			// Sleep expecting thet the preStop wiil complete and the container will get into TerminationSeconds window.
+			time.Sleep(25 * time.Second)


Though I don't remember exactly why I chose these values, I might have tried to make this test pass with EventedPLEG against #124704 that causes delays at deletion.

hshiina · 2025-04-07T19:17:29Z

This is not related to this PR. I just want to see how this job currently works.
/test pull-kubernetes-node-kubelet-serial-podresources

hshiina · 2025-06-27T15:36:29Z

This is not related to this PR. I just want to see how this job currently works.
/test pull-kubernetes-e2e-gce-serial

hshiina · 2025-06-28T12:11:19Z

/test pull-kubernetes-e2e-gce-serial

hshiina · 2025-06-28T20:28:47Z

/test pull-kubernetes-e2e-gce-serial

hshiina · 2025-06-28T20:31:07Z

/test pull-kubernetes-e2e-gce-serial-canary

k8s-ci-robot · 2025-06-29T05:19:21Z

@hshiina: The following tests 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-node-kubelet-serial-podresources	`eaef931`	link	false	`/test pull-kubernetes-node-kubelet-serial-podresources`
pull-kubernetes-e2e-gce-serial	`eaef931`	link	false	`/test pull-kubernetes-e2e-gce-serial`
pull-kubernetes-e2e-gce-serial-canary	`eaef931`	link	false	`/test pull-kubernetes-e2e-gce-serial-canary`

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.

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 4, 2024

k8s-ci-robot requested review from krmayankk and matthyx October 4, 2024 18:07

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

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/bug Categorizes issue or PR as related to a bug. labels Oct 4, 2024

hshiina force-pushed the probe-terminate branch from 79dccff to 6dbc51b Compare October 5, 2024 10:36

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 5, 2024

hshiina force-pushed the probe-terminate branch from 6dbc51b to 85ee561 Compare October 5, 2024 10:47

aojea reviewed Oct 5, 2024

View reviewed changes

test/e2e_node/container_lifecycle_test.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 6, 2024

e2e node: Test probes during pod termination

49fccc1

In order to test probes during pod termination, this fix uses ActiveDeadlineSeconds to trigger the pod termination. A failing liveness probe is not available for this purpose because a container is stopped before pod termination.

hshiina added 2 commits October 7, 2024 16:23

e2e node: Test probes during pod deletion

176be2d

In order to test probes during pod termination, this fix add test cases to delete a pod with probes to trigger the pod termination.

Restore removed test cases

eaef931

hshiina force-pushed the probe-terminate branch from 85ee561 to eaef931 Compare October 7, 2024 14:34

k8s-ci-robot removed the kind/flake Categorizes issue or PR as related to a flaky test. label Oct 7, 2024

k8s-ci-robot assigned AnishShah Oct 9, 2024

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Oct 13, 2024

k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 13, 2024

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 13, 2024

HirazawaUi reviewed Jan 25, 2025

View reviewed changes

hshiina commented Jan 26, 2025

View reviewed changes

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

	kl.probeManager.StopLivenessAndStartup(pod)

	p := kubecontainer.ConvertPodStatusToRunningPod(kl.getRuntime().Type(), podStatus)
	if err := kl.killPod(ctx, pod, p, gracePeriod); err != nil {
	kl.recorder.Eventf(pod, v1.EventTypeWarning, events.FailedToKillPod, "error killing pod: %v", err)
	// there was an error killing the pod, so we return that error directly
	utilruntime.HandleError(err)
	return err
	}

	// Once the containers are stopped, we can stop probing for liveness and readiness.
	// TODO: once a pod is terminal, certain probes (liveness exec) could be stopped immediately after
	// the detection of a container shutdown or (for readiness) after the first failure. Tracked as
	// https://github.com/kubernetes/kubernetes/issues/107894 although may not be worth optimizing.
	kl.probeManager.RemovePod(pod)

	} else if liveness, found := m.livenessManager.Get(containerStatus.ID); found && liveness == proberesults.Failure {
	// If the container failed the liveness probe, we should kill it.
	message = fmt.Sprintf("Container %s failed liveness probe", container.Name)
	reason = reasonLivenessProbe

e2e node: Test probes during pod termination #127863

Are you sure you want to change the base?

e2e node: Test probes during pod termination #127863

Uh oh!

Conversation

hshiina commented Oct 4, 2024 • 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 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.:

Uh oh!

k8s-ci-robot commented Oct 4, 2024

Uh oh!

k8s-ci-robot commented Oct 4, 2024

Uh oh!

hshiina commented Oct 4, 2024

Uh oh!

Uh oh!

matthyx commented Oct 6, 2024

Uh oh!

hshiina commented Oct 7, 2024

Uh oh!

AnishShah commented Oct 9, 2024

Uh oh!

bart0sh commented Oct 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 13, 2024

Uh oh!

bart0sh commented Oct 13, 2024

Uh oh!

HirazawaUi left a comment

Choose a reason for hiding this comment

Uh oh!

HirazawaUi Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

hshiina Jan 26, 2025

Choose a reason for hiding this comment

Uh oh!

HirazawaUi Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

hshiina Jan 26, 2025

Choose a reason for hiding this comment

Uh oh!

hshiina left a comment

Choose a reason for hiding this comment

Uh oh!

hshiina Jan 26, 2025

Choose a reason for hiding this comment

Uh oh!

hshiina Jan 26, 2025

Choose a reason for hiding this comment

Uh oh!

hshiina commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hshiina commented Jun 27, 2025

Uh oh!

hshiina commented Jun 28, 2025

Uh oh!

hshiina commented Jun 28, 2025

Uh oh!

hshiina commented Jun 28, 2025

Uh oh!

k8s-ci-robot commented Jun 29, 2025

Uh oh!

Uh oh!

hshiina commented Oct 4, 2024 •

edited

Loading

bart0sh commented Oct 13, 2024 •

edited

Loading

hshiina commented Apr 7, 2025 •

edited

Loading