Pipes #650

tomwilkie · 2015-11-11T18:29:01Z

Fixes #365, Fixes #648

UI Todo:

Backend Todo:

Pipes that don't have a shell on the other end are garbled
Add docker exec control.
Test for all the timeout logic.
Make close propagate from the probe -> UI.
Timeout pipes for probes that disappear.

xfer/controls.go

@@ -20,6 +22,35 @@ type Response struct {
 	ID    int64
 	Value interface{}
 	Error string
+	Pipe  int64


tomwilkie · 2015-11-30T15:04:50Z

I'll put all the feedback in one comment for now:

x to close the terminal window is on the right hand side for the details panel; would be nice to be consistent.
cannot drag window around by the text area on the title.
want a way to 'pop out' the terminal into another window/tab
resize is broken (doesn't seem to do the right thing if you load up a vi, for instance). Might just want to disable it for now.
should we offer a way to minimize the windows? closing and reopening them doesn't do the right thing, as the contents is lost, and the new terminal doesn't seem to work.
I can't drag the terminal out beyond the outer window edge, which seems counter-intuitive (with normal windows on the desktop, for instance)

In general I'm not super-keen on the windows-within-windows approach, maybe you should put this in front of @pidster, @rade and @monadic to see what they think?

Also we need to nail down the lifecycle of these things. I'm thinking:

When user closes the window, UI should DELETE the /api/pipe/foo - this will cause the pipe to get closed all the way down the stack

- UI<->App reconnects should work fine (as the app won't delete the pipe when you disconnect, it will just buffer up stuff) - There are buffers at two stages in the process - one in the app for data coming from the probe, and one in the probe from data coming from the app. If either buffer hits a limit (a few KB I think), I will make it tear down the pipe. In this case, I will close the UI<->App ws. You will need to retry the ws when ever it the connection dropped, and if you get a 404, you'll need to show an error to the user. - If probe<->app communication is disrupted, the associated pipes will be destroyed. This should deal with apps or probes going away. You will see your ws get closed, and then a 404 on retry.

Note with this I'm not proposing we garbage collect pipes at any stage - so if you open a pipe and then never use it, it will stay around for ever. They don't consume much resource, so this shouldn't be a problem. new 'design' below.

What do you think? (@paul would be good to get your input too)

tomwilkie · 2015-11-30T15:11:14Z

Also, if you stop scope underneath the UI, it resized an open terminal very small. If you put try to reconnect (maybe with a 'reconectting...' overlay on the terminal) with a backoff, and if you get an error code (404, as opposed to bad hostname or can't connect) it should show an error ("this pipe has gone away. Sooooory")

Another thing: click-and-drag doesn't move the window to the foreground - need to move it to foreground on mouse down I think.

davkal · 2015-11-30T16:01:12Z

Comprehensive feedback!
Before implementing a full-blown window manager, I'd like to focus on correctness of display and input being sent, as well as reconnection issues.
When that's working reliably, I'd like to show it to the rest of the team for further UI feedback.
Re window manager, I think we should keep it rudimentary but allow a popout to a new tab.

tomwilkie · 2015-11-30T16:05:17Z

I've talked to @pidster, @bboreham, @awh, @dpw, @monadic and @rade about the window-manger-in-a-browser-window 'feature' for feedback, which summarized was:

having the terminal 'pop out' into a separate tab is a must have.
only having the pop out (ie ditching the current window-in-a-window approach) was not desirable, as it would be 'surprising' to the users (everyone agreed here)
@pidster suggested instead of a movable, overlappable set of windows we only allow a single window to be open at once, and it wouldn't be moveable, and would resize with the browser window:

This way, if the user wants to have multiple terminals (or topology view) side-by-side, they have to use their own windows manager and separate scope tabs/windows. Everyone I spoke to (except @monadic) preferred this idea. Note it doesn't involved us implementing a window manager ourselves, and fits nicely with the 'window management' of the details panel.
4. a final suggestions (from @monadic) was not not allow them to move or resize, but fix them to the left hand side of the window, maybe allowing some accordion-style minimisation.

paulbellamy · 2015-11-30T17:38:41Z

re: lifecycle

I'm not sure about having a buffer overflow teardown the pipe. Some sort of pipe-specific backpressure would be better, but not sure on the feasibility of implementing that in probe/UI.

I'd rather have some sort of timeout/teardown for unused pipes, with not connections open. Even if they're not active. It's just bad form to leave them around. Doesn't have to be aggressive, something on the order of an hour without usage would be fine.

Other than that, sounds roughly sane.

common/bufferedpipe/bufferedpipe.go

+	}
+	go p.loop(w)
+	return p
+}


tomwilkie · 2015-12-01T11:23:32Z

After discussion with @paul, I've been convinced the previous pipe-lifecycle design was floored and we need to introduce some for of back pressure to ensure cat bigfile doesn't cause pipes to close. Initial ideas where:

make pipe read/writes blocking functions calls across the WS using the existing control/rpc mechanisms. We believe this would make the keyboard echo unbearable on high latency connections.
implement some kind of fixes window of pipe ios and sequence numbers, blocking sending of new ones when the window fills up, and adding messages to the WS to ack up to a certain sequences number. Basically reimplement TCP.
As neither of these two ideas are particularly attractive, @paul & I think going for a separate WS from the app <-> probe per pipe is the best way forward. We we then make all the operations (read/write) blocking, and rely on the OS's TCP stack for back pressure etc.

This brings us on to the pipe lifecycle. Again, @paul convinced me we need to timeout pipes that are unused. This need to deal with App and Probe restarts (not leaking resources in either), but also long lived idle pipes - we want pipes to stay open when you go home for the night, for instance.

Timeouts will be done by the App, based on no UI connected to the pipe for some period (10mins?).
If the app gets a connection from the probe for a pipe it doesn't know about, it will 'create' a pipe for it. If it gets a connections for a pipe it doesn't know about from the UI (or one that has been deleted), it will 404.
Deleting / timing out a pipe will mark it as 'tombstoned' in the App, and close any connections to the pipe (from both the UI and the App).
The probe will gracefully deal with dropped connections to the app, reconnecting if necessary.
When the probe reconnects to a pipe, if the pipe is known and tombstoned, it will get a 404. The probe will treat 404 as 'this pipe has been deleted' and stop the pipe locally.

This should deal nicely with app restarts (pipes will eventually get reconnected) and probe restarts (pipes will timeout in the app).

peterbourgon · 2015-12-01T11:30:51Z

we want pipes to stay open when you go home for the night, for instance

This seems incredibly odd to me. Pipes are a quick-and-easy way to get at specific parts of your infrastructure, ephemeral and not long-lived. Similarly,

Timeouts will be done by the App, no UI connected to the pipe for some period

I'd rather expect the moment a user closes the UI element that spawned the pipe, the pipe should be torn down. — edit Timeouts are still necessary, in case the teardown signal doesn't make it from the UI to the app for whatever reason.

The probe will gracefully deal with dropped connections to the app, reconnecting if necessary.

This seems like a huge can of worms, better left unopened. If a pipe connection fails, tear it down cleanly on all sides, and wait for the user to re-initiate a new connection via the UI.

Scope isn't a terminal emulator...

tomwilkie · 2015-12-01T13:18:33Z

I'd rather expect the moment a user closes the UI element that spawned the pipe,

Yes we'll add this as well, using the same mechanism for timeouts. Sorry that wasn't clear.

tomwilkie · 2015-12-01T13:39:37Z

Just did the demo with @squaremo and he had the suggestion that ctrl-click on the attach button should open in new window. Otherwise I think he was 50/50 between option (3) & (4).

squaremo · 2015-12-01T13:54:37Z

Otherwise I think he was 50/50 between option (3) & (4).

I think

things you can move and things you can't move should look different;
MDI is awful (https://en.wikipedia.org/wiki/Multiple_document_interface)

so that leads me to suggest that the info pane is attached to the side of the window, and the console is attached to that.

EDIT: link to MDI

foot · 2015-12-02T17:28:25Z

What happens if we fail to send the DELETE /api/pipe/:pid in some cases? (E.g. user quitting the browser). Time's out after a few minutes and no harm done?

paulbellamy · 2015-12-02T17:40:15Z

Or, user hitting the back button...

tomwilkie · 2015-12-02T22:17:09Z

Yes, we'll timeout the pipes after 10mins or so. I think that was in the design.

foot · 2015-12-03T08:11:02Z

Came across a nifty API in chrome/firefox (and can be reasonably polyfilled elsewhere) for sending fire-and-forget requests as the browser is unloading the page: sendBeacon! Played with it this morning and it works as advertised.

It has a very simple api: sendBeacon(url, data) and only POSTs. Could we add a POSTable endpoint for cleaning up pipes? Something terrible like this perhaps:

POST /api/pipe/:pid/_delete.

paulbellamy · 2015-12-03T10:37:49Z

I'd suggest POST /api/pipe/:pid?_method=delete

Edit: For no particular reason, other than it matches rails.

foot · 2015-12-03T10:42:03Z

+1 Good call much nicer.

app/pipes.go

+			log.Printf("Timeing out pipe %s", id)
+			delete(pr.lastPut, id)
+			pipe := pr.pipes[id]
+			pipe.Close(now)


davkal · 2015-12-10T16:26:07Z

2nd round of UI feedback (popouts and resizing work now)

(10) action to close should be all caps, no underline (and maybe a different color), like the ones in the topo options.

(11) popout window has url bar, which suggests that it's sharable, which does not seem to work

(12) chrome v45 crashes when doing 2 popouts in a row (will create separate issue):

1. exec -> popout -> close
2. exec -> popout -> crash

client/app/scripts/components/terminal.js

+        if (wereConnected) {
+          this.createWebsocket(term);
+        } else {
+          reconnectTimer = setTimeout(


foot · 2015-12-10T18:47:08Z

(11) popout window has url bar, which suggests that it's sharable, which does not seem to work

Can't get rid of url bar these days. =/

(12) chrome v45 crashes when doing 2 popouts in a row (will create separate issue):

Couldn't figure this out! =/

But the rest of it should be working!

in case we ever have 2 terminals

davkal · 2015-12-11T09:32:12Z

UI and JS LGTM

paulbellamy · 2015-12-11T10:04:29Z

Backend code, LGTM.

Main Concerns:

Hitting escape seems to close the terminal? Which makes using vim difficult
Neither escape, nor pressing the X button terminates the remote exec. the processes in the exec continue running.
"Pop-out" in general doesn't redraw anything, which makes it seem like it broke.
Running htop (and I'd imagine other ncurses programs) then popping out behaves oddly (not redrawing):

Minor Concerns:

Text-wrapping and resize still seem a bit off. There's a blank space on the right side of the terminal that text never enters/uses.
Not sure the TTY-sizing is making it back to the terminal when execing. Vim seems to only occupy the top-left quarter of the "screen"
Ctrl- on the cli seems to not send anything? (or send some odd letters)
Now that there are more controls, error messages get pushed to the next line.

Edit: Reformatted into main and minor concerns.

paulbellamy · 2015-12-11T11:04:12Z

I've opened #746 to deal with ncurses/popout/resize issues.

paulbellamy · 2015-12-11T11:15:09Z

Ok, have opened issues for remaining issues. So all LGTM.

…ng from AppClient.Close()

Pipes

tomwilkie self-assigned this Nov 11, 2015

tomwilkie force-pushed the 365-pipes branch 2 times, most recently from bd4df51 to cb07d8f Compare November 16, 2015 11:30

tomwilkie reviewed Nov 16, 2015
View reviewed changes

xfer/controls.go

@@ -20,6 +22,35 @@ type Response struct {

ID int64

Value interface{}

Error string

Pipe int64

This comment was marked as abuse.

Sign in to view

tomwilkie force-pushed the 365-pipes branch from 9d72ee8 to a586ec4 Compare November 30, 2015 14:38

paulbellamy reviewed Dec 1, 2015
View reviewed changes

common/bufferedpipe/bufferedpipe.go

}

go p.loop(w)

return p

}

This comment was marked as abuse.

Sign in to view

davkal force-pushed the 365-pipes branch from 222b916 to 7bcb7cf Compare December 1, 2015 17:44

paulbellamy reviewed Dec 3, 2015
View reviewed changes

app/pipes.go

log.Printf("Timeing out pipe %s", id)

delete(pr.lastPut, id)

pipe := pr.pipes[id]

pipe.Close(now)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

Fix infinite loop when closing AppClient.

ff37488

tomwilkie force-pushed the 365-pipes branch from 1747913 to ff37488 Compare December 10, 2015 12:53

foot and others added 3 commits December 10, 2015 14:38

Try to reconnect immediately giving a nicer ctrl-d diconnect exp.

50be8c6

Don't use a global variable to store the pipe client.

6259307

Wait for pipes to close in Close().

0b1a2ef

tomwilkie force-pushed the 365-pipes branch from d9cbfba to 0b1a2ef Compare December 10, 2015 15:45

davkal reviewed Dec 10, 2015
View reviewed changes

client/app/scripts/components/terminal.js

if (wereConnected) {

this.createWebsocket(term);

} else {

reconnectTimer = setTimeout(

This comment was marked as abuse.

Sign in to view

foot added 2 commits December 10, 2015 19:43

Cleanup our timeouts.

e0ff3e3

More Terminal-UI and code post review fixes.

921a7b7

Add terminal setTimeouts as instance properties instead of globals

8a63615

in case we ever have 2 terminals

Dont deselect node on ESC if there is a controlPipe

d93a3e2

This was referenced Dec 11, 2015

Closing a pipe should close the exec #747

Open

Pipe should allow me to type all characters #748

Closed

Correctly wait for all background goroutines to finish before returni…

5050bac

…ng from AppClient.Close()

tomwilkie added a commit that referenced this pull request Dec 11, 2015

Merge pull request #650 from weaveworks/365-pipes

d8c759c

Pipes

tomwilkie merged commit d8c759c into master Dec 11, 2015

tomwilkie deleted the 365-pipes branch December 11, 2015 12:01

foot mentioned this pull request Mar 2, 2016

Opening a pipe fails on first attempt #1095

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pipes #650

Pipes #650

tomwilkie commented Nov 11, 2015

This comment was marked as abuse.

tomwilkie commented Nov 30, 2015

tomwilkie commented Nov 30, 2015

davkal commented Nov 30, 2015

tomwilkie commented Nov 30, 2015

paulbellamy commented Nov 30, 2015

This comment was marked as abuse.

tomwilkie commented Dec 1, 2015

peterbourgon commented Dec 1, 2015

tomwilkie commented Dec 1, 2015

tomwilkie commented Dec 1, 2015

squaremo commented Dec 1, 2015

foot commented Dec 2, 2015

paulbellamy commented Dec 2, 2015

tomwilkie commented Dec 2, 2015

foot commented Dec 3, 2015

paulbellamy commented Dec 3, 2015

foot commented Dec 3, 2015

This comment was marked as abuse.

This comment was marked as abuse.

davkal commented Dec 10, 2015

This comment was marked as abuse.

foot commented Dec 10, 2015

davkal commented Dec 11, 2015

paulbellamy commented Dec 11, 2015

paulbellamy commented Dec 11, 2015

paulbellamy commented Dec 11, 2015

Pipes #650

Pipes #650

Conversation

tomwilkie commented Nov 11, 2015

This comment was marked as abuse.

tomwilkie commented Nov 30, 2015

tomwilkie commented Nov 30, 2015

davkal commented Nov 30, 2015

tomwilkie commented Nov 30, 2015

paulbellamy commented Nov 30, 2015

This comment was marked as abuse.

tomwilkie commented Dec 1, 2015

peterbourgon commented Dec 1, 2015

tomwilkie commented Dec 1, 2015

tomwilkie commented Dec 1, 2015

squaremo commented Dec 1, 2015

foot commented Dec 2, 2015

paulbellamy commented Dec 2, 2015

tomwilkie commented Dec 2, 2015

foot commented Dec 3, 2015

paulbellamy commented Dec 3, 2015

foot commented Dec 3, 2015

This comment was marked as abuse.

This comment was marked as abuse.

davkal commented Dec 10, 2015

This comment was marked as abuse.

foot commented Dec 10, 2015

davkal commented Dec 11, 2015

paulbellamy commented Dec 11, 2015

paulbellamy commented Dec 11, 2015

paulbellamy commented Dec 11, 2015