Fix default BSL setting not work #6771

qiuming-best · 2023-09-06T01:32:44Z

Thank you for contributing to Velero!

Please add a summary of your change

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 \
  --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.
```
velero backup-location set backups-primary --default
```
We also could remove the default backup storage location by this command, below is one example
```
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.
```
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)

Does your change fix a particular issue?

Fixes #(issue)
#6572

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
Updated the corresponding documentation in site/content/docs/main.

qiuming-best · 2023-09-06T01:35:12Z

pkg/cmd/cli/backuplocation/create.go

-		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 {


bug with using &locations.Items[i], which could not set the specific location to Default false

qiuming-best · 2023-09-06T01:36:34Z

pkg/cmd/cli/backuplocation/create.go

-				if err := kbClient.Update(context.Background(), &locations.Items[i], &kbclient.UpdateOptions{}); err != nil {
-					return errors.WithStack(err)
-				}
-				break


we could not break here, per the comment above // There is one and only one default backup storage location. // Disable any existing default backup storage location., If break here only one BSL could be unset

qiuming-best · 2023-09-06T01:37:15Z

pkg/cmd/cli/backuplocation/set.go

 				break
 			}
-			location.Spec.Default = false
-			if err := kbClient.Update(context.Background(), &locations.Items[i], &kbclient.UpdateOptions{}); err != nil {


bug with using &locations.Items[i], which could not set the specific location to Default false

qiuming-best · 2023-09-06T01:37:48Z

pkg/cmd/cli/backuplocation/set.go

-			if err := kbClient.Update(context.Background(), &locations.Items[i], &kbclient.UpdateOptions{}); err != nil {
-				return errors.WithStack(err)
-			}
-			break


we could not break here, per the comment above // There is one and only one default backup storage location. // Disable any existing default backup storage location., If break here only one BSL could be unset

qiuming-best · 2023-09-06T01:40:36Z

After going through the original codes, it wants to always keep one default BSL, but it does not work to unset the default BSL for the bug mentioned.

The difference between the original codes is that Velero does not unset the default BSL actively but fails, which means the user should unset the default BSL at first. and new codes still keep the three ways of setting priority as original.

codecov · 2023-09-06T02:05:06Z

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (b9b2c88) 60.35% compared to head (c6cba30) 61.78%.
Report is 264 commits behind head on main.

Files	Patch %	Lines
pkg/cmd/cli/backuplocation/set.go	12.50%	14 Missing ⚠️
internal/storage/storagelocation.go	0.00%	11 Missing ⚠️
...g/controller/backup_storage_location_controller.go	79.62%	8 Missing and 3 partials ⚠️
pkg/cmd/cli/backuplocation/create.go	40.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6771      +/-   ##
==========================================
+ Coverage   60.35%   61.78%   +1.42%     
==========================================
  Files         242      259      +17     
  Lines       25994    27940    +1946     
==========================================
+ Hits        15689    17262    +1573     
- Misses       9200     9468     +268     
- Partials     1105     1210     +105

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/cmd/cli/backuplocation/set.go

blackpiglet · 2023-11-29T02:51:07Z

pkg/cmd/cli/backuplocation/set.go

-		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 the velero backup-location set command doesn't set the --default parameter, it can also hit this logic.
I think it can confuse the user because the user could meet the default BSL validation error when the user doesn't have the intention to set the default BSL.

you are right, I've modified it, please help me to check it again

ywk253100 · 2023-11-29T06:04:29Z

pkg/cmd/cli/backuplocation/set.go

-		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 {


Seems the set command only targets one specific BSL, why do you need to check others?

this checking logic is for when the user uses the command velero backup-location set $BSL --default=false
to unset the default for one specific BSL

I've removed the unset logic check, we allow the user unset one specific BSL repeatedly

ywk253100 · 2023-12-01T00:26:50Z

pkg/cmd/cli/backuplocation/set.go

@@ -104,6 +107,11 @@ func (o *SetOptions) Run(c *cobra.Command, f client.Factory) error {
 		}
 	}

+	defalutBSLs, err := storage.GetDefaultBackupStorageLocations(context.Background(), kbClient, f.Namespace())


Move this logic inside the if block in line 126 to avoid unnecessary BSL listing

pkg/controller/backup_storage_location_controller.go

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

github-actions bot added the Documentation label Sep 6, 2023

github-actions bot requested review from anshulahuja98 and ywk253100 September 6, 2023 01:32

github-actions bot assigned qiuming-best Sep 6, 2023

qiuming-best commented Sep 6, 2023

View reviewed changes

qiuming-best force-pushed the bsl-fix branch from c4df504 to 3fd7aec Compare September 6, 2023 01:54

github-actions bot added has-changelog has-unit-tests labels Sep 6, 2023

qiuming-best force-pushed the bsl-fix branch 2 times, most recently from 28afd82 to f4a1de4 Compare September 6, 2023 04:02

qiuming-best requested review from blackpiglet, Lyndon-Li and reasonerjt September 6, 2023 06:05

qiuming-best commented Nov 29, 2023

View reviewed changes

pkg/cmd/cli/backuplocation/set.go Outdated Show resolved Hide resolved

blackpiglet reviewed Nov 29, 2023

View reviewed changes

ywk253100 reviewed Nov 29, 2023

View reviewed changes

qiuming-best force-pushed the bsl-fix branch 4 times, most recently from b3365c4 to 73d520a Compare November 29, 2023 06:37

blackpiglet removed the request for review from reasonerjt November 29, 2023 08:18

qiuming-best force-pushed the bsl-fix branch 5 times, most recently from 32dbfd7 to 831a011 Compare November 30, 2023 03:26

ywk253100 reviewed Dec 1, 2023

View reviewed changes

pkg/controller/backup_storage_location_controller.go Outdated Show resolved Hide resolved

Fix default BSL setting not work

c6cba30

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

qiuming-best force-pushed the bsl-fix branch from 831a011 to c6cba30 Compare December 1, 2023 02:06

reasonerjt added this to the v1.13 milestone Dec 4, 2023

reasonerjt added the target/FC label Dec 4, 2023

ywk253100 approved these changes Dec 5, 2023

View reviewed changes

blackpiglet approved these changes Dec 5, 2023

View reviewed changes

qiuming-best merged commit ea04a86 into vmware-tanzu:main Dec 5, 2023
24 of 25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix default BSL setting not work #6771

Fix default BSL setting not work #6771

qiuming-best commented Sep 6, 2023 •

edited

Loading

qiuming-best Sep 6, 2023

qiuming-best Sep 6, 2023

qiuming-best Sep 6, 2023

qiuming-best Sep 6, 2023

qiuming-best commented Sep 6, 2023 •

edited

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading

blackpiglet Nov 29, 2023

qiuming-best Nov 29, 2023

ywk253100 Nov 29, 2023

qiuming-best Nov 29, 2023

qiuming-best Nov 30, 2023

ywk253100 Dec 1, 2023

qiuming-best Dec 1, 2023

Fix default BSL setting not work #6771

Fix default BSL setting not work #6771

Conversation

qiuming-best commented Sep 6, 2023 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiuming-best commented Sep 6, 2023 • edited Loading

codecov bot commented Sep 6, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiuming-best commented Sep 6, 2023 •

edited

Loading

qiuming-best commented Sep 6, 2023 •

edited

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading