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 <spampatt@redhat.com>

* Add changelog for PR #9995

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

* 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 <spampatt@redhat.com>

* 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 <spampatt@redhat.com>

---------

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

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

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

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)

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

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)
+		})
+	}
+}