fix(backups): delete a restored disk's PVC only once the restore can run

Deleting the original PVC of a keepOriginalPVC=false restore is only
safe when Velero can put the disk back. The Backup artifact outlives
the Velero Backup it points to, which Velero garbage-collects at TTL,
and nothing checked it before prepareForRestore halted the VM and
deleted the disks: a restore of an expired backup lost the volume where
it used to fail with the disk intact. The Velero Backup must now exist
and be Completed before any preparation starts.

A DataVolume that survives its deletion makes CDI recreate the claim
under the original name, which Velero then skips as an existing PVC,
so the restore would end Succeeded on a freshly imported disk. A failed
DataVolume deletion now fails the restore, and the PVCs are deleted
only once every DataVolume is gone. Reading the DataVolume back needs
get on datavolumes, which the ClusterRole did not grant.

Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
(cherry picked from commit 6d064a8602)
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>

internal/backupcontroller/velerostrategy_controller.go

@@ -686,8 +686,26 @@ func (r *RestoreJobReconciler) reconcileVeleroRestore(ctx context.Context, resto
 			}
 		}
 
+		// The Backup artifact outlives the Velero Backup it points to, which
+		// Velero garbage-collects once its TTL expires. Checked before
+		// prepareForRestore halts the application and deletes its disks, so a
+		// restore that cannot run leaves them in place.
+		veleroBackup := &velerov1.Backup{}
+		if err := r.Get(ctx, client.ObjectKey{Namespace: veleroNamespace, Name: veleroBackupName}, veleroBackup); err != nil {
+			if errors.IsNotFound(err) {
+				return r.markRestoreJobFailed(ctx, restoreJob, fmt.Sprintf(
+					"Velero Backup %s/%s not found; it may have expired", veleroNamespace, veleroBackupName))
+			}
+			return ctrl.Result{}, fmt.Errorf("failed to get Velero Backup %s: %w", veleroBackupName, err)
+		}
+		if veleroBackup.Status.Phase != velerov1.BackupPhaseCompleted {
+			return r.markRestoreJobFailed(ctx, restoreJob, fmt.Sprintf(
+				"Velero Backup %s/%s is %q, not %q", veleroNamespace, veleroBackupName,
+				veleroBackup.Status.Phase, velerov1.BackupPhaseCompleted))
+		}
+
 		// Resolve underlying resources once; prefer Backup status, fall back to Velero annotation.
-		ur := r.resolveUnderlyingResourcesForRestore(ctx, backup, veleroBackupName)
+		ur := resolveUnderlyingResourcesForRestore(backup, veleroBackup)
 
 		// Pre-restore: graceful shutdown, suspend HRs, rename PVCs (skipped for copy)
 		ready, result, err := r.prepareForRestore(ctx, restoreJob, backup, ur, target, restoreOpts)
@@ -949,14 +967,10 @@ func (r *RestoreJobReconciler) createResourceModifiersConfigMap(ctx context.Cont
 // resolveUnderlyingResourcesForRestore returns underlying resources for symmetric
 // restore label selectors. Velero Backup annotation is used when Backup.status was empty
 // (e.g. CRD without underlyingResources in schema).
-func (r *RestoreJobReconciler) resolveUnderlyingResourcesForRestore(ctx context.Context, backup *backupsv1alpha1.Backup, veleroBackupName string) *runtime.RawExtension {
+func resolveUnderlyingResourcesForRestore(backup *backupsv1alpha1.Backup, vb *velerov1.Backup) *runtime.RawExtension {
 	if backup.Status.UnderlyingResources != nil && len(backup.Status.UnderlyingResources.Raw) > 0 {
 		return backup.Status.UnderlyingResources
 	}
-	vb := &velerov1.Backup{}
-	if err := r.Get(ctx, client.ObjectKey{Namespace: veleroNamespace, Name: veleroBackupName}, vb); err != nil {
-		return backup.Status.UnderlyingResources
-	}
 	if urJSON, ok := vb.Annotations[underlyingResourcesAnnotation]; ok && urJSON != "" {
 		return &runtime.RawExtension{Raw: []byte(urJSON)}
 	}
@@ -1015,13 +1029,14 @@ func shortHash(input string) string {
 //  5. Renames existing PVCs to <name>-orig-<hash> so Velero can create fresh
 //     ones via Data Movement.
 //
-// Failures in individual steps are non-fatal: missing resources are expected
-// (e.g. restore requested when app was already deleted). Each action emits
-// a Kubernetes Event on the RestoreJob for observability. The rename is the
-// exception: the DataVolume deletion after it would hand a disk that was not
-// kept to the restore, as keepOriginalPVC=false does. A failed rename holds
-// the restore back and is retried, and an -orig name another PVC holds fails
-// the restore.
+// Missing resources are expected (e.g. restore requested when app was already
+// deleted), and a failure to suspend or halt is non-fatal. A failed rename
+// holds the restore back and is retried, since the DataVolume deletion after
+// it would hand a disk that was not kept to the restore, as
+// keepOriginalPVC=false does, and an -orig name another PVC holds fails the
+// restore. A DataVolume or a replaced PVC that cannot be deleted fails the
+// restore, since Velero would skip the claim left in place. Each action emits
+// a Kubernetes Event on the RestoreJob for observability.
 //
 // postRestoreRename renames VMInstance HelmRelease after Velero Restore completes.
 // Velero resource modifiers cannot change metadata.name, so this step creates
@@ -1183,15 +1198,28 @@ func (r *RestoreJobReconciler) prepareForRestore(ctx context.Context, restoreJob
 	}
 
 	// --- Step 4: Delete DataVolumes so CDI doesn't recreate PVCs ---
+	// A DataVolume still present makes CDI recreate its claim under the original
+	// name, which Velero then skips as an existing PVC: the restore would end
+	// Succeeded on a freshly imported disk, so nothing goes further until every
+	// DataVolume is gone.
 	if vmRes != nil {
+		var terminating []string
 		for _, dv := range vmRes.DataVolumes {
-			if err := r.deleteDataVolume(ctx, ns, dv.DataVolumeName); err != nil {
-				r.Recorder.Event(restoreJob, corev1.EventTypeWarning, "PrepareForRestore",
-					fmt.Sprintf("Failed to delete DataVolume %s: %v", dv.DataVolumeName, err))
-			} else {
-				r.Recorder.Event(restoreJob, corev1.EventTypeNormal, "PrepareForRestore",
-					fmt.Sprintf("Deleted DataVolume %s", dv.DataVolumeName))
+			gone, err := r.deleteDataVolume(ctx, ns, dv.DataVolumeName)
+			if err != nil {
+				return false, ctrl.Result{}, fmt.Errorf("failed to delete DataVolume %s: %w", dv.DataVolumeName, err)
 			}
+			if !gone {
+				terminating = append(terminating, dv.DataVolumeName)
+				continue
+			}
+			r.Recorder.Event(restoreJob, corev1.EventTypeNormal, "PrepareForRestore",
+				fmt.Sprintf("Deleted DataVolume %s", dv.DataVolumeName))
+		}
+		if len(terminating) > 0 {
+			r.Recorder.Event(restoreJob, corev1.EventTypeNormal, "PrepareForRestore",
+				fmt.Sprintf("Waiting for DataVolumes %s to be deleted", strings.Join(terminating, ", ")))
+			return false, ctrl.Result{RequeueAfter: defaultRestoreRequeueAfter}, nil
 		}
 	}
 
@@ -1277,19 +1305,35 @@ func (r *RestoreJobReconciler) haltVirtualMachine(ctx context.Context, ns, vmNam
 	return false, nil
 }
 
-// deleteDataVolume deletes a DataVolume so CDI doesn't recreate the PVC after rename.
-// Uses Orphan propagation so that the PVC the DV owns via ownerReference is
-// removed by prepareForRestore, which waits for it, rather than by background
-// garbage collection, which nothing waits for before the Velero Restore starts.
-func (r *RestoreJobReconciler) deleteDataVolume(ctx context.Context, ns, name string) error {
-	orphan := metav1.DeletePropagationOrphan
-	err := r.Resource(dataVolumeGVR).Namespace(ns).Delete(ctx, name, metav1.DeleteOptions{
-		PropagationPolicy: &orphan,
-	})
-	if err != nil && !errors.IsNotFound(err) {
-		return err
+// deleteDataVolume deletes a DataVolume so CDI doesn't recreate the PVC after
+// rename, and reports whether it is gone. Orphan propagation holds the DV behind
+// the orphan finalizer until garbage collection has released its PVC, and leaves
+// that PVC to prepareForRestore, which waits for its deletion, rather than to
+// background garbage collection, which nothing waits for before the Velero
+// Restore starts.
+func (r *RestoreJobReconciler) deleteDataVolume(ctx context.Context, ns, name string) (bool, error) {
+	dvClient := r.Resource(dataVolumeGVR).Namespace(ns)
+	dv, err := dvClient.Get(ctx, name, metav1.GetOptions{})
+	if err != nil {
+		if errors.IsNotFound(err) {
+			return true, nil
+		}
+		return false, err
 	}
-	return nil
+	if dv.GetDeletionTimestamp() == nil {
+		orphan := metav1.DeletePropagationOrphan
+		err := dvClient.Delete(ctx, name, metav1.DeleteOptions{PropagationPolicy: &orphan})
+		if err != nil && !errors.IsNotFound(err) {
+			return false, err
+		}
+	}
+	if _, err := dvClient.Get(ctx, name, metav1.GetOptions{}); err != nil {
+		if errors.IsNotFound(err) {
+			return true, nil
+		}
+		return false, err
+	}
+	return false, nil
 }
 
 // getOrigPVC returns the PVC named name, or nil when there is none. It reads

internal/backupcontroller/velerostrategy_controller_test.go

@@ -15,6 +15,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	dynamicfake "k8s.io/client-go/dynamic/fake"
 	"k8s.io/client-go/kubernetes/scheme"
+	k8stesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	clientfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -532,6 +533,203 @@ func TestPrepareForRestore_KeepOriginalPVCFalse_WaitsForTerminatingPVC(t *testin
 	}
 }
 
+func makeUnstructuredDataVolume(name, namespace string) *unstructured.Unstructured {
+	dv := &unstructured.Unstructured{}
+	dv.SetAPIVersion("cdi.kubevirt.io/v1beta1")
+	dv.SetKind("DataVolume")
+	dv.SetName(name)
+	dv.SetNamespace(namespace)
+	return dv
+}
+
+// A DataVolume still in place makes CDI recreate the claim under its original
+// name, so the PVC must survive until the DataVolume is confirmed gone.
+func TestPrepareForRestore_KeepOriginalPVCFalse_KeepsPVCWhileDataVolumeRemains(t *testing.T) {
+	tests := []struct {
+		name      string
+		deleteErr error
+		wantErr   bool
+	}{
+		{
+			name:      "DataVolume deletion fails",
+			deleteErr: errors.NewForbidden(dataVolumeGVR.GroupResource(), "vm-disk-ubuntu-source", nil),
+			wantErr:   true,
+		},
+		{
+			name: "DataVolume deletion is still pending",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ns := "tenant-root"
+			restoreJob, backup, ur, target, opts := inPlaceVMRestoreWithoutKeep(ns)
+			pvc := &corev1.PersistentVolumeClaim{
+				ObjectMeta: metav1.ObjectMeta{Name: "vm-disk-ubuntu-source", Namespace: ns},
+			}
+			reconciler := newTestRestoreJobReconcilerWithDynamic(t,
+				[]runtime.Object{makeUnstructuredDataVolume("vm-disk-ubuntu-source", ns)},
+				pvc, restoreJob, backup)
+			reconciler.Interface.(*dynamicfake.FakeDynamicClient).PrependReactor("delete", "datavolumes",
+				func(k8stesting.Action) (bool, runtime.Object, error) {
+					return true, nil, tt.deleteErr
+				})
+
+			ctx := context.Background()
+			ready, result, err := reconciler.prepareForRestore(ctx, restoreJob, backup, ur, target, opts)
+			if tt.wantErr {
+				if err == nil {
+					t.Fatal("expected an error when the DataVolume cannot be deleted")
+				}
+			} else {
+				if err != nil {
+					t.Fatalf("prepareForRestore() error = %v", err)
+				}
+				if result.RequeueAfter == 0 {
+					t.Error("expected a requeue while the DataVolume is still present")
+				}
+			}
+			if ready {
+				t.Fatal("expected ready=false while the DataVolume is still present")
+			}
+
+			got := &corev1.PersistentVolumeClaim{}
+			if err := reconciler.Get(ctx, client.ObjectKeyFromObject(pvc), got); err != nil {
+				t.Fatalf("original PVC should survive while its DataVolume remains: %v", err)
+			}
+			if got.DeletionTimestamp != nil {
+				t.Error("original PVC should not be marked for deletion while its DataVolume remains")
+			}
+		})
+	}
+}
+
+// keepOriginalPVC=true must never delete the original claim, even when the
+// rename that preserves it fails and leaves it under its original name.
+func TestPrepareForRestore_KeepOriginalPVCTrue_FailedRenameKeepsPVC(t *testing.T) {
+	ns := "tenant-root"
+	restoreJob, backup, ur, target, _ := inPlaceVMRestoreWithoutKeep(ns)
+	opts := RestoreOptions{KeepOriginalPVC: new(true)}
+
+	pvc := &corev1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{Name: "vm-disk-ubuntu-source", Namespace: ns},
+		// The PV does not exist, so renamePVC fails before touching the claim.
+		Spec: corev1.PersistentVolumeClaimSpec{VolumeName: "pv-missing"},
+	}
+	reconciler := newTestRestoreJobReconciler(t, pvc, restoreJob, backup)
+
+	ctx := context.Background()
+	if _, _, err := reconciler.prepareForRestore(ctx, restoreJob, backup, ur, target, opts); err != nil {
+		t.Fatalf("prepareForRestore() error = %v", err)
+	}
+
+	if err := reconciler.Get(ctx, client.ObjectKeyFromObject(pvc), &corev1.PersistentVolumeClaim{}); err != nil {
+		t.Errorf("original PVC should survive a failed rename when keepOriginalPVC=true, got error: %v", err)
+	}
+}
+
+// A Backup artifact can outlive its Velero Backup, which Velero garbage-collects
+// at TTL. The restore must fail before it halts the VM or deletes its disks.
+func TestReconcileVeleroRestore_UnusableVeleroBackupTouchesNothing(t *testing.T) {
+	tests := []struct {
+		name         string
+		veleroBackup *velerov1.Backup
+		wantMessage  string
+	}{
+		{
+			name:        "Velero Backup expired",
+			wantMessage: "Velero Backup cozy-velero/vb not found",
+		},
+		{
+			name: "Velero Backup being deleted",
+			veleroBackup: &velerov1.Backup{
+				ObjectMeta: metav1.ObjectMeta{Name: "vb", Namespace: veleroNamespace},
+				Status:     velerov1.BackupStatus{Phase: velerov1.BackupPhaseDeleting},
+			},
+			wantMessage: `Velero Backup cozy-velero/vb is "Deleting"`,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ns := "tenant-root"
+			restoreJob, backup, ur, _, opts := inPlaceVMRestoreWithoutKeep(ns)
+			rawOpts, err := json.Marshal(opts)
+			if err != nil {
+				t.Fatalf("marshal options: %v", err)
+			}
+			started := metav1.Now()
+			restoreJob.Spec.Options = &runtime.RawExtension{Raw: rawOpts}
+			restoreJob.Status = backupsv1alpha1.RestoreJobStatus{
+				Phase:     backupsv1alpha1.RestoreJobPhaseRunning,
+				StartedAt: &started,
+			}
+			backup.Spec.StrategyRef = corev1.TypedLocalObjectReference{Kind: "Velero", Name: "cozy-default-velero"}
+			backup.Spec.DriverMetadata = map[string]string{veleroBackupNameMetadataKey: "vb"}
+			backup.Status.UnderlyingResources = ur
+
+			pvc := &corev1.PersistentVolumeClaim{
+				ObjectMeta: metav1.ObjectMeta{Name: "vm-disk-ubuntu-source", Namespace: ns},
+			}
+			strategy := &strategyv1alpha1.Velero{ObjectMeta: metav1.ObjectMeta{Name: "cozy-default-velero"}}
+			objs := []client.Object{restoreJob, backup, pvc, strategy}
+			if tt.veleroBackup != nil {
+				objs = append(objs, tt.veleroBackup)
+			}
+
+			vm := &unstructured.Unstructured{}
+			vm.SetAPIVersion("kubevirt.io/v1")
+			vm.SetKind("VirtualMachine")
+			vm.SetName(vmNamePrefix + "test-vm")
+			vm.SetNamespace(ns)
+			if err := unstructured.SetNestedField(vm.Object, "Always", "spec", "runStrategy"); err != nil {
+				t.Fatalf("set runStrategy: %v", err)
+			}
+			reconciler := newTestRestoreJobReconcilerWithDynamic(t,
+				[]runtime.Object{vm, makeUnstructuredDataVolume("vm-disk-ubuntu-source", ns)})
+			testScheme := runtime.NewScheme()
+			_ = scheme.AddToScheme(testScheme)
+			_ = backupsv1alpha1.AddToScheme(testScheme)
+			_ = velerov1.AddToScheme(testScheme)
+			_ = strategyv1alpha1.AddToScheme(testScheme)
+			reconciler.Client = clientfake.NewClientBuilder().
+				WithScheme(testScheme).
+				WithObjects(objs...).
+				WithStatusSubresource(&backupsv1alpha1.RestoreJob{}).
+				Build()
+			ctx := context.Background()
+
+			if _, err := reconciler.reconcileVeleroRestore(ctx, restoreJob, backup); err != nil {
+				t.Fatalf("reconcileVeleroRestore() error = %v", err)
+			}
+
+			got := &backupsv1alpha1.RestoreJob{}
+			if err := reconciler.Get(ctx, client.ObjectKeyFromObject(restoreJob), got); err != nil {
+				t.Fatalf("get RestoreJob: %v", err)
+			}
+			if got.Status.Phase != backupsv1alpha1.RestoreJobPhaseFailed {
+				t.Fatalf("phase = %q, want %q", got.Status.Phase, backupsv1alpha1.RestoreJobPhaseFailed)
+			}
+			ready := meta.FindStatusCondition(got.Status.Conditions, "Ready")
+			if ready == nil || !strings.Contains(ready.Message, tt.wantMessage) {
+				t.Errorf("Ready condition = %+v, want a message containing %q", ready, tt.wantMessage)
+			}
+
+			if err := reconciler.Get(ctx, client.ObjectKeyFromObject(pvc), &corev1.PersistentVolumeClaim{}); err != nil {
+				t.Errorf("original PVC should survive a restore that cannot run: %v", err)
+			}
+			if _, err := reconciler.Resource(dataVolumeGVR).Namespace(ns).Get(ctx, "vm-disk-ubuntu-source", metav1.GetOptions{}); err != nil {
+				t.Errorf("DataVolume should survive a restore that cannot run: %v", err)
+			}
+			gotVM, err := reconciler.Resource(virtualMachineGVR).Namespace(ns).Get(ctx, vm.GetName(), metav1.GetOptions{})
+			if err != nil {
+				t.Fatalf("get VirtualMachine: %v", err)
+			}
+			if rs, _, _ := unstructured.NestedString(gotVM.Object, "spec", "runStrategy"); rs != "Always" {
+				t.Errorf("VirtualMachine runStrategy = %q, want it left at %q", rs, "Always")
+			}
+		})
+	}
+}
+
 func TestResolveRestoreTarget_VMDisk_CommonRestoreOptions(t *testing.T) {
 	tests := []struct {
 		name           string

packages/system/backupstrategy-controller/templates/rbac.yaml

@@ -49,10 +49,12 @@ rules:
 - apiGroups: ["kubevirt.io"]
   resources: ["virtualmachineinstances"]
   verbs: ["get"]
-# CDI DataVolumes: pre-restore deletes DVs so CDI doesn't recreate PVCs after rename
+# CDI DataVolumes: pre-restore deletes DVs so CDI doesn't recreate PVCs after rename,
+# and reads each one back to hold the restore until it is gone (a point Get
+# through the dynamic client, so no list/watch)
 - apiGroups: ["cdi.kubevirt.io"]
   resources: ["datavolumes"]
-  verbs: ["delete"]
+  verbs: ["get", "delete"]
 # HelmReleases: pre-restore suspends HRs; post-restore rename creates new HR
 # and deletes old. patch additionally lets the DefaultObjectsGate stamp
 # reconcile.fluxcd.io/forceAt + requestedAt on THIS chart's own release to

packages/system/backupstrategy-controller/tests/restore_rbac_test.yaml

@@ -0,0 +1,23 @@
+suite: RBAC of the Velero restore preparation
+
+# prepareForRestore reads every DataVolume back after deleting it and holds
+# the Velero Restore until it is gone. The read goes through the dynamic
+# client, which the Go unit tests run with no authorization, so a missing verb
+# only shows up on a cluster, where it fails every in-place VM restore.
+
+templates:
+  - templates/rbac.yaml
+
+release:
+  name: backupstrategy-controller
+  namespace: cozy-backup-controller
+
+tests:
+  - it: "ClusterRole can read and delete DataVolumes"
+    asserts:
+      - contains:
+          path: rules
+          content:
+            apiGroups: ["cdi.kubevirt.io"]
+            resources: ["datavolumes"]
+            verbs: ["get", "delete"]