Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: use cases for kubectl attach? #23335

Closed
philips opened this issue Mar 22, 2016 · 35 comments
Closed

API: use cases for kubectl attach? #23335

philips opened this issue Mar 22, 2016 · 35 comments
Labels
area/api Indicates an issue on api area. area/rkt priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/node Categorizes an issue or PR as relevant to SIG Node.

Comments

@philips
Copy link
Contributor

philips commented Mar 22, 2016

In the rkt container engine integration we are trying to get full feature parity with the existing k8s API. We are very close.

The most recent snag we hit though is that rkt does not support the concept of "attach". For those who aren't familiar see the help output of kubectl attach:

$ kubectl attach --help
Attach to a process that is already running inside an existing container.

Usage:
  kubectl attach POD -c CONTAINER [flags]

Examples:
# Get output from running pod 123456-7890, using the first container by default
$ kubectl attach 123456-7890

# Get output from ruby-container from pod 123456-7890
$ kubectl attach 123456-7890 -c ruby-container

# Switch to raw terminal mode, sends stdin to 'bash' in ruby-container from pod 123456-7890
# and sends stdout/stderr from 'bash' back to the client
$ kubectl attach 123456-7890 -c ruby-container -i -t

The reason rkt doesn't support this is quite practical: for applications that are long running we attach the applications stdout/stdin to a logging daemon without a man-in-the-middle. And the stdin is attached to stdin because this is sort of the accepted default for most long-running processes.

Now, rkt does support interactive applications with kubectl exec. But, you chose to do interactive ahead of time which means we skip attaching stdout/stdin/stderr to the log daemon and attach it to the calling process instead.

So, attach is this weird middle ground between kubectl logs and kubectl exec which is rather odd. I really don't understand the use case and the k8s issue introducing attach isn't super helpful either: #1521

I want to understand what are the use cases for attach and if we can do one of the following things:

  1. Simply not support it in the rkt integration because the use cases are so narrow and the complexity is high to implement it

  2. Remove attach from the next revision of the API, I feel like nearly all use cases of debugging can be handled with exec or even a proper session inside of the container created with sshd, etc.

  3. Enumerate the compelling use cases for attach so we have some concrete use cases to justify the complexity of figuring this out inside of rkt

rkt upstream issue: rkt/rkt#1799

@philips
Copy link
Contributor Author

philips commented Mar 22, 2016

cc'ing the relevant people from the historical discussions here: @davidopp @bgrant0607 @vishh @yifan-gu @spotter @dchen1107

@dchen1107
Copy link
Member

@philips, kubectl attach is introduced mainly for debuggability. I agreed with you that most, if not all use cases can be handled through different mechanism(s). I accepted that feature initially without much questions because I treated it as a usability level compatible with docker. :-) cc/ @ncdc who might know the real use cases for this one.

I am ok with 1) for now unless someone can come up a real use case.

@dchen1107 dchen1107 added area/api Indicates an issue on api area. sig/node Categorizes an issue or PR as relevant to SIG Node. team/api area/rkt priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Mar 22, 2016
@bgrant0607
Copy link
Member

cc @brendandburns

@ncdc
Copy link
Member

ncdc commented Mar 22, 2016

cc @smarterclayton as I believe we use attach as part of kubectl run

@yifan-gu
Copy link
Contributor

cc @smarterclayton as I believe we use attach as part of kubectl run

IIRC kubectl run essentially creates/starts a container then attaches to it.

@ncdc
Copy link
Member

ncdc commented Mar 22, 2016

Yes, that is the flow in the Docker case for kubectl run - create a container, start it, then attach to its process. If you can do that in rkt some other way than "attach", that works for me.

@bgrant0607
Copy link
Member

We definitely need kubectl run -ti ... to work.

@smarterclayton
Copy link
Contributor

We use attach to stream build contents to build pods.

On Thu, Mar 24, 2016 at 2:14 PM, Brian Grant notifications@github.com
wrote:

We definitely need kubectl run -ti ... to work.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#23335 (comment)

@philips
Copy link
Contributor Author

philips commented Mar 25, 2016

@bgrant0607 run works
@smarterclayton why not use logs?

@ncdc It is the ability to "re-attach" and also being able to attach stdin to any arbitrary process ever that is troublesome for us. I just don't get the use case and feel like it is something that is a candidate for removal because implementing it is annoying and incurs cost even when not in use.

@yifan-gu
Copy link
Contributor

We use attach to stream build contents to build pods.

@smarterclayton Are you saying you need to stream contents to the stdin of the running pod?

@ncdc
Copy link
Member

ncdc commented Mar 25, 2016

@philips I think that the ability to reattach an arbitrary number of times is a limited use case, specific to debugging. I would be fine deprecating and eventually removing this ability. However, it's not currently that simple, because:

@yifan-gu yes, we start a pod that waits to receive data via stdin. We then use attach to connect a client to the running process in the pod, and the client streams data to the process's stdin.

@smarterclayton
Copy link
Contributor

smarterclayton commented Mar 25, 2016 via email

@smarterclayton
Copy link
Contributor

smarterclayton commented Mar 25, 2016 via email

@yifan-gu
Copy link
Contributor

@philips

  • Run without -i
$ kubectl run busybox --restart=Never --image=busybox --command sleep 1000
job "busybox" created
$ kubectl get pods
NAME            READY     STATUS    RESTARTS   AGE
busybox-h0kis   1/1       Running   0          13s
  • Run with -i
