search

cybozu-go / moco

MySQL operator on Kubernetes using GTID-based semi-synchronous replication.
https://cybozu-go.github.io/moco/
Apache License 2.0
269 stars 22 forks source link

CRDs in Helm chart should not be part of templates #682

Closed vholer closed 3 months ago

vholer commented 3 months ago

What

Currently, the CRDs for Moco are part of the templates (charts/moco/**templates**/generated/crds/) so they are always created on Helm chart install and (nearly) always deleted on Helm chart uninstall. If anybody uninstalls Helm chart by accident while there are some MySQLClusters, this might lead to the data loss - not immediately on uninstall due to finalizers not being able to proceed with stopped controller, but after Helm chart with controller is installed back again!!

Since CRDs are part of templates, Helm doesn't distinguish them from other objects. Because of that, one can't use Helm's option to skip CRDs installation (helm install --skip-crds) if handled different way or even show them. E.g.,

$ helm show crds moco/moco
$

How

Support for CRDs in Helm is not great, but the common practice (as described in documentation) is to place them into top-level directory crds/ within a chart, not into templates/.... This way they would be treated as CRDs and they could be skipped on installation (--skip-crds) or displayed on terminal.

I.e., move from charts/moco/templates/generated/crds/ into charts/moco/crds/.

Checklist

masa213f commented 3 months ago

@vholer Thank you for the report. Sounds good. Would you like to fix it on your end?

vholer commented 3 months ago

I was trying to do the change, but found a templated parts in CRDs, which make the move to crds/ impossible (CRDs in Helm are not templated, https://helm.sh/docs/topics/charts/#custom-resource-definitions-crds). So I came with a different approach in #684 , which at least improves current situation - installs the CRDs conditionally (so user can disable them in Helm chart and handle installation on their own if they want), but mainly never deletes them (to avoid data loss).

If the PR is merged, I think this issue can be closed (although not fixed the best way).

masa213f commented 3 months ago

@vholer Thank you for adding the excellent feature! I added caution about updating the chart. Please read on :) https://github.com/cybozu-go/moco/pull/685

If there are no other concerns, I would close this issue. Is it ok?

vholer commented 3 months ago

@masa213f LGTM

It's true that the case when upgrading from older version while having CRDs disabled would delete them (as part of cleanup of previous Helm chart version). Thanks for the catch, we can close this.

Thank you!