From 0ea7b143b0a8366799c83a955be676aaf1345214 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 13 Jan 2017 01:05:39 +0100 Subject: [PATCH] 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 --- .../router/container/container_routes.go | 5 +++ cli/command/container/opts.go | 4 ++ cli/command/container/opts_test.go | 8 ++++ cli/command/container/run.go | 12 ++---- client/container_create.go | 6 +++ client/container_create_test.go | 42 +++++++++++++++++++ 6 files changed, 69 insertions(+), 8 deletions(-) diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 9c9bc0f8c3..3c389245ce 100644 --- a/api/server/router/container/container_routes.go +++ b/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, diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index c5fc152168..55cc3c3b29 100644 --- a/cli/command/container/opts.go +++ b/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 diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go index d02a0f7bfc..ce3bb21b41 100644 --- a/cli/command/container/opts_test.go +++ b/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) diff --git a/cli/command/container/run.go b/cli/command/container/run.go index 0f8da3fa4e..4d85ee77ac 100644 --- a/cli/command/container/run.go +++ b/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 } diff --git a/client/container_create.go b/client/container_create.go index 9f627aafa6..6841b0b282 100644 --- a/client/container_create.go +++ b/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) diff --git a/client/container_create_test.go b/client/container_create_test.go index 15dbd5ea01..73474cf56f 100644 --- a/client/container_create_test.go +++ b/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) + } +}