helm icon indicating copy to clipboard operation
helm copied to clipboard

Pre-rollback hooks execution

Open eugenekainara opened this issue 5 years ago • 38 comments

Not really sure that it's a bug. But behavior of Helm is not very clear for me. I'm want to user pre-install(or pre-upgrade) hooks for database migration step that will create(or update) database schema. Also my plan was to use pre-rollback hook to rollback database schema changes to previous one. So If my release is broken for any reason - I'm just need to execute helm rollback <releaseName> 0 and helm will rollback database changes and redeployed my previous app version.

But for some reason when creating objects for pre-rollback hook Helm is using templates from successfully deployed release. And it confuses me a little.

This solution is not working for me due to strict versioning of every my image and just for e.g. If i'm deploying application v1.1.1 - I've had also migration scripts with v.1.1.1. Later when I'm updating to v1.1.2 - I'm also use scripts for v1.1.2 - and if release become failed - helm start to do rollback with migration scripts v1.1.1 - but this script doesn't include rollback instructions to go from v1.1.2 to v.1.1.1.

I'm using java and liquibase. Also I've create a small reproducing repo for better explain what's going wrong for me.

Please help me with some advice if I'm wrong with some configuration etc.

Output of helm version: Client: &version.Version{SemVer:"v2.14.0", GitCommit:"05811b84a3f93603dd6c2fcfe57944dfa7ab7fd0", GitTreeState:"clean"} Server: &version.Version{SemVer:"v2.14.0", GitCommit:"05811b84a3f93603dd6c2fcfe57944dfa7ab7fd0", GitTreeState:"clean"}

