enable concurrency for pp and cpp #3511

zach593 · 2023-05-10T04:46:30Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Recently, we encountered a slow restart issue where it took 15 minutes to clear the items in queues. We suspect that the number of workers in pp/cpp could be one of the reasons for this problem. Therefore, it would be reasonable to add a concurrency configuration for this.

Although the pp/cpp reconcile function appears to be capable of handling concurrency, the number of workers is hardcoded as 1. Please let me know if there is any particular reason for this.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-controller-manager`: Introduced `--concurrent-propagation-policy-syncs`/`--concurrent-cluster-propagation-policy-syncs` flags to specify concurrent syncs for PropagationPolicy and ClusterPropagationPolicy.

codecov-commenter · 2023-05-10T05:07:04Z

Codecov Report

Merging #3511 (cac02e0) into master (17eae7d) will increase coverage by 0.00%.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master    #3511   +/-   ##
=======================================
  Coverage   52.64%   52.64%           
=======================================
  Files         213      213           
  Lines       19581    19583    +2     
=======================================
+ Hits        10308    10310    +2     
  Misses       8721     8721           
  Partials      552      552

Flag	Coverage Δ
unittests	`52.64% <0.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/controller-manager/app/options/options.go	`0.00% <0.00%> (ø)`
pkg/detector/detector.go	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

XiShanYongYe-Chang

@zach593, thanks for your feedback, I think it's a reasonable update.

cmd/controller-manager/app/controllermanager.go

Signed-off-by: zach593 <zach_li@outlook.com>

XiShanYongYe-Chang

/lgtm

XiShanYongYe-Chang · 2023-05-10T09:36:54Z

Hi @zach593 can you help add release note to describe the update?

RainbowMango

/assign
I'll take a look tomorrow.

jwcesign · 2023-05-11T02:52:17Z

cmd/controller-manager/app/options/options.go

