docker_node: Docker Swarm node operations module #50584

WojciechowskiPiotr · 2019-01-06T15:01:13Z

SUMMARY

New module to handle Docker Swarm node operations. It must be executed on Swarm manager node and allows changing the node availability, role, and labels (fixes #49547). Part of the commit is also a shared library for all common methods for other Swarm modules.

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

docker_node

ADDITIONAL INFORMATION

This PR is a copy of #50428, required due to the wrong branch in original PR. The open discussion in old PR is on module name and where put some inspect features, either in docker_node or docker_node_facts (#50571). The upcoming docker_swarm_facts will use the shared code from this PR.
This module is equivalent of docker node CLI command allowing now changing the node availability and role as well as applying or removing the labels from the node. It must be executed on Swarm manager node otherwise exception is raised.
When RP is merged into ansible/devel then we can update docker_swarm module so it uses the shared methods from module_utils/docker_swarm instead of own local wherever it is possible.

ansibot · 2019-01-06T15:03:30Z

cc @DBendit @akshay196 @danihodovic @dariko @felixfontein @jwitko @kassiansun @tbouvet
click here for bot help

WojciechowskiPiotr · 2019-01-06T15:07:44Z

ansible/community#408

ansibot · 2019-01-06T15:11:39Z

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

lib/ansible/modules/cloud/docker/docker_node.py:0:0: E322 "force" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/docker/docker_node.py:0:0: E325 argument_spec for "force" defines type="bool" but documentation does not

click here for bot help

ansibot · 2019-01-06T15:26:50Z

@DBendit @agronholm @akshay196 @cove @danihodovic @dariko @dusdanig @joshuaconner @jwitko @kassiansun @keitwb @olsaki @softzilla @tbouvet @ushuz @zfil

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

WojciechowskiPiotr · 2019-01-07T23:39:33Z

ready_for_review

Would be nice to get this PR reviewed and hopefully merged in parallel to #50571 as those are not in conflict to each other

felixfontein · 2019-01-08T00:01:19Z

I think it makes more sense to first get #50571 merged, and then modify docker_node_facts to use the new module_utils as well (in this PR) -- as well as discuss whether the list mode should be moved to docker_node_facts (I'm kind of in favor of this).

Anyway, to get something merged, some more reviews are needed. Anyone? :)

felixfontein · 2019-01-08T18:58:48Z

Now that #50571 has been merged, can you rebase this PR against devel and make docker_node_facts use the new module_utils as well?

WojciechowskiPiotr · 2019-01-08T23:39:26Z

The change in docker_node_facts is purely cosmetic, already committed in my repo, will be part of next push to this PR.

As @tbouvet agreed with me in #50428, we will use the docker_node as the module name. That would put changing the name of docker_service into docker_compose in more flavor so we can later rename docker_swarm_service into docker_service. It will take six releases, but I think it is good for modules convention sanity in cloud/docker.

As for the list as I said before in #50428 only the docker_image_facts does not require name parameter, and it accepts a list of strings. Other modules require a single string as name. The CLI commands aways allows a list of objects to inspect (empty list not allowed) and provides the ls action to just list objects with basic information. I like the approach where the _facts module is like an inspect and non-_facts have an implementation of ls command.

I think the options to consider are:

Leave docker_node_facts as it is, so it returns information on a single node, adjusts the docker_image_facts to this approach, create the ls command implementation in an operational module, it may cover the filters as well

Pros: Easy docker_*_facts modules in code, the single way you can use them in playbooks
Cons: Playbook developer need to know the name of the object to inspect every time, so there should be some kind of implementation of CLI command docker [network,image,container,etc] ls in each module to give a list of objects
Consistency with CLI - partial

Let parameter name optional in all docker_*_facts modules and accept the list as a parameter, if name not provided then return information about all objects.

Pros: Easy way to get information on every object, specific objects or a single object, shouldn't be hard to implement. May include filters too.
Cons: If called without a name parameter it returns lots of information, and later the playbook code may not be so clear due to big multi-levels structures user have to parse. If it covers the filters as well, it will increase the complexity of the facts modules
Consistency with CLI - partial

Let parameter name optional in all docker_*_facts modules and accept the list as a parameter, if name not provided then return information about all objects but also add the ls command output to the operation modules.

Pros: Easy way to get information on every object, specific objects or a single object, shouldn't be hard to implement. The _facts will not allow additional filtering, this feature will be implemented as part of the non-_facts module.
Cons: If called without a name parameter it returns lots of information and later the playbook code may not be so clear due to big multi-levels structures user have to parse. If it covers the filters as well, it will increase the complexity of the facts modules
Consistency with CLI - full

Let parameter name required as list of strings and implement the ls method in each non-facts module. This way facts modules will return the full information on at least single object, while ls command implementation returns limited information and can cover filters

Pros: Playbook developer will call the facts module to inspect known objects, because it will be a list it will be possible to get information in one task, the ls command implementation in each module may return the structure identical to the CLI, so limited in number of information, it can also cover the filters
Cons: Need a ls command implementation in each module
Consistency with CLI - full

