Introduce --enable-cluster-resource-modeling to customize resource modeling feature
#2387
Conversation
Signed-off-by: jingxueli <jingxueli@trip.com>
karmada-bot
added
the
kind/feature
karmada-bot
added
the
size/M
RainbowMango
force-pushed
the
pr_resource_modeling_control_flag
branch
from
6c88217
to
214cff2
Compare
RainbowMango
added
the
approved
RainbowMango
force-pushed
the
pr_resource_modeling_control_flag
branch
from
214cff2
to
6495f80
Compare
RainbowMango
removed
the
approved
|
I forgot to add the controller parameters at here, that's the root cause of the CI failure. |
| } | ||
| if c.EnableClusterResourceModeling { | ||
| // get or create informer for pods and nodes in member cluster | ||
| clusterInformerManager, err := c.buildInformerForCluster(clusterClient) |
There was a problem hiding this comment.
The buildInformerForCluster now helps to initialize the informer for clusters, the informer will be used by other controllers.
We must de-coupling it before moving this forward.
/hold
There was a problem hiding this comment.
+1
The current control coupling is a little severe.
There was a problem hiding this comment.
Maybe we can build the generic informer as usual but not build the typed informer(just for cluster modeling) in buildInformerForCluster?
There was a problem hiding this comment.
That's definitely a workable solution.
But the annoying thing is it's a bad program paradigm. Too much coupling leads to hard to maintain.
Now, the multi-cluster-informer-manager is initialized by cluster-status-controller but used by execution-controller , and work-status-controller. :(
I'm trying to decouple them.
karmada-bot
added
the
do-not-merge/hold
karmada-bot
added
size/L
|
I just pushed two more commits, the first one is used to address the temporary solution, and the last one is just for verification and it will be removed before moving forward. |
Signed-off-by: RainbowMango <qdurenhongcai@gmail.com>
RainbowMango
force-pushed
the
pr_resource_modeling_control_flag
branch
from
0926e6c
to
95036ff
Compare
|
ready for review again :) @dddddai |
| dynamicClient, err := c.ClusterDynamicClientSetFunc(clusterClient.ClusterName, c.Client) | ||
| if err != nil { | ||
| klog.Errorf("Failed to build dynamic cluster client for cluster %s.", clusterClient.ClusterName) | ||
| return |
There was a problem hiding this comment.
Should we return err? (keep the behavior as before)
There was a problem hiding this comment.
I think just one log is enough.
Even in case of error, what else can the cluster status controller do except log again the error?
The error shouldn't block the normal process.
|
/lgtm |
RainbowMango
added
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR actually takes over #1858, fixed comments.
Which issue(s) this PR fixes:
Fixes #1858
Special notes for your reviewer:
Does this PR introduce a user-facing change?: