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

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
297 changes: 297 additions & 0 deletions lib/ansible/modules/cloud/docker/docker_plugin.py
@@ -0,0 +1,297 @@
#!/usr/bin/python
# coding: utf-8
#
# Copyright: (c) 2019, Vladimir Porshkevich (@porshkevich) <neosonic@mail.ru>
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

from __future__ import absolute_import, division, print_function
__metaclass__ = type


ANSIBLE_METADATA = {'metadata_version': '1.1',
'status': ['preview'],
'supported_by': 'community'}


DOCUMENTATION = u'''
module: docker_plugin
version_added: "2.8"
short_description: Manage Docker plugins
description:
- Install/remove Docker plugins.
- Performs largely the same function as the "docker plugin" CLI subcommand.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

options:
name:
description:
- Name of the plugin to operate on.
required: true
type: str

alias:
description:
- Alias of the plugin to operate on. Same plugin can be installed with different alias.
type: str

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.
- C(enable) enable the plugin.
- C(disable) disable the plugin.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the upgrade feature not implemented intentionally?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

In either way it's not covered

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

default: present
choices:
- absent
- present
- enable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- enable
- enabled
- disabled

Otherwise it is an action, not a state.

- disable

extends_documentation_fragment:
- docker
- docker.docker_py_2_documentation

author:
- Vladimir Porshkevich (@porshkevich)

requirements:
- "python >= 2.7"
- "docker-py >= 2.6.0"
- "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).
Version 2.1.0 or newer is only available with the C(docker) module."
- "Docker API >= 1.25"
'''

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

'''

RETURN = '''
facts:
description: Plugin inspection results for the affected plugin.
returned: success
type: dict
sample: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

'''

try:
from docker.errors import APIError, NotFound
from docker.models.plugins import Plugin
from docker import DockerClient
except ImportError:
# missing docker-py handled in ansible.module_utils.docker_common
pass

from ansible.module_utils.docker.common import DockerBaseClass, AnsibleDockerClient
from ansible.module_utils.six import iteritems, text_type


class TaskParameters(DockerBaseClass):
def __init__(self, client):
super(TaskParameters, self).__init__()
self.client = client

self.name = None
self.alias = None
self.plugin_options = None
self.debug = None

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

Choose a reason for hiding this comment

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

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

setattr(self, key, value)


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

Choose a reason for hiding this comment

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

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



def parse_options(options_list):
return dict((k, v) for k, v in map(lambda x: x.split('=', 1), options_list)) if options_list else {}


class DockerPluginManager(object):

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

self.dclient = DockerClient()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

self.dclient.api = client

self.parameters = TaskParameters(client)
self.check_mode = self.client.check_mode
self.results = {
u'changed': False,
u'actions': []
}
self.diff = self.client.module._diff

self.existing_plugin = self.get_existing_plugin()

state = self.parameters.state
porshkevich marked this conversation as resolved.
Show resolved Hide resolved
if state == 'present':
self.present()
elif state == 'absent':
self.absent()
elif state == 'enable':
self.enable()
elif state == 'disable':
self.disable()

def get_plugin_name(self):
return self.parameters.alias or self.parameters.name

def get_existing_plugin(self):
name = self.get_plugin_name()
try:
plugin = self.dclient.plugins.get(name)
except NotFound:
return None
except APIError as e:
self.client.fail(text_type(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

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 plugin

def has_different_config(self):
"""
Return the list of differences between the current parameters and the existing volume.

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

Choose a reason for hiding this comment

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

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

if self.parameters.plugin_options:
if not self.existing_plugin.settings:
differences.append('plugin_options')
else:
existing_options_list = self.existing_plugin.settings['Env']
existing_options = parse_options(existing_options_list)

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

Choose a reason for hiding this comment

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

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

if ((not existing_options.get(key) and value) or
not value or
value != existing_options[key]):
differences.append('plugin_settings.%s' % key)

return differences

def install_plugin(self):
if not self.existing_plugin:
if not self.check_mode:
try:
self.existing_plugin = self.dclient.plugins.install(self.parameters.plugin_name, self.parameters.plugin_alias)
except APIError as e:
self.client.fail(text_type(e))

self.results['actions'].append("Installed plugin %s" % self.get_plugin_name())
self.results['changed'] = True

def remove_plugin(self):
if self.existing_plugin:
if not self.check_mode:
try:
self.existing_plugin.remove()
except APIError as e:
self.client.fail(text_type(e))

self.results['actions'].append("Removed plugin %s" % self.get_plugin_name())
self.results['changed'] = True

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

Choose a reason for hiding this comment

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

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

self.existing_plugin.disable()
self.existing_plugin.configure(prepare_options(self.parameters.plugin_options))
self.existing_plugin.enable(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

self.results['actions'].append("Updated plugin %s settings" % self.get_plugin_name())
self.results['changed'] = True

def present(self):
differences = []
if self.existing_plugin:
differences = self.has_different_config()

if self.existing_plugin:
self.update_plugin()
else:
self.install_plugin()

if self.diff or self.check_mode or self.parameters.debug:
self.results['diff'] = differences
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not how a module should return differences. Some docker_* modules have done this in the past (and some might still do that), but it's simply wrong (and won't show up when the user runs ansible-playbook --diff).


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

def absent(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

self.remove_plugin()

def enable(self):
if self.existing_plugin:
if not self.check_mode:
try:
self.existing_plugin.enable(1)
except APIError as e:
self.client.fail(text_type(e))

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


def disable(self):
if self.existing_plugin:
if not self.check_mode:
try:
self.existing_plugin.disable()
except APIError as e:
self.client.fail(text_type(e))

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


def main():
argument_spec = dict(
name=dict(type='str', required=True),
alias=dict(type='str'),
state=dict(type='str', default='present', choices=['present', 'absent', 'enable', 'disable']),
plugin_options=dict(type='dict', default={}),
debug=dict(type='bool', default=False)
)

client = AnsibleDockerClient(
argument_spec=argument_spec,
supports_check_mode=True,
min_docker_version='2.6.0',
min_docker_api_version='1.25'
)

cm = DockerPluginManager(client)
client.module.exit_json(**cm.results)


if __name__ == '__main__':
main()