Merge pull request #31582 from dnephin/misc-flag-cleanup

Cleanup some memory flags used in the CLI

cli/command/container/opts.go

@@ -18,7 +18,6 @@ import (
 	"github.com/docker/docker/pkg/signal"
 	runconfigopts "github.com/docker/docker/runconfig/opts"
 	"github.com/docker/go-connections/nat"
-	units "github.com/docker/go-units"
 	"github.com/spf13/pflag"
 )
 
@@ -72,10 +71,10 @@ type containerOptions struct {
 	containerIDFile    string
 	entrypoint         string
 	hostname           string
-	memoryString       string
-	memoryReservation  string
-	memorySwap         string
-	kernelMemory       string
+	memory             opts.MemBytes
+	memoryReservation  opts.MemBytes
+	memorySwap         opts.MemSwapBytes
+	kernelMemory       opts.MemBytes
 	user               string
 	workingDir         string
 	cpuCount           int64
@@ -89,7 +88,7 @@ type containerOptions struct {
 	cpusetCpus         string
 	cpusetMems         string
 	blkioWeight        uint16
-	ioMaxBandwidth     string
+	ioMaxBandwidth     opts.MemBytes
 	ioMaxIOps          uint64
 	swappiness         int64
 	netMode            string
@@ -254,12 +253,12 @@ func addFlags(flags *pflag.FlagSet) *containerOptions {
 	flags.Var(&copts.deviceReadIOps, "device-read-iops", "Limit read rate (IO per second) from a device")
 	flags.Var(&copts.deviceWriteBps, "device-write-bps", "Limit write rate (bytes per second) to a device")
 	flags.Var(&copts.deviceWriteIOps, "device-write-iops", "Limit write rate (IO per second) to a device")
-	flags.StringVar(&copts.ioMaxBandwidth, "io-maxbandwidth", "", "Maximum IO bandwidth limit for the system drive (Windows only)")
+	flags.Var(&copts.ioMaxBandwidth, "io-maxbandwidth", "Maximum IO bandwidth limit for the system drive (Windows only)")
 	flags.Uint64Var(&copts.ioMaxIOps, "io-maxiops", 0, "Maximum IOps limit for the system drive (Windows only)")
-	flags.StringVar(&copts.kernelMemory, "kernel-memory", "", "Kernel memory limit")
-	flags.StringVarP(&copts.memoryString, "memory", "m", "", "Memory limit")
-	flags.StringVar(&copts.memoryReservation, "memory-reservation", "", "Memory soft limit")
-	flags.StringVar(&copts.memorySwap, "memory-swap", "", "Swap limit equal to memory plus swap: '-1' to enable unlimited swap")
+	flags.Var(&copts.kernelMemory, "kernel-memory", "Kernel memory limit")
+	flags.VarP(&copts.memory, "memory", "m", "Memory limit")
+	flags.Var(&copts.memoryReservation, "memory-reservation", "Memory soft limit")
+	flags.Var(&copts.memorySwap, "memory-swap", "Swap limit equal to memory plus swap: '-1' to enable unlimited swap")
 	flags.Int64Var(&copts.swappiness, "memory-swappiness", -1, "Tune container memory swappiness (0 to 100)")
 	flags.BoolVar(&copts.oomKillDisable, "oom-kill-disable", false, "Disable OOM Killer")
 	flags.IntVar(&copts.oomScoreAdj, "oom-score-adj", 0, "Tune host's OOM preferences (-1000 to 1000)")
@@ -308,59 +307,11 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 
 	var err error
 
-	var memory int64
-	if copts.memoryString != "" {
-		memory, err = units.RAMInBytes(copts.memoryString)
-		if err != nil {
-			return nil, nil, nil, err
-		}
-	}
-
-	var memoryReservation int64
-	if copts.memoryReservation != "" {
-		memoryReservation, err = units.RAMInBytes(copts.memoryReservation)
-		if err != nil {
-			return nil, nil, nil, err
-		}
-	}
-
-	var memorySwap int64
-	if copts.memorySwap != "" {
-		if copts.memorySwap == "-1" {
-			memorySwap = -1
-		} else {
-			memorySwap, err = units.RAMInBytes(copts.memorySwap)
-			if err != nil {
-				return nil, nil, nil, err
-			}
-		}
-	}
-
-	var kernelMemory int64
-	if copts.kernelMemory != "" {
-		kernelMemory, err = units.RAMInBytes(copts.kernelMemory)
-		if err != nil {
-			return nil, nil, nil, err
-		}
-	}
-
 	swappiness := copts.swappiness
 	if swappiness != -1 && (swappiness < 0 || swappiness > 100) {
 		return nil, nil, nil, fmt.Errorf("invalid value: %d. Valid memory swappiness range is 0-100", swappiness)
 	}
 
-	// TODO FIXME units.RAMInBytes should have a uint64 version
-	var maxIOBandwidth int64
-	if copts.ioMaxBandwidth != "" {
-		maxIOBandwidth, err = units.RAMInBytes(copts.ioMaxBandwidth)
-		if err != nil {
-			return nil, nil, nil, err
-		}
-		if maxIOBandwidth < 0 {
-			return nil, nil, nil, fmt.Errorf("invalid value: %s. Maximum IO Bandwidth must be positive", copts.ioMaxBandwidth)
-		}
-	}
-
 	var binds []string
 	volumes := copts.volumes.GetMap()
 	// add any bind targets to the list of container volumes
@@ -530,11 +481,11 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 
 	resources := container.Resources{
 		CgroupParent:         copts.cgroupParent,
-		Memory:               memory,
-		MemoryReservation:    memoryReservation,
-		MemorySwap:           memorySwap,
+		Memory:               copts.memory.Value(),
+		MemoryReservation:    copts.memoryReservation.Value(),
+		MemorySwap:           copts.memorySwap.Value(),
 		MemorySwappiness:     &copts.swappiness,
-		KernelMemory:         kernelMemory,
+		KernelMemory:         copts.kernelMemory.Value(),
 		OomKillDisable:       &copts.oomKillDisable,
 		NanoCPUs:             copts.cpus.Value(),
 		CPUCount:             copts.cpuCount,
@@ -554,7 +505,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 		BlkioDeviceReadIOps:  copts.deviceReadIOps.GetList(),
 		BlkioDeviceWriteIOps: copts.deviceWriteIOps.GetList(),
 		IOMaximumIOps:        copts.ioMaxIOps,
-		IOMaximumBandwidth:   uint64(maxIOBandwidth),
+		IOMaximumBandwidth:   uint64(copts.ioMaxBandwidth),
 		Ulimits:              copts.ulimits.GetList(),
 		DeviceCgroupRules:    copts.deviceCgroupRules.GetAll(),
 		Devices:              deviceMappings,

cli/command/container/opts_test.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/docker/docker/api/types/container"
 	networktypes "github.com/docker/docker/api/types/network"
+	"github.com/docker/docker/pkg/testutil/assert"
 	"github.com/docker/docker/runconfig"
 	"github.com/docker/go-connections/nat"
 	"github.com/spf13/pflag"
@@ -235,28 +236,24 @@ func TestParseWithMacAddress(t *testing.T) {
 
 func TestParseWithMemory(t *testing.T) {
 	invalidMemory := "--memory=invalid"
-	validMemory := "--memory=1G"
-	if _, _, _, err := parseRun([]string{invalidMemory, "img", "cmd"}); err != nil && err.Error() != "invalid size: 'invalid'" {
-		t.Fatalf("Expected an error with '%v' Memory, got '%v'", invalidMemory, err)
-	}
-	if _, hostconfig := mustParse(t, validMemory); hostconfig.Memory != 1073741824 {
-		t.Fatalf("Expected the config to have '1G' as Memory, got '%v'", hostconfig.Memory)
-	}
+	_, _, _, err := parseRun([]string{invalidMemory, "img", "cmd"})
+	assert.Error(t, err, invalidMemory)
+
+	_, hostconfig := mustParse(t, "--memory=1G")
+	assert.Equal(t, hostconfig.Memory, int64(1073741824))
 }
 
 func TestParseWithMemorySwap(t *testing.T) {
 	invalidMemory := "--memory-swap=invalid"
-	validMemory := "--memory-swap=1G"
-	anotherValidMemory := "--memory-swap=-1"
-	if _, _, _, err := parseRun([]string{invalidMemory, "img", "cmd"}); err == nil || err.Error() != "invalid size: 'invalid'" {
-		t.Fatalf("Expected an error with '%v' MemorySwap, got '%v'", invalidMemory, err)
-	}
-	if _, hostconfig := mustParse(t, validMemory); hostconfig.MemorySwap != 1073741824 {
-		t.Fatalf("Expected the config to have '1073741824' as MemorySwap, got '%v'", hostconfig.MemorySwap)
-	}
-	if _, hostconfig := mustParse(t, anotherValidMemory); hostconfig.MemorySwap != -1 {
-		t.Fatalf("Expected the config to have '-1' as MemorySwap, got '%v'", hostconfig.MemorySwap)
-	}
+
+	_, _, _, err := parseRun([]string{invalidMemory, "img", "cmd"})
+	assert.Error(t, err, invalidMemory)
+
+	_, hostconfig := mustParse(t, "--memory-swap=1G")
+	assert.Equal(t, hostconfig.MemorySwap, int64(1073741824))
+
+	_, hostconfig = mustParse(t, "--memory-swap=-1")
+	assert.Equal(t, hostconfig.MemorySwap, int64(-1))
 }
 
 func TestParseHostname(t *testing.T) {

cli/command/container/update.go

@@ -8,8 +8,8 @@ import (
 	containertypes "github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/cli"
 	"github.com/docker/docker/cli/command"
+	"github.com/docker/docker/opts"
 	runconfigopts "github.com/docker/docker/runconfig/opts"
-	"github.com/docker/go-units"
 	"github.com/spf13/cobra"
 	"golang.org/x/net/context"
 )
@@ -23,10 +23,10 @@ type updateOptions struct {
 	cpusetCpus         string
 	cpusetMems         string
 	cpuShares          int64
-	memoryString       string
-	memoryReservation  string
-	memorySwap         string
-	kernelMemory       string
+	memory             opts.MemBytes
+	memoryReservation  opts.MemBytes
+	memorySwap         opts.MemSwapBytes
+	kernelMemory       opts.MemBytes
 	restartPolicy      string
 
 	nFlag int
@@ -60,10 +60,10 @@ func NewUpdateCommand(dockerCli *command.DockerCli) *cobra.Command {
 	flags.StringVar(&opts.cpusetCpus, "cpuset-cpus", "", "CPUs in which to allow execution (0-3, 0,1)")
 	flags.StringVar(&opts.cpusetMems, "cpuset-mems", "", "MEMs in which to allow execution (0-3, 0,1)")
 	flags.Int64VarP(&opts.cpuShares, "cpu-shares", "c", 0, "CPU shares (relative weight)")
-	flags.StringVarP(&opts.memoryString, "memory", "m", "", "Memory limit")
-	flags.StringVar(&opts.memoryReservation, "memory-reservation", "", "Memory soft limit")
-	flags.StringVar(&opts.memorySwap, "memory-swap", "", "Swap limit equal to memory plus swap: '-1' to enable unlimited swap")
-	flags.StringVar(&opts.kernelMemory, "kernel-memory", "", "Kernel memory limit")
+	flags.VarP(&opts.memory, "memory", "m", "Memory limit")
+	flags.Var(&opts.memoryReservation, "memory-reservation", "Memory soft limit")
+	flags.Var(&opts.memorySwap, "memory-swap", "Swap limit equal to memory plus swap: '-1' to enable unlimited swap")
+	flags.Var(&opts.kernelMemory, "kernel-memory", "Kernel memory limit")
 	flags.StringVar(&opts.restartPolicy, "restart", "", "Restart policy to apply when a container exits")
 
 	return cmd
@@ -76,42 +76,6 @@ func runUpdate(dockerCli *command.DockerCli, opts *updateOptions) error {
 		return errors.New("You must provide one or more flags when using this command.")
 	}
 
-	var memory int64
-	if opts.memoryString != "" {
-		memory, err = units.RAMInBytes(opts.memoryString)
-		if err != nil {
-			return err
-		}
-	}
-
-	var memoryReservation int64
-	if opts.memoryReservation != "" {
-		memoryReservation, err = units.RAMInBytes(opts.memoryReservation)
-		if err != nil {
-			return err
-		}
-	}
-
-	var memorySwap int64
-	if opts.memorySwap != "" {
-		if opts.memorySwap == "-1" {
-			memorySwap = -1
-		} else {
-			memorySwap, err = units.RAMInBytes(opts.memorySwap)
-			if err != nil {
-				return err
-			}
-		}
-	}
-
-	var kernelMemory int64
-	if opts.kernelMemory != "" {
-		kernelMemory, err = units.RAMInBytes(opts.kernelMemory)
-		if err != nil {
-			return err
-		}
-	}
-
 	var restartPolicy containertypes.RestartPolicy
 	if opts.restartPolicy != "" {
 		restartPolicy, err = runconfigopts.ParseRestartPolicy(opts.restartPolicy)
@@ -125,10 +89,10 @@ func runUpdate(dockerCli *command.DockerCli, opts *updateOptions) error {
 		CpusetCpus:         opts.cpusetCpus,
 		CpusetMems:         opts.cpusetMems,
 		CPUShares:          opts.cpuShares,
-		Memory:             memory,
-		MemoryReservation:  memoryReservation,
-		MemorySwap:         memorySwap,
-		KernelMemory:       kernelMemory,
+		Memory:             opts.memory.Value(),
+		MemoryReservation:  opts.memoryReservation.Value(),
+		MemorySwap:         opts.memorySwap.Value(),
+		KernelMemory:       opts.kernelMemory.Value(),
 		CPUPeriod:          opts.cpuPeriod,
 		CPUQuota:           opts.cpuQuota,
 		CPURealtimePeriod:  opts.cpuRealtimePeriod,

cli/command/image/build.go

@@ -40,8 +40,8 @@ type buildOptions struct {
 	buildArgs      opts.ListOpts
 	extraHosts     opts.ListOpts
 	ulimits        *opts.UlimitOpt
-	memory         string
-	memorySwap     string
+	memory         opts.MemBytes
+	memorySwap     opts.MemSwapBytes
 	shmSize        opts.MemBytes
 	cpuShares      int64
 	cpuPeriod      int64
@@ -89,8 +89,8 @@ func NewBuildCommand(dockerCli *command.DockerCli) *cobra.Command {
 	flags.Var(&options.buildArgs, "build-arg", "Set build-time variables")
 	flags.Var(options.ulimits, "ulimit", "Ulimit options")
 	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.VarP(&options.memory, "memory", "m", "Memory limit")
+	flags.Var(&options.memorySwap, "memory-swap", "Swap limit equal to memory plus swap: '-1' to enable unlimited swap")
 	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")
@@ -138,7 +138,6 @@ func (out *lastProgressOutput) WriteProgress(prog progress.Progress) error {
 }
 
 func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
-
 	var (
 		buildCtx      io.ReadCloser
 		err           error
@@ -255,32 +254,10 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 
 	var body io.Reader = progress.NewProgressReader(buildCtx, progressOutput, 0, "", "Sending build context to Docker daemon")
 
-	var memory int64
-	if options.memory != "" {
-		parsedMemory, err := units.RAMInBytes(options.memory)
-		if err != nil {
-			return err
-		}
-		memory = parsedMemory
-	}
-
-	var memorySwap int64
-	if options.memorySwap != "" {
-		if options.memorySwap == "-1" {
-			memorySwap = -1
-		} else {
-			parsedMemorySwap, err := units.RAMInBytes(options.memorySwap)
-			if err != nil {
-				return err
-			}
-			memorySwap = parsedMemorySwap
-		}
-	}
-
 	authConfigs, _ := dockerCli.GetAllCredentials()
 	buildOptions := types.ImageBuildOptions{
-		Memory:         memory,
-		MemorySwap:     memorySwap,
+		Memory:         options.memory.Value(),
+		MemorySwap:     options.memorySwap.Value(),
 		Tags:           options.tags.GetAll(),
 		SuppressOutput: options.quiet,
 		NoCache:        options.noCache,
@@ -333,7 +310,11 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 	// Windows: show error message about modified file permissions if the
 	// daemon isn't running Windows.
 	if response.OSType != "windows" && runtime.GOOS == "windows" && !options.quiet {
-		fmt.Fprintln(dockerCli.Out(), `SECURITY WARNING: You are building a Docker image from Windows against a non-Windows Docker host. All files and directories added to build context will have '-rwxr-xr-x' permissions. It is recommended to double check and reset permissions for sensitive files and directories.`)
+		fmt.Fprintln(dockerCli.Out(), "SECURITY WARNING: You are building a Docker "+
+			"image from Windows against a non-Windows Docker host. All files and "+
+			"directories added to build context will have '-rwxr-xr-x' permissions. "+
+			"It is recommended to double check and reset permissions for sensitive "+
+			"files and directories.")
 	}
 
 	// Everything worked so if -q was provided the output from the daemon

opts/opts.go

@@ -444,3 +444,39 @@ func (m *MemBytes) UnmarshalJSON(s []byte) error {
 	*m = MemBytes(val)
 	return err
 }
+
+// MemSwapBytes is a type for human readable memory bytes (like 128M, 2g, etc).
+// It differs from MemBytes in that -1 is valid and the default.
+type MemSwapBytes int64
+
+// Set sets the value of the MemSwapBytes by passing a string
+func (m *MemSwapBytes) Set(value string) error {
+	if value == "-1" {
+		*m = MemSwapBytes(-1)
+		return nil
+	}
+	val, err := units.RAMInBytes(value)
+	*m = MemSwapBytes(val)
+	return err
+}
+
+// Type returns the type
+func (m *MemSwapBytes) Type() string {
+	return "bytes"
+}
+
+// Value returns the value in int64
+func (m *MemSwapBytes) Value() int64 {
+	return int64(*m)
+}
+
+func (m *MemSwapBytes) String() string {
+	b := MemBytes(*m)
+	return b.String()
+}
+
+// UnmarshalJSON is the customized unmarshaler for MemSwapBytes
+func (m *MemSwapBytes) UnmarshalJSON(s []byte) error {
+	b := MemBytes(*m)
+	return b.UnmarshalJSON(s)
+}