Skip to content
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

docker_node: Module for swarm node operations #50428

Closed
wants to merge 19 commits into from
Closed

docker_node: Module for swarm node operations #50428

wants to merge 19 commits into from

Conversation

WojciechowskiPiotr
Copy link
Contributor

@WojciechowskiPiotr WojciechowskiPiotr commented Jan 1, 2019

SUMMARY

New state 'node' that allows changing Swarm node availability and role or returns node_facts about the node from Swarm manager

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

docker_swarm

ADDITIONAL INFORMATION

This is an extension to docker_swarm, but if required it could be easily a new module. The current docker modules do not support Swarm nodes role and availability changes. I use the API update_node call to make changes.

WojciechowskiPiotr added 2 commits January 1, 2019 19:43
@WojciechowskiPiotr WojciechowskiPiotr changed the title devel dokcer_swarm: allow changing node role and availability via manager node Jan 1, 2019
@WojciechowskiPiotr WojciechowskiPiotr changed the title dokcer_swarm: allow changing node role and availability via manager node docker_swarm: Changing the node availability and role Jan 1, 2019
@ansibot
Copy link
Contributor

ansibot commented Jan 1, 2019

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/cloud/docker/docker_swarm.py:199:1: W293 blank line contains whitespace
lib/ansible/modules/cloud/docker/docker_swarm.py:205:1: W293 blank line contains whitespace

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/cloud/docker/docker_swarm.py:0:0: E309 version_added for new option (node_availability) should be 2.8. Currently 0.0
lib/ansible/modules/cloud/docker/docker_swarm.py:0:0: E309 version_added for new option (node_role) should be 2.8. Currently 0.0

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Jan 1, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 ci_verified Changes made in this PR are causing tests to fail. cloud docker feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:community This issue/PR relates to code supported by the Ansible community. labels Jan 1, 2019
…ne contains whitespace

lib/ansible/modules/cloud/docker/docker_swarm.py:205:1: W293 blank line contains whitespace
lib/ansible/modules/cloud/docker/docker_swarm.py:0:0: E309 version_added for new option (node_availability) should be 2.8. Currently 0.0
lib/ansible/modules/cloud/docker/docker_swarm.py:0:0: E309 version_added for new option (node_role) should be 2.8. Currently 0.0
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 1, 2019
@tbouvet
Copy link
Contributor

tbouvet commented Jan 2, 2019

@WojciechowskiPiotr I think it will be better to change the state's name. Node is not a state, maybe you can use update instead.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jan 2, 2019
@felixfontein
Copy link
Contributor

How about update-node instead of update (which is pretty generic)?

@felixfontein
Copy link
Contributor

I think it's best to create two new PRs (for docker_swarm_facts and docker_node_facts), and keep the docker_node module in this PR. That way you can edit docker_swarm and docker_node in parallel in this PR. The *_facts modules should be pretty straightforward anyway...

And it's great to know that you like to be involved in maintaining the modules :-) (You might also be interested in ansible/community#408)

Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Some comments on the facts modules.

lib/ansible/modules/cloud/docker/docker_swarm_facts.py Outdated Show resolved Hide resolved
lib/ansible/modules/cloud/docker/docker_node_facts.py Outdated Show resolved Hide resolved
lib/ansible/modules/cloud/docker/docker_node_facts.py Outdated Show resolved Hide resolved

'''

RETURN = '''
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it return when the current Docker daemon is not part of a swarm? (Then this module could also be used to detect whether Docker daemon is part of a swarm.)

'''

EXAMPLES = '''
- name: Get info on node
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be 'swarm' (and not 'node')?

lib/ansible/modules/cloud/docker/docker_swarm_facts.py Outdated Show resolved Hide resolved
@felixfontein
Copy link
Contributor

Thinking about it, I would suggest to still name the modules docker_swarm_node* instead of docker_node*. That groups them closer together in the module index and makes it clearer to people not using Docker swarm that these modules aren't of interest to them.

This is just my personal opinion, I'm leaving the decision to you two (@tbouvet @WojciechowskiPiotr).

self.results['actions'].append("Node updated")
self.results['changed'] = True

def node_list(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this function could be in docker_swarm_facts module because it's all nodes in a swarm environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked several other _facts modules from the cloud category and all of them returns the full information of all existing objects or just one\multiple objects specified usually as a name parameter. But it is always the full structure.

To be more align with the docker command line I am thinking about changing the docker_node_facts by setting the name as optional and when not provided return information about all nodes, or just one if name is provided. Then amend the docker_node state list so data returned by method matches the command line output.

Copy link
Contributor

Choose a reason for hiding this comment

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

Making name optional is probably a good idea.

lib/ansible/modules/cloud/docker/docker_node.py Outdated Show resolved Hide resolved
lib/ansible/modules/cloud/docker/docker_node.py Outdated Show resolved Hide resolved
@WojciechowskiPiotr
Copy link
Contributor Author

@felixfontein I would stick to docker_node because of the following reasons:

Thinking about it, I would suggest to still name the modules docker_swarm_node* instead of docker_node*. That groups them closer together in the module index and makes it clearer to people not using Docker swarm that these modules aren't of interest to them.

This is just my personal opinion, I'm leaving the decision to you two (@tbouvet @WojciechowskiPiotr).

I think it depends if modules should reflect the command line and API from Docker or we want to name them independently. It is still docker node and /nodes API, the docker service and /services API, the docker secret and /secrets API... For all Swarm related commands only the docker service module name is docker_swarm_service but I believe it is because docker_service was already in use for a module that reflects the docker-compose usage (the module name is quite misleading in my opinion).

I don't know the reason why docker developers decided to not put them under swarm category in a command line or API, but I can think of one - right now those commands are related to Swarm, but in future, they might be extended to support other types of orchestrators. The docker stack already support Kubernetes and Swarm.

Lastly, I think most people who will use the modules first got to know the Swarm via command line so it will be more natural for them to understand docker_node than docker_swarm_node. At least it will be for me.

So unless @tbouvet disagree with me I opt for naming that align with the docker command line and API.

@WojciechowskiPiotr
Copy link
Contributor Author

I think it's best to create two new PRs (for docker_swarm_facts and docker_node_facts), and keep the docker_node module in this PR. That way you can edit docker_swarm and docker_node in parallel in this PR. The *_facts modules should be pretty straightforward anyway...

And it's great to know that you like to be involved in maintaining the modules :-) (You might also be interested in ansible/community#408)

I will follow your recommendations during the weekend altogether with the comments to the code

This file will be presented in separate PR
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 4, 2019
This file will be added in separate PR
@ansibot ansibot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 4, 2019
WojciechowskiPiotr added 2 commits January 5, 2019 01:38
@felixfontein
Copy link
Contributor

I think it depends if modules should reflect the command line and API from Docker or we want to name them independently.

That's probably a matter of taste :) I would vote to stick mostly to the command line, but extend names to make them clearer.

For all Swarm related commands only the docker service module name is docker_swarm_service but I believe it is because docker_service was already in use for a module that reflects the docker-compose usage (the module name is quite misleading in my opinion).

I think so too, and I fully agree ;-)

I don't know the reason why docker developers decided to not put them under swarm category in a command line or API, but I can think of one - right now those commands are related to Swarm, but in future, they might be extended to support other types of orchestrators. The docker stack already support Kubernetes and Swarm.

That's one more reason to name them more explicit in Ansible, since other orchestrators have their own modules.

Lastly, I think most people who will use the modules first got to know the Swarm via command line so it will be more natural for them to understand docker_node than docker_swarm_node. At least it will be for me.

For me, I was already bitten by this from the other modules: I assumed that modules like docker_config and docker_secret is independent of Swarm mode because they didn't have _swarm_ in them, and after looking a bit into them I was somewhat disappointed that I cannot use them (since I don't use Docker Swarm). But I would have had the same problem if I found out about these functionalities from the docker command line... :)

So unless @tbouvet disagree with me I opt for naming that align with the docker command line and API.

Fine with me.

@WojciechowskiPiotr
Copy link
Contributor Author

For me, I was already bitten by this from the other modules: I assumed that modules like docker_config and docker_secret is independent of Swarm mode because they didn't have _swarm_ in them, and after looking a bit into them I was somewhat disappointed that I cannot use them (since I don't use Docker Swarm). But I would have had the same problem if I found out about these functionalities from the docker command line... :)

You just provided an argument against naming it docker_swarm_node instead of docker_node - docker_secret and docker_config should be renamed for consistent approach to docker_swarm_config and docker_swarm_secret. Considering the fact the docker_service is full of bugs and unmaintained then it might be easier to rename it to docker_compose if someone picks it up and have the docker_swarm_service renamed to docker_service.

If there are different orchestrators in future supported in Docker natively I would like to prepare components like secrets or config in a unified way, just not care about such fact as different orchestrators in playbooks unless I perform orchestrator related actions. Otherwise, it will just increase the complexity of playbooks. Like I said, maybe the Docker developers had the same approach in mind

WojciechowskiPiotr added 2 commits January 5, 2019 22:06
…hat will extend the AnsibleDockerClient

docker_node: Update documentation section
docker_node: Convert to use the AnsibleDockerSwarmClient for client connection
@felixfontein
Copy link
Contributor

You just provided an argument against naming it docker_swarm_node instead of docker_node - docker_secret and docker_config should be renamed for consistent approach to docker_swarm_config and docker_swarm_secret.

I wouldn't mind renaming them as well ;-) But as I said, we can also keep things as they are now (and name the new module docker_node).

Considering the fact the docker_service is full of bugs and unmaintained then it might be easier to rename it to docker_compose if someone picks it up and have the docker_swarm_service renamed to docker_service.

Actually, renaming docker_service to docker_compose is a good idea. That more closely describes what it is. For backwards compatibility, there will be a symlink from the old module name to the new module name for four Ansible versions (that's the standard deprecation cycle). When that's gone, we can think about renaming docker_swarm_service.

If there are different orchestrators in future supported in Docker natively I would like to prepare components like secrets or config in a unified way, just not care about such fact as different orchestrators in playbooks unless I perform orchestrator related actions. Otherwise, it will just increase the complexity of playbooks. Like I said, maybe the Docker developers had the same approach in mind

If/when that happens, it might also make sense to create a orchestrator-agnostic set of modules for managing them, similar to the package module which uses apt, yum, or whatever else is around in the background to install packages. But I guess it's better to wait until that happens and then decide how to proceed, especially since now we don't know how the unified thing will look like ;-)

@WojciechowskiPiotr WojciechowskiPiotr changed the title docker_swarm: Changing the node availability and role docker_node: Module for swarm node operations Jan 5, 2019
@WojciechowskiPiotr
Copy link
Contributor Author

Moved PR to #50584

@tbouvet
Copy link
Contributor

tbouvet commented Jan 8, 2019

@WojciechowskiPiotr I agree with you, I prefer docker_node as docker_swarm_node. Otherwise, we have to rename docker_secret to docker_swarm_secret, ...

@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 cloud docker feature This issue/PR relates to a feature request. merge_commit This PR contains at least one merge commit. Please resolve! module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants