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

Adds JUJU to the Kubernetes Provider listing #5414

Merged
merged 3 commits into from Apr 2, 2015

Conversation

@lazypower
Copy link
Contributor

@lazypower lazypower commented Mar 12, 2015

This feature adds Juju provisioning to the kube-up script. It currently
parses out the pre-requisits on debian/ubuntu based systems and installs
them if they are missing.

From there we followed the integration path that was found in the
libvirt-coreos path, implementing the methods found in the boilerplate
and calling juju service calls. There are a few "arbitrary sleeps" in
the code to allow the cloud provider to settle and properly deploy.
These are work-around cases from the script executing faster than juju
was able to communicate from the state server to subsequent nodes. I
left comments inline at these points.

To exercise this:

export KUBERNETES_PROVIDER=juju
cluster/kube-up.sh

It will spin up a ref arch with 1 Kubernetes Master, 2 minions, and run
the cluster validation checks against the deployment. Bridging the gap
between the juju specific bits and the upstream recommended guides for
getting started with Kubernetes.

To note, if you do not have a "current environment" set in Juju, it will
spin up the quickstart integration wizard in interactive mode, allowing
you to configure juju, and add the proper provider/use it. Otherwise it
assumes you're in the provider you wish to use, and will deploy there.

This feature adds Juju provisioning to the kube-up script. It currently
parses out the pre-requisits on debian/ubuntu based systems and installs
them if they are missing.

From there we followed the integration path that was found in the
libvirt-coreos path, implementing the methods found in the boilerplate
and calling juju service calls. There are a few "arbitrary sleeps" in
the code to allow the cloud provider to settle and properly deploy.
These are work-around cases from the script executing faster than juju
was able to communicate from the state server to subsequent nodes. I
left comments inline at these points.

To exercise this:

    export KUBERNETES_PROVIDER=juju
    cluster/kube-up.sh

It will spin up a ref arch with 1 Kubernetes Master, 2 minions, and run
the cluster validation checks against the deployment. Bridging the gap
between the juju specific bits and the upstream recommended guides for
getting started with Juju.

To note, if you do not have a "current environment" set in Juju, it will
spin up the quickstart integration wizard in interactive mode, allowing
you to configure juju, and add the proper provider/use it. Otherwise it
assumes you're in the provider you wish to use, and will deploy there.
@googlebot
Copy link

@googlebot googlebot commented Mar 12, 2015

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

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 the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@erictune
Copy link
Member

@erictune erictune commented Mar 13, 2015

It seems that the flags for the various binaries are stored outside this repo, at places like https://github.com/whitmo/charm-kubernetes-master/blob/master/files/apiserver.upstart.tmpl.

For the other cloud providers which support kube-up.sh, the flags are in our repository. This allows us to make changes in a single commit to both the source code and the flags for all those distros.

Is there a reason to have the code for kubernetes-on-juju split across repos in this way?

@lazypower
Copy link
Contributor Author

@lazypower lazypower commented Mar 13, 2015

Thanks for the review!

The current structure of our repositories reflects a pattern we use to separate concerns in the charm store - but this does not reflect the pattern you would need to perform easy updates as the kubernetes project moves foward. I apologize for not recognizing that before issuing the PR.

I'll get the charms/bundle pulled into this branch and we will certainly look into conglomerating our resources that are outside of the GCP namespace so it's easier to track.

# Kubernetes Juju Charms project - located here: https://github.com/whitmo/bundle-kubernetes

function check_for_ppa(){
grep -s ^ /etc/apt/sources.list /etc/apt/sources.list.d/* | grep juju
Copy link
Member

@filbranden filbranden Mar 13, 2015

Why not just:

  grep -qsw juju /etc/apt/sources.list /etc/apt/sources.list.d/*

Use -q to make it quiet and -w to make sure you're matching a whole word (i.e. you want to match juju but not jujuba).

Copy link
Member

@filbranden filbranden Mar 13, 2015

You want to make the ppa name parameterizable, or rename the function to check_for_juju_ppa.

@erictune
Copy link
Member

@erictune erictune commented Mar 13, 2015

@chuckbutler @whitmo

Have you thought about whether your goal is (1) to have periodic releases of Kubernetes-on-JuJu, or (2) to ensure that Kubernetes-on-JuJu is always compatible with the very latest K8s source code. Option (2) comes with higher expectations in terms of test automation and your overall level of involvement in the kubernetes project.

If your goal is (1), then we should talk about whether this PR is needed at all.

If it is (2), then this PR is definitely the right direction, but I expect additional steps will be needed too, such as moving JuJu Charm files into the kubernetes repo, as mentioned above; and running kubernets e2e test continuously, etc.

I'm happy to see either outcome. Also, it is fine to start with 1 and move to 2 in the future.

}

function detect-minions(){
KUBE_MINION_IP_ADDRESSES=($(juju run --service kubernetes "unit-get private-address" --format=yaml | grep Stdout | cut -d "'" -f 2))
Copy link
Member

@filbranden filbranden Mar 13, 2015

awk can do both of what grep and cut do in a single command... can you use it instead?

@filbranden
Copy link
Member

@filbranden filbranden commented Mar 13, 2015

Bunch of comments on the bash script... I'll reassign to @erictune to decide whether it's OK to merge though, I'm not very familiar with Juju (well, I know what it is and that's that...) so I'd prefer if someone else would take a look at it.

Let me know if you need me to take another look at the shell scripts later in the code review.

Cheers,
Filipe

@lazypower
Copy link
Contributor Author

@lazypower lazypower commented Mar 13, 2015

Thank you for the review @filbranden. I'll be working on implementing the comments and should have this ready for re-review on Monday.

Cheers!

@erictune
Copy link
Member

@erictune erictune commented Mar 13, 2015

Thanks, @filbranden. All your comments look reasonable to me.

- Changed check_for_ppa to be parameterized
- Added bash strictmode
- refactored the package_status method to consume variables and be a bit
  nicer to future re-use of the method.
- Cut out extra echo -n statements in favor of tr -d or native awk
- Refactored branching logic paths to leverage double brackets
- normalized local variable annotation
- Updated globals to be all CAPS
- remainder of filbrandens feedback in validate-cluster.sh
@erictune
Copy link
Member

@erictune erictune commented Mar 17, 2015

@chuckbutler hoping to hear your thoughts on #5414 (comment)

@mbruzek
Copy link
Contributor

@mbruzek mbruzek commented Mar 18, 2015

@erictune,

We’ve had a bit of a discussion around this to ensure everyone working on the Juju Kubernetes integration is on the same page. We’re all in agreement that having the Juju Charms and Bundles in the Kubernetes repository is in the best interest of our project moving forward. We’re actively working on addressing the automation of e2e testing - and currently have some basic stand up testing and validation running in our CI. It’s not as automated as requested above, but we’re working towards that end. If you’re interested in the CI reports, you can view them here: http://reports.vapour.ws/all-bundle-and-charm-results/gh%3Awhitmo%252Fbundle-kubernetes

Adding the visibility of Juju as an officially supported (and hopefully recommended) method to deploy K8’s - we feel this brings users a new and different way to deploy a cluster that has implications beyond just standing up Kubes.

We plan to continue adjusting the charms to ensure they work with the latest upstream, and to provide configuration options to run different releases. We would like to continue to support this moving forward as the project evolves.

What do you feel are the next steps for the team to proceed forward?

@erictune
Copy link
Member

@erictune erictune commented Mar 26, 2015

I'm happy to hear you plan to work on e2e testing, and I'm happy for users to have Juju as an option for deploying Kubernetes.

I think the next steps are:

  • you fix the build failure
  • I'll merge this PR
  • over time, the Juju+Kubernetes folks to try to get as many of the e2e tests passing as possible.
  • as you do that, you will probably send additional PRs to extent the cluster/juju/... files.
  • we may make reasonable requests for features or file reorganizaton on those future PRs

@erictune
Copy link
Member

@erictune erictune commented Mar 26, 2015

Looking at the build failure, it looks like it doesn't like the boilerplate at the top of your shell scripts.
You may need to do this to run precommit checks:

cd kubernetes/.git/hooks/
ln -s ../../hooks/prepare-commit-msg .
ln -s ../../hooks/commit-msg .


set -euo pipefail

# Copyright 2014 Canonical LTD. All rights reserved.
Copy link
Member

@erictune erictune Mar 26, 2015

I don't think we can take code that has anything other than the Kubernetes boilerplate at the top. See comment on conversation tab about github hooks.

Copy link
Contributor Author

@lazypower lazypower Mar 26, 2015

Brilliant. Thanks for following up on this Eric. I'll get the headers updated and link in the hooks.
Sent using Boxer
On Mar 26, 2015 1:50 PM, Eric Tune notifications@github.com wrote:In cluster/juju/prereqs/ubuntu-juju.sh:

@@ -0,0 +1,48 @@
+#!/bin/bash
+
+set -euo pipefail
+
+# Copyright 2014 Canonical LTD. All rights reserved.

I don't think we can take code that has anything other than the Kubernetes boilerplate at the top. See comment on conversation tab about github hooks.

—Reply to this email directly or view it on GitHub.

@erictune
Copy link
Member

@erictune erictune commented Mar 26, 2015

I've looked several places and I don't see a CLA that would apply to this PR. Let me know if you think this is wrong, or need to talk about it off-PR.

@lazypower
Copy link
Contributor Author

@lazypower lazypower commented Mar 26, 2015

@erictune we're at a point that we should probably talk off PR. I'm going to pull @whitmo and @mbruzek into the thread.

@lazypower
Copy link
Contributor Author

@lazypower lazypower commented Mar 31, 2015

@erictune I've got confirmation from legal that our CLA is now signed, and our team has joined the CLA google group. Can you confirm that this is indeed the case?

@erictune
Copy link
Member

@erictune erictune commented Mar 31, 2015

I can't see the CLA yet. Let's wait one day to see if things need time to propagate through my side.

@erictune
Copy link
Member

@erictune erictune commented Apr 1, 2015

@chuckbutler contact me at etune@google.com

@erictune
Copy link
Member

@erictune erictune commented Apr 2, 2015

Ongoing offline CLA discussion...

@erictune
Copy link
Member

@erictune erictune commented Apr 2, 2015

I've verified CLA offline.

erictune added a commit that referenced this issue Apr 2, 2015
Adds JUJU to the Kubernetes Provider listing
@erictune erictune merged commit ecf39a6 into kubernetes:master Apr 2, 2015
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants