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

Helm chart CRDs optional and not deleted on uninstall #684

Closed vholer closed 3 months ago

vholer commented 3 months ago

I was trying to deal with #682 to move CRDs from templates/generated/crds/ to crds/, but I found out that one CRD has configured conversion webhook and certificate, which are deployment/environment specific: https://github.com/cybozu-go/moco/blob/a683b05a3cc2692114743f4c3f71ab660d22109c/charts/moco/templates/generated/crds/moco_crds.yaml#L2073-L2074 Hmm... So I came with a different approach, which is used also in some other projects: leave it part of templates, but make it conditional and prevent Helm to remove it.

Similar approach can be seen in cert-manager or Rook, e.g.:

Changes:

masa213f commented 3 months ago

@vholer Thanks for the change. It looks good to me at first glance.

However, I have one point of concern about the crds.enabled. There was a little worrisome warning in the rook's chart description.

https://github.com/rook/rook/blob/v1.14.5/deploy/charts/rook-ceph/values.yaml#L17

WARNING Only set during first deployment. If later disabled the cluster may be DESTROYED.

Do you know the behavior if the crds.enabled is disabled later?

vholer commented 3 months ago

@masa213f Thank you for the comment.

IMHO the note in Rook is outdated. It was commited in 2021 (https://github.com/rook/rook/commit/5e86ef8b0c7e6f683f84bde0cf179d5686f2eadb), but later in 2022 they added safety annotations helm.sh/resource-policy: keep to avoid CRDs delete (https://github.com/rook/rook/commit/b0fc7c9b92314d6a119b27f135ae86eb4758d580#diff-135a2170f779ffda6b3904b9ee0d1520ebe5b6c58994ff29423a02602a60fb07R7) and forgot to update the note. The Rook's CRDs handling is fine, even though the doc tells otherwise, so as the Moco's will be.

Here is a demo of Helm chart installation/update/uninstall against the branch as a proove.

### Install with disabled CRDs
$ helm upgrade --install --create-namespace -n moco moco charts/moco/ --set crds.enabled=false
Release "moco" does not exist. Installing it now.
NAME: moco
LAST DEPLOYED: Fri May 31 19:23:07 2024
NAMESPACE: moco
STATUS: deployed
REVISION: 1
TEST SUITE: None
$ kubectl get crds | grep moco

### Enabled CRDs later
$ helm upgrade --install --create-namespace -n moco moco charts/moco/ --set crds.enabled=true
Release "moco" has been upgraded. Happy Helming!
NAME: moco
LAST DEPLOYED: Fri May 31 19:23:22 2024
NAMESPACE: moco
STATUS: deployed
REVISION: 2
TEST SUITE: None
$ kubectl get crds | grep moco
backuppolicies.moco.cybozu.com                        2024-05-31T17:23:24Z
mysqlclusters.moco.cybozu.com                         2024-05-31T17:23:24Z

### ✔ CRDs disabled, but left untouched in Kubernetes
$ helm upgrade --install --create-namespace -n moco moco charts/moco/ --set crds.enabled=false
Release "moco" has been upgraded. Happy Helming!
NAME: moco
LAST DEPLOYED: Fri May 31 19:23:32 2024
NAMESPACE: moco
STATUS: deployed
REVISION: 3
TEST SUITE: None
$ kubectl get crds | grep moco
backuppolicies.moco.cybozu.com                        2024-05-31T17:23:24Z
mysqlclusters.moco.cybozu.com                         2024-05-31T17:23:24Z

### ✔ CRDs not deleted even on Helm chart uninstall
$ helm uninstall -n moco moco
release "moco" uninstalled
$ kubectl get crds | grep moco
backuppolicies.moco.cybozu.com                        2024-05-31T17:23:24Z
mysqlclusters.moco.cybozu.com                         2024-05-31T17:23:24Z
masa213f commented 3 months ago

@vholer I see. Thanks for the confirmation! It looks fine.