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

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

Merged
merged 2 commits into from Jan 11, 2017

Conversation

@yongtang
Member

yongtang commented 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:
docker/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:

docker/swarmkit#1271

@vdemeester

This comment has been minimized.

Show comment
Hide comment
Member

vdemeester commented Jul 25, 2016

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 25, 2016

Member

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

Member

thaJeztah commented 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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 25, 2016

Member

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

Member

thaJeztah commented Jul 25, 2016

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

@thaJeztah thaJeztah added this to the 1.12.0 milestone Jul 25, 2016

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Jul 25, 2016

Contributor

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

Do we have something open in swarmkit?

Contributor

stevvooe commented 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

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jul 26, 2016

Member

@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.

Member

yongtang commented 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

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Jul 27, 2016

Contributor

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).

Contributor

icecrime commented 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).

@icecrime icecrime removed this from the 1.12.0 milestone Jul 27, 2016

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Aug 12, 2016

Contributor

ping @stevvooe

Contributor

cpuguy83 commented Aug 12, 2016

ping @stevvooe

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 12, 2016

Contributor

@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.

Contributor

stevvooe commented 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.

Show outdated Hide outdated daemon/cluster/cluster.go Outdated
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Aug 26, 2016

Contributor

ping @yongtang

Contributor

cpuguy83 commented Aug 26, 2016

ping @yongtang

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Aug 27, 2016

Member

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

Member

yongtang commented Aug 27, 2016

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 22, 2016

Member

Sorry @yongtang this needs another rebase

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

Member

thaJeztah commented Sep 22, 2016

Sorry @yongtang this needs another rebase

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

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Sep 28, 2016

Contributor

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.

Contributor

stevvooe commented 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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Oct 19, 2016

Contributor

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.

Contributor

aaronlehmann commented 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

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Oct 19, 2016

Contributor

@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?

Contributor

stevvooe commented 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?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Oct 19, 2016

Contributor

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.

Agreed.

Do we also incorporate this into the join token?

My feeling is that we shouldn't, to prevent the setting from being opaque and to limit the number of permutations on the join token from exploding, but it's an interesting idea.

Contributor

aaronlehmann commented Oct 19, 2016

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.

Agreed.

Do we also incorporate this into the join token?

My feeling is that we shouldn't, to prevent the setting from being opaque and to limit the number of permutations on the join token from exploding, but it's an interesting idea.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 20, 2016

Member

@yongtang do you have enough information to work on this?

Member

thaJeztah commented Oct 20, 2016

@yongtang do you have enough information to work on this?

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Oct 20, 2016

Member

Thanks @stevvooe @aaronlehmann @thaJeztah. Let me take a look and update the PR accordingly.

Member

yongtang commented Oct 20, 2016

Thanks @stevvooe @aaronlehmann @thaJeztah. Let me take a look and update the PR accordingly.

yongtang added a commit to yongtang/swarmkit that referenced this pull request Oct 22, 2016

Allow swarm join with `--availability=drain`
This fix (together with moby/moby#24993) tries to address the issue raised in

moby/moby#24596

where it was not possible to join as manager only (`--availability=drain`).

This fix adds NodeSpec to IssueNodeCertificateRequest so that it is
possible to specify the availability of the node when it joins the swarm.

This fix is related to
moby/moby#24596
moby/moby#24993

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/swarmkit that referenced this pull request Oct 22, 2016

Allow swarm join with `--availability=drain`
This fix (together with moby/moby#24993) tries to address the issue raised in

moby/moby#24596

where it was not possible to join as manager only (`--availability=drain`).

This fix adds NodeSpec to IssueNodeCertificateRequest so that it is
possible to specify the availability of the node when it joins the swarm.

This fix is related to
moby/moby#24596
moby/moby#24993

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Oct 24, 2016

Member

@stevvooe @aaronlehmann @thaJeztah The PR has been updated. Please take a look and let me know if there are any issues.

Member

yongtang commented Oct 24, 2016

@stevvooe @aaronlehmann @thaJeztah The PR has been updated. Please take a look and let me know if there are any issues.

yongtang added a commit to yongtang/swarmkit that referenced this pull request Nov 21, 2016

Allow swarm join with `--availability=drain`
This fix (together with moby/moby#24993) tries to address the issue raised in

moby/moby#24596

where it was not possible to join as manager only (`--availability=drain`).

This fix adds Availability to IssueNodeCertificateRequest so that it is
possible to specify the availability of the node when it joins the swarm.

This fix is related to
moby/moby#24596
moby/moby#24993

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/swarmkit that referenced this pull request Nov 21, 2016

Allow swarm join with `--availability=drain`
This fix (together with moby/moby#24993) tries to address the issue raised in

moby/moby#24596

where it was not possible to join as manager only (`--availability=drain`).

This fix adds Availability to IssueNodeCertificateRequest so that it is
possible to specify the availability of the node when it joins the swarm.

This fix is related to
moby/moby#24596
moby/moby#24993

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Nov 21, 2016

Revendor swarmkit to 4bdf7c0506c37bcb19b9a48693909be01e85e53c
This fix revendor swarmkit to 4bdf7c0506c37bcb19b9a48693909be01e85e53c
The following PRs are related:
docker/swarmkit#1271 (docker PR moby#24993)
docker/swarmkit#1766
docker/swarmkit#1767

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Nov 22, 2016

Revendor swarmkit to 4bdf7c0506c37bcb19b9a48693909be01e85e53c
This fix revendor swarmkit to 4bdf7c0506c37bcb19b9a48693909be01e85e53c
The following PRs are related:
docker/swarmkit#1271 (docker PR moby#24993)
docker/swarmkit#1766
docker/swarmkit#1767

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Nov 23, 2016

Member

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.

Member

yongtang commented 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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Nov 28, 2016

Contributor

LGTM after rebase

Contributor

aaronlehmann commented Nov 28, 2016

LGTM after rebase

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Nov 28, 2016

Member

@aaronlehmann Rebased. Thanks.

Member

yongtang commented Nov 28, 2016

@aaronlehmann Rebased. Thanks.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Dec 21, 2016

Contributor

What's the status here?

Contributor

cpuguy83 commented Dec 21, 2016

What's the status here?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Dec 21, 2016

Contributor

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.

Contributor

aaronlehmann commented 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

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 22, 2016

Member

@cpuguy83 @aaronlehmann The PR has been rebased.

Member

yongtang commented Dec 22, 2016

@cpuguy83 @aaronlehmann The PR has been rebased.

@cpuguy83

LGTM

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jan 9, 2017

Contributor

@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.

Contributor

aaronlehmann commented 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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 9, 2017

Member

oh, my bad, lost track of this one 😞

Member

thaJeztah commented Jan 9, 2017

oh, my bad, lost track of this one 😞

@thaJeztah

left some suggestions

Show outdated Hide outdated docs/reference/commandline/swarm_init.md Outdated
Show outdated Hide outdated docs/reference/commandline/swarm_init.md Outdated
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`.

This comment has been minimized.

@thaJeztah

thaJeztah Jan 9, 2017

Member
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
@thaJeztah

thaJeztah Jan 9, 2017

Member
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

This comment has been minimized.

@thaJeztah

thaJeztah Jan 9, 2017

Member

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

@thaJeztah

thaJeztah Jan 9, 2017

Member

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

Show outdated Hide outdated docs/reference/commandline/swarm_join.md Outdated
Show outdated Hide outdated docs/reference/commandline/swarm_join.md Outdated
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 10, 2017

Member

Thanks @aaronlehmann @thaJeztah the PR has been updated.

Member

yongtang commented Jan 10, 2017

Thanks @aaronlehmann @thaJeztah the PR has been updated.

yongtang added some commits Dec 22, 2016

Allow swarm join with `--availability=drain`
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>
Allow swarm init with `--availability=drain`
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

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

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 29418 has succeeded
Details
janky Jenkins build Docker-PRs 38011 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9081 has succeeded
Details

@yongtang yongtang deleted the yongtang:24596-swarm-join-with-drain branch Jan 11, 2017

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