From 2d43d93410c29cec87deb9cd940c3b2a8af5fbbb Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Fri, 11 Sep 2015 22:50:21 -0400 Subject: [PATCH] Make exec start return proper error codes Exec start was sending HTTP 500 for every error. Fixed an error where pausing a container and then calling exec start caused the daemon to freeze. Updated API docs which incorrectly showed that a successful exec start was an HTTP 201, in reality it is HTTP 200. Signed-off-by: Brian Goff --- api/server/router/local/exec.go | 7 ++ daemon/exec.go | 76 ++++++++++--------- docs/reference/api/docker_remote_api.md | 1 + docs/reference/api/docker_remote_api_v1.15.md | 2 +- docs/reference/api/docker_remote_api_v1.16.md | 2 +- docs/reference/api/docker_remote_api_v1.17.md | 2 +- docs/reference/api/docker_remote_api_v1.18.md | 2 +- docs/reference/api/docker_remote_api_v1.19.md | 2 +- docs/reference/api/docker_remote_api_v1.20.md | 2 +- docs/reference/api/docker_remote_api_v1.21.md | 3 +- errors/daemon.go | 4 +- integration-cli/docker_api_exec_test.go | 42 ++++++++++ 12 files changed, 102 insertions(+), 43 deletions(-) diff --git a/api/server/router/local/exec.go b/api/server/router/local/exec.go index b5b6364480..d525ac1b82 100644 --- a/api/server/router/local/exec.go +++ b/api/server/router/local/exec.go @@ -75,6 +75,10 @@ func (s *router) postContainerExecStart(ctx context.Context, w http.ResponseWrit return err } + if exists, err := s.daemon.ExecExists(execName); !exists { + return err + } + if !execStartCheck.Detach { var err error // Setting up the streaming http interface. @@ -102,6 +106,9 @@ func (s *router) postContainerExecStart(ctx context.Context, w http.ResponseWrit // Now run the user process in container. if err := s.daemon.ContainerExecStart(execName, stdin, stdout, stderr); err != nil { + if execStartCheck.Detach { + return err + } fmt.Fprintf(outStream, "Error running exec in container: %v\n", err) } return nil diff --git a/daemon/exec.go b/daemon/exec.go index 8ec8a1c583..99331ed257 100644 --- a/daemon/exec.go +++ b/daemon/exec.go @@ -92,8 +92,19 @@ func (d *Daemon) registerExecCommand(ExecConfig *ExecConfig) { d.execCommands.Add(ExecConfig.ID, ExecConfig) } +// ExecExists looks up the exec instance and returns a bool if it exists or not. +// It will also return the error produced by `getExecConfig` +func (d *Daemon) ExecExists(name string) (bool, error) { + if _, err := d.getExecConfig(name); err != nil { + return false, err + } + return true, nil +} + +// getExecConfig looks up the exec instance by name. If the container associated +// with the exec instance is stopped or paused, it will return an error. func (d *Daemon) getExecConfig(name string) (*ExecConfig, error) { - ExecConfig := d.execCommands.Get(name) + ec := d.execCommands.Get(name) // If the exec is found but its container is not in the daemon's list of // containers then it must have been delete, in which case instead of @@ -101,12 +112,14 @@ func (d *Daemon) getExecConfig(name string) (*ExecConfig, error) { // the user sees the same error now that they will after the // 5 minute clean-up loop is run which erases old/dead execs. - if ExecConfig != nil && d.containers.Get(ExecConfig.Container.ID) != nil { - - if !ExecConfig.Container.IsRunning() { - return nil, derr.ErrorCodeContainerNotRunning.WithArgs(ExecConfig.Container.ID) + if ec != nil && d.containers.Get(ec.Container.ID) != nil { + if !ec.Container.IsRunning() { + return nil, derr.ErrorCodeContainerNotRunning.WithArgs(ec.Container.ID, ec.Container.State.String()) } - return ExecConfig, nil + if ec.Container.isPaused() { + return nil, derr.ErrorCodeExecPaused.WithArgs(ec.Container.ID) + } + return ec, nil } return nil, derr.ErrorCodeNoExecID.WithArgs(name) @@ -181,35 +194,30 @@ func (d *Daemon) ContainerExecCreate(config *runconfig.ExecConfig) (string, erro // ContainerExecStart starts a previously set up exec instance. The // std streams are set up. -func (d *Daemon) ContainerExecStart(execName string, stdin io.ReadCloser, stdout io.Writer, stderr io.Writer) error { +func (d *Daemon) ContainerExecStart(name string, stdin io.ReadCloser, stdout io.Writer, stderr io.Writer) error { var ( cStdin io.ReadCloser cStdout, cStderr io.Writer ) - ExecConfig, err := d.getExecConfig(execName) + ec, err := d.getExecConfig(name) if err != nil { - return err + return derr.ErrorCodeNoExecID.WithArgs(name) } - func() { - ExecConfig.Lock() - defer ExecConfig.Unlock() - if ExecConfig.Running { - err = derr.ErrorCodeExecRunning.WithArgs(execName) - } - ExecConfig.Running = true - }() - if err != nil { - return err + ec.Lock() + if ec.Running { + ec.Unlock() + return derr.ErrorCodeExecRunning.WithArgs(ec.ID) } + ec.Running = true + ec.Unlock() - logrus.Debugf("starting exec command %s in container %s", ExecConfig.ID, ExecConfig.Container.ID) - container := ExecConfig.Container + logrus.Debugf("starting exec command %s in container %s", ec.ID, ec.Container.ID) + container := ec.Container + container.logEvent("exec_start: " + ec.ProcessConfig.Entrypoint + " " + strings.Join(ec.ProcessConfig.Arguments, " ")) - container.logEvent("exec_start: " + ExecConfig.ProcessConfig.Entrypoint + " " + strings.Join(ExecConfig.ProcessConfig.Arguments, " ")) - - if ExecConfig.OpenStdin { + if ec.OpenStdin { r, w := io.Pipe() go func() { defer w.Close() @@ -218,23 +226,23 @@ func (d *Daemon) ContainerExecStart(execName string, stdin io.ReadCloser, stdout }() cStdin = r } - if ExecConfig.OpenStdout { + if ec.OpenStdout { cStdout = stdout } - if ExecConfig.OpenStderr { + if ec.OpenStderr { cStderr = stderr } - ExecConfig.streamConfig.stderr = broadcastwriter.New() - ExecConfig.streamConfig.stdout = broadcastwriter.New() + ec.streamConfig.stderr = broadcastwriter.New() + ec.streamConfig.stdout = broadcastwriter.New() // Attach to stdin - if ExecConfig.OpenStdin { - ExecConfig.streamConfig.stdin, ExecConfig.streamConfig.stdinPipe = io.Pipe() + if ec.OpenStdin { + ec.streamConfig.stdin, ec.streamConfig.stdinPipe = io.Pipe() } else { - ExecConfig.streamConfig.stdinPipe = ioutils.NopWriteCloser(ioutil.Discard) // Silently drop stdin + ec.streamConfig.stdinPipe = ioutils.NopWriteCloser(ioutil.Discard) // Silently drop stdin } - attachErr := attach(&ExecConfig.streamConfig, ExecConfig.OpenStdin, true, ExecConfig.ProcessConfig.Tty, cStdin, cStdout, cStderr) + attachErr := attach(&ec.streamConfig, ec.OpenStdin, true, ec.ProcessConfig.Tty, cStdin, cStdout, cStderr) execErr := make(chan error) @@ -243,8 +251,8 @@ func (d *Daemon) ContainerExecStart(execName string, stdin io.ReadCloser, stdout // the exitStatus) even after the cmd is done running. go func() { - if err := container.exec(ExecConfig); err != nil { - execErr <- derr.ErrorCodeExecCantRun.WithArgs(execName, container.ID, err) + if err := container.exec(ec); err != nil { + execErr <- derr.ErrorCodeExecCantRun.WithArgs(ec.ID, container.ID, err) } }() select { diff --git a/docs/reference/api/docker_remote_api.md b/docs/reference/api/docker_remote_api.md index 0a3d9e48a0..0315ba92fd 100644 --- a/docs/reference/api/docker_remote_api.md +++ b/docs/reference/api/docker_remote_api.md @@ -90,6 +90,7 @@ list of DNS options to be used in the container. * `GET /events` now includes a `timenano` field, in addition to the existing `time` field. * `GET /info` now lists engine version information. * `GET /containers/json` will return `ImageID` of the image used by container. +* `POST /exec/(name)/start` will now return an HTTP 409 when the container is either stopped or paused. ### v1.20 API changes diff --git a/docs/reference/api/docker_remote_api_v1.15.md b/docs/reference/api/docker_remote_api_v1.15.md index e4ca3286bf..89deb61d01 100644 --- a/docs/reference/api/docker_remote_api_v1.15.md +++ b/docs/reference/api/docker_remote_api_v1.15.md @@ -1677,7 +1677,7 @@ Json Parameters: Status Codes: -- **201** – no error +- **200** – no error - **404** – no such exec instance **Stream details**: diff --git a/docs/reference/api/docker_remote_api_v1.16.md b/docs/reference/api/docker_remote_api_v1.16.md index df93963576..3a1faa4100 100644 --- a/docs/reference/api/docker_remote_api_v1.16.md +++ b/docs/reference/api/docker_remote_api_v1.16.md @@ -1638,7 +1638,7 @@ Json Parameters: Status Codes: -- **201** – no error +- **200** – no error - **404** – no such exec instance **Stream details**: diff --git a/docs/reference/api/docker_remote_api_v1.17.md b/docs/reference/api/docker_remote_api_v1.17.md index 1bb66c2286..6c7df5068f 100644 --- a/docs/reference/api/docker_remote_api_v1.17.md +++ b/docs/reference/api/docker_remote_api_v1.17.md @@ -1801,7 +1801,7 @@ Json Parameters: Status Codes: -- **201** – no error +- **200** – no error - **404** – no such exec instance **Stream details**: diff --git a/docs/reference/api/docker_remote_api_v1.18.md b/docs/reference/api/docker_remote_api_v1.18.md index aca5cf0cee..e3886f8718 100644 --- a/docs/reference/api/docker_remote_api_v1.18.md +++ b/docs/reference/api/docker_remote_api_v1.18.md @@ -1922,7 +1922,7 @@ Json Parameters: Status Codes: -- **201** – no error +- **200** – no error - **404** – no such exec instance **Stream details**: diff --git a/docs/reference/api/docker_remote_api_v1.19.md b/docs/reference/api/docker_remote_api_v1.19.md index 7733aaa54d..7b258fdf32 100644 --- a/docs/reference/api/docker_remote_api_v1.19.md +++ b/docs/reference/api/docker_remote_api_v1.19.md @@ -1984,7 +1984,7 @@ Json Parameters: Status Codes: -- **201** – no error +- **200** – no error - **404** – no such exec instance **Stream details**: diff --git a/docs/reference/api/docker_remote_api_v1.20.md b/docs/reference/api/docker_remote_api_v1.20.md index 327662333a..100b4a3f58 100644 --- a/docs/reference/api/docker_remote_api_v1.20.md +++ b/docs/reference/api/docker_remote_api_v1.20.md @@ -2126,7 +2126,7 @@ Json Parameters: Status Codes: -- **201** – no error +- **200** – no error - **404** – no such exec instance **Stream details**: diff --git a/docs/reference/api/docker_remote_api_v1.21.md b/docs/reference/api/docker_remote_api_v1.21.md index 71742d49e1..df905c26ad 100644 --- a/docs/reference/api/docker_remote_api_v1.21.md +++ b/docs/reference/api/docker_remote_api_v1.21.md @@ -2226,8 +2226,9 @@ Json Parameters: Status Codes: -- **201** – no error +- **200** – no error - **404** – no such exec instance +- **409** - container is stopped or paused **Stream details**: Similar to the stream behavior of `POST /container/(id)/attach` API diff --git a/errors/daemon.go b/errors/daemon.go index 07187d8bca..7a6e61e61d 100644 --- a/errors/daemon.go +++ b/errors/daemon.go @@ -654,7 +654,7 @@ var ( Value: "CONTAINERNOTRUNNING", Message: "Container %s is not running: %s", Description: "An attempt was made to retrieve the information about an 'exec' but the container is not running", - HTTPStatusCode: http.StatusInternalServerError, + HTTPStatusCode: http.StatusConflict, }) // ErrorCodeNoExecID is generated when we try to get the info @@ -672,7 +672,7 @@ var ( Value: "EXECPAUSED", Message: "Container %s is paused, unpause the container before exec", Description: "An attempt to start an 'exec' was made, but the owning container is paused", - HTTPStatusCode: http.StatusInternalServerError, + HTTPStatusCode: http.StatusConflict, }) // ErrorCodeExecRunning is generated when we try to start an exec diff --git a/integration-cli/docker_api_exec_test.go b/integration-cli/docker_api_exec_test.go index 46c3a987d1..cecbd76f2f 100644 --- a/integration-cli/docker_api_exec_test.go +++ b/integration-cli/docker_api_exec_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "net/http" + "strings" "github.com/go-check/check" ) @@ -47,3 +48,44 @@ func (s *DockerSuite) TestExecApiCreateNoValidContentType(c *check.C) { c.Fatalf("Expected message when creating exec command with invalid Content-Type specified") } } + +func (s *DockerSuite) TestExecAPIStart(c *check.C) { + dockerCmd(c, "run", "-d", "--name", "test", "busybox", "top") + + createExec := func() string { + _, b, err := sockRequest("POST", fmt.Sprintf("/containers/%s/exec", "test"), map[string]interface{}{"Cmd": []string{"true"}}) + c.Assert(err, check.IsNil, check.Commentf(string(b))) + + createResp := struct { + ID string `json:"Id"` + }{} + c.Assert(json.Unmarshal(b, &createResp), check.IsNil, check.Commentf(string(b))) + return createResp.ID + } + + startExec := func(id string, code int) { + resp, body, err := sockRequestRaw("POST", fmt.Sprintf("/exec/%s/start", id), strings.NewReader(`{"Detach": true}`), "application/json") + c.Assert(err, check.IsNil) + + b, err := readBody(body) + c.Assert(err, check.IsNil, check.Commentf(string(b))) + c.Assert(resp.StatusCode, check.Equals, code, check.Commentf(string(b))) + } + + startExec(createExec(), http.StatusOK) + + id := createExec() + dockerCmd(c, "stop", "test") + + startExec(id, http.StatusNotFound) + + dockerCmd(c, "start", "test") + startExec(id, http.StatusNotFound) + + // make sure exec is created before pausing + id = createExec() + dockerCmd(c, "pause", "test") + startExec(id, http.StatusConflict) + dockerCmd(c, "unpause", "test") + startExec(id, http.StatusOK) +}