馃悰 Fix ClusterCacheTracker memory leak #9543
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @ionutbalutoiu! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @ionutbalutoiu. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/M
k8s-ci-robot
added
ok-to-test
ionutbalutoiu
changed the title
ionutbalutoiu
force-pushed
the
fix/cluster-cache-tracker-memory-leak
branch
from
36c0b2c
to
88e53de
Compare
|
Fixed the following
|
ionutbalutoiu
force-pushed
the
fix/cluster-cache-tracker-memory-leak
branch
from
88e53de
to
9dee01c
Compare
|
/test |
|
@killianmuldoon: The
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test pull-cluster-api-e2e-main |
|
/test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main |
|
/assign Would like some time to review this PR in a bit more details |
ionutbalutoiu
force-pushed
the
fix/cluster-cache-tracker-memory-leak
branch
from
9dee01c
to
8cf7a66
Compare
|
When you get the chance, please see the updated PR. It would be nice to have this fix included in the next CAPI release. |
ionutbalutoiu
force-pushed
the
fix/cluster-cache-tracker-memory-leak
branch
from
8cf7a66
to
5009688
Compare
|
@vincepri Implemented the following changes for your most recent code review. LE: Also, fixed the above changes linter errors with this. Thank-you! |
ionutbalutoiu
force-pushed
the
fix/cluster-cache-tracker-memory-leak
branch
from
5009688
to
a1e613e
Compare
|
@ionutbalutoiu Sorry for the long delay. I sort of lost track of this PR a bit. Happy to quickly review again and merge once the open points are addressed |
Decouple the code that creates uncached client from the code that creates the cached client. This way, we don't start the cache until the cached client is created. This avoids scenarios where the `ClusterCacheTracker` cache is started when only an uncached client is needed. There is an existing code path, described in the original issue, where the `ClusterCacheTracker` cache is started when only an uncached client is needed. The cache is re-created later, and the initial cache is still running continuously in the background. Signed-off-by: Ionut Balutoiu <ionut@balutoiu.com>
ionutbalutoiu
force-pushed
the
fix/cluster-cache-tracker-memory-leak
branch
from
a1e613e
to
97534b8
Compare
Thank-you for taking the time to review this! I've addressed all the remaining review open items. I also did a rebase against latest Please take a look at the latest PR changes and let me know if there's anything else. |
|
Thank you very much! Let's see if we have some luck with cherry-picking /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 71997f2a16ee7e4a9234d20c20f8fe449eee9a18
|
|
/cherry-pick release-1.6 |
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
/cherry-pick release-1.5 |
k8s-ci-robot
added
the
approved
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@sbueringer: new pull request created: #10064 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@sbueringer: new pull request created: #10065 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Decouple the code that creates uncached client from the code that creates the cached client. This way, we don't start the cache until the cached client is created.
This avoids scenarios where the
ClusterCacheTrackercache is started when only an uncached client is needed.There is an existing code path, described in the original issue, where the
ClusterCacheTrackercache is started when only an uncached client is needed. The cache is re-created later, and the initial cache is still running continuously in the background.What this PR does / why we need it:
Fix the memory leak described in the issue #9542, when cluster
kubeconfigis rotated frequently,Which issue(s) this PR fixes:
Fixes #9542