Set ExecSync timeout in liveness prober. #50176

Random-Liu · 2017-08-04T22:31:49Z

Although Dockershim doesn't actually support ExecSync timeout (see here), we should set the timeout, so that the other runtime which supports the timeout could work properly.

Fixes #50389.

/cc @yujuhong @timstclair @feiskyer

feiskyer

LGTM

tallclair · 2017-08-04T23:12:43Z

pkg/kubelet/remote/remote_runtime.go

-		// Do not set timeout when timeout is 0.
-		ctx, cancel = getContextWithCancel()
+	// Do not set timeout when timeout is 0.
+	ctx, cancel := getContextWithCancel()


getContextWithCancel does a bunch of stuff. We should avoid calling it if it's not going to be used (i.e. move this into an else block)

Hmm, not much, it just returns context.WithCancel(context.Background())

Yes, but context.WithCancel potentially brings up a go routine. It might be trivial, but it's better practice not to do unecessary work when it's easily avoidable..

xiang90 · 2017-08-05T15:25:31Z

pkg/kubelet/remote/remote_runtime.go

+	var cancel context.CancelFunc
+	if timeout != 0 {
+		// Use timeout + default timeout (2 minutes) as timeout.
+		ctx, cancel = getContextWithTimeout(r.timeout + time.Duration(timeout)*time.Second)


why do you multiple the user pass in timeout with time.Second?

Not sure this is the effect you want here, see https://play.golang.org/p/4hJFFXqoga

we probably need a unit test for this if this is indeed an issue.

Good catch. It looks like timeout is already a Duration?

Copied the code from StopContainer. Yeah, there is a mismatch. timeout in StopContainer is int64.

Will update this.

@Random-Liu Is it possible to add a unit test for this?

tallclair · 2017-08-09T22:34:02Z

pkg/kubelet/remote/remote_runtime.go

+	var cancel context.CancelFunc
+	if timeout != 0 {
+		// Use timeout + default timeout (2 minutes) as timeout.
+		ctx, cancel = getContextWithTimeout(r.timeout + timeout*time.Second)


Why are you multiplying the timeout by time.Second? That should have been done by the caller. I'd expect it to just be passed through here...

tallclair · 2017-08-09T22:36:16Z

pkg/kubelet/remote/remote_runtime.go

+	var ctx context.Context
+	var cancel context.CancelFunc
+	if timeout != 0 {
+		// Use timeout + default timeout (2 minutes) as timeout.


It seems a little odd to add the timeout with the default here? Can you explain the reasoning?

Same with stop, we want to leave some time for the runtime to do cleanup.
Runtime can't finish running immediately when timeout exceed.

tallclair · 2017-08-09T22:36:27Z

pkg/kubelet/remote/remote_runtime.go

+		// Use timeout + default timeout (2 minutes) as timeout.
+		ctx, cancel = getContextWithTimeout(r.timeout + timeout*time.Second)
+	} else {
 		ctx, cancel = getContextWithCancel()


Should this use the default timeout?

No. We should not set any timeout by default because exec may take long time.

Random-Liu · 2017-08-17T22:11:16Z

@yujuhong Sorry, I forgot to push my local fix. Pushed newest fix.

yujuhong · 2017-08-17T22:17:04Z

/lgtm

k8s-github-robot · 2017-08-17T22:17:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Random-Liu, yujuhong

Associated issue: 50389

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

Details

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

~~pkg/kubelet/OWNERS~~ [Random-Liu,yujuhong]

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

Random-Liu · 2017-08-17T23:36:42Z

@yujuhong Thanks for reviewing!

k8s-github-robot · 2017-08-18T01:12:21Z

Automatic merge from submit-queue (batch tested with PRs 50536, 50809, 50220, 50399, 50176)

k8s-ci-robot requested review from feiskyer, timstclair and yujuhong August 4, 2017 22:31

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

Random-Liu assigned yujuhong and tallclair Aug 4, 2017

feiskyer approved these changes Aug 4, 2017

View reviewed changes

tallclair reviewed Aug 4, 2017

View reviewed changes

Random-Liu force-pushed the set-exec-timeout branch from 9147731 to 7e06e55 Compare August 5, 2017 00:47

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Aug 5, 2017

Random-Liu added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 5, 2017

xiang90 reviewed Aug 5, 2017

View reviewed changes

Random-Liu force-pushed the set-exec-timeout branch from 7e06e55 to 7267576 Compare August 9, 2017 18:00

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

tallclair reviewed Aug 9, 2017

View reviewed changes

Set ExecSync timeout in liveness prober.

ef29b83

Random-Liu force-pushed the set-exec-timeout branch from 7267576 to ef29b83 Compare August 17, 2017 22:10

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

Random-Liu mentioned this pull request Aug 17, 2017

Update Kubernetes version to fix a bug containerd/cri#141

Closed

k8s-github-robot merged commit d490e2c into kubernetes:master Aug 18, 2017

Random-Liu deleted the set-exec-timeout branch August 25, 2017 05:58

dims mentioned this pull request Aug 20, 2020

kubelet: respect exec probe timeouts #94115

Merged

Set ExecSync timeout in liveness prober. #50176

Set ExecSync timeout in liveness prober. #50176

Conversation

Random-Liu commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feiskyer left a comment

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!

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!

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!

Random-Liu commented Aug 17, 2017

Uh oh!

yujuhong commented Aug 17, 2017

Uh oh!

k8s-github-robot commented Aug 17, 2017

Uh oh!

Random-Liu commented Aug 17, 2017

Uh oh!

k8s-github-robot commented Aug 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Random-Liu commented Aug 4, 2017 •

edited

Loading