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

Implement before-hook-creation delete policy #3540

Merged
merged 2 commits into from Apr 3, 2018

Conversation

Projects
None yet
9 participants
@alexey-igrychev
Copy link
Contributor

alexey-igrychev commented Feb 20, 2018

Existing helm.sh/hook-delete-policy annotation policies (hook-failed, hook-succeeded) do not allow to leave failed jobs for debugging without blocking the next job launching: every failed job must be deleted manually before the next related release is launching (installing, updating or rolling back).

New policy, hook-recreate, removes the hook from previous release if there is one before the new hook is launched and can be used with another policies.

@jascott1

This comment has been minimized.

Copy link
Collaborator

jascott1 commented Feb 27, 2018

Hi @alexey-igrychev

Thanks for the PR. I think its looking good and might be useful to a number of people. Since you didn't reference a proposal for this feature I will ask a couple other maintainers to weigh in. @bacongobbler @thomastaylor312

@thomastaylor312 thomastaylor312 self-requested a review Mar 1, 2018

@bacongobbler

This comment has been minimized.

Copy link
Member

bacongobbler commented Mar 5, 2018

I think it looks great. Only thing I'd ask for is

  1. docs: people need to know how to use this, when to apply the hook-recreate annotation, and when it is not useful. Bonus points if there's an example/tutorial.
  2. unit tests to cover this hook, if possible

Thanks!

@thomastaylor312

This comment has been minimized.

Copy link
Collaborator

thomastaylor312 commented Mar 8, 2018

I agree with @bacongobbler on this. It is looking good but I think a unit test and docs will be needed before merging. I'll give this another review once those are there. Thanks for adding this!

@Lentil1016

This comment has been minimized.

Copy link

Lentil1016 commented Mar 20, 2018

Hi @alexey-igrychev

I have an embryo thinking on a Create Policy thing, and thinking that the Recreate Delete Policy you implemented is more likely to be a Recreate Create Policy, against Reuse Create Policy or sth like that.

In some specified cases, people might want to reuse the old hook object not deleted last time, like a NodePort service with a random Port that not supposed to be redefined unless we delete it manually.

If so then user will have "helm.sh/hook-create-policy":

  • create: default policy, try create a new one, if exist, return failed.
  • recreate: delete the old one and create new, otherwise just create a new one.
  • reuse: if old one exist, just simply reuse it, otherwise create a new one.

I'm a golang beginner and this is honestly an embryo idea that suits my need. I'm starting to read the source code and hopefully I can work on it later.

Is that make sense? Is that looks good to you? I think I need some suggestions on this.

@distorhead

This comment has been minimized.

Copy link
Contributor

distorhead commented Mar 20, 2018

@Lentil1016
Good idea! We were not sure about naming too. What about create-once instead of reuse? Not sure what is being "reused" when the hook-job for example already exists, it is just an already existing job that being ignored.
From the tiller standpoint reuse may have sence, though reuse concept does not apply to hooks as far as I know, the state of the hook-job is not recorded into release. But for the user who writes hook-job-template, reuse may seem confusing.

@distorhead

This comment has been minimized.

Copy link
Contributor

distorhead commented Mar 20, 2018

By the way, the current hook-delete-policy maps ACTION => HOOK_STATE. For example hook-delete-policy: hook-succeeded means action=delete on hook-state=hook-succeeded.

But for some actions it is more convinient to describe policy in reverse: HOOK_STATE => ACTION.

hook-exists-policy: fail|recreate|reuse

VS

hook-recreate-policy: hook-exists
hook-reuse-policy: hook-exists
@diafour

This comment has been minimized.

Copy link

diafour commented Mar 20, 2018

Further collaboration reveals more things:

  1. We came up with 4 create situations:
  • No previous resource, create hook (this shouldn't be ignored with annotations)
  • Previous resource exists, create hook (default for now)
  • Previous resource exists, delete it, create hook (recreate)
  • Previous resource exists, leave it, do not try to create new hook (reuse)

So behaviour is different only if previous resource exists.

  1. hook-delete-policy name has invertion of cause and consequence: hook-<consequent action>-policy: <cause>
    For consistency with hook-delete-policy names and values for new annotations should be like this:
  • hook-recreate-policy: hook-exists delete previous resource if it exists. Default is never recreate.
  • hook-reuse-policy: hook-exists create only if previous resource not exists. Default is create always.

This variant should causes error if two annotations are used.

  1. recreate policy that implemented in this PR does not check for previous hook. It just deletes the resource "blindly". That is why it is implemented as delete policy. For implementing something like reuse we need to implement that checking for existing resource.

  2. After this check will be implemented we can detect a new state of the hook: exists. And as @distorhead said it will be more clarifying if annotation have form of hook-<hook state>-policy: <consequent action>:

hook-exists-policy: delete | keep | fail
  1. fail is a new policy — if resource exists, fail early without "blindly" call to kubeCli.Create.
@Lentil1016

This comment has been minimized.

Copy link

Lentil1016 commented Mar 21, 2018

Cool, keep or create-once is both better than reuse. Let's just forget about reuse.

I think hook-exists-policy is a cooler idea, though is a little bit ugly, for its form is not HOOK_STATE => ACTION like hook-recreate-policy or hook-keep-policy dose. But it's logically complete without any spot.

As a rude hungry user, like I did yesterday, I can hardly realize the gentle design behind these annotations. When I learn hook-recreate-policy and hook-keep-policy, I could probably start grumbling about the "two keys have the same single value, and conflict with each other" thing. And "a single key have three optional values" looks more normal and simpler.

In summary:

hook-exists-policy: delete | keep | fail

Do check before any installation, policy enabled only if specified previous exists hook object detected.

  • delete: delete the previous resource.
  • keep: leave the previous resource it is, and will not create a new one.
  • fail: stop installation, return error with the specified hook object name.

Temporarily chose fail as the default policy.

@distorhead, @diafour Thanks for your advice, it's very inspiring. If there is no more advice I can ask for for now, I will start working on it ASAP. Any further considerations, we can do it after I've actually done something and created a PR. Is that OK?

@distorhead

This comment has been minimized.

Copy link
Contributor

distorhead commented Mar 21, 2018

We were not sure about naming too.

@Lentil1016 Sorry for confusing, I said "we" meaning my team who working on this PR (@alexey-igrychev @diafour and me), we are just trying to contribute to helm some experience from our project https://github.com/flant/dapp. I do not know what helm maintainers think about this policy renaming.

The main goal of this PR was to introduce especially recreate policy, because current hook-delete-policy is not convinient to use in real practice as noticed @alexey-igrychev in the PR message.

So we can take your notice about confusing hook-delete-policy with recreate and move this recreate to another policy in this PR, and @alexey-igrychev is ready to make corrections.

I think we are gonna implement this variant hook-exists-policy: recreate today.

  • By default, when no hook-exists-policy is specified, helm will fail from kubeCli.Create call just as it is now, without any incompatibility.
  • When hook-exists-policy is specified and the value is recreate, helm will delete already existing hook.

What about keep/reuse/create-once -- you can implement it on top of this PR then, if it will be accepted, because if I get you right, you have your own needs for something like reuse.

@Lentil1016

This comment has been minimized.

Copy link

Lentil1016 commented Mar 21, 2018

Oh, sorry for that misunderstanding. I think this PR is cool and worthwhile. Thank you and wish you best of luck.

@alexey-igrychev alexey-igrychev changed the title Implement hook-recreate delete policy WIP: Implement hook-recreate policy Mar 21, 2018

@distorhead

This comment has been minimized.

Copy link
Contributor

distorhead commented Mar 21, 2018

@Lentil1016 After another look at the situation we have decided just to rename hook-recreate into before-hook-creation and keep this function under already existing hook-delete-policy. Because having another policy "hook-exists-policy": "delete" will make a total mess together wih "hook-delete-policy" because of this HOOK_STATE => ACTION inversed form with overlapping delete action.

We made this decision also, because that way this hook-recreation function can be merged into helm 2. But there are talks about helm 3 major release, which will allow more radical changes.

By the way there is some experiments with "hook-exists-policy" in a separate branch of our fork: https://github.com/flant/helm/tree/hook_delete_exists_policy -- you can use that freely if you want for your implementation of new policy for keep action.

@alexey-igrychev alexey-igrychev force-pushed the flant:hook_recreate_delete_policy branch 2 times, most recently from 71d6303 to 2c1b7a2 Mar 21, 2018

Implement before-hook-creation delete policy
Existing helm.sh/hook-delete-policy annotation variables (hook-failed, hook-succeeded) do not allow to leave failed jobs for debugging without blocking the next job launching: every failed job must be deleted manually before the next related release is launching (installing, updating or rolling back).

New policy, before-hook-creation, removes the hook from previous release if there is one before the new hook is launched and can be used with another variable.

@alexey-igrychev alexey-igrychev force-pushed the flant:hook_recreate_delete_policy branch from 94c76e8 to 1d4883b Mar 21, 2018

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels Mar 21, 2018

@alexey-igrychev alexey-igrychev changed the title WIP: Implement hook-recreate policy Implement hook-rec policy Mar 21, 2018

@alexey-igrychev alexey-igrychev changed the title Implement hook-rec policy Implement before-hook-creation delete policy Mar 21, 2018

@distorhead distorhead force-pushed the flant:hook_recreate_delete_policy branch 2 times, most recently from c058fc9 to 1f4fe10 Mar 21, 2018

@distorhead

This comment has been minimized.

Copy link
Contributor

distorhead commented Mar 21, 2018

@bacongobbler @thomastaylor312 Hi!

We have added more info to docs about why this policy is useful: https://github.com/kubernetes/helm/pull/3540/files#diff-642a0a9c006dc089bfeecf0eaaf39749R183

And added unit-tests for all hook-delete-policies: https://github.com/kubernetes/helm/pull/3540/files#diff-92fec9d1af21a7a6caabc49a1b3ecffaR18

Also hook-recreate has been renamed to before-hook-creation.

@thomastaylor312
Copy link
Collaborator

thomastaylor312 left a comment

This is looking good to go. I left a few nits/suggestions that I don't think are necessary to hold up the PR on. If you'd like to change them before the second review comes in, just let us know. Thanks for the hard work!

Resources map[string]*hooksEmulatingKubeClientManifest
}

var errHooksEmulatingKubeClientResourceAlreadyExists = errors.New("resource already exists")

This comment has been minimized.

@thomastaylor312

thomastaylor312 Apr 2, 2018

Collaborator

Nit: This is a really long and confusing variable name. Would errResourceExists work?

This comment has been minimized.

@distorhead

distorhead Apr 3, 2018

Contributor
@@ -345,3 +352,460 @@ func (rs mockRunReleaseTestServer) SetTrailer(m metadata.MD) {}
func (rs mockRunReleaseTestServer) SendMsg(v interface{}) error { return nil }
func (rs mockRunReleaseTestServer) RecvMsg(v interface{}) error { return nil }
func (rs mockRunReleaseTestServer) Context() context.Context { return helm.NewContext() }

type hooksEmulatingKubeClientManifest struct {

This comment has been minimized.

@thomastaylor312

thomastaylor312 Apr 2, 2018

Collaborator

Nit: This type and the type below it have long and confusing names. Could it be something shorter like mockHooksManifest?

This comment has been minimized.

@distorhead
return core.PodUnknown, nil
}

func hookDeletePolicyReleaseServerStub(emulatingKubeClient *hooksEmulatingKubeClient) *ReleaseServer {

This comment has been minimized.

@thomastaylor312

thomastaylor312 Apr 2, 2018

Collaborator

Nit: Likewise from above, long name. Would something like deletePolicyStub suffice?

This comment has been minimized.

@distorhead
@thomastaylor312

This comment has been minimized.

Copy link
Collaborator

thomastaylor312 commented Apr 2, 2018

@bacongobbler This is ready for a second review

Add hook-delete-policy ReleaseServer unit tests
Cover hook-succeeded, hook-failed and before-hook-creation hook delete policies.

@distorhead distorhead force-pushed the flant:hook_recreate_delete_policy branch from dbfeca8 to b7fe0d0 Apr 3, 2018

@distorhead

This comment has been minimized.

Copy link
Contributor

distorhead commented Apr 3, 2018

Renamed as suggested:

  • hooksEmulatingKubeClient -> mockHooksKubeClient
  • hooksEmulatingKubeClientManifest -> mockHooksManifest
  • errHooksEmulatingKubeClientResourceAlreadyExists -> errResourceExists
  • hookDeletePolicyReleaseServerStub -> deletePolicyStub
  • hookDeletePolicyHookStub -> deletePolicyHookStub
  • annotation for mock kube client: TestHookDeletePolicy/Emulate -> mockHooksKubeClient/Emulate

Had not rebased on master because there is build error in current master.

@bacongobbler
Copy link
Member

bacongobbler left a comment

I only found a few minor doc typos. The rest of it looks great! Once those are fixed I'd be more than happy to approve this.


One might choose `"helm.sh/hook-delete-policy": "before-hook-creation"` over `"helm.sh/hook-delete-policy": "hook-succeeded,hook-failed"` because:

* It is convinient to keep failed hook job resource in kubernetes for example for manual debug.

This comment has been minimized.

@bacongobbler

bacongobbler Apr 3, 2018

Member

typo: convenient


* It is convinient to keep failed hook job resource in kubernetes for example for manual debug.
* It may be necessary to keep succeeded hook resource in kubernetes for some reason.
* At the same time it is not desireable to do manual resource deletion before helm release upgrade.

This comment has been minimized.

@bacongobbler

bacongobbler Apr 3, 2018

Member

typo: desirable

* It may be necessary to keep succeeded hook resource in kubernetes for some reason.
* At the same time it is not desireable to do manual resource deletion before helm release upgrade.

`"helm.sh/hook-delete-policy": "before-hook-creation"` annotation on hook causes tiller to remove the hook from previous release if there is one before the new hook is launched and can be used with another policies.

This comment has been minimized.

@bacongobbler

bacongobbler Apr 3, 2018

Member

typo: policies should be policy

@distorhead

This comment has been minimized.

Copy link
Contributor

distorhead commented Apr 3, 2018

@thomastaylor312 I think we are done enough here, have no more time for this.

@bacongobbler

This comment has been minimized.

Copy link
Member

bacongobbler commented Apr 3, 2018

Sounds good. We can clean up these typos afterwards. Cc'ing @mattfarina on this since we're cutting the 2.9 release candidate today

@bacongobbler bacongobbler merged commit e794c48 into helm:master Apr 3, 2018

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
cla/linuxfoundation distorhead authorized
Details

dwalleck added a commit to dwalleck/helm that referenced this pull request Apr 4, 2018

bacongobbler added a commit that referenced this pull request Apr 5, 2018

@raoy1990

This comment has been minimized.

Copy link

raoy1990 commented Apr 8, 2018

@distorhead As you describe we use the policy before-hook-creation will check if the resource you want to create is already exist, it will be deleted and be recreated.
But if the resource is used by other resource such as config map used by a deployment.The new one you will create is very different with the one exists in the cluster.
When the pod created by the deployment is abnormal and be recreated,but the config map has been changed and the pod will not be running just since the config the pod need is not exist(the config map has been changed).How to deal with this?

@Lentil1016

This comment has been minimized.

Copy link

Lentil1016 commented Apr 8, 2018

@raoy1990
My practice is to split the resources that is being depended in to another chart. And tirm a certain suffix from the release name of the dependency chart. like {{ .Release.Name }} and {{ .Release.Name | trimSuffix "-dependency" }}.
Therefor I deploy my configmap with release name test-deploy-dependency, then deploy my workload with release name test-deploy. They will have a common value test-deploy, then I can use it to make a binding. If I need to upgrade my workload, or reinstall it, I will only need to do changes on the release test-deploy. This is ugly, but it works for me.

splisson added a commit to splisson/helm that referenced this pull request Dec 6, 2018

splisson pushed a commit to splisson/helm that referenced this pull request Dec 6, 2018

Merge pull request helm#3540 from flant/hook_recreate_delete_policy
Implement before-hook-creation delete policy

splisson pushed a commit to splisson/helm that referenced this pull request Dec 6, 2018

splisson pushed a commit to splisson/helm that referenced this pull request Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment