diff --git a/changelogs/unreleased/6771-qiuming-best b/changelogs/unreleased/6771-qiuming-best new file mode 100644 index 00000000000..73c18f973e4 --- /dev/null +++ b/changelogs/unreleased/6771-qiuming-best @@ -0,0 +1 @@ +Fix default BSL setting not work diff --git a/internal/storage/storagelocation.go b/internal/storage/storagelocation.go index 1def65d17b8..8847ba6892c 100644 --- a/internal/storage/storagelocation.go +++ b/internal/storage/storagelocation.go @@ -18,6 +18,7 @@ package storage import ( "context" + "fmt" "time" "github.com/pkg/errors" @@ -28,6 +29,8 @@ import ( velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" ) +const defaultTimestampAnnotation = "bsl.velero.io/default-timestamp" + // DefaultBackupLocationInfo holds server default backup storage location information type DefaultBackupLocationInfo struct { // StorageLocation is the name of the backup storage location designated as the default on the server side. @@ -92,3 +95,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 +} diff --git a/pkg/cmd/cli/backuplocation/create.go b/pkg/cmd/cli/backuplocation/create.go index 7e305d1f9c5..343bc790af6 100644 --- a/pkg/cmd/cli/backuplocation/create.go +++ b/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") } } diff --git a/pkg/cmd/cli/backuplocation/set.go b/pkg/cmd/cli/backuplocation/set.go index 60703d6c53a..ebbe55dca29 100644 --- a/pkg/cmd/cli/backuplocation/set.go +++ b/pkg/cmd/cli/backuplocation/set.go @@ -28,11 +28,13 @@ 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" "github.com/vmware-tanzu/velero/pkg/cmd" "github.com/vmware-tanzu/velero/pkg/cmd/util/flag" + "github.com/vmware-tanzu/velero/pkg/util/boolptr" ) func NewSetCommand(f client.Factory, use string) *cobra.Command { @@ -58,7 +60,7 @@ type SetOptions struct { Name string CACertFile string Credential flag.Map - DefaultBackupStorageLocation bool + DefaultBackupStorageLocation flag.OptionalBool } func NewSetOptions() *SetOptions { @@ -70,7 +72,8 @@ func NewSetOptions() *SetOptions { func (o *SetOptions) BindFlags(flags *pflag.FlagSet) { flags.StringVar(&o.CACertFile, "cacert", o.CACertFile, "File containing a certificate bundle to use when verifying TLS connections to the object store. Optional.") flags.Var(&o.Credential, "credential", "Sets the credential to be used by this location as a key-value pair, where the key is the Kubernetes Secret name, and the value is the data key name within the Secret. Optional, one value only.") - flags.BoolVar(&o.DefaultBackupStorageLocation, "default", o.DefaultBackupStorageLocation, "Sets this new location to be the new default backup storage location. Optional.") + f := flags.VarPF(&o.DefaultBackupStorageLocation, "default", "", "Sets this new location to be the new default backup storage location. Optional.") + f.NoOptDefVal = cmd.TRUE } func (o *SetOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { @@ -104,6 +107,11 @@ func (o *SetOptions) Run(c *cobra.Command, f client.Factory) error { } } + defalutBSLs, err := storage.GetDefaultBackupStorageLocations(context.Background(), kbClient, f.Namespace()) + if err != nil { + return errors.WithStack(err) + } + location := &velerov1api.BackupStorageLocation{} err = kbClient.Get(context.Background(), kbclient.ObjectKey{ Namespace: f.Namespace(), @@ -113,30 +121,21 @@ func (o *SetOptions) Run(c *cobra.Command, f client.Factory) error { 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) - } - 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. - break - } - location.Spec.Default = false - if err := kbClient.Update(context.Background(), &locations.Items[i], &kbclient.UpdateOptions{}); err != nil { - return errors.WithStack(err) + defaultOpt := o.DefaultBackupStorageLocation.Value + if defaultOpt != nil { + if *defaultOpt { // 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") } - break } + } else { + // user do not specify default option + // we should keep the original default option + o.DefaultBackupStorageLocation = flag.OptionalBool{Value: &location.Spec.Default} } - location.Spec.Default = o.DefaultBackupStorageLocation + location.Spec.Default = boolptr.IsSetToTrue(o.DefaultBackupStorageLocation.Value) location.Spec.StorageType.ObjectStorage.CACert = caCertData for name, key := range o.Credential.Data() { diff --git a/pkg/cmd/cli/backuplocation/set_test.go b/pkg/cmd/cli/backuplocation/set_test.go index b57f6a3208b..cac77d12925 100644 --- a/pkg/cmd/cli/backuplocation/set_test.go +++ b/pkg/cmd/cli/backuplocation/set_test.go @@ -31,6 +31,7 @@ import ( cmdtest "github.com/vmware-tanzu/velero/pkg/cmd/test" veleroflag "github.com/vmware-tanzu/velero/pkg/cmd/util/flag" velerotest "github.com/vmware-tanzu/velero/pkg/test" + "github.com/vmware-tanzu/velero/pkg/util/boolptr" veleroexec "github.com/vmware-tanzu/velero/pkg/util/exec" ) @@ -73,7 +74,7 @@ func TestNewSetCommand(t *testing.T) { // verify all options are set as expected assert.Equal(t, backupName, o.Name) assert.Equal(t, cacert, o.CACertFile) - assert.Equal(t, defaultBackupStorageLocation, o.DefaultBackupStorageLocation) + assert.Equal(t, defaultBackupStorageLocation, boolptr.IsSetToTrue(o.DefaultBackupStorageLocation.Value)) assert.Equal(t, true, reflect.DeepEqual(credential, o.Credential)) assert.Contains(t, e.Error(), fmt.Sprintf("%s: no such file or directory", cacert)) @@ -102,7 +103,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, "backupstoragelocations.velero.io \"bsl-1\" not found") return } t.Fatalf("process ran with err %v, want backup delete successfully", err) diff --git a/pkg/controller/backup_storage_location_controller.go b/pkg/controller/backup_storage_location_controller.go index 26cc1e73415..1e17f53a953 100644 --- a/pkg/controller/backup_storage_location_controller.go +++ b/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,70 @@ 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 + } + // For lack of a better way to compare timestamps, we use the CreationTimestamp + // it cloud not really find the most recent updated BSL, but it is good enough for now + if mostRecentUpdatedBSL.CreationTimestamp.Before(&bsl.CreationTimestamp) { + 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 +} diff --git a/pkg/controller/backup_storage_location_controller_test.go b/pkg/controller/backup_storage_location_controller_test.go index 06d3458c4de..f7a6f27f917 100644 --- a/pkg/controller/backup_storage_location_controller_test.go +++ b/pkg/controller/backup_storage_location_controller_test.go @@ -17,12 +17,15 @@ limitations under the License. package controller import ( + "context" + "testing" "time" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "github.com/pkg/errors" "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -172,3 +175,125 @@ var _ = Describe("Backup Storage Location Reconciler", func() { } }) }) + +func TestEnsureSingleDefaultBSL(t *testing.T) { + tests := []struct { + name string + locations velerov1api.BackupStorageLocationList + defaultBackupInfo storage.DefaultBackupLocationInfo + expectedDefaultSet bool + expectedError error + }{ + { + name: "MultipleDefaults", + locations: func() velerov1api.BackupStorageLocationList { + var locations velerov1api.BackupStorageLocationList + locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-1").LastValidationTime(time.Now()).Default(true).Result()) + locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-2").LastValidationTime(time.Now().Add(-1 * time.Hour)).Default(true).Result()) + return locations + }(), + expectedDefaultSet: true, + expectedError: nil, + }, + { + name: "NoDefault with exist default bsl in defaultBackupInfo", + locations: func() velerov1api.BackupStorageLocationList { + var locations velerov1api.BackupStorageLocationList + locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-1").Default(false).Result()) + locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-2").Default(false).Result()) + return locations + }(), + defaultBackupInfo: storage.DefaultBackupLocationInfo{ + StorageLocation: "location-2", + }, + expectedDefaultSet: true, + expectedError: nil, + }, + { + name: "NoDefault with non-exist default bsl in defaultBackupInfo", + locations: func() velerov1api.BackupStorageLocationList { + var locations velerov1api.BackupStorageLocationList + locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-1").Default(false).Result()) + locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-2").Default(false).Result()) + return locations + }(), + defaultBackupInfo: storage.DefaultBackupLocationInfo{ + StorageLocation: "location-3", + }, + expectedDefaultSet: false, + expectedError: nil, + }, + { + name: "SingleDefault", + locations: func() velerov1api.BackupStorageLocationList { + var locations velerov1api.BackupStorageLocationList + locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-1").Default(true).Result()) + locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-2").Default(false).Result()) + return locations + }(), + expectedDefaultSet: true, + expectedError: nil, + }, + } + + for _, test := range tests { + // Setup reconciler + assert.Nil(t, velerov1api.AddToScheme(scheme.Scheme)) + t.Run(test.name, func(t *testing.T) { + r := &backupStorageLocationReconciler{ + ctx: context.Background(), + client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(&test.locations).Build(), + defaultBackupLocationInfo: test.defaultBackupInfo, + log: velerotest.NewLogger(), + } + defaultFound, err := r.ensureSingleDefaultBSL(test.locations) + + assert.Equal(t, test.expectedDefaultSet, defaultFound) + assert.Equal(t, test.expectedError, err) + }) + } +} + +func TestBSLReconcile(t *testing.T) { + tests := []struct { + name string + locationList velerov1api.BackupStorageLocationList + defaultFound bool + expectedError error + }{ + { + name: "NoBSL", + locationList: velerov1api.BackupStorageLocationList{}, + defaultFound: false, + expectedError: nil, + }, + { + name: "BSLNotFound", + locationList: func() velerov1api.BackupStorageLocationList { + var locations velerov1api.BackupStorageLocationList + locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-2").Result()) + return locations + }(), + defaultFound: false, + expectedError: nil, + }, + } + pluginManager := &pluginmocks.Manager{} + pluginManager.On("CleanupClients").Return(nil) + for _, test := range tests { + // Setup reconciler + assert.Nil(t, velerov1api.AddToScheme(scheme.Scheme)) + t.Run(test.name, func(t *testing.T) { + r := &backupStorageLocationReconciler{ + ctx: context.Background(), + client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(&test.locationList).Build(), + newPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + log: velerotest.NewLogger(), + } + + result, err := r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: velerov1api.DefaultNamespace, Name: "location-1"}}) + assert.Equal(t, test.expectedError, err) + assert.Equal(t, ctrl.Result{}, result) + }) + } +} diff --git a/site/content/docs/main/customize-installation.md b/site/content/docs/main/customize-installation.md index 0d8621685d7..1ddbaf4b179 100644 --- a/site/content/docs/main/customize-installation.md +++ b/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. ```