Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lazy cleanup of PVC on Mysql cluster deletion #125

Closed
wants to merge 1 commit into from

Conversation

smanpathak
Copy link
Contributor

Initial fix #73 set ownership for pvcs to mysql CRD. Kubernetes deletes the pvcs along with the CRD using its garbage cleaning design. The pods belonging to the statefulset created by CRD are not immediately terminated. Kubernetes pods have certain grace period before deletion. As a result, the PVCs get deleted first and pods later.
This works well on GKE, but causes issues on other implementations. E.g. I used it on a cluster with Datera storage provider. Datera seems to keep the volume information in the PVCs. When the PVC is deleted along with Mysql CRD, it looses that information. So when kubelet tries to delete the pod, the volume cannot be disconnected.
This fix creates a cleanup worker like reconcile worker in operator to wait till pods are deleted before deleting the corresponding pvcs.

@AMecea
Copy link
Contributor

AMecea commented Oct 12, 2018

Hi @smanpathak, nice work! Interesting issue with Datera storage provided.

I've worked for several weeks at refactoring the operator code, we use kubebuilder as SDK for the operator. The code is much clearer now. Can you please make this PR to the kubebuilder branch? The kubebuilder will be merged into master next week and if we merge this into master we will lose this bug fix.

Also in the kubebuiler branch, the structure has changed we should investigate how this can be implemented. I'm thinking of using finalizers on PVC so those won't be deleted when the cluster is deleted just after a grace period, as described in this comment. What do you think?

@smanpathak
Copy link
Contributor Author

@AMecea thanks, I will look into porting the change over to kubebuilder branch. Here's my proposal --

  1. Set MySQL cluster/Statefulset as owner of PVCs. This is the only workable solution. Because pods come and go, they cannot be made owner of PVCs. Add finalizer to PVCs.
  2. Have the finalizer wait for (a) cleanupGracePeriod expiry (b) pod deletion. Once both conditions are met, PVCs can be deleted.
  3. I think this should work for scale up/scale down as well. I will test further.

@AMecea
Copy link
Contributor

AMecea commented Oct 12, 2018

This solution looks pretty good to me. You should find a way to implement a loop where to remove the finalizers that meet condition from 2. Take a look at how reconciliation is implemented in the new version, as a separated controller. In this refactoring, we focus on separation of concerns, and we give our best to make the code as clean as possible.

@calind what is your opinion about this solution that @smanpathak proposed?

@smanpathak
Copy link
Contributor Author

Looks like k8s has what we are trying to build in the operator: kubernetes/kubernetes#55824. It basically puts a finalizer on PVC and removes the finalizer after all pods referencing the volume are deleted, allowing the deletion to go through. It is disabled by default, though

@smanpathak
Copy link
Contributor Author

@AMecea given the new ability of kubernetes finalizer, I intend to

  1. add a finalizer to pvc
  2. once a mysql cluster is deleted, the timer starts
  3. upon timer expiry, the finalizer is removed and pvc deleted. Will post a review

@smanpathak
Copy link
Contributor Author

Closing in favor of: #134

@smanpathak smanpathak closed this Oct 17, 2018
@smanpathak smanpathak deleted the lazy-pvc-cleanup branch October 17, 2018 21:40
chapsuk pushed a commit to chapsuk/mysql-operator that referenced this pull request Oct 16, 2023
Signed-off-by: Anthony Yeh <enisoc@planetscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to delete PV/PVCs
2 participants