Output of kubectl version: Client Version: version.Info{Major:"1", Minor:"14", GitVersion:"v1.14.2", GitCommit:"66049e3b21efe110454d67df4fa62b08ea79a19b", GitTreeState:"clean", BuildDate:"2019-05-16T18:55:03Z", GoVersion:"go1.12.5", Compiler:"gc", Platform:"darwin/amd64"} Server Version: version.Info{Major:"1", Minor:"10", GitVersion:"v1.10.11", GitCommit:"637c7e288581ee40ab4ca210618a89a555b6e7e9", GitTreeState:"clean", BuildDate:"2018-11-26T14:25:46Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"linux/amd64"}

Cloud Provider/Platform (AKS, GKE, Minikube etc.): Kubernetes under Docker for Mac.

eugenekainara avatar May 31 '19 17:05 eugenekainara

Has there been any update on this issue?

I am seeing the following similar problem.

I use the reproducing repo listed above, but I have 2 versions: version1 only has the pre-install hook defined (no pre-rollback hook) version2 has both hooks defined.

If I helm install version1, then upgrade to version2, and then rollback to version1, I don't seen the pre-rollback hook being created. From the config, I expect that even though the pre-rollback job might not survive the rollback (since it did not exist in version1) I would expect the pod that ran the job would still be around so I could look at its logs.

If I do the opposite (version1 with just a pre-rollback hook, version2 with both hooks) and run the same steps, I see both hooks being created and run.

Am I missing something, or is this a bug in helm (ie a pre-rollback hook cannot be created by an upgrade)?

bwillusibmcom avatar Aug 01 '19 15:08 bwillusibmcom

I'm affected by the same issue (pre-rollback hook "taken" from the "target" version for the rollback). Is there any progress?

max-gotlib avatar Dec 30 '19 19:12 max-gotlib

I'm pretty sure this is the intended behaviour from Helm's architecture. helm rollback works the same as helm upgrade: on upgrade (rollback), it runs the hooks from the target release. Hooks from the "source" are ignored. helm rollback is designed identical to helm upgrade: it takes a target (a chart previously installed through helm install or helm upgrade) and updates the live resources to match the target

Here's helm upgrade's implementation:

https://github.com/helm/helm/blob/1ff8272748592dd05a87eb9b73e12b361898283f/pkg/action/upgrade.go#L235-L242

And here's helm rollback's implementation:

https://github.com/helm/helm/blob/1ff8272748592dd05a87eb9b73e12b361898283f/pkg/action/rollback.go#L153-L160

Suggestions/feedback welcome.

bacongobbler avatar Dec 30 '19 20:12 bacongobbler

@bacongobbler I understand the intention, but it looks a bit illogical:

  • there was a version transition: A -> B
  • now a B -> A rollback is being processed
  • changes, introduced with version B, are handled by A-version hook.

max-gotlib avatar Dec 31 '19 12:12 max-gotlib

in a similar, but slightly different situation.

  • my current deployed revision is 251, so my pre-install hook saw the .Release.Revision to be 251
  • updated to newer version, deployed revision is 252, my pre-install hook saw the .Release.Revision to be 252
  • now I manually triggered a rollback (using helm rollback command), to version 251. Helm created a new revision 253, but my pre-rollback (which is same as my pre-install hook), saw the .Release.Revision to be 251. I was expecting the .Release.Revision to be 253.

is there a way to get NEW revision when doing the rollback?

rajatjindal avatar Jan 09 '20 10:01 rajatjindal

@rajatjindal revision 251 should be identical to 253 in your case.

eugenekainara avatar Jan 09 '20 10:01 eugenekainara

I was hoping to use revision number to create new resource. Basically on every new deployment of helm release i create a new configmap, and then want to delete configmap when that particular revision is deleted in helm history

On Thu, Jan 9, 2020 at 4:10 PM Eugene Kainara [email protected] wrote:

@rajatjindal https://github.com/rajatjindal revision 251 should be identical to 253 in your case.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/helm/helm/issues/5825?email_source=notifications&email_token=AAEVN7CTIOFINDPDJNESQY3Q435LVA5CNFSM4HR4XNNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIP3FFA#issuecomment-572502676, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEVN7CFEMETHBUUCW2MTZTQ435LVANCNFSM4HR4XNNA .

-- Rajat Jindal | Pune | https://rajatjindal.com | [email protected]

rajatjindal avatar Jan 09 '20 10:01 rajatjindal

+1 I believe that what @max-gotlib mention makes sense. To keep it simple during rollback, can helm invoke pre and post hooks of both source and target release with source and target version information. This allows services to adjust their resources (e.g. data schema etc) required for older release to work..

itsrajivbansal avatar Apr 03 '20 09:04 itsrajivbansal

@itsrajivbansal In my case I had to use two stage upgrade procedure:

  1. upgrade existing chart with hooks turned off, where only hooks were actually upgraded
  2. run the "main" upgrade, having correct hooks. Got a lot of "supplemental damage", but in general the schema appears to be operational.

max-gotlib avatar Apr 03 '20 11:04 max-gotlib

+1 I think this is a feature request. I can't see helm changing which chart version the rollback hooks use, because that would be a breaking change.

For this use case, there needs to be a new hook. Something like pre-upgrade-rollback (sorry; I hate that name, too). And helm would need to make available the chart version and the appVersion from both the "source" chart and the "target" chart, so that whatever job ran could determine what actually needed to be rolled back.

kycrow32 avatar May 06 '20 18:05 kycrow32

How come it has to be a new hook? Couldn't that just be part of the pre-rollback and post-rollback hooks?

bacongobbler avatar May 06 '20 18:05 bacongobbler

@bacongobbler I suggested a new hook to avoid changing existing functionality in helm. max-gotlib implied (or maybe I just inferred) that a rollback hook be run from the source release, rather than the target release. itsrajivbansal suggested invoking pre-rollback hooks from both source and target releases. Both suggestions would result in changing behavior for existing charts.

A new hook would not change existing behavior for existing charts.

To clarify what I'm suggesting a little bit, and building off of max-gotlib's example: there was a version transition: A -> B now a B -> A rollback is being processed:

  1. B pre-upgrade-rollback hooks are processed
  2. A pre-rollback hooks are processed
  3. A post-rollback hooks are processed

EDIT: I had originally transposed A and B in the numbered steps

kycrow32 avatar May 18 '20 16:05 kycrow32

I have also run into this issue using alembic. My use case relates to upgrading/downgrading my database schemas and I expected the pre-rollback hook to use the current image rather than target image. I have a post-upgrade hook that upgrades the database schema. Once the helm upgrade completes, some tests are run on the current infrastructure and if any of those fail, a rollback will occur.

Ideally this pre-rollback job will be able to use the current image when the downgrade command is issued as the alembic revision file that contains the downgrade SQL command(s) must be present in the image. What are thoughts around another annotation that lets the user choose from where (current vs. target) the hooks should be run from like helm.sh/hook-source: current? The default functionality would be helm.sh/hook-source: target if that annotation is not present in order to prevent breaking existing charts.

Ulminator avatar Jul 14 '20 19:07 Ulminator

Despite the hooks being similarly designed to be tied to their respective Helm commands, it is counterintuitive that a rollback defaults to using the target release instead of the source release. Would love to see support for being able to override this as it would greatly simplify our needs to automate downgrading of databases. Otherwise, we seem to have to jump through hoops to get the functionality we need.

Related issues that previously got closed:

  • https://github.com/helm/helm/issues/4395
  • https://github.com/helm/helm/issues/3549

defrank avatar Jul 14 '20 23:07 defrank

👍 I have another use case for using pre-rollback from source release - during the upgrade I change the docker image version, which uses different (fixed) permissions, since previous version used different generated perms for each release (this is used as a sub-chart in multiple places). Since I am connecting to a stateful set PVC (need to preserve data), during upgrade I'm normalizing permissions, but need to write down the previous ones as backup and if new version fails, before the rollback need to bring back the old permissions. Since there is no hook in old release for setting the perms.. you know the drill :)

I really like @Ulminator proposal for the helm.sh/hook-source annotation.

Is this being even remotely considered to add this kind of functionality whatsoever?

Ziwi01 avatar Sep 08 '20 07:09 Ziwi01

I also agree it is essential feature. Without "source -> target" executing progress, "hook" system cannot be beautiful.

Is there any updates or plan for it ?

chancejin avatar Sep 10 '20 11:09 chancejin

We're looking at leveraging pre/post hooks for db migrations. It's possible without this, but its not very intuitive. I ended up spinning up a k8s cluster, and playing around with rollback hooks to see what was happening. Can we add documentation?

Addtionally, with the above comments its clear there is a need for this feature. Any thougths on this? Can we get a comment on whether this is a desirable change, but just blocked by priorities? ie: would you welcome PR's for this?

steeling avatar Sep 29 '20 02:09 steeling

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

github-actions[bot] avatar Dec 29 '20 00:12 github-actions[bot]

bump

kycrow32 avatar Jan 08 '21 14:01 kycrow32

This is still a very needed change - affecting some teams I am working with too.

VoidShaper avatar Mar 17 '21 15:03 VoidShaper

+1

jackhodkinson avatar May 02 '21 06:05 jackhodkinson

Agree that the current behavior is counter-intuitive at best. I think the helm.sh/hook-source annotation would be sufficient for my needs and those listed in this ticket, and would be backwards compatible.

rkleinman-hpe avatar May 03 '21 19:05 rkleinman-hpe

The only problem I can see with a simple helm.sh/hook-source is that it will be applied to all hooks. Maybe you'd like some hooks to run as source and some as target.

VoidShaper avatar May 04 '21 12:05 VoidShaper

I've been experiencing the exact same issue as @Ulminator. I, too, am using alembic to perform database migrations and using helm hooks. Let's say that I have a current release using image_1 and the alembic_version in my database is 0000. Now, I do a helm upgrade ... with a new image_2, where my new migration file with a revision ID of 0001 is present. The database now has alembic_version of 0001. If, upon upgrade, I find that there are some breaking changes, I will now want to perform a helm rollback where a pre-rollback job get's triggered. This pre-rollback job uses image_1 but my latest migrations files are actually present in image_2. This results in the pre-rollback job failing saying that it couldn't validate the identity 0001 because image_1 has only a file with revision ID 0000. I'm sorry if this is a little confusing.

It would be nice to let the pre-rollback hook use image_2 in such a scenario.

ishaan-rd avatar May 10 '21 06:05 ishaan-rd

Running the pre-rollback hook of the target helm release is not just counter-intuitive, it is downright incorrect. Seriously, it is a rollback from B (source) to A (target), how the heck could A know anything about what B did? B is the future, A cannot be expected to execute the "pre-rollback" hook. The only way for A hooks to know how to undo B migrations is to have saved all the actions taken by all the intermediate releases going from A to B (including those of B), and for helm to pass some data to A hooks so they know which portion of that data to use.

The target, A, can of course run the "post-rollback" hook. That makes sense. Then again, some data, like what was the previous release, may be required.

The desire for uniformity has caused a serious design flaw in the use of rollback hooks: not all hooks can be run on target. Some have to be run on source.

schollii avatar May 28 '21 20:05 schollii

I agree with the sentiments above. Can anyone provide a use case in which the current implementation (ie. past release handling the rollback from a future release) would be desired or even useful?

kovacsur avatar Jun 14 '21 10:06 kovacsur

+1

jhautin avatar Sep 08 '21 07:09 jhautin

+1 here as well. We're using alembic as well and currently there's no intuitive way to perform down migrations.

davidwneary avatar Sep 09 '21 11:09 davidwneary

+1

hobbytp avatar Sep 25 '21 06:09 hobbytp

+1

randreev1321 avatar Oct 19 '21 07:10 randreev1321