-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support configure BSL CR to indicate which one is the default #3092
Merged
nrb
merged 12 commits into
vmware-tanzu:main
from
jenting:feat-add-default-location-for-bsl
Dec 8, 2020
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
0af7d2f
Add default field to BSL CRD
4d998a1
Add a new flag `--default` under `velero backup-location create`
8ecc9d2
Add a new default field under `velero backup-location get`
fa8be78
Add a new sub-command and flag under `velero backup-location`
8f81959
Add new flag to get the default backup-location
978cc0f
Configures default BSL in BSL controller
23f961a
Configures the default BSL in BSL controller for velero upgrade
2f94639
Add unit test to test default BSL behavior
8a98229
Update check which one is the default BSL in backup/backup_sync/resto…
9e2ee8a
Add changelog
f1659ef
Update docs locations.md and upgrade-to-1.6.md
5e2187a
Merge remote-tracking branch 'upstream/main' into feat-add-default-lo…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
feat: support configures BackupStorageLocation custom resources to indicate which one is the default |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
/* | ||
Copyright 2020 the Velero contributors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package backuplocation | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
|
||
"github.com/pkg/errors" | ||
"github.com/spf13/cobra" | ||
"github.com/spf13/pflag" | ||
|
||
kbclient "sigs.k8s.io/controller-runtime/pkg/client" | ||
|
||
velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" | ||
"github.com/vmware-tanzu/velero/pkg/client" | ||
"github.com/vmware-tanzu/velero/pkg/cmd" | ||
) | ||
|
||
func NewSetCommand(f client.Factory, use string) *cobra.Command { | ||
o := NewSetOptions() | ||
|
||
c := &cobra.Command{ | ||
Use: use + " NAME", | ||
Short: "Set a backup storage location", | ||
Args: cobra.ExactArgs(1), | ||
Run: func(c *cobra.Command, args []string) { | ||
cmd.CheckError(o.Complete(args, f)) | ||
cmd.CheckError(o.Run(c, f)) | ||
}, | ||
} | ||
|
||
o.BindFlags(c.Flags()) | ||
|
||
return c | ||
} | ||
|
||
type SetOptions struct { | ||
Name string | ||
DefaultBackupStorageLocation bool | ||
} | ||
|
||
func NewSetOptions() *SetOptions { | ||
return &SetOptions{} | ||
} | ||
|
||
func (o *SetOptions) BindFlags(flags *pflag.FlagSet) { | ||
flags.BoolVar(&o.DefaultBackupStorageLocation, "default", o.DefaultBackupStorageLocation, "Sets this new location to be the new default backup storage location. Optional.") | ||
} | ||
|
||
func (o *SetOptions) Complete(args []string, f client.Factory) error { | ||
o.Name = args[0] | ||
return nil | ||
} | ||
|
||
func (o *SetOptions) Run(c *cobra.Command, f client.Factory) error { | ||
kbClient, err := f.KubebuilderClient() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
location := &velerov1api.BackupStorageLocation{} | ||
err = kbClient.Get(context.Background(), kbclient.ObjectKey{ | ||
Namespace: f.Namespace(), | ||
Name: o.Name, | ||
}, location) | ||
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) | ||
} | ||
for _, 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(), &location, &kbclient.UpdateOptions{}); err != nil { | ||
return errors.WithStack(err) | ||
} | ||
break | ||
} | ||
} | ||
|
||
location.Spec.Default = o.DefaultBackupStorageLocation | ||
if err := kbClient.Update(context.Background(), location, &kbclient.UpdateOptions{}); err != nil { | ||
return errors.WithStack(err) | ||
} | ||
|
||
fmt.Printf("Backup storage location %q configured successfully.\n", o.Name) | ||
return nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if both of these are set? We should document that behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me summarize the scenario I've thought:
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 orkubectl 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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I'll make server-side and client-side change both work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 issecondary
.default
as default BSL.secondary
by Velero CLI.default
BSL back as the default.It means that every time the velero pod start/restart, it might change the default BSL setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.