Skip to content

Commit

Permalink
Modify the Non Admin Backup API deletion to align with sync controller
Browse files Browse the repository at this point in the history 
Non Admin Backup delete event should remove corresponding Velero Backup

This allows Velero Sync controller to recreate Velero Backup object and
cascate such recreation back to the Non Admin Backup. The cascade action
is a responsibility of an Non Admin Sync controller.

Signed-off-by: Michal Pryc <[email protected]>
  • Loading branch information
@mpryc
mpryc committed Jan 23, 2025
1 parent 019bb2a commit 9ee0f0f
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 16 deletions.
17 changes: 10 additions & 7 deletions docs/design/nab_and_nar_status_update.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,10 +260,13 @@ 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"]
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]
deleteVeleroBackupObjects --> removeApiDeleteFinalizer[Remove NAB Finalizer]
checkVeleroBackupObjects -->|Don't Exist| removeApiDeleteFinalizer
removeApiDeleteFinalizer --> endDelete["End API Delete"]
%% Force Delete Path
FORCE_DELETE --> setDeletingPhase[NAB Phase: **Deleting**]
Expand Down Expand Up @@ -327,12 +330,12 @@ 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 SWITCH,initNabCreate,validateSpec,setFinalizer,createVB,checkVeleroBackup,validateDelete,checkDeletionTimestamp,checkDeletionTimestampDelete,checkRequeueFlagDelete,checkVeleroObjects,checkRequeueFlag,checkStatusChanged,checkStatusChangedDelete,checkDeleteBackupRequest,checkVeleroBackupObjects decision
class start,CREATE_UPDATE,NAB_API_DELETE,FORCE_DELETE,DELETE_BACKUP,generateNACUUID,createNewVB,removeBackup,initiateForceDelete,deleteVeleroObjects,deleteVeleroBackupObjects,initiateDelete process
class terminalError,endCreateUpdate,endDelete,endForce,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
Expand Down
48 changes: 39 additions & 9 deletions internal/controller/nonadminbackup_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,15 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque
switch {
case nab.Spec.ForceDeleteBackup:
// Force delete path - immediately removes both VeleroBackup and DeleteBackupRequest
// 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 force delete path")
reconcileSteps = []nonAdminBackupReconcileStepFunction{
r.setStatusAndConditionForDeletionAndCallDelete,
r.deleteVeleroBackupAndDeleteBackupRequestObjects,
r.deleteVeleroBackupObjects,
r.deleteDeleteBackupRequestObjects,
r.removeNabFinalizerUponVeleroBackupDeletion,
}

Expand All @@ -114,11 +119,15 @@ 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.removeNabFinalizerUponVeleroBackupDeletion,
}

default:
Expand Down Expand Up @@ -211,7 +220,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 {
Expand Down Expand Up @@ -320,8 +329,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
Expand All @@ -331,9 +340,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
}
Expand All @@ -358,6 +367,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
Expand Down
44 changes: 44 additions & 0 deletions internal/controller/nonadminbackup_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ type nonAdminBackupSingleReconcileScenario struct {
uuidCreatedByReconcile bool
uuidFromTestCase bool
nonAdminBackupExpectedDeleted bool
veleroBackupExpectedDeleted bool
addNabDeletionTimestamp bool
}

Expand Down Expand Up @@ -365,6 +366,17 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
)
gomega.Expect(errors.IsNotFound(err)).To(gomega.BeTrue())
}

veleroBackup := &velerov1.Backup{}
veleroBackupErr := k8sClient.Get(ctx, types.NamespacedName{
Name: veleroBackupNACUUID,
Namespace: oadpNamespace,
}, veleroBackup)
if scenario.veleroBackupExpectedDeleted && scenario.createVeleroBackup {
gomega.Expect(errors.IsNotFound(veleroBackupErr)).To(gomega.BeTrue(), "Expected VeleroBackup to be deleted")
} else if scenario.createVeleroBackup {
gomega.Expect(veleroBackupErr).To(gomega.Not(gomega.HaveOccurred()))
}
// 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()))
Expand Down Expand Up @@ -637,8 +649,40 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
createVeleroBackup: true,
uuidFromTestCase: true,
nonAdminBackupExpectedDeleted: true,
veleroBackupExpectedDeleted: true,
result: reconcile.Result{Requeue: false},
}),
ginkgo.Entry("When triggered by NonAdminBackup delete event, should delete associated Velero Backup and NonAdminBackup objects", nonAdminBackupSingleReconcileScenario{
createVeleroBackup: true,
veleroBackupExpectedDeleted: true,
nonAdminBackupExpectedDeleted: true,
addFinalizer: true,
addNabDeletionTimestamp: true,
nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{
BackupSpec: &velerov1.BackupSpec{},
},
nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{
Phase: nacv1alpha1.NonAdminPhaseCreated,
Conditions: []metav1.Condition{
{
Type: string(nacv1alpha1.NonAdminConditionAccepted),
Status: metav1.ConditionTrue,
Reason: "BackupAccepted",
Message: "backup accepted",
LastTransitionTime: metav1.NewTime(time.Now()),
},
{
Type: string(nacv1alpha1.NonAdminConditionQueued),
Status: metav1.ConditionTrue,
Reason: "BackupScheduled",
Message: "Created Velero Backup object",
LastTransitionTime: metav1.NewTime(time.Now()),
},
},
},
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{
BackupSpec: &velerov1.BackupSpec{},
Expand Down

0 comments on commit 9ee0f0f

Please sign in to comment.