Do not try to run preStopHook when the gracePeriod is 0 #49449

dhilipkumars · 2017-07-22T19:37:34Z

What this PR does / why we need it:

Sometimes when the user force deletes a POD with no gracePeriod, its possible that kubelet attempts to execute the preStopHook which will certainly fail. This PR prevents this inavitable PreStopHook failure.

kubectl delete --force --grace-period=0 po/<pod-name>

This also adds UT for LifeCycle Hooks

time go test --cover -v --run "Hook" ./pkg/kubelet/kuberuntime/
.
.
.
--- PASS: TestLifeCycleHook (0.00s)
    --- PASS: TestLifeCycleHook/PreStop-CMDExec (0.00s)
    --- PASS: TestLifeCycleHook/PreStop-HTTPGet (0.00s)
    --- PASS: TestLifeCycleHook/PreStop-NoTimeToRun (0.00s)
    --- PASS: TestLifeCycleHook/PostStart-CmdExe (0.00s)
PASS
coverage: 15.3% of statements
ok      k8s.io/kubernetes/pkg/kubelet/kuberuntime       0.429s

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

Do not try to run preStopHook when the gracePeriod is 0

k8s-ci-robot · 2017-07-22T19:37:41Z

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

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

dhilipkumars · 2017-07-22T19:38:51Z

cc: @dchen1107 and @derekwaynecarr

dhilipkumars · 2017-07-22T19:42:32Z

/sig node
/sig app-lifecycle
/area app-lifecycle

dixudx · 2017-07-24T02:26:51Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

