Resize TTYs #1966

2opremio · 2016-10-29T11:35:01Z

This PR adds the possibility to resize TTYs. It adds (optional) arguments to controls and modifies the UI<->App contract in the following way:

Control requests now take optional arguments (a JSON string map)
Pipe request add an additional key in the response: resize_tty_control which indicates what control to use in order to resize the TTY
That control takes three (mandatory) parameters: width, height and pipeID

Here's an example:

The UI requests POST http://172.16.0.3:4040/api/control/3b89ec4023a94f3e/516cc48f99788d7d6b558d4ebf3d0e3bd1d3f2eae65ceb4e405a53498ed99b45%3B%3Ccontainer%3E/docker_exec_container to create a containter TTY
The App responds with {"pipe":"pipe-4012265930510560261","raw_tty":true,"resize_tty_control":"docker_resize_exec_tty"}. Note the new resize_tty_control field.
We can set the terminal size by doing curl -X POST -d '{ "pipeID": "pipe-4012265930510560261", "height": "100", "width": "100"}' http://172.16.0.3:4040/api/control/3b89ec4023a94f3e/516cc48f99788d7d6b558d4ebf3d0e3bd1d3f2eae65ceb4e405a53498ed99b45%3B%3Ccontainer%3E/docker_resize_exec_tty
The tty gets adjusted accordingly by the App:

TODO:

Implement (3) in the UI (CC @foot )
Implement resizing for host TTYs in the backend

I would appreciate a design review (maybe by @tomwilkie )

Fixes #746.
Fixs #1964

…ze ttys

foot · 2016-10-31T14:56:03Z

Works great for containers!

Using the controls endpoint is a bit awkward right now: to construct the url /api/control/:control.probeId}/:control.nodeId/docker_resize_exec_tty we have to look up probeId and nodeId from some other control.

A pipe based endpoint feels like it would be cleaner: /api/pipes/:pipeId/resize.

If thats not feasible I guess either:

nodeDetails.resizeControl property
as suggested somewhere else include a docker_resize_exec_tty control in nodeDetails.controls w/ a hidden: true, a bit awkward.

2opremio · 2016-10-31T15:57:20Z

Works great for containers!

Awesome! but shouldn't it be generic? (i.e. if the pipe response sets raw_tty and has a resize_tty_control field, set the initial size and every time the user resizes the window, regardless of what control it is (container, host and what not)

Using the controls endpoint is a bit awkward right now: to construct the url /api/control/:control.probeId}/:control.nodeId/docker_resize_exec_tty we have to look up probeId and nodeId from some other control.

Can't you save them with the pipeID and associate them to the terminal instance?

A pipe based endpoint feels like it would be cleaner: /api/pipes/:pipeId/resize

That makes it awkward for the backend. I gave it a lot of thought and this was the best option I could come up with, respecting what we already have for controls (we cannot use the pipes directly because we don't have a way to send out of band control information cleanly without breaking backwards compatibility in the pipes protocol).

We could use your proposed API and still use controls under the hood, but that would make it internally stateful (we would have to keep a map from pipeIDs to probeIDs and control)

foot · 2016-10-31T16:03:02Z

Can't you save them with the pipeID and associate them to the terminal instance?

Yep, true, we can do that.

if the pipe response sets raw_tty and has a resize_tty_control field

Ah hah! This is probably why host is not working right now.

Will clean up and fix this.

Adds terminal resizing on popped out windows

foot · 2016-10-31T17:12:19Z

Working on hosts and popped out windows too now.

Can the resize_tty_control be missing sometimes?

2opremio · 2016-10-31T17:24:03Z

Can the resize_tty_control be missing sometimes?

It won't be missing in the response of docker_exec/host_exec controls but it's optional, so I would handle it as such.

jml

Go code LGTM. One comment that you can act on at your discretion.

common/xfer/controls.go

+			return ResponseErrorf("Bad parameter: width (%q): %v", widthS, err)
+		}
+
+		return next(pipeID, uint(height), uint(width))


davkal

Works well, nicely done. Apart from naming issues, LGTM

client/app/scripts/components/terminal.js

-
-  log('Caculated (col, row) sizes in px: ', pixelPerCol, pixelPerRow);
-  return {pixelPerCol, pixelPerRow};
+  log('Caculated (charWidth, charHeight) sizes in px: ', characterWidth, characterHeight);


