[velero] Add enable/disable option for helm hooks #171

igorcezar · 2020-10-14T19:47:56Z

Special notes for your reviewer:

Create an option in values to disable helm hooks used in CRDs (for Helm v2) and templates manifest (like BackupStorageLocation), introduced in #109 and #153.

With this change it's possible to install this chart using ArgoCD without any problems, setting the new option to false:

enableHelmHooks: false

It's possible to disable only CRD hooks or template hooks, in case you don't want to disable both.

The default option is true, which enables all helm hooks.

Helm hooks in crds folder, for Helm v3, are untouched, because they can't be templated.

Fix #158

Checklist

DCO signed
Chart Version bumped
Title of the PR starts with chart name (e.g. [velero])

Signed-off-by: Igor Cezar <[email protected]>

cidsasaki

Works as expected! Thanks for the correction.

cidsasaki · 2020-10-14T21:47:05Z

Resolves to #146 / #44

jenting · 2020-10-14T21:55:49Z

charts/velero/values.yaml

@@ -72,6 +72,12 @@ metrics:
 # Install CRDs as a templates. Enabled by default.
 installCRDs: true

+# Enable/disable helm hooks annotations for Helm v2 in CRDs and/or templates


May I say ArgoCD uses helm v2?

ArgoCD uses only Helm v2 until v1.4. Starting in v1.5 it has both v2 and v3, using v3 when Chart.yaml apiVersion is v2.

okay, since we're using apiVersion: v1 https://github.com/vmware-tanzu/helm-charts/blob/c90e413/charts/velero/Chart.yaml#L1, so the ArgoCD would use helm v2 behavior even we use ArgoCD version >= 1.5, right?

That’s right!

Not exactly, if you want to provide your own values.yaml for this chart for the helm install you need to build your own chart which has this one as a dependency. Which results in usually helm v3 being the actual engine.
This is because argocd requires the source of both helm chart and altered values.yaml to be in the same repository.

Not exactly, if you want to provide your own values.yaml for this chart for the helm install you need to build your own chart which has this one as a dependency. Which results in usually helm v3 being the actual engine.
This is because argocd requires the source of both helm chart and altered values.yaml to be in the same repository.

I use it that way, with velero chart as a dependency, and ArgoCD also uses Helm v2. For Helm v3 I have to change Chart.yaml apiVersion of the chart that I build (the one which imports velero chart) to v2, or force ArgoCD to use Helm v3 as described here.

charts/velero/values.yaml

jenting · 2020-10-14T22:02:36Z

@simran3695, @fabianmet could you please take a look a this 🙇 ? It seems to fix #158.

jenting · 2020-10-14T22:04:33Z

Resolves to #146 / #44

thanks, but I don't think it resolves the #44. #146 was want to resolve we always have to manual sync CRDs against each Velero release and also fixes the issues that helm upgrade won't upgrade the CRDs.

fabianmet

The actual issue arises because if you install this chart with helm v3 the pre-install pre-sync hooks are also triggered on the CRD's in the CRD directory.

This causes the state to be dropped by ArgoCD because a hook has no state. It is a one off job.
The only way to fix this is either remove the hook notations in the CRD directory all together, or change them back to the helm hook for crd-install.

I understand you want this helm chart to keep the CRD's in line with your actual helm velero installation but this goes against what helm themselves say as explained here.

I tried the same approach as @igorcezar did in this PR but it did not solve the issue where the CRD's were installed but constantly in a bad state because of the pre-install pre-sync hooks still being present on the CRD's themselves.

Also its a shame but the CRD directory does not treat files as templates, so there is no way to properly disable helm hooks in the CRD files.

Having the option to see what changes in a CRD is really helpful for upgrade purposes. So im a big fan of having them available easily in git instead of having velero themselves generate it but that is my opinion.

fabianmet · 2020-10-15T06:49:57Z

charts/velero/values.yaml

@@ -72,6 +72,12 @@ metrics:
 # Install CRDs as a templates. Enabled by default.
 installCRDs: true

+# Enable/disable helm hooks annotations for Helm v2 in CRDs and/or templates


Not exactly, if you want to provide your own values.yaml for this chart for the helm install you need to build your own chart which has this one as a dependency. Which results in usually helm v3 being the actual engine.
This is because argocd requires the source of both helm chart and altered values.yaml to be in the same repository.

Signed-off-by: Igor Cezar <[email protected]>

igorcezar · 2020-10-15T19:52:22Z

The only way to fix this is either remove the hook notations in the CRD directory all together, or change them back to the helm hook for crd-install.

In this PR we remove hook notations from templates/crds.yaml and the CR's templates, which for ArgoCD solves the problem, since it uses Helm v2 if you don't change the chart apiVersion to v2. But for sure the best solution would be to also remove from CRD folder so ArgoCD can work even using Helm v3.

@jenting the hook notations were included in CRD directory just for templates/crds.yaml could import the crds with the hooks, right?
Could we remove it from CRD folder (Helm v3) and enable only in crds.yaml for Helm v2?

jenting · 2020-10-15T22:34:07Z

The only way to fix this is either remove the hook notations in the CRD directory all together, or change them back to the helm hook for crd-install.

In this PR we remove hook notations from templates/crds.yaml and the CR's templates, which for ArgoCD solves the problem, since it uses Helm v2 if you don't change the chart apiVersion to v2. But for sure the best solution would be to also remove from CRD folder so ArgoCD can work even using Helm v3.

@jenting the hook notations were included in CRD directory just for templates/crds.yaml could import the crds with the hooks, right?

I think so.

Could we remove it from CRD folder (Helm v3) and enable only in crds.yaml for Helm v2?

Not sure what you'd like to remove? The helm annotation or CRD folder?

igorcezar · 2020-10-15T23:17:05Z

Not sure what you'd like to remove? The helm annotation or CRD folder?

The helm annotation in all *.yaml of CRD folder.
Keep annotation only for templates/crds.yaml file.

Signed-off-by: Igor Cezar <[email protected]>

igorcezar · 2020-10-15T23:43:23Z

@jenting I have pushed the changes I was talking about.

With this, the chart works for ArgoCD with Helm v2 (setting enableHelmHooks: false) and Helm v3 ( no matter how enableHelmHooks is set, but the preference is set to false, so that BSL/VSL/Schedule don't become Post Sync hooks).

Also works with helm install for both versions too (tested with helm v2.15.2 and v3.1.2), with enableHelmHooks: true (which is the default).

jenting · 2020-10-21T08:13:53Z

I want to play ArgoCD so I'll review this PR slowly.

jenting

@igorcezar could you please address the comments and fix the conflicts? thank you.

jenting · 2020-10-26T03:39:36Z

charts/velero/values.yaml

+# Enable/disable helm hooks annotations for CRDs (Helm v2) and templates
+# You should disable this if using a deploy tool that doesn't support helm hooks


I don't think it's for helm2 only, these helm hooks also available on helm3

maybe we could mention the deploy tool ArgoCD to help people easier to understand when to disable helm hooks annotations.

For CRDs it was helm2 only, because in helm3 CRDs can't be templated, but for all else it was for both v2 and v3.

For CRDs it was helm2 only, because in helm3 CRDs can't be templated, but for all else it was for both v2 and v3.

helm template vmware-tanzu/velero --include-crds

Hooks are available for Helm3, but in this chart the option enableHelmHooks will only enable/disable helm hooks for helm2 CRDs (templates/crds.yaml) and for all other templates on both versions.
I've made the changes in the comments and chart version.

Signed-off-by: Igor Cezar <[email protected]> Remove whitespaces Signed-off-by: Igor Cezar <[email protected]>

jenting

LGTM, thank you

igorcezar · 2020-11-13T20:40:59Z

Any news on this?

jenting · 2020-11-18T03:11:52Z

Any news on this?

@carlisia @cpanato PTAL

cpanato

lgtm, did not see any issues

thanks!

igorcezar added 3 commits October 14, 2020 16:35

Add enable/disable option for helm hooks

03db401

Signed-off-by: Igor Cezar <[email protected]>

Bump chart version

0215ba0

Signed-off-by: Igor Cezar <[email protected]>

Adjust crds.yaml

0d0acb0

Signed-off-by: Igor Cezar <[email protected]>

cidsasaki previously approved these changes Oct 14, 2020

View reviewed changes

jenting reviewed Oct 14, 2020

View reviewed changes

jenting added the velero label Oct 14, 2020

fabianmet reviewed Oct 15, 2020

View reviewed changes

Make one variable for all hooks

cec4978

Signed-off-by: Igor Cezar <[email protected]>

igorcezar dismissed cidsasaki’s stale review via cec4978 October 15, 2020 11:37

cidsasaki previously approved these changes Oct 15, 2020

View reviewed changes

igorcezar added 2 commits October 15, 2020 20:26

Remove all helm hooks in crds folder

aa4cfc6

Signed-off-by: Igor Cezar <[email protected]>

Add Helm v2 hooks for CRDs from _helpers template

b92e0bc

Signed-off-by: Igor Cezar <[email protected]>

igorcezar dismissed cidsasaki’s stale review via b92e0bc October 15, 2020 23:29

cidsasaki previously approved these changes Oct 19, 2020

View reviewed changes

jenting requested review from cpanato, carlisia and jenting October 21, 2020 08:15

carlisia assigned igorcezar Oct 21, 2020

jenting requested changes Oct 26, 2020

View reviewed changes

igorcezar dismissed cidsasaki’s stale review via d6ca9f6 October 27, 2020 15:32

Bump chart version and change hook comments

da91321

Signed-off-by: Igor Cezar <[email protected]> Remove whitespaces Signed-off-by: Igor Cezar <[email protected]>

igorcezar force-pushed the config-helm-hooks branch from 0739463 to da91321 Compare October 27, 2020 15:55

Merge branch 'main' into config-helm-hooks

5cbeeb2

jenting approved these changes Oct 28, 2020

View reviewed changes

Merge branch 'main' into config-helm-hooks

2eded10

cpanato approved these changes Nov 18, 2020

View reviewed changes

Merge branch 'main' into config-helm-hooks

efab963

jenting merged commit 0fc0edc into vmware-tanzu:main Nov 18, 2020

jenting mentioned this pull request Nov 19, 2020

After upgrading to 2.13.2 (1.5.1 velero) backup location not found error #168

Closed

phantasm66 mentioned this pull request Aug 17, 2022

Sync to 2.31.0 upstream chart repo & add missing volume snapshot resource and CRDs (SC-3707) ezcater/velero-helm#2

Merged

jenting mentioned this pull request Oct 8, 2023

Gitops tools (argocd) removes created backup resources #503

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[velero] Add enable/disable option for helm hooks #171

[velero] Add enable/disable option for helm hooks #171

igorcezar commented Oct 14, 2020 •

edited by jenting

Loading

cidsasaki left a comment

cidsasaki commented Oct 14, 2020 •

edited

Loading

jenting Oct 14, 2020 •

edited

Loading

igorcezar Oct 14, 2020

jenting Oct 15, 2020

igorcezar Oct 15, 2020

fabianmet Oct 15, 2020

igorcezar Oct 15, 2020

jenting commented Oct 14, 2020

jenting commented Oct 14, 2020 •

edited

Loading

fabianmet left a comment

fabianmet Oct 15, 2020

igorcezar commented Oct 15, 2020

jenting commented Oct 15, 2020

igorcezar commented Oct 15, 2020

igorcezar commented Oct 15, 2020 •

edited

Loading

jenting commented Oct 21, 2020

jenting left a comment

jenting Oct 26, 2020

igorcezar Oct 27, 2020

jenting Oct 27, 2020

igorcezar Oct 27, 2020

jenting left a comment

igorcezar commented Nov 13, 2020

jenting commented Nov 18, 2020

cpanato left a comment

		# Enable/disable helm hooks annotations for CRDs (Helm v2) and templates
		# You should disable this if using a deploy tool that doesn't support helm hooks

[velero] Add enable/disable option for helm hooks #171

[velero] Add enable/disable option for helm hooks #171

Conversation

igorcezar commented Oct 14, 2020 • edited by jenting Loading

Special notes for your reviewer:

Checklist

cidsasaki left a comment

Choose a reason for hiding this comment

cidsasaki commented Oct 14, 2020 • edited Loading

jenting Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenting commented Oct 14, 2020

jenting commented Oct 14, 2020 • edited Loading

fabianmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorcezar commented Oct 15, 2020

jenting commented Oct 15, 2020

igorcezar commented Oct 15, 2020

igorcezar commented Oct 15, 2020 • edited Loading

jenting commented Oct 21, 2020

jenting left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenting left a comment

Choose a reason for hiding this comment

igorcezar commented Nov 13, 2020

jenting commented Nov 18, 2020

cpanato left a comment

Choose a reason for hiding this comment

igorcezar commented Oct 14, 2020 •

edited by jenting

Loading

cidsasaki commented Oct 14, 2020 •

edited

Loading

jenting Oct 14, 2020 •

edited

Loading

jenting commented Oct 14, 2020 •

edited

Loading

igorcezar commented Oct 15, 2020 •

edited

Loading