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

Add support for Docker 18.09.3. #6347

Merged
merged 1 commit into from
Mar 15, 2019
Merged

Conversation

tsuna
Copy link
Contributor

@tsuna tsuna commented Jan 16, 2019

Starting from Docker 18.09.0, the Docker distribution has been split in
3 packages: the Docker daemon, the Docker CLI, and for containerd. This
adds a twist to how to upgrade Docker from the base image as the daemon
and CLI packages must be installed at the same time, otherwise dpkg/rpm
will refuse to upgrade (the new CLI is incompatible with the old package
and the daemon can't be installed without first installing the CLI and
the new containerd, so the upgrade MUST happen in a single transaction).

This code change thus adds the possibility to specify sources for the
CLI and containerd in nodeup, and the ability to apply the multi-package
upgrade atomically with dpkg/rpm.

Note: I tested this on one of my kops clusters in AWS.

This fixes #5747

@k8s-ci-robot
Copy link
Contributor

Hi @tsuna. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 16, 2019
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 16, 2019
@tsuna
Copy link
Contributor Author

tsuna commented Jan 17, 2019

/assign @justinsb @andrewsykim @chrisz100

@mikesplain
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 18, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2019
@tsuna
Copy link
Contributor Author

tsuna commented Jan 22, 2019

I reworked my change a bit by instead adding a more generic provision for specifying extra packages to install at the same time as Docker. I noticed that RHEL/CentOS has needed this for a while to deploy SELinux policies anyway, so I figured it made more sense to unify this existing need with the new one (the small difference is that the SELinux policies could be installed on their own, which is why the existing kludge in the code was working fine, whereas now to upgrade Docker with the newer 3-package distribution everything needs to happen in a single dpkg transaction).

Let me know what you think.

@tsuna
Copy link
Contributor Author

tsuna commented Jan 25, 2019

I just rebased my change, can someone take a look please?

@tsuna
Copy link
Contributor Author

tsuna commented Feb 4, 2019

Ping, pretty please :)

@@ -162,7 +169,7 @@ func (e *Package) findDpkg(c *fi.Context) (*Package, error) {
installed = true
installedVersion = version
healthy = fi.Bool(true)
case "iF":
case "iF", "iU":
Copy link
Contributor

Choose a reason for hiding this comment

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

What’s this iU?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happened in an earlier version of this change that attempted to install the packages one by one (which, ultimately, doesn't work). iF means the package is selected to be installed and is currently in half-configured state, while iU means it's selected to be installed and is currently merely unpacked.

It might not be needed anymore since this iteration of the code installs all 3 packages in a single transaction, but having the code craps out if it sees the package in iU state instead of continuing to try/wait until the install is complete seems like a bad idea. So I figured I'd leave this here as it makes sense to handle iU in the same way that iF is handled.

@tsuna
Copy link
Contributor Author

tsuna commented Feb 4, 2019

Just rebased my change one more time, to drop a commit that had become redundant since commit 24222ff merged 8 days ago.

@tsuna
Copy link
Contributor Author

tsuna commented Feb 5, 2019

/retest
Looks like a spurious infrastructure failure:

error reading s3://k8s-kops-prow/e2e-8592-ff1eb.test-cncf-aws.k8s.io/config: Unable to list AWS regions: AuthFailure: AWS was not able to validate the provided access credentials

@tsuna
Copy link
Contributor Author

tsuna commented Feb 7, 2019

/retest (it was the same infrastructure failure I mentioned above)
🤞

@coreypobrien
Copy link
Contributor

Can we bump this to 18.09.2 to address CVE-2019-5736 at the same time?

@tsuna tsuna changed the title Add support for Docker 18.09.2. Add support for Docker 18.09.3. Feb 28, 2019
@tsuna
Copy link
Contributor Author

tsuna commented Feb 28, 2019

I rebased again and adjusted the change for Docker 18.09.3 that came out today.

I didn't understand the e2e failure that hit previously. It says should support inline execution and attach [It], dunno what that means.

Starting from Docker 18.09.0, the Docker distribution has been split in
3 packages: the Docker daemon, the Docker CLI, and for containerd.  This
adds a twist to how to upgrade Docker from the base image as the daemon
and CLI packages must be installed at the same time, otherwise dpkg/rpm
will refuse to upgrade (the new CLI is incompatible with the old package
and the daemon can't be installed without first installing the CLI and
the new containerd, so the upgrade MUST happen in a single transaction).

This code change thus adds the possibility to specify additional packages
to install in the same dpkg/yum transaction, such as the Docker CLI and
containerd in nodeup, and the ability to apply the multi-package upgrade
atomically with dpkg/rpm.

We also use this new mechanism for the SELinux policy on RHEL/CentOS.
@tsuna
Copy link
Contributor Author

tsuna commented Mar 5, 2019

Just rebased again. Please, pretty please, help me push this through the finish line this week. 🙏

@tsuna
Copy link
Contributor Author

tsuna commented Mar 8, 2019

Pretty please, can I haz LGTM & approvalz? 😢

@chrisz100
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2019
@tsuna
Copy link
Contributor Author

tsuna commented Mar 8, 2019

Thanks @chrisz100! Almost in time for me to check this off my list for our weekly standup in 10min. How can I get the approval now? If you know who I need to bribe, let me know 😄

@montanaslim
Copy link

any idea when this will be approved?

@tsuna
Copy link
Contributor Author

tsuna commented Mar 12, 2019

I don't know, I don't even understand the difference between LGTM and approval, but if anyone knows who I need to bribe, let me know.

@tsuna
Copy link
Contributor Author

tsuna commented Mar 13, 2019

@mikesplain @justinsb can one of you guys please approve this PR? I'll pay you 100 KubeCoin on the day of the ICO. Just need to finish my business plan for a blockchain-backed k8s control plane. etcd is for losers. TIA.

@montanaslim
Copy link

@tsuna lol'd irl

@justinsb justinsb added this to the 1.12 milestone Mar 14, 2019
@chrisz100
Copy link
Contributor

@tsuna it’s the two step review process - lgtm is technically does the code look good and technically doable by anyone in the Kubernetes GitHub org- approver is a higher level that is supposes to check for does it add to our roadmap when we approve it right now or is later a better time.

As it seems like it’s targeted for 1.12 now so rather soon!

@justinsb
Copy link
Member

Sorry this took so long @tsuna - it hit in the middle of the test account problems followed by the docker CVE, and we had to prioritize the CVE.

I think the package version changed in one place, but I'm just going to send another PR rather than risk forcing another rebase on you. Plus I want those kubecoins :-)

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, tsuna

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2019
justinsb added a commit to justinsb/kops that referenced this pull request Mar 15, 2019
Follow up to kubernetes#6347 - add a test for some of the names based on some
heuristics, and fix some of the problems that popped up.
@k8s-ci-robot k8s-ci-robot merged commit 16e846d into kubernetes:master Mar 15, 2019
justinsb added a commit to justinsb/kops that referenced this pull request Mar 15, 2019
Follow up to kubernetes#6347 - add a test for some of the names based on some
heuristics, and fix some of the problems that popped up.
@shahbhavik01
Copy link

shahbhavik01 commented Mar 26, 2019

@tsuna
I did:
kops edit cluster

Under cluster spec:

spec:
    docker:
        version: 18.09.3

kops version: 1.11.1
kubernetes version 1.10.12

and then I ran:

kops update cluster <CLUSTER-NAME> --yes
kops rolling-update cluster --yes

Even then the docker-version remains 17.3.2. Any help would be appreciated!

kubectl describe nodes | grep docker
 Container Runtime Version:  docker://17.3.2
 Container Runtime Version:  docker://17.3.2

@rdrgmnzs
Copy link
Contributor

@shahbhavik01 this new version will not be in kops until 1.12 is released.

@tsuna
Copy link
Contributor Author

tsuna commented Mar 26, 2019

You can look at #6448 (edit: more specifically, this comment) if you wanna try a version you build yourself from master, I posted some instructions there :)

@marek-obuchowicz
Copy link

marek-obuchowicz commented Apr 18, 2019

@tsuna / @shahbhavik01 I tried kops 1.12.0-beta.2 but for some reason it uses still docker://18.6.3 with kubernetes v1.12.7. Is this expected (looking on the fact that this pr was merged already)?

update: all good, 18.06.3 was chosen as default, but it's possible to select 18.09.3 (as value for spec.docker.version). Is it also possible when using kops 1.12 with older k8s (like 1.11)?

@tsuna
Copy link
Contributor Author

tsuna commented Apr 18, 2019

Cześć Marek, the default version used by kops remains unchanged. I'm not sure why, TBH, but you need to edit your cluster manifest to pick the Docker version you want.

apiVersion: kops/v1alpha2
kind: Cluster
[...]
spec:
[...]
  docker:
    version: 18.09.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for newer docker versions like 18.03