Helm integration #149
Helm integration #149
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
1 similar comment
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
|
Build Succeeded 👏 Build Id: 6d8f744c-575c-4088-ac6a-9e536c60d27f The following development artifacts have been built, and will exist for the next 30 days:
|
|
I signed it! |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
1 similar comment
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
This looks cool!!! I'll do a proper review after GDC this week. In the meantime, would be great to get this rebased down to a single commit (or you can do it post review, up to you) |
|
@markmandel fixed to a single commit |
|
Build Succeeded 👏 Build Id: a5fffbde-91ba-4a0c-8c93-8b9e147a3a59 The following development artifacts have been built, and will exist for the next 30 days:
|
|
@markmandel could you please setup integration with Travis CI or smth? Next step for Helm integration is create chart repository (I believe we can use github pages to publish charts or google cloud storage) and automate building process using CI. @fooock what do you think? |
There was a problem hiding this comment.
I'm really excited for this - looks great. I've yet to run it, but put down my initial thoughts on some feedback.
Question as well - so we're on the same page, what's the scope of this PR? I noticed there hasn't been any change to the root README.md and the build tooling also hasn't been adjusted, but the install.yaml has been removed from the root directory.
All this is fine, but it would be good to have a design on how this will roll out. Maybe in #101 or explicit description in the PR + Commit. WDYT?
build/build-image/Dockerfile
Outdated
| RUN curl -L ${HELM_URL} > /tmp/helm.tar.gz \ | ||
| && tar -zxvf /tmp/helm.tar.gz -C /tmp \ | ||
| && mv /tmp/linux-amd64/helm /usr/local/bin/helm \ | ||
| && chmod go+rx /usr/local/bin/helm |
There was a problem hiding this comment.
Minor thing - would be nice to delete the downloaded files after moving the helm - just to keep the build image layers nice and clean.
install/agones/Chart.yaml
Outdated
| @@ -0,0 +1,20 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
Curious - what's the canonical directory naming here - chart or install?
Looks like Istio uses install. Was trying to find some more examples, but couldn't find anything else also working with helm in a monorepo.
install/agones/README.md
Outdated
|
|
||
| - Kubernetes 1.9+ | ||
| - Role-based access controls (RBAC) activated | ||
| - MutatingAdmissionWebhook admission controller activated, see [recommendation](https://kubernetes.io/docs/admin/admission-controllers/#is-there-a-recommended-set-of-admission-controllers-to-use) |
There was a problem hiding this comment.
Would Helm be a prerequisite now as well?
There was a problem hiding this comment.
only for the Chart usage yes !
There was a problem hiding this comment.
Are we planning on having another install option other than the Helm chart? (I didn't think so, but wanted to check)
There was a problem hiding this comment.
@markmandel I will add more content in the prerequisite section.
Maybe another install option can be Terraform. It has a Kubernetes provider and support installation, upgrade and rollback operations. Another thing, Terraform doesn't install anything inside Kubernetes, whereas Helm need to install Tiller. WDYT?
There was a problem hiding this comment.
Not so sure about Terraform, but I would definitively add a line for helm prerequisites
install/agones/README.md
Outdated
| To install the chart with the release name `my-release`: | ||
|
|
||
| ```bash | ||
| $ helm install --name my-release agones |
There was a problem hiding this comment.
What directory does this command need to be run from? Do I need to run it from within install ?
(I haven't tried it yet, but it would be good to be explicit)
There was a problem hiding this comment.
The helm install command need to be run from install/ directory:
cd install/
helm install --name my-release agonesI will submit a patch to indicate it
install/agones/README.md
Outdated
|
|
||
| ## Configuration | ||
|
|
||
| The following tables lists the configurable parameters of the MySQL chart and their default values. |
There was a problem hiding this comment.
MySQL chart though my bad :/
install/agones/templates/crd.yaml
Outdated
| @@ -0,0 +1,113 @@ | |||
| apiVersion: apiextensions.k8s.io/v1beta1 | |||
There was a problem hiding this comment.
Since we're going to be making several more CRDs, shall we call this gameserver.yaml or maybe gameserver.yaml in a crds directory? WDYT?
| @@ -0,0 +1,72 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
Can we do subdirectories - have a serviceaccounts folder rather than prefixes?
I just feel like that would be more scalable as we get more code. WDYT?
install/agones/values.yaml
Outdated
| @@ -0,0 +1,29 @@ | |||
| # Default values for agones. | |||
| # This is a YAML-formatted file. | |||
There was a problem hiding this comment.
Is this comment redundant? 😆 since it is a yaml file 😄
|
I will rebase down to a single commit when the review is finished |
|
Build Succeeded 👏 Build Id: 72768ff4-2fb7-471a-9949-05a33eab9bbf The following development artifacts have been built, and will exist for the next 30 days:
|
|
Build Succeeded 👏 Build Id: 347fbb76-76fd-4b11-b163-3b9fabea48b6 The following development artifacts have been built, and will exist for the next 30 days:
|
|
Can we also update the page docs/installing_agones.md:
|
|
Build Succeeded 👏 Build Id: 7c7a484c-86f9-4186-947d-204100878d01 The following development artifacts have been built, and will exist for the next 30 days:
|
|
Build Succeeded 👏 Build Id: bd98c231-d751-424c-b990-93e8c32d2a60 The following development artifacts have been built, and will exist for the next 30 days:
|
There was a problem hiding this comment.
Took this for a spin, found some more feedback, and got an error - details in the next comment.
Another small thins - when installing, I saw the following at the bottom:
==> MISSING
KIND NAME
secrets agones-controller-secret
secrets agones-sdk-secret
Any idea what is causing that?
docs/installing_agones.md
Outdated
| @@ -17,6 +17,8 @@ In this quickstart, we will create a Kubernetes cluster, and populate it with th | |||
| 1. [Starting Minikube](#starting-minikube) | |||
| 1. [Enabling creation of RBAC resources](#enabling-creation-of-rbac-resources) | |||
| 1. [Installing Agones](#installing-agones) | |||
| 1. [Install with file](#install-with-file) | |||
There was a problem hiding this comment.
"Install with yaml" ? rather than "file" - feels a bit weird?
There was a problem hiding this comment.
ok, I will change it
docs/installing_agones.md
Outdated
| Finally, we install Agones to the cluster. | ||
| This will install Agones in your cluster. | ||
|
|
||
| ## Install with file |
install/agones/README.md
Outdated
|
|
||
| This chart install the Agones application and defines deployment on a [Kubernetes](http://kubernetes.io) cluster using the [Helm](https://helm.sh) package manager. | ||
|
|
||
| ## Prerequisites |
There was a problem hiding this comment.
This feel redundant - should we not just move the Helm dependency as "optional" in the install document? Also it means that the copy paste could be wrong.
Also, we should be explicit in what version(s) we are supporting.
There was a problem hiding this comment.
It makes sense to me. The document installing_agones.md points to this file if the user wants to install Agones using Helm.
You're right about the supported version. Currently I am doing this functionality using version 2.8.2 on the client and server. I will put this version in the document, but it would be good to try other versions, wdyt?
install/agones/README.md
Outdated
|
|
||
| To begin working with Helm, run the command: | ||
| ```bash | ||
| $ helm init |
There was a problem hiding this comment.
Isn't this the command to install helm?
I feel like this should be something more like:
If you don't have
Helminstalled locally, orTillerinstalled in your Kubernetes cluster, read the Using Helm documentation to get started.
There was a problem hiding this comment.
Yes, this command initialize Helm cli and install Tiller on the cluster, but you're right, I will change it
install/agones/Chart.yaml
Outdated
| @@ -0,0 +1,20 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
Does the directory have to be /install/agones or could it be /install/chart ? (Not sure if helm needs it to be "agones" so we can do helm install agones)
/install/agones/ feels kinda redundant to me, and doesn't actually explain what is contained within.
Since we have 2 install paths (yaml and chart), should it be /install/yaml' (with the install.yaml in it) and '/install/chart ?
Alternative option: rather than /install/chart /install/helm` - that's also quite explicit.
WDYT?
There was a problem hiding this comment.
If we want to execute the command helm install agones, the Chart.yaml file needs to live in the agones directory. In this case this folder is in install/.
I feel you're right, install/agones sound redundant, and doesn't explain what is contained in.
For me is a good idea to create 2 paths:
- For the yaml file:
install/yaml, and move theinstall.yamlfile to it - For the chart: For me is a good idea name it
install/helm, and put inside it theREADME.mdthat actually is inside the Agones folder. The result can be:
agones/
├── install/
│ ├── yaml/
| | ├── install.yaml
| ├── helm
| | ├── README.md
| | ├── agones/
| | | ├── Chart.yaml
| | | ├── values.yaml
| | | ├── ...
Using this directory structure, to install Agones:
$ cd install/helm
$ helm install agones
install/agones/README.md
Outdated
| $ helm install --name my-release -f values.yaml agones | ||
| ``` | ||
|
|
||
| > **Tip**: You can use the default [values.yaml](values.yaml) |
There was a problem hiding this comment.
At the end of the documentation, should we redirect them back to the "## Confirming Agones started successfully" section of docs/installing_agones.md ? otherwise they may not catch the "what to do next" section? WDYT?
install/agones/README.md
Outdated
| $ helm init | ||
| ``` | ||
|
|
||
| To install the chart with the release name `my-release`: |
There was a problem hiding this comment.
Do we need a section on updating releases? Or not for this PR?
There was a problem hiding this comment.
I think there is enough work on this PR.
build/build-image/Dockerfile
Outdated
| && mv /tmp/linux-amd64/helm /usr/local/bin/helm \ | ||
| && chmod go+rx /usr/local/bin/helm \ | ||
| && rm /tmp/helm.tar.gz && rm -rf /tmp/linux-amd64 | ||
|
|
There was a problem hiding this comment.
We should add helm completion to the shell:
RUN echo "source <(helm completion bash)" >> /root/.bashrc
| @@ -17,6 +17,8 @@ In this quickstart, we will create a Kubernetes cluster, and populate it with th | |||
| 1. [Starting Minikube](#starting-minikube) | |||
There was a problem hiding this comment.
Thought: should docs/installing_agones.md be the README.md in /install ?
There was a problem hiding this comment.
i don't think it's good idea move this file to install directory. With the new directory structure proposed above, it would not be necessary, but is my opinion, open to changes
There was a problem hiding this comment.
My thought is that (a) it would be contextual - i.e. the install folder has a README, and the content for the README, would be the contents of docs/intaling_agones.md
It will also (b) make release a tad cleaned - we can zip up the install dir and it has the docs, and the yaml and the chart and we are good to go for both pathways.
There was a problem hiding this comment.
I also think it's a good idea, though it would required to update the docs/intaling_agones.md to fit it new destination. Should we create a new issue for it ?
There was a problem hiding this comment.
I like this - let's create a separate issue for this, and track it in a seperate PR 😄 follow my own advice for smaller PRs
install/agones/Chart.yaml
Outdated
| @@ -0,0 +1,20 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
the make do-release will need to be changed so that it zips up the install dir and grabs both the install.yaml and the helm chart.
|
So took this for a spin, and my GameServer couldn't transition from {
"error": "could not retrieve GameServer default/cpp-simple-hlqhp: gameservers.stable.agones.dev \"cpp-simple-hlqhp\" is forbidden: User \"system:serviceaccount:default:agones-sdk\" cannot get gameservers.stable.agones.dev in the namespace \"default\"",
"level": "error",
"msg": "",
"obj": "RequestReady",
"queue": "stable.agones.dev.default.cpp-simple-hlqhp",
"source": "*gameservers.SDKServer",
"time": "2018-03-29T01:40:48Z"
}{
"level": "error",
"msg": "could not retrieve GameServer default/cpp-simple-hlqhp: gameservers.stable.agones.dev \"cpp-simple-hlqhp\" is forbidden: User \"system:serviceaccount:default:agones-sdk\" cannot get gameservers.stable.agones.dev in the namespace \"default\"",
"stack": [
"agones.dev/agones/pkg/gameservers.(*SDKServer).updateState\n\t/go/src/agones.dev/agones/pkg/gameservers/sdkserver.go:166",
"agones.dev/agones/pkg/gameservers.NewSDKServer.func3\n\t/go/src/agones.dev/agones/pkg/gameservers/sdkserver.go:119",
"agones.dev/agones/pkg/util/workerqueue.(*WorkerQueue).processNextWorkItem\n\t/go/src/agones.dev/agones/pkg/util/workerqueue/workerqueue.go:97",
"agones.dev/agones/pkg/util/workerqueue.(*WorkerQueue).runWorker\n\t/go/src/agones.dev/agones/pkg/util/workerqueue/workerqueue.go:73",
"agones.dev/agones/pkg/util/workerqueue.(*WorkerQueue).(agones.dev/agones/pkg/util/workerqueue.runWorker)-fm\n\t/go/src/agones.dev/agones/pkg/util/workerqueue/workerqueue.go:115",
"agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil.func1\n\t/go/src/agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133",
"agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil\n\t/go/src/agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:134",
"agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait.Until\n\t/go/src/agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:88",
"runtime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:2337"
],
"time": "2018-03-29T01:40:48Z"
}Looks like there is a RBAC misconfiguration somewhere. |
|
Build Succeeded 👏 Build Id: c8c47279-9f22-49fc-b005-ea2379556bc5 The following development artifacts have been built, and will exist for the next 30 days:
|
|
Build Succeeded 👏 Build Id: 4d9e177f-2635-44b9-a0ad-a6a769f5449c The following development artifacts have been built, and will exist for the next 30 days:
|
Scope of this PRAdd the ability to install Agones using the Helm package manager. For it, is necessary to maintain installation compatibility also through yaml, which is the currently supported mode. It would be a good option to keep the two forms of installation, since not everything people uses Helm, apart from the fact that the use of Helm adds a series of prerequisites to the installation of Agones (like install Tiller in the cluster). Also keep in mind that maintaining two versions can be a nightmare and potentially prone to errors. Why we need this?The install.yaml file is big, supports multiple configurations and It can grow over time. It would be interesting to be able to extract variables like names of services, namespace, port range etc. It is also interesting to be able to divide the install.yaml file into multiple parts, so that its edition is more simple. Pros of using Helm
Cons of using Helm
Things to considerApart from completing the documentation, we need to complete a series of requirements to complete this functionality. Before ship this we need to verify it (missing things?). Build toolsWe need to modify the CI system to be able to
For this process, we can make a Helm plugin (WDYT?). Also, we need to modify the Testing Helm chartsIt would be necessary to carry out a series of e2e tests to verify that the changes do not affect the performance of Agones when installed using Helm. An idea for this process would be to create a Vagrantfile that installs minikube and Helm, and perform a series of checks such as:
Add char to repositoryIt has been talked about in issue #153. At first, a good option would be to use Kubernetes charts repo, since It is the site that is normally used. To be able to upload the chart to this repository we must meet a series of technical requirements indicated in the following url: ConclusionWhen this functionality is finished, the user will be able to install Agones in two different ways
We provide a complete documentation of installing Agones with any of the two available methods. We will have an automatic e2e test system. The continuous integration system will be able to package the necessary files to be able to install Agones in any of the two ways, and have the same behavior. We will have a repository to upload the chart(s). Surely I forget something, opinions? |
|
So this feels like a lot of work for one PR - I think this is great scope for ALL of the packaging, but can we break this out into several PRs to be easier to test, integrate, review and work on in parallel? (I have thoughts on a few of the design above - and maybe that should be moved into #101 as the overarching ticket for all the PRs), but here is a breakdown of separate PR work that I think would make sense, rather than trying do it all in one go. Happily take opinions.
WDYT? Smaller pieces of work makes things easier. Mark |
|
I'm all in for smaller pieces. |
|
@markmandel perfect, really good idea, divide and conquer. I will submit a comment to issue #101 with a task list |
|
Build Succeeded 👏 Build Id: d3dfa054-ff56-47e7-9759-d60e1c1bd973 The following development artifacts have been built, and will exist for the next 30 days:
|
markmandel
added
area/build-tools
|
Build Succeeded 👏 Build Id: 646bf13a-d43c-46f8-a53b-697dc14943af The following development artifacts have been built, and will exist for the next 30 days:
|
There was a problem hiding this comment.
I still see this section when I install:
==> MISSING
KIND NAME
secrets agones-controller-secret
secrets agones-sdk-secret
Should I be concerned?
Full installation output:
root@markmandel2:/go/src/agones.dev/agones/install/helm# helm install --name agones ./agones/
NAME: agones
LAST DEPLOYED: Fri Apr 6 17:21:07 2018
NAMESPACE: default
STATUS: DEPLOYED
RESOURCES:
==> v1beta1/CustomResourceDefinition
NAME AGE
gameservers.stable.agones.dev 0s
==> v1/ClusterRole
agones-controller 0s
agones-sdk 0s
==> v1/ClusterRoleBinding
NAME AGE
agones-controller-access 0s
agones-sdk-access 0s
==> v1/Service
NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE
agones-controller-service ClusterIP 10.110.29.227 <none> 443/TCP 0s
==> v1/Namespace
NAME STATUS AGE
agones-system Active 0s
==> v1beta1/Deployment
NAME DESIRED CURRENT UP-TO-DATE AVAILABLE AGE
agones-controller 1 1 1 0 0s
==> v1beta1/MutatingWebhookConfiguration
NAME AGE
agones-mutation-webhook 0s
==> v1/Pod(related)
NAME READY STATUS RESTARTS AGE
agones-controller-6675c6b57d-fh8wz 0/1 ContainerCreating 0 0s
==> v1/ServiceAccount
NAME SECRETS AGE
agones-controller 2 0s
agones-sdk 2 0s
==> MISSING
KIND NAME
secrets agones-controller-secret
secrets agones-sdk-secret
And I'm still hitting the same issue as I had previously unfortunately.
| @@ -0,0 +1,80 @@ | |||
| # Install Agones using Helm | |||
There was a problem hiding this comment.
Almost a meta question: Do we want a standard header when we write documentation for features that are only currently available in development (which would then be removed when its time for release)? Or do we not care while we are pre-1.0? @Kuqd @enocom @dzlier-gcp WDYT?
There was a problem hiding this comment.
Like appending an UNRELEASED or DEV ONLY in parentheses?
There was a problem hiding this comment.
(ONLY AVAILABLE IN THE DEVELOPMENT BRANCH)
Or whatever. I don't actually mind what it is.
|
|
||
| ## Confirm Agones is running | ||
|
|
||
| To confirm Agones is up and running, [go to the next section](../../docs/installing_agones#confirming-agones-started-successfully) |
install/helm/README.md
Outdated
| To install the chart with the release name `my-release`: | ||
|
|
||
| ```bash | ||
| $ cd install/ |
There was a problem hiding this comment.
This should be cd instal/helm - since we moved things.
build/build-image/Dockerfile
Outdated
| @@ -42,6 +42,16 @@ ENV PATH /usr/local/go/bin:/go/bin:/opt/google-cloud-sdk/bin:$PATH | |||
| RUN gcloud components update && gcloud components install kubectl | |||
| RUN echo "source <(kubectl completion bash)" >> /root/.bashrc | |||
|
|
|||
| # install Helm package manager | |||
| ENV HELM_VER 2.8.0 | |||
There was a problem hiding this comment.
This should be updated to 2.8.2
fooock
force-pushed
the
master
branch
2 times, most recently
from
1fc20c3 to
130d8de
Compare
| @@ -0,0 +1,29 @@ | |||
| apiVersion: admissionregistration.k8s.io/v1beta1 | |||
There was a problem hiding this comment.
We also need the ValidatingWebhookConfiguration
There was a problem hiding this comment.
Actually, we don't right now. I lie - we can do this when we integrate into the build tooling.
|
Build Succeeded 👏 Build Id: 827f3161-0622-4fbc-bdd1-ee542d7e1163 The following development artifacts have been built, and will exist for the next 30 days:
|
|
Build Succeeded 👏 Build Id: 914ce56a-b3c4-4f68-ae89-4275eefa84b5 The following development artifacts have been built, and will exist for the next 30 days:
|
Now you can deploy agones using ```bash helm install --name my-release agones ``` Note that the build-image Dockerfile was modified to download Helm and add it to the path For more info see the documentation located at `install/helm/README.md`
|
Build Succeeded 👏 Build Id: 3acb8527-01ec-47df-a9dc-a469532eaaca The following development artifacts have been built, and will exist for the next 30 days:
|
|
Build Succeeded 👏 Build Id: 91f99f29-d9a5-46e6-b82c-ab4e7124e082 The following development artifacts have been built, and will exist for the next 30 days:
|
|
Build Succeeded 👏 Build Id: f906c34a-d7b9-4907-8bd7-641a3998e518 The following development artifacts have been built, and will exist for the next 30 days:
|
|
Build Succeeded 👏 Build Id: 758323fe-ca4b-43b8-bcea-d25b310aa242 The following development artifacts have been built, and will exist for the next 30 days:
|
|
Build Succeeded 👏 Build Id: cabefac0-4e4d-4e55-a19d-394db4e21c22 The following development artifacts have been built, and will exist for the next 30 days:
|
Related to #101