Skip to content

cluster: Only pass a join address when in the process of joining a cluster#33361

Merged
mlaventure merged 1 commit into
moby:masterfrom
aaronlehmann:no-join-address
Jun 14, 2017
Merged

cluster: Only pass a join address when in the process of joining a cluster#33361
mlaventure merged 1 commit into
moby:masterfrom
aaronlehmann:no-join-address

Conversation

@aaronlehmann

@aaronlehmann aaronlehmann commented May 23, 2017 •
edited
Loading

Copy link
Copy Markdown

This code currently passes a random manager address when creating a new Node. This doesn't really make sense - we should only pass a join address on the initial join, or when retrying that join. An upcoming change to swarmkit will pay attention to JoinAddr when a node is already part of a cluster, so passing in the random value needs to be avoided.

Fixes #32980

cc @tonistiigi

cluster: Only pass a join address when in the process of joining a cl…
24477e7 
…uster

This code currently passes a random manager address when creating a new
Node. This doesn't really make sense - we should only pass a join
address on the initial join, or when retrying that join. An upcoming
change to swarmkit will pay attention to JoinAddr significant when a
node is already part of a cluster, so passing in the random value needs
to be avoided.

Signed-off-by: Aaron Lehmann <[email protected]>
@tonistiigi

tonistiigi commented May 23, 2017

Copy link
Copy Markdown
Member

LGTM

@cyli cyli mentioned this pull request May 30, 2017
Merged
mlaventure
mlaventure approved these changes Jun 14, 2017
View reviewed changes

@mlaventure mlaventure left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

But I'd like for the test to be re-run given this is an old PR. I don't seem to be able to do so though. (cc @thaJeztah @tiborvass any idea why not?)

@thaJeztah

thaJeztah commented Jun 14, 2017

Copy link
Copy Markdown
Member

No, no idea why it doesn't work, I think I've seen the same on some PR's. Let me try through Slack

@thaJeztah

thaJeztah commented Jun 14, 2017

Copy link
Copy Markdown
Member

Yup, works through slack. Odd

@mlaventure

mlaventure commented Jun 14, 2017

Copy link
Copy Markdown
Contributor

All 3 Janky errors seem unrelated:

15:44:58 ----------------------------------------------------------------------
15:44:58 FAIL: docker_cli_exec_test.go:226: DockerSuite.TestExecStopNotHanging
15:44:58 
15:44:58 docker_cli_exec_test.go:248:
15:44:58     c.Fatal("Container stop timed out")
15:44:58 ... Error: Container stop timed out
15:44:58 
15:44:59 
15:44:59 ----------------------------------------------------------------------

15:57:41 ----------------------------------------------------------------------
15:57:41 FAIL: docker_cli_start_test.go:94: DockerSuite.TestStartPausedContainer
15:57:41 
15:57:41 docker_cli_start_test.go:98:
15:57:41     runSleepingContainer(c, "-d", "--name", "testing")
15:57:41 /go/src/github.com/docker/docker/pkg/testutil/cmd/command.go:64:
15:57:41     t.Fatalf("at %s:%d - %s", filepath.Base(file), line, err.Error())
15:57:41 ... Error: at cli.go:48 - 
15:57:41 Command:  /usr/local/bin/docker run -d -d --name testing busybox top
15:57:41 ExitCode: 125
15:57:41 Error:    exit status 125
15:57:41 Stdout:   cdb603cce5eb040a3003afb27fdb892b359a1cf3e39f2f68301c34ab7f2a1842
15:57:41 
15:57:41 Stderr:   /usr/local/bin/docker: Error response from daemon: endpoint with name testing already exists in network bridge.
15:57:41 
15:57:41 
15:57:41 Failures:
15:57:41 ExitCode was 125 expected 0
15:57:41 Expected no error
15:57:41 
15:57:41 
15:57:43 
15:57:43 ----------------------------------------------------------------------

17:05:55 ----------------------------------------------------------------------
17:05:55 FAIL: docker_cli_external_volume_driver_unix_test.go:446: DockerExternalVolumeSuite.TestExternalVolumeDriverBindExternalVolume
17:05:55 
17:05:55 docker_cli_external_volume_driver_unix_test.go:448:
17:05:55     dockerCmd(c, "run", "-d", "--name", "testing", "-v", "foo:/bar", "busybox", "top")
17:05:55 /go/src/github.com/docker/docker/pkg/testutil/cmd/command.go:64:
17:05:55     t.Fatalf("at %s:%d - %s", filepath.Base(file), line, err.Error())
17:05:55 ... Error: at cli.go:48 - 
17:05:55 Command:  /usr/local/bin/docker run -d --name testing -v foo:/bar busybox top
17:05:55 ExitCode: 125
17:05:55 Error:    exit status 125
17:05:55 Stdout:   d906acd135e600278a4463e3b82d0039430be0652d6309752d6369e599f0397e
17:05:55 
17:05:55 Stderr:   /usr/local/bin/docker: Error response from daemon: endpoint with name testing already exists in network bridge.
17:05:55 
17:05:55 
17:05:55 Failures:
17:05:55 ExitCode was 125 expected 0
17:05:55 Expected no error
17:05:55 
17:05:55 
17:05:56 
17:05:56 ----------------------------------------------------------------------

going to restart it for good mesure.

@mlaventure

mlaventure commented Jun 14, 2017

Copy link
Copy Markdown
Contributor

Alright, it passed, merging.

The other are flaky test. the TestStartPausedContainer seems to be an network issue though. I'll open an issue for it.

@mlaventure
mlaventure merged commit c86323c into moby:master Jun 14, 2017
@aaronlehmann
aaronlehmann deleted the no-join-address branch July 12, 2017 18:43
@aaronlehmann aaronlehmann mentioned this pull request Jul 12, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@mlaventure mlaventure mlaventure approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mlaventure mlaventure

Labels

impact/changelog status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aaronlehmann @tonistiigi @thaJeztah @mlaventure @GordonTheTurtle