Cleanup: simplify flag parsing and merge into mflag by shykes · Pull Request #7551 · moby/moby

shykes · 2014-08-13T00:16:59Z

Refactor and document the flag parsing API in opts
Merge it into mflag to expose a unified flag-parsing interface
This interface is basically an augmented version of the standard flag

This depends on #7506. Please review that first.

jamtur01 · 2014-08-13T13:59:49Z

Docs, such as they are, LGTM.

crosbymichael · 2014-08-13T22:32:41Z

needs rebased

Signed-off-by: Solomon Hykes <[email protected]>

shykes · 2014-08-15T00:29:00Z

Rebased

shykes · 2014-08-15T00:34:26Z

The general idea of this refactor is that, instead of doing this:

config := new(struct {
   SomeOption opts.SomeSpecialType // special wrapper type to allow flag parsing
})

flag.Var(&config.SomeOption, ...)
var actualOptionValue []string := config.SomeOption.GetAll() // Now forced to carry useless wrapper type in my entire configuration

... I can now do this:

config := new(struct {
   SomeOption []string  // Actual native type
})
flag.ListVar(&config.SomeOption, ...) // Special "augmented" parsing function
// Now free to use native type without being polluted by a wrapper

vieux · 2014-08-15T01:43:07Z

When I launch the daemon: http://fpaste.org/125750/raw/

shykes · 2014-08-15T08:26:55Z

@vieux looks like I did something wrong when fixing rebase conflicts. Will check tomorrow. Thanks.

Signed-off-by: Solomon Hykes <[email protected]>

shykes · 2014-08-16T01:28:29Z

@vieux fixed and added regression test

shykes · 2014-08-18T02:38:22Z

Ping

vieux · 2014-08-18T16:53:05Z

This panic when building docker

/home/vagrant/go/bin/docker flag redefined: v
[ab5e3881] -job build()
2014/08/18 16:45:49 http: panic serving @: /home/vagrant/go/bin/docker flag redefined: v
goroutine 47 [running]:
net/http.func·011()
    /usr/local/go/src/pkg/net/http/server.go:1100 +0xb7
runtime.panic(0xa2d620, 0xc2084a7140)
    /usr/local/go/src/pkg/runtime/panic.c:248 +0x18d
github.com/docker/docker/pkg/mflag.(*FlagSet).Var(0xc208004420, 0x7f19488bb350, 0xc2

shykes · 2014-08-19T18:23:49Z

@vieux could you show the full command? I can't reproduce.

vieux · 2014-08-20T15:52:24Z

$ pulls checkout 7551
$ git rebase master
$ make
# restart docker daemon, use newly created one
$ make
docker build -t "docker:HEAD" .
Sending build context to Docker daemon 57.66 MB
Sending build context to Docker daemon
Step 0 : docker-version 0.6.1
# Skipping unknown instruction DOCKER-VERSION
Step 1 : FROM ubuntu:14.04
 ---> c4ff7513909d
Step 2 : MAINTAINER Tianon Gravi <[email protected]> (@tianon)
 ---> Using cache
 ---> c5933eedd355
Step 3 : RUN apt-get update && apt-get install -y  aufs-tools  automake  btrfs-tools  build-essential  curl  dpkg-sig  git  iptables  libapparmor-dev  libcap-dev  libsqlite3-dev  lxc=1.0*  mercurial  parallel  reprepro  ruby1.9.1  ruby1.9.1-dev  s3cmd=1.1.0*  --no-install-recommends
2014/08/20 15:50:56 unexpected EOF
make: *** [build] Error 1

and panic on the daemon side:

2014/08/20 15:50:56 http: panic serving @: /home/vagrant/go/bin/docker flag redefined: v
goroutine 47 [running]:
net/http.func·011()
    /usr/local/go/src/pkg/net/http/server.go:1100 +0xb7
runtime.panic(0xa2b640, 0xc20824ce90)
    /usr/local/go/src/pkg/runtime/panic.c:248 +0x18d
github.com/docker/docker/pkg/mflag.(*FlagSet).Var(0xc208004420, 0x7f6013aca458, 0xc2083340c0, 0xc2083340a0, 0x2, 0x2, 0xcbc0f0, 0x5a)
    /go/src/github.com/docker/docker/pkg/mflag/flag.go:736 +0x477
github.com/docker/docker/pkg/mflag.Var(0x7f6013aca458, 0xc2083340c0, 0xc2083340a0, 0x2, 0x2, 0xcbc0f0, 0x5a)
    /go/src/github.com/docker/docker/pkg/mflag/flag.go:752 +0x75
github.com/docker/docker/pkg/mflag.PathPairSetVar(0xc20803a8e8, 0xc2083340a0, 0x2, 0x2, 0xcbc0f0, 0x5a)
    /go/src/github.com/docker/docker/pkg/mflag/validators.go:53 +0x11e
github.com/docker/docker/runconfig.parseRun(0xc20824ea80, 0xc2082ca1c0, 0x4, 0x4, 0x0, 0x40, 0x4, 0x431a8b, 0x0, 0x0)
    /go/src/github.com/docker/docker/runconfig/parse.go:82 +0xf33
github.com/docker/docker/runconfig.Parse(0xc2082ca1c0, 0x4, 0x4, 0x0, 0x3, 0xc2082ca1c0, 0x1, 0x0, 0x0)
    /go/src/github.com/docker/docker/runconfig/parse.go:32 +0x101
github.com/docker/docker/daemon.(*buildFile).CmdRun(0xc208102f00, 0xc208324fc4, 0x10e, 0x0, 0x0)
    /go/src/github.com/docker/docker/daemon/build.go:280 +0x28a
reflect.Value.call(0xa8fd20, 0xb9b638, 0x0, 0x130, 0xbb6ae0, 0x4, 0xc2082ca080, 0x2, 0x2, 0x0, ...)
    /usr/local/go/src/pkg/reflect/value.go:563 +0x1210
reflect.Value.Call(0xa8fd20, 0xb9b638, 0x0, 0x130, 0xc2082ca080, 0x2, 0x2, 0x0, 0x0, 0x0)
    /usr/local/go/src/pkg/reflect/value.go:411 +0xd7
...

shykes · 2014-08-20T16:52:10Z

Can you reproduce without rebasing on master?

On Wednesday, August 20, 2014, Victor Vieux [email protected]
wrote:

$ pulls checkout 7551
$ git rebase master
$ make

restart docker daemon, use newly created one

$ make
docker build -t "docker:HEAD" .
Sending build context to Docker daemon 57.66 MB
Sending build context to Docker daemon
Step 0 : docker-version 0.6.1

Skipping unknown instruction DOCKER-VERSION

Step 1 : FROM ubuntu:14.04
---> c4ff7513909d
Step 2 : MAINTAINER Tianon Gravi <[email protected] javascript:_e(%7B%7D,'cvml','[email protected]');> (@tianon)
---> Using cache
---> c5933eedd355
Step 3 : RUN apt-get update && apt-get install -y aufs-tools automake btrfs-tools build-essential curl dpkg-sig git iptables libapparmor-dev libcap-dev libsqlite3-dev lxc=1.0* mercurial parallel reprepro ruby1.9.1 ruby1.9.1-dev s3cmd=1.1.0* --no-install-recommends
2014/08/20 15:50:56 unexpected EOF
make: *** [build] Error 1

and panic on the daemon side:

2014/08/20 15:50:56 http: panic serving @: /home/vagrant/go/bin/docker flag redefined: v
goroutine 47 [running]:
net/http.func·011()
/usr/local/go/src/pkg/net/http/server.go:1100 +0xb7
runtime.panic(0xa2b640, 0xc20824ce90)
/usr/local/go/src/pkg/runtime/panic.c:248 +0x18dgithub.com/docker/docker/pkg/mflag.(_FlagSet).Var(0xc208004420, 0x7f6013aca458, 0xc2083340c0, 0xc2083340a0, 0x2, 0x2, 0xcbc0f0, 0x5a)
/go/src/github.com/docker/docker/pkg/mflag/flag.go:736 +0x477github.com/docker/docker/pkg/mflag.Var(0x7f6013aca458, 0xc2083340c0, 0xc2083340a0, 0x2, 0x2, 0xcbc0f0, 0x5a)
/go/src/github.com/docker/docker/pkg/mflag/flag.go:752 +0x75github.com/docker/docker/pkg/mflag.PathPairSetVar(0xc20803a8e8, 0xc2083340a0, 0x2, 0x2, 0xcbc0f0, 0x5a)
/go/src/github.com/docker/docker/pkg/mflag/validators.go:53 +0x11egithub.com/docker/docker/runconfig.parseRun(0xc20824ea80, 0xc2082ca1c0, 0x4, 0x4, 0x0, 0x40, 0x4, 0x431a8b, 0x0, 0x0)
/go/src/github.com/docker/docker/runconfig/parse.go:82 +0xf33github.com/docker/docker/runconfig.Parse(0xc2082ca1c0, 0x4, 0x4, 0x0, 0x3, 0xc2082ca1c0, 0x1, 0x0, 0x0)
/go/src/github.com/docker/docker/runconfig/parse.go:32 +0x101github.com/docker/docker/daemon.(_buildFile).CmdRun(0xc208102f00, 0xc208324fc4, 0x10e, 0x0, 0x0)
/go/src/github.com/docker/docker/daemon/build.go:280 +0x28a
reflect.Value.call(0xa8fd20, 0xb9b638, 0x0, 0x130, 0xbb6ae0, 0x4, 0xc2082ca080, 0x2, 0x2, 0x0, ...)
/usr/local/go/src/pkg/reflect/value.go:563 +0x1210
reflect.Value.Call(0xa8fd20, 0xb9b638, 0x0, 0x130, 0xc2082ca080, 0x2, 0x2, 0x0, 0x0, 0x0)
/usr/local/go/src/pkg/reflect/value.go:411 +0xd7
...

—
Reply to this email directly or view it on GitHub
#7551 (comment).

vieux · 2014-09-05T00:40:31Z

@shykes If I'm remember correctly, you said you found the fix during LinuxCon

SvenDowideit · 2014-09-19T07:27:21Z

I'm working on this, I'll update with a new PR early next week

SvenDowideit · 2014-09-22T07:18:50Z

please see #8158

LK4D4 · 2014-09-23T16:04:43Z

I'm closing this in favor of #8158

jamtur01 added the /project/doc label Aug 13, 2014

shykes mentioned this pull request Aug 13, 2014

Cleanup: refactor parsing and handling of Daemon config #7506

Merged

Solomon Hykes added 2 commits August 14, 2014 09:18

Refactor flag parsing utilities in opts/

b953cd5

Signed-off-by: Solomon Hykes <[email protected]>

Merge opts/ into mflag/ to simplify the interface

065ec89

Signed-off-by: Solomon Hykes <[email protected]>

Fix regression in pkg/mflag.IP.String

0c7c31a

Signed-off-by: Solomon Hykes <[email protected]>

SvenDowideit mentioned this pull request Sep 22, 2014

Cleanup: simplify flag parsing and merge into mflag (rebase ++) #8158

Closed

LK4D4 closed this Sep 23, 2014

Conversation

shykes commented Aug 13, 2014

Uh oh!

jamtur01 commented Aug 13, 2014

Uh oh!

crosbymichael commented Aug 13, 2014

Uh oh!

shykes commented Aug 15, 2014

Uh oh!

shykes commented Aug 15, 2014

Uh oh!

vieux commented Aug 15, 2014

Uh oh!

shykes commented Aug 15, 2014

Uh oh!

shykes commented Aug 16, 2014

Uh oh!

shykes commented Aug 18, 2014

Uh oh!

vieux commented Aug 18, 2014

Uh oh!

shykes commented Aug 19, 2014

Uh oh!

vieux commented Aug 20, 2014

Uh oh!

shykes commented Aug 20, 2014

restart docker daemon, use newly created one

Skipping unknown instruction DOCKER-VERSION

Uh oh!

vieux commented Sep 5, 2014

Uh oh!

SvenDowideit commented Sep 19, 2014

Uh oh!

SvenDowideit commented Sep 22, 2014

Uh oh!

LK4D4 commented Sep 23, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants