search

crossplane-contrib / provider-argocd

Crossplane provider to provision and manage Argo CD objects
Apache License 2.0
68 stars 35 forks source link

fix: resolves memory leak issue by using `WithExternalConnectDisconnecter` #146

Closed cychiang closed 6 months ago

cychiang commented 6 months ago

Description of your changes

Fixes #138

This PR is trying to solve the memory leak issue by handling the io.Closer when creating the argocd client. The original implementation ignore the io.Closer, and this PR is trying to handle it on WithExternalConnectDisconnecter.

Before the fix, you can see that the memory usage is growing overtime, just like mentioned in the #138

profile002 profile004

Afterr the fix, you can see that the memory usage becomes stable overtime.

profile007 profile011

I haven't figure it out how to make a good graph to present the result, but it would be good to get some help to verify the changes.

I have:

How has this code been tested

MisterMX commented 6 months ago

E2E are still failing due to a bug in the script. https://github.com/crossplane-contrib/provider-argocd/pull/149 is about to fix this.

Can you rebase on master once the fix is merged - and squash your commits?

cychiang commented 6 months ago

E2E are still failing due to a bug in the script. #149 is about to fix this.

Can you rebase on master once the fix is merged - and squash your commits?

No problem. I will wait until #149 is merged.

cychiang commented 6 months ago

I updated the PR and rebase to the latest main branch.

jefflantz commented 5 months ago

Thank you cychiang!

@MisterMX, thanks for helping resolve this. Since this change has been out for a few months now and seems stable, can you please release + push a new patch version that includes this change? I have been referring to the package built on this commit, but a proper release would be appreciated, thanks.

Xtigyro commented 5 months ago

Thank you cychiang!

@MisterMX, thanks for helping resolve this. Since this change has been out for a few months now and seems stable, can you please release + push a new patch version that includes this change? I have been referring to the package built on this commit, but a proper release would be appreciated, thanks.

Yeah - this will be useful!