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

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

Merged
merged 12 commits into from Jul 30, 2017

Conversation

marcusianlevine
Copy link
Collaborator

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

@marcusianlevine marcusianlevine changed the title Support multiple versions with official Docker and Rancher setup scripts Support different versions of Docker Engine with official Docker and Rancher setup scripts Jul 19, 2017
@mongrelion mongrelion self-assigned this Jul 20, 2017
@mongrelion
Copy link
Owner

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 Integration tests #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
Copy link
Collaborator Author

marcusianlevine commented Jul 22, 2017

@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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

marcusianlevine commented Jul 23, 2017

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

mongrelion commented Jul 30, 2017

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
Copy link
Collaborator Author

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

@mongrelion
Copy link
Owner

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants