PV remapClaimRefNS was being skipped when there was no snapshot (#3708)

Signed-off-by: Scott Seago <sseago@redhat.com>

changelogs/unreleased/3708-sseago

@@ -0,0 +1 @@
+Handle namespace mapping for PVs without snapshots on restore

pkg/restore/restore.go

@@ -1102,6 +1102,12 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso
 		default:
 			ctx.log.Infof("Restoring persistent volume as-is because it doesn't have a snapshot and its reclaim policy is not Delete.")
 
+			// Check to see if the claimRef.namespace field needs to be remapped, and do so if necessary.
+			_, err = remapClaimRefNS(ctx, obj)
+			if err != nil {
+				errs.Add(namespace, err)
+				return warnings, errs
+			}
 			obj = resetVolumeBindingInfo(obj)
 			// We call the pvRestorer here to clear out the PV's claimRef.UID,
 			// so it can be re-claimed when its PVC is restored and gets a new UID.

pkg/restore/restore_test.go

@@ -1811,6 +1811,8 @@ func TestRestorePersistentVolumes(t *testing.T) {
 		volumeSnapshotLocations []*velerov1api.VolumeSnapshotLocation
 		volumeSnapshotterGetter volumeSnapshotterGetter
 		want                    []*test.APIResource
+		wantError               bool
+		wantWarning             bool
 	}{
 		{
 			name:    "when a PV with a reclaim policy of delete has no snapshot and does not exist in-cluster, it does not get restored, and its PVC gets reset for dynamic provisioning",
@@ -2191,6 +2193,95 @@ func TestRestorePersistentVolumes(t *testing.T) {
 				),
 			},
 		},
+		{
+			name:    "when a PV without a snapshot is used by a PVC in a namespace that's being remapped, and the original PV exists in-cluster, the PV is not replaced and there is a restore warning",
+			restore: defaultRestore().NamespaceMappings("source-ns", "target-ns").Result(),
+			backup:  defaultBackup().Result(),
+			tarball: test.NewTarWriter(t).
+				AddItems(
+					"persistentvolumes",
+					builder.ForPersistentVolume("source-pv").
+						//ReclaimPolicy(corev1api.PersistentVolumeReclaimRetain).
+						AWSEBSVolumeID("source-volume").
+						ClaimRef("source-ns", "pvc-1").
+						Result(),
+				).
+				AddItems(
+					"persistentvolumeclaims",
+					builder.ForPersistentVolumeClaim("source-ns", "pvc-1").VolumeName("source-pv").Result(),
+				).
+				Done(),
+			apiResources: []*test.APIResource{
+				test.PVs(
+					builder.ForPersistentVolume("source-pv").
+						//ReclaimPolicy(corev1api.PersistentVolumeReclaimRetain).
+						AWSEBSVolumeID("source-volume").
+						ClaimRef("source-ns", "pvc-1").
+						Result(),
+				),
+				test.PVCs(),
+			},
+			want: []*test.APIResource{
+				test.PVs(
+					builder.ForPersistentVolume("source-pv").
+						AWSEBSVolumeID("source-volume").
+						ClaimRef("source-ns", "pvc-1").
+						Result(),
+				),
+				test.PVCs(
+					builder.ForPersistentVolumeClaim("target-ns", "pvc-1").
+						ObjectMeta(
+							builder.WithLabels("velero.io/backup-name", "backup-1", "velero.io/restore-name", "restore-1"),
+						).
+						VolumeName("source-pv").
+						Result(),
+				),
+			},
+			wantWarning: true,
+		},
+		{
+			name:    "when a PV without a snapshot is used by a PVC in a namespace that's being remapped, and the original PV does not exist in-cluster, the PV is not renamed",
+			restore: defaultRestore().NamespaceMappings("source-ns", "target-ns").Result(),
+			backup:  defaultBackup().Result(),
+			tarball: test.NewTarWriter(t).
+				AddItems(
+					"persistentvolumes",
+					builder.ForPersistentVolume("source-pv").
+						AWSEBSVolumeID("source-volume").
+						ClaimRef("source-ns", "pvc-1").
+						Result(),
+				).
+				AddItems(
+					"persistentvolumeclaims",
+					builder.ForPersistentVolumeClaim("source-ns", "pvc-1").VolumeName("source-pv").Result(),
+				).
+				Done(),
+			apiResources: []*test.APIResource{
+				test.PVs(),
+				test.PVCs(),
+			},
+			want: []*test.APIResource{
+				test.PVs(
+					builder.ForPersistentVolume("source-pv").
+						//ReclaimPolicy(corev1api.PersistentVolumeReclaimRetain).
+						ObjectMeta(
+							builder.WithLabels("velero.io/backup-name", "backup-1", "velero.io/restore-name", "restore-1"),
+						).
+						// the namespace for this PV's claimRef should be the one that the PVC was remapped into.
+						ClaimRef("target-ns", "pvc-1").
+						AWSEBSVolumeID("source-volume").
+						Result(),
+				),
+				test.PVCs(
+					builder.ForPersistentVolumeClaim("target-ns", "pvc-1").
+						ObjectMeta(
+							builder.WithLabels("velero.io/backup-name", "backup-1", "velero.io/restore-name", "restore-1"),
+						).
+						VolumeName("source-pv").
+						Result(),
+				),
+			},
+		},
 		{
 			name:    "when a PV is renamed and the original PV does not exist in-cluster, the PV should be renamed",
 			restore: defaultRestore().NamespaceMappings("source-ns", "target-ns").Result(),
@@ -2423,7 +2514,16 @@ func TestRestorePersistentVolumes(t *testing.T) {
 				tc.volumeSnapshotterGetter,
 			)
 
-			assertEmptyResults(t, warnings, errs)
+			if tc.wantWarning {
+				assertNonEmptyResults(t, "warning", warnings)
+			} else {
+				assertEmptyResults(t, warnings)
+			}
+			if tc.wantError {
+				assertNonEmptyResults(t, "error", errs)
+			} else {
+				assertEmptyResults(t, errs)
+			}
 			assertAPIContents(t, h, wantIDs)
 			assertRestoredItems(t, h, tc.want)
 		})
@@ -2804,6 +2904,17 @@ func assertEmptyResults(t *testing.T, res ...Result) {
 	}
 }
 
+func assertNonEmptyResults(t *testing.T, typeMsg string, res ...Result) {
+	t.Helper()
+	total := 0
+	for _, r := range res {
+		total += len(r.Cluster)
+		total += len(r.Namespaces)
+		total += len(r.Velero)
+	}
+	assert.Greater(t, total, 0, "Expected at least one "+typeMsg)
+}
+
 type harness struct {
 	*test.APIServer