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

New module docker_plugin: Install/remove Docker plugins #52643

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@porshkevich
Copy link

porshkevich commented Feb 20, 2019

SUMMARY

New module docker_plugin: Install/remove Docker plugins

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

docker_plugin

MAIN PARAMETERS
  name:
    description:
      - Name of the plugin to operate on.
    required: true
    type: str
    aliases:
      - plugin_name
      
  alias:
    description:
      - Alias of the plugin to operate on. Same plugin can be installed with different alias.
    type: str
    aliases:
      - plugin_alias

  plugin_options:
    description:
      - "Dictionary of plugin settings.
    type: dict

  state:
    description:
      - C(absent) remove the plugin.
      - C(present) install the plugin, if it does not already exist.
    default: present
    choices:
      - absent
      - present

EXAMPLES

- name: Install a plugin
  docker_plugin:
    name: plugin_one

- name: Remove a plugin
  docker_plugin:
    name: plugin_one
    state: absent

- name: Install a plugin with options
  docker_plugin:
    name: weaveworks/net-plugin:latest_release
    alias: weave-net-plugin
    plugin_options:
      IPALLOC_RANGE: "10.32.0.0/12"
      WEAVE_PASSWORD: XXXXXXXX
@ansibot

This comment was marked as outdated.

Copy link
Contributor

