Install docker 17.03 via binary and configure logs rotation on all systems via daemon.json #382

mrIncompetent · 2018-10-30T08:39:33Z

What this PR does / why we need it:
This PR will unify the docker installation on all operating systems.
Instead of installing the best(Or close to best) version via the individual OS repositories, docker will now be installed by simply downloading the release binary & creating the systemdunit.
This ensures that we always install the same version across all operating systems.

Additionally - and this was actually the initial idea - all operating systems now use the docker log-rotation via daemon.json

Fixes #292

Install docker 17.03 via binary and configure logs rotation on all systems via daemon.json

alvaroaleman · 2018-10-30T08:41:44Z

pkg/userdata/ubuntu/testdata/dist-upgrade-on-boot.golden

+      "log-driver": "json-file",
+      "log-opts": {
+        "max-size": "10m",
+        "max-file": "2"


Is this a per-container setting?

Unfortunately yes.

Do we already limit the overall journald size?

Yes, on every system we set:
https://github.com/kubermatic/machine-controller/pull/382/files#diff-8e277877f731647a2f08ec1c985fb9abR16

alvaroaleman · 2018-10-30T08:43:10Z

Is there a way we can find out via the Kube api which storage driver docker uses?

mrIncompetent · 2018-10-30T08:48:30Z

Is there a way we can find out via the Kube API which storage driver docker uses?

I don't think so. The Node Object does only container the container runtime version. The storage driver seems more an implementation detail, which should not even appear in the Node object.

alvaroaleman · 2018-10-30T17:06:58Z

/lgtm

alvaroaleman · 2018-10-31T17:28:22Z

/hold

@mrIncompetent The issue with reading logs from the journald log driver only exists with the docker package from the built-in repo (docker.io), not with the one from the upstream repo (docker-ce) - Maybe just start using docker-ce for all Kube versions and not only 1.12?

mrIncompetent · 2018-10-31T17:43:27Z

@mrIncompetent The issue with reading logs from the journald log driver only exists with the docker package from the built-in repo (docker.io), not with the one from the upstream repo (docker-ce) - Maybe just start using docker-ce for all Kube versions and not only 1.12?

docker-ce from the docker repositories only contains 18.03 (Only verified with kubernetes 1.12+).
See https://download.docker.com/linux/ubuntu/dists/bionic/stable/binary-amd64/

Though, the docker.io (from the ubuntu bionic repo) only has version 17.12: https://packages.ubuntu.com/bionic/docker.io

Kubernetes 1.9 - 1.11 supports 17.03 as highest version.

alvaroaleman · 2018-10-31T18:02:23Z

Kubernetes 1.9 - 1.11 supports 17.03 as highest version.

Sort-Of, 17.03 is the highest verified Docker version, that doesn't mean higher Docker versions won't work. E2Es do pass for all Kubernetes versions with Docker 1806

mrIncompetent · 2018-10-31T18:02:23Z

Maybe going with the kops way of doing it, could be a solution?
https://github.com/kubernetes/kops/blob/master/nodeup/pkg/model/docker.go#L426

Basically downloading the release & manually setting it up.

It would decouple us from the operating system specific package management - which has up & downsides.
But it would enable us to use the latest, supported docker version available

alvaroaleman · 2018-10-31T18:20:29Z

Doesn't help here, the problem is that we need a dynamically linked version of Docker in order for docker logs to work with the journald driver. So we basically must use packages from the repo

mrIncompetent · 2018-11-05T09:25:42Z

@alvaroaleman and me agreed on using json-file as log driver for Docker on all operating systems.

We'll use the /etc/docker/daemon.json for all operating systems to specify that.

mrIncompetent · 2018-11-05T11:31:40Z

@alvaroaleman PTAL

mrIncompetent · 2018-12-12T14:53:06Z

This works finally.
Went a non-pretty shortcut with that setup.sh script for CoreOS, but that did it :)

PTAL @alvaroaleman

alvaroaleman · 2018-12-12T16:25:22Z

pkg/userdata/ubuntu/userdata.go

 # Never set the hostname on AWS nodes. Kubernetes(kube-proxy) requires the hostname to be the private dns name
 {{ end }}

+groups:


What for do we need this?

I would like to stick as close as possible to "normal" setup of docker as possible.
Using a docker group is the common way of handling docker permission, no?

Oh wait, i just realized that we don't actually use that group anywhere

Removed with b4f71d6

We did use it on the socket ;)

Oh that explains a lot :D

alvaroaleman · 2018-12-12T16:36:37Z

pkg/userdata/centos/testdata/kubelet-v1.10-aws.golden

+    [Install]
+    WantedBy=multi-user.target
+
+- path: /etc/systemd/system/docker.socket


Is there any reason to have a docker.socket and a containerd.service aside from "CoreOS does it that way"? The docker package on Ubuntu manages perfectly fine without a containerd.service and the socket is only useful to have socket activation which we don't need because we already explicitly declare docker as dependency of the Kubelet

containerd.service:
Even though has a compiled in containerd, docker will look on the host for a /usr/bin/containerd first. If one is found, that binary will be started instead of the compiled in one.
On CoreOS /usr/bin/containerd exists - as it gets shipped with CoreOS, but does not work together with Docker 17.04. As Thus the way via the service is needed.

docker.socket
That is how CoreOS does it. I can remove it

docker.socket removed with ce9322d

containerd.service:
Even though has a compiled in containerd, docker will look on the host for a /usr/bin/containerd first. If one is found, that binary will be started instead of the compiled in one.

Yeah but that would mean that we need it on coreos only, not on the rest. I'd really like to prefer all of this as simple as possible and right now IMHO the templates are extremely hard to read already, so I'd prefer to not put anything in them which we dont absolutely need

So you prefer a special handling for CoreOS?
I'm not sure if that would improve readability, as we then would either have to introduce special templating inside the docker.service to support CoreOS and the rest or we would have a copy of the docker.service just for CoreOS.

I would prefer a unified handling of Docker, but as you prefer.

We can use a drop-in to add the additional dependency without having to alter the main unit file

Great idea, i'll adapt the pull!

…stems via daemon.json

mrIncompetent · 2018-12-13T10:53:48Z

@alvaroaleman PTAL

alvaroaleman · 2018-12-13T11:10:47Z

/lgtm
/approve

kubermatic-bot · 2018-12-13T11:10:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

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:

~~OWNERS~~ [alvaroaleman]

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

mrIncompetent · 2018-12-13T11:55:09Z

/test pull-machine-controller-e2e

alvaroaleman · 2018-12-13T13:21:44Z

/test pull-machine-controller-e2e

alvaroaleman · 2018-12-13T14:06:24Z

/test pull-machine-controller-e2e

alvaroaleman · 2018-12-13T14:43:32Z

/hold cancel

…n all systems via daemon.json (#382)" This reverts commit 3a51615. Under some circumstances this results in exceeding the AWS userdata limit. We will revisit the approach later.

…n all systems via daemon.json (#382)" (#437) This reverts commit 3a51615. Under some circumstances this results in exceeding the AWS userdata limit. We will revisit the approach later.

mrIncompetent requested a review from alvaroaleman October 30, 2018 08:39

kubermatic-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2018

alvaroaleman reviewed Oct 30, 2018

View reviewed changes

kubermatic-bot assigned alvaroaleman Oct 30, 2018

kubermatic-bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Oct 30, 2018

kubermatic-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 31, 2018

mrIncompetent force-pushed the ubuntu-limit-docker-log-size branch from 1e69786 to 2b10b67 Compare November 5, 2018 11:25

kubermatic-bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2018

mrIncompetent changed the title ~~Ubuntu limit docker log size~~ Ubuntu + CoreOS limit docker log size Nov 5, 2018

mrIncompetent force-pushed the ubuntu-limit-docker-log-size branch from 2b10b67 to 7d9da33 Compare November 6, 2018 12:33

kubermatic-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2018

mrIncompetent force-pushed the ubuntu-limit-docker-log-size branch from 5983c9c to 372b56d Compare December 7, 2018 09:47

kubermatic-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2018

mrIncompetent force-pushed the ubuntu-limit-docker-log-size branch from 899405a to 4c114d2 Compare December 12, 2018 14:18

alvaroaleman reviewed Dec 12, 2018

View reviewed changes

Install docker 17.03 via binary and configure logs rotation on all sy…

44979bd

…stems via daemon.json

mrIncompetent force-pushed the ubuntu-limit-docker-log-size branch from b4f71d6 to 44979bd Compare December 13, 2018 10:46

mrIncompetent changed the title ~~Ubuntu + CoreOS limit docker log size~~ Install docker 17.03 via binary and configure logs rotation on all systems via daemon.json Dec 13, 2018

kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2018

kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2018

kubermatic-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2018

kubermatic-bot merged commit 3a51615 into master Dec 13, 2018

alvaroaleman deleted the ubuntu-limit-docker-log-size branch December 13, 2018 14:43

alvaroaleman mentioned this pull request Dec 20, 2018

Revert "Install docker 17.03 via binary and configure logs rotation o… #437

Merged

Install docker 17.03 via binary and configure logs rotation on all systems via daemon.json #382

Install docker 17.03 via binary and configure logs rotation on all systems via daemon.json #382

Uh oh!

Conversation

mrIncompetent commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvaroaleman commented Oct 30, 2018

Uh oh!

mrIncompetent commented Oct 30, 2018

Uh oh!

alvaroaleman commented Oct 30, 2018

Uh oh!

alvaroaleman commented Oct 31, 2018

Uh oh!

mrIncompetent commented Oct 31, 2018

Uh oh!

alvaroaleman commented Oct 31, 2018

Uh oh!

mrIncompetent commented Oct 31, 2018

Uh oh!

alvaroaleman commented Oct 31, 2018

Uh oh!

mrIncompetent commented Nov 5, 2018

Uh oh!

mrIncompetent commented Nov 5, 2018

Uh oh!

mrIncompetent commented Dec 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrIncompetent Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrIncompetent commented Dec 13, 2018

Uh oh!

alvaroaleman commented Dec 13, 2018

Uh oh!

kubermatic-bot commented Dec 13, 2018

Uh oh!

mrIncompetent commented Dec 13, 2018

Uh oh!

alvaroaleman commented Dec 13, 2018

Uh oh!

alvaroaleman commented Dec 13, 2018

Uh oh!

alvaroaleman commented Dec 13, 2018

Uh oh!

Reviewers

mrIncompetent commented Oct 30, 2018 •

edited

Loading

mrIncompetent Dec 13, 2018 •

edited

Loading