-
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 PartiallyFailed phase for backups, don't exit on first error #1386
Conversation
output of
output of
|
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.
Overall this is looking good!
bc313a1
to
86c69a7
Compare
Made a few more updates - this is ready for review. Still testing. |
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.
Super minor suggestions. Besides those, I didn't see in this code any logging of warning. I suppose you are displaying warnings that we were already adding.
addressed comments in latest commits |
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, just needs rebasing.
a578fdc
to
ab31ae3
Compare
Signed-off-by: Steve Kriss <krisss@vmware.com>
ab31ae3
to
893d73b
Compare
rebased/squashed/changelogged |
…-tanzu#1386) Signed-off-by: Steve Kriss <krisss@vmware.com>
rel #1371
This PR adds the PartiallyFailed phase for backups. It modifies the backup execution logic so that rather than immediately exiting when the first error is encountered, the error is logged and execution continues. At the end, the number of errors logged is counted, and a >0 result moves the backup to PartiallyFailed.
Still a WIP, but the basic functionality is working. An easy way to see what it looks like is to create a cluster role binding for a cluster role that doesn't exist, to a service account in the namespace you're backing up -- this will create a "partial failure" error.
@nrb @carlisia appreciate any input at this point!