diff --git a/pkg/resource-handler/controller/shard/reconcile_deletion.go b/pkg/resource-handler/controller/shard/reconcile_deletion.go index 5f01a8a5..956ba767 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 8c7c6cda..d56eb92f 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 030fa5c4..2f7b32a2 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 c5b09a24..5f5338ec 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 94a624ff..2dab036e 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, }, }