[lxc-devel] [lxd/master] VM: Further attempts to fix VM exec instability

[tomponline on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 497 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20200211/03e93b97/attachment.bin>
-------------- next part --------------
From 0d76248959f4b96ecbff4e8740d616455cb6a061 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Tue, 11 Feb 2020 12:13:32 +0000
Subject: [PATCH 1/3] shared/network: Don't log contents of file handle in
 WebsocketRecvStream

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 shared/network.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/shared/network.go b/shared/network.go
index 71e5f09551..3a49052850 100644
--- a/shared/network.go
+++ b/shared/network.go
@@ -211,7 +211,7 @@ func WebsocketRecvStream(w io.Writer, conn *websocket.Conn) chan bool {
 			}
 
 			if err != nil {
-				logger.Debugf("Got error getting next reader %s, %s", err, w)
+				logger.Debugf("Got error getting next reader %s", err)
 				break
 			}
 

From 5106f95ceb32b8a0603326906279f30e2d66deea Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Tue, 11 Feb 2020 12:13:57 +0000
Subject: [PATCH 2/3] shared/netutils/network/linux: Adds ability to switch
 WebsocketExecMirror to use ReaderToChannel

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 shared/netutils/network_linux.go | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/shared/netutils/network_linux.go b/shared/netutils/network_linux.go
index cbea0423b3..93f0846768 100644
--- a/shared/netutils/network_linux.go
+++ b/shared/netutils/network_linux.go
@@ -171,14 +171,22 @@ func NetnsGetifaddrs(initPID int32) (map[string]api.InstanceStateNetwork, error)
 	return networks, nil
 }
 
-func WebsocketExecMirror(conn *websocket.Conn, w io.WriteCloser, r io.ReadCloser, exited chan bool, fd int) (chan bool, chan bool) {
+// WebsocketExecMirror if supplied with pollFd then it uses polling on that descriptor.
+func WebsocketExecMirror(conn *websocket.Conn, w io.WriteCloser, r io.ReadCloser, exited chan bool, pollFd int) (chan bool, chan bool) {
 	readDone := make(chan bool, 1)
 	writeDone := make(chan bool, 1)
 
 	go shared.DefaultWriter(conn, w, writeDone)
 
 	go func(conn *websocket.Conn, r io.ReadCloser) {
-		in := shared.ExecReaderToChannel(r, -1, exited, fd)
+		var in <-chan []byte
+		if pollFd > 0 {
+			in = shared.ExecReaderToChannel(r, -1, exited, pollFd)
+
+		} else {
+			in = shared.ReaderToChannel(r, -1)
+		}
+
 		for {
 			buf, ok := <-in
 			if !ok {

From 28c0f927e295e3bb5873ef780b478a86338c8fce Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Tue, 11 Feb 2020 12:14:41 +0000
Subject: [PATCH 3/3] lxd/container/exec: Switch WebsocketMirror to use
 standard read mechanism for VMs

Closes PTY earlier so it doesn't block the mirror function.

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd-agent/exec.go     | 15 ++++++++-------
 lxd/container_exec.go | 22 ++++++++++++++--------
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/lxd-agent/exec.go b/lxd-agent/exec.go
index 1feddb8325..7bbc84643a 100644
--- a/lxd-agent/exec.go
+++ b/lxd-agent/exec.go
@@ -349,8 +349,9 @@ func (s *execWs) Do(op *operations.Operation) error {
 			conn := s.conns[0]
 			s.connsLock.Unlock()
 
-			logger.Info("Started mirroring websocket")
-			readDone, writeDone := netutils.WebsocketExecMirror(conn, ptys[0], ptys[0], attachedChildIsDead, int(ptys[0].Fd()))
+			pollFD := int(ptys[0].Fd())
+			logger.Infof("Started mirroring websocket with FD %v", pollFD)
+			readDone, writeDone := netutils.WebsocketExecMirror(conn, ptys[0], ptys[0], attachedChildIsDead, pollFD)
 
 			<-readDone
 			<-writeDone
@@ -388,6 +389,11 @@ func (s *execWs) Do(op *operations.Operation) error {
 			tty.Close()
 		}
 
+		// Ensure PTYs are closed before websocket to end wgEOF.Wait().
+		for _, pty := range ptys {
+			pty.Close()
+		}
+
 		s.connsLock.Lock()
 		conn := s.conns[-1]
 		s.connsLock.Unlock()
@@ -401,13 +407,8 @@ func (s *execWs) Do(op *operations.Operation) error {
 		}
 
 		attachedChildIsDead <- true
-
 		wgEOF.Wait()
 
-		for _, pty := range ptys {
-			pty.Close()
-		}
-
 		metadata := shared.Jmap{"return": cmdResult}
 		err = op.UpdateMetadata(metadata)
 		if err != nil {
diff --git a/lxd/container_exec.go b/lxd/container_exec.go
index 2302ee54b3..0853b8b923 100644
--- a/lxd/container_exec.go
+++ b/lxd/container_exec.go
@@ -240,10 +240,16 @@ func (s *execWs) Do(op *operations.Operation) error {
 			conn := s.conns[0]
 			s.connsLock.Unlock()
 
-			logger.Debugf("Started mirroring websocket")
-			defer logger.Debugf("Finished mirroring websocket")
-			readDone, writeDone := netutils.WebsocketExecMirror(conn, ptys[0], ptys[0], attachedChildIsDead, int(ptys[0].Fd()))
+			pollFD := 0
+
+			// Use the polling mechanism for websocket mirror if local container, otherwise use reads.
+			if s.instance.Type() == instancetype.Container {
+				pollFD = int(ptys[0].Fd())
+			}
 
+			logger.Debugf("Started mirroring websocket with FD %v", pollFD)
+			defer logger.Debugf("Finished mirroring websocket")
+			readDone, writeDone := netutils.WebsocketExecMirror(conn, ptys[0], ptys[0], attachedChildIsDead, pollFD)
 			<-readDone
 			<-writeDone
 			conn.Close()
@@ -279,6 +285,11 @@ func (s *execWs) Do(op *operations.Operation) error {
 			tty.Close()
 		}
 
+		// Ensure PTYs are closed before websocket to end wgEOF.Wait().
+		for _, pty := range ptys {
+			pty.Close()
+		}
+
 		s.connsLock.Lock()
 		conn := s.conns[-1]
 		s.connsLock.Unlock()
@@ -292,13 +303,8 @@ func (s *execWs) Do(op *operations.Operation) error {
 		}
 
 		attachedChildIsDead <- true
-
 		wgEOF.Wait()
 
-		for _, pty := range ptys {
-			pty.Close()
-		}
-
 		metadata := shared.Jmap{"return": cmdResult}
 		err = op.UpdateMetadata(metadata)
 		if err != nil {