@@ -202,6 +206,8 @@ func (o *Options) AddFlags(flags *pflag.FlagSet, allControllers, disabledByDefau
 	flags.IntVar(&o.ConcurrentResourceBindingSyncs, "concurrent-resourcebinding-syncs", 5, "The number of ResourceBindings that are allowed to sync concurrently.")
 	flags.IntVar(&o.ConcurrentWorkSyncs, "concurrent-work-syncs", 5, "The number of Works that are allowed to sync concurrently.")
 	flags.IntVar(&o.ConcurrentNamespaceSyncs, "concurrent-namespace-syncs", 1, "The number of Namespaces that are allowed to sync concurrently.")
+	flags.IntVar(&o.ConcurrentPropagationPolicySyncs, "concurrent-propagation-policy-syncs", 1, "The number of PropagationPolicy that are allowed to sync concurrently.")
+	flags.IntVar(&o.ConcurrentClusterPropagationPolicySyncs, "concurrent-cluster-propagation-policy-syncs", 1, "The number of ClusterPropagationPolicy that are allowed to sync concurrently.")


Do CPP and PP need to be configured separately? @XiShanYongYe-Chang

For users, the usage of cpp and pp may be different, so separate control may be a little more fine-grained.

jwcesign · 2023-05-11T02:53:09Z

cmd/controller-manager/app/options/options.go

@@ -202,6 +206,8 @@ func (o *Options) AddFlags(flags *pflag.FlagSet, allControllers, disabledByDefau
 	flags.IntVar(&o.ConcurrentResourceBindingSyncs, "concurrent-resourcebinding-syncs", 5, "The number of ResourceBindings that are allowed to sync concurrently.")
 	flags.IntVar(&o.ConcurrentWorkSyncs, "concurrent-work-syncs", 5, "The number of Works that are allowed to sync concurrently.")
 	flags.IntVar(&o.ConcurrentNamespaceSyncs, "concurrent-namespace-syncs", 1, "The number of Namespaces that are allowed to sync concurrently.")
+	flags.IntVar(&o.ConcurrentPropagationPolicySyncs, "concurrent-propagation-policy-syncs", 1, "The number of PropagationPolicy that are allowed to sync concurrently.")


If it's slow by default, I prefer to set the default value bigger, like 3?

Before this pull request, concurrency was hardcoded as "1". I believe it would be better to set the default value to be the same as the value before the changes, so that users who did not modify this flag do not need to consider this change. If you think it's necessary, we can increase this value.

RainbowMango

I'd say it makes sense to add a flag to specify the concurrency, before looking into the code, I might want to ask some questions to better understand this issue.

Recently, we encountered a slow restart issue where it took 15 minutes to clear the items in queues.

I wonder which queue we are talking about here? And How do you know it is the queue that is blocked starting? Are there some logs or metrics?
How many resource templates are in your system?

zishen

Good to add concurrency configuration for pp and cpp
.

Poor12 · 2023-05-12T03:29:15Z

Hi @zach593, you can get metrics from karmada-controller-manager to prove that work queue is heavy to deal with items:
eg:

workqueue_adds_total{name="propagationPolicy reconciler"} 
workqueue_depth{name="propagationPolicy reconciler"}

RainbowMango · 2023-05-13T08:01:53Z

Good to add concurrency configuration for pp and cpp

Yes, I totally agree that allows people the specify the concurrency.

But I'm curious about the root cause of the issue:

Recently, we encountered a slow restart issue where it took 15 minutes to clear the items in queues.

We need to identify the root cause of the problem, develop solutions to fix it, and ultimately be able to verify it.

zach593 · 2023-05-15T02:25:01Z

this is monitoring data on grafana, for last time we restart the karmada-controller-manager. you could see pp reconciler used about 3-4 minutes to clear the queue items.

pp/cpp reconciler is the upstream reconciler of the resource detector, and the output (workload template) of the upstream reconciler (pp/cpp) will be added to the downstream (resource detector) queue. Therefore, if the upstream reconciler works slowly, the downstream will need to reconcile some items twice. The same situation can also occur between detector -> rb/crb, rb/crb -> work, and the slow processing speed of the upstream will amplify the number of reconciles that the downstream needs to do.

therefore, adding concurrency for pp/cpp is reasonable. This will reduce the total number of items that karmada-controller-manager needs to reconcile when restarted.

@RainbowMango @Poor12

zach593 · 2023-05-15T03:05:04Z

Hi @zach593 can you help add release note to describe the update?

User can control the concurrency of PropagationPolicyWorker and ClusterPolicyReconcileWorker by using the flags `concurrent-propagation-policy-syncs` and `concurrent-cluster-propagation-policy-syncs`.

done @XiShanYongYe-Chang

RainbowMango · 2023-05-15T07:38:33Z

The grafana graph is awesome!

Have you tested this patch? Has the consumption speed of the item queue been improved? It would be great if we have two graphs from that we can see the difference in their effects.

XiShanYongYe-Chang · 2023-05-15T08:37:19Z

Hi @zach593 can you help add release note to describe the update?

User can control the concurrency of PropagationPolicyWorker and ClusterPolicyReconcileWorker by using the flags `concurrent-propagation-policy-syncs` and `concurrent-cluster-propagation-policy-syncs`.

done @XiShanYongYe-Chang

How about update like this:

karmada-controller-manager: add `concurrent-propagation-policy-syncs`, `concurrent-cluster-propagation-policy-syncs` flags to adjust pp/cpp syncs speed.

zach593 · 2023-05-16T11:54:20Z

Have you tested this patch? Has the consumption speed of the item queue been improved? It would be great if we have two graphs from that we can see the difference in their effects.

Yes, we have tested it, but the monitoring data is not convincing enough yet.

before patch:

patched (10 workers):

seems reconciles per minute have increase from 3-4k to 5.6k

because cpu was already bursting, effect of this patch may not have been fully released.
and because the monitoring system's service discovery is not timely, some monitoring data is missing

zach593 · 2023-05-16T11:55:50Z

How about update like this:

karmada-controller-manager: add `concurrent-propagation-policy-syncs`, `concurrent-cluster-propagation-policy-syncs` flags to adjust pp/cpp syncs speed.

cool, done

RainbowMango

/approve

Many thanks to @zach593, we can go with the default concurrency (1), I appreciate all your effort on it. If you have any evidence that we need to increase the concurrency, please feel free to send a PR. Thanks in advance. :)

karmada-bot · 2023-05-17T10:32:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

~~OWNERS~~ [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label May 10, 2023

karmada-bot requested review from Garrybest and lonelyCZ May 10, 2023 04:46

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 10, 2023

zach593 force-pushed the concurrent-pp branch from 6003199 to 6101bfe Compare May 10, 2023 04:47

XiShanYongYe-Chang reviewed May 10, 2023

View reviewed changes

cmd/controller-manager/app/controllermanager.go Outdated Show resolved Hide resolved

enable concurrency for pp and cpp

8d221b1

Signed-off-by: zach593 <zach_li@outlook.com>

zach593 force-pushed the concurrent-pp branch from cac02e0 to 8d221b1 Compare May 10, 2023 08:12

XiShanYongYe-Chang reviewed May 10, 2023

View reviewed changes

karmada-bot assigned XiShanYongYe-Chang May 10, 2023

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2023

RainbowMango reviewed May 10, 2023

View reviewed changes

karmada-bot assigned RainbowMango May 10, 2023

jwcesign reviewed May 11, 2023

View reviewed changes

RainbowMango reviewed May 11, 2023

View reviewed changes

zishen reviewed May 12, 2023

View reviewed changes

RainbowMango added this to the v1.6 milestone May 16, 2023

RainbowMango approved these changes May 17, 2023

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2023

karmada-bot merged commit 0789c83 into karmada-io:master May 17, 2023
11 checks passed

zishen mentioned this pull request Aug 21, 2023

REQUEST: New membership for zishen karmada-io/community#51

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable concurrency for pp and cpp #3511

enable concurrency for pp and cpp #3511

zach593 commented May 10, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented May 10, 2023 •

edited

Loading

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang commented May 10, 2023

RainbowMango left a comment

jwcesign May 11, 2023

XiShanYongYe-Chang May 11, 2023

jwcesign May 11, 2023

zach593 May 15, 2023

RainbowMango left a comment

zishen left a comment

Poor12 commented May 12, 2023

RainbowMango commented May 13, 2023

zach593 commented May 15, 2023

zach593 commented May 15, 2023 •

edited

Loading

RainbowMango commented May 15, 2023

XiShanYongYe-Chang commented May 15, 2023

zach593 commented May 16, 2023

zach593 commented May 16, 2023

RainbowMango left a comment

karmada-bot commented May 17, 2023

enable concurrency for pp and cpp #3511

enable concurrency for pp and cpp #3511

Conversation

zach593 commented May 10, 2023 • edited by RainbowMango Loading

codecov-commenter commented May 10, 2023 • edited Loading

Codecov Report

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented May 10, 2023

RainbowMango left a comment

Choose a reason for hiding this comment

jwcesign May 11, 2023

Choose a reason for hiding this comment

XiShanYongYe-Chang May 11, 2023

Choose a reason for hiding this comment

jwcesign May 11, 2023

Choose a reason for hiding this comment

zach593 May 15, 2023

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

zishen left a comment

Choose a reason for hiding this comment

Poor12 commented May 12, 2023

RainbowMango commented May 13, 2023

zach593 commented May 15, 2023

zach593 commented May 15, 2023 • edited Loading

RainbowMango commented May 15, 2023

XiShanYongYe-Chang commented May 15, 2023

zach593 commented May 16, 2023

zach593 commented May 16, 2023

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented May 17, 2023

zach593 commented May 10, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented May 10, 2023 •

edited

Loading

zach593 commented May 15, 2023 •

edited

Loading