From d1a710ae91cca51b226c0ce9e269cdc4cc21dffe Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Wed, 6 Mar 2024 13:50:20 +0100 Subject: [PATCH 1/8] Adjust the Status of NonAdminBackup Moves Status outside of Spec and adjusts this to reflect Velero Backup status as well additional Status when the Spec within NonAdminBackup is not defined. Signed-off-by: Michal Pryc --- config/manager/kustomization.yaml | 2 +- internal/common/function/function.go | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 1771d3e..4d98597 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -5,4 +5,4 @@ kind: Kustomization images: - name: controller newName: quay.io/konveyor/oadp-non-admin - newTag: latest + newTag: latest \ No newline at end of file diff --git a/internal/common/function/function.go b/internal/common/function/function.go index a8bc4f9..81bf037 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -262,8 +262,6 @@ func CheckVeleroBackupLabels(backup *velerov1api.Backup) bool { return exists && value == constant.ManagedByLabelValue } -// TODO not used - // GetNonAdminBackupFromVeleroBackup return referenced NonAdminBackup object from Velero Backup object, if no error occurs func GetNonAdminBackupFromVeleroBackup(ctx context.Context, clientInstance client.Client, backup *velerov1api.Backup) (*nacv1alpha1.NonAdminBackup, error) { // Check if the backup has the required annotations to identify the associated NonAdminBackup object From 3ed92ed9eb524200de41eef0d746417ce5aec4fc Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Tue, 23 Apr 2024 12:52:19 +0200 Subject: [PATCH 2/8] NonAdminBackup updates to align with updated design Additional improvements to the NonAdminBackup: - Use generic condition names (Accepted, Queued) - Introduce VeleroBackupNamespace - Change Status: OadpVeleroBackup to VeleroBackupName - Use CammelCase for condition reasoan Signed-off-by: Michal Pryc --- config/manager/kustomization.yaml | 2 +- internal/common/function/function.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 4d98597..1771d3e 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -5,4 +5,4 @@ kind: Kustomization images: - name: controller newName: quay.io/konveyor/oadp-non-admin - newTag: latest \ No newline at end of file + newTag: latest diff --git a/internal/common/function/function.go b/internal/common/function/function.go index 81bf037..2475040 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -262,6 +262,7 @@ func CheckVeleroBackupLabels(backup *velerov1api.Backup) bool { return exists && value == constant.ManagedByLabelValue } +// TODO not used // GetNonAdminBackupFromVeleroBackup return referenced NonAdminBackup object from Velero Backup object, if no error occurs func GetNonAdminBackupFromVeleroBackup(ctx context.Context, clientInstance client.Client, backup *velerov1api.Backup) (*nacv1alpha1.NonAdminBackup, error) { // Check if the backup has the required annotations to identify the associated NonAdminBackup object From 841d653e18e18e3285bf71e9a64c1912763ea0c1 Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Tue, 23 Apr 2024 16:58:53 +0200 Subject: [PATCH 3/8] Move NonAdminCondition to it's new type Adjust NonAdminCondition and move it to new types, so it can be use as well in NonAdminRestore object. Signed-off-by: Michal Pryc --- internal/common/function/function.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/common/function/function.go b/internal/common/function/function.go index 2475040..a8bc4f9 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -263,6 +263,7 @@ func CheckVeleroBackupLabels(backup *velerov1api.Backup) bool { } // TODO not used + // GetNonAdminBackupFromVeleroBackup return referenced NonAdminBackup object from Velero Backup object, if no error occurs func GetNonAdminBackupFromVeleroBackup(ctx context.Context, clientInstance client.Client, backup *velerov1api.Backup) (*nacv1alpha1.NonAdminBackup, error) { // Check if the backup has the required annotations to identify the associated NonAdminBackup object From 476783c17769cb237b5ddbdc0b97a22e3e18b527 Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Mon, 29 Apr 2024 15:09:53 +0200 Subject: [PATCH 4/8] NonAdminBackup reconcile loop major rework Rework of the reconcile loop to include batch reconcile. Move assignment of the Log or Context outisde of the reconcile loop. Signed-off-by: Michal Pryc --- internal/controller/nab_init.go | 58 ++++++ internal/controller/nab_validator.go | 90 +++++++++ internal/controller/nab_velero_backupspec.go | 136 +++++++++++++ internal/controller/nac_reconcile_utils.go | 81 ++++++++ .../controller/nac_reconcile_utils_test.go | 110 +++++++++++ .../controller/nonadminbackup_controller.go | 182 +++--------------- 6 files changed, 502 insertions(+), 155 deletions(-) create mode 100644 internal/controller/nab_init.go create mode 100644 internal/controller/nab_validator.go create mode 100644 internal/controller/nab_velero_backupspec.go create mode 100644 internal/controller/nac_reconcile_utils.go create mode 100644 internal/controller/nac_reconcile_utils_test.go diff --git a/internal/controller/nab_init.go b/internal/controller/nab_init.go new file mode 100644 index 0000000..6e402b9 --- /dev/null +++ b/internal/controller/nab_init.go @@ -0,0 +1,58 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + + "github.com/go-logr/logr" + + nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/migtools/oadp-non-admin/internal/common/constant" + "github.com/migtools/oadp-non-admin/internal/common/function" +) + +// InitNonAdminBackup Initsets the New Phase on a NonAdminBackup object if it is not already set. +// +// Parameters: +// +// ctx: Context for the request. +// log: Logger instance for logging messages. +// nab: Pointer to the NonAdminBackup object. +// +// The function checks if the Phase of the NonAdminBackup object is empty. +// If it is empty, it sets the Phase to "New". +// It then returns boolean values indicating whether the reconciliation loop should requeue +// and whether the status was updated. +func (r *NonAdminBackupReconciler) InitNonAdminBackup(ctx context.Context, log logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + logger := log.WithValues("NonAdminBackup", nab.Namespace) + // Set initial Phase + if nab.Status.Phase == constant.EmptyString { + // Phase: New + updatedStatus, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseNew) + if updatedStatus { + logger.V(1).Info("NonAdminBackup CR - Rqueue after Phase Update") + return false, true, nil + } + if errUpdate != nil { + logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: New", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdate + } + } + + return false, false, nil +} diff --git a/internal/controller/nab_validator.go b/internal/controller/nab_validator.go new file mode 100644 index 0000000..ff00be8 --- /dev/null +++ b/internal/controller/nab_validator.go @@ -0,0 +1,90 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + + "github.com/go-logr/logr" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/migtools/oadp-non-admin/internal/common/constant" + "github.com/migtools/oadp-non-admin/internal/common/function" +) + +// ValidateVeleroBackupSpec validates the VeleroBackup Spec from the NonAdminBackup. +// +// Parameters: +// +// ctx: Context for the request. +// log: Logger instance for logging messages. +// nab: Pointer to the NonAdminBackup object. +// +// The function attempts to get the BackupSpec from the NonAdminBackup object. +// If an error occurs during this process, the function sets the NonAdminBackup status to "BackingOff" +// and updates the corresponding condition accordingly. +// If the BackupSpec is invalid, the function sets the NonAdminBackup condition to "InvalidBackupSpec". +// If the BackupSpec is valid, the function sets the NonAdminBackup condition to "BackupAccepted". +func (r *NonAdminBackupReconciler) ValidateVeleroBackupSpec(ctx context.Context, log logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + logger := log.WithValues("NonAdminBackup", nab.Namespace) + + // Main Validation point for the VeleroBackup included in NonAdminBackup spec + _, err := function.GetBackupSpecFromNonAdminBackup(nab) + + if err != nil { + errMsg := "NonAdminBackup CR does not contain valid BackupSpec" + logger.Error(err, errMsg) + + updatedStatus, errUpdateStatus := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseBackingOff) + if errUpdateStatus != nil { + logger.Error(errUpdateStatus, "Unable to set NonAdminBackup Phase: BackingOff", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdateStatus + } else if updatedStatus { + // We do not requeue - the State was set to BackingOff + return true, false, nil + } + + // Continue. VeleroBackup looks fine, setting Accepted condition + updatedCondition, errUpdateCondition := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionFalse, "InvalidBackupSpec", errMsg) + if updatedCondition { + // We do not requeue - this was only Condition update + return true, false, nil + } + + if errUpdateCondition != nil { + logger.Error(errUpdateCondition, "Unable to set BackupAccepted Condition: False", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdateCondition + } + // We do not requeue - this was error from getting Spec from NAB + return true, false, err + } + + updatedStatus, errUpdateStatus := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "BackupAccepted", "backup accepted") + if updatedStatus { + // We do requeue - The VeleroBackup got accepted and next reconcile loop will continue + // with further work on the VeleroBackup such as creating it + return false, true, nil + } + + if errUpdateStatus != nil { + logger.Error(errUpdateStatus, "Unable to set BackupAccepted Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdateStatus + } + + return false, false, nil +} diff --git a/internal/controller/nab_velero_backupspec.go b/internal/controller/nab_velero_backupspec.go new file mode 100644 index 0000000..9f878b5 --- /dev/null +++ b/internal/controller/nab_velero_backupspec.go @@ -0,0 +1,136 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "errors" + + "github.com/go-logr/logr" + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/migtools/oadp-non-admin/internal/common/constant" + "github.com/migtools/oadp-non-admin/internal/common/function" +) + +// CreateVeleroBackupSpec creates or updates a Velero Backup object based on the provided NonAdminBackup object. +// +// Parameters: +// +// ctx: Context for the request. +// log: Logger instance for logging messages. +// nab: Pointer to the NonAdminBackup object. +// +// The function generates a name for the Velero Backup object based on the provided namespace and name. +// It then checks if a Velero Backup object with that name already exists. If it does not exist, it creates a new one. +// The function returns boolean values indicating whether the reconciliation loop should exit or requeue +func (r *NonAdminBackupReconciler) CreateVeleroBackupSpec(ctx context.Context, log logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + logger := log.WithValues("NonAdminBackup", nab.Namespace) + + veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name) + + if veleroBackupName == constant.EmptyString { + return true, false, errors.New("unable to generate Velero Backup name") + } + + veleroBackup := velerov1api.Backup{} + err := r.Get(ctx, client.ObjectKey{Namespace: constant.OadpNamespace, Name: veleroBackupName}, &veleroBackup) + + if err != nil && apierrors.IsNotFound(err) { + // Create VeleroBackup + // Don't update phase nor conditions yet. + // Those will be updated when then Reconcile loop is triggered by the VeleroBackup object + logger.Info("No backup found", nameField, veleroBackupName) + + // We don't validate error here. + // This was already validated in the ValidateVeleroBackupSpec + backupSpec, errBackup := function.GetBackupSpecFromNonAdminBackup(nab) + + if errBackup != nil { + // Should never happen as it was already checked + return true, false, errBackup + } + + veleroBackup = velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Name: veleroBackupName, + Namespace: constant.OadpNamespace, + }, + Spec: *backupSpec, + } + } else if err != nil && !apierrors.IsNotFound(err) { + logger.Error(err, "Unable to fetch VeleroBackup") + return true, false, err + } else { + // We should not update already created VeleroBackup object. + // The VeleroBackup within NonAdminBackup will + // be reverted back to the previous state - the state which created VeleroBackup + // in a first place, so they will be in sync. + logger.Info("Backup already exists, updating NonAdminBackup status", nameField, veleroBackupName) + updatedNab, errBackupUpdate := function.UpdateNonAdminBackupFromVeleroBackup(ctx, r.Client, logger, nab, &veleroBackup) + // Regardless if the status was updated or not, we should not + // requeue here as it was only status update. + if errBackupUpdate != nil { + return true, false, errBackupUpdate + } else if updatedNab { + logger.V(1).Info("NonAdminBackup CR - Rqueue after Status Update") + return false, true, nil + } + return true, false, nil + } + + // Ensure labels are set for the Backup object + existingLabels := veleroBackup.Labels + naManagedLabels := function.AddNonAdminLabels(existingLabels) + veleroBackup.Labels = naManagedLabels + + // Ensure annotations are set for the Backup object + existingAnnotations := veleroBackup.Annotations + ownerUUID := string(nab.ObjectMeta.UID) + nabManagedAnnotations := function.AddNonAdminBackupAnnotations(nab.Namespace, nab.Name, ownerUUID, existingAnnotations) + veleroBackup.Annotations = nabManagedAnnotations + + _, err = controllerutil.CreateOrPatch(ctx, r.Client, &veleroBackup, nil) + if err != nil { + logger.Error(err, "Failed to create backup", nameField, veleroBackupName) + return true, false, err + } + logger.Info("VeleroBackup successfully created", nameField, veleroBackupName) + + _, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseCreated) + if errUpdate != nil { + logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: Created", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdate + } + _, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "Validated", "Valid Backup config") + if errUpdate != nil { + logger.Error(errUpdate, "Unable to set BackupAccepted Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdate + } + _, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionQueued, metav1.ConditionTrue, "BackupScheduled", "Created Velero Backup object") + if errUpdate != nil { + logger.Error(errUpdate, "Unable to set BackupQueued Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdate + } + + return false, false, nil +} diff --git a/internal/controller/nac_reconcile_utils.go b/internal/controller/nac_reconcile_utils.go new file mode 100644 index 0000000..e6c4131 --- /dev/null +++ b/internal/controller/nac_reconcile_utils.go @@ -0,0 +1,81 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import "errors" + +// ReconcileFunc represents a function that performs a reconciliation operation. +// Parameters: +// - ...any: A variadic number of arguments, +// allowing for flexibility in passing parameters to the reconciliation function. +// +// Returns: +// - bool: Indicates whether the reconciliation process should exit. +// - bool: Indicates whether the reconciliation process should requeue. +// - error: An error encountered during reconciliation, if any. +type ReconcileFunc func(...any) (bool, bool, error) + +// ReconcileBatch executes a batch of reconcile functions sequentially, +// allowing for complex reconciliation processes in a controlled manner. +// It iterates through the provided reconcile functions until one of the +// following conditions is met: +// - A function signals the need to exit the reconciliation process. +// - A function signals the need to exit and requeue the reconciliation process. +// - An error occurs during reconciliation. +// +// If any reconcile function indicates a need to exit or requeue, +// the function immediately returns with the respective exit and requeue +// flags set. If an error occurs during any reconciliation function call, +// it is propagated up and returned. If none of the reconcile functions +// indicate a need to exit, requeue, or result in an error, the function +// returns false for both exit and requeue flags, indicating successful reconciliation. +// +// If a reconcile function signals both the need to exit and requeue, indicating +// conflicting signals, the function returns an error with the exit flag set to true +// and the requeue flag set to false, so no . +// +// Parameters: +// - reconcileFuncs: A list of ReconcileFunc functions representing +// the reconciliation steps to be executed sequentially. +// +// Returns: +// - bool: Indicates whether the reconciliation process should exit. +// - bool: Indicates whether the reconciliation process should requeue. +// - error: An error encountered during reconciliation, if any. +func ReconcileBatch(reconcileFuncs ...ReconcileFunc) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + var exit, requeue bool + var err error + var conflictError = errors.New("conflicting exit and requeue signals - can not be both true") + + for _, f := range reconcileFuncs { + exit, requeue, err = f() + + // Check if both exit and requeue are true + // If this happens do not requeue, but exit with error + if exit && requeue { + return true, false, conflictError + } + + // Return if there is a need to exit, requeue, or an error occurred + if exit || requeue || err != nil { + return exit, requeue, err + } + } + + // Do not requeue or exit reconcile. Also no error occurred. + return false, false, nil +} diff --git a/internal/controller/nac_reconcile_utils_test.go b/internal/controller/nac_reconcile_utils_test.go new file mode 100644 index 0000000..13722ed --- /dev/null +++ b/internal/controller/nac_reconcile_utils_test.go @@ -0,0 +1,110 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "errors" + "testing" +) + +func sampleReconcileFunc1(_ ...any) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + return false, false, nil +} + +func sampleReconcileFunc2(_ ...any) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + return true, false, nil +} + +func sampleReconcileFunc3(_ ...any) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + return false, true, nil +} + +func sampleReconcileFuncWithError(_ ...any) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + return false, false, errors.New("sample error") +} + +func sampleConflictingReconcileFunc(_ ...any) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + return true, true, nil +} + +func TestReconcileBatch(t *testing.T) { + tests := []struct { + expectedError error + name string + reconcileFuncs []ReconcileFunc + expectedExit bool + expectedRequeue bool + }{ + { + name: "No functions", + reconcileFuncs: nil, + expectedExit: false, + expectedRequeue: false, + expectedError: nil, + }, + { + name: "All functions return false", + reconcileFuncs: []ReconcileFunc{sampleReconcileFunc1, sampleReconcileFunc1}, + expectedExit: false, + expectedRequeue: false, + expectedError: nil, + }, + { + name: "First function returns true", + reconcileFuncs: []ReconcileFunc{sampleReconcileFunc2, sampleReconcileFunc1}, + expectedExit: true, + expectedRequeue: false, + expectedError: nil, + }, + { + name: "Second function returns requeue", + reconcileFuncs: []ReconcileFunc{sampleReconcileFunc1, sampleReconcileFunc3}, + expectedExit: false, + expectedRequeue: true, + expectedError: nil, + }, + { + name: "Function returns error", + reconcileFuncs: []ReconcileFunc{sampleReconcileFunc1, sampleReconcileFuncWithError}, + expectedExit: false, + expectedRequeue: false, + expectedError: errors.New("sample error"), + }, + { + name: "Function signals conflicting exit and requeue", + reconcileFuncs: []ReconcileFunc{sampleConflictingReconcileFunc}, + expectedExit: true, + expectedRequeue: false, + expectedError: errors.New("conflicting exit and requeue signals - can not be both true"), + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + exit, requeue, err := ReconcileBatch(test.reconcileFuncs...) + if exit != test.expectedExit { + t.Errorf("Expected exit: %v, but got: %v", test.expectedExit, exit) + } + if requeue != test.expectedRequeue { + t.Errorf("Expected requeue: %v, but got: %v", test.expectedRequeue, requeue) + } + if (err == nil && test.expectedError != nil) || (err != nil && test.expectedError == nil) || (err != nil && test.expectedError != nil && err.Error() != test.expectedError.Error()) { + t.Errorf("Expected error: %v, but got: %v", test.expectedError, err) + } + }) + } +} diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 55141ec..8000557 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -19,23 +19,17 @@ package controller import ( "context" - "errors" "time" - "github.com/go-logr/logr" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "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" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" "github.com/migtools/oadp-non-admin/internal/common/constant" - "github.com/migtools/oadp-non-admin/internal/common/function" "github.com/migtools/oadp-non-admin/internal/handler" "github.com/migtools/oadp-non-admin/internal/predicate" ) @@ -43,10 +37,8 @@ import ( // NonAdminBackupReconciler reconciles a NonAdminBackup object type NonAdminBackupReconciler struct { client.Client - Scheme *runtime.Scheme - Log logr.Logger - Context context.Context - NamespacedName types.NamespacedName + Scheme *runtime.Scheme + Context context.Context } const ( @@ -70,23 +62,18 @@ const ( // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.0/pkg/reconcile func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - r.Log = log.FromContext(ctx) - logger := r.Log.WithValues("NonAdminBackup", req.NamespacedName) + rLog := log.FromContext(ctx) + logger := rLog.WithValues("NonAdminBackup", req.NamespacedName) logger.V(1).Info(">>> Reconcile NonAdminBackup - loop start") - // Resource version - // Generation version - metadata - only one is updated... - r.Context = ctx - r.NamespacedName = req.NamespacedName - - // Get the Non Admin Backup object + // Get the NonAdminBackup object nab := nacv1alpha1.NonAdminBackup{} err := r.Get(ctx, req.NamespacedName, &nab) // Bail out when the Non Admin Backup reconcile was triggered, when the NAB got deleted // Reconcile loop was triggered when Velero Backup object got updated and NAB isn't there if err != nil && apierrors.IsNotFound(err) { - logger.V(1).Info("Deleted NonAdminBackup CR", nameField, req.Name, constant.NameSpaceString, req.Namespace) + logger.V(1).Info("Non existing NonAdminBackup CR", nameField, req.Name, constant.NameSpaceString, req.Namespace) return ctrl.Result{}, nil } @@ -95,142 +82,27 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - if nab.Status.Phase == constant.EmptyString { - // Phase: New - updatedStatus, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, &nab, nacv1alpha1.NonAdminBackupPhaseNew) - if updatedStatus { - logger.V(1).Info("NonAdminBackup CR - Rqueue after Phase Update") - return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, nil - } - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: New", nameField, req.Name, constant.NameSpaceString, req.Namespace) - return ctrl.Result{}, errUpdate - } - } - - backupSpec, err := function.GetBackupSpecFromNonAdminBackup(&nab) - - if err != nil { - errMsg := "NonAdminBackup CR does not contain valid BackupSpec" - logger.Error(err, errMsg) - - // Phase: BackingOff - // BackupAccepted: False - // BackupQueued: False # already set - updatedStatus, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, &nab, nacv1alpha1.NonAdminBackupPhaseBackingOff) - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: BackingOff", nameField, req.Name, constant.NameSpaceString, req.Namespace) - return ctrl.Result{}, errUpdate - } else if updatedStatus { - // We do not requeue as the State is BackingOff - return ctrl.Result{}, nil - } - - updatedStatus, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, &nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionFalse, "InvalidBackupSpec", errMsg) - if updatedStatus { - return ctrl.Result{}, nil - } - - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set BackupAccepted Condition: False", nameField, req.Name, constant.NameSpaceString, req.Namespace) - return ctrl.Result{}, errUpdate - } - return ctrl.Result{}, err - } - - // Phase: New # already set - // BackupAccepted: True - // BackupQueued: False # already set - updatedStatus, errUpdate := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, &nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "BackupAccepted", "backup accepted") - if updatedStatus { - return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, nil - } - - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set BackupAccepted Condition: True", nameField, req.Name, constant.NameSpaceString, req.Namespace) - return ctrl.Result{}, errUpdate - } - - veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name) - - if veleroBackupName == constant.EmptyString { - return ctrl.Result{}, errors.New("unable to generate Velero Backup name") - } - - veleroBackup := velerov1api.Backup{} - err = r.Get(ctx, client.ObjectKey{Namespace: constant.OadpNamespace, Name: veleroBackupName}, &veleroBackup) - - if err != nil && apierrors.IsNotFound(err) { - // Create backup - // Don't update phase nor conditions yet. - // Those will be updated when then Reconcile loop is triggered by the VeleroBackup object - logger.Info("No backup found", nameField, veleroBackupName) - veleroBackup = velerov1api.Backup{ - ObjectMeta: metav1.ObjectMeta{ - Name: veleroBackupName, - Namespace: constant.OadpNamespace, - }, - Spec: *backupSpec, - } - } else if err != nil && !apierrors.IsNotFound(err) { - logger.Error(err, "Unable to fetch VeleroBackup") - return ctrl.Result{}, err - } else { - // TODO: Currently when one updates VeleroBackup spec inside the NonAdminBackup - // We do not update already created VeleroBackup object. - // Should we update the previously created VeleroBackup object and reflect what was - // updated? In the current situation the VeleroBackup within NonAdminBackup will - // be reverted back to the previous state - the state which created VeleroBackup - // in a first place. - logger.Info("Backup already exists, updating NonAdminBackup status", nameField, veleroBackupName) - updatedNab, errBackupUpdate := function.UpdateNonAdminBackupFromVeleroBackup(ctx, r.Client, logger, &nab, &veleroBackup) - // Regardless if the status was updated or not, we should not - // requeue here as it was only status update. - if errBackupUpdate != nil { - return ctrl.Result{}, errBackupUpdate - } else if updatedNab { - logger.V(1).Info("NonAdminBackup CR - Rqueue after Status Update") - return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, nil - } - return ctrl.Result{}, nil - } - - // Ensure labels are set for the Backup object - existingLabels := veleroBackup.Labels - naManagedLabels := function.AddNonAdminLabels(existingLabels) - veleroBackup.Labels = naManagedLabels - - // Ensure annotations are set for the Backup object - existingAnnotations := veleroBackup.Annotations - ownerUUID := string(nab.ObjectMeta.UID) - nabManagedAnnotations := function.AddNonAdminBackupAnnotations(nab.Namespace, nab.Name, ownerUUID, existingAnnotations) - veleroBackup.Annotations = nabManagedAnnotations - - _, err = controllerutil.CreateOrPatch(ctx, r.Client, &veleroBackup, nil) - if err != nil { - logger.Error(err, "Failed to create backup", nameField, veleroBackupName) - return ctrl.Result{}, err - } - logger.Info("VeleroBackup successfully created", nameField, veleroBackupName) - - // Phase: Created - // BackupAccepted: True # do not update - // BackupQueued: True - _, errUpdate = function.UpdateNonAdminPhase(ctx, r.Client, logger, &nab, nacv1alpha1.NonAdminBackupPhaseCreated) - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: Created", nameField, req.Name, constant.NameSpaceString, req.Namespace) - return ctrl.Result{}, errUpdate - } - _, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, &nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "Validated", "Valid Backup config") - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set BackupAccepted Condition: True", nameField, req.Name, constant.NameSpaceString, req.Namespace) - return ctrl.Result{}, errUpdate - } - _, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, &nab, nacv1alpha1.NonAdminConditionQueued, metav1.ConditionTrue, "BackupScheduled", "Created Velero Backup object") - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set BackupQueued Condition: True", nameField, req.Name, constant.NameSpaceString, req.Namespace) - return ctrl.Result{}, errUpdate - } + // Run Reconcile Batch as we have the NonAdminBackup object + reconcileExit, reconcileRequeue, reconcileErr := ReconcileBatch( + ReconcileFunc(func(...any) (bool, bool, error) { + return r.InitNonAdminBackup(ctx, rLog, &nab) // Phase: New + }), + ReconcileFunc(func(...any) (bool, bool, error) { + return r.ValidateVeleroBackupSpec(ctx, rLog, &nab) // Phase: New + }), + ReconcileFunc(func(...any) (bool, bool, error) { + return r.CreateVeleroBackupSpec(ctx, rLog, &nab) // Phase: New + }), + ) + + // Return vars from the ReconcileBatch + if reconcileExit { + return ctrl.Result{}, reconcileErr + } else if reconcileRequeue { + return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr + } else if reconcileErr != nil { + return ctrl.Result{}, reconcileErr + } // No error and both reconcileExit and reconcileRequeue false, continue... logger.V(1).Info(">>> Reconcile NonAdminBackup - loop end") From c91e31a9788e167b4b88013bc3e980c1e9adfbe1 Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Mon, 29 Apr 2024 16:32:54 +0200 Subject: [PATCH 5/8] Simplified error statement in the Reconcile loop. Small nit to make if statement easier to read. Signed-off-by: Michal Pryc --- internal/controller/nonadminbackup_controller.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 8000557..11239aa 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -72,12 +72,11 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque // Bail out when the Non Admin Backup reconcile was triggered, when the NAB got deleted // Reconcile loop was triggered when Velero Backup object got updated and NAB isn't there - if err != nil && apierrors.IsNotFound(err) { - logger.V(1).Info("Non existing NonAdminBackup CR", nameField, req.Name, constant.NameSpaceString, req.Namespace) - return ctrl.Result{}, nil - } - if err != nil { + if apierrors.IsNotFound(err) { + logger.V(1).Info("Non existing NonAdminBackup CR", nameField, req.Name, constant.NameSpaceString, req.Namespace) + return ctrl.Result{}, nil + } logger.Error(err, "Unable to fetch NonAdminBackup CR", nameField, req.Name, constant.NameSpaceString, req.Namespace) return ctrl.Result{}, err } From f4f4e557fc2bada4ce0c9def05935487b89d9b04 Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Mon, 29 Apr 2024 16:50:10 +0200 Subject: [PATCH 6/8] Adjust the reconcile loop to use simpler logic from reconcile batch Change of the main reconcile loop to easy logic of the return from the reconcile batch. We only requeue when there is need, otherwise we do exit the reconcile loop at the same place and individual reconcile functions control if that should happen now or should be requeued. Signed-off-by: Michal Pryc --- .../controller/nonadminbackup_controller.go | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 11239aa..84e1e6f 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -82,30 +82,25 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque } // Run Reconcile Batch as we have the NonAdminBackup object - reconcileExit, reconcileRequeue, reconcileErr := ReconcileBatch( + _, reconcileRequeue, reconcileErr := ReconcileBatch( ReconcileFunc(func(...any) (bool, bool, error) { - return r.InitNonAdminBackup(ctx, rLog, &nab) // Phase: New + return r.InitNonAdminBackup(ctx, rLog, &nab) }), ReconcileFunc(func(...any) (bool, bool, error) { - return r.ValidateVeleroBackupSpec(ctx, rLog, &nab) // Phase: New + return r.ValidateVeleroBackupSpec(ctx, rLog, &nab) }), ReconcileFunc(func(...any) (bool, bool, error) { - return r.CreateVeleroBackupSpec(ctx, rLog, &nab) // Phase: New + return r.CreateVeleroBackupSpec(ctx, rLog, &nab) }), ) // Return vars from the ReconcileBatch - if reconcileExit { - return ctrl.Result{}, reconcileErr - } else if reconcileRequeue { + if reconcileRequeue { return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr - } else if reconcileErr != nil { - return ctrl.Result{}, reconcileErr - } // No error and both reconcileExit and reconcileRequeue false, continue... + } logger.V(1).Info(">>> Reconcile NonAdminBackup - loop end") - - return ctrl.Result{}, nil + return ctrl.Result{}, reconcileErr } // SetupWithManager sets up the controller with the Manager. From 0d184217f2e100ad5830a1e369ad40ca81574016 Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Tue, 30 Apr 2024 11:00:01 +0200 Subject: [PATCH 7/8] Rework of rework of reconcile functions In this implementation we don't have ReconcileBatch, instead we do have functions with arguments that has appropriate types. Also there was modified implementation to first check for errors and then continue with app flow. Signed-off-by: Michal Pryc --- internal/controller/nab_init.go | 14 ++- internal/controller/nab_validator.go | 21 ++-- internal/controller/nab_velero_backupspec.go | 2 +- internal/controller/nac_reconcile_utils.go | 81 ------------- .../controller/nac_reconcile_utils_test.go | 110 ------------------ .../controller/nonadminbackup_controller.go | 33 +++--- 6 files changed, 36 insertions(+), 225 deletions(-) delete mode 100644 internal/controller/nac_reconcile_utils.go delete mode 100644 internal/controller/nac_reconcile_utils_test.go diff --git a/internal/controller/nab_init.go b/internal/controller/nab_init.go index 6e402b9..a704c21 100644 --- a/internal/controller/nab_init.go +++ b/internal/controller/nab_init.go @@ -26,7 +26,7 @@ import ( "github.com/migtools/oadp-non-admin/internal/common/function" ) -// InitNonAdminBackup Initsets the New Phase on a NonAdminBackup object if it is not already set. +// InitNonAdminBackup sets the New Phase on a NonAdminBackup object if it is not already set. // // Parameters: // @@ -39,19 +39,21 @@ import ( // It then returns boolean values indicating whether the reconciliation loop should requeue // and whether the status was updated. func (r *NonAdminBackupReconciler) InitNonAdminBackup(ctx context.Context, log logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := log.WithValues("NonAdminBackup", nab.Namespace) + logger := log.WithValues("InitNonAdminBackup", nab.Namespace) // Set initial Phase if nab.Status.Phase == constant.EmptyString { // Phase: New updatedStatus, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseNew) - if updatedStatus { - logger.V(1).Info("NonAdminBackup CR - Rqueue after Phase Update") - return false, true, nil - } + if errUpdate != nil { logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: New", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) return true, false, errUpdate } + + if updatedStatus { + logger.V(1).Info("NonAdminBackup CR - Requeue after Phase Update") + return false, true, nil + } } return false, false, nil diff --git a/internal/controller/nab_validator.go b/internal/controller/nab_validator.go index ff00be8..e7dad6b 100644 --- a/internal/controller/nab_validator.go +++ b/internal/controller/nab_validator.go @@ -41,7 +41,7 @@ import ( // If the BackupSpec is invalid, the function sets the NonAdminBackup condition to "InvalidBackupSpec". // If the BackupSpec is valid, the function sets the NonAdminBackup condition to "BackupAccepted". func (r *NonAdminBackupReconciler) ValidateVeleroBackupSpec(ctx context.Context, log logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := log.WithValues("NonAdminBackup", nab.Namespace) + logger := log.WithValues("ValidateVeleroBackupSpec", nab.Namespace) // Main Validation point for the VeleroBackup included in NonAdminBackup spec _, err := function.GetBackupSpecFromNonAdminBackup(nab) @@ -61,29 +61,26 @@ func (r *NonAdminBackupReconciler) ValidateVeleroBackupSpec(ctx context.Context, // Continue. VeleroBackup looks fine, setting Accepted condition updatedCondition, errUpdateCondition := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionFalse, "InvalidBackupSpec", errMsg) - if updatedCondition { - // We do not requeue - this was only Condition update - return true, false, nil - } if errUpdateCondition != nil { logger.Error(errUpdateCondition, "Unable to set BackupAccepted Condition: False", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) return true, false, errUpdateCondition + } else if updatedCondition { + return true, false, nil } - // We do not requeue - this was error from getting Spec from NAB + + // We do not requeue - this was an error from getting Spec from NAB return true, false, err } updatedStatus, errUpdateStatus := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "BackupAccepted", "backup accepted") - if updatedStatus { - // We do requeue - The VeleroBackup got accepted and next reconcile loop will continue - // with further work on the VeleroBackup such as creating it - return false, true, nil - } - if errUpdateStatus != nil { logger.Error(errUpdateStatus, "Unable to set BackupAccepted Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) return true, false, errUpdateStatus + } else if updatedStatus { + // We do requeue - The VeleroBackup got accepted and next reconcile loop will continue + // with further work on the VeleroBackup such as creating it + return false, true, nil } return false, false, nil diff --git a/internal/controller/nab_velero_backupspec.go b/internal/controller/nab_velero_backupspec.go index 9f878b5..4278d37 100644 --- a/internal/controller/nab_velero_backupspec.go +++ b/internal/controller/nab_velero_backupspec.go @@ -44,7 +44,7 @@ import ( // It then checks if a Velero Backup object with that name already exists. If it does not exist, it creates a new one. // The function returns boolean values indicating whether the reconciliation loop should exit or requeue func (r *NonAdminBackupReconciler) CreateVeleroBackupSpec(ctx context.Context, log logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := log.WithValues("NonAdminBackup", nab.Namespace) + logger := log.WithValues("CreateVeleroBackupSpec", nab.Namespace) veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name) diff --git a/internal/controller/nac_reconcile_utils.go b/internal/controller/nac_reconcile_utils.go deleted file mode 100644 index e6c4131..0000000 --- a/internal/controller/nac_reconcile_utils.go +++ /dev/null @@ -1,81 +0,0 @@ -/* -Copyright 2024. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller - -import "errors" - -// ReconcileFunc represents a function that performs a reconciliation operation. -// Parameters: -// - ...any: A variadic number of arguments, -// allowing for flexibility in passing parameters to the reconciliation function. -// -// Returns: -// - bool: Indicates whether the reconciliation process should exit. -// - bool: Indicates whether the reconciliation process should requeue. -// - error: An error encountered during reconciliation, if any. -type ReconcileFunc func(...any) (bool, bool, error) - -// ReconcileBatch executes a batch of reconcile functions sequentially, -// allowing for complex reconciliation processes in a controlled manner. -// It iterates through the provided reconcile functions until one of the -// following conditions is met: -// - A function signals the need to exit the reconciliation process. -// - A function signals the need to exit and requeue the reconciliation process. -// - An error occurs during reconciliation. -// -// If any reconcile function indicates a need to exit or requeue, -// the function immediately returns with the respective exit and requeue -// flags set. If an error occurs during any reconciliation function call, -// it is propagated up and returned. If none of the reconcile functions -// indicate a need to exit, requeue, or result in an error, the function -// returns false for both exit and requeue flags, indicating successful reconciliation. -// -// If a reconcile function signals both the need to exit and requeue, indicating -// conflicting signals, the function returns an error with the exit flag set to true -// and the requeue flag set to false, so no . -// -// Parameters: -// - reconcileFuncs: A list of ReconcileFunc functions representing -// the reconciliation steps to be executed sequentially. -// -// Returns: -// - bool: Indicates whether the reconciliation process should exit. -// - bool: Indicates whether the reconciliation process should requeue. -// - error: An error encountered during reconciliation, if any. -func ReconcileBatch(reconcileFuncs ...ReconcileFunc) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - var exit, requeue bool - var err error - var conflictError = errors.New("conflicting exit and requeue signals - can not be both true") - - for _, f := range reconcileFuncs { - exit, requeue, err = f() - - // Check if both exit and requeue are true - // If this happens do not requeue, but exit with error - if exit && requeue { - return true, false, conflictError - } - - // Return if there is a need to exit, requeue, or an error occurred - if exit || requeue || err != nil { - return exit, requeue, err - } - } - - // Do not requeue or exit reconcile. Also no error occurred. - return false, false, nil -} diff --git a/internal/controller/nac_reconcile_utils_test.go b/internal/controller/nac_reconcile_utils_test.go deleted file mode 100644 index 13722ed..0000000 --- a/internal/controller/nac_reconcile_utils_test.go +++ /dev/null @@ -1,110 +0,0 @@ -/* -Copyright 2024. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller - -import ( - "errors" - "testing" -) - -func sampleReconcileFunc1(_ ...any) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - return false, false, nil -} - -func sampleReconcileFunc2(_ ...any) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - return true, false, nil -} - -func sampleReconcileFunc3(_ ...any) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - return false, true, nil -} - -func sampleReconcileFuncWithError(_ ...any) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - return false, false, errors.New("sample error") -} - -func sampleConflictingReconcileFunc(_ ...any) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - return true, true, nil -} - -func TestReconcileBatch(t *testing.T) { - tests := []struct { - expectedError error - name string - reconcileFuncs []ReconcileFunc - expectedExit bool - expectedRequeue bool - }{ - { - name: "No functions", - reconcileFuncs: nil, - expectedExit: false, - expectedRequeue: false, - expectedError: nil, - }, - { - name: "All functions return false", - reconcileFuncs: []ReconcileFunc{sampleReconcileFunc1, sampleReconcileFunc1}, - expectedExit: false, - expectedRequeue: false, - expectedError: nil, - }, - { - name: "First function returns true", - reconcileFuncs: []ReconcileFunc{sampleReconcileFunc2, sampleReconcileFunc1}, - expectedExit: true, - expectedRequeue: false, - expectedError: nil, - }, - { - name: "Second function returns requeue", - reconcileFuncs: []ReconcileFunc{sampleReconcileFunc1, sampleReconcileFunc3}, - expectedExit: false, - expectedRequeue: true, - expectedError: nil, - }, - { - name: "Function returns error", - reconcileFuncs: []ReconcileFunc{sampleReconcileFunc1, sampleReconcileFuncWithError}, - expectedExit: false, - expectedRequeue: false, - expectedError: errors.New("sample error"), - }, - { - name: "Function signals conflicting exit and requeue", - reconcileFuncs: []ReconcileFunc{sampleConflictingReconcileFunc}, - expectedExit: true, - expectedRequeue: false, - expectedError: errors.New("conflicting exit and requeue signals - can not be both true"), - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - exit, requeue, err := ReconcileBatch(test.reconcileFuncs...) - if exit != test.expectedExit { - t.Errorf("Expected exit: %v, but got: %v", test.expectedExit, exit) - } - if requeue != test.expectedRequeue { - t.Errorf("Expected requeue: %v, but got: %v", test.expectedRequeue, requeue) - } - if (err == nil && test.expectedError != nil) || (err != nil && test.expectedError == nil) || (err != nil && test.expectedError != nil && err.Error() != test.expectedError.Error()) { - t.Errorf("Expected error: %v, but got: %v", test.expectedError, err) - } - }) - } -} diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 84e1e6f..90ada93 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -81,26 +81,29 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - // Run Reconcile Batch as we have the NonAdminBackup object - _, reconcileRequeue, reconcileErr := ReconcileBatch( - ReconcileFunc(func(...any) (bool, bool, error) { - return r.InitNonAdminBackup(ctx, rLog, &nab) - }), - ReconcileFunc(func(...any) (bool, bool, error) { - return r.ValidateVeleroBackupSpec(ctx, rLog, &nab) - }), - ReconcileFunc(func(...any) (bool, bool, error) { - return r.CreateVeleroBackupSpec(ctx, rLog, &nab) - }), - ) - - // Return vars from the ReconcileBatch + reconcileExit, reconcileRequeue, reconcileErr := r.InitNonAdminBackup(ctx, rLog, &nab) if reconcileRequeue { return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr + } else if reconcileExit || reconcileErr != nil { + return ctrl.Result{}, reconcileErr + } + + reconcileExit, reconcileRequeue, reconcileErr = r.ValidateVeleroBackupSpec(ctx, rLog, &nab) + if reconcileRequeue { + return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr + } else if reconcileExit || reconcileErr != nil { + return ctrl.Result{}, reconcileErr + } + + reconcileExit, reconcileRequeue, reconcileErr = r.CreateVeleroBackupSpec(ctx, rLog, &nab) + if reconcileRequeue { + return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr + } else if reconcileExit || reconcileErr != nil { + return ctrl.Result{}, reconcileErr } logger.V(1).Info(">>> Reconcile NonAdminBackup - loop end") - return ctrl.Result{}, reconcileErr + return ctrl.Result{}, nil } // SetupWithManager sets up the controller with the Manager. From 957fc3c309e3c01d63c63cea72b4f3e3471e306b Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Mon, 6 May 2024 16:04:10 +0200 Subject: [PATCH 8/8] Move reconcile functions inside NAB controller With this change there is no separate file per reconcile function and we move all the reconcile related functions inside nonadminbackup_controller. Signed-off-by: Michal Pryc --- internal/controller/nab_init.go | 60 ------ internal/controller/nab_validator.go | 87 -------- internal/controller/nab_velero_backupspec.go | 136 ------------ .../controller/nonadminbackup_controller.go | 200 ++++++++++++++++++ 4 files changed, 200 insertions(+), 283 deletions(-) delete mode 100644 internal/controller/nab_init.go delete mode 100644 internal/controller/nab_validator.go delete mode 100644 internal/controller/nab_velero_backupspec.go diff --git a/internal/controller/nab_init.go b/internal/controller/nab_init.go deleted file mode 100644 index a704c21..0000000 --- a/internal/controller/nab_init.go +++ /dev/null @@ -1,60 +0,0 @@ -/* -Copyright 2024. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller - -import ( - "context" - - "github.com/go-logr/logr" - - nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" - "github.com/migtools/oadp-non-admin/internal/common/constant" - "github.com/migtools/oadp-non-admin/internal/common/function" -) - -// InitNonAdminBackup sets the New Phase on a NonAdminBackup object if it is not already set. -// -// Parameters: -// -// ctx: Context for the request. -// log: Logger instance for logging messages. -// nab: Pointer to the NonAdminBackup object. -// -// The function checks if the Phase of the NonAdminBackup object is empty. -// If it is empty, it sets the Phase to "New". -// It then returns boolean values indicating whether the reconciliation loop should requeue -// and whether the status was updated. -func (r *NonAdminBackupReconciler) InitNonAdminBackup(ctx context.Context, log logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := log.WithValues("InitNonAdminBackup", nab.Namespace) - // Set initial Phase - if nab.Status.Phase == constant.EmptyString { - // Phase: New - updatedStatus, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseNew) - - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: New", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdate - } - - if updatedStatus { - logger.V(1).Info("NonAdminBackup CR - Requeue after Phase Update") - return false, true, nil - } - } - - return false, false, nil -} diff --git a/internal/controller/nab_validator.go b/internal/controller/nab_validator.go deleted file mode 100644 index e7dad6b..0000000 --- a/internal/controller/nab_validator.go +++ /dev/null @@ -1,87 +0,0 @@ -/* -Copyright 2024. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller - -import ( - "context" - - "github.com/go-logr/logr" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" - "github.com/migtools/oadp-non-admin/internal/common/constant" - "github.com/migtools/oadp-non-admin/internal/common/function" -) - -// ValidateVeleroBackupSpec validates the VeleroBackup Spec from the NonAdminBackup. -// -// Parameters: -// -// ctx: Context for the request. -// log: Logger instance for logging messages. -// nab: Pointer to the NonAdminBackup object. -// -// The function attempts to get the BackupSpec from the NonAdminBackup object. -// If an error occurs during this process, the function sets the NonAdminBackup status to "BackingOff" -// and updates the corresponding condition accordingly. -// If the BackupSpec is invalid, the function sets the NonAdminBackup condition to "InvalidBackupSpec". -// If the BackupSpec is valid, the function sets the NonAdminBackup condition to "BackupAccepted". -func (r *NonAdminBackupReconciler) ValidateVeleroBackupSpec(ctx context.Context, log logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := log.WithValues("ValidateVeleroBackupSpec", nab.Namespace) - - // Main Validation point for the VeleroBackup included in NonAdminBackup spec - _, err := function.GetBackupSpecFromNonAdminBackup(nab) - - if err != nil { - errMsg := "NonAdminBackup CR does not contain valid BackupSpec" - logger.Error(err, errMsg) - - updatedStatus, errUpdateStatus := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseBackingOff) - if errUpdateStatus != nil { - logger.Error(errUpdateStatus, "Unable to set NonAdminBackup Phase: BackingOff", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdateStatus - } else if updatedStatus { - // We do not requeue - the State was set to BackingOff - return true, false, nil - } - - // Continue. VeleroBackup looks fine, setting Accepted condition - updatedCondition, errUpdateCondition := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionFalse, "InvalidBackupSpec", errMsg) - - if errUpdateCondition != nil { - logger.Error(errUpdateCondition, "Unable to set BackupAccepted Condition: False", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdateCondition - } else if updatedCondition { - return true, false, nil - } - - // We do not requeue - this was an error from getting Spec from NAB - return true, false, err - } - - updatedStatus, errUpdateStatus := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "BackupAccepted", "backup accepted") - if errUpdateStatus != nil { - logger.Error(errUpdateStatus, "Unable to set BackupAccepted Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdateStatus - } else if updatedStatus { - // We do requeue - The VeleroBackup got accepted and next reconcile loop will continue - // with further work on the VeleroBackup such as creating it - return false, true, nil - } - - return false, false, nil -} diff --git a/internal/controller/nab_velero_backupspec.go b/internal/controller/nab_velero_backupspec.go deleted file mode 100644 index 4278d37..0000000 --- a/internal/controller/nab_velero_backupspec.go +++ /dev/null @@ -1,136 +0,0 @@ -/* -Copyright 2024. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller - -import ( - "context" - "errors" - - "github.com/go-logr/logr" - velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - - nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" - "github.com/migtools/oadp-non-admin/internal/common/constant" - "github.com/migtools/oadp-non-admin/internal/common/function" -) - -// CreateVeleroBackupSpec creates or updates a Velero Backup object based on the provided NonAdminBackup object. -// -// Parameters: -// -// ctx: Context for the request. -// log: Logger instance for logging messages. -// nab: Pointer to the NonAdminBackup object. -// -// The function generates a name for the Velero Backup object based on the provided namespace and name. -// It then checks if a Velero Backup object with that name already exists. If it does not exist, it creates a new one. -// The function returns boolean values indicating whether the reconciliation loop should exit or requeue -func (r *NonAdminBackupReconciler) CreateVeleroBackupSpec(ctx context.Context, log logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := log.WithValues("CreateVeleroBackupSpec", nab.Namespace) - - veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name) - - if veleroBackupName == constant.EmptyString { - return true, false, errors.New("unable to generate Velero Backup name") - } - - veleroBackup := velerov1api.Backup{} - err := r.Get(ctx, client.ObjectKey{Namespace: constant.OadpNamespace, Name: veleroBackupName}, &veleroBackup) - - if err != nil && apierrors.IsNotFound(err) { - // Create VeleroBackup - // Don't update phase nor conditions yet. - // Those will be updated when then Reconcile loop is triggered by the VeleroBackup object - logger.Info("No backup found", nameField, veleroBackupName) - - // We don't validate error here. - // This was already validated in the ValidateVeleroBackupSpec - backupSpec, errBackup := function.GetBackupSpecFromNonAdminBackup(nab) - - if errBackup != nil { - // Should never happen as it was already checked - return true, false, errBackup - } - - veleroBackup = velerov1api.Backup{ - ObjectMeta: metav1.ObjectMeta{ - Name: veleroBackupName, - Namespace: constant.OadpNamespace, - }, - Spec: *backupSpec, - } - } else if err != nil && !apierrors.IsNotFound(err) { - logger.Error(err, "Unable to fetch VeleroBackup") - return true, false, err - } else { - // We should not update already created VeleroBackup object. - // The VeleroBackup within NonAdminBackup will - // be reverted back to the previous state - the state which created VeleroBackup - // in a first place, so they will be in sync. - logger.Info("Backup already exists, updating NonAdminBackup status", nameField, veleroBackupName) - updatedNab, errBackupUpdate := function.UpdateNonAdminBackupFromVeleroBackup(ctx, r.Client, logger, nab, &veleroBackup) - // Regardless if the status was updated or not, we should not - // requeue here as it was only status update. - if errBackupUpdate != nil { - return true, false, errBackupUpdate - } else if updatedNab { - logger.V(1).Info("NonAdminBackup CR - Rqueue after Status Update") - return false, true, nil - } - return true, false, nil - } - - // Ensure labels are set for the Backup object - existingLabels := veleroBackup.Labels - naManagedLabels := function.AddNonAdminLabels(existingLabels) - veleroBackup.Labels = naManagedLabels - - // Ensure annotations are set for the Backup object - existingAnnotations := veleroBackup.Annotations - ownerUUID := string(nab.ObjectMeta.UID) - nabManagedAnnotations := function.AddNonAdminBackupAnnotations(nab.Namespace, nab.Name, ownerUUID, existingAnnotations) - veleroBackup.Annotations = nabManagedAnnotations - - _, err = controllerutil.CreateOrPatch(ctx, r.Client, &veleroBackup, nil) - if err != nil { - logger.Error(err, "Failed to create backup", nameField, veleroBackupName) - return true, false, err - } - logger.Info("VeleroBackup successfully created", nameField, veleroBackupName) - - _, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseCreated) - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: Created", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdate - } - _, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "Validated", "Valid Backup config") - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set BackupAccepted Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdate - } - _, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionQueued, metav1.ConditionTrue, "BackupScheduled", "Created Velero Backup object") - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set BackupQueued Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdate - } - - return false, false, nil -} diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 90ada93..279f5d8 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -19,17 +19,22 @@ package controller import ( "context" + "errors" "time" + "github.com/go-logr/logr" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" 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" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" "github.com/migtools/oadp-non-admin/internal/common/constant" + "github.com/migtools/oadp-non-admin/internal/common/function" "github.com/migtools/oadp-non-admin/internal/handler" "github.com/migtools/oadp-non-admin/internal/predicate" ) @@ -106,6 +111,201 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, nil } +// InitNonAdminBackup sets the New Phase on a NonAdminBackup object if it is not already set. +// +// Parameters: +// +// ctx: Context for the request. +// logrLogger: Logger instance for logging messages. +// nab: Pointer to the NonAdminBackup object. +// +// The function checks if the Phase of the NonAdminBackup object is empty. +// If it is empty, it sets the Phase to "New". +// It then returns boolean values indicating whether the reconciliation loop should requeue +// and whether the status was updated. +func (r *NonAdminBackupReconciler) InitNonAdminBackup(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + logger := logrLogger.WithValues("InitNonAdminBackup", nab.Namespace) + // Set initial Phase + if nab.Status.Phase == constant.EmptyString { + // Phase: New + updatedStatus, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseNew) + + if errUpdate != nil { + logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: New", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdate + } + + if updatedStatus { + logger.V(1).Info("NonAdminBackup CR - Requeue after Phase Update") + return false, true, nil + } + } + + return false, false, nil +} + +// ValidateVeleroBackupSpec validates the VeleroBackup Spec from the NonAdminBackup. +// +// Parameters: +// +// ctx: Context for the request. +// logrLogger: Logger instance for logging messages. +// nab: Pointer to the NonAdminBackup object. +// +// The function attempts to get the BackupSpec from the NonAdminBackup object. +// If an error occurs during this process, the function sets the NonAdminBackup status to "BackingOff" +// and updates the corresponding condition accordingly. +// If the BackupSpec is invalid, the function sets the NonAdminBackup condition to "InvalidBackupSpec". +// If the BackupSpec is valid, the function sets the NonAdminBackup condition to "BackupAccepted". +func (r *NonAdminBackupReconciler) ValidateVeleroBackupSpec(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + logger := logrLogger.WithValues("ValidateVeleroBackupSpec", nab.Namespace) + + // Main Validation point for the VeleroBackup included in NonAdminBackup spec + _, err := function.GetBackupSpecFromNonAdminBackup(nab) + + if err != nil { + errMsg := "NonAdminBackup CR does not contain valid BackupSpec" + logger.Error(err, errMsg) + + updatedStatus, errUpdateStatus := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseBackingOff) + if errUpdateStatus != nil { + logger.Error(errUpdateStatus, "Unable to set NonAdminBackup Phase: BackingOff", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdateStatus + } else if updatedStatus { + // We do not requeue - the State was set to BackingOff + return true, false, nil + } + + // Continue. VeleroBackup looks fine, setting Accepted condition + updatedCondition, errUpdateCondition := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionFalse, "InvalidBackupSpec", errMsg) + + if errUpdateCondition != nil { + logger.Error(errUpdateCondition, "Unable to set BackupAccepted Condition: False", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdateCondition + } else if updatedCondition { + return true, false, nil + } + + // We do not requeue - this was an error from getting Spec from NAB + return true, false, err + } + + updatedStatus, errUpdateStatus := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "BackupAccepted", "backup accepted") + if errUpdateStatus != nil { + logger.Error(errUpdateStatus, "Unable to set BackupAccepted Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdateStatus + } else if updatedStatus { + // We do requeue - The VeleroBackup got accepted and next reconcile loop will continue + // with further work on the VeleroBackup such as creating it + return false, true, nil + } + + return false, false, nil +} + +// CreateVeleroBackupSpec creates or updates a Velero Backup object based on the provided NonAdminBackup object. +// +// Parameters: +// +// ctx: Context for the request. +// log: Logger instance for logging messages. +// nab: Pointer to the NonAdminBackup object. +// +// The function generates a name for the Velero Backup object based on the provided namespace and name. +// It then checks if a Velero Backup object with that name already exists. If it does not exist, it creates a new one. +// The function returns boolean values indicating whether the reconciliation loop should exit or requeue +func (r *NonAdminBackupReconciler) CreateVeleroBackupSpec(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + logger := logrLogger.WithValues("CreateVeleroBackupSpec", nab.Namespace) + + veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name) + + if veleroBackupName == constant.EmptyString { + return true, false, errors.New("unable to generate Velero Backup name") + } + + veleroBackup := velerov1api.Backup{} + err := r.Get(ctx, client.ObjectKey{Namespace: constant.OadpNamespace, Name: veleroBackupName}, &veleroBackup) + + if err != nil && apierrors.IsNotFound(err) { + // Create VeleroBackup + // Don't update phase nor conditions yet. + // Those will be updated when then Reconcile loop is triggered by the VeleroBackup object + logger.Info("No backup found", nameField, veleroBackupName) + + // We don't validate error here. + // This was already validated in the ValidateVeleroBackupSpec + backupSpec, errBackup := function.GetBackupSpecFromNonAdminBackup(nab) + + if errBackup != nil { + // Should never happen as it was already checked + return true, false, errBackup + } + + veleroBackup = velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Name: veleroBackupName, + Namespace: constant.OadpNamespace, + }, + Spec: *backupSpec, + } + } else if err != nil && !apierrors.IsNotFound(err) { + logger.Error(err, "Unable to fetch VeleroBackup") + return true, false, err + } else { + // We should not update already created VeleroBackup object. + // The VeleroBackup within NonAdminBackup will + // be reverted back to the previous state - the state which created VeleroBackup + // in a first place, so they will be in sync. + logger.Info("Backup already exists, updating NonAdminBackup status", nameField, veleroBackupName) + updatedNab, errBackupUpdate := function.UpdateNonAdminBackupFromVeleroBackup(ctx, r.Client, logger, nab, &veleroBackup) + // Regardless if the status was updated or not, we should not + // requeue here as it was only status update. + if errBackupUpdate != nil { + return true, false, errBackupUpdate + } else if updatedNab { + logger.V(1).Info("NonAdminBackup CR - Rqueue after Status Update") + return false, true, nil + } + return true, false, nil + } + + // Ensure labels are set for the Backup object + existingLabels := veleroBackup.Labels + naManagedLabels := function.AddNonAdminLabels(existingLabels) + veleroBackup.Labels = naManagedLabels + + // Ensure annotations are set for the Backup object + existingAnnotations := veleroBackup.Annotations + ownerUUID := string(nab.ObjectMeta.UID) + nabManagedAnnotations := function.AddNonAdminBackupAnnotations(nab.Namespace, nab.Name, ownerUUID, existingAnnotations) + veleroBackup.Annotations = nabManagedAnnotations + + _, err = controllerutil.CreateOrPatch(ctx, r.Client, &veleroBackup, nil) + if err != nil { + logger.Error(err, "Failed to create backup", nameField, veleroBackupName) + return true, false, err + } + logger.Info("VeleroBackup successfully created", nameField, veleroBackupName) + + _, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseCreated) + if errUpdate != nil { + logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: Created", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdate + } + _, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "Validated", "Valid Backup config") + if errUpdate != nil { + logger.Error(errUpdate, "Unable to set BackupAccepted Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdate + } + _, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionQueued, metav1.ConditionTrue, "BackupScheduled", "Created Velero Backup object") + if errUpdate != nil { + logger.Error(errUpdate, "Unable to set BackupQueued Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) + return true, false, errUpdate + } + + return false, false, nil +} + // SetupWithManager sets up the controller with the Manager. func (r *NonAdminBackupReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr).