Don't use AutoRemove on older daemons

Docker 1.13 moves the `--rm` flag to the daemon,
through an AutoRemove option in HostConfig.

When using API 1.24 and under, AutoRemove should not be
used, even if the daemon is version 1.13 or above and
"supports" this feature.

This patch fixes a situation where an 1.13 client,
talking to an 1.13 daemon, but using the 1.24 API
version, still set the AutoRemove property.

As a result, both the client _and_ the daemon
were attempting to remove the container, resulting
in an error:

    ERRO[0000] error removing container: Error response from daemon:
    removal of container ce0976ad22495c7cbe9487752ea32721a282164862db036b2f3377bd07461c3a
    is already in progress

In addition, the validation of conflicting options
is moved from `docker run` to `opts.parse()`, so
that conflicting options are also detected when
running `docker create` and `docker start` separately.

To resolve the issue, the `AutoRemove` option is now
always set to `false` both by the client and the
daemon, if API version 1.24 or under is used.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

api/server/router/container/container_routes.go

@@ -369,6 +369,11 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo
 	version := httputils.VersionFromContext(ctx)
 	adjustCPUShares := versions.LessThan(version, "1.19")
 
+	// When using API 1.24 and under, the client is responsible for removing the container
+	if hostConfig != nil && versions.LessThan(version, "1.25") {
+		hostConfig.AutoRemove = false
+	}
+
 	ccr, err := s.backend.ContainerCreate(types.ContainerCreateConfig{
 		Name:             name,
 		Config:           config,

cli/command/container/opts.go

@@ -622,6 +622,10 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 		Runtime:        copts.runtime,
 	}
 
+	if copts.autoRemove && !hostConfig.RestartPolicy.IsNone() {
+		return nil, nil, nil, fmt.Errorf("Conflicting options: --restart and --rm")
+	}
+
 	// only set this value if the user provided the flag, else it should default to nil
 	if flags.Changed("init") {
 		hostConfig.Init = &copts.init

cli/command/container/opts_test.go

@@ -456,6 +456,14 @@ func TestParseRestartPolicy(t *testing.T) {
 	}
 }
 
+func TestParseRestartPolicyAutoRemove(t *testing.T) {
+	expected := "Conflicting options: --restart and --rm"
+	_, _, _, err := parseRun([]string{"--rm", "--restart=always", "img", "cmd"})
+	if err == nil || err.Error() != expected {
+		t.Fatalf("Expected error %v, but got none", expected)
+	}
+}
+
 func TestParseHealth(t *testing.T) {
 	checkOk := func(args ...string) *container.HealthConfig {
 		config, _, _, err := parseRun(args)

cli/command/container/run.go

@@ -73,9 +73,8 @@ func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions
 	cmdPath := "run"
 
 	var (
-		flAttach                              *opttypes.ListOpts
+		flAttach                *opttypes.ListOpts
-		ErrConflictAttachDetach               = errors.New("Conflicting options: -a and -d")
+		ErrConflictAttachDetach = errors.New("Conflicting options: -a and -d")
-		ErrConflictRestartPolicyAndAutoRemove = errors.New("Conflicting options: --restart and --rm")
 	)
 
 	config, hostConfig, networkingConfig, err := parse(flags, copts)
@@ -86,9 +85,6 @@ func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions
 		return cli.StatusError{StatusCode: 125}
 	}
 
-	if hostConfig.AutoRemove && !hostConfig.RestartPolicy.IsNone() {
-		return ErrConflictRestartPolicyAndAutoRemove
-	}
 	if hostConfig.OomKillDisable != nil && *hostConfig.OomKillDisable && hostConfig.Memory == 0 {
 		fmt.Fprintln(stderr, "WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous.")
 	}
@@ -209,7 +205,7 @@ func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions
 		})
 	}
 
-	statusChan := waitExitOrRemoved(ctx, dockerCli, createResponse.ID, hostConfig.AutoRemove)
+	statusChan := waitExitOrRemoved(ctx, dockerCli, createResponse.ID, copts.autoRemove)
 
 	//start the container
 	if err := client.ContainerStart(ctx, createResponse.ID, types.ContainerStartOptions{}); err != nil {
@@ -222,7 +218,7 @@ func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions
 		}
 
 		reportError(stderr, cmdPath, err.Error(), false)
-		if hostConfig.AutoRemove {
+		if copts.autoRemove {
 			// wait container to be removed
 			<-statusChan
 		}

client/container_create.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/network"
+	"github.com/docker/docker/api/types/versions"
 	"golang.org/x/net/context"
 )
 
@@ -25,6 +26,11 @@ func (cli *Client) ContainerCreate(ctx context.Context, config *container.Config
 		return response, err
 	}
 
+	// When using API 1.24 and under, the client is responsible for removing the container
+	if hostConfig != nil && versions.LessThan(cli.ClientVersion(), "1.25") {
+		hostConfig.AutoRemove = false
+	}
+
 	query := url.Values{}
 	if containerName != "" {
 		query.Set("name", containerName)

client/container_create_test.go

@@ -74,3 +74,45 @@ func TestContainerCreateWithName(t *testing.T) {
 		t.Fatalf("expected `container_id`, got %s", r.ID)
 	}
 }
+
+// TestContainerCreateAutoRemove validates that a client using API 1.24 always disables AutoRemove. When using API 1.25
+// or up, AutoRemove should not be disabled.
+func TestContainerCreateAutoRemove(t *testing.T) {
+	autoRemoveValidator := func(expectedValue bool) func(req *http.Request) (*http.Response, error) {
+		return func(req *http.Request) (*http.Response, error) {
+			var config configWrapper
+
+			if err := json.NewDecoder(req.Body).Decode(&config); err != nil {
+				return nil, err
+			}
+			if config.HostConfig.AutoRemove != expectedValue {
+				return nil, fmt.Errorf("expected AutoRemove to be %v, got %v", expectedValue, config.HostConfig.AutoRemove)
+			}
+			b, err := json.Marshal(container.ContainerCreateCreatedBody{
+				ID: "container_id",
+			})
+			if err != nil {
+				return nil, err
+			}
+			return &http.Response{
+				StatusCode: http.StatusOK,
+				Body:       ioutil.NopCloser(bytes.NewReader(b)),
+			}, nil
+		}
+	}
+
+	client := &Client{
+		client:  newMockClient(autoRemoveValidator(false)),
+		version: "1.24",
+	}
+	if _, err := client.ContainerCreate(context.Background(), nil, &container.HostConfig{AutoRemove: true}, nil, ""); err != nil {
+		t.Fatal(err)
+	}
+	client = &Client{
+		client:  newMockClient(autoRemoveValidator(true)),
+		version: "1.25",
+	}
+	if _, err := client.ContainerCreate(context.Background(), nil, &container.HostConfig{AutoRemove: true}, nil, ""); err != nil {
+		t.Fatal(err)
+	}
+}