beenull/moby
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Merge pull request #31621 from dnephin/cleanup-container-run-command

Browse source 
Some cleanup of container run command
This commit is contained in:
Vincent Demeester 2017-03-21 16:01:11 +01:00 committed by GitHub
commit f965aab5e9
4 changed files with 138 additions and 104 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
cli/command/container/create.go
View file

@ -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 {

58
cli/command/container/opts.go
View file

@ -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) {

7
cli/command/container/opts_test.go
View file

@ -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) {

166
cli/command/container/run.go
View file

@ -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) {