Remove PVCs when removing StatefulSets nodes

[sebgl]

When we scale down or remove a StatefulSet, we don't remove associated PVCs of nodes that have been removed.
The StatefulSet controller itself does not remove PVCs either.

This can lead to confusing scenarios:

• scale down a cluster, then scale it up 3 months later: it will reuse 3-months old volumes
• remove a cluster, create it again with the same name: it will reuse the old cluster data

I think we should remove PVCs (I don't see why not), but not necessarily remove the associated PVs: leave that up to the storageClass reclaim policy.

[sebgl]

Related Kubernetes issue to have it part of the StatefulSet spec: kubernetes/kubernetes#55045

[sebgl]

We discussed making this an option of the Elasticsearch CRD:

apiVersion: elasticsearch.k8s.elastic.co/v1alpha1
kind: Elasticsearch
metadata:
  name: elasticsearch-sample
spec:
  version: 7.2.0
  nodes:
  - name: hot
    nodeCount: 3
  - name: warm
    nodeCount: 2
    deletePVC: false

We could distinguish downscales (eg. 3 -> 1 nodes) from StatefulSet removal (eg. 3 -> 0 nodes), but I think this setting would apply in both cases?

I also think this should be opt-out (default to removing PVCs).

[thbkrkr]

The current behavior of removing PVC when removing the pods/sset seems to me completely logical and conveninent so I am completely to keep it. Be able to opt-out would also be very nice.

[anyasabo]

I'm also for removing PVCs when we remove a pod, and perhaps working with upstream to integrate that as part of sset functionality.

Going by the linked issue, ssets do not delete the PVCs to make scaling up quicker. That's not very persuasive to me, but I don't have any data to support either. I doubt people will be scaling up and down often enough for that to be a significant benefit, especially when compared to the "surprise" of scaling down an ES cluster but still paying for the data.

My understanding is that we already have this behavior without stateful sets (when we delete a pod, we delete a PVC no questions asked), so there's also at least some precedent for this in our product.

I don't really feel strongly either way about being able to toggle this behavior in the ES spec. My inclination is to only delete the PVCs for now. If we end up wanting to be able to opt in or out in the future we can add a switch then.

If someone can clarify the current use case for leaving the data around but removing the nodes in SaaS (I think it was described as "pausing"?) I would appreciate it.

---

When we scale down or remove a StatefulSet, we don't remove associated PVCs of nodes that have been removed.
...
remove a cluster, create it again with the same name: it will reuse the old cluster data

For this point, we can and should set an owner ref on the PVC to the ES instance IMO. So that if the ES instance is deleted, the PVCs are as well. This does not help with scaling down pods on an existing instance though.

[pebrc]

I wonder though if our feeling that keeping the PVCs is annoying and hard to debug is due to the fact that we are in dev mode where we constantly spin up clusters with the same name and that this behaviour could turn out to be a feature in 'production' mode where you would want to err on the side of keeping data around? It can be useful if a user accidentally deletes a cluster and improves recovery over a restore from snapshot (if the user have snapshots)

I don't feel super strongly for keeping the PVCs though.

If we decide to go with deleting the PVCs by default despite this concern I think finding the right flag name/toggle is important.
deletePVC on the node level leaves me wondering what this actually does, should it be on the cluster level and affect all nodes to same extent. Why would you only want to delete parts of your data and keep other nodes around. Is that not what retention in ILM is for, why are we trying to recreate that here? So maybe a top-level retainPVConDelete or something like that?