diff --git a/go.sum b/go.sum index 45182d4..c16234b 100644 --- a/go.sum +++ b/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= diff --git a/nomad/nomad.go b/nomad/nomad.go index a102db1..f25a36f 100644 --- a/nomad/nomad.go +++ b/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) } diff --git a/nomad/nomad_test.go b/nomad/nomad_test.go index f971b11..1a38032 100644 --- a/nomad/nomad_test.go +++ b/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]) }) diff --git a/tests/e2e/websocket_test.go b/tests/e2e/websocket_test.go index 6392ebe..0e218bf 100644 --- a/tests/e2e/websocket_test.go +++ b/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) {