Run kubelet outside container #2584

CecileRobertMichon · 2018-04-03T20:08:44Z

What this PR does / why we need it: In current (v0.14.6) acs-engine implementation, kubelet starts in a Docker container.
There are a lot of regressions of containerized kubelet reported in Kubernetes v1.10 (kubernetes/kubernetes#61456) . Since there are no full e2e tests for this, it may also happen in future releases (thanks @feiskyer for reporting). This PR moves kubelet to run outside the container, which is expected to be more stable.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

If applicable:

documentation
unit tests
tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

CecileRobertMichon · 2018-04-03T20:16:36Z

parts/k8s/kubernetesmastercustomdata.yml

    ExecStartPre=/bin/mv /tmp/kubectldir/hyperkube /opt/kubectl
-    ExecStart=/bin/chmod a+x /opt/kubectl
+    ExecStart=/bin/chmod a+x /opt/kubelet && /bin/chmod a+x /opt/kubectl


TODO: adapt kubelet start for coreos filepath

jackfrancis · 2018-04-03T23:11:00Z

parts/k8s/kubernetesagentcustomdata.yml

    ExecStartPre=/bin/mv /tmp/kubectldir/hyperkube /usr/local/bin/kubectl
-    ExecStart=/bin/chmod a+x /usr/local/bin/kubectl
+    ExecStart=/bin/chmod a+x /usr/local/bin/kubelet && /bin/chmod a+x /usr/local/bin/kubectl


We can shorten this to /bin/chmod a+x /usr/local/bin/kubelet /usr/local/bin/kubectl (the build of chmod on ubuntu hosts accepts a list of file targets to apply the mode changes against). Verified:

azureuser@k8s-master-18440544-0:/tmp$ ls -la test* -rw-rw-r-- 1 azureuser azureuser 0 Apr 3 23:08 test1 -rw-rw-r-- 1 azureuser azureuser 0 Apr 3 23:08 test2 azureuser@k8s-master-18440544-0:/tmp$ /bin/chmod a+x test1 test2 azureuser@k8s-master-18440544-0:/tmp$ ls -la test* -rwxrwxr-x 1 azureuser azureuser 0 Apr 3 23:08 test1 -rwxrwxr-x 1 azureuser azureuser 0 Apr 3 23:08 test2

Yes good catch, thanks

jackfrancis · 2018-04-03T23:11:36Z

parts/k8s/kubernetesagentcustomdata.yml

    ExecStartPre=/bin/mv /tmp/kubectldir/hyperkube /opt/kubectl
-    ExecStart=/bin/chmod a+x /opt/kubectl
+    ExecStart=/bin/chmod a+x /opt/kubelet && /bin/chmod a+x /opt/kubectl


See below. Didn't verify on a coreos host, but I'm confident this is a standard pattern.

jackfrancis · 2018-04-03T23:12:20Z

parts/k8s/kubernetesmastercustomdata.yml

    ExecStartPre=/bin/mv /tmp/kubectldir/hyperkube /opt/kubectl
-    ExecStart=/bin/chmod a+x /opt/kubectl
+    ExecStart=/bin/chmod a+x /opt/kubelet && /bin/chmod a+x /opt/kubectl


ditto above about chmod command

jackfrancis · 2018-04-03T23:12:27Z

parts/k8s/kubernetesmastercustomdata.yml

    ExecStartPre=/bin/mv /tmp/kubectldir/hyperkube /usr/local/bin/kubectl
-    ExecStart=/bin/chmod a+x /usr/local/bin/kubectl
+    ExecStart=/bin/chmod a+x /usr/local/bin/kubelet && /bin/chmod a+x /usr/local/bin/kubectl


jackfrancis

In the systemd extract definition, I think we want a ConditionPathExists=!/usr/local/bin/kubelet as well. (and the CoreOS flavor also)

jackfrancis

Let's change the directory name kubectldir (under /tmp on the host OS and /opt inside the container) to something more general, like hyperkubedir or something.

jackfrancis · 2018-04-03T23:34:41Z

parts/k8s/kubernetesmastercustomdata.yml

 {{else}}
+    ExecStartPre=/bin/cp /tmp/kubectldir/hyperkube /usr/local/bin/kubelet


Let's do this in one line to make it a little clearer that we are copying the same file to two different places. One example:

for dest in /usr/local/bin/kubelet /usr/local/bin/kubectl ; do cp /tmp/kubectldir/hyperkube $dest; done

And then just add a cleanup line after, like:

rm -Rf /tmp/kubectldir

(but replace kubectldir with the new, general name in both instances :) )

I don't really find that

ExecStartPre=for dest in /usr/local/bin/kubelet /usr/local/bin/kubectl ; do /bin/cp /tmp/hyperkube/hyperkube $dest; done ExecStartPre=rm -Rf /tmp/kubectldir

is much more readable than

ExecStartPre=/bin/cp /tmp/hyperkubedir/hyperkube /usr/local/bin/kubelet ExecStartPre=/bin/mv /tmp/hyperkubedir/hyperkube /usr/local/bin/kubectl

How about:

ExecStartPre=/bin/mv /tmp/hyperkubedir/hyperkube /usr/local/bin/kubelet ExecStartPre=/bin/cp /usr/local/bin/kubelet /usr/local/bin/kubectl

?

I mean I see where you're coming from in terms of logic, I just find that it makes the syntax more complex to have a for loop for only two files. Please feel free to disagree with me :)

CecileRobertMichon · 2018-04-04T00:23:09Z

@jackfrancis re: In the systemd extract definition, I think we want a ConditionPathExists=!/usr/local/bin/kubelet as well. (and the CoreOS flavor also) : I thought about it when I was writing the code but I didn't add it because we want the condition to be: run if either file is missing. A ConditionPathExists will prevent the extract service from running if any of the two files is present (&&, not ||) since both conditions will need to be satisfied. But what happens if the first file gets copied but the second one doesn't (unlikely, but software is unpredictable)? we still want to run the extraction again (read retry) in that case, so I left the condition as !/usr/local/bin/kubectl since /usr/local/bin/kubectl gets created last. Thoughts?

feiskyer · 2018-04-04T01:35:35Z

parts/k8s/kubernetesagentcustomdata.yml

    ExecStartPre=/usr/bin/docker pull {{WrapAsVariable "kubernetesHyperkubeSpec"}}
-    ExecStartPre=/usr/bin/docker run --rm -v /tmp/kubectldir:/opt/kubectldir {{WrapAsVariable "kubernetesHyperkubeSpec"}} /bin/bash -c "cp /hyperkube /opt/kubectldir/"
+    ExecStartPre=/usr/bin/docker run --rm -v /tmp/hyperkubedir:/opt/hyperkubedir {{WrapAsVariable "kubernetesHyperkubeSpec"}} /bin/bash -c "cp /hyperkube /opt/hyperkubedir/"


Besides the extraction, dependencies should also be installed. e.g. on ubuntu, kubelet depends on iptables (>= 1.4.21), kubernetes-cni (= 0.6.0), iproute2, socat, util-linux, mount, ebtables, ethtool, init-system-helpers (>= 1.18~)

Thanks @feiskyer. Just checked and all of these are already being installed except for socat. I will add a step to install them if they are missing so we are safe. I can't find a package kubernetes-cni however, do you have more info on that one?

For the cni plugin, I think we are already installing it at

acs-engine/parts/k8s/kubernetesmastercustomscript.sh

Line 217 in b463cd0

retrycmd_get_tarball 60 1 $CONTAINERNETWORKING_CNI_TGZ_TMP ${CNI_PLUGINS_URL}

CecileRobertMichon · 2018-04-04T18:34:30Z

parts/k8s/artifacts/1.5/kuberneteskubelet.service

@@ -1,45 +0,0 @@
-[Unit]


Removed the 1.5 version since k8s 1.5 was deprecated in #2394 and it's not in use anymore

CecileRobertMichon · 2018-04-04T18:46:15Z

parts/k8s/kubernetesagentcustomdata.yml

-
+    . /opt/azure/containers/provision_source.sh
+    apt_get_update
+    retrycmd_if_failure 20 10 120 apt-get install -y iptables iproute2 socat util-linux mount ebtables ethtool init-system-helpers


Ensure dependencies are installed on agents

CecileRobertMichon · 2018-04-04T18:46:26Z

parts/k8s/kubernetesmastercustomdata.yml

@@ -177,6 +179,9 @@ MASTER_ARTIFACTS_CONFIG_PLACEHOLDER
  content: |
    #!/bin/bash
    set -e
+    . /opt/azure/containers/provision_source.sh
+    apt_get_update
+    retrycmd_if_failure 20 10 120 apt-get install -y iptables iproute2 socat util-linux mount ebtables ethtool init-system-helpers


Ensure dependencies are installed on masters

jackfrancis · 2018-04-04T22:18:19Z

parts/k8s/kubernetesagentcustomdata.yml

-
+    source /opt/azure/containers/provision_source.sh
+    apt_get_update
+    retrycmd_if_failure 20 10 120 apt-get install -y iptables iproute2 socat util-linux mount ebtables ethtool init-system-helpers


Are we confident that all of these packages will reliably install in under 2 mins? (speaking to the 120 second timeout)

Yes most of them should already be there so it's very quick. In my tests it took under 10 seconds. I can increase the timeout if we want to be sure.

jackfrancis · 2018-04-04T22:22:46Z

parts/k8s/kubernetesprovisionsource.sh

+retrycmd_get_tarball() { retries=$1; wait=$2; tarball=$3; url=$4; for i in $(seq 1 $retries); do tar -tzf $tarball; [ $? -eq 0  ] && break || retrycmd_if_failure_no_stats $retries 1 10 curl -fsSL $url -o $tarball; sleep $wait; done; }
+ensure_etcd_ready() { for i in $(seq 1 1800); do if [ -e /opt/azure/containers/certs.ready ]; then break; fi; sleep 1; done }


Oh yeah I just moved it out because I moved out apt_get_update so I thought might as well but we should generalize it in another PR

I increased it to 5 minutes... Should be plenty enough

jackfrancis

lgtm if we feel good about the 2 minute timeout for apt-get install for kubelet deps

jackfrancis

lgtm

ghost assigned CecileRobertMichon Apr 3, 2018

ghost added the in progress label Apr 3, 2018

CecileRobertMichon commented Apr 3, 2018

View reviewed changes

CecileRobertMichon requested a review from lachie83 April 3, 2018 20:16

jackfrancis reviewed Apr 3, 2018

View reviewed changes

feiskyer reviewed Apr 4, 2018

View reviewed changes

CecileRobertMichon commented Apr 4, 2018

View reviewed changes

jackfrancis reviewed Apr 4, 2018

View reviewed changes

jackfrancis previously approved these changes Apr 4, 2018

View reviewed changes

CecileRobertMichon dismissed jackfrancis’s stale review via 54c97e5 April 4, 2018 22:44

CecileRobertMichon added 10 commits April 4, 2018 16:24

Run kubelet outside container

40e432e

restore kubelet 1.5 flags

4600ae0

hyperkubedir and chmod in one line

c413f4d

remove 1.5

b90fffc

install kubelet dependencies

57eaf25

fix syntax

b69bb99

move set -e

f3de172

remove kubelet DOCKER_OPTS

5e3e80b

unblock circleci

1fab66b

increase timeout

9826c81

jackfrancis force-pushed the kubelet-out branch from 54c97e5 to 9826c81 Compare April 4, 2018 23:25

ghost assigned jackfrancis Apr 4, 2018

jackfrancis approved these changes Apr 4, 2018

View reviewed changes

jackfrancis merged commit ed0944f into Azure:master Apr 5, 2018

ghost removed the in progress label Apr 5, 2018

CecileRobertMichon mentioned this pull request Apr 6, 2018

Fix kubenet plugins #2610

Merged

3 tasks

karataliu mentioned this pull request May 21, 2018

Add mount.nfs when kubelet running outside container #3022

Closed

CecileRobertMichon deleted the kubelet-out branch June 6, 2018 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run kubelet outside container #2584

Run kubelet outside container #2584

CecileRobertMichon commented Apr 3, 2018 •

edited by jackfrancis

Loading

CecileRobertMichon Apr 3, 2018

jackfrancis Apr 3, 2018

CecileRobertMichon Apr 4, 2018

jackfrancis Apr 3, 2018

jackfrancis Apr 3, 2018

jackfrancis Apr 3, 2018

jackfrancis left a comment

jackfrancis left a comment

jackfrancis Apr 3, 2018

CecileRobertMichon Apr 4, 2018

CecileRobertMichon Apr 4, 2018

CecileRobertMichon commented Apr 4, 2018

feiskyer Apr 4, 2018

CecileRobertMichon Apr 4, 2018

CecileRobertMichon Apr 4, 2018

CecileRobertMichon Apr 4, 2018

CecileRobertMichon Apr 4, 2018

CecileRobertMichon Apr 4, 2018

jackfrancis Apr 4, 2018

CecileRobertMichon Apr 4, 2018

jackfrancis Apr 4, 2018

CecileRobertMichon Apr 4, 2018

CecileRobertMichon Apr 4, 2018

jackfrancis left a comment

jackfrancis left a comment

		{{else}}
		ExecStartPre=/bin/cp /tmp/kubectldir/hyperkube /usr/local/bin/kubelet

		retrycmd_get_tarball() { retries=$1; wait=$2; tarball=$3; url=$4; for i in $(seq 1 $retries); do tar -tzf $tarball; [ $? -eq 0 ] && break \|\| retrycmd_if_failure_no_stats $retries 1 10 curl -fsSL $url -o $tarball; sleep $wait; done; }
		ensure_etcd_ready() { for i in $(seq 1 1800); do if [ -e /opt/azure/containers/certs.ready ]; then break; fi; sleep 1; done }

Run kubelet outside container #2584

Run kubelet outside container #2584

Conversation

CecileRobertMichon commented Apr 3, 2018 • edited by jackfrancis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis left a comment

Choose a reason for hiding this comment

jackfrancis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis left a comment

Choose a reason for hiding this comment

jackfrancis left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented Apr 3, 2018 •

edited by jackfrancis

Loading