Improvements to BSL logic

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

internal/storage/storagelocation.go

@@ -30,10 +30,11 @@ import (
 
 // DefaultBackupLocationInfo holds server default backup storage location information
 type DefaultBackupLocationInfo struct {
-	// StorageLocation is the name of the backup storage location designated as the default
+	// StorageLocation is the name of the backup storage location designated as the default on the server side.
+	// Deprecated TODO(2.0)
 	StorageLocation string
-	// StoreValidationFrequency is the default validation frequency for any backup storage location
-	StoreValidationFrequency time.Duration
+	// ServerValidationFrequency is the server default validation frequency for all backup storage locations
+	ServerValidationFrequency time.Duration
 }
 
 // IsReadyToValidate calculates if a given backup storage location is ready to be validated.
@@ -44,16 +45,16 @@ type DefaultBackupLocationInfo struct {
 // This will always return "true" for the first attempt at validating a location, regardless of its validation frequency setting
 // Otherwise, it returns "ready" only when NOW is equal to or after the next validation time
 // (next validation time: last validation time + validation frequency)
-func IsReadyToValidate(bslValidationFrequency *metav1.Duration, lastValidationTime *metav1.Time, defaultLocationInfo DefaultBackupLocationInfo, log logrus.FieldLogger) bool {
-	validationFrequency := defaultLocationInfo.StoreValidationFrequency
+func IsReadyToValidate(bslValidationFrequency *metav1.Duration, lastValidationTime *metav1.Time, serverValidationFrequency time.Duration, log logrus.FieldLogger) bool {
+	validationFrequency := serverValidationFrequency
 	// If the bsl validation frequency is not specifically set, skip this block and continue, using the server's default
 	if bslValidationFrequency != nil {
 		validationFrequency = bslValidationFrequency.Duration
 	}
 
 	if validationFrequency < 0 {
-		log.Debugf("Validation period must be non-negative, changing from %d to %d", validationFrequency, defaultLocationInfo.StoreValidationFrequency)
-		validationFrequency = defaultLocationInfo.StoreValidationFrequency
+		log.Debugf("Validation period must be non-negative, changing from %d to %d", validationFrequency, validationFrequency)
+		validationFrequency = serverValidationFrequency
 	}
 
 	lastValidation := lastValidationTime

internal/storage/storagelocation_test.go

@@ -42,7 +42,7 @@ func TestIsReadyToValidate(t *testing.T) {
 			name:                   "validate when true when validation frequency is zero and lastValidationTime is nil",
 			bslValidationFrequency: &metav1.Duration{Duration: 0},
 			defaultLocationInfo: DefaultBackupLocationInfo{
-				StoreValidationFrequency: 0,
+				ServerValidationFrequency: 0,
 			},
 			ready: true,
 		},
@@ -51,7 +51,7 @@ func TestIsReadyToValidate(t *testing.T) {
 			bslValidationFrequency: &metav1.Duration{Duration: 0},
 			lastValidationTime:     &metav1.Time{Time: time.Now()},
 			defaultLocationInfo: DefaultBackupLocationInfo{
-				StoreValidationFrequency: 0,
+				ServerValidationFrequency: 0,
 			},
 			ready: false,
 		},
@@ -59,7 +59,7 @@ func TestIsReadyToValidate(t *testing.T) {
 			name:                   "validate as per location setting, as that takes precedence, and always if it has never been validated before regardless of the frequency setting",
 			bslValidationFrequency: &metav1.Duration{Duration: 1 * time.Hour},
 			defaultLocationInfo: DefaultBackupLocationInfo{
-				StoreValidationFrequency: 0,
+				ServerValidationFrequency: 0,
 			},
 			ready: true,
 		},
@@ -67,7 +67,7 @@ func TestIsReadyToValidate(t *testing.T) {
 			name:                   "don't validate as per location setting, as it is set to zero and that takes precedence",
 			bslValidationFrequency: &metav1.Duration{Duration: 0},
 			defaultLocationInfo: DefaultBackupLocationInfo{
-				StoreValidationFrequency: 1,
+				ServerValidationFrequency: 1,
 			},
 			lastValidationTime: &metav1.Time{Time: time.Now()},
 			ready:              false,
@@ -75,14 +75,14 @@ func TestIsReadyToValidate(t *testing.T) {
 		{
 			name: "validate as per default setting when location setting is not set",
 			defaultLocationInfo: DefaultBackupLocationInfo{
-				StoreValidationFrequency: 1,
+				ServerValidationFrequency: 1,
 			},
 			ready: true,
 		},
 		{
 			name: "don't validate when default setting is set to zero and the location setting is not set",
 			defaultLocationInfo: DefaultBackupLocationInfo{
-				StoreValidationFrequency: 0,
+				ServerValidationFrequency: 0,
 			},
 			lastValidationTime: &metav1.Time{Time: time.Now()},
 			ready:              false,
@@ -92,7 +92,7 @@ func TestIsReadyToValidate(t *testing.T) {
 			bslValidationFrequency: &metav1.Duration{Duration: 1 * time.Second},
 			lastValidationTime:     &metav1.Time{Time: time.Now()},
 			defaultLocationInfo: DefaultBackupLocationInfo{
-				StoreValidationFrequency: 0,
+				ServerValidationFrequency: 0,
 			},
 			ready: false,
 		},
@@ -101,7 +101,7 @@ func TestIsReadyToValidate(t *testing.T) {
 			bslValidationFrequency: &metav1.Duration{Duration: 1 * time.Second},
 			lastValidationTime:     &metav1.Time{Time: time.Now().Add(-1 * time.Second)},
 			defaultLocationInfo: DefaultBackupLocationInfo{
-				StoreValidationFrequency: 0,
+				ServerValidationFrequency: 0,
 			},
 			ready: true,
 		},
@@ -110,7 +110,7 @@ func TestIsReadyToValidate(t *testing.T) {
 			bslValidationFrequency: &metav1.Duration{Duration: 1 * time.Second},
 			lastValidationTime:     &metav1.Time{Time: time.Now().Add(-2 * time.Second)},
 			defaultLocationInfo: DefaultBackupLocationInfo{
-				StoreValidationFrequency: 0,
+				ServerValidationFrequency: 0,
 			},
 			ready: true,
 		},
@@ -120,7 +120,7 @@ func TestIsReadyToValidate(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 			log := velerotest.NewLogger()
-			actual := IsReadyToValidate(tt.bslValidationFrequency, tt.lastValidationTime, tt.defaultLocationInfo, log)
+			actual := IsReadyToValidate(tt.bslValidationFrequency, tt.lastValidationTime, tt.defaultLocationInfo.ServerValidationFrequency, log)
 			g.Expect(actual).To(BeIdenticalTo(tt.ready))
 		})
 	}

pkg/cmd/cli/backuplocation/create.go

@@ -182,20 +182,19 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error {
 
 	if o.DefaultBackupStorageLocation {
 		// There is one and only one default backup storage location.
-		// Disable the origin default backup storage location.
+		// Disable any existing default backup storage location.
 		locations := new(velerov1api.BackupStorageLocationList)
 		if err := kbClient.List(context.Background(), locations, &kbclient.ListOptions{Namespace: f.Namespace()}); err != nil {
 			return errors.WithStack(err)
 		}
 		for _, location := range locations.Items {
-			if !location.Spec.Default {
-				continue
+			if location.Spec.Default {
+				location.Spec.Default = false
+				if err := kbClient.Update(context.Background(), &location, &kbclient.UpdateOptions{}); err != nil {
+					return errors.WithStack(err)
+				}
+				break
 			}
-			location.Spec.Default = false
-			if err := kbClient.Update(context.Background(), &location, &kbclient.UpdateOptions{}); err != nil {
-				return errors.WithStack(err)
-			}
-			break
 		}
 	}
 

pkg/cmd/cli/backuplocation/get.go

@@ -57,6 +57,7 @@ func NewGetCommand(f client.Factory, use string) *cobra.Command {
 					if showDefaultOnly {
 						if location.Spec.Default {
 							locations.Items = append(locations.Items, *location)
+							break
 						}
 					} else {
 						locations.Items = append(locations.Items, *location)

pkg/cmd/cli/backuplocation/set.go

@@ -36,7 +36,7 @@ func NewSetCommand(f client.Factory, use string) *cobra.Command {
 
 	c := &cobra.Command{
 		Use:   use + " NAME",
-		Short: "Set a backup storage location",
+		Short: "Set specific features for a backup storage location",
 		Args:  cobra.ExactArgs(1),
 		Run: func(c *cobra.Command, args []string) {
 			cmd.CheckError(o.Complete(args, f))

pkg/cmd/server/server.go

@@ -101,6 +101,7 @@ const (
 )
 
 type serverConfig struct {
+	// TODO(2.0) Deprecate defaultBackupLocation
 	pluginDir, metricsAddress, defaultBackupLocation                        string
 	backupSyncPeriod, podVolumeOperationTimeout, resourceTerminatingTimeout time.Duration
 	defaultBackupTTL, storeValidationFrequency                              time.Duration
@@ -830,8 +831,8 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string
 		Client: s.mgr.GetClient(),
 		Scheme: s.mgr.GetScheme(),
 		DefaultBackupLocationInfo: storage.DefaultBackupLocationInfo{
-			StorageLocation:          s.config.defaultBackupLocation,
-			StoreValidationFrequency: s.config.storeValidationFrequency,
+			StorageLocation:           s.config.defaultBackupLocation,
+			ServerValidationFrequency: s.config.storeValidationFrequency,
 		},
 		NewPluginManager: newPluginManager,
 		NewBackupStore:   persistence.NewObjectBackupStore,

pkg/controller/backup_controller.go

@@ -343,8 +343,15 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg
 	// calculate expiration
 	request.Status.Expiration = &metav1.Time{Time: c.clock.Now().Add(request.Spec.TTL.Duration)}
 
-	// default storage location if not specified
+	if request.Spec.DefaultVolumesToRestic == nil {
+		request.Spec.DefaultVolumesToRestic = &c.defaultVolumesToRestic
+	}
+
+	// find which storage location to use
+	var serverSpecified bool
 	if request.Spec.StorageLocation == "" {
+		// when the user doesn't specify a location, use the server default unless there is an existing BSL marked as default
+		// TODO(2.0) c.defaultBackupLocation will be deprecated
 		request.Spec.StorageLocation = c.defaultBackupLocation
 
 		locationList, err := storage.ListBackupStorageLocations(context.Background(), c.kbClient, request.Namespace)
@@ -356,10 +363,32 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg
 				}
 			}
 		}
+		serverSpecified = true
 	}
 
-	if request.Spec.DefaultVolumesToRestic == nil {
-		request.Spec.DefaultVolumesToRestic = &c.defaultVolumesToRestic
+	// get the storage location, and store the BackupStorageLocation API obj on the request
+	storageLocation := &velerov1api.BackupStorageLocation{}
+	if err := c.kbClient.Get(context.Background(), kbclient.ObjectKey{
+		Namespace: request.Namespace,
+		Name:      request.Spec.StorageLocation,
+	}, storageLocation); err != nil {
+		if apierrors.IsNotFound(err) {
+			if serverSpecified {
+				// TODO(2.0) remove this. For now, without mentioning "server default" it could be confusing trying to grasp where the default came from.
+				request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("an existing backup storage location wasn't specified at backup creation time and the server default '%s' doesn't exist. Please address this issue (see `velero backup-location -h` for options) and create a new backup. Error: %v", request.Spec.StorageLocation, err))
+			} else {
+				request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("an existing backup storage location wasn't specified at backup creation time and the default '%s' wasn't found. Please address this issue (see `velero backup-location -h` for options) and create a new backup. Error: %v", request.Spec.StorageLocation, err))
+			}
+		} else {
+			request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("error getting backup storage location: %v", err))
+		}
+	} else {
+		request.StorageLocation = storageLocation
+
+		if request.StorageLocation.Spec.AccessMode == velerov1api.BackupStorageLocationAccessModeReadOnly {
+			request.Status.ValidationErrors = append(request.Status.ValidationErrors,
+				fmt.Sprintf("backup can't be created because backup storage location %s is currently in read-only mode", request.StorageLocation.Name))
+		}
 	}
 
 	// add the storage location as a label for easy filtering later.
@@ -368,6 +397,18 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg
 	}
 	request.Labels[velerov1api.StorageLocationLabel] = label.GetValidName(request.Spec.StorageLocation)
 
+	// validate and get the backup's VolumeSnapshotLocations, and store the
+	// VolumeSnapshotLocation API objs on the request
+	if locs, errs := c.validateAndGetSnapshotLocations(request.Backup); len(errs) > 0 {
+		request.Status.ValidationErrors = append(request.Status.ValidationErrors, errs...)
+	} else {
+		request.Spec.VolumeSnapshotLocations = nil
+		for _, loc := range locs {
+			request.Spec.VolumeSnapshotLocations = append(request.Spec.VolumeSnapshotLocations, loc.Name)
+			request.SnapshotLocations = append(request.SnapshotLocations, loc)
+		}
+	}
+
 	// Getting all information of cluster version - useful for future skip-level migration
 	if request.Annotations == nil {
 		request.Annotations = make(map[string]string)
@@ -386,37 +427,6 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg
 		request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid included/excluded namespace lists: %v", err))
 	}
 
-	// validate the storage location, and store the BackupStorageLocation API obj on the request
-	storageLocation := &velerov1api.BackupStorageLocation{}
-	if err := c.kbClient.Get(context.Background(), kbclient.ObjectKey{
-		Namespace: request.Namespace,
-		Name:      request.Spec.StorageLocation,
-	}, storageLocation); err != nil {
-		if apierrors.IsNotFound(err) {
-			request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("a BackupStorageLocation CRD with the name specified in the backup spec needs to be created before this backup can be executed. Error: %v", err))
-		} else {
-			request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("error getting backup storage location: %v", err))
-		}
-	} else {
-		request.StorageLocation = storageLocation
-
-		if request.StorageLocation.Spec.AccessMode == velerov1api.BackupStorageLocationAccessModeReadOnly {
-			request.Status.ValidationErrors = append(request.Status.ValidationErrors,
-				fmt.Sprintf("backup can't be created because backup storage location %s is currently in read-only mode", request.StorageLocation.Name))
-		}
-	}
-	// validate and get the backup's VolumeSnapshotLocations, and store the
-	// VolumeSnapshotLocation API objs on the request
-	if locs, errs := c.validateAndGetSnapshotLocations(request.Backup); len(errs) > 0 {
-		request.Status.ValidationErrors = append(request.Status.ValidationErrors, errs...)
-	} else {
-		request.Spec.VolumeSnapshotLocations = nil
-		for _, loc := range locs {
-			request.Spec.VolumeSnapshotLocations = append(request.Spec.VolumeSnapshotLocations, loc.Name)
-			request.SnapshotLocations = append(request.SnapshotLocations, loc)
-		}
-	}
-
 	return request
 }
 

pkg/controller/backup_controller_test.go

@@ -156,7 +156,7 @@ func TestProcessBackupValidationFailures(t *testing.T) {
 		{
 			name:         "non-existent backup location fails validation",
 			backup:       defaultBackup().StorageLocation("nonexistent").Result(),
-			expectedErrs: []string{"a BackupStorageLocation CRD with the name specified in the backup spec needs to be created before this backup can be executed. Error: backupstoragelocations.velero.io \"nonexistent\" not found"},
+			expectedErrs: []string{"an existing backup storage location wasn't specified at backup creation time and the default 'nonexistent' wasn't found. Please address this issue (see `velero backup-location -h` for options) and create a new backup. Error: backupstoragelocations.velero.io \"nonexistent\" not found"},
 		},
 		{
 			name:           "backup for read-only backup location fails validation",

pkg/controller/backup_storage_location_controller.go

@@ -81,6 +81,7 @@ func (r *BackupStorageLocationReconciler) Reconcile(req ctrl.Request) (ctrl.Resu
 		isDefault := location.Spec.Default
 		log := r.Log.WithField("controller", BackupStorageLocation).WithField(BackupStorageLocation, location.Name)
 
+		// 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
@@ -89,7 +90,7 @@ func (r *BackupStorageLocationReconciler) Reconcile(req ctrl.Request) (ctrl.Resu
 			defaultFound = true
 		}
 
-		if !storage.IsReadyToValidate(location.Spec.ValidationFrequency, location.Status.LastValidationTime, r.DefaultBackupLocationInfo, log) {
+		if !storage.IsReadyToValidate(location.Spec.ValidationFrequency, location.Status.LastValidationTime, r.DefaultBackupLocationInfo.ServerValidationFrequency, log) {
 			log.Debug("Validation not required, skipping...")
 			continue
 		}
@@ -165,11 +166,11 @@ func (r *BackupStorageLocationReconciler) logReconciledPhase(defaultFound bool,
 			log.Errorf("Current backup storage locations available/unavailable/unknown: %v/%v/%v)", numAvailable, numUnavailable, numUnknown)
 		}
 	} else if numUnavailable > 0 { // some but not all BSL unavailable
-		log.Warnf("Invalid backup storage locations detected: available/unavailable/unknown: %v/%v/%v, %s)", numAvailable, numUnavailable, numUnknown, strings.Join(errs, "; "))
+		log.Warnf("Unavailable backup storage locations detected: available/unavailable/unknown: %v/%v/%v, %s)", numAvailable, numUnavailable, numUnknown, strings.Join(errs, "; "))
 	}
 
 	if !defaultFound {
-		log.Warn("The default backup storage location was not found; for convenience, be sure to create one or make another backup location that is designated as the default")
+		log.Warn("There is no existing backup storage location set as default. Please see `velero backup-location -h` for options.")
 	}
 }
 

pkg/controller/backup_storage_location_controller_test.go

@@ -87,8 +87,8 @@ var _ = Describe("Backup Storage Location Reconciler", func() {
 			Ctx:    ctx,
 			Client: fake.NewFakeClientWithScheme(scheme.Scheme, locations),
 			DefaultBackupLocationInfo: storage.DefaultBackupLocationInfo{
-				StorageLocation:          "location-1",
-				StoreValidationFrequency: 0,
+				StorageLocation:           "location-1",
+				ServerValidationFrequency: 0,
 			},
 			NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager },
 			NewBackupStore: func(loc *velerov1api.BackupStorageLocation, _ persistence.ObjectStoreGetter, _ logrus.FieldLogger) (persistence.BackupStore, error) {
@@ -156,8 +156,8 @@ var _ = Describe("Backup Storage Location Reconciler", func() {
 			Ctx:    ctx,
 			Client: fake.NewFakeClientWithScheme(scheme.Scheme, locations),
 			DefaultBackupLocationInfo: storage.DefaultBackupLocationInfo{
-				StorageLocation:          "default",
-				StoreValidationFrequency: 0,
+				StorageLocation:           "default",
+				ServerValidationFrequency: 0,
 			},
 			NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager },
 			NewBackupStore: func(loc *velerov1api.BackupStorageLocation, _ persistence.ObjectStoreGetter, _ logrus.FieldLogger) (persistence.BackupStore, error) {
@@ -229,8 +229,8 @@ var _ = Describe("Backup Storage Location Reconciler", func() {
 			Ctx:    ctx,
 			Client: fake.NewFakeClientWithScheme(scheme.Scheme, locations),
 			DefaultBackupLocationInfo: storage.DefaultBackupLocationInfo{
-				StorageLocation:          "default",
-				StoreValidationFrequency: 0,
+				StorageLocation:           "default",
+				ServerValidationFrequency: 0,
 			},
 			NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager },
 			NewBackupStore: func(loc *velerov1api.BackupStorageLocation, _ persistence.ObjectStoreGetter, _ logrus.FieldLogger) (persistence.BackupStore, error) {

pkg/controller/restore_controller.go

@@ -88,7 +88,6 @@ type restoreController struct {
 	kbClient               client.Client
 	snapshotLocationLister velerov1listers.VolumeSnapshotLocationLister
 	restoreLogLevel        logrus.Level
-	defaultBackupLocation  string
 	metrics                *metrics.ServerMetrics
 	logFormat              logging.Format
 	clock                  clock.Clock

pkg/install/resources.go

@@ -159,7 +159,8 @@ func BackupStorageLocation(namespace, provider, bucket, prefix string, config ma
 					CACert: caCert,
 				},
 			},
-			Config: config,
+			Config:  config,
+			Default: true,
 		},
 	}
 }

site/content/docs/main/locations.md

@@ -79,13 +79,18 @@ velero backup-location create backups-secondary \
     --config region=us-west-1
 ```
 
-You can alter which backup storage location as default by setting the `--default` flag under the
-`velero backup-location set` command to configure another location to be the default backup storage location.
+A "default" backup storage location (BSL) is where backups get saved to when no BSL is specified at backup creation time.
+
+You can change the default backup storage location at any time by setting the `--default` flag using the
+`velero backup-location set` command and configure a different location to be the default.
+
+Examples:
+
 ```shell
 velero backup-location set backups-secondary --default
 ```
 
-Once the defaulted backup storage location existed under `velero backup-location get --default`, then changes the default backup storage location name by `velero server --default-backup-storage-location` takes no effects anymore because the velero backup storage location controller prefers to use velero client-side setting. However, if there is no defaulted backup storage location under `velero backup-location get --default`, then changes the default backup storage location name by `velero server --default-backup-storage-location` would work.
+
 
 During backup creation:
 

site/content/docs/main/upgrade-to-1.6.md

@@ -71,7 +71,11 @@ If you're not yet running at least Velero v1.5, see the following:
         Version: v1.6.0
     ```
 
-1. We've deprecated the way to indicate the default backup storage location according to the backup storage location name on the velero server-side `velero server --default-backup-storage-location`, but instead configure the default backup storage location at the velero client-side, please refer to the [About locations][9] on how to indicate which backup storage location is the default one. Moreover, during the velero upgrading process, the velero backup storage location controller helps you configure which backup storage location name matches against the backup storage location name on the velero server-side `velero server --default-backup-storage-location`, then sets the BackupStorageLocation custom resource `.spec.default` to `true`.
+## Notes
+### Default backup storage location
+We have deprecated the way to indicate the default backup storage location. Previously, that was indicated according to the backup storage location name set on the velero server-side via the flag `velero server --default-backup-storage-location`. Now we configure the default backup storage location on the velero client-side. Please refer to the [About locations][9] on how to indicate which backup storage location is the default one. 
+
+After upgrading, if there is a previously created backup storage location with the name that matches what was defined on the server side as the default, it will be automatically set as the `default`.
 
 [0]: basic-install.md#install-the-cli
 [1]: https://velero.io/docs/v1.1.0/upgrade-to-1.1/