-
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 status patching retry configuration design. #8063
Add status patching retry configuration design. #8063
Conversation
/kind changelog-not-required |
e42441e
to
15dc7af
Compare
15dc7af
to
653a2c8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8063 +/- ##
==========================================
+ Coverage 58.88% 59.07% +0.18%
==========================================
Files 351 364 +13
Lines 29367 30307 +940
==========================================
+ Hits 17294 17904 +610
- Misses 10639 10960 +321
- Partials 1434 1443 +9 ☔ View full report in Codecov by Sentry. |
92fa2a6
to
4dfbacd
Compare
Maybe this should also cover #7799? |
113939e
to
ca91cff
Compare
@kaovilai I think 7799 should be handled separately. That addresses non-APIServer errors, where retry isn't required, and where status updates are possible -- i.e. that issue is where we're not trying to update status but we could. Here we have the opposite problem -- we're trying to update status but can't. |
We will add retries with timeout to existing patch calls that moves a backup/restore from InProgress to a final status such as | ||
- Completed | ||
- PartiallyFailed | ||
- Failed |
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'm not sure there are cases here where the transition is to Failed or FailedValidation -- FailedValidation -- in those cases, I think we're mostly moving from New, so a future reconcile can just handle it normally. Although maybe retry is still appropriate 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.
Future reconcile isn't generally handled. So we should still retry or it can be stuck in a phase other than new. New or "" is the only phase being retried via future reconcile.
switch restore.Status.Phase {
case "", api.RestorePhaseNew:
// only process new restores
default:
r.logger.WithFields(logrus.Fields{
"restore": kubeutil.NamespaceAndName(restore),
"phase": restore.Status.Phase,
}).Debug("Restore is not handled")
return ctrl.Result{}, nil
}
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.
@kaovilai well the various WaitingForPluginOperations will also be retried on future reconcile, since the backup/restore operations controller re-reconcile to check progress on a periodic basis rather than on a cr-updated basis.
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.
But those aren't handled by the backup/restore controller, which only handles new CRs.
71b9ac3
to
0e967e4
Compare
|
||
and from above non final phases to | ||
- Completed | ||
- PartiallyFailed |
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 guess we could retry for WaitingForPluginOperations->final -- but since the backup/restore operations controllers re-reconcile all WaitingForPluginOperations CRs every 10 seconds by default anyway, it's probably unnecessary. Finalizing CRs definitely need retry though.
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.
Easily removable if deemed unnecessary and already covered by opreation controllers
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.
feedbacks from Shubham
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
0f3c911
to
eebc4af
Compare
fixed DCO |
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]> update to design vmware-tanzu#8063 Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai [email protected]
Thank you for contributing to Velero!
Please add a summary of your change
Does your change fix a particular issue?
Design for #7207
Please indicate you've done the following:
/kind changelog-not-required
as a comment on this pull request.site/content/docs/main
.