-
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
Add option for Azure cross subscription backups #1895
Add option for Azure cross subscription backups #1895
Conversation
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
…king up to a different Azure Subscription than the cluster's resources. Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Thanks for this @boxcee! I'll review and do some testing this week. |
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.
@boxcee thanks again for this PR! Have you been able to test this? I don't currently have an Azure cluster set up so trying to decide if I need to for testing or not.
Let's also document the subscriptionId
config key at site/docs/master/api-types/backupstoragelocation.md
, under the Azure section.
@skriss For testing, is there anything I should document? We have an Azure account and I will test this today. |
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
…velero into allow-cross-subscription-backups
refactor regarding to comment, add more documentation
…velero into allow-cross-subscription-backups
@boxcee just some quick notes on exactly what scenario/steps you tested would be great - don't need anything long, just enough to get the idea. thanks! |
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.
code LGTM - just waiting for confirmation on testing
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Soooo, I did test this and it does work. But, please don't review just yet. During testing I realized that I also need to store snapshots in a different subscription. I was testing this today but couldn't finish it. Will continue on Monday and update PR. |
hey @boxcee, how do you test this succesfully? I get the following error |
@jweldonca I assume this error shows up in the controller? By default the Your problem is basically that you have built a new velero binary, but the images being deployed are still the For reference: https://github.com/heptio/velero/blob/2d845683a2ddf06b956433ff57e04b86890f4776/pkg/install/deployment.go#L96 |
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
@skriss Can confirm it does work cross-subscription now!
|
👍 I didn't realize that cross-subscription volume snapshots were even possible - sounds great! I'll take another look at the code shortly. |
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.
LGTM. @carlisia - is it going to be an issue for you if we merge this Azure-specific PR into master
now? If yes and you want to hold it until the separate plugin repo is created, then we'll want to help migrate the PR over there.
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.
It'd be best to merge now wrt the plugins!
Great contribution, thank you!
Closes #1646