Merge pull request #125 from numtide/fix/native-gc-migration-and-coverage

fix(gc): replace manual finalizers with native k8s cascading deletion

Author: fernando-villalba

pkg/cluster-handler/controller/multigrescluster/integration_test.go

@@ -5,7 +5,6 @@ package multigrescluster_test
 
 import (
 	"path/filepath"
-	"slices"
 	"testing"
 	"time"
 
@@ -671,14 +670,6 @@ func TestMultigresCluster_HappyPath(t *testing.T) {
 				t.Errorf("Resources mismatch:\n%v", err)
 			}
 
-			// Check Finalizer
-			fetchedCluster := &multigresv1alpha1.MultigresCluster{}
-			if err := k8sClient.Get(t.Context(), client.ObjectKeyFromObject(tc.cluster), fetchedCluster); err != nil {
-				t.Fatalf("Failed to get cluster: %v", err)
-			}
-			if !slices.Contains(fetchedCluster.Finalizers, "multigres.com/finalizer") {
-				t.Errorf("Expected finalizer 'multigres.com/finalizer' to be present, got %v", fetchedCluster.Finalizers)
-			}
 		})
 	}
 }

pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller.go

@@ -12,17 +12,12 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1"
 	"github.com/numtide/multigres-operator/pkg/resolver"
 )
 
-const (
-	finalizerName = "multigres.com/finalizer"
-)
-
 // MultigresClusterReconciler reconciles a MultigresCluster object.
 type MultigresClusterReconciler struct {
 	client.Client
@@ -54,18 +49,6 @@ func (r *MultigresClusterReconciler) Reconcile(
 		return ctrl.Result{}, fmt.Errorf("failed to get MultigresCluster: %w", err)
 	}
 
-	if !cluster.DeletionTimestamp.IsZero() {
-		return r.handleDelete(ctx, cluster)
-	}
-
-	if !controllerutil.ContainsFinalizer(cluster, finalizerName) {
-		controllerutil.AddFinalizer(cluster, finalizerName)
-		if err := r.Update(ctx, cluster); err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to add finalizer: %w", err)
-		}
-		return ctrl.Result{}, nil
-	}
-
 	res := resolver.NewResolver(r.Client, cluster.Namespace, cluster.Spec.TemplateDefaults)
 
 	// Apply defaults (in-memory) to ensure we have images/configs/system-catalog even if webhook didn't run.
@@ -74,6 +57,11 @@ func (r *MultigresClusterReconciler) Reconcile(
 		return ctrl.Result{}, err
 	}
 
+	// If being deleted, let Kubernetes GC handle cleanup
+	if !cluster.DeletionTimestamp.IsZero() {
+		return ctrl.Result{}, nil
+	}
+
 	if err := r.reconcileGlobalComponents(ctx, cluster, res); err != nil {
 		l.Error(err, "Failed to reconcile global components")
 		return ctrl.Result{}, err
@@ -97,100 +85,6 @@ func (r *MultigresClusterReconciler) Reconcile(
 	return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil
 }
 
-func (r *MultigresClusterReconciler) handleDelete(
-	ctx context.Context,
-	cluster *multigresv1alpha1.MultigresCluster,
-) (ctrl.Result, error) {
-	if controllerutil.ContainsFinalizer(cluster, finalizerName) {
-		if err := r.checkChildrenDeleted(ctx, cluster); err != nil {
-			// If we are waiting for children, emit an event so the user knows why it's stuck in Terminating
-			r.Recorder.Event(
-				cluster,
-				"Normal",
-				"Cleanup",
-				"Waiting for child resources (Cells/TableGroups) to be deleted",
-			)
-			return ctrl.Result{}, err
-		}
-		controllerutil.RemoveFinalizer(cluster, finalizerName)
-		if err := r.Update(ctx, cluster); err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to remove finalizer: %w", err)
-		}
-	}
-	return ctrl.Result{}, nil
-}
-
-func (r *MultigresClusterReconciler) checkChildrenDeleted(
-	ctx context.Context,
-	cluster *multigresv1alpha1.MultigresCluster,
-) error {
-	childrenStillExist := false
-
-	// Helper to delete a list of objects
-	deleteObjects := func(objects []client.Object) error {
-		for _, obj := range objects {
-			if obj.GetDeletionTimestamp().IsZero() {
-				if err := r.Delete(ctx, obj); err != nil {
-					if !errors.IsNotFound(err) {
-						return fmt.Errorf("failed to delete child %s: %w", obj.GetName(), err)
-					}
-				}
-			}
-		}
-		if len(objects) > 0 {
-			childrenStillExist = true
-		}
-		return nil
-	}
-
-	// 1. Check Cells
-	cells := &multigresv1alpha1.CellList{}
-	if err := r.List(ctx, cells, client.InNamespace(cluster.Namespace), client.MatchingLabels{"multigres.com/cluster": cluster.Name}); err != nil {
-		return fmt.Errorf("failed to list cells: %w", err)
-	}
-	cellObjs := make([]client.Object, len(cells.Items))
-	for i := range cells.Items {
-		cellObjs[i] = &cells.Items[i]
-	}
-	if err := deleteObjects(cellObjs); err != nil {
-		return err
-	}
-
-	// 2. Check TableGroups
-	tgs := &multigresv1alpha1.TableGroupList{}
-	if err := r.List(ctx, tgs, client.InNamespace(cluster.Namespace), client.MatchingLabels{"multigres.com/cluster": cluster.Name}); err != nil {
-		return fmt.Errorf("failed to list tablegroups: %w", err)
-	}
-	tgObjs := make([]client.Object, len(tgs.Items))
-	for i := range tgs.Items {
-		tgObjs[i] = &tgs.Items[i]
-	}
-	if err := deleteObjects(tgObjs); err != nil {
-		return err
-	}
-
-	// 3. Check TopoServers
-	ts := &multigresv1alpha1.TopoServerList{}
-	if err := r.List(ctx, ts, client.InNamespace(cluster.Namespace), client.MatchingLabels{"multigres.com/cluster": cluster.Name}); err != nil {
-		return fmt.Errorf("failed to list toposervers: %w", err)
-	}
-	tsObjs := make([]client.Object, len(ts.Items))
-	for i := range ts.Items {
-		tsObjs[i] = &ts.Items[i]
-	}
-	if err := deleteObjects(tsObjs); err != nil {
-		return err
-	}
-
-	if childrenStillExist {
-		return fmt.Errorf(
-			"waiting for children to be deleted (cells, tablegroups, or toposervers still exist)",
-		)
-	}
-
-	return nil
-}
-
 // SetupWithManager sets up the controller with the Manager.
 func (r *MultigresClusterReconciler) SetupWithManager(
 	mgr ctrl.Manager,