-
Notifications
You must be signed in to change notification settings - Fork 102
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 json decoding errors from the Kubernetes API to prevent flaky tests. #423
Retry json decoding errors from the Kubernetes API to prevent flaky tests. #423
Conversation
aee135a
to
eece55b
Compare
for ok := true; ok; ok = err != nil { | ||
done := make(chan bool) | ||
|
||
// run the function in a goroutine and close it once it is finished so that |
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.
This feels atrocious. Is there a better way to wait for either function return or context expiration?
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 think the only change I'd make here is having an errChan so that you're not trying to set the error across goroutine bondaries - and then add your errChan to your select. Otherwise, yeah, it is what it is.
Additionally, would https://github.com/kubernetes/kubernetes/blob/master/pkg/util/goroutinemap/exponentialbackoff/exponential_backoff.go be useful?
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.
Agree with the channel fix. I think there will be a race condition otherwise
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.
Okay, I think it should be fixed now. Better?
Had to re-run the build because it turns out Circle CI gets confused if you have it enabled on your fork and you open a PR 🤦♂️ |
for ok := true; ok; ok = err != nil { | ||
done := make(chan bool) | ||
|
||
// run the function in a goroutine and close it once it is finished so that |
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 think the only change I'd make here is having an errChan so that you're not trying to set the error across goroutine bondaries - and then add your errChan to your select. Otherwise, yeah, it is what it is.
Additionally, would https://github.com/kubernetes/kubernetes/blob/master/pkg/util/goroutinemap/exponentialbackoff/exponential_backoff.go be useful?
What type of PR is this?
/kind bug
What this PR does / why we need it:
This retries json decoding errors returned by the
CreateOrUpdate
method to prevent flaky tests.Which issue(s) this PR fixes:
Fixes #415
Does this PR introduce a user-facing change?: