Enable docker connector command override. #16198

bigjools · 2016-06-09T00:41:47Z

ISSUE TYPE

Feature Pull Request

ANSIBLE VERSION

2.2.0 0.0.devel from VERSION file

SUMMARY

If a hostvar 'docker_command' is present, it will be used in preference
to the default 'docker' command which runs locally. This enables, for
example, the use of ssh to connect to a remote docker daemon
instead of trying to connect to it directly, which is useful if you are using
ssh jumphosts.

Example config:

- name: Add containers to inventory
  add_host:
      name: "foo-{{item}}"
      groups: mygroup
      ansible_connection: docker
      docker_command: "/usr/bin/ssh {{hostvars[item].ansible_ssh_host}} docker"
  with_items: groups.containerhosts

This will iterate over all hosts in the 'containerhosts' group, add
a previously-created container to the inventory, and make sure it's
accessed via SSH through the original hosts.

bigjools · 2016-06-09T00:42:07Z

There are no tests for this, but I would like to add some. The reason there's no tests is because:

there are no existing tests that I can see at all for the docker connection
the _get_connection() method in the task exector has no unit tests itself, so I can't verify that the change is correctly passing the variables
I would welcome some suggestions since I am new to the Ansible code.

If a hostvar 'docker_command' is present, it will be used in preference to the default. This enables, for example, the use of ssh to connect to a remote docker daemon instead of trying to connect to it directly, which is useful if you are using ssh jumphosts. Example config: - name: Add containers to inventory add_host: name: "foo-{{item}}" groups: mygroup ansible_connection: docker docker_command: "/usr/bin/ssh {{hostvars[item].ansible_ssh_host}} docker" with_items: groups.containerhosts This will iterate over all hosts in the 'containerhosts' group, add a previously-created container to the inventory, and make sure it's accessed via SSH through the original hosts.

bigjools · 2016-09-06T00:06:41Z

This PR is now so old that it got conflicts. Is anyone interested in reviewing this? How do I get code to land in Ansible?

lathama · 2017-09-23T18:45:08Z

@bigjools from a quick look at http://docs.ansible.com/ansible/latest/docker_module.html this looks to be implemented. Can you confirm? If this is not still of interest can you close please.

bigjools · 2017-09-25T23:44:59Z

@lathama It looks like that module is deprecated (and I didn't find any equivalent to my change anyway?). I looked at http://docs.ansible.com/ansible/latest/docker_container_module.html#docker-container as well and I didn't see anything there either, I think it assumes direct connectivity to the docker daemon. What next?

ansibot · 2018-09-18T14:44:17Z

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

jwitko · 2018-10-23T14:14:35Z

lib/ansible/plugins/connection/docker.py

    @staticmethod
    def _sanitize_version(version):
        return re.sub('[^0-9a-zA-Z\.]', '', version)

    def _get_docker_version(self):

-        cmd = [self.docker_cmd]
+        # Force a quick copy, don't want to mutate base command.


Can you explain this a bit more? I don't think anything is wrong, I just don't understand :)

If the line would be cmd = self.docker_cmd, the original value of self.docker_cmd (which is a list) would be modified by the cmd += ...'s below. That's why a copy must be created.

jwitko · 2018-10-23T14:16:02Z

lib/ansible/plugins/connection/docker.py

@@ -120,8 +139,8 @@ def _get_docker_version(self):
                return self._sanitize_version(line.split()[2])

        # no result yet, must be newer Docker version
-        new_docker_cmd = [
-            self.docker_cmd,
+        # NOTE(bigjools): Is this missing docker_extra_args?


hm? is what missing docker_extra_args?

It is added in some other cases (but not all).

jwitko · 2018-10-23T14:17:49Z

I think this is a pretty neat option to have. My only concern is that how will anyone know that this is available for them to use? Is it possible to add some documentation surrounding this? Also thoughts on an integration test to spin up and connect to a container using this method?

felixfontein

Before it makes any sense to continue discussing this, you really need to rebase to current devel. This PR is based on a pretty old version of the codebase. The current codebase for example has documentation in docker.py, which doesn't even exist here.

(Also, note that the synchronize action plugin has some docker connection plugin specific code, which probably has to be adjusted as well.)

felixfontein · 2018-10-23T17:49:23Z

lib/ansible/plugins/connection/docker.py

    @staticmethod
    def _sanitize_version(version):
        return re.sub('[^0-9a-zA-Z\.]', '', version)

    def _get_docker_version(self):

-        cmd = [self.docker_cmd]
+        # Force a quick copy, don't want to mutate base command.
+        cmd = [] + self.docker_cmd


You should either use

Suggested change

cmd = [] + self.docker_cmd

cmd = self.docker_cmd[:]

or

Suggested change

cmd = [] + self.docker_cmd

cmd = list(self.docker_cmd)

felixfontein · 2018-10-23T17:50:33Z

lib/ansible/executor/task_executor.py

@@ -633,7 +633,8 @@ def _get_connection(self, variables, templar):
                except OSError:
                    conn_type = "paramiko"

-        connection = self._shared_loader_obj.connection_loader.get(conn_type, self._play_context, self._new_stdin)
+        connection = self._shared_loader_obj.connection_loader.get(
+            conn_type, self._play_context, self._new_stdin, variables=variables)


This probably needs a core review.

felixfontein · 2018-10-23T17:51:05Z

lib/ansible/plugins/connection/docker.py

        else:
            self.docker_cmd = distutils.spawn.find_executable('docker')
            if not self.docker_cmd:
                raise AnsibleError("docker command not found in PATH")
+        self.docker_cmd = self._split_cmd(self.docker_cmd)


felixfontein · 2018-10-23T17:52:03Z

lib/ansible/plugins/connection/docker.py

@@ -150,7 +172,8 @@ def _build_exec_cmd(self, cmd):
            version we are using, it will be provided to docker exec.
        """

-        local_cmd = [self.docker_cmd]
+        # Force a quick copy, don't want to mutate base command.
+        local_cmd = [] + self.docker_cmd


Also change this one, please.

felixfontein · 2018-10-23T17:59:54Z

lib/ansible/plugins/connection/docker.py

@@ -120,8 +139,8 @@ def _get_docker_version(self):
                return self._sanitize_version(line.split()[2])

        # no result yet, must be newer Docker version
-        new_docker_cmd = [
-            self.docker_cmd,
+        # NOTE(bigjools): Is this missing docker_extra_args?


It is added in some other cases (but not all).

ansibot · 2018-10-31T18:15:04Z

cc @akshay196 @danihodovic
click here for bot help

felixfontein · 2018-11-29T18:38:14Z

@bigjools are you still around? Can you rebase to remove the conflicts?

bigjools · 2018-11-29T22:04:17Z

Hi yes I am still around.

Given that it's taken 2 years to get a review, I've long moved on to different projects and I have lost the context for this change. Sorry.

If someone wants to take it over, that's fine.

felixfontein · 2018-11-30T07:14:01Z

I'm sorry it took so long until you got a review! The docker modules have been looking for a maintainer for some time; and it looks like that the docker connection plugin never had someone looking after it.

Anyway, thanks for your effort! I hope that someone will use it as a basis for a new PR for this feature.

bigjools · 2018-12-02T01:41:24Z

No problem, it happens sometimes on busy projects. And yes if someone needs this in the future, hopefully this is of some use.

…

On Fri, 30 Nov 2018 at 17:14, Felix Fontein ***@***.***> wrote: I'm sorry it took so long until you got a review! The docker modules have been looking for a maintainer for some time; and it looks like that the docker connection plugin never had someone looking after it. Anyway, thanks for your effort! I hope that someone will use it as a basis for a new PR for this feature. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16198 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALHikdmmTcTL3C9S6K3G6mPut8vsIfJ_ks5u0NrkgaJpZM4IxijL> .

gundalow · 2018-12-04T12:13:46Z

@bigjools Yup, sorry that this didn't get the reviews it deserved at the time.

Hi,
This PR seems to have gotten stuck, so we are going to close it. We like to include community contributions, so this is a difficult call for us. Here's why we're closing this one:

Given the discussion in Docker Agenda/Task list community#408 (comment)
It was created from an old version of the code
We want to clear out the backlog so it's easier to find bug fixes
There haven't been any comments or other activity in a long time

If you're the PR author, and you want to start the conversation back up, please:

reopen the PR and
rebase the branch

If you're a community member, and you want to see this change merged, please:

Create a new PR and
Reference this author in the git commit & PR and
Reference this PR in the new PR

If you've like to help with the Docker modules please comment on the Docker Agenda/Task List

How to rebase a branch: https://docs.ansible.com/ansible/latest/dev_guide/developing_rebasing.html
How to communicate with the Ansible community: https://docs.ansible.com/ansible/devel/community/communication.html
How to contribute to Ansible: https://docs.ansible.com/ansible/devel/community/index.html

bigjools force-pushed the connection_plugin_variables branch from 4c6fd6d to eb838d0 Compare June 9, 2016 06:04

nitzmahone added the feature_pull_request label Jun 9, 2016

ansibot added affects_2.3 This issue/PR affects Ansible v2.3 cloud docker 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. labels Dec 13, 2016

ansibot added c:executor/task_executor c:plugins/connection c:plugins/connection/docker labels Jan 12, 2017

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 11, 2017

ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Jun 29, 2017

calfonso requested a review from ryansb August 2, 2017 18:12

ansibot added executor/task_executor plugins/connection/docker labels Sep 7, 2017

ansibot added the new_contributor This PR is the first contribution by a new community member. label Oct 18, 2017

ansibot removed the new_contributor This PR is the first contribution by a new community member. label Nov 3, 2017

ansibot added the new_contributor This PR is the first contribution by a new community member. label Jan 22, 2018

ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 2, 2018

jwitko reviewed Oct 23, 2018

View reviewed changes

felixfontein suggested changes Oct 23, 2018

View reviewed changes

ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Nov 26, 2018

felixfontein mentioned this pull request Dec 3, 2018

Docker Agenda/Task list ansible/community#408

Closed

gundalow closed this Dec 4, 2018

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable docker connector command override. #16198

Enable docker connector command override. #16198

bigjools commented Jun 9, 2016

bigjools commented Jun 9, 2016

bigjools commented Sep 6, 2016

lathama commented Sep 23, 2017

bigjools commented Sep 25, 2017

ansibot commented Sep 18, 2018

jwitko Oct 23, 2018

felixfontein Oct 23, 2018

jwitko Oct 23, 2018

felixfontein Oct 23, 2018

jwitko commented Oct 23, 2018

felixfontein left a comment

felixfontein Oct 23, 2018

felixfontein Oct 23, 2018

felixfontein Oct 23, 2018

felixfontein Oct 23, 2018

felixfontein Oct 23, 2018

ansibot commented Oct 31, 2018

felixfontein commented Nov 29, 2018

bigjools commented Nov 29, 2018

felixfontein commented Nov 30, 2018

bigjools commented Dec 2, 2018 via email

gundalow commented Dec 4, 2018

Enable docker connector command override. #16198

Enable docker connector command override. #16198

Conversation

bigjools commented Jun 9, 2016

ISSUE TYPE

ANSIBLE VERSION

SUMMARY

bigjools commented Jun 9, 2016

bigjools commented Sep 6, 2016

lathama commented Sep 23, 2017

bigjools commented Sep 25, 2017

ansibot commented Sep 18, 2018

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

jwitko commented Oct 23, 2018

felixfontein left a 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

ansibot commented Oct 31, 2018

felixfontein commented Nov 29, 2018

bigjools commented Nov 29, 2018

felixfontein commented Nov 30, 2018

bigjools commented Dec 2, 2018 via email

gundalow commented Dec 4, 2018