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

CRI: Handle streaming requests (attach/exec/port-forward) #29579

Closed
yujuhong opened this issue Jul 25, 2016 · 84 comments

Comments

@yujuhong
Copy link
Member

commented Jul 25, 2016

I re-titled the issue since the discussion has naturally shifted to the general design and implementation of streaming requests.
Please read the summary doc before joining the discussion. Thanks!
https://docs.google.com/document/d/1MreuHzNvkBW6q7o_zehm1CBOBof3shbtMTGtUpjpRmY/edit?usp=sharing

------ The original post -----
#25273 added terminal resizing for run/attach/exec. We need to support this with the new container runtime interface.

Current implementations of terminal resizing:
docker (both nsenter and docker exec): https://github.com/kubernetes/kubernetes/blob/3b21a9901bcd48bb452d3bf1a0cddc90dae142c4/pkg/kubelet/dockertools/exec.go
rkt:

func (r *Runtime) ExecInContainer(containerID kubecontainer.ContainerID, cmd []string, stdin io.Reader, stdout, stderr io.WriteCloser, tty bool, resize <-chan term.Size) error {

The new container interface/api (

) has only a Exec method with no resizing support.

@ncdc, any suggestion of how we could achieve this? I think it may make sense to add this to the interface and push it down to the runtime.

/cc @kubernetes/sig-node

@ncdc

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

It has to be pushed down to the runtime given that the docker exec
implementation is different from rkt.

On Monday, July 25, 2016, Yu-Ju Hong notifications@github.com wrote:

#25273 #25273 added
terminal resizing for run/attach/exec. We need to support this with the new
container runtime interface.

Current implementations of terminal resizing:
docker (both nsenter and docker exec):
https://github.com/kubernetes/kubernetes/blob/3b21a9901bcd48bb452d3bf1a0cddc90dae142c4/pkg/kubelet/dockertools/exec.go
rkt:

func (r *Runtime) ExecInContainer(containerID kubecontainer.ContainerID, cmd []string, stdin io.Reader, stdout, stderr io.WriteCloser, tty bool, resize <-chan term.Size) error {

The new container interface/api (

)
has only a Exec method with no resizing support.

@ncdc https://github.com/ncdc, any suggestion of how we could achieve
this? I think it may make sense to add this to the interface and push it
down to the runtime.

/cc @kubernetes/sig-node
https://github.com/orgs/kubernetes/teams/sig-node


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#29579, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AAABYiIKAfW7i3_hqO_3FwxqprvJuuAYks5qZTlvgaJpZM4JUnQ0
.

@euank

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

The only other option I can think of is the interface being a pty-path, in which case the resizes would not be pushed down to the runtime, though initial pty creation would be.

@yujuhong

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2016

The only other option I can think of is the interface being a pty-path, in which case the resizes would not be pushed down to the runtime, though initial pty creation would be.

Hm... I don't think this'd work with hyper. /cc @feiskyer

The implementation that uses docker exec today does:

  • call docker CreateExec
  • start the HandleResizing goroutine with docker ResizeExecTTY
  • call docker StartExec.

That means to support terminal resizing in the runtime interface, we'll need to replace the current Exec with CreateExec and StartExec, while also adding ResizeExecTTY. Is that correct? @ncdc @euank
I am not too keen on the change, but I also don't see other alternatives. Any suggestions?

@feiskyer

This comment has been minimized.

Copy link
Member

commented Jul 29, 2016

Hyper supports same APIs like docker: CreateExec, ResizeTTY and StartExec. The problem is could we suppose all runtimes support these three apis? Is it work for rkt and oci-runtime?

The only other option I can think of is the interface being a pty-path, in which case the resizes would not be pushed down to the runtime, though initial pty creation would be.

Could we just add a new terminal resize api to new runtime API, e.g. ResizeTTY, and let runtime itself decides how to implements the tty resizing?

@ncdc

This comment has been minimized.

Copy link
Member

commented Jul 29, 2016

I'm traveling today - please ping me on Monday so I don't forget to reply.
Thanks!

On Thursday, July 28, 2016, Yu-Ju Hong notifications@github.com wrote:

The only other option I can think of is the interface being a pty-path, in
which case the resizes would not be pushed down to the runtime, though
initial pty creation would be.

Hm... I don't think this'd work with hyper. /cc @feiskyer
https://github.com/feiskyer

The implementation that uses docker exec today does:

  • call docker CreateExec
  • start the HandleResizing goroutine with docker ResizeExecTTY
  • call docker StartExec.

That means to support terminal resizing in the runtime interface, we'll
need to replace the current Exec with CreateExec and StartExec, while
also adding ResizeExecTTY. Is that correct? @ncdc
https://github.com/ncdc @euank https://github.com/euank
I am not too keen on the change, but I also don't see other alternatives.
Any suggestions?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#29579 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABYiZ9ImG5PeI28CTlaA3V47U-mWmTks5qaVRpgaJpZM4JUnQ0
.

@yujuhong

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2016

I'm traveling today - please ping me on Monday so I don't forget to reply.
Thanks!

@ncdc, to help with tracking, let's assign this to you until you reply.

@yujuhong

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2016

As instructed, @ncdc, here is a friendly ping to remind you to update the issue!

@euank

This comment has been minimized.

Copy link
Member

commented Aug 1, 2016

The interface I was trying to suggest is an alternative is the following:

  1. kubelet calls Exec(interactive=true,initialWidth=x,initialHeight=y) or maybe ExecInteractive or whatever
    1a) runtime creates a pty and connects the slave end to the runtime somehow (whether by controlling it and proxying through to a CreateExec, StartExec, and HandleResizing or by just connecting to an existing pty or constructing one from some pipes it has laying around)
    1b) runtime returns the path to the pty on the host
  2. kubelet opens the pty to read, write, and resize
    2b) if the pty was constructed by the runtime, the runtime handles each of these and translates them appropriately
  3. kubelet closes pty

Any runtime that can actually support attach in any manner should be able to construct a pty, even if it has to act as the slave side of it and translate it back into runtime api calls on the other end.

That feels like a slightly cleaner interface than create/resize/exec.

@ncdc

This comment has been minimized.

Copy link
Member

commented Aug 2, 2016

@euank how would a call in the kubelet to resize the pty translate to a remote call to Docker for ResizeExecTTY?

@yujuhong

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2016

@euank how would a call in the kubelet to resize the pty translate to a remote call to Docker for ResizeExecTTY?

+1. I am not sure how this would work.

@euank

This comment has been minimized.

Copy link
Member

commented Aug 3, 2016

@ncdc in this case the "runtime" would be the docker-shim package within the kubelet process, which would hold the slave end of the pty and then execute the ResizeExecTTY call against docker.

@ncdc

This comment has been minimized.

Copy link
Member

commented Aug 3, 2016

@euank ok, trying to reason through this... so kubectl receives SIGWINCH and sends a message to the Kubelet that the container's terminal needs to be resized to 100x50. Does the Kubelet then call term.SetSize(ptyFD, 100x50)? How can we, in Go, react to the term.SetSize call and a) for Docker, call ResizeExecTTY, b) for rkt, do whatever is appropriate for rkt, etc?

cc @mrunalp

@yujuhong

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2016

@ncdc @euank @feiskyer can we just add a resize stream in attach/exec in CRI, and let kubelet proxy the resize requests to the runtime? That way, runtime can handle the resize stream however it likes. Does this make sense?

@feiskyer

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

can we just add a resize stream in attach/exec in CRI, and let kubelet proxy the resize requests to the runtime? That way, runtime can handle the resize stream however it likes. Does this make sense?

+1. (Same idea as #29579 (comment).)

@ncdc

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

I need to look at the CRI in more detail when I'm at my desk but that
sounds reasonable.

On Friday, August 5, 2016, Pengfei Ni notifications@github.com wrote:

can we just add a resize stream in attach/exec in CRI, and let kubelet
proxy the resize requests to the runtime? That way, runtime can handle the
resize stream however it likes. Does this make sense?

+1. (Same idea as #29579 (comment)
#29579 (comment)
.)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#29579 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABYtWllXUM9IXjIBNfmOQTKbZDVatUks5qc9rPgaJpZM4JUnQ0
.

@yujuhong

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2016

+1. (Same idea as #29579 (comment).)

What I suggested is a little different than your comment, which was to add a ResizeTTY call :)
I am suggesting that we add a new "resize" stream in the Exec call (in additional to stdin stream), so that kubelet can just forward the resize request directly. That way, we don't have to split Exec to CreateExec and StartExec to get the exec ID that can be used to call ResizeTTY. Of course, the runtime will need to spawn a thread reading the resize stream and react (via calling ResizeTTY or something else) to it.

@yujuhong

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2016

What I suggested is a little different than your comment, which was to add a ResizeTTY call :)
I am suggesting that we add a new "resize" stream in the Exec call (in additional to stdin stream), so that kubelet can just forward the resize request directly. That way, we don't have to split Exec to CreateExec and StartExec to get the exec ID that can be used to call ResizeTTY. Of course, the runtime will need to spawn a thread reading the resize stream and react (via calling ResizeTTY or something else) to it.

It could be as simple as yujuhong@355a9ce for CRI. The implementation is another story.

@feiskyer

This comment has been minimized.

Copy link
Member

commented Aug 9, 2016

@yujuhong I see. This could keep the CRI simple.

@yujuhong

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

@ncdc @euank any thoughts on yujuhong@355a9ce?

@ncdc

This comment has been minimized.

Copy link
Member

commented Aug 9, 2016

@yujuhong that seems reasonable. I don't have much familiarity with protobuf - will we be able to treat Stdin and Resize as io.Readers?

@yujuhong

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

As discussed in the sig-node meeting today, how stdin/stdout/stderr streams are handled in the proto file have some issues. We need to fix that.

/cc @smarterclayton @feiskyer

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2016

I commented on the old PR, but we know for these we'll need a multiplexed multi-channel stream with non-blocking. That's http2 - I'm ok saying that's our new model, but we'll have to deal with the fact that we want to have the API server be able to connect to the directly (kubelet can't be a network bandwidth hog). So exec flow is really different than all other calls - ideally it's:

  1. apiserver talks to kubelet
  2. Kubelet ensures that CRI knows an exec session is incoming, and returns a URL that the kubelet can pass back to the APIserver
  3. APIServer direct connects to the info the kubelet passed (which points to the exec stub in container)
  4. APIServer talks HTTP2 to the exec stub.
@yujuhong

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

@smarterclayton I don't have prior experience with protobuf/grpc, so here what I've read so far. grpc is built on http2, but it streams messages (not arbitrary streams).
We can emulate a stream by sending out a chunk of data encapsulated in a message.
E.g.,

message DataChunk {
    bytes data = 1;
}

I think kubelet should also be able to read messages, and demultiplex the fields in the message and send them back to apiserver via different streams. There will definitely incur overhead on the kubelet's side but it should be doable(?). What do you think about it?

message ExecResponse {
    bytes stdout = 1;
    bytes stderr = 2;
}
@feiskyer

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

That sounds good. We can simply timebox a proof-of-concept, and if that doesn't seem on-track, fall back on (1) which seems easiest.

Agree with @thockin. Need a POC of option(3) first before making a final decision (even for the alpha API).

Looking forward to @timstclair's progress on this.

@lucab

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2016

While implementing the attach/exec part for rkt, I had some concerns related to streams/tty/VMs interaction that I summarized here #32869. In the context of @timstclair work on this, I'd gladly have a more simpler abstraction which doesn't expose single streams for out and err across host/pod border.

@ncdc

This comment has been minimized.

Copy link
Member

commented Sep 23, 2016

FYI, one other thing we may need to consider is whether or not attach should include or exclude the container's logs. For example, if you run a container that prints some lines to stdout, docker attach mycontainer will not show anything printed to stdout prior to attaching. Any new output printed to stdout will show up in the attach session. There is, however, a feature that the docker daemon supports where, when attaching, you can get any output previously logged. We actually used to use this feature when we were still using go-dockerclient, but it went away when we switched to docker's engine-api, because it doesn't support it. I have created moby/moby#26718 to add Logs bool to ContainerAttachOptions so we can restore this functionality if docker merges the PR and if we decide we want to display logs when attaching.

@timstclair

This comment has been minimized.

Copy link

commented Oct 13, 2016

Design doc: https://docs.google.com/document/d/1OE_QoInPlVCK9rMAx9aybRmgFiVjHpJCHI9LrfdNM_s/edit?usp=sharing

There shouldn't be any surprises there, it's the same design we had previously discussed (option 3), but with more details filled in.

@euank

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

Well, we've started implementing from that design-doc. I think we can close this issue and carry on in others.

@timstclair timstclair changed the title CRI: How to handle streaming requests (attach/exec/port-forward)? CRI: Handle streaming requests (attach/exec/port-forward) Nov 2, 2016

@timstclair

This comment has been minimized.

Copy link

commented Nov 2, 2016

I'm still using this as the umbrella issue. I changed the title to not be a question.

@euank

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

If this is meant to be an umbrella issue we should link issues to it / link to issues from it.

Neither of those has happened for the many recent PRs related to this implementation, so it felt more like a 🌂 than ☂️

@timstclair

This comment has been minimized.

Copy link

commented Nov 2, 2016

Yeah, I should have been linking PRs to an issue. I just back-filled. ☂️ 👍

@resouer resouer referenced this issue Nov 4, 2016
0 of 10 tasks complete

k8s-github-robot pushed a commit that referenced this issue Nov 4, 2016

Kubernetes Submit Queue
Merge pull request #36020 from timstclair/klet-stream
Automatic merge from submit-queue

Separate Direct and Indirect streaming paths, implement indirect path for CRI

This PR refactors the `pkg/kubelet/container.Runtime` interface to remove the `ExecInContainer`, `PortForward` and `AttachContainer` methods. Instead, those methods are part of the `DirectStreamingRuntime` interface which all "legacy" runtimes implement. I also added an `IndirectStreamingRuntime` which handles the redirect path and is implemented by CRI runtimes. To control the size of this PR, I did not fully setup the indirect streaming path for the dockershim, so I left legacy path behind.

Most of this PR is moving & renaming associated with the refactoring. To understand the functional changes, I suggest tracing the code from `getExec` in `pkg/kubelet/server/server.go`, which calls `GetExec` in `pkg/kubelet/kubelet_pods.go` to determine whether to follow the direct or indirect path.

For #29579

/cc @kubernetes/sig-node

k8s-github-robot pushed a commit that referenced this issue Nov 5, 2016

Kubernetes Submit Queue
Merge pull request #34987 from timstclair/redirect
Automatic merge from submit-queue

Handle redirects in apiserver proxy handler

Overview:
1. Peek at the HTTP response from the proxied backend
2. If it is a redirect response (302/3), redo the request to the redirect location
3. If it's not a redirect, forward the response to the client and then set up the proxy as before

This change is required for implementing streaming requests in the Container Runtime Interface (CRI). See [design](https://docs.google.com/document/d/1OE_QoInPlVCK9rMAx9aybRmgFiVjHpJCHI9LrfdNM_s/edit).

For #29579

/cc @yujuhong

k8s-github-robot pushed a commit that referenced this issue Nov 10, 2016

Kubernetes Submit Queue
Merge pull request #36253 from timstclair/klet-stream-config-pr
Automatic merge from submit-queue

Use indirect streaming path for remote CRI shim

Last step for #29579

- Wire through the remote indirect streaming methods in the docker remote shim
- Add the docker streaming server as a handler at `<node>:10250/cri/{exec,attach,portforward}`
- Disable legacy streaming for dockershim

Note: This requires PR #34987 to work.

Tested manually on an E2E cluster.

/cc @euank @feiskyer @kubernetes/sig-node
@timstclair

This comment has been minimized.

Copy link

commented Nov 11, 2016

I think we can close this now, since the indirect (CRI) path is setup for the dockershim. Remaining work includes:

  • Resolving TTY in Kubelet (#36615)
  • Deal with very long commands (#36187)
  • Implement authentication interface (#36666)
  • Serve streaming commands from Pod namespace (#36667)

@timstclair timstclair closed this Nov 11, 2016

deads2k pushed a commit to deads2k/kubernetes that referenced this issue Dec 2, 2016

Kubernetes Submit Queue
Merge pull request kubernetes#36615 from timstclair/cri-attach-tty
Automatic merge from submit-queue

[CRI] Add TTY flag to AttachRequest

Follow up from kubernetes#35661
For kubernetes#29579

- Add TTY to the CRI AttachRequest
- Moves responsibility from the runtime shim to the Kubelet for populating the TTY bool in the request based on the container spec

/cc @euank @feiskyer @kubernetes/sig-node
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.