From 87a53468b230e6592c547f29f6279a640e368445 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 16 Jan 2017 08:40:00 -0500 Subject: [PATCH] Merge pull request #30113 from thaJeztah/fix-autoremove-on-older-api Don't use AutoRemove on older daemons (cherry picked from commit e74623d283111dc6492b1a97a8299a9377794b99) Signed-off-by: Sebastiaan van Stijn --- .../router/container/container_routes.go | 5 +++ cli/command/container/run.go | 8 +--- client/container_create.go | 6 +++ client/container_create_test.go | 42 +++++++++++++++++++ runconfig/opts/parse.go | 4 ++ runconfig/opts/parse_test.go | 8 ++++ 6 files changed, 67 insertions(+), 6 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/run.go b/cli/command/container/run.go index 0fad93e688..fb3681a9f5 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -74,9 +74,8 @@ func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions cmdPath := "run" var ( - flAttach *opttypes.ListOpts - ErrConflictAttachDetach = fmt.Errorf("Conflicting options: -a and -d") - ErrConflictRestartPolicyAndAutoRemove = fmt.Errorf("Conflicting options: --restart and --rm") + flAttach *opttypes.ListOpts + ErrConflictAttachDetach = fmt.Errorf("Conflicting options: -a and -d") ) config, hostConfig, networkingConfig, err := runconfigopts.Parse(flags, copts) @@ -87,9 +86,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.Fprintf(stderr, "WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous.\n") } 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) + } +} diff --git a/runconfig/opts/parse.go b/runconfig/opts/parse.go index 71a89277ec..3a0da7984b 100644 --- a/runconfig/opts/parse.go +++ b/runconfig/opts/parse.go @@ -621,6 +621,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/runconfig/opts/parse_test.go b/runconfig/opts/parse_test.go index a1be379ae8..98894bb4b8 100644 --- a/runconfig/opts/parse_test.go +++ b/runconfig/opts/parse_test.go @@ -586,6 +586,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)