mongrelion / ansible-role-docker

Ansible role for installing Docker
MIT License
62 stars 39 forks source link

Support different versions of Docker Engine with official Docker and Rancher setup scripts #12

Closed marcusianlevine closed 7 years ago

marcusianlevine commented 7 years ago

The current release of the role uses apt and yum packages for installation, which tend to be several releases behind stable.

Docker provides a standard OS-agnostic setup script for the latest version of Docker at https//get.docker.com, which this PR implements fetching and running

To install a specific version of Docker, Rancher Labs provide a set of equivalent OS-agnostic setup scripts by version, which will be used if the new optional variable docker_version is provided to the role.

Could add checksum verification if security is a concern

mongrelion commented 7 years ago

Hello, @marcusianlevine

This looks like a great improvement over the current way that we handle the installation of Docker across the different distros.

A couple of points that I find important (some you have already mentioned):

  1. I'm not a big fan of pulling scripts from Internet and running them in production environments (I do this on my laptop, though, but never in production) so a way to verify the checksum of the installer is a must.

  2. Since the checksum of the installer is going to be a variable (I'm assuming this), it's important to leave a visible note for the user right next to it so that they change it only if they are a 100% sure what they are doing. Clumsy users might just adjust the checksum accordingly without knowing exactly what the impact/implication of it is.

  3. I think it's cleaner if we stick to one source for installing Docker. If Rancher's sources offer support for all of the versions that the users are interested in (my criteria would be current stable, one version before current and edge/testing), then let's just stick to that. This means that you can get rid of the step If docker_version is defined, switch to rancher setup script in the tasks/install.yml file.

  4. I think it's important that we let the users know that we are not pulling Docker from the official repo but from Rancher's. Not sure how this would impact people from choosing this role for their deployments but if it's a major concern I guess someone will just open an issue.

  5. Unfortunately our test suite is not complete yet (I just opened an issue https://github.com/mongrelion/ansible-role-docker/issues/13) so I'm going to have to ask you to also make sure that your changes don't break the installation on the list of distros that we officially support: https://github.com/mongrelion/ansible-role-docker/blob/master/meta/main.yml#L8-L17

Thanks!

marcusianlevine commented 7 years ago

@mongrelion,

Thanks for reviewing, I'll add optional checksum verification with appropriate warnings.

Looks like Rancher's install-docker repo includes scripts for all major and minor versions since 1.10, including edge releases (e.g. 17.05), so I'll switch everything to use Rancher's scripts and install latest stable release by default (including a default checksum).

Also Rancher's scripts still reference the official Docker apt and yum repositories, so I don't think we need to be concerned about Rancher's scripts installing unofficial releases.

Once I implement checksum I'll run some tests on the supported OS distros

marcusianlevine commented 7 years ago

Added checksum support and condensed URLs to use Rancher's scripts by default.

One could still use the official Docker setup script for latest release just by specifying the setup_script_url and overriding the checksum. We will need to update the default docker_version over time since it's pinned at 17.06...

Just need to run those tests now

marcusianlevine commented 7 years ago

Turns out that the official Docker 17.06.0-ce apt and yum distributions has a deprecation-related bug that is caused by an upstream error in docker-machine.

Should be resolved in 17.06.1 or 17.12, but for now I just downgraded the default version to 17.03 and that seems to work on the 3 target OS distros in the Vagrantfile

Zesty is giving me some trouble, looks like there isn't an official Docker apt repo for Zesty yet. I think that working around this issue as described here would require modifying the Rancher setup scripts though...

Personally I'm using 1.12.6 and 17.03 with Rancher on Ubuntu Xenial, so the issues with 17.06 and Zesty don't effect my use case

mongrelion commented 7 years ago

I agree that modifying the Rancher installation script is not a good idea so let's not do that.

I just took this for a spin and everything seems to be working, with the exception of the Zesty part, which is a pity because it breaks backwards compatibility in the role BUT I am willing to push it forward in the hope that soon™ a fix will be introduced on either side of Docker, Rancher or any other divine entity that cares enough to fix it here.

So, as a last step, could you please update the meta.yml file so that we don't advertise support for Zetsy?

marcusianlevine commented 7 years ago

Sure, I just commented the zesty line out since it will (hopefully) be supported again "soon" 😝

mongrelion commented 7 years ago

LGTM