Feature/add pvc delete toggle

[Lukkie]

In our setup, the postgresql custom resource is often deleted. The operator will first delete the StatefulSets and then explicitly delete the PersistentVolumeClaims. However, we would like to keep our data. A simple solution is to add a toggle for this: .spec.volume.keepPVC, which defaults to false. This will keep all PVCs and Secrets (because otherwise credentials would become inconsistent).

I also did some minor changes for consistency:

• crd in helm chart and manifest directory are now identical
• Formatting
• Small change in developer.md

[FxKu]

Thanks @Lukkie for your contribution. We have it also on our radar, see #874. It just needs some more testing, if we really not delete volumes in some edge cases.

[Lukkie]

Thanks @Lukkie for your contribution. We have it also on our radar, see #874. It just needs some more testing, if we really not delete volumes in some edge cases.

Hi @FxKu
Am I correct to assume that PR #874 does not solve the same issue? In their case, the pg resource remains in the cluster and the number of replicas is set to zero. In my case, the pg resource is deleted entirely. Their PR doesn't modify anything in the deleteStatefulSet method in resources.go

[FxKu]

On a second look, yes it's rather the opposite. Sorry 😃

So this option is for protecting the volumes. I think, it could make more sense to have a global config option to keep PVCs, which you can overwrite in the Postgres manifest. What do you think?

[Lukkie]

That's fine for me. I'll try to implement this when I have some time.

[Lukkie]

@FxKu I added keep_pvc to config. It will be used as default value when KeepPVC is not assigned in the postgresql custom resource.

[FxKu]

Thanks for the update. I would like to see this renamed to enable_pvc_deletion, so that it follows the same pattern we use for other flag options. The default should be true to reflect the current behavior. Therefore, the datatype must be *bool in order to distinguish between false and not set (nil).

For the manifest option, I'm not sure if it should also be a boolean field enablePVCDeletion. In #1069 we are introducing a delete protection for the Postgres manifest where a user has to add the annotation with the cluster name. This makes it harder to accidentally delete the wrong resource. For example, if somebody else needs to approve the deletion, that person could notice that the name is wrong.

I could think of something similar here. To delete the PVC when it's globally disabled one has to add a field deletePVC under volume section and specify the name of the PVC (without the instance number):

spec:
  volume:
    deletePVC: "pgdata-acid-minimal-cluster"

What do you think about this? Also @Jan-M ?
Yes I know, now we simply delete PVCs and if the default for enable_pvc_deletion is true, why all the effort to make this safe? But it could be one step into the right direction.

[Lukkie]

@FxKu For my purposes, the code that I submitted suffices. However I acknowledge that it's important to design and implement this as good as possible. If you and your team agree on a proposal, let me know and I'll implement it.

[Lukkie]

Hi @FxKu
I implemented your suggestions. I think #1069 would have also fixed our particular issue, but can't hurt to add some extra features.

[menardorama]

Hi

Any chance to have this released ?

We would like to be abble to disable pvc deletion.

We find it to risky....

[jicki]

Hi

Any chance to have this released ?

We would like to be abble to disable pvc deletion.

We find it to risky....

[FxKu]

@jicki @menardorama we will put it on our list for v1.9.0. At the current state I still see way too many issues in this PR. Will close it for now. Sorry @Lukkie to not get back to you sooner. We can still have a discussion on how this feature should look like. After two years I do have better overview now.

[pratapagiri]

Any update on this PR?
We are looking for functionality to disable PVC deletion during uninstall.