$ kubectl run busybox1 -i --restart=Never --image=busybox --command sleep 1000
Waiting for pod default/busybox1-cu06m to be running, status is Pending, pod ready: false
Error attaching, falling back to logs: error executing remote command: Error executing command in container: unimplemented
yifan@jenkins-slave-kubernetes-e2e:~/kubernetes$ kubectl get pods
NAME             READY     STATUS    RESTARTS   AGE
busybox-h0kis    1/1       Running   0          1m
busybox1-cu06m   1/1       Running   0          13s

It cannot attach the pod, but the pod is still running.

Besides, if the pod completes quickly after start, run with -i will succeed, since the kubelet will know the pod is not running and so it won't attach to it. Instead, it will just return the logs:

$ kubectl run busybox2 -i --restart=Never --image=busybox --command echo "hello world"
hello world

@dchen1107
Copy link
Member

Looks like we don't have the issue to provide kubectl run -i through rkt run -i.

How about kubectl attach?

@yifan-gu
Copy link
Contributor

@dchen1107 We can do through rkt run --interactive but I don't know if we can do that when the rkt run is running as a systemd service.
Or we do something special for kubectl run -i: When runs interactively, we invoke rkt run --interactive directly instead from systemd service.

But I am not sure what if we detach the container after kubectl run -i, (in fact, I don't know whether we are able to detach from a docker container that's created by kubectl run -i today?)

@dchen1107
Copy link
Member

Using CTRL-C can detach the container.

@yifan-gu
Copy link
Contributor

yifan-gu commented Apr 1, 2016

Using CTRL-C can detach the container.

@dchen1107 I tried that after kubectl run -i --tty busybox --image=busybox --restart=Never -- /bin/sleep 1000 doesn't work for me for docker as runtime.

@yifan-gu
Copy link
Contributor

yifan-gu commented Apr 1, 2016

Ok, remove the --tty, then CTRL-C detaches the container

@vishh
Copy link
Contributor

vishh commented Apr 1, 2016 via email

@smarterclayton
Copy link
Contributor

smarterclayton commented Apr 1, 2016 via email

@ncdc
Copy link
Member

ncdc commented Apr 1, 2016

@smarterclayton reviewing #16451 is a good start. Once that's in, we can move on to resize, etc. I'd also like to start looking more into deprecating spdy and replacing with http/2.

@yifan-gu
Copy link
Contributor

FYI, related e2e tests that testing the kubectl attach functionality in the default gce e2e testing suit are:

[k8s.io] Kubectl client [k8s.io] Kubectl run --rm job should create a job from an image, then delete the job [Conformance]
[k8s.io] Kubectl client [k8s.io] Simple pod should support inline execution and attach

@resouer
Copy link
Contributor

resouer commented Jun 3, 2016

cc @feiskyer

@pskrzyns
Copy link

cc @kubernetes/sig-rktnetes

@tmrts
Copy link
Contributor

tmrts commented Jul 27, 2016

@feiskyer what's the plan for this feature. Is there any current PR/discussion going around this that I'm missing?

@ncdc
Copy link
Member

ncdc commented Jul 27, 2016

OpenShift still needs it to support our image build functionality. @bparees @smarterclayton have we done any design work to replace attach with another mechanism?

@bparees
Copy link
Contributor

bparees commented Jul 27, 2016

@smarterclayton
Copy link
Contributor

smarterclayton commented Jul 27, 2016 via email

@feiskyer
Copy link
Member

@tmrts As discussed in sig-node and @yujuhong, I'm planning to add AttachContainer() interface to new runtime API after #28396.

@yujuhong
Copy link
Contributor

Just to summarize, we've had a discussion on sig-node slack channel about this feature.
@euank pointed out that there needs to be a daemon that drains the fifo when no process is attached to it. This is how docker handles it as well today.
"page 12 of http://www.slideshare.net/Docker/containerd-building-a-container-supervisor-by-michael-crosby it's /proc/sys/fs/pipe-max-size; that's the talk where he talked about it"

IIRC, the conclusion is that we should try implementing this feature and share code between rkt and the oci integration if possible.

@vishh
Copy link
Contributor

vishh commented Jul 28, 2016

IIUC, containerd uses pipes for stdin & stdout/stderr. Can we avoid that and use pipes just for stdin? That way, there is no necessity for a daemon to drain data from the pipes.

@yifan-gu
Copy link
Contributor

IIUC, containerd uses pipes for stdin & stdout/stderr. Can we avoid that and use pipes just for stdin? That way, there is no necessity for a daemon to drain data from the pipes.

This is an interesting idea. To be clear, are you suggesting the stdout/stderr come from the log?

@vishh
Copy link
Contributor

vishh commented Aug 15, 2016

@yifan-gu Yeah. instead of using pipes, would it be possible to use regular files for stdout & stderr?

@philips
Copy link
Contributor Author

philips commented Sep 15, 2016

OK, so I think we have moved past the point of considering whether we can not implement attach. It is part of the CRI and so we will work to get it implemented there. @lucab and @timstclair are working on that.

@philips philips closed this as completed Sep 15, 2016
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this issue Jul 8, 2019
…herry-pick-23332-to-release-3.11

[release-3.11] UPSTREAM: 1679612: kubelet_stats: fix potential e2e crash dereferencing CPU

Origin-commit: 01dd9628b3bb0d9f310b5faaab63b36797242585
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area. area/rkt priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/node Categorizes an issue or PR as relevant to SIG Node.
Projects
None yet
Development

No branches or pull requests