New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not clear swarm directory on `swarm init` and `swarm join` #33341

Merged
merged 1 commit into from May 23, 2017

Conversation

Projects
None yet
5 participants
@cyli
Contributor

cyli commented May 22, 2017

However, this continues to clear the directory if init fails, because we don't want to leave it in a half-finished state.

This would fix #33216 to be compatible with the method for pre-generating external CA certs that was in 17.03.

After some discussion with @aaronlehmann and @tonistiigi, this is probably the correct behavior (unless the state dir can get corrupted?) either way.

This would also allow the extra flags as suggested in #33216 to be added after 17.06 if necessary.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 22, 2017

Contributor

Do we need to also clear the state if a join operation fails (<-nr.Ready() provides an error)?

Contributor

aaronlehmann commented May 22, 2017

Do we need to also clear the state if a join operation fails (<-nr.Ready() provides an error)?

@cyli

This comment has been minimized.

Show comment
Hide comment
@cyli

cyli May 22, 2017

Contributor

@aaronlehmann That seems like a good idea - is there any reason we'd want to keep state from prior to a join, though?

Similarly, if init fails, the pre-generated certificates would also be wiped out, which seems ok to me but may be inconvenient for users to have to re-generate (although I'd like to deprecate having to do that anyway).

Contributor

cyli commented May 22, 2017

@aaronlehmann That seems like a good idea - is there any reason we'd want to keep state from prior to a join, though?

Similarly, if init fails, the pre-generated certificates would also be wiped out, which seems ok to me but may be inconvenient for users to have to re-generate (although I'd like to deprecate having to do that anyway).

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 22, 2017

Contributor

is there any reason we'd want to keep state from prior to a join, though?

I don't think so.

I think the reason we need to remove it after a failed join is to avoid ending up with an inconsistent state that prevents a second swarm join operation from succeeding.

Contributor

aaronlehmann commented May 22, 2017

is there any reason we'd want to keep state from prior to a join, though?

I don't think so.

I think the reason we need to remove it after a failed join is to avoid ending up with an inconsistent state that prevents a second swarm join operation from succeeding.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi May 22, 2017

Member

@cyli build error in ci

Member

tonistiigi commented May 22, 2017

@cyli build error in ci

@cyli

This comment has been minimized.

Show comment
Hide comment
@cyli

cyli May 23, 2017

Contributor

@tonistiigi Oops, sorry, thanks!

Contributor

cyli commented May 23, 2017

@tonistiigi Oops, sorry, thanks!

Show outdated Hide outdated daemon/cluster/swarm.go Outdated
Do not clear swarm directory at the begining of swarm init and swarm …
…join now.

However, do clear the directory if init or join fails, because we don't
want to leave it in a half-finished state.

Signed-off-by: Ying Li <ying.li@docker.com>
@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi May 23, 2017

Member

LGTM

Member

tonistiigi commented May 23, 2017

LGTM

1 similar comment
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 23, 2017

Contributor

LGTM

Contributor

aaronlehmann commented May 23, 2017

LGTM

@aaronlehmann aaronlehmann merged commit a9fcaee into moby:master May 23, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34444 has succeeded
Details
janky Jenkins build Docker-PRs 43035 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3430 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 14307 has succeeded
Details
z Jenkins build Docker-PRs-s390x 3145 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.06.0 milestone May 23, 2017

@cyli cyli deleted the cyli:do-not-clear-state-on-swarm-init-join branch May 23, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 23, 2017

Contributor

@cyli: Does this need to be cherry-picked in 17.06?

@thaJeztah: @GordonTheTurtle added a 17.06.0 milestone automatically, but I believe this is an error, since 17.06 has already branched.

Contributor

aaronlehmann commented May 23, 2017

@cyli: Does this need to be cherry-picked in 17.06?

@thaJeztah: @GordonTheTurtle added a 17.06.0 milestone automatically, but I believe this is an error, since 17.06 has already branched.

@cyli

This comment has been minimized.

Show comment
Hide comment
@cyli

cyli May 23, 2017

Contributor

@aaronlehmann Yes that would be ideal, thank you.

Contributor

cyli commented May 23, 2017

@aaronlehmann Yes that would be ideal, thank you.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 24, 2017

Member

@aaronlehmann 17.06 already branched, but we may have to remove milestones from this repository, because moby milestones != docker milestones

Member

thaJeztah commented May 24, 2017

@aaronlehmann 17.06 already branched, but we may have to remove milestones from this repository, because moby milestones != docker milestones

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 24, 2017

Contributor
Contributor

aaronlehmann commented May 24, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 24, 2017

Member

I added the cherry-pick label for visibility, but not sure at this moment what to do with the VERSION and "milestones" until I know what the replacement is for keeping track

Member

thaJeztah commented May 24, 2017

I added the cherry-pick label for visibility, but not sure at this moment what to do with the VERSION and "milestones" until I know what the replacement is for keeping track

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment