Refactor container run cli command.

Signed-off-by: Daniel Nephin <dnephin@docker.com>

command/container/create.go

@@ -8,7 +8,6 @@ import (
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
-	networktypes "github.com/docker/docker/api/types/network"
 	"github.com/docker/docker/cli"
 	"github.com/docker/docker/cli/command"
 	"github.com/docker/docker/cli/command/image"
@@ -57,12 +56,12 @@ func NewCreateCommand(dockerCli *command.DockerCli) *cobra.Command {
 }
 
 func runCreate(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *createOptions, copts *containerOptions) error {
-	config, hostConfig, networkingConfig, err := parse(flags, copts)
+	containerConfig, err := parse(flags, copts)
 	if err != nil {
 		reportError(dockerCli.Err(), "create", err.Error(), true)
 		return cli.StatusError{StatusCode: 125}
 	}
-	response, err := createContainer(context.Background(), dockerCli, config, hostConfig, networkingConfig, hostConfig.ContainerIDFile, opts.name)
+	response, err := createContainer(context.Background(), dockerCli, containerConfig, opts.name)
 	if err != nil {
 		return err
 	}
@@ -146,7 +145,10 @@ func newCIDFile(path string) (*cidFile, error) {
 	return &cidFile{path: path, file: f}, nil
 }
 
-func createContainer(ctx context.Context, dockerCli *command.DockerCli, config *container.Config, hostConfig *container.HostConfig, networkingConfig *networktypes.NetworkingConfig, cidfile, name string) (*container.ContainerCreateCreatedBody, error) {
+func createContainer(ctx context.Context, dockerCli *command.DockerCli, containerConfig *containerConfig, name string) (*container.ContainerCreateCreatedBody, error) {
+	config := containerConfig.Config
+	hostConfig := containerConfig.HostConfig
+	networkingConfig := containerConfig.NetworkingConfig
 	stderr := dockerCli.Err()
 
 	var (
@@ -155,6 +157,7 @@ func createContainer(ctx context.Context, dockerCli *command.DockerCli, config *
 		namedRef        reference.Named
 	)
 
+	cidfile := hostConfig.ContainerIDFile
 	if cidfile != "" {
 		var err error
 		if containerIDFile, err = newCIDFile(cidfile); err != nil {

command/container/opts.go

@@ -285,10 +285,16 @@ func addFlags(flags *pflag.FlagSet) *containerOptions {
 	return copts
 }
 
+type containerConfig struct {
+	Config           *container.Config
+	HostConfig       *container.HostConfig
+	NetworkingConfig *networktypes.NetworkingConfig
+}
+
 // parse parses the args for the specified command and generates a Config,
 // a HostConfig and returns them with the specified command.
 // If the specified args are not valid, it will return an error.
-func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *container.HostConfig, *networktypes.NetworkingConfig, error) {
+func parse(flags *pflag.FlagSet, copts *containerOptions) (*containerConfig, error) {
 	var (
 		attachStdin  = copts.attach.Get("stdin")
 		attachStdout = copts.attach.Get("stdout")
@@ -298,7 +304,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 	// Validate the input mac address
 	if copts.macAddress != "" {
 		if _, err := opts.ValidateMACAddress(copts.macAddress); err != nil {
-			return nil, nil, nil, fmt.Errorf("%s is not a valid mac address", copts.macAddress)
+			return nil, fmt.Errorf("%s is not a valid mac address", copts.macAddress)
 		}
 	}
 	if copts.stdin {
@@ -314,7 +320,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 
 	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)
+		return nil, fmt.Errorf("invalid value: %d. Valid memory swappiness range is 0-100", swappiness)
 	}
 
 	var binds []string
@@ -359,13 +365,13 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 
 	ports, portBindings, err := nat.ParsePortSpecs(copts.publish.GetAll())
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 
 	// Merge in exposed ports to the map of published ports
 	for _, e := range copts.expose.GetAll() {
 		if strings.Contains(e, ":") {
-			return nil, nil, nil, fmt.Errorf("invalid port format for --expose: %s", e)
+			return nil, fmt.Errorf("invalid port format for --expose: %s", e)
 		}
 		//support two formats for expose, original format <portnum>/[<proto>] or <startport-endport>/[<proto>]
 		proto, port := nat.SplitProtoPort(e)
@@ -373,12 +379,12 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 		//if expose a port, the start and end port are the same
 		start, end, err := nat.ParsePortRange(port)
 		if err != nil {
-			return nil, nil, nil, fmt.Errorf("invalid range format for --expose: %s, error: %s", e, err)
+			return nil, fmt.Errorf("invalid range format for --expose: %s, error: %s", e, err)
 		}
 		for i := start; i <= end; i++ {
 			p, err := nat.NewPort(proto, strconv.FormatUint(i, 10))
 			if err != nil {
-				return nil, nil, nil, err
+				return nil, err
 			}
 			if _, exists := ports[p]; !exists {
 				ports[p] = struct{}{}
@@ -391,7 +397,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 	for _, device := range copts.devices.GetAll() {
 		deviceMapping, err := parseDevice(device)
 		if err != nil {
-			return nil, nil, nil, err
+			return nil, err
 		}
 		deviceMappings = append(deviceMappings, deviceMapping)
 	}
@@ -399,53 +405,53 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 	// collect all the environment variables for the container
 	envVariables, err := runconfigopts.ReadKVStrings(copts.envFile.GetAll(), copts.env.GetAll())
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 
 	// collect all the labels for the container
 	labels, err := runconfigopts.ReadKVStrings(copts.labelsFile.GetAll(), copts.labels.GetAll())
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 
 	ipcMode := container.IpcMode(copts.ipcMode)
 	if !ipcMode.Valid() {
-		return nil, nil, nil, fmt.Errorf("--ipc: invalid IPC mode")
+		return nil, fmt.Errorf("--ipc: invalid IPC mode")
 	}
 
 	pidMode := container.PidMode(copts.pidMode)
 	if !pidMode.Valid() {
-		return nil, nil, nil, fmt.Errorf("--pid: invalid PID mode")
+		return nil, fmt.Errorf("--pid: invalid PID mode")
 	}
 
 	utsMode := container.UTSMode(copts.utsMode)
 	if !utsMode.Valid() {
-		return nil, nil, nil, fmt.Errorf("--uts: invalid UTS mode")
+		return nil, fmt.Errorf("--uts: invalid UTS mode")
 	}
 
 	usernsMode := container.UsernsMode(copts.usernsMode)
 	if !usernsMode.Valid() {
-		return nil, nil, nil, fmt.Errorf("--userns: invalid USER mode")
+		return nil, fmt.Errorf("--userns: invalid USER mode")
 	}
 
 	restartPolicy, err := runconfigopts.ParseRestartPolicy(copts.restartPolicy)
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 
 	loggingOpts, err := parseLoggingOpts(copts.loggingDriver, copts.loggingOpts.GetAll())
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 
 	securityOpts, err := parseSecurityOpts(copts.securityOpt.GetAll())
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 
 	storageOpts, err := parseStorageOpts(copts.storageOpt.GetAll())
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, err
 	}
 
 	// Healthcheck
@@ -456,7 +462,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 		copts.healthRetries != 0
 	if copts.noHealthcheck {
 		if haveHealthSettings {
-			return nil, nil, nil, fmt.Errorf("--no-healthcheck conflicts with --health-* options")
+			return nil, fmt.Errorf("--no-healthcheck conflicts with --health-* options")
 		}
 		test := strslice.StrSlice{"NONE"}
 		healthConfig = &container.HealthConfig{Test: test}
@@ -467,13 +473,13 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 			probe = strslice.StrSlice(args)
 		}
 		if copts.healthInterval < 0 {
-			return nil, nil, nil, fmt.Errorf("--health-interval cannot be negative")
+			return nil, fmt.Errorf("--health-interval cannot be negative")
 		}
 		if copts.healthTimeout < 0 {
-			return nil, nil, nil, fmt.Errorf("--health-timeout cannot be negative")
+			return nil, fmt.Errorf("--health-timeout cannot be negative")
 		}
 		if copts.healthRetries < 0 {
-			return nil, nil, nil, fmt.Errorf("--health-retries cannot be negative")
+			return nil, fmt.Errorf("--health-retries cannot be negative")
 		}
 
 		healthConfig = &container.HealthConfig{
@@ -588,7 +594,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 	}
 
 	if copts.autoRemove && !hostConfig.RestartPolicy.IsNone() {
-		return nil, nil, nil, fmt.Errorf("Conflicting options: --restart and --rm")
+		return nil, fmt.Errorf("Conflicting options: --restart and --rm")
 	}
 
 	// only set this value if the user provided the flag, else it should default to nil
@@ -640,7 +646,11 @@ func parse(flags *pflag.FlagSet, copts *containerOptions) (*container.Config, *c
 		networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] = epConfig
 	}
 
-	return config, hostConfig, networkingConfig, nil
+	return &containerConfig{
+		Config:           config,
+		HostConfig:       hostConfig,
+		NetworkingConfig: networkingConfig,
+	}, nil
 }
 
 func parseLoggingOpts(loggingDriver string, loggingOpts []string) (map[string]string, error) {

command/container/opts_test.go

@@ -51,7 +51,12 @@ func parseRun(args []string) (*container.Config, *container.HostConfig, *network
 	if err := flags.Parse(args); err != nil {
 		return nil, nil, nil, err
 	}
-	return parse(flags, copts)
+	// TODO: fix tests to accept ContainerConfig
+	containerConfig, err := parse(flags, copts)
+	if err != nil {
+		return nil, nil, nil, err
+	}
+	return containerConfig.Config, containerConfig.HostConfig, containerConfig.NetworkingConfig, err
 }
 
 func parsetest(t *testing.T, args string) (*container.Config, *container.HostConfig, error) {

command/container/run.go

@@ -12,9 +12,9 @@ import (
 
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/cli"
 	"github.com/docker/docker/cli/command"
-	opttypes "github.com/docker/docker/opts"
 	"github.com/docker/docker/pkg/promise"
 	"github.com/docker/docker/pkg/signal"
 	"github.com/docker/libnetwork/resolvconf/dns"
@@ -66,40 +66,44 @@ func NewRunCommand(dockerCli *command.DockerCli) *cobra.Command {
 	return cmd
 }
 
-func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions, copts *containerOptions) error {
+func warnOnOomKillDisable(hostConfig container.HostConfig, stderr io.Writer) {
-	stdout, stderr, stdin := dockerCli.Out(), dockerCli.Err(), dockerCli.In()
-	client := dockerCli.Client()
-	// TODO: pass this as an argument
-	cmdPath := "run"
-
-	var (
-		flAttach                *opttypes.ListOpts
-		ErrConflictAttachDetach = errors.New("Conflicting options: -a and -d")
-	)
-
-	config, hostConfig, networkingConfig, err := parse(flags, copts)
-
-	// just in case the parse does not exit
-	if err != nil {
-		reportError(stderr, cmdPath, err.Error(), true)
-		return cli.StatusError{StatusCode: 125}
-	}
-
 	if hostConfig.OomKillDisable != nil && *hostConfig.OomKillDisable && hostConfig.Memory == 0 {
 		fmt.Fprintln(stderr, "WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous.")
 	}
+}
 
-	if len(hostConfig.DNS) > 0 {
+// check the DNS settings passed via --dns against localhost regexp to warn if
-		// check the DNS settings passed via --dns against
+// they are trying to set a DNS to a localhost address
-		// localhost regexp to warn if they are trying to
+func warnOnLocalhostDNS(hostConfig container.HostConfig, stderr io.Writer) {
-		// set a DNS to a localhost address
+	for _, dnsIP := range hostConfig.DNS {
-		for _, dnsIP := range hostConfig.DNS {
+		if dns.IsLocalhost(dnsIP) {
-			if dns.IsLocalhost(dnsIP) {
+			fmt.Fprintf(stderr, "WARNING: Localhost DNS setting (--dns=%s) may fail in containers.\n", dnsIP)
-				fmt.Fprintf(stderr, "WARNING: Localhost DNS setting (--dns=%s) may fail in containers.\n", dnsIP)
+			return
-				break
-			}
 		}
 	}
+}
+
+func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions, copts *containerOptions) error {
+	containerConfig, err := parse(flags, copts)
+	// just in case the parse does not exit
+	if err != nil {
+		reportError(dockerCli.Err(), "run", err.Error(), true)
+		return cli.StatusError{StatusCode: 125}
+	}
+	return runContainer(dockerCli, opts, copts, containerConfig)
+}
+
+func runContainer(dockerCli *command.DockerCli, opts *runOptions, copts *containerOptions, containerConfig *containerConfig) error {
+	config := containerConfig.Config
+	hostConfig := containerConfig.HostConfig
+	stdout, stderr := dockerCli.Out(), dockerCli.Err()
+	client := dockerCli.Client()
+
+	// TODO: pass this as an argument
+	cmdPath := "run"
+
+	warnOnOomKillDisable(*hostConfig, stderr)
+	warnOnLocalhostDNS(*hostConfig, stderr)
 
 	config.ArgsEscaped = false
 
@@ -108,11 +112,8 @@ func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions
 			return err
 		}
 	} else {
-		if fl := flags.Lookup("attach"); fl != nil {
+		if copts.attach.Len() != 0 {
-			flAttach = fl.Value.(*opttypes.ListOpts)
+			return errors.New("Conflicting options: -a and -d")
-			if flAttach.Len() != 0 {
-				return ErrConflictAttachDetach
-			}
 		}
 
 		config.AttachStdin = false
@@ -135,7 +136,7 @@ func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions
 
 	ctx, cancelFun := context.WithCancel(context.Background())
 
-	createResponse, err := createContainer(ctx, dockerCli, config, hostConfig, networkingConfig, hostConfig.ContainerIDFile, opts.name)
+	createResponse, err := createContainer(ctx, dockerCli, containerConfig, opts.name)
 	if err != nil {
 		reportError(stderr, cmdPath, err.Error(), true)
 		return runStartContainerErr(err)
@@ -158,51 +159,15 @@ func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions
 	}
 	attach := config.AttachStdin || config.AttachStdout || config.AttachStderr
 	if attach {
-		var (
-			out, cerr io.Writer
-			in        io.ReadCloser
-		)
-		if config.AttachStdin {
-			in = stdin
-		}
-		if config.AttachStdout {
-			out = stdout
-		}
-		if config.AttachStderr {
-			if config.Tty {
-				cerr = stdout
-			} else {
-				cerr = stderr
-			}
-		}
-
 		if opts.detachKeys != "" {
 			dockerCli.ConfigFile().DetachKeys = opts.detachKeys
 		}
 
-		options := types.ContainerAttachOptions{
+		close, err := attachContainer(ctx, dockerCli, &errCh, config, createResponse.ID)
-			Stream:     true,
+		defer close()
-			Stdin:      config.AttachStdin,
+		if err != nil {
-			Stdout:     config.AttachStdout,
+			return err
-			Stderr:     config.AttachStderr,
-			DetachKeys: dockerCli.ConfigFile().DetachKeys,
 		}
-
-		resp, errAttach := client.ContainerAttach(ctx, createResponse.ID, options)
-		if errAttach != nil && errAttach != httputil.ErrPersistEOF {
-			// ContainerAttach returns an ErrPersistEOF (connection closed)
-			// means server met an error and put it in Hijacked connection
-			// keep the error and read detailed error message from hijacked connection later
-			return errAttach
-		}
-		defer resp.Close()
-
-		errCh = promise.Go(func() error {
-			if errHijack := holdHijackedConnection(ctx, dockerCli, config.Tty, in, out, cerr, resp); errHijack != nil {
-				return errHijack
-			}
-			return errAttach
-		})
 	}
 
 	statusChan := waitExitOrRemoved(ctx, dockerCli, createResponse.ID, copts.autoRemove)
@@ -252,6 +217,57 @@ func runRun(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *runOptions
 	return nil
 }
 
+func attachContainer(
+	ctx context.Context,
+	dockerCli *command.DockerCli,
+	errCh *chan error,
+	config *container.Config,
+	containerID string,
+) (func(), error) {
+	stdout, stderr := dockerCli.Out(), dockerCli.Err()
+	var (
+		out, cerr io.Writer
+		in        io.ReadCloser
+	)
+	if config.AttachStdin {
+		in = dockerCli.In()
+	}
+	if config.AttachStdout {
+		out = stdout
+	}
+	if config.AttachStderr {
+		if config.Tty {
+			cerr = stdout
+		} else {
+			cerr = stderr
+		}
+	}
+
+	options := types.ContainerAttachOptions{
+		Stream:     true,
+		Stdin:      config.AttachStdin,
+		Stdout:     config.AttachStdout,
+		Stderr:     config.AttachStderr,
+		DetachKeys: dockerCli.ConfigFile().DetachKeys,
+	}
+
+	resp, errAttach := dockerCli.Client().ContainerAttach(ctx, containerID, options)
+	if errAttach != nil && errAttach != httputil.ErrPersistEOF {
+		// ContainerAttach returns an ErrPersistEOF (connection closed)
+		// means server met an error and put it in Hijacked connection
+		// keep the error and read detailed error message from hijacked connection later
+		return nil, errAttach
+	}
+
+	*errCh = promise.Go(func() error {
+		if errHijack := holdHijackedConnection(ctx, dockerCli, config.Tty, in, out, cerr, resp); errHijack != nil {
+			return errHijack
+		}
+		return errAttach
+	})
+	return resp.Close, nil
+}
+
 // reportError is a utility method that prints a user-friendly message
 // containing the error that occurred during parsing and a suggestion to get help
 func reportError(stderr io.Writer, name string, str string, withHelp bool) {