From 08fcf432939f44ae2734207f077b7575bd959af8 Mon Sep 17 00:00:00 2001 From: Jeff van Dam Date: Tue, 28 Feb 2023 13:21:18 +0000 Subject: [PATCH] Add HIP for hooks with equivalent weight run in parallel Signed-off-by: Jeff van Dam --- hips/hip-9999.md | 93 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 hips/hip-9999.md diff --git a/hips/hip-9999.md b/hips/hip-9999.md new file mode 100644 index 00000000..d719c61c --- /dev/null +++ b/hips/hip-9999.md @@ -0,0 +1,93 @@ +--- +hip: 9999 +title: "Hook Parallelism" +authors: [ "Jeff van Dam " ] +created: "2023-02-28" +type: "feature " +status: "draft" + +--- + +## Abstract +This feature would allow for hooks to be run in parallel instead of serially +only for lifecycle hooks e.g pre-update, post-delete, pre-rollback, etc. Since +Helm does not do this already this will save a significant amount of time +during release operations with many hooks of the same weight. + +This feature will NOT work for test hooks! + +## Motivation +The motivation behind this feature is to save time during release lifecycle +operations. Many hooks can run in parallel as they are not in any way tied to +each other, so these hooks can be given the same weight. However even hooks +marked with the same weight are not ran in parallel in helm. This can lead to a +significant amount of time during the install/upgrade/rollback/uninstall, +running hooks in order. Many hooks may only take a few milliseconds to run, +however helm seems to have a built in timer which it seems to use to fire each +hook in order, check that it completes, run the next hook. Many minutes are +lost because of this. + +## Rationale +The proposed design is to add a flag called "hook-parallelism", which defaults +to 1. When hooks are run they are then run in go routines, up to, but not +exceeding the value of the flag concurrently. This will allow for users to only +use the flag if they require their hooks to run in parallel. Not including the +flag, or including the flag with a value of 1 or less will be the same as +running serially. + +This feature will not allow test hooks to be run in parallel because it is up +to the Application Distributor to write tests that they think should run in +parallel or serially. + +## Specification +The flag (hook-parallelism) will be added to all applicable commands (install, +rollback, uninstall, upgrade), and will allow for any hooks attached to those +commands to be run in parallel when their weight is equivalent. Not including a +flag will have the hooks run serially. The actual implementation will use a +semaphore based on the value of the flag to limit the number of hooks being +run. While the number of go routines run will be equal to the number of hooks +(sorted by weight), only the value of the flag number of go routines will be +run at the same time. This will allow the user flexibility in their usage in +relation to their system constraints. + +## Backwards compatibility +This should have no backward compatibility issues. Not including the flag will +have the value default to 1. This will only allow one hook to run at a time, +and the user should notice no difference in the execution of their hooks. + +## Security implications +A malicious user could provide an unreasonably high number for the value of the +hook parallelism flag. This would set the length of the semaphore. This should +not be something that should affect the normal user. + +## How to teach this +A good example would be to have two separate test hooks written. Run initially +and watch one pod spin up and run, then a second after the first has completed. +Next the flag could be added, and shown that both test pods are spun up at the +same time. It might be helpful to mention a general estimate of how many hooks +you want to run in parallel compared to the number of hooks you have associated +with a command. + +## Reference implementation +Currently there is an open Github Pull Request [here](https://github.com/helm/helm/pull/11804) which was based on +[this review](https://github.com/helm/helm/pull/8946) that was based on [this review](https://github.com/helm/helm/pull/7792) which was made from [this issue](https://github.com/helm/helm/issues/7763). + +The reviews that this HIP is based on includes the abilty to parallelize test +hooks which is not the case for this HIP. + + +## Rejected ideas +This HIP is based on [another HIP](https://github.com/helm/community/pull/165) which stalled after several rounds of reviews +pertaining to running test hooks in parallel which this HIP does not want to +accomplish. + + +## Open issues +N/A + +## References + - https://github.com/helm/community/pull/165 + - https://github.com/helm/helm/pull/8946 + - https://github.com/helm/helm/pull/7792 + - https://github.com/helm/helm/issues/7763 + - https://github.com/helm/helm/pull/11804