Bug 1958759: #417 insights report - add basic retry logic in case of 404 #418
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tremes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
| @@ -152,11 +165,14 @@ func (r *Gatherer) RetrieveReport() { | |||
| } | |||
|
|
|||
| firstPullDone = true | |||
| if !delayTimer.Stop() { | |||
| <-delayTimer.C | |||
There was a problem hiding this comment.
It's still not clear to me whether we need to call this @joselsegura. This part of code was almost never (with waiting for clusterversion exception) called before AFAIK.
My thinking is following. We already drained the channel in this moment and thus the delayTimer.Stop() returns false (see https://golang.org/pkg/time/#Timer.Stop), which leads to blocking call of <-delayTimer.C. Maybe it should be:
if delayTimer.Stop() {
<-delayTimer.C
}
or maybe I am completely wrong....
There was a problem hiding this comment.
TBH I wrote it a long time ago and I'm not 100% sure, but according to the example in the link to Golang time library, the Stop call will return "false" «if the timer has already expired or been stopped. Stop does not close the channel, to prevent a read from the channel succeeding incorrectly.»
So I think this logic is correct: if Stop returns "true", you are done with it. If it returns "false", you need to read from the channel to ensure anybody else is consuming it (if I am understanding the library reference correctly!).
It is probably a very rare corner case, so probably your solution will work much better
There was a problem hiding this comment.
The documentation for Reset describes it even better IMO https://golang.org/pkg/time/#Timer.Reset and I think we don't need to call/check Stop, because it will always return false in this situation, because the timer has already expired (as per doc).
tremes
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
It looks pretty straightforward, so just reading the code it looks good. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rluders, tremes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
tremes
changed the title
openshift-ci
bot
added
bugzilla/severity-medium
|
@tremes: This pull request references Bugzilla bug 1958759, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@tremes: All pull requests linked via external trackers have merged: Bugzilla bug 1958759 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This is adding retry logic for the insights report pulling - addressing #417 and also updating Policy API version to v1.
Categories
Sample archive
No new data
Documentation
No update
Unit Tests
No new test
Privacy
Yes. There are no sensitive data in the newly collected information.
Changelog
References
https://issues.redhat.com/browse/???
https://bugzilla.redhat.com/show_bug.cgi?id=???
https://access.redhat.com/solutions/???