From 7d9cd21bee23af84c04de9b1673f020aafc596c6 Mon Sep 17 00:00:00 2001 From: Sachin Singla Date: Tue, 6 Feb 2024 19:05:25 +0530 Subject: [PATCH] fix #1128: Removed multiple DPA get calls for Reconcilers --- controllers/bsl.go | 6 +----- controllers/bsl_test.go | 9 +++++++-- controllers/dpa_controller.go | 9 ++++----- controllers/monitor.go | 7 +------ controllers/nodeagent.go | 16 +++------------- controllers/nodeagent_test.go | 3 ++- controllers/registry.go | 31 +++++-------------------------- controllers/validator.go | 7 +------ controllers/validator_test.go | 2 +- controllers/velero.go | 9 ++------- controllers/vsl.go | 14 ++------------ controllers/vsl_test.go | 2 +- 12 files changed, 30 insertions(+), 85 deletions(-) diff --git a/controllers/bsl.go b/controllers/bsl.go index 5e338c43d83..b62a30878de 100644 --- a/controllers/bsl.go +++ b/controllers/bsl.go @@ -93,11 +93,7 @@ func (r *DPAReconciler) ValidateBackupStorageLocations(dpa oadpv1alpha1.DataProt return true, nil } -func (r *DPAReconciler) ReconcileBackupStorageLocations(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } +func (r *DPAReconciler) ReconcileBackupStorageLocations(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { // Loop through all configured BSLs for i, bslSpec := range dpa.Spec.BackupLocations { // Create BSL as is, we can safely assume they are valid from diff --git a/controllers/bsl_test.go b/controllers/bsl_test.go index 88657c21abb..2888dcd8c71 100644 --- a/controllers/bsl_test.go +++ b/controllers/bsl_test.go @@ -2003,7 +2003,7 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { }}, }, } - got, err := r.ReconcileBackupStorageLocations(r.Log) + got, err := r.ReconcileBackupStorageLocations(r.Log, *tt.dpa) if (err != nil) != tt.wantErr { t.Errorf("ReconcileBackupStorageLocations() error = %v, wantErr %v", err, tt.wantErr) return @@ -2326,7 +2326,12 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { }, EventRecorder: record.NewFakeRecorder(10), } - got, err := r.ReconcileBackupStorageLocations(r.Log) + dpa, ok := tt.objects[0].(*oadpv1alpha1.DataProtectionApplication) + if !ok { + t.Errorf("Unexpected object type: %T", tt.objects[0]) + return + } + got, err := r.ReconcileBackupStorageLocations(r.Log, *dpa) if (err != nil) != tt.wantErr { t.Errorf("ReconcileBackupStorageLocations() error =%v, wantErr %v", err, tt.wantErr) return diff --git a/controllers/dpa_controller.go b/controllers/dpa_controller.go index ca068614d85..298a4e4dfc2 100644 --- a/controllers/dpa_controller.go +++ b/controllers/dpa_controller.go @@ -91,7 +91,7 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R // set client to pkg/client for use in non-reconcile functions oadpClient.SetClient(r.Client) - _, err := ReconcileBatch(r.Log, + _, err := ReconcileBatch(r.Log, dpa, r.ValidateDataProtectionCR, r.ReconcileFsRestoreHelperConfig, r.ReconcileBackupStorageLocations, @@ -208,15 +208,14 @@ func (l *labelHandler) Generic(evt event.GenericEvent, q workqueue.RateLimitingI } -type ReconcileFunc func(logr.Logger) (bool, error) +type ReconcileFunc func(logr.Logger, oadpv1alpha1.DataProtectionApplication) (bool, error) // reconcileBatch steps through a list of reconcile functions until one returns // false or an error. -func ReconcileBatch(l logr.Logger, reconcileFuncs ...ReconcileFunc) (bool, error) { +func ReconcileBatch(l logr.Logger, dpa oadpv1alpha1.DataProtectionApplication, reconcileFuncs ...ReconcileFunc) (bool, error) { // TODO: #1127 DPAReconciler already have a logger, use it instead of passing to each reconcile functions - // TODO: #1128 Right now each reconcile functions call get for DPA, we can do it once and pass it to each function for _, f := range reconcileFuncs { - if cont, err := f(l); !cont || err != nil { + if cont, err := f(l, dpa); !cont || err != nil { return cont, err } } diff --git a/controllers/monitor.go b/controllers/monitor.go index 9ff4b908877..d73b7f45039 100644 --- a/controllers/monitor.go +++ b/controllers/monitor.go @@ -11,12 +11,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -func (r *DPAReconciler) ReconcileVeleroMetricsSVC(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - +func (r *DPAReconciler) ReconcileVeleroMetricsSVC(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { svc := corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: "openshift-adp-velero-metrics-svc", diff --git a/controllers/nodeagent.go b/controllers/nodeagent.go index 4d55af8a96a..24d89668497 100644 --- a/controllers/nodeagent.go +++ b/controllers/nodeagent.go @@ -70,13 +70,8 @@ func getNodeAgentObjectMeta(r *DPAReconciler) metav1.ObjectMeta { } } -func (r *DPAReconciler) ReconcileNodeAgentDaemonset(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} +func (r *DPAReconciler) ReconcileNodeAgentDaemonset(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { var deleteDaemonSet bool = true - - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } // Define "static" portion of daemonset ds := &appsv1.DaemonSet{ ObjectMeta: getNodeAgentObjectMeta(r), @@ -162,7 +157,7 @@ func (r *DPAReconciler) ReconcileNodeAgentDaemonset(log logr.Logger) (bool, erro if err != nil { return false, err } - return r.ReconcileNodeAgentDaemonset(log) + return r.ReconcileNodeAgentDaemonset(log, dpa) } } return false, err @@ -367,12 +362,7 @@ func (r *DPAReconciler) customizeNodeAgentDaemonset(dpa *oadpv1alpha1.DataProtec return ds, nil } -func (r *DPAReconciler) ReconcileFsRestoreHelperConfig(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - +func (r *DPAReconciler) ReconcileFsRestoreHelperConfig(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { fsRestoreHelperCM := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: FsRestoreHelperCM, diff --git a/controllers/nodeagent_test.go b/controllers/nodeagent_test.go index 3759367607e..d5594ab4cbc 100644 --- a/controllers/nodeagent_test.go +++ b/controllers/nodeagent_test.go @@ -38,6 +38,7 @@ func TestDPAReconciler_ReconcileNodeAgentDaemonset(t *testing.T) { } type args struct { log logr.Logger + dpa oadpv1alpha1.DataProtectionApplication } tests := []struct { name string @@ -58,7 +59,7 @@ func TestDPAReconciler_ReconcileNodeAgentDaemonset(t *testing.T) { NamespacedName: tt.fields.NamespacedName, EventRecorder: tt.fields.EventRecorder, } - got, err := r.ReconcileNodeAgentDaemonset(tt.args.log) + got, err := r.ReconcileNodeAgentDaemonset(tt.args.log, tt.args.dpa) if (err != nil) != tt.wantErr { t.Errorf("DPAReconciler.ReconcileNodeAgentDaemonset() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/controllers/registry.go b/controllers/registry.go index 3ba9bd5cedd..060c1cdb477 100644 --- a/controllers/registry.go +++ b/controllers/registry.go @@ -159,12 +159,7 @@ type azureCredentials struct { strorageAccountKey string } -func (r *DPAReconciler) ReconcileRegistries(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - +func (r *DPAReconciler) ReconcileRegistries(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { bslLabels := map[string]string{ "app.kubernetes.io/name": common.OADPOperatorVelero, "app.kubernetes.io/managed-by": common.OADPOperator, @@ -472,12 +467,7 @@ func (r *DPAReconciler) getMatchedKeyValue(key string, s string) (string, error) return s, nil } -func (r *DPAReconciler) ReconcileRegistrySVCs(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - +func (r *DPAReconciler) ReconcileRegistrySVCs(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { // fetch the bsl instances bslList := velerov1.BackupStorageLocationList{} if err := r.List(r.Context, &bslList, &client.ListOptions{ @@ -520,12 +510,7 @@ func (r *DPAReconciler) ReconcileRegistrySVCs(log logr.Logger) (bool, error) { return true, nil } -func (r *DPAReconciler) ReconcileRegistryRoutes(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - +func (r *DPAReconciler) ReconcileRegistryRoutes(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { // fetch the bsl instances bslList := velerov1.BackupStorageLocationList{} if err := r.List(r.Context, &bslList, &client.ListOptions{ @@ -576,8 +561,7 @@ func (r *DPAReconciler) ReconcileRegistryRoutes(log logr.Logger) (bool, error) { return true, nil } -func (r *DPAReconciler) ReconcileRegistryRouteConfigs(log logr.Logger) (bool, error) { - +func (r *DPAReconciler) ReconcileRegistryRouteConfigs(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { // Now for each of these bsl instances, create a registry route cm for each of them registryRouteCM := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -608,12 +592,7 @@ func (r *DPAReconciler) ReconcileRegistryRouteConfigs(log logr.Logger) (bool, er } // Create secret for registry to be parsed by openshift-velero-plugin -func (r *DPAReconciler) ReconcileRegistrySecrets(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - +func (r *DPAReconciler) ReconcileRegistrySecrets(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { // fetch the bsl instances bslList := velerov1.BackupStorageLocationList{} if err := r.List(r.Context, &bslList, &client.ListOptions{ diff --git a/controllers/validator.go b/controllers/validator.go index e593db668db..12cc3d07992 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -13,12 +13,7 @@ import ( // ValidateDataProtectionCR function validates the DPA CR, returns true if valid, false otherwise // it calls other validation functions to validate the DPA CR -// TODO: #1129 Clean up duplicate logic for validating backupstoragelocations and volumesnapshotlocations in dpa -func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } +func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { if dpa.Spec.Configuration == nil || dpa.Spec.Configuration.Velero == nil { return false, errors.New("DPA CR Velero configuration cannot be nil") } diff --git a/controllers/validator_test.go b/controllers/validator_test.go index 2ddc916e352..83da4764e7d 100644 --- a/controllers/validator_test.go +++ b/controllers/validator_test.go @@ -1378,7 +1378,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { EventRecorder: record.NewFakeRecorder(10), } t.Run(tt.name, func(t *testing.T) { - got, err := r.ValidateDataProtectionCR(r.Log) + got, err := r.ValidateDataProtectionCR(r.Log, *tt.dpa) if err != nil && !tt.wantErr { t.Errorf("ValidateDataProtectionCR() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/controllers/velero.go b/controllers/velero.go index 3b56880afeb..77c47de1956 100644 --- a/controllers/velero.go +++ b/controllers/velero.go @@ -64,12 +64,7 @@ var ( } ) -func (r *DPAReconciler) ReconcileVeleroDeployment(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - +func (r *DPAReconciler) ReconcileVeleroDeployment(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { veleroDeployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: common.Velero, @@ -112,7 +107,7 @@ func (r *DPAReconciler) ReconcileVeleroDeployment(log logr.Logger) (bool, error) if err != nil { return false, err } - return r.ReconcileVeleroDeployment(log) + return r.ReconcileVeleroDeployment(log, dpa) } } diff --git a/controllers/vsl.go b/controllers/vsl.go index d6181bf6628..7d9fda488bd 100644 --- a/controllers/vsl.go +++ b/controllers/vsl.go @@ -47,12 +47,7 @@ var validAzureKeys = map[string]bool{ AzureResourceGroup: true, } -func (r *DPAReconciler) LabelVSLSecrets(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - +func (r *DPAReconciler) LabelVSLSecrets(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { for _, vsl := range dpa.Spec.SnapshotLocations { provider := strings.TrimPrefix(vsl.Velero.Provider, veleroIOPrefix) switch provider { @@ -203,12 +198,7 @@ func (r *DPAReconciler) ValidateVolumeSnapshotLocations(dpa oadpv1alpha1.DataPro return true, nil } -func (r *DPAReconciler) ReconcileVolumeSnapshotLocations(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - +func (r *DPAReconciler) ReconcileVolumeSnapshotLocations(log logr.Logger, dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { // Loop through all configured VSLs for i, vslSpec := range dpa.Spec.SnapshotLocations { // Create VSL as is, we can safely assume they are valid from diff --git a/controllers/vsl_test.go b/controllers/vsl_test.go index af51d77828d..b859c784e56 100644 --- a/controllers/vsl_test.go +++ b/controllers/vsl_test.go @@ -588,7 +588,7 @@ func TestDPAReconciler_ReconcileVolumeSnapshotLocations(t *testing.T) { }}, }, } - got, err := r.ReconcileVolumeSnapshotLocations(r.Log) + got, err := r.ReconcileVolumeSnapshotLocations(r.Log, *tt.dpa) if (err != nil) != tt.wantErr { t.Errorf("ReconcileVolumeSnapshotLocations() error = %v, wantErr %v", err, tt.wantErr) return