Add --skip-refresh flag to the build command
#444
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@indrekj Please fix DCO issue. |
This improves the `helmfile sync` performance. From the code: `BuildDeps` is used only by `runHelmDepBuilds`, which only is used by `PrepareCharts` which is finally only used by `withPreparedCharts`. `withPreparedCharts` already does `SyncReposOnce` which means we do not have to refresh the local repository cache on each chart build. This is only supported in Helm v3. This seems to be mostly affecting helmfiles which have a lot of releases and those release charts use sub dependencies. I saw significant performance improvements for a helmfile with 45 releases, 2 repositories, and most of the charts also had their own dependencies. Results: Before the patch: * real 9m10.565s * real 9m38.335s * real 9m14.941s * real 5m13.106s (with cache) After the patch: * real 6m51.965s * real 6m36.605s * real 6m31.685s * real 3m0.271s (with cache) These were tested with: ``` rm -rf ~/.cache/helmfile ~/.cache/helm ~/.config/helm/repositories.* && helmfile sync ... ``` The result with `(with cache)` was without deleting the caches first. From these metrics it seems that the sync duration decreased 20-45% depending on the run, release count, dependencies and if the cache was used or not. As far as I understand, this should be backward-compatible change. Signed-off-by: Indrek Juhkam <indrek@urgas.eu>
|
Fixed, and fixed a test issue as well. The next run should be green now. |
itscaro
approved these changes
Oct 18, 2022
|
@indrekj I have a question. why not add the skip-refresh option? from the logic. it will add --the skip-refresh option by default when helm v3. |
|
To my understanding, there's no need to do a refresh if we're already doing |
|
@indrekj oh. I see. |
|
Seems good to me. Thanks for your proposal and contribution! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Discussion: #426
This improves the
helmfile syncperformance.From the code:
BuildDepsis used only byrunHelmDepBuilds, which only is used byPrepareChartswhich is finally only used bywithPreparedCharts.withPreparedChartsalready doesSyncReposOncewhich means we do not have to refresh the local repository cache on each chart build.This is only supported in Helm v3.
This seems to be mostly affecting helmfiles which have a lot of releases and those release charts use sub dependencies.
I saw significant performance improvements for a helmfile with 45 releases, 2 repositories, and most of the charts also had their own dependencies. Results:
Before the patch:
After the patch:
These were tested with:
The result with
(with cache)was without deleting the caches first.From these metrics it seems that the sync duration decreased 20-45% depending on the run, release count, dependencies and if the cache was used or not.
As far as I understand, this should be backward-compatible change.