From 67d60d6c8d2c6ed0d42683135c1d9811ca6f6166 Mon Sep 17 00:00:00 2001 From: Nicolas Nosenzo Date: Fri, 18 Sep 2026 11:18:05 +0200 Subject: [PATCH 1/2] fix(shard): always orphan PVCs on scale-down instead of hard-deleting Always orphan instead and let multigres-gc reclaim after the retention window, except when the owning MultigresCluster is being deleted, where there is nothing left to roll back to. Signed-off-by: Nicolas Nosenzo --- .../controller/shard/reconcile_deletion.go | 60 ++++++------- .../shard/reconcile_deletion_test.go | 58 +++++++++++++ .../controller/shard/reconcile_pool_pods.go | 70 ++------------- .../controller/shard/shard_controller.go | 37 ++++---- .../shard/shard_controller_internal_test.go | 86 +++++++++++++------ 5 files changed, 175 insertions(+), 136 deletions(-) diff --git a/pkg/resource-handler/controller/shard/reconcile_deletion.go b/pkg/resource-handler/controller/shard/reconcile_deletion.go index 5f01a8a5b..956ba7675 100644 --- a/pkg/resource-handler/controller/shard/reconcile_deletion.go +++ b/pkg/resource-handler/controller/shard/reconcile_deletion.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "slices" - "strings" "time" "github.com/multigres/multigres/go/common/topoclient" @@ -123,10 +122,10 @@ func (r *ShardReconciler) handleDeletion( return ctrl.Result{RequeueAfter: podTerminationRequeueDelay}, nil } - // All pods gone. Clean up PVCs whose policy resolves to Delete. Small - // shards (<= pvcOrphanReplicasThreshold replicas) defer the work to - // multigres-gc by labelling the PVC with multigres.com/orphan-since=, - // larger shards are deleted in-line. + // All pods gone. Clean up PVCs whose policy resolves to Delete. Unless the + // owning MultigresCluster is being torn down, this defers to multigres-gc + // by labelling the PVC with multigres.com/orphan-since= instead of + // deleting in-line. if err := r.cleanupShardPVCs(ctx, shard); err != nil { return ctrl.Result{}, err } @@ -149,14 +148,23 @@ func (r *ShardReconciler) handleDeletion( // cleanupShardPVCs handles per-PVC cleanup when a Shard is being deleted. // Only PVCs whose effective WhenDeleted policy is Delete are touched. +// +// If the owning MultigresCluster is confirmed still present and being +// deleted, PVCs are deleted in line. The cluster is going away, so there is +// nothing left to roll a scale down back to. Otherwise, either the Shard is +// being individually removed (e.g. a shard count scale down) while the +// cluster stays up, or the parent cluster is unexpectedly unreadable, so PVCs +// are orphaned instead. That gives multigres gc's retention window a chance +// to recover from an accidental removal. See clusterIsChurning. func (r *ShardReconciler) cleanupShardPVCs( ctx context.Context, shard *multigresv1alpha1.Shard, ) error { logger := log.FromContext(ctx) + clusterName := shard.Labels[metadata.LabelMultigresCluster] selector := map[string]string{ - metadata.LabelMultigresCluster: shard.Labels[metadata.LabelMultigresCluster], + metadata.LabelMultigresCluster: clusterName, metadata.LabelMultigresDatabase: string(shard.Spec.DatabaseName), metadata.LabelMultigresTableGroup: string(shard.Spec.TableGroupName), metadata.LabelMultigresShard: string(shard.Spec.ShardName), @@ -172,42 +180,28 @@ func (r *ShardReconciler) cleanupShardPVCs( return fmt.Errorf("failed to list PVCs for cleanup: %w", err) } - // Group eligible PVCs by pool+cell so the keep-threshold decision is made - // per group (matching replicasPerCell semantics). - groups := map[string][]*corev1.PersistentVolumeClaim{} + churning, err := r.clusterIsChurning(ctx, shard.Namespace, clusterName) + if err != nil { + return fmt.Errorf("failed to determine MultigresCluster deletion state: %w", err) + } + + now := time.Now() for i := range pvcList.Items { pvc := &pvcList.Items[i] if !shardPVCShouldBeCleaned(shard, pvc) { continue } - key := pvc.Labels[metadata.LabelMultigresPool] + "/" + pvc.Labels[metadata.LabelMultigresCell] - groups[key] = append(groups[key], pvc) - } - - now := time.Now() - for _, group := range groups { - // Sort descending by name so the highest-ordinal PVCs are visited first - // and become the deleted excess, the lowest are kept as orphans. - slices.SortFunc(group, func(a, b *corev1.PersistentVolumeClaim) int { - return strings.Compare(b.Name, a.Name) - }) - live := len(group) - for _, pvc := range group { - _, hasIndex := resolvePodIndex(pvc.Name) - if !hasIndex || orphanByRemainingCount(live) { - if err := pvcutil.MarkOrphan(ctx, r.Client, pvc, shard.GetUID(), now); err != nil { - return fmt.Errorf("failed to mark PVC %s orphan: %w", pvc.Name, err) - } - logger.Info("Marked PVC orphan on Shard deletion", "pvc", pvc.Name) - live-- - continue - } + if churning { if err := r.Delete(ctx, pvc); err != nil && !errors.IsNotFound(err) { return fmt.Errorf("failed to delete PVC %s: %w", pvc.Name, err) } - logger.Info("Deleted PVC on Shard deletion", "pvc", pvc.Name) - live-- + logger.Info("Deleted PVC on MultigresCluster deletion", "pvc", pvc.Name) + continue + } + if err := pvcutil.MarkOrphan(ctx, r.Client, pvc, shard.GetUID(), now); err != nil { + return fmt.Errorf("failed to mark PVC %s orphan: %w", pvc.Name, err) } + logger.Info("Marked PVC orphan on Shard deletion", "pvc", pvc.Name) } return nil } diff --git a/pkg/resource-handler/controller/shard/reconcile_deletion_test.go b/pkg/resource-handler/controller/shard/reconcile_deletion_test.go index 8c7c6cda1..d56eb92fa 100644 --- a/pkg/resource-handler/controller/shard/reconcile_deletion_test.go +++ b/pkg/resource-handler/controller/shard/reconcile_deletion_test.go @@ -8,6 +8,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" policyv1 "k8s.io/api/policy/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -363,6 +364,63 @@ func TestHandleDeletion(t *testing.T) { } }) + t.Run( + "MultigresCluster being deleted hard-deletes PVC instead of orphaning", + func(t *testing.T) { + t.Parallel() + + shard := baseShard.DeepCopy() + shard.Spec.PVCDeletionPolicy = &multigresv1alpha1.PVCDeletionPolicy{ + WhenDeleted: multigresv1alpha1.DeletePVCRetentionPolicy, + } + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "data-pvc-churn", + Namespace: "default", + Labels: shardLabels, + }, + } + cluster := &multigresv1alpha1.MultigresCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "default", + DeletionTimestamp: &metav1.Time{Time: metav1.Now().Time}, + Finalizers: []string{multigresv1alpha1.FinalizerClusterCleanup}, + }, + } + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(shard, pvc, cluster). + WithStatusSubresource(&multigresv1alpha1.Shard{}). + Build() + + r := &ShardReconciler{ + Client: c, + Scheme: scheme, + Recorder: record.NewFakeRecorder(10), + CreateTopoStore: newMemoryTopoFactory(), + } + + result, err := r.handleDeletion(context.Background(), shard) + if err != nil { + t.Fatalf("handleDeletion returned error: %v", err) + } + if result.RequeueAfter != 0 { + t.Errorf("Expected no requeue once pods are gone, got %v", result.RequeueAfter) + } + got := &corev1.PersistentVolumeClaim{} + err = c.Get(context.Background(), + types.NamespacedName{Name: "data-pvc-churn", Namespace: "default"}, got) + if !apierrors.IsNotFound(err) { + t.Errorf( + "PVC should be hard-deleted while cluster is being deleted, got err=%v", + err, + ) + } + }, + ) + t.Run("pod stuck terminating past timeout does not block PVC cleanup", func(t *testing.T) { t.Parallel() diff --git a/pkg/resource-handler/controller/shard/reconcile_pool_pods.go b/pkg/resource-handler/controller/shard/reconcile_pool_pods.go index 030fa5c48..2f7b32a23 100644 --- a/pkg/resource-handler/controller/shard/reconcile_pool_pods.go +++ b/pkg/resource-handler/controller/shard/reconcile_pool_pods.go @@ -1043,12 +1043,11 @@ func (r *ShardReconciler) cleanupDrainedPod( return nil } -// cleanupPodPVC removes a pod's data PVC from the operator's care. The choice -// between orphaning (deferred deletion via multigres-gc) and in-line deletion -// is based on how many sibling PVCs remain in the same pool+cell: if removing -// this one still leaves >= pvcOrphanReplicasThreshold volumes, it is excess and -// is deleted, otherwise it is orphaned so the data can be recovered. See -// orphanByRemainingCount. +// cleanupPodPVC removes a pod's data PVC from the operator's care by marking +// it orphan. The multigres-gc CronJob deletes it once the retention window +// elapses, giving an accidental scale down or replace a window to be rolled +// back. This only runs while the Shard itself is not being deleted, see +// cleanupShardPVCs for the Shard and cluster teardown path. func (r *ShardReconciler) cleanupPodPVC( ctx context.Context, shard *multigresv1alpha1.Shard, @@ -1078,65 +1077,14 @@ func (r *ShardReconciler) cleanupPodPVC( return fmt.Errorf("failed to fetch PVC %s for cleanup: %w", pvcName, err) } - liveCount, err := r.countPoolCellPVCs(ctx, shard, poolName, cellName) - if err != nil { - return err - } - - if orphanByRemainingCount(liveCount) { - if err := pvcutil.MarkOrphan(ctx, r.Client, pvc, shard.GetUID(), time.Now()); err != nil { - logger.Error(err, "Failed to mark PVC orphan for "+reason+" pod", "pvc", pvcName) - return fmt.Errorf("failed to mark PVC %s orphan: %w", pvcName, err) - } - logger.Info("Marked PVC orphan for "+reason+" pod", "pvc", pvcName, "liveCount", liveCount) - return nil - } - - if err := r.Delete(ctx, pvc); err != nil && !errors.IsNotFound(err) { - logger.Error(err, "Failed to delete PVC for "+reason+" pod", "pvc", pvcName) - return fmt.Errorf("failed to delete PVC %s: %w", pvcName, err) + if err := pvcutil.MarkOrphan(ctx, r.Client, pvc, shard.GetUID(), time.Now()); err != nil { + logger.Error(err, "Failed to mark PVC orphan for "+reason+" pod", "pvc", pvcName) + return fmt.Errorf("failed to mark PVC %s orphan: %w", pvcName, err) } - logger.Info("Deleted PVC for "+reason+" pod", "pvc", pvcName, "liveCount", liveCount) + logger.Info("Marked PVC orphan for "+reason+" pod", "pvc", pvcName) return nil } -// countPoolCellPVCs returns the number of PVCs currently present for the given -// pool+cell, used to decide orphan-vs-delete. Already-orphaned PVCs are -// excluded: they are no longer part of the live serving set, so they must not -// inflate the count and cause a still-needed volume to be hard-deleted. -func (r *ShardReconciler) countPoolCellPVCs( - ctx context.Context, - shard *multigresv1alpha1.Shard, - poolName, cellName string, -) (int, error) { - labels := buildPoolLabelsWithCell(shard, poolName, cellName) - selector := metadata.GetSelectorLabels(labels) - - pvcList := &corev1.PersistentVolumeClaimList{} - if err := r.List( - ctx, - pvcList, - client.InNamespace(shard.Namespace), - client.MatchingLabels(selector), - ); err != nil { - return 0, fmt.Errorf( - "failed to list PVCs for pool %s cell %s: %w", - poolName, - cellName, - err, - ) - } - - count := 0 - for i := range pvcList.Items { - if pvcutil.HasOrphanLabel(&pvcList.Items[i]) { - continue - } - count++ - } - return count, nil -} - // podNeedsUpdate checks if a pod requires recreation due to spec changes. // Since most pod fields are immutable, we rely on the pre-computed spec-hash annotation. func podNeedsUpdate( diff --git a/pkg/resource-handler/controller/shard/shard_controller.go b/pkg/resource-handler/controller/shard/shard_controller.go index c5b09a24b..5f5338ece 100644 --- a/pkg/resource-handler/controller/shard/shard_controller.go +++ b/pkg/resource-handler/controller/shard/shard_controller.go @@ -39,24 +39,29 @@ const ( // multigres-gc cronjob can clean them up instead of k8s cascade-GC // nuking them the moment the Shard CR is deleted. shardFinalizer = "multigres.com/shard-pvc-orphan" - - // pvcOrphanReplicasThreshold is the number of pool pod PVCs that are - // kept (orphaned, deferred to the multigres-gc cronjob) rather than deleted - // in-line when a pod is scaled down, drained, or the shard is removed. - // Keeping a few volumes around lets an accidental scale-down/removal be - // rolled back, beyond the threshold the excess is deleted immediately. - pvcOrphanReplicasThreshold = 3 ) -// orphanByRemainingCount decides between orphaning and in-line deletion based -// on how many sibling PVCs currently exist. -// -// liveCount includes the PVC being cleaned up. After it is removed, liveCount-1 -// PVCs remain: if that is still >= pvcOrphanReplicasThreshold we have plenty of -// volumes left, so this one is excess and is hard-deleted, otherwise we keep it -// as an orphan so the data can be recovered. -func orphanByRemainingCount(liveCount int) bool { - return liveCount-1 < pvcOrphanReplicasThreshold +// clusterIsChurning reports whether the owning MultigresCluster is being +// deleted. Pod/pool scale down and shard removal always orphan their PVCs so +// an accidental change can be rolled back within the retention window, but +// that protection is pointless once the whole cluster is being torn down, so +// PVCs are hard deleted immediately in that case. +func (r *ShardReconciler) clusterIsChurning( + ctx context.Context, + namespace, clusterName string, +) (bool, error) { + if clusterName == "" { + return false, nil + } + cluster := &multigresv1alpha1.MultigresCluster{} + err := r.Get(ctx, client.ObjectKey{Namespace: namespace, Name: clusterName}, cluster) + if errors.IsNotFound(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("failed to get MultigresCluster %s: %w", clusterName, err) + } + return !cluster.DeletionTimestamp.IsZero(), nil } // ShardReconciler reconciles a Shard object. diff --git a/pkg/resource-handler/controller/shard/shard_controller_internal_test.go b/pkg/resource-handler/controller/shard/shard_controller_internal_test.go index 94a624ff1..2dab036e9 100644 --- a/pkg/resource-handler/controller/shard/shard_controller_internal_test.go +++ b/pkg/resource-handler/controller/shard/shard_controller_internal_test.go @@ -1257,30 +1257,65 @@ func TestSetupWithManager(t *testing.T) { }) } -// TestOrphanByRemainingCount verifies that cleanupDrainedPod handles -// PVC deletion correctly for DRAINED replacement pods (idx < deletion threshold), -// scale-down pods (idx >= deletion threshold), and rolling-update pods under different -// PVC deletion policies. -func TestOrphanByRemainingCount(t *testing.T) { +func TestClusterIsChurning(t *testing.T) { t.Parallel() + scheme := runtime.NewScheme() + _ = multigresv1alpha1.AddToScheme(scheme) + + deletionTimestamp := metav1.Now() + tests := map[string]struct { - liveCount int - wantOrphan bool + clusterName string + cluster *multigresv1alpha1.MultigresCluster + want bool }{ - "scale 4->3 keeps enough -> delete": {liveCount: 4, wantOrphan: false}, - "exactly threshold+1 -> delete": {liveCount: 4, wantOrphan: false}, - "scale 3->2 below threshold -> orphan": {liveCount: 3, wantOrphan: true}, - "single PVC -> orphan": {liveCount: 1, wantOrphan: true}, - "large pool -> delete": {liveCount: 10, wantOrphan: false}, + "empty cluster name": { + clusterName: "", + want: false, + }, + "cluster not found": { + clusterName: "missing-cluster", + want: false, + }, + "cluster present, not deleting": { + clusterName: "test-cluster", + cluster: &multigresv1alpha1.MultigresCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cluster", Namespace: "default"}, + }, + want: false, + }, + "cluster present, being deleted": { + clusterName: "test-cluster", + cluster: &multigresv1alpha1.MultigresCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "default", + DeletionTimestamp: &deletionTimestamp, + Finalizers: []string{multigresv1alpha1.FinalizerClusterCleanup}, + }, + }, + want: true, + }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { t.Parallel() - if got := orphanByRemainingCount(tc.liveCount); got != tc.wantOrphan { - t.Errorf("orphanByRemainingCount(%d) = %v, want %v", - tc.liveCount, got, tc.wantOrphan) + + var objs []client.Object + if tc.cluster != nil { + objs = append(objs, tc.cluster) + } + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() + r := &ShardReconciler{Client: fakeClient} + + got, err := r.clusterIsChurning(context.Background(), "default", tc.clusterName) + if err != nil { + t.Fatalf("clusterIsChurning() returned unexpected error: %v", err) + } + if got != tc.want { + t.Errorf("clusterIsChurning() = %v, want %v", got, tc.want) } }) } @@ -1331,8 +1366,8 @@ func TestCleanupDrainedPod_PVCDeletion(t *testing.T) { }, } } - // makePVC builds a PVC carrying the pool+cell labels so countPoolCellPVCs - // finds it when deciding orphan-vs-delete. + // makePVC builds a PVC carrying the pool+cell labels, matching what a real + // pool data PVC looks like. makePVC := func(n string) *corev1.PersistentVolumeClaim { return &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ @@ -1351,11 +1386,8 @@ func TestCleanupDrainedPod_PVCDeletion(t *testing.T) { } tests := map[string]struct { - podName string - pvcName string - // siblingCount is the number of PVCs present in the pool+cell (including - // pvcName). orphanByRemainingCount uses this: siblingCount-1 >= threshold - // deletes, otherwise orphans. + podName string + pvcName string siblingCount int podRoles map[string]string policy *multigresv1alpha1.PVCDeletionPolicy @@ -1398,21 +1430,23 @@ func TestCleanupDrainedPod_PVCDeletion(t *testing.T) { wantPVC: true, wantOrphan: true, }, - "DRAINED large pool (4, scaling to 3) -> in-line delete": { + "DRAINED large pool (4, scaling to 3) -> orphan": { podName: podName0, pvcName: pvcName0, siblingCount: 4, podRoles: map[string]string{podName0: "DRAINED"}, policy: deletePolicy, - wantPVC: false, + wantPVC: true, + wantOrphan: true, }, - "scale-down large pool (4, scaling to 3) -> in-line delete": { + "scale-down large pool (4, scaling to 3) -> orphan": { podName: podName5, pvcName: pvcName5, siblingCount: 4, podRoles: map[string]string{}, policy: deletePolicy, - wantPVC: false, + wantPVC: true, + wantOrphan: true, }, } From c2e1ee345b4a9cab39ed28dfb739ca56898616a1 Mon Sep 17 00:00:00 2001 From: Nicolas Nosenzo Date: Tue, 22 Sep 2026 17:05:52 +0200 Subject: [PATCH 2/2] fix(multigrescluster): hold cleanup finalizer until child CRs are gone The finalizer was dropped before Shards finished deleting, so clusterIsChurning saw NotFound and orphaned PVCs on cluster teardown. Requeue until Shards, TableGroups, and Cells are gone. Signed-off-by: Nicolas Nosenzo --- .../multigrescluster_controller.go | 28 +++- .../multigrescluster_controller_test.go | 143 ++++++++++++++++++ .../controller/shard/reconcile_deletion.go | 10 +- 3 files changed, 173 insertions(+), 8 deletions(-) diff --git a/pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller.go b/pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller.go index 35070f95d..83ad16e33 100644 --- a/pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller.go +++ b/pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller.go @@ -391,8 +391,16 @@ func (r *MultigresClusterReconciler) ensureClusterFinalizer( return nil } -// handleDeletion deletes cluster children and retained topology PVCs before -// releasing the cluster cleanup finalizer. +// childDeletionRequeueDelay is how long handleDeletion waits before +// rechecking for remaining children. The cluster controller does not watch +// Shards, so it must poll rather than wait for a watch event. +const childDeletionRequeueDelay = 2 * time.Second + +// handleDeletion deletes cluster children and retained topology PVCs, then +// holds the cluster cleanup finalizer until all Shards, TableGroups, and +// Cells are gone. That keeps the cluster's DeletionTimestamp visible so the +// Shard controller's clusterIsChurning check can detect the teardown and +// hard-delete PVCs instead of orphaning them. func (r *MultigresClusterReconciler) handleDeletion( ctx context.Context, cluster *multigresv1alpha1.MultigresCluster, @@ -465,6 +473,20 @@ func (r *MultigresClusterReconciler) handleDeletion( } } + shards := &multigresv1alpha1.ShardList{} + if err := r.List(ctx, shards, ns, clusterLabels); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to list shards: %w", err) + } + + if len(shards.Items) > 0 || len(tableGroups.Items) > 0 || len(cells.Items) > 0 { + l.Info("Waiting for child resources to finish deleting", + "shards", len(shards.Items), + "tablegroups", len(tableGroups.Items), + "cells", len(cells.Items), + ) + return ctrl.Result{RequeueAfter: childDeletionRequeueDelay}, nil + } + if slices.Contains(cluster.Finalizers, multigresv1alpha1.FinalizerClusterCleanup) { before := cluster.DeepCopy() @@ -495,7 +517,7 @@ func (r *MultigresClusterReconciler) handleDeletion( } l.Info("Cluster cleanup complete") - r.Recorder.Event(cluster, "Normal", "CleanupComplete", "Initiated deletion of child resources") + r.Recorder.Event(cluster, "Normal", "CleanupComplete", "All child resources deleted") return ctrl.Result{}, nil } diff --git a/pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller_test.go b/pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller_test.go index f7757eee9..2b70fe5eb 100644 --- a/pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller_test.go +++ b/pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller_test.go @@ -409,6 +409,149 @@ func TestHandleDeletionReleasesPoolerClient(t *testing.T) { }) } +func TestHandleDeletionWaitsForChildren(t *testing.T) { + clusterKey := types.NamespacedName{Name: "test-cluster", Namespace: "test-ns"} + deletionTimestamp := metav1.Now() + + newCluster := func() *multigresv1alpha1.MultigresCluster { + return &multigresv1alpha1.MultigresCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterKey.Name, + Namespace: clusterKey.Namespace, + DeletionTimestamp: &deletionTimestamp, + Finalizers: []string{multigresv1alpha1.FinalizerClusterCleanup}, + }, + } + } + childLabels := map[string]string{metadata.LabelMultigresCluster: clusterKey.Name} + + tests := []struct { + name string + child client.Object + }{ + { + name: "shard still present", + child: &multigresv1alpha1.Shard{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-shard", + Namespace: clusterKey.Namespace, + Labels: childLabels, + }, + }, + }, + { + name: "tablegroup still present", + child: &multigresv1alpha1.TableGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-tablegroup", + Namespace: clusterKey.Namespace, + Labels: childLabels, + Finalizers: []string{"test/hold"}, + }, + }, + }, + { + name: "cell still present", + child: &multigresv1alpha1.Cell{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cell", + Namespace: clusterKey.Namespace, + Labels: childLabels, + Finalizers: []string{"test/hold"}, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cluster := newCluster() + forgotten := false + r := &MultigresClusterReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(setupScheme()). + WithObjects(cluster, tt.child). + Build(), + Recorder: record.NewFakeRecorder(1), + PoolerClientCache: poolerClientCacheFunc(func(types.NamespacedName) { + forgotten = true + }), + } + + result, err := r.handleDeletion(t.Context(), cluster.DeepCopy()) + if err != nil { + t.Fatalf("handleDeletion() error = %v", err) + } + if result.RequeueAfter != childDeletionRequeueDelay { + t.Fatalf( + "RequeueAfter = %v, want %v", + result.RequeueAfter, + childDeletionRequeueDelay, + ) + } + if forgotten { + t.Fatal("pooler client cache notified while children remain") + } + + got := &multigresv1alpha1.MultigresCluster{} + if err := r.Get(t.Context(), clusterKey, got); err != nil { + t.Fatalf("failed to get cluster: %v", err) + } + if !slices.Contains(got.Finalizers, multigresv1alpha1.FinalizerClusterCleanup) { + t.Fatal("cluster cleanup finalizer removed while children remain") + } + }) + } + + t.Run("no children", func(t *testing.T) { + cluster := newCluster() + r := &MultigresClusterReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(setupScheme()). + WithObjects(cluster). + Build(), + Recorder: record.NewFakeRecorder(1), + } + forgotten := make(chan types.NamespacedName, 1) + r.PoolerClientCache = poolerClientCacheFunc(func(key types.NamespacedName) { + forgotten <- key + }) + + fetched := &multigresv1alpha1.MultigresCluster{} + if err := r.Get(t.Context(), clusterKey, fetched); err != nil { + t.Fatalf("failed to get cluster: %v", err) + } + + result, err := r.handleDeletion(t.Context(), fetched) + if err != nil { + t.Fatalf("handleDeletion() error = %v", err) + } + if result.RequeueAfter != 0 { + t.Fatalf("RequeueAfter = %v, want 0", result.RequeueAfter) + } + select { + case got := <-forgotten: + if got != clusterKey { + t.Fatalf("forgot cluster %v, want %v", got, clusterKey) + } + default: + t.Fatal("pooler client cache was not notified") + } + + got := &multigresv1alpha1.MultigresCluster{} + err = r.Get(t.Context(), clusterKey, got) + switch { + case err == nil: + if slices.Contains(got.Finalizers, multigresv1alpha1.FinalizerClusterCleanup) { + t.Fatal("cluster cleanup finalizer not removed") + } + case apierrors.IsNotFound(err): + default: + t.Fatalf("failed to get cluster: %v", err) + } + }) +} + func TestReconcileNotFoundReleasesPoolerClient(t *testing.T) { key := types.NamespacedName{Name: "missing", Namespace: "test-ns"} forgotten := make(chan types.NamespacedName, 1) diff --git a/pkg/resource-handler/controller/shard/reconcile_deletion.go b/pkg/resource-handler/controller/shard/reconcile_deletion.go index 956ba7675..4a949499c 100644 --- a/pkg/resource-handler/controller/shard/reconcile_deletion.go +++ b/pkg/resource-handler/controller/shard/reconcile_deletion.go @@ -151,11 +151,11 @@ func (r *ShardReconciler) handleDeletion( // // If the owning MultigresCluster is confirmed still present and being // deleted, PVCs are deleted in line. The cluster is going away, so there is -// nothing left to roll a scale down back to. Otherwise, either the Shard is -// being individually removed (e.g. a shard count scale down) while the -// cluster stays up, or the parent cluster is unexpectedly unreadable, so PVCs -// are orphaned instead. That gives multigres gc's retention window a chance -// to recover from an accidental removal. See clusterIsChurning. +// nothing left to roll a scale down back to. The cluster controller holds +// its own finalizer until Shards are gone, so this is the normal path for a +// full cluster teardown. Otherwise the Shard is being individually removed +// (e.g. a shard count scale down) while the cluster stays up, so PVCs are +// orphaned instead. func (r *ShardReconciler) cleanupShardPVCs( ctx context.Context, shard *multigresv1alpha1.Shard,