@@ -28,6 +28,8 @@ import (
 	runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
 	containertest "k8s.io/kubernetes/pkg/kubelet/container/testing"
+	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
+	"strings"


Need running gofmt.

they are go fmt'ed but gofmt wouldnt re-arrange imports, now fixed.

dhilipkumars · 2017-07-25T04:18:28Z

/sig node-reviewers

dixudx · 2017-07-26T02:10:35Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

@@ -20,6 +20,7 @@ import (
 	"path/filepath"
 	"testing"
 	"time"
+	"strings"


This is not still right. Should use gofmt to update.

okay got the problem, I was doing go fmt instead of gofmt, now fixed. PTAL

dhilipkumars · 2017-07-28T02:08:27Z

/kind bug

resouer · 2017-07-29T01:44:14Z

/ok-to-test

resouer

Please fix CI first, then I will review

dhilipkumars · 2017-07-29T10:22:16Z

/test pull-kubernetes-verify
/test pull-kubernetes-bazel

dhilipkumars · 2017-07-29T11:17:38Z

/test pull-kubernetes-e2e-gce-etcd3

dhilipkumars · 2017-07-29T13:33:44Z

/test pull-kubernetes-e2e-gce-etcd3

dhilipkumars · 2017-07-29T14:33:34Z

/test pull-kubernetes-e2e-gce-etcd3

dhilipkumars · 2017-07-29T16:48:45Z

@resouer looks like a flake the failures are unrelated.

/test pull-kubernetes-e2e-gce-etcd3

dhilipkumars · 2017-07-29T17:56:05Z

Flake #49761

dhilipkumars · 2017-07-30T07:17:27Z

/retest

resouer · 2017-07-30T13:48:22Z

/test pull-kubernetes-e2e-gce-etcd3

resouer

lgtm, let's run test again

resouer · 2017-07-31T02:04:23Z

Kindly ping @tallclair @Random-Liu , the code lgtm, please take a final look, thanks!

dhilipkumars · 2017-07-31T02:36:11Z

/test pull-kubernetes-e2e-kops-aw

dhilipkumars · 2017-08-01T05:34:47Z

/test pull-kubernetes-e2e-kops-aw

dhilipkumars · 2017-08-01T05:36:51Z

/retest

resouer · 2017-08-01T05:54:30Z

The pull-kubernetes-e2e-kops-aws is flake, I noticed it fails in many PRs.

dhilipkumars · 2017-08-01T06:12:00Z

Flake: #48223

dhilipkumars · 2017-08-01T10:37:17Z

/retest

dhilipkumars · 2017-08-01T15:47:51Z

/retest

tallclair

Just needs a little cleanup, logic looks good.

tallclair · 2017-08-01T18:23:21Z

pkg/kubelet/kuberuntime/fake_kuberuntime_manager.go

@@ -75,7 +75,6 @@ func NewFakeKubeRuntimeManager(runtimeService internalapi.RuntimeService, imageS
 	if err != nil {
 		return nil, err
 	}
-


nit: try not to include stray changes like this.

tallclair · 2017-08-01T18:37:32Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

+	//TestCase 1:  Configured and works as expected
+	t.Run("PreStop-CMDExec", func(t *testing.T) {
+		testPod.Spec.Containers[0].Lifecycle = cmdLifeCycle
+		//m.executePreStopHook(testPod, cID, &testPod.Spec.Containers[0], gracePeriod)


delete this

tallclair · 2017-08-01T18:37:56Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

+		//m.executePreStopHook(testPod, cID, &testPod.Spec.Containers[0], gracePeriod)
+		m.killContainer(testPod, cID, "foo", "testKill", &gracePeriod)
+		if fakeRunner.Cmd[0] != cmdLifeCycle.PreStop.Exec.Command[0] {
+			t.Errorf("CMD Prestop hook is no invoked")


s/is no/was not/

tallclair · 2017-08-01T18:39:36Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

+
+	m.runner = lcHanlder
+
+	//TestCase 1:  Configured and works as expected


Nit: delete the enumeration in the comment (they just become annoying to update when test cases are added). Just the description part is sufficient.

tallclair · 2017-08-01T18:41:06Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

+
+	//TestCase 3:  When there is no time to run PreStopHook
+	t.Run("PreStop-NoTimeToRun", func(t *testing.T) {
+		defer func() { gracePeriod = 30 }()


rather than overriding this, just define a local version

tallclair · 2017-08-01T18:41:32Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

+		gracePeriod = 0
+
+		m.killContainer(testPod, cID, "foo", "testKill", &gracePeriod)
+		//m.executePreStopHook(testPod, cID, &testPod.Spec.Containers[0], gracePeriod)


tallclair · 2017-08-01T18:42:32Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

+	//TestCase 4:  Post Start script
+	t.Run("PostStart-CmdExe", func(t *testing.T) {
+
+		//Fake all the things you need before trying to create a container


nit: add a space after //, here and everywhere.

tallclair · 2017-08-01T18:44:07Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

+			},
+		}
+
+		//Now try to create a container, which should inturn invoke PostStart Hook


nit: s/inturn/in turn/

tallclair · 2017-08-01T18:44:19Z

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

+		}
+
+	})
+	//TearDown


dhilipkumars · 2017-08-02T04:49:31Z

@tallclair Thanks for the review, could you PTAL now?

tallclair · 2017-08-02T22:10:08Z

Thanks for the clean up.

/lgtm

tallclair · 2017-08-02T22:11:06Z

/lgtm cancel

Please squash your commits.

Add UT for lifeCycle hooks

dhilipkumars · 2017-08-03T02:27:59Z

@tallclair Done.

dhilipkumars · 2017-08-03T03:25:18Z

/test pull-kubernetes-e2e-kops-aws

dhilipkumars · 2017-08-03T03:49:00Z

/retest

tallclair · 2017-08-03T18:16:15Z

/lgtm

tallclair · 2017-08-03T18:17:42Z

I've updated this with a release note, because it is user facing behavior.

/approve no-issue

k8s-github-robot · 2017-08-03T18:18:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhilipkumars, resouer, tallclair

Associated issue requirement bypassed by: tallclair

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubelet/OWNERS~~ [tallclair]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-08-03T23:43:35Z

Automatic merge from submit-queue (batch tested with PRs 50103, 49677, 49449, 43586, 48969)

k8s-github-robot assigned resouer and tallclair Jul 22, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 22, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 22, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 22, 2017

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. area/app-lifecycle labels Jul 22, 2017

dixudx reviewed Jul 24, 2017

View reviewed changes

dixudx reviewed Jul 26, 2017

View reviewed changes

dhilipkumars force-pushed the PreStopFix branch from bf31582 to 34b0e95 Compare July 26, 2017 02:39

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 28, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 29, 2017

resouer suggested changes Jul 29, 2017

View reviewed changes

dhilipkumars force-pushed the PreStopFix branch from fbf5fb1 to 7f63b34 Compare July 29, 2017 12:17

resouer approved these changes Jul 30, 2017

View reviewed changes

tallclair reviewed Aug 1, 2017

View reviewed changes

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2017

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

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

dhilipkumars force-pushed the PreStopFix branch from d773155 to 25bc76d Compare August 3, 2017 02:27

Do not try run preStop hook when the gracePeriod is 0

25bc76d

Add UT for lifeCycle hooks

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

tallclair added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 3, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2017

k8s-github-robot merged commit 6843ca5 into kubernetes:master Aug 3, 2017

liangjingtao11 mentioned this pull request Feb 21, 2024

force-delete pod execute prestop hook #123408

Open


		m.runner = lcHanlder

		//TestCase 1: Configured and works as expected

Do not try to run preStopHook when the gracePeriod is 0 #49449

Do not try to run preStopHook when the gracePeriod is 0 #49449

Conversation

dhilipkumars commented Jul 22, 2017 • edited by tallclair Loading

k8s-ci-robot commented Jul 22, 2017

dhilipkumars commented Jul 22, 2017

dhilipkumars commented Jul 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhilipkumars commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhilipkumars commented Jul 28, 2017

resouer commented Jul 29, 2017

resouer left a comment

Choose a reason for hiding this comment

dhilipkumars commented Jul 29, 2017

dhilipkumars commented Jul 29, 2017

dhilipkumars commented Jul 29, 2017

dhilipkumars commented Jul 29, 2017

dhilipkumars commented Jul 29, 2017

dhilipkumars commented Jul 29, 2017 • edited Loading

dhilipkumars commented Jul 30, 2017

resouer commented Jul 30, 2017

resouer left a comment

Choose a reason for hiding this comment

resouer commented Jul 31, 2017

dhilipkumars commented Jul 31, 2017

dhilipkumars commented Aug 1, 2017

dhilipkumars commented Aug 1, 2017

resouer commented Aug 1, 2017

dhilipkumars commented Aug 1, 2017

dhilipkumars commented Aug 1, 2017

dhilipkumars commented Aug 1, 2017

tallclair 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

dhilipkumars commented Aug 2, 2017

tallclair commented Aug 2, 2017

tallclair commented Aug 2, 2017

dhilipkumars commented Aug 3, 2017

dhilipkumars commented Aug 3, 2017

dhilipkumars commented Aug 3, 2017

tallclair commented Aug 3, 2017

tallclair commented Aug 3, 2017

k8s-github-robot commented Aug 3, 2017

k8s-github-robot commented Aug 3, 2017

dhilipkumars commented Jul 22, 2017 •

edited by tallclair

Loading

dhilipkumars commented Jul 29, 2017 •

edited

Loading