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.

Sign up for GitHub

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/cleanup #962

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Refactor/cleanup #962

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
08ad314
Refactor findDcForDeletion, findReaperForDeletion and findStargateFor…
emerkle826 May 3, 2023
b16d82d
Refactor deleteDc
emerkle826 May 3, 2023
2d7eaec
simplify findForDelete functions
emerkle826 May 3, 2023
3c3c884
[fixup] fix remoteClient issues
emerkle826 May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
307 changes: 186 additions & 121 deletions controllers/k8ssandra/cleanup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,144 @@ func (r *K8ssandraClusterReconciler) checkDcDeletion(ctx context.Context, kc *ap
}

func (r *K8ssandraClusterReconciler) deleteDc(ctx context.Context, kc *api.K8ssandraCluster, dcName string, logger logr.Logger) result.ReconcileResult {

// delete Stargate
if result := r.deleteStargate(ctx, kc, dcName, logger); result != nil {
return result
}

// delete Reaper
if result := r.deleteReaper(ctx, kc, dcName, logger); result != nil {
return result
}

// delete the DC
if result := r.deleteDatacenter(ctx, kc, dcName, logger); result != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to improve naming here, there's a deleteDc and a deleteDatacenter and they do different things.
Maybe deleteDcComponents for the first one, and deleteCassandraDatacenter for the second (matches the name of the CR that it deletes)?

return result
}
delete(kc.Status.Datacenters, dcName)
logger.Info("DC deletion finished", "DC", dcName)
return result.Continue()
}

func (r *K8ssandraClusterReconciler) findStargateForDeletion(
ctx context.Context,
kcKey client.ObjectKey,
dcName string,
remoteClient client.Client) (*stargateapi.Stargate, error) {

if remoteClient == nil {
return nil, nil
}
stargateName := kcKey.Name + "-" + dcName + "-stargate"
selector := k8ssandralabels.PartOfLabels(kcKey)
options := &client.ListOptions{LabelSelector: labels.SelectorFromSet(selector)}
stargateList := &stargateapi.StargateList{}

err := remoteClient.List(ctx, stargateList, options)
if err != nil {
return nil, fmt.Errorf("failed to find Stargate (%s) for DC (%s) deletion: %v", stargateName, dcName, err)
}
for _, stargate := range stargateList.Items {
if stargate.Name == stargateName {
return &stargate, nil
}
}
Copy link
Contributor

@olim7t olim7t May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

(Also applies to the original code) Can't we do a simple lookup by key with remoteClient.Get() instead of this?
We just need the namespace but it should be the same as kcKey if I'm not mistaken.

return nil, nil
}

func (r *K8ssandraClusterReconciler) findReaperForDeletion(
ctx context.Context,
kcKey client.ObjectKey,
dcName string,
remoteClient client.Client) (*reaperapi.Reaper, error) {

if remoteClient == nil {
return nil, nil
}
reaperName := kcKey.Name + "-" + dcName + "-reaper"
selector := k8ssandralabels.PartOfLabels(kcKey)
options := &client.ListOptions{LabelSelector: labels.SelectorFromSet(selector)}
reaperList := &reaperapi.ReaperList{}

err := remoteClient.List(ctx, reaperList, options)
if err != nil {
return nil, fmt.Errorf("failed to find Reaper (%s) for DC (%s) deletion: %v", reaperName, dcName, err)
}
for _, reaper := range reaperList.Items {
if reaper.Name == reaperName {
return &reaper, nil
}
}
return nil, nil
}

func (r *K8ssandraClusterReconciler) findDcForDeletion(
ctx context.Context,
kcKey client.ObjectKey,
dcName string,
remoteClient client.Client) (*cassdcapi.CassandraDatacenter, error) {

if remoteClient == nil {
return nil, nil
}
selector := k8ssandralabels.PartOfLabels(kcKey)
options := &client.ListOptions{LabelSelector: labels.SelectorFromSet(selector)}
dcList := &cassdcapi.CassandraDatacenterList{}

err := remoteClient.List(ctx, dcList, options)
if err != nil {
return nil, fmt.Errorf("failed to find CassandraDatacenter (%s) for deletion: %v", dcName, err)
}

for _, dc := range dcList.Items {
if dc.Name == dcName {
return &dc, nil
}
}
return nil, nil
}

func (r *K8ssandraClusterReconciler) deleteK8ssandraConfigMaps(
ctx context.Context,
kc *k8ssandraapi.K8ssandraCluster,
dcTemplate k8ssandraapi.CassandraDatacenterTemplate,
namespace string,
remoteClient client.Client,
kcLogger logr.Logger,
) (hasErrors bool) {
selector := k8ssandralabels.PartOfLabels(client.ObjectKey{Namespace: kc.Namespace, Name: kc.SanitizedName()})
configMaps := &corev1.ConfigMapList{}
options := client.ListOptions{
Namespace: namespace,
LabelSelector: labels.SelectorFromSet(selector),
}
if err := remoteClient.List(ctx, configMaps, &options); err != nil {
kcLogger.Error(err, "Failed to list ConfigMap objects", "Context", dcTemplate.K8sContext)
return true
}
for _, rp := range configMaps.Items {
if err := remoteClient.Delete(ctx, &rp); err != nil {
key := client.ObjectKey{Namespace: namespace, Name: rp.Name}
if !apierrors.IsNotFound(err) {
kcLogger.Error(err, "Failed to delete configmap", "ConfigMap", key,
"Context", dcTemplate.K8sContext)
hasErrors = true
}
}
}
return
}

func (r *K8ssandraClusterReconciler) deleteStargate(ctx context.Context, kc *api.K8ssandraCluster, dcName string, logger logr.Logger) result.ReconcileResult {

kcKey := utils.GetKey(kc)
remoteClient, err := r.findRemoteClientForStargate(ctx, kcKey, dcName)
if err != nil {
return result.Error(err)
}

stargate, remoteClient, err := r.findStargateForDeletion(ctx, kcKey, dcName, nil)
stargate, err := r.findStargateForDeletion(ctx, kcKey, dcName, remoteClient)
if err != nil {
return result.Error(err)
}
Expand All @@ -166,8 +301,18 @@ func (r *K8ssandraClusterReconciler) deleteDc(ctx context.Context, kc *api.K8ssa
}
logger.Info("Deleted Stargate", "Stargate", utils.GetKey(stargate))
}
return nil
}

func (r *K8ssandraClusterReconciler) deleteReaper(ctx context.Context, kc *api.K8ssandraCluster, dcName string, logger logr.Logger) result.ReconcileResult {

kcKey := utils.GetKey(kc)
remoteClient, err := r.findRemoteClientForReaper(ctx, kcKey, dcName)
if err != nil {
return result.Error(err)
}

reaper, remoteClient, err := r.findReaperForDeletion(ctx, kcKey, dcName, remoteClient)
reaper, err := r.findReaperForDeletion(ctx, kcKey, dcName, remoteClient)
if err != nil {
return result.Error(err)
}
Expand All @@ -178,8 +323,18 @@ func (r *K8ssandraClusterReconciler) deleteDc(ctx context.Context, kc *api.K8ssa
}
logger.Info("Deleted Reaper", "Reaper", utils.GetKey(reaper))
}
return nil
}

func (r *K8ssandraClusterReconciler) deleteDatacenter(ctx context.Context, kc *api.K8ssandraCluster, dcName string, logger logr.Logger) result.ReconcileResult {

dc, remoteClient, err := r.findDcForDeletion(ctx, kcKey, dcName, remoteClient)
kcKey := utils.GetKey(kc)
remoteClient, err := r.findRemoteClientForDc(ctx, kcKey, dcName)
if err != nil {
return result.Error(err)
}

dc, err := r.findDcForDeletion(ctx, kcKey, dcName, remoteClient)
if err != nil {
return result.Error(err)
}
Expand All @@ -206,154 +361,64 @@ func (r *K8ssandraClusterReconciler) deleteDc(ctx context.Context, kc *api.K8ssa
// There is no need to requeue here. Reconciliation will be trigger by updates made by cass-operator.
return result.Done()
}

delete(kc.Status.Datacenters, dcName)
logger.Info("DC deletion finished", "DC", dcName)
return result.Continue()
return nil
}

