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

Add optional in-memory cache of HelmRepository index files #626

Merged
merged 3 commits into from
Apr 4, 2022

Conversation

souleb
Copy link
Member

@souleb souleb commented Mar 19, 2022

If implemented, will provide users with a way to cache index files.

This addresses issues where the index file is loaded and unmarshalled in
concurrent reconciliation resulting in a heavy memory footprint.

The caching strategy used is cache aside, and the cache is a k/v store
with expiration.

The cache number of entries and ttl for entries are configurable.

The cache is optional and is disabled by default

Signed-off-by: Soule BA soule@weave.works

Fix: fluxcd/flux2#2072

@souleb souleb marked this pull request as draft March 19, 2022 23:00
@souleb souleb added the enhancement New feature or request label Mar 19, 2022
@tehlers320
Copy link

.DS_Store should be checked in here.

@souleb souleb force-pushed the reuse-index-pool branch 2 times, most recently from 0551e72 to 7865323 Compare March 20, 2022 07:27
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.

Do we remove the cache entry on HelmRepository finalization?

@souleb
Copy link
Member Author

souleb commented Mar 21, 2022

Do we remove the cache entry on HelmRepository finalization?

My assumption was that if a source is unavailable, the chart reconciliation fails early, before attempting to load the index. hence we just wait for the corresponding cache entry expiration and deletion.

@hiddeco
Copy link
Member

hiddeco commented Mar 21, 2022

Nit:

[..]
The caching strategy used is cache aside, and the cache is a k/v store
with expiration.
[..]
The cache is optional and is disabled by default

General comment, but not blocking for introduction:

I feel that the introduction of this cache could partly eliminate the need of some of the simpler and naive caching approaches introduced by me, thereby simplifying the overall code. This might be worth looking into as a follow up.

@hiddeco hiddeco added the area/helm Helm related issues and pull requests label Mar 21, 2022
@souleb souleb force-pushed the reuse-index-pool branch 2 times, most recently from aaf287c to b36c837 Compare March 21, 2022 16:12
internal/cache/cache.go Outdated Show resolved Hide resolved
@souleb souleb force-pushed the reuse-index-pool branch 3 times, most recently from b94bdbc to 26bfa28 Compare March 29, 2022 06:55
@souleb souleb marked this pull request as ready for review March 29, 2022 06:55
@souleb
Copy link
Member Author

souleb commented Mar 29, 2022

This PR is to introduce the cache. There will be follow up work, but for now I want to merge it so users can start testing.

@stefanprodan stefanprodan changed the title Cache HelmRepository index files Add optional in-memory cache of HelmRepository index files Mar 29, 2022
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 @souleb 🥇

main.go Outdated Show resolved Hide resolved
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
@souleb souleb force-pushed the reuse-index-pool branch 2 times, most recently from 3a633e4 to 435359b Compare March 31, 2022 10:47
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/helmcharts.md Outdated Show resolved Hide resolved
@souleb souleb force-pushed the reuse-index-pool branch 2 times, most recently from e561349 to d2761b6 Compare March 31, 2022 13:14
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Happy with this for experimental introduction, thanks a lot @souleb, and looking forward to the further improvements 🥇 🙇

@stefanprodan
Copy link
Member

After we release flux with this change, we need to update the bootstrap cheatsheet to show users how to enable the cache.

@stefanprodan stefanprodan added this to the GA milestone Apr 1, 2022
@souleb
Copy link
Member Author

souleb commented Apr 1, 2022

After we release flux with this change, we need to update the bootstrap cheatsheet to show users how to enable the cache.

I will create an issue for that.

If implemented, will provide users with a way to cache index files.

This addresses issues where the index file is loaded and unmarshalled in
concurrent reconciliation resulting in a heavy memory footprint.

The caching strategy used is cache aside, and the cache is a k/v store
with expiration.

The cache number of entries and ttl for entries are configurable.

The cache is optional and is disabled by default

Signed-off-by: Soule BA <soule@weave.works>
Signed-off-by: Soule BA <soule@weave.works>
Signed-off-by: Soule BA <soule@weave.works>
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!

@stefanprodan stefanprodan merged commit 362bc56 into fluxcd:main Apr 4, 2022
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 enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Source controller is unstable
6 participants