-
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
Fail backup if it already exists in object storage #1390
Conversation
Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>
Signed-off-by: Carlisia <carlisiac@vmware.com>
Signed-off-by: Carlisia <carlisiac@vmware.com>
Signed-off-by: Carlisia <carlisiac@vmware.com>
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.
initial review. I can sign up for Azure testing since I have a cluster up and running
Signed-off-by: Carlisia <carlisiac@vmware.com>
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.
Two small comments, otherwise LGTM. Please add a changelog file as well!
Signed-off-by: Carlisia <carlisiac@vmware.com>
code LGTM, just waiting on testing |
After locally fixing the first comment from above, I was able to confirm that on Azure, backups correctly fail if there's already a |
Sweet!!! @ncdc did great work! |
An observation: in the case that this code is intended to catch, we end up with a I'm guessing that to fix this, we'll need to add velero-managed UIDs to backups, so that we have a unique ID for each backup across time and space that we can use to determine if two items are the same or not. I don't think it's in scope for this PR, but we should follow up on it. It's somewhat related to #629 (comment). |
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
The UID fix seems reasonable to me. I briefly wondered if it would then be better to use those identifiers on the object storage vs the user-provided name, as that would remove the need for this code. However, that's too invasive a change at this stage, so I'm for just attaching it and using it on sync comparison. |
In lieu of #629
Fixes #623
Notes:
The test below "as is" was left out of this version. The intention of testing for failures in the case of a backup already existing was incorporated in other existing (current) tests: https://github.com/heptio/velero/pull/629/files#diff-d0a3ff018929aec3abbaa4c8a2c299ccR148
This commit was in the original PR, but I don't think we want it? d1cd91d
WIP: working on testing these against the main SCPs.