Skip to content

Commit

Permalink
fix openshift#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 Oct 11, 2024
1 parent 1a17d7c commit ee2f611
Show file tree
Hide file tree
Showing 17 changed files with 157 additions and 187 deletions.
62 changes: 30 additions & 32 deletions controllers/bsl.go
View file
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
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
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{}

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
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
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

0 comments on commit ee2f611

Please sign in to comment.