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

Install app-repository CRD via a hook #813

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

migmartri
Copy link
Contributor

@migmartri migmartri commented Nov 14, 2018

This patch basically moves the creation of the CRD to the hook lifecycle so Helm will not try to upgrade it causing the issue described here #812

I manually tested it

# No CRDs installed in my cluster
$ k get crd
No resources found.

# Install the first instance of kubeapps in namespace-1
$ helm install -n kubeapps-1 --namespace namespace-1 chart/kubeapps/

# The apprepositories.kubeapps.com crd is now "installed" and belongs to kubeapps-1
$ k get crd apprepositories.kubeapps.com -o jsonpath={.metadata.labels.release}
kubeapps-1

# Install the second instance of kubeapps in namespace-2
$ helm install -n kubeapps-2 --namespace namespace-2 chart/kubeapps/

# And now I got two releases installed
$ helm list
NAME      	REVISION	UPDATED                 	STATUS  	CHART         	NAMESPACE  
kubeapps-1	1       	Tue Nov 13 15:56:02 2018	DEPLOYED	kubeapps-0.9.7	namespace-1
kubeapps-2	1       	Tue Nov 13 15:56:44 2018	DEPLOYED	kubeapps-0.9.7	namespace-2

# The apprepositories.kubeapps.com crd still belongs to kubeapps-1 as expected
$ k get crd apprepositories.kubeapps.com -o jsonpath={.metadata.labels.release}
kubeapps-1

# Upgrading kubeapps-1 works
$ helm upgrade kubeapps-1 chart/kubeapps/
=> OK

# ====> Before this patch, here is where it used to fail

# kubeapps-2 upgrades successfully 
$ helm upgrade kubeapps-2 chart/kubeapps/
=> OK

# I can also delete the CRD before upgrading kubeapps-2 and this release will create the required CRDs
$ k delete crd apprepositories.kubeapps.com
$ helm upgrade kubeapps-2 chart/kubeapps/
=> OK
$ k get crd apprepositories.kubeapps.com -o jsonpath={.metadata.labels.release}
kubeapps-2

# Upgrading kubeapps-1 works fine as expected too
$ helm upgrade kubeapps-1 chart/kubeapps/
=> OK

The main issue about this approach is that this way we are not "updating CRDs" so any change in the chart that affects those CRDs will not be applied. We should do a follow up task that could kubectl apply the changes by using a job.

Fixes #812

@migmartri migmartri requested a review from prydonius November 14, 2018 01:05
@migmartri
Copy link
Contributor Author

The main issue about this approach is that this way we are not "updating CRDs" so any change in the chart that affects those CRDs will not be applied. We should do a follow up task that could kubectl apply the changes by using a job.

Actually, I believe that this was the case before we did the change to also keep the resource on upgrades #801 (superseded by this PR)

This is a follow up issue #814

# The condition above will be true if another instance of Kubeapps is
# already installed
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
name: apprepositories.kubeapps.com
annotations:
"helm.sh/resource-policy": keep
Copy link
Contributor

Choose a reason for hiding this comment

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

if I remember correctly, if it's installed with a hook it won't be deleted by helm, isn't it?

Copy link
Contributor Author

@migmartri migmartri Nov 14, 2018

Choose a reason for hiding this comment

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

It wont be deleted no.

From their docs:

Practically speaking, this means that if you create resources in a hook, you cannot rely upon helm delete to remove the resources. To destroy such resources, you need to either write code to perform this operation in a pre-delete or post-delete hook or add "helm.sh/hook-delete-policy" annotation to the hook template file.

@migmartri migmartri merged commit 1184726 into vmware-tanzu:master Nov 14, 2018
@migmartri migmartri deleted the 812-crd-issue branch November 14, 2018 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants