From 8de489929ec40afbf92d582f4f47d14cd34f3a87 Mon Sep 17 00:00:00 2001 From: sirkrypt0 <22522058+sirkrypt0@users.noreply.github.com> Date: Fri, 11 Jun 2021 16:29:14 +0200 Subject: [PATCH] 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 | 1 + nomad/nomad.go | 31 ++++++++++++++++++++++++++---- nomad/nomad_test.go | 7 ++++--- tests/e2e/websocket_test.go | 38 +++++++++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 7 deletions(-) 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) {