Centralize + rename controller names and list (#2936)

* Centralize + rename controller names and list

Signed-off-by: Carlisia <carlisia@vmware.com>

* Rename file

Signed-off-by: Carlisia <carlisia@vmware.com>

* Reset restic-repo name

Signed-off-by: Carlisia <carlisia@vmware.com>

* Reset gc controller name

Signed-off-by: Carlisia <carlisia@vmware.com>

pkg/cmd/server/server.go

@@ -95,35 +95,11 @@ const (
 
 	defaultProfilerAddress = "localhost:6060"
 
-	// keys used to map out available controllers with disable-controllers flag
-	BackupControllerKey              = "backup"
-	BackupSyncControllerKey          = "backup-sync"
-	ScheduleControllerKey            = "schedule"
-	GcControllerKey                  = "gc"
-	BackupDeletionControllerKey      = "backup-deletion"
-	RestoreControllerKey             = "restore"
-	DownloadRequestControllerKey     = "download-request"
-	ResticRepoControllerKey          = "restic-repo"
-	ServerStatusRequestControllerKey = "server-status-request"
-
 	defaultControllerWorkers = 1
 	// the default TTL for a backup
 	defaultBackupTTL = 30 * 24 * time.Hour
 )
 
-// list of available controllers for input validation
-var disableControllerList = []string{
-	BackupControllerKey,
-	BackupSyncControllerKey,
-	ScheduleControllerKey,
-	GcControllerKey,
-	BackupDeletionControllerKey,
-	RestoreControllerKey,
-	DownloadRequestControllerKey,
-	ResticRepoControllerKey,
-	ServerStatusRequestControllerKey,
-}
-
 type serverConfig struct {
 	pluginDir, metricsAddress, defaultBackupLocation                        string
 	backupSyncPeriod, podVolumeOperationTimeout, resourceTerminatingTimeout time.Duration
@@ -217,7 +193,7 @@ func NewCommand(f client.Factory) *cobra.Command {
 	command.Flags().DurationVar(&config.backupSyncPeriod, "backup-sync-period", config.backupSyncPeriod, "How often to ensure all Velero backups in object storage exist as Backup API objects in the cluster. This is the default sync period if none is explicitly specified for a backup storage location.")
 	command.Flags().DurationVar(&config.podVolumeOperationTimeout, "restic-timeout", config.podVolumeOperationTimeout, "How long backups/restores of pod volumes should be allowed to run before timing out.")
 	command.Flags().BoolVar(&config.restoreOnly, "restore-only", config.restoreOnly, "Run in a mode where only restores are allowed; backups, schedules, and garbage-collection are all disabled. DEPRECATED: this flag will be removed in v2.0. Use read-only backup storage locations instead.")
-	command.Flags().StringSliceVar(&config.disabledControllers, "disable-controllers", config.disabledControllers, fmt.Sprintf("List of controllers to disable on startup. Valid values are %s", strings.Join(disableControllerList, ",")))
+	command.Flags().StringSliceVar(&config.disabledControllers, "disable-controllers", config.disabledControllers, fmt.Sprintf("List of controllers to disable on startup. Valid values are %s", strings.Join(controller.DisableableControllers, ",")))
 	command.Flags().StringSliceVar(&config.restoreResourcePriorities, "restore-resource-priorities", config.restoreResourcePriorities, "Desired order of resource restores; any resource not in the list will be restored alphabetically after the prioritized resources.")
 	command.Flags().StringVar(&config.defaultBackupLocation, "default-backup-storage-location", config.defaultBackupLocation, "Name of the default backup storage location.")
 	command.Flags().DurationVar(&config.storeValidationFrequency, "store-validation-frequency", config.storeValidationFrequency, "How often to verify if the storage is valid. Optional. Set this to `0s` to disable sync. Default 1 minute.")
@@ -782,26 +758,26 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string
 	}
 
 	enabledControllers := map[string]func() controllerRunInfo{
-		BackupSyncControllerKey:      backupSyncControllerRunInfo,
-		BackupControllerKey:          backupControllerRunInfo,
-		ScheduleControllerKey:        scheduleControllerRunInfo,
-		GcControllerKey:              gcControllerRunInfo,
-		BackupDeletionControllerKey:  deletionControllerRunInfo,
-		RestoreControllerKey:         restoreControllerRunInfo,
-		ResticRepoControllerKey:      resticRepoControllerRunInfo,
-		DownloadRequestControllerKey: downloadrequestControllerRunInfo,
+		controller.BackupSync:        backupSyncControllerRunInfo,
+		controller.Backup:            backupControllerRunInfo,
+		controller.Schedule:          scheduleControllerRunInfo,
+		controller.GarbageCollection: gcControllerRunInfo,
+		controller.BackupDeletion:    deletionControllerRunInfo,
+		controller.Restore:           restoreControllerRunInfo,
+		controller.ResticRepo:        resticRepoControllerRunInfo,
+		controller.DownloadRequest:   downloadrequestControllerRunInfo,
 	}
 	// Note: all runtime type controllers that can be disabled are grouped separately, below:
 	enabledRuntimeControllers := make(map[string]struct{})
-	enabledRuntimeControllers[ServerStatusRequestControllerKey] = struct{}{}
+	enabledRuntimeControllers[controller.ServerStatusRequest] = struct{}{}
 
 	if s.config.restoreOnly {
 		s.logger.Info("Restore only mode - not starting the backup, schedule, delete-backup, or GC controllers")
 		s.config.disabledControllers = append(s.config.disabledControllers,
-			BackupControllerKey,
-			ScheduleControllerKey,
-			GcControllerKey,
-			BackupDeletionControllerKey,
+			controller.Backup,
+			controller.Schedule,
+			controller.GarbageCollection,
+			controller.BackupDeletion,
 		)
 	}
 
@@ -816,7 +792,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string
 				s.logger.Infof("Disabling controller: %s", controllerName)
 				delete(enabledRuntimeControllers, controllerName)
 			} else {
-				s.logger.Fatalf("Invalid value for --disable-controllers flag provided: %s. Valid values are: %s", controllerName, strings.Join(disableControllerList, ","))
+				s.logger.Fatalf("Invalid value for --disable-controllers flag provided: %s. Valid values are: %s", controllerName, strings.Join(controller.DisableableControllers, ","))
 			}
 		}
 	}
@@ -863,10 +839,10 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string
 		Log:              s.logger,
 	}
 	if err := bslr.SetupWithManager(s.mgr); err != nil {
-		s.logger.Fatal(err, "unable to create controller", "controller", "backup-storage-location")
+		s.logger.Fatal(err, "unable to create controller", "controller", controller.BackupStorageLocation)
 	}
 
-	if _, ok := enabledRuntimeControllers[ServerStatusRequestControllerKey]; ok {
+	if _, ok := enabledRuntimeControllers[controller.ServerStatusRequest]; ok {
 		r := controller.ServerStatusRequestReconciler{
 			Scheme: s.mgr.GetScheme(),
 			Client: s.mgr.GetClient(),
@@ -878,7 +854,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string
 			Log: s.logger,
 		}
 		if err := r.SetupWithManager(s.mgr); err != nil {
-			s.logger.Fatal(err, "unable to create controller", "controller", ServerStatusRequestControllerKey)
+			s.logger.Fatal(err, "unable to create controller", "controller", controller.ServerStatusRequest)
 		}
 	}
 

pkg/controller/backup_controller.go

@@ -106,7 +106,7 @@ func NewBackupController(
 	volumeSnapshotContentLister snapshotv1beta1listers.VolumeSnapshotContentLister,
 ) Interface {
 	c := &backupController{
-		genericController:           newGenericController("backup", logger),
+		genericController:           newGenericController(Backup, logger),
 		discoveryHelper:             discoveryHelper,
 		backupper:                   backupper,
 		lister:                      backupInformer.Lister(),
@@ -150,7 +150,7 @@ func NewBackupController(
 
 				key, err := cache.MetaNamespaceKeyFunc(backup)
 				if err != nil {
-					c.logger.WithError(err).WithField("backup", backup).Error("Error creating queue key, item not added to queue")
+					c.logger.WithError(err).WithField(Backup, backup).Error("Error creating queue key, item not added to queue")
 					return
 				}
 				c.queue.Add(key)
@@ -509,7 +509,7 @@ func (c *backupController) validateAndGetSnapshotLocations(backup *velerov1api.B
 // causes the backup to be Failed; if no error is returned, the backup's status's Errors
 // field is checked to see if the backup was a partial failure.
 func (c *backupController) runBackup(backup *pkgbackup.Request) error {
-	c.logger.WithField("backup", kubeutil.NamespaceAndName(backup)).Info("Setting up backup log")
+	c.logger.WithField(Backup, kubeutil.NamespaceAndName(backup)).Info("Setting up backup log")
 
 	logFile, err := ioutil.TempFile("", "")
 	if err != nil {
@@ -529,7 +529,7 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error {
 	logCounter := logging.NewLogCounterHook()
 	logger.Hooks.Add(logCounter)
 
-	backupLog := logger.WithField("backup", kubeutil.NamespaceAndName(backup))
+	backupLog := logger.WithField(Backup, kubeutil.NamespaceAndName(backup))
 
 	backupLog.Info("Setting up backup temp file")
 	backupFile, err := ioutil.TempFile("", "")

pkg/controller/backup_deletion_controller.go

@@ -103,7 +103,7 @@ func NewBackupDeletionController(
 	helper discovery.Helper,
 ) Interface {
 	c := &backupDeletionController{
-		genericController:         newGenericController("backup-deletion", logger),
+		genericController:         newGenericController(BackupDeletion, logger),
 		deleteBackupRequestClient: deleteBackupRequestClient,
 		deleteBackupRequestLister: deleteBackupRequestInformer.Lister(),
 		backupClient:              backupClient,

pkg/controller/backup_storage_location_controller.go

@@ -53,7 +53,7 @@ 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(req ctrl.Request) (ctrl.Result, error) {
-	log := r.Log.WithField("controller", "backupstoragelocation")
+	log := r.Log.WithField("controller", BackupStorageLocation)
 
 	log.Info("Checking for existing backup locations ready to be verified; there needs to be at least 1 backup location available")
 
@@ -71,7 +71,7 @@ func (r *BackupStorageLocationReconciler) Reconcile(req ctrl.Request) (ctrl.Resu
 	var anyVerified bool
 	for i := range locationList.Items {
 		location := &locationList.Items[i]
-		log := r.Log.WithField("controller", "backupstoragelocation").WithField("backupstoragelocation", location.Name)
+		log := r.Log.WithField("controller", BackupStorageLocation).WithField(BackupStorageLocation, location.Name)
 
 		if location.Name == r.DefaultBackupLocationInfo.StorageLocation {
 			defaultFound = true
@@ -129,7 +129,7 @@ func (r *BackupStorageLocationReconciler) logReconciledPhase(defaultFound bool,
 	var availableBSLs []*velerov1api.BackupStorageLocation
 	var unAvailableBSLs []*velerov1api.BackupStorageLocation
 	var unknownBSLs []*velerov1api.BackupStorageLocation
-	log := r.Log.WithField("controller", "backupstoragelocation")
+	log := r.Log.WithField("controller", BackupStorageLocation)
 
 	for i, location := range locationList.Items {
 		phase := location.Status.Phase

pkg/controller/backup_sync_controller.go

@@ -76,7 +76,7 @@ func NewBackupSyncController(
 	logger.Infof("Backup sync period is %v", syncPeriod)
 
 	c := &backupSyncController{
-		genericController:       newGenericController("backup-sync", logger),
+		genericController:       newGenericController(BackupSync, logger),
 		backupClient:            backupClient,
 		kbClient:                kbClient,
 		podVolumeBackupClient:   podVolumeBackupClient,

pkg/controller/constants.go

@@ -0,0 +1,45 @@
+/*
+Copyright 2020 the Velero contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controller
+
+const (
+	Backup                = "backup"
+	BackupDeletion        = "backup-deletion"
+	BackupStorageLocation = "backup-storage-location"
+	BackupSync            = "backup-sync"
+	DownloadRequest       = "download-request"
+	GarbageCollection     = "gc"
+	PodVolumeBackup       = "pod-volume-backup"
+	PodVolumeRestore      = "pod-volume-restore"
+	ResticRepo            = "restic-repo"
+	Restore               = "restore"
+	Schedule              = "schedule"
+	ServerStatusRequest   = "server-status-request"
+)
+
+// DisableableControllers is a list of controllers that can be disabled
+var DisableableControllers = []string{
+	Backup,
+	BackupDeletion,
+	BackupSync,
+	DownloadRequest,
+	GarbageCollection,
+	ResticRepo,
+	Restore,
+	Schedule,
+	ServerStatusRequest,
+}

pkg/controller/download_request_controller.go

@@ -65,7 +65,7 @@ func NewDownloadRequestController(
 	logger logrus.FieldLogger,
 ) Interface {
 	c := &downloadRequestController{
-		genericController:     newGenericController("downloadrequest", logger),
+		genericController:     newGenericController(DownloadRequest, logger),
 		downloadRequestClient: downloadRequestClient,
 		downloadRequestLister: downloadRequestInformer.Lister(),
 		restoreLister:         restoreLister,
@@ -89,7 +89,7 @@ func NewDownloadRequestController(
 				if err != nil {
 					downloadRequest := obj.(*velerov1api.DownloadRequest)
 					c.logger.WithError(errors.WithStack(err)).
-						WithField("downloadRequest", downloadRequest.Name).
+						WithField(DownloadRequest, downloadRequest.Name).
 						Error("Error creating queue key, item not added to queue")
 					return
 				}
@@ -213,7 +213,7 @@ func (c *downloadRequestController) resync() {
 	for _, dr := range list {
 		key, err := cache.MetaNamespaceKeyFunc(dr)
 		if err != nil {
-			c.logger.WithError(errors.WithStack(err)).WithField("downloadRequest", dr.Name).Error("error generating key for download request")
+			c.logger.WithError(errors.WithStack(err)).WithField(DownloadRequest, dr.Name).Error("error generating key for download request")
 			continue
 		}
 

pkg/controller/gc_controller.go

@@ -63,7 +63,7 @@ func NewGCController(
 	kbClient client.Client,
 ) Interface {
 	c := &gcController{
-		genericController:         newGenericController("gc-controller", logger),
+		genericController:         newGenericController(GarbageCollection, logger),
 		clock:                     clock.RealClock{},
 		backupLister:              backupInformer.Lister(),
 		deleteBackupRequestLister: deleteBackupRequestLister,

pkg/controller/pod_volume_backup_controller.go

@@ -81,7 +81,7 @@ func NewPodVolumeBackupController(
 	nodeName string,
 ) Interface {
 	c := &podVolumeBackupController{
-		genericController:     newGenericController("pod-volume-backup", logger),
+		genericController:     newGenericController(PodVolumeBackup, logger),
 		podVolumeBackupClient: podVolumeBackupClient,
 		podVolumeBackupLister: podVolumeBackupInformer.Lister(),
 		podLister:             corev1listers.NewPodLister(podInformer.GetIndexer()),

pkg/controller/pod_volume_backup_controller_test.go

@@ -155,7 +155,7 @@ func TestPVBHandler(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			c := &podVolumeBackupController{
-				genericController: newGenericController("pod-volume-backup", velerotest.NewLogger()),
+				genericController: newGenericController(PodVolumeBackup, velerotest.NewLogger()),
 				nodeName:          controllerNode,
 				metrics:           metrics.NewResticServerMetrics(),
 			}

pkg/controller/pod_volume_restore_controller.go

@@ -81,7 +81,7 @@ func NewPodVolumeRestoreController(
 	nodeName string,
 ) Interface {
 	c := &podVolumeRestoreController{
-		genericController:      newGenericController("pod-volume-restore", logger),
+		genericController:      newGenericController(PodVolumeRestore, logger),
 		podVolumeRestoreClient: podVolumeRestoreClient,
 		podVolumeRestoreLister: podVolumeRestoreInformer.Lister(),
 		podLister:              corev1listers.NewPodLister(podInformer.GetIndexer()),

pkg/controller/pod_volume_restore_controller_test.go

@@ -211,7 +211,7 @@ func TestPVRHandler(t *testing.T) {
 			var (
 				podInformer = cache.NewSharedIndexInformer(nil, new(corev1api.Pod), 0, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})
 				c           = &podVolumeRestoreController{
-					genericController: newGenericController("pod-volume-restore", velerotest.NewLogger()),
+					genericController: newGenericController(PodVolumeRestore, velerotest.NewLogger()),
 					podLister:         corev1listers.NewPodLister(podInformer.GetIndexer()),
 					nodeName:          controllerNode,
 				}
@@ -394,7 +394,7 @@ func TestPodHandler(t *testing.T) {
 				informers   = veleroinformers.NewSharedInformerFactory(client, 0)
 				pvrInformer = informers.Velero().V1().PodVolumeRestores()
 				c           = &podVolumeRestoreController{
-					genericController:      newGenericController("pod-volume-restore", velerotest.NewLogger()),
+					genericController:      newGenericController(PodVolumeRestore, velerotest.NewLogger()),
 					podVolumeRestoreLister: velerov1listers.NewPodVolumeRestoreLister(pvrInformer.Informer().GetIndexer()),
 					nodeName:               controllerNode,
 				}

pkg/controller/restic_repository_controller.go

@@ -63,7 +63,7 @@ func NewResticRepositoryController(
 	defaultMaintenanceFrequency time.Duration,
 ) Interface {
 	c := &resticRepositoryController{
-		genericController:           newGenericController("restic-repository", logger),
+		genericController:           newGenericController(ResticRepo, logger),
 		resticRepositoryClient:      resticRepositoryClient,
 		resticRepositoryLister:      resticRepositoryInformer.Lister(),
 		kbClient:                    kbClient,

pkg/controller/restore_controller.go

@@ -114,7 +114,7 @@ func NewRestoreController(
 	logFormat logging.Format,
 ) Interface {
 	c := &restoreController{
-		genericController:      newGenericController("restore", logger),
+		genericController:      newGenericController(Restore, logger),
 		namespace:              namespace,
 		restoreClient:          restoreClient,
 		podVolumeBackupClient:  podVolumeBackupClient,

pkg/controller/schedule_controller.go

@@ -66,7 +66,7 @@ func NewScheduleController(
 	metrics *metrics.ServerMetrics,
 ) *scheduleController {
 	c := &scheduleController{
-		genericController: newGenericController("schedule", logger),
+		genericController: newGenericController(Schedule, logger),
 		namespace:         namespace,
 		schedulesClient:   schedulesClient,
 		backupsClient:     backupsClient,

pkg/controller/server_status_request_controller.go

@@ -57,7 +57,7 @@ type ServerStatusRequestReconciler struct {
 // +kubebuilder:rbac:groups=velero.io,resources=serverstatusrequests/status,verbs=get;update;patch
 func (r *ServerStatusRequestReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	log := r.Log.WithFields(logrus.Fields{
-		"controller":          "serverstatusrequest",
+		"controller":          ServerStatusRequest,
 		"serverStatusRequest": req.NamespacedName,
 	})
 
@@ -76,7 +76,7 @@ func (r *ServerStatusRequestReconciler) Reconcile(req ctrl.Request) (ctrl.Result
 	}
 
 	log = r.Log.WithFields(logrus.Fields{
-		"controller":          "serverstatusrequest",
+		"controller":          ServerStatusRequest,
 		"serverStatusRequest": req.NamespacedName,
 		"phase":               statusRequest.Status.Phase,
 	})