-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 Kcp use one workload cluster for reconcile #8900
🌱 Kcp use one workload cluster for reconcile #8900
Conversation
/test pull-cluster-api-e2e-full-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, just two nits
/retest |
/test pull-cluster-api-e2e-full-main |
/lgtm Not sure about the unit test failure. Just a guess, maybe a race condition with the cache? |
LGTM label has been added. Git tree hash: 2441217d5c3b7a7e9e6705761acae9f2a8aee955
|
/lgtm /hold P.S. I was using this commit yesterday during scale tests and everything worked well |
LGTM label has been added. Git tree hash: 69037646a93380793ae4251266b9f201b8abdc61
|
/lgtm (keeping hold for squash) |
67e162d
to
479ca19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested the performance impact of this, but the changes in the code make complete sense.
/lgtm
/approve
(I think you can unhold whenever you'd like @fabriziopandini)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: killianmuldoon, sbueringer 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 |
/hold cancel |
I can't speak about the exact delta, but overall everything worked and the performance got better |
/area provider/control-plane-kubeadm |
What this PR does / why we need it:
As of today, KCP creates the workloadCluster internal struct several times during every reconciliation, and this is one of the factors slowing down reconciliation duration in a way that can impact performance at scale.
This PR refactor KCP so only one workloadCluster internal struct is going to be created and re-used all around during the same reconcile
Which issue(s) this PR fixes:
Part of #8814