Merge pull request moby#30113 from thaJeztah/fix-autoremove-on-older-api

Don't use AutoRemove on older daemons

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
-		ErrConflictAttachDetach               = errors.New("Conflicting options: -a and -d")
-		ErrConflictRestartPolicyAndAutoRemove = errors.New("Conflicting options: --restart and --rm")
+		flAttach                *opttypes.ListOpts
+		ErrConflictAttachDetach = errors.New("Conflicting options: -a and -d")
 	)
 
 	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)
+	}
+}