Skip to content

fix(shard): always orphan PVCs on scale-down instead of hard-deleting #663

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

Open
niconosenzo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(shard): always orphan PVCs on scale-down instead of hard-deleting #663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
60 changes: 27 additions & 33 deletions pkg/resource-handler/controller/shard/reconcile_deletion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"fmt"
"slices"
"strings"
"time"

"github.com/multigres/multigres/go/common/topoclient"
Expand Down Expand Up @@ -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=<now>,
// 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=<now> instead of
// deleting in-line.
if err := r.cleanupShardPVCs(ctx, shard); err != nil {
return ctrl.Result{}, err
}
Expand All @@ -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),
Expand All @@ -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
}
Expand Down
58 changes: 58 additions & 0 deletions pkg/resource-handler/controller/shard/reconcile_deletion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()

Expand Down
70 changes: 9 additions & 61 deletions pkg/resource-handler/controller/shard/reconcile_pool_pods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand Down
37 changes: 21 additions & 16 deletions pkg/resource-handler/controller/shard/shard_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

@Verolop Verolop Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the cluster controller drops its finalizer before the shards finish cleaning up. If the cluster is already gone by the time we get here, we’ll get NotFound and orphan the PVCs anyway. We can orphan here too unless you want to make the cluster wait for shard cleanup.
I tested this by running the cluster deletion reconciliation before shard cleanup. The parent disappeared, and the PVC remained with an orphan timestamp and no owner references.

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.
Expand Down
Loading
Loading