From a40217476bb4d7b877a69322e57f6301231e2cbf Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Fri, 25 Oct 2024 13:45:27 +0200 Subject: [PATCH] DNM: Fix for #58 Signed-off-by: Michal Pryc --- api/v1alpha1/nonadminbackup_types.go | 6 + api/v1alpha1/nonadmincontroller_types.go | 1 + ...nac.oadp.openshift.io_nonadminbackups.yaml | 4 + docs/design/nab_and_nar_status_update.md | 30 ++- internal/common/constant/constant.go | 1 + .../controller/nonadminbackup_controller.go | 210 +++++++++++++++++- 6 files changed, 244 insertions(+), 8 deletions(-) diff --git a/api/v1alpha1/nonadminbackup_types.go b/api/v1alpha1/nonadminbackup_types.go index 6b9fa28..d873c9b 100644 --- a/api/v1alpha1/nonadminbackup_types.go +++ b/api/v1alpha1/nonadminbackup_types.go @@ -32,6 +32,8 @@ const ( NonAdminBackupPhaseBackingOff NonAdminBackupPhase = "BackingOff" // NonAdminBackupPhaseCreated - Velero Backup was created. The Phase will not have additional informations about the Backup. NonAdminBackupPhaseCreated NonAdminBackupPhase = "Created" + // NonAdminBackupPhaseDeletion - Velero Backup is pending deletion. The Phase will not have additional informations about the Backup. + NonAdminBackupPhaseDeletion NonAdminBackupPhase = "Deletion" ) // NonAdminBackupSpec defines the desired state of NonAdminBackup @@ -43,6 +45,10 @@ type NonAdminBackupSpec struct { // +optional // +kubebuilder:validation:Enum=trace;debug;info;warning;error;fatal;panic LogLevel string `json:"logLevel,omitempty"` + + // DeleteBackup tells the controller to remove created Velero Backup. + // +optional + DeleteBackup bool `json:"deleteBackup,omitempty"` } // VeleroBackup contains information of the related Velero backup object. diff --git a/api/v1alpha1/nonadmincontroller_types.go b/api/v1alpha1/nonadmincontroller_types.go index 61c5608..1a54a95 100644 --- a/api/v1alpha1/nonadmincontroller_types.go +++ b/api/v1alpha1/nonadmincontroller_types.go @@ -27,4 +27,5 @@ type NonAdminCondition string const ( NonAdminConditionAccepted NonAdminCondition = "Accepted" NonAdminConditionQueued NonAdminCondition = "Queued" + NonAdminConditionDeletion NonAdminCondition = "Deletion" ) diff --git a/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml b/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml index c1b4390..1c64126 100644 --- a/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml +++ b/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml @@ -515,6 +515,10 @@ spec: type: string type: array type: object + deleteBackup: + description: DeleteBackup tells the controller to remove created Velero + Backup. + type: boolean logLevel: description: NonAdminBackup log level (use debug for the most logging, leave unset for default) diff --git a/docs/design/nab_and_nar_status_update.md b/docs/design/nab_and_nar_status_update.md index 0fbb98c..a8db56f 100644 --- a/docs/design/nab_and_nar_status_update.md +++ b/docs/design/nab_and_nar_status_update.md @@ -28,6 +28,7 @@ Those are are the possible values for phase: | New | *NonAdminBackup/NonAdminRestore* resource was accepted by the NAB/NAR Controller, but it has not yet been validated by the NAB/NAR Controller | | BackingOff | *NonAdminBackup/NonAdminRestore* resource was invalidated by the NAB/NAR Controller, due to invalid Spec. NAB/NAR Controller will not reconcile the object further, until user updates it | | Created | *NonAdminBackup/NonAdminRestore* resource was validated by the NAB/NAR Controller and Velero *Backup/restore* was created. The Phase will not have additional information about the *Backup/Restore* run | +| Deletion | *NonAdminBackup/NonAdminRestore* resource has been marked for deletion. The NAB/NAR Controller will delete the corresponding Velero *Backup/Restore* if it exists. Once this deletion completes, the *NonAdminBackup/NonAdminRestore* object itself will also be removed | ### Conditions @@ -123,7 +124,7 @@ It is similar for a NonAdminRestore. ```mermaid %%{init: {'theme':'neutral'}}%% -graph +flowchart TD predicateUpdateNabEvent{{predicate: Accepted **update** event NAB}}; @@ -137,6 +138,8 @@ requeueFalseTerminalErr[\"Requeue: false, Terminal error"/]; reconcileStartAcceptedPredicate[/Reconcile start\]; +questionIsMarkedForDeletion{"Is NAB marked for deletion ?"}; +questionIsFinalizerForDeletionSet{"Is NonAdminBackup finalizer set ?"}; questionPhaseStatusSetToNew{"Is status.phase: **New** ?"}; questionConditionAcceptedTrue{"Is status.conditions[Accepted]: **True** ?"}; questionIsNabValid{"Is NonAdminBackup Spec valid ?"}; @@ -152,13 +155,17 @@ questionSuccessWithNoRequeue{"Success ?"}; questionSuccessGetVB{"Error ?"}; questionGetSingleVB{"Is Single Velero Backup found ?"}; questionNabStatusUpdateFromVB{"Does NAB Object status requires update ?"}; +questionPhaseStatusSetToDeletion{"Is status.phase: **Deletion** ?"}; +questionDoesVeleroObjectExists("Does Velero Object with corresponding status.VeleroBackup.NameUUID exist ?") createVBObject{{"Create New Velero Backup Object"}}; - +deleteVeleroObject{{"Delete Velero Backup Object"}}; +deleteNonAdminBackupObject{{"Delete Non Admin Backup Object"}} getUUIDMatchingVeleroBackup("Get Velero Backup with label openshift.io/oadp-nab-origin-nameuuid matching status.VeleroBackup.NameUUID"); statusPhaseSetToNew["Set status.phase to: **New**"]; +statusPhaseSetToDeletion["Set status.phase to: **Deletion**"]; statusPhaseStatusSetToBackingOff["Set status.phase: **BackingOff** and status.conditions[Accepted]: **False** ?"]; statusConditionSetAcceptedToTrue["Set status.conditions[Accepted] to **True**"]; @@ -166,12 +173,29 @@ statusPhaseStatusSetToCreated["Set status.phase: **Created** and status.conditions[BackupScheduled]: **True** ?"]; statusSetVeleroBackupUUID["Generate a NameUUID and set it as status.VeleroBackup.NameUUID"]; statusNabStatusUpdateFromVB["Update NonAdminBackup status from Velero Backup"]; +setFinalizerOnNab["Set finalizer on NonAdminBackup"] predicateCreateNabEvent --> reconcileStartAcceptedPredicate; predicateUpdateNabEvent --> reconcileStartAcceptedPredicate; predicateUpdateVBEvent --> reconcileStartAcceptedPredicate; -reconcileStartAcceptedPredicate --> questionPhaseStatusSetToNew; +reconcileStartAcceptedPredicate --> questionIsMarkedForDeletion; +questionIsMarkedForDeletion -- Yes --> questionIsFinalizerForDeletionSet; +questionIsFinalizerForDeletionSet -- No --> setFinalizerOnNab; +setFinalizerOnNab --> questionSuccess; + +questionIsFinalizerForDeletionSet -- Yes --> questionPhaseStatusSetToDeletion; + +questionPhaseStatusSetToDeletion -- No --> statusPhaseSetToDeletion; +statusPhaseSetToDeletion --> questionSuccess; +questionPhaseStatusSetToDeletion -- Yes --> questionDoesVeleroObjectExists; +questionDoesVeleroObjectExists -- No --> deleteNonAdminBackupObject; +questionDoesVeleroObjectExists -- Yes --> deleteVeleroObject; +deleteVeleroObject --> questionSuccess; +deleteNonAdminBackupObject --> questionSuccessWithNoRequeue; + +questionIsMarkedForDeletion -- No --> questionPhaseStatusSetToNew; + questionPhaseStatusSetToNew -- No --> statusPhaseSetToNew; questionPhaseStatusSetToNew -- Yes --> questionIsNabValid; diff --git a/internal/common/constant/constant.go b/internal/common/constant/constant.go index ed9dfc6..d3e08e9 100644 --- a/internal/common/constant/constant.go +++ b/internal/common/constant/constant.go @@ -32,6 +32,7 @@ const ( NabOriginNamespaceAnnotation = "openshift.io/oadp-nab-origin-namespace" NabOriginNameUUIDLabel = "openshift.io/oadp-nab-origin-nameuuid" NarOriginNameUUIDLabel = "openshift.io/oadp-nar-origin-nameuuid" + NabFinalizerName = "nonadminbackup.oadp.openshift.io/finalizer" ) // Common environment variables for the Non Admin Controller diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 104cc06..e96cd09 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -56,6 +57,9 @@ const ( veleroReferenceUpdateRequeue = "NonAdminBackup - Requeue after Status Update with UUID reference" statusUpdateExit = "NonAdminBackup - Exit after Status Update" statusUpdateError = "Failed to update NonAdminBackup Status" + findSingleVBError = "Failed to find single VeleroBackup object" + uuidString = "UUID" + nameString = "name" ) // +kubebuilder:rbac:groups=nac.oadp.openshift.io,resources=nonadminbackups,verbs=get;list;watch;create;update;patch;delete @@ -84,10 +88,14 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque } reconcileSteps := []reconcileStepFunction{ - r.init, + r.setFinalizerForDeletion, + r.initVeleroBackupDeletion, + r.initNabDeletion, + r.initNabCreate, r.validateSpec, r.setBackupUUIDInStatus, r.createVeleroBackupAndSyncWithNonAdminBackup, + r.finalizeNabDeletion, } for _, step := range reconcileSteps { requeue, err := step(ctx, logger, nab) @@ -101,7 +109,179 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, nil } -// init initializes the Status.Phase from the NonAdminBackup. +// setFinalizerForDeletion ensures that the finalizer is set on the NonAdminBackup object +// if it is marked for deletion by the `deleteBackup` spec field. +// +// Parameters: +// - ctx: Context for managing request lifetime. +// - logger: Logger instance for logging messages. +// - nab: Pointer to the NonAdminBackup object to be updated. +// +// This function checks if the NonAdminBackup object’s `deleteBackup` field is set to true. +// If so, it verifies whether the finalizer is already present. If not, the finalizer is added +// and the function requeues the reconciliation to avoid continuing until the finalizer is set. +// The function also updates the status phase to `Deletion` if necessary and initiates deletion +// of the corresponding VeleroBackup object. It returns a boolean indicating whether to requeue +// and an error if the status update or VeleroBackup deletion fails. +func (r *NonAdminBackupReconciler) setFinalizerForDeletion(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + // Check if the object is marked for deletion by checking the deleteBackup spec + if !nab.Spec.DeleteBackup { + return false, nil + } + + // If the object does not have the finalizer, add it + if !controllerutil.ContainsFinalizer(nab, constant.NabFinalizerName) { + controllerutil.AddFinalizer(nab, constant.NabFinalizerName) + if err := r.Update(ctx, nab); err != nil { + logger.Error(err, "Failed to add finalizer") + return false, err + } + logger.V(1).Info("Finalizer added to NonAdminBackup", "finalizer", constant.NabFinalizerName) + return true, nil // Requeue after adding finalizer + } + + // Update Phase if needed, outside finalizer block to avoid excessive updates + updated := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseDeletion) + if updated { + if err := r.Status().Update(ctx, nab); err != nil { + logger.Error(err, statusUpdateError) + return false, err + } + logger.V(1).Info(phaseUpdateRequeue) + return true, nil // Requeue after status update + } + + logger.V(1).Info("NonAdminBackup marked for deletetion") + return false, nil // Continue so initVeleroBackupDeletion can delete VeleroBackup object +} + +// initVeleroBackupDeletion initiates the deletion of the associated VeleroBackup +// if the NonAdminBackup (NAB) object is marked for deletion and contains the finalizer. +// +// Parameters: +// - ctx: Context to manage request lifetime. +// - logger: Logger instance for logging messages. +// - nab: Pointer to the NonAdminBackup object to be managed. +// +// This function checks if the `DeleteBackup` spec field is set to true and verifies the +// presence of the finalizer on the NAB object. If these conditions are met, it attempts +// to locate the associated VeleroBackup object based on the UUID stored in NAB's status. +// If a single VeleroBackup object is found, it initiates deletion of that VeleroBackup +// and requeues the reconcile loop. If VeleroBackup deletion fails or multiple objects +// are found, it logs the error and returns for further handling. +// +// Returns a boolean indicating whether to requeue and an error if deletion or retrieval +// of the VeleroBackup fails. +func (r *NonAdminBackupReconciler) initVeleroBackupDeletion(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + // Check if DeleteBackup is set and finalizer is present + if !nab.Spec.DeleteBackup || !controllerutil.ContainsFinalizer(nab, constant.NabFinalizerName) { + return false, nil + } + + // Initiate VeleroBackup deletion + veleroBackupNameUUID := nab.Status.VeleroBackup.NameUUID + veleroBackup, err := function.GetVeleroBackupByLabel(ctx, r.Client, r.OADPNamespace, veleroBackupNameUUID) + + if err != nil { + // Case in which more then one VeleroBackup is found with the same label UUID + // TODO: Should we delete all of the objects with such UUID ? + logger.Error(err, findSingleVBError, uuidString, veleroBackupNameUUID) + return false, err + } + + if veleroBackup != nil { + if err := r.Delete(ctx, veleroBackup); err != nil { + logger.Error(err, "Failed to delete VeleroBackup", nameString, veleroBackupNameUUID) + return false, err + } + logger.V(1).Info("VeleroBackup deletion initiated", nameString, veleroBackupNameUUID) + return true, nil // Requeue after deletion of VeleroBackup was initiated + } + + logger.V(1).Info("VeleroBackup already deleted") + return false, nil // Continue so initNabDeletion can initialize deletion of an NonAdminBackup object +} + +// initNabDeletion initiates the deletion of the NonAdminBackup (NAB) object if +// it is marked for deletion and has the required finalizer. +// +// Parameters: +// - ctx: Context for managing request lifetime. +// - logger: Logger instance for logging messages. +// - nab: Pointer to the NonAdminBackup object to be deleted. +// +// This function checks if the `DeleteBackup` spec field is set to true and if +// the NAB object contains the finalizer. If both conditions are met, and the +// deletion has not previously been initiated, it marks the NAB object for deletion. +// It returns a boolean indicating whether to requeue and an error if any deletion operation fails. +func (r *NonAdminBackupReconciler) initNabDeletion(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + // Check if DeleteBackup is set and finalizer is present + if !nab.Spec.DeleteBackup || !controllerutil.ContainsFinalizer(nab, constant.NabFinalizerName) { + return false, nil + } + + if nab.ObjectMeta.DeletionTimestamp.IsZero() { + logger.V(1).Info("Marking NonAdminBackup for deletion", nameString, nab.Name) + if err := r.Delete(ctx, nab); err != nil { + logger.Error(err, "Failed to mark NonAdminBackup for deletion") + return false, err + } + return true, nil // Requeue to allow deletion to proceed + } + + logger.V(1).Info("NonAdminBackup already marked for deletion") + return false, nil // Continue so finalizeNabDeletion can delete NonAdminBackup object +} + +// finalizeNabDeletion handles the deletion of the NonAdminBackup (NAB) object by ensuring +// the associated VeleroBackup is deleted and the NAB finalizer is removed once cleanup is complete. +// +// Parameters: +// - ctx: Context for managing request lifetime. +// - logger: Logger instance for logging messages. +// - nab: Pointer to the NonAdminBackup object to be cleaned up. +// +// The function first checks if the `deleteBackup` spec field is set to true and if the NAB object +// contains the finalizer. If these conditions are met, it attempts to locate the VeleroBackup +// object associated with the NAB and if it is found it ensures it's deleted. +// Once the VeleroBackup object is confirmed as deleted, the finalizer +// is removed from the NAB, allowing the object itself to be deleted by Kubernetes garbage collection. +// Returns a boolean indicating whether to requeue and an error if any update or deletion operation fails. +func (r *NonAdminBackupReconciler) finalizeNabDeletion(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + // Check if DeleteBackup is set and finalizer is present + if !nab.Spec.DeleteBackup || !controllerutil.ContainsFinalizer(nab, constant.NabFinalizerName) { + return false, nil + } + + veleroBackupNameUUID := nab.Status.VeleroBackup.NameUUID + veleroBackup, err := function.GetVeleroBackupByLabel(ctx, r.Client, r.OADPNamespace, veleroBackupNameUUID) + + if err != nil { + // Case in which more then one VeleroBackup is found with the same label UUID + // TODO: Should we delete all of the objects with such UUID ? + logger.Error(err, findSingleVBError, uuidString, veleroBackupNameUUID) + return false, err + } + + if veleroBackup != nil { + logger.V(1).Info("Waiting for VeleroBackup to be deleted", nameString, veleroBackupNameUUID) + return true, nil // Requeue + } + + // VeleroBackup is deleted, proceed with deleting the NonAdminBackup + logger.V(1).Info("VeleroBackup deleted, removing NonAdminBackup finalizer") + controllerutil.RemoveFinalizer(nab, constant.NabFinalizerName) + + if err := r.Update(ctx, nab); err != nil { + logger.Error(err, "Failed to remove finalizer from NonAdminBackup") + return false, err + } + + logger.V(1).Info("NonAdminBackup finalizer removed and object deleted") + return false, nil +} + +// initNabCreate initializes the Status.Phase from the NonAdminBackup. // // Parameters: // @@ -113,7 +293,12 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque // If it is empty, it sets the Phase to "New". // It then returns boolean values indicating whether the reconciliation loop should requeue or exit // and error value whether the status was updated successfully. -func (r *NonAdminBackupReconciler) init(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { +func (r *NonAdminBackupReconciler) initNabCreate(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + // Skip the init, due to finalizer being set + if controllerutil.ContainsFinalizer(nab, constant.NabFinalizerName) { + return false, nil + } + if nab.Status.Phase == constant.EmptyString { updated := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseNew) if updated { @@ -144,6 +329,11 @@ func (r *NonAdminBackupReconciler) init(ctx context.Context, logger logr.Logger, // If the BackupSpec is invalid, the function sets the NonAdminBackup condition Accepted to "False". // If the BackupSpec is valid, the function sets the NonAdminBackup condition Accepted to "True". func (r *NonAdminBackupReconciler) validateSpec(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + // Skip the validateSpec, due to finalizer being set + if controllerutil.ContainsFinalizer(nab, constant.NabFinalizerName) { + return false, nil + } + err := function.ValidateBackupSpec(nab) if err != nil { updatedPhase := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseBackingOff) @@ -198,6 +388,11 @@ func (r *NonAdminBackupReconciler) validateSpec(ctx context.Context, logger logr // // This function generates a UUID and stores it in the VeleroBackup status field of NonAdminBackup. func (r *NonAdminBackupReconciler) setBackupUUIDInStatus(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + // Skip the setBackupUUIDInStatus, due to finalizer being set + if controllerutil.ContainsFinalizer(nab, constant.NabFinalizerName) { + return false, nil + } + if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NameUUID == constant.EmptyString { veleroBackupNameUUID := function.GenerateNacObjectNameWithUUID(nab.Namespace, nab.Name) nab.Status.VeleroBackup = &nacv1alpha1.VeleroBackup{ @@ -226,6 +421,11 @@ func (r *NonAdminBackupReconciler) setBackupUUIDInStatus(ctx context.Context, lo // logger: Logger instance for logging messages. // nab: Pointer to the NonAdminBackup object. func (r *NonAdminBackupReconciler) createVeleroBackupAndSyncWithNonAdminBackup(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + // Skip the createVeleroBackupAndSyncWithNonAdminBackup, due to finalizer being set + if controllerutil.ContainsFinalizer(nab, constant.NabFinalizerName) { + return false, nil + } + if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NameUUID == constant.EmptyString { return false, errors.New("unable to get Velero Backup UUID from NonAdminBackup Status") } @@ -236,12 +436,12 @@ func (r *NonAdminBackupReconciler) createVeleroBackupAndSyncWithNonAdminBackup(c if err != nil { // Case in which more then one VeleroBackup is found with the same label UUID - logger.Error(err, "Failed to find single VeleroBackup object", "UUID", veleroBackupNameUUID) + logger.Error(err, findSingleVBError, uuidString, veleroBackupNameUUID) return false, err } if veleroBackup == nil { - logger.Info("VeleroBackup with label not found, creating one", "UUID", veleroBackupNameUUID) + logger.Info("VeleroBackup with label not found, creating one", uuidString, veleroBackupNameUUID) backupSpec := nab.Spec.BackupSpec.DeepCopy() backupSpec.IncludedNamespaces = []string{nab.Namespace}