From ecf889816c8ac58d13020a76df36cf48546d9f9f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 1 Dec 2016 22:24:30 +0100 Subject: [PATCH] Fix restartpolicy max-retry validation the restart policy validation was moved from the client to the daemon in 94e95e4711643640701bd614902e75a2d01f12c5 As part of that change, retry-counts < 1 were marked as "invalid". However, the default is 0 (unlimited), causing docker run -d --restart=on-failure nginx To fail. This changes the validation to only invalidate retry-counts < 0. A test was added, and other tests renamed to allow running just these tests :) Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 9db5d649aea1c3d4728d0159bb5175a49f77748e) Signed-off-by: Victor Vieux --- daemon/container.go | 6 ++-- integration-cli/docker_api_containers_test.go | 26 ++++++++++++--- integration-cli/docker_cli_restart_test.go | 32 ++++++++++++++++--- 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/daemon/container.go b/daemon/container.go index d27ed27dbf..71fe06d592 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -268,11 +268,11 @@ func (daemon *Daemon) verifyContainerSettings(hostConfig *containertypes.HostCon switch p.Name { case "always", "unless-stopped", "no": if p.MaximumRetryCount != 0 { - return nil, fmt.Errorf("maximum restart count not valid with restart policy of '%s'", p.Name) + return nil, fmt.Errorf("maximum retry count cannot be used with restart policy '%s'", p.Name) } case "on-failure": - if p.MaximumRetryCount < 1 { - return nil, fmt.Errorf("maximum restart count must be a positive integer") + if p.MaximumRetryCount < 0 { + return nil, fmt.Errorf("maximum retry count cannot be negative") } case "": // do nothing diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index cd3dd30a0f..d046ec0684 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -728,7 +728,7 @@ func (s *DockerSuite) TestContainerAPIInvalidPortSyntax(c *check.C) { c.Assert(string(b[:]), checker.Contains, "invalid port") } -func (s *DockerSuite) TestContainerAPIInvalidRestartPolicyName(c *check.C) { +func (s *DockerSuite) TestContainerAPIRestartPolicyInvalidPolicyName(c *check.C) { config := `{ "Image": "busybox", "HostConfig": { @@ -748,7 +748,7 @@ func (s *DockerSuite) TestContainerAPIInvalidRestartPolicyName(c *check.C) { c.Assert(string(b[:]), checker.Contains, "invalid restart policy") } -func (s *DockerSuite) TestContainerAPIInvalidRestartPolicyRetryMismatch(c *check.C) { +func (s *DockerSuite) TestContainerAPIRestartPolicyRetryMismatch(c *check.C) { config := `{ "Image": "busybox", "HostConfig": { @@ -765,10 +765,10 @@ func (s *DockerSuite) TestContainerAPIInvalidRestartPolicyRetryMismatch(c *check b, err := readBody(body) c.Assert(err, checker.IsNil) - c.Assert(string(b[:]), checker.Contains, "maximum restart count not valid with restart policy") + c.Assert(string(b[:]), checker.Contains, "maximum retry count cannot be used with restart policy") } -func (s *DockerSuite) TestContainerAPIInvalidRestartPolicyPositiveRetryCount(c *check.C) { +func (s *DockerSuite) TestContainerAPIRestartPolicyNegativeRetryCount(c *check.C) { config := `{ "Image": "busybox", "HostConfig": { @@ -785,7 +785,23 @@ func (s *DockerSuite) TestContainerAPIInvalidRestartPolicyPositiveRetryCount(c * b, err := readBody(body) c.Assert(err, checker.IsNil) - c.Assert(string(b[:]), checker.Contains, "maximum restart count must be a positive integer") + c.Assert(string(b[:]), checker.Contains, "maximum retry count cannot be negative") +} + +func (s *DockerSuite) TestContainerAPIRestartPolicyDefaultRetryCount(c *check.C) { + config := `{ + "Image": "busybox", + "HostConfig": { + "RestartPolicy": { + "Name": "on-failure", + "MaximumRetryCount": 0 + } + } + }` + + res, _, err := sockRequestRaw("POST", "/containers/create", strings.NewReader(config), "application/json") + c.Assert(err, checker.IsNil) + c.Assert(res.StatusCode, checker.Equals, http.StatusCreated) } // Issue 7941 - test to make sure a "null" in JSON is just ignored. diff --git a/integration-cli/docker_cli_restart_test.go b/integration-cli/docker_cli_restart_test.go index ca71aaa144..7d585289eb 100644 --- a/integration-cli/docker_cli_restart_test.go +++ b/integration-cli/docker_cli_restart_test.go @@ -74,7 +74,7 @@ func (s *DockerSuite) TestRestartWithVolumes(c *check.C) { } func (s *DockerSuite) TestRestartPolicyNO(c *check.C) { - out, _ := dockerCmd(c, "run", "-d", "--restart=no", "busybox", "false") + out, _ := dockerCmd(c, "create", "--restart=no", "busybox") id := strings.TrimSpace(string(out)) name := inspectField(c, id, "HostConfig.RestartPolicy.Name") @@ -82,7 +82,7 @@ func (s *DockerSuite) TestRestartPolicyNO(c *check.C) { } func (s *DockerSuite) TestRestartPolicyAlways(c *check.C) { - out, _ := dockerCmd(c, "run", "-d", "--restart=always", "busybox", "false") + out, _ := dockerCmd(c, "create", "--restart=always", "busybox") id := strings.TrimSpace(string(out)) name := inspectField(c, id, "HostConfig.RestartPolicy.Name") @@ -95,12 +95,36 @@ func (s *DockerSuite) TestRestartPolicyAlways(c *check.C) { } func (s *DockerSuite) TestRestartPolicyOnFailure(c *check.C) { - out, _ := dockerCmd(c, "run", "-d", "--restart=on-failure:1", "busybox", "false") + out, _, err := dockerCmdWithError("create", "--restart=on-failure:-1", "busybox") + c.Assert(err, check.NotNil, check.Commentf(out)) + c.Assert(out, checker.Contains, "maximum retry count cannot be negative") + + out, _ = dockerCmd(c, "create", "--restart=on-failure:1", "busybox") id := strings.TrimSpace(string(out)) name := inspectField(c, id, "HostConfig.RestartPolicy.Name") - c.Assert(name, checker.Equals, "on-failure") + maxRetry := inspectField(c, id, "HostConfig.RestartPolicy.MaximumRetryCount") + c.Assert(name, checker.Equals, "on-failure") + c.Assert(maxRetry, checker.Equals, "1") + + out, _ = dockerCmd(c, "create", "--restart=on-failure:0", "busybox") + + id = strings.TrimSpace(string(out)) + name = inspectField(c, id, "HostConfig.RestartPolicy.Name") + maxRetry = inspectField(c, id, "HostConfig.RestartPolicy.MaximumRetryCount") + + c.Assert(name, checker.Equals, "on-failure") + c.Assert(maxRetry, checker.Equals, "0") + + out, _ = dockerCmd(c, "create", "--restart=on-failure", "busybox") + + id = strings.TrimSpace(string(out)) + name = inspectField(c, id, "HostConfig.RestartPolicy.Name") + maxRetry = inspectField(c, id, "HostConfig.RestartPolicy.MaximumRetryCount") + + c.Assert(name, checker.Equals, "on-failure") + c.Assert(maxRetry, checker.Equals, "0") } // a good container with --restart=on-failure:3