Improvements to BSL logic

changelogs/unreleased/3172-carlisia

@@ -0,0 +1 @@
+Set the BSL created at install time as the "default"

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,38 +51,38 @@ func TestIsReadyToValidate(t *testing.T) {
 			bslValidationFrequency: &metav1.Duration{Duration: 0},
 			lastValidationTime:     &metav1.Time{Time: time.Now()},
 			defaultLocationInfo: DefaultBackupLocationInfo{
-				StoreValidationFrequency: 0,
+				ServerValidationFrequency: 0,
 			},
 			ready: false,
 		},
 		{
 			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,
 		},
 		{
 			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,
 		},
 		{
 			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,44 +363,22 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg
 				}
 			}
 		}
+		serverSpecified = true
 	}
 
-	if request.Spec.DefaultVolumesToRestic == nil {
-		request.Spec.DefaultVolumesToRestic = &c.defaultVolumesToRestic
-	}
-
-	// add the storage location as a label for easy filtering later.
-	if request.Labels == nil {
-		request.Labels = make(map[string]string)
-	}
-	request.Labels[velerov1api.StorageLocationLabel] = label.GetValidName(request.Spec.StorageLocation)
-
-	// Getting all information of cluster version - useful for future skip-level migration
-	if request.Annotations == nil {
-		request.Annotations = make(map[string]string)
-	}
-	request.Annotations[velerov1api.SourceClusterK8sGitVersionAnnotation] = c.discoveryHelper.ServerVersion().String()
-	request.Annotations[velerov1api.SourceClusterK8sMajorVersionAnnotation] = c.discoveryHelper.ServerVersion().Major
-	request.Annotations[velerov1api.SourceClusterK8sMinorVersionAnnotation] = c.discoveryHelper.ServerVersion().Minor
-
-	// validate the included/excluded resources
-	for _, err := range collections.ValidateIncludesExcludes(request.Spec.IncludedResources, request.Spec.ExcludedResources) {
-		request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid included/excluded resource lists: %v", err))
-	}
-
-	// validate the included/excluded namespaces
-	for _, err := range collections.ValidateIncludesExcludes(request.Spec.IncludedNamespaces, request.Spec.ExcludedNamespaces) {
-		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
+	// 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) {
-			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))
+			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))
 		}
@@ -405,6 +390,13 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg
 				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.
+	if request.Labels == nil {
+		request.Labels = make(map[string]string)
+	}
+	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 {
@@ -417,6 +409,24 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg
 		}
 	}
 
+	// Getting all information of cluster version - useful for future skip-level migration
+	if request.Annotations == nil {
+		request.Annotations = make(map[string]string)
+	}
+	request.Annotations[velerov1api.SourceClusterK8sGitVersionAnnotation] = c.discoveryHelper.ServerVersion().String()
+	request.Annotations[velerov1api.SourceClusterK8sMajorVersionAnnotation] = c.discoveryHelper.ServerVersion().Major
+	request.Annotations[velerov1api.SourceClusterK8sMinorVersionAnnotation] = c.discoveryHelper.ServerVersion().Minor
+
+	// validate the included/excluded resources
+	for _, err := range collections.ValidateIncludesExcludes(request.Spec.IncludedResources, request.Spec.ExcludedResources) {
+		request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid included/excluded resource lists: %v", err))
+	}
+
+	// validate the included/excluded namespaces
+	for _, err := range collections.ValidateIncludesExcludes(request.Spec.IncludedNamespaces, request.Spec.ExcludedNamespaces) {
+		request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid included/excluded namespace lists: %v", err))
+	}
+
 	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,
 		},
 	}
 }