From 0d30f215b487eb522cf51ecc9e9d478e77c5c602 Mon Sep 17 00:00:00 2001 From: Mateus Oliveira Date: Thu, 3 Oct 2024 04:02:43 -0300 Subject: [PATCH] fix: Update NAB status phase and condition together (#86) Signed-off-by: Mateus Oliveira --- internal/common/constant/constant.go | 5 +- internal/common/function/function.go | 59 +++++++- internal/common/function/function_test.go | 142 ++++++++++++++---- .../controller/nonadminbackup_controller.go | 74 ++++----- .../nonadminbackup_controller_test.go | 78 +++------- internal/handler/velerobackup_handler.go | 51 ++----- internal/predicate/composite_predicate.go | 28 +--- .../predicate/nonadminbackup_predicate.go | 78 ++-------- internal/predicate/velerobackup_predicate.go | 51 ++----- 9 files changed, 267 insertions(+), 299 deletions(-) diff --git a/internal/common/constant/constant.go b/internal/common/constant/constant.go index b5a6dec..c7a41a8 100644 --- a/internal/common/constant/constant.go +++ b/internal/common/constant/constant.go @@ -23,6 +23,7 @@ package constant // of the specific Object, such as Backup/Restore. const ( OadpLabel = "openshift.io/oadp" // TODO import? + OadpLabelValue = TrueString ManagedByLabel = "app.kubernetes.io/managed-by" ManagedByLabelValue = "oadp-nac-controller" // TODO why not use same project name as in PROJECT file? NabOriginNameAnnotation = "openshift.io/oadp-nab-origin-name" @@ -38,8 +39,8 @@ const ( // EmptyString defines a constant for the empty string const EmptyString = "" -// MaxKubernetesNameLength represents maximum length of the name in k8s -const MaxKubernetesNameLength = 253 +// TrueString defines a constant for the True string +const TrueString = "True" // VeleroBackupNamePrefix represents the prefix for the object name generated // by the NonAdminController diff --git a/internal/common/function/function.go b/internal/common/function/function.go index a321e6d..583210b 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -23,8 +23,10 @@ import ( "encoding/hex" "fmt" + "github.com/go-logr/logr" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -38,7 +40,7 @@ const requiredAnnotationError = "backup does not have the required annotation '% // If error occurs, a map with only the default Non Admin labels is returned func AddNonAdminLabels(labels map[string]string) map[string]string { defaultLabels := map[string]string{ - constant.OadpLabel: "True", + constant.OadpLabel: constant.OadpLabelValue, constant.ManagedByLabel: constant.ManagedByLabelValue, } @@ -125,9 +127,9 @@ func GenerateVeleroBackupName(namespace, nabName string) string { veleroBackupName := fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace, nameHash) // Ensure the name is within the character limit - if len(veleroBackupName) > constant.MaxKubernetesNameLength { + if len(veleroBackupName) > validation.DNS1123SubdomainMaxLength { // Truncate the namespace if necessary - maxNamespaceLength := constant.MaxKubernetesNameLength - len(nameHash) - prefixLength + maxNamespaceLength := validation.DNS1123SubdomainMaxLength - len(nameHash) - prefixLength if len(namespace) > maxNamespaceLength { namespace = namespace[:maxNamespaceLength] } @@ -137,11 +139,46 @@ func GenerateVeleroBackupName(namespace, nabName string) string { return veleroBackupName } -// CheckVeleroBackupLabels return true if Velero Backup object has required Non Admin labels, false otherwise -func CheckVeleroBackupLabels(labels map[string]string) bool { - // TODO also need to check for constant.OadpLabel label? - value, exists := labels[constant.ManagedByLabel] - return exists && value == constant.ManagedByLabelValue +// CheckVeleroBackupMetadata return true if Velero Backup object has required Non Admin labels and annotations, false otherwise +func CheckVeleroBackupMetadata(obj client.Object) bool { + labels := obj.GetLabels() + if !checkLabelValue(labels, constant.OadpLabel, constant.OadpLabelValue) { + return false + } + if !checkLabelValue(labels, constant.ManagedByLabel, constant.ManagedByLabelValue) { + return false + } + + annotations := obj.GetAnnotations() + if !checkAnnotationValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) { + return false + } + if !checkAnnotationValueIsValid(annotations, constant.NabOriginNameAnnotation) { + return false + } + // TODO what is a valid uuid? + if !checkAnnotationValueIsValid(annotations, constant.NabOriginUUIDAnnotation) { + return false + } + + return true +} + +func checkLabelValue(labels map[string]string, key string, value string) bool { + got, exists := labels[key] + if !exists { + return false + } + return got == value +} + +func checkAnnotationValueIsValid(annotations map[string]string, key string) bool { + value, exists := annotations[key] + if !exists { + return false + } + length := len(value) + return length > 0 && length < validation.DNS1123SubdomainMaxLength } // TODO not used @@ -217,3 +254,9 @@ func mergeMaps[T comparable](maps ...map[T]T) (map[T]T, error) { } return merge, nil } + +// GetLogger return a logger from input ctx, with additional key/value pairs being +// input key and input obj name and namespace +func GetLogger(ctx context.Context, obj client.Object, key string) logr.Logger { + return log.FromContext(ctx).WithValues(key, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}) +} diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index a0fd10f..ce02091 100644 --- a/internal/common/function/function_test.go +++ b/internal/common/function/function_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "reflect" + "strings" "testing" "github.com/onsi/ginkgo/v2" @@ -27,6 +28,7 @@ import ( velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -38,6 +40,12 @@ import ( var _ = ginkgo.Describe("PLACEHOLDER", func() {}) +const ( + testNonAdminBackupNamespace = "non-admin-backup-namespace" + testNonAdminBackupName = "non-admin-backup-name" + testNonAdminBackupUUID = "12345678-1234-1234-1234-123456789abc" +) + func TestMergeMaps(t *testing.T) { const ( d = "d" @@ -294,18 +302,18 @@ func TestGetNonAdminBackupFromVeleroBackup(t *testing.T) { Namespace: "test-namespace", Name: "test-backup", Annotations: map[string]string{ - constant.NabOriginNamespaceAnnotation: "non-admin-backup-namespace", - constant.NabOriginNameAnnotation: "non-admin-backup-name", - constant.NabOriginUUIDAnnotation: "12345678-1234-1234-1234-123456789abc", + constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, + constant.NabOriginNameAnnotation: testNonAdminBackupName, + constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, }, }, } nonAdminBackup := &nacv1alpha1.NonAdminBackup{ ObjectMeta: metav1.ObjectMeta{ - Namespace: "non-admin-backup-namespace", - Name: "non-admin-backup-name", - UID: types.UID("12345678-1234-1234-1234-123456789abc"), + Namespace: testNonAdminBackupNamespace, + Name: testNonAdminBackupName, + UID: types.UID(testNonAdminBackupUUID), }, } @@ -317,32 +325,110 @@ func TestGetNonAdminBackupFromVeleroBackup(t *testing.T) { assert.Equal(t, nonAdminBackup, result, "Returned NonAdminBackup should match expected NonAdminBackup") } -func TestCheckVeleroBackupLabels(t *testing.T) { - // Backup has the required label - backupWithLabel := &velerov1.Backup{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - constant.ManagedByLabel: constant.ManagedByLabelValue, +func TestCheckVeleroBackupMetadata(t *testing.T) { + tests := []struct { + backup *velerov1.Backup + name string + expected bool + }{ + { + name: "Velero Backup without required non admin labels and annotations", + backup: &velerov1.Backup{}, + expected: false, + }, + { + name: "Velero Backup without required non admin annotations", + backup: &velerov1.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + }, + }, }, + expected: false, }, - } - assert.True(t, CheckVeleroBackupLabels(backupWithLabel.GetLabels()), "Expected backup to have required label") - - // Backup does not have the required label - backupWithoutLabel := &velerov1.Backup{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{}, + { + name: "Velero Backup with wrong required non admin label", + backup: &velerov1.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: "foo", + }, + }, + }, + expected: false, }, - } - assert.False(t, CheckVeleroBackupLabels(backupWithoutLabel.GetLabels()), "Expected backup to not have required label") - - // Backup has the required label with incorrect value - backupWithIncorrectValue := &velerov1.Backup{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - constant.ManagedByLabel: "incorrect-value", + { + name: "Velero Backup without required non admin labels", + backup: &velerov1.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, + constant.NabOriginNameAnnotation: testNonAdminBackupName, + constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, + }, + }, + }, + expected: false, + }, + { + name: "Velero Backup with wrong required non admin annotation [empty]", + backup: &velerov1.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + }, + Annotations: map[string]string{ + constant.NabOriginNamespaceAnnotation: constant.EmptyString, + constant.NabOriginNameAnnotation: testNonAdminBackupName, + constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, + }, + }, + }, + expected: false, + }, + { + name: "Velero Backup with wrong required non admin annotation [long]", + backup: &velerov1.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + }, + Annotations: map[string]string{ + constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, + constant.NabOriginNameAnnotation: strings.Repeat("nn", validation.DNS1123SubdomainMaxLength), + constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, + }, + }, }, + expected: false, }, + { + name: "Velero Backup with required non admin labels and annotations", + backup: &velerov1.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + }, + Annotations: map[string]string{ + constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, + constant.NabOriginNameAnnotation: testNonAdminBackupName, + constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, + }, + }, + }, + expected: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := CheckVeleroBackupMetadata(test.backup) + assert.Equal(t, test.expected, result) + }) } - assert.False(t, CheckVeleroBackupLabels(backupWithIncorrectValue.GetLabels()), "Expected backup to not have required label") } diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index a084e18..b64a7cd 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -51,8 +51,7 @@ type NonAdminBackupReconciler struct { const ( phaseUpdateRequeue = "NonAdminBackup - Requeue after Phase Update" conditionUpdateRequeue = "NonAdminBackup - Requeue after Condition Update" - phaseUpdateError = "Failed to update NonAdminBackup Phase" - conditionUpdateError = "Failed to update NonAdminBackup Condition" + statusUpdateError = "Failed to update NonAdminBackup Status" ) // +kubebuilder:rbac:groups=nac.oadp.openshift.io,resources=nonadminbackups,verbs=get;list;watch;create;update;patch;delete @@ -130,7 +129,7 @@ func (r *NonAdminBackupReconciler) Init(ctx context.Context, logrLogger logr.Log updated := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseNew) if updated { if err := r.Status().Update(ctx, nab); err != nil { - logger.Error(err, phaseUpdateError) + logger.Error(err, statusUpdateError) return true, false, err } @@ -161,18 +160,8 @@ func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger // Main Validation point for the VeleroBackup included in NonAdminBackup spec _, err := function.GetBackupSpecFromNonAdminBackup(nab) if err != nil { - updated := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseBackingOff) - if updated { - if updateErr := r.Status().Update(ctx, nab); updateErr != nil { - logger.Error(updateErr, phaseUpdateError) - return true, false, updateErr - } - - logger.V(1).Info(phaseUpdateRequeue) - return false, true, nil - } - - updated = meta.SetStatusCondition(&nab.Status.Conditions, + updatedPhase := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseBackingOff) + updatedCondition := meta.SetStatusCondition(&nab.Status.Conditions, metav1.Condition{ Type: string(nacv1alpha1.NonAdminConditionAccepted), Status: metav1.ConditionFalse, @@ -180,9 +169,9 @@ func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger Message: err.Error(), }, ) - if updated { + if updatedPhase || updatedCondition { if updateErr := r.Status().Update(ctx, nab); updateErr != nil { - logger.Error(updateErr, conditionUpdateError) + logger.Error(updateErr, statusUpdateError) return true, false, updateErr } } @@ -201,7 +190,7 @@ func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger ) if updated { if err := r.Status().Update(ctx, nab); err != nil { - logger.Error(err, conditionUpdateError) + logger.Error(err, statusUpdateError) return true, false, err } @@ -223,7 +212,7 @@ func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger // ctx: Context for the request. // logrLogger: Logger instance for logging messages. // nab: Pointer to the NonAdminBackup object. -func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { +func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { //nolint:unparam // will address in https://github.com/migtools/oadp-non-admin/issues/62 logger := logrLogger veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name) @@ -277,18 +266,8 @@ func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx contex veleroBackupLogger.Info("VeleroBackup successfully created") } - updated := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseCreated) - if updated { - if err := r.Status().Update(ctx, nab); err != nil { - logger.Error(err, phaseUpdateError) - return true, false, err - } - - logger.V(1).Info(phaseUpdateRequeue) - return false, true, nil - } - - updated = meta.SetStatusCondition(&nab.Status.Conditions, + updatedPhase := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseCreated) + updatedCondition := meta.SetStatusCondition(&nab.Status.Conditions, metav1.Condition{ Type: string(nacv1alpha1.NonAdminConditionQueued), Status: metav1.ConditionTrue, @@ -296,21 +275,22 @@ func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx contex Message: "Created Velero Backup object", }, ) - if updated { + updatedReference := updateNonAdminBackupVeleroBackupReference(&nab.Status, &veleroBackup) + if updatedPhase || updatedCondition || updatedReference { if err := r.Status().Update(ctx, nab); err != nil { - logger.Error(err, conditionUpdateError) + logger.Error(err, statusUpdateError) return true, false, err } - logger.V(1).Info(conditionUpdateRequeue) - return false, true, nil + logger.V(1).Info("NonAdminBackup - Exit after Status Update") + return false, false, nil } // Ensure that the NonAdminBackup's NonAdminBackupStatus is in sync // with the VeleroBackup. Any required updates to the NonAdminBackup // Status will be applied based on the current state of the VeleroBackup. veleroBackupLogger.Info("VeleroBackup already exists, verifying if NonAdminBackup Status requires update") - updated = updateNonAdminBackupVeleroBackupStatus(&nab.Status, &veleroBackup) + updated := updateNonAdminBackupVeleroBackupStatus(&nab.Status, &veleroBackup) if updated { if err := r.Status().Update(ctx, nab); err != nil { veleroBackupLogger.Error(err, "Failed to update NonAdminBackup Status after VeleroBackup reconciliation") @@ -327,13 +307,14 @@ func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx contex func (r *NonAdminBackupReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&nacv1alpha1.NonAdminBackup{}). - Watches(&velerov1.Backup{}, &handler.VeleroBackupHandler{}). WithEventFilter(predicate.CompositePredicate{ NonAdminBackupPredicate: predicate.NonAdminBackupPredicate{}, VeleroBackupPredicate: predicate.VeleroBackupPredicate{ - OadpVeleroNamespace: r.OADPNamespace, + OADPNamespace: r.OADPNamespace, }, }). + // handler runs after predicate + Watches(&velerov1.Backup{}, &handler.VeleroBackupHandler{}). Complete(r) } @@ -353,14 +334,23 @@ func updateNonAdminPhase(phase *nacv1alpha1.NonAdminBackupPhase, newPhase nacv1a return true } -// updateNonAdminBackupVeleroBackupStatus sets the VeleroBackup fields in NonAdminBackup object status and returns true +// updateNonAdminBackupVeleroBackupStatus sets the VeleroBackup reference fields in NonAdminBackup object status and returns true // if the VeleroBackup fields are changed by this call. -func updateNonAdminBackupVeleroBackupStatus(status *nacv1alpha1.NonAdminBackupStatus, veleroBackup *velerov1.Backup) bool { - if !reflect.DeepEqual(status.VeleroBackupStatus, &veleroBackup.Status) || status.VeleroBackupName != veleroBackup.Name || status.VeleroBackupNamespace != veleroBackup.Namespace { - status.VeleroBackupStatus = veleroBackup.Status.DeepCopy() +func updateNonAdminBackupVeleroBackupReference(status *nacv1alpha1.NonAdminBackupStatus, veleroBackup *velerov1.Backup) bool { + if status.VeleroBackupName != veleroBackup.Name || status.VeleroBackupNamespace != veleroBackup.Namespace { status.VeleroBackupName = veleroBackup.Name status.VeleroBackupNamespace = veleroBackup.Namespace return true } return false } + +// updateNonAdminBackupVeleroBackupStatus sets the VeleroBackup status field in NonAdminBackup object status and returns true +// if the VeleroBackup fields are changed by this call. +func updateNonAdminBackupVeleroBackupStatus(status *nacv1alpha1.NonAdminBackupStatus, veleroBackup *velerov1.Backup) bool { + if !reflect.DeepEqual(status.VeleroBackupStatus, &veleroBackup.Status) { + status.VeleroBackupStatus = veleroBackup.Status.DeepCopy() + return true + } + return false +} diff --git a/internal/controller/nonadminbackup_controller_test.go b/internal/controller/nonadminbackup_controller_test.go index ba1dbd8..fe06d6b 100644 --- a/internal/controller/nonadminbackup_controller_test.go +++ b/internal/controller/nonadminbackup_controller_test.go @@ -211,8 +211,10 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func if scenario.createVeleroBackup { veleroBackup := &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ - Name: function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName), - Namespace: oadpNamespaceName, + Name: function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName), + Namespace: oadpNamespaceName, + Labels: function.AddNonAdminLabels(nil), + Annotations: function.AddNonAdminBackupAnnotations(nonAdminNamespaceName, testNonAdminBackupName, "", nil), }, Spec: velerov1.BackupSpec{ IncludedNamespaces: []string{nonAdminNamespaceName}, @@ -296,7 +298,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, result: reconcile.Result{Requeue: true}, }), - ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True), should update NonAdminBackup phase to created and Requeue", nonAdminBackupSingleReconcileScenario{ + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True), should update NonAdminBackup phase to created and Condition to Queued True and VeleroBackup reference and Exit", nonAdminBackupSingleReconcileScenario{ spec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{}, }, @@ -313,39 +315,9 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ - // TODO should not have VeleroBackupName and VeleroBackupNamespace? - Phase: nacv1alpha1.NonAdminBackupPhaseCreated, - Conditions: []metav1.Condition{ - { - Type: "Accepted", - Status: metav1.ConditionTrue, - Reason: "BackupAccepted", - Message: "backup accepted", - }, - }, - }, - result: reconcile.Result{Requeue: true}, - }), - ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase created; Conditions Accepted True), should update NonAdminBackup Condition to Queued True and Requeue", nonAdminBackupSingleReconcileScenario{ - createVeleroBackup: true, - spec: nacv1alpha1.NonAdminBackupSpec{ - BackupSpec: &velerov1.BackupSpec{}, - }, - priorStatus: &nacv1alpha1.NonAdminBackupStatus{ - Phase: nacv1alpha1.NonAdminBackupPhaseCreated, - Conditions: []metav1.Condition{ - { - Type: "Accepted", - Status: metav1.ConditionTrue, - Reason: "BackupAccepted", - Message: "backup accepted", - LastTransitionTime: metav1.NewTime(time.Now()), - }, - }, - }, - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ - // TODO should not have VeleroBackupName and VeleroBackupNamespace? - Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + VeleroBackupName: placeholder, + VeleroBackupNamespace: placeholder, Conditions: []metav1.Condition{ { Type: "Accepted", @@ -361,15 +333,17 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, }, - result: reconcile.Result{Requeue: true}, + result: reconcile.Result{}, }), - ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase created; Conditions Accepted True, Queued True), should update NonAdminBackup VeleroBackupStatus and Requeue", nonAdminBackupSingleReconcileScenario{ + ginkgo.Entry("When triggered by VeleroBackup Update event, should update NonAdminBackup VeleroBackupStatus and Exit", nonAdminBackupSingleReconcileScenario{ createVeleroBackup: true, spec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{}, }, priorStatus: &nacv1alpha1.NonAdminBackupStatus{ - Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + VeleroBackupName: placeholder, + VeleroBackupNamespace: placeholder, Conditions: []metav1.Condition{ { Type: "Accepted", @@ -407,9 +381,9 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, }, - result: reconcile.Result{Requeue: false}, + result: reconcile.Result{}, }), - ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new) [invalid spec], should update NonAdminBackup phase to BackingOff and Requeue", nonAdminBackupSingleReconcileScenario{ + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new) [invalid spec], should update NonAdminBackup phase to BackingOff and Condition to Accepted False and Exit with terminal error", nonAdminBackupSingleReconcileScenario{ spec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{ IncludedNamespaces: []string{"not-valid"}, @@ -418,21 +392,6 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func priorStatus: &nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, }, - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ - Phase: nacv1alpha1.NonAdminBackupPhaseBackingOff, - }, - result: reconcile.Result{Requeue: true}, - }), - ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase BackingOff), should update NonAdminBackup Condition to Accepted False and stop with terminal error", nonAdminBackupSingleReconcileScenario{ - // TODO this validates spec again... - spec: nacv1alpha1.NonAdminBackupSpec{ - BackupSpec: &velerov1.BackupSpec{ - IncludedNamespaces: []string{"not-valid"}, - }, - }, - priorStatus: &nacv1alpha1.NonAdminBackupStatus{ - Phase: nacv1alpha1.NonAdminBackupPhaseBackingOff, - }, ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseBackingOff, Conditions: []metav1.Condition{ @@ -472,7 +431,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", time.Sleep(1 * time.Second) }) - ginkgo.DescribeTable("full reconcile loop", + ginkgo.DescribeTable("Reconcile triggered by NonAdminBackup Create event", func(scenario nonAdminBackupFullReconcileScenario) { updateTestScenario() @@ -550,6 +509,9 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", if err != nil { return false, err } + if nonAdminBackup.Status.VeleroBackupStatus == nil { + return false, nil + } return nonAdminBackup.Status.VeleroBackupStatus.Phase == velerov1.BackupPhaseCompleted, nil }, 5*time.Second, 1*time.Second).Should(gomega.BeTrue()) } @@ -566,7 +528,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", Phase: nacv1alpha1.NonAdminBackupPhaseCreated, VeleroBackupName: placeholder, VeleroBackupNamespace: placeholder, - VeleroBackupStatus: &velerov1.BackupStatus{}, + VeleroBackupStatus: nil, Conditions: []metav1.Condition{ { Type: "Accepted", diff --git a/internal/handler/velerobackup_handler.go b/internal/handler/velerobackup_handler.go index ce484b9..3a4aa0b 100644 --- a/internal/handler/velerobackup_handler.go +++ b/internal/handler/velerobackup_handler.go @@ -20,71 +20,44 @@ package handler import ( "context" - "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/migtools/oadp-non-admin/internal/common/constant" + "github.com/migtools/oadp-non-admin/internal/common/function" ) // VeleroBackupHandler contains event handlers for Velero Backup objects -type VeleroBackupHandler struct { - Logger logr.Logger -} - -func getVeleroBackupHandlerLogger(ctx context.Context, name, namespace string) logr.Logger { - return log.FromContext(ctx).WithValues("VeleroBackupHandler", types.NamespacedName{Name: name, Namespace: namespace}) -} +type VeleroBackupHandler struct{} // Create event handler -func (*VeleroBackupHandler) Create(ctx context.Context, evt event.CreateEvent, _ workqueue.RateLimitingInterface) { - namespace := evt.Object.GetNamespace() - name := evt.Object.GetName() - logger := getVeleroBackupHandlerLogger(ctx, name, namespace) - logger.V(1).Info("Received Create VeleroBackupHandler") +func (VeleroBackupHandler) Create(_ context.Context, _ event.CreateEvent, _ workqueue.RateLimitingInterface) { + // Create event handler for the Backup object } -// Update event handler -func (*VeleroBackupHandler) Update(ctx context.Context, evt event.UpdateEvent, q workqueue.RateLimitingInterface) { - namespace := evt.ObjectNew.GetNamespace() - name := evt.ObjectNew.GetName() - logger := getVeleroBackupHandlerLogger(ctx, name, namespace) - logger.V(1).Info("Received Update VeleroBackupHandler") +// Update event handler adds Velero Backup's NonAdminBackup to controller queue +func (VeleroBackupHandler) Update(ctx context.Context, evt event.UpdateEvent, q workqueue.RateLimitingInterface) { + logger := function.GetLogger(ctx, evt.ObjectNew, "VeleroBackupHandler") annotations := evt.ObjectNew.GetAnnotations() - - if annotations == nil { - logger.V(1).Info("Backup annotations not found") - return - } - - nabOriginNamespace, ok := annotations[constant.NabOriginNamespaceAnnotation] - if !ok { - logger.V(1).Info("Backup NonAdminBackup origin namespace annotation not found") - return - } - - nabOriginName, ok := annotations[constant.NabOriginNameAnnotation] - if !ok { - logger.V(1).Info("Backup NonAdminBackup origin name annotation not found") - return - } + nabOriginNamespace := annotations[constant.NabOriginNamespaceAnnotation] + nabOriginName := annotations[constant.NabOriginNameAnnotation] q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ Name: nabOriginName, Namespace: nabOriginNamespace, }}) + logger.V(1).Info("Handled Update event") } // Delete event handler -func (*VeleroBackupHandler) Delete(_ context.Context, _ event.DeleteEvent, _ workqueue.RateLimitingInterface) { +func (VeleroBackupHandler) Delete(_ context.Context, _ event.DeleteEvent, _ workqueue.RateLimitingInterface) { // Delete event handler for the Backup object } // Generic event handler -func (*VeleroBackupHandler) Generic(_ context.Context, _ event.GenericEvent, _ workqueue.RateLimitingInterface) { +func (VeleroBackupHandler) Generic(_ context.Context, _ event.GenericEvent, _ workqueue.RateLimitingInterface) { // Generic event handler for the Backup object } diff --git a/internal/predicate/composite_predicate.go b/internal/predicate/composite_predicate.go index bec739d..19e2499 100644 --- a/internal/predicate/composite_predicate.go +++ b/internal/predicate/composite_predicate.go @@ -26,29 +26,24 @@ import ( nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" ) -// CompositePredicate is a combination of all project event filters +// CompositePredicate is a combination of NonAdminBackup and Velero Backup event filters type CompositePredicate struct { Context context.Context NonAdminBackupPredicate NonAdminBackupPredicate VeleroBackupPredicate VeleroBackupPredicate } -// Create event filter +// Create event filter only accepts NonAdminBackup create events func (p CompositePredicate) Create(evt event.CreateEvent) bool { switch evt.Object.(type) { case *nacv1alpha1.NonAdminBackup: - // Apply NonAdminBackupPredicate return p.NonAdminBackupPredicate.Create(p.Context, evt) - case *velerov1.Backup: - // Apply VeleroBackupPredicate - return p.VeleroBackupPredicate.Create(p.Context, evt) default: - // Unknown object type, return false return false } } -// Update event filter +// Update event filter accepts both NonAdminBackup and Velero Backup update events func (p CompositePredicate) Update(evt event.UpdateEvent) bool { switch evt.ObjectNew.(type) { case *nacv1alpha1.NonAdminBackup: @@ -60,26 +55,17 @@ func (p CompositePredicate) Update(evt event.UpdateEvent) bool { } } -// Delete event filter +// Delete event filter only accepts NonAdminBackup delete events func (p CompositePredicate) Delete(evt event.DeleteEvent) bool { switch evt.Object.(type) { case *nacv1alpha1.NonAdminBackup: return p.NonAdminBackupPredicate.Delete(p.Context, evt) - case *velerov1.Backup: - return p.VeleroBackupPredicate.Delete(p.Context, evt) default: return false } } -// Generic event filter -func (p CompositePredicate) Generic(evt event.GenericEvent) bool { - switch evt.Object.(type) { - case *nacv1alpha1.NonAdminBackup: - return p.NonAdminBackupPredicate.Generic(p.Context, evt) - case *velerov1.Backup: - return p.VeleroBackupPredicate.Generic(p.Context, evt) - default: - return false - } +// Generic event filter does not accept any generic events +func (CompositePredicate) Generic(_ event.GenericEvent) bool { + return false } diff --git a/internal/predicate/nonadminbackup_predicate.go b/internal/predicate/nonadminbackup_predicate.go index 6597de5..c4d001c 100644 --- a/internal/predicate/nonadminbackup_predicate.go +++ b/internal/predicate/nonadminbackup_predicate.go @@ -19,86 +19,40 @@ package predicate import ( "context" - "github.com/go-logr/logr" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/event" - "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" ) -// NonAdminBackupPredicate contains event filters for Non Admin Backup objects -type NonAdminBackupPredicate struct { - Logger logr.Logger -} +const nonAdminBackupPredicateKey = "NonAdminBackupPredicate" -func getNonAdminBackupPredicateLogger(ctx context.Context, name, namespace string) logr.Logger { - return log.FromContext(ctx).WithValues("NonAdminBackupPredicate", types.NamespacedName{Name: name, Namespace: namespace}) -} +// NonAdminBackupPredicate contains event filters for Non Admin Backup objects +type NonAdminBackupPredicate struct{} -// Create event filter +// Create event filter accepts all NonAdminBackup create events func (NonAdminBackupPredicate) Create(ctx context.Context, evt event.CreateEvent) bool { - namespace := evt.Object.GetNamespace() - name := evt.Object.GetName() - logger := getNonAdminBackupPredicateLogger(ctx, name, namespace) - logger.V(1).Info("NonAdminBackupPredicate: Received Create event") - if nonAdminBackup, ok := evt.Object.(*nacv1alpha1.NonAdminBackup); ok { - if nonAdminBackup.Status.Phase == constant.EmptyString || nonAdminBackup.Status.Phase == nacv1alpha1.NonAdminBackupPhaseNew { - logger.V(1).Info("NonAdminBackupPredicate: Accepted Create event") - return true - } - } - logger.V(1).Info("NonAdminBackupPredicate: Rejecting Create event") - return false + logger := function.GetLogger(ctx, evt.Object, nonAdminBackupPredicateKey) + logger.V(1).Info("Accepted Create event") + return true } -// Update event filter +// Update event filter only accepts NonAdminBackup update events that include spec change func (NonAdminBackupPredicate) Update(ctx context.Context, evt event.UpdateEvent) bool { - namespace := evt.ObjectNew.GetNamespace() - name := evt.ObjectNew.GetName() - logger := getNonAdminBackupPredicateLogger(ctx, name, namespace) - logger.V(1).Info("NonAdminBackupPredicate: Received Update event") + logger := function.GetLogger(ctx, evt.ObjectNew, nonAdminBackupPredicateKey) + // spec change if evt.ObjectNew.GetGeneration() != evt.ObjectOld.GetGeneration() { - logger.V(1).Info("NonAdminBackupPredicate: Accepted Update event - generation change") + logger.V(1).Info("Accepted Update event") return true } - if nonAdminBackupOld, ok := evt.ObjectOld.(*nacv1alpha1.NonAdminBackup); ok { - if nonAdminBackupNew, ok := evt.ObjectNew.(*nacv1alpha1.NonAdminBackup); ok { - oldPhase := nonAdminBackupOld.Status.Phase - newPhase := nonAdminBackupNew.Status.Phase - - // New phase set, reconcile - if oldPhase == constant.EmptyString && newPhase != constant.EmptyString { - logger.V(1).Info("NonAdminBackupPredicate: Accepted Update event - phase change") - return true - } else if oldPhase == nacv1alpha1.NonAdminBackupPhaseNew && newPhase == nacv1alpha1.NonAdminBackupPhaseCreated { - logger.V(1).Info("NonAdminBackupPredicate: Accepted Update event - phase created") - return true - } - } - } - logger.V(1).Info("NonAdminBackupPredicate: Rejecting Update event") - + logger.V(1).Info("Rejected Update event") return false } -// Delete event filter +// Delete event filter accepts all NonAdminBackup delete events func (NonAdminBackupPredicate) Delete(ctx context.Context, evt event.DeleteEvent) bool { - namespace := evt.Object.GetNamespace() - name := evt.Object.GetName() - logger := getNonAdminBackupPredicateLogger(ctx, name, namespace) - logger.V(1).Info("NonAdminBackupPredicate: Accepted Delete event") - return true -} - -// Generic event filter -func (NonAdminBackupPredicate) Generic(ctx context.Context, evt event.GenericEvent) bool { - namespace := evt.Object.GetNamespace() - name := evt.Object.GetName() - logger := getNonAdminBackupPredicateLogger(ctx, name, namespace) - logger.V(1).Info("NonAdminBackupPredicate: Accepted Generic event") + logger := function.GetLogger(ctx, evt.Object, nonAdminBackupPredicateKey) + logger.V(1).Info("Accepted Delete event") return true } diff --git a/internal/predicate/velerobackup_predicate.go b/internal/predicate/velerobackup_predicate.go index 22da1d3..55ecf93 100644 --- a/internal/predicate/velerobackup_predicate.go +++ b/internal/predicate/velerobackup_predicate.go @@ -19,56 +19,29 @@ package predicate import ( "context" - "github.com/go-logr/logr" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/log" "github.com/migtools/oadp-non-admin/internal/common/function" ) // VeleroBackupPredicate contains event filters for Velero Backup objects type VeleroBackupPredicate struct { - // We are watching only Velero Backup objects within - // namespace where OADP is. - OadpVeleroNamespace string - Logger logr.Logger + OADPNamespace string } -// TODO try to remove calls to get logger functions, try to initialize it -func getBackupPredicateLogger(ctx context.Context, name, namespace string) logr.Logger { - return log.FromContext(ctx).WithValues("VeleroBackupPredicate", types.NamespacedName{Name: name, Namespace: namespace}) -} - -// Create event filter -func (veleroBackupPredicate VeleroBackupPredicate) Create(ctx context.Context, evt event.CreateEvent) bool { - namespace := evt.Object.GetNamespace() - if namespace != veleroBackupPredicate.OadpVeleroNamespace { - return false - } - - name := evt.Object.GetName() - logger := getBackupPredicateLogger(ctx, name, namespace) - logger.V(1).Info("VeleroBackupPredicate: Received Create event") - - return function.CheckVeleroBackupLabels(evt.Object.GetLabels()) -} +// Update event filter only accepts Velero Backup update events from OADP namespace +// and from Velero Backups that have required metadata +func (p VeleroBackupPredicate) Update(ctx context.Context, evt event.UpdateEvent) bool { + logger := function.GetLogger(ctx, evt.ObjectNew, "VeleroBackupPredicate") -// Update event filter -func (veleroBackupPredicate VeleroBackupPredicate) Update(ctx context.Context, evt event.UpdateEvent) bool { namespace := evt.ObjectNew.GetNamespace() - name := evt.ObjectNew.GetName() - logger := getBackupPredicateLogger(ctx, name, namespace) - logger.V(1).Info("VeleroBackupPredicate: Received Update event") - return namespace == veleroBackupPredicate.OadpVeleroNamespace -} - -// Delete event filter -func (VeleroBackupPredicate) Delete(_ context.Context, _ event.DeleteEvent) bool { - return false -} + if namespace == p.OADPNamespace { + if function.CheckVeleroBackupMetadata(evt.ObjectNew) { + logger.V(1).Info("Accepted Update event") + return true + } + } -// Generic event filter -func (VeleroBackupPredicate) Generic(_ context.Context, _ event.GenericEvent) bool { + logger.V(1).Info("Rejected Update event") return false }