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

Fix initial exec terminal dimensions #47991

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 21 additions & 3 deletions pkg/kubelet/dockershim/exec.go
Expand Up @@ -135,6 +135,9 @@ func (*NsenterExecHandler) ExecInContainer(client libdocker.Interface, container
type NativeExecHandler struct{}

func (*NativeExecHandler) ExecInContainer(client libdocker.Interface, container *dockertypes.ContainerJSON, cmd []string, stdin io.Reader, stdout, stderr io.WriteCloser, tty bool, resize <-chan remotecommand.TerminalSize, timeout time.Duration) error {
done := make(chan struct{})
defer close(done)

createOpts := dockertypes.ExecConfig{
Cmd: cmd,
AttachStdin: stdin != nil,
Expand All @@ -149,16 +152,31 @@ func (*NativeExecHandler) ExecInContainer(client libdocker.Interface, container

// Have to start this before the call to client.StartExec because client.StartExec is a blocking
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fix this comment to reflect the "delay"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dims what delay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ncdc - "Delay attempting to send a terminal resize" mentioned in the body of the PR

// call :-( Otherwise, resize events don't get processed and the terminal never resizes.
kubecontainer.HandleResizing(resize, func(size remotecommand.TerminalSize) {
client.ResizeExecTTY(execObj.ID, uint(size.Height), uint(size.Width))
})
//
// We also have to delay attempting to send a terminal resize request to docker until after the
// exec has started; otherwise, the initial resize request will fail.
execStarted := make(chan struct{})
go func() {
select {
case <-execStarted:
// client.StartExec has started the exec, so we can start resizing
case <-done:
// ExecInContainer has returned, so short-circuit
return
}

kubecontainer.HandleResizing(resize, func(size remotecommand.TerminalSize) {
client.ResizeExecTTY(execObj.ID, uint(size.Height), uint(size.Width))
})
}()

startOpts := dockertypes.ExecStartCheck{Detach: false, Tty: tty}
streamOpts := libdocker.StreamOptions{
InputStream: stdin,
OutputStream: stdout,
ErrorStream: stderr,
RawTerminal: tty,
ExecStarted: execStarted,
}
err = client.StartExec(execObj.ID, startOpts, streamOpts)
if err != nil {
Expand Down
10 changes: 10 additions & 0 deletions pkg/kubelet/dockershim/libdocker/kube_docker_client.go
Expand Up @@ -454,6 +454,15 @@ func (d *kubeDockerClient) StartExec(startExec string, opts dockertypes.ExecStar
return err
}
defer resp.Close()

if sopts.ExecStarted != nil {
// Send a message to the channel indicating that the exec has started. This is needed so
// interactive execs can handle resizing correctly - the request to resize the TTY has to happen
// after the call to d.client.ContainerExecAttach, and because d.holdHijackedConnection below
// blocks, we use sopts.ExecStarted to signal the caller that it's ok to resize.
sopts.ExecStarted <- struct{}{}
}

return d.holdHijackedConnection(sopts.RawTerminal || opts.Tty, sopts.InputStream, sopts.OutputStream, sopts.ErrorStream, resp)
}

Expand Down Expand Up @@ -584,6 +593,7 @@ type StreamOptions struct {
InputStream io.Reader
OutputStream io.Writer
ErrorStream io.Writer
ExecStarted chan struct{}
}

// operationTimeout is the error returned when the docker operations are timeout.
Expand Down