Service-catalog: CRDs should not be uninstalled when `helm uninstall` is run

Created on 19 Dec 2019 · 9Comments · Source: kubernetes-sigs/service-catalog

From looking at the helm chart docs, it is standard practice that running helm uninstall does not delete the CRDs previously installed. Specifically saying:

Operators who want to upgrade or delete CRDs are encouraged to do this manually and with great care.

When the service catalog chart is combined with other charts that follow the standard practice, it leads to a strange mix of behaviours.

The questions we have are the following:

What is the reason behind implementing a CRD uninstall hook?
If there is a reason you believe this behaviour can be desirable would you be open to parameterising it so that charts which want to depend on service catalog can disable it?

cc @edwardecook

Thanks!

triagneeds-information

Source

gmrodgers

👍1

Most helpful comment

We have just discovered this feature, luckily in a temporary environment.

I'd be Interested to hear what the use case of keeping CRDs while uninstalling the controller for these CRDs actually is.

Let's say we have a working environment with multiple ServiceInstances running, and for whatever reason (helm release was damaged, tiller glitch, networking issue, helm release became unrecoverable), we have to helm delete service-catalog and install it again. Upon deletion, all CRDs get destroyed and existing ServiceInstances too.

We were testing out this specific scenario and found out all the resources gone.

I believe whatever tool was used to create certain entities should be also used to remove them, for the sake of consistency.

antonu17 on 18 Nov 2020

👍7

All 9 comments

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot on 18 Mar 2020

/remove-lifecycle stale

gmrodgers on 18 Mar 2020

Hi @gmrodgers
(sorry for such late response 🤦 )

Before CRDs we had the api-server with underlying etcd server. When it was uninstalled then all resources were also not available anymore (ServiceInstance, ServiceBindings etc).

We just wanted to have the same behavior with CRDs as we didn't want to have a breaking change.

Pros when removing CRDs and its instances:

you do not leak any resources which cannot be later managed
it's not misleading to the user as you do not leave any CRDs which cannot be used as backend(controller) was removed

But I can also imagine that leaving CRDs also has some benefits. But the worst thing is that you cannot say if you want to keep those resources when doing the helm delete action because it does not take any params. You need to take that decision while doing helm install which is another problem.

We can think about creating some ConfigMap with some value before doing helm delete and base on that we will know if delete or not the CRDs but it's making this process more complex.

When we think about removing CRDs we also have to think about it in conjunction with ServiceCatalog reinstallation. If you will not delete and install ServiceCatalog once again then it will fail because CRDs will be already on the cluster.

TBH I still do not know which behavior is better, delete or not. IMO we can make it optional but also take care of all aspects that I mentioned above.

@gmrodgers are you interested to create a proposal/POC?

We are definitely open for feedback and ideas :)

mszostok on 19 May 2020

I'd be interested to hear what the use case of keeping CRDs while uninstalling the controller for these CRDs actually is.

A possible way to allow implicit control over this behaviour could be to separate out the CRDs and controller into their own Helm charts and add both as child charts to a third.
Installing the child charts would support the "keep CRs around" use case while installing the parent chart would keep the current behaviour of deleting them.

From my point of view, we should not adapt the Helm 3 crds folder concept, as this offers no support at all for updating CRDs.

What we should think about is adopting Kubernetes 1.16s CRD versioning, so that we minimize the risk of breaking clients with changes in the CRDs.

loewenstein on 2 Jun 2020

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot on 31 Aug 2020

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot on 30 Sep 2020

This issue in question is the difference between helm charts for v2 and v3, as the v3 charts treat CRDs with more caution.
We may update our chart to use the new schema, as the old schema is no longer supported; but that is not the top priority.
Closing the issue.
/remove-lifecycle rotten
/close

jhvhs on 27 Oct 2020

@jhvhs: Closing this issue.

In response to this:

This issue in question is the difference between helm charts for v2 and v3, as the v3 charts treat CRDs with more caution.
We may update our chart to use the new schema, as the old schema is no longer supported; but that is not the top priority.
Closing the issue.
/remove-lifecycle rotten
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.