Skip to content
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

Reuse transport for Helm downloads #590

Merged
merged 5 commits into from
Mar 2, 2022
Merged

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Feb 23, 2022

This commit updates to a version of Helm 3.8.0, with patches applied to
deal with memory leak and HTTP transport issues. The latter being
described in #578.

Potentially fixes #578

@hiddeco hiddeco added bug Something isn't working area/helm Helm related issues and pull requests labels Feb 23, 2022
@pjbgf
Copy link
Member

pjbgf commented Feb 23, 2022

Initial tests on source controller shows that the behaviour is still the same. After 20 minutes running:

ghcr.io/fluxcd/source-controller:v0.21.2
image

this version
image

To fix #578 we will have to manage the transport lifecycle and send it across to the helm getter.

@pjbgf pjbgf force-pushed the helm-getter-http-transport branch 3 times, most recently from ec93675 to 822c860 Compare February 25, 2022 08:51
@pjbgf
Copy link
Member

pjbgf commented Feb 25, 2022

The last patch seem to have resolved it.
Over 12 hours the number of https connections established for the helm chart targets (github via https) is stable at 2:
image

@pjbgf pjbgf force-pushed the helm-getter-http-transport branch 3 times, most recently from b47d573 to 260637b Compare February 28, 2022 09:25
@hiddeco hiddeco marked this pull request as ready for review March 2, 2022 12:07
@hiddeco hiddeco changed the title Update to patched version of Helm 3.8.0 Reuse transport for Helm downloads Mar 2, 2022
hiddeco and others added 4 commits March 2, 2022 13:02
This commit updates to a version of Helm 3.8.0, with patches applied to
deal with memory leak and HTTP transport issues. The latter being
described in #578.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Reuses the same transport across different helm chart downloads,
whilst resetting the tlsconfig to avoid cross-contamination.

Crypto material is now only processed in-memory and does not
touch the disk.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@pjbgf pjbgf force-pushed the helm-getter-http-transport branch from 632545d to f63681f Compare March 2, 2022 13:55
@hiddeco
Copy link
Member Author

hiddeco commented Mar 2, 2022

This looks good to me, great work @pjbgf 🙇

Blocked by GitHub to stamp it as I initialized the PR, but someone else can probably work the magic.

go.mod Outdated Show resolved Hide resolved
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@pjbgf pjbgf force-pushed the helm-getter-http-transport branch from bb816d9 to 7d61553 Compare March 2, 2022 17:58
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @hiddeco and @pjbgf

Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@pjbgf pjbgf merged commit 59108f4 into main Mar 2, 2022
@pjbgf pjbgf deleted the helm-getter-http-transport branch March 2, 2022 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Source controller does not reuse TCP connections when fetching helm charts
4 participants