This PR adds support for having helm chart of foundationDB #59

viveksinghggits · 2019-12-24T09:39:26Z

Fixes #49

To test the chart please follow below commands

# create namespace 
kubectl create ns fdb-test

# create operator
helm install fdb-operator helm/fdb-operator -n fdb-test

Once we have have the operator running we can go ahead and create the foundation db instance using below command

helm install fdb-instance helm/fdb-instance -n fdb-test

and you will have foundationDB pod running in the fdb-test namespace with double redundancy mode.

Note
we will have to push the operator image to a public repository so that the image name can be used in the values.yaml for fdb-operator chart.

brownleej · 2019-12-29T02:47:27Z

Sorry for the delay. Apple's on holiday from the 24th through the 1st, so I haven't been looking at PRs. I'll try to take a look at this when I get back next week.

brownleej · 2020-01-02T15:05:00Z

helm/fdb-instance/templates/local_cluster.yaml

The local_cluster file is designed for local development, so I don't think we want to use it as part of a helm chart. Most of the things that are defined in this sample are things that aren't as likely to be good fits for real clusters. In general, I'm not sure how much value there is having a helm chart for the CRD, rather than just having the customization go through the CRD.

Hi @brownleej
It totally depends on us, is we want to get the CR,which in our case is a FoundationDBCluster, created as part of helm or not. By in my experience some database use this approach where they install the operator through one helm installation and actual database (CR) through another.
What I suggest is to install the CR using second helm deployment and improve that for real use. WDYT

I think we should leave the creation of FDB clusters out of this PR, and focus it on deploying the operator itself.

brownleej · 2020-01-02T15:06:07Z

helm/fdb-operator/templates/crds/foundationdbcluster.yaml

Will this file updated automatically when we make changes to the CRD?

I am also not an expert here but I think we will have to make the changes manually.

In all the other places we use it, we generate the CRD from the code. Is there a reason we can't do that with Helm? I'd rather not introduce any way for things to get out of sync if we can avoid it.

brownleej · 2020-01-02T15:07:15Z

helm/fdb-operator/templates/manager/manager.yaml

Is it the best practice to create a namespace when deploying things through Helm, or is this an artifact of the defaults from kubebuilder?

If we talk specifically this namespace creation its not required.

OK, then let's leave the namespace out. I think it's better to give people the flexibility to run the operator in whatever namespace makes sense for them.

brownleej · 2020-01-02T15:08:12Z

helm/fdb-operator/templates/manager/secret.yaml

Are we doing anything with this webhook secret? I think it came from the defaults in kubebuilder, but we don't have any webhooks right now. We may add some in the future, though.

I agree, I just tried to make helm chart of whatever was already present, if we dont use this, I think it can be removed.

brownleej · 2020-01-02T15:08:59Z

helm/fdb-operator/templates/manager/statefulset.yaml

The fdb-certs secret is part of the local development environment, but I don't think we want it in the helm chart.

if you see the statefulset that gets created for the operator, manifest of that statefulset has this secret mounted, so I just made that more manageable but having the name in the values.yaml file, but it was already in the statefulset's manifest.

