Remove stderr fifo after interactive execution with stderr finished

Previously the stderr fifo would not be removed, leaving unwanted
artifacts from the execution behind. We now remove the stderr fifo
after the command finished.

go.sum

@@ -607,6 +607,7 @@ github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnIn
 github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48=
 github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE=
 github.com/stretchr/objx v0.3.0 h1:NGXK3lHquSN08v5vWalVI/L8XU9hdzE/G6xsrze47As=
 github.com/stretchr/objx v0.3.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE=

nomad/nomad.go

@@ -224,6 +224,8 @@ func (r nullReader) Read(_ []byte) (int, error) {
 // If tty is true, Nomad would normally write stdout and stderr of the command
 // both on the stdout stream. However, if the InteractiveStderr server config option is true,
 // we make sure that stdout and stderr are split correctly.
+// In order for the stderr splitting to work, the command must have the structure
+// []string{..., "sh", "-c", "my-command"}.
 func (a *APIClient) ExecuteCommand(allocationID string,
 	ctx context.Context, command []string, tty bool,
 	stdin io.Reader, stdout, stderr io.Writer) (int, error) {
@@ -233,6 +235,11 @@ func (a *APIClient) ExecuteCommand(allocationID string,
 	return a.apiQuerier.Execute(allocationID, ctx, command, tty, stdin, stdout, stderr)
 }
 
+// executeCommandInteractivelyWithStderr executes the given command interactively and splits stdout
+// and stderr correctly. Normally, using Nomad to execute a command with tty=true (in order to have
+// an interactive connection and possibly a fully working shell), would result in stdout and stderr
+// to be served both over stdout. This function circumvents this by creating a fifo for the stderr
+// of the command and starting a second execution that reads the stderr from that fifo.
 func (a *APIClient) executeCommandInteractivelyWithStderr(allocationID string, ctx context.Context,
 	command []string, stdin io.Reader, stdout, stderr io.Writer) (int, error) {
 	// Use current nano time to make the stderr fifo kind of unique.
@@ -260,14 +267,30 @@ func (a *APIClient) executeCommandInteractivelyWithStderr(allocationID string, c
 }
 
 const (
-	stderrFifoCommandFormat    = "mkfifo /tmp/stderr_%d.fifo && cat /tmp/stderr_%d.fifo"
-	stderrWrapperCommandFormat = "until [ -e /tmp/stderr_%d.fifo ]; do sleep 0.01; done; (%s) 2> /tmp/stderr_%d.fifo"
+	// stderrFifoFormat represents the format we use for our stderr fifos. The %d should be unique for the execution
+	// as otherwise multiple executions are not possible.
+	// Example: /tmp/stderr_1623330777825234133.fifo
+	stderrFifoFormat = "/tmp/stderr_%d.fifo"
+	// stderrFifoCommandFormat, if executed, is supposed to create a fifo, read from it and remove it in the end.
+	// Example: mkfifo my.fifo && (cat my.fifo; rm my.fifo)
+	stderrFifoCommandFormat = "mkfifo %s && (cat %s; rm %s)"
+	// stderrWrapperCommandFormat, if executed, is supposed to wait until a fifo exists (it sleeps 10ms to reduce load
+	// cause by busy waiting on the system). Once the fifo exists, the given command is executed and its stderr
+	// redirected to the fifo.
+	// Example: until [ -e my.fifo ]; do sleep 0.01; done; (echo "my.fifo exists") 2> my.fifo
+	stderrWrapperCommandFormat = "until [ -e %s ]; do sleep 0.01; done; (%s) 2> %s"
 )
 
 func stderrFifoCommand(id int64) []string {
-	return []string{"sh", "-c", fmt.Sprintf(stderrFifoCommandFormat, id, id)}
+	stderrFifoPath := stderrFifo(id)
+	return []string{"sh", "-c", fmt.Sprintf(stderrFifoCommandFormat, stderrFifoPath, stderrFifoPath, stderrFifoPath)}
 }
 
 func wrapCommandForStderrFifo(id int64, command string) string {
-	return fmt.Sprintf(stderrWrapperCommandFormat, id, command, id)
+	stderrFifoPath := stderrFifo(id)
+	return fmt.Sprintf(stderrWrapperCommandFormat, stderrFifoPath, command, stderrFifoPath)
+}
+
+func stderrFifo(id int64) string {
+	return fmt.Sprintf(stderrFifoFormat, id)
 }

nomad/nomad_test.go

@@ -690,14 +690,15 @@ func (s *ExecuteCommandTestSuite) TestWithSeparateStderr() {
 
 	s.Run("should wrap command in stderr wrapper", func() {
 		s.Require().NotNil(calledStdoutCommand)
-		stdoutFifoRegexp := strings.ReplaceAll(regexp.QuoteMeta(stderrWrapperCommandFormat), "%d", "\\d*")
-		stdoutFifoRegexp = fmt.Sprintf(stdoutFifoRegexp, s.testCommand)
+		stderrWrapperCommand := fmt.Sprintf(stderrWrapperCommandFormat, stderrFifoFormat, s.testCommand, stderrFifoFormat)
+		stdoutFifoRegexp := strings.ReplaceAll(regexp.QuoteMeta(stderrWrapperCommand), "%d", "\\d*")
 		s.Regexp(stdoutFifoRegexp, calledStdoutCommand[len(calledStdoutCommand)-1])
 	})
 
 	s.Run("should call correct stderr command", func() {
 		s.Require().NotNil(calledStderrCommand)
-		stderrFifoRegexp := strings.ReplaceAll(regexp.QuoteMeta(stderrFifoCommandFormat), "%d", "\\d*")
+		stderrFifoCommand := fmt.Sprintf(stderrFifoCommandFormat, stderrFifoFormat, stderrFifoFormat, stderrFifoFormat)
+		stderrFifoRegexp := strings.ReplaceAll(regexp.QuoteMeta(stderrFifoCommand), "%d", "\\d*")
 		s.Regexp(stderrFifoRegexp, calledStderrCommand[len(calledStderrCommand)-1])
 	})
 

tests/e2e/websocket_test.go

@@ -1,12 +1,15 @@
 package e2e
 
 import (
+	"bytes"
+	"context"
 	"encoding/json"
 	"fmt"
 	"github.com/gorilla/websocket"
 	"github.com/stretchr/testify/suite"
 	"gitlab.hpi.de/codeocean/codemoon/poseidon/api"
 	"gitlab.hpi.de/codeocean/codemoon/poseidon/api/dto"
+	"gitlab.hpi.de/codeocean/codemoon/poseidon/nomad"
 	"gitlab.hpi.de/codeocean/codemoon/poseidon/tests"
 	"gitlab.hpi.de/codeocean/codemoon/poseidon/tests/helpers"
 	"net/http"
@@ -145,6 +148,41 @@ func (s *E2ETestSuite) TestEchoEnvironment() {
 	s.Equal("world\r\n", stdout)
 }
 
+func (s *E2ETestSuite) TestStderrFifoIsRemoved() {
+	runnerID, err := ProvideRunner(&dto.RunnerRequest{
+		ExecutionEnvironmentId: tests.DefaultEnvironmentIDAsInteger,
+	})
+	s.Require().NoError(err)
+
+	webSocketURL, err := ProvideWebSocketURL(&s.Suite, runnerID, &dto.ExecutionRequest{Command: "ls -a /tmp/"})
+	s.Require().NoError(err)
+	connection, err := ConnectToWebSocket(webSocketURL)
+	s.Require().NoError(err)
+
+	messages, err := helpers.ReceiveAllWebSocketMessages(connection)
+	s.Require().Error(err)
+	s.Equal(&websocket.CloseError{Code: websocket.CloseNormalClosure}, err)
+
+	stdout, _, _ := helpers.WebSocketOutputMessages(messages)
+	s.Contains(stdout, ".fifo", "there should be a .fifo file during the execution")
+
+	s.NotContains(s.ListTempDirectory(runnerID), ".fifo", "/tmp/ should not contain any .fifo files after the execution")
+}
+
+func (s *E2ETestSuite) ListTempDirectory(runnerID string) string {
+	alloc, _, err := nomadClient.Allocations().Info(runnerID, nil)
+	s.Require().NoError(err)
+
+	var stdout, stderr bytes.Buffer
+	exit, err := nomadClient.Allocations().Exec(context.Background(), alloc, nomad.TaskName,
+		false, []string{"ls", "-a", "/tmp/"}, strings.NewReader(""), &stdout, &stderr, nil, nil)
+
+	s.Require().NoError(err)
+	s.Require().Equal(0, exit)
+	s.Require().Empty(stderr)
+	return stdout.String()
+}
+
 // ProvideWebSocketConnection establishes a client WebSocket connection to run the passed ExecutionRequest.
 // It requires a running Poseidon instance.
 func ProvideWebSocketConnection(suite *suite.Suite, request *dto.ExecutionRequest) (connection *websocket.Conn, err error) {