From e593ba73f98cbea9ffa679bb2a880300709ddc9a Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Tue, 14 Jul 2026 09:22:52 -0700 Subject: [PATCH] Fix PodVolumeBackup metadata loss on fs-backup timeout (#9995) * Fix PodVolumeBackup metadata loss on fs-backup timeout When a backup hits the fs-backup timeout, WaitAllPodVolumesProcessed returned nil because PVBs were only collected from the indexer in the done branch of the select. This discarded all PVB metadata including already-completed PVBs, making their data unrestorable. Move the PVB collection loop to run after the select so tracked PVBs are always persisted regardless of timeout. Fixes #9986 Signed-off-by: Shubham Pampattiwar * Add changelog for PR #9995 Signed-off-by: Shubham Pampattiwar * Filter non-completed PVBs in hasPodVolumeBackup After preserving tracked PVBs on timeout, non-completed PVBs (in-progress or with no snapshot ID) would cause hasPodVolumeBackup to return true, leading the restore to skip the original PV and dynamically re-provision it without any data to restore from. Only match PVBs that are Completed with a valid SnapshotID. Signed-off-by: Shubham Pampattiwar * Add unit tests for hasPodVolumeBackup phase filtering Verify that hasPodVolumeBackup only matches PVBs that are Completed with a valid SnapshotID, and rejects in-progress, failed, or empty-snapshot PVBs. Signed-off-by: Shubham Pampattiwar --------- Signed-off-by: Shubham Pampattiwar --- .../unreleased/9995-shubham-pampattiwar | 1 + pkg/podvolume/backupper.go | 27 ++++--- pkg/podvolume/backupper_test.go | 14 +++- pkg/restore/restore.go | 3 + pkg/restore/restore_test.go | 81 +++++++++++++++++++ 5 files changed, 110 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/9995-shubham-pampattiwar diff --git a/changelogs/unreleased/9995-shubham-pampattiwar b/changelogs/unreleased/9995-shubham-pampattiwar new file mode 100644 index 000000000..691ab8d1a --- /dev/null +++ b/changelogs/unreleased/9995-shubham-pampattiwar @@ -0,0 +1 @@ +Fix PodVolumeBackup metadata loss on fs-backup timeout, which caused all fs-backup volumes to become unrestorable diff --git a/pkg/podvolume/backupper.go b/pkg/podvolume/backupper.go index c99ab8a77..5864a2090 100644 --- a/pkg/podvolume/backupper.go +++ b/pkg/podvolume/backupper.go @@ -412,18 +412,21 @@ func (b *backupper) WaitAllPodVolumesProcessed(log logrus.FieldLogger) []*velero case <-b.ctx.Done(): log.Error("timed out waiting for all PodVolumeBackups to complete") case <-done: - for _, obj := range b.pvbIndexer.List() { - pvb, ok := obj.(*velerov1api.PodVolumeBackup) - if !ok { - log.Errorf("expected PodVolumeBackup, but got %T", obj) - continue - } - podVolumeBackups = append(podVolumeBackups, pvb) - if pvb.Status.Phase == velerov1api.PodVolumeBackupPhaseFailed { - log.Errorf("pod volume backup failed: %s", pvb.Status.Message) - } else if pvb.Status.Phase == velerov1api.PodVolumeBackupPhaseCanceled { - log.Errorf("pod volume backup canceled: %s", pvb.Status.Message) - } + } + + // Collect tracked PVBs regardless of whether we timed out or completed normally. + // On timeout, already-completed PVBs must still be persisted so their data remains restorable. + for _, obj := range b.pvbIndexer.List() { + pvb, ok := obj.(*velerov1api.PodVolumeBackup) + if !ok { + log.Errorf("expected PodVolumeBackup, but got %T", obj) + continue + } + podVolumeBackups = append(podVolumeBackups, pvb) + if pvb.Status.Phase == velerov1api.PodVolumeBackupPhaseFailed { + log.Errorf("pod volume backup failed: %s", pvb.Status.Message) + } else if pvb.Status.Phase == velerov1api.PodVolumeBackupPhaseCanceled { + log.Errorf("pod volume backup canceled: %s", pvb.Status.Message) } } return podVolumeBackups diff --git a/pkg/podvolume/backupper_test.go b/pkg/podvolume/backupper_test.go index e6042ede1..66ad9e5ae 100644 --- a/pkg/podvolume/backupper_test.go +++ b/pkg/podvolume/backupper_test.go @@ -757,16 +757,18 @@ func TestWaitAllPodVolumesProcessed(t *testing.T) { statusToBeUpdated *velerov1api.PodVolumeBackupStatus expectedErr string expectedPVBPhase velerov1api.PodVolumeBackupPhase + expectedPVBCount int }{ { name: "contains no pvb should report no error", ctx: timeoutCtx, }, { - name: "context canceled", - ctx: timeoutCtx, - pvb: pvb, - expectedErr: "timed out waiting for all PodVolumeBackups to complete", + name: "context canceled should still return tracked pvbs", + ctx: timeoutCtx, + pvb: pvb, + expectedErr: "timed out waiting for all PodVolumeBackups to complete", + expectedPVBCount: 1, }, { name: "failed pvbs", @@ -834,6 +836,10 @@ func TestWaitAllPodVolumesProcessed(t *testing.T) { assert.Nil(t, logHook.entry) } + if c.expectedPVBCount > 0 { + require.Len(t, pvbs, c.expectedPVBCount) + } + if c.expectedPVBPhase != "" { require.Len(t, pvbs, 1) assert.Equal(t, c.expectedPVBPhase, pvbs[0].Status.Phase) diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index a1213eec0..a71fc4b23 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -2347,6 +2347,9 @@ func hasPodVolumeBackup(unstructuredPV *unstructured.Unstructured, ctx *restoreC var found bool for _, pvb := range ctx.podVolumeBackups { + if pvb.Status.Phase != velerov1api.PodVolumeBackupPhaseCompleted || pvb.Status.SnapshotID == "" { + continue + } if pvb.Spec.Pod.Namespace == pv.Spec.ClaimRef.Namespace && pvb.GetAnnotations()[configs.PVCNameAnnotation] == pv.Spec.ClaimRef.Name { found = true break diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 6863784fb..fc4051387 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -4246,3 +4246,84 @@ func TestDetermineRestoreStatus(t *testing.T) { }) } } + +func TestHasPodVolumeBackup(t *testing.T) { + pvUnstructured := func() *unstructured.Unstructured { + pv := &corev1api.PersistentVolume{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "PersistentVolume"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pv", + }, + Spec: corev1api.PersistentVolumeSpec{ + ClaimRef: &corev1api.ObjectReference{ + Namespace: "test-ns", + Name: "test-pvc", + }, + }, + } + obj, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(pv) + return &unstructured.Unstructured{Object: obj} + } + + makePVB := func(phase velerov1api.PodVolumeBackupPhase, snapshotID string) *velerov1api.PodVolumeBackup { + return &velerov1api.PodVolumeBackup{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "velero.io/pvc-name": "test-pvc", + }, + }, + Spec: velerov1api.PodVolumeBackupSpec{ + Pod: corev1api.ObjectReference{ + Namespace: "test-ns", + }, + }, + Status: velerov1api.PodVolumeBackupStatus{ + Phase: phase, + SnapshotID: snapshotID, + }, + } + } + + tests := []struct { + name string + pvbs []*velerov1api.PodVolumeBackup + expected bool + }{ + { + name: "no pvbs", + pvbs: nil, + expected: false, + }, + { + name: "completed pvb with snapshot ID", + pvbs: []*velerov1api.PodVolumeBackup{makePVB(velerov1api.PodVolumeBackupPhaseCompleted, "snap-123")}, + expected: true, + }, + { + name: "in-progress pvb should not match", + pvbs: []*velerov1api.PodVolumeBackup{makePVB(velerov1api.PodVolumeBackupPhaseInProgress, "")}, + expected: false, + }, + { + name: "completed pvb with empty snapshot ID should not match", + pvbs: []*velerov1api.PodVolumeBackup{makePVB(velerov1api.PodVolumeBackupPhaseCompleted, "")}, + expected: false, + }, + { + name: "failed pvb should not match", + pvbs: []*velerov1api.PodVolumeBackup{makePVB(velerov1api.PodVolumeBackupPhaseFailed, "")}, + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx := &restoreContext{ + podVolumeBackups: tc.pvbs, + log: logrus.New(), + } + result := hasPodVolumeBackup(pvUnstructured(), ctx) + assert.Equal(t, tc.expected, result) + }) + } +}