run multiple backup reconcilers, only reconcile ReadyToStart backups

Signed-off-by: Scott Seago <sseago@redhat.com>

pkg/controller/backup_controller.go

@@ -36,7 +36,11 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/utils/clock"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/builder"
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	"github.com/vmware-tanzu/velero/internal/credentials"
 	"github.com/vmware-tanzu/velero/internal/resourcepolicies"
@@ -161,7 +165,7 @@ func NewBackupReconciler(
 		maxConcurrentK8SConnections: maxConcurrentK8SConnections,
 		defaultSnapshotMoveData:     defaultSnapshotMoveData,
 		itemBlockWorkerCount:        itemBlockWorkerCount,
-		concurrentBackups:           concurrentBackups,
+		concurrentBackups:           max(concurrentBackups, 1),
 		globalCRClient:              globalCRClient,
 		workerPool:                  pkgbackup.StartItemBlockWorkerPool(ctx, itemBlockWorkerCount, logger),
 	}
@@ -171,7 +175,24 @@ func NewBackupReconciler(
 
 func (b *backupReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
-		For(&velerov1api.Backup{}).
+		For(&velerov1api.Backup{}, builder.WithPredicates(predicate.Funcs{
+			UpdateFunc: func(ue event.UpdateEvent) bool {
+				backup := ue.ObjectNew.(*velerov1api.Backup)
+				return backup.Status.Phase == velerov1api.BackupPhaseReadyToStart
+			},
+			CreateFunc: func(ce event.CreateEvent) bool {
+				return false
+			},
+			DeleteFunc: func(de event.DeleteEvent) bool {
+				return false
+			},
+			GenericFunc: func(ge event.GenericEvent) bool {
+				return false
+			},
+		})).
+		WithOptions(controller.Options{
+			MaxConcurrentReconciles: b.concurrentBackups,
+		}).
 		Named(constant.ControllerBackup).
 		Complete(b)
 }
@@ -257,8 +278,8 @@ func (b *backupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	// InProgress, we still need this check so we can return nil to indicate we've finished processing
 	// this key (even though it was a no-op).
 	switch original.Status.Phase {
-	case "", velerov1api.BackupPhaseNew:
+	case velerov1api.BackupPhaseReadyToStart:
-		// only process new backups
+		// only process ReadytToStart backups
 	default:
 		b.logger.WithFields(logrus.Fields{
 			"backup": kubeutil.NamespaceAndName(original),

pkg/controller/backup_controller_test.go

@@ -95,7 +95,11 @@ func (b *fakeBackupper) FinalizeBackup(
 }
 
 func defaultBackup() *builder.BackupBuilder {
-	return builder.ForBackup(velerov1api.DefaultNamespace, "backup-1")
+	return builder.ForBackup(velerov1api.DefaultNamespace, "backup-1").Phase(velerov1api.BackupPhaseReadyToStart)
+}
+
+func namedBackup(name string) *builder.BackupBuilder {
+	return builder.ForBackup(velerov1api.DefaultNamespace, name).Phase(velerov1api.BackupPhaseReadyToStart)
 }
 
 func TestProcessBackupNonProcessedItems(t *testing.T) {
@@ -104,6 +108,16 @@ func TestProcessBackupNonProcessedItems(t *testing.T) {
 		key    string
 		backup *velerov1api.Backup
 	}{
+		{
+			name:   "New backup is not processed",
+			key:    "velero/backup-1",
+			backup: defaultBackup().Phase(velerov1api.BackupPhaseNew).Result(),
+		},
+		{
+			name:   "Queued backup is not processed",
+			key:    "velero/backup-1",
+			backup: defaultBackup().Phase(velerov1api.BackupPhaseQueued).Result(),
+		},
 		{
 			name:   "FailedValidation backup is not processed",
 			key:    "velero/backup-1",
@@ -331,7 +345,7 @@ func Test_prepareBackupRequest_BackupStorageLocation(t *testing.T) {
 	}{
 		{
 			name:                             "BackupLocation is specified in backup CR'spec and it can be found in ApiServer",
-			backup:                           builder.ForBackup("velero", "backup-1").Result(),
+			backup:                           defaultBackup().Result(),
 			backupLocationNameInBackup:       "test-backup-location",
 			backupLocationInAPIServer:        builder.ForBackupStorageLocation("velero", "test-backup-location").Result(),
 			defaultBackupLocationInAPIServer: builder.ForBackupStorageLocation("velero", "default-location").Result(),
@@ -340,7 +354,7 @@ func Test_prepareBackupRequest_BackupStorageLocation(t *testing.T) {
 		},
 		{
 			name:                             "BackupLocation is specified in backup CR'spec and it can't be found in ApiServer",
-			backup:                           builder.ForBackup("velero", "backup-1").Result(),
+			backup:                           defaultBackup().Result(),
 			backupLocationNameInBackup:       "test-backup-location",
 			backupLocationInAPIServer:        nil,
 			defaultBackupLocationInAPIServer: nil,
@@ -349,7 +363,7 @@ func Test_prepareBackupRequest_BackupStorageLocation(t *testing.T) {
 		},
 		{
 			name:                             "Using default BackupLocation and it can be found in ApiServer",
-			backup:                           builder.ForBackup("velero", "backup-1").Result(),
+			backup:                           defaultBackup().Result(),
 			backupLocationNameInBackup:       "",
 			backupLocationInAPIServer:        builder.ForBackupStorageLocation("velero", "test-backup-location").Result(),
 			defaultBackupLocationInAPIServer: builder.ForBackupStorageLocation("velero", "default-location").Result(),
@@ -358,7 +372,7 @@ func Test_prepareBackupRequest_BackupStorageLocation(t *testing.T) {
 		},
 		{
 			name:                             "Using default BackupLocation and it can't be found in ApiServer",
-			backup:                           builder.ForBackup("velero", "backup-1").Result(),
+			backup:                           defaultBackup().Result(),
 			backupLocationNameInBackup:       "",
 			backupLocationInAPIServer:        nil,
 			defaultBackupLocationInAPIServer: nil,
@@ -497,7 +511,7 @@ func TestPrepareBackupRequest_SetsVGSLabelKey(t *testing.T) {
 	}{
 		{
 			name: "backup with spec label key set",
-			backup: builder.ForBackup("velero", "backup-1").
+			backup: defaultBackup().
 				VolumeGroupSnapshotLabelKey("spec-key").
 				Result(),
 			serverFlagKey:    "server-key",
@@ -505,13 +519,13 @@ func TestPrepareBackupRequest_SetsVGSLabelKey(t *testing.T) {
 		},
 		{
 			name:             "backup with no spec key, uses server flag",
-			backup:           builder.ForBackup("velero", "backup-2").Result(),
+			backup:           namedBackup("backup-2").Result(),
 			serverFlagKey:    "server-key",
 			expectedLabelKey: "server-key",
 		},
 		{
 			name:             "backup with no spec or server flag, uses default",
-			backup:           builder.ForBackup("velero", "backup-3").Result(),
+			backup:           namedBackup("backup-3").Result(),
 			serverFlagKey:    velerov1api.DefaultVGSLabelKey,
 			expectedLabelKey: velerov1api.DefaultVGSLabelKey,
 		},
@@ -1390,7 +1404,7 @@ func TestProcessBackupCompletions(t *testing.T) {
 		},
 		{
 			name: "backup with namespace-scoped and cluster-scoped resource filters",
-			backup: builder.ForBackup(velerov1api.DefaultNamespace, "backup-1").
+			backup: defaultBackup().
 				ExcludedClusterScopedResources("clusterroles").
 				IncludedClusterScopedResources("storageclasses").
 				ExcludedNamespaceScopedResources("secrets").
@@ -1439,7 +1453,7 @@ func TestProcessBackupCompletions(t *testing.T) {
 		},
 		{
 			name: "backup's include filter overlap with default exclude resources",
-			backup: builder.ForBackup(velerov1api.DefaultNamespace, "backup-1").
+			backup: defaultBackup().
 				ExcludedClusterScopedResources("clusterroles").
 				IncludedClusterScopedResources("storageclasses", "volumesnapshotcontents.snapshot.storage.k8s.io").
 				ExcludedNamespaceScopedResources("secrets").

pkg/controller/backup_queue_controller.go

@@ -64,7 +64,7 @@ func NewBackupQueueReconciler(
 		Client:            client,
 		Scheme:            scheme,
 		logger:            logger,
-		concurrentBackups: concurrentBackups,
+		concurrentBackups: max(concurrentBackups, 1),
 		frequency:         defaultQueuedBackupRecheckFrequency,
 	}
 }