local-up-cluster additions #38713

alejandroEsc · 2016-12-13T17:35:18Z

What this PR does / why we need it:
Changes to local-cluster-up: These include: 1) a simple additional help option. 2) additional error message to not being able to run docker ps. 3) fail faster when etcd is not found in path. Hopefully these make developing a bit more pleasant.

Release note:

k8s-reviewable · 2016-12-13T17:35:24Z

This change is

ixdy · 2016-12-14T02:09:00Z

hack/local-up-cluster.sh

@@ -681,6 +716,9 @@ EOF
 fi
 }

+# let's check that etcd is in path before moving on


kube::etcd::start basically does this check, and also verifies that it's the correct version. do we need to duplicate this check?

alternately, factor out the test logic from kube::etcd::start and call it explicitly here?

I like that idea, my only concern was starting etcd too early in the process while i just wanted to ensure that the process even existed, additionally that would sort of break the nice logic starting in line 748 where services are all started together.

Would you be ok with me instead refactoring the logic found in kube::etcd::start so that a new function is created kube::etcd::validate that is called in start, so that the validation check is at least in one place?

yeah, making a separate kube::etcd::validate sounds good to me. (might want to have kube::etcd::start call it to maintain existing behavior.)

ixdy · 2016-12-14T02:10:56Z

hack/local-up-cluster.sh

-      echo "Failed to successfully run 'docker ps', please verify that docker is installed and \$DOCKER_HOST is set correctly."
+      echo
+      cat <<'EOF' >&2
+Failed to successfully run 'docker ps', please verify that docker is installed and \$DOCKER_HOST is set correctly. 


it looks like you took this from kube::build::ensure_docker_daemon_connectivity in build-tools/common.sh.

I wonder if it might make more sense to refactor that function into hack/lib/util.sh, and then call that from both build-tools/common.sh and here.

I will double check that the logic is the same, if so would you have no problem with me doing this? If not would this still be ok?

they look pretty similar; it appears that the main difference is that ensure_docker_daemon_connectivity is using docker info, whereas this uses docker ps, but I think the effect is basically the same: if docker is installed and working, both should succeed; if docker isn't working, both will fail.

alejandroEsc · 2016-12-14T16:20:54Z

@ixdy changes made, looks great, thanks again for reviewing so promptly!

ixdy · 2016-12-16T00:16:08Z

hack/local-up-cluster.sh

-  test_docker
+  # source to get common docker test
+  #source "${KUBE_ROOT}/build-tools/common.sh"
+  ${KUBE_ROOT}/build-tools/common.sh kube::build::ensure_docker_daemon_connectivity


a few notes:

this is now build/ instead of build-tools/.

does this actually work as-written? (rather than sourcing common.sh)

I'd also be fine with having this function moved into hack/lib/common.sh. I think both build/common.sh and this script (hack/local-up-cluster.sh) include hack/lib/.

yeah saw that happened recently

yep it works, it's how you call a function when you don't want to source it, tried pulling from source but it got complicated because of the environment variables called in that common.sh script.

I am guessing you mean /hack/lib/init.sh? if so yeah they both source this file and would be a good place assuming that the function can cleanly be moved. Ill try it out.

alejandroEsc · 2016-12-16T06:45:12Z

@ixdy done: made changes and was able to refactor out common code.

ixdy

sorry for so much back-and-forth. it's almost there.

ixdy · 2016-12-16T08:08:17Z

hack/lib/init.sh

@@ -156,3 +156,28 @@ kube::realpath() {
  fi
  kube::readlinkdashf "$1"
 }
+
+function kube::build::ensure_docker_daemon_connectivity {


please put this in hack/lib/util.sh instead; hack/lib/init.sh loads the other scripts in hack/lib, but shouldn't contain any other functionality itself.

also s/build/util/ in the name, to match the rest of the functions in hack/lib/util.sh.

yikes, you are correct, should have double checked the init.sh and what it sources out.

…why docker ps fails and how to recover, added test to check if etcd is in your path to fail fast when not found. from etcd.sh split the start process into validate fucntion + start function so that the validate piece can be reused elsewhere. the up-cluster script has been changed to remove duplicate docker logic to the one used in buid-tools/common.sh and the validate etcd function is now used here. moved docker daemon check function to util.sh and made function name changes and upstream changes.

alejandroEsc · 2016-12-16T15:42:34Z

@ixdy made changes you requested. Thanks!

k8s-ci-robot · 2016-12-16T15:43:20Z

Jenkins GCE etcd3 e2e failed for commit d6f107f9bdff253589ee35824c8a5e18c56be95c. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 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-12-16T15:43:20Z

Jenkins verification failed for commit d6f107f9bdff253589ee35824c8a5e18c56be95c. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify 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-12-16T15:44:21Z

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

The magic incantation to run this job again is @k8s-bot cvm 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-12-16T15:44:45Z

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

The magic incantation to run this job again is @k8s-bot cvm 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-12-16T15:44:54Z

Jenkins GCE Node e2e failed for commit d6f107f9bdff253589ee35824c8a5e18c56be95c. Full PR test history.

The magic incantation to run this job again is @k8s-bot node 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-12-16T15:45:16Z

Jenkins unit/integration failed for commit d6f107f9bdff253589ee35824c8a5e18c56be95c. 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.

k8s-ci-robot · 2016-12-16T15:45:26Z

Jenkins Kubemark GCE e2e failed for commit d6f107f9bdff253589ee35824c8a5e18c56be95c. 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.

k8s-ci-robot · 2016-12-16T15:45:54Z

Jenkins GCI GKE smoke e2e failed for commit d6f107f9bdff253589ee35824c8a5e18c56be95c. 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-12-16T15:46:35Z

Jenkins GCI GCE e2e failed for commit d6f107f9bdff253589ee35824c8a5e18c56be95c. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci 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-12-16T15:46:46Z

Jenkins CRI GCE Node e2e failed for commit d6f107f9bdff253589ee35824c8a5e18c56be95c. Full PR test history.

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

ixdy · 2016-12-16T22:25:21Z

/lgtm

thanks!

alejandroEsc · 2016-12-16T23:05:34Z

@ixdy all checks passed, ready for you to merge, thanks again!

k8s-github-robot · 2016-12-17T02:55:31Z

Automatic merge from submit-queue (batch tested with PRs 37468, 36546, 38713, 38902, 38614)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 13, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Dec 13, 2016

k8s-github-robot assigned ixdy Dec 13, 2016

ixdy reviewed Dec 14, 2016

View reviewed changes

alejandroEsc force-pushed the ae/localup1 branch from 3cf78e0 to e10aee0 Compare December 14, 2016 16:19

ixdy reviewed Dec 16, 2016

View reviewed changes

alejandroEsc force-pushed the ae/localup1 branch 2 times, most recently from 9340ded to ba5b081 Compare December 16, 2016 06:43

ixdy reviewed Dec 16, 2016

View reviewed changes

alejandroEsc force-pushed the ae/localup1 branch from ba5b081 to d6f107f Compare December 16, 2016 15:41

alejandroEsc force-pushed the ae/localup1 branch from d6f107f to 7d9c06f Compare December 16, 2016 15:42

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

k8s-github-robot merged commit 9a1c607 into kubernetes:master Dec 17, 2016

alejandroEsc deleted the ae/localup1 branch December 19, 2016 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

local-up-cluster additions #38713

local-up-cluster additions #38713

alejandroEsc commented Dec 13, 2016 •

edited

k8s-reviewable commented Dec 13, 2016

ixdy Dec 14, 2016

ixdy Dec 14, 2016

alejandroEsc Dec 14, 2016

ixdy Dec 14, 2016

ixdy Dec 14, 2016

alejandroEsc Dec 14, 2016

ixdy Dec 14, 2016

alejandroEsc commented Dec 14, 2016

ixdy Dec 16, 2016

alejandroEsc Dec 16, 2016

alejandroEsc commented Dec 16, 2016

ixdy left a comment

ixdy Dec 16, 2016

alejandroEsc Dec 16, 2016

alejandroEsc commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

ixdy commented Dec 16, 2016

alejandroEsc commented Dec 16, 2016 •

edited

k8s-github-robot commented Dec 17, 2016

local-up-cluster additions #38713

local-up-cluster additions #38713

Conversation

alejandroEsc commented Dec 13, 2016 • edited

k8s-reviewable commented Dec 13, 2016

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

alejandroEsc commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandroEsc commented Dec 16, 2016

ixdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandroEsc commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

k8s-ci-robot commented Dec 16, 2016

ixdy commented Dec 16, 2016

alejandroEsc commented Dec 16, 2016 • edited

k8s-github-robot commented Dec 17, 2016

alejandroEsc commented Dec 13, 2016 •

edited

alejandroEsc commented Dec 16, 2016 •

edited