Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

docs: Pin docker to version 18.06 #374

Merged
merged 2 commits into from
Mar 4, 2019
Merged

docs: Pin docker to version 18.06 #374

merged 2 commits into from
Mar 4, 2019

Conversation

jodh-intel
Copy link
Contributor

Docker 18.09 removed devicemapper support but did not provide an
alternative. This can cause problems for users so update the install
docs to install Docker at version 18.06 (the last version that supports
devicemapper).

This is a temporary solution until either docker provide an alternative
or we find a way to work around the Docker feature being removed.

Fixes #373.

Signed-off-by: James O. D. Hunt [email protected]

@jodh-intel jodh-intel force-pushed the pin-docker-to-version-18.06 branch from ee7fa79 to c9d6f59 Compare February 5, 2019 10:45
@jodh-intel
Copy link
Contributor Author

And now for the caveats...

/cc @egernst.

@jodh-intel
Copy link
Contributor Author

/test

@jodh-intel jodh-intel force-pushed the pin-docker-to-version-18.06 branch from c9d6f59 to 85dc9c7 Compare February 5, 2019 11:17
@jodh-intel
Copy link
Contributor Author

Some of the CI builds are failing with:

Cloning into 'tests'...
/home/centos/tests/.ci/lib.sh: line 19: GOPATH: unbound variable

@jodh-intel
Copy link
Contributor Author

/retest

@jodh-intel
Copy link
Contributor Author

Fedora CI:

[kata-containers-documentation-fedora-PR] $ /bin/bash /tmp/jenkins7239932866990890670.sh
Cloning into 'tests'...
/home/fedora/tests/.ci/lib.sh: line 19: GOPATH: unbound variable
Build step 'Execute shell' marked build as failure
Setting status of 85dc9c7d17f69987e7512e9395e5ff7e4c8e4e06 to FAILURE with url http://jenkins.katacontainers.io/job/kata-containers-documentation-fedora-PR/50/ and message: 'Build finished. '
Using context: jenkins-ci-fedora
Finished: FAILURE

CentOS CI:

[kata-containers-documentation-centos-7-4-PR] $ /bin/bash /tmp/jenkins1058249980225164497.sh
Cloning into 'tests'...
/home/centos/tests/.ci/lib.sh: line 19: GOPATH: unbound variable
Build step 'Execute shell' marked build as failure
Setting status of 85dc9c7d17f69987e7512e9395e5ff7e4c8e4e06 to FAILURE with url http://jenkins.katacontainers.io/job/kata-containers-documentation-centos-7-4-PR/56/ and message: 'Build finished. '
Using context: jenkins-ci-centos-7-4
Finished: FAILURE

@jodh-intel
Copy link
Contributor Author

Blocked on kata-containers/tests#1134, which should fix the GOPATH error.

@jodh-intel jodh-intel mentioned this pull request Feb 5, 2019
Closed
@jodh-intel
Copy link
Contributor Author

This PR is effectively also blocked on kata-containers/tests#1136 I think as we need to tweak the CI scripts to remove docker between running each documentation test.

marcov
marcov reviewed Feb 5, 2019
View reviewed changes
install/docker/opensuse-docker-install.md Outdated

