feat: support configure BSL CR to indicate which one is the default #3092

jenting · 2020-11-18T02:21:59Z

Add "default" for backup storage location

Add --default flag under velero backup-location create to sets this new location to be the new default backup location.
Add sub-command velero backup-location set and flag --default to sets this new location to be the new default backup location.
Add --default flag under velero backup-location get to displays the current default backup storage location.
Add a new print column DEFAULT under velero backup get and kubectl get backupstoragelocations to indicate which backup location is the default one, for example:

$ velero backup-location get
NAME        PROVIDER   BUCKET/PREFIX   PHASE       LAST VALIDATED                  ACCESS MODE   DEFAULT
default     aws        velero          Available   2020-11-17 15:21:12 +0800 CST   ReadWrite     true
secondary   aws        velero          Available   2020-11-17 15:20:42 +0800 CST   ReadWrite

$ kubectl get backupstoragelocations
NAME        PROVIDER   BUCKET   PHASE       LAST VALIDATED   ACCESS MODE   AGE    DEFAULT
default     aws        velero   Available   21s              ReadWrite     120m   true
secondary   aws        velero   Available   51s              ReadWrite     93m

It covers the upgrade case: after CRD upgrade, the BSL controller will set default BSL if the BSL name matches the designated backup storage location (velero server --default-backup-storage-location). After that, the user could specify another BSL as default by velero backup-location set <NAME> --default. (The unit tests cover this scenario).

Fixes #2421 (updated on 11/18: cleaned up 2421 to only contain this fix)
ref to #2419

carlisia

This is such great work, thank you! I'm very much looking forward to adding this.

I have a minor request.

I also found a bug. steps to reproduce:

be sure to have some existing BSLs created against main or 1.5.x
on this branch, run make local
update the CRDs
run v get backup-location --default
get panic here: https://github.com/vmware-tanzu/velero/pull/3092/files#diff-692b6ad01521aa5bc42b3f67637f50c59c3dc1481f0d372e093a011e3109e61eR74

❯ v get backup-location --default
panic: runtime error: slice bounds out of range [2:1]

goroutine 1 [running]:
github.com/vmware-tanzu/velero/pkg/cmd/cli/backuplocation.NewGetCommand.func1(0xc0009a7340, 0xc000885220, 0x0, 0x1)
        /Users/carlisiac/work/src/github.com/vmware-tanzu/velero/pkg/cmd/cli/backuplocation/get.go:74 +0x7d0
github.com/spf13/cobra.(*Command).execute(0xc0009a7340, 0xc000885200, 0x1, 0x1, 0xc0009a7340, 0xc000885200)
        /Users/carlisiac/work/pkg/mod/github.com/spf13/[email protected]/command.go:842 +0x29d
github.com/spf13/cobra.(*Command).ExecuteC(0xc0000d0840, 0x6, 0xc0000d0840, 0x6)
        /Users/carlisiac/work/pkg/mod/github.com/spf13/[email protected]/command.go:943 +0x317
github.com/spf13/cobra.(*Command).Execute(...)
        /Users/carlisiac/work/pkg/mod/github.com/spf13/[email protected]/command.go:883
main.main()
        /Users/carlisiac/work/src/github.com/vmware-tanzu/velero/cmd/velero/main.go:34 +0x91

pkg/cmd/util/output/backup_storage_location_printer.go

jenting · 2020-11-20T05:16:57Z

This is ready to be reviewed.

carlisia

@jenting thanks for fixing the panic.

I'm not sure about the "per provider" setting option. To recap, the purpose of having a default BSL is so one doesn't have to be specified in the cmd line when creating a backup. So, a user can have multiple BSLs, each for different providers, and only 1 of them should be the default, regardless of the provider. Otherwise, the user would have to specify what provider to use, and then the BSL used would be the default. Needing to define the provider would sorta defeat the purpose.

carlisia

There is one additional aspect to consider: when running a backup, if the backup does not include a BSL specified with the --storage-location flag), then it will use what the server understands as the default location. Currently, the name of the BSL that is supposed to be the default is set on the server here (

velero/pkg/cmd/server/server.go

Line 131 in a1e182e

defaultBackupLocation: "default",

).

