diff --git a/internal/backupcontroller/velerostrategy_controller.go b/internal/backupcontroller/velerostrategy_controller.go index b3cd81239..65bea99fb 100644 --- a/internal/backupcontroller/velerostrategy_controller.go +++ b/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 -orig- 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 diff --git a/internal/backupcontroller/velerostrategy_controller_test.go b/internal/backupcontroller/velerostrategy_controller_test.go index e07b20ee4..d17078d4e 100644 --- a/internal/backupcontroller/velerostrategy_controller_test.go +++ b/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 diff --git a/packages/system/backupstrategy-controller/templates/rbac.yaml b/packages/system/backupstrategy-controller/templates/rbac.yaml index b61c7eb56..cb84c6309 100644 --- a/packages/system/backupstrategy-controller/templates/rbac.yaml +++ b/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 diff --git a/packages/system/backupstrategy-controller/tests/restore_rbac_test.yaml b/packages/system/backupstrategy-controller/tests/restore_rbac_test.yaml new file mode 100644 index 000000000..b13b88173 --- /dev/null +++ b/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"]