-
Notifications
You must be signed in to change notification settings - Fork 52
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
envtest: retry on failure in EnsurePVCfromRD function #1424
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
|
||
return errors.As(err, &statusError) && | ||
statusError.ErrStatus.Details != nil && | ||
statusError.ErrStatus.Details.Kind != "PersistentVolumeClaim" |
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.
Why do we care about the details of the not found error?
Trying to extract the underlying error means we should have returned it as is from the caller. It does not make sense to create unhelpful error and then work hard on extracting the helpful details from the error. This should be fixed in vshandler.
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.
Here is comment from @ShyamsundarR in the issue itself: #1371 (comment)
statusError.ErrStatus.Details.Kind != "PersistentVolumeClaim" | ||
} | ||
|
||
return !kerrors.IsConflict(err) |
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.
So this code handles both missing PVC and conflict creating it?
Can simplify this using the standard retryOnConflict to handle conflicts calling vsHandler.EnsurePVCfromRD
? This is what I expected based on the commit message:
"Retry on conflict in EnsurePVCfromRD function"
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, this code filters both both missing PVC and conflict creating it, these two cases must be retried, other errors are fine, we need them in our tests
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.
Do we have other copies of this code for running vsHandler.EnsurePVCfromRD with retries?
If we do, we need one copy reused by all the tests using this function.
For fixing conflicts like this:
retryOnConflict should be enough, no? |
@@ -1335,7 +1336,7 @@ var _ = Describe("VolSync_Handler", func() { | |||
It("ensure PVC should not fail", func() { | |||
// Previous ensurePVC will already have created the PVC (see parent context) | |||
// Now run ensurePVC again - additional runs should just ensure the PVC is ok |
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.
If we called vsHandler.EnsurePVCfromRD(rdSpec, false)
and it succeeed, it should not never fail with "pvc not found" here, and likely it will not fail with a conflict. So maybe the retry is needed only in the first call?
@@ -1372,7 +1373,7 @@ var _ = Describe("VolSync_Handler", func() { | |||
// At this point we should have a PVC from previous but it should have a datasource | |||
// that maches our old snapshot - the rd has been updated with a new latest image | |||
// Expect ensurePVC from RD to remove the old one and return an error |
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.
Here we expect an error, why do we need to retry? can this fail with pvc not found or conflict?
@@ -1398,7 +1399,7 @@ var _ = Describe("VolSync_Handler", func() { | |||
// | |||
// Now should be able to re-try ensurePVC and get a new one with proper datasource | |||
// |
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.
Can this fail with conflict or pvc not found?
It seems that this change protect too many calls with a retry when it may not be needed. This can cause unwanted delays or confuse the reader that the retry is needed when it is not. Lets try to add the retry only for the calls that need it, and only handle the expected failure (pvc not found, conflict, or both).
statusError.ErrStatus.Details.Kind != "PersistentVolumeClaim" | ||
} | ||
|
||
return !kerrors.IsConflict(err) |
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.
Do we have other copies of this code for running vsHandler.EnsurePVCfromRD with retries?
If we do, we need one copy reused by all the tests using this function.
Cache issues during test - retry, based on error returning from function, is added.
Fixes: #1371