Reconciling the server default BSL with the user configured default BSL

[carlisia]

I wanted to surface this and get a sanity check if this has been addressed best we can, or if there's still a use case we need to cover. This is in regards to PR #3092. More specifically, this is in regards to how we pick the default BSL to use when the Velero server starts up with its configured default BSL when at the same time there is a BSL marked as default (in the CRD).

For context/recall:

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?
Source: #3092 (comment)

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.
Source: #3092 (review)

Currently, this is how we pick the default BSL to use:

1. Did the user specify the default at backup creation time? If so, use that.

2. Is there a BSL marked as default (in the CRD)? Either the user did that manually or the BSL is from a new installation that automatically configured that first BSL as default. If so, use that (refs:

   velero/pkg/controller/backup_storage_location_controller.go

   Line 72 in 844cc16

   defaultFound = true

   and pending PR to automatically make BSL the default on install: https://github.com/vmware-tanzu/velero/pull/3172/files#diff-ff4ac83c63c469932ea321327ca5368dbf304c8ea2affe85539d9225aea93247R163)

3. If none of the above cases are true, use the server configuration to set a BSL as the default, IF AND ONLY IF there exists a BSL with the same name. If no BSL with that name exists, nothing can be done, except warn. Refs:

   velero/pkg/controller/backup_storage_location_controller.go

   Line 84 in 844cc16

   if !defaultFound && location.Name == r.DefaultBackupLocationInfo.StorageLocation {

   and pending PR (with better warning): https://github.com/vmware-tanzu/velero/pull/3172/files#diff-392430c1e526e2d5a98e4a69deb90350eabcb7a77e58e2f967b4c806568dda2aR173.

4. If none of the above cases are possible, either there is no existing BSL, or none of the existing BSL is marked as "default". Likely these are legacy BSLs. In this case, the BSL controller will continue to throw the warning (see right above), and the failed backup will contain the error message explaining why and how to fix it. Ref: see pending PR for better error message: https://github.com/vmware-tanzu/velero/pull/3172/files#diff-f0334755c17add3306aecd57a34bc24dfa20a66f617dbaf541042dfd8e0887deR380.

❓ This is the question to answer:

Do we want the user configured default BSL to take precedence over the server configured default?

If so, this is handled. This is my preference, especially since the server config will be deprecated.

If not, we need to invert the existing logic to give preference to the server configuration.

Let me know if I missed anything.

c/c @nrb and @jenting

[jenting]

If we want to cover both user-configured and server-configured at the same time, the only way I could though now is to have a new field in BSL CRD like .spec.serverDefaultBSLName to indicates the server-side default BSL name.

Once the Velero pod restart, it compare the .spec.serverDefaultBSLName equals to current server-side BSL name.
If mismatch, use the server-side BSL name as the default BSL and updates the .spec.serverDefaultBSLName.

[carlisia]

But this would not translate into covering both at the same time. This would mean giving precedence to the server config over the user config. In this proposed way, it would also break the principle of "do the least surprising thing", since the user would probably be surprised something they intentionally set was changed to something else they might not even be aware of (the server config, which will always have a default value of "default").

[jenting]

But this would not translate into covering both at the same time. This would mean giving precedence to the server config over the user config. In this proposed way, it would also break the principle of "do the least surprising thing", since the user would probably be surprised something they intentionally set was changed to something else they might not even be aware of (the server config, which will always have a default value of "default").

Let me recap the scenario I've thought, for example, we have 2 BSLs, one is default and the other one is secondary.

1a. During the fresh install, the velero CLI configures a BSL named "default" as the default BSL. (.spec.serverDefaultBSLName: default)
1a. During the upgrade, the BSL controller sets the defaulted BSL according to the BSL name "default". (.spec.serverDefaultBSLName: default)
2. The user changes the defaulted BSL to secondary by Velero CLI, so the configuration .spec.serverDefaultBSLName: default keeps the same.
3. The Velero pod restarts due to pod crash or delete, the Velero server start and the BSL controller checks the .spec.serverDefaultBSLName. Since it's unchanged, so the BSL controller won't update the defaulted BSL from the server-side configuration.
4. The user changes velero-server side --default-backup-storage-location=secondary. The .spec.serverDefaultBSLName: default mismatch the velero-server side configuration, change to use server-side configuration and update the .spec.serverDefaultBSLName: secondary.

[dsu-igeek]

Can I add one more case here? I think the majority of installations only have 1 BSL configured, so if that's the case can we use it irregardless of whether it's marked as default or not?

[carlisia]

https://github.com/vmware-tanzu/velero/pull/3172/files#diff-ff4ac83c63c469932ea321327ca5368dbf304c8ea2affe85539d9225aea93247R163

The PR above marks the BSL to be created as the default, since at this time there's only 1.

Another point where to handle this is when looping over the controller (BSL or Sync, doesn't matter), if there's only 1 BSL then mark it as default. Either this way or by just using the BSL if there's no other (it'd be odd for the backup to be doing this check but let's roll with it), the question of what if the server configuration specifies a different default?

[carlisia]

I just realized we have a user (cli) and a global (server)l level config for the ttl. Let me look at how the precedence is handled with that one.