-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: helm concurrent deploys with cross dependency #9588
base: main
Are you sure you want to change the base?
feat: helm concurrent deploys with cross dependency #9588
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
5049354
to
15a88c1
Compare
15a88c1
to
88599a4
Compare
@@ -295,8 +307,33 @@ func (h *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Art | |||
return helm.UserErr(fmt.Sprintf("cannot expand chart path %q", r.ChartPath), err) | |||
} | |||
|
|||
for _, dependency := range r.DependsOn { |
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.
what about nested dependencies?
ServiceA depends on ServiceB and ServiceB depends on Service C
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.
I have not created a test for this case but, in theory, it should work the same way. Here's why:
- The first action is to check all services that has dependents, then create the channels to use as wait controllers;
- So, In the case you're suggesting:
- If ServiceC starts first, it will hit the signal break with channels; It's gonna wait for Service B to continue;
- If ServiceB starts in second, it will hit the signal break too, and wait for ServiceA
- ServiceA does not depends on anyone, so it deploys and send the "all clear" signal to his dependents
- ServiceB get's the "all clear" signal, and starts this deployment. When finished, it will also sends his signal for ServiceC
- Finally, ServiceC get's his all clear and do this deployment.
One scenario that may happen is a deadlock. I did not think about it on this PR.
Another thing that I'm thinking is that this deserve a integration testing. I'm still learning about this repo and I will create a separated PR for that. This PR already has the label "size/L" 😱
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.
I suggest you to check this approach #9588 (comment), because the current approach will lead to deadlock(concurrency = 3 and 3 dependencies are waiting, for example).
don't worry about the "size/L", it's ok)
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.
Ok thanks for the feedback @idsulik . I'll work on that. Now I see the point, it's easy to get to a deadlock situation because of the lack of resources.
@orlandoburli welcome! good feature!
flow:
|
Fixes: #5417
Description
This PR is adding a option for concurrent helm releases to wait for other helm charts.
On the following example, the
service3
will wait until helm deploy forservice1
andservice2
to finish. If some of those services fails, theservice3
deployment won't start.User facing changes
User can add the
dependsOn
option to a helm release template.