We should base this on the latest sample deployment YAML (https://github.com/FoundationDB/fdb-kubernetes-operator/blob/master/config/samples/deployment.yaml) after the Kubebuilder 2 upgrade.

brownleej · 2020-01-02T15:10:11Z

helm/fdb-operator/templates/manager/statefulset.yaml

This shouldn't be necessary outside of local development environments.

a comment above addresses this.

brownleej · 2020-01-02T15:11:15Z

helm/fdb-operator/templates/rbac/rbac_role.yaml

I think it's better to use a role rather than a cluster role, to support using the operator in environments where the person running the operator doesn't have cluster-wide access.

I agree, we should have role in place of ClusterRole here.

viveksinghggits · 2020-01-13T06:53:24Z

Hi @brownleej
can you please look into/address the comments.

brownleej · 2020-01-15T16:51:31Z

helm/fdb-instance/templates/local_cluster.yaml

I think we should leave the creation of FDB clusters out of this PR, and focus it on deploying the operator itself.

brownleej · 2020-01-15T16:52:24Z

helm/fdb-operator/templates/manager/manager.yaml

OK, then let's leave the namespace out. I think it's better to give people the flexibility to run the operator in whatever namespace makes sense for them.

viveksinghggits · 2020-01-23T05:29:38Z

Hi @brownleej
I've been a bit busy, I will try to push the changes today.

2. after installing the operator you will hvae to create the cluster cr in the samenamespace where you deployed the operatgor

viveksinghggits · 2020-01-23T13:53:50Z

Hi @brownleej
I have made the changes that were about removing the system namespace and the fdb-instance chart.
However I suggest that we should have instance chart here as well.

brownleej · 2020-01-23T18:20:03Z

What problem does the instance chart solve?

viveksinghggits · 2020-01-24T06:36:29Z

What problem does the instance chart solve?

Hi @brownleej
So, the operator-chart creates the CRD and the instance chart actually creates a CR that run the foundationdb cluster in double redundancy mode. So if we don't deploy the instance chart we wont have foundationdb cluster up and running. we will anyway have to deploy the instance chart separately if we dont include that in this repo with operator chart.

brownleej · 2020-01-24T17:48:39Z

What is the advantage of creating the cluster through Helm rather than directly?

viveksinghggits · 2020-01-25T06:49:35Z

What is the advantage of creating the cluster through Helm rather than directly?

Hi @brownleej,
Here is one of the many reasons why we use helm or any other package manager to deploy the resources on Kubernetes-
Right now if we see the resource manifest for the CR FoundationDBCluster that spins up the FoundationDB cluster we dont actually know which redundancy mode the cluster will be started in. Even if we know, and want to change that we will have to checkout that file and then make some modifications in that file to get the redundancy mode reflected.
Now if we had created the same resource using helm, we will read the redundancy_mode from values.yaml file and we can change any value in values.yaml file using --set flag while installing helm chart.

viveksinghggits · 2020-01-31T08:08:02Z

Hi @brownleej
Did you get time to look into the comments.?

brownleej · 2020-02-08T02:06:17Z

I see you've removed the instance chart; I had missed that before. That clears up my biggest area of concern. There are still some open questions and suggestions in the PR that I would like to see resolved. A lot of that comes down to bringing this in line with other things that have changed since you opened the PR, such as moving to a deployment rather than a statefulset for the operator and removing defaults that Kubebuilder added that we're not using now. I also have a question about how the CRD in the helm chart gets defined.

1. Change statefulset to deployment 2. remove unnecessary resources 3. improve helm install success message

viveksinghggits · 2020-02-12T13:13:51Z

Hi @brownleej,
I have addressed the comment, can you please have another look.
This is how we can install the chart

~   helm install fdb helm/fdb-operator/    

NAME: fdb
LAST DEPLOYED: Wed Feb 12 18:25:11 2020
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
FoundationDB operator has been installed successfully.

To see the logs of the operator you can use below command

kubectl logs deployment/fdb-kubernetes-operator-controller-manager -n <namespace-name> -f

brownleej · 2020-02-13T21:45:37Z

@viveksinghggits What are your thoughts on generating the CRD in the helm chart from the code so it automatically updates?

viveksinghggits · 2020-02-14T07:01:30Z

Hey @brownleej,
If I understood you correctly, you want the CRD manifest that is there as part of helm chart to be updated automatically according to the code change. I am not very familiar how we can achieve that, per my understanding as of now, we can configure the helm chart manifest using only the Values.yaml files. But let me ask around my team once and update.

Update
Hey @brownleej
So, I read about/asked around and it seems we can not actually have CRD automatically getting changed base on code that we have. The only thing we can use to make CRD/CR configuration dynamic is Values/Chart.yaml (templating) files but those are not going to help us in the CRDs case because CRDs dont support templating in helm.

brownleej · 2020-02-18T21:21:14Z

Why can't we have a step in the Makefile that updates the copy of the CRD in the Helm chart?

viveksinghggits · 2020-02-19T06:55:26Z

Hi @brownleej,
The reason why we want to introduce helm chart is to not have anything other than

helm install fdb helm/fdb-operator/

if we are including Makfile in between, we are not actually resolving the problem that we are trying to resolve by introducing Helm installation.

On the other note, can you please explain what do you actually mean by update the copy of CRD, because in that also we will have to have the CRD somewhere right?

brownleej · 2020-02-19T17:18:36Z

What I mean is that when the CRD changes, like when we add a new field, we should update the copy in the helm chart in addition to updating the copy in config/crd.

viveksinghggits · 2020-02-20T10:16:58Z

Hey @brownleej,
What I suggest to do is store that CRD at only one place, inside helm/fdb-operator/templates/crds. Will that be problem in any way?

brownleej · 2020-02-20T16:31:26Z

I suspect it will be a problem, since it means removing the CRD from the standard location in config/crd.

viveksinghggits · 2020-02-24T06:52:12Z

Hey @brownleej,
I have created a soft link to the CRD file that is in the config dir, now even if we change the content of the file in the cofig dir, helm would be updated.

We will have to discuss about two things now, how do we host this helm chart, one option I know id github pages.
How are we going to support the CR installation.

brownleej

👍

brownleej · 2020-02-27T20:07:52Z

This looks good. I think the best course is to merge this in as it is and file smaller-scoped issues for the things we don't have in this PR.

viveksinghggits · 2020-02-28T09:47:09Z

Hey @brownleej,
The first thing I think we should focus on host this chart. We can easily use github pages to host this chart.

This was referenced Dec 24, 2019

Add chart for foundationDB kanisterio/kanister#465

Merged

Provide a way to install the operator in non default namespaces #55

Closed

brownleej self-assigned this Dec 29, 2019

brownleej requested changes Jan 2, 2020

View reviewed changes

brownleej requested changes Jan 18, 2020

View reviewed changes

viveksinghggits added 3 commits January 23, 2020 18:58

1. Operator can now be installed using helm

c3e683a

2. after installing the operator you will hvae to create the cluster cr in the samenamespace where you deployed the operatgor

Add chart support

418009d

Read name from release name

a4f9222

viveksinghggits force-pushed the master branch from 176691c to a4f9222 Compare January 23, 2020 13:31

viveksinghggits added 2 commits January 23, 2020 19:07

Remove system ns and instance chart

396bc08

Correct the foundationdb image

1576769

Address review comments

494c9e8

1. Change statefulset to deployment 2. remove unnecessary resources 3. improve helm install success message

Make link for the CRD to config dir

e72f67f

brownleej approved these changes Feb 27, 2020

View reviewed changes

brownleej merged commit 1d3f43c into FoundationDB:master Feb 27, 2020

viveksinghggits mentioned this pull request Mar 11, 2020

Host helm chart into a repository #163

Closed

This PR adds support for having helm chart of foundationDB #59

This PR adds support for having helm chart of foundationDB #59

Uh oh!

Conversation

viveksinghggits commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brownleej commented Dec 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viveksinghggits commented Jan 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viveksinghggits commented Jan 23, 2020

Uh oh!

viveksinghggits commented Jan 23, 2020

Uh oh!

brownleej commented Jan 23, 2020

Uh oh!

viveksinghggits commented Jan 24, 2020

Uh oh!

brownleej commented Jan 24, 2020

Uh oh!

viveksinghggits commented Jan 25, 2020

Uh oh!

viveksinghggits commented Jan 31, 2020

Uh oh!

brownleej commented Feb 8, 2020

Uh oh!

viveksinghggits commented Feb 12, 2020

Uh oh!

brownleej commented Feb 13, 2020

Uh oh!

viveksinghggits commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brownleej commented Feb 18, 2020

Uh oh!

viveksinghggits commented Feb 19, 2020

Uh oh!

brownleej commented Feb 19, 2020

Uh oh!

viveksinghggits commented Dec 24, 2019 •

edited

Loading

viveksinghggits commented Feb 14, 2020 •

edited

Loading