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 <cpuguy83@gmail.com>

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

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())
+		}
+		if ec.Container.isPaused() {
+			return nil, derr.ErrorCodeExecPaused.WithArgs(ec.Container.ID)
 		}
-		return ExecConfig, nil
+		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
-
-	container.logEvent("exec_start: " + ExecConfig.ProcessConfig.Entrypoint + " " + strings.Join(ExecConfig.ProcessConfig.Arguments, " "))
+	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, " "))
 
-	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 {

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
 

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**:

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**:

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**:

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**:

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**:

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**:

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

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

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)
+}