Improve thread-safety of sync status updates #301
Conversation
- Wrap applier errors in their own mutex - Move applier.Syncing() to Parser.Syncing() with updater.Updating() doing the heavy lifting. The Syncing condition status should reflect the whole updater, not just the applier. - Rename applier.Interface -> KptApplier to make room for adding KptDestroyer in the future. - Rename KptApplier.sync -> applyInner to make room for adding destroyInner in the future. - Refactor state.syncStatus and Parser.SetSyncStatus to use a new syncStatus struct, which is like the sourceStatus struct but with an added sync bool. This should also be less confusing, because state.syncStatus no longer uses a sourceStatus struct as its value. - Add a new setSyncStatusErrors func to use in prependRootSyncRemediatorStatus, to avoid needing to to re-construct the initial syncStatus when prepending errors. Change-Id: Ib4316b8094c371d9371e7bb48bd3b309e668a78b
| @@ -77,38 +77,37 @@ type Applier struct { | |||
| // errs tracks all the errors the applier encounters. | |||
| // This field is cleared at the start of the `Applier.Apply` method | |||
| errs status.MultiError | |||
| // syncing indicates whether the applier is syncing. | |||
| syncing bool | |||
There was a problem hiding this comment.
oh man was this another bool used for synchronization?
There was a problem hiding this comment.
yup. and set outside of the lock.
| // Ideally we want to avoid invalidating errors that will continue to happen, | ||
| // but for now, invalidate all errors until they recur. | ||
| // TODO: improve error cache invalidation to make rsync status more stable | ||
| a.invalidateErrors() |
There was a problem hiding this comment.
What's the plan for how we want to handle this in the long run?
There was a problem hiding this comment.
I'm writing a doc. But the TLDR is to do more careful invalidation by caching what event type and object added/owns the error, and not invalidating that error until we see that event again, or the apply ends.
| u.watchErrs = errs | ||
| } | ||
|
|
||
| // Updating returns true if the Update method is running. | ||
| func (u *updater) Updating() bool { |
There was a problem hiding this comment.
This smells a bit like the other bool. Is the idea that by setting this at a higher level (rather than reaching into the applier to get the value) it is less prone to race conditions?
There was a problem hiding this comment.
Moving it up a level allows including the other actions performed by updater.Update().
Setting the bool inside the newly added mutex ensures that the bool value is not affected by race conditions.
But yes, it's a lazy fix, not an ideal fix. Ideally the control flow would be modified to use channels, but I decided to start with the quick fix, because I've had trouble getting large rewrites tested and approved. And I'd like to get back to finishing the finalizer.
There was a problem hiding this comment.
The errors and syncing bool could be bundled together in a Status() result or something, to be atomic, and the current status could be the result of a for loop that reads from a channel accepting errors and start/stop signals.
But once you go down that road, you realize that the applier, updater, parser, and reconciler layers ALL have the same problematic pattern and ALL need to be rewritten to use a top level reconcile loop. Then you wouldn't need to synchronize any of these lower layers.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nan-yu 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 |
|
/hold |
|
/unhold |
|
/retest Looks like TestInvalidRepoSyncBranchStatus might be flakey with a missing |
.status.sync.errsand.status.sync.lastUpdatewithout updating the Syncing condition orreconciler_errorsmetric, which means they will continue to be out of sync when there's a conflict. But this is not a new problem.)