Modify the Non Admin Backup API deletion to align with sync controller (

#148)

Signed-off-by: Michal Pryc <[email protected]>

api/v1alpha1/nonadminbackup_types.go

@@ -30,11 +30,6 @@ type NonAdminBackupSpec struct {
 	// as well as the corresponding object storage
 	// +optional
 	DeleteBackup bool `json:"deleteBackup,omitempty"`
-
-	// ForceDeleteBackup removes the NonAdminBackup and its associated VeleroBackup from the cluster,
-	// regardless of whether deletion from object storage succeeds or fails
-	// +optional
-	ForceDeleteBackup bool `json:"forceDeleteBackup,omitempty"`
 }
 
 // VeleroBackup contains information of the related Velero backup object.

config/crd/bases/oadp.openshift.io_nonadminbackups.yaml

@@ -526,11 +526,6 @@ spec:
                   DeleteBackup removes the NonAdminBackup and its associated VeleroBackup from the cluster,
                   as well as the corresponding object storage
                 type: boolean
-              forceDeleteBackup:
-                description: |-
-                  ForceDeleteBackup removes the NonAdminBackup and its associated VeleroBackup from the cluster,
-                  regardless of whether deletion from object storage succeeds or fails
-                type: boolean
             required:
             - backupSpec
             type: object

docs/design/nab_and_nar_status_update.md

@@ -227,7 +227,6 @@ flowchart TD
     %% Paths with Consistent Labels
     SWITCH -->|**API Create/Update Event**| CREATE_UPDATE[**Process Create/Update**]
     SWITCH -->|**API Delete Event**| NAB_API_DELETE[**Process API Delete**]
-    SWITCH -->|**Force Delete Spec: true**| FORCE_DELETE[**Process Force Delete**]
     SWITCH -->|**Delete Spec: true**| DELETE_BACKUP[**Process Delete Request**]
 
     %% Create/Update Path - Detailed Version
@@ -260,28 +259,19 @@ flowchart TD
     NAB_API_DELETE --> setPhase["NAB Phase: **Deleting**"]
     setPhase --> setCondition["NAB Condition: Deleting=True
     Reason: DeletionPending
-    Message: backup deletion requires setting
-    spec.deleteBackup or spec.forceDeleteBackup
-    to true or finalizer removal"]
-    setCondition -->|Update Status if Changed<br>▶ Continue ║No Requeue║| endDelete["End"]
-
-    %% Force Delete Path
-    FORCE_DELETE --> setDeletingPhase[NAB Phase: **Deleting**]
-    setDeletingPhase --> setDeletingCondition[NAB Condition:: Deleting=True<br>Reason: DeletionPending<br>Message: backup accepted for deletion]
-    setDeletingCondition --> checkStatusChanged{NAB Status<br>requires update?}
-    checkStatusChanged -->|No| checkDeletionTimestamp{NAB DeletionTimestamp<br>Is Zero?}
-    checkStatusChanged -->|Yes| updateAndRequeue[Update NAB Status<br>║↻ Set To Requeue║]
-    updateAndRequeue --> checkDeletionTimestamp
-    checkDeletionTimestamp -->|No| checkRequeueFlag
-    checkDeletionTimestamp -->|Yes| initiateForceDelete[Set Delete on NAB Object]
-    initiateForceDelete --> requeueAfterForceDelete[║↻ Set To Requeue]
-    checkVeleroObjects -->|Don't Exist| removeFinalizer
-    requeueAfterForceDelete --> checkRequeueFlag{Is Set <br>To Requeue?}
-    checkRequeueFlag --> |No| checkVeleroObjects
-    checkRequeueFlag --> |Yes| requeueForceDeletePath[║↻ Requeue║]
-    checkVeleroObjects{Check VeleroBackup<br> or DeleteBackupRequest} -->|Exist| deleteVeleroObjects[Delete VeleroBackup and<br>DeleteBackupRequest Objects]
-    deleteVeleroObjects --> removeFinalizer[Remove NAB Finalizer]
-    removeFinalizer --> endForce[End Force Delete]
+    Message: permanent backup deletion requires
+    setting spec.deleteBackup to true"]
+    setCondition -->|Update Status if Changed<br>▶ Continue ║No Requeue║| checkVeleroBackupObjects
+    checkVeleroBackupObjects{Check VeleroBackup} -->|Exist| deleteVeleroBackupObjects[Delete VeleroBackup Objects]
+    checkVeleroBackupObjects -->|Don't Exist| checkVeleroDeleteBackupRequestObjects
+    deleteVeleroBackupObjects --> checkVeleroDeleteBackupRequestObjects{Check Velero DeleteBackupRequest}
+    checkVeleroDeleteBackupRequestObjects -->|Exist| deleteVeleroDeleteBackupRequestObjects[Delete DeleteBackupRequest Objects]
+    checkVeleroDeleteBackupRequestObjects -->|Don't Exist| removeApiDeleteFinalizer
+    deleteVeleroDeleteBackupRequestObjects --> waitForVBApiDeletion{Check if VeleroBackup<br>still exists?}
+    waitForVBApiDeletion -->|No| removeApiDeleteFinalizer[Remove NAB Finalizer]
+    waitForVBApiDeletion -->|Yes| requeueForVBApiDeletion[║↻ Requeue║]
+    removeApiDeleteFinalizer --> endDelete["End API Delete"]
+
 
     %% Delete Backup Path
     DELETE_BACKUP --> setDeletingPhaseDelete[NAB Phase: **Deleting**]
@@ -304,12 +294,12 @@ flowchart TD
     checkDeleteBackupRequest -->|Exists| updateDBRStatus[Update NAB Status from<br>DeleteBackupRequest Info]
     createDeleteBackupRequest --> updateDBRStatus[Update NAB Status from<br>DeleteBackupRequest Info]
     updateDBRStatus --> |Update Status if Changed<br>▶ Continue ║No Requeue║| waitForVBDeletion{Check if VeleroBackup<br>still exists?}
-    waitForVBDeletion -->|No| removeNABFinalizer[Remove NAB Finalizer]
     waitForVBDeletion -->|Yes| requeueForVBDeletion[║↻ Requeue║]
+    waitForVBDeletion -->|No| removeNABFinalizer[Remove NAB Finalizer]
     removeNABFinalizer --> endDeleteBackup
 
     %% Apply additional styles
-    class checkFinalizer,checkVeleroBackupInfo,waitForVBDeletion decision
+    class checkFinalizer,checkVeleroBackupInfo,waitForVBDeletion,waitForVBApiDeletion decision
     class createDeleteBackupRequest,removeNABFinalizer process
     class updateDBRStatus update
     class endDeleteBackup endpoint
@@ -327,15 +317,14 @@ flowchart TD
     classDef waitState fill:#ccccff,stroke:#333,stroke-width:2px
 
     %% Apply styles to all nodes
-    class SWITCH,initNabCreate,validateSpec,setFinalizer,createVB,checkVeleroBackup,validateDelete,checkDeletionTimestamp,checkDeletionTimestampDelete,checkRequeueFlagDelete,checkVeleroObjects,checkRequeueFlag,checkStatusChanged,checkStatusChangedDelete,checkDeleteBackupRequest decision
-    class start,CREATE_UPDATE,NAB_API_DELETE,FORCE_DELETE,DELETE_BACKUP,generateNACUUID,createNewVB,removeBackup,initiateForceDelete,deleteVeleroObjects,initiateDelete process
-    class terminalError,endCreateUpdate,endDelete,endForce,endDeleteBackup endpoint
+    class SWITCH,initNabCreate,validateSpec,setFinalizer,createVB,checkVeleroBackup,validateDelete,checkDeletionTimestamp,checkDeletionTimestampDelete,checkRequeueFlagDelete,checkVeleroObjects,checkRequeueFlag,checkStatusChanged,checkStatusChangedDelete,checkDeleteBackupRequest,checkVeleroBackupObjects,checkVeleroDeleteBackupRequestObjects decision
+    class start,CREATE_UPDATE,NAB_API_DELETE,DELETE_BACKUP,generateNACUUID,createNewVB,removeBackup,deleteVeleroObjects,deleteVeleroBackupObjects,deleteVeleroDeleteBackupRequestObjects,initiateDelete process
+    class terminalError,endCreateUpdate,endDelete,endDeleteBackup endpoint
     class setNewPhase,setBackingOffPhase,setCreatedPhase,setPhase,setDeletePhase,setDeletionPhase,setDeletingPhase,setDeletingPhaseDelete phase
     class setInitialCondition,setInvalidCondition,setAcceptedCondition,setQueuedCondition,setCondition,setDeletingCondition,setDeletingConditionDelete condition
-    class updateFromVB,updateNABStatus,addFinalizer,removeFinalizer update
+    class updateFromVB,updateNABStatus,addFinalizer,removeFinalizer,removeApiDeleteFinalizer update
     class refetchNAB refetch
     class setDeleteStatus,createDBR,updateStatus deleteProcess
     class waitForDeletion waitState
-
 ```
 

internal/controller/nonadminbackup_controller.go

@@ -94,15 +94,6 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 
 	// First switch statement takes precedence over the next one
 	switch {
-	case nab.Spec.ForceDeleteBackup:
-		// Force delete path - immediately removes both VeleroBackup and DeleteBackupRequest
-		logger.V(1).Info("Executing force delete path")
-		reconcileSteps = []nonAdminBackupReconcileStepFunction{
-			r.setStatusAndConditionForDeletionAndCallDelete,
-			r.deleteVeleroBackupAndDeleteBackupRequestObjects,
-			r.removeNabFinalizerUponVeleroBackupDeletion,
-		}
-
 	case nab.Spec.DeleteBackup:
 		// Standard delete path - creates DeleteBackupRequest and waits for VeleroBackup deletion
 		logger.V(1).Info("Executing standard delete path")
@@ -114,11 +105,16 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 
 	case !nab.ObjectMeta.DeletionTimestamp.IsZero():
 		// Direct deletion path - sets status and condition
-		// Initializes deletion of the NonAdminBackup object without removing
-		// dependent VeleroBackup object
+		// Remove dependent VeleroBackup object
+		// Remove finalizer from the NonAdminBackup object
+		// If there was existing BSL pointing to the Backup object
+		// the Backup will be restored causing the NAB to be recreated
 		logger.V(1).Info("Executing direct deletion path")
 		reconcileSteps = []nonAdminBackupReconcileStepFunction{
 			r.setStatusForDirectKubernetesAPIDeletion,
+			r.deleteVeleroBackupObjects,
+			r.deleteDeleteBackupRequestObjects,
+			r.removeNabFinalizerUponVeleroBackupDeletion,
 		}
 
 	default:
@@ -211,7 +207,7 @@ func (r *NonAdminBackupReconciler) setStatusForDirectKubernetesAPIDeletion(ctx c
 			Type:    string(nacv1alpha1.NonAdminConditionDeleting),
 			Status:  metav1.ConditionTrue,
 			Reason:  "DeletionPending",
-			Message: "backup deletion requires setting spec.deleteBackup or spec.forceDeleteBackup to true or finalizer removal",
+			Message: "permanent backup deletion requires setting spec.deleteBackup to true",
 		},
 	)
 	if updatedPhase || updatedCondition {
@@ -254,7 +250,6 @@ func (r *NonAdminBackupReconciler) createVeleroDeleteBackupRequest(ctx context.C
 	}
 
 	// Initiate deletion of the VeleroBackup object only when the finalizer exists.
-	// For the ForceDelete we do not create DeleteBackupRequest
 	veleroBackupNACUUID := nab.Status.VeleroBackup.NACUUID
 	veleroBackup, err := function.GetVeleroBackupByLabel(ctx, r.Client, r.OADPNamespace, veleroBackupNACUUID)
 
@@ -320,8 +315,8 @@ func (r *NonAdminBackupReconciler) createVeleroDeleteBackupRequest(ctx context.C
 	return false, nil // Continue so initNabDeletion can initialize deletion of a NonAdminBackup object
 }
 
-// deleteVeleroBackupAndDeleteBackupRequestObjects deletes both the VeleroBackup and any associated
-// DeleteBackupRequest objects for a given NonAdminBackup when force deletion is requested.
+// deleteVeleroBackupObjects deletes the VeleroBackup objects
+// associated with a given NonAdminBackup
 //
 // Parameters:
 //   - ctx: Context for managing request lifetime
@@ -331,9 +326,9 @@ func (r *NonAdminBackupReconciler) createVeleroDeleteBackupRequest(ctx context.C
 // Returns:
 //   - bool: whether to requeue (always false)
 //   - error: any error encountered during deletion
-func (r *NonAdminBackupReconciler) deleteVeleroBackupAndDeleteBackupRequestObjects(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) {
-	// This function is called just after setStatusAndConditionForDeletionAndCallDelete - force delete path, which already
-	// requeued the reconciliation to get the latest NAB object. There is no need to fetch the latest NAB object here.
+func (r *NonAdminBackupReconciler) deleteVeleroBackupObjects(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) {
+	// This function is called in a workflows where requeue just happened.
+	// There is no need to fetch the latest NAB object here.
 	if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NACUUID == constant.EmptyString {
 		return false, nil
 	}
@@ -358,6 +353,27 @@ func (r *NonAdminBackupReconciler) deleteVeleroBackupAndDeleteBackupRequestObjec
 		logger.V(1).Info("VeleroBackup already deleted")
 	}
 
+	return false, nil
+}
+
+// deleteDeleteBackupRequestObjects deletes the VeleroBackup DeleteBackupRequestObjects
+// associated with a given NonAdminBackup
+//
+// Parameters:
+//   - ctx: Context for managing request lifetime
+//   - logger: Logger instance
+//   - nab: NonAdminBackup object
+//
+// Returns:
+//   - bool: whether to requeue (always false)
+//   - error: any error encountered during deletion
+func (r *NonAdminBackupReconciler) deleteDeleteBackupRequestObjects(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) {
+	// There is no need to fetch the latest NAB object here.
+	if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NACUUID == constant.EmptyString {
+		return false, nil
+	}
+
+	veleroBackupNACUUID := nab.Status.VeleroBackup.NACUUID
 	deleteBackupRequest, err := function.GetVeleroDeleteBackupRequestByLabel(ctx, r.Client, r.OADPNamespace, veleroBackupNACUUID)
 	if err != nil {
 		// Log error if multiple DeleteBackupRequest objects are found
@@ -399,7 +415,7 @@ func (r *NonAdminBackupReconciler) removeNabFinalizerUponVeleroBackupDeletion(ct
 	//      corresponding VeleroBackup object is found based on the NACUUID stored in the NAB status for which we already
 	//      have the latest NAB object (point 1 above).
 	if !nab.ObjectMeta.DeletionTimestamp.IsZero() {
-		if !nab.Spec.ForceDeleteBackup && nab.Status.VeleroBackup != nil && nab.Status.VeleroBackup.NACUUID != constant.EmptyString {
+		if nab.Status.VeleroBackup != nil && nab.Status.VeleroBackup.NACUUID != constant.EmptyString {
 			veleroBackupNACUUID := nab.Status.VeleroBackup.NACUUID
 
 			veleroBackup, err := function.GetVeleroBackupByLabel(ctx, r.Client, r.OADPNamespace, veleroBackupNACUUID)