Verify pod termination with E2E PreStop hook #94922

knabben · 2020-09-20T02:40:05Z

What type of PR is this?
/kind cleanup
/kind flake
/sig node
/sig testing

What this PR does / why we need it:

This PR de-flaky the PreStop E2E node test by reducing the wait time period for checking the preStop hook test is being executed.

Which issue(s) this PR fixes:

Related #94918

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2020-09-20T02:40:13Z

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

knabben · 2020-09-21T17:24:22Z

/assign @spiffxp

SergeyKanzhelev · 2020-09-21T20:26:13Z

test/e2e/node/pre_stop.go

@@ -181,8 +179,8 @@ var _ = SIGDescribe("PreStop", func() {
 		testPreStop(f.ClientSet, f.Namespace.Name)
 	})

-	ginkgo.It("graceful pod terminated should wait until preStop hook completes the process [Flaky]", func() {


this PR effectively change what the test is validating. Original one was testing that if gracePeriod allows it and the pod preStop hook is configured to be long enough, pod will be running that preStop hook for the whole duration. New test checks that pod will not exist after grace period. Which doesn't confirm that preStop hook was executed. Is it correct understanding or a proposed change?

SergeyKanzhelev · 2020-09-21T20:27:22Z

test/e2e/node/pre_stop.go

@@ -201,28 +199,8 @@ var _ = SIGDescribe("PreStop", func() {
 		err = podClient.Delete(context.TODO(), pod.Name, *metav1.NewDeleteOptions(gracefulTerminationPeriodSeconds))
 		framework.ExpectNoError(err, "failed to delete pod")

-		//wait up to graceful termination period seconds
-		time.Sleep(30 * time.Second)


should the original test be fixed by changing this timeout to 15 second? 30 seconds is on the edge of graceful termination timeout and pod MAY or MAY NOT run at this ponit. After 15 seconds is is still running for sure if preStop hook is being executed.

SergeyKanzhelev · 2020-09-21T21:14:01Z

test/e2e/node/pre_stop.go

@@ -202,7 +202,7 @@ var _ = SIGDescribe("PreStop", func() {
 		framework.ExpectNoError(err, "failed to delete pod")

 		//wait up to graceful termination period seconds


just a small suggestion - perhaps we can explain why it is 15 seconds in a comment here.

SergeyKanzhelev · 2020-09-21T21:15:09Z

/ok-to-test
/lgtm
(will re-lgtm with the update of comment)

knabben · 2020-09-23T00:34:35Z

Failed with another flaky - [sig-storage] HostPath should support r/w [NodeConformance]

SergeyKanzhelev · 2020-09-23T04:18:11Z

/test pull-kubernetes-node-e2e

knabben · 2020-09-23T11:33:22Z

/test pull-kubernetes-node-e2e

spiffxp · 2020-09-23T14:28:53Z

/test pull-kubernetes-node-e2e

SergeyKanzhelev · 2020-09-23T16:15:07Z

/test pull-kubernetes-node-e2e

knabben · 2020-09-23T22:26:27Z

/test pull-kubernetes-node-e2e

@spiffxp good to go here?

oomichi · 2020-09-25T18:00:25Z

This mark flake has been added since #83792 and the test was failed flaky at

kubernetes/test/e2e/node/pre_stop.go

Line 212 in 14d380c

framework.ExpectNoError(err, "validate-pod-is-running")

I am not sure what ratio of this test failure, can we know that at this time to make the test un-flake safely?
/test pull-kubernetes-node-e2e

knabben · 2020-09-25T20:24:51Z

@oomichi the test was sleeping for the entire graceful period (30 seconds), and checking if the pod is running in the extended grace period +2 seconds, reducing the sleep time in half will ensure the test is still running and is inside the grace period.

/test pull-kubernetes-node-e2e

oomichi · 2020-09-25T23:10:02Z

test/e2e/node/pre_stop.go

-		time.Sleep(30 * time.Second)
+		// wait for less than the gracePeriod termination ensuring the
+		// preStop hook is still executing.
+		time.Sleep(15 * time.Second)

 		ginkgo.By("verifying the pod running state after graceful termination")


@oomichi the test was sleeping for the entire graceful period (30 seconds), and checking if the pod is running in the extended grace period +2 seconds, reducing the sleep time in half will ensure the test is still running and is inside the grace period.

Thanks for your explanation. I guess I got the point.
I'd like to summarize my understanding to know that is correct:

The original e2e test expects the pod is still Running after graceful termination (30 seconds in this test case) because of the extended 2 seconds as you said since

If one of the Pod's containers has defined a preStop hook, the kubelet runs that hook inside of the container. If the preStop hook is still running after the grace period expires, the kubelet requests a small, one-off grace period extension of 2 seconds.

of https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-termination

However that 2 seconds can make the test flake due to high workload at upstream CI systems sometimes.
Then this PR changes to verify the pod is still running before expiring the graceful termination during preStop hook is running.

If the above is correct, there are 2 points.

The above message needs to be updated because the graceful termination doesn't happen at the time.

Technically this test doesn't check preStop hook is running. the preStop outputs preStop to its stdout every 1 second but that is not checked on the test side. I don't think we need to add this check in this PR for the scope, it is better to add it with another PR.

Yes, this is the rationality here.

Maybe verifying the pod is running while in the graceful period termination

Sure, the conformance test for the preHook follows this idea. I can propose another PR or even a new test with the stdout check logic.

Thanks, the above sounds good for me.

SergeyKanzhelev · 2020-09-28T16:40:20Z

/lgtm

thank you for updating the comment

knabben · 2020-09-28T18:13:15Z

/test pull-kubernetes-conformance-kind-ipv6-parallel

oomichi · 2020-09-28T21:27:48Z

Thanks for updating.

/lgtm

spiffxp · 2020-10-01T19:58:56Z

I think I would prefer to merge the change in this PR, but remove the Flaky tag in a followup PR, based on data from https://testgrid.k8s.io/google-gce#gce-cos-master-flaky-repro

knabben · 2020-10-01T20:05:21Z

I agree, @spiffxp PTAL, I open another one to remove the flaky tag after we ensure it's been running flat.

oomichi · 2020-10-01T20:09:16Z

Nice point to keep Flaky label to get data.

/lgtm

spiffxp

/approve
Thank you! And thanks for your patience in getting this through

k8s-ci-robot · 2020-10-01T21:49:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knabben, spiffxp

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:

~~test/e2e/node/OWNERS~~ [spiffxp]

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

knabben · 2020-10-01T22:57:02Z

Flaky from:

go_test: //staging/src/k8s.io/legacy-cloud-providers/vsphere/go_default_test:run_2_of_2

/test pull-kubernetes-bazel-test

k8s-ci-robot added the area/test label Sep 20, 2020

k8s-ci-robot requested review from andrewsykim and mtaufen September 20, 2020 02:41

knabben force-pushed the pre-stop-e2e branch from 3ec96e6 to f0e2992 Compare September 20, 2020 02:43

k8s-ci-robot assigned spiffxp Sep 21, 2020

SergeyKanzhelev reviewed Sep 21, 2020

View reviewed changes

knabben force-pushed the pre-stop-e2e branch from f0e2992 to f68cf33 Compare September 21, 2020 21:10

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 21, 2020

knabben force-pushed the pre-stop-e2e branch from f68cf33 to 6e9ad57 Compare September 21, 2020 21:11

SergeyKanzhelev reviewed Sep 21, 2020

View reviewed changes

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 Sep 21, 2020

k8s-ci-robot assigned SergeyKanzhelev Sep 21, 2020

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

knabben force-pushed the pre-stop-e2e branch from 6e9ad57 to 2766930 Compare September 21, 2020 21:26

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2020

oomichi reviewed Sep 25, 2020

View reviewed changes

knabben force-pushed the pre-stop-e2e branch from 5e9c13f to d3165bc Compare September 28, 2020 16:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2020

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

k8s-ci-robot assigned oomichi Sep 28, 2020

Verify pod termination with E2E PreStop hook

68ce3cf

knabben force-pushed the pre-stop-e2e branch from d3165bc to 68ce3cf Compare October 1, 2020 20:04

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2020

spiffxp approved these changes Oct 1, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2020

k8s-ci-robot merged commit 1ec3345 into kubernetes:master Oct 1, 2020

k8s-ci-robot added this to the v1.20 milestone Oct 1, 2020

knabben mentioned this pull request Oct 15, 2020

Removing flaky tag from preStop test #95577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify pod termination with E2E PreStop hook #94922

Verify pod termination with E2E PreStop hook #94922

knabben commented Sep 20, 2020 •

edited

k8s-ci-robot commented Sep 20, 2020

knabben commented Sep 21, 2020

SergeyKanzhelev Sep 21, 2020

SergeyKanzhelev Sep 21, 2020

SergeyKanzhelev Sep 21, 2020

SergeyKanzhelev commented Sep 21, 2020

knabben commented Sep 23, 2020 •

edited

SergeyKanzhelev commented Sep 23, 2020

knabben commented Sep 23, 2020

spiffxp commented Sep 23, 2020

SergeyKanzhelev commented Sep 23, 2020

knabben commented Sep 23, 2020

oomichi commented Sep 25, 2020

knabben commented Sep 25, 2020

oomichi Sep 25, 2020

knabben Sep 26, 2020 •

edited

oomichi Sep 28, 2020

SergeyKanzhelev commented Sep 28, 2020

knabben commented Sep 28, 2020

oomichi commented Sep 28, 2020

spiffxp commented Oct 1, 2020

knabben commented Oct 1, 2020

oomichi commented Oct 1, 2020

spiffxp left a comment

k8s-ci-robot commented Oct 1, 2020

knabben commented Oct 1, 2020

		@@ -202,7 +202,7 @@ var _ = SIGDescribe("PreStop", func() {
		framework.ExpectNoError(err, "failed to delete pod")

		//wait up to graceful termination period seconds

Verify pod termination with E2E PreStop hook #94922

Verify pod termination with E2E PreStop hook #94922

Conversation

knabben commented Sep 20, 2020 • edited

k8s-ci-robot commented Sep 20, 2020

knabben commented Sep 21, 2020

SergeyKanzhelev Sep 21, 2020

Choose a reason for hiding this comment

SergeyKanzhelev Sep 21, 2020

Choose a reason for hiding this comment

SergeyKanzhelev Sep 21, 2020

Choose a reason for hiding this comment

SergeyKanzhelev commented Sep 21, 2020

knabben commented Sep 23, 2020 • edited

SergeyKanzhelev commented Sep 23, 2020

knabben commented Sep 23, 2020

spiffxp commented Sep 23, 2020

SergeyKanzhelev commented Sep 23, 2020

knabben commented Sep 23, 2020

oomichi commented Sep 25, 2020

knabben commented Sep 25, 2020

oomichi Sep 25, 2020

Choose a reason for hiding this comment

knabben Sep 26, 2020 • edited

Choose a reason for hiding this comment

oomichi Sep 28, 2020

Choose a reason for hiding this comment

SergeyKanzhelev commented Sep 28, 2020

knabben commented Sep 28, 2020

oomichi commented Sep 28, 2020

spiffxp commented Oct 1, 2020

knabben commented Oct 1, 2020

oomichi commented Oct 1, 2020

spiffxp left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 1, 2020

knabben commented Oct 1, 2020

knabben commented Sep 20, 2020 •

edited

knabben commented Sep 23, 2020 •

edited

knabben Sep 26, 2020 •

edited