diff --git a/controllers/bsl.go b/controllers/bsl.go index 5e338c43d8..3665a78e95 100644 --- a/controllers/bsl.go +++ b/controllers/bsl.go @@ -94,10 +94,7 @@ func (r *DPAReconciler) ValidateBackupStorageLocations(dpa oadpv1alpha1.DataProt } 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 - } + dpa := r.dpa // Loop through all configured BSLs for i, bslSpec := range dpa.Spec.BackupLocations { // Create BSL as is, we can safely assume they are valid from @@ -134,7 +131,7 @@ func (r *DPAReconciler) ReconcileBackupStorageLocations(log logr.Logger) (bool, // TODO: check for BSL status condition errors and respond here if bslSpec.Velero != nil { - err := r.updateBSLFromSpec(&bsl, &dpa, *bslSpec.Velero) + err := r.updateBSLFromSpec(&bsl, dpa, *bslSpec.Velero) return err } @@ -144,7 +141,7 @@ func (r *DPAReconciler) ReconcileBackupStorageLocations(log logr.Logger) (bool, if err != nil { return err } - err = controllerutil.SetControllerReference(&dpa, &bsl, r.Scheme) + err = controllerutil.SetControllerReference(dpa, &bsl, r.Scheme) if err != nil { return err } diff --git a/controllers/bsl_test.go b/controllers/bsl_test.go index 88657c21ab..9fda9c8950 100644 --- a/controllers/bsl_test.go +++ b/controllers/bsl_test.go @@ -1988,6 +1988,7 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { Name: tt.dpa.Name, }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.dpa, } wantBSL := &velerov1.BackupStorageLocation{ ObjectMeta: metav1.ObjectMeta{ @@ -2311,6 +2312,11 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { } for _, tt := range bslPrefixCATests { t.Run(tt.name, func(t *testing.T) { + dpa, ok := tt.objects[0].(*oadpv1alpha1.DataProtectionApplication) + if !ok { + t.Errorf("Unexpected object type: %T", tt.objects[0]) + return + } fakeClient, err := getFakeClientFromObjects(tt.objects...) if err != nil { t.Errorf("error in creating fake client, likely programmer error") @@ -2325,7 +2331,9 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { Name: tt.objects[0].GetName(), }, EventRecorder: record.NewFakeRecorder(10), + dpa: dpa, } + got, err := r.ReconcileBackupStorageLocations(r.Log) if (err != nil) != tt.wantErr { t.Errorf("ReconcileBackupStorageLocations() error =%v, wantErr %v", err, tt.wantErr) diff --git a/controllers/dpa_controller.go b/controllers/dpa_controller.go index ca068614d8..25e38d74c1 100644 --- a/controllers/dpa_controller.go +++ b/controllers/dpa_controller.go @@ -52,6 +52,7 @@ type DPAReconciler struct { Context context.Context NamespacedName types.NamespacedName EventRecorder record.EventRecorder + dpa *oadpv1alpha1.DataProtectionApplication } var debugMode = os.Getenv("DEBUG") == "true" @@ -81,9 +82,9 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R // Set reconciler context + name r.Context = ctx r.NamespacedName = req.NamespacedName - dpa := oadpv1alpha1.DataProtectionApplication{} + r.dpa = &oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(ctx, req.NamespacedName, &dpa); err != nil { + if err := r.Get(ctx, req.NamespacedName, r.dpa); err != nil { log.Error(err, "unable to fetch DataProtectionApplication CR") return result, nil } @@ -108,7 +109,7 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R ) if err != nil { - apimeta.SetStatusCondition(&dpa.Status.Conditions, + apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{ Type: oadpv1alpha1.ConditionReconciled, Status: metav1.ConditionFalse, @@ -118,7 +119,7 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R ) } else { - apimeta.SetStatusCondition(&dpa.Status.Conditions, + apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{ Type: oadpv1alpha1.ConditionReconciled, Status: metav1.ConditionTrue, @@ -127,7 +128,7 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R }, ) } - statusErr := r.Client.Status().Update(ctx, &dpa) + statusErr := r.Client.Status().Update(ctx, r.dpa) if err == nil { // Don't mask previous error err = statusErr } @@ -214,7 +215,6 @@ type ReconcileFunc func(logr.Logger) (bool, error) // false or an error. func ReconcileBatch(l logr.Logger, 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 { return cont, err diff --git a/controllers/monitor.go b/controllers/monitor.go index 9ff4b90887..5db27d84dc 100644 --- a/controllers/monitor.go +++ b/controllers/monitor.go @@ -12,11 +12,7 @@ import ( ) 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 - } - + dpa := r.dpa svc := corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: "openshift-adp-velero-metrics-svc", @@ -27,7 +23,7 @@ func (r *DPAReconciler) ReconcileVeleroMetricsSVC(log logr.Logger) (bool, error) // Create SVC op, err := controllerutil.CreateOrPatch(r.Context, r.Client, &svc, func() error { // TODO: check for svc status condition errors and respond here - err := r.updateVeleroMetricsSVC(&svc, &dpa) + err := r.updateVeleroMetricsSVC(&svc, dpa) return err }) if err != nil { diff --git a/controllers/nodeagent.go b/controllers/nodeagent.go index 4d55af8a96..42032838c3 100644 --- a/controllers/nodeagent.go +++ b/controllers/nodeagent.go @@ -71,12 +71,8 @@ func getNodeAgentObjectMeta(r *DPAReconciler) metav1.ObjectMeta { } func (r *DPAReconciler) ReconcileNodeAgentDaemonset(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} + dpa := r.dpa 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), @@ -86,7 +82,7 @@ func (r *DPAReconciler) ReconcileNodeAgentDaemonset(log logr.Logger) (bool, erro // V(-1) corresponds to the warn level var deprecationMsg string = "(Deprecation Warning) Use nodeAgent instead of restic, which is deprecated and will be removed with the OADP 1.4" log.V(-1).Info(deprecationMsg) - r.EventRecorder.Event(&dpa, corev1.EventTypeWarning, "DeprecationResticConfig", deprecationMsg) + r.EventRecorder.Event(dpa, corev1.EventTypeWarning, "DeprecationResticConfig", deprecationMsg) } if dpa.Spec.Configuration.Restic != nil && dpa.Spec.Configuration.Restic.Enable != nil && *dpa.Spec.Configuration.Restic.Enable { @@ -142,10 +138,10 @@ func (r *DPAReconciler) ReconcileNodeAgentDaemonset(log logr.Logger) (bool, erro } } - if _, err := r.buildNodeAgentDaemonset(&dpa, ds); err != nil { + if _, err := r.buildNodeAgentDaemonset(dpa, ds); err != nil { return err } - if err := controllerutil.SetControllerReference(&dpa, ds, r.Scheme); err != nil { + if err := controllerutil.SetControllerReference(dpa, ds, r.Scheme); err != nil { return err } return nil @@ -368,11 +364,6 @@ func (r *DPAReconciler) customizeNodeAgentDaemonset(dpa *oadpv1alpha1.DataProtec } 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 - } - fsRestoreHelperCM := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: FsRestoreHelperCM, @@ -394,7 +385,7 @@ func (r *DPAReconciler) ReconcileFsRestoreHelperConfig(log logr.Logger) (bool, e op, err := controllerutil.CreateOrPatch(r.Context, r.Client, &fsRestoreHelperCM, func() error { // update the Config Map - err := r.updateFsRestoreHelperCM(&fsRestoreHelperCM, &dpa) + err := r.updateFsRestoreHelperCM(&fsRestoreHelperCM, r.dpa) return err }) diff --git a/controllers/nodeagent_test.go b/controllers/nodeagent_test.go index 3759367607..18f3031632 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 @@ -57,6 +58,7 @@ func TestDPAReconciler_ReconcileNodeAgentDaemonset(t *testing.T) { Context: tt.fields.Context, NamespacedName: tt.fields.NamespacedName, EventRecorder: tt.fields.EventRecorder, + dpa: &tt.args.dpa, } got, err := r.ReconcileNodeAgentDaemonset(tt.args.log) if (err != nil) != tt.wantErr { diff --git a/controllers/registry.go b/controllers/registry.go index 3ba9bd5ced..c2974d247d 100644 --- a/controllers/registry.go +++ b/controllers/registry.go @@ -160,11 +160,6 @@ type azureCredentials struct { } 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 - } - bslLabels := map[string]string{ "app.kubernetes.io/name": common.OADPOperatorVelero, "app.kubernetes.io/managed-by": common.OADPOperator, @@ -473,11 +468,6 @@ func (r *DPAReconciler) getMatchedKeyValue(key string, s string) (string, error) } 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 - } - // fetch the bsl instances bslList := velerov1.BackupStorageLocationList{} if err := r.List(r.Context, &bslList, &client.ListOptions{ @@ -521,11 +511,6 @@ func (r *DPAReconciler) ReconcileRegistrySVCs(log logr.Logger) (bool, error) { } 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 - } - // fetch the bsl instances bslList := velerov1.BackupStorageLocationList{} if err := r.List(r.Context, &bslList, &client.ListOptions{ @@ -577,7 +562,6 @@ func (r *DPAReconciler) ReconcileRegistryRoutes(log logr.Logger) (bool, error) { } func (r *DPAReconciler) ReconcileRegistryRouteConfigs(log logr.Logger) (bool, error) { - // Now for each of these bsl instances, create a registry route cm for each of them registryRouteCM := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -609,11 +593,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 - } - + dpa := r.dpa // fetch the bsl instances bslList := velerov1.BackupStorageLocationList{} if err := r.List(r.Context, &bslList, &client.ListOptions{ @@ -666,7 +646,7 @@ func (r *DPAReconciler) ReconcileRegistrySecrets(log logr.Logger) (bool, error) // Create Secret op, err := controllerutil.CreateOrPatch(r.Context, r.Client, &secret, func() error { // TODO: check for secret status condition errors and respond here - err := r.patchRegistrySecret(&secret, &bsl, &dpa) + err := r.patchRegistrySecret(&secret, &bsl, dpa) return err }) diff --git a/controllers/validator.go b/controllers/validator.go index e593db668d..ec4c890a56 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -13,12 +13,10 @@ 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 - } + + dpa := r.dpa + if dpa.Spec.Configuration == nil || dpa.Spec.Configuration.Velero == nil { return false, errors.New("DPA CR Velero configuration cannot be nil") } @@ -40,10 +38,10 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) } } - if validBsl, err := r.ValidateBackupStorageLocations(dpa); !validBsl || err != nil { + if validBsl, err := r.ValidateBackupStorageLocations(*dpa); !validBsl || err != nil { return validBsl, err } - if validVsl, err := r.ValidateVolumeSnapshotLocations(dpa); !validVsl || err != nil { + if validVsl, err := r.ValidateVolumeSnapshotLocations(*dpa); !validVsl || err != nil { return validVsl, err } @@ -55,11 +53,11 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) return false, err } - if _, err := r.getVeleroResourceReqs(&dpa); err != nil { + if _, err := r.getVeleroResourceReqs(dpa); err != nil { return false, err } - if _, err := getResticResourceReqs(&dpa); err != nil { + if _, err := getResticResourceReqs(dpa); err != nil { return false, err } return true, nil diff --git a/controllers/validator_test.go b/controllers/validator_test.go index 2ddc916e35..6992a220ca 100644 --- a/controllers/validator_test.go +++ b/controllers/validator_test.go @@ -1375,6 +1375,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Namespace: tt.dpa.Namespace, Name: tt.dpa.Name, }, + dpa: tt.dpa, EventRecorder: record.NewFakeRecorder(10), } t.Run(tt.name, func(t *testing.T) { diff --git a/controllers/velero.go b/controllers/velero.go index 3b56880afe..29f0d4cc12 100644 --- a/controllers/velero.go +++ b/controllers/velero.go @@ -65,10 +65,8 @@ 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 - } + + dpa := r.dpa veleroDeployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -84,18 +82,18 @@ func (r *DPAReconciler) ReconcileVeleroDeployment(log logr.Logger) (bool, error) // Setting Deployment selector if a new object is created as it is immutable if veleroDeployment.ObjectMeta.CreationTimestamp.IsZero() { veleroDeployment.Spec.Selector = &metav1.LabelSelector{ - MatchLabels: getDpaAppLabels(&dpa), + MatchLabels: getDpaAppLabels(dpa), } } // update the Deployment template - err := r.buildVeleroDeployment(veleroDeployment, &dpa) + err := r.buildVeleroDeployment(veleroDeployment, dpa) if err != nil { return err } // Setting controller owner reference on the velero deployment - return controllerutil.SetControllerReference(&dpa, veleroDeployment, r.Scheme) + return controllerutil.SetControllerReference(dpa, veleroDeployment, r.Scheme) }) if debugMode && op != controllerutil.OperationResultNone { // for debugging purposes fmt.Printf("DEBUG: There was a diff which resulted in an operation on Velero Deployment: %s\n", cmp.Diff(orig, veleroDeployment)) diff --git a/controllers/vsl.go b/controllers/vsl.go index d6181bf662..9b1f740f9c 100644 --- a/controllers/vsl.go +++ b/controllers/vsl.go @@ -48,11 +48,7 @@ var validAzureKeys = map[string]bool{ } 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 - } - + dpa := r.dpa for _, vsl := range dpa.Spec.SnapshotLocations { provider := strings.TrimPrefix(vsl.Velero.Provider, veleroIOPrefix) switch provider { @@ -204,11 +200,7 @@ func (r *DPAReconciler) ValidateVolumeSnapshotLocations(dpa oadpv1alpha1.DataPro } 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 - } - + dpa := r.dpa // Loop through all configured VSLs for i, vslSpec := range dpa.Spec.SnapshotLocations { // Create VSL as is, we can safely assume they are valid from @@ -228,7 +220,7 @@ func (r *DPAReconciler) ReconcileVolumeSnapshotLocations(log logr.Logger) (bool, // SetOwnerReference instead // Set controller reference to Velero controller - err := controllerutil.SetControllerReference(&dpa, &vsl, r.Scheme) + err := controllerutil.SetControllerReference(dpa, &vsl, r.Scheme) if err != nil { return err } diff --git a/controllers/vsl_test.go b/controllers/vsl_test.go index af51d77828..9a28963f94 100644 --- a/controllers/vsl_test.go +++ b/controllers/vsl_test.go @@ -573,6 +573,7 @@ func TestDPAReconciler_ReconcileVolumeSnapshotLocations(t *testing.T) { Name: tt.dpa.Name, }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.dpa, } wantVSL := &velerov1.VolumeSnapshotLocation{ ObjectMeta: metav1.ObjectMeta{