One more thing worth noticing is the difference between Swarm and non-Swarm hosts. The non-Swarm modules (image, container, volume, network) you need to run on every host in inventory while swarm modules (with some exceptions of node operations not proposed yet in this PR) have to be run on hosts acting as Swarm Managers. This role is dynamic by its nature and promoting/demoting nodes shouldn't be unusual operation to keep a quorum in the cluster. Also, each manager will return same information about the cluster and its components (node, config, secret, other swarm modules) so it will be a waste of time and resources to run facts module on each node from inventory every time.

With the approach in point 4) the playbook developer may fetch the nodes lists and roles from docker_node using state: list to get a simple structure with just 6 or 7 attributes as in docker node ls command output, he may use additional filters as well. It is easy to use the list if playbook runs on localhost and uses the API. With SSH and hosts in inventory dynamic states in Swarm are not so easy to handle when you try to minimize number of tasks in playbook, time to run it and resources it will require.

To sum this up - I like the consistency between the modules and I like the idea that modules are working in similar way as CLI because most people will first know the CLI then will try automate the tasks. My preference is option 4 in this case, but I am happy to go with approach most of the docker modules maintainers here will recommend as preferred.

felixfontein · 2019-01-09T05:45:33Z

There are a few things to consider:

Normal modules should do something (except for check mode), and _facts modules should query stuff without modifying. Having data querying functionality in normal modules is discouraged. I'm definitely against adding a list/ls method to any non-_facts module. If there is list/ls support, it should be handled by the _facts module (or by a new module).
Ansible should not mirror exactly what can be done via CLI or API, but what is useful. Is there a concrete need for getting a list of containers, images, ...? I can see that it makes sense for Swarm nodes, but I doubt it for other objects.

Also, for offering lists in a _facts module, I would prefer having a choices option results with default value single, and alternative choice list (names open for bikeshedding as usual ;) ). For single, name would be required; for list, it would not be (and will be ignored).

…e to separate PR

…ibleDockerSwarmClient instead of AnsibleDockerClient docker_node: cosmetic code changes BOTMETA: updated on $team_docker

WojciechowskiPiotr · 2019-02-02T18:30:26Z

@felixfontein no problem :) Rebased and waiting for review and merging now

gundalow · 2019-02-04T11:19:58Z

.github/BOTMETA.yml

@@ -584,6 +584,7 @@ files:
    maintainers: $team_windows_core
    support: core
  $module_utils/docker_common.py: *docker
+  $module_utils/docker_swarm.py: *docker


Thanks for remembering to update this, if you replace the above line with:

$module_utils/docker: *docker

We can future proof this.

gundalow · 2019-02-04T11:21:36Z

lib/ansible/modules/cloud/docker/docker_node.py

+extends_documentation_fragment:
+    - docker
+requirements:
+    - "python >= 2.6"


Ansible 2.8 only supports Python 2.6+ on the remote, so this line isn't needed.

gundalow · 2019-02-04T11:21:47Z

lib/ansible/modules/cloud/docker/docker_node.py

