feat(backups): no manual actions for restore VMI - #2251

internal/backupcontroller/backup_controller.go (1)

25-29: Consider removing unused Recorder field.

The Recorder field is declared but never used in this controller. The cleanupVeleroBackup method logs events but doesn't emit Kubernetes events via the recorder. Either remove the field or add event recording for cleanup operations.

🔧 Option 1: Remove unused field

 type BackupReconciler struct {
 	client.Client
 	Scheme   *runtime.Scheme
-	Recorder record.EventRecorder
 }

And update main.go accordingly.

🔧 Option 2: Add event recording for cleanup

 	if err := r.Delete(ctx, veleroBackup); err != nil {
 		if apierrors.IsNotFound(err) {
 			return nil
 		}
 		return fmt.Errorf("failed to delete Velero backup %s/%s: %w", veleroBackupNamespace, veleroBackupName, err)
 	}

+	r.Recorder.Event(backup, corev1.EventTypeNormal, "VeleroBackupDeleted",
+		fmt.Sprintf("Deleted Velero Backup %s/%s", veleroBackupNamespace, veleroBackupName))
 	logger.Info("deleted Velero backup", "name", veleroBackupName, "namespace", veleroBackupNamespace)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/backupcontroller/backup_controller.go` around lines 25 - 29, The
BackupReconciler currently declares an unused Recorder field; either remove the
field from the BackupReconciler struct and delete any related initialization in
main.go, or wire the recorder into cleanup operations by using r.Recorder to
emit Kubernetes events (e.g., r.Recorder.Eventf or r.Recorder.Event) inside
cleanupVeleroBackup and initialize it in main.go with
mgr.GetEventRecorderFor("backup-controller") when constructing BackupReconciler;
update references accordingly so there are no unused fields or missing event
recorder initialization.

cmd/backupstrategy-controller/main.go (1)

179-186: Consider using a distinct event recorder name for BackupReconciler.

The event recorder name "backup-controller" on line 182 is the same as the one used for BackupJobReconciler on line 164. While this won't cause functional issues, using a distinct name like "backup-cleanup-controller" would improve event attribution clarity when debugging.

🔧 Suggested change

 	if err = (&backupcontroller.BackupReconciler{
 		Client:   mgr.GetClient(),
 		Scheme:   mgr.GetScheme(),
-		Recorder: mgr.GetEventRecorderFor("backup-controller"),
+		Recorder: mgr.GetEventRecorderFor("backup-cleanup-controller"),
 	}).SetupWithManager(mgr); err != nil {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/backupstrategy-controller/main.go` around lines 179 - 186, The
BackupReconciler is using the same event recorder name string
"backup-controller" as BackupJobReconciler; update the recorder invocation in
the BackupReconciler setup (the mgr.GetEventRecorderFor(...) call inside the
BackupReconciler initialization used by BackupReconciler.SetupWithManager) to a
distinct name such as "backup-cleanup-controller" so events are attributed
clearly when debugging.

internal/backupcontroller/backup_controller.go (1)

117-123: IsAlreadyExists check is unreachable with GenerateName.

When using GenerateName, the API server generates a unique name for each Create request, so IsAlreadyExists will never be returned. This dead code path could mask issues if the logic changes. Consider removing it or adding a comment explaining the defensive intent.

🧹 Suggested cleanup

 	if err := r.Create(ctx, dbr); err != nil {
-		if apierrors.IsAlreadyExists(err) {
-			logger.V(1).Info("DeleteBackupRequest already exists", "backup", veleroBackupName)
-			return nil
-		}
 		return fmt.Errorf("failed to create DeleteBackupRequest for %s/%s: %w", veleroBackupNamespace, veleroBackupName, err)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/backupcontroller/backup_controller.go` around lines 117 - 123, The
IsAlreadyExists check inside the Create handling for the DeleteBackupRequest is
unreachable because the request uses GenerateName (so the API server will never
return AlreadyExists); update the Create error handling in the function that
calls r.Create for the DeleteBackupRequest to either remove the
apierrors.IsAlreadyExists branch (and just return the formatted error) or
replace it with a short comment clarifying this is defensive and why it remains;
reference the r.Create call that constructs the DeleteBackupRequest and the
apierrors.IsAlreadyExists/ logger.V(1).Info code paths when making the change.

internal/backupcontroller/velerostrategy_controller.go (1)

932-963: Consider crash recovery scenario in PVC rename sequence.

If the controller crashes after deleting the old PVC (line 933) but before creating the new PVC (line 961), the PV will be left orphaned with Retain policy and no associated PVC. On requeue, the idempotency check at line 901 would pass (new PVC doesn't exist), but the original PVC is already gone.

The current logic handles this gracefully since line 907-909 returns nil when the original PVC is not found. However, the user would lose the ability to "rename" the PVC if this edge case occurs.

Consider adding a check for an existing PV with a cleared claimRef that matches the expected pvName to improve recovery:

💡 Recovery improvement suggestion

+	// Check if PV already exists with cleared claimRef (recovery from partial rename)
+	pv := &corev1.PersistentVolume{}
+	if err := r.Get(ctx, client.ObjectKey{Name: pvName}, pv); err == nil {
+		if pv.Spec.ClaimRef == nil || pv.Spec.ClaimRef.Name == "" {
+			// PV exists and is unbound - proceed to create new PVC
+			goto createNewPVC
+		}
+	}
+
 	// Get the original PVC
 	oldPVC := &corev1.PersistentVolumeClaim{}

Alternatively, document this limitation in the function comment.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/backupcontroller/velerostrategy_controller.go` around lines 932 -
963, The delete-then-create PVC flow can leave the PV orphaned if the controller
crashes after deleting oldPVC but before creating newPVC; modify the
reconciliation around the oldPVC removal (the block handling r.Get for oldPVC
and the subsequent pv/pvName handling) so that if oldPVC is not found you
re-fetch the PV (r.Get with client.ObjectKey{Name: pvName}) and, when that PV
exists and pv.Spec.ClaimRef == nil (and optionally PV.Status.Phase indicates it
was bound), proactively recreate the new PVC (newPVC with VolumeName: pvName and
the same spec as oldPVC) or adopt the PV by setting ClaimRef appropriately
before calling r.Create/r.Update; otherwise fall back to returning nil or
document the limitation—use the existing variables (oldPVC, newPVC, pv, pvName)
and methods (r.Get, r.Create, r.Update) to implement the recovery path.

packages/system/backupstrategy-controller/templates/rbac.yaml (1)

60-63: Remove unnecessary delete verb for Velero resources.

The delete verb is granted for backups, restores, and datauploads, but the controller never directly deletes these resources. Instead, it creates DeleteBackupRequest objects (see backup_controller.go lines 74-125), which is the proper Velero mechanism. Direct deletion of Backup objects would be ineffective anyway since BSL sync recreates the object from storage. The RBAC already correctly permits deletebackuprequests management.

Suggested least-privilege adjustment

 # Velero Backup/Restore/DataUpload in cozy-velero namespace (DataUpload: enrich BackupJob failure messages)
 - apiGroups: ["velero.io"]
   resources: ["backups", "restores", "datauploads"]
-  verbs: ["create", "get", "list", "watch", "update", "patch", "delete"]
+  verbs: ["create", "get", "list", "watch", "update", "patch"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/backupstrategy-controller/templates/rbac.yaml` around lines
60 - 63, Remove the unnecessary "delete" verb from the Velero resource rules in
the RBAC template so the Role no longer grants delete on resources
["backups","restores","datauploads"]; the controller uses DeleteBackupRequest
objects (see backup_controller.go handling around DeleteBackupRequest creation)
instead, and delete privilege is already provided for deletebackuprequests, so
simply delete "delete" from the verbs array for that rule in rbac.yaml.

internal/backupcontroller/restorejob_controller.go (3)

69-75: Consider returning after adding finalizer.

After successfully adding the finalizer, the common controller-runtime pattern is to return early and let the reconciliation re-trigger with the updated object. This avoids operating on a potentially stale in-memory copy.

♻️ Suggested change

 	// Ensure finalizer is present
 	if !controllerutil.ContainsFinalizer(restoreJob, restoreJobFinalizer) {
 		controllerutil.AddFinalizer(restoreJob, restoreJobFinalizer)
 		if err := r.Update(ctx, restoreJob); err != nil {
 			return ctrl.Result{}, err
 		}
+		return ctrl.Result{}, nil
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/backupcontroller/restorejob_controller.go` around lines 69 - 75, The
block that adds the finalizer (using controllerutil.ContainsFinalizer,
controllerutil.AddFinalizer, and r.Update on restoreJob with
restoreJobFinalizer) should return immediately after a successful r.Update to
avoid continuing reconciliation on a stale in-memory object; change the flow so
that after adding the finalizer and updating the API you return (e.g., return
ctrl.Result{Requeue: true}, nil or return ctrl.Result{}, nil) so the reconcile
loop is re-triggered with the fresh object instead of proceeding.

---

56-67: Cleanup errors don't block finalizer removal, potentially orphaning Velero resources.

If cleanupVeleroRestore fails to delete Velero Restore or ConfigMap resources (e.g., transient API error), the finalizer is still removed and the RestoreJob deletion proceeds. This could leave orphaned Velero resources in the velero namespace.

Consider returning an error from cleanupVeleroRestore and blocking finalizer removal on failure, or at minimum re-queuing for a retry:

♻️ Suggested approach to handle cleanup failures

 	// Handle deletion: clean up Velero Restore
 	if !restoreJob.DeletionTimestamp.IsZero() {
 		if controllerutil.ContainsFinalizer(restoreJob, restoreJobFinalizer) {
-			r.cleanupVeleroRestore(ctx, restoreJob)
+			if err := r.cleanupVeleroRestore(ctx, restoreJob); err != nil {
+				logger.Error(err, "cleanup failed, will retry")
+				return ctrl.Result{}, err
+			}
 			controllerutil.RemoveFinalizer(restoreJob, restoreJobFinalizer)

And update cleanupVeleroRestore to return an error (see related comment below).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/backupcontroller/restorejob_controller.go` around lines 56 - 67, The
finalizer removal block currently calls r.cleanupVeleroRestore(ctx, restoreJob)
but ignores its failure; change cleanupVeleroRestore to return an error and in
the deletion path check that error and abort removal if non-nil: call err :=
r.cleanupVeleroRestore(ctx, restoreJob), if err != nil return ctrl.Result{}, err
(or requeue), only then call controllerutil.RemoveFinalizer(restoreJob,
restoreJobFinalizer) and r.Update(ctx, restoreJob); ensure
cleanupVeleroRestore's signature and all callers are updated to handle/propagate
the error.

---

156-177: Consider returning errors and consistent event recording.

Two issues with this cleanup method:

1. No error return: The method logs errors but doesn't return them, preventing the caller from deciding whether to retry or block deletion.

2. Inconsistent observability: Velero Restore deletion failure records a warning event (line 170-171), but ConfigMap deletion failure does not. Consider consistent behavior for both.

♻️ Proposed refactor to return errors and add consistent event recording

-func (r *RestoreJobReconciler) cleanupVeleroRestore(ctx context.Context, restoreJob *backupsv1alpha1.RestoreJob) {
+func (r *RestoreJobReconciler) cleanupVeleroRestore(ctx context.Context, restoreJob *backupsv1alpha1.RestoreJob) error {
 	logger := log.FromContext(ctx)
 	opts := []client.DeleteAllOfOption{
 		client.InNamespace(veleroNamespace),
 		client.MatchingLabels{
 			backupsv1alpha1.OwningJobNameLabel:      restoreJob.Name,
 			backupsv1alpha1.OwningJobNamespaceLabel: restoreJob.Namespace,
 		},
 	}
 
 	if err := r.DeleteAllOf(ctx, &velerov1.Restore{}, opts...); err != nil {
 		logger.Error(err, "failed to delete Velero Restore(s)")
 		r.Recorder.Event(restoreJob, corev1.EventTypeWarning, "CleanupFailed",
 			fmt.Sprintf("Failed to delete Velero Restore: %v", err))
+		return err
 	}
 
 	if err := r.DeleteAllOf(ctx, &corev1.ConfigMap{}, opts...); err != nil {
 		logger.Error(err, "failed to delete resourceModifiers ConfigMap(s)")
+		r.Recorder.Event(restoreJob, corev1.EventTypeWarning, "CleanupFailed",
+			fmt.Sprintf("Failed to delete resourceModifier ConfigMap: %v", err))
+		return err
 	}
+	return nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/backupcontroller/restorejob_controller.go` around lines 156 - 177,
The cleanupVeleroRestore function should return an error and consistently record
events for both deletion failures: change the signature of cleanupVeleroRestore
to (ctx context.Context, restoreJob *backupsv1alpha1.RestoreJob) error, capture
errors from both r.DeleteAllOf calls (for &velerov1.Restore{} and
&corev1.ConfigMap{}), call r.Recorder.Event with a matching warning event and
descriptive message for the ConfigMap deletion failure (same pattern used for
Velero Restore), and return a consolidated error (or the first error) to the
caller so the reconciler can decide retry/abort; update any callers to handle
the returned error accordingly and keep logger.Error calls for both failures
(use RestoreJobReconciler, cleanupVeleroRestore, r.DeleteAllOf,
r.Recorder.Event, and log.FromContext to locate places to edit).

internal/backupcontroller/velerostrategy_controller.go (1)

667-671: ⚠️ Potential issue | 🟡 Minor

ConfigMap name truncation may produce invalid names.

The truncation at exactly 253 characters could cut a name mid-character or leave a trailing hyphen, which would be invalid. Kubernetes names must also start and end with alphanumeric characters.

🔧 Suggested improvement

 	cmName := fmt.Sprintf("restore-modifiers-%s-%s", restoreJob.Namespace, restoreJob.Name)
 	// Truncate name to fit Kubernetes 253-char limit
 	if len(cmName) > 253 {
-		cmName = cmName[:253]
+		cmName = strings.TrimRight(cmName[:253], "-.")
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/backupcontroller/velerostrategy_controller.go` around lines 667 -
671, The current creation of cmName ("restore-modifiers-%s-%s") truncates by
byte length and can cut multi-byte runes or leave invalid start/end characters;
change logic in the block that builds cmName to (1) work with runes (not bytes)
when truncating to the 253-character DNS-1123 label max, (2) ensure the name
starts and ends with an alphanumeric character by trimming any leading/trailing
non-alphanumerics (remove trailing hyphens) and, if necessary, prefixing a safe
alphanumeric character, and (3) after truncation re-validate against DNS-1123
rules (lowercase letters, digits, hyphen) replacing or removing invalid
characters; apply this to the variable cmName constructed from
restoreJob.Namespace and restoreJob.Name so the final cmName is a valid
Kubernetes resource name.

internal/backupcontroller/velerostrategy_controller.go (2)

959-973: Clarify the "atomic" comment - the operation is safe but not truly atomic.

The comment says "This is atomic" but the PV rebinding involves multiple API calls. The safety comes from pre-binding the new PVC (via VolumeName) before deleting the old one, which prevents other PVCs from claiming the PV. Consider clarifying:

📝 Suggested comment clarification

-	// Point the PV's claimRef directly to the new -orig PVC.
-	// This is atomic — no window where the PV is Available for other PVCs to grab.
+	// Point the PV's claimRef directly to the new -orig PVC.
+	// Safe because the new PVC was pre-bound via VolumeName, preventing other claims.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/backupcontroller/velerostrategy_controller.go` around lines 959 -
973, Update the misleading comment above the PV rebinding sequence (around
r.Get, pv.Spec.ClaimRef assignment, and r.Update) to clarify that the operation
is not truly atomic because it uses multiple API calls; explain that safety is
achieved by pre-binding the new PVC via its VolumeName (newPVC.Spec.VolumeName)
before removing the old PVC, which prevents other PVCs from claiming the PV, and
state any remaining small race windows rather than calling it "atomic".

---

270-278: Multiple VM pods may exist during live migration.

When a VM is live migrating, there can be multiple pods with the same vm.kubevirt.io/name label. The current code picks the first pod arbitrarily. Consider selecting the pod in Running phase or with the most recent creation timestamp to get consistent network info.

♻️ Optional improvement to prefer the running pod

 	var ip, mac string
 	if len(podList.Items) > 0 {
-		pod := podList.Items[0]
+		// Prefer the running pod if multiple exist (e.g., during live migration)
+		pod := podList.Items[0]
+		for _, p := range podList.Items {
+			if p.Status.Phase == corev1.PodRunning {
+				pod = p
+				break
+			}
+		}
 		ip = pod.Annotations[ovnIPAnnotation]
 		mac = pod.Annotations[ovnMACAnnotation]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/backupcontroller/velerostrategy_controller.go` around lines 270 -
278, The code currently uses podList.Items[0] arbitrarily which can pick the
wrong pod during live migration; update the selection logic to choose the most
appropriate pod (prefer a pod whose Status.Phase == "Running", otherwise pick
the pod with the newest CreationTimestamp) before reading ovnIPAnnotation and
ovnMACAnnotation. Concretely, iterate podList.Items to find a Running pod (fall
back to comparing CreationTimestamp to pick the latest), assign that pod to the
local variable used to extract ip and mac (the same variables currently set from
pod.Annotations using ovnIPAnnotation and ovnMACAnnotation), and keep the
existing logger.Debug call to emit pod.Name and the extracted values (and still
log the "no VM pod found for OVN info" case using vmName if the list is empty).