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

A build and deploy script to manage cluster federation lifecycle and a Makefile wrapper to drive that process. #29515

Merged
merged 12 commits into from
Aug 13, 2016

Conversation

madhusudancs
Copy link
Contributor

@madhusudancs madhusudancs commented Jul 24, 2016

Also includes a sample config file to describe clusters.

The build script implements the following things:

  1. Generates the required configs.
  2. Builds the hyperkube binary and the corresponding docker image.
  3. Pushes the image to a specified repository.
  4. Pulls the federation installer docker images.
  5. Builds the Kubernetes clusters described the config.json file.
  6. Pushes the federation components to one of the Kubernetes clusters
    built in the previous step.
  7. Also turns down the federation components and the Kubernetes
    clusters.

NOTE: Installer images are right now being pulled from my public repository of docker images. I am working on pushing them to our release repository.

Cluster Federation components can now be built and deployed using the make command. Please see federation/README.md for details.

cc @kubernetes/sig-cluster-federation @colhom

Fixes: Issue #26655


This change is Reviewable

@madhusudancs madhusudancs added release-note Denotes a PR that will be considered when it comes time to generate release notes. area/cluster-federation labels Jul 24, 2016
@madhusudancs madhusudancs added this to the v1.4 milestone Jul 24, 2016
@madhusudancs madhusudancs force-pushed the fed-makefile branch 3 times, most recently from 6a3b6f5 to 66b3feb Compare July 24, 2016 00:39
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 24, 2016
@madhusudancs madhusudancs force-pushed the fed-makefile branch 3 times, most recently from 1ef472d to 94b25f2 Compare July 24, 2016 21:23
@colhom
Copy link

colhom commented Jul 25, 2016

@madhusudancs this is awesome!!! I feel like a human can actually understand what's going on now!

I will give this a thorough review later today. Just wanted to say how happy this makes me.

@madhusudancs
Copy link
Contributor Author

@colhom thanks!

@colhom
Copy link

colhom commented Jul 25, 2016

