Allow exec prober to timeout under Docker #58510

louyihua · 2018-01-19T09:01:52Z

What this PR does / why we need it:
Currently, the timeout setting is only effective for HTTP/TCP prober.
This patch add an implementation for the exec prober to timeout.

Release note:

Make timeout setting available to the exec prober

k8s-ci-robot · 2018-01-19T09:02:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: louyihua
We suggest the following additional approver: dchen1107

Assign the PR to them by writing /assign @dchen1107 in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

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

m1093782566 · 2018-01-19T12:32:34Z

/ok-to-test

Currently, the timeout setting is only effective for HTTP/TCP prober. This patch add an implementation for the exec prober to timeout.

mtaufen · 2018-01-19T17:32:02Z

code mostly looks good
/assign @yujuhong

yujuhong · 2018-01-23T00:54:12Z

Why not setting the timeout to the context in the docker client? https://github.com/kubernetes/kubernetes/blob/v1.10.0-alpha.1/pkg/kubelet/dockershim/libdocker/kube_docker_client.go#L462

/assign @ncdc

louyihua · 2018-01-23T02:14:31Z

@yujuhong The exec prober does not use this interface, so this PR does not touch it.

yujuhong · 2018-01-23T17:51:28Z

I think it does use the libdocker.Interface. Check again?

louyihua · 2018-01-24T01:20:04Z

@yujuhong Checked again under OpenShift 3.9.
An exec probe starts with this stack trace:

 0  0x00000000035f88ab in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime.(*runtimeServiceClient).ExecSync
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime/api.pb.go:3738
 1  0x00000000041c54c3 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/remote.(*RemoteRuntimeService).ExecSync
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/remote/remote_runtime.go:330
 2  0x000000000434b33e in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/kuberuntime.instrumentedRuntimeService.ExecSync
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/kuberuntime/instrumented_services.go:147
 3  0x0000000004368ab0 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/kuberuntime.(*instrumentedRuntimeService).ExecSync
    at ./openshift-3.9.0/<autogenerated>:1
 4  0x0000000004354b8d in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/kuberuntime.(*kubeGenericRuntimeManager).RunInContainer
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/kuberuntime/kuberuntime_container.go:805
 5  0x000000000437bccf in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober.(*prober).newExecInContainer.func1
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober/prober.go:245
 6  0x0000000004378ec7 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober.execInContainer.CombinedOutput
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober/prober.go:254
 7  0x00000000043759a9 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/probe/exec.execProber.Probe
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/probe/exec/exec.go:37
 8  0x0000000004375dc0 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/probe/exec.(*execProber).Probe
    at ./openshift-3.9.0/<autogenerated>:1
 9  0x000000000437854c in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober.(*prober).runProbe
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober/prober.go:152
10  0x000000000437749e in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober.(*prober).runProbeWithRetries
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober/prober.go:129
11  0x00000000043766de in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober.(*prober).probe
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober/prober.go:98
12  0x000000000437b546 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober.(*worker).doProbe
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober/worker.go:201
13  0x000000000437ab68 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober.(*worker).run
    at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/prober/worker.go:120
14  0x0000000000460531 in runtime.goexit
    at /usr/local/go/src/runtime/asm_amd64.s:2337

At the top frame of the above stack trace, a gRPC call is invoked and the the control transfers to ExecInContainer defined in dockershim/exec.go as the following stack trace shows:

0  0x0000000004282211 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/dockershim.(*NativeExecHandler).ExecInContainer
   at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/dockershim/exec.go:63
1  0x0000000004280895 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/dockershim.(*streamingRuntime).exec
   at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/dockershim/docker_streaming.go:58
2  0x0000000004280d01 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/dockershim.(*dockerService).ExecSync
   at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/dockershim/docker_streaming.go:81
3  0x000000000431d64c in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/dockershim/remote.(*dockerService).ExecSync
   at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/dockershim/remote/docker_service.go:158
4  0x00000000035fbce6 in github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime._RuntimeService_ExecSync_Handler
   at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime/api.pb.go:4121
5  0x0000000001f8c97b in github.com/openshift/origin/vendor/google.golang.org/grpc.(*Server).processUnaryRPC
   at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/google.golang.org/grpc/server.go:753
6  0x0000000001f901c2 in github.com/openshift/origin/vendor/google.golang.org/grpc.(*Server).handleStream
   at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/google.golang.org/grpc/server.go:955
7  0x0000000001f9607f in github.com/openshift/origin/vendor/google.golang.org/grpc.(*Server).serveStreams.func1.1
   at ./openshift-3.9.0/_output/local/go/src/github.com/openshift/origin/vendor/google.golang.org/grpc/server.go:517
8  0x0000000000460531 in runtime.goexit
   at /usr/local/go/src/runtime/asm_amd64.s:2337

Neither of above stack traces touchs the libdocker package.

