Skip to content

Commit

Permalink
fix #1128: Removed multiple DPA get calls for Reconcilers
Browse files Browse the repository at this point in the history
  • Loading branch information
@stillalearner
stillalearner committed Apr 24, 2024
1 parent f23e821 commit a595dc2
Show file tree
Hide file tree
Showing 12 changed files with 47 additions and 83 deletions.
9 changes: 3 additions & 6 deletions controllers/bsl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,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
Expand Down Expand Up @@ -136,7 +133,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
}
Expand All @@ -146,7 +143,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
}
Expand Down
8 changes: 8 additions & 0 deletions controllers/bsl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1990,6 +1990,7 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) {
Name: tt.dpa.Name,
},
EventRecorder: record.NewFakeRecorder(10),
dpa: tt.dpa,
}
wantBSL := &velerov1.BackupStorageLocation{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -2313,6 +2314,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")
Expand All @@ -2327,7 +2333,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)
Expand Down
16 changes: 8 additions & 8 deletions controllers/dpa_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ type DPAReconciler struct {
Context context.Context
NamespacedName types.NamespacedName
EventRecorder record.EventRecorder
dpa *oadpv1alpha1.DataProtectionApplication
}

var debugMode = os.Getenv("DEBUG") == "true"
Expand All @@ -75,15 +76,15 @@ var debugMode = os.Getenv("DEBUG") == "true"
// - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
r.Log = log.FromContext(ctx)
logger := r.Log.WithValues("dpa", req.NamespacedName)
log := r.Log.WithValues("dpa", req.NamespacedName)
result := ctrl.Result{}
// 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 {
logger.Error(err, "unable to fetch DataProtectionApplication CR")
if err := r.Get(ctx, req.NamespacedName, r.dpa); err != nil {
log.Error(err, "unable to fetch DataProtectionApplication CR")
return result, nil
}

Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
8 changes: 2 additions & 6 deletions controllers/monitor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,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",
Expand All @@ -28,7 +24,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 {
Expand Down
19 changes: 5 additions & 14 deletions controllers/nodeagent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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
})

Expand Down
2 changes: 2 additions & 0 deletions controllers/nodeagent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ func TestDPAReconciler_ReconcileNodeAgentDaemonset(t *testing.T) {
}
type args struct {
log logr.Logger
dpa oadpv1alpha1.DataProtectionApplication
}
tests := []struct {
name string
Expand All @@ -55,6 +56,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 {
Expand Down
24 changes: 2 additions & 22 deletions controllers/registry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,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,
Expand Down Expand Up @@ -471,11 +466,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{
Expand Down Expand Up @@ -519,11 +509,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{
Expand Down Expand Up @@ -575,7 +560,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{
Expand Down Expand Up @@ -607,11 +591,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{
Expand Down Expand Up @@ -664,7 +644,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
})
Expand Down
16 changes: 7 additions & 9 deletions controllers/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,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")
}
Expand All @@ -41,10 +39,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
}

Expand All @@ -56,11 +54,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
Expand Down
1 change: 1 addition & 0 deletions controllers/validator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1376,6 +1376,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) {
Expand Down
12 changes: 5 additions & 7 deletions controllers/velero.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,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{
Expand All @@ -83,18 +81,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))
Expand Down
Loading

0 comments on commit a595dc2

Please sign in to comment.