gci: decouple from the built-in kubelet version #31367

Amey-D · 2016-08-24T19:58:37Z

Prior to this change, configure.sh would:
(1) compare versions of built-in kubelet and downloaded kubelet, and
(2) bind-mount downloaded kubelet at /usr/bin/kubelet in case of
version mismatch

With this change, configure.sh:
(1) compares the two versions only on test clusters, and
(2) uses the actual file paths to start kubelet w/o any bind-mounting

To allow (2), this change also provides its own version of kubelet
systemd service file.

Effectively with this change we will always use the downloaded kubelet
binary along with its own systemd service file on non-test clusters. The
main advantage is this change does not rely on the kubelet being built in to
the OS image.

gci: decouple from the built-in kubelet version

@dchen1107 @wonderfly can you please review

cc/ @kubernetes/goog-image FYI

This change is

dchen1107 · 2016-08-24T20:44:38Z

#31376

wonderfly · 2016-08-24T22:05:08Z

(1) compare the two versions (same as before), and
Curious, why still the check the builtin version? Isn't it more consistent to just use the downloaded version, and it's systemd config, no matter what? My concern is that as we go further this way, the builtin systemd config could diverge with upstream and trying to use it may cause unexpected problems.

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

cluster/gce/gci/configure-helper.sh, line 535 [r1] (raw file):

Write the systemd service file for kubelet.

cat </etc/systemd/system/kubelet.service
[Unit]
Description=Kubernetes kubelet
Requires=network-online.target
After=network-online.target

[Service]
Restart=always
RestartSec=10
ExecStart=${kubelet_bin} ${flags}

[Install]
WantedBy=multi-user.target
EOF

Many packages come with their own systemd service file as part of their distributions. Is it possible to have kubernetes do so too so that this doesn't need to be hard coded? Also, that will also make the service file versioned, which is good.

If not, can it at least be in one of the yaml configs?

cluster/gce/gci/configure-helper.sh, line 1099 [r1] (raw file):

"${KUBE_HOME}"
In your current logic, you compare the builtin version with the download one. If the same, you would use the builtin version, right? Then do you copy it from /usr/bin to $KUBE_HOME/bin?

cluster/gce/gci/configure.sh, line 174 [r1] (raw file):

"${kube_bin}
Is $KUBE_HOME in $PATH? Or is kubelet/kubectl always started with full path?

Comments from Reviewable

adityakali · 2016-08-24T22:23:34Z

cluster/gce/gci/configure-helper.sh

+[Service]
+Restart=always
+RestartSec=10
+ExecStart=${kubelet_bin} ${flags}


Can we continue use the /etc/default/kubelet for flags? What we currently have is:

[Service] Restart=always RestartSec=10 EnvironmentFile=/etc/default/kubelet ExecStartPre=/bin/mkdir -p /etc/kubernetes/manifests ExecStart=${kubelet_bin} $KUBELET_OPTS

Additionally, if you want, you can even do something like:

ExecStart=$KUBELET_BIN $KUBELET_OPTS

and set KUBELET_BIN also in /etc/default/kubelet. This can allow you to keep the kubelet.service file completely static.

Amey-D · 2016-08-24T22:41:09Z

I added an additional check for TEST_CLUSTER, that is the version comparison happens only if TEST_CLUSTER is 'true'. On non-test clusters we will always use the downloaded kubelet and systemd service file.

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.

cluster/gce/gci/configure-helper.sh, line 531 [r1] (raw file):

Previously, adityakali (Aditya Kali) wrote…

Can we continue use the /etc/default/kubelet for flags? What we currently have is:
[Service]

Restart=always

RestartSec=10

EnvironmentFile=/etc/default/kubelet

ExecStartPre=/bin/mkdir -p /etc/kubernetes/manifests

ExecStart=${kubelet_bin} $KUBELET_OPTS
Additionally, if you want, you can even do something like:
ExecStart=$KUBELET_BIN $KUBELET_OPTS
and set KUBELET_BIN also in /etc/default/kubelet. This can allow you to keep the kubelet.service file completely static.

