Allow swarm join with `--availability=drain` #24993

yongtang · 2016-07-25T05:00:34Z

- 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 · 2016-07-25T07:38:50Z

/cc @thaJeztah @aaronlehmann @stevvooe @aluzzardi

thaJeztah · 2016-07-25T08:41:28Z

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

thaJeztah · 2016-07-25T08:43:36Z

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

aaronlehmann · 2016-07-25T17:50:36Z

vendor/src/github.com/docker/swarmkit/api/ca.proto

@@ -43,6 +43,9 @@ message IssueNodeCertificateRequest {
 	// Token represents a user-provided string that is necessary for new
 	// nodes to join the cluster
 	string token = 3;
+
+	// Availability allows a user to control the current scheduling status of a node.
+	string availability = 4;


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 · 2016-07-25T20:32:05Z

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

Do we have something open in swarmkit?

yongtang · 2016-07-26T14:38:43Z

@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 · 2016-07-27T18:12:38Z

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 · 2016-08-12T17:21:23Z

ping @stevvooe

stevvooe · 2016-08-12T19:56:04Z

@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 · 2016-08-16T21:03:08Z

daemon/cluster/cluster.go

@@ -235,7 +235,7 @@ func (c *Cluster) reconnectOnFailure(n *node) {
 	}
 }

-func (c *Cluster) startNewNode(forceNewCluster bool, localAddr, remoteAddr, listenAddr, advertiseAddr, joinAddr, joinToken string) (*node, error) {
+func (c *Cluster) startNewNode(forceNewCluster bool, localAddr, remoteAddr, listenAddr, advertiseAddr, joinAddr, joinToken string, availability types.NodeAvailability) (*node, error) {


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 · 2016-08-26T20:11:51Z

ping @yongtang

yongtang · 2016-08-27T21:02:35Z

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

thaJeztah · 2016-09-22T18:56:03Z

Sorry @yongtang this needs another rebase

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

stevvooe · 2016-09-28T00:40:38Z

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 · 2016-10-19T17:55:24Z

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 · 2016-10-19T18:05:47Z

@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 · 2016-11-23T21:52:42Z

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 · 2016-11-28T21:16:44Z

LGTM after rebase

yongtang · 2016-11-28T22:47:45Z

@aaronlehmann Rebased. Thanks.

cpuguy83 · 2016-12-21T21:35:44Z

What's the status here?

aaronlehmann · 2016-12-21T21:39:20Z

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 · 2016-12-22T15:38:47Z

@cpuguy83 @aaronlehmann The PR has been rebased.

cpuguy83

LGTM

aaronlehmann · 2017-01-09T19:59:55Z

@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 · 2017-01-09T20:48:44Z

oh, my bad, lost track of this one 😞

thaJeztah

left some suggestions

thaJeztah · 2017-01-09T20:49:03Z

docs/reference/commandline/swarm_init.md

@@ -133,6 +134,16 @@ Snapshots compact the Raft log and allow for more efficient transfer of the
 state to new managers. However, there is a performance cost to taking snapshots
 frequently.

+### `--availability string`


Can you remove string here?

thaJeztah · 2017-01-09T20:50:15Z

docs/reference/commandline/swarm_init.md

+### `--availability string`
+
+This flag specifies the availability of the node at the time the node joins a master.
+Possiblle availability value includes `active`, `pause`, or `drain`.


typo Possiblle -> Possible

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

thaJeztah · 2017-01-09T20:59:39Z

docs/reference/commandline/swarm_init.md

+This flag is useful in certain situations. For example, a cluster may want to have
+dedicated manager nodes that are not served as worker nodes. This could be achieved
+by passing `--availability=drain` to `docker swarm init`.
+


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 · 2017-01-09T21:00:48Z

docs/reference/commandline/swarm_join.md

@@ -94,6 +95,15 @@ This flag is generally not necessary when joining an existing swarm.

 Secret value required for nodes to join the swarm

+### `--availability string`


Remove string here

Thanks. Done.

thaJeztah · 2017-01-09T21:01:29Z

docs/reference/commandline/swarm_join.md

+### `--availability string`
+
+This flag specifies the availability of the node at the time the node joins a master.
+Possiblle availability value includes `active`, `pause`, or `drain`.


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

Thanks. Done.

yongtang · 2017-01-10T08:32:27Z

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

		@@ -94,6 +95,15 @@ This flag is generally not necessary when joining an existing swarm.

		Secret value required for nodes to join the swarm

		### `--availability string`

Allow swarm join with --availability=drain #24993

Allow swarm join with --availability=drain #24993

Conversation

yongtang commented Jul 25, 2016 • edited Loading

vdemeester commented Jul 25, 2016

thaJeztah commented Jul 25, 2016

thaJeztah commented Jul 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Jul 25, 2016

yongtang commented Jul 26, 2016

icecrime commented Jul 27, 2016

cpuguy83 commented Aug 12, 2016

stevvooe commented Aug 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 commented Aug 26, 2016

yongtang commented Aug 27, 2016

thaJeztah commented Sep 22, 2016

stevvooe commented Sep 28, 2016

aaronlehmann commented Oct 19, 2016

stevvooe commented Oct 19, 2016

yongtang commented Nov 23, 2016

aaronlehmann commented Nov 28, 2016

yongtang commented Nov 28, 2016

cpuguy83 commented Dec 21, 2016

aaronlehmann commented Dec 21, 2016

yongtang commented Dec 22, 2016

cpuguy83 left a comment

Choose a reason for hiding this comment

aaronlehmann commented Jan 9, 2017

thaJeztah commented Jan 9, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongtang commented Jan 10, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

Allow swarm join with `--availability=drain` #24993

Allow swarm join with `--availability=drain` #24993

yongtang commented Jul 25, 2016 •

edited

Loading