yujuhong · 2018-01-24T01:28:40Z

https://github.com/kubernetes/kubernetes/blob/v1.10.0-alpha.1/pkg/kubelet/dockershim/exec.go#L61

func (*NativeExecHandler) ExecInContainer(client libdocker.Interface, ...) {

louyihua · 2018-01-30T14:11:21Z

Another PR that modifies the libdocker.Interface is proposed in #58925.

yujuhong · 2018-02-06T23:55:05Z

I'd like @ncdc's input on what's the best place to do the timeout check.

ncdc · 2018-02-07T14:52:04Z

@yujuhong I honestly have a hard time keeping track of what the code path is, given CRI and dockershim and libdocker and whatnot. I'd like to help out, but maybe you could give me an overview of how things work in the kubelet these days?

yujuhong · 2018-02-07T19:00:50Z

@yujuhong I honestly have a hard time keeping track of what the code path is, given CRI and dockershim and libdocker and whatnot. I'd like to help out, but maybe you could give me an overview of how things work in the kubelet these days?

@ncdc this part of the code actually hasn't changed much, although being moved around a couple of times and harder to trace now :-)
I think the main question is where to handle the timeout. This PR does it in the ExecInContainer(), while the other PR at the lower level StartExec(). Initially I thought we could leverage the context timeout used in docker operations more by doing it a the lower level, but it doesn't seem to be the case and the change is even larger. Given that ExecInContainer is the only caller of StartExec, maybe we should just add the code here (i.e., this PR).

dims · 2018-02-13T20:17:01Z

/unassign @dims

louyihua · 2018-03-07T02:12:36Z

@ncdc any comments to this PR?

ncdc · 2018-03-07T14:44:40Z

My apologies for the delayed response.

I think there are 2 possible courses of action here.

Control the timeout in ExecInContainer, killing the exec'd pid when you hit the timeout (this PR)
Control the timeout in StartExec, killing the exec'd pid inside StartExec when you hit the timeout, so that StartExec fully owns the management of the exec'd process. I don't think the other PR is ideal as-is, because it splits the ownership of the timeout and the killing of the process between StartExec and ExecInContainer.

From a design purity standpoint, I think option 2 makes more sense, since it allows all current and future callers of StartExec to have timeout support.

Would you agree?

@yujuhong I will, however, defer to sig-node to make the final decision. I think either this PR or a re-worked #58925 that matches my option 2 would be acceptable.

louyihua · 2018-03-07T15:03:27Z

@ncdc I agree

ncdc · 2018-05-10T15:30:24Z

Can we close this since it looks like #58925 is the way we want to go?

louyihua · 2018-05-10T15:35:44Z

@ncdc OK, close this

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 19, 2018

k8s-github-robot assigned mtaufen and dims Jan 19, 2018

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 19, 2018

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 Jan 19, 2018

Allow exec prober to timeout under Docker

bed47b0

Currently, the timeout setting is only effective for HTTP/TCP prober. This patch add an implementation for the exec prober to timeout.

louyihua force-pushed the allow-exec-timeout branch from f859994 to bed47b0 Compare January 19, 2018 13:28

k8s-ci-robot assigned yujuhong Jan 19, 2018

k8s-ci-robot assigned ncdc Jan 23, 2018

louyihua mentioned this pull request Jan 28, 2018

Add timeout option for docker's exec operation #58925

Closed

mtaufen removed their assignment Feb 7, 2018

k8s-ci-robot unassigned dims Feb 13, 2018

louyihua closed this May 10, 2018

SergeyKanzhelev mentioned this pull request Aug 21, 2020

kubelet: respect exec probe timeouts #94115

Merged

Allow exec prober to timeout under Docker #58510

Allow exec prober to timeout under Docker #58510

Uh oh!

Conversation

louyihua commented Jan 19, 2018

Uh oh!

k8s-ci-robot commented Jan 19, 2018

Uh oh!

m1093782566 commented Jan 19, 2018

Uh oh!

mtaufen commented Jan 19, 2018

Uh oh!

yujuhong commented Jan 23, 2018

Uh oh!

louyihua commented Jan 23, 2018

Uh oh!

yujuhong commented Jan 23, 2018

Uh oh!

louyihua commented Jan 24, 2018

Uh oh!

yujuhong commented Jan 24, 2018

Uh oh!

louyihua commented Jan 30, 2018

Uh oh!

yujuhong commented Feb 6, 2018

Uh oh!

ncdc commented Feb 7, 2018

Uh oh!

yujuhong commented Feb 7, 2018

Uh oh!

dims commented Feb 13, 2018

Uh oh!

louyihua commented Mar 7, 2018

Uh oh!

ncdc commented Mar 7, 2018

Uh oh!

louyihua commented Mar 7, 2018

Uh oh!

ncdc commented May 10, 2018

Uh oh!

louyihua commented May 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants