Merge pull request #4894 from blackpiglet/bsl-refactor

Refactor BSL controller with periodical enqueue source

pkg/controller/backup_storage_location_controller.go

@@ -29,11 +29,18 @@ import (
 	"sigs.k8s.io/cluster-api/util/patch"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	"github.com/vmware-tanzu/velero/internal/storage"
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/persistence"
 	"github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt"
+	"github.com/vmware-tanzu/velero/pkg/util/kube"
+)
+
+const (
+	backupStorageLocationSyncPeriod = 1 * time.Minute
 )
 
 // BackupStorageLocationReconciler reconciles a BackupStorageLocation object
@@ -53,96 +60,92 @@ type BackupStorageLocationReconciler struct {
 // +kubebuilder:rbac:groups=velero.io,resources=backupstoragelocations,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=velero.io,resources=backupstoragelocations/status,verbs=get;update;patch
 func (r *BackupStorageLocationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	log := r.Log.WithField("controller", BackupStorageLocation)
+	var unavailableErrors []string
+	var location velerov1api.BackupStorageLocation
 
-	log.Debug("Validating availability of backup storage locations.")
+	log := r.Log.WithField("controller", BackupStorageLocation).WithField(BackupStorageLocation, req.NamespacedName.String())
+	log.Debug("Validating availability of BackupStorageLocation")
 
 	locationList, err := storage.ListBackupStorageLocations(r.Ctx, r.Client, req.Namespace)
 	if err != nil {
-		log.WithError(err).Error("No backup storage locations found, at least one is required")
-		return ctrl.Result{}, err
+		log.WithError(err).Error("No BackupStorageLocations found, at least one is required")
+		return ctrl.Result{}, nil
 	}
 
 	pluginManager := r.NewPluginManager(log)
 	defer pluginManager.CleanupClients()
 
 	var defaultFound bool
-	for _, location := range locationList.Items {
-		if location.Spec.Default {
+	for _, bsl := range locationList.Items {
+		if bsl.Spec.Default {
 			defaultFound = true
-			break
+		}
+		if bsl.Name == req.Name && bsl.Namespace == req.Namespace {
+			location = bsl
 		}
 	}
 
-	var unavailableErrors []string
-	var anyVerified bool
-	for i := range locationList.Items {
-		location := &locationList.Items[i]
-		isDefault := location.Spec.Default
-		log := r.Log.WithField("controller", BackupStorageLocation).WithField(BackupStorageLocation, location.Name)
+	if location.Name == "" || location.Namespace == "" {
+		log.WithError(err).Error("BackupStorageLocation is not found")
+		return ctrl.Result{}, nil
+	}
 
-		// TODO(2.0) remove this check since the server default will be deprecated
-		if !defaultFound && location.Name == r.DefaultBackupLocationInfo.StorageLocation {
-			// For backward-compatible, to configure the backup storage location as the default if
-			// none of the BSLs be marked as the default and the BSL name matches against the
-			// "velero server --default-backup-storage-location".
-			isDefault = true
-			defaultFound = true
+	isDefault := location.Spec.Default
+
+	// TODO(2.0) remove this check since the server default will be deprecated
+	if !defaultFound && location.Name == r.DefaultBackupLocationInfo.StorageLocation {
+		// For backward-compatible, to configure the backup storage location as the default if
+		// none of the BSLs be marked as the default and the BSL name matches against the
+		// "velero server --default-backup-storage-location".
+		isDefault = true
+		defaultFound = true
+	}
+
+	func() {
+		// Initialize the patch helper.
+		patchHelper, err := patch.NewHelper(&location, r.Client)
+		if err != nil {
+			log.WithError(err).Error("Error getting a patch helper to update BackupStorageLocation")
+			return
 		}
-
-		if !storage.IsReadyToValidate(location.Spec.ValidationFrequency, location.Status.LastValidationTime, r.DefaultBackupLocationInfo.ServerValidationFrequency, log) {
-			log.Debug("Validation not required, skipping...")
-			continue
-		}
-
-		anyVerified = true
-
-		func() {
-			// Initialize the patch helper.
-			patchHelper, err := patch.NewHelper(location, r.Client)
+		defer func() {
+			location.Status.LastValidationTime = &metav1.Time{Time: time.Now().UTC()}
 			if err != nil {
-				log.WithError(err).Error("Error getting a patch helper to update this resource")
-				return
+				log.Info("BackupStorageLocation is invalid, marking as unavailable")
+				err = errors.Wrapf(err, "BackupStorageLocation %q is unavailable", location.Name)
+				unavailableErrors = append(unavailableErrors, err.Error())
+				location.Status.Phase = velerov1api.BackupStorageLocationPhaseUnavailable
+				location.Status.Message = err.Error()
+			} else {
+				log.Info("BackupStorageLocations is valid, marking as available")
+				location.Status.Phase = velerov1api.BackupStorageLocationPhaseAvailable
+				location.Status.Message = ""
 			}
-			defer func() {
-				location.Status.LastValidationTime = &metav1.Time{Time: time.Now().UTC()}
-				if err != nil {
-					log.Info("Backup storage location is invalid, marking as unavailable")
-					err = errors.Wrapf(err, "Backup storage location %q is unavailable", location.Name)
-					unavailableErrors = append(unavailableErrors, err.Error())
-					location.Status.Phase = velerov1api.BackupStorageLocationPhaseUnavailable
-					location.Status.Message = err.Error()
-				} else {
-					log.Info("Backup storage location valid, marking as available")
-					location.Status.Phase = velerov1api.BackupStorageLocationPhaseAvailable
-					location.Status.Message = ""
-				}
-				if err := patchHelper.Patch(r.Ctx, location); err != nil {
-					log.WithError(err).Error("Error updating backup storage location phase")
-				}
-			}()
-
-			backupStore, err := r.BackupStoreGetter.Get(location, pluginManager, log)
-			if err != nil {
-				err = errors.Wrapf(err, "Error getting a backup store")
-				return
+			if err := patchHelper.Patch(r.Ctx, &location); err != nil {
+				log.WithError(err).Error("Error updating BackupStorageLocation phase")
 			}
-
-			// updates the default backup location
-			location.Spec.Default = isDefault
-
-			log.Info("Validating backup storage location")
-			err = backupStore.IsValid()
 		}()
-	}
 
-	if !anyVerified {
-		log.Debug("No backup storage locations needed to be validated")
-	}
+		backupStore, err := r.BackupStoreGetter.Get(&location, pluginManager, log)
+		if err != nil {
+			log.WithError(err).Error("Error getting a backup store")
+			return
+		}
+
+		log.Info("Validating BackupStorageLocation")
+		err = backupStore.IsValid()
+		if err != nil {
+			log.WithError(err).Error("fail to validate backup store")
+			return
+		}
+
+		// updates the default backup location
+		location.Spec.Default = isDefault
+	}()
 
 	r.logReconciledPhase(defaultFound, locationList, unavailableErrors)
 
-	return ctrl.Result{Requeue: true}, nil
+	return ctrl.Result{}, nil
 }
 
 func (r *BackupStorageLocationReconciler) logReconciledPhase(defaultFound bool, locationList velerov1api.BackupStorageLocationList, errs []string) {
@@ -169,21 +172,48 @@ func (r *BackupStorageLocationReconciler) logReconciledPhase(defaultFound bool,
 
 	if numUnavailable+numUnknown == len(locationList.Items) { // no available BSL
 		if len(errs) > 0 {
-			log.Errorf("Current backup storage locations available/unavailable/unknown: %v/%v/%v, %s)", numAvailable, numUnavailable, numUnknown, strings.Join(errs, "; "))
+			log.Errorf("Current BackupStorageLocations available/unavailable/unknown: %v/%v/%v, %s)", numAvailable, numUnavailable, numUnknown, strings.Join(errs, "; "))
 		} else {
-			log.Errorf("Current backup storage locations available/unavailable/unknown: %v/%v/%v)", numAvailable, numUnavailable, numUnknown)
+			log.Errorf("Current BackupStorageLocations available/unavailable/unknown: %v/%v/%v)", numAvailable, numUnavailable, numUnknown)
 		}
 	} else if numUnavailable > 0 { // some but not all BSL unavailable
-		log.Warnf("Unavailable backup storage locations detected: available/unavailable/unknown: %v/%v/%v, %s)", numAvailable, numUnavailable, numUnknown, strings.Join(errs, "; "))
+		log.Warnf("Unavailable BackupStorageLocations detected: available/unavailable/unknown: %v/%v/%v, %s)", numAvailable, numUnavailable, numUnknown, strings.Join(errs, "; "))
 	}
 
 	if !defaultFound {
-		log.Warn("There is no existing backup storage location set as default. Please see `velero backup-location -h` for options.")
+		log.Warn("There is no existing BackupStorageLocation set as default. Please see `velero backup-location -h` for options.")
 	}
 }
 
 func (r *BackupStorageLocationReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	g := kube.NewPeriodicalEnqueueSource(
+		r.Log,
+		mgr.GetClient(),
+		&velerov1api.BackupStorageLocationList{},
+		backupStorageLocationSyncPeriod,
+		// Add filter function to enqueue BSL per ValidationFrequency setting.
+		func(object client.Object) bool {
+			location := object.(*velerov1api.BackupStorageLocation)
+			return storage.IsReadyToValidate(location.Spec.ValidationFrequency, location.Status.LastValidationTime, r.DefaultBackupLocationInfo.ServerValidationFrequency, r.Log.WithField("controller", BackupStorageLocation))
+		},
+	)
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&velerov1api.BackupStorageLocation{}).
+		// Handle BSL's creation event and spec update event to let changed BSL got validation immediately.
+		WithEventFilter(predicate.Funcs{
+			CreateFunc: func(ce event.CreateEvent) bool {
+				return true
+			},
+			UpdateFunc: func(ue event.UpdateEvent) bool {
+				return ue.ObjectNew.GetGeneration() != ue.ObjectOld.GetGeneration()
+			},
+			DeleteFunc: func(de event.DeleteEvent) bool {
+				return false
+			},
+			GenericFunc: func(ge event.GenericEvent) bool {
+				return false
+			},
+		}).
+		Watches(g, nil).
 		Complete(r)
 }

pkg/controller/backup_storage_location_controller_test.go

@@ -81,7 +81,7 @@ var _ = Describe("Backup Storage Location Reconciler", func() {
 		Expect(velerov1api.AddToScheme(scheme.Scheme)).To(Succeed())
 		r := BackupStorageLocationReconciler{
 			Ctx:    ctx,
-			Client: fake.NewFakeClientWithScheme(scheme.Scheme, locations),
+			Client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(locations).Build(),
 			DefaultBackupLocationInfo: storage.DefaultBackupLocationInfo{
 				StorageLocation:           "location-1",
 				ServerValidationFrequency: 0,
@@ -91,18 +91,17 @@ var _ = Describe("Backup Storage Location Reconciler", func() {
 			Log:               velerotest.NewLogger(),
 		}
 
-		actualResult, err := r.Reconcile(ctx, ctrl.Request{
-			NamespacedName: types.NamespacedName{Namespace: "ns-1"},
-		})
-
-		Expect(actualResult).To(BeEquivalentTo(ctrl.Result{Requeue: true}))
-		Expect(err).To(BeNil())
-
 		// Assertions
 		for i, location := range locations.Items {
+			actualResult, err := r.Reconcile(ctx, ctrl.Request{
+				NamespacedName: types.NamespacedName{Namespace: location.Namespace, Name: location.Name},
+			})
+			Expect(actualResult).To(BeEquivalentTo(ctrl.Result{}))
+			Expect(err).To(BeNil())
+
 			key := client.ObjectKey{Name: location.Name, Namespace: location.Namespace}
 			instance := &velerov1api.BackupStorageLocation{}
-			err := r.Client.Get(ctx, key, instance)
+			err = r.Client.Get(ctx, key, instance)
 			Expect(err).To(BeNil())
 			Expect(instance.Spec.Default).To(BeIdenticalTo(tests[i].expectedIsDefault))
 			Expect(instance.Status.Phase).To(BeIdenticalTo(tests[i].expectedPhase))
@@ -147,7 +146,7 @@ var _ = Describe("Backup Storage Location Reconciler", func() {
 		Expect(velerov1api.AddToScheme(scheme.Scheme)).To(Succeed())
 		r := BackupStorageLocationReconciler{
 			Ctx:    ctx,
-			Client: fake.NewFakeClientWithScheme(scheme.Scheme, locations),
+			Client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(locations).Build(),
 			DefaultBackupLocationInfo: storage.DefaultBackupLocationInfo{
 				StorageLocation:           "default",
 				ServerValidationFrequency: 0,
@@ -157,91 +156,19 @@ var _ = Describe("Backup Storage Location Reconciler", func() {
 			Log:               velerotest.NewLogger(),
 		}
 
-		actualResult, err := r.Reconcile(ctx, ctrl.Request{
-			NamespacedName: types.NamespacedName{Namespace: "ns-1"},
-		})
-
-		Expect(actualResult).To(BeEquivalentTo(ctrl.Result{Requeue: true}))
-		Expect(err).To(BeNil())
-
 		// Assertions
 		for i, location := range locations.Items {
+			actualResult, err := r.Reconcile(ctx, ctrl.Request{
+				NamespacedName: types.NamespacedName{Namespace: location.Namespace, Name: location.Name},
+			})
+			Expect(actualResult).To(BeEquivalentTo(ctrl.Result{}))
+			Expect(err).To(BeNil())
+
 			key := client.ObjectKey{Name: location.Name, Namespace: location.Namespace}
 			instance := &velerov1api.BackupStorageLocation{}
-			err := r.Client.Get(ctx, key, instance)
+			err = r.Client.Get(ctx, key, instance)
 			Expect(err).To(BeNil())
 			Expect(instance.Spec.Default).To(BeIdenticalTo(tests[i].expectedIsDefault))
 		}
 	})
-
-	It("Should not patch a backup storage location object status phase if the location's validation frequency is specifically set to zero", func() {
-		tests := []struct {
-			backupLocation    *velerov1api.BackupStorageLocation
-			isValidError      error
-			expectedIsDefault bool
-			expectedPhase     velerov1api.BackupStorageLocationPhase
-			wantErr           bool
-		}{
-			{
-				backupLocation:    builder.ForBackupStorageLocation("ns-1", "location-1").ValidationFrequency(0).LastValidationTime(time.Now()).Result(),
-				isValidError:      nil,
-				expectedIsDefault: false,
-				expectedPhase:     "",
-				wantErr:           false,
-			},
-			{
-				backupLocation:    builder.ForBackupStorageLocation("ns-1", "location-2").ValidationFrequency(0).LastValidationTime(time.Now()).Result(),
-				isValidError:      nil,
-				expectedIsDefault: false,
-				expectedPhase:     "",
-				wantErr:           false,
-			},
-		}
-
-		// Setup
-		var (
-			pluginManager = &pluginmocks.Manager{}
-			backupStores  = make(map[string]*persistencemocks.BackupStore)
-		)
-		pluginManager.On("CleanupClients").Return(nil)
-
-		locations := new(velerov1api.BackupStorageLocationList)
-		for i, test := range tests {
-			location := test.backupLocation
-			locations.Items = append(locations.Items, *location)
-			backupStores[location.Name] = &persistencemocks.BackupStore{}
-			backupStores[location.Name].On("IsValid").Return(tests[i].isValidError)
-		}
-
-		// Setup reconciler
-		Expect(velerov1api.AddToScheme(scheme.Scheme)).To(Succeed())
-		r := BackupStorageLocationReconciler{
-			Ctx:    ctx,
-			Client: fake.NewFakeClientWithScheme(scheme.Scheme, locations),
-			DefaultBackupLocationInfo: storage.DefaultBackupLocationInfo{
-				StorageLocation:           "default",
-				ServerValidationFrequency: 0,
-			},
-			NewPluginManager:  func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager },
-			BackupStoreGetter: NewFakeObjectBackupStoreGetter(backupStores),
-			Log:               velerotest.NewLogger(),
-		}
-
-		actualResult, err := r.Reconcile(ctx, ctrl.Request{
-			NamespacedName: types.NamespacedName{Namespace: "ns-1"},
-		})
-
-		Expect(actualResult).To(BeEquivalentTo(ctrl.Result{Requeue: true}))
-		Expect(err).To(BeNil())
-
-		// Assertions
-		for i, location := range locations.Items {
-			key := client.ObjectKey{Name: location.Name, Namespace: location.Namespace}
-			instance := &velerov1api.BackupStorageLocation{}
-			err := r.Client.Get(ctx, key, instance)
-			Expect(err).To(BeNil())
-			Expect(instance.Spec.Default).To(BeIdenticalTo(tests[i].expectedIsDefault))
-			Expect(instance.Status.Phase).To(BeIdenticalTo(tests[i].expectedPhase))
-		}
-	})
 })