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

Refactor K8ssandraCluster deletion cleanup operation #992

Open
adejanovski opened this issue Jun 2, 2023 · 1 comment · May be fixed by #1022
Open

Refactor K8ssandraCluster deletion cleanup operation #992

adejanovski opened this issue Jun 2, 2023 · 1 comment · May be fixed by #1022
Assignees
Labels
needs-dod ready Issues in the state 'ready' refactoring

Comments

@adejanovski
Copy link
Contributor

adejanovski commented Jun 2, 2023

In controllers/k8ssandra/cleanup.go we delete all the objects that are tied to a deleted K8ssandraCluster object.
The deleteDeployments(), deleteServices() and deleteConfigMaps() could benefit from the following improvements:

  • Combine them into a generic function/method if possible
  • Move a few function arguments to the K8ssandraClusterReconciler object in order to reduce their number in the calls
  • Evaluate the possibility of returning a more meaningful object than a boolean, which could carry more information (such as an error or a ReconcileResult)
  • make use of DeleteAllOf, using the labels in the LabelSelector, instead of looping over objects to delete
  • Write unit tests for it
@adejanovski adejanovski mentioned this issue Jun 2, 2023
5 tasks
@adejanovski adejanovski added the ready Issues in the state 'ready' label Jun 14, 2023
@olim7t
Copy link
Contributor

olim7t commented Jul 14, 2023

Note: this is closely related to #961.

As I'm digging into the code I'm encountering more questions:

  • there are two main methods: checkDeletion which handles the deletion of the whole cluster, and checkDcDeletion which handles a single DC (if it's been removed from the cluster spec). However they don't share any code, which feels like a wasted opportunity for reuse.
  • case in point: checkDeletion removes Stargate, Reaper, deployments, services and configMaps. checkDcDeletion only removes Stargate and Reaper. Decommissioning a DC does indeed leave at least one configMap behind (tested on a very simple cluster, there's probably more in complex scenarios).
  • checkDcDeletion looks up the components by exact name. checkDeletion deletes every cluster sub-component in the current namespace. This works now but is something we'll need to keep in mind if we factor the code (two DCs can exist in the same namespace, so we can't reuse the shotgun approach for decommission).
  • the Stargate/Reaper deployments and services already use a controller reference for garbage collection. It looks like deleteDeployments and deleteServices were only added for Medusa. Why can't we use controller references there too?

In fact, I think we could generalize the use of controller references. As long as every remote object we create is owned by the CassandraDatacenter (directly or transitively), we shouldn't need to do anything beyond deleting the DC.
I'm going to experiment with that.

@olim7t olim7t linked a pull request Jul 14, 2023 that will close this issue
5 tasks
@adejanovski adejanovski added ready-for-review Issues in the state 'ready-for-review' in-progress Issues in the state 'in-progress' and removed ready Issues in the state 'ready' ready-for-review Issues in the state 'ready-for-review' labels Jul 17, 2023
@adejanovski adejanovski added ready Issues in the state 'ready' and removed in-progress Issues in the state 'in-progress' labels Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-dod ready Issues in the state 'ready' refactoring
Projects
Status: Ready
Development

Successfully merging a pull request may close this issue.

2 participants