From 4e388c22d3c721093d844a95da2c4a97ef9a300b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 7 Mar 2017 17:19:54 -0500 Subject: [PATCH] Refactor container run cli command. Signed-off-by: Daniel Nephin --- command/container/create.go | 11 ++- command/container/opts.go | 58 +++++++----- command/container/opts_test.go | 7 +- command/container/run.go | 166 ++++++++++++++++++--------------- 4 files changed, 138 insertions(+), 104 deletions(-) diff --git a/command/container/create.go b/command/container/create.go index 9559ba0c05..ef894bad5a 100644 --- a/command/container/create.go +++ b/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 { diff --git a/command/container/opts.go b/command/container/opts.go index 4ce872b556..febddbc5d1 100644 --- a/command/container/opts.go +++ b/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 /[] or /[] 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) { diff --git a/command/container/opts_test.go b/command/container/opts_test.go index 1448dae8db..3c7753cd00 100644 --- a/command/container/opts_test.go +++ b/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) { diff --git a/command/container/run.go b/command/container/run.go index e805ca1a57..fe869f7958 100644 --- a/command/container/run.go +++ b/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 { - 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} - } - +func warnOnOomKillDisable(hostConfig container.HostConfig, stderr io.Writer) { 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 they are trying to - // set a DNS to a localhost address - for _, dnsIP := range hostConfig.DNS { - if dns.IsLocalhost(dnsIP) { - fmt.Fprintf(stderr, "WARNING: Localhost DNS setting (--dns=%s) may fail in containers.\n", dnsIP) - break - } +// check the DNS settings passed via --dns against localhost regexp to warn if +// they are trying to set a DNS to a localhost address +func warnOnLocalhostDNS(hostConfig container.HostConfig, stderr io.Writer) { + for _, dnsIP := range hostConfig.DNS { + if dns.IsLocalhost(dnsIP) { + fmt.Fprintf(stderr, "WARNING: Localhost DNS setting (--dns=%s) may fail in containers.\n", dnsIP) + return } } +} + +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 { - flAttach = fl.Value.(*opttypes.ListOpts) - if flAttach.Len() != 0 { - return ErrConflictAttachDetach - } + if copts.attach.Len() != 0 { + return errors.New("Conflicting options: -a and -d") } 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{ - Stream: true, - Stdin: config.AttachStdin, - Stdout: config.AttachStdout, - Stderr: config.AttachStderr, - DetachKeys: dockerCli.ConfigFile().DetachKeys, + close, err := attachContainer(ctx, dockerCli, &errCh, config, createResponse.ID) + defer close() + if err != nil { + return err } - - 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) {