ansibot commented Feb 20, 2019

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Command 'make singlehtmldocs' failed with status code: 2
--> Standard Output
cat _themes/srtd/static/css/theme.css | sed -e 's/^[ 	]*//g; s/[ 	]*$//g; s/\([:{;,]\) /\1/g; s/ {/{/g; s/\/\*.*\*\///g; /^$/d' | sed -e :a -e '$!N; s/\n\(.\)/\1/; ta' > _themes/srtd/static/css/theme.min.css
PYTHONPATH=../../lib ../bin/dump_config.py --template-file=../templates/config.rst.j2 --output-dir=rst/reference_appendices/ -d ../../lib/ansible/config/base.yml
mkdir -p rst/cli
PYTHONPATH=../../lib ../bin/generate_man.py --template-file=../templates/cli_rst.j2 --output-dir=rst/cli/ --output-format rst ../../lib/ansible/cli/*.py
PYTHONPATH=../../lib ../bin/dump_keywords.py --template-dir=../templates --output-dir=rst/reference_appendices/ -d ./keyword_desc.yml
PYTHONPATH=../../lib ../bin/plugin_formatter.py -t rst --template-dir=../templates --module-dir=../../lib/ansible/modules -o rst/modules/ 
Evaluating module files...
Makefile:93: recipe for target 'modules' failed
--> Standard Error
Traceback (most recent call last):
  File "../bin/plugin_formatter.py", line 774, in <module>
    main()
  File "../bin/plugin_formatter.py", line 729, in main
    plugin_info, categories = get_plugin_info(options.module_dir, limit_to=options.limit_to, verbose=(options.verbosity > 0))
  File "../bin/plugin_formatter.py", line 294, in get_plugin_info
    doc, examples, returndocs, metadata = plugin_docs.get_docstring(module_path, fragment_loader, verbose=verbose)
  File "/root/ansible/lib/ansible/utils/plugin_docs.py", line 103, in get_docstring
    data = read_docstring(filename, verbose=verbose, ignore_errors=ignore_errors)
  File "/root/ansible/lib/ansible/parsing/plugin_docs.py", line 59, in read_docstring
    data[varkey] = AnsibleLoader(child.value.s, file_name=filename).get_single_data()
  File "/usr/local/lib/python3.6/dist-packages/yaml/constructor.py", line 35, in get_single_data
    node = self.get_single_node()
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 36, in get_single_node
    document = self.compose_document()
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 55, in compose_document
    node = self.compose_node(None, None)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 82, in compose_node
    node = self.compose_sequence_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 110, in compose_sequence_node
    while not self.check_event(SequenceEndEvent):
  File "/usr/local/lib/python3.6/dist-packages/yaml/parser.py", line 98, in check_event
    self.current_event = self.state()
  File "/usr/local/lib/python3.6/dist-packages/yaml/parser.py", line 393, in parse_block_sequence_entry
    "expected <block end>, but found %r" % token.id, token.start_mark)
yaml.parser.ParserError: while parsing a block collection
  in "<unicode string>", line 26, column 7:
          - "Dictionary of plugin settings.
          ^
expected <block end>, but found '<scalar>'
  in "<unicode string>", line 46, column 8:
        - "python >= 2.7"
           ^
make: *** [modules] Error 1

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:162:24: SyntaxError: return {k: v for k, v in map(lambda x: x.split('=', 1), options_list)} if options_list else {}

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:162:24: SyntaxError: invalid syntax

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:105:0: ImportError: No module named docker_common

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:105:0: ImportError: No module named 'ansible.module_utils.docker_common'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:105:0: ModuleNotFoundError: No module named 'ansible.module_utils.docker_common'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:105:0: ModuleNotFoundError: No module named 'ansible.module_utils.docker_common'

The test ansible-test sanity --test import --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:105:0: ModuleNotFoundError: No module named 'ansible.module_utils.docker_common'

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:31: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_plugin.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'No module named 'ansible.module_utils.docker_common''
lib/ansible/modules/cloud/docker/docker_plugin.py:61:8: E302 DOCUMENTATION is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_plugin.py:61:8: error DOCUMENTATION: syntax error: expected <block end>, but found '<scalar>'

click here for bot help

@porshkevich porshkevich force-pushed the porshkevich:docker_plugin branch from 2d42100 to 907dfed Feb 20, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 20, 2019

@DBendit @WojciechowskiPiotr @agronholm @akshay196 @cove @danihodovic @dariko @dusdanig @joshuaconner @jwitko @kassiansun @keitwb @olsaki @softzilla @tbouvet @ushuz @WojciechowskiPiotr @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
Copy link
Contributor

WojciechowskiPiotr left a comment

I don't see how you enable and disable plugin. You disable plugin to make changes and reenable it, but I cannot how can you enable installed plugin or just disable the plugin

@ansibot ansibot removed the needs_triage label Feb 20, 2019

def __init__(self, client):
self.client = client

self.dclient = DockerClient()

This comment has been minimized.

@WojciechowskiPiotr

WojciechowskiPiotr Feb 20, 2019

Contributor

I think this should be an extension to ansible.module_utils.docker.common instead of a direct call of a docker library class methods

This comment has been minimized.

@felixfontein

felixfontein Feb 20, 2019

Contributor

self.client is already derived from DockerClient (and ready to use), there's no need for self.dclient.

This comment has been minimized.

@porshkevich

porshkevich Feb 25, 2019

Author

Sorry, but self.client is docker.APIClient not docker.DockerClient

This comment has been minimized.

@porshkevich

porshkevich Feb 25, 2019

Author

I think this should be an extension to ansible.module_utils.docker.common instead of a direct call of a docker library class methods

I understand you, but I have no idea what to change in ansible.module_utils.docker.common for this

This comment has been minimized.

@felixfontein

felixfontein Feb 26, 2019

Contributor

Ah, docker.DockerClient not our common.DockerClient. Sorry, got confused :)

There's one problem with using docker.DockerClient: it needs to be set-up similarly to the API client, otherwise it might talk to a completely different docker daemon (or not talk to a docker daemon at all, while the other one does). To set it up the same way, you should add a method to common.AnsibleDockerClient which sets it up for you (and call that one to get your instance).

As a first iteration, you can also do the setup in your module (and not touch common.AnsibleDockerClient), but you should look at how common.AnsibleDockerClient does the setup of the API client. The relevant lines are: https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/docker/common.py#L230-L237
Maybe you can simply pass client._connect_params on to docker.DockerClient's constructor and everything is fine. Looking at how docker.DockerClient works (https://github.com/docker/docker-py/blob/master/docker/client.py#L39-L40), this seems to be the correct approach.

This comment has been minimized.

@porshkevich

porshkevich Mar 14, 2019

Author

docker.DockerClient use docker.APIClient as property. common.AnsibleDockerClient extend docker.APIClient. Instead of setting up a new docker.APIClient, I simply replace it not configured with what we already have common.AnsibleDockerClient in next line.

This comment has been minimized.

@porshkevich

porshkevich Mar 14, 2019

Author

If this is still not the right decision, I can make a separate option that includes changes to common.AnsibleDockerClient.

This comment has been minimized.

@felixfontein

felixfontein Mar 14, 2019

Contributor

It's probably not a good long-term solution, but currently it's fine, especially because docker-py's DockerClient at the moment simply creates self.api = APIClient(...) and does nothing else. Once this PR is merged, we can improve this behavior in a bugfix PR (which can also be merged after next Thursday).

This comment has been minimized.

@felixfontein

felixfontein Mar 16, 2019

Contributor

While working on #53906, I explored the docker client creation process a bit more in detail. You can use self.dclient = DockerClient(**self.client._connect_params) to initialize the client with the same arguments as self.client is initialized. Relying on a Ansible docker internal API should be safer than relying on docker-py internal API.

Show resolved Hide resolved lib/ansible/modules/cloud/docker/docker_plugin.py
@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Feb 24, 2019

@porshkevich thank you very much for this new module PR! I've got a few general questions and remarks about it first:

  • As @WojciechowskiPiotr remarked, it would probably be a good idea if you can also use docker_plugin to enable and disable plugins, and not just to install/uninstall them. Also, being able to upgrade could be of interest.
  • Some general API remarks: please avoid adding aliases for option names if possible, and do not return facts as ansible_facts. Some existing docker_* modules do that, but that's not good and especially for the facts, we're trying to get rid of that and shouldn't add anything new. Also, the way you are returning differences is not something that should be used in new modules. Please look at docker_container or docker_swarm_service to see how diff results should work.
  • All new module PRs should have integration tests. Even if they only work locally at first (and not on CI). That helps us judging that they actually do what is promised.

If you have any questions, please ask.

@porshkevich

This comment has been minimized.

Copy link
Author

porshkevich commented Feb 25, 2019

@felixfontein

* All new module PRs should have integration tests. Even if they only work locally at first (and not on CI). That helps us judging that they actually do what is promised.

Sorry, but I did not find examples for similar tests. I can not imagine in what form it should be designed.

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Feb 26, 2019

Sorry, but I did not find examples for similar tests. I can not imagine in what form it should be designed.

Take a look at the docker_swarm tests: https://github.com/ansible/ansible/tree/devel/test/integration/targets/docker_swarm
The integration tests essentially work like a Ansible role; it is executed, and if it succeeds, the tests pass. There's a dependency mechanism (see meta; the docker_swarm tests require setup_docker, which installs docker and docker-py on the CI nodes), and aliases describes where/how the tests are run (explanation). You can probably keep these files.

Here is some general documentation on Ansible integration testing: https://docs.ansible.com/ansible/latest/dev_guide/testing_integration.html

If you want to simply run the tests against your local docker daemon, you can add a file run-tests-locally.yml with the following content into the test/integration/targets/docker_xxx/ directory:

---
- hosts: localhost
  tasks:
  - name: Check Docker API version
    command: "{{ ansible_python.executable }} -c 'import docker; print(docker.from_env().version()[\"ApiVersion\"])'"
    register: docker_api_version_stdout
    ignore_errors: yes

  - name: Check docker-py API version
    command: "{{ ansible_python.executable }} -c 'import docker; print(docker.__version__)'"
    register: docker_py_version_stdout
    ignore_errors: yes

  - set_fact:
      docker_api_version: "{{ docker_api_version_stdout.stdout or '0.0' }}"
      docker_py_version: "{{ docker_py_version_stdout.stdout or '0.0' }}"

  - debug:
      msg: "Docker API version: {{ docker_api_version }}; docker-py library version: {{ docker_py_version }}"

  - include_tasks: tasks/main.yml
    vars:
      role_path: '.'

That script sets up similar variables as setup_docker (but doesn't try to install somthing) and runs the test role.

@felixfontein
Copy link
Contributor

felixfontein left a comment

Some general remarks. I've never used docker plugins, so I can't really cover details :)

- C(absent) remove the plugin.
- C(present) install the plugin, if it does not already exist.
- C(enable) enable the plugin.
- C(disable) disable the plugin.

This comment has been minimized.

@felixfontein

felixfontein Feb 27, 2019

Contributor

One question is whether enable includes present or not. There are two philosophies: either enabled and disabled assume that the plugin is installed, or they install the plugin if it isn't installed. You're currently following the first one. I don't care what you want to do in the end, I just want to mention this in case you didn't knew/thought about the second.

This comment has been minimized.

@WojciechowskiPiotr

WojciechowskiPiotr Feb 27, 2019

Contributor

Is the upgrade feature not implemented intentionally?

This comment has been minimized.

@felixfontein

felixfontein Feb 27, 2019

Contributor

I'm not sure whether upgrade should be another state, or a flag (in the latter case, it's orthogonal to present/enable/disable).

This comment has been minimized.

@WojciechowskiPiotr

WojciechowskiPiotr Feb 27, 2019

Contributor

In either way it's not covered

This comment has been minimized.

@porshkevich

porshkevich Mar 14, 2019

Author

Initially, I did not set myself the task of covering the possibility of upgrade. as @felixfontein said, i'm not sure if it can be done as a separate state. I am ready to listen to the proposals, but this should not interfere with the adoption of this pool request

This comment has been minimized.

@felixfontein

felixfontein Mar 14, 2019

Contributor

You should probably mention in the module's description that it currently doesn't support upgrading plugins.

This comment has been minimized.

@porshkevich

porshkevich Mar 14, 2019

Author

If set alias option as required, we can use upgrade as state. By alias can find the plugin and call disable and upgrade methods. But this work only with plugins with alias.

This comment has been minimized.

@felixfontein

felixfontein Mar 14, 2019

Contributor

You can make alias a required_if in case state equals upgrade. See for example https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/docker/docker_container.py#L2928-L2934 where image is required if state equals present.

def update_plugin(self):
if not self.check_mode:
try:
if self.existing_plugin.enabled:

This comment has been minimized.

@felixfontein

felixfontein Feb 27, 2019

Contributor

You should check if the options actually changed before reconfiguring the plugin. It only makes sense to reconfigure if this actually changes something.

- C(absent) remove the plugin.
- C(present) install the plugin, if it does not already exist.
- C(enable) enable the plugin.
- C(disable) disable the plugin.

This comment has been minimized.

@WojciechowskiPiotr

WojciechowskiPiotr Feb 27, 2019

Contributor

Is the upgrade feature not implemented intentionally?

if not self.check_mode and not self.parameters.debug:
self.results.pop('actions')

def absent(self):

This comment has been minimized.

@WojciechowskiPiotr

WojciechowskiPiotr Feb 27, 2019

Contributor

Any reason not to call self.remove_plugin() directly?

@ansibot ansibot added the stale_ci label Mar 7, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Mar 9, 2019

@porshkevich please note that Ansible community/feature freeze for Ansible 2.8 is coming up soon (March 21st, see here for details). From that point on, no new modules or other features will be merged for Ansible 2.8, and this PR would have to wait until Ansible 2.9 (which will be many months in the future).

If you need help with anything, please tell us.

short_description: Manage Docker plugins
description:
- Install/remove Docker plugins.
- Performs largely the same function as the "docker plugin" CLI subcommand.

This comment has been minimized.

@felixfontein

felixfontein Mar 14, 2019

Contributor
Suggested change
- Performs largely the same function as the "docker plugin" CLI subcommand.
- Performs largely the same function as the C(docker plugin) CLI subcommand.
@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Mar 14, 2019

I'll try to do another review of this PR this evening (or tomorrow).

@felixfontein
Copy link
Contributor

felixfontein left a comment

Some more comments. And you should really add at least some basic integration tests so it's more obvious that this actually works :)

choices:
- absent
- present
- enable

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor
Suggested change
- enable
- enabled
- disabled

Otherwise it is an action, not a state.

alias: weave-net-plugin
plugin_options:
IPALLOC_RANGE: "10.32.0.0/12"
WEAVE_PASSWORD: XXXXXXXX

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor

Maybe add at least one more example where a plugin is activated.

description: Plugin inspection results for the affected plugin.
returned: success
type: dict
sample: {}

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor

I would return an explicit boolean flag which informs whether the plugin is activated or not (for state != 'absent').

except NotFound:
return None
except APIError as e:
self.client.fail(text_type(e))

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor

It is usually a good idea to prepend some text so that it is easier to find out where an error was raised (in case it's not clear from the error message).

:return: list of options that differ
"""
differences = []

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor

Please don't use lists for tracking differences, but DifferenceTracker. That produces a much better output.

existing_options_list = self.existing_plugin.settings['Env']
existing_options = parse_options(existing_options_list)

for key, value in iteritems(self.parameters.plugin_options):

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor
Suggested change
for key, value in iteritems(self.parameters.plugin_options):
for key, value in self.parameters.plugin_options.items():
self.plugin_options = None
self.debug = None

for key, value in iteritems(client.module.params):

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor
Suggested change
for key, value in iteritems(client.module.params):
for key, value in client.module.params.items():


def prepare_options(options):
return ['%s=%s' % (k, v if v is not None else "") for k, v in iteritems(options)] if options else []

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor
Suggested change
return ['%s=%s' % (k, v if v is not None else "") for k, v in iteritems(options)] if options else []
return ['%s=%s' % (k, v if v is not None else "") for k, v in options.items()] if options else []
if self.existing_plugin.enabled:
self.existing_plugin.disable()
self.existing_plugin.configure(prepare_options(self.parameters.plugin_options))
self.existing_plugin.enable(1)

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor

That should only be called if the plugin should in enabled state afterwards.

self.results['actions'].append("Enabled plugin %s" % self.get_plugin_name())
self.results['changed'] = True
else:
self.fail("Cannot enable plugin: Plugin not exist")

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor

enabled should mean the same as present and make sure the plugin is enabled, i.e. if it is not installed, install it. (Same for disabled.) present means "make sure it is there, I don't care if enabled or not".

This comment has been minimized.

@felixfontein

felixfontein Mar 15, 2019

Contributor

If for some reason you want to be able to enable without installing, you could add a flag which for enable/disable fails instead of installing (if the plugin doesn't exist). But the default behavior should be "install if not there, then make sure you have the correct state".

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Mar 15, 2019

FYI: the core and community freeze dates for Ansible 2.8 have been pushed back by a week (ansible/community#346 (comment)). So we have a bit more time go get this merged :)

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Mar 21, 2019

Just a friendly reminder: the merge window for new features and modules for Ansible 2.8 closes in a week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.