+    - "Please note that the L(docker-py,https://pypi.org/project/docker-py/) Python
+       module has been superseded by L(docker,https://pypi.org/project/docker/)
+       (see L(here,https://github.com/docker/docker-py/issues/1310) for details).
+       For Python 2.6, C(docker-py) must be used. Otherwise, it is recommended to


Ansible 2.8 only supports Python 2.6+ on the remote, so this could be reworded.

Let's keep this for now, and let's change all of these in a subsequent PR for all docker_* modules, so that all modules get the same text.

Agreed, follow up PR is a good idea.

gundalow · 2019-02-04T11:22:45Z

lib/ansible/modules/cloud/docker/docker_node.py

+'''
+
+RETURN = '''
+node_facts:


If we know the structure of the dict then adding some of those details would be good, see the example at https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#return-block

We only know a subset of the structure, as newer API versions will add more data here. For other docker_* modules, we "solved" this by adding sample: with an excerpt of a result containing the most interesting things. No idea what's the best approach here.

(Another thing is that depending on the data returned, this can be really complex. For docker_container, the returned inspect result is a huge dict; documenting everything would be a big task.)

Maybe a direct link to the latest Docker API documentation instead if sample: and information to check the API documentation for the specific API version on remote?

That would be fine, too. Maybe also have sample: as well, so people get an idea what they will get.

gundalow · 2019-02-04T13:09:40Z

@WojciechowskiPiotr Thank you for this PR
@felixfontein Thank you for the review

felixfontein · 2019-02-04T15:32:02Z

@WojciechowskiPiotr thanks for the PR! You will notice that you'll get CC'ed in all docker issues/PRs; ansibot will take some time to add you to all existing issues/PRs.
@tbouvet thanks for your feedback!
@gundalow thanks for reviewing and merging!

I've opened a first follow-up PR in #51700 with some things discussed with @gundalow.

felixfontein · 2019-02-04T15:59:21Z

More stuff for follow-up PRs (before I forget about this):

~~integration tests (for both docker_node and docker_node_facts)~~
~~minimal version checking for docker-py and docker API versions~~ docker_swarm: support older docker-py versions #53129
~~other changes to docker_node_facts which were temporarily in this PR~~ docker_node_facts: Query may cover single or multiple nodes #51701

@WojciechowskiPiotr anything else? :)

WojciechowskiPiotr · 2019-02-04T16:42:07Z

@felixfontein docker_swarm_facts from #50622

felixfontein · 2019-02-04T20:58:53Z

@WojciechowskiPiotr that already has a PR (and is in the tab to the left of this PR in my Ansible docker tab collection ;-) ), so I didn't mention it.

WojciechowskiPiotr mentioned this pull request Jan 6, 2019

docker_node: Module for swarm node operations #50428

Closed

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 6, 2019

WojciechowskiPiotr mentioned this pull request Jan 7, 2019

docker_swarm_facts: New module to retrieve Docker Swarm information #50622

Merged

ansibot removed the needs_triage Needs a first human triage before being processed. label Jan 8, 2019

felixfontein mentioned this pull request Jan 8, 2019

docker_node_facts: Read information about swarm node from swarm manager #50571

Merged

ansibot removed the new_contributor This PR is the first contribution by a new community member. label Jan 8, 2019

WojciechowskiPiotr added 8 commits February 2, 2019 19:16

* docker_node: revised labels manipulation

547acbe

* docker_node: Documentation update

b40ffa4

* docker_node: Update to node availability and role modification

c5d4c28

* docker_node: Update to check_mode handling

a119948

* docker_node: Code cleanup

b342457

docker_node_facts: Reverting to repository version, moving this chang…

fa284a2

…e to separate PR

docker_node_facts: Code cleanup

ff35b25

docker_node_facts: Adding back the module with only update to use Ans…

012b6d5

…ibleDockerSwarmClient instead of AnsibleDockerClient docker_node: cosmetic code changes BOTMETA: updated on $team_docker

ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Feb 2, 2019

docker_node: BOTMETA update

0378f9b

ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Feb 2, 2019

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 2, 2019

gundalow reviewed Feb 4, 2019

View reviewed changes

gundalow approved these changes Feb 4, 2019

View reviewed changes

gundalow merged commit daf1cfb into ansible:devel Feb 4, 2019

WojciechowskiPiotr deleted the devel-docker_node branch February 4, 2019 13:26

WojciechowskiPiotr mentioned this pull request Feb 4, 2019

docker_node_facts: Query may cover single or multiple nodes #51701

Merged

dagwieers added the botmeta This PR modifies the BOTMETA.yml and this requires special attention! label Feb 21, 2019

ansible locked and limited conversation to collaborators Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker_node: Docker Swarm node operations module #50584

docker_node: Docker Swarm node operations module #50584

WojciechowskiPiotr commented Jan 6, 2019

ansibot commented Jan 6, 2019

WojciechowskiPiotr commented Jan 6, 2019

ansibot commented Jan 6, 2019

ansibot commented Jan 6, 2019

WojciechowskiPiotr commented Jan 7, 2019

felixfontein commented Jan 8, 2019

felixfontein commented Jan 8, 2019

WojciechowskiPiotr commented Jan 8, 2019

felixfontein commented Jan 9, 2019

WojciechowskiPiotr commented Feb 2, 2019

gundalow Feb 4, 2019

gundalow Feb 4, 2019

gundalow Feb 4, 2019

felixfontein Feb 4, 2019

gundalow Feb 4, 2019

gundalow Feb 4, 2019

felixfontein Feb 4, 2019

WojciechowskiPiotr Feb 4, 2019

felixfontein Feb 4, 2019

gundalow commented Feb 4, 2019

felixfontein commented Feb 4, 2019

felixfontein commented Feb 4, 2019 •

edited

WojciechowskiPiotr commented Feb 4, 2019

felixfontein commented Feb 4, 2019

docker_node: Docker Swarm node operations module #50584

docker_node: Docker Swarm node operations module #50584

Conversation

WojciechowskiPiotr commented Jan 6, 2019

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibot commented Jan 6, 2019

WojciechowskiPiotr commented Jan 6, 2019

ansibot commented Jan 6, 2019

ansibot commented Jan 6, 2019

WojciechowskiPiotr commented Jan 7, 2019

felixfontein commented Jan 8, 2019

felixfontein commented Jan 8, 2019

WojciechowskiPiotr commented Jan 8, 2019

felixfontein commented Jan 9, 2019

WojciechowskiPiotr commented Feb 2, 2019

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

gundalow commented Feb 4, 2019

felixfontein commented Feb 4, 2019

felixfontein commented Feb 4, 2019 • edited

WojciechowskiPiotr commented Feb 4, 2019

felixfontein commented Feb 4, 2019

felixfontein commented Feb 4, 2019 •

edited