-
Notifications
You must be signed in to change notification settings - Fork 183
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 HIP for running hooks in parallel #349
Open
MichaelMorrisEst
wants to merge
3
commits into
helm:main
Choose a base branch
from
Nordix:hip-hook-parallelism
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
--- | ||
hip: 0019 | ||
title: "Enable Running Hooks in Parallel" | ||
authors: [ "Michael Morris <[email protected]>" ] | ||
created: "2024-06-06" | ||
type: "feature" | ||
status: "draft" | ||
--- | ||
|
||
## Abstract | ||
|
||
Helm hooks are currently executed serially. In many cases there will be no reason why they cannot be run in parallel. Running hooks in parallel can significantly improve install/upgrade times | ||
|
||
## Motivation | ||
|
||
Decrease install/upgrade times by executing hooks in parallel | ||
|
||
## Specification | ||
|
||
Add a new flag to the install, upgrade, rollback and uninstall commands to enable parallel behavior for hook execution. The flag will be of integer type and will allow the user to specify the maximum number of hooks that can be executed in parallel. | ||
|
||
The flag shall be optional. If not set or set to "0" or "1", hook execution shall be done serially. If set to a value greater than "1", hook execution shall be done in parallel, subject to not exceeding the given value in the number of hooks executed in parallel. | ||
|
||
Only hooks of the same weight shall be executed in parallel. | ||
|
||
## Rationale | ||
|
||
While it may be desirable to run hooks in parallel, particularly those of the same weight, doing so by default may cause issues for users depending on the existing serial behavior. Therefore it is proposed to keep the existing serial behavior by default and enable parallel behavior through the use of a flag. | ||
|
||
Making the flag an integer rather than boolean allows the user to possibility to restrict the number of parallel processes where resource constraints are a concern. | ||
|
||
Allowing hooks of different weights to be executed in parallel is probably not consistent with the documented behavior of lower weight hooks being executed before higher weighted hooks and, therefore only hooks of the same weight shall be executed in parallel | ||
|
||
## Backwards compatibility | ||
|
||
Proposed solution is fully backwards compatible with no changes in default behavior. | ||
|
||
## Security implications | ||
|
||
None | ||
|
||
## How to teach this | ||
|
||
Document in command line help and https://helm.sh/docs/topics/charts_hooks/. | ||
|
||
## Reference implementation | ||
|
||
PR submitted some time ago: https://github.com/helm/helm/pull/11804 | ||
May need to be updated depending on decisions taken on what is proposed here. | ||
|
||
## Rejected ideas | ||
|
||
Why certain ideas that were brought while discussing this HIP were not | ||
ultimately pursued. | ||
|
||
## Open issues | ||
|
||
|Issue |Proposal|Decision| | ||
|-----------------------------------------------------------------------------------------|--------|--------| | ||
|Should we allow hooks to be run in parallel |Yes | | | ||
|Should the default behaviour continue to be to execute in serial |Yes | | | ||
|Is there benefit in allowing the user limit the max number of hooks executing in parallel|Yes | | | ||
|Should only hooks of the same weight be executed in parallel |Yes | | | ||
|
||
|
||
## References | ||
|
||
There exists an older closed HIP on hook parallelism, but its focus was on test hooks: https://github.com/helm/community/pull/165 |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the different user roles for a Helm user. The person installing a chart may have little knowledge of how the package works. Installers of a chart aren't expected to know the business logic of the application. Instead, that knowledge is held by someone distributing the application.
Consider a classic package management situation like installing postgresql on a linux server. The one installing the package may not know where all the files need to live on the operating system, what configuration files need to go where, or the way this app should be added to startup as a daemon. Without having all that knowledge they are able to use the application.
Someone using the app may likely not know if hooks can be run in parallel. The one who would know that is the developer of the chart.
Can we bake this into the chart metadata so the responsibility is on the side of the chart developer instead of the chart user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mattfarina
Proposed solution updated to address comment. Please take a look when you get time and let me know if there are any further comments