```bash
$ sudo zypper -n install libcgroup1
$ sudo zypper -n install docker
$ sudo zypper -n install -f docker-18.06.1_ce-51.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Another neat way to do this without hard coding a specific docker version is:

docker -n install docker<18.09'

This is going to work on SLES too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @marcov - I've updated the branch for opensuse + sles.

@jodh-intel jodh-intel mentioned this pull request Feb 5, 2019
Closed
@GabyCT
Copy link
Collaborator

GabyCT commented Feb 5, 2019

@jodh-intel tested on OpenSuse and it is working, I do not have a license anymore of RHEL.

@marcov
Copy link
Contributor

marcov commented Feb 5, 2019

@jodh-intel fyi the docker packages for SUSE/SLE are built with a custom commands, and the devicemapper storage driver is still available in the 18.09 version we distribute.

@chavafg
Copy link
Contributor

chavafg commented Feb 6, 2019

kata-containers/tests#1134 merged

Now I see this error:

1 upgraded, 1 newly installed, 0 to remove and 125 not upgraded.
E: Held packages were changed and -y was used without --allow-change-held-packages.
Build step 'Execute shell' marked build as failure

@jodh-intel
Copy link
Contributor Author

@chavafg - yep, I need to update the doc CI tests on this PR to remove docker between running each doc test now that kata-containers/tests#1136 is merged...

@jodh-intel jodh-intel force-pushed the pin-docker-to-version-18.06 branch from 85dc9c7 to a20e46b Compare February 6, 2019 09:52
@jodh-intel
Copy link
Contributor Author

/test

@jodh-intel jodh-intel force-pushed the pin-docker-to-version-18.06 branch from a20e46b to 09f7479 Compare February 6, 2019 09:58
@jodh-intel
Copy link
Contributor Author

@fatherlinux, @mrguitar, @lsm5 - could you provide any input from the RHEL perspective? We don't have licenses any more and therefore cannot test updates to the RHEL install docs here.

@jodh-intel
Copy link
Contributor Author

/retest

@jodh-intel
Copy link
Contributor Author

Now blocked on kata-containers/tests#1144.

@jodh-intel jodh-intel mentioned this pull request Feb 6, 2019
Merged
@chavafg
Copy link
Contributor

chavafg commented Feb 6, 2019

/test

@jodh-intel
Copy link
Contributor Author

/retest

@jodh-intel jodh-intel force-pushed the pin-docker-to-version-18.06 branch from 09f7479 to 7a49371 Compare February 6, 2019 17:42
@jodh-intel
Copy link
Contributor Author

/retest

@jodh-intel
Copy link
Contributor Author

Huzzah! Atlast we have a CI pass for this PR! ;)

@chavafg, @GabyCT - btw, I spotted this in the Jenkins logs:

Switched to branch 'PR_374'
Current branch PR_374 is up to date.
bash: .ci/hypervisors//filter_docker_.sh: No such file or directory
make -C cmd/checkcommits

@grahamwhaley
Copy link
Contributor

Yeah, I think that is because we don't check that $HYPERVISOR or ARCH or similar is not "" before we try to deref it and find the filter file. That occurred to me as I saw the PR fly by earlier in the week. @GabyCT - can we add a null/non-exist check to avoid that pls?

@grahamwhaley
Copy link
Contributor

hmm, maybe I need to enable github ack checks on this repo..... can I get an ack from @marcov maybe before we actually press the button?
I'm going to DNM it until that point, but am v.happy if others agree that is over cautions and go ahead and merge....

@jodh-intel jodh-intel force-pushed the pin-docker-to-version-18.06 branch from fa23540 to a9ce5ff Compare February 8, 2019 11:59
@jodh-intel
Copy link
Contributor Author

@marcov, @mssola, @flavio - could you comment on this?

@egernst - I know you were keen to see this land so could you also ptal?

egernst
egernst previously approved these changes Feb 11, 2019
View reviewed changes
Copy link
Member

@egernst egernst left a comment

Choose a reason for hiding this comment

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

@jodh-intel - this looks good to me. Hopefully we'll be able to get this fixed soon and move to latest.

@krsna1729
Copy link

krsna1729 commented Feb 11, 2019 •
edited
Loading

Quick question @jodh-intel - Any reason why we don't install as shown below, which supports several distros, other than friends don't let friends do curl-pipe-bash :P

curl -fsSL https://get.docker.com | VERSION=18.06 sh

@krsna1729
Copy link

also 18.06.2 vs 18.06.1 in the current changes to use the one patched for the runc CVE?
https://docs.docker.com/engine/release-notes/#18062

@jcvenegas
Copy link
Member

@jodh-intel please rebase this PR.

@jodh-intel jodh-intel force-pushed the pin-docker-to-version-18.06 branch from a9ce5ff to 31dd5eb Compare February 18, 2019 08:50
@jodh-intel
docs: Pin docker to version 18.06
ceae7b4 
Docker 18.09 removed devicemapper support but did not provide an
alternative. This can cause problems for users so update the install
docs to install Docker at version 18.06 (the last version that supports
devicemapper).

This is a temporary solution until either docker provide an alternative
or we find a way to work around the Docker feature being removed.

Note the extra logic required for Fedora since 18.06 is not available
for that release.

Fixes kata-containers#373.

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel jodh-intel force-pushed the pin-docker-to-version-18.06 branch from 31dd5eb to ceae7b4 Compare February 18, 2019 08:56
@jodh-intel
Copy link
Contributor Author

@jcvenegas - branch updated.

@krsna1729 - Updated for 18.06.2. Regarding your comment about using get.docker.com, that needs a lot more discussion and is not appropriate for this PR imho. You are of course free to install docker how you wish. However, although the get.docker.com approach is convenient, I'm not convinced many users would be happy running an opaque script downloaded off the web as root, particularly on a server. Yes, I know you can browse to that site and see the script, but it could change. Or the site could get hacked, or, or, or... By providing clear instructions in our installation guides, admins comfortable with shell script can see exactly what is happening.

If you wish to raise a PR using the get.docker.com approach, we can maybe force the issue and get more input from the rest of the community.

@jodh-intel
Copy link
Contributor Author

Ping @kata-containers/documentation - The dnm was added by @grahamwhaley to get an extra review or two I think but this should have landed long ago. Please review.

klynnrif
klynnrif previously requested changes Feb 28, 2019
View reviewed changes
Copy link

@klynnrif klynnrif left a comment

Choose a reason for hiding this comment

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

Scrubbed for grammar, structure, and flow. One suggested line rewrite the is duplicated in the Notes section. Thanks!

install/docker/centos-docker-install.md Outdated
> - This step is only required if Docker is not installed on the system.
> - Newer versions of Docker have
> [removed devicemapper support](https://github.com/kata-containers/documentation/issues/373)
> so the commands below install the latest version which includes
Copy link

Choose a reason for hiding this comment

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

Suggested rewrite:
so the following commands install the latest version, which includes

install/docker/debian-docker-install.md Outdated
> - This step is only required if Docker is not installed on the system.
> - Newer versions of Docker have
> [removed devicemapper support](https://github.com/kata-containers/documentation/issues/373)
> so the commands below install the latest version which includes
Copy link

Choose a reason for hiding this comment

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

Suggested rewrite:
so the following commands install the latest version, which includes

install/docker/fedora-docker-install.md Outdated
> - This step is only required if Docker is not installed on the system.
> - Newer versions of Docker have
> [removed devicemapper support](https://github.com/kata-containers/documentation/issues/373)
> so the commands below install the latest version which includes
Copy link

Choose a reason for hiding this comment

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

Suggested rewrite:
so the following commands install the latest version, which includes

install/docker/opensuse-docker-install.md Outdated
> - This step is only required if Docker is not installed on the system.
> - Newer versions of Docker have
> [removed devicemapper support](https://github.com/kata-containers/documentation/issues/373)
> so the commands below install the latest version which includes
Copy link

Choose a reason for hiding this comment

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

Suggested rewrite:
so the following commands install the latest version, which includes

install/docker/sles-docker-install.md Outdated
> - This step is only required if Docker is not installed on the system.
> - Newer versions of Docker have
> [removed devicemapper support](https://github.com/kata-containers/documentation/issues/373)
> so the commands below install the latest version which includes
Copy link

Choose a reason for hiding this comment

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

Suggested rewrite:
so the following commands install the latest version, which includes

install/docker/ubuntu-docker-install.md Outdated
> - This step is only required if Docker is not installed on the system.
> - Newer versions of Docker have
> [removed devicemapper support](https://github.com/kata-containers/documentation/issues/373)
> so the commands below install the latest version which includes
Copy link

Choose a reason for hiding this comment

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

Suggested rewrite:
so the following commands install the latest version, which includes

@jodh-intel
docs: Clarified docker install note
5524fc3 
Improved wording in docker installation Note based on review feedback.

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel
Copy link
Contributor Author

@klynnrif - thanks for reviewing. Branch updated.

@marcov - please tal (and remove the dnm label if you're happy ;)

@grahamwhaley - did you originally ack (I think we didn't save the acks way back then)? Either way, could you tal?

Ping @kata-containers/documentation - would be great to get this landed!

grahamwhaley
grahamwhaley approved these changes Mar 1, 2019
View reviewed changes
Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

lgtm

@grahamwhaley
Copy link
Contributor

dropping the dnm. I think you will need @klynnrif to ack to clear the change request....

@egernst egernst dismissed klynnrif’s stale review March 4, 2019 18:42

Review addressed

@egernst egernst merged commit a2fe011 into kata-containers:master Mar 4, 2019
@jodh-intel jodh-intel mentioned this pull request Mar 5, 2019
Closed
This was referenced Mar 21, 2019
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marcov marcov marcov left review comments

@grahamwhaley grahamwhaley grahamwhaley approved these changes

@egernst egernst egernst approved these changes

@GabyCT GabyCT GabyCT approved these changes

@klynnrif klynnrif klynnrif left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jodh-intel @GabyCT @marcov @chavafg @grahamwhaley @krsna1729 @jcvenegas @egernst @klynnrif