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

helm: optimise repository index loading #685

Merged
merged 1 commit into from
Apr 26, 2022

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Apr 25, 2022

Avoid validating (and thus loading) indexes if the checksum already exists in storage.
In other words, if the YAML is identical to the Artifact in storage, the reconciliation should
be a no-op, and therefore can short-circuit long/heavy operations.

hiddeco
hiddeco previously approved these changes Apr 25, 2022
@hiddeco hiddeco added enhancement New feature or request area/helm Helm related issues and pull requests labels Apr 25, 2022
@hiddeco hiddeco dismissed their stale review April 25, 2022 09:51

Commit metadata requires rewording.

@hiddeco hiddeco changed the title helm: optimise chart loading helm: optimise repository index loading Apr 25, 2022
@pjbgf
Copy link
Member Author

pjbgf commented Apr 25, 2022

Failures are due to GitHub Packages being degraded and therefore not being able to pull the libgit2 image.

@hiddeco
Copy link
Member

hiddeco commented Apr 25, 2022

why

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.

Other than the degraded GitHub Packages, this looks good to me. Thanks @pjbgf 🙇

@hiddeco hiddeco added the hold Issues and pull requests put on hold label Apr 25, 2022
@hiddeco
Copy link
Member

hiddeco commented Apr 25, 2022

Putting this on hold until we have run it past some affected users.

Avoid validating (and thus loading) indexes if the checksum already exists in storage.
In other words, if the YAML is identical to the Artifact in storage, the reconciliation should
be a no-op, and therefore can short-circuit long/heavy operations.

Co-authored-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@pjbgf pjbgf force-pushed the optimise-helm-load branch 2 times, most recently from 759a2ea to 009504b Compare April 25, 2022 15:18
Comment on lines +71 to +74
if in == nil {
return false
}
return in.Checksum == checksum
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if in == nil {
return false
}
return in.Checksum == checksum
return in != nil && in.Checksum == checksum

Copy link
Member

Choose a reason for hiding this comment

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

The reason I do not use this for nil checks is that when logic changes, there is a chance someone accidentally omits the in != nil check, and introduces a panic.

@hiddeco
Copy link
Member

hiddeco commented Apr 26, 2022

Patch has been confirmed to reduce the memory usage compared to v0.24.1 with ~50%, making it close to the footprint of v0.21.x range.

@hiddeco hiddeco removed the hold Issues and pull requests put on hold label Apr 26, 2022
@hiddeco hiddeco merged commit d7e36e2 into fluxcd:main Apr 26, 2022
@pjbgf pjbgf deleted the optimise-helm-load branch April 26, 2022 06:24
@pjbgf pjbgf added this to the GA milestone Apr 26, 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.

3 participants