Skip to content

Commit

Permalink
fix: Put app to the operation queue after refresh queue processing to…
Browse files Browse the repository at this point in the history
… avoid race condition - Fixes [#18500]

Adding app to the operation queue and refresh queue could cause waiting for resource for minutes to tens of minutes.
Sync state operates on resources gathered from reconciliation, so if the app operation event is processed before the refresh one (when triggered on resource update/creation), the refresh doesn’t help sync to progress and it essentially needs to wait for another app refresh.

The fix seems to be to schedule app operation event after refresh event is finished processing. There’s one place where operation event is scheduled without refresh event (which can be kept there), and one place where refresh even is scheduled without the operation one during the app deletion handling https://github.com/argoproj/argo-cd/blob/3e2cfb138795e24df98c9745d813e4b7f1720dbd/controller/appcontroller.go#L2177. It’s probably safe to schedule operation even after that, since it has some code to check that app was deleted. If not, an update can be made to have refresh queue storing a tuple with app key and bool whether to enqueue app operation.

If there are issues: try keeping both old places to add to app operation queue and new addition after refresh.

Note on cherry pick: add to as many releases as you can. This can be a significant performance boost.

Signed-off-by: Andrii Korotkov <[email protected]>
  • Loading branch information
andrii-korotkov-verkada committed Jun 27, 2024
1 parent 52b1b43 commit 73b7607
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions controller/appcontroller.go
Original file line number Diff line number Diff line change
Expand Up @@ -866,10 +866,8 @@ func (ctrl *ApplicationController) requestAppRefresh(appName string, compareWith
}
if after != nil {
ctrl.appRefreshQueue.AddAfter(key, *after)
ctrl.appOperationQueue.AddAfter(key, *after)
} else {
ctrl.appRefreshQueue.AddRateLimited(key)
ctrl.appOperationQueue.AddRateLimited(key)
}
}
}
Expand Down Expand Up @@ -1480,6 +1478,9 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo
log.Errorf("Recovered from panic: %+v\n%s", r, debug.Stack())
}
ctrl.appRefreshQueue.Done(appKey)
// In most cases we want to have app operation update happen after the sync, so
// there's no race condition and app updates not proceeding. See https://github.com/argoproj/argo-cd/issues/18500.
ctrl.appOperationQueue.AddRateLimited(appKey)
}()
obj, exists, err := ctrl.appInformer.GetIndexer().GetByKey(appKey.(string))
if err != nil {
Expand Down

0 comments on commit 73b7607

Please sign in to comment.