refactor(dataprotection): clarify restore lifecycle coordination

Author: leon-ape

controllers/dataprotection/cluster_restore_controller.go

@@ -41,8 +41,7 @@ import (
 	dptypes "github.com/apecloud/kubeblocks/pkg/dataprotection/types"
 )
 
-// ClusterRestoreReconciler owns only the Cluster restore-protection finalizer.
-// VolumePopulator remains the owner of all PVC-scoped restore resources.
+// ClusterRestoreReconciler coordinates the Cluster-level restore lifecycle.
 type ClusterRestoreReconciler struct {
 	client.Client
 	Recorder record.EventRecorder
@@ -64,20 +63,20 @@ func (r *ClusterRestoreReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return intctrlutil.CheckedRequeueWithError(err, reqCtx.Log, "")
 	}
 
-	active, err := r.hasRestoreResources(ctx, r.Client, cluster)
+	hasResources, err := r.hasRestoreResources(ctx, cluster)
 	if err != nil {
 		return intctrlutil.CheckedRequeueWithError(err, reqCtx.Log, "failed to inspect Cluster restore resources")
 	}
 	if cluster.DeletionTimestamp.IsZero() {
-		if clusterRestoreConditionActive(cluster) || active {
+		if clusterRestoreConditionActive(cluster) || hasResources {
 			return r.ensureFinalizer(reqCtx, cluster)
 		}
 		return r.removeFinalizer(reqCtx, cluster)
 	}
 	if !controllerutil.ContainsFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) {
 		return intctrlutil.Reconciled()
 	}
-	if active {
+	if hasResources {
 		return intctrlutil.RequeueAfter(reconcileInterval, reqCtx.Log,
 			"waiting for restore resource owners to finish Cluster termination")
 	}
@@ -113,10 +112,10 @@ func clusterRestoreConditionActive(cluster *appsv1.Cluster) bool {
 	return condition == nil || condition.Status != metav1.ConditionTrue
 }
 
-func (r *ClusterRestoreReconciler) hasRestoreResources(ctx context.Context, reader client.Reader,
+func (r *ClusterRestoreReconciler) hasRestoreResources(ctx context.Context,
 	cluster *appsv1.Cluster) (bool, error) {
 	restores := &dpv1alpha1.RestoreList{}
-	if err := reader.List(ctx, restores, client.InNamespace(cluster.Namespace), client.MatchingLabels{
+	if err := r.Client.List(ctx, restores, client.InNamespace(cluster.Namespace), client.MatchingLabels{
 		constant.AppInstanceLabelKey: cluster.Name,
 	}); err != nil {
 		return false, err
@@ -126,17 +125,17 @@ func (r *ClusterRestoreReconciler) hasRestoreResources(ctx context.Context, read
 		if restore.Labels[dprestore.DataProtectionRestoreLabelKey] != restore.Name {
 			continue
 		}
-		ownedByCurrentCluster := restore.Labels[dptypes.ClusterUIDLabelKey] == string(cluster.UID)
+		owned := restore.Labels[dptypes.ClusterUIDLabelKey] == string(cluster.UID)
 		terminal := restore.Status.Phase == dpv1alpha1.RestorePhaseCompleted ||
 			restore.Status.Phase == dpv1alpha1.RestorePhaseFailed
-		if ownedByCurrentCluster && (!cluster.DeletionTimestamp.IsZero() ||
+		if owned && (!cluster.DeletionTimestamp.IsZero() ||
 			!terminal || !restore.DeletionTimestamp.IsZero()) {
 			return true, nil
 		}
 	}
 
 	pvcs := &corev1.PersistentVolumeClaimList{}
-	if err := reader.List(ctx, pvcs, client.InNamespace(cluster.Namespace), client.MatchingLabels{
+	if err := r.Client.List(ctx, pvcs, client.InNamespace(cluster.Namespace), client.MatchingLabels{
 		constant.AppInstanceLabelKey: cluster.Name,
 	}); err != nil {
 		return false, err

controllers/dataprotection/cluster_restore_controller_test.go

@@ -39,7 +39,7 @@ import (
 	dptypes "github.com/apecloud/kubeblocks/pkg/dataprotection/types"
 )
 
-func TestClusterRestoreControllerOwnsOnlyClusterFinalizer(t *testing.T) {
+func TestClusterRestoreControllerAddsProtectionForRestoreIntent(t *testing.T) {
 	scheme := clusterRestoreTestScheme(t)
 	cluster := activeRestoreCluster()
 	cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster).Build()
@@ -93,7 +93,7 @@ func TestClusterRestoreControllerReleasesFinalizerAfterOwnersFinish(t *testing.T
 	require.Contains(t, current.Finalizers, "example.io/keep")
 }
 
-func TestClusterRestoreControllerTreatsCompletedRestoreAsInactive(t *testing.T) {
+func TestClusterRestoreControllerReleasesProtectionAfterSuccessfulRestore(t *testing.T) {
 	scheme := clusterRestoreTestScheme(t)
 	cluster := activeRestoreCluster()
 	cluster.Finalizers = []string{dptypes.RestoreProtectionFinalizerName, "example.io/keep"}
@@ -112,7 +112,7 @@ func TestClusterRestoreControllerTreatsCompletedRestoreAsInactive(t *testing.T)
 	require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(cluster), current))
 	require.NotContains(t, current.Finalizers, dptypes.RestoreProtectionFinalizerName)
 	require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(restore), &dpv1alpha1.Restore{}),
-		"the Cluster controller observes terminal Restore objects but does not delete them")
+		"ClusterRestoreReconciler must leave terminal Restore objects for their resource owner to manage")
 }
 
 func TestClusterRestoreControllerKeepsProtectionAfterRestoreFailure(t *testing.T) {

controllers/dataprotection/volumepopulator_controller.go

@@ -60,7 +60,7 @@ import (
 	viper "github.com/apecloud/kubeblocks/pkg/viperx"
 )
 
-// VolumePopulatorReconciler reconciles Backup dataSource PVCs.
+// VolumePopulatorReconciler coordinates data population and restore for PVCs.
 type VolumePopulatorReconciler struct {
 	client.Client
 	Scheme   *runtime.Scheme
@@ -93,11 +93,7 @@ type pvcRestoreDecision struct {
 // +kubebuilder:rbac:groups=apps.kubeblocks.io,resources=components,verbs=get;list;watch
 // +kubebuilder:rbac:groups=apps.kubeblocks.io,resources=componentdefinitions,verbs=get;list;watch
 
-// Reconcile is part of the main kubernetes reconciliation loop which aims to
-// move the current state of the cluster closer to the desired state.
-//
-// For more details, check Reconcile and its Result here:
-// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.11.0/pkg/reconcile
+// Reconcile advances a PVC's population and restore lifecycle.
 func (r *VolumePopulatorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	reqCtx := intctrlutil.RequestCtx{
 		Ctx:      ctx,
@@ -185,9 +181,9 @@ func (r *VolumePopulatorReconciler) mapRestoreToPVCs(ctx context.Context, obj cl
 	}
 	includeTerminal := !restore.DeletionTimestamp.IsZero() ||
 		restore.Status.Phase == dpv1alpha1.RestorePhaseCompleted || restore.Status.Phase == dpv1alpha1.RestorePhaseFailed
-	// A postReady Restore is owned by its target Component, while its labels
-	// identify only the first source PVC that created it. Other Components in
-	// the Cluster can wait on the same Restore through postReady redirection.
+	// The component-name label identifies the first source PVC's Component,
+	// while the owner reference identifies the target Component. Other
+	// Components can wait on the same Restore through postReady redirection.
 	return r.mapRestorePVCs(ctx, restore.Namespace, client.MatchingLabels{
 		constant.AppInstanceLabelKey: clusterName,
 	}, restore.Labels[dptypes.ClusterUIDLabelKey], includeTerminal)
@@ -203,10 +199,9 @@ func (r *VolumePopulatorReconciler) mapComponentToPVCs(ctx context.Context, obj
 	if clusterName == "" || componentName == "" {
 		return nil
 	}
-	// A PVC can depend on another Component through a redirected postReady
-	// Restore. That relationship is derived from Backup status and is not
-	// represented on the Component, so a Component event must fan out to all
-	// active restore PVCs in its Cluster.
+	// Normal Component updates can unblock redirected postReady restores in
+	// other Components. Fan out to unfinished restore PVCs in the Cluster,
+	// since those dependencies are not represented on the Component itself.
 	labels := client.MatchingLabels{
 		constant.AppInstanceLabelKey: clusterName,
 	}
@@ -216,8 +211,8 @@ func (r *VolumePopulatorReconciler) mapComponentToPVCs(ctx context.Context, obj
 	}
 	includeTerminal := !comp.DeletionTimestamp.IsZero()
 	if includeTerminal {
-		// Component deletion is a termination signal only for restore PVCs
-		// physically owned by that Component.
+		// Deletion targets this Component's restore PVCs, including terminal
+		// PVCs that may still have restore resources to clean up.
 		labels[constant.KBAppComponentLabelKey] = componentName
 	}
 	return r.mapRestorePVCs(ctx, comp.Namespace, labels, string(clusterOwner.UID), includeTerminal)
@@ -423,9 +418,10 @@ func (r *VolumePopulatorReconciler) syncPVC(reqCtx intctrlutil.RequestCtx, pvc *
 	return nil
 }
 
-// handleRestoreParentLifecycle makes Cluster and Component dependencies part of
-// the PVC restore state machine. Parent deletion is the only cancellation
-// signal; the target PVC deletion timestamp is intentionally irrelevant.
+// handleRestoreParentLifecycle validates parent identity and protection, and
+// initiates restore termination when the Cluster or Component is deleting.
+// It returns true when normal restore processing should stop. Target PVC
+// deletion alone does not request restore termination.
 func (r *VolumePopulatorReconciler) handleRestoreParentLifecycle(reqCtx intctrlutil.RequestCtx,
 	pvc *corev1.PersistentVolumeClaim) (bool, error) {
 	clusterName := pvc.Labels[constant.AppInstanceLabelKey]
@@ -438,15 +434,13 @@ func (r *VolumePopulatorReconciler) handleRestoreParentLifecycle(reqCtx intctrlu
 	cluster := &appsv1.Cluster{}
 	clusterKey := types.NamespacedName{Namespace: pvc.Namespace, Name: clusterName}
 	if err := r.Client.Get(reqCtx.Ctx, clusterKey, cluster); err != nil {
-		// Unmarked PVCs may be standalone DP restores that happen to use app
-		// labels. Marked PVCs are known Cluster restores and must retry.
+		// App labels alone do not establish Cluster restore identity;
+		// standalone DP restores may use the same labels.
 		if apierrors.IsNotFound(err) && !hasClusterIdentity {
 			return false, nil
 		}
-		// A retained target PVC can outlive its Cluster after its VP-owned
-		// finalizer and side effects have been released. Do not restart or poll
-		// the completed cancellation path merely because App-owned restore
-		// identity remains on that PVC.
+		// Do not restart a retained target after its Cluster is gone and VP
+		// has released the target finalizer.
 		if apierrors.IsNotFound(err) &&
 			!controllerutil.ContainsFinalizer(pvc, dptypes.DataProtectionFinalizerName) {
 			return true, nil

controllers/dataprotection/volumepopulator_controller_test.go

@@ -3372,7 +3372,7 @@ func TestMapExecutionRestoreToTargetPVC(t *testing.T) {
 	require.Empty(t, reconciler.mapRestoreToPVCs(context.Background(), sourceRestore))
 }
 
-func TestMapPostReadyRestoreToNonTerminalComponentPVCs(t *testing.T) {
+func TestMapPostReadyRestoreToNonTerminalClusterPVCs(t *testing.T) {
 	comp := &kbappsv1.Component{ObjectMeta: metav1.ObjectMeta{
 		Namespace: "default", Name: "cluster-mysql", UID: "component-uid",
 		Labels: map[string]string{
@@ -3610,7 +3610,7 @@ func TestParentDeletionTerminatesOnlyVolumePopulatorResourcesInOrder(t *testing.
 	require.True(t, apierrors.IsNotFound(cli.Get(context.Background(), client.ObjectKeyFromObject(helper), helper)))
 	require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(target), currentTarget))
 	require.Contains(t, currentTarget.Finalizers, dptypes.DataProtectionFinalizerName,
-		"target DP finalizer must remain until an API read confirms helper deletion")
+		"target DP finalizer must remain until helper deletion is observed")
 
 	terminated, err = reconciler.handleRestoreParentLifecycle(reqCtx, currentTarget)
 	require.True(t, terminated)
@@ -3620,7 +3620,7 @@ func TestParentDeletionTerminatesOnlyVolumePopulatorResourcesInOrder(t *testing.
 	require.Contains(t, currentTarget.Finalizers, "example.io/app-owner")
 	require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(cluster), cluster))
 	require.Contains(t, cluster.Finalizers, dptypes.RestoreProtectionFinalizerName,
-		"VolumePopulator must never remove the Cluster controller finalizer")
+		"VolumePopulator must not remove the Cluster restore-protection finalizer")
 }
 
 func TestClusterDeletionTerminatesPostReadyRestoreAfterComponentIsGone(t *testing.T) {
@@ -3714,7 +3714,7 @@ func TestComponentTerminationContinuesAfterRetainedPVCIsDetachedFromWorkload(t *
 	require.False(t, currentRestore.DeletionTimestamp.IsZero())
 }
 
-func TestITS2ScaleInRetainedPVCContinuesRestoreWithVerifiedIdentity(t *testing.T) {
+func TestRetainedPVCContinuesRestoreWithVerifiedIdentity(t *testing.T) {
 	scheme, cluster, component, _, target := parentRestoreObjects(t)
 	target.OwnerReferences = nil
 	target.Finalizers = []string{dptypes.DataProtectionFinalizerName}
@@ -3739,7 +3739,7 @@ func TestITS2ScaleInRetainedPVCContinuesRestoreWithVerifiedIdentity(t *testing.T
 	}, helper), "verified retained target must continue the normal restore state machine")
 }
 
-func TestComponentReplacementFailsClosedBeforeRestoreProgression(t *testing.T) {
+func TestVolumePopulatorRejectsMismatchedComponentUID(t *testing.T) {
 	scheme, cluster, component, _, target := parentRestoreObjects(t)
 	target.OwnerReferences = nil
 	target.Labels[dptypes.ComponentUIDLabelKey] = "previous-component-uid"
@@ -3823,7 +3823,7 @@ func TestComponentTerminationDeletesPostReadyRestoreByOwnerNotSourceLabel(t *tes
 	currentRestore := &dpv1alpha1.Restore{}
 	require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(postReady), currentRestore))
 	require.False(t, currentRestore.DeletionTimestamp.IsZero(),
-		"the deleting owner Component must terminate postReady Restore regardless of source label")
+		"VolumePopulator must terminate postReady Restore when its owner Component is deleting, regardless of source label")
 }
 
 func TestTargetPVCDeletionIsNotParentTermination(t *testing.T) {

pkg/dataprotection/types/constant.go

@@ -46,9 +46,9 @@ const (
 const (
 	// DataProtectionFinalizerName is the name of our custom finalizer
 	DataProtectionFinalizerName = "dataprotection.kubeblocks.io/finalizer"
-	// RestoreProtectionFinalizerName is owned independently by the Cluster
-	// restore lifecycle controller.
-	RestoreProtectionFinalizerName = "dataprotection.kubeblocks.io/restore-protection"
+	// RestoreProtectionFinalizerName prevents Cluster deletion from completing
+	// before its restore resources have been cleaned up.
+	RestoreProtectionFinalizerName = "dataprotection.kubeblocks.io/restore-protection-finalizer"
 )
 
 // annotation keys