fix(catalog): fix issue where subscriptions sometimes get "stuck" #847
Conversation
we were not resetting the client when updating a catalogsource, which meant it was possible for the client to be stale and never attempt a reconnect if it didn't go unhealthy "in time" for us to detect and reconnect.
| @@ -446,6 +446,12 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) { | |||
| o.sourcesLastUpdate = timeNow() | |||
| logger.Debug("registry server recreated") | |||
|
|
|||
| func() { | |||
There was a problem hiding this comment.
this is the fix, everything else is small things I noticed when reviewing
There was a problem hiding this comment.
Any reason you use a closure instead of just lock/unlock without defer?
o.sourcesLock.Lock()
delete(o.sources, sourceKey)
o.sourcesLock.Unlock()
There was a problem hiding this comment.
it's purely to keep the lock/unlock always next to each other, so that in the future if we need to do additional work with sources here it's harder to make a mistake
There was a problem hiding this comment.
Cool, just curious if I could learn something here, thanks.
openshift-ci-robot
added
the
size/M
openshift-ci-robot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, tkashem 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 |
1 similar comment
|
/retest |
|
/retest |
we were not resetting the client when updating a catalogsource, which
meant it was possible for the client to be stale and never attempt
a reconnect if it didn't go unhealthy "in time" for us to detect and
reconnect.
I ran
InstallPlanWithCSVsAcrossMultipleCatalogSources20 times in a row to verify it no longer flakes (previously it would error within ~5 tries)