Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Retry on transient error when waiting for snapshot to be ready #2508
Retry on transient error when waiting for snapshot to be ready #2508
Changes from 2 commits
5b8f944
fbd6e78
026debb
ea245c8
25b1ab0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Question: Is this the right poll to use here? Why is 100 retries the correct value?
It appears to me that there is currently only 1 other file where Kanister code uses poll.WaitWithRetries.
It is more common to use
poll.Wait
, sometimes with the ctx set to have a timeout. Or to callpoll.WaitWithBackoff
and a maximum time set.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.
We could do that. The difference here is that we only retry on this specific type of error, while
poll.WaitWithBackoff
would not distinguish that.The
100
value is pretty arbitrary just to avoid the infinite wait. We could usepoll.WaitWithBackoffWithRetries
to configure max time to wait as well, but I don't know what's the good value in this context.Current setting for the context timeout is infinity because there's a long-running process. Inserting a new time limit here would mean potentially creating a new failure scenario. Setting retries would worst case result in similar behaviour to what we have right now.
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.
As @pavannd1 indicated we can check this in as is and iterate on the polling mechanism-- this will improve robustness as is.
We can retry only on a specific error when using poll.Wait or poll.WaitWithBackoff by making the check for that error within the function called by poll. See for example how an
apierrors.IsNotFound
error is handled in RepoServiceHandler.createService.See
kanister/pkg/controllers/repositoryserver/handler.go
Line 138 in 2756ffb
Yes, there are some subtle differences between terminating a poll with a timeout or by retry count-- in particular whether ctx expiration can affect a call made within the poll loop. But I don't think those differences matter when the call being made is just a Get.
Ultimately the retry count is going to be picked based on a some knowledge of how long it is reasonable to wait for the snapshot controller to resolve the transient. In my mind it is clearer to just express that as a time value instead of trying to calculate the retry count from desired time and the backoff parameters.
See the many instances of
poll.Wait(timeoutCtx,...
in pkg/app where an app-specific timeout is known and used. Or pkg/kube.WaitForPodReady.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.
Upon further reflection, I've realized that the code as implemented has the following behavior:
Importantly, setting a timeout on the context at beginning of all polling will affect the first wait, the wait for controller to ready snapshot before setting any error, transient or not.
While I still think it would be better to have an explicit time limit on retrying of transients, and we probably could work out a way to build it into a retry function closure, at that point it is no longer simple and clear.
Best solution is probably to keep retry count on trasients and add a comment about expected time that count corresponds to using default backoff.
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.
Just wanted to confirm that the events are in
VolumeSnapshot
resource right, and not in theVolumeSnapshotContent
resource?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.
Yes. I don't think we read
VolumeSnapshotContent
status anywhere in our code. But status errors are propagated by snapshot_controller toVolumeSnapshot
if there is an error inVolumeSnapshotContent
as I understand.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.
[optional] formatting for readability
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.
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.
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.
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.