Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix #1128: Removed multiple DPA get calls for Reconcilers #1316

Merged
merged 1 commit into from
Oct 17, 2024
Merged

fix #1128: Removed multiple DPA get calls for Reconcilers #1316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 30 additions & 32 deletions controllers/bsl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,22 @@ import (
"github.com/openshift/oadp-operator/pkg/storage/aws"
)

func (r *DPAReconciler) ValidateBackupStorageLocations(dpa oadpv1alpha1.DataProtectionApplication) (bool, error) {
func (r *DPAReconciler) ValidateBackupStorageLocations() (bool, error) {
// Ensure BSL is a valid configuration
// First, check for provider and then call functions based on the cloud provider for each backupstoragelocation configured
dpa := r.dpa
numDefaultLocations := 0
for _, bslSpec := range dpa.Spec.BackupLocations {

if err := r.ensureBackupLocationHasVeleroOrCloudStorage(&bslSpec); err != nil {
return false, err
}

if err := r.ensurePrefixWhenBackupImages(&dpa, &bslSpec); err != nil {
if err := r.ensurePrefixWhenBackupImages(&bslSpec); err != nil {
return false, err
}

if err := r.ensureSecretDataExists(&dpa, &bslSpec); err != nil {
if err := r.ensureSecretDataExists(&bslSpec); err != nil {
return false, err
}

Expand All @@ -49,17 +50,17 @@ func (r *DPAReconciler) ValidateBackupStorageLocations(dpa oadpv1alpha1.DataProt
// TODO: cases might need some updates for IBM/Minio/noobaa
switch provider {
case AWSProvider, "velero.io/aws":
err := r.validateAWSBackupStorageLocation(*bslSpec.Velero, &dpa)
err := r.validateAWSBackupStorageLocation(*bslSpec.Velero)
if err != nil {
return false, err
}
case AzureProvider, "velero.io/azure":
err := r.validateAzureBackupStorageLocation(*bslSpec.Velero, &dpa)
err := r.validateAzureBackupStorageLocation(*bslSpec.Velero)
if err != nil {
return false, err
}
case GCPProvider, "velero.io/gcp":
err := r.validateGCPBackupStorageLocation(*bslSpec.Velero, &dpa)
err := r.validateGCPBackupStorageLocation(*bslSpec.Velero)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -97,12 +98,9 @@ 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
dpaBSLNames := []string{}

// 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 @@ -146,7 +144,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, *bslSpec.Velero)

return err
}
Expand All @@ -156,7 +154,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 Expand Up @@ -263,9 +261,9 @@ func (r *DPAReconciler) UpdateCredentialsSecretLabels(secretName string, namespa
return true, nil
}

func (r *DPAReconciler) updateBSLFromSpec(bsl *velerov1.BackupStorageLocation, dpa *oadpv1alpha1.DataProtectionApplication, bslSpec velerov1.BackupStorageLocationSpec) error {
func (r *DPAReconciler) updateBSLFromSpec(bsl *velerov1.BackupStorageLocation, bslSpec velerov1.BackupStorageLocationSpec) error {
// Set controller reference to Velero controller
err := controllerutil.SetControllerReference(dpa, bsl, r.Scheme)
err := controllerutil.SetControllerReference(r.dpa, bsl, r.Scheme)
if err != nil {
return err
}
Expand Down Expand Up @@ -313,9 +311,9 @@ func (r *DPAReconciler) updateBSLFromSpec(bsl *velerov1.BackupStorageLocation, d
return nil
}

func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error {
func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec) error {
// validate provider plugin and secret
err := r.validateProviderPluginAndSecret(bslSpec, dpa)
err := r.validateProviderPluginAndSecret(bslSpec)
if err != nil {
return err
}
Expand All @@ -329,7 +327,7 @@ func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.Backup
return fmt.Errorf("bucket name for AWS backupstoragelocation cannot be empty")
}

if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && dpa.BackupImages() {
if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && r.dpa.BackupImages() {
return fmt.Errorf("prefix for AWS backupstoragelocation object storage cannot be empty. It is required for backing up images")
}

Expand All @@ -347,9 +345,9 @@ func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.Backup
return nil
}

func (r *DPAReconciler) validateAzureBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error {
func (r *DPAReconciler) validateAzureBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec) error {
// validate provider plugin and secret
err := r.validateProviderPluginAndSecret(bslSpec, dpa)
err := r.validateProviderPluginAndSecret(bslSpec)
if err != nil {
return err
}
Expand All @@ -371,16 +369,16 @@ func (r *DPAReconciler) validateAzureBackupStorageLocation(bslSpec velerov1.Back
return fmt.Errorf("storageAccount for Azure backupstoragelocation config cannot be empty")
}

if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && dpa.BackupImages() {
if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && r.dpa.BackupImages() {
return fmt.Errorf("prefix for Azure backupstoragelocation object storage cannot be empty. it is required for backing up images")
}

return nil
}

func (r *DPAReconciler) validateGCPBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error {
func (r *DPAReconciler) validateGCPBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec) error {
// validate provider plugin and secret
err := r.validateProviderPluginAndSecret(bslSpec, dpa)
err := r.validateProviderPluginAndSecret(bslSpec)
if err != nil {
return err
}
Expand All @@ -393,7 +391,7 @@ func (r *DPAReconciler) validateGCPBackupStorageLocation(bslSpec velerov1.Backup
if len(bslSpec.ObjectStorage.Bucket) == 0 {
return fmt.Errorf("bucket name for GCP backupstoragelocation cannot be empty")
}
if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && dpa.BackupImages() {
if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && r.dpa.BackupImages() {
return fmt.Errorf("prefix for GCP backupstoragelocation object storage cannot be empty. it is required for backing up images")
}

Expand All @@ -409,12 +407,12 @@ func pluginExistsInVeleroCR(configuredPlugins []oadpv1alpha1.DefaultPlugin, expe
return false
}

func (r *DPAReconciler) validateProviderPluginAndSecret(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error {
if dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret") {
func (r *DPAReconciler) validateProviderPluginAndSecret(bslSpec velerov1.BackupStorageLocationSpec) error {
if r.dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret") {
return nil
}
// check for existence of provider plugin and warn if the plugin is absent
if !pluginExistsInVeleroCR(dpa.Spec.Configuration.Velero.DefaultPlugins, oadpv1alpha1.DefaultPlugin(bslSpec.Provider)) {
if !pluginExistsInVeleroCR(r.dpa.Spec.Configuration.Velero.DefaultPlugins, oadpv1alpha1.DefaultPlugin(bslSpec.Provider)) {
r.Log.Info(fmt.Sprintf("%s backupstoragelocation is configured but velero plugin for %s is not present", bslSpec.Provider, bslSpec.Provider))
//TODO: set warning condition on Velero CR
}
Expand All @@ -440,22 +438,22 @@ func (r *DPAReconciler) ensureBackupLocationHasVeleroOrCloudStorage(bsl *oadpv1a
return nil
}

func (r *DPAReconciler) ensurePrefixWhenBackupImages(dpa *oadpv1alpha1.DataProtectionApplication, bsl *oadpv1alpha1.BackupLocation) error {
func (r *DPAReconciler) ensurePrefixWhenBackupImages(bsl *oadpv1alpha1.BackupLocation) error {

if bsl.Velero != nil && bsl.Velero.ObjectStorage != nil && bsl.Velero.ObjectStorage.Prefix == "" && dpa.BackupImages() {
if bsl.Velero != nil && bsl.Velero.ObjectStorage != nil && bsl.Velero.ObjectStorage.Prefix == "" && r.dpa.BackupImages() {
return fmt.Errorf("BackupLocation must have velero prefix when backupImages is not set to false")
}

if bsl.CloudStorage != nil && bsl.CloudStorage.Prefix == "" && dpa.BackupImages() {
if bsl.CloudStorage != nil && bsl.CloudStorage.Prefix == "" && r.dpa.BackupImages() {
return fmt.Errorf("BackupLocation must have cloud storage prefix when backupImages is not set to false")
}

return nil
}

func (r *DPAReconciler) ensureSecretDataExists(dpa *oadpv1alpha1.DataProtectionApplication, bsl *oadpv1alpha1.BackupLocation) error {
func (r *DPAReconciler) ensureSecretDataExists(bsl *oadpv1alpha1.BackupLocation) error {
// Check if the Velero feature flag 'no-secret' is not set
if !(dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret")) {
if !(r.dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret")) {
// Check if the user specified credential under velero
if bsl.Velero != nil && bsl.Velero.Credential != nil {
// Check if user specified empty credential key
Expand Down
13 changes: 10 additions & 3 deletions controllers/bsl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1503,8 +1503,9 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
Name: tt.dpa.Name,
},
EventRecorder: record.NewFakeRecorder(10),
dpa: tt.dpa,
}
got, err := r.ValidateBackupStorageLocations(*tt.dpa)
got, err := r.ValidateBackupStorageLocations()
if (err != nil) != tt.wantErr {
t.Errorf("ValidateBackupStorageLocations() error = %v, wantErr %v", err, tt.wantErr)
return
Expand Down Expand Up @@ -1780,9 +1781,10 @@ func TestDPAReconciler_updateBSLFromSpec(t *testing.T) {
}
r := &DPAReconciler{
Scheme: scheme,
dpa: tt.dpa,
}

err = r.updateBSLFromSpec(tt.bsl, tt.dpa, *tt.dpa.Spec.BackupLocations[0].Velero)
err = r.updateBSLFromSpec(tt.bsl, *tt.dpa.Spec.BackupLocations[0].Velero)
if (err != nil) != tt.wantErr {
t.Errorf("updateBSLFromSpec() error = %v, wantErr %v", err, tt.wantErr)
return
Expand Down Expand Up @@ -1912,6 +1914,7 @@ func TestDPAReconciler_ensureBackupLocationHasVeleroOrCloudStorage(t *testing.T)
}
r := &DPAReconciler{
Scheme: scheme,
dpa: tt.dpa,
}
for _, bsl := range tt.dpa.Spec.BackupLocations {
if err := r.ensureBackupLocationHasVeleroOrCloudStorage(&bsl); (err != nil) != tt.wantErr {
Expand Down Expand Up @@ -2086,9 +2089,10 @@ func TestDPAReconciler_ensurePrefixWhenBackupImages(t *testing.T) {
}
r := &DPAReconciler{
Scheme: scheme,
dpa: tt.dpa,
}
for _, bsl := range tt.dpa.Spec.BackupLocations {
err := r.ensurePrefixWhenBackupImages(tt.dpa, &bsl)
err := r.ensurePrefixWhenBackupImages(&bsl)
if (err != nil) != tt.wantErr {
t.Errorf("ensurePrefixWhenBackupImages() error = %v, wantErr %v", err, tt.wantErr)
}
Expand Down Expand Up @@ -2207,6 +2211,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 @@ -2558,7 +2563,9 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) {
Name: tt.objects[0].GetName(),
},
EventRecorder: record.NewFakeRecorder(10),
dpa: tt.objects[0].(*oadpv1alpha1.DataProtectionApplication),
}

got, err := r.ReconcileBackupStorageLocations(r.Log)
if (err != nil) != tt.wantErr {
t.Errorf("ReconcileBackupStorageLocations() error =%v, wantErr %v", err, tt.wantErr)
Expand Down
12 changes: 6 additions & 6 deletions controllers/dpa_controller.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @shubham-pampattiwar
Please check if this change is ok with you.

Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ type DPAReconciler struct {
Context context.Context
NamespacedName types.NamespacedName
EventRecorder record.EventRecorder
dpa *oadpv1alpha1.DataProtectionApplication
}

var debugMode = os.Getenv("DEBUG") == "true"
Expand Down Expand Up @@ -79,9 +80,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{}
mateusoliveira43 marked this conversation as resolved.
Show resolved Hide resolved

if err := r.Get(ctx, req.NamespacedName, &dpa); err != nil {
if err := r.Get(ctx, req.NamespacedName, r.dpa); err != nil {
logger.Error(err, "unable to fetch DataProtectionApplication CR")
return result, nil
}
Expand All @@ -107,7 +108,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 @@ -117,7 +118,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 @@ -126,7 +127,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 @@ -213,7 +214,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
17 changes: 5 additions & 12 deletions controllers/monitor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,9 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
)

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
}

svc := corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "openshift-adp-velero-metrics-svc",
Expand All @@ -28,7 +21,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)
return err
})
if err != nil {
Expand All @@ -46,16 +39,16 @@ func (r *DPAReconciler) ReconcileVeleroMetricsSVC(log logr.Logger) (bool, error)
return true, nil
}

func (r *DPAReconciler) updateVeleroMetricsSVC(svc *corev1.Service, dpa *oadpv1alpha1.DataProtectionApplication) error {
func (r *DPAReconciler) updateVeleroMetricsSVC(svc *corev1.Service) error {
// Setting controller owner reference on the metrics svc
err := controllerutil.SetControllerReference(dpa, svc, r.Scheme)
err := controllerutil.SetControllerReference(r.dpa, svc, r.Scheme)
if err != nil {
return err
}

// when updating the spec fields we update each field individually
// to get around the immutable fields
svc.Spec.Selector = getDpaAppLabels(dpa)
svc.Spec.Selector = getDpaAppLabels(r.dpa)

svc.Spec.Type = corev1.ServiceTypeClusterIP
svc.Spec.Ports = []corev1.ServicePort{
Expand All @@ -69,6 +62,6 @@ func (r *DPAReconciler) updateVeleroMetricsSVC(svc *corev1.Service, dpa *oadpv1a
},
}

svc.Labels = getDpaAppLabels(dpa)
svc.Labels = getDpaAppLabels(r.dpa)
return nil
}
3 changes: 2 additions & 1 deletion controllers/monitor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,10 @@ func TestDPAReconciler_updateVeleroMetricsSVC(t *testing.T) {
Name: tt.svc.Name,
},
EventRecorder: record.NewFakeRecorder(10),
dpa: tt.dpa,
}

err = r.updateVeleroMetricsSVC(tt.svc, tt.dpa)
err = r.updateVeleroMetricsSVC(tt.svc)
if !reflect.DeepEqual(tt.wantVeleroMtricsSVC.Labels, tt.svc.Labels) {
t.Errorf("expected velero metrics svc labels to be %#v, got %#v", tt.wantVeleroMtricsSVC.Labels, tt.svc.Labels)
}
Expand Down
Loading