Add upload timeout #743
Add upload timeout #743
Conversation
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
| notificationFunc := backoff.Notify(func(err error, t time.Duration) { | ||
| eventf("Warning", "PushingErr", "retrying in %v after error: %s", t, err) | ||
| log.Info("Warning: PushingErr: retrying", "in", t, "reason", err) | ||
| log.Error(err, "Warning: PushingErr: will retry", "retry_after", t) |
There was a problem hiding this comment.
This will make the temporary problem more prominent (Error instead of Info) in the logs of the affected user.
There's a risk that other users might find it annoying to suddenly get new errors logged in normal operation.
This backoff feature is intended to smooth over temporary network errors or API errors, so that the user doesn't have to worry about them.
There was a problem hiding this comment.
I agree with Richard. I believe a warning level is more appropriate.
| post := func() (any, error) { | ||
| return struct{}{}, postData(klog.NewContext(ctx, log), config, preflightClient, readings) | ||
| postCtx, cancel := context.WithTimeout(ctx, config.BackoffMaxTime) | ||
| defer cancel() |
There was a problem hiding this comment.
👍 Good idea, using the existing BackoffMaxTime as the operation timeout, to ensure that no single upload attempt can hang for ever and block the retry.
It seems to me that the backoff module should do this, but it doesn't seem to.
- https://github.com/search?q=repo%3Acenkalti%2Fbackoff+WithTimeout&type=code
- https://github.com/search?q=repo%3Acenkalti%2Fbackoff+WithCancel&type=code
...and the author seems to be adamant that the supplied function should do it's own context cancellation:
|
You'll need to push another commit to trigger the E2E tests. It only runs them in response to a commit pushed after the test-e2e label has been added. |
This should help with debugging https://venafi.atlassian.net/browse/VC-46449.