diff --git a/api/v1alpha1/nonadminbackup_types.go b/api/v1alpha1/nonadminbackup_types.go index a2fdbe37..7f8029ee 100644 --- a/api/v1alpha1/nonadminbackup_types.go +++ b/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. diff --git a/config/crd/bases/oadp.openshift.io_nonadminbackups.yaml b/config/crd/bases/oadp.openshift.io_nonadminbackups.yaml index 8e9726ee..3dbe2fba 100644 --- a/config/crd/bases/oadp.openshift.io_nonadminbackups.yaml +++ b/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 diff --git a/docs/design/nab_and_nar_status_update.md b/docs/design/nab_and_nar_status_update.md index 179c2281..6336a99f 100644 --- a/docs/design/nab_and_nar_status_update.md +++ b/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
▶ Continue ║No Requeue║| endDelete["End"] - - %% Force Delete Path - FORCE_DELETE --> setDeletingPhase[NAB Phase: **Deleting**] - setDeletingPhase --> setDeletingCondition[NAB Condition:: Deleting=True
Reason: DeletionPending
Message: backup accepted for deletion] - setDeletingCondition --> checkStatusChanged{NAB Status
requires update?} - checkStatusChanged -->|No| checkDeletionTimestamp{NAB DeletionTimestamp
Is Zero?} - checkStatusChanged -->|Yes| updateAndRequeue[Update NAB Status
║↻ 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
To Requeue?} - checkRequeueFlag --> |No| checkVeleroObjects - checkRequeueFlag --> |Yes| requeueForceDeletePath[║↻ Requeue║] - checkVeleroObjects{Check VeleroBackup
or DeleteBackupRequest} -->|Exist| deleteVeleroObjects[Delete VeleroBackup and
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
▶ 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
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
DeleteBackupRequest Info] createDeleteBackupRequest --> updateDBRStatus[Update NAB Status from
DeleteBackupRequest Info] updateDBRStatus --> |Update Status if Changed
▶ Continue ║No Requeue║| waitForVBDeletion{Check if VeleroBackup
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 - ``` diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 8841917b..f75270d4 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/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) diff --git a/internal/controller/nonadminbackup_controller_test.go b/internal/controller/nonadminbackup_controller_test.go index b44ba3ae..b9f3f56c 100644 --- a/internal/controller/nonadminbackup_controller_test.go +++ b/internal/controller/nonadminbackup_controller_test.go @@ -307,10 +307,9 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func gomega.Expect(k8sClient.Create(ctx, veleroBackup)).To(gomega.Succeed()) } - // We allow to have deleteNonAdminBackup and forceDeleteNonAdminBackup set to true at the same time + // DeletionTimestamp is immutable and can only be set by the API server + // We need to use Delete() instead of trying to set it directly if scenario.addNabDeletionTimestamp { - // DeletionTimestamp is immutable and can only be set by the API server - // We need to use Delete() instead of trying to set it directly gomega.Expect(k8sClient.Delete(ctx, nonAdminBackupAfterCreate)).To(gomega.Succeed()) } @@ -337,6 +336,12 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func gomega.Expect(err.Error()).To(gomega.ContainSubstring(scenario.resultError.Error())) } nonAdminBackupAfterReconcile := &nacv1alpha1.NonAdminBackup{} + veleroBackup := &velerov1.Backup{} + veleroBackupErr := k8sClient.Get(ctx, types.NamespacedName{ + Name: veleroBackupNACUUID, + Namespace: oadpNamespace, + }, veleroBackup) + if !scenario.nonAdminBackupExpectedDeleted { gomega.Expect(k8sClient.Get( ctx, @@ -353,6 +358,9 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func gomega.Expect(nonAdminBackupAfterReconcile.Status.VeleroBackup.NACUUID).To(gomega.ContainSubstring(nonAdminObjectNamespace)) gomega.Expect(nonAdminBackupAfterReconcile.Status.VeleroBackup.Namespace).To(gomega.Equal(oadpNamespace)) } + if scenario.createVeleroBackup { + gomega.Expect(veleroBackupErr).To(gomega.Not(gomega.HaveOccurred())) + } } else { // Fetch the nonAdminBackup after the delete event and ensure it is deleted by checking for NotFound error err = k8sClient.Get( @@ -364,7 +372,11 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func nonAdminBackupAfterReconcile, ) gomega.Expect(errors.IsNotFound(err)).To(gomega.BeTrue()) + // No need to check for scenario.createVeleroBackup as this object was never intended to be created + // and it should always be absent in the cluster + gomega.Expect(errors.IsNotFound(veleroBackupErr)).To(gomega.BeTrue(), "Expected VeleroBackup to be deleted") } + // easy hack to test that only one update call happens per reconcile // currentResourceVersion, err := strconv.Atoi(nonAdminBackup.ResourceVersion) // gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) @@ -549,15 +561,14 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func nonAdminBackupExpectedDeleted: true, result: reconcile.Result{Requeue: true}, }), - ginkgo.Entry("When triggered by NonAdminBackup forceDeleteNonAdmin spec field with Finalizer set and NonAdminBackup phase Created without DeletionTimestamp, should trigger delete NonAdminBackup and requeue", nonAdminBackupSingleReconcileScenario{ + ginkgo.Entry("When triggered by NonAdminBackup API Delete with Finalizer set, should delete NonAdminBackup and exit", nonAdminBackupSingleReconcileScenario{ addFinalizer: true, - addNabDeletionTimestamp: false, + addNabDeletionTimestamp: true, nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ - BackupSpec: &velerov1.BackupSpec{}, - ForceDeleteBackup: true, + BackupSpec: &velerov1.BackupSpec{}, }, nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ - Phase: nacv1alpha1.NonAdminPhaseCreated, + Phase: nacv1alpha1.NonAdminPhaseDeleting, Conditions: []metav1.Condition{ { Type: string(nacv1alpha1.NonAdminConditionAccepted), @@ -573,43 +584,30 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func Message: "Created Velero Backup object", LastTransitionTime: metav1.NewTime(time.Now()), }, - }, - }, - nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ - Phase: nacv1alpha1.NonAdminPhaseDeleting, - Conditions: []metav1.Condition{ { - Type: string(nacv1alpha1.NonAdminConditionAccepted), - Status: metav1.ConditionTrue, - Reason: "BackupAccepted", - Message: "backup accepted", - }, - { - Type: string(nacv1alpha1.NonAdminConditionQueued), - Status: metav1.ConditionTrue, - Reason: "BackupScheduled", - Message: "Created Velero Backup object", - }, - { - Type: string(nacv1alpha1.NonAdminConditionDeleting), - Status: metav1.ConditionTrue, - Reason: "DeletionPending", - Message: "backup accepted for deletion", + Type: string(nacv1alpha1.NonAdminConditionDeleting), + Status: metav1.ConditionTrue, + Reason: "DeletionPending", + Message: "backup accepted for deletion", + LastTransitionTime: metav1.NewTime(time.Now()), }, }, }, - nonAdminBackupExpectedDeleted: false, - result: reconcile.Result{Requeue: true}, + createVeleroBackup: true, + uuidFromTestCase: true, + nonAdminBackupExpectedDeleted: true, + result: reconcile.Result{Requeue: false}, }), - ginkgo.Entry("When triggered by NonAdminBackup forceDeleteNonAdmin spec field with Finalizer set, should delete NonAdminBackup and exit", nonAdminBackupSingleReconcileScenario{ - addFinalizer: true, - addNabDeletionTimestamp: true, + ginkgo.Entry("When triggered by NonAdminBackup delete event, should delete associated Velero Backup and NonAdminBackup objects", nonAdminBackupSingleReconcileScenario{ + createVeleroBackup: true, + nonAdminBackupExpectedDeleted: true, + addFinalizer: true, + addNabDeletionTimestamp: true, nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ - BackupSpec: &velerov1.BackupSpec{}, - ForceDeleteBackup: true, + BackupSpec: &velerov1.BackupSpec{}, }, nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ - Phase: nacv1alpha1.NonAdminPhaseDeleting, + Phase: nacv1alpha1.NonAdminPhaseCreated, Conditions: []metav1.Condition{ { Type: string(nacv1alpha1.NonAdminConditionAccepted), @@ -625,19 +623,10 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func Message: "Created Velero Backup object", LastTransitionTime: metav1.NewTime(time.Now()), }, - { - Type: string(nacv1alpha1.NonAdminConditionDeleting), - Status: metav1.ConditionTrue, - Reason: "DeletionPending", - Message: "backup accepted for deletion", - LastTransitionTime: metav1.NewTime(time.Now()), - }, }, }, - createVeleroBackup: true, - uuidFromTestCase: true, - nonAdminBackupExpectedDeleted: true, - result: reconcile.Result{Requeue: false}, + uuidFromTestCase: true, + result: reconcile.Result{Requeue: false}, }), ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new), should update NonAdminBackup Phase to Created and Condition to Accepted True and NOT Requeue", nonAdminBackupSingleReconcileScenario{ nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{