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

feat: "dependsOn" for release dependencies #715

Closed
ilyasotkov opened this issue Jun 21, 2019 · 39 comments · Fixed by #914
Closed

feat: "dependsOn" for release dependencies #715

ilyasotkov opened this issue Jun 21, 2019 · 39 comments · Fixed by #914

Comments

@ilyasotkov
Copy link

I know Helmfile can already do release dependencies by ordering releases manually via naming conventions for helmfiles or limiting concurrency.

Ability to explicitly declare dependencies would be a major step towards making Helmfile more declarative.

My initial idea was to base it on release names like so:

releases:

- name: prometheus
  dependsOn: ["nginx-ingress"]
  chart: stable/prometheus
  values: []

- name: nginx-ingress
  chart: stable/nginx-ingress
  values: []

It would be even nicer to base it on release labels:

releases:

- name: prometheus
  dependsOn:
    group: core
  namespace: prometheus
  chart: stable/prometheus
  values: []

- name: nginx-ingress
  labels:
    group: core
  chart: stable/nginx-ingress
  values: []
@ilyasotkov ilyasotkov changed the title feat: "dependsOn" for releases dependencies feat: "dependsOn" for release dependencies Jun 21, 2019
@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

@ilyasotkov Thx!

Just curious but how do you like helmfile to process dependsOn across helmfiles? Are you ok if it works only within each helmfile?

@ilyasotkov
Copy link
Author

@mumoshu It would be ideal if Helmfile would merge everything into a single Helmfile and then resolve dependencies.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

Interesting idea! But doesn't it defeat the purpose of helmfiles: where each sub-helmfile can be run independently? Are you referring to bases: then?

@ilyasotkov
Copy link
Author

Somewhat similar to bases: but it would also merge the list of releases: across helmfiles and then run all in parallel if there's no dependsOn but resolve dependencies between releases if there is.

The feature is definitely a major one and I don't know if implementing it in a backwards-compatible manner is even possible.

When running helmfile -l label=value sync the default behavior could be to only resolve dependecies that match label=value, or resolve all dependencies regardless of label if we add a flag like --resolve-all-deps.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

Thanks! Much appreciated.

all dependencies regardless of label if we add a flag like --resolve-all-deps.

Just curious but why and when you need this?

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

Merging releases should be possible. Actually we have a dedicated issue for that #684

@ilyasotkov
Copy link
Author

But doesn't it defeat the purpose of helmfiles: where each sub-helmfile can be run independently?

It could still work quite cleanly when running independent helmfiles:

# helmfile1.yaml
releases:
- name: my-release
  dependsOn:
    group: core
  chart: ./charts/app
# helmfile2.yaml
releases:
- name: my-core-app
  labels:
    group: core
  chart: ./charts/app

If we run only helmfile1.yaml, we could give a warning that no dependencies were found matching group=core label.

@ilyasotkov
Copy link
Author

Just curious but why and when you need this?

The idea comes from Terragrunt's apply-all when run on a sub-directory of live.

It could be useful if you have a massive helmfile.d and want to deploy only a single release that depends on many other releases but you don't want to think about dependencies (which could be a long chain).

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

@ilyasotkov Interesting! So you leave users the responsibility to label releases consistently across all the involved helmfiles?

In case the user had mistakenly added the same label to two different releases in separate helmfiles, maybe they have no easy way to notice that?

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

It could be useful if you have a massive helmfile.d and want to deploy only a single release that depends on many other releases but you don't want to think about dependencies (which could be a long chain).

Still trying to understand - How is it different from running helmfile -f helmfile.d/ without selectors?

@ilyasotkov
Copy link
Author

ilyasotkov commented Jun 21, 2019

Still trying to understand - How is it different from running helmfile -f helmfile.d/ without selectors?

If we have a massive helmfile.d where one file would be:

releases:
- name: my-app
  labels:
     app: my-app
  dependsOn:
    app: ["storage", "ingress"]
  chart: ./charts/app
  • Running helmfile -f helmfile.d/ would deploy 1000 releases.
  • Running helmfile -l app=my-app would deploy only that application and would warn about unresolved dependencies for labels app: ["storage", "ingress"].
  • Running helmfile -l app=my-app --resolve-all-deps would deploy all releases matching app=ingress and app=storage, then deploy app=my-app.

Edit: in the last case it should actually resolve all dependencies declared for releases matching app=ingress and app=storage as well.

@ilyasotkov
Copy link
Author

So you leave users the responsibility to label releases consistently across all the involved helmfiles?

I think that if a user is syncing a collection of helmfiles, it's totally the user's responsibility to make sure he knows what that collection contains. That includes knowing how each release in the collection is labelled.

@ilyasotkov
Copy link
Author

@mumoshu Regarding possible implementation, this type of functionality would almost certainly require building a DAG and topologically sorting the list of releases to deploy. Circular dependencies would lead to critical failure.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

  • Running helmfile -l app=my-app would deploy only that application and would warn about unresolved dependencies for labels app: ["storage", "ingress"].
  • Running helmfile -l app=my-app --resolve-all-deps would deploy all releases matching app=ingress and app=storage, then deploy app=my-app.

I see!

I'd prefer helmfile -l app=myapp "fails" due to unresolved dependencies, while helmfile -l app=myapp --resolve-all-deps and helmfile -l app=myapp -l app=ingress -l app=storage succeeds.

But I agree with the necessity of --resolve-all-deps.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

I think that if a user is syncing a collection of helmfiles, it's totally the user's responsibility to make sure he knows what that collection contains. That includes knowing how each release in the collection is labelled.

I imagine I would get a lot of support tickets from users due to that :) But I have no better idea.

@ilyasotkov
Copy link
Author

ilyasotkov commented Jun 21, 2019

I'd prefer helmfile -l app=myapp "fails" due to unresolved dependencies

I meant that Helmfile would give a warning that it didn't try to resolve dependencies (because no --resolve-all-deps was specified) and the release would probably fail but could also succeed if the dependencies were deployed earlier.

Edit: Though Helmfile would already "know" whether the dependencies were deployed earlier (via helm diff) and could use that information to fail early. Edit 2: That's for helmfile apply and not helmfile sync though.

@ilyasotkov
Copy link
Author

In case the user had mistakenly added the same label to two different releases in separate helmfiles
I imagine I would get a lot of support tickets from users due to that :) But I have no better idea.

Not sure I understand, what would be the harm (besides the surprise factor)? If a user accidentally adds a label to a random application, then adds a dependsOn for that label to some other release declaration, then both the intended dependency and the random application would get deployed. It's not such a bad a thing unless the random application includes some other unintended changes.

And it's not all that different from accidentally adding a label in the current version of Helmfile and wondering why 2 applications were deployed instead of 1 :)

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

And it's not all that different from accidentally adding a label in the current version of Helmfile and wondering why 2 applications were deployed instead of 1 :)

Good point. True 😄

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

I meant that Helmfile would give a warning that it didn't try to resolve dependencies

If we were to allow that behavior, I'd prefer requiring an explicit flag for it like --allow-unresolved-deps. So in overall we may have:

  • Running helmfile -l app=my-app would fail due to unresolved dependencies for labels app: ["storage", "ingress"].
  • Running helmfile -l app=my-app --allow-unresolved-deps would warn about unresolved dependencies for labels app: ["storage", "ingress"].
  • Running helmfile -l app=my-app --resolve-all-deps would deploy all releases matching app=ingress and app=storage, then deploy app=my-app.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

@mumoshu Regarding possible implementation, this type of functionality would almost certainly require building a DAG and topologically sorting the list of releases to deploy. Circular dependencies would lead to critical failure.

@ilyasotkov Noted. Thanks!

I am to work on this after #688 and #684 because dependsOn would work with bases, not helmfiles. I'd appreciate it if you could also comment on those issues, too.

I'll give you an example usage of this feature based on what we've discussed so far.
Any feedback is welcomed 😃

Let's start by creating your helmfile.yaml:

bases:
- path: logging.yaml
   inheritValues: true
- path: telemetry.yaml
  inheritValues: true
- path: servicemesh.yaml
  inheritValues: true

releases:
- name: mymainapp
  dependsOn:
  - servicemesh

logging.yaml:

releases:
- name: filebeat
   labels:
    group: logging
    env: prod
  values:
  - filebeat.prod.values.yaml
- name: filebeat
   labels:
    group: logging
    env: preview

telemetry.yaml:

releases:
- name: prometheus
  labels:
    group: telemetry
    env: prod
- name: prometheus
  labels:
    group: telemetry
    env: preview

servicemesh.yaml:

releases:
- name: istio
  labels:
    group: servicemesh
    env: prod
  dependsOn:
  - telemetry
- name: istio
  labels:
    group: servicemesh
    env: preview
  dependsOn:
  - telemetry

Now let's run helmfile:

  • helmfile env=preview would deploy everything including mymainapp, istio, prometheus, filebeat whose env is preview.
  • helmfile -l name=mymainapp,env=preview would fail due to missing dependency(istio
  • helmfile -l name=mymainapp,env=preview --allow-unresolved-deps would install mymainapp while warning about missing dependency(istio
  • helmfile -l name=myapp,env=preview --resolve-all-deps would deploy mymainapp, its dependency istio and the transitive dependency prometheus.
    • How do we filter istio and prometheus? Ideally, onlyenv=preview in -l name=myapp,env=preview should be used for filtering istio and prometheus. But it seems a bit confusing.

This isn't how the selector works today, but I'm considering to extend -l to be a OR operation when two or more of them are provided. Then:

  • helmfile -l name=myapp,env=preview -l servicemesh,env=preview -l telemetry,env=preview would deploy mymainapp, istio, and prometheus for env=preview without errors. Here you resolved all the deps manually.

Alternatively you can just put the same label to all the releases involved in a deployment scenario so that:

  • helmfile -o deploy-myapp-and-deps=true would deploy myapp istio telemetry assuming these 3 releases has deploy-myapp-and-deps=true.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 21, 2019

@ilyasotkov Please see above ⬆️ I'm not sure how --resolve-all-deps should work when you've provided two or more kvs in a selector.

@ilyasotkov
Copy link
Author

I'd prefer helmfile -l app=myapp "fails" due to unresolved dependencies, while helmfile -l app=myapp --resolve-all-deps and helmfile -l app=myapp -l app=ingress -l app=storage succeeds.

I think that's a good alternative 👍

Let's start by creating your helmfile.yaml

In your example dependsOn: is a list of strings. Is your suggestion to refer to bases, not labels (with .yaml suffix removed)? Or are you implicitly referring to group labels?

helmfile env=preview mwould deploy everything including mymainapp, istio, prometheus, filebeat whose env is preview.

How so? 🤔 In your example mymainapp is doesn't have env=preview label.

This isn't how the selector works today, but I'm considering to extend -l to be a OR operation when two or more of them are provided. helmfile -l name=myapp,env=preview -l servicemesh,env=preview -l telemetry,env=preview would deploy mymainapp, istio, and prometheus for env=preview without errors. Here you resolved all the deps manually.

This part is super confusing to me 😕 As a new user I'd assume

  • -l key=value -l key2=value2 and
  • l key=value,key2=value2

are the same thing.

helmfile -o deploy-myapp-and-deps=true would deploy myapp istio telemetry assuming these 3 releases has deploy-myapp-and-deps=true.

That part makes sense.

@ilyasotkov
Copy link
Author

ilyasotkov commented Jun 22, 2019

@mumoshu Maybe it's best to keep it simple and have dependsOn as a list of references to Helm releases (not labels). In Helm 3 releases are namespaced, so namespace should be part of the reference to a Helm release.

releases:

- name: myapp
  dependsOn:
  - release: prometheus
    namespace: telemetry

- name: prometheus
  namespace: telemetry  

dependsOn would be allowed

  • within the same helmfile OR
  • in one of the bases

Implementation details:

  1. Parse all available releases: (after merging in all of the bases:). Each Helm release is a DAG vertex/node.
  2. Parse dependsOn for all available releases, validate that they can be found from the available releases. If not found, only proceed if --allow-unresolved-deps flag is specified.
  3. Add each valid dependsOn item as a directed edge in the DAG.
  4. Topologically sort the DAG, end up with a two-dimensional array.
  5. Loop over the resulting array, deploying all items of the inner arrays (Helm releases) in parallel.

@ilyasotkov
Copy link
Author

ilyasotkov commented Jun 22, 2019

That way, it would be

  1. Easier for users to understand how dependencies are meant to work.
  2. Easier to implement: if a user specifies a --selector, dependencies not matching the selector are ignored, unless --resolve-all-deps flag is specified.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 23, 2019

@ilyasotkov Thanks for the suggestion! It looks feasible. But on the other hand it blurs the whole purpose of this feature to me.

If it works only within a single helmfile, why you need this?

Can't you just split your helmfile to two or more and order them under helmfiles:, cuz helmfiles: is a declarative way to order groups of helmfiles.

Or is it just that you want a simpler(or a more declarative - you feel so?) way than that?

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 23, 2019

have dependsOn as a list of references to Helm releases (not labels). In Helm 3 releases are namespaced, so namespace should be part of the reference to a Helm release.

That would be simpler to start. Sounds good.

Also, I love the config syntax you proposed as it is concise while being extendable to support labels or other things in the future 😄

dependsOn would be allowed

  • within the same helmfile OR

OK. Each helmfile is isolated so that would be natural.

  • in one of the bases

This one is hard. bases is for including other helmfiles(as modules) before processing releases in the caller helmfile as you'd also taken into account in:

  1. Parse all available releases: (after merging in all of the bases:). Each Helm release is a DAG vertex/node.

all available releases naturally contain all the releases loaded from bases, which affects dependsOn. Are you ok with that?

A possible upside of it would be that you can extract a dependency into a helmfile module and switch implementation by using go template in bases. Downside? No idea yet.

And,

Implementation details:

It looks great! Thanks for all the suggestions and proposals again.

@ilyasotkov
Copy link
Author

ilyasotkov commented Jun 23, 2019

If it works only within a single helmfile, why you need this?
Can't you just split your helmfile to two or more and order them under helmfiles:, cuz helmfiles: is a declarative way to order groups of helmfiles.
Or is it just that you want a simpler(or a more declarative - you feel so?) way than that?

I really like purely declarative tools (like Terraform and CloudFormation) where the order of declaration doesn't matter. It's a bit more pleasant to reason about (arguably, of course).

all available releases naturally contain all the releases loaded from bases, which affects dependsOn. Are you ok with that?

That's exactly my proposal.

@ilyasotkov
Copy link
Author

ilyasotkov commented Jun 23, 2019

cuz helmfiles: is a declarative way to order groups of helmfiles.

With helmfiles: order of items is vitally important, so in my view it's not really declarative. If we implemented dependsOn with bases: instead of helmfiles:, order of bases doesn't matter at all as long as you declare all necessary dependencies.

Instead of thinking:

I'm adding a new release myapp, it must be deployed after which releases? 🤔 (Is myotherapp, a dependency, declared in helmfile1.yaml or helmfile3.yaml or helmfile5.yaml, I forgot?)

I prefer this:

I'm adding a new release myapp, I know it depends on myotherapp, so I just add myotherapp to dependsOn.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 23, 2019

I really like purely declarative tools (like Terraform and CloudFormation) where the order of declaration doesn't matter. It's a bit more pleasant to reason about (arguably, of course).

Understood 👍

That's exactly my proposal.

Okay. Thanks for confirming 👍

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 23, 2019

In your example dependsOn: is a list of strings. Is your suggestion to refer to bases, not labels (with .yaml suffix removed)? Or are you implicitly referring to group labels?

Ah nope, my mistake. I meant e.g. name=prometheus as helmfile has a convention that automatically put name=<RELEASE NAME> label to every release.

How so? 🤔 In your example mymainapp is doesn't have env=preview label.

Good catch! My mistake. I should have created two mymainapp releases with varying env labels.

This part is super confusing to me 😕 As a new user I'd assume

Okay then let's omit this part :) FWIW, it was also based on my frustration that kubectl -l foo=bar -l foo=baz doesn't work cuz the latter silently overrides the former.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 25, 2019

In Helm 3 releases are namespaced, so namespace should be part of the reference to a Helm release.

releases:

- name: myapp
  dependsOn:
  - release: prometheus
    namespace: telemetry

- name: prometheus
  namespace: telemetry  

Sounds correct! I believe it should be tillerNamespace for helm v2 though, as helm2 releases are isolated per tiller ns.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 25, 2019

...And the gotcha is that even helm v2 allows isolating releases per tiller namespace, helmfile as of today doesn't allow having two or more releases with the same name across different tiller namespaces.

So regarding this feature, we'd better start without namespace or tillerNamespace, that is:

releases:

- name: myapp
  dependsOn:
  - release: prometheus

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 25, 2019

So here's the plan for the first version of this feature based on @ilyasotkov's awesome proposal above:

Goal

Make helmfile even more declarative regarding release dependencies

Enhancement to the config syntax

The configuration syntax is enhanced to accept dependsOn per each release under releases. Each dependsOn item receives solely release.name.

releases:
- name: myapp
  dependsOn:
  - release:
      name: prometheus
- name: prometheus
  namespace: telemetry  

From within dependsOn you can refer to releases defined in the same helmfile and its bases only. You can't refer to other releases defined in sub-helmfiles, as each sub-helmfile is isolated by design.

Helm v2 release is isolated per tiller namespace. Helm v3 release is isolated per namespace. But this syntax, for its first version, doesn't take that into account - helmfile as of today doesn't support two or more releases with the same name regardless of the tiller namespace and it remains so. Please submit an another feature request to enhance helmfile to accept two or more releases per (tiller) ns, which should also address this.

Implementation details

  1. helmfile parses all available releases: (after merging in all of the bases:). Each Helm release is a DAG vertex/node.
  2. It then parses dependsOn for all available releases, validate that they can be found from the available releases. If not found, only proceed if --allow-unresolved-deps flag is specified.
  3. Add each valid dependsOn item as a directed edge in the DAG.
  4. Topologically sort the DAG, end up with a two-dimensional array.
  5. Loop over the resulting array, deploying all items of the inner arrays (Helm releases) in parallel.

Future plans

  • Add --resolve-all-deps that is used like -l labelkey=val --resolve-all-deps, so that you can deploy releases whose labels contain labelkey=val, along with its dependencies.

@ilyasotkov
Copy link
Author

The configuration syntax is enhanced to accept dependsOn per each release under releases. Each dependsOn item receives solely release.name.

I like that enhancement as it makes things a bit more clear and a lot more extensible 👍

So here's the plan for the first version of this feature based on @ilyasotkov's awesome proposal above

Great summary of the proposal, thank you @mumoshu for your amazing work and exceptional communication ❤️

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 27, 2019

A possibly unavoidable limitation to this feature is that helmfile diff and helmfile apply would still fail due to missing CRDs in a case like #538 (comment)

Are you okay with that? I'd appreciate it if you could share your insights to resolve it fully. I'm thinking that we'd need:

helmfiles:
- crds.yaml

releases:
- name: something-depends-on-crd-to-exist-in-the-cluster-before-diffing

Instead of the below, which would certainly fail while helm diffing due to that at the time of running it there's no crds installed:

releases:
- name: crds
- name: something-depends-on-crd-to-exist-in-the-cluster-before-diffing
  dependsOn:
    release:
      name: crds

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 27, 2019

Missed to mention you @ilyasotkov :)

@ilyasotkov
Copy link
Author

@mumoshu I'm not currently a user of those features (apply and diff) so I am not able to offer any real insight until I try them out and understand how they work :)

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 20, 2019

So here's the DAG implementation I'll be integrating into Helmfile.
https://github.com/mumoshu/dag

@mumoshu mumoshu mentioned this issue Sep 1, 2019
mumoshu added a commit that referenced this issue Oct 28, 2019
…der declaratively

Introduces DAG-aware installation/deletion ordering to Helmfile.

`needs` controls the order of the installation/deletion of the release:

```yaml
relesaes:
- name: somerelease
  needs:
  - [TILLER_NAMESPACE/][NAMESPACE/]anotherelease
```

All the releases listed under `needs` are installed before(or deleted after) the release itself.

For the following example, `helmfile [sync|apply]` installs releases in this order:

1. logging
2. servicemesh
3. myapp1 and myapp2

```yaml
  - name: myapp1
    chart: charts/myapp
    needs:
    - servicemesh
    - logging
  - name: myapp2
    chart: charts/myapp
    needs:
    - servicemesh
    - logging
  - name: servicemesh
    chart: charts/istio
    needs:
    - logging
  - name: logging
    chart: charts/fluentd
```

Note that all the releases in a same group is installed concurrently. That is, myapp1 and myapp2 are installed concurrently.

On `helmdile [delete|destroy]`, deleations happen in the reverse order.

That is, `myapp1` and `myapp2` are deleted first, then `servicemesh`, and finally `logging`.

Resolves #715
@mumoshu
Copy link
Collaborator

mumoshu commented Oct 28, 2019

The pull request for this is live at #914.

A few notes:

  • I've chosen needs instead of dependencies to avoid ambiguity between this, release's dependencies, and chart's dependencies
  • needs is just an array of IDs of releases. Each ID is formatted [TILLER_NS/][NS/]NAME where [TILLER_NS/] and [NS/] are optional and can be omitted. Also, it can't be specified when the target release has no tillerNamespace and/or namespace declared. This turned out easier to read/write and flexible enough to support expected use-cases the latter idea considered in feat: "dependsOn" for release dependencies #715 (comment)

mumoshu added a commit that referenced this issue Oct 28, 2019
…der declaratively (#914)

Introduces DAG-aware installation/deletion ordering to Helmfile.

`needs` controls the order of the installation/deletion of the release:

```yaml
relesaes:
- name: somerelease
  needs:
  - [TILLER_NAMESPACE/][NAMESPACE/]anotherelease
```

All the releases listed under `needs` are installed before(or deleted after) the release itself.

For the following example, `helmfile [sync|apply]` installs releases in this order:

1. logging
2. servicemesh
3. myapp1 and myapp2

```yaml
  - name: myapp1
    chart: charts/myapp
    needs:
    - servicemesh
    - logging
  - name: myapp2
    chart: charts/myapp
    needs:
    - servicemesh
    - logging
  - name: servicemesh
    chart: charts/istio
    needs:
    - logging
  - name: logging
    chart: charts/fluentd
```

Note that all the releases in a same group is installed concurrently. That is, myapp1 and myapp2 are installed concurrently.

On `helmdile [delete|destroy]`, deleations happen in the reverse order.

That is, `myapp1` and `myapp2` are deleted first, then `servicemesh`, and finally `logging`.

Resolves #715
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants