From 944d952cc04a0ed554331ba70ffbd59b6ab89c50 Mon Sep 17 00:00:00 2001 From: Leon Date: Fri, 4 Sep 2026 11:27:04 +0800 Subject: [PATCH 1/3] fix(dataprotection): identify and route restore dependencies --- controllers/apps/cluster/restore_intent.go | 3 + .../apps/cluster/restore_intent_test.go | 4 + .../volumepopulator_controller.go | 93 ++++++++++++----- .../volumepopulator_controller_test.go | 99 +++++++++++++++---- pkg/dataprotection/types/constant.go | 2 + 5 files changed, 158 insertions(+), 43 deletions(-) diff --git a/controllers/apps/cluster/restore_intent.go b/controllers/apps/cluster/restore_intent.go index 128b7f8bd5e..2c1841230fd 100644 --- a/controllers/apps/cluster/restore_intent.go +++ b/controllers/apps/cluster/restore_intent.go @@ -84,6 +84,7 @@ func injectRestoreIntentToVCT(cluster *appsv1.Cluster, componentName string, vct vct.Annotations[constant.RestoreSourceKindAnnotationKey] = restore.Source.Kind vct.Annotations[constant.RestoreSourceNameAnnotationKey] = restore.Source.Name vct.Annotations[constant.RestoreSourceNamespaceAnnotationKey] = sourceNamespace + vct.Annotations[constant.KBAppClusterUIDKey] = string(cluster.UID) vct.Annotations[constant.RestoreComponentAnnotationKey] = componentName vct.Annotations[constant.RestoreVolumeTemplateAnnotationKey] = vct.Name delete(vct.Annotations, constant.RestorePITRAnnotationKey) @@ -118,6 +119,7 @@ func cleanupRestoreIntentFromVCT(vct *appsv1.PersistentVolumeClaimTemplate) { delete(vct.Annotations, constant.RestoreSourceKindAnnotationKey) delete(vct.Annotations, constant.RestoreSourceNameAnnotationKey) delete(vct.Annotations, constant.RestoreSourceNamespaceAnnotationKey) + delete(vct.Annotations, constant.KBAppClusterUIDKey) delete(vct.Annotations, constant.RestorePITRAnnotationKey) delete(vct.Annotations, constant.RestoreParametersAnnotationKey) delete(vct.Annotations, constant.RestoreComponentAnnotationKey) @@ -138,6 +140,7 @@ func hasRestoreIntent(vct *appsv1.PersistentVolumeClaimTemplate) bool { constant.RestoreSourceKindAnnotationKey, constant.RestoreSourceNameAnnotationKey, constant.RestoreSourceNamespaceAnnotationKey, + constant.KBAppClusterUIDKey, constant.RestoreComponentAnnotationKey, constant.RestoreVolumeTemplateAnnotationKey, } { diff --git a/controllers/apps/cluster/restore_intent_test.go b/controllers/apps/cluster/restore_intent_test.go index 025ea5bd0ab..750e13e793f 100644 --- a/controllers/apps/cluster/restore_intent_test.go +++ b/controllers/apps/cluster/restore_intent_test.go @@ -47,6 +47,7 @@ func TestInjectRestoreIntentRemovesStaleOptionalAnnotations(t *testing.T) { cluster := &appsv1.Cluster{} cluster.Name = "test-cluster" cluster.Namespace = "test-ns" + cluster.UID = "cluster-uid" cluster.Spec.Restore = &appsv1.ClusterRestore{ Source: appsv1.ClusterRestoreSource{ APIGroup: testRestoreSourceAPIGroup, @@ -72,6 +73,7 @@ func TestInjectRestoreIntentRemovesStaleOptionalAnnotations(t *testing.T) { require.Equal(t, "backup", vct.Spec.DataSourceRef.Name) require.NotNil(t, vct.Spec.DataSourceRef.Namespace) require.Equal(t, "backup-ns", *vct.Spec.DataSourceRef.Namespace) + require.Equal(t, string(cluster.UID), vct.Annotations[constant.KBAppClusterUIDKey]) } func TestInjectRestoreIntentOmitsDataSourceRefNamespaceForSameNamespaceSource(t *testing.T) { @@ -154,6 +156,7 @@ func TestApplyClusterRestoreIntentCleansTemplatesAfterRestoreCompleted(t *testin Annotations: map[string]string{ constant.RestoreSourceKindAnnotationKey: testRestoreSourceKind, constant.RestorePITRAnnotationKey: "stale-pitr", + constant.KBAppClusterUIDKey: "cluster-uid", }, Spec: corev1.PersistentVolumeClaimSpec{ DataSourceRef: &corev1.TypedObjectReference{ @@ -172,6 +175,7 @@ func TestApplyClusterRestoreIntentCleansTemplatesAfterRestoreCompleted(t *testin require.Nil(t, vct.Annotations) require.NotContains(t, vct.Annotations, constant.RestoreSourceKindAnnotationKey) require.NotContains(t, vct.Annotations, constant.RestorePITRAnnotationKey) + require.NotContains(t, vct.Annotations, constant.KBAppClusterUIDKey) } func TestApplyClusterRestoreIntentKeepsNonRestoreDataSourceAfterRestoreCompleted(t *testing.T) { diff --git a/controllers/dataprotection/volumepopulator_controller.go b/controllers/dataprotection/volumepopulator_controller.go index 792fdd56c5c..e4811694246 100644 --- a/controllers/dataprotection/volumepopulator_controller.go +++ b/controllers/dataprotection/volumepopulator_controller.go @@ -156,35 +156,32 @@ func (r *VolumePopulatorReconciler) mapRestoreToPVCs(ctx context.Context, obj cl pvc := &corev1.PersistentVolumeClaim{} key := types.NamespacedName{Namespace: restore.Namespace, Name: owner.Name} if err := r.Client.Get(ctx, key, pvc); err != nil || pvc.UID != owner.UID || - !isClusterRestorePVC(pvc) || restore.Name != getPopulatePVCName(pvc.UID) { + !isClusterRestorePVC(pvc) || restore.Name != getPopulatePVCName(pvc.UID) || + restore.Labels[dptypes.ClusterUIDLabelKey] != clusterRestorePVCUID(pvc) { return nil } return []reconcile.Request{{NamespacedName: key}} } - owner := exactOwnerReference(restore.OwnerReferences, appsv1.GroupVersion.String(), "Component") - if owner == nil { + // A Component can disappear before a postReady Restore deletion event is + // observed. The Restore carries enough correlation identity to notify its + // PVC dependents without resolving the live Component. + if internalPostReadyRestoreOwner(restore) == nil { return nil } - comp := &appsv1.Component{} - key := types.NamespacedName{Namespace: restore.Namespace, Name: owner.Name} - if err := r.Client.Get(ctx, key, comp); err != nil || comp.UID != owner.UID || - restore.Name != postReadyRestoreName(comp.UID) { - return nil - } - clusterName := comp.Labels[constant.AppInstanceLabelKey] - componentName := restore.Labels[constant.KBAppComponentLabelKey] - ownerComponentName := comp.Labels[constant.KBAppComponentLabelKey] - if clusterName == "" || componentName == "" || ownerComponentName == "" || - restore.Labels[constant.AppInstanceLabelKey] != clusterName { + clusterName := restore.Labels[constant.AppInstanceLabelKey] + if clusterName == "" || restore.Labels[constant.KBAppComponentLabelKey] == "" { return nil } - // 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. + includeTerminal := !restore.DeletionTimestamp.IsZero() || + restore.Status.Phase == dpv1alpha1.RestorePhaseCompleted || + restore.Status.Phase == dpv1alpha1.RestorePhaseFailed + // The component label identifies the first source PVC, while the owner + // reference identifies the target Component. Redirected postReady restores + // can therefore have dependents in other Components of the same Cluster. return r.mapRestorePVCs(ctx, restore.Namespace, client.MatchingLabels{ constant.AppInstanceLabelKey: clusterName, - }) + }, restore.Labels[dptypes.ClusterUIDLabelKey], includeTerminal) } func (r *VolumePopulatorReconciler) mapComponentToPVCs(ctx context.Context, obj client.Object) []reconcile.Request { @@ -197,13 +194,16 @@ 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. + clusterOwner := exactOwnerReference(comp.OwnerReferences, appsv1.GroupVersion.String(), appsv1.ClusterKind) + if clusterOwner == nil || clusterOwner.Name != clusterName { + return nil + } + // A PVC can depend on another Component through redirected postReady. The + // dependency is not represented on the Component, so normal Component + // changes fan out to unfinished restore PVCs in the exact Cluster instance. return r.mapRestorePVCs(ctx, comp.Namespace, client.MatchingLabels{ constant.AppInstanceLabelKey: clusterName, - }) + }, string(clusterOwner.UID), false) } func (r *VolumePopulatorReconciler) mapClusterToPVCs(ctx context.Context, obj client.Object) []reconcile.Request { @@ -213,11 +213,14 @@ func (r *VolumePopulatorReconciler) mapClusterToPVCs(ctx context.Context, obj cl } return r.mapRestorePVCs(ctx, cluster.Namespace, client.MatchingLabels{ constant.AppInstanceLabelKey: cluster.Name, - }) + }, string(cluster.UID), false) } func (r *VolumePopulatorReconciler) mapRestorePVCs(ctx context.Context, namespace string, - labels client.MatchingLabels) []reconcile.Request { + labels client.MatchingLabels, clusterUID string, includeTerminal bool) []reconcile.Request { + if clusterUID == "" { + return nil + } list := &corev1.PersistentVolumeClaimList{} if err := r.Client.List(ctx, list, client.InNamespace(namespace), labels); err != nil { return nil @@ -225,7 +228,8 @@ func (r *VolumePopulatorReconciler) mapRestorePVCs(ctx context.Context, namespac requests := make([]reconcile.Request, 0, len(list.Items)) for i := range list.Items { pvc := &list.Items[i] - if !isClusterRestorePVC(pvc) || pvcRestoreTerminal(pvc) { + if !isClusterRestorePVC(pvc) || clusterRestorePVCUID(pvc) != clusterUID || + (!includeTerminal && pvcRestoreTerminal(pvc)) { continue } requests = append(requests, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(pvc)}) @@ -242,6 +246,9 @@ func isClusterRestorePVC(pvc *corev1.PersistentVolumeClaim) bool { if pvc.Labels[constant.AppInstanceLabelKey] == "" || pvc.Labels[constant.KBAppComponentLabelKey] == "" { return false } + if clusterRestorePVCUID(pvc) == "" { + return false + } for _, key := range []string{ constant.RestoreSourceAPIGroupAnnotationKey, constant.RestoreSourceKindAnnotationKey, @@ -259,6 +266,21 @@ func isClusterRestorePVC(pvc *corev1.PersistentVolumeClaim) bool { restoreComponent == pvc.Labels[constant.KBAppShardTemplateLabelKey] } +// clusterRestorePVCUID returns the Cluster correlation identity inherited +// from restore intent. A later verified label may repeat it, but conflicting +// identities are never accepted. +func clusterRestorePVCUID(pvc *corev1.PersistentVolumeClaim) string { + annotationUID := pvc.Annotations[constant.KBAppClusterUIDKey] + labelUID := pvc.Labels[dptypes.ClusterUIDLabelKey] + if annotationUID != "" && labelUID != "" && annotationUID != labelUID { + return "" + } + if labelUID != "" { + return labelUID + } + return annotationUID +} + func pvcRestoreTerminal(pvc *corev1.PersistentVolumeClaim) bool { condition := findPVCConditionByType(pvc, appsv1.ConditionTypeRestore) return condition != nil && (condition.Status == corev1.ConditionTrue || condition.Status == corev1.ConditionFalse) @@ -273,6 +295,16 @@ func exactOwnerReference(refs []metav1.OwnerReference, apiVersion, kind string) return nil } +func internalPostReadyRestoreOwner(restore *dpv1alpha1.Restore) *metav1.OwnerReference { + owner := exactOwnerReference(restore.OwnerReferences, appsv1.GroupVersion.String(), appsv1.ComponentKind) + if owner == nil || restore.Name != postReadyRestoreName(owner.UID) || + restore.Labels[dprestore.DataProtectionRestoreLabelKey] != restore.Name || + restore.Labels[dptypes.ComponentUIDLabelKey] != string(owner.UID) { + return nil + } + return owner +} + func restoreDependencyPredicate() predicate.Predicate { return predicate.Funcs{ CreateFunc: func(event.CreateEvent) bool { return true }, @@ -899,6 +931,9 @@ func internalRestoreLabels(pvc *corev1.PersistentVolumeClaim) map[string]string dprestore.DataProtectionRestoreNamespaceLabelKey: pvc.Namespace, dprestore.DataProtectionPopulatePVCLabelKey: getPopulatePVCName(pvc.UID), } + if clusterUID := clusterRestorePVCUID(pvc); clusterUID != "" { + labels[dptypes.ClusterUIDLabelKey] = clusterUID + } for _, key := range []string{ constant.AppInstanceLabelKey, constant.KBAppComponentLabelKey, @@ -1836,6 +1871,10 @@ func postReadyRestoreLabels(pvc *corev1.PersistentVolumeClaim, comp *appsv1.Comp labels := map[string]string{ dprestore.DataProtectionRestoreLabelKey: restoreName, dprestore.DataProtectionRestoreNamespaceLabelKey: pvc.Namespace, + dptypes.ComponentUIDLabelKey: string(comp.UID), + } + if clusterUID := clusterRestorePVCUID(pvc); clusterUID != "" { + labels[dptypes.ClusterUIDLabelKey] = clusterUID } for _, key := range []string{ constant.AppInstanceLabelKey, @@ -1947,6 +1986,7 @@ func (r *VolumePopulatorReconciler) getPopulatePVC(reqCtx intctrlutil.RequestCtx ObjectMeta: metav1.ObjectMeta{ Name: populatePVCName, Namespace: pvc.Namespace, + Labels: internalRestoreLabels(pvc), }, Spec: corev1.PersistentVolumeClaimSpec{ AccessModes: pvc.Spec.AccessModes, @@ -2004,6 +2044,7 @@ func (r *VolumePopulatorReconciler) getProvisionOnlyPVC(reqCtx intctrlutil.Reque ObjectMeta: metav1.ObjectMeta{ Name: populatePVCName, Namespace: pvc.Namespace, + Labels: internalRestoreLabels(pvc), }, Spec: corev1.PersistentVolumeClaimSpec{ AccessModes: pvc.Spec.AccessModes, diff --git a/controllers/dataprotection/volumepopulator_controller_test.go b/controllers/dataprotection/volumepopulator_controller_test.go index 2cc00988a72..8c5211fbb97 100644 --- a/controllers/dataprotection/volumepopulator_controller_test.go +++ b/controllers/dataprotection/volumepopulator_controller_test.go @@ -3970,7 +3970,10 @@ func TestMapExecutionRestoreToTargetPVC(t *testing.T) { pvc := dependencyRestorePVC("data-mysql-0", "mysql", "pvc-uid") restore := &dpv1alpha1.Restore{ObjectMeta: metav1.ObjectMeta{ Namespace: pvc.Namespace, Name: getPopulatePVCName(pvc.UID), - Labels: map[string]string{dprestore.DataProtectionRestoreLabelKey: getPopulatePVCName(pvc.UID)}, + Labels: map[string]string{ + dprestore.DataProtectionRestoreLabelKey: getPopulatePVCName(pvc.UID), + dptypes.ClusterUIDLabelKey: "cluster-uid", + }, OwnerReferences: []metav1.OwnerReference{{ APIVersion: corev1.SchemeGroupVersion.String(), Kind: "PersistentVolumeClaim", Name: pvc.Name, UID: pvc.UID, @@ -3983,12 +3986,15 @@ func TestMapExecutionRestoreToTargetPVC(t *testing.T) { badOwner := restore.DeepCopy() badOwner.OwnerReferences[0].UID = "another-pvc" require.Empty(t, reconciler.mapRestoreToPVCs(context.Background(), badOwner)) + wrongCluster := restore.DeepCopy() + wrongCluster.Labels[dptypes.ClusterUIDLabelKey] = "unmatched-cluster-uid" + require.Empty(t, reconciler.mapRestoreToPVCs(context.Background(), wrongCluster)) sourceRestore := restore.DeepCopy() sourceRestore.Labels = nil require.Empty(t, reconciler.mapRestoreToPVCs(context.Background(), sourceRestore)) } -func TestMapPostReadyRestoreToNonTerminalComponentPVCs(t *testing.T) { +func TestMapPostReadyRestoreWithoutComponent(t *testing.T) { comp := &kbappsv1.Component{ObjectMeta: metav1.ObjectMeta{ Namespace: "default", Name: "cluster-mysql", UID: "component-uid", Labels: map[string]string{ @@ -4000,32 +4006,59 @@ func TestMapPostReadyRestoreToNonTerminalComponentPVCs(t *testing.T) { terminal.Status.Conditions = []corev1.PersistentVolumeClaimCondition{{ Type: corev1.PersistentVolumeClaimConditionType(kbappsv1.ConditionTypeRestore), Status: corev1.ConditionTrue, }} + failed := dependencyRestorePVC("failed", "mysql", "failed-pvc") + failed.Status.Conditions = []corev1.PersistentVolumeClaimCondition{{ + Type: corev1.PersistentVolumeClaimConditionType(kbappsv1.ConditionTypeRestore), Status: corev1.ConditionFalse, + }} redirectTarget := dependencyRestorePVC("data-postgresql-0", "postgresql", "redirect-pvc") otherRedirectSource := dependencyRestorePVC("data-tikv-0", "tikv", "other-redirect-pvc") + foreign := dependencyRestorePVC("foreign", "mysql", "foreign-pvc") + foreign.Annotations[constant.KBAppClusterUIDKey] = "another-cluster-uid" + conflicting := dependencyRestorePVC("conflicting", "mysql", "conflicting-pvc") + conflicting.Labels[dptypes.ClusterUIDLabelKey] = "another-cluster-uid" restore := &dpv1alpha1.Restore{ObjectMeta: metav1.ObjectMeta{ Namespace: comp.Namespace, Name: postReadyRestoreName(comp.UID), - Labels: map[string]string{ - dprestore.DataProtectionRestoreLabelKey: postReadyRestoreName(comp.UID), - constant.AppInstanceLabelKey: "cluster", constant.KBAppComponentLabelKey: "mysql", - }, + Labels: postReadyRestoreLabels(running, comp), OwnerReferences: []metav1.OwnerReference{{ APIVersion: kbappsv1.GroupVersion.String(), Kind: "Component", Name: comp.Name, UID: comp.UID, }}, }} - reconciler := dependencyTestReconciler(t, comp, running, terminal, redirectTarget, otherRedirectSource) - require.ElementsMatch(t, []reconcile.Request{ + // The event must be sufficient after the Component itself has disappeared. + reconciler := dependencyTestReconciler(t, running, terminal, failed, redirectTarget, + otherRedirectSource, foreign, conflicting) + pending := []reconcile.Request{ {NamespacedName: client.ObjectKeyFromObject(running)}, {NamespacedName: client.ObjectKeyFromObject(redirectTarget)}, {NamespacedName: client.ObjectKeyFromObject(otherRedirectSource)}, - }, reconciler.mapRestoreToPVCs(context.Background(), restore)) + } + require.ElementsMatch(t, pending, reconciler.mapRestoreToPVCs(context.Background(), restore)) redirected := restore.DeepCopy() redirected.Labels[constant.KBAppComponentLabelKey] = "postgresql" - require.ElementsMatch(t, []reconcile.Request{ - {NamespacedName: client.ObjectKeyFromObject(running)}, - {NamespacedName: client.ObjectKeyFromObject(redirectTarget)}, - {NamespacedName: client.ObjectKeyFromObject(otherRedirectSource)}, - }, reconciler.mapRestoreToPVCs(context.Background(), redirected)) + require.ElementsMatch(t, pending, reconciler.mapRestoreToPVCs(context.Background(), redirected)) + + all := append(append([]reconcile.Request(nil), pending...), + reconcile.Request{NamespacedName: client.ObjectKeyFromObject(terminal)}, + reconcile.Request{NamespacedName: client.ObjectKeyFromObject(failed)}) + for _, phase := range []dpv1alpha1.RestorePhase{dpv1alpha1.RestorePhaseCompleted, dpv1alpha1.RestorePhaseFailed} { + obj := restore.DeepCopy() + obj.Status.Phase = phase + require.ElementsMatch(t, all, reconciler.mapRestoreToPVCs(context.Background(), obj)) + } + deleting := restore.DeepCopy() + now := metav1.Now() + deleting.DeletionTimestamp = &now + require.ElementsMatch(t, all, reconciler.mapRestoreToPVCs(context.Background(), deleting)) + + wrongOwner := restore.DeepCopy() + wrongOwner.OwnerReferences[0].UID = "another-component" + require.Empty(t, reconciler.mapRestoreToPVCs(context.Background(), wrongOwner)) + wrongIdentity := restore.DeepCopy() + wrongIdentity.Labels[dptypes.ComponentUIDLabelKey] = "another-component" + require.Empty(t, reconciler.mapRestoreToPVCs(context.Background(), wrongIdentity)) + wrongCluster := restore.DeepCopy() + wrongCluster.Labels[dptypes.ClusterUIDLabelKey] = "unmatched-cluster-uid" + require.Empty(t, reconciler.mapRestoreToPVCs(context.Background(), wrongCluster)) } func TestMapComponentAndClusterDependencies(t *testing.T) { @@ -4037,14 +4070,22 @@ func TestMapComponentAndClusterDependencies(t *testing.T) { terminal.Status.Conditions = []corev1.PersistentVolumeClaimCondition{{ Type: corev1.PersistentVolumeClaimConditionType(kbappsv1.ConditionTypeRestore), Status: corev1.ConditionFalse, }} + foreign := dependencyRestorePVC("foreign", "mysql", "foreign-pvc") + foreign.Annotations[constant.KBAppClusterUIDKey] = "another-cluster-uid" comp := &kbappsv1.Component{ObjectMeta: metav1.ObjectMeta{ - Namespace: "default", Name: "cluster-mysql", + Namespace: "default", Name: "cluster-mysql", UID: "component-uid", Labels: map[string]string{ constant.AppInstanceLabelKey: "cluster", constant.KBAppComponentLabelKey: "mysql", }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: kbappsv1.GroupVersion.String(), Kind: kbappsv1.ClusterKind, + Name: "cluster", UID: "cluster-uid", + }}, + }} + cluster := &kbappsv1.Cluster{ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", Name: "cluster", UID: "cluster-uid", }} - cluster := &kbappsv1.Cluster{ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "cluster"}} - reconciler := dependencyTestReconciler(t, mysql, postgresql, invalid, terminal) + reconciler := dependencyTestReconciler(t, mysql, postgresql, invalid, terminal, foreign) require.ElementsMatch(t, []reconcile.Request{ {NamespacedName: client.ObjectKeyFromObject(mysql)}, @@ -4085,6 +4126,29 @@ func TestClusterRestorePVCIdentitySupportsSharding(t *testing.T) { } } +func TestInternalRestoreCorrelationIdentity(t *testing.T) { + pvc := dependencyRestorePVC("data-mysql-0", "mysql", "pvc-uid") + comp := &kbappsv1.Component{ObjectMeta: metav1.ObjectMeta{ + Namespace: pvc.Namespace, Name: "cluster-mysql", UID: "component-uid", + }} + + require.Equal(t, "cluster-uid", internalRestoreLabels(pvc)[dptypes.ClusterUIDLabelKey]) + postReadyLabels := postReadyRestoreLabels(pvc, comp) + require.Equal(t, "cluster-uid", postReadyLabels[dptypes.ClusterUIDLabelKey]) + require.Equal(t, string(comp.UID), postReadyLabels[dptypes.ComponentUIDLabelKey]) + + reconciler := dependencyTestReconciler(t) + helper, err := reconciler.getProvisionOnlyPVC( + intctrlutil.RequestCtx{Ctx: context.Background()}, pvc, "") + require.NoError(t, err) + require.Equal(t, "cluster-uid", helper.Labels[dptypes.ClusterUIDLabelKey]) + + conflicting := pvc.DeepCopy() + conflicting.Labels[dptypes.ClusterUIDLabelKey] = "another-cluster-uid" + require.Empty(t, clusterRestorePVCUID(conflicting)) + require.False(t, isClusterRestorePVC(conflicting)) +} + func TestDependencyPredicates(t *testing.T) { now := metav1.NewTime(time.Now()) restoreOld := &dpv1alpha1.Restore{} @@ -4143,6 +4207,7 @@ func dependencyRestorePVC(name, componentName string, uid types.UID) *corev1.Per constant.AppInstanceLabelKey: "cluster", constant.KBAppComponentLabelKey: componentName, }, Annotations: map[string]string{ + constant.KBAppClusterUIDKey: "cluster-uid", constant.RestoreSourceAPIGroupAnnotationKey: dptypes.DataprotectionAPIGroup, constant.RestoreSourceKindAnnotationKey: dptypes.BackupKind, constant.RestoreSourceNameAnnotationKey: "backup", diff --git a/pkg/dataprotection/types/constant.go b/pkg/dataprotection/types/constant.go index c24e9834c7a..b653dc6e86a 100644 --- a/pkg/dataprotection/types/constant.go +++ b/pkg/dataprotection/types/constant.go @@ -86,6 +86,8 @@ const ( const ( // ClusterUIDLabelKey specifies the cluster UID label key. ClusterUIDLabelKey = "dataprotection.kubeblocks.io/cluster-uid" + // ComponentUIDLabelKey specifies the component UID label key. + ComponentUIDLabelKey = "dataprotection.kubeblocks.io/component-uid" // BackupNameLabelKey specifies the backup name label key. BackupNameLabelKey = "dataprotection.kubeblocks.io/backup-name" // BackupNamespaceLabelKey specifies the backup namespace label key. From e309a7e0953d954aaa8cbac47b4c90a7f85a6f28 Mon Sep 17 00:00:00 2001 From: Leon Date: Fri, 4 Sep 2026 11:49:30 +0800 Subject: [PATCH 2/3] fix(dataprotection): protect the cluster restore lifecycle --- cmd/dataprotection/main.go | 8 + .../cluster_restore_controller.go | 194 +++++++++ .../cluster_restore_controller_test.go | 149 +++++++ controllers/dataprotection/suite_test.go | 6 + .../volumepopulator_controller.go | 412 +++++++++++++++++- .../volumepopulator_controller_test.go | 388 +++++++++++++++++ pkg/dataprotection/types/constant.go | 3 + 7 files changed, 1142 insertions(+), 18 deletions(-) create mode 100644 controllers/dataprotection/cluster_restore_controller.go create mode 100644 controllers/dataprotection/cluster_restore_controller_test.go diff --git a/cmd/dataprotection/main.go b/cmd/dataprotection/main.go index 4d7cc2c09ca..c28c7004d86 100644 --- a/cmd/dataprotection/main.go +++ b/cmd/dataprotection/main.go @@ -340,6 +340,14 @@ func main() { os.Exit(1) } + if err = (&dpcontrollers.ClusterRestoreReconciler{ + Client: mgr.GetClient(), + Recorder: mgr.GetEventRecorderFor("cluster-restore-controller"), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "ClusterRestore") + os.Exit(1) + } + if err = (&dpcontrollers.BackupScheduleReconciler{ Client: dputils.NewCompatClient(mgr.GetClient()), Scheme: mgr.GetScheme(), diff --git a/controllers/dataprotection/cluster_restore_controller.go b/controllers/dataprotection/cluster_restore_controller.go new file mode 100644 index 00000000000..694ba5f5d7d --- /dev/null +++ b/controllers/dataprotection/cluster_restore_controller.go @@ -0,0 +1,194 @@ +/* +Copyright (C) 2022-2026 ApeCloud Co., Ltd + +This file is part of KubeBlocks project + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU Affero General Public License as published by +the Free Software Foundation, either version 3 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Affero General Public License for more details. + +You should have received a copy of the GNU Affero General Public License +along with this program. If not, see . +*/ + +package dataprotection + +import ( + "context" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + appsv1 "github.com/apecloud/kubeblocks/apis/apps/v1" + dpv1alpha1 "github.com/apecloud/kubeblocks/apis/dataprotection/v1alpha1" + "github.com/apecloud/kubeblocks/pkg/constant" + intctrlutil "github.com/apecloud/kubeblocks/pkg/controllerutil" + dprestore "github.com/apecloud/kubeblocks/pkg/dataprotection/restore" + dptypes "github.com/apecloud/kubeblocks/pkg/dataprotection/types" +) + +// ClusterRestoreReconciler coordinates the Cluster-level restore lifecycle. +type ClusterRestoreReconciler struct { + client.Client + Recorder record.EventRecorder +} + +// +kubebuilder:rbac:groups=apps.kubeblocks.io,resources=clusters,verbs=get;list;watch;patch;update +// +kubebuilder:rbac:groups=apps.kubeblocks.io,resources=clusters/finalizers,verbs=update;patch +// +kubebuilder:rbac:groups=dataprotection.kubeblocks.io,resources=restores,verbs=get;list;watch +// +kubebuilder:rbac:groups=core,resources=persistentvolumeclaims,verbs=get;list;watch + +func (r *ClusterRestoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + reqCtx := intctrlutil.RequestCtx{ + Ctx: ctx, Req: req, + Log: log.FromContext(ctx).WithValues("cluster-restore", req.NamespacedName), + Recorder: r.Recorder, + } + cluster := &appsv1.Cluster{} + if err := r.Client.Get(ctx, req.NamespacedName, cluster); err != nil { + return intctrlutil.CheckedRequeueWithError(err, reqCtx.Log, "") + } + + if cluster.DeletionTimestamp.IsZero() { + if clusterRestoreConditionActive(cluster) { + return r.ensureFinalizer(reqCtx, cluster) + } + } else if !controllerutil.ContainsFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) { + return intctrlutil.Reconciled() + } + + 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 hasResources { + return r.ensureFinalizer(reqCtx, cluster) + } + return r.removeFinalizer(reqCtx, cluster) + } + if hasResources { + return intctrlutil.RequeueAfter(reconcileInterval, reqCtx.Log, + "waiting for restore resource owners to finish Cluster termination") + } + return r.removeFinalizer(reqCtx, cluster) +} + +func (r *ClusterRestoreReconciler) SetupWithManager(mgr ctrl.Manager) error { + return intctrlutil.NewControllerManagedBy(mgr). + Named("cluster_restore"). + For(&appsv1.Cluster{}). + Watches(&dpv1alpha1.Restore{}, handler.EnqueueRequestsFromMapFunc(r.mapObjectToCluster)). + Watches(&corev1.PersistentVolumeClaim{}, handler.EnqueueRequestsFromMapFunc(r.mapObjectToCluster)). + Complete(r) +} + +func (r *ClusterRestoreReconciler) mapObjectToCluster(_ context.Context, obj client.Object) []reconcile.Request { + clusterName := obj.GetLabels()[constant.AppInstanceLabelKey] + if clusterName == "" { + return nil + } + return []reconcile.Request{{NamespacedName: client.ObjectKey{Namespace: obj.GetNamespace(), Name: clusterName}}} +} + +func clusterRestoreConditionActive(cluster *appsv1.Cluster) bool { + if cluster.Spec.Restore == nil { + return false + } + condition := meta.FindStatusCondition(cluster.Status.Conditions, appsv1.ConditionTypeRestore) + // Restore=False is terminal for status aggregation, but the failed Cluster + // still carries initial-restore intent. Keep protection until Cluster deletion + // so remaining PVC restores never lose the lifecycle coordinator. + return condition == nil || condition.Status != metav1.ConditionTrue +} + +func (r *ClusterRestoreReconciler) hasRestoreResources(ctx context.Context, + cluster *appsv1.Cluster) (bool, error) { + // Inspect PVCs before Restores. VP releases temporary target protection only + // after observing postReady Restore, so this order closes the handoff window. + pvcs := &corev1.PersistentVolumeClaimList{} + if err := r.Client.List(ctx, pvcs, client.InNamespace(cluster.Namespace), client.MatchingLabels{ + constant.AppInstanceLabelKey: cluster.Name, + }); err != nil { + return false, err + } + for i := range pvcs.Items { + pvc := &pvcs.Items[i] + if pvc.Labels[dptypes.ClusterUIDLabelKey] != string(cluster.UID) { + continue + } + if isClusterRestoreHelperPVC(pvc) || + (isClusterRestoreTargetPVC(pvc) && controllerutil.ContainsFinalizer(pvc, dptypes.DataProtectionFinalizerName)) { + return true, nil + } + } + + restores := &dpv1alpha1.RestoreList{} + if err := r.Client.List(ctx, restores, client.InNamespace(cluster.Namespace), client.MatchingLabels{ + constant.AppInstanceLabelKey: cluster.Name, + }); err != nil { + return false, err + } + for i := range restores.Items { + restore := &restores.Items[i] + if restore.Labels[dprestore.DataProtectionRestoreLabelKey] != restore.Name { + continue + } + owned := restore.Labels[dptypes.ClusterUIDLabelKey] == string(cluster.UID) + terminal := restore.Status.Phase == dpv1alpha1.RestorePhaseCompleted || + restore.Status.Phase == dpv1alpha1.RestorePhaseFailed + if owned && (!cluster.DeletionTimestamp.IsZero() || !terminal || !restore.DeletionTimestamp.IsZero()) { + return true, nil + } + } + return false, nil +} + +func (r *ClusterRestoreReconciler) ensureFinalizer(reqCtx intctrlutil.RequestCtx, + cluster *appsv1.Cluster) (ctrl.Result, error) { + if controllerutil.ContainsFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) { + return intctrlutil.Reconciled() + } + patch := client.MergeFromWithOptions(cluster.DeepCopy(), client.MergeFromWithOptimisticLock{}) + controllerutil.AddFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) + if err := r.Client.Patch(reqCtx.Ctx, cluster, patch); err != nil { + return intctrlutil.CheckedRequeueWithError(err, reqCtx.Log, "failed to add Cluster restore-protection finalizer") + } + return intctrlutil.Reconciled() +} + +func (r *ClusterRestoreReconciler) removeFinalizer(reqCtx intctrlutil.RequestCtx, + cluster *appsv1.Cluster) (ctrl.Result, error) { + if !controllerutil.ContainsFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) { + return intctrlutil.Reconciled() + } + patch := client.MergeFromWithOptions(cluster.DeepCopy(), client.MergeFromWithOptimisticLock{}) + controllerutil.RemoveFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) + if err := r.Client.Patch(reqCtx.Ctx, cluster, patch); err != nil { + return intctrlutil.CheckedRequeueWithError(err, reqCtx.Log, "failed to remove Cluster restore-protection finalizer") + } + return intctrlutil.Reconciled() +} + +func isClusterRestoreHelperPVC(pvc *corev1.PersistentVolumeClaim) bool { + return pvc.Labels[dprestore.DataProtectionPopulatePVCLabelKey] != "" +} + +func isClusterRestoreTargetPVC(pvc *corev1.PersistentVolumeClaim) bool { + return pvc.Spec.DataSourceRef != nil && pvc.Spec.DataSourceRef.APIGroup != nil && + *pvc.Spec.DataSourceRef.APIGroup == dptypes.DataprotectionAPIGroup +} diff --git a/controllers/dataprotection/cluster_restore_controller_test.go b/controllers/dataprotection/cluster_restore_controller_test.go new file mode 100644 index 00000000000..051960f4eea --- /dev/null +++ b/controllers/dataprotection/cluster_restore_controller_test.go @@ -0,0 +1,149 @@ +/* +Copyright (C) 2022-2026 ApeCloud Co., Ltd + +This file is part of KubeBlocks project + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU Affero General Public License as published by +the Free Software Foundation, either version 3 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Affero General Public License for more details. + +You should have received a copy of the GNU Affero General Public License +along with this program. If not, see . +*/ + +package dataprotection + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + appsv1 "github.com/apecloud/kubeblocks/apis/apps/v1" + dpv1alpha1 "github.com/apecloud/kubeblocks/apis/dataprotection/v1alpha1" + dptypes "github.com/apecloud/kubeblocks/pkg/dataprotection/types" +) + +func TestClusterRestoreProtectionLifecycle(t *testing.T) { + for _, tc := range []struct { + name string + deleting bool + protected bool + status metav1.ConditionStatus + resource string + wantKeep bool + wantWait bool + }{ + {"initial intent", false, false, metav1.ConditionUnknown, "", true, false}, + {"deletion without protection", true, false, metav1.ConditionUnknown, "target", false, false}, + {"failed restore", false, true, metav1.ConditionFalse, "", true, false}, + {"successful restore", false, true, metav1.ConditionTrue, "completed restore", false, false}, + {"target still protected", false, true, metav1.ConditionTrue, "target", true, false}, + {"helper remains", false, true, metav1.ConditionTrue, "helper", true, false}, + {"execution still running", false, true, metav1.ConditionTrue, "running restore", true, false}, + {"deletion waits for target", true, true, metav1.ConditionTrue, "target", true, true}, + {"deletion waits for helper", true, true, metav1.ConditionTrue, "helper", true, true}, + {"deletion waits for failed restore", true, true, metav1.ConditionTrue, "failed restore", true, true}, + {"deletion waits for completed restore", true, true, metav1.ConditionTrue, "completed restore", true, true}, + {"deletion cleanup finished", true, true, metav1.ConditionTrue, "", false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + scheme, cluster, _, _, target := parentRestoreObjects(t) + cluster.Finalizers = []string{"example.io/app-owner"} + if tc.protected { + cluster.Finalizers = append(cluster.Finalizers, dptypes.RestoreProtectionFinalizerName) + } + if tc.deleting { + now := metav1.Now() + cluster.DeletionTimestamp = &now + } + cluster.Status.Conditions = []metav1.Condition{{ + Type: appsv1.ConditionTypeRestore, Status: tc.status, + }} + var resource client.Object + switch tc.resource { + case "target": + target.Finalizers = []string{dptypes.DataProtectionFinalizerName} + resource = target + case "helper": + resource = restoreHelperForTarget(target, cluster) + case "running restore", "failed restore", "completed restore": + restore := executionRestoreForTarget(target, cluster) + switch tc.resource { + case "failed restore": + restore.Status.Phase = dpv1alpha1.RestorePhaseFailed + case "completed restore": + restore.Status.Phase = dpv1alpha1.RestorePhaseCompleted + default: + restore.Status.Phase = dpv1alpha1.RestorePhaseRunning + } + restore.Finalizers = []string{dptypes.DataProtectionFinalizerName} + resource = restore + } + objects := []client.Object{cluster} + if resource != nil { + objects = append(objects, resource) + } + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + if resource != nil { + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(resource), resource)) + } + reconciler := &ClusterRestoreReconciler{Client: cli} + + result, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cluster)}) + require.NoError(t, err) + require.Equal(t, tc.wantWait, result.RequeueAfter > 0) + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(cluster), cluster)) + expected := []string{"example.io/app-owner"} + if tc.wantKeep { + expected = append(expected, dptypes.RestoreProtectionFinalizerName) + } + require.ElementsMatch(t, expected, cluster.Finalizers) + if resource != nil { + current := resource.DeepCopyObject().(client.Object) + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(resource), current)) + require.Equal(t, resource, current) + } + }) + } +} + +func TestClusterRestoreControllerIgnoresResourcesWithoutExactClusterUID(t *testing.T) { + for _, uid := range []string{"", "another-cluster-uid"} { + t.Run("uid="+uid, func(t *testing.T) { + ctx := context.Background() + scheme, cluster, _, _, target := parentRestoreObjects(t) + now := metav1.Now() + cluster.DeletionTimestamp = &now + cluster.Finalizers = append(cluster.Finalizers, "example.io/app-owner") + target.Finalizers = []string{dptypes.DataProtectionFinalizerName} + helper := restoreHelperForTarget(target, cluster) + restore := executionRestoreForTarget(target, cluster) + for _, obj := range []client.Object{target, helper, restore} { + obj.GetLabels()[dptypes.ClusterUIDLabelKey] = uid + } + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster, target, helper, restore).Build() + reconciler := &ClusterRestoreReconciler{Client: cli} + + _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cluster)}) + require.NoError(t, err) + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(cluster), cluster)) + require.Equal(t, []string{"example.io/app-owner"}, cluster.Finalizers) + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(target), &corev1.PersistentVolumeClaim{})) + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(helper), &corev1.PersistentVolumeClaim{})) + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(restore), &dpv1alpha1.Restore{})) + }) + } +} diff --git a/controllers/dataprotection/suite_test.go b/controllers/dataprotection/suite_test.go index 337d93d66f2..9da6f79fa2b 100644 --- a/controllers/dataprotection/suite_test.go +++ b/controllers/dataprotection/suite_test.go @@ -187,6 +187,12 @@ var _ = BeforeSuite(func() { }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) + err = (&ClusterRestoreReconciler{ + Client: k8sClient, + Recorder: k8sManager.GetEventRecorderFor("cluster-restore-controller"), + }).SetupWithManager(k8sManager) + Expect(err).ToNot(HaveOccurred()) + err = (&BackupScheduleReconciler{ Client: k8sClient, Scheme: k8sManager.GetScheme(), diff --git a/controllers/dataprotection/volumepopulator_controller.go b/controllers/dataprotection/volumepopulator_controller.go index e4811694246..4739a26865b 100644 --- a/controllers/dataprotection/volumepopulator_controller.go +++ b/controllers/dataprotection/volumepopulator_controller.go @@ -60,7 +60,7 @@ import ( viper "github.com/apecloud/kubeblocks/pkg/viperx" ) -// VolumePopulatorReconciler reconciles PVCs with Backup or Restore data sources. +// VolumePopulatorReconciler coordinates data population and restore for PVCs. type VolumePopulatorReconciler struct { client.Client Scheme *runtime.Scheme @@ -88,7 +88,7 @@ type pvcRestoreDecision struct { // +kubebuilder:rbac:groups=core,resources=persistentvolumeclaims/status,verbs=get;update;patch // +kubebuilder:rbac:groups=core,resources=persistentvolumeclaims/finalizers,verbs=update -// +kubebuilder:rbac:groups=dataprotection.kubeblocks.io,resources=restores,verbs=get;list;watch +// +kubebuilder:rbac:groups=dataprotection.kubeblocks.io,resources=restores,verbs=get;list;watch;delete // +kubebuilder:rbac:groups=apps.kubeblocks.io,resources=clusters,verbs=get;list;watch // +kubebuilder:rbac:groups=apps.kubeblocks.io,resources=components,verbs=get;list;watch // +kubebuilder:rbac:groups=apps.kubeblocks.io,resources=componentdefinitions,verbs=get;list;watch @@ -213,7 +213,7 @@ func (r *VolumePopulatorReconciler) mapClusterToPVCs(ctx context.Context, obj cl } return r.mapRestorePVCs(ctx, cluster.Namespace, client.MatchingLabels{ constant.AppInstanceLabelKey: cluster.Name, - }, string(cluster.UID), false) + }, string(cluster.UID), !cluster.DeletionTimestamp.IsZero()) } func (r *VolumePopulatorReconciler) mapRestorePVCs(ctx context.Context, namespace string, @@ -343,7 +343,9 @@ func clusterDependencyPredicate() predicate.Predicate { oldCluster, oldOK := e.ObjectOld.(*appsv1.Cluster) newCluster, newOK := e.ObjectNew.(*appsv1.Cluster) return oldOK && newOK && (oldCluster.Status.Phase != newCluster.Status.Phase || - !reflect.DeepEqual(oldCluster.DeletionTimestamp, newCluster.DeletionTimestamp)) + !reflect.DeepEqual(oldCluster.DeletionTimestamp, newCluster.DeletionTimestamp) || + controllerutil.ContainsFinalizer(oldCluster, dptypes.RestoreProtectionFinalizerName) != + controllerutil.ContainsFinalizer(newCluster, dptypes.RestoreProtectionFinalizerName)) }, } } @@ -387,6 +389,10 @@ func (r *VolumePopulatorReconciler) MatchToPopulate(pvc *corev1.PersistentVolume } func (r *VolumePopulatorReconciler) syncPVC(reqCtx intctrlutil.RequestCtx, pvc *corev1.PersistentVolumeClaim) error { + // Kubernetes does not allow registering new finalizers after deletion starts. + if !pvc.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(pvc, dptypes.DataProtectionFinalizerName) { + return nil + } matched, err := r.MatchToPopulate(pvc) if err != nil { return err @@ -394,6 +400,10 @@ func (r *VolumePopulatorReconciler) syncPVC(reqCtx intctrlutil.RequestCtx, pvc * if !matched { return nil } + terminated, err := r.handleRestoreClusterLifecycle(reqCtx, pvc) + if err != nil || terminated { + return err + } // A non-deleting bound PVC with a terminal Restore condition does not need // its source Backup/Restore. Populating can finish while postReady is pending. if pvc.Spec.VolumeName != "" && pvc.DeletionTimestamp.IsZero() && pvcRestoreTerminal(pvc) { @@ -418,6 +428,347 @@ func (r *VolumePopulatorReconciler) syncPVC(reqCtx intctrlutil.RequestCtx, pvc * return nil } +// handleRestoreClusterLifecycle validates the Cluster identity and protection +// before restore work starts, and initiates owner-driven cleanup when the +// Cluster is deleting. Target PVC deletion alone is not a termination signal. +func (r *VolumePopulatorReconciler) handleRestoreClusterLifecycle(reqCtx intctrlutil.RequestCtx, + pvc *corev1.PersistentVolumeClaim) (bool, error) { + clusterName := pvc.Labels[constant.AppInstanceLabelKey] + componentName := pvc.Labels[constant.KBAppComponentLabelKey] + if clusterName == "" || componentName == "" { + return false, nil + } + hasClusterIdentity := pvc.Annotations[constant.KBAppClusterUIDKey] != "" || + pvc.Labels[dptypes.ClusterUIDLabelKey] != "" + // App labels alone do not establish Cluster restore identity; standalone DP + // restores may use the same labels. + if !hasClusterIdentity { + return false, nil + } + + cluster := &appsv1.Cluster{} + clusterKey := types.NamespacedName{Namespace: pvc.Namespace, Name: clusterName} + if err := r.Client.Get(reqCtx.Ctx, clusterKey, cluster); err != nil { + if apierrors.IsNotFound(err) { + if !controllerutil.ContainsFinalizer(pvc, dptypes.DataProtectionFinalizerName) { + return true, nil + } + // Registration can lose the race with Cluster deletion, but no work + // starts until a later reconcile has rechecked the parent. + if releaseErr := r.releaseUnusedTargetFinalizer(reqCtx, pvc); releaseErr != nil { + return true, restoreParentRequeue(releaseErr) + } + return true, nil + } + return false, restoreParentRequeue(err) + } + for _, clusterUID := range []string{ + pvc.Annotations[constant.KBAppClusterUIDKey], + pvc.Labels[dptypes.ClusterUIDLabelKey], + } { + if clusterUID != "" && clusterUID != string(cluster.UID) { + return false, restoreParentRequeue(fmt.Errorf( + "PVC %s/%s identifies Cluster %s/%s UID %s, not current UID %s", + pvc.Namespace, pvc.Name, cluster.Namespace, cluster.Name, clusterUID, cluster.UID)) + } + } + if !cluster.DeletionTimestamp.IsZero() { + return r.terminateClusterVolumePopulation(reqCtx, pvc, cluster) + } + + committed := volumePopulationIdentityCommitted(pvc, cluster) + if committed { + if _, err := r.committedVolumePopulationComponent(reqCtx.Ctx, pvc, cluster); err != nil { + return false, restoreParentRequeue(err) + } + } + // Aggregate restore status gates normal progression, not owner cleanup. + if !clusterRestoreConditionActive(cluster) && !pvcRestoreTerminal(pvc) { + return false, intctrlutil.NewRequeueError(reconcileInterval, "Cluster restore is no longer active") + } + if !committed { + if pvcRestoreTerminal(pvc) && !controllerutil.ContainsFinalizer(pvc, dptypes.DataProtectionFinalizerName) { + return false, nil + } + comp, err := r.validateClusterRestorePVCOwnership(reqCtx.Ctx, pvc, cluster) + if err != nil { + return false, restoreParentRequeue(err) + } + if err = r.registerVolumePopulation(reqCtx.Ctx, pvc, cluster, comp); err != nil { + return false, restoreParentRequeue(err) + } + return false, intctrlutil.NewRequeueError(reconcileInterval, "waiting for target PVC restore protection") + } + if !controllerutil.ContainsFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) { + if pvcRestoreTerminal(pvc) && !controllerutil.ContainsFinalizer(pvc, dptypes.DataProtectionFinalizerName) { + return false, nil + } + return false, intctrlutil.NewRequeueError(reconcileInterval, + "waiting for Cluster restore-protection finalizer") + } + if !pvcPopulateReleased(pvc) && !pvcRestoreTerminal(pvc) { + if err := r.ensureTargetFinalizer(reqCtx, pvc); err != nil { + return false, err + } + } + return false, nil +} + +// releaseUnusedTargetFinalizer rolls back an unused registration after its +// Cluster disappears. Existing population or Restore resources retain it. +func (r *VolumePopulatorReconciler) releaseUnusedTargetFinalizer(reqCtx intctrlutil.RequestCtx, + pvc *corev1.PersistentVolumeClaim) error { + clusterUID := clusterRestorePVCUID(pvc) + if clusterUID == "" || pvc.Labels[dptypes.ComponentUIDLabelKey] == "" { + return fmt.Errorf("restore PVC %s/%s has no committed parent identity", pvc.Namespace, pvc.Name) + } + if r.ContainPopulatingCondition(pvc) && !pvcPopulateReleased(pvc) { + return fmt.Errorf("cluster is missing after population started for PVC %s/%s", pvc.Namespace, pvc.Name) + } + key := types.NamespacedName{Namespace: pvc.Namespace, Name: getPopulatePVCName(pvc.UID)} + for _, obj := range []client.Object{&corev1.PersistentVolumeClaim{}, &dpv1alpha1.Restore{}} { + if err := r.Client.Get(reqCtx.Ctx, key, obj); !apierrors.IsNotFound(err) { + if err != nil { + return err + } + return fmt.Errorf("cluster is missing while restore resources for PVC %s/%s remain", pvc.Namespace, pvc.Name) + } + } + + list := &dpv1alpha1.RestoreList{} + if err := r.Client.List(reqCtx.Ctx, list, client.InNamespace(pvc.Namespace), client.MatchingLabels{ + constant.AppInstanceLabelKey: pvc.Labels[constant.AppInstanceLabelKey], + dptypes.ClusterUIDLabelKey: clusterUID, + }); err != nil { + return err + } + for i := range list.Items { + if internalPostReadyRestoreOwner(&list.Items[i]) != nil { + return fmt.Errorf("cluster is missing while postReady Restore %s/%s remains", + list.Items[i].Namespace, list.Items[i].Name) + } + } + return r.releaseTargetPVC(reqCtx, pvc) +} + +func volumePopulationIdentityCommitted(pvc *corev1.PersistentVolumeClaim, cluster *appsv1.Cluster) bool { + return pvc.Labels[dptypes.ClusterUIDLabelKey] == string(cluster.UID) && + pvc.Labels[dptypes.ComponentUIDLabelKey] != "" +} + +// committedVolumePopulationComponent resolves the Component identity recorded +// before VP creates restore resources. Retention may detach the workload owner +// chain, but it does not change this identity. +func (r *VolumePopulatorReconciler) committedVolumePopulationComponent(ctx context.Context, + pvc *corev1.PersistentVolumeClaim, cluster *appsv1.Cluster) (*appsv1.Component, error) { + componentName := pvc.Labels[constant.KBAppComponentLabelKey] + comp := &appsv1.Component{} + key := types.NamespacedName{ + Namespace: pvc.Namespace, + Name: constant.GenerateClusterComponentName(cluster.Name, componentName), + } + if err := r.Client.Get(ctx, key, comp); err != nil { + return nil, err + } + if string(comp.UID) != pvc.Labels[dptypes.ComponentUIDLabelKey] { + return nil, fmt.Errorf("restore PVC %s/%s Component UID changed from %s to %s", + pvc.Namespace, pvc.Name, pvc.Labels[dptypes.ComponentUIDLabelKey], comp.UID) + } + return comp, nil +} + +func restoreParentRequeue(err error) error { + return intctrlutil.NewRequeueError(reconcileInterval, err.Error()) +} + +func (r *VolumePopulatorReconciler) terminateClusterVolumePopulation(reqCtx intctrlutil.RequestCtx, + pvc *corev1.PersistentVolumeClaim, cluster *appsv1.Cluster) (bool, error) { + err := r.cleanupClusterVolumePopulation(reqCtx, pvc, cluster) + if err != nil && !intctrlutil.IsRequeueError(err) { + err = restoreParentRequeue(err) + } + return true, err +} + +func (r *VolumePopulatorReconciler) cleanupClusterVolumePopulation(reqCtx intctrlutil.RequestCtx, + pvc *corev1.PersistentVolumeClaim, cluster *appsv1.Cluster) error { + pending, err := r.deleteExecutionRestoreAndWait(reqCtx.Ctx, pvc, cluster) + if err != nil { + return err + } + postReadyPending, err := r.deleteClusterPostReadyRestoresAndWait(reqCtx.Ctx, cluster) + if err != nil { + return err + } + if pending || postReadyPending { + return intctrlutil.NewRequeueError(reconcileInterval, "waiting for Restore owners to finish termination") + } + pending, err = r.deletePopulatePVCAndWait(reqCtx.Ctx, pvc, cluster) + if err != nil { + return err + } + if pending { + return intctrlutil.NewRequeueError(reconcileInterval, "waiting for helper PVC to disappear") + } + return r.releaseTargetPVC(reqCtx, pvc) +} + +func (r *VolumePopulatorReconciler) deleteExecutionRestoreAndWait(ctx context.Context, + pvc *corev1.PersistentVolumeClaim, cluster *appsv1.Cluster) (bool, error) { + restore := &dpv1alpha1.Restore{} + key := types.NamespacedName{Namespace: pvc.Namespace, Name: getPopulatePVCName(pvc.UID)} + if err := r.Client.Get(ctx, key, restore); err != nil { + return false, client.IgnoreNotFound(err) + } + if restore.Labels[dptypes.ClusterUIDLabelKey] != string(cluster.UID) || + restore.Labels[dprestore.DataProtectionRestoreLabelKey] != restore.Name || + !hasExactOwnerReference(restore.OwnerReferences, corev1.SchemeGroupVersion.String(), + "PersistentVolumeClaim", pvc.Name, pvc.UID) { + return false, fmt.Errorf("refusing to delete execution Restore %s/%s without exact VP ownership", + restore.Namespace, restore.Name) + } + if restore.DeletionTimestamp.IsZero() { + if err := r.Client.Delete(ctx, restore); err != nil && !apierrors.IsNotFound(err) { + return false, err + } + } + return true, nil +} + +func (r *VolumePopulatorReconciler) deleteClusterPostReadyRestoresAndWait(ctx context.Context, + cluster *appsv1.Cluster) (bool, error) { + list := &dpv1alpha1.RestoreList{} + if err := r.Client.List(ctx, list, client.InNamespace(cluster.Namespace), client.MatchingLabels{ + constant.AppInstanceLabelKey: cluster.Name, + dptypes.ClusterUIDLabelKey: string(cluster.UID), + }); err != nil { + return false, err + } + pending := false + for i := range list.Items { + restore := &list.Items[i] + if internalPostReadyRestoreOwner(restore) == nil { + continue + } + pending = true + if !restore.DeletionTimestamp.IsZero() { + continue + } + if err := r.Client.Delete(ctx, restore); err != nil && !apierrors.IsNotFound(err) { + return false, err + } + } + return pending, nil +} + +func (r *VolumePopulatorReconciler) deletePopulatePVCAndWait(ctx context.Context, + pvc *corev1.PersistentVolumeClaim, cluster *appsv1.Cluster) (bool, error) { + helper := &corev1.PersistentVolumeClaim{} + key := types.NamespacedName{Namespace: pvc.Namespace, Name: getPopulatePVCName(pvc.UID)} + if err := r.Client.Get(ctx, key, helper); err != nil { + return false, client.IgnoreNotFound(err) + } + if helper.Labels[dptypes.ClusterUIDLabelKey] != string(cluster.UID) || + helper.Labels[dprestore.DataProtectionPopulatePVCLabelKey] != helper.Name { + return false, fmt.Errorf("refusing to delete helper PVC %s/%s without exact VP identity", + helper.Namespace, helper.Name) + } + if helper.DeletionTimestamp.IsZero() { + if err := r.Client.Delete(ctx, helper); err != nil && !apierrors.IsNotFound(err) { + return false, err + } + } + return true, nil +} + +func hasExactOwnerReference(refs []metav1.OwnerReference, apiVersion, kind, name string, uid types.UID) bool { + for i := range refs { + ref := refs[i] + if ref.APIVersion == apiVersion && ref.Kind == kind && ref.Name == name && ref.UID == uid { + return true + } + } + return false +} + +func (r *VolumePopulatorReconciler) validateClusterRestorePVCOwnership(ctx context.Context, + pvc *corev1.PersistentVolumeClaim, cluster *appsv1.Cluster) (*appsv1.Component, error) { + owner := metav1.GetControllerOf(pvc) + if owner == nil || owner.APIVersion != workloads.GroupVersion.String() { + return nil, fmt.Errorf("restore PVC %s/%s has no supported workload controller owner", pvc.Namespace, pvc.Name) + } + var itsOwner *metav1.OwnerReference + switch owner.Kind { + case workloads.InstanceSetKind: + itsOwner = owner + case "Instance": + instance := &workloads.Instance{} + if err := r.Client.Get(ctx, client.ObjectKey{Namespace: pvc.Namespace, Name: owner.Name}, instance); err != nil { + return nil, err + } + if instance.UID != owner.UID { + return nil, fmt.Errorf("restore PVC %s/%s Instance owner UID does not match", pvc.Namespace, pvc.Name) + } + itsOwner = metav1.GetControllerOf(instance) + if itsOwner == nil || itsOwner.APIVersion != workloads.GroupVersion.String() || + itsOwner.Kind != workloads.InstanceSetKind { + return nil, fmt.Errorf("instance %s/%s has no InstanceSet controller owner", instance.Namespace, instance.Name) + } + default: + return nil, fmt.Errorf("restore PVC %s/%s has unsupported workload owner kind %s", + pvc.Namespace, pvc.Name, owner.Kind) + } + + its := &workloads.InstanceSet{} + if err := r.Client.Get(ctx, client.ObjectKey{Namespace: pvc.Namespace, Name: itsOwner.Name}, its); err != nil { + return nil, err + } + if its.UID != itsOwner.UID { + return nil, fmt.Errorf("restore PVC %s/%s InstanceSet owner UID does not match", pvc.Namespace, pvc.Name) + } + if its.Labels[constant.AppInstanceLabelKey] != cluster.Name || + its.Labels[constant.KBAppComponentLabelKey] != pvc.Labels[constant.KBAppComponentLabelKey] { + return nil, fmt.Errorf("InstanceSet %s/%s does not match restore PVC parent identity", its.Namespace, its.Name) + } + componentOwner := metav1.GetControllerOf(its) + if componentOwner == nil || componentOwner.APIVersion != appsv1.GroupVersion.String() || + componentOwner.Kind != appsv1.ComponentKind { + return nil, fmt.Errorf("InstanceSet %s/%s has no Component controller owner", its.Namespace, its.Name) + } + comp := &appsv1.Component{} + if err := r.Client.Get(ctx, client.ObjectKey{Namespace: pvc.Namespace, Name: componentOwner.Name}, comp); err != nil { + return nil, err + } + if comp.UID != componentOwner.UID || comp.Labels[constant.AppInstanceLabelKey] != cluster.Name || + comp.Labels[constant.KBAppComponentLabelKey] != pvc.Labels[constant.KBAppComponentLabelKey] { + return nil, fmt.Errorf("InstanceSet %s/%s is not owned by the PVC Component in Cluster %s/%s", + its.Namespace, its.Name, cluster.Namespace, cluster.Name) + } + clusterOwner := metav1.GetControllerOf(comp) + if clusterOwner == nil || clusterOwner.APIVersion != appsv1.GroupVersion.String() || + clusterOwner.Kind != appsv1.ClusterKind || clusterOwner.Name != cluster.Name || clusterOwner.UID != cluster.UID { + return nil, fmt.Errorf("component %s/%s is not owned by current Cluster UID %s", + comp.Namespace, comp.Name, cluster.UID) + } + return comp, nil +} + +// registerVolumePopulation records verified App ownership and target protection +// together. The caller returns so the shared cache observes it before work starts. +func (r *VolumePopulatorReconciler) registerVolumePopulation(ctx context.Context, + pvc *corev1.PersistentVolumeClaim, cluster *appsv1.Cluster, comp *appsv1.Component) error { + if uid := pvc.Labels[dptypes.ComponentUIDLabelKey]; uid != "" && uid != string(comp.UID) { + return fmt.Errorf("restore PVC %s/%s Component UID %s does not match %s", + pvc.Namespace, pvc.Name, uid, comp.UID) + } + originalPVC := pvc.DeepCopy() + pvc.Labels[dptypes.ClusterUIDLabelKey] = string(cluster.UID) + pvc.Labels[dptypes.ComponentUIDLabelKey] = string(comp.UID) + controllerutil.AddFinalizer(pvc, dptypes.DataProtectionFinalizerName) + return r.Client.Patch(ctx, pvc, + client.MergeFromWithOptions(originalPVC, client.MergeFromWithOptimisticLock{})) +} + // dispatchUnboundPVC routes an unbound PVC to either Populate or ProvisionOnly. // When mode is RestoreData but PrepareDataBackupSets is empty, it checks // PostReadyBackupSets: if postReady actions exist, fall back to ProvisionOnly @@ -939,6 +1290,7 @@ func internalRestoreLabels(pvc *corev1.PersistentVolumeClaim) map[string]string constant.KBAppComponentLabelKey, constant.KBAppShardingNameLabelKey, constant.VolumeClaimTemplateNameLabelKey, + dptypes.ComponentUIDLabelKey, } { if value := pvc.Labels[key]; value != "" { labels[key] = value @@ -1132,13 +1484,8 @@ func (r *VolumePopulatorReconciler) Populate(reqCtx intctrlutil.RequestCtx, pvc if err != nil || wait { return err } - // Make sure the PVC finalizer is present - if !slices.Contains(pvc.Finalizers, dptypes.DataProtectionFinalizerName) { - pvcPatch := client.MergeFrom(pvc.DeepCopy()) - controllerutil.AddFinalizer(pvc, dptypes.DataProtectionFinalizerName) - if err = r.Client.Patch(reqCtx.Ctx, pvc, pvcPatch); err != nil { - return err - } + if err = r.ensureTargetFinalizer(reqCtx, pvc); err != nil { + return err } if err = r.UpdatePVCConditions(reqCtx, pvc, ReasonPopulatingProcessing, "Populator started"); err != nil { return err @@ -1194,12 +1541,8 @@ func (r *VolumePopulatorReconciler) ProvisionOnly(reqCtx intctrlutil.RequestCtx, if err != nil || wait { return err } - if !slices.Contains(pvc.Finalizers, dptypes.DataProtectionFinalizerName) { - pvcPatch := client.MergeFrom(pvc.DeepCopy()) - controllerutil.AddFinalizer(pvc, dptypes.DataProtectionFinalizerName) - if err = r.Client.Patch(reqCtx.Ctx, pvc, pvcPatch); err != nil { - return err - } + if err = r.ensureTargetFinalizer(reqCtx, pvc); err != nil { + return err } if err = r.UpdatePVCConditions(reqCtx, pvc, ReasonPopulatingProcessing, "Provisioning PVC without data restore"); err != nil { return err @@ -1221,6 +1564,24 @@ func (r *VolumePopulatorReconciler) ProvisionOnly(reqCtx intctrlutil.RequestCtx, return r.completeBoundPVCIfNeeded(reqCtx, pvc, restoreCtx) } +func (r *VolumePopulatorReconciler) ensureTargetFinalizer( + reqCtx intctrlutil.RequestCtx, pvc *corev1.PersistentVolumeClaim) error { + if slices.Contains(pvc.Finalizers, dptypes.DataProtectionFinalizerName) { + return nil + } + pvcPatch := client.MergeFromWithOptions(pvc.DeepCopy(), client.MergeFromWithOptimisticLock{}) + controllerutil.AddFinalizer(pvc, dptypes.DataProtectionFinalizerName) + if err := r.Client.Patch(reqCtx.Ctx, pvc, pvcPatch); err != nil { + return err + } + if clusterRestorePVCUID(pvc) != "" { + // Reobserve the marker through the shared PVC cache before another + // informer can expose newly-created restore resources. + return intctrlutil.NewRequeueError(reconcileInterval, "waiting for target PVC restore protection") + } + return nil +} + func (r *VolumePopulatorReconciler) completeBoundPVCIfNeeded(reqCtx intctrlutil.RequestCtx, pvc *corev1.PersistentVolumeClaim, restoreCtx *pvcRestoreContext) error { @@ -1245,6 +1606,9 @@ func (r *VolumePopulatorReconciler) completeBoundPVCIfNeeded(reqCtx intctrlutil. if !postReadyCompleted { return intctrlutil.NewRequeueError(reconcileInterval, "waiting for postReady restore") } + if err := r.releaseTargetPVC(reqCtx, pvc); err != nil { + return err + } return r.UpdatePVCConditions(reqCtx, pvc, reason, message) } @@ -1477,6 +1841,11 @@ func (r *VolumePopulatorReconciler) ensurePostReadyRestoreCompleted(reqCtx intct if !apierrors.IsNotFound(err) { return false, err } + if clusterRestorePVCUID(pvc) != "" { + if err = r.ensureTargetFinalizer(reqCtx, pvc); err != nil { + return false, err + } + } if err = r.Client.Create(reqCtx.Ctx, postReadyRestore); err != nil && !apierrors.IsAlreadyExists(err) { return false, err } @@ -1488,6 +1857,13 @@ func (r *VolumePopulatorReconciler) ensurePostReadyRestoreCompleted(reqCtx intct if err = validatePostReadyRestore(existing, postReadyRestore, comp); err != nil { return false, err } + // The Restore is now visible to the Cluster lifecycle resource scan. The + // helper has already been released, so temporary target protection can go. + if pvcPopulateReleased(pvc) { + if err = r.releaseTargetPVC(reqCtx, pvc); err != nil { + return false, err + } + } switch existing.Status.Phase { case dpv1alpha1.RestorePhaseCompleted: return true, nil @@ -1920,7 +2296,7 @@ func (r *VolumePopulatorReconciler) deletePopulatePVC(reqCtx intctrlutil.Request func (r *VolumePopulatorReconciler) releaseTargetPVC(reqCtx intctrlutil.RequestCtx, pvc *corev1.PersistentVolumeClaim) error { if slices.Contains(pvc.Finalizers, dptypes.DataProtectionFinalizerName) { - pvcPatch := client.MergeFrom(pvc.DeepCopy()) + pvcPatch := client.MergeFromWithOptions(pvc.DeepCopy(), client.MergeFromWithOptimisticLock{}) controllerutil.RemoveFinalizer(pvc, dptypes.DataProtectionFinalizerName) if err := r.Client.Patch(reqCtx.Ctx, pvc, pvcPatch); err != nil { return client.IgnoreNotFound(err) diff --git a/controllers/dataprotection/volumepopulator_controller_test.go b/controllers/dataprotection/volumepopulator_controller_test.go index 8c5211fbb97..ba57560c68e 100644 --- a/controllers/dataprotection/volumepopulator_controller_test.go +++ b/controllers/dataprotection/volumepopulator_controller_test.go @@ -4221,6 +4221,394 @@ func dependencyRestorePVC(name, componentName string, uid types.UID) *corev1.Per } } +func TestClusterDeletionTerminatesVolumePopulationInOrder(t *testing.T) { + for _, retained := range []bool{false, true} { + t.Run(fmt.Sprintf("retained=%t", retained), func(t *testing.T) { + ctx := context.Background() + scheme, cluster, component, its, target := parentRestoreObjects(t) + now := metav1.Now() + cluster.DeletionTimestamp = &now + cluster.Finalizers = append(cluster.Finalizers, "example.io/app-owner") + target.Finalizers = []string{dptypes.DataProtectionFinalizerName, "example.io/app-owner"} + target.Labels[dptypes.ComponentUIDLabelKey] = string(component.UID) + objects := []client.Object{cluster, target} + if retained { + target.OwnerReferences = nil + } else { + objects = append(objects, its) + } + helper := restoreHelperForTarget(target, cluster) + execution := executionRestoreForTarget(target, cluster) + execution.Finalizers = []string{"example.io/restore-owner"} + postReady := postReadyRestoreForComponent(target, cluster, component) + postReady.Finalizers = []string{"example.io/restore-owner"} + objects = append(objects, helper, execution, postReady) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} + reqCtx := intctrlutil.RequestCtx{Ctx: ctx} + + require.ErrorContains(t, vp.syncPVC(reqCtx, target), "waiting for Restore owners") + for _, expected := range []*dpv1alpha1.Restore{execution, postReady} { + current := &dpv1alpha1.Restore{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(expected), current)) + require.False(t, current.DeletionTimestamp.IsZero()) + require.Equal(t, expected.Finalizers, current.Finalizers) + current.Finalizers = nil + require.NoError(t, cli.Update(ctx, current)) + } + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(helper), &corev1.PersistentVolumeClaim{})) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(target), target)) + require.ErrorContains(t, vp.syncPVC(reqCtx, target), "waiting for helper PVC to disappear") + require.True(t, apierrors.IsNotFound(cli.Get(ctx, client.ObjectKeyFromObject(helper), helper))) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(target), target)) + require.NoError(t, vp.syncPVC(reqCtx, target)) + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(target), target)) + require.Equal(t, []string{"example.io/app-owner"}, target.Finalizers) + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(cluster), cluster)) + require.Contains(t, cluster.Finalizers, dptypes.RestoreProtectionFinalizerName) + }) + } +} + +func TestClusterLifecycleRegistersBeforeWaitingForProtection(t *testing.T) { + for _, existingFinalizer := range []bool{false, true} { + t.Run(fmt.Sprintf("existing-finalizer=%t", existingFinalizer), func(t *testing.T) { + ctx := context.Background() + scheme, cluster, component, its, target := parentRestoreObjects(t) + cluster.Finalizers = nil + delete(target.Labels, dptypes.ClusterUIDLabelKey) + if existingFinalizer { + target.Finalizers = []string{dptypes.DataProtectionFinalizerName} + } + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster, component, its, target).Build() + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} + + require.ErrorContains(t, vp.syncPVC(intctrlutil.RequestCtx{Ctx: ctx}, target), + "waiting for target PVC restore protection") + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(target), target)) + require.Equal(t, string(cluster.UID), target.Labels[dptypes.ClusterUIDLabelKey]) + require.Equal(t, string(component.UID), target.Labels[dptypes.ComponentUIDLabelKey]) + require.Contains(t, target.Finalizers, dptypes.DataProtectionFinalizerName) + require.Empty(t, target.Status.Conditions) + + require.ErrorContains(t, vp.syncPVC(intctrlutil.RequestCtx{Ctx: ctx}, target), + "waiting for Cluster restore-protection finalizer") + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(cluster), cluster)) + require.NotContains(t, cluster.Finalizers, dptypes.RestoreProtectionFinalizerName, + "VolumePopulator must not add the Cluster finalizer") + pvcs := &corev1.PersistentVolumeClaimList{} + require.NoError(t, cli.List(ctx, pvcs)) + require.Len(t, pvcs.Items, 1) + restores := &dpv1alpha1.RestoreList{} + require.NoError(t, cli.List(ctx, restores)) + require.Empty(t, restores.Items) + }) + } +} + +func TestClusterLifecycleRegistrationReturnsBeforeRestoreValidation(t *testing.T) { + ctx := context.Background() + scheme, cluster, component, its, target := parentRestoreObjects(t) + delete(target.Labels, dptypes.ClusterUIDLabelKey) + target.Spec.DataSourceRef.Kind = dptypes.RestoreKind + target.Spec.DataSourceRef.Name = "source" + target.Annotations[constant.RestoreSourceKindAnnotationKey] = dptypes.RestoreKind + target.Annotations[constant.RestoreSourceNameAnnotationKey] = "source" + base := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster, component, its, target).Build() + validated := false + cli := interceptor.NewClient(base, interceptor.Funcs{Get: func(ctx context.Context, inner client.WithWatch, + key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*dpv1alpha1.Restore); ok { + validated = true + } + return inner.Get(ctx, key, obj, opts...) + }}) + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} + + require.ErrorContains(t, vp.syncPVC(intctrlutil.RequestCtx{Ctx: ctx}, target), + "waiting for target PVC restore protection") + require.False(t, validated) + require.NoError(t, base.Get(ctx, client.ObjectKeyFromObject(target), target)) + require.Contains(t, target.Finalizers, dptypes.DataProtectionFinalizerName) +} + +func TestClusterLifecyclePostReadyProtectionHandoff(t *testing.T) { + ctx := context.Background() + scheme, cluster, component, _, target := parentRestoreObjects(t) + target.Labels[dptypes.ComponentUIDLabelKey] = string(component.UID) + target.Finalizers = nil + target.Spec.VolumeName = "target-pv" + target.Status.Conditions = []corev1.PersistentVolumeClaimCondition{{ + Type: PersistentVolumeClaimPopulating, Status: corev1.ConditionTrue, Reason: ReasonPopulatingProvisioned, + }} + component.Status.Phase = kbappsv1.RunningComponentPhase + backup, actionSet := restoreBackupObjects() + actionSet.Spec.Restore.PostReady = []dpv1alpha1.ActionSpec{{Job: &dpv1alpha1.JobActionSpec{}}} + worker := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "worker", Namespace: target.Namespace}} + for key, value := range map[string]string{ + dptypes.CfgKeyWorkerServiceAccountName: "worker", + dptypes.CfgKeyWorkerClusterRoleName: "worker-role", + } { + previous := viper.Get(key) + viper.Set(key, value) + t.Cleanup(func() { viper.Set(key, previous) }) + } + cli := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(target). + WithObjects(cluster, component, target, backup, actionSet, worker).Build() + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme, Recorder: record.NewFakeRecorder(10)} + mgr := dprestore.NewRestoreManager(&dpv1alpha1.Restore{}, nil, scheme, cli) + mgr.PostReadyBackupSets = []dprestore.BackupActionSet{{Backup: backup}} + restoreCtx := &pvcRestoreContext{restoreMgr: mgr, mode: pvcRestoreModeProvisionOnly} + reqCtx := intctrlutil.RequestCtx{Ctx: ctx} + + require.ErrorContains(t, vp.completeBoundPVCIfNeeded(reqCtx, target, restoreCtx), + "waiting for target PVC restore protection") + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(target), target)) + require.Contains(t, target.Finalizers, dptypes.DataProtectionFinalizerName) + restores := &dpv1alpha1.RestoreList{} + require.NoError(t, cli.List(ctx, restores)) + require.Empty(t, restores.Items) + + require.ErrorContains(t, vp.completeBoundPVCIfNeeded(reqCtx, target, restoreCtx), "waiting for postReady restore") + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(target), target)) + require.Contains(t, target.Finalizers, dptypes.DataProtectionFinalizerName) + require.NoError(t, cli.List(ctx, restores)) + require.Len(t, restores.Items, 1) + + require.ErrorContains(t, vp.completeBoundPVCIfNeeded(reqCtx, target, restoreCtx), "waiting for postReady restore") + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(target), target)) + require.NotContains(t, target.Finalizers, dptypes.DataProtectionFinalizerName) + coordinator := &ClusterRestoreReconciler{Client: cli} + hasResources, err := coordinator.hasRestoreResources(ctx, cluster) + require.NoError(t, err) + require.True(t, hasResources) +} + +func TestClusterLifecycleSafetyBoundaries(t *testing.T) { + t.Run("target deletion is not termination", func(t *testing.T) { + scheme, cluster, component, its, target := parentRestoreObjects(t) + now := metav1.Now() + target.DeletionTimestamp = &now + target.Finalizers = []string{dptypes.DataProtectionFinalizerName, "example.io/app-owner"} + target.Labels[dptypes.ComponentUIDLabelKey] = string(component.UID) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster, component, its, target).Build() + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} + + terminated, err := vp.handleRestoreClusterLifecycle(intctrlutil.RequestCtx{Ctx: context.Background()}, target) + require.NoError(t, err) + require.False(t, terminated) + }) + + t.Run("deleting target without VP finalizer is ignored", func(t *testing.T) { + scheme, _, _, _, target := parentRestoreObjects(t) + now := metav1.Now() + target.DeletionTimestamp = &now + target.Finalizers = []string{"kubernetes.io/pvc-protection"} + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(target).WithInterceptorFuncs(interceptor.Funcs{ + Get: func(context.Context, client.WithWatch, client.ObjectKey, client.Object, ...client.GetOption) error { + t.Fatal("deleting target without VP protection must not start restore") + return nil + }, + Patch: func(context.Context, client.WithWatch, client.Object, client.Patch, ...client.PatchOption) error { + t.Fatal("deleting target must not acquire VP protection") + return nil + }, + }).Build() + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} + require.NoError(t, vp.syncPVC(intctrlutil.RequestCtx{Ctx: context.Background()}, target)) + }) + + t.Run("missing Cluster does not authorize active cleanup", func(t *testing.T) { + scheme, cluster, component, _, target := parentRestoreObjects(t) + target.Labels[dptypes.ComponentUIDLabelKey] = string(component.UID) + target.Finalizers = []string{dptypes.DataProtectionFinalizerName} + helper := restoreHelperForTarget(target, cluster) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(target, helper). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(context.Context, client.WithWatch, client.Object, ...client.DeleteOption) error { + t.Fatal("missing Cluster must not authorize deletion") + return nil + }, + Patch: func(context.Context, client.WithWatch, client.Object, client.Patch, ...client.PatchOption) error { + t.Fatal("active protection must remain") + return nil + }, + }).Build() + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} + require.True(t, intctrlutil.IsRequeueError(vp.syncPVC( + intctrlutil.RequestCtx{Ctx: context.Background()}, target))) + }) + + t.Run("completed Cluster restore does not register", func(t *testing.T) { + scheme, cluster, component, its, target := parentRestoreObjects(t) + cluster.Status.Conditions = []metav1.Condition{{ + Type: kbappsv1.ConditionTypeRestore, Status: metav1.ConditionTrue, + }} + delete(target.Labels, dptypes.ClusterUIDLabelKey) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster, component, its, target).Build() + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} + err := vp.syncPVC(intctrlutil.RequestCtx{Ctx: context.Background()}, target) + require.ErrorContains(t, err, "Cluster restore is no longer active") + require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(target), target)) + require.NotContains(t, target.Finalizers, dptypes.DataProtectionFinalizerName) + }) +} + +func TestClusterLifecycleRefusesForeignExecutionRestore(t *testing.T) { + scheme, cluster, component, _, target := parentRestoreObjects(t) + now := metav1.Now() + cluster.DeletionTimestamp = &now + target.Finalizers = []string{dptypes.DataProtectionFinalizerName} + target.Labels[dptypes.ComponentUIDLabelKey] = string(component.UID) + helper := restoreHelperForTarget(target, cluster) + foreign := executionRestoreForTarget(target, cluster) + foreign.OwnerReferences[0].UID = "foreign-pvc" + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster, target, helper, foreign).Build() + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} + + terminated, err := vp.handleRestoreClusterLifecycle(intctrlutil.RequestCtx{Ctx: context.Background()}, target) + require.True(t, terminated) + require.ErrorContains(t, err, "refusing to delete execution Restore") + require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(foreign), &dpv1alpha1.Restore{})) + require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(helper), &corev1.PersistentVolumeClaim{})) +} + +func TestValidateClusterRestorePVCOwnershipThroughInstance(t *testing.T) { + scheme, cluster, component, its, target := parentRestoreObjects(t) + controller := true + instance := &workloadsv1.Instance{ObjectMeta: metav1.ObjectMeta{ + Namespace: target.Namespace, Name: "cluster-mysql-0", UID: "instance-uid", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: workloadsv1.GroupVersion.String(), Kind: workloadsv1.InstanceSetKind, + Name: its.Name, UID: its.UID, Controller: &controller, + }}, + }} + target.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: workloadsv1.GroupVersion.String(), Kind: "Instance", + Name: instance.Name, UID: instance.UID, Controller: &controller, + }} + cli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(cluster, component, its, instance, target).Build() + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} + + actual, err := vp.validateClusterRestorePVCOwnership(context.Background(), target, cluster) + require.NoError(t, err) + require.Equal(t, component.UID, actual.UID) +} + +func restoreBackupObjects() (*dpv1alpha1.Backup, *dpv1alpha1.ActionSet) { + backup := newBackupForRestoreDecision([]string{"data"}, nil) + backup.Status.Phase = dpv1alpha1.BackupPhaseCompleted + backup.Status.BackupMethod.ActionSetName = "full" + backup.Status.Target.PodSelector = &dpv1alpha1.PodSelector{Strategy: dpv1alpha1.PodSelectionStrategyAny} + actionSet := &dpv1alpha1.ActionSet{ObjectMeta: metav1.ObjectMeta{Name: "full"}, Spec: dpv1alpha1.ActionSetSpec{ + BackupType: dpv1alpha1.BackupTypeFull, + Restore: &dpv1alpha1.RestoreActionSpec{PrepareData: &dpv1alpha1.JobActionSpec{}}, + }} + return backup, actionSet +} + +func parentRestoreObjects(t *testing.T) (*runtime.Scheme, *kbappsv1.Cluster, *kbappsv1.Component, + *workloadsv1.InstanceSet, *corev1.PersistentVolumeClaim) { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, kbappsv1.AddToScheme(scheme)) + require.NoError(t, workloadsv1.AddToScheme(scheme)) + require.NoError(t, dpv1alpha1.AddToScheme(scheme)) + controller := true + cluster := &kbappsv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", Name: "cluster", UID: "cluster-uid", + Finalizers: []string{dptypes.RestoreProtectionFinalizerName}, + }, + Spec: kbappsv1.ClusterSpec{Restore: &kbappsv1.ClusterRestore{}}, + } + component := &kbappsv1.Component{ObjectMeta: metav1.ObjectMeta{ + Namespace: cluster.Namespace, Name: "cluster-mysql", UID: "component-uid", + Labels: map[string]string{ + constant.AppInstanceLabelKey: cluster.Name, constant.KBAppComponentLabelKey: "mysql", + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: kbappsv1.GroupVersion.String(), Kind: kbappsv1.ClusterKind, + Name: cluster.Name, UID: cluster.UID, Controller: &controller, + }}, + }} + its := &workloadsv1.InstanceSet{ObjectMeta: metav1.ObjectMeta{ + Namespace: cluster.Namespace, Name: "cluster-mysql", UID: "its-uid", + Labels: map[string]string{ + constant.AppInstanceLabelKey: cluster.Name, constant.KBAppComponentLabelKey: "mysql", + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: kbappsv1.GroupVersion.String(), Kind: kbappsv1.ComponentKind, + Name: component.Name, UID: component.UID, Controller: &controller, + }}, + }} + target := dependencyRestorePVC("data-mysql-0", "mysql", "target-uid") + target.Annotations[constant.KBAppClusterUIDKey] = string(cluster.UID) + target.Labels[dptypes.ClusterUIDLabelKey] = string(cluster.UID) + target.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: workloadsv1.GroupVersion.String(), Kind: workloadsv1.InstanceSetKind, + Name: its.Name, UID: its.UID, Controller: &controller, + }} + return scheme, cluster, component, its, target +} + +func restoreHelperForTarget(target *corev1.PersistentVolumeClaim, + cluster *kbappsv1.Cluster) *corev1.PersistentVolumeClaim { + name := getPopulatePVCName(target.UID) + return &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{ + Namespace: target.Namespace, Name: name, + Labels: map[string]string{ + constant.AppInstanceLabelKey: cluster.Name, + constant.KBAppComponentLabelKey: target.Labels[constant.KBAppComponentLabelKey], + dptypes.ClusterUIDLabelKey: string(cluster.UID), + dprestore.DataProtectionRestoreLabelKey: name, + dprestore.DataProtectionRestoreNamespaceLabelKey: target.Namespace, + dprestore.DataProtectionPopulatePVCLabelKey: name, + }, + }} +} + +func executionRestoreForTarget(target *corev1.PersistentVolumeClaim, + cluster *kbappsv1.Cluster) *dpv1alpha1.Restore { + name := getPopulatePVCName(target.UID) + return &dpv1alpha1.Restore{ObjectMeta: metav1.ObjectMeta{ + Namespace: target.Namespace, Name: name, + Labels: map[string]string{ + constant.AppInstanceLabelKey: cluster.Name, + constant.KBAppComponentLabelKey: target.Labels[constant.KBAppComponentLabelKey], + dptypes.ClusterUIDLabelKey: string(cluster.UID), + dprestore.DataProtectionRestoreLabelKey: name, + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: corev1.SchemeGroupVersion.String(), Kind: "PersistentVolumeClaim", + Name: target.Name, UID: target.UID, + }}, + }} +} + +func postReadyRestoreForComponent(target *corev1.PersistentVolumeClaim, cluster *kbappsv1.Cluster, + component *kbappsv1.Component) *dpv1alpha1.Restore { + name := postReadyRestoreName(component.UID) + return &dpv1alpha1.Restore{ObjectMeta: metav1.ObjectMeta{ + Namespace: target.Namespace, Name: name, + Labels: map[string]string{ + constant.AppInstanceLabelKey: cluster.Name, + constant.KBAppComponentLabelKey: target.Labels[constant.KBAppComponentLabelKey], + dptypes.ClusterUIDLabelKey: string(cluster.UID), + dptypes.ComponentUIDLabelKey: string(component.UID), + dprestore.DataProtectionRestoreLabelKey: name, + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: kbappsv1.GroupVersion.String(), Kind: kbappsv1.ComponentKind, + Name: component.Name, UID: component.UID, + }}, + }} +} + func TestEnsurePostReadyRestore_ShardingMissingTargetSkip_DoesNotRedirect(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, corev1.AddToScheme(scheme)) diff --git a/pkg/dataprotection/types/constant.go b/pkg/dataprotection/types/constant.go index b653dc6e86a..810adca70ff 100644 --- a/pkg/dataprotection/types/constant.go +++ b/pkg/dataprotection/types/constant.go @@ -46,6 +46,9 @@ const ( const ( // DataProtectionFinalizerName is the name of our custom finalizer DataProtectionFinalizerName = "dataprotection.kubeblocks.io/finalizer" + // RestoreProtectionFinalizerName prevents Cluster deletion from completing + // before its restore resources have been cleaned up. + RestoreProtectionFinalizerName = "dataprotection.kubeblocks.io/restore-protection-finalizer" ) // annotation keys From b839747d01f3637053c261928323a2d97a68fe2a Mon Sep 17 00:00:00 2001 From: Leon Date: Fri, 4 Sep 2026 14:49:18 +0800 Subject: [PATCH 3/3] fix(dataprotection): clarify cluster restore lifecycle --- .../cluster_restore_controller.go | 70 ++++++++++--------- .../volumepopulator_controller.go | 17 +++-- .../volumepopulator_controller_test.go | 47 +++++++++---- 3 files changed, 83 insertions(+), 51 deletions(-) diff --git a/controllers/dataprotection/cluster_restore_controller.go b/controllers/dataprotection/cluster_restore_controller.go index 694ba5f5d7d..304a7b4bc82 100644 --- a/controllers/dataprotection/cluster_restore_controller.go +++ b/controllers/dataprotection/cluster_restore_controller.go @@ -63,34 +63,30 @@ func (r *ClusterRestoreReconciler) Reconcile(ctx context.Context, req ctrl.Reque return intctrlutil.CheckedRequeueWithError(err, reqCtx.Log, "") } - if cluster.DeletionTimestamp.IsZero() { - if clusterRestoreConditionActive(cluster) { - return r.ensureFinalizer(reqCtx, cluster) - } - } else if !controllerutil.ContainsFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) { - return intctrlutil.Reconciled() - } - - hasResources, err := r.hasRestoreResources(ctx, cluster) + restoring, err := r.isClusterRestoring(ctx, cluster) if err != nil { - return intctrlutil.CheckedRequeueWithError(err, reqCtx.Log, "failed to inspect Cluster restore resources") + return intctrlutil.CheckedRequeueWithError(err, reqCtx.Log, "failed to determine Cluster restore state") + } + if !restoring { + return r.releaseClusterRestoreProtection(reqCtx, cluster) } - if cluster.DeletionTimestamp.IsZero() { - if hasResources { - return r.ensureFinalizer(reqCtx, cluster) + if !isClusterRestoreProtected(cluster) { + // If deletion began without our finalizer, this controller cannot acquire + // protection and must not delay Cluster deletion. + if !cluster.DeletionTimestamp.IsZero() { + return intctrlutil.Reconciled() } - return r.removeFinalizer(reqCtx, cluster) + return r.protectClusterRestore(reqCtx, cluster) } - if hasResources { + if !cluster.DeletionTimestamp.IsZero() { return intctrlutil.RequeueAfter(reconcileInterval, reqCtx.Log, - "waiting for restore resource owners to finish Cluster termination") + "waiting for restore owners to finish Cluster termination") } - return r.removeFinalizer(reqCtx, cluster) + return intctrlutil.Reconciled() } func (r *ClusterRestoreReconciler) SetupWithManager(mgr ctrl.Manager) error { return intctrlutil.NewControllerManagedBy(mgr). - Named("cluster_restore"). For(&appsv1.Cluster{}). Watches(&dpv1alpha1.Restore{}, handler.EnqueueRequestsFromMapFunc(r.mapObjectToCluster)). Watches(&corev1.PersistentVolumeClaim{}, handler.EnqueueRequestsFromMapFunc(r.mapObjectToCluster)). @@ -105,19 +101,12 @@ func (r *ClusterRestoreReconciler) mapObjectToCluster(_ context.Context, obj cli return []reconcile.Request{{NamespacedName: client.ObjectKey{Namespace: obj.GetNamespace(), Name: clusterName}}} } -func clusterRestoreConditionActive(cluster *appsv1.Cluster) bool { - if cluster.Spec.Restore == nil { - return false +func (r *ClusterRestoreReconciler) isClusterRestoring(ctx context.Context, + cluster *appsv1.Cluster) (bool, error) { + if cluster.DeletionTimestamp.IsZero() && clusterAllowsRestoreProgress(cluster) { + return true, nil } - condition := meta.FindStatusCondition(cluster.Status.Conditions, appsv1.ConditionTypeRestore) - // Restore=False is terminal for status aggregation, but the failed Cluster - // still carries initial-restore intent. Keep protection until Cluster deletion - // so remaining PVC restores never lose the lifecycle coordinator. - return condition == nil || condition.Status != metav1.ConditionTrue -} -func (r *ClusterRestoreReconciler) hasRestoreResources(ctx context.Context, - cluster *appsv1.Cluster) (bool, error) { // Inspect PVCs before Restores. VP releases temporary target protection only // after observing postReady Restore, so this order closes the handoff window. pvcs := &corev1.PersistentVolumeClaimList{} @@ -158,9 +147,24 @@ func (r *ClusterRestoreReconciler) hasRestoreResources(ctx context.Context, return false, nil } -func (r *ClusterRestoreReconciler) ensureFinalizer(reqCtx intctrlutil.RequestCtx, +func clusterAllowsRestoreProgress(cluster *appsv1.Cluster) bool { + if cluster.Spec.Restore == nil { + return false + } + condition := meta.FindStatusCondition(cluster.Status.Conditions, appsv1.ConditionTypeRestore) + // Restore=False is terminal for status aggregation, but the Cluster still + // has initial-restore intent. Keep the lifecycle active until deletion so + // PVC restores cannot lose protection while converging on the failure. + return condition == nil || condition.Status != metav1.ConditionTrue +} + +func isClusterRestoreProtected(cluster *appsv1.Cluster) bool { + return controllerutil.ContainsFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) +} + +func (r *ClusterRestoreReconciler) protectClusterRestore(reqCtx intctrlutil.RequestCtx, cluster *appsv1.Cluster) (ctrl.Result, error) { - if controllerutil.ContainsFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) { + if isClusterRestoreProtected(cluster) { return intctrlutil.Reconciled() } patch := client.MergeFromWithOptions(cluster.DeepCopy(), client.MergeFromWithOptimisticLock{}) @@ -171,9 +175,9 @@ func (r *ClusterRestoreReconciler) ensureFinalizer(reqCtx intctrlutil.RequestCtx return intctrlutil.Reconciled() } -func (r *ClusterRestoreReconciler) removeFinalizer(reqCtx intctrlutil.RequestCtx, +func (r *ClusterRestoreReconciler) releaseClusterRestoreProtection(reqCtx intctrlutil.RequestCtx, cluster *appsv1.Cluster) (ctrl.Result, error) { - if !controllerutil.ContainsFinalizer(cluster, dptypes.RestoreProtectionFinalizerName) { + if !isClusterRestoreProtected(cluster) { return intctrlutil.Reconciled() } patch := client.MergeFromWithOptions(cluster.DeepCopy(), client.MergeFromWithOptimisticLock{}) diff --git a/controllers/dataprotection/volumepopulator_controller.go b/controllers/dataprotection/volumepopulator_controller.go index 4739a26865b..27731830954 100644 --- a/controllers/dataprotection/volumepopulator_controller.go +++ b/controllers/dataprotection/volumepopulator_controller.go @@ -389,10 +389,6 @@ func (r *VolumePopulatorReconciler) MatchToPopulate(pvc *corev1.PersistentVolume } func (r *VolumePopulatorReconciler) syncPVC(reqCtx intctrlutil.RequestCtx, pvc *corev1.PersistentVolumeClaim) error { - // Kubernetes does not allow registering new finalizers after deletion starts. - if !pvc.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(pvc, dptypes.DataProtectionFinalizerName) { - return nil - } matched, err := r.MatchToPopulate(pvc) if err != nil { return err @@ -404,6 +400,13 @@ func (r *VolumePopulatorReconciler) syncPVC(reqCtx intctrlutil.RequestCtx, pvc * if err != nil || terminated { return err } + // Parent deletion is checked first because it authorizes cleanup even after + // target protection has been handed off. Target deletion alone remains a + // no-op, and Kubernetes does not allow acquiring a new finalizer here. + if !pvc.DeletionTimestamp.IsZero() && + !controllerutil.ContainsFinalizer(pvc, dptypes.DataProtectionFinalizerName) { + return nil + } // A non-deleting bound PVC with a terminal Restore condition does not need // its source Backup/Restore. Populating can finish while postReady is pending. if pvc.Spec.VolumeName != "" && pvc.DeletionTimestamp.IsZero() && pvcRestoreTerminal(pvc) { @@ -475,6 +478,10 @@ func (r *VolumePopulatorReconciler) handleRestoreClusterLifecycle(reqCtx intctrl if !cluster.DeletionTimestamp.IsZero() { return r.terminateClusterVolumePopulation(reqCtx, pvc, cluster) } + if !pvc.DeletionTimestamp.IsZero() && + !controllerutil.ContainsFinalizer(pvc, dptypes.DataProtectionFinalizerName) { + return true, nil + } committed := volumePopulationIdentityCommitted(pvc, cluster) if committed { @@ -483,7 +490,7 @@ func (r *VolumePopulatorReconciler) handleRestoreClusterLifecycle(reqCtx intctrl } } // Aggregate restore status gates normal progression, not owner cleanup. - if !clusterRestoreConditionActive(cluster) && !pvcRestoreTerminal(pvc) { + if !clusterAllowsRestoreProgress(cluster) && !pvcRestoreTerminal(pvc) { return false, intctrlutil.NewRequeueError(reconcileInterval, "Cluster restore is no longer active") } if !committed { diff --git a/controllers/dataprotection/volumepopulator_controller_test.go b/controllers/dataprotection/volumepopulator_controller_test.go index ba57560c68e..9ddc90e2f28 100644 --- a/controllers/dataprotection/volumepopulator_controller_test.go +++ b/controllers/dataprotection/volumepopulator_controller_test.go @@ -4344,6 +4344,9 @@ func TestClusterLifecyclePostReadyProtectionHandoff(t *testing.T) { Type: PersistentVolumeClaimPopulating, Status: corev1.ConditionTrue, Reason: ReasonPopulatingProvisioned, }} component.Status.Phase = kbappsv1.RunningComponentPhase + cluster.Status.Conditions = []metav1.Condition{{ + Type: kbappsv1.ConditionTypeRestore, Status: metav1.ConditionTrue, + }} backup, actionSet := restoreBackupObjects() actionSet.Spec.Restore.PostReady = []dpv1alpha1.ActionSpec{{Job: &dpv1alpha1.JobActionSpec{}}} worker := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "worker", Namespace: target.Namespace}} @@ -4381,9 +4384,10 @@ func TestClusterLifecyclePostReadyProtectionHandoff(t *testing.T) { require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(target), target)) require.NotContains(t, target.Finalizers, dptypes.DataProtectionFinalizerName) coordinator := &ClusterRestoreReconciler{Client: cli} - hasResources, err := coordinator.hasRestoreResources(ctx, cluster) + _, err := coordinator.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(cluster)}) require.NoError(t, err) - require.True(t, hasResources) + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(cluster), cluster)) + require.True(t, isClusterRestoreProtected(cluster)) } func TestClusterLifecycleSafetyBoundaries(t *testing.T) { @@ -4402,22 +4406,39 @@ func TestClusterLifecycleSafetyBoundaries(t *testing.T) { }) t.Run("deleting target without VP finalizer is ignored", func(t *testing.T) { - scheme, _, _, _, target := parentRestoreObjects(t) + scheme, cluster, _, _, target := parentRestoreObjects(t) now := metav1.Now() target.DeletionTimestamp = &now target.Finalizers = []string{"kubernetes.io/pvc-protection"} - cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(target).WithInterceptorFuncs(interceptor.Funcs{ - Get: func(context.Context, client.WithWatch, client.ObjectKey, client.Object, ...client.GetOption) error { - t.Fatal("deleting target without VP protection must not start restore") - return nil - }, - Patch: func(context.Context, client.WithWatch, client.Object, client.Patch, ...client.PatchOption) error { - t.Fatal("deleting target must not acquire VP protection") - return nil - }, - }).Build() + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster, target).Build() vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} require.NoError(t, vp.syncPVC(intctrlutil.RequestCtx{Ctx: context.Background()}, target)) + require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(target), target)) + require.Equal(t, []string{"kubernetes.io/pvc-protection"}, target.Finalizers) + restores := &dpv1alpha1.RestoreList{} + require.NoError(t, cli.List(context.Background(), restores)) + require.Empty(t, restores.Items) + }) + + t.Run("Cluster deletion cleans postReady after target protection handoff", func(t *testing.T) { + scheme, cluster, component, _, target := parentRestoreObjects(t) + now := metav1.Now() + cluster.DeletionTimestamp = &now + cluster.Finalizers = append(cluster.Finalizers, "example.io/app-owner") + target.DeletionTimestamp = &now + target.Finalizers = []string{"example.io/app-owner"} + postReady := postReadyRestoreForComponent(target, cluster, component) + postReady.Finalizers = []string{"example.io/restore-owner"} + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster, target, postReady).Build() + vp := &VolumePopulatorReconciler{Client: cli, Scheme: scheme} + + err := vp.syncPVC(intctrlutil.RequestCtx{Ctx: context.Background()}, target) + + require.ErrorContains(t, err, "waiting for Restore owners") + require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(postReady), postReady)) + require.False(t, postReady.DeletionTimestamp.IsZero()) + require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(target), target)) + require.NotContains(t, target.Finalizers, dptypes.DataProtectionFinalizerName) }) t.Run("missing Cluster does not authorize active cleanup", func(t *testing.T) {