🐛 fix work agent performance issue #785
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #785 +/- ##
==========================================
+ Coverage 63.77% 63.78% +0.01%
==========================================
Files 192 192
Lines 18596 18608 +12
==========================================
+ Hits 11860 11870 +10
- Misses 5756 5759 +3
+ Partials 980 979 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/hold |
elgnay
force-pushed
the
fix_work_perf_issue
branch
2 times, most recently
from
ce649d1 to
387676c
Compare
| @@ -175,6 +175,9 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac | |||
| // if needed. | |||
| if !mwUpdated && !amwUpdated && requeueTime < MaxRequeueDuration { | |||
| controllerContext.Queue().AddAfter(manifestWorkName, requeueTime) | |||
| } else { | |||
| // resync each manifestwork every 5 minutes | |||
| controllerContext.Queue().AddAfter(manifestWorkName, 5*time.Minute) | |||
There was a problem hiding this comment.
I do not think we need else here, we do not need to requeue when manifestwork or appliedmanfiestwork is udpated. I think we only need to change this line
if !mwUpdated && !amwUpdated && requeueTime < MaxRequeueDuration
to
if !mwUpdated && !amwUpdated
There was a problem hiding this comment.
Updated as suggested.
| spokeWorkInformerFactory := workinformers.NewSharedInformerFactory(spokeWorkClient, 5*time.Minute) | ||
| // resync with a small interval could result in performance issue when the number of appliedmanifestworks | ||
| // is large. | ||
| spokeWorkInformerFactory := workinformers.NewSharedInformerFactory(spokeWorkClient, 21*time.Hour) |
There was a problem hiding this comment.
If both manifestwork informer and appliedmanifestwork informer resync every 24 hours, they may resync at the same time that will result in a huge peak. However, if they have different resync intervals, it will bring less pressure to the work agent.
There was a problem hiding this comment.
could we add some commenst on this?
elgnay
force-pushed
the
fix_work_perf_issue
branch
from
387676c to
3952304
Compare
| @@ -138,7 +137,7 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac | |||
| } | |||
| newAppliedManifestWork := appliedManifestWork.DeepCopy() | |||
|
|
|||
| var requeueTime = MaxRequeueDuration | |||
| var requeueTime = ResyncInterval | |||
There was a problem hiding this comment.
ResyncInterval is also set in withResync. Should these two to be set the same?
There was a problem hiding this comment.
Do you mean ResyncEvery? It has already used the ResyncInterval although I think it is useless for the ManifestWorkAgent controller.
WithSync(controller.sync).ResyncEvery(ResyncInterval).ToController("ManifestWorkAgent", recorder)There was a problem hiding this comment.
can we add some comment for ResyncInterval, given it is used at multiple locations.
Signed-off-by: Yang Le <[email protected]>
elgnay
force-pushed
the
fix_work_perf_issue
branch
from
3952304 to
f5a77b3
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elgnay, qiujian16 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 |
|
@elgnay So after this change, after a manifestwork containing a configmap is applied, if I delete the configmap on the manged cluster manually, how long will the work agent recreate the configmap? 24 hours? |
Still each ManifestWork will be resynced at intervals not exceeding 5 minutes. The only change is that this logic no longer leverages the resync of the informers. In another word, the ManifestWork will be requeued by the controller (https://github.com/open-cluster-management-io/ocm/pull/785/files#diff-f0293b9137d86645469d2f7303ca98e1845a7b482671a25427e774a5c56c08abL177) after it has been processed successfully. |
|
/lgtm |
|
/unhold |
openshift-merge-bot
bot
merged commit 9af100f
into
open-cluster-management-io:main
Summary
Related issue(s)
Fixes #