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

Refactor cluster/juju #22726

Merged
merged 11 commits into from
Mar 23, 2016
Merged

Conversation

lazypower
Copy link
Contributor

Apologies in advance for the XL PR

The Juju team has been hard at work refactoring the Kubernetes charms. A new charming pattern has emerged where charms are now assembled from layers to build a deployable artifact. Allowing our footprint in the Kubernetes upstream repository to remain light and only pertain to the concerns of Kubernetes itself.

A few things to note about this PR:

  • It requires Juju 2.0 for full validation
    • Juju 1.25 support is incoming - on this PR if its deemed necessary prior to landing
  • We make use of /tmp/${random} to warehouse the ssl key(s) and kubeconfig during cluster/kube-up.sh - this is not intended to stand up a long running kubernetes cluster, but instead to validate the charm deployment for devel purposes. (building and packing a custom hyperkube image is planned but unscheduled at the moment)
  • This also removes the compile from source option, and instead moves to delivery via hyperkube

Prereq work to address #9107

@k8s-bot
Copy link

k8s-bot commented Mar 9, 2016

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

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

2 similar comments
@k8s-bot
Copy link

k8s-bot commented Mar 9, 2016

Can one of the admins verify that this patch is reasonable to test in $JOB_NAME? (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-bot
Copy link

k8s-bot commented Mar 9, 2016

Can one of the admins verify that this patch is reasonable to test in $JOB_NAME? (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/XXL

@k8s-github-robot k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 9, 2016
args:
# command = "/kube2sky"
{% if dns_domain -%}- -domain={{ dns_domain }} {% else %} - -domain=cluster.local {% endif %}
- -kube_master_url=http://{{private_address}}:8080

Choose a reason for hiding this comment

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

This could be your issue. I believe you need it to be {{ private_address }} and not {{private_address}}

@lazypower
Copy link
Contributor Author

Ok, I think i have this building cleanly now, and have updated the necessary Meta. Let me know if you'd like me to squash this before the review happens.

@lazypower
Copy link
Contributor Author

@zmerlynn - any update on getting a review? the travis and cla bot seem fine. submit-queue still however states its pending failing tests? is that the mesos smoke test bot that i need to satisfy before I get a review?

@k8s-bot
Copy link

k8s-bot commented Mar 15, 2016

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")

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

Charles Butler added 6 commits March 16, 2016 14:25
This commit imports the latest development focus from the Charmer team
working to deliver Kubernetes charms with Juju.

Notable Changes:

- The charm is now assembled from layers in $JUJU_ROOT/layers
- Prior, the juju provider would compile and fat-pack the charms, this
  new approach delivers the entirety of Kubernetes via hyperkube.
- Adds Kubedns as part of `cluster/kube-up.sh` and verification
- Removes the hard-coded port 8080 for the Kubernetes Master
- Includes TLS validation
- Validates kubernetes config from leader charm
- Targets Juju 2.0 commands
@zmerlynn
Copy link
Member

@k8s-bot ok to test

@zmerlynn
Copy link
Member

@chuckbutler: I'm sorry for the long delay, this went to the bottom due to size. Is there any other maintainer that can help review this as well? This is mammoth and I have little Juju context.

@k8s-bot
Copy link

k8s-bot commented Mar 21, 2016

GCE e2e build/test passed for commit b8b54ac.

@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?

@lazypower
Copy link
Contributor Author

@zmerlynn - you bet, theres a couple

If you're OK with canonical being the reviewers, @marcoceppi or @mbruzek is on our ticket.

In terms of googlers that have looked at our code in the past:

@eparis and @erictune both lent a hand in bringin us on board and looking over our initial PR's that landed in cluster. I apologize again for the size, it wasn't intended to be this large, but it is a big deletion of footprint and re-scoping of what needs to be here, vs supporting infra code that revs independently of our work here.

If there's anything else I can do to help expedite the request, such as test run logs or an environment stood up with the charms to poke at and validate I'm happy to lend a hand.

expose: true
num_units: 2
etcd:
charm: cs:~lazypower/trusty/etcd
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Would it be better to put etcd in ~containers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll bring this in on the next update w/ the docs. @zmerlynn - is that still going to be pointed here or have the docs migrated to their new home already?

@marcoceppi
Copy link
Contributor

LGTM

@mbruzek
Copy link
Contributor

mbruzek commented Mar 21, 2016

I independently verified running ./kube-up.sh from the cluster directory. Everything came up OK. I will take a further look at the code.

... calling validate-cluster
Found 2 node(s).
NAME            LABELS                                 STATUS    AGE
172.31.10.125   kubernetes.io/hostname=172.31.10.125   Ready     8m
172.31.33.22    kubernetes.io/hostname=172.31.33.22    Ready     7m
Validate output:
NAME                 STATUS    MESSAGE              ERROR
etcd-0               Healthy   {"health": "true"}   nil
controller-manager   Healthy   ok                   nil
scheduler            Healthy   ok                   nil
Cluster validation succeeded
Done, listing cluster services:

Kubernetes master is running at https://52.91.207.25:6443
KubeDNS is running at https://52.91.207.25:6443/api/v1/proxy/namespaces/kube-system/services/kube-dns

@mbruzek
Copy link
Contributor

mbruzek commented Mar 21, 2016

I know this looks like an XXL pull request, but looking at the lines changed: +915 −2,513 that is a net loss of 1598 lines of code in the cluster/juju/ directory that makes the Juju specific files much smaller.

The heart of this change is in the cluster/juju/util.sh and that also was reduced in size. I deployed this via kube-up.sh on amazon with KUBERNETES_REPOSITORY=juju and it worked for me.

+1 on the code and the change from my end (for what it is worth).

@erictune
Copy link
Member

@zmerlynn I'll pick this up.

@erictune erictune assigned erictune and unassigned zmerlynn Mar 22, 2016

- [Kubernetes github project](https://github.com/kubernetes/kubernetes)
- [Kubernetes issue tracker](https://github.com/kubernetes/kubernetes/issues)
- [Kubernetes Documenation](https://github.com/kubernetes/kubernetes/tree/master/docs)
Copy link
Member

Choose a reason for hiding this comment

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

We prefer that you link to http://kubernetes.io/docs/

@erictune
Copy link
Member

two comments.

@k8s-bot
Copy link

k8s-bot commented Mar 23, 2016

GCE e2e build/test passed for commit 326a51f.

@k8s-bot
Copy link

k8s-bot commented Mar 23, 2016

GCE e2e build/test passed for commit cc1fb15.

@erictune
Copy link
Member

LGTM. Thanks!

@erictune erictune added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2016
@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 Mar 23, 2016

GCE e2e build/test passed for commit cc1fb15.

@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 Mar 23, 2016

GCE e2e build/test passed for commit cc1fb15.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 69b3cb3 into kubernetes:master Mar 23, 2016
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants