Vendoring in libnetwork 2da2dc055de5a474c8540871ad88a48213b0994f by mrjana · Pull Request #13518 · moby/moby

mrjana · 2015-05-27T19:31:41Z

Fixes #13498

Signed-off-by: Jana Radhakrishnan mrjana@docker.com

tiborvass · 2015-05-27T19:34:11Z

vendor/src/github.com/docker/libnetwork/sandbox/namespace_linux.go

add unit here 60 * time.Second

jessfraz · 2015-05-27T19:39:31Z

upgrading the servers ;) going to give this bad boy a spin

tiborvass · 2015-05-27T19:43:05Z

vendor/src/github.com/docker/libnetwork/sandbox/namespace_linux.go

remove defer and put it at end of function

tiborvass · 2015-05-27T19:47:07Z

@mrjana what happens if docker quits before routine cleanup?

mrjana · 2015-05-27T20:03:04Z

@tiborvass when we come back up we cleanup the stale paths in cleanupNamespaceFiles

jessfraz · 2015-05-27T20:04:08Z

panic in the unit-tests

+ go test -test.timeout=30m github.com/docker/docker/pkg/httputils
--- FAIL: TestResumableRequestReader (0.00s)
panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: bind: cannot assign requested address [recovered]
    panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: bind: cannot assign requested address

goroutine 7 [running]:
testing.func·006()
    /usr/local/go/src/testing/testing.go:441 +0x181
net/http/httptest.newLocalListener(0x0, 0x0)
    /usr/local/go/src/net/http/httptest/server.go:68 +0x457
net/http/httptest.NewUnstartedServer(0x7fd6135fa110, 0xc20802ad80, 0x7fd6135fa110)
    /usr/local/go/src/net/http/httptest/server.go:96 +0x1f
net/http/httptest.NewServer(0x7fd6135fa110, 0xc20802ad80, 0xac3578)
    /usr/local/go/src/net/http/httptest/server.go:83 +0x32
github.com/docker/docker/pkg/httputils.TestResumableRequestReader(0xc20800c120)
    /go/src/github.com/docker/docker/pkg/httputils/resumablerequestreader_test.go:18 +0xe5
testing.tRunner(0xc20800c120, 0xa9d9a0)
    /usr/local/go/src/testing/testing.go:447 +0xbf
created by testing.RunTests
    /usr/local/go/src/testing/testing.go:555 +0xa8b

goroutine 1 [chan receive]:
testing.RunTests(0x7c25a0, 0xa9d9a0, 0x2, 0x2, 0x100000001)
    /usr/local/go/src/testing/testing.go:556 +0xad6
testing.(*M).Run(0xc20802e460, 0xac2e40)
    /usr/local/go/src/testing/testing.go:485 +0x6c
main.main()
    github.com/docker/docker/pkg/httputils/_test/_testmain.go:102 +0x291

goroutine 5 [syscall]:
os/signal.loop()
    /usr/local/go/src/os/signal/signal_unix.go:21 +0x1f
created by os/signal.init·1
    /usr/local/go/src/os/signal/signal_unix.go:27 +0x35

Tests failed: ./pkg/httputils

+ go test -test.timeout=30m github.com/docker/docker/pkg/ioutils
PASS
coverage: 79.6% of statements

+ go test -test.timeout=30m github.com/docker/docker/

jessfraz · 2015-05-27T20:10:47Z

so running this binary on the servers and testing this pr with it, i get

... Panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: bind: cannot assign requested address (PC=0x43D7E5)

in like every other test-integration-cli test

LK4D4 · 2015-05-27T20:22:30Z

If GC will occur right after syscall.Umount what difference will be with old behavior?

mrjana · 2015-05-27T20:31:35Z

@LK4D4 it's not that remove by itself is a problem. It's that when we make the kernel interleave between create namespace path and remove is when this problem happens. We make sure with this fix that remove and create never interleaves

jessfraz · 2015-05-27T21:21:22Z

LGTM

jessfraz · 2015-05-27T21:21:28Z

🎊

LK4D4 · 2015-05-27T21:30:34Z

vendor/src/github.com/docker/libnetwork/sandbox/namespace_linux.go

@mrjana this should be without the defer :)

Yeah taking care of it :-)

mrjana · 2015-05-27T22:20:38Z

@tiborvass @LK4D4 @vieux updated the PR taking care of the comments

icecrime · 2015-05-27T22:54:24Z

vendor/src/github.com/docker/libnetwork/sandbox/namespace_linux.go

I'm not sure it's a supported use case, but I'm pretty sure reading some people start multiple daemons on a single host, and in this case this call might be very problematic.

icecrime · 2015-05-27T23:06:39Z

I think the cleanup logic is fine, but I'm afraid somebody, somewhere, will complain that it breaks running multiple daemon on a single host (was that @SvenDowideit?). I wish we didn't need in-memory state to manage collection, but I don't see an easy to query files to determine if they can safely be removed.

LGTM with a less than 100% confidence ;-) It's already great news that you managed to workaround those kernel bugs: that unblocks 1.7.0-rc1, we can refine if the cleanup logic becomes problematic.

mrjana · 2015-05-27T23:09:04Z

