Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ubuntu provider - can't set docker opts or complete install #17615

Merged
merged 2 commits into from
Dec 10, 2015

Conversation

robdaemon
Copy link

Setting custom DOCKER_OPTS so that you can use a private registry isn't working - the config-default.sh file needs to be sourced so it's visible when the /etc/default/docker file is written.

Additionally, the recent change to add quoting to PROXY_SETTING is not allowing the certificates to be created, which breaks cluster creation. The apiserver never starts because it's missing its certificates.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@k8s-bot
Copy link

k8s-bot commented Nov 21, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

1 similar comment
@k8s-bot
Copy link

k8s-bot commented Nov 21, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@k8s-github-robot
Copy link

Labelling this PR as size/S

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 21, 2015
@robdaemon
Copy link
Author

It sounds like I should be covered under the Hewlett Packard Enterprise CLA.

@resouer
Copy link
Contributor

resouer commented Nov 21, 2015

@robdaemon Are you insist on enable bash -x by default?

Personally, I would prefer a debug mode, but not sure if I asked too much :)

@robdaemon
Copy link
Author

@resouer I don't mind doing it as a debug mode instead. I put -x because it helped me find the root cause of the problem to begin with. I can rework that part if you'd like.

@resouer
Copy link
Contributor

resouer commented Nov 21, 2015

Yes, please do it :)

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 23, 2015
@robdaemon
Copy link
Author

@resouer Okay, added the DEBUG flag as you requested!

@@ -71,3 +71,4 @@ ENABLE_CLUSTER_UI="${KUBE_ENABLE_CLUSTER_UI:-true}"
# Add envitonment variable separated with blank space like "http_proxy=http://10.x.x.x:8080 https_proxy=https://10.x.x.x:8443"
PROXY_SETTING=${PROXY_SETTING:-""}

DEBUG=${DEBUG:-"false"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line here

@resouer
Copy link
Contributor

resouer commented Nov 24, 2015

Thanks! Just a few nits.

And please sign your CLA :)

@robdaemon
Copy link
Author

I've signed the CLA - it's through the Hewlett Packard Enterprise corporate CLA :)

@googlebot
Copy link

CLAs look good, thanks!

@robdaemon
Copy link
Author

@resouer - feedback integrated, CLA in-place, this should be ready now?

@resouer
Copy link
Contributor

resouer commented Nov 25, 2015

Thanks! This LGTM

cc owners: @zmerlynn @davidopp

@dalanlan
Copy link
Contributor

ping @zmerlynn

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2015
@dalanlan
Copy link
Contributor

dalanlan commented Dec 2, 2015

@robdaemon Rebase needed;-)

@dalanlan
Copy link
Contributor

dalanlan commented Dec 8, 2015

ping @robdaemon

Robert Roland added 2 commits December 8, 2015 07:52
…efault/docker file, otherwise the DOCKER_OPTS set there do not get applied to new minion nodes.
When PROXY_SETTING is empty, you end up an empty
command of "", as witnessed by this bash debug
output when +x is enabled:

    + '' /home/ubuntu/kube/make-ca-cert.sh 10.0.0.232 IP:10.0.0.232,IP:192.168.3.1,DNS:kubernetes,DNS:kubernetes.default,DNS:kubernetes.default.svc,DNS:kubernetes.default.svc.cluster.local

Given the example:

    PROXY_SETTING="http_proxy=http://server:port https_proxy=https://server:port"

You would not want this quoted on the script executed
on the remote master or minion node.

Enabling +e, for additional tracing and to
abort on any failure in the remote SSH session.

Adding a DEBUG parameter into config-default.sh allowing additional
debug information to be present in the logs during node rollout, using
bash's "set -x" when DEBUG=true
@robdaemon
Copy link
Author

@dalanlan sorry about the delay, this is rebased now.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2015
@dalanlan
Copy link
Contributor

dalanlan commented Dec 9, 2015

ping @zmerlynn
This patch has been backlogged for quite a while. PTAL

@zmerlynn
Copy link
Member

zmerlynn commented Dec 9, 2015

@dalanlan: Looking.
@k8s-bot: ok to test

@zmerlynn zmerlynn added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e test build/test passed for commit ebeb7bc.

@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e build/test failed for commit ebeb7bc.

@dalanlan
Copy link
Contributor

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e test build/test passed for commit ebeb7bc.

@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e test build/test passed for commit ebeb7bc.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e test build/test passed for commit ebeb7bc.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Dec 10, 2015
@k8s-github-robot k8s-github-robot merged commit 184171c into kubernetes:master Dec 10, 2015
@robdaemon robdaemon deleted the source-config-default branch October 31, 2016 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants