From 3fd7aecae8cdd322029f4b6d551428555cec9597 Mon Sep 17 00:00:00 2001 From: Ming Qiu Date: Tue, 5 Sep 2023 14:43:40 +0800 Subject: [PATCH] Fix default BSL setting not work Signed-off-by: Ming Qiu --- changelogs/unreleased/6771-qiuming-best | 1 + internal/storage/storagelocation.go | 16 ++++ pkg/cmd/cli/backuplocation/create.go | 17 ++-- pkg/cmd/cli/backuplocation/set.go | 49 ++++++----- pkg/cmd/cli/backuplocation/set_test.go | 2 +- .../backup_storage_location_controller.go | 87 +++++++++++++++---- .../docs/main/customize-installation.md | 37 ++++++-- 7 files changed, 153 insertions(+), 56 deletions(-) create mode 100644 changelogs/unreleased/6771-qiuming-best 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..006ce773edf 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" @@ -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 +} 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..2fd4a1d58bd 100644 --- a/pkg/cmd/cli/backuplocation/set.go +++ b/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 diff --git a/pkg/cmd/cli/backuplocation/set_test.go b/pkg/cmd/cli/backuplocation/set_test.go index b57f6a3208b..86eaf4dacbb 100644 --- a/pkg/cmd/cli/backuplocation/set_test.go +++ b/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) diff --git a/pkg/controller/backup_storage_location_controller.go b/pkg/controller/backup_storage_location_controller.go index 26cc1e73415..faefd099097 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,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 +} 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. ```