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

Proposal: Provide a way to run "install --replace" the first time for certain releases #84

Closed
cmeury opened this issue Apr 4, 2018 · 19 comments · Fixed by #746
Closed

Comments

@cmeury
Copy link
Contributor

cmeury commented Apr 4, 2018

Usecase

I'm creating a small chart with out StorageClass configuration, but there's already one present with the same name, so I'd like helm to "take it under its wings". This can be done by running helm install --replace. However, this goes against the idempotent way of running helm upgrade --install that we do when we run the charts or sync action.

Proposal

Could we:

  • Mark releases to be "replaceable" so we check whether it's run for the first time and we do a install instead of an upgrade?
  • Also, provide a way to pass arguments to helm on a release level, so we could pass --replace directly in the state file (now I have to do it on the command-line and it's valid for all releases, so I have to use the label selector).
@mumoshu
Copy link
Collaborator

mumoshu commented Apr 6, 2018

@cmeury Thanks as always for your feedback!

I'm a bit hesitant to encourage helmfile users to do this because there may be chances that multiple helmfile.yaml tries to steal the ownership of the shared resource(the storage class in this specific case) from each other!

I'd rather add something like a "prerequisites" section in helmfile.yml so that helmfile could validate for existence of specific shared resources(again the storage class in this specific case) before a sync, if that helps.

@cmeury
Copy link
Contributor Author

cmeury commented Apr 6, 2018

And what about the second option of passing extra arguments for every release?

releases:
  - name: vault
    arguments:
      - "--replace"
    namespace: vault 
    labels:
      foo: bar
    chart: roboll/vault-secret-manager
    set:
      - name: address
        value: https://vault.example.com

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 6, 2018

@cmeury I think I had somewhat relevant discussion with @sstarcher in #60 (comment). Basically, I'd rather wanted helmfile to be declarative about release itself, rather than specific operations to manage the release.

In that regard, for example, shared: true(I do need a better name here!) which implies you need --replace would look slightly better to me.

However, I'm still nervous because then you may need to ensure all the vault releases seen across multiple helmfile's to have the exact same set of configs. Otherwise, the shared vault release would get updated and then rolled-back after each helmfile sync run.

@sstarcher
Copy link
Contributor

I'm confused by the underlying question of why you would want to do this. Could you please elaborate on why you have a existing chart release and you want to --replace it instead of upgrading it? This really seems like a migration that an admin needs to do.

Such as a admin would helm delete --purge and afterwards you would run the helmfile. A --replace is a one time operation and does not make sense to run on every helmfile sync.

@cmeury
Copy link
Contributor Author

cmeury commented Apr 7, 2018

@mumoshu The use case is not replace other releases, but "taking over" resources not managed by Helm. So shared is maybe the wrong term, but what about just using the same word as the helm argument? I know you're concerned about keeping the keyword pool small, but I can't help but suggest this; similar to "wait: true", but maybe grouped under a common "options" section:

releases:
  - name: vault
    options:
      replace: true
      wait: true
      ...
    namespace: vault 
    labels:
      foo: bar
    chart: roboll/vault-secret-manager

@sstarcher You're right, my last comment does not match my original proposal. This is a one-off operation when installing a chart for the first time. The use case is basically objects already existing on a cluster such as the kube-dns ConfigMap. Currently I solved my issue by adding a label to the relevant chart and then having this target in our Makefile:

charts-%:
	helmfile --file ./targets/${*}/helmfile.yaml --selector mode=force charts --args "--force"
	helmfile --file ./targets/${*}/helmfile.yaml --selector mode!=force charts

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 7, 2018

@cmeury Thx for the response. I believe that your primary purpose here is to take over existing resources.

However, my concern was more about that there seems like no way to enforce which helmfile can take over what. It wouldn't be problem if you're the only helmfile operator but I can't help worrying about an another operator's helmfile mistakenly take over the existing resource you originally wanted. Without such enforcement, the existing resource you had taken over would be conceptually "shared" hence the name I suggested.

But I guess I would start to feel ok if you really wanted to name it replace, especially after we have wait in helmfile.yml. To me, both seems like indication of leaking operational knowledge to helmfile.yml, but with enough documentation it wouldn't be so much trouble. Also, as both are more or less standard helm args, it would be friendlier for helm users!

@sstarcher
Copy link
Contributor

My issue with --replace is you would only want to run it once and you would not want any subsequent helmfile runs to contain it. Which is why I believe it should be a operational operation when a cluster is first initialized.

I have never actually used replace, but my understanding was it was for overwriting a release and not for getting ahold of existing manifests that were in the cluster?

@cmeury
Copy link
Contributor Author

cmeury commented Apr 9, 2018

Documentation about install --replace:

re-use the given name, even if that name is already used. This is unsafe in production
So, yeah, it is quite unusual and a one-time operation. My use case would be: I'd like to start minikube and "replace" certain resources such as the kube-dns config map with the helm chart, in an automated fashion for integration testing.

But I see your point about this being an operational concern and should not find a place in helmfile. So I'm fine with closing this as "won't fix".

@sstarcher
Copy link
Contributor

@cmeury does replace actually replace resources? I assumed from the documentation it would replace an existing release.

@cmeury
Copy link
Contributor Author

cmeury commented Apr 9, 2018

Charts Tips and Tricks:

The annotation "helm.sh/resource-policy": keep instructs Tiller to skip this resource during a helm delete operation. However, this resource becomes orphaned. Helm will no longer manage it in any way. This can lead to problems if using helm install --replace on a release that has already been deleted, but has kept resources.

Using Helm:

Because Helm keeps records of deleted releases, a release name cannot be re-used. (If you really need to re-use a release name, you can use the --replace flag, but it will simply re-use the existing release and replace its resources.)

Looking at the code in performUpdateForce() in release_update.go, it looks we're not failing if there is no previous release, but just forcing the release. But I just glanced through the code.

But you're making me questioning my whole approach or whether my assumption is not correct. I'll retest.

@sstarcher
Copy link
Contributor

sstarcher commented Apr 9, 2018

ya, the exact way I read that documentation is. If you have a chart that contains "helm.sh/resource-policy" and you run a helm install --replace the new install will fail as helm will not cleanup the resource marked with the resource-policy and that the new install will try to create that same resource and it will collide and fail.

My understanding from the documentation is --replace deletes a current release and installs it again. Not that it is replacing existing kubernetes objects that are installed.

Basically helm install --replace is like running helm delete --purge && helm install

I have never used --replace, but that's my understanding from the documentation.

@mumoshu
Copy link
Collaborator

mumoshu commented May 9, 2019

Commenting on an old issue - I think this can be handled in a way described in mumoshu/helm-x#7

@grebois
Copy link

grebois commented May 15, 2019

Would be great to have this.

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 17, 2019

@grebois @cmeury @sstarcher Are you still interested in seeing this in helmfile?

I finished the helm-x feature. In #673 we already added the initial helm-x integration.

So it should be a matter of enhancing our config syntax to accept adopt: [NS/KIND/RESOURCE_NAME] at the release level so that helmfile calls helm-x to transparently import existing resources at the installation time.

Example:

releases:
- name: kubedns
  chart: charts/kube-dns
  adopt:
  - deploy/kube-dns
  - configmap/kube-dns

@msutter
Copy link
Contributor

msutter commented Jul 8, 2019

Hi @mumoshu

I would love to be able to define 'adopt' in a release definition to patch existing resources.
Any plan when this would be implemented ?

By the way, thanks for your work. Great tool !

@mumoshu
Copy link
Collaborator

mumoshu commented Jul 8, 2019

No specific plan yet.

But it's already implemented in helm-x and it should be a matter of just adding the new configuration key to helmfile.yaml syntax. I'd appreciate it if you could try adding it!

@msutter
Copy link
Contributor

msutter commented Jul 8, 2019

I will try. I also tried to use helm-x with tillerless set to true. This does'nt work as helm-x don't know the tiller command.

@mumoshu
Copy link
Collaborator

mumoshu commented Jul 8, 2019

Thanks! That sounds like a good feature request to helm-x

@msutter
Copy link
Contributor

msutter commented Jul 8, 2019

No specific plan yet.

But it's already implemented in helm-x and it should be a matter of just adding the new configuration key to helmfile.yaml syntax. I'd appreciate it if you could try adding it!

@mumoshu PR Done -> #746

mumoshu pushed a commit that referenced this issue Jul 11, 2019
Use with the helm-x support(#673)

This enhances config syntax to accept adopt: [NS/KIND/RESOURCE_NAME] at the release level so that helmfile calls helm-x to transparently import existing resources at the installation time.

Resolves #84
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.

5 participants