client/app/scripts/utils/web-api-utils.js

@@ -248,6 +252,22 @@ export function doControlRequest(nodeId, control, dispatch) {
  });
 }

+
+export function doResizePipe(pipeId, control, cols, rows) {


client/app/scripts/utils/web-api-utils.js

+            res.pipe,
+            nodeId,
+            res.raw_tty,
+            {id: res.resize_tty_control, probeId: control.probeId, nodeId: control.nodeId}));


client/app/scripts/actions/app-actions.js

@@ -580,16 +580,17 @@ export function receiveControlNodeRemoved(nodeId) {
  };
 }

-export function receiveControlPipeFromParams(pipeId, rawTty) {
+export function receiveControlPipeFromParams(pipeId, rawTty, resizeTtyControl) {


foot · 2016-11-03T09:20:45Z

So we don't receive a resize control from attach. Does it makes sense to have one there? @2opremio

…tach) - Fix text selection in terminal (was our force focus stuff, introduced if you clicked on the black border that was not actually the terminal

2opremio · 2016-11-03T09:33:07Z

So we don't receive a resize control from attach. Does it makes sense to have one there?

I don't think it does. Logs are not in raw mode, so it shouldn't matter (in general at least)

2opremio · 2016-11-03T09:36:17Z

Docker provides https://godoc.org/github.com/fsouza/go-dockerclient#Client.ResizeContainerTTY so it should be easy to add, but I don't think it's even worth it. That could change if I am presented with a reasonable usecase.

Alfonso Acosta added 5 commits October 31, 2016 11:04

Implement App endpoint /pipe/{id}/resize_tty/{height}/{width} to resi…

c2f2a90

…ze ttys

Disable pipes endpoint, we are going for controls instead

403b70d

Allow passing arguments to controls

37ba071

Implement resize control for docker TTYs

2443f7a

Remove leftover functions

411f8d7

foot force-pushed the 746-resize-ttys branch from 92dacdd to 411f8d7 Compare October 31, 2016 10:05

Alfonso Acosta and others added 6 commits October 31, 2016 10:15

Fix backend tests

6a3910d

Vendor github.com/docker/docker/pkg/term

6cbf2bf

Implement TTY resize for hosts

2536578

Make linter happy

1cf6641

Fix warning about text nodes in thead in details panel

ea9101a

Resizing working w/ containers!

305bf76

Adds terminal resizing on hosts.

487635c

Adds terminal resizing on popped out windows

2opremio changed the title ~~[WIP] Resize TTYs~~ Resize TTYs Oct 31, 2016

2opremio assigned davkal and jml Nov 2, 2016

jml approved these changes Nov 2, 2016

View reviewed changes

common/xfer/controls.go

return ResponseErrorf("Bad parameter: width (%q): %v", widthS, err)

}

return next(pipeID, uint(height), uint(width))

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

davkal reviewed Nov 2, 2016

View reviewed changes

foot mentioned this pull request Nov 2, 2016

Cannot copy/paste out of a scope terminal #1964

Closed

Don't resize terminal if we don't receive a resize control (e.g. onAt…

d3878cc

…tach) - Fix text selection in terminal (was our force focus stuff, introduced if you clicked on the black border that was not actually the terminal

foot merged commit 7e5166e into master Nov 3, 2016

foot deleted the 746-resize-ttys branch November 3, 2016 10:06

2opremio mentioned this pull request Nov 4, 2016

kubernetes logs don't use full width #1977

Closed

Resize TTYs #1966

Resize TTYs #1966

Uh oh!

Conversation

2opremio commented Oct 29, 2016 • edited by foot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foot commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2opremio commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foot commented Oct 31, 2016

Uh oh!

foot commented Oct 31, 2016

Uh oh!

2opremio commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jml left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

davkal left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

foot commented Nov 3, 2016

Uh oh!

2opremio commented Nov 3, 2016

Uh oh!

2opremio commented Nov 3, 2016

Uh oh!

Uh oh!

2opremio commented Oct 29, 2016 •

edited by foot

Loading

foot commented Oct 31, 2016 •

edited

Loading

2opremio commented Oct 31, 2016 •

edited

Loading

2opremio commented Oct 31, 2016 •

edited

Loading