This check is done here:

velero/pkg/controller/backup_controller.go

Line 346 in a1e182e

if request.Spec.StorageLocation == "" {

So, in the backup controller, we will now want to check:

is there a given BSL being passed in with the flag for the backup being created? If yes, use that
if not, check if one of the existing BSLs is marked as default. If yes, use that
If not, use the server default; this BSL might or not exist, but... to maintain the original logic, just set the backup to that.

The same idea applies with the sync:

velero/pkg/controller/backup_sync_controller.go

Line 133 in a1e182e

locations := orderedBackupLocations(&locationList, c.defaultBackupLocation)

to find out which default location to sync first, first search the existing locations for one marked as "default"; if none exists, use the name of the server one (this might or not exist).

carlisia · 2020-11-20T17:43:33Z

@vmware-tanzu/velero-maintainers, bringing this up here since it's in context: having now (after this is merged) the ability to set which existing BSL is the default, what do you think of removing the server configuration with the default BSL name? While I understand how it made it super easy to "magically" get a BSL setup with the install cmd, it has always bugged me that the configuration for the default was in name only, in other words, a BSL itself with that name might or not exist.

The change would be: At backup creation, if there is no BSL configured to be the default, we would warn for the user to either set it or specify it with the proper flag, and not allow the backup creation. I think this is infinitely better usability than allowing them to create a backup for a BSL that might not even exist, then trying to communicate that with log messages on the controller side.

This change does not have to be in this PR. I'd be glad to work on it so it is merged right after this one.

nrb

it has always bugged me that the configuration for the default was in name only, in other words, a BSL itself with that name might or not exist.

That's not the only way to set the default. On the velero server command, the --default-backup-storage-location argument can be used to specify which other BackupStorageLocation to use. Using the location named default gets the 80-90% use case where users don't really care what it's named.

That said: I like the approach in this PR better :) It should remove the need to restart Velero for changing default BackupStorageLoctions. And it moves that change to a client-side command that's not kubectl edit.

I do want to be sure we handle the transition gracefully, though - there needs to be a deprecation period of at least 1 release, with documentation and notice. So I want to have both be usable in v1.6.0, personally. That's probably going to require some code that makes the existing flag edit the BSL CRs in much the same way this does PR does. However, I also don't think that compatibility needs to be done in this same PR - it just needs to be done by v1.6.0 release.

@jenting, can you please add documentation for using this into https://github.com/vmware-tanzu/velero/blob/main/site/content/docs/main/locations.md ?

carlisia · 2020-11-20T19:17:04Z

can be used to specify which other BackupStorageLocation to use

Correct. And yet, any name can go there meaning, there might not be an existing BSL with that name. When it works, it's great, but when it doesn't, it's quirky to try and communicate that.

jenting · 2020-11-23T01:40:51Z

The same idea applies with the sync:

velero/pkg/controller/backup_sync_controller.go

Line 133 in a1e182e

locations := orderedBackupLocations(&locationList, c.defaultBackupLocation)

to find out which default location to sync first, first search the existing locations for one marked as "default"; if none exists, use the name of the server one (this might or not exist).

Good call, I'll double-check on it.

jenting · 2020-11-23T01:43:59Z

@jenting thanks for fixing the panic.

I'm not sure about the "per provider" setting option. To recap, the purpose of having a default BSL is so one doesn't have to be specified in the cmd line when creating a backup. So, a user can have multiple BSLs, each for different providers, and only 1 of them should be the default, regardless of the provider. Otherwise, the user would have to specify what provider to use, and then the BSL used would be the default. Needing to define the provider would sorta defeat the purpose.

okay, I think I might be overthinking on each provider should have a defaulted BSL, I'll remove it.

jenting · 2020-11-23T01:48:18Z

To recap, from the design doc https://github.com/vmware-tanzu/velero/blob/master/design/cli-install-changes.md#deprecation, the time to deprecated the commands and flags was scheduled at Velero 2.0.

So we should make sure both the server-side and the client-side change the default BSL both work correctly.

nrb

I think this looks really good, and thank you for pointing out the design doc deprecation schedule!

I noted a few changes inline, but also I have another - could you create a new file, site/contents/docs/main/upgrade-to-1.6.md and document this new command, since it's an added feature and users upgrading should know? I think after these changes, I'm 👍 .

changelogs/unreleased/3092-jenting

nrb · 2020-11-30T19:11:30Z

pkg/cmd/server/server.go

@@ -195,7 +195,7 @@ func NewCommand(f client.Factory) *cobra.Command {
 	command.Flags().BoolVar(&config.restoreOnly, "restore-only", config.restoreOnly, "Run in a mode where only restores are allowed; backups, schedules, and garbage-collection are all disabled. DEPRECATED: this flag will be removed in v2.0. Use read-only backup storage locations instead.")
 	command.Flags().StringSliceVar(&config.disabledControllers, "disable-controllers", config.disabledControllers, fmt.Sprintf("List of controllers to disable on startup. Valid values are %s", strings.Join(controller.DisableableControllers, ",")))
 	command.Flags().StringSliceVar(&config.restoreResourcePriorities, "restore-resource-priorities", config.restoreResourcePriorities, "Desired order of resource restores; any resource not in the list will be restored alphabetically after the prioritized resources.")
-	command.Flags().StringVar(&config.defaultBackupLocation, "default-backup-storage-location", config.defaultBackupLocation, "Name of the default backup storage location.")
+	command.Flags().StringVar(&config.defaultBackupLocation, "default-backup-storage-location", config.defaultBackupLocation, "Name of the default backup storage location. DEPRECATED: this flag will be removed in v2.0. Use \"velero backup-location set --default\" instead.")


What happens if both of these are set? We should document that behavior.

Let me summarize the scenario I've thought:

During the upgrade, the BSL controller sets the defaulted BSL according to the BSL name because none of the BSLs be marked as default.

The user could change the defaulted BSL by velero CLI.

Now that we have the default BSL. If the user changes the velero server --default-backup-storage-location, it takes no effect anymore. The only way to configure which BSL is the default is to change BSL CR (velero CLI or kubectl edit backupstoragelocations).

The only way we can't prevent is the user directly edit BSL CRs to have more than one BSLs having .spec.default=true.

That behavior sounds to me like it's not deprecating this flag, but making it entirely a no-op. To me, that's worse than removing the flag, because if it's present, users will expect it to work.

Deprecation means that it should still work, but is not the preferred method and will be removed.

Ideally, I think the flag should remain until complete removal and cause the server to execute similar logic to what the command does by altering the CRDs. This would happen at server startup, so it's a "one-time" operation, but the users can still use the new command to alter the default.

Ideally, I think the flag should remain until complete removal and cause the server to execute similar logic to what the command does by altering the CRDs. This would happen at server startup, so it's a "one-time" operation, but the users can still use the new command to alter the default.

sure, I'll make server-side and client-side change both work.

I think another use case that might break the user's expectation, for example, we have 2 BSLs, one is default and the other one is secondary.

During the upgrade, the BSL controller sets BSL default as default BSL.

The user changes the defaulted BSL to secondary by Velero CLI.

The Velero pod restart due to deleting the pod, then at velero server startup, it'll set the default BSL back as the default.

It means that every time the velero pod start/restart, it might change the default BSL setting.

It means that every time the velero pod start/restart, it might change the default BSL setting.

Yeah, that's a very good point. That's definitely not something we want, because that will send data to the wrong place based on other configuration changes. Perhaps the server-level option sets the default BSL once and only once, for migration purposes, then it doesn't work anymore?

How about we address these in a follow up PR to keep this one on track? I think the implementation here is pretty good, but the challenges for making it backwards compatible look trickier than I first thought and probably warrant their own set of focused changes.

Signed-off-by: JenTing Hsiao <[email protected]>

add a new flag `--default` under `velero backup-location create` to specify this new location to be the new default BSL. Signed-off-by: JenTing Hsiao <[email protected]>

Add a new flag `--default` under `velero backup-location get` to displays the current default BSL. Signed-off-by: JenTing Hsiao <[email protected]>

When upgrade the BSL CRDs, none of the BSL has been labeled as default. Sets the BSL default field to true if the BSL name matches to the default BSL setting. Signed-off-by: JenTing Hsiao <[email protected]>

When upgrade the BSL CRDs, none of the BSL be marked as the default. Sets the BSL `.spec.default: true` if the BSL name matches against the `velero server --default-backup-storage-location`. Signed-off-by: JenTing Hsiao <[email protected]>

Signed-off-by: JenTing Hsiao <[email protected]>

…re controller Signed-off-by: JenTing Hsiao <[email protected]>

Signed-off-by: JenTing Hsiao <[email protected]>

nrb

I like this functionality a lot, but I would like to see a follow up PR that makes the --default-storage-location argument on velero server set the Default field.

Since that change might be a little tricky, I'm ok with merging this change and then focusing another PR on backwards compatibility.

carlisia

This is such a good feature to have but it's also complicated because of the way we currently try to default to a BSL. All the get/set/create are working. But I found this bug:

Create a BSL named "default"
Create another BSL (say, "mystorage"), and make that the default.
Create a backup, but don't specify the BSL to use. It should use "mystorage".
Describe the backup and you'll see the BSL used was "default", when it should've been "mystorage" since that was set as the default.

carlisia · 2020-12-07T23:00:21Z

I'm still looking at the code for this PR to resolve the issue/bug above. The logic is per my guidance, but I wasn't taking into account the server level config that sets, by default, a BSL named "default" as default, even when there's no BSL with that name. Thinking of some possible ways out of this.

jenting · 2020-12-08T01:20:22Z

This is such a good feature to have but it's also complicated because of the way we currently try to default to a BSL. All the get/set/create are working. But I found this bug:

Create a BSL named "default"

Create another BSL (say, "mystorage"), and make that the default.

Create a backup, but don't specify the BSL to use. It should use "mystorage".

Describe the backup and you'll see the BSL used was "default", when it should've been "mystorage" since that was set as the default.

Thanks for the review, however, I can't reproduce it follows the above steps. Here are the commands:

# make sure no BSL
$ velero backup-location get

# create BSL named "default"
$ velero backup-location create default \
    --provider aws \
    --bucket velero \
    --config region=minio-default,s3ForcePathStyle=true,s3Url=http://minio-default.velero.svc.cluster.local:9000

# create BSL named "mystorage" and make it default
$ velero backup-location create mystorage \
    --provider aws \
    --bucket velero \
    --config region=minio-primary,s3ForcePathStyle=true,s3Url=http://minio-primary.velero.svc.cluster.local:9000 \
    --default

# check "mystorage" is the default BSL
$ velero backup-location get
NAME        PROVIDER   BUCKET/PREFIX   PHASE       LAST VALIDATED                  ACCESS MODE   DEFAULT
default     aws        velero          Available   2020-12-08 09:18:15 +0800 CST   ReadWrite     
mystorage   aws        velero          Available   2020-12-08 09:18:15 +0800 CST   ReadWrite     true

# create a backup and don't specify the backup-storage-location
$ velero backup create tomystorage

# get and describe the "tomystorage" backup
$ velero backup get tomystorage
NAME          STATUS      ERRORS   WARNINGS   CREATED                         EXPIRES   STORAGE LOCATION   SELECTOR
tomystorage   Completed   0        0          2020-12-08 09:15:20 +0800 CST   29d       mystorage          <none>

$ velero backup describe tomystorage
...
Storage Location:  mystorage
...

…cation-for-bsl

carlisia

Sorry about my previous review @jenting, I thought I had updated my image but I hadn't.

This is ready to go in! Thanks a bunch for adding this.

carlisia · 2020-12-08T03:04:48Z

@nrb you have a chance pls give this an approval again, thanks.

…mware-tanzu#3092) * Add default field to BSL CRD Signed-off-by: JenTing Hsiao <[email protected]> * Add a new flag `--default` under `velero backup-location create` add a new flag `--default` under `velero backup-location create` to specify this new location to be the new default BSL. Signed-off-by: JenTing Hsiao <[email protected]> * Add a new default field under `velero backup-location get` add a new default field under `velero backup-location get` to indicate which BSL is the default one. Signed-off-by: JenTing Hsiao <[email protected]> * Add a new sub-command and flag under `velero backup-location` Add a new sub-command called `velero backup-location set` sub-command and a new flag `velero backup-cation set --default` to configure which BSL is the default one. Signed-off-by: JenTing Hsiao <[email protected]> * Add new flag to get the default backup-location Add a new flag `--default` under `velero backup-location get` to displays the current default BSL. Signed-off-by: JenTing Hsiao <[email protected]> * Configures default BSL in BSL controller When upgrade the BSL CRDs, none of the BSL has been labeled as default. Sets the BSL default field to true if the BSL name matches to the default BSL setting. Signed-off-by: JenTing Hsiao <[email protected]> * Configures the default BSL in BSL controller for velero upgrade When upgrade the BSL CRDs, none of the BSL be marked as the default. Sets the BSL `.spec.default: true` if the BSL name matches against the `velero server --default-backup-storage-location`. Signed-off-by: JenTing Hsiao <[email protected]> * Add unit test to test default BSL behavior Signed-off-by: JenTing Hsiao <[email protected]> * Update check which one is the default BSL in backup/backup_sync/restore controller Signed-off-by: JenTing Hsiao <[email protected]> * Add changelog Signed-off-by: JenTing Hsiao <[email protected]> * Update docs locations.md and upgrade-to-1.6.md Signed-off-by: JenTing Hsiao <[email protected]>

jenting changed the title ~~feat: add default for BSL~~ feat: support configures which BSL is the default one Nov 18, 2020

jenting marked this pull request as ready for review November 18, 2020 03:06

github-actions bot requested review from ashish-amarnath, carlisia, dsu-igeek, nrb and zubron November 18, 2020 03:06

github-actions bot assigned jenting Nov 18, 2020

carlisia added this to the v1.6.0 milestone Nov 19, 2020

carlisia suggested changes Nov 19, 2020

View reviewed changes

pkg/cmd/util/output/backup_storage_location_printer.go Show resolved Hide resolved

github-actions bot requested a review from carlisia November 20, 2020 03:06

jenting changed the title ~~feat: support configures which BSL is the default one~~ feat: support configures which BSL is the default one per-provider Nov 20, 2020

jenting changed the title ~~feat: support configures which BSL is the default one per-provider~~ feat: support configures which BSL is the defaulted per-provider Nov 20, 2020

carlisia reviewed Nov 20, 2020

View reviewed changes

carlisia suggested changes Nov 20, 2020

View reviewed changes

nrb suggested changes Nov 20, 2020

View reviewed changes

github-actions bot requested review from carlisia and nrb November 23, 2020 06:09

jenting changed the title ~~feat: support configures which BSL is the defaulted per-provider~~ feat: support configure BSR CR to indicate which is the default Nov 23, 2020

jenting changed the title ~~feat: support configure BSR CR to indicate which is the default~~ feat: support configure BSR CR to indicate which one is the default Nov 23, 2020

nrb changed the title ~~feat: support configure BSR CR to indicate which one is the default~~ feat: support configure BSL CR to indicate which one is the default Nov 30, 2020

nrb suggested changes Nov 30, 2020

View reviewed changes

JenTing Hsiao added 2 commits December 1, 2020 11:21

Add default field to BSL CRD

0af7d2f

Signed-off-by: JenTing Hsiao <[email protected]>

Add a new flag --default under velero backup-location create

4d998a1

add a new flag `--default` under `velero backup-location create` to specify this new location to be the new default BSL. Signed-off-by: JenTing Hsiao <[email protected]>

JenTing Hsiao added 7 commits December 1, 2020 14:51

Add new flag to get the default backup-location

8f81959

Add a new flag `--default` under `velero backup-location get` to displays the current default BSL. Signed-off-by: JenTing Hsiao <[email protected]>

Configures default BSL in BSL controller

978cc0f

When upgrade the BSL CRDs, none of the BSL has been labeled as default. Sets the BSL default field to true if the BSL name matches to the default BSL setting. Signed-off-by: JenTing Hsiao <[email protected]>

Add unit test to test default BSL behavior

2f94639

Signed-off-by: JenTing Hsiao <[email protected]>

Update check which one is the default BSL in backup/backup_sync/resto…

8a98229

…re controller Signed-off-by: JenTing Hsiao <[email protected]>

Add changelog

9e2ee8a

Signed-off-by: JenTing Hsiao <[email protected]>

Update docs locations.md and upgrade-to-1.6.md

f1659ef

Signed-off-by: JenTing Hsiao <[email protected]>

nrb previously approved these changes Dec 2, 2020

View reviewed changes

carlisia suggested changes Dec 7, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feat-add-default-lo…

5e2187a

…cation-for-bsl

jenting dismissed nrb’s stale review via 5e2187a December 8, 2020 01:23

github-actions bot requested review from carlisia and nrb December 8, 2020 01:23

carlisia approved these changes Dec 8, 2020

View reviewed changes

nrb approved these changes Dec 8, 2020

View reviewed changes

nrb merged commit 9dd158d into vmware-tanzu:main Dec 8, 2020

jenting deleted the feat-add-default-location-for-bsl branch December 8, 2020 23:13

This was referenced Dec 9, 2020

Reconciling the server default BSL with the user configured default BSL #3173

Open

Improve the velero backup-location command #2425

Closed

georgettica mentioned this pull request Dec 23, 2020

georgettica/bump external snapshotter version #3201

Closed

carlisia mentioned this pull request Jan 11, 2021

When using a backupstoragelocation not name default the logs get tons of spam about the default missing. #3228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support configure BSL CR to indicate which one is the default #3092

feat: support configure BSL CR to indicate which one is the default #3092

jenting commented Nov 18, 2020 •

edited by carlisia

Loading

carlisia left a comment •

edited

Loading

jenting commented Nov 20, 2020

carlisia left a comment

carlisia left a comment •

edited

Loading

carlisia commented Nov 20, 2020

nrb left a comment

carlisia commented Nov 20, 2020

jenting commented Nov 23, 2020

jenting commented Nov 23, 2020

jenting commented Nov 23, 2020

nrb left a comment

nrb Nov 30, 2020

jenting Dec 1, 2020

nrb Dec 1, 2020

jenting Dec 2, 2020

jenting Dec 2, 2020 •

edited

Loading

nrb Dec 2, 2020

nrb left a comment

carlisia left a comment

carlisia commented Dec 7, 2020

jenting commented Dec 8, 2020

carlisia left a comment

carlisia commented Dec 8, 2020

feat: support configure BSL CR to indicate which one is the default #3092

feat: support configure BSL CR to indicate which one is the default #3092

Conversation

jenting commented Nov 18, 2020 • edited by carlisia Loading

carlisia left a comment • edited Loading

Choose a reason for hiding this comment

jenting commented Nov 20, 2020

carlisia left a comment

Choose a reason for hiding this comment

carlisia left a comment • edited Loading

Choose a reason for hiding this comment

carlisia commented Nov 20, 2020

nrb left a comment

Choose a reason for hiding this comment

carlisia commented Nov 20, 2020

jenting commented Nov 23, 2020

jenting commented Nov 23, 2020

jenting commented Nov 23, 2020

nrb left a comment

Choose a reason for hiding this comment

nrb Nov 30, 2020

Choose a reason for hiding this comment

jenting Dec 1, 2020

Choose a reason for hiding this comment

nrb Dec 1, 2020

Choose a reason for hiding this comment

jenting Dec 2, 2020

Choose a reason for hiding this comment

jenting Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

nrb Dec 2, 2020

Choose a reason for hiding this comment

nrb left a comment

Choose a reason for hiding this comment

carlisia left a comment

Choose a reason for hiding this comment

carlisia commented Dec 7, 2020

jenting commented Dec 8, 2020

carlisia left a comment

Choose a reason for hiding this comment

carlisia commented Dec 8, 2020

jenting commented Nov 18, 2020 •

edited by carlisia

Loading

carlisia left a comment •

edited

Loading

carlisia left a comment •

edited

Loading

jenting Dec 2, 2020 •

edited

Loading