From 8d29e003177e908bf09bc21d4311921407e8980d Mon Sep 17 00:00:00 2001 From: Wei Cao Date: Wed, 24 Jun 2026 00:57:37 +0800 Subject: [PATCH 1/6] fix(dp): don't skip postReady restore when no volume-level data exists When a backup ActionSet has only postReady (no prepareData/targetVolumes), decidePVCRestore incorrectly propagated skipPostReady=true for PVCs whose component did not match the backup target. This blocked the sole restore path for multi-component architectures like TiDB where the backup targets the SQL frontend but data lives in PD/TiKV PVCs. Override skipPostReady to false when restoreData is false: if there is no volume-level restore, PVC-selector mismatch should not block postReady. Fixes #10418 --- .../volumepopulator_controller.go | 5 ++++ .../volumepopulator_controller_test.go | 23 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/controllers/dataprotection/volumepopulator_controller.go b/controllers/dataprotection/volumepopulator_controller.go index 18877477e3b..3ec7696a1b2 100644 --- a/controllers/dataprotection/volumepopulator_controller.go +++ b/controllers/dataprotection/volumepopulator_controller.go @@ -397,6 +397,11 @@ func (r *VolumePopulatorReconciler) decidePVCRestore(reqCtx intctrlutil.RequestC if restoreData && !skipPostReady { mode = pvcRestoreModeRestoreData } + // When no volume-level restore exists, postReady may be the only restore + // path (e.g. multi-component logical backup). Don't block it. + if !restoreData { + skipPostReady = false + } return &pvcRestoreDecision{ mode: mode, sourceTarget: sourceTarget, diff --git a/controllers/dataprotection/volumepopulator_controller_test.go b/controllers/dataprotection/volumepopulator_controller_test.go index 9853931410b..d2316bab018 100644 --- a/controllers/dataprotection/volumepopulator_controller_test.go +++ b/controllers/dataprotection/volumepopulator_controller_test.go @@ -859,6 +859,29 @@ func TestDecidePVCRestoreTreatsNilTargetVolumesAsProvisionOnly(t *testing.T) { require.False(t, decision.skipPostReady) } +func TestDecidePVCRestoreDoesNotSkipPostReadyWhenNoTargetVolumes(t *testing.T) { + reconciler := &VolumePopulatorReconciler{} + backup := newBackupForRestoreDecision(nil, nil) + backup.Status.BackupMethod.TargetVolumes = nil + backup.Status.Target.PodSelector = &dpv1alpha1.PodSelector{ + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + constant.AppInstanceLabelKey: "source-cluster", + constant.KBAppComponentLabelKey: "tidb", + }, + }, + Strategy: dpv1alpha1.PodSelectionStrategyAny, + } + pvc := newPVCForRestoreDecision("data", "pd", "") + + decision, err := reconciler.decidePVCRestore(intctrlutil.RequestCtx{Ctx: context.Background()}, pvc, backup, nil) + + require.NoError(t, err) + require.Equal(t, pvcRestoreModeProvisionOnly, decision.mode) + require.False(t, decision.skipPostReady, + "postReady must not be skipped when no volume-level restore exists — it may be the only restore path") +} + func TestMatchToPopulateSupportsRestoreAndBackupDataSources(t *testing.T) { apiGroup := dptypes.DataprotectionAPIGroup reconciler := &VolumePopulatorReconciler{} From 7e7501c49810a04676a24ac80ce92350c8aa4a86 Mon Sep 17 00:00:00 2001 From: Wei Cao Date: Wed, 24 Jun 2026 17:55:42 +0800 Subject: [PATCH 2/6] fix(dp): narrow skipPostReady override to target component PVCs only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address leon-ape's review: the previous fix `if !restoreData { skipPostReady = false }` was too broad — in multi-component scenarios (e.g. TiDB with pd/tikv), non-target component PVCs would also get skipPostReady cleared, potentially executing PostReady restore on wrong components. Refined approach: when !restoreData && skipPostReady, check if the PVC's component label matches the backup target component. Only clear skipPostReady (and recover sourceTarget) when the PVC belongs to the backup target component. Non-target component PVCs keep skipPostReady=true. --- .../volumepopulator_controller.go | 48 +++++++++++++++++-- .../volumepopulator_controller_test.go | 45 +++++++++++++++-- 2 files changed, 85 insertions(+), 8 deletions(-) diff --git a/controllers/dataprotection/volumepopulator_controller.go b/controllers/dataprotection/volumepopulator_controller.go index 3ec7696a1b2..d9ccb917ea2 100644 --- a/controllers/dataprotection/volumepopulator_controller.go +++ b/controllers/dataprotection/volumepopulator_controller.go @@ -397,10 +397,16 @@ func (r *VolumePopulatorReconciler) decidePVCRestore(reqCtx intctrlutil.RequestC if restoreData && !skipPostReady { mode = pvcRestoreModeRestoreData } - // When no volume-level restore exists, postReady may be the only restore - // path (e.g. multi-component logical backup). Don't block it. - if !restoreData { - skipPostReady = false + // When no volume-level restore exists and the PVC belongs to the backup + // target component, postReady must not be blocked — it may be the only + // data delivery path (e.g. logical backup via dataLoad). Only clear + // skipPostReady for the target component; non-target component PVCs + // (e.g. pd/tikv in a TiDB backup) must stay skipped. + if !restoreData && skipPostReady { + if target := backupTargetForPVCComponent(backup, pvc); target != nil { + skipPostReady = false + sourceTarget = target + } } return &pvcRestoreDecision{ mode: mode, @@ -473,6 +479,40 @@ func resolveSingleSourceTargetForPVC(target *dpv1alpha1.BackupStatusTarget, pvc return target, false, nil } +// backupTargetForPVCComponent returns the backup target whose component label +// matches the PVC's component. This is a fallback for when backupTargetMatchesPVC +// rejected the PVC due to pod-only labels (role, pod-name, etc.) that PVCs +// never carry. Returns nil if no target matches the PVC's component. +func backupTargetForPVCComponent(backup *dpv1alpha1.Backup, pvc *corev1.PersistentVolumeClaim) *dpv1alpha1.BackupStatusTarget { + pvcComp := pvc.Labels[constant.KBAppComponentLabelKey] + if pvcComp == "" { + return nil + } + if backup.Status.Target != nil { + if targetMatchesComponent(backup.Status.Target, pvcComp) { + return backup.Status.Target + } + return nil + } + for i := range backup.Status.Targets { + if targetMatchesComponent(&backup.Status.Targets[i], pvcComp) { + return &backup.Status.Targets[i] + } + } + return nil +} + +func targetMatchesComponent(target *dpv1alpha1.BackupStatusTarget, componentName string) bool { + if target.PodSelector == nil || target.PodSelector.LabelSelector == nil { + return true + } + targetComp := target.PodSelector.MatchLabels[constant.KBAppComponentLabelKey] + if targetComp == "" { + return true + } + return targetComp == componentName +} + func (r *VolumePopulatorReconciler) resolveShardingSourceTarget(reqCtx intctrlutil.RequestCtx, pvc *corev1.PersistentVolumeClaim, backup *dpv1alpha1.Backup) (*dpv1alpha1.BackupStatusTarget, bool, error) { diff --git a/controllers/dataprotection/volumepopulator_controller_test.go b/controllers/dataprotection/volumepopulator_controller_test.go index d2316bab018..baad992c3dc 100644 --- a/controllers/dataprotection/volumepopulator_controller_test.go +++ b/controllers/dataprotection/volumepopulator_controller_test.go @@ -859,7 +859,7 @@ func TestDecidePVCRestoreTreatsNilTargetVolumesAsProvisionOnly(t *testing.T) { require.False(t, decision.skipPostReady) } -func TestDecidePVCRestoreDoesNotSkipPostReadyWhenNoTargetVolumes(t *testing.T) { +func TestDecidePVCRestoreNoTargetVolumesTargetComponent(t *testing.T) { reconciler := &VolumePopulatorReconciler{} backup := newBackupForRestoreDecision(nil, nil) backup.Status.BackupMethod.TargetVolumes = nil @@ -868,18 +868,55 @@ func TestDecidePVCRestoreDoesNotSkipPostReadyWhenNoTargetVolumes(t *testing.T) { MatchLabels: map[string]string{ constant.AppInstanceLabelKey: "source-cluster", constant.KBAppComponentLabelKey: "tidb", + constant.RoleLabelKey: "leader", }, }, Strategy: dpv1alpha1.PodSelectionStrategyAny, } - pvc := newPVCForRestoreDecision("data", "pd", "") + // PVC from the backup target component: postReady must not be skipped + pvc := newPVCForRestoreDecision("data", "tidb", "") decision, err := reconciler.decidePVCRestore(intctrlutil.RequestCtx{Ctx: context.Background()}, pvc, backup, nil) - require.NoError(t, err) require.Equal(t, pvcRestoreModeProvisionOnly, decision.mode) require.False(t, decision.skipPostReady, - "postReady must not be skipped when no volume-level restore exists — it may be the only restore path") + "target component PVC must not skip postReady when no volume-level restore exists") + require.NotNil(t, decision.sourceTarget, + "sourceTarget must be recovered for the target component") + + // PVC from a different component: postReady must be skipped + pvc = newPVCForRestoreDecision("data", "pd", "") + decision, err = reconciler.decidePVCRestore(intctrlutil.RequestCtx{Ctx: context.Background()}, pvc, backup, nil) + require.NoError(t, err) + require.Equal(t, pvcRestoreModeProvisionOnly, decision.mode) + require.True(t, decision.skipPostReady, + "non-target component PVC must skip postReady to avoid running restore on wrong component") + require.Nil(t, decision.sourceTarget) +} + +func TestDecidePVCRestoreNoTargetVolumesMultiComponentTargets(t *testing.T) { + reconciler := &VolumePopulatorReconciler{} + backup := newBackupForRestoreDecision(nil, []string{"etcd"}) + backup.Status.BackupMethod.TargetVolumes = nil + backup.Status.Targets[0].PodSelector = &dpv1alpha1.PodSelector{ + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + constant.AppInstanceLabelKey: "source-cluster", + constant.KBAppComponentLabelKey: "etcd", + constant.RoleLabelKey: "leader", + }, + }, + Strategy: dpv1alpha1.PodSelectionStrategyAny, + } + + // Etcd PVC matches target component — postReady enabled + pvc := newPVCForRestoreDecision("data", "etcd", "") + decision, err := reconciler.decidePVCRestore(intctrlutil.RequestCtx{Ctx: context.Background()}, pvc, backup, nil) + require.NoError(t, err) + require.Equal(t, pvcRestoreModeProvisionOnly, decision.mode) + require.False(t, decision.skipPostReady) + require.NotNil(t, decision.sourceTarget) + require.Equal(t, "etcd", decision.sourceTarget.Name) } func TestMatchToPopulateSupportsRestoreAndBackupDataSources(t *testing.T) { From 32020fe43c6ffe2d3cac3e335861938739bce405 Mon Sep 17 00:00:00 2001 From: Wei Cao Date: Wed, 24 Jun 2026 17:59:52 +0800 Subject: [PATCH 3/6] fix(dp): tighten targetMatchesComponent when selector lacks component label When the backup target PodSelector exists but has no KBAppComponentLabelKey, targetMatchesComponent now returns false instead of true. This prevents the fallback from matching arbitrary component PVCs when we cannot confirm component membership. The no-PodSelector case (nil selector) still returns true -- that means the backup applies to all components by design. --- .../volumepopulator_controller.go | 2 +- .../volumepopulator_controller_test.go | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/controllers/dataprotection/volumepopulator_controller.go b/controllers/dataprotection/volumepopulator_controller.go index d9ccb917ea2..46d83a9b7e3 100644 --- a/controllers/dataprotection/volumepopulator_controller.go +++ b/controllers/dataprotection/volumepopulator_controller.go @@ -508,7 +508,7 @@ func targetMatchesComponent(target *dpv1alpha1.BackupStatusTarget, componentName } targetComp := target.PodSelector.MatchLabels[constant.KBAppComponentLabelKey] if targetComp == "" { - return true + return false } return targetComp == componentName } diff --git a/controllers/dataprotection/volumepopulator_controller_test.go b/controllers/dataprotection/volumepopulator_controller_test.go index baad992c3dc..519d17670bb 100644 --- a/controllers/dataprotection/volumepopulator_controller_test.go +++ b/controllers/dataprotection/volumepopulator_controller_test.go @@ -919,6 +919,29 @@ func TestDecidePVCRestoreNoTargetVolumesMultiComponentTargets(t *testing.T) { require.Equal(t, "etcd", decision.sourceTarget.Name) } +func TestDecidePVCRestoreNoTargetVolumesNoComponentLabel(t *testing.T) { + reconciler := &VolumePopulatorReconciler{} + backup := newBackupForRestoreDecision(nil, nil) + backup.Status.BackupMethod.TargetVolumes = nil + backup.Status.Target.PodSelector = &dpv1alpha1.PodSelector{ + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + constant.AppInstanceLabelKey: "source-cluster", + constant.RoleLabelKey: "leader", + }, + }, + Strategy: dpv1alpha1.PodSelectionStrategyAny, + } + + pvc := newPVCForRestoreDecision("data", "mysql", "") + decision, err := reconciler.decidePVCRestore(intctrlutil.RequestCtx{Ctx: context.Background()}, pvc, backup, nil) + require.NoError(t, err) + require.Equal(t, pvcRestoreModeProvisionOnly, decision.mode) + require.True(t, decision.skipPostReady, + "when target selector has no component label, cannot confirm PVC belongs to target — keep skipPostReady") + require.Nil(t, decision.sourceTarget) +} + func TestMatchToPopulateSupportsRestoreAndBackupDataSources(t *testing.T) { apiGroup := dptypes.DataprotectionAPIGroup reconciler := &VolumePopulatorReconciler{} From 22180f746ec79d30181bfce38e1e6113ff4555cd Mon Sep 17 00:00:00 2001 From: Wei Cao Date: Thu, 25 Jun 2026 12:10:12 +0800 Subject: [PATCH 4/6] fix(dp): match target component expressions --- .../volumepopulator_controller.go | 29 +++++++++++++-- .../volumepopulator_controller_test.go | 36 +++++++++++++++++++ 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/controllers/dataprotection/volumepopulator_controller.go b/controllers/dataprotection/volumepopulator_controller.go index 46d83a9b7e3..5803b76eaf0 100644 --- a/controllers/dataprotection/volumepopulator_controller.go +++ b/controllers/dataprotection/volumepopulator_controller.go @@ -35,6 +35,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" @@ -506,11 +507,33 @@ func targetMatchesComponent(target *dpv1alpha1.BackupStatusTarget, componentName if target.PodSelector == nil || target.PodSelector.LabelSelector == nil { return true } - targetComp := target.PodSelector.MatchLabels[constant.KBAppComponentLabelKey] - if targetComp == "" { + componentSelector := componentLabelSelector(target.PodSelector.LabelSelector) + if componentSelector == nil { return false } - return targetComp == componentName + selector, err := metav1.LabelSelectorAsSelector(componentSelector) + if err != nil { + return false + } + return selector.Matches(labels.Set{constant.KBAppComponentLabelKey: componentName}) +} + +func componentLabelSelector(selector *metav1.LabelSelector) *metav1.LabelSelector { + componentSelector := &metav1.LabelSelector{} + if component, ok := selector.MatchLabels[constant.KBAppComponentLabelKey]; ok { + componentSelector.MatchLabels = map[string]string{ + constant.KBAppComponentLabelKey: component, + } + } + for _, expression := range selector.MatchExpressions { + if expression.Key == constant.KBAppComponentLabelKey { + componentSelector.MatchExpressions = append(componentSelector.MatchExpressions, expression) + } + } + if len(componentSelector.MatchLabels) == 0 && len(componentSelector.MatchExpressions) == 0 { + return nil + } + return componentSelector } func (r *VolumePopulatorReconciler) resolveShardingSourceTarget(reqCtx intctrlutil.RequestCtx, diff --git a/controllers/dataprotection/volumepopulator_controller_test.go b/controllers/dataprotection/volumepopulator_controller_test.go index 519d17670bb..7200e38aa4c 100644 --- a/controllers/dataprotection/volumepopulator_controller_test.go +++ b/controllers/dataprotection/volumepopulator_controller_test.go @@ -894,6 +894,42 @@ func TestDecidePVCRestoreNoTargetVolumesTargetComponent(t *testing.T) { require.Nil(t, decision.sourceTarget) } +func TestDecidePVCRestoreNoTargetVolumesTargetComponentMatchExpression(t *testing.T) { + reconciler := &VolumePopulatorReconciler{} + backup := newBackupForRestoreDecision(nil, nil) + backup.Status.BackupMethod.TargetVolumes = nil + backup.Status.Target.PodSelector = &dpv1alpha1.PodSelector{ + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + constant.AppInstanceLabelKey: "source-cluster", + constant.RoleLabelKey: "leader", + }, + MatchExpressions: []metav1.LabelSelectorRequirement{{ + Key: constant.KBAppComponentLabelKey, + Operator: metav1.LabelSelectorOpIn, + Values: []string{"tidb"}, + }}, + }, + Strategy: dpv1alpha1.PodSelectionStrategyAny, + } + + pvc := newPVCForRestoreDecision("data", "tidb", "") + decision, err := reconciler.decidePVCRestore(intctrlutil.RequestCtx{Ctx: context.Background()}, pvc, backup, nil) + require.NoError(t, err) + require.Equal(t, pvcRestoreModeProvisionOnly, decision.mode) + require.False(t, decision.skipPostReady, + "target component selected by MatchExpressions must not skip postReady") + require.NotNil(t, decision.sourceTarget) + + pvc = newPVCForRestoreDecision("data", "pd", "") + decision, err = reconciler.decidePVCRestore(intctrlutil.RequestCtx{Ctx: context.Background()}, pvc, backup, nil) + require.NoError(t, err) + require.Equal(t, pvcRestoreModeProvisionOnly, decision.mode) + require.True(t, decision.skipPostReady, + "non-target component must still skip postReady when component expression does not match") + require.Nil(t, decision.sourceTarget) +} + func TestDecidePVCRestoreNoTargetVolumesMultiComponentTargets(t *testing.T) { reconciler := &VolumePopulatorReconciler{} backup := newBackupForRestoreDecision(nil, []string{"etcd"}) From 9b8d41d3e3636045acf03ec3a3d105bc73c7f00c Mon Sep 17 00:00:00 2001 From: Wei Cao Date: Fri, 26 Jun 2026 01:51:33 +0800 Subject: [PATCH 5/6] fix(dp): requeue volume populator conflicts --- .../volumepopulator_controller.go | 40 ++++++++++- .../volumepopulator_controller_test.go | 67 +++++++++++++++++++ 2 files changed, 105 insertions(+), 2 deletions(-) diff --git a/controllers/dataprotection/volumepopulator_controller.go b/controllers/dataprotection/volumepopulator_controller.go index 5803b76eaf0..b1791c98eb9 100644 --- a/controllers/dataprotection/volumepopulator_controller.go +++ b/controllers/dataprotection/volumepopulator_controller.go @@ -126,6 +126,9 @@ func (r *VolumePopulatorReconciler) handleSyncPVCError(reqCtx intctrlutil.Reques if requeueErr, ok := err.(intctrlutil.RequeueError); ok { return intctrlutil.RequeueAfter(requeueErr.RequeueAfter(), reqCtx.Log, requeueErr.Reason()) } + if apierrors.IsConflict(err) { + return intctrlutil.RequeueWithError(err, reqCtx.Log, "") + } if r.ContainPopulatingCondition(pvc) { // Ignore the error if an external controller handles this PVC. return intctrlutil.Reconciled() @@ -1037,7 +1040,17 @@ func (r *VolumePopulatorReconciler) patchInternalRestoreStatus(reqCtx intctrluti if reflect.DeepEqual(restoreMgr.OriginalRestore.Status, restoreMgr.Restore.Status) { return nil } - return r.Client.Status().Patch(reqCtx.Ctx, restoreMgr.Restore, client.MergeFrom(restoreMgr.OriginalRestore)) + latest := &dpv1alpha1.Restore{} + if err := r.Client.Get(reqCtx.Ctx, client.ObjectKeyFromObject(restoreMgr.Restore), latest); err != nil { + return err + } + mergedStatus := mergeRestoreStatusChanges(restoreMgr.OriginalRestore.Status, restoreMgr.Restore.Status, latest.Status) + if reflect.DeepEqual(latest.Status, mergedStatus) { + return nil + } + statusPatch := client.MergeFrom(latest.DeepCopy()) + latest.Status = mergedStatus + return r.Client.Status().Patch(reqCtx.Ctx, latest, statusPatch) } func (r *VolumePopulatorReconciler) patchInternalRestoreMetaAndStatus(reqCtx intctrlutil.RequestCtx, @@ -1055,10 +1068,33 @@ func (r *VolumePopulatorReconciler) patchInternalRestoreMetaAndStatus(reqCtx int return err } statusPatch := client.MergeFrom(latest.DeepCopy()) - latest.Status = restore.Status + latest.Status = mergeRestoreStatusChanges(original.Status, restore.Status, latest.Status) return r.Client.Status().Patch(reqCtx.Ctx, latest, statusPatch) } +func mergeRestoreStatusChanges(original, desired, latest dpv1alpha1.RestoreStatus) dpv1alpha1.RestoreStatus { + merged := latest + if !reflect.DeepEqual(original.Phase, desired.Phase) { + merged.Phase = desired.Phase + } + if !reflect.DeepEqual(original.StartTimestamp, desired.StartTimestamp) { + merged.StartTimestamp = desired.StartTimestamp + } + if !reflect.DeepEqual(original.CompletionTimestamp, desired.CompletionTimestamp) { + merged.CompletionTimestamp = desired.CompletionTimestamp + } + if !reflect.DeepEqual(original.Duration, desired.Duration) { + merged.Duration = desired.Duration + } + if !reflect.DeepEqual(original.Actions, desired.Actions) { + merged.Actions = desired.Actions + } + if !reflect.DeepEqual(original.Conditions, desired.Conditions) { + merged.Conditions = desired.Conditions + } + return merged +} + func (r *VolumePopulatorReconciler) completeBoundPVCIfNeeded(reqCtx intctrlutil.RequestCtx, pvc *corev1.PersistentVolumeClaim, restoreCtx *pvcRestoreContext) error { diff --git a/controllers/dataprotection/volumepopulator_controller_test.go b/controllers/dataprotection/volumepopulator_controller_test.go index 7200e38aa4c..b2402f49eeb 100644 --- a/controllers/dataprotection/volumepopulator_controller_test.go +++ b/controllers/dataprotection/volumepopulator_controller_test.go @@ -31,14 +31,17 @@ import ( batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" kbappsv1 "github.com/apecloud/kubeblocks/apis/apps/v1" dpv1alpha1 "github.com/apecloud/kubeblocks/apis/dataprotection/v1alpha1" @@ -1261,6 +1264,70 @@ func TestHandleSyncPVCErrorKeepsInternalRequeueWhenPVCIsPopulating(t *testing.T) require.Equal(t, reconcileInterval, result.RequeueAfter) } +func TestHandleSyncPVCErrorRequeuesConflictWhenPVCIsPopulating(t *testing.T) { + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "target"}, + Status: corev1.PersistentVolumeClaimStatus{ + Conditions: []corev1.PersistentVolumeClaimCondition{{ + Type: PersistentVolumeClaimPopulating, + Status: corev1.ConditionTrue, + }}, + }, + } + reconciler := &VolumePopulatorReconciler{Recorder: record.NewFakeRecorder(10)} + conflict := apierrors.NewConflict( + schema.GroupResource{Resource: "persistentvolumeclaims"}, + pvc.Name, + fmt.Errorf("stale resource version"), + ) + + _, err := reconciler.handleSyncPVCError(intctrlutil.RequestCtx{Ctx: context.Background()}, pvc, conflict) + + require.Error(t, err) + require.True(t, apierrors.IsConflict(err), err.Error()) +} + +func TestPatchInternalRestoreStatusUsesLatestRestoreResourceVersion(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, dpv1alpha1.AddToScheme(scheme)) + latest := &dpv1alpha1.Restore{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "restore", + ResourceVersion: "42", + }, + Status: dpv1alpha1.RestoreStatus{Phase: dpv1alpha1.RestorePhaseAsDataSource}, + } + original := latest.DeepCopy() + original.ResourceVersion = "1" + original.Status = dpv1alpha1.RestoreStatus{} + var patchedResourceVersion string + cli := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(latest). + WithObjects(latest). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourcePatch: func(ctx context.Context, c client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + require.Equal(t, "status", subResourceName) + patchedResourceVersion = obj.GetResourceVersion() + return c.SubResource(subResourceName).Patch(ctx, obj, patch, opts...) + }, + }). + Build() + reconciler := &VolumePopulatorReconciler{Client: cli} + restoreMgr := dprestore.NewRestoreManager(original.DeepCopy(), nil, scheme, cli) + dprestore.SetRestoreStageCondition(restoreMgr.Restore, dpv1alpha1.PrepareData, dprestore.ReasonSucceed, "prepare data successfully") + + err := reconciler.patchInternalRestoreStatus(intctrlutil.RequestCtx{Ctx: context.Background()}, restoreMgr) + + require.NoError(t, err) + require.Equal(t, "42", patchedResourceVersion) + got := &dpv1alpha1.Restore{} + require.NoError(t, cli.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "restore"}, got)) + require.Equal(t, dpv1alpha1.RestorePhaseAsDataSource, got.Status.Phase) + require.NotEmpty(t, got.Status.Conditions) +} + func TestDecidePVCRestoreAssignsShardingTargetsByStableComponentOrder(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, corev1.AddToScheme(scheme)) From 943044aed667a71c2848da47c7b103920d6f6245 Mon Sep 17 00:00:00 2001 From: Wei Cao Date: Fri, 26 Jun 2026 04:35:07 +0800 Subject: [PATCH 6/6] fix(dp): requeue pending volume populate jobs --- .../volumepopulator_controller.go | 2 +- .../volumepopulator_controller_test.go | 120 ++++++++++++++++++ 2 files changed, 121 insertions(+), 1 deletion(-) diff --git a/controllers/dataprotection/volumepopulator_controller.go b/controllers/dataprotection/volumepopulator_controller.go index b1791c98eb9..9b1e4cfe04b 100644 --- a/controllers/dataprotection/volumepopulator_controller.go +++ b/controllers/dataprotection/volumepopulator_controller.go @@ -976,7 +976,7 @@ func (r *VolumePopulatorReconciler) Populate(reqCtx intctrlutil.RequestCtx, pvc if err = r.patchInternalRestoreStatus(reqCtx, restoreMgr); err != nil { return err } - return nil + return intctrlutil.NewRequeueError(reconcileInterval, "waiting for prepareData job") } if actionFailed { dprestore.SetRestoreStageCondition(restoreMgr.Restore, dpv1alpha1.PrepareData, dprestore.ReasonFailed, "restore prepareData job failed") diff --git a/controllers/dataprotection/volumepopulator_controller_test.go b/controllers/dataprotection/volumepopulator_controller_test.go index b2402f49eeb..41f2cf7aae3 100644 --- a/controllers/dataprotection/volumepopulator_controller_test.go +++ b/controllers/dataprotection/volumepopulator_controller_test.go @@ -1442,6 +1442,126 @@ func TestProvisionOnlyCreatesPopulatePVCWithoutJob(t *testing.T) { require.Empty(t, jobs.Items) } +func TestPopulateRequeuesWhenRestoreContainerFinishedBeforeJobComplete(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, batchv1.AddToScheme(scheme)) + require.NoError(t, dpv1alpha1.AddToScheme(scheme)) + apiGroup := dptypes.DataprotectionAPIGroup + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "data-target-0", + UID: "target-pvc-uid", + ResourceVersion: "1", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, + Resources: corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("1Gi")}, + }, + DataSourceRef: &corev1.TypedObjectReference{ + APIGroup: &apiGroup, + Kind: dptypes.BackupKind, + Name: "backup", + }, + }, + } + backup := newBackupForRestoreDecision([]string{"data"}, nil) + backup.Status.Target.PodSelector = &dpv1alpha1.PodSelector{Strategy: dpv1alpha1.PodSelectionStrategyAny} + actionSet := &dpv1alpha1.ActionSet{ + ObjectMeta: metav1.ObjectMeta{Name: "prepare-action"}, + Spec: dpv1alpha1.ActionSetSpec{ + BackupType: dpv1alpha1.BackupTypeFull, + Restore: &dpv1alpha1.RestoreActionSpec{ + PrepareData: &dpv1alpha1.JobActionSpec{ + BaseJobActionSpec: dpv1alpha1.BaseJobActionSpec{ + Image: "busybox", + Command: []string{"restore"}, + }, + }, + }, + }, + } + restore := &dpv1alpha1.Restore{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "internal-restore", + }, + Spec: dpv1alpha1.RestoreSpec{ + Backup: dpv1alpha1.BackupRef{Name: backup.Name, Namespace: backup.Namespace}, + PrepareDataConfig: &dpv1alpha1.PrepareDataConfig{ + DataSourceRef: &dpv1alpha1.VolumeConfig{ + VolumeSource: "data", + MountPath: "/data", + }, + }, + }, + } + populatePVCName := getPopulatePVCName(pvc.UID) + jobName := fmt.Sprintf("%s-%d", populatePVCName, 0) + job := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: jobName, + Labels: map[string]string{ + dprestore.DataProtectionRestoreLabelKey: restore.Name, + dprestore.DataProtectionPopulatePVCLabelKey: populatePVCName, + }, + }, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: jobName + "-pod", + Labels: map[string]string{"job-name": jobName}, + }, + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: dprestore.Restore, + State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 0}}, + }, + { + Name: "restore-manager", + State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}, + }, + }, + }, + } + cli := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(pvc, restore). + WithObjects(pvc, backup, actionSet, restore, job, pod). + Build() + reconciler := &VolumePopulatorReconciler{ + Client: cli, + Scheme: scheme, + Recorder: record.NewFakeRecorder(10), + } + restoreMgr := dprestore.NewRestoreManager(restore.DeepCopy(), nil, scheme, cli) + restoreMgr.PrepareDataBackupSets = []dprestore.BackupActionSet{{ + Backup: backup, + ActionSet: actionSet, + }} + + err := reconciler.Populate( + intctrlutil.RequestCtx{Ctx: context.Background()}, + pvc, + &pvcRestoreContext{restoreMgr: restoreMgr, mode: pvcRestoreModeRestoreData}, + ) + + require.Error(t, err) + require.True(t, intctrlutil.IsRequeueError(err), err.Error()) + currentPod := &corev1.Pod{} + require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(pod), currentPod)) + require.Equal(t, "true", currentPod.Annotations[dprestore.DataProtectionStopRestoreManagerAnnotationKey]) + currentRestore := &dpv1alpha1.Restore{} + require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(restore), currentRestore)) + require.Len(t, currentRestore.Status.Actions.PrepareData, 1) + require.Equal(t, dpv1alpha1.RestoreActionProcessing, currentRestore.Status.Actions.PrepareData[0].Status) +} + func TestBuildPostReadyRestoreSelectsHighestPriorityRole(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, corev1.AddToScheme(scheme))