@madhusudancs if I'm reading this correctly- we don't really need the federation/cluster/*.sh scripts anymore? Maybe we should just replace them in this PR

@madhusudancs
Copy link
Contributor Author

@colhom you are right. However, kubernetes-anywhere, on which this depends, does not work very well with AWS yet. I think it is a good idea to leave the cluster/*.sh scripts as is until we are fairly confident that it works with AWS as well, i.e. we have tested out this thing well on AWS.

@colhom
Copy link

colhom commented Jul 25, 2016

@madhusudancs what aws-specific issues are you running into? i see you're using the kube-up bash libraries, so there shouldn't (theoretically) be any provider-specific problems.

@madhusudancs
Copy link
Contributor Author

@colhom Although it is theoretically possible to use kube-up with this script, this PR uses kubernetes-anywhere to turn up clusters.

I don't remember the exact error and I tried it only once. When I tried to bring up the cluster with kubernetes-anywhere, the AWS resources were all created correctly but I could still not reach kube-apiserver. I did not spend time debugging. But any help there is welcome.


.PHONY: build
build:
./build.sh $(do)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to make these individual make targets? make gen, make deploy, make destroy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the things that I am going to work on next, unless someone offers to help (as listed in the TODO in the README file). I started out this way because it was easier to experiment with things if I had to make changes to only one script (build.sh).

@madhusudancs
Copy link
Contributor Author

@mml @nikhiljindal PTAL.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2016
@mml
Copy link
Contributor

mml commented Aug 10, 2016

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


federation/build.sh, line 74 [r3] (raw file):

function cleanup {
  rm -rf "${TMP_DIR}"

Tab literals here (through the whole file). Not sure we have a style guide, but it seems to me that 2 blank spaces is what I've seen, except in go files.


federation/Makefile, line 16 [r2] (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Not entirely sure. Assuming FreeBSD actually ships BSD make, this should work - http://www.freebsd.org/cgi/man.cgi?make(1). I don't have access to a Mac, so I need your help here.

Also, this isn't specific to federation, the top-level Makefile (the one in the root directory of the source) also has this, so I think this should work on Mac.

OK. I checked and it's actually GNU make on Mac.

federation/README.md, line 12 [r2] (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Thanks for starting this conversation. I meant to debate about this but somehow missed it while sending this PR.

My arguments to self so far:

  1. jq is really easy to install. It is in the upstream repos of popular Linux distributions. No additional repositories needed, just apt-get install jq, dnf install jq and so on. There are packages and/or binaries readily available for Mac, Windows and other platforms - https://stedolan.github.io/jq/download/. So it is not that much of a hassle IMO.
  2. And it is required only for dev environments. We will not require jq in the shipped release/distribution.
  3. It is not doing something special that we cannot accomplish with sed, but the intent is so much clear with jq. Also, it guarantees that it modifies only that particular JSON object we intend to modify and not accidentally modify some other field which matches the regexp.

Please let me know your thoughts. Open to suggestions.

The only thing I have to add is that we should do this however kubernetes anywhere does it. From a glance, it looks like they use jsonnet as a way to templatize json.

If you want to check this in with jq, that is fine. Just please leave a TODO comment to revisit this and rationalize it with KA.


Comments from Reviewable

Also, wrap the script around a Makefile. And also provide a sample
config file to describe clusters.

The build script implements the following things:
1. Generates the required configs.
2. Builds the hyperkube binary and the corresponding docker image.
3. Pushes the image to a specified repository.
4. Pulls the federation installer docker images.
5. Builds the Kubernetes clusters described the config.json file.
6. Pushes the federation components to one of the Kubernetes clusters
   built in the previous step.
7. Also turns down the federation components and the Kubernetes
   clusters.
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2016
@madhusudancs
Copy link
Contributor Author

@mml Addressed review comments. PTAL.


Review status: 2 of 5 files reviewed at latest revision, 6 unresolved discussions.


federation/build.sh, line 74 [r3] (raw file):

Previously, mml (Matt Liggett) wrote…

Tab literals here (through the whole file). Not sure we have a style guide, but it seems to me that 2 blank spaces is what I've seen, except in go files.

Switched to two-spaces.

federation/README.md, line 12 [r2] (raw file):

Previously, mml (Matt Liggett) wrote…

The only thing I have to add is that we should do this however kubernetes anywhere does it. From a glance, it looks like they use jsonnet as a way to templatize json.

If you want to check this in with jq, that is fine. Just please leave a TODO comment to revisit this and rationalize it with KA.

KA also has `jq` dependency today. I have left a TODO anyway, saying we need to re-evaluate using `jq`.

Comments from Reviewable

@mml
Copy link
Contributor

mml commented Aug 12, 2016

:lgtm:


Reviewed 1 of 1 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


federation/README.md, line 12 [r2] (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

KA also has jq dependency today. I have left a TODO anyway, saying we need to re-evaluate using jq.

OK SGTM

Comments from Reviewable

@madhusudancs
Copy link
Contributor Author

@k8s-bot test this issue: #IGNORE

@k8s-bot
Copy link

k8s-bot commented Aug 12, 2016

GCE e2e build/test passed for commit f3c82af.

`config.default.json` file to describe your clusters and run

```shell
make build do=deploy
Copy link
Contributor

Choose a reason for hiding this comment

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

Is do=gen a required step or can I directly run deploy if I want to deploy release images.
Also how can I specify the image that I want to use for federation (if I want to use a release image)?

(Am just asking for my curiosity. Its fine to say we dont support it right now, if thats the case).

@nikhiljindal
Copy link
Contributor

My comments are all optional and mostly questions for my own curiosity. Dont need to block this PR on those.

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

GCE e2e build/test passed for commit f3c82af.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants