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

Added a Docker Machine dynamic inventory plugin #54946

Open
wants to merge 31 commits into
base: devel
from

Conversation

Projects
None yet
7 participants
@ximon18
Copy link
Contributor

ximon18 commented Apr 6, 2019

SUMMARY

Added a new dynamic inventory plugin to integrate Ansible with Docker Machine, just like the existing Docker Swarm dynamic inventory plugin in Ansible 2.8.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

docker_machine.py

ADDITIONAL INFORMATION

See https://github.com/ximon18/ansible-docker-machine-inventory-plugin for more information.

The intent of this PR is to begin the process of proposing the module for inclusion in Ansible core. However, there are no unit or integration tests yet. The docker_swarm inventory plugin has integration
tests but has some concerning note in its 'aliases' file about disabling docker due to test instability and also I wouldn't know at this point how to get Docker Machine installed on the integration test platform. I did not find any unit tests for the existing docker_swarm inventory plugin to use as a basis for my own module.

Tested with Ansible 2.7.10, Docker Machine 0.16.0, Python 3.6.7 on Ubuntu 18.04.2 LTS.

Thanks,

Ximon

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 6, 2019

Thanks for creating this PR! Unfortunately, it only contains a symlink to a file not included: /home/ximon/src/ansible-docker-machine-inventory-plugin/docker_machine.py Could you please update the PR? :)

@ximon18

This comment has been minimized.

Copy link
Contributor Author

ximon18 commented Apr 6, 2019

Thanks for creating this PR! Unfortunately, it only contains a symlink to a file not included: /home/ximon/src/ansible-docker-machine-inventory-plugin/docker_machine.py Could you please update the PR? :)

Apologies for that silly mistake. Corrected.

@ansibot ansibot removed the needs_triage label Apr 6, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 6, 2019

The test ansible-test sanity --test pylint [explain] failed with 12 errors:

lib/ansible/plugins/inventory/docker_machine.py:94:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:111:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:123:46: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:124:52: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:130:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:134:56: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:136:56: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:146:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:151:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:156:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:159:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:174:22: ansible-format-automatic-specification Format string contains automatic field numbering specification

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

docs/docsite/rst/plugins/inventory/docker_machine.rst:23:0: warning: Unknown target name: "dm".

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

lib/ansible/plugins/inventory/docker_machine.py:44:1: W293 blank line contains whitespace
lib/ansible/plugins/inventory/docker_machine.py:79:1: E302 expected 2 blank lines, found 1

click here for bot help

@felixfontein
Copy link
Contributor

felixfontein left a comment

Just some quick comments for tonight. ansibot also listed some problems from the CI runs.

Show resolved Hide resolved lib/ansible/plugins/inventory/docker_machine.py Outdated
Show resolved Hide resolved lib/ansible/plugins/inventory/docker_machine.py Outdated
@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 6, 2019

Apologies for that silly mistake. Corrected.

No problem! Thanks again for submitting this.

Please note that Feature Freeze for Ansible 2.8 is coming up very soon (Thursday, April 11), so the chances for this PR making it into Ansible 2.8 aren't very high. I'll try to give you more feedback tomorrow (gotta go sleeping now), but be warned that I don't have any experience with docker machine.

PS: there are more places where you can improve formatting using the syntax from the link. I just didn't want to mark everything now ;)

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 6, 2019

The test ansible-test sanity --test pylint [explain] failed with 12 errors:

lib/ansible/plugins/inventory/docker_machine.py:94:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:111:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:123:46: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:124:52: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:130:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:134:56: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:136:56: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:146:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:151:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:156:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:159:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:174:22: ansible-format-automatic-specification Format string contains automatic field numbering specification

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

docs/docsite/rst/plugins/inventory/docker_machine.rst:23:0: warning: Unknown target name: "dm".

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

lib/ansible/plugins/inventory/docker_machine.py:44:1: W293 blank line contains whitespace
lib/ansible/plugins/inventory/docker_machine.py:79:1: E302 expected 2 blank lines, found 1

click here for bot help

@ximon18

This comment has been minimized.

Copy link
Contributor Author

ximon18 commented Apr 7, 2019

Please note that Feature Freeze for Ansible 2.8 is coming up very soon (Thursday, April 11), so the chances for this PR making it into Ansible 2.8 aren't very high. I'll try to give you more feedback tomorrow (gotta go sleeping now), but be warned that I don't have any experience with docker machine.

Thx, v appreciated.

FYI this is where I am using Docker Machine and this is where I invoke Ansible with my Docker Machine dynamic inventory plugin, and this is a playbook that references the created variables. Perhaps it helps to see it used.

PS: there are more places where you can improve formatting using the syntax from the link. I just didn't want to mark everything now ;)

I took a look at the Docker Swarm inventory plugin docs and added a few more uses of the link syntax.

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 11, 2019

@ximon18 should be fine, ansibot / manual merge will squash anyway.

@felixfontein
Copy link
Contributor

felixfontein left a comment

I think all points are resolved now, at least from my point of view. I hope it is OK to defer tests running in CI until later. @bcoca @s-hertel can you re-check / say something if this isn't ok?

shipit

ximon18 added some commits Apr 11, 2019

ximon18 added some commits Apr 11, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 11, 2019

Have you tested it both with Py2 and Py3? (Can't test Py2 right now, am mostly AFK this evening.)

@felixfontein
Copy link
Contributor

felixfontein left a comment

Looks OK from my POV, can't run tests now since mostly AFK; if runs with Py2 and Py3 (the latter did work earlier), I'm for shipit.

@ximon18

This comment has been minimized.

Copy link
Contributor Author

ximon18 commented Apr 11, 2019

On Thu, 11 Apr 2019, 18:15 Felix Fontein, @.***> wrote: Have you tested it both with Py2 and Py3? (Can't test Py2 right now, am mostly AFK this evening.)

I just tested with Python 2.7.15rc1 on Ubuntu 18.04 and I didn't encounter any problems, including when the docker-machine binary was missing and when docker-machine env (mocked) returned a non-zero exit code. I was able to run my normal playbooks and ansible-inventory commands without any issues.

To be clear: all of my other testing until now was with Python 3.

@ximon18
Copy link
Contributor Author

ximon18 left a comment

ready_for_review

@ximon18

This comment has been minimized.

Copy link
Contributor Author

ximon18 commented Apr 11, 2019

@bcoca: Will this make the 2.8 freeze deadline?

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 11, 2019

@ximon18 looks like it didn't, stable-2.8 was branched some hours ago: this is the first commit not in devel.

@ximon18

This comment has been minimized.

Copy link
Contributor Author

ximon18 commented Apr 12, 2019

@bcoca & @felixfontein: As this didn't make the 2.8 freeze but the basic intent and logic seems to be acceptable, if you can assist me with a means of removing 'unsupported' from the integration test then I will continue to improve it, and I would also like to know the right way to add unit test coverage.

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 13, 2019

@ximon18 I will try to do so :) I'll try to talk to testing people about this next week to find out how to get this working, resp. what their preferred approach to this is.

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 15, 2019

I've talked to @mattclay and @sivel about this; the preferred ways to do tests are (in this order):

  1. use localhost as a docker-machine "VM";
  2. use a fake docker-machine binary

The first way should be possible with the generic driver: https://docs.docker.com/machine/drivers/generic/ This should definitely not be done on a test running itself in docker, i.e. it should only be run on the "real" VMs, which currently are the RHEL7 and RHEL8 nodes. (This is similar to the docker_swarm module tests, which are also restricted to VMs.)

I would assume that this should be done in the following way:

  1. install docker (via setup_docker)
  2. install sshd, make it listen on localhost
  3. generate a random SSH key and install it for root
  4. use docker-machine to connect to localhost via this SSH key
  5. after the tests, purge docker daemon (so it will be properly reinstalled for the next docker_* tests, and won't have any config from docker-machine left)

Maybe it's also sufficient to fake a config.json in ~/.docker/machine/machines/xxx, and providing some fake certs, though then running commands on that node definitely won't work, and it might randomly break in the future if docker-machine changes something in its internals...

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 15, 2019

A "template" for the aliases file which contains all restrictions that should be useful: https://github.com/ansible/ansible/blob/devel/test/integration/targets/inventory_docker_swarm/aliases

@mattclay

This comment has been minimized.

Copy link
Member

mattclay commented Apr 15, 2019

These steps shouldn't be needed, as that's already provided by the test infrastructure:

  1. install sshd, make it listen on localhost
  2. generate a random SSH key and install it for root
@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Apr 15, 2019

@mattclay can you point us to where this is set up, i.e. where can we get the private ssh key from? And can root log in, or is sudo needed (and password-less sudo set up for the ssh user)?

@mattclay

This comment has been minimized.

Copy link
Member

mattclay commented Apr 15, 2019

@felixfontein For the tests running on remote VMs (RHEL, FreeBSD, macOS) it's set up here:

# Generate our ssh key and add it to our authorized_keys file.
# We also need to add localhost's server keys to known_hosts.
if [ ! -f "${HOME}/.ssh/id_rsa.pub" ]; then
ssh-keygen -m PEM -q -t rsa -N '' -f "${HOME}/.ssh/id_rsa"
cp "${HOME}/.ssh/id_rsa.pub" "${HOME}/.ssh/authorized_keys"
for key in /etc/ssh/ssh_host_*_key.pub; do
pk=$(cat "${key}")
echo "localhost ${pk}" >> "${HOME}/.ssh/known_hosts"
done
fi

Yes, root can log in without sudo.

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.