Allow swarm join with `--availability=drain` by yongtang · Pull Request #24993 · moby/moby

yongtang · Jul 25, 2016

- What I did

This fix tries to address the issue raised in #24596 where it was not possible to join as manager only (--availability=drain).

The related PR in swarmkit is:
moby/swarmkit#1271

- How I did it

This fix adds a new flag --availability to swarm join.

- How to verify it

An integration test has been added.

- Description for the changelog

Add a new flag --availability to swarm join.

- A picture of a cute animal (not mandatory but encouraged)

This fix fixes #24596.

The related PR in swarmkit is:

moby/swarmkit#1271

vdemeester · Jul 25, 2016

/cc @thaJeztah @aaronlehmann @stevvooe @aluzzardi

thaJeztah · Jul 25, 2016

I'm +1 on design, I think it makes sense to haven an option to set the initial availability of a node

thaJeztah · Jul 25, 2016

Tentatively adding this to the milestone (because the related issue is), but it's no show-stopper

aaronlehmann · Jul 25, 2016

This will be ignored for a renewal. I think that's okay, but we need to make it clear in the comment.

I'm not sure we want to add availability to the top level of IssueNodeCertificateRequest. Either there should be a nested messages with parameters for the new node object, or we should pass a NodeSpec and only honor certain fields from it (because not everything in a NodeSpec is under the node's control).

@aluzzardi @stevvooe any thoughts?

I don't like much availability leaking to the NodeCA.

Also - if we go down this path, we should probably take an initial NodeSpec rather than a bunch of fields

This really isn't the right place for it.

Agree with the concept of providing a NodeSpec at join time.

@yongtang: It sounds like the consensus is that we should provide a NodeSpec instead of just an availability setting. But as I mentioned earlier, the node CA should only use certain fields from this spec when it creates the node. There are things like node labels that are controlled from elsewhere.

stevvooe · Jul 25, 2016

In general, this is a good feature, but the implementation needs some design work.

Do we have something open in swarmkit?

yongtang · Jul 26, 2016

@stevvooe @aaronlehmann @aluzzardi @vdemeester @thaJeztah Thanks a lot for the review and help!

I was looking for feedbacks on this PR before, so I haven't created the pull request on swarmkit side yet.

Let me rework on this pull request based on the comments. Will work on NodeSpec and update the pull request soon.

icecrime · Jul 27, 2016

Thanks @yongtang! In the meantime, I'll remove the milestone as it won't make it in time for 1.12.0 (which is ok).

cpuguy83 · Aug 12, 2016

ping @stevvooe

stevvooe · Aug 12, 2016

@cpuguy83 I am not necessarily on board with using NodeSpec here, since we don't honer the other aspects of the structure, such as node role.

stevvooe · Aug 16, 2016

This is way too many string arguments in a row.

Thanks @stevvooe. The pull request has been updated with localAddr, listenAddr, and advertiseAddr into one consolidated nodeAddr. Please let me know if there are any issues.

cpuguy83 · Aug 26, 2016

ping @yongtang

yongtang · Aug 27, 2016

Thanks @cpuguy83. The pull request has been rebased and updated. Please let me know if there are any issues.

thaJeztah · Sep 22, 2016

Sorry @yongtang this needs another rebase

ping @stevvooe is this ok to be moved to code review?

stevvooe · Sep 28, 2016

This was my last comment here:

@cpuguy83 I am not necessarily on board with using NodeSpec here, since we don't honer the other aspects of the structure, such as node role.

If we can reconcile this, then we can probably move along.

aaronlehmann · Oct 19, 2016

To unblock this, maybe we can go back to the approach of not using NodeSpec.

Using NodeSpec was my suggestion. I'm sorry it delayed this so much.

stevvooe · Oct 19, 2016

@aaronlehmann I have no problem using NodeSpec, as long as we don't cherry pick parameters. I think the problem is that NodeSpec can set things like the name, which should be under control of the cluster administrator, or membership, which is the purview of the managers. NodeSpec is supposed to be the user input for a node.

Let's just make this a field value for "request availability", ultimately leaving the policy to the orchestrator as to whether it is honored or not.

Do we also incorporate this into the join token?

yongtang · Nov 23, 2016

Thanks all for the review. The PR has been updated with vendoring of swarmkit done. Now all tests passes. Please take a look and let me know if there are additional issues.

aaronlehmann · Nov 28, 2016

LGTM after rebase

yongtang · Nov 28, 2016

@aaronlehmann Rebased. Thanks.

cpuguy83 · Dec 21, 2016

What's the status here?

aaronlehmann · Dec 21, 2016

The swarmkit-side PR was merged, so this is most of the way there. Sadly, it needs yet another rebase. It still looks good to me. Needs a second LGTM.

yongtang · Dec 22, 2016

@cpuguy83 @aaronlehmann The PR has been rebased.

cpuguy83

LGTM

aaronlehmann · Jan 9, 2017

@thaJeztah: What are the next steps to get this merged? It's not an urgent PR, but I feel bad about it being open so long, and asking for rebases every few weeks.

thaJeztah · Jan 9, 2017

oh, my bad, lost track of this one 😞

thaJeztah

left some suggestions

thaJeztah · Jan 9, 2017

Can you remove string here?

thaJeztah · Jan 9, 2017

typo Possiblle -> Possible

Possible availability values are `active`, `pause`, or `drain`.

thaJeztah · Jan 9, 2017

