karmada

karmada copied to clipboard

What type of PR is this?

/kind feature

What this PR does / why we need it: When member cluster is large(3k nodes, 10w pods), karmada-controller-manager is easy to be OOMkilled and retry to sync nodes and pods again and again, the member cluster also stays in unknown.

Whether to execute buildInformerForCluster() to sync nodes and pods should be configurable especially for large cluster.

Does this PR introduce a user-facing change?:

A new flag `build-informer-for-cluster` for controller-manager and agent.

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please assign kevin-wangzefeng after the PR has been reviewed. You can assign the PR to them by writing /assign @kevin-wangzefeng in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

When member cluster is large(3k nodes, 10w pods), karmada-controller-manager is easy to be OOMkilled

That's very bad. I wonder to know how many memory reserved for karmada-controller-manager?

Yeah I have the same concern, control-plane stores all pods and nodes(for push mode clusters) in the informer cache, which consumes lots of memory in large scale scenarios

IMO we can transform objects in informers, this is a new feature in client-go 0.24 while we are using 0.23 now

@dddddai Great idea, I like this feature! Just created #1866 to track this. Let's do it at release-1.3.

Before that feature, I still want to know more details about how bad the situation is now. It can also evaluate the effect of this feature in the future.

When member cluster is large(3k nodes, 10w pods), karmada-controller-manager is easy to be OOMkilled

That's very bad. I wonder to know how many memory reserved for karmada-controller-manager?

One cluster with 3k nodes and 10w pods needs rough100G( far exceed 100G when cluster failure occurs) for kube-controller-manager, so karmada-controller-manager may need more than 100G * n, n is the number of clusters.

One cluster with 3k nodes and 10w pods needs rough100G( far exceed 100G when cluster failure occurs) for kube-controller-manager, so karmada-controller-manager may need more than 100G * n, n is the number of clusters.

I'm very surprised! @Poor12 is leading the effort to do performance testing and improvement. He might confirm it later.

But if we don't sync node and pods here, we can't get the resource summary, then the general estimator will always deny workloads as no resources left .
https://github.com/karmada-io/karmada/blob/d9b043bedd4507b9f81196f55ff1bd3fd5df6ab0/pkg/estimator/client/general.go#L36-L39

@RainbowMango member cluster probably can calculate resource summary and report to fed cluster.

@RainbowMango member cluster probably can calculate resource summary and report to fed cluster.

I'm not sure if you are saying that karmada-agent now takes the responsibility to report resource summary for clusters in Pull mode.

I want to let you know that @Poor12 is trying to figure out how serious the issue is.

Let's wait for a while. cc guys who met this situation here. cc @likakuli

copy that, thx

@snowplayfire We have been doing performance tests on this for the last few weeks. Now we can confirm that there is a performance issue but seems not so bad as mentioned on https://github.com/karmada-io/karmada/pull/1858#issuecomment-1134170421.

There are several improvement ideas:

• 1. change the generic informer to typed informer which is addressed by #2318
• 2. Add the cache then we don't need to list all things out. it is addressed by #2034
• 3. Adopt informer transform func to save memory. It is addressed by #2089

I think it's fair enough to make a flag for people to enable/disable the resource-modeling thing. Since we are going to make changes to the code that this PR also touching, I'd like to move this forward first to avoid conflict.

I'm sorry for let this sit so long, now there has conflicts that need to be resolved, please let me know if you can solve them and push again.

Hi @snowplayfire Your branch has conflicts with the master, you might need to rebase your branch.

By the way, I see you are using your Chinese name in commit author:

Author: ljx李静雪 <[email protected]>

Hi @snowplayfire Your branch has conflicts with the master, you might need to rebase your branch.

By the way, I see you are using your Chinese name in commit author:

Author: ljx李静雪 <[email protected]>

done

Hi @snowplayfire I picked your commit and fixed the conflicts as well as my comments at #2387.