func (r *K8ssandraClusterReconciler) findStargateForDeletion(
ctx context.Context,
kcKey client.ObjectKey,
dcName string,
remoteClient client.Client) (*stargateapi.Stargate, client.Client, error) {

func (r *K8ssandraClusterReconciler) findRemoteClientForReaper(ctx context.Context, kcKey client.ObjectKey, dcName string) (client.Client, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We know the remote client is going to be the same for all components.

In the original code, as soon as we found a client for Stargate or Reaper, it would get passed to the following methods instead of looking it up again. I think we should preserve that in some form.

In fact we can even improve the lookup, because the current approach is very inefficient. We don't need to look for the components to delete in every context, we already know the context because it's declared in the DC's K8sContext field.
The only problem is that we only have the DC name right now, but that's an easy fix:

  • in checkDcDeletion(), either modify GetDatacenterForDecommission so that it returns the full CassandraDatacenterTemplate, or re-iterate kc.Spec.Cassandra.Datacenters to look it up by name.
  • then look up the client: 
    remoteClient, err := r.ClientCache.GetRemoteClient(dcTemplate.K8sContext)
if err != nil { ... }
  • and finally pass it into deleteDc and its child methods.

That should allow us to remove all the findRemoteClient methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I had trouble figuring out this bit, and the old code was confusing. But this makes sense now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only problem is that we only have the DC name right now, but that's an easy fix:

  • in checkDcDeletion(), either modify GetDatacenterForDecommission so that it returns the full CassandraDatacenterTemplate, or re-iterate kc.Spec.Cassandra.Datacenters to look it up by name.

Actually it's not that easy, because the DC has already been removed from kc.Spec.Cassandra.Datacenters by the time we start the cleanup.

reaperName := kcKey.Name + "-" + dcName + "-reaper"
selector := k8ssandralabels.PartOfLabels(kcKey)
options := &client.ListOptions{LabelSelector: labels.SelectorFromSet(selector)}
stargateList := &stargateapi.StargateList{}
stargateName := kcKey.Name + "-" + dcName + "-stargate"
reaperList := &reaperapi.ReaperList{}

if remoteClient == nil {
for _, remoteClient := range r.ClientCache.GetAllClients() {
err := remoteClient.List(ctx, stargateList, options)
if err != nil {
return nil, nil, fmt.Errorf("failed to find Stargate (%s) for DC (%s) deletion: %v", stargateName, dcName, err)
}
for _, stargate := range stargateList.Items {
if stargate.Name == stargateName {
return &stargate, remoteClient, nil
}
}
}
} else {
err := remoteClient.List(ctx, stargateList, options)
for _, remoteClient := range r.ClientCache.GetAllClients() {
err := remoteClient.List(ctx, reaperList, options)
if err != nil {
return nil, nil, fmt.Errorf("failed to find Stargate (%s) for DC (%s) deletion: %v", stargateName, dcName, err)
return nil, fmt.Errorf("failed to find Reaper (%s) for DC (%s) deletion: %v", reaperName, dcName, err)
}

for _, stargate := range stargateList.Items {
if stargate.Name == stargateName {
return &stargate, remoteClient, nil
for _, reaper := range reaperList.Items {
if reaper.Name == reaperName {
return remoteClient, nil
}
}
}

return nil, nil, nil
return nil, nil
}

func (r *K8ssandraClusterReconciler) findReaperForDeletion(
ctx context.Context,
kcKey client.ObjectKey,
dcName string,
remoteClient client.Client) (*reaperapi.Reaper, client.Client, error) {

func (r *K8ssandraClusterReconciler) findRemoteClientForStargate(ctx context.Context, kcKey client.ObjectKey, dcName string) (client.Client, error) {
stargateName := kcKey.Name + "-" + dcName + "-stargate"
selector := k8ssandralabels.PartOfLabels(kcKey)
options := &client.ListOptions{LabelSelector: labels.SelectorFromSet(selector)}
reaperList := &reaperapi.ReaperList{}
reaperName := kcKey.Name + "-" + dcName + "-reaper"
stargateList := &stargateapi.StargateList{}

if remoteClient == nil {
for _, remoteClient := range r.ClientCache.GetAllClients() {
err := remoteClient.List(ctx, reaperList, options)
if err != nil {
return nil, nil, fmt.Errorf("failed to find Reaper (%s) for DC (%s) deletion: %v", reaperName, dcName, err)
}
for _, reaper := range reaperList.Items {
if reaper.Name == reaperName {
return &reaper, remoteClient, nil
}
}
}
} else {
err := remoteClient.List(ctx, reaperList, options)
for _, remoteClient := range r.ClientCache.GetAllClients() {
err := remoteClient.List(ctx, stargateList, options)
if err != nil {
return nil, nil, fmt.Errorf("failed to find Reaper (%s) for DC (%s) deletion: %v", reaperName, dcName, err)
return nil, fmt.Errorf("failed to find Stargate (%s) for DC (%s) deletion: %v", stargateName, dcName, err)
}

for _, reaper := range reaperList.Items {
if reaper.Name == reaperName {
return &reaper, remoteClient, nil
for _, stargate := range stargateList.Items {
if stargate.Name == stargateName {
return remoteClient, nil
}
}
}

return nil, nil, nil
return nil, nil
}

func (r *K8ssandraClusterReconciler) findDcForDeletion(
ctx context.Context,
kcKey client.ObjectKey,
dcName string,
remoteClient client.Client) (*cassdcapi.CassandraDatacenter, client.Client, error) {
func (r *K8ssandraClusterReconciler) findRemoteClientForDc(ctx context.Context, kcKey client.ObjectKey, dcName string) (client.Client, error) {
selector := k8ssandralabels.PartOfLabels(kcKey)
options := &client.ListOptions{LabelSelector: labels.SelectorFromSet(selector)}
dcList := &cassdcapi.CassandraDatacenterList{}

if remoteClient == nil {
for _, remoteClient := range r.ClientCache.GetAllClients() {
err := remoteClient.List(ctx, dcList, options)
if err != nil {
return nil, nil, fmt.Errorf("failed to CassandraDatacenter (%s) for DC (%s) deletion: %v", dcName, dcName, err)
}
for _, dc := range dcList.Items {
if dc.Name == dcName {
return &dc, remoteClient, nil
}
}
}
} else {
for _, remoteClient := range r.ClientCache.GetAllClients() {
err := remoteClient.List(ctx, dcList, options)
if err != nil {
return nil, nil, fmt.Errorf("failed to find CassandraDatacenter (%s) for deletion: %v", dcName, err)
return nil, fmt.Errorf("failed to find CassandraDatacenter (%s) for deletion: %v", dcName, err)
}

for _, dc := range dcList.Items {
if dc.Name == dcName {
return &dc, remoteClient, nil
for _, reaper := range dcList.Items {
if reaper.Name == dcName {
return remoteClient, nil
}
}
}

return nil, nil, nil
}

func (r *K8ssandraClusterReconciler) deleteK8ssandraConfigMaps(
ctx context.Context,
kc *k8ssandraapi.K8ssandraCluster,
dcTemplate k8ssandraapi.CassandraDatacenterTemplate,
namespace string,
remoteClient client.Client,
kcLogger logr.Logger,
) (hasErrors bool) {
selector := k8ssandralabels.PartOfLabels(client.ObjectKey{Namespace: kc.Namespace, Name: kc.SanitizedName()})
configMaps := &corev1.ConfigMapList{}
options := client.ListOptions{
Namespace: namespace,
LabelSelector: labels.SelectorFromSet(selector),
}
if err := remoteClient.List(ctx, configMaps, &options); err != nil {
kcLogger.Error(err, "Failed to list ConfigMap objects", "Context", dcTemplate.K8sContext)
return true
}
for _, rp := range configMaps.Items {
if err := remoteClient.Delete(ctx, &rp); err != nil {
key := client.ObjectKey{Namespace: namespace, Name: rp.Name}
if !apierrors.IsNotFound(err) {
kcLogger.Error(err, "Failed to delete configmap", "ConfigMap", key,
"Context", dcTemplate.K8sContext)
hasErrors = true
}
}
}
return
return nil, nil
}