Specifying the availability when initialising a swarm allows you to initialize a dedicated manager node that do not get tasks scheduled. The following example initializes a swarm, and sets the availability to `drain`; $ docker swarm init --availability=drain

Not 100% satisfied with that one, so open to suggestions

thaJeztah · Jan 9, 2017

Remove string here

Thanks. Done.

thaJeztah · Jan 9, 2017

Possible availability values are `active`, `pause`, or `drain`.

Thanks. Done.

yongtang · Jan 10, 2017

Thanks @aaronlehmann @thaJeztah the PR has been updated.

This fix tries to address the issue raised in 24596 where it was not possible to join as manager only (`--availability=drain`). This fix adds a new flag `--availability` to `swarm join`. Related documentation has been updated. An integration test has been added. NOTE: Additional pull request for swarmkit and engine-api will be created separately. This fix fixes 24596. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

This fix adds a new flag `--availability` to `swarm join`. Related documentation has been updated. An integration test has been added. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

thaJeztah

LGTM

GordonTheTurtle added the status/0-triage label Jul 25, 2016

vdemeester added status/1-design-review and removed status/0-triage labels Jul 25, 2016

thaJeztah added the status/needs-vendoring label Jul 25, 2016

thaJeztah added this to the 1.12.0 milestone Jul 25, 2016

aaronlehmann reviewed Jul 25, 2016
View reviewed changes

icecrime removed this from the 1.12.0 milestone Jul 27, 2016

yongtang mentioned this pull request Jul 29, 2016

Allow swarm join with --availability=drain moby/swarmkit#1271

Merged

yongtang force-pushed the 24596-swarm-join-with-drain branch 2 times, most recently from a48810c to 303fb5d Compare July 29, 2016 02:02

yongtang force-pushed the 24596-swarm-join-with-drain branch from 303fb5d to efc32f8 Compare August 16, 2016 18:47

stevvooe reviewed Aug 16, 2016
View reviewed changes

yongtang force-pushed the 24596-swarm-join-with-drain branch 3 times, most recently from 82cac7d to 8f71074 Compare August 27, 2016 21:02

yongtang force-pushed the 24596-swarm-join-with-drain branch from 5f626fa to 009f3e3 Compare November 23, 2016 19:05

aaronlehmann removed the status/needs-vendoring label Nov 28, 2016

yongtang force-pushed the 24596-swarm-join-with-drain branch from 009f3e3 to 1c13b15 Compare November 28, 2016 22:47

yongtang force-pushed the 24596-swarm-join-with-drain branch from 1c13b15 to 663e1d1 Compare December 22, 2016 02:14

cpuguy83 approved these changes Dec 22, 2016

View reviewed changes

thaJeztah added status/3-docs-review and removed status/2-code-review labels Dec 22, 2016

thaJeztah requested changes Jan 9, 2017

View reviewed changes

yongtang force-pushed the 24596-swarm-join-with-drain branch from 663e1d1 to 02b9334 Compare January 10, 2017 07:53

yongtang added 2 commits January 10, 2017 16:31

Allow swarm init with --availability=drain

0f30c64

This fix adds a new flag `--availability` to `swarm join`. Related documentation has been updated. An integration test has been added. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang force-pushed the 24596-swarm-join-with-drain branch from 02b9334 to 0f30c64 Compare January 11, 2017 00:32

thaJeztah approved these changes Jan 11, 2017

View reviewed changes

thaJeztah merged commit 66aba12 into moby:master Jan 11, 2017

albers mentioned this pull request Jan 11, 2017

Add bash completion for swarm join --availability #30073

Merged

yongtang deleted the 24596-swarm-join-with-drain branch January 11, 2017 20:53

sdurrheimer mentioned this pull request Jan 19, 2017

Add zsh completion for 'docker swarm --availability' #30282

Merged

thaJeztah added impact/api impact/changelog impact/cli labels Jan 27, 2017

thaJeztah mentioned this pull request Sep 9, 2020

Availability not documented for SwarmJoin API #41429

Open

Search code, repositories, users, issues, pull requests...

Conversation

yongtang commented Jul 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester commented Jul 25, 2016

Uh oh!

thaJeztah commented Jul 25, 2016

Uh oh!

thaJeztah commented Jul 25, 2016

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!

stevvooe commented Jul 25, 2016

Uh oh!

yongtang commented Jul 26, 2016

Uh oh!

icecrime commented Jul 27, 2016

Uh oh!

cpuguy83 commented Aug 12, 2016

Uh oh!

stevvooe commented Aug 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Aug 26, 2016

Uh oh!

yongtang commented Aug 27, 2016

Uh oh!

thaJeztah commented Sep 22, 2016

Uh oh!

stevvooe commented Sep 28, 2016

Uh oh!

aaronlehmann commented Oct 19, 2016

Uh oh!

stevvooe commented Oct 19, 2016

Uh oh!

yongtang commented Nov 23, 2016

Uh oh!

aaronlehmann commented Nov 28, 2016

Uh oh!

yongtang commented Nov 28, 2016

Uh oh!

cpuguy83 commented Dec 21, 2016

Uh oh!

aaronlehmann commented Dec 21, 2016

Uh oh!

yongtang commented Dec 22, 2016

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Jan 9, 2017

Uh oh!

thaJeztah commented Jan 9, 2017

Uh oh!

thaJeztah left a comment

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!

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

yongtang commented Jul 25, 2016 •

edited

Loading