If we specify the full systemd service file as part of configure-helper.sh, then I do not see any benefits of using /etc/default/kubelet. The /etc/default/kubelet would be useful if we put the service config in the yaml files, but in that case it would have to be duplicated in both master and node yaml. LMK if you have a strong preference to moving the service config to yaml files.

cluster/gce/gci/configure-helper.sh, line 535 [r1] (raw file):

Previously, wonderfly (Daniel Wang) wrote…

Write the systemd service file for kubelet.

cat </etc/systemd/system/kubelet.service
[Unit]
Description=Kubernetes kubelet
Requires=network-online.target
After=network-online.target

[Service]
Restart=always
RestartSec=10
ExecStart=${kubelet_bin} ${flags}

[Install]
WantedBy=multi-user.target
EOF

Many packages come with their own systemd service file as part of their distributions. Is it possible to have kubernetes do so too so that this doesn't need to be hard coded? Also, that will also make the service file versioned, which is good.

If not, can it at least be in one of the yaml configs?

It could be in the yaml file, but then it would have to be in both master and node yaml files and populating the binary file path and commandline args would be more work.

cluster/gce/gci/configure-helper.sh, line 1099 [r1] (raw file):

Previously, wonderfly (Daniel Wang) wrote…

"${KUBE_HOME}"
In your current logic, you compare the builtin version with the download one. If the same, you would use the builtin version, right? Then do you copy it from /usr/bin to $KUBE_HOME/bin?

If the versions match, I use the path /usr/bin/kubelet in the systemd service file. No copying is involved.

cluster/gce/gci/configure.sh, line 174 [r1] (raw file):

Previously, wonderfly (Daniel Wang) wrote…

"${kube_bin}
Is $KUBE_HOME in $PATH? Or is kubelet/kubectl always started with full path?

kubelet is started with full path through systemd service. kubectl is not run automatically -- users can run it manually if they wa

Comments from Reviewable

adityakali · 2016-08-24T23:26:56Z

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

cluster/gce/gci/configure-helper.sh, line 531 [r1] (raw file):

Previously, dchen1107 (Dawn Chen) wrote…

+1

It doesn't have to be in yaml either. Just package it as-is into the release tar.gz as we do other manifest files (add an entry at https://github.com/kubernetes/kubernetes/blob/master/build/common.sh#L955).

Having the flags read from /etc/default/kubelet means that when debugging its easy to figure out what settings are in use and to changes. Otherwise even small changes like increasing log verbosity will require users (or system admins) to edit the systemd service file, do a systemd daemon reload, before restarting the service.

This is just for convenience though. So not strong preference.

cluster/gce/gci/configure-helper.sh, line 535 [r1] (raw file):

Comments from Reviewable

wonderfly · 2016-08-24T23:29:56Z

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.

cluster/gce/gci/configure-helper.sh, line 531 [r1] (raw file):

Previously, adityakali (Aditya Kali) wrote…

It doesn't have to be in yaml either. Just package it as-is into the release tar.gz as we do other manifest files (add an entry at https://github.com/kubernetes/kubernetes/blob/master/build/common.sh#L955).

Having the flags read from /etc/default/kubelet means that when debugging its easy to figure out what settings are in use and to changes. Otherwise even small changes like increasing log verbosity will require users (or system admins) to edit the systemd service file, do a systemd daemon reload, before restarting the service.

This is just for convenience though. So not strong preference.

nit: Can you all get a GitHub profile photo? Hard to tell which is who. :)

Comments from Reviewable

adityakali · 2016-08-24T23:44:43Z

cluster/gce/gci/configure.sh

-  else
-    rm -f "${kube_bin}/kubelet"
-  fi
+  mv "${src_dir}/kubelet" "${kube_bin}"


So if the instance reboots, I think we end up re-doing the entire download and untar thing right?
It might be worth testing if this continues to work with even after a reboot.

Amey-D · 2016-08-24T23:59:34Z

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.

cluster/gce/gci/configure-helper.sh, line 531 [r1] (raw file):

Previously, wonderfly (Daniel Wang) wrote…

nit: Can you all get a GitHub profile photo? Hard to tell which is who. :)

I'll keep the service file configuration how it is right now.

As for the photos I consider it beyond the scope of this CL :)

cluster/gce/gci/configure-helper.sh, line 1101 [r2] (raw file):

Previously, adityakali (Aditya Kali) wrote…

Does it make sense to also log the kubelet install location here? (i.e., whether its "${KUBE_HOME}"/bin/kubelet or /usr/bin/kubelet).

(again, only to avoid confusion).

Done.

cluster/gce/gci/configure.sh, line 131 [r2] (raw file):

Previously, adityakali (Aditya Kali) wrote…

So if the instance reboots, I think we end up re-doing the entire download and untar thing right?

It might be worth testing if this continues to work with even after a reboot.

Done. Verified the installation and configuration services finish successfully upon reboot on master and nodes. Besides I expect the e2e tests to test reboots.

Comments from Reviewable

Amey-D · 2016-08-25T01:03:58Z

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.

cluster/gce/gci/configure-helper.sh, line 531 [r1] (raw file):

Previously, Amey-D (Amey Deshpande) wrote…

I'll keep the service file configuration how it is right now.

As for the photos I consider it beyond the scope of this CL :)

Added /etc/default/kubelet/service for KUBELET_OPTS. Turned out systemd does not allows reading the binary file name from the environment file.

Comments from Reviewable

Amey-D · 2016-08-25T01:05:29Z

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.

cluster/gce/gci/configure-helper.sh, line 531 [r1] (raw file):

Previously, Amey-D (Amey Deshpande) wrote…

Added /etc/default/kubelet/service for KUBELET_OPTS. Turned out systemd does not allows reading the binary file name from the environment file.

Done.

Comments from Reviewable

wonderfly · 2016-08-25T01:28:08Z

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from Reviewable

wonderfly · 2016-08-25T01:28:50Z

LGTM but @adityakali should take a further look...

Prior to this change, configure.sh would: (1) compare versions of built-in kubelet and downloaded kubelet, and (2) bind-mount downloaded kubelet at /usr/bin/kubelet in case of version mismatch With this change, configure.sh: (1) compares the two versions only on test clusters, and (2) uses the actual file paths to start kubelet w/o any bind-mounting To allow (2), this change also provides its own version of kubelet systemd service file. Effectively with this change we will always use the downloaded kubelet binary along with its own systemd service file on non-test clusters. The main advantage is this change does not rely on the kubelet being built in to the OS image.

k8s-bot · 2016-08-25T07:21:13Z

GCE e2e build/test passed for commit 2939ebd.

adityakali · 2016-08-25T16:30:56Z

LGTM
Thanks for the changes Amey!

dchen1107 · 2016-08-25T21:19:28Z

LGTM

k8s-github-robot · 2016-08-25T21:58:29Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-08-25T22:43:18Z

GCE e2e build/test passed for commit 2939ebd.

k8s-github-robot · 2016-08-25T22:55:14Z

Automatic merge from submit-queue

k8s-cherrypick-bot · 2016-10-24T20:52:57Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

…-upstream-release-1.3 Automated cherry pick of #31367

…-of-#31367-upstream-release-1.3 Automated cherry pick of kubernetes#31367

googlebot added the cla: yes label Aug 24, 2016

dchen1107 self-assigned this Aug 24, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Aug 24, 2016

dchen1107 added release-note-none Denotes a PR that doesn't merit a release note. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed release-note-label-needed labels Aug 24, 2016

dchen1107 added this to the v1.4 milestone Aug 24, 2016

adityakali reviewed Aug 24, 2016
View reviewed changes

Amey-D force-pushed the gci-kubelet branch from 56c1ac6 to 9fb10f6 Compare August 24, 2016 22:33

dchen1107 assigned adityakali and wonderfly Aug 24, 2016

adityakali reviewed Aug 24, 2016
View reviewed changes

Amey-D force-pushed the gci-kubelet branch from 9fb10f6 to f24bb11 Compare August 24, 2016 23:58

Amey-D force-pushed the gci-kubelet branch from f24bb11 to c4cd12b Compare August 25, 2016 01:02

Amey-D force-pushed the gci-kubelet branch from c4cd12b to 2939ebd Compare August 25, 2016 06:36

dchen1107 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2016

k8s-github-robot merged commit f090fd1 into kubernetes:master Aug 25, 2016

k8s-github-robot mentioned this pull request Aug 25, 2016

[k8s.io] Kubectl client [k8s.io] Kubectl run default should create an rc or deployment from an image [Conformance] {Kubernetes e2e suite} #27834

Closed

dchen1107 mentioned this pull request Aug 26, 2016

GCI: Decouple image building from Kubernetes release process #31376

Closed

dchen1107 mentioned this pull request Sep 9, 2016

Master is not working after downgrade #32337

Closed

Amey-D mentioned this pull request Sep 13, 2016

upgrading master doesn't upgrade kubectl #32538

Closed

maisem mentioned this pull request Oct 24, 2016

Automated cherry pick of #31367 #35455

Merged

maisem added the cherrypick-candidate label Oct 24, 2016

jessfraz removed this from the v1.4 milestone Oct 24, 2016

k8s-cherrypick-bot removed the cherrypick-candidate label Oct 24, 2016

fabioy added a commit that referenced this pull request Oct 31, 2016

Merge pull request #35455 from maisem/automated-cherry-pick-of-#31367…

2601926

…-upstream-release-1.3 Automated cherry pick of #31367

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016

Merge pull request kubernetes#35455 from maisem/automated-cherry-pick…

83391a5

…-of-#31367-upstream-release-1.3 Automated cherry pick of kubernetes#31367

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gci: decouple from the built-in kubelet version #31367

gci: decouple from the built-in kubelet version #31367

Amey-D commented Aug 24, 2016 •

edited by jessfraz

Loading

dchen1107 commented Aug 24, 2016

wonderfly commented Aug 24, 2016

Write the systemd service file for kubelet.

adityakali Aug 24, 2016

dchen1107 Aug 24, 2016

Amey-D commented Aug 24, 2016

Write the systemd service file for kubelet.

adityakali commented Aug 24, 2016

wonderfly commented Aug 24, 2016

adityakali Aug 24, 2016

Amey-D commented Aug 24, 2016

Amey-D commented Aug 25, 2016

Amey-D commented Aug 25, 2016

wonderfly commented Aug 25, 2016

wonderfly commented Aug 25, 2016

k8s-bot commented Aug 25, 2016

adityakali commented Aug 25, 2016

dchen1107 commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

k8s-bot commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

k8s-cherrypick-bot commented Oct 24, 2016

gci: decouple from the built-in kubelet version #31367

gci: decouple from the built-in kubelet version #31367

Conversation

Amey-D commented Aug 24, 2016 • edited by jessfraz Loading

dchen1107 commented Aug 24, 2016

wonderfly commented Aug 24, 2016

Write the systemd service file for kubelet.

adityakali Aug 24, 2016

Choose a reason for hiding this comment

dchen1107 Aug 24, 2016

Choose a reason for hiding this comment

Amey-D commented Aug 24, 2016

Write the systemd service file for kubelet.

adityakali commented Aug 24, 2016

wonderfly commented Aug 24, 2016

adityakali Aug 24, 2016

Choose a reason for hiding this comment

Amey-D commented Aug 24, 2016

Amey-D commented Aug 25, 2016

Amey-D commented Aug 25, 2016

wonderfly commented Aug 25, 2016

wonderfly commented Aug 25, 2016

k8s-bot commented Aug 25, 2016

adityakali commented Aug 25, 2016

dchen1107 commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

k8s-bot commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

k8s-cherrypick-bot commented Oct 24, 2016

Amey-D commented Aug 24, 2016 •

edited by jessfraz

Loading