@icecrime if the multiple daemons in a single host is a supported configuration then this could be problematic. Let me think about it a bit more. May be we can rework this a bit after 1.70-rc1?

tiborvass · 2015-05-27T23:17:55Z

vendor/src/github.com/docker/libnetwork/sandbox/namespace_linux.go

could this be settable? I wonder whether --exec-root which defaults to /var/run/docker should set this

Right now this is not settable. We can make this settable as an enhancement in the future

Signed-off-by: Jana Radhakrishnan <mrjana@docker.com>

icecrime · 2015-05-28T00:18:40Z

So now the trade-off is that uncleanly exiting the daemon will leave dangling files behind. Not ideal, but good enough for 1.7.0?

Still LGTM.

vieux · 2015-05-28T00:19:16Z

LGTM

vieux · 2015-05-28T00:19:31Z

It'll be improved in RC2

mrjana · 2015-05-28T00:20:26Z

@icecrime but only for those container which haven't been restarted and only until a system reboot. But may be we can make it better

icecrime · 2015-05-28T00:42:04Z

Alright, let's do this ;-)

Vendoring in libnetwork 2da2dc055de5a474c8540871ad88a48213b0994f

tiborvass · 2015-05-28T00:42:24Z

----------------------------------------------------------------------
FAIL: docker_cli_run_unix_test.go:162: DockerSuite.TestRunDeviceDirectory

docker_cli_run_unix_test.go:168:
    c.Fatal(err, out)
... Error: exit status 1Error response from daemon: Cannot start container 73017df47d63f1c30f8af70f742fef0ba9f7ef3c45a4431d78c1e8a289b0a9e6: error gathering device information while adding custom device "/dev/snd": lstat /dev/snd: no such file or directory

not sure if its related

mrjana · 2015-05-28T00:43:03Z

@tiborvass that happens when you don't have /dev/snd in your machine

tiborvass · 2015-05-28T00:43:30Z

----------------------------------------------------------------------
FAIL: docker_cli_nat_test.go:61: DockerSuite.TestNetworkNat

docker_cli_nat_test.go:71:
    c.Fatalf("Failed to connect to server: %v (output: %q)", err, string(out))
... Error: Failed to connect to server: exit status 1 (output: "nc: timed out\n")

this is another one

jessfraz · 2015-05-28T00:46:04Z

test network nat is super unstable it fails all the time for me

GordonTheTurtle added the status/0-triage label May 27, 2015

tiborvass added status/2-code-review and removed status/0-triage labels May 27, 2015

tiborvass reviewed May 27, 2015
View reviewed changes

vendor/src/github.com/docker/libnetwork/sandbox/namespace_linux.go Outdated

Copy link
Copy Markdown

Contributor

tiborvass May 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add unit here 60 * time.Second

tiborvass reviewed May 27, 2015
View reviewed changes

mrjana force-pushed the clone branch from a995c9f to c50caae Compare May 27, 2015 20:51

LK4D4 reviewed May 27, 2015
View reviewed changes

mrjana force-pushed the clone branch from c50caae to 28ea5ca Compare May 27, 2015 22:19

estesp changed the title ~~Vendoring in libnetwork a4d595b6e7e07c2cf6cf32ea1aae8a28080c01d4~~ Vendoring in libnetwork 60da50e183e6195ee770dec9c57193bd94cd34fc May 27, 2015

icecrime reviewed May 27, 2015
View reviewed changes

tiborvass reviewed May 27, 2015
View reviewed changes

Vendoring in libnetwork 2da2dc055de5a474c8540871ad88a48213b0994f

d429465

Signed-off-by: Jana Radhakrishnan <mrjana@docker.com>

mrjana force-pushed the clone branch from 28ea5ca to d429465 Compare May 28, 2015 00:07

mavenugo changed the title ~~Vendoring in libnetwork 60da50e183e6195ee770dec9c57193bd94cd34fc~~ Vendoring in libnetwork 2da2dc055de5a474c8540871ad88a48213b0994f May 28, 2015

icecrime pushed a commit that referenced this pull request May 28, 2015

Merge pull request #13518 from mrjana/clone

c2e8f07

Vendoring in libnetwork 2da2dc055de5a474c8540871ad88a48213b0994f

icecrime merged commit c2e8f07 into moby:master May 28, 2015

Conversation

mrjana commented May 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessfraz commented May 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented May 27, 2015

Uh oh!

mrjana commented May 27, 2015

Uh oh!

jessfraz commented May 27, 2015

Uh oh!

jessfraz commented May 27, 2015

Uh oh!

LK4D4 commented May 27, 2015

Uh oh!

mrjana commented May 27, 2015

Uh oh!

jessfraz commented May 27, 2015

Uh oh!

jessfraz commented May 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrjana commented May 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icecrime commented May 27, 2015

Uh oh!

mrjana commented May 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icecrime commented May 28, 2015

Uh oh!

vieux commented May 28, 2015

Uh oh!

vieux commented May 28, 2015

Uh oh!

mrjana commented May 28, 2015

Uh oh!

icecrime commented May 28, 2015

Uh oh!

tiborvass commented May 28, 2015

Uh oh!

mrjana commented May 28, 2015

Uh oh!

tiborvass commented May 28, 2015

Uh oh!

jessfraz commented May 28, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants