Coreos kube-up now with less cloud init #33965

euank · 2016-10-03T19:58:06Z

This update includes significant refactoring. It moves almost all of the
logic into bash scripts, modeled after the gci cluster scripts.

The reason to do this is:

Avoid duplicating the saltbase manifests by reusing gci's parsing logic (easier maintenance)
Take an incremental step towards sharing more code between gci/trusty/coreos, again for better maintenance
Pave the way for making future changes (e.g. improved rkt support, kubelet support) easier to share

The primary differences from the gci scripts are the following:

Use of the /opt/kubernetes directory over /home/kubernetes
Support for rkt as a runtime
No use of logrotate
No use of /etc/default/
No logic related to noexec mounts or gci-specific firewall-stuff

It will make sense to move 2 over to gci, as well as perhaps a few other small improvements. That will be a separate PR for ease of review.

Ref #29720, this is a part of that because it removes a copy of them.

Fixes #24165

cc @yifan-gu

Since this logic largely duplicates logic from the gci folder, it would be nice if someone closely familiar with that gave an OK or made sure I didn't fall into any gotchas related to that, so cc @andyzheng0831

This change is

yifan-gu · 2016-10-04T01:12:11Z

cluster/gce/coreos/README.md

@@ -0,0 +1,8 @@
+# CoreOS image
+
+The [CoreOS operating oystem](https://coreos.com/why/) is a Linux distribution optimized for running containers securely at scale.


s/oystem/system

yifan-gu · 2016-10-04T01:36:34Z

@euank Looks like we don't need cluster/gce/coreos/configure-kubelet.sh as well.

k8s-ci-robot · 2016-10-04T01:39:28Z

Jenkins unit/integration failed for commit 082275dc4fdfc9ef19f83335ef29007df10ff304. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

yifan-gu · 2016-10-04T01:44:58Z

cluster/gce/coreos/configure-helper.sh

+function setup-kubelet-dir {
+    echo "Making /var/lib/kubelet executable for kubelet"
+    mount --bind /var/lib/kubelet /var/lib/kubelet/
+    mount -B -o remount,exec,suid,dev /var/lib/kubelet    


Remove some tailing spaces?

yifan-gu · 2016-10-04T01:49:00Z

cluster/gce/coreos/configure.sh

@@ -0,0 +1,173 @@
+#!/bin/bash
+
+# Copyright 2015 The Kubernetes Authors.


yifan-gu · 2016-10-04T01:51:31Z

cluster/gce/coreos/configure.sh

+    -o "${tmp_kube_env}" \
+    http://metadata.google.internal/computeMetadata/v1/instance/attributes/kube-env
+  # Convert the yaml format file into a shell-style file.
+  sed 's/: /=/' < "${tmp_kube_env}" > "${KUBE_DIR}/kube-env"


@euank Have you verified that this will always work for today's kube env yamls?

It does work as best as I can tell based on sourcing the file and printenv and based on skimming it.

Any specific concerns?

@euank Thank you.

yifan-gu · 2016-10-04T01:56:14Z

LGTM other than the comments above. Thanks for the work!!! @euank

euank · 2016-10-04T18:52:27Z

As a hint for reviewers, doing a (vim)diff between the gci and the coreos versions of configure.sh and configure-helper.sh is incredibly useful for review.

k8s-ci-robot · 2016-10-11T00:20:58Z

Jenkins GCE e2e failed for commit aca0aa29541b024c0eb436c6cdab6c82957c22c5. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-11T00:30:41Z

Jenkins GKE smoke e2e failed for commit aca0aa29541b024c0eb436c6cdab6c82957c22c5. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-11T00:35:36Z

Jenkins GCI GKE smoke e2e failed for commit aca0aa29541b024c0eb436c6cdab6c82957c22c5. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-11T03:34:43Z

Jenkins Kubemark GCE e2e failed for commit 6bb8c4928668118446f048f5a6516ff0dbe08e10. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

googlebot · 2016-10-14T22:40:36Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

roberthbailey · 2016-12-16T18:34:15Z

@euank

Sorry for the long delay on the code review. I only two concerns:

It seems you've duplicated some code between shell scripts. They are likely to diverge in the future and it would be nice to consolidate them into one location.
Can you verify that coreos/gci are mostly copies of each other now rather than intimately sharing the node startup scripts (as appears to be the case from https://github.com/euank/kubernetes/tree/3536ecc6ff7f043d5a104b92b803881e5f7e33af/cluster/gce/coreos)? I'm concerned that we'll make a change to gci that will break the coreos startup scripts and won't notice as we don't have continuous testing of them.

bgrant0607 · 2016-12-16T18:41:46Z

@grodrigues3 Is there a PR that adds an OWNERS file for cluster/gce/coreos? Could we break it out and get it in earlier than the rest?

roberthbailey · 2016-12-16T18:45:47Z

@bgrant0607 -- juju and vagrant are the only directories under cluster that have an OWNERS file.

euank · 2016-12-16T18:49:19Z

@roberthbailey Per the original comment, number 2. This PR is "an incremental step towards sharing more code between gci/trusty/coreos, again for better maintenance"

I want to do that in a followup PR because that change will also be intrusive to GCI and would best be reviewed separately.

I don't believe there is yet any shared code between the gci/coreos folders yet (though I want them to in the future as above).
I don't think any change in the gci folder will impact the coreos folder at this time.

The new thing that could break this is the shared saltbase manifests (which all of gci/coreos/trusty share with this) since there's not much testing there, but there wasn't testing before and on at least one occasion the non-shared coreos-dedicated manifests were broken regardless.

roberthbailey · 2016-12-16T18:51:06Z

@euank - thanks. That answered by second question. My only remaining question is about the duplication of functions in shell.

roberthbailey · 2016-12-16T18:24:50Z

build/common.sh

+# such as GCI and Ubuntu Trusty. We directly copy manifests from
+# cluster/addons and cluster/saltbase/salt. The script of cluster initialization
+# will remove the salt configuration and evaluate the variables in the manifests.
+function kube::release::package_kube_manifests_tarball() {


Isn't this redundant with the function of the same name in build/lib/release.sh?

The was a rebase error. After fixing it, it's a 3-line diff in the original function

I'll update the PR after verifying things still work..

Thanks for catching it!

roberthbailey · 2016-12-16T18:30:18Z

cluster/gce/coreos/configure.sh

 set -o errexit
 set -o nounset
 set -o pipefail

-function set-broken-motd {


FYI We've found that setting the motd is really useful for debugging node startup failure.

bgrant0607 · 2016-12-16T20:11:56Z

@roberthbailey I was suggesting that we create a new OWNERS file

roberthbailey · 2016-12-16T20:23:30Z

Yup. I asked @euank to send a quick PR to add one.

euank · 2016-12-16T22:07:55Z

@roberthbailey I addressed your comment and rebased it back on master again to pick up any missing gci changes.

I validated that it works for me with my kube-up environment of:

export KUBE_SSH_USER=core
export KUBE_OS_DISTRIBUTION=coreos

export KUBE_GCE_MASTER_PROJECT=coreos-cloud
export KUBE_GCE_MASTER_IMAGE=$(gcloud compute images list --project coreos-cloud --filter  coreos-alpha --format=json | jq '.[0].name' -r)
export KUBE_GCE_NODE_PROJECT=coreos-cloud
export KUBE_GCE_NODE_IMAGE=${KUBE_GCE_MASTER_IMAGE}

I'll send an owners pr pronto as well including myself, @yifan-gu, and @ethernetdan as owners.

@yifan-gu

Automatic merge from submit-queue cluster/gce/coreos: add OWNERS See #33965 for context. The code in `cluster/gce/coreos` has mostly been written/maintained by @yifan-gu and myself thusfar, so I added our names to the owner list. @ethernetdan has also volunteered as well (thanks!). **Release note**: ```release-note NONE ``` cc @roberthbailey

roberthbailey · 2016-12-17T08:43:31Z

@euank looks like it needs another rebase.

This is for reviewing ease as the following commits introduce changes to make the coreos kube-up deployment share significant code with the gci code.

This update includes significant refactoring. It moves almost all of the logic into bash scripts, modeled after the `gci` cluster scripts. The primary differences between the two are the following: 1. Use of the `/opt/kubernetes` directory over `/home/kubernetes` 2. Support for rkt as a runtime 3. No use of logrotate 4. No use of `/etc/default/` 5. No logic related to noexec mounts or gci-specific firewall-stuff

We don't use this bit of gci currently.

euank · 2016-12-19T19:30:10Z

Rebased ..

roberthbailey · 2016-12-20T08:24:25Z

/lgtm

k8s-github-robot · 2016-12-20T08:36:33Z

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

k8s-github-robot · 2016-12-20T09:13:45Z

Automatic merge from submit-queue

googlebot added the cla: yes label Oct 3, 2016

k8s-github-robot assigned jbeda Oct 3, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Oct 3, 2016

yifan-gu assigned yifan-gu and dchen1107 and unassigned jbeda Oct 3, 2016

yifan-gu added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 3, 2016

euank force-pushed the coreos-kube-up-now-with-less-cloud-init branch from e3e2f25 to 3653a37 Compare October 3, 2016 20:36

yifan-gu reviewed Oct 4, 2016

View reviewed changes

cluster/gce/coreos/configure.sh

@@ -0,0 +1,173 @@

#!/bin/bash

# Copyright 2015 The Kubernetes Authors.

Copy link

Contributor

yifan-gu Oct 4, 2016

Choose a reason for hiding this comment

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

2016

yifan-gu reviewed Oct 4, 2016

View reviewed changes

calebamiles mentioned this pull request Oct 5, 2016

Umbrella issue for the Linux Foundation (CNCF) CLA system #27796

Closed

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2016

euank force-pushed the coreos-kube-up-now-with-less-cloud-init branch from 082275d to aca0aa2 Compare October 10, 2016 23:34

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2016

euank force-pushed the coreos-kube-up-now-with-less-cloud-init branch from aca0aa2 to 6bb8c49 Compare October 11, 2016 03:07

googlebot added cla: no and removed cla: yes labels Oct 14, 2016

euank force-pushed the coreos-kube-up-now-with-less-cloud-init branch from 05ec52f to 3536ecc Compare December 15, 2016 19:46

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2016

euank mentioned this pull request Dec 16, 2016

cluster/*: Renaming coreos to container-linux in necessary places. #38848

Closed

roberthbailey reviewed Dec 16, 2016

View reviewed changes

euank force-pushed the coreos-kube-up-now-with-less-cloud-init branch from 3536ecc to 65e8d77 Compare December 16, 2016 22:02

euank mentioned this pull request Dec 16, 2016

cluster/gce/coreos: add OWNERS #38906

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2016

euank added 4 commits December 17, 2016 21:36

cluster/gce: copy gci -> coreos

e2644bb

This is for reviewing ease as the following commits introduce changes to make the coreos kube-up deployment share significant code with the gci code.

cluster/coreos: delete mounter

028a014

We don't use this bit of gci currently.

cluster/coreos: update veify-flags exceptions

5a2d080

euank force-pushed the coreos-kube-up-now-with-less-cloud-init branch from 65e8d77 to 5a2d080 Compare December 18, 2016 05:36

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2016

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2016

k8s-github-robot merged commit 3994845 into kubernetes:master Dec 20, 2016

euank deleted the coreos-kube-up-now-with-less-cloud-init branch December 20, 2016 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coreos kube-up now with less cloud init #33965

Coreos kube-up now with less cloud init #33965

euank commented Oct 3, 2016 •

edited

yifan-gu Oct 4, 2016

yifan-gu commented Oct 4, 2016

k8s-ci-robot commented Oct 4, 2016

yifan-gu Oct 4, 2016 •

edited

yifan-gu Oct 4, 2016

yifan-gu Oct 4, 2016

euank Oct 4, 2016

yifan-gu Oct 7, 2016

yifan-gu commented Oct 4, 2016

euank commented Oct 4, 2016

k8s-ci-robot commented Oct 11, 2016

k8s-ci-robot commented Oct 11, 2016

k8s-ci-robot commented Oct 11, 2016

k8s-ci-robot commented Oct 11, 2016

googlebot commented Oct 14, 2016

roberthbailey commented Dec 16, 2016

bgrant0607 commented Dec 16, 2016

roberthbailey commented Dec 16, 2016

euank commented Dec 16, 2016 •

edited

roberthbailey commented Dec 16, 2016

roberthbailey Dec 16, 2016

euank Dec 16, 2016

roberthbailey Dec 16, 2016

bgrant0607 commented Dec 16, 2016

roberthbailey commented Dec 16, 2016

euank commented Dec 16, 2016

roberthbailey commented Dec 17, 2016

euank commented Dec 19, 2016

roberthbailey commented Dec 20, 2016

k8s-github-robot commented Dec 20, 2016

k8s-github-robot commented Dec 20, 2016

		@@ -0,0 +1,8 @@
		# CoreOS image

		The [CoreOS operating oystem](https://coreos.com/why/) is a Linux distribution optimized for running containers securely at scale.

		@@ -0,0 +1,173 @@
		#!/bin/bash

		# Copyright 2015 The Kubernetes Authors.

Coreos kube-up now with less cloud init #33965

Coreos kube-up now with less cloud init #33965

Conversation

euank commented Oct 3, 2016 • edited

Choose a reason for hiding this comment

yifan-gu commented Oct 4, 2016

k8s-ci-robot commented Oct 4, 2016

yifan-gu Oct 4, 2016 • edited

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

yifan-gu commented Oct 4, 2016

euank commented Oct 4, 2016

k8s-ci-robot commented Oct 11, 2016

k8s-ci-robot commented Oct 11, 2016

k8s-ci-robot commented Oct 11, 2016

k8s-ci-robot commented Oct 11, 2016

googlebot commented Oct 14, 2016

roberthbailey commented Dec 16, 2016

bgrant0607 commented Dec 16, 2016

roberthbailey commented Dec 16, 2016

euank commented Dec 16, 2016 • edited

roberthbailey commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Dec 16, 2016

roberthbailey commented Dec 16, 2016

euank commented Dec 16, 2016

roberthbailey commented Dec 17, 2016

euank commented Dec 19, 2016

roberthbailey commented Dec 20, 2016

k8s-github-robot commented Dec 20, 2016

k8s-github-robot commented Dec 20, 2016

euank commented Oct 3, 2016 •

edited

yifan-gu Oct 4, 2016 •

edited

euank commented Dec 16, 2016 •

edited