Exec cleanup improvements #12542
Exec cleanup improvements #12542
Conversation
tomponline
force-pushed
the
tp-exec
branch
3 times, most recently
from
5812caf to
1b1f768
Compare
…hes in ExecInstance Signed-off-by: Thomas Parrott <[email protected]>
Not since ce2aad5 Signed-off-by: Thomas Parrott <[email protected]>
Signed-off-by: Thomas Parrott <[email protected]>
This reverts commit 781905a. Signed-off-by: Thomas Parrott <[email protected]>
…rol and stdin channels Signed-off-by: Thomas Parrott <[email protected]>
|
|
||
| <-t.C | ||
| for { | ||
| err := conn.WriteControl(websocket.PingMessage, []byte("keepalive"), time.Now().Add(5*time.Second)) |
There was a problem hiding this comment.
are pings here written always, for any connection type? (both interactive and non-interactive)
There was a problem hiding this comment.
Yes the (s *execWs) Connect is called for both non-interactive and interactive channels so it happens for all of them.
| @@ -1249,10 +1253,16 @@ func (r *ProtocolLXD) ExecInstance(instanceName string, exec api.InstanceExecPos | |||
| return nil, err | |||
| } | |||
|
|
|||
| go func() { | |||
| _, _, _ = conn.ReadMessage() // Consume pings from server. | |||
There was a problem hiding this comment.
and over here pings are only consumed for non-interactive sessions ?!
expanding context, I cannot immediately understand if "interactive" branch of code just above needs to consume pings or not.
There was a problem hiding this comment.
Only the control channel (for both interactive and non-interactive) and the stdin channel for non-interactive sessions requires a conn.ReadMessage() call (which internally then loops reading websocket frames and consumes the ping messages).
The other channels are already consuming ping messages because their direction is to read from the websocket and write to the stdout/stderr or the PTY.
I have observed this by setting up a custom ping handler on the client that logs when a ping was received and I was initially only seeing it for the stdout & stderr (both interactive and non-interactive), and not for the control and stdin for non-interactive sessions.
Yes it does, and it is happening already. |
|
@xnox I am not sure if this will fix the issue on riscv LP when it restores pings over unix sockets - that will be tested and remains to be seen. But it is certainly the case that at this time pings are not being consumed for stdin (non-interactive) and control (non-interactive and interactive) so it needs fixing anyway. |
Following on from @cjwatson PR #12530 that disabled websocket level ping messages when using Unix sockets, this PR re-enables them for the purposes of trying to identify whether the same issues exist when using TCP sockets and trying to fix the issue more generally.
We will test this PR with the Launchpad folks before moving this to latest/stable.
This PR:
Fixes #10034