Fix default BSL setting not work

Signed-off-by: Ming Qiu <[email protected]>

changelogs/unreleased/6771-qiuming-best

@@ -0,0 +1 @@
+Fix default BSL setting not work

internal/storage/storagelocation.go

@@ -18,6 +18,7 @@ package storage
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	"github.com/pkg/errors"
@@ -92,3 +93,18 @@ func ListBackupStorageLocations(ctx context.Context, kbClient client.Client, nam
 
 	return locations, nil
 }
+
+func GetDefaultBackupStorageLocations(ctx context.Context, kbClient client.Client, namespace string) (*velerov1api.BackupStorageLocationList, error) {
+	locations := new(velerov1api.BackupStorageLocationList)
+	defaultLocations := new(velerov1api.BackupStorageLocationList)
+	if err := kbClient.List(context.Background(), locations, &client.ListOptions{Namespace: namespace}); err != nil {
+		return defaultLocations, errors.Wrapf(err, fmt.Sprintf("failed to list backup storage locations in namespace %s", namespace))
+	}
+
+	for _, location := range locations.Items {
+		if location.Spec.Default {
+			defaultLocations.Items = append(defaultLocations.Items, location)
+		}
+	}
+	return defaultLocations, nil
+}

pkg/cmd/cli/backuplocation/create.go

@@ -31,6 +31,7 @@ import (
 
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/vmware-tanzu/velero/internal/storage"
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/builder"
 	"github.com/vmware-tanzu/velero/pkg/client"
@@ -204,19 +205,13 @@ 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 any existing default backup storage location.
-		locations := new(velerov1api.BackupStorageLocationList)
-		if err := kbClient.List(context.Background(), locations, &kbclient.ListOptions{Namespace: f.Namespace()}); err != nil {
+		// Disable any existing default backup storage location first.
+		defalutBSLs, err := storage.GetDefaultBackupStorageLocations(context.Background(), kbClient, f.Namespace())
+		if err != nil {
 			return errors.WithStack(err)
 		}
-		for i, location := range locations.Items {
-			if location.Spec.Default {
-				location.Spec.Default = false
-				if err := kbClient.Update(context.Background(), &locations.Items[i], &kbclient.UpdateOptions{}); err != nil {
-					return errors.WithStack(err)
-				}
-				break
-			}
+		if len(defalutBSLs.Items) > 0 {
+			return errors.New("there is already exist default backup storage location, please unset it first or do not set --default flag")
 		}
 	}
 

pkg/cmd/cli/backuplocation/set.go

@@ -28,6 +28,7 @@ import (
 
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/vmware-tanzu/velero/internal/storage"
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/builder"
 	"github.com/vmware-tanzu/velero/pkg/client"
@@ -104,36 +105,40 @@ func (o *SetOptions) Run(c *cobra.Command, f client.Factory) error {
 		}
 	}
 
-	location := &velerov1api.BackupStorageLocation{}
-	err = kbClient.Get(context.Background(), kbclient.ObjectKey{
-		Namespace: f.Namespace(),
-		Name:      o.Name,
-	}, location)
+	defalutBSLs, err := storage.GetDefaultBackupStorageLocations(context.Background(), kbClient, f.Namespace())
 	if err != nil {
 		return errors.WithStack(err)
 	}
 
-	if o.DefaultBackupStorageLocation {
-		// There is one and only one default backup storage location.
-		// Disable the origin 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)
+	if !o.DefaultBackupStorageLocation { // unset default backup storage location
+		if len(defalutBSLs.Items) == 0 {
+			return errors.New("no default backup storage location found for removing default")
 		}
-		for i, location := range locations.Items {
-			if !location.Spec.Default {
-				continue
-			}
-			if location.Name == o.Name {
-				// Do not update if the origin default BSL is the current one.
+		// check whether it is already unset default
+		foundDefault := false
+		for _, bsl := range defalutBSLs.Items {
+			if bsl.Name == o.Name {
+				foundDefault = true
 				break
 			}
-			location.Spec.Default = false
-			if err := kbClient.Update(context.Background(), &locations.Items[i], &kbclient.UpdateOptions{}); err != nil {
-				return errors.WithStack(err)
-			}
-			break
 		}
+		if !foundDefault {
+			return errors.New("it is already one non-default backup storage location")
+		}
+	} else { // set default backup storage location
+		// need first check if there is already a default backup storage location
+		if len(defalutBSLs.Items) > 0 {
+			return errors.New("there is already a default backup storage location")
+		}
+	}
+
+	location := &velerov1api.BackupStorageLocation{}
+	err = kbClient.Get(context.Background(), kbclient.ObjectKey{
+		Namespace: f.Namespace(),
+		Name:      o.Name,
+	}, location)
+	if err != nil {
+		return errors.WithStack(err)
 	}
 
 	location.Spec.Default = o.DefaultBackupStorageLocation

pkg/cmd/cli/backuplocation/set_test.go

@@ -102,7 +102,7 @@ func TestSetCommand_Execute(t *testing.T) {
 	_, stderr, err := veleroexec.RunCommand(cmd)
 
 	if err != nil {
-		assert.Contains(t, stderr, fmt.Sprintf("backupstoragelocations.velero.io \"%s\" not found", bsl))
+		assert.Contains(t, stderr, "no default backup storage location found")
 		return
 	}
 	t.Fatalf("process ran with err %v, want backup delete successfully", err)

pkg/controller/backup_storage_location_controller.go

@@ -96,11 +96,8 @@ func (r *backupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr
 	pluginManager := r.newPluginManager(log)
 	defer pluginManager.CleanupClients()
 
-	var defaultFound bool
+	// find the BSL that matches the request
 	for _, bsl := range locationList.Items {
-		if bsl.Spec.Default {
-			defaultFound = true
-		}
 		if bsl.Name == req.Name && bsl.Namespace == req.Namespace {
 			location = bsl
 		}
@@ -111,15 +108,11 @@ func (r *backupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr
 		return ctrl.Result{}, nil
 	}
 
-	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
+	// decide the default BSL
+	defaultFound, err := r.ensureSingleDefaultBSL(locationList)
+	if err != nil {
+		log.WithError(err).Error("failed to ensure single default bsl")
+		return ctrl.Result{}, nil
 	}
 
 	func() {
@@ -155,9 +148,6 @@ func (r *backupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr
 			log.WithError(err).Error("fail to validate backup store")
 			return
 		}
-
-		// updates the default backup location
-		location.Spec.Default = isDefault
 	}()
 
 	r.logReconciledPhase(defaultFound, locationList, unavailableErrors)
@@ -220,3 +210,68 @@ func (r *backupStorageLocationReconciler) SetupWithManager(mgr ctrl.Manager) err
 		Watches(g, nil, builder.WithPredicates(gp)).
 		Complete(r)
 }
+
+// ensureSingleDefaultBSL ensures that there is only one default BSL in the namespace.
+// the default BSL priority is as follows:
+// 1. follow the user's setting (the most recent validation BSL is the default BSL)
+// 2. follow the server's setting ("velero server --default-backup-storage-location")
+func (r *backupStorageLocationReconciler) ensureSingleDefaultBSL(locationList velerov1api.BackupStorageLocationList) (bool, error) {
+	// get all default BSLs
+	var defaultBSLs []*velerov1api.BackupStorageLocation
+	var defaultFound bool
+	for i, location := range locationList.Items {
+		if location.Spec.Default {
+			defaultBSLs = append(defaultBSLs, &locationList.Items[i])
+		}
+	}
+
+	if len(defaultBSLs) > 1 { // more than 1 default BSL
+		// find the most recent updated default BSL
+		var mostRecentUpdatedBSL *velerov1api.BackupStorageLocation
+		defaultFound = true
+		for _, bsl := range defaultBSLs {
+			if mostRecentUpdatedBSL == nil {
+				mostRecentUpdatedBSL = bsl
+				continue
+			}
+			if mostRecentUpdatedBSL.Status.LastValidationTime.Before(bsl.Status.LastValidationTime) {
+				mostRecentUpdatedBSL = bsl
+			}
+		}
+
+		// unset all other default BSLs
+		for _, bsl := range defaultBSLs {
+			if bsl.Name != mostRecentUpdatedBSL.Name {
+				bsl.Spec.Default = false
+				if err := r.client.Update(r.ctx, bsl); err != nil {
+					return defaultFound, errors.Wrapf(err, "failed to unset default backup storage location %q", bsl.Name)
+				}
+				r.log.Debugf("update default backup storage location %q to false", bsl.Name)
+			}
+		}
+	} else if len(defaultBSLs) == 0 { // no default BSL
+		// find the BSL that matches the "velero server --default-backup-storage-location"
+		var defaultBSL *velerov1api.BackupStorageLocation
+		for i, location := range locationList.Items {
+			if location.Name == r.defaultBackupLocationInfo.StorageLocation {
+				defaultBSL = &locationList.Items[i]
+				break
+			}
+		}
+
+		// set the default BSL
+		if defaultBSL != nil {
+			defaultBSL.Spec.Default = true
+			defaultFound = true
+			if err := r.client.Update(r.ctx, defaultBSL); err != nil {
+				return defaultFound, errors.Wrapf(err, "failed to set default backup storage location %q", defaultBSL.Name)
+			}
+			r.log.Debugf("update default backup storage location %q to true", defaultBSL.Name)
+		} else {
+			defaultFound = false
+		}
+	} else { // only 1 default BSL
+		defaultFound = true
+	}
+	return defaultFound, nil
+}

site/content/docs/main/customize-installation.md

@@ -173,18 +173,43 @@ To configure additional locations after running `velero install`, use the `veler
 
 ### Set default backup storage location or volume snapshot locations
 
-When performing backups, Velero needs to know where to backup your data. This means that if you configure multiple locations, you must specify the location Velero should use each time you run `velero backup create`, or you can set a default backup storage location or default volume snapshot locations. If you only have one backup storage llocation or volume snapshot location set for a provider, Velero will automatically use that location as the default.
+When performing backups, Velero needs to know where to backup your data. This means that if you configure multiple locations, you must specify the location Velero should use each time you run `velero backup create`, or you can set a default backup storage location or default volume snapshot locations. If you only have one backup storage location or volume snapshot location set for a provider, Velero will automatically use that location as the default.
 
-Set a default backup storage location by passing a `--default` flag with when running `velero backup-location create`.
+#### Set default backup storage location
+currently, Velero could set the default backup storage location as below:
+- First way: Set a default backup storage location by passing a `--default` flag when running `velero backup-location create`.
 
-```
-velero backup-location create backups-primary \
+  ```
+  velero backup-location create backups-primary \
     --provider aws \
     --bucket velero-backups \
     --config region=us-east-1 \
     --default
-```
-
+  ```
+- Second way: Set a default backup storage location by passing a  `--default` flag when running `velero backup-location set`.
+  ```bash
+  velero backup-location set backups-primary --default
+  ```
+  We also could remove the default backup storage location by this command, below is one example
+  ```bash
+  velero backup-location set backups-primary --default=false
+  ```
+- Third way: Set a default backup storage location by passing `--default-backup-storage-location` flag on the `velero server` command.
+   ```bash
+  velero server --default-backup-storage-location backups-primary
+   ```
+Note: Only could have one default backup storage location, which means it's not allowed to set two default backup storage locations at the same time, the priorities among these three are as follows:
+- if velero server side has specified one default backup storage location, suppose it's `A`
+  - if `A` backup storage location exists, it's not allowed to set a new default backup storage location
+  - if `A` does not exist
+    - if using `velero backup-location set` or `velero backup-location create --default` command
+      - it could be successful if no default backup storage location exists.
+      - it would fail if already exist one default backup storage location. (So it need to remove other default backup storage location at first)
+- if velero server side has not specified one default backup storage location
+  - if using `velero backup-location set` or `velero backup-location create --default` command
+    - it could be successful if no default backup storage location exists.
+    - it would fail if already exist one default backup storage location. (So it need to remove other default backup storage location at first)
+#### Set default volume snapshot location
 You can set a default volume snapshot location for each of your volume snapshot providers using the `--default-volume-snapshot-locations` flag on the `velero server` command.
 
 ```