Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

helm: only use Digest to calculcate index revision #1035

Merged
merged 3 commits into from
Feb 23, 2023
Merged

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Feb 22, 2023

Release candidate: ghcr.io/fluxcd/source-controller:rc-e5c0313d

In #1001 bits around the Helm repository reconciliation logic were rewritten, mostly based
on the documented behavior instead of the actual code. This resulted in the reintroduction
of a YAML marshal of the (sorted) index YAML instead of reliance of just the checksum of
the file.

This to take situations into account in which a repository would e.g. provide a new
random order on every generation. However, this approach is (extremely) expensive as
the encoder goes through a JSON -> YAML loop, eating lots of RAM in the process.

As the further (silently) introduced behavior has not resulted in any reported issues, I deem
this approach safe and better than e.g. encoding to just JSON which would still require a
substantial amount of memory.

Fixes fluxcd/flux2#3620

@hiddeco hiddeco added bug Something isn't working area/helm Helm related issues and pull requests labels Feb 22, 2023
@hiddeco hiddeco force-pushed the helm-index-digest-rev branch from 03300fd to 5f2bcfc Compare February 22, 2023 22:19
In #1001 bits around the Helm repository reconciliation logic were
rewritten, mostly based on the documented behavior instead of the
actual code. This resulted in the reintroduction of a YAML marshal of
the (sorted) index YAML instead of reliance of just the checksum of the
file.

This to take situations into account in which a repository would e.g.
provide a new random order on every generation. However, this approach
is (extremely) expensive as the marshal goes through a JSON -> YAML
loop, eating lots of RAM in the process.

As the further (silently) introduced behavior has not resulted in any
reported issues, I deem this approach safe and better than e.g.
encoding to just JSON which would still require a substantial amount of
memory.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco force-pushed the helm-index-digest-rev branch from 5f2bcfc to c712fed Compare February 22, 2023 22:35
@hiddeco hiddeco marked this pull request as ready for review February 22, 2023 22:39
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

@hiddeco hiddeco merged commit b951cbb into main Feb 23, 2023
@hiddeco hiddeco deleted the helm-index-digest-rev branch February 23, 2023 10:29
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area/helm Helm related issues and pull requests bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

source-controller Deployment v0.35.1 needs higher memory limit
2 participants