Skip to content
This repository has been archived by the owner on Nov 9, 2022. It is now read-only.

Delete PVCs of StatefulSets #50

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

timebertt
Copy link
Contributor

What this PR does / why we need it:
With this PR, grm deletes PVCs belonging to StatefulSets, if the ManagedResource has .spec.deletePersistentVolumeClaims=true, when the corresponding StatefulSet is removed from the ManagedResource or the respective ManagedResource is deleted.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
One thing, I am not so sure about, how to do it properly (feedback/ideas would be appreciated):

In order to be able to cleanup PVCs belonging to a StatefulSet, after it has been deleted, I am saving the StatefulSet before deleting it (we need .spec.selector from it for deleting the PVCs) here:
https://github.com/gardener/gardener-resource-manager/blob/8d73ce9bbabda106b24c5e5dc77efafffe3006fd/pkg/controller/managedresources/controller.go#L585-L595

However, if DELETE statefulset succeeds but any call during PVC cleanup fails, grm won't be able to delete the corresponding PVCs in the next retry, because it can't get the StatefulSet anymore and therefore doesn't know that it should still cleanup some PVCs.
Things, I have already considered:

  • retry cleanup calls: I think, this would be not a good idea, because it would block the whole reconciliation
  • save StatefulSet object in-memory: more implementation effort needed, problem would still persist, if grm panics / is evicted. Also, it can't be sure, that in the meanwhile the PVC is not reused again (i.e. a new StatefulSet might have been created after some backoff time) because object name != object identity and we only store the objects' names in the MR status, not the uid.
  • add the names of PVCs, that are still to be deleted to the MR status: problem with reuse by new/recreated StatefulSet from above persists.
  • add selector of StatefulSets to MR status during deletion: this speaks against, grm's generic way of handling different resources. What if we need an additional field for cleanup of a different resource kind...

Long story short, I have no solution/idea, how to properly deal with this scenario. Also, I am not sure, if it's even worth considering it, or if this feature should be rather seen as a "best-effort cleanup".

Release note:

The `ManagedResource` CRD features a new field `.spec.deletePersistentVolumeClaims`. If set to `true`, gardener-resource-manager will delete PVCs belonging to managed StatefulSets, when they are deleted.

@timebertt timebertt requested a review from a team as a code owner April 28, 2020 14:07
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 28, 2020
Copy link
Contributor

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long story short, I have no solution/idea, how to properly deal with this scenario. Also, I am not sure, if it's even worth considering it, or if this feature should be rather seen as a "best-effort cleanup".

Why not simply delaying the StatefulSet deletion and only do it if the corresponding PVCs have been deleted/have deletion timestamps?

pkg/apis/resources/v1alpha1/types.go Show resolved Hide resolved
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 29, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 29, 2020
@timebertt
Copy link
Contributor Author

Why not simply delaying the StatefulSet deletion and only do it if the corresponding PVCs have been deleted/have deletion timestamps?

Good question, it tried this before, but there was something bugging me about it before. But I gave it another thought and try, now it works perfectly :)
PTAL.

Copy link
Contributor

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@timebertt timebertt merged commit 8b3cb0b into gardener-attic:master Apr 30, 2020
@timebertt timebertt deleted the feature/delete-pvcs branch May 3, 2020 18:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants