fix #1128: Removed multiple DPA get calls for Reconcilers

openshift · Feb 6, 2024 · 7d9cd21 · 7d9cd21
1 parent 8471880
commit 7d9cd21
Show file tree

Hide file tree

Showing 12 changed files with 30 additions and 85 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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