From d1982862cacb74fb28f8103d0774960fd59a5373 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Wed, 28 Dec 2016 14:44:07 -0800 Subject: [PATCH] Update opts.MemBytes to disable default, and move `docker run/create/build` to use opts.MemBytes This fix made several updates: 1. Update opts.MemBytes so that default value will not show up. The reason is that in case a default value is decided by daemon, instead of client, we actually want to not show default value. 2. Move `docker run/create/build` to use opts.MemBytes for `--shm-size` This is to bring consistency between daemon and docker run 3. docs updates. Signed-off-by: Yong Tang --- cli/command/container/opts.go | 14 +++----------- cli/command/container/opts_test.go | 5 +++-- cli/command/image/build.go | 14 +++----------- docs/reference/commandline/build.md | 2 +- docs/reference/commandline/create.md | 2 +- docs/reference/commandline/run.md | 2 +- docs/reference/commandline/service_create.md | 4 ++-- docs/reference/commandline/service_update.md | 4 ++-- integration-cli/docker_cli_events_unix_test.go | 2 +- opts/opts.go | 8 +++++++- 10 files changed, 24 insertions(+), 33 deletions(-) diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index 55cc3c3b29..4f70c7a921 100644 --- a/cli/command/container/opts.go +++ b/cli/command/container/opts.go @@ -100,7 +100,7 @@ type containerOptions struct { stopSignal string stopTimeout int isolation string - shmSize string + shmSize opts.MemBytes noHealthcheck bool healthCmd string healthInterval time.Duration @@ -259,7 +259,7 @@ func addFlags(flags *pflag.FlagSet) *containerOptions { flags.StringVar(&copts.ipcMode, "ipc", "", "IPC namespace to use") flags.StringVar(&copts.isolation, "isolation", "", "Container isolation technology") flags.StringVar(&copts.pidMode, "pid", "", "PID namespace to use") - flags.StringVar(&copts.shmSize, "shm-size", "", "Size of /dev/shm, default value is 64MB") + flags.Var(&copts.shmSize, "shm-size", "Size of /dev/shm") flags.StringVar(&copts.utsMode, "uts", "", "UTS namespace to use") flags.StringVar(&copts.runtime, "runtime", "", "Runtime to use for this container") @@ -336,14 +336,6 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c return nil, nil, nil, fmt.Errorf("invalid value: %d. Valid memory swappiness range is 0-100", swappiness) } - var shmSize int64 - if copts.shmSize != "" { - shmSize, err = units.RAMInBytes(copts.shmSize) - if err != nil { - return nil, nil, nil, err - } - } - // TODO FIXME units.RAMInBytes should have a uint64 version var maxIOBandwidth int64 if copts.ioMaxBandwidth != "" { @@ -615,7 +607,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c LogConfig: container.LogConfig{Type: copts.loggingDriver, Config: loggingOpts}, VolumeDriver: copts.volumeDriver, Isolation: container.Isolation(copts.isolation), - ShmSize: shmSize, + ShmSize: copts.shmSize.Value(), Resources: resources, Tmpfs: tmpfs, Sysctls: copts.sysctls.GetAll(), diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go index ce3bb21b41..d0655069e9 100644 --- a/cli/command/container/opts_test.go +++ b/cli/command/container/opts_test.go @@ -411,8 +411,9 @@ func TestParseModes(t *testing.T) { t.Fatalf("Expected a valid UTSMode, got %v", hostconfig.UTSMode) } // shm-size ko - if _, _, _, err = parseRun([]string{"--shm-size=a128m", "img", "cmd"}); err == nil || err.Error() != "invalid size: 'a128m'" { - t.Fatalf("Expected an error with message 'invalid size: a128m', got %v", err) + expectedErr := `invalid argument "a128m" for --shm-size=a128m: invalid size: 'a128m'` + if _, _, _, err = parseRun([]string{"--shm-size=a128m", "img", "cmd"}); err == nil || err.Error() != expectedErr { + t.Fatalf("Expected an error with message '%v', got %v", expectedErr, err) } // shm-size ok _, hostconfig, _, err = parseRun([]string{"--shm-size=128m", "img", "cmd"}) diff --git a/cli/command/image/build.go b/cli/command/image/build.go index 67753bea14..09625d5b2c 100644 --- a/cli/command/image/build.go +++ b/cli/command/image/build.go @@ -41,7 +41,7 @@ type buildOptions struct { ulimits *opts.UlimitOpt memory string memorySwap string - shmSize string + shmSize opts.MemBytes cpuShares int64 cpuPeriod int64 cpuQuota int64 @@ -89,7 +89,7 @@ func NewBuildCommand(dockerCli *command.DockerCli) *cobra.Command { flags.StringVarP(&options.dockerfileName, "file", "f", "", "Name of the Dockerfile (Default is 'PATH/Dockerfile')") flags.StringVarP(&options.memory, "memory", "m", "", "Memory limit") flags.StringVar(&options.memorySwap, "memory-swap", "", "Swap limit equal to memory plus swap: '-1' to enable unlimited swap") - flags.StringVar(&options.shmSize, "shm-size", "", "Size of /dev/shm, default value is 64MB") + flags.Var(&options.shmSize, "shm-size", "Size of /dev/shm") flags.Int64VarP(&options.cpuShares, "cpu-shares", "c", 0, "CPU shares (relative weight)") flags.Int64Var(&options.cpuPeriod, "cpu-period", 0, "Limit the CPU CFS (Completely Fair Scheduler) period") flags.Int64Var(&options.cpuQuota, "cpu-quota", 0, "Limit the CPU CFS (Completely Fair Scheduler) quota") @@ -271,14 +271,6 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error { } } - var shmSize int64 - if options.shmSize != "" { - shmSize, err = units.RAMInBytes(options.shmSize) - if err != nil { - return err - } - } - authConfigs, _ := dockerCli.GetAllCredentials() buildOptions := types.ImageBuildOptions{ Memory: memory, @@ -297,7 +289,7 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error { CPUPeriod: options.cpuPeriod, CgroupParent: options.cgroupParent, Dockerfile: relDockerfile, - ShmSize: shmSize, + ShmSize: options.shmSize.Value(), Ulimits: options.ulimits.GetList(), BuildArgs: runconfigopts.ConvertKVStringsToMapWithNil(options.buildArgs.GetAll()), AuthConfigs: authConfigs, diff --git a/docs/reference/commandline/build.md b/docs/reference/commandline/build.md index 2f8c59c387..5b6f1fec61 100644 --- a/docs/reference/commandline/build.md +++ b/docs/reference/commandline/build.md @@ -49,7 +49,7 @@ Options: -q, --quiet Suppress the build output and print image ID on success --rm Remove intermediate containers after a successful build (default true) --security-opt value Security Options (default []) - --shm-size string Size of /dev/shm, default value is 64MB. + --shm-size bytes Size of /dev/shm The format is ``. `number` must be greater than `0`. Unit is optional and can be `b` (bytes), `k` (kilobytes), `m` (megabytes), or `g` (gigabytes). If you omit the unit, the system uses bytes. diff --git a/docs/reference/commandline/create.md b/docs/reference/commandline/create.md index 253c72da70..0504f806bb 100644 --- a/docs/reference/commandline/create.md +++ b/docs/reference/commandline/create.md @@ -106,7 +106,7 @@ Options: --rm Automatically remove the container when it exits --runtime string Runtime to use for this container --security-opt value Security Options (default []) - --shm-size string Size of /dev/shm, default value is 64MB. + --shm-size bytes Size of /dev/shm The format is ``. `number` must be greater than `0`. Unit is optional and can be `b` (bytes), `k` (kilobytes), `m` (megabytes), or `g` (gigabytes). If you omit the unit, the system uses bytes. diff --git a/docs/reference/commandline/run.md b/docs/reference/commandline/run.md index 9bacea4b1f..42253b01b4 100644 --- a/docs/reference/commandline/run.md +++ b/docs/reference/commandline/run.md @@ -116,7 +116,7 @@ Options: --rm Automatically remove the container when it exits --runtime string Runtime to use for this container --security-opt value Security Options (default []) - --shm-size string Size of /dev/shm, default value is 64MB. + --shm-size bytes Size of /dev/shm The format is ``. `number` must be greater than `0`. Unit is optional and can be `b` (bytes), `k` (kilobytes), `m` (megabytes), or `g` (gigabytes). If you omit the unit, the system uses bytes. diff --git a/docs/reference/commandline/service_create.md b/docs/reference/commandline/service_create.md index c9e298096b..721aef8364 100644 --- a/docs/reference/commandline/service_create.md +++ b/docs/reference/commandline/service_create.md @@ -39,7 +39,7 @@ Options: --hostname string Container hostname -l, --label list Service labels (default []) --limit-cpu decimal Limit CPUs (default 0.000) - --limit-memory bytes Limit Memory (default 0 B) + --limit-memory bytes Limit Memory --log-driver string Logging driver for service --log-opt list Logging driver options (default []) --mode string Service mode (replicated or global) (default "replicated") @@ -50,7 +50,7 @@ Options: -p, --publish port Publish a port as a node port --replicas uint Number of tasks --reserve-cpu decimal Reserve CPUs (default 0.000) - --reserve-memory bytes Reserve Memory (default 0 B) + --reserve-memory bytes Reserve Memory --restart-condition string Restart when condition is met (none, on-failure, or any) --restart-delay duration Delay between restart attempts (ns|us|ms|s|m|h) --restart-max-attempts uint Maximum number of restarts before giving up diff --git a/docs/reference/commandline/service_update.md b/docs/reference/commandline/service_update.md index 301a0eabe8..9c004c099a 100644 --- a/docs/reference/commandline/service_update.md +++ b/docs/reference/commandline/service_update.md @@ -50,7 +50,7 @@ Options: --label-add list Add or update a service label (default []) --label-rm list Remove a label by its key (default []) --limit-cpu decimal Limit CPUs (default 0.000) - --limit-memory bytes Limit Memory (default 0 B) + --limit-memory bytes Limit Memory --log-driver string Logging driver for service --log-opt list Logging driver options (default []) --mount-add mount Add or update a mount on a service @@ -60,7 +60,7 @@ Options: --publish-rm port Remove a published port by its target port --replicas uint Number of tasks --reserve-cpu decimal Reserve CPUs (default 0.000) - --reserve-memory bytes Reserve Memory (default 0 B) + --reserve-memory bytes Reserve Memory --restart-condition string Restart when condition is met (none, on-failure, or any) --restart-delay duration Delay between restart attempts (ns|us|ms|s|m|h) --restart-max-attempts uint Maximum number of restarts before giving up diff --git a/integration-cli/docker_cli_events_unix_test.go b/integration-cli/docker_cli_events_unix_test.go index eaffd08474..39650d2372 100644 --- a/integration-cli/docker_cli_events_unix_test.go +++ b/integration-cli/docker_cli_events_unix_test.go @@ -427,7 +427,7 @@ func (s *DockerDaemonSuite) TestDaemonEvents(c *check.C) { out, err = s.d.Cmd("events", "--since=0", "--until", daemonUnixTime(c)) c.Assert(err, checker.IsNil) - c.Assert(out, checker.Contains, fmt.Sprintf("daemon reload %s (cluster-advertise=, cluster-store=, cluster-store-opts={}, debug=true, default-runtime=runc, default-shm-size=67108864, insecure-registries=[], labels=[\"bar=foo\"], live-restore=false, max-concurrent-downloads=1, max-concurrent-uploads=5, name=%s, runtimes=runc:{docker-runc []}, shutdown-timeout=10)", daemonID, daemonName)) + c.Assert(out, checker.Contains, fmt.Sprintf("daemon reload %s (cluster-advertise=, cluster-store=, cluster-store-opts={}, debug=true, default-runtime=runc, default-shm-size=67108864, insecure-registries=[], labels=[\"bar=foo\"], live-restore=false, max-concurrent-downloads=1, max-concurrent-uploads=5, name=%s, registry-mirrors=[], runtimes=runc:{docker-runc []}, shutdown-timeout=10)", daemonID, daemonName)) } func (s *DockerDaemonSuite) TestDaemonEventsWithFilters(c *check.C) { diff --git a/opts/opts.go b/opts/opts.go index c7d6c291bb..3ae0fdb6b9 100644 --- a/opts/opts.go +++ b/opts/opts.go @@ -409,7 +409,13 @@ type MemBytes int64 // String returns the string format of the human readable memory bytes func (m *MemBytes) String() string { - return units.BytesSize(float64(m.Value())) + // NOTE: In spf13/pflag/flag.go, "0" is considered as "zero value" while "0 B" is not. + // We return "0" in case value is 0 here so that the default value is hidden. + // (Sometimes "default 0 B" is actually misleading) + if m.Value() != 0 { + return units.BytesSize(float64(m.Value())) + } + return "0" } // Set sets the value of the MemBytes by passing a string