From 5fc912d2c87c1986e830a7d4f6b62bec385e9a14 Mon Sep 17 00:00:00 2001 From: Dong Chen Date: Mon, 10 Apr 2017 14:12:44 -0700 Subject: [PATCH 1/2] do not allow duration less than 1 ms in healthcheck parameters Signed-off-by: Dong Chen --- cli/command/container/opts.go | 6 +++--- cli/command/service/opts.go | 6 +++--- daemon/container.go | 12 ++++++------ docs/reference/commandline/service_create.md | 6 +++--- docs/reference/commandline/service_update.md | 6 +++--- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index 7480bfaced..cff3bf67bb 100644 --- a/cli/command/container/opts.go +++ b/cli/command/container/opts.go @@ -230,10 +230,10 @@ func addFlags(flags *pflag.FlagSet) *containerOptions { // Health-checking flags.StringVar(&copts.healthCmd, "health-cmd", "", "Command to run to check health") - flags.DurationVar(&copts.healthInterval, "health-interval", 0, "Time between running the check (ns|us|ms|s|m|h) (default 0s)") + flags.DurationVar(&copts.healthInterval, "health-interval", 0, "Time between running the check (ms|s|m|h) (default 0s)") flags.IntVar(&copts.healthRetries, "health-retries", 0, "Consecutive failures needed to report unhealthy") - flags.DurationVar(&copts.healthTimeout, "health-timeout", 0, "Maximum time to allow one check to run (ns|us|ms|s|m|h) (default 0s)") - flags.DurationVar(&copts.healthStartPeriod, "health-start-period", 0, "Start period for the container to initialize before starting health-retries countdown (ns|us|ms|s|m|h) (default 0s)") + flags.DurationVar(&copts.healthTimeout, "health-timeout", 0, "Maximum time to allow one check to run (ms|s|m|h) (default 0s)") + flags.DurationVar(&copts.healthStartPeriod, "health-start-period", 0, "Start period for the container to initialize before starting health-retries countdown (ms|s|m|h) (default 0s)") flags.SetAnnotation("health-start-period", "version", []string{"1.29"}) flags.BoolVar(&copts.noHealthcheck, "no-healthcheck", false, "Disable any container-specified HEALTHCHECK") diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index 4211c5bf8c..ecc37d7b00 100644 --- a/cli/command/service/opts.go +++ b/cli/command/service/opts.go @@ -802,13 +802,13 @@ func addServiceFlags(flags *pflag.FlagSet, opts *serviceOptions, defaultFlagValu flags.StringVar(&opts.healthcheck.cmd, flagHealthCmd, "", "Command to run to check health") flags.SetAnnotation(flagHealthCmd, "version", []string{"1.25"}) - flags.Var(&opts.healthcheck.interval, flagHealthInterval, "Time between running the check (ns|us|ms|s|m|h)") + flags.Var(&opts.healthcheck.interval, flagHealthInterval, "Time between running the check (ms|s|m|h)") flags.SetAnnotation(flagHealthInterval, "version", []string{"1.25"}) - flags.Var(&opts.healthcheck.timeout, flagHealthTimeout, "Maximum time to allow one check to run (ns|us|ms|s|m|h)") + flags.Var(&opts.healthcheck.timeout, flagHealthTimeout, "Maximum time to allow one check to run (ms|s|m|h)") flags.SetAnnotation(flagHealthTimeout, "version", []string{"1.25"}) flags.IntVar(&opts.healthcheck.retries, flagHealthRetries, 0, "Consecutive failures needed to report unhealthy") flags.SetAnnotation(flagHealthRetries, "version", []string{"1.25"}) - flags.Var(&opts.healthcheck.startPeriod, flagHealthStartPeriod, "Start period for the container to initialize before counting retries towards unstable (ns|us|ms|s|m|h)") + flags.Var(&opts.healthcheck.startPeriod, flagHealthStartPeriod, "Start period for the container to initialize before counting retries towards unstable (ms|s|m|h)") flags.SetAnnotation(flagHealthStartPeriod, "version", []string{"1.29"}) flags.BoolVar(&opts.healthcheck.noHealthcheck, flagNoHealthcheck, false, "Disable any container-specified HEALTHCHECK") flags.SetAnnotation(flagNoHealthcheck, "version", []string{"1.25"}) diff --git a/daemon/container.go b/daemon/container.go index 50878f01e9..a3482687b7 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -244,20 +244,20 @@ func (daemon *Daemon) verifyContainerSettings(hostConfig *containertypes.HostCon // Validate the healthcheck params of Config if config.Healthcheck != nil { - if config.Healthcheck.Interval != 0 && config.Healthcheck.Interval < time.Second { - return nil, fmt.Errorf("Interval in Healthcheck cannot be less than one second") + if config.Healthcheck.Interval != 0 && config.Healthcheck.Interval < time.Millisecond { + return nil, fmt.Errorf("Interval in Healthcheck cannot be less than one millisecond") } - if config.Healthcheck.Timeout != 0 && config.Healthcheck.Timeout < time.Second { - return nil, fmt.Errorf("Timeout in Healthcheck cannot be less than one second") + if config.Healthcheck.Timeout != 0 && config.Healthcheck.Timeout < time.Millisecond { + return nil, fmt.Errorf("Timeout in Healthcheck cannot be less than one millisecond") } if config.Healthcheck.Retries < 0 { return nil, fmt.Errorf("Retries in Healthcheck cannot be negative") } - if config.Healthcheck.StartPeriod < 0 { - return nil, fmt.Errorf("StartPeriod in Healthcheck cannot be negative") + if config.Healthcheck.StartPeriod != 0 && config.Healthcheck.StartPeriod < time.Millisecond { + return nil, fmt.Errorf("StartPeriod in Healthcheck cannot be less than one millisecond") } } } diff --git a/docs/reference/commandline/service_create.md b/docs/reference/commandline/service_create.md index 082dffb827..78faa98bf7 100644 --- a/docs/reference/commandline/service_create.md +++ b/docs/reference/commandline/service_create.md @@ -33,10 +33,10 @@ Options: --env-file list Read in a file of environment variables --group list Set one or more supplementary user groups for the container --health-cmd string Command to run to check health - --health-interval duration Time between running the check (ns|us|ms|s|m|h) + --health-interval duration Time between running the check (ms|s|m|h) --health-retries int Consecutive failures needed to report unhealthy - --health-start-period duration Start period for the container to initialize before counting retries towards unstable (ns|us|ms|s|m|h) - --health-timeout duration Maximum time to allow one check to run (ns|us|ms|s|m|h) + --health-start-period duration Start period for the container to initialize before counting retries towards unstable (ms|s|m|h) + --health-timeout duration Maximum time to allow one check to run (ms|s|m|h) --help Print usage --host list Set one or more custom host-to-IP mappings (host:ip) --hostname string Container hostname diff --git a/docs/reference/commandline/service_update.md b/docs/reference/commandline/service_update.md index fae6b0af89..93c5750eee 100644 --- a/docs/reference/commandline/service_update.md +++ b/docs/reference/commandline/service_update.md @@ -41,10 +41,10 @@ Options: --group-add list Add an additional supplementary user group to the container --group-rm list Remove a previously added supplementary user group from the container --health-cmd string Command to run to check health - --health-interval duration Time between running the check (ns|us|ms|s|m|h) + --health-interval duration Time between running the check (ms|s|m|h) --health-retries int Consecutive failures needed to report unhealthy - --health-start-period duration Start period for the container to initialize before counting retries towards unstable (ns|us|ms|s|m|h) - --health-timeout duration Maximum time to allow one check to run (ns|us|ms|s|m|h) + --health-start-period duration Start period for the container to initialize before counting retries towards unstable (ms|s|m|h) + --health-timeout duration Maximum time to allow one check to run (ms|s|m|h) --help Print usage --host-add list Add or update a custom host-to-IP mapping (host:ip) --host-rm list Remove a custom host-to-IP mapping (host:ip) From d8b6a35d0272d9bb121dda7d0bc53d0e53d8bd22 Mon Sep 17 00:00:00 2001 From: Dong Chen Date: Mon, 10 Apr 2017 18:58:31 -0700 Subject: [PATCH 2/2] set 1ms as container duration minimum value Signed-off-by: Dong Chen --- api/swagger.yaml | 6 ++-- api/types/container/config.go | 6 ++++ builder/dockerfile/dispatchers.go | 6 ++-- builder/dockerfile/dispatchers_test.go | 18 ++++++++++ daemon/container.go | 12 +++---- docs/api/v1.24.md | 8 +++-- integration-cli/docker_api_create_test.go | 44 ++++++++++++++++------- 7 files changed, 72 insertions(+), 28 deletions(-) diff --git a/api/swagger.yaml b/api/swagger.yaml index 1a6d7503ff..69fb193321 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -499,16 +499,16 @@ definitions: items: type: "string" Interval: - description: "The time to wait between checks in nanoseconds. It should be 0 or not less than 1000000000(1s). 0 means inherit." + description: "The time to wait between checks in nanoseconds. It should be 0 or at least 1000000 (1 ms). 0 means inherit." type: "integer" Timeout: - description: "The time to wait before considering the check to have hung. It should be 0 or not less than 1000000000(1s). 0 means inherit." + description: "The time to wait before considering the check to have hung. It should be 0 or at least 1000000 (1 ms). 0 means inherit." type: "integer" Retries: description: "The number of consecutive failures needed to consider a container as unhealthy. 0 means inherit." type: "integer" StartPeriod: - description: "Start period for the container to initialize before starting health-retries countdown in nanoseconds. 0 means inherit." + description: "Start period for the container to initialize before starting health-retries countdown in nanoseconds. It should be 0 or at least 1000000 (1 ms). 0 means inherit." type: "integer" HostConfig: diff --git a/api/types/container/config.go b/api/types/container/config.go index 02e1b87a78..55a03fc981 100644 --- a/api/types/container/config.go +++ b/api/types/container/config.go @@ -7,6 +7,12 @@ import ( "github.com/docker/go-connections/nat" ) +// MinimumDuration puts a minimum on user configured duration. +// This is to prevent API error on time unit. For example, API may +// set 3 as healthcheck interval with intention of 3 seconds, but +// Docker interprets it as 3 nanoseconds. +const MinimumDuration = 1 * time.Millisecond + // HealthConfig holds configuration settings for the HEALTHCHECK feature. type HealthConfig struct { // Test is the test to perform to check that the container is healthy. diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 9a91756002..e8b2768c86 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -497,7 +497,7 @@ func cmd(b *Builder, args []string, attributes map[string]bool, original string) } // parseOptInterval(flag) is the duration of flag.Value, or 0 if -// empty. An error is reported if the value is given and less than 1 second. +// empty. An error is reported if the value is given and less than minimum duration. func parseOptInterval(f *Flag) (time.Duration, error) { s := f.Value if s == "" { @@ -507,8 +507,8 @@ func parseOptInterval(f *Flag) (time.Duration, error) { if err != nil { return 0, err } - if d < time.Duration(time.Second) { - return 0, fmt.Errorf("Interval %#v cannot be less than 1 second", f.name) + if d < time.Duration(container.MinimumDuration) { + return 0, fmt.Errorf("Interval %#v cannot be less than %s", f.name, container.MinimumDuration) } return d, nil } diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index 2ce8d75e53..f877798bd8 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/builder/dockerfile/dispatchers_test.go @@ -530,3 +530,21 @@ func TestShell(t *testing.T) { t.Fatalf("Shell should be set to %s, got %s", expectedShell, b.runConfig.Shell) } } + +func TestParseOptInterval(t *testing.T) { + flInterval := &Flag{ + name: "interval", + flagType: stringType, + Value: "50ns", + } + _, err := parseOptInterval(flInterval) + if err == nil { + t.Fatalf("Error should be presented for interval %s", flInterval.Value) + } + + flInterval.Value = "1ms" + _, err = parseOptInterval(flInterval) + if err != nil { + t.Fatalf("Unexpected error: %s", err.Error()) + } +} diff --git a/daemon/container.go b/daemon/container.go index a3482687b7..6a660ba4f3 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -244,20 +244,20 @@ func (daemon *Daemon) verifyContainerSettings(hostConfig *containertypes.HostCon // Validate the healthcheck params of Config if config.Healthcheck != nil { - if config.Healthcheck.Interval != 0 && config.Healthcheck.Interval < time.Millisecond { - return nil, fmt.Errorf("Interval in Healthcheck cannot be less than one millisecond") + if config.Healthcheck.Interval != 0 && config.Healthcheck.Interval < containertypes.MinimumDuration { + return nil, fmt.Errorf("Interval in Healthcheck cannot be less than %s", containertypes.MinimumDuration) } - if config.Healthcheck.Timeout != 0 && config.Healthcheck.Timeout < time.Millisecond { - return nil, fmt.Errorf("Timeout in Healthcheck cannot be less than one millisecond") + if config.Healthcheck.Timeout != 0 && config.Healthcheck.Timeout < containertypes.MinimumDuration { + return nil, fmt.Errorf("Timeout in Healthcheck cannot be less than %s", containertypes.MinimumDuration) } if config.Healthcheck.Retries < 0 { return nil, fmt.Errorf("Retries in Healthcheck cannot be negative") } - if config.Healthcheck.StartPeriod != 0 && config.Healthcheck.StartPeriod < time.Millisecond { - return nil, fmt.Errorf("StartPeriod in Healthcheck cannot be less than one millisecond") + if config.Healthcheck.StartPeriod != 0 && config.Healthcheck.StartPeriod < containertypes.MinimumDuration { + return nil, fmt.Errorf("StartPeriod in Healthcheck cannot be less than %s", containertypes.MinimumDuration) } } } diff --git a/docs/api/v1.24.md b/docs/api/v1.24.md index b7586c93ea..4adfcab949 100644 --- a/docs/api/v1.24.md +++ b/docs/api/v1.24.md @@ -285,7 +285,8 @@ Create a container "Test": ["CMD-SHELL", "curl localhost:3000"], "Interval": 1000000000, "Timeout": 10000000000, - "Retries": 10 + "Retries": 10, + "StartPeriod": 60000000000 }, "WorkingDir": "", "NetworkDisabled": false, @@ -397,9 +398,10 @@ Create a container + `{"NONE"}` disable healthcheck + `{"CMD", args...}` exec arguments directly + `{"CMD-SHELL", command}` run command with system's default shell - - **Interval** - The time to wait between checks in nanoseconds. It should be 0 or not less than 1000000000(1s). 0 means inherit. - - **Timeout** - The time to wait before considering the check to have hung. It should be 0 or not less than 1000000000(1s). 0 means inherit. + - **Interval** - The time to wait between checks in nanoseconds. It should be 0 or at least 1000000 (1 ms). 0 means inherit. + - **Timeout** - The time to wait before considering the check to have hung. It should be 0 or at least 1000000 (1 ms). 0 means inherit. - **Retries** - The number of consecutive failures needed to consider a container as unhealthy. 0 means inherit. + - **StartPeriod** - The time to wait for container initialization before starting health-retries countdown in nanoseconds. It should be 0 or at least 1000000 (1 ms). 0 means inherit. - **WorkingDir** - A string specifying the working directory for commands to run in. - **NetworkDisabled** - Boolean value, when true disables networking for the diff --git a/integration-cli/docker_api_create_test.go b/integration-cli/docker_api_create_test.go index 7328f4d068..e404b6cf58 100644 --- a/integration-cli/docker_api_create_test.go +++ b/integration-cli/docker_api_create_test.go @@ -1,9 +1,11 @@ package main import ( + "fmt" "net/http" "time" + "github.com/docker/docker/api/types/container" "github.com/docker/docker/integration-cli/checker" "github.com/docker/docker/integration-cli/request" "github.com/go-check/check" @@ -91,8 +93,8 @@ func (s *DockerSuite) TestAPICreateWithInvalidHealthcheckParams(c *check.C) { config := map[string]interface{}{ "Image": "busybox", "Healthcheck": map[string]interface{}{ - "Interval": time.Duration(-10000000), - "Timeout": time.Duration(1000000000), + "Interval": -10 * time.Millisecond, + "Timeout": time.Second, "Retries": int(1000), }, } @@ -100,39 +102,38 @@ func (s *DockerSuite) TestAPICreateWithInvalidHealthcheckParams(c *check.C) { status, body, err := request.SockRequest("POST", "/containers/create?name="+name, config, daemonHost()) c.Assert(err, check.IsNil) c.Assert(status, check.Equals, http.StatusInternalServerError) - expected := "Interval in Healthcheck cannot be less than one second" + expected := fmt.Sprintf("Interval in Healthcheck cannot be less than %s", container.MinimumDuration) c.Assert(getErrorMessage(c, body), checker.Contains, expected) - // test invalid Interval in Healthcheck: larger than 0s but less than 1s + // test invalid Interval in Healthcheck: larger than 0s but less than 1ms name = "test2" config = map[string]interface{}{ "Image": "busybox", "Healthcheck": map[string]interface{}{ - "Interval": time.Duration(500000000), - "Timeout": time.Duration(1000000000), + "Interval": 500 * time.Microsecond, + "Timeout": time.Second, "Retries": int(1000), }, } status, body, err = request.SockRequest("POST", "/containers/create?name="+name, config, daemonHost()) c.Assert(err, check.IsNil) c.Assert(status, check.Equals, http.StatusInternalServerError) - expected = "Interval in Healthcheck cannot be less than one second" c.Assert(getErrorMessage(c, body), checker.Contains, expected) - // test invalid Timeout in Healthcheck: less than 1s + // test invalid Timeout in Healthcheck: less than 1ms name = "test3" config = map[string]interface{}{ "Image": "busybox", "Healthcheck": map[string]interface{}{ - "Interval": time.Duration(1000000000), - "Timeout": time.Duration(-100000000), + "Interval": time.Second, + "Timeout": -100 * time.Millisecond, "Retries": int(1000), }, } status, body, err = request.SockRequest("POST", "/containers/create?name="+name, config, daemonHost()) c.Assert(err, check.IsNil) c.Assert(status, check.Equals, http.StatusInternalServerError) - expected = "Timeout in Healthcheck cannot be less than one second" + expected = fmt.Sprintf("Timeout in Healthcheck cannot be less than %s", container.MinimumDuration) c.Assert(getErrorMessage(c, body), checker.Contains, expected) // test invalid Retries in Healthcheck: less than 0 @@ -140,8 +141,8 @@ func (s *DockerSuite) TestAPICreateWithInvalidHealthcheckParams(c *check.C) { config = map[string]interface{}{ "Image": "busybox", "Healthcheck": map[string]interface{}{ - "Interval": time.Duration(1000000000), - "Timeout": time.Duration(1000000000), + "Interval": time.Second, + "Timeout": time.Second, "Retries": int(-10), }, } @@ -150,4 +151,21 @@ func (s *DockerSuite) TestAPICreateWithInvalidHealthcheckParams(c *check.C) { c.Assert(status, check.Equals, http.StatusInternalServerError) expected = "Retries in Healthcheck cannot be negative" c.Assert(getErrorMessage(c, body), checker.Contains, expected) + + // test invalid StartPeriod in Healthcheck: not 0 and less than 1ms + name = "test3" + config = map[string]interface{}{ + "Image": "busybox", + "Healthcheck": map[string]interface{}{ + "Interval": time.Second, + "Timeout": time.Second, + "Retries": int(1000), + "StartPeriod": 100 * time.Microsecond, + }, + } + status, body, err = request.SockRequest("POST", "/containers/create?name="+name, config, daemonHost()) + c.Assert(err, check.IsNil) + c.Assert(status, check.Equals, http.StatusInternalServerError) + expected = fmt.Sprintf("StartPeriod in Healthcheck cannot be less than %s", container.MinimumDuration) + c.Assert(getErrorMessage(c, body), checker.Contains, expected) }