refactor(chart): provide CRDs in a subchart #414

cmontemuino · 2024-02-06T08:52:10Z

Description

This changeset is mainly intended to skip Jaeger CRDs if it is not enabled. It is especially relevant when Jaeger tracing is installed separately from OpenEBS Mayastor.

Moving CRDs to a chart solves several issues: clean uninstall, possibility for upgrades, templating.

Motivation and Context

Helm 3 does not manage CRDs (see https://helm.sh/docs/chart_best_practices/custom_resource_definitions/). helm uninstall won't remove CRDs, and helm updgrade won't upgrade them. Manual intervention is required with the current setup.

CRDs installed from crds folder are not included into the helm release.

Additionally, it is not possible to template CRDs in Helm 3 in the crds folder.

Regression

No

How Has This Been Tested?

Same results as before this MR:

helm install --set crds.jaeger.enabled=true mayastor

Skip the installation of Jaeger CRDs:

helm install mayastor

Types of changes

Bug fix (non-breaking change which fixes an issue)
- [chart]: skip Jaeger CRD if Jaeger tracing is disabled mayastor#1586
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
~~I have added unit tests to cover my changes.~~
- It seems like this project is not using helm-unittest

tiagolobocastro · 2024-02-07T13:50:56Z

Seems the commit lint must have a bug as it's crashing :)
Anyhow, I think the fix here is to rename:
fixup! chore: allow chart/charts/crds
to
chore: allow chart/charts/crds
or perhaps
feat: allow chart/charts/crds

@cmontemuino any pros/cons to having the CRD files as a sub-chart, rather than just using a plain crd: true/false and checking for this in the yaml template?

@niladrih please take a look at this PR

niladrih

@cmontemuino -- I understand if you want to move the csi-snapshot CRDs to the chart/template directory. What is the motivation for moving them into a subchart? Maybe a new directory inside the template directory would suffice for this use-case?

chart/README.md

cmontemuino · 2024-02-13T16:46:50Z

Seems the commit lint must have a bug as it's crashing :) Anyhow, I think the fix here is to rename: fixup! chore: allow chart/charts/crds to chore: allow chart/charts/crds or perhaps feat: allow chart/charts/crds

@cmontemuino any pros/cons to having the CRD files as a sub-chart, rather than just using a plain crd: true/false and checking for this in the yaml template?

@niladrih please take a look at this PR

thanks for pointing this out. I've just squashed the offending commit

cmontemuino · 2024-02-13T16:48:01Z

@cmontemuino -- I understand if you want to move the csi-snapshot CRDs to the chart/template directory. What is the motivation for moving them into a subchart? Maybe a new directory inside the template directory would suffice for this use-case?

@niladrih do you mean adding the Jaeger CRD as a template?

cmontemuino · 2024-02-13T16:53:19Z

@cmontemuino any pros/cons to having the CRD files as a sub-chart, rather than just using a plain crd: true/false and checking for this in the yaml template?

@tiagolobocastro using Helm 3 approach to install CRDs forces you to upgrade CRDs manually (Helm won't upgrade them if you touch the files). That's problematic when doing GitOps.

That said, the main problem here is to avoid installing the Jaeger CRD. It is not possible to template CRDs in the crd folder.

A better approach here would be to publish CRDs in a separate chart. This PR is about applying the minimum required changes.

niladrih · 2024-02-22T05:43:27Z

@cmontemuino -- I understand if you want to move the csi-snapshot CRDs to the chart/template directory. What is the motivation for moving them into a subchart? Maybe a new directory inside the template directory would suffice for this use-case?

@niladrih do you mean adding the Jaeger CRD as a template?

That's right. Something like this:

{{- if and .Values.crds.enabled .Values.base.jaeger.enabled .Values.crds.jaeger-}}
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: jaegers.jaegertracing.io
...
{{- end -}}

{{- if and .Values.crds.enabled .Values.crds.volumeSnapshots-}}
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: volumesnapshots.snapshot.storage.k8s.io
...
{{- end -}}

chart/
├── crds
└── templates/
    └── crds/
        ├── jaeger.yaml
        ├── volumesnapshots.yaml
        ├── volumesnapshotclasses.yaml
        └── volumesnapshotcontents.yaml

crds:
  # Toggle installation for all CRDs. A 'false' disables installation for all.
  enabled: true
  # Toggle installation for Kubernetes CSI VolumeSnapshot CRDs.
  volumeSnapshots: true
  # Toggle installation for Jaeger CRD(s). 
  jaeger: true

WDYT @cmontemuino @tiagolobocastro? Is the crds.enabled a little too much?

cmontemuino · 2024-02-23T07:06:53Z

@cmontemuino -- I understand if you want to move the csi-snapshot CRDs to the chart/template directory. What is the motivation for moving them into a subchart? Maybe a new directory inside the template directory would suffice for this use-case?

@niladrih do you mean adding the Jaeger CRD as a template?

That's right. Something like this:

{{- if and .Values.crds.enabled .Values.base.jaeger.enabled .Values.crds.jaeger-}}
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: jaegers.jaegertracing.io
...
{{- end -}}

{{- if and .Values.crds.enabled .Values.crds.volumeSnapshots-}}
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: volumesnapshots.snapshot.storage.k8s.io
...
{{- end -}}

chart/
├── crds
└── templates/
    └── crds/
        ├── jaeger.yaml
        ├── volumesnapshots.yaml
        ├── volumesnapshotclasses.yaml
        └── volumesnapshotcontents.yaml

crds:
  # Toggle installation for all CRDs. A 'false' disables installation for all.
  enabled: true
  # Toggle installation for Kubernetes CSI VolumeSnapshot CRDs.
  volumeSnapshots: true
  # Toggle installation for Jaeger CRD(s). 
  jaeger: true

WDYT @cmontemuino @tiagolobocastro? Is the crds.enabled a little too much?

Hi @niladrih! What you proposed could work for this repo.
I've checked the CRDs are being referenced from a ClusterRole only, that that's ok with Helm's installation order:

That said, I don't really know what could happen in the operator is consuming some of the CRDs, especially in the case of an upgrade when removing/adding APIs.

There is also the option to use chart hooks, but they don't come free of troubles during upgrades.

The following comes from helm best practices around CRDs:

https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you

There is no support at this time for upgrading or deleting CRDs using Helm. This was an explicit decision after much community discussion due to the danger for unintentional data loss.

**This is why I've filed issue openebs/mayastor/issues/1586 **

https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-2-separate-charts:

Another way to do this is to put the CRD definition in one chart, and then put any resources that use that CRD in another chart.

In this method, each chart must be installed separately. However, this workflow may be more useful for cluster operators who have admin access to a cluster

This PR is a convenient shortcut if you will. A completely separate chart for CRDs might make more sense.

niladrih · 2024-03-01T16:02:03Z

@cmontemuino -- Hmm... let me try this out once. The doc seems convincing, I'll spin up a cluster later today.

niladrih · 2024-03-08T13:23:33Z

@cmontemuino -- Changes look good to me, could you also add a switch for the volumeSnapshot CRDs?

crds:
  # -- Install CRDs
  enabled: true
  jaeger:
    # -- Install Jaeger CRDs
    enabled: false
  volumeSnapshots:
    enabled: true

niladrih

Left few comments.

chart/charts/crds/values.yaml

chart/charts/crds/templates/csi-volume-snapshot-class.yaml

chart/README.md

cmontemuino · 2024-03-12T15:58:11Z

Hi @niladrih, I've addressed all the comments already.
I'll fix the commit lint by squashing all commits into one. Is this ok?

niladrih

Left one comment

chart/charts/crds/values.yaml

niladrih · 2024-03-12T16:30:09Z

@cmontemuino -- I see one issue. I've highlighted that above. Otherwise, changes look good to me. I think I'll approve and queue for a merge when you make the change. Thanks!

cmontemuino · 2024-03-12T19:09:59Z

@cmontemuino -- I see one issue. I've highlighted that above. Otherwise, changes look good to me. I think I'll approve and queue for a merge when you make the change. Thanks!

I see it now. Thanks!

cmontemuino · 2024-03-12T19:26:19Z

@cmontemuino -- I see one issue. I've highlighted that above. Otherwise, changes look good to me. I think I'll approve and queue for a merge when you make the change. Thanks!

I see it now. Thanks!

@niladrih -- the PR is ready to go now

niladrih · 2024-03-13T10:07:11Z

bors merge

bors-openebs-mayastor · 2024-03-13T10:07:16Z

👎 Rejected by too few approved reviews

niladrih · 2024-03-13T10:09:17Z

bors merge

414: refactor(chart): provide CRDs in a subchart r=niladrih a=cmontemuino ## Description This changeset is mainly intended to skip Jaeger CRDs if it is not enabled. It is especially relevant when Jaeger tracing is installed separately from OpenEBS Mayastor. Moving CRDs to a chart solves several issues: clean uninstall, possibility for upgrades, templating. ## Motivation and Context Helm 3 does not manage CRDs (see https://helm.sh/docs/chart_best_practices/custom_resource_definitions/). `helm uninstall` won't remove CRDs, and `helm updgrade` won't upgrade them. Manual intervention is required with the current setup. - CRDs installed from `crds` folder **are not included into the helm release**. Additionally, it is not possible to template CRDs in Helm 3 in the `crds` folder. ## Regression  No  ## How Has This Been Tested? <! Co-authored-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com>

bors-openebs-mayastor · 2024-03-13T10:14:21Z

Build failed:

continuous-integration/jenkins/branch

cmontemuino · 2024-03-14T07:20:11Z

@niladrih -- could you please issue bors merge one more time?

niladrih · 2024-03-14T08:33:16Z

It failed because of an issue with the helm-doc script.

niladrih · 2024-03-14T09:02:24Z

This requires #435.

niladrih · 2024-03-14T12:14:16Z

bors merge

414: refactor(chart): provide CRDs in a subchart r=niladrih a=cmontemuino ## Description This changeset is mainly intended to skip Jaeger CRDs if it is not enabled. It is especially relevant when Jaeger tracing is installed separately from OpenEBS Mayastor. Moving CRDs to a chart solves several issues: clean uninstall, possibility for upgrades, templating. ## Motivation and Context Helm 3 does not manage CRDs (see https://helm.sh/docs/chart_best_practices/custom_resource_definitions/). `helm uninstall` won't remove CRDs, and `helm updgrade` won't upgrade them. Manual intervention is required with the current setup. - CRDs installed from `crds` folder **are not included into the helm release**. Additionally, it is not possible to template CRDs in Helm 3 in the `crds` folder. ## Regression  No  ## How Has This Been Tested? <! Co-authored-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com>

bors-openebs-mayastor · 2024-03-14T12:21:21Z

Build failed:

continuous-integration/jenkins/branch

niladrih · 2024-03-14T12:54:17Z

@cmontemuino -- could you rebase your PR and run this helm-docs script against your changes:

nix-shell ./chart/shell.nix --run ./scripts/helm/generate-readme.sh

Add the changes...

git status
git add chart/README.md
git commit ...

And then push to this branch..?

This changeset is mainly intended to skip Jaeger CRDs if it is not enabled. This is especially relevant when Jaeger tracing is installed separately from OpenEBS Mayastor. Helm 3 does not manage CRDs. - https://helm.sh/docs/chart_best_practices/custom_resource_definitions `helm uninstall` won't remove CRDs, and `helm updgrade` won't upgrade them. Manual intervention is required with the current setup. - CRDs installed from `crds` folder are not included into the helm release. Additionally, it is not possible to template CRDs in Helm 3 in the `crds` folder. Moving CRDs to a chart solves several issues: clean uninstall, possibility for upgrades, templating. Also make volumeSnapshots CRD optional Signed-off-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com>

Signed-off-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com>

cmontemuino · 2024-03-14T13:20:41Z

@cmontemuino -- could you rebase your PR and run this helm-docs script against your changes:
nix-shell ./chart/shell.nix --run ./scripts/helm/generate-readme.sh 
Add the changes...
git status
git add chart/README.md
git commit ...
And then push to this branch..?

@niladrih -- all done

niladrih · 2024-03-14T13:22:13Z

bors merge

bors-openebs-mayastor · 2024-03-14T13:26:44Z

Build succeeded:

continuous-integration/jenkins/branch

454: fix(upgrade-job): look for crds in charts dir r=niladrih a=niladrih The upgrade-job validates the chart that it's got by checking if all the right directories are where they're supposed to be. Recent changes to the CRD installation method (ref: #414) has resulted in the upgrade-job failing, because things aren't how it expects. This PR changes the CRDs directory path to that of the subchart which encapsulates the CRDs now. Co-authored-by: Niladri Halder <niladri.halder26@gmail.com>

tiagolobocastro · 2024-03-26T15:09:01Z

@cmontemuino a side effect we're finding from this is that if we uninstall when there are CRs present then as the CRD is removed then the CRs are also removed... this essentially means you lose the snapshots!

I think we should add helm.sh/resource-policy: keep by default and provide an option to change it if required, but this should probably not be the default behaviour.
What do you think?

cmontemuino · 2024-03-26T17:40:08Z

@cmontemuino a side effect we're finding from this is that if we uninstall when there are CRs present then as the CRD is removed then the CRs are also removed... this essentially means you lose the snapshots!

I think we should add helm.sh/resource-policy: keep by default and provide an option to change it if required, but this should probably not be the default behaviour. What do you think?

I think you didn't notice the situation before because helm uninstall does not remove CRDs from crds directory.
What you suggest will work just fine. For people using Argo CD, another different annotation is required.

Would you like me to draft a PR to handle this scenario + documentation?
Basically, we need a new element, something like crds.keep: true.
Then, in the CRD templates, we add the annotation you've mentioned above.

Something like the following:

# CRD code here ...
  metadata:
    {{- if .Values.crds.keep }}
    annotations:
      helm.sh/resource-policy: keep
      {{- if .Capabilities.APIVersions.Has "argoproj.io/v1alpha1" }}
      argocd.argoproj.io/sync-options: Prune=false
      {{- end }}
    {{- end }}

Of course it means tight-coupling to Argo CD (i.e., more maintenance, etc.), which might be non desirable....thus, something like the following might be a better fit:

# CRD code here ...
  metadata:
    {{- if or (.Values.crds.keep) (.Values.crds.annotations }}
    annotations:
      {{- if .Values.crds.keep }}
      helm.sh/resource-policy: keep
      {{- end }}
      {{- range $key, $value := .Values.crds.annotations }}
        {{ $key }}: {{ $value | quote }}
      {{- end }}
    {{- end }}

With good documentation everything should be super clear.

Just let me know if you want me to do it and I'll prepare the PR this week.

tiagolobocastro · 2024-03-26T17:46:08Z

That sounds good @cmontemuino, we'd like to take that PR if you can make it, otherwise we can look into adding it ourselves, let us know :)

Also, do we need to keep the crd presence check on the template? This seems to be required only if upgrading from v2.5.0 which packed the yaml in the crd folder. In case case we could simply document that one needs to use --set crds.enabled=false rather than keep the crd presence check?

cmontemuino · 2024-03-26T17:57:17Z

      {{- range $key, $value := .Values.crds.annotations }}
        {{ $key }}: {{ $value | quote }}
      {{- end }}

Working on it already. Let me draft a PR and we discuss based on it.

cmontemuino · 2024-03-26T18:23:21Z

      {{- range $key, $value := .Values.crds.annotations }}
        {{ $key }}: {{ $value | quote }}
      {{- end }}

Working on it already. Let me draft a PR and we discuss based on it.

Can we move the conversation here? #458

I need a bit of help to write the PR description too... not sure if this is a feature or regression for example.

cmontemuino force-pushed the conditional-install-jaeger-crd branch from ce5a45c to de4761b Compare February 6, 2024 08:53

cmontemuino mentioned this pull request Feb 6, 2024

[chart]: skip Jaeger CRD if Jaeger tracing is disabled openebs/mayastor#1586

Closed

niladrih reviewed Feb 7, 2024

View reviewed changes

chart/README.md Outdated Show resolved Hide resolved

cmontemuino force-pushed the conditional-install-jaeger-crd branch 3 times, most recently from 6621a36 to c93fde9 Compare February 13, 2024 16:46

cmontemuino force-pushed the conditional-install-jaeger-crd branch from c93fde9 to 3117120 Compare February 13, 2024 16:55

Abhinandan-Purkait approved these changes Mar 8, 2024

View reviewed changes

niladrih reviewed Mar 8, 2024

View reviewed changes

chart/charts/crds/values.yaml Outdated Show resolved Hide resolved

chart/charts/crds/templates/csi-volume-snapshot-class.yaml Outdated Show resolved Hide resolved

chart/README.md Show resolved Hide resolved

chart/README.md Outdated Show resolved Hide resolved

cmontemuino force-pushed the conditional-install-jaeger-crd branch from 3117120 to 846d869 Compare March 12, 2024 15:52

niladrih reviewed Mar 12, 2024

View reviewed changes

chart/charts/crds/values.yaml Outdated Show resolved Hide resolved

cmontemuino force-pushed the conditional-install-jaeger-crd branch 3 times, most recently from 5ecab24 to b11cfe5 Compare March 12, 2024 19:20

niladrih approved these changes Mar 13, 2024

View reviewed changes

cmontemuino force-pushed the conditional-install-jaeger-crd branch from 105e441 to 7571cd6 Compare March 14, 2024 06:48

cmontemuino force-pushed the conditional-install-jaeger-crd branch from 7571cd6 to 1891c92 Compare March 14, 2024 13:19

cmontemuino added 3 commits March 14, 2024 14:19

fix: wrong indentation in crd values

31f7852

Signed-off-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com>

docs: update readme

92e2c38

Signed-off-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com>

cmontemuino force-pushed the conditional-install-jaeger-crd branch from 1891c92 to 92e2c38 Compare March 14, 2024 13:19

bors-openebs-mayastor bot merged commit a00529d into openebs:develop Mar 14, 2024
5 checks passed

cmontemuino deleted the conditional-install-jaeger-crd branch March 14, 2024 13:29

This was referenced Mar 16, 2024

refactor(charts): move CRDs into a subchart openebs/zfs-localpv#512

Closed

refactor(charts): move CRDs to a subchart openebs/lvm-localpv#284

Merged

fix(upgrade-job): look for crds in charts dir #454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(chart): provide CRDs in a subchart #414

refactor(chart): provide CRDs in a subchart #414

cmontemuino commented Feb 6, 2024

tiagolobocastro commented Feb 7, 2024

niladrih left a comment

cmontemuino commented Feb 13, 2024

cmontemuino commented Feb 13, 2024

cmontemuino commented Feb 13, 2024 •

edited

Loading

niladrih commented Feb 22, 2024 •

edited

Loading

cmontemuino commented Feb 23, 2024

niladrih commented Mar 1, 2024

niladrih commented Mar 8, 2024 •

edited

Loading

niladrih left a comment

cmontemuino commented Mar 12, 2024

niladrih left a comment

niladrih commented Mar 12, 2024

cmontemuino commented Mar 12, 2024

cmontemuino commented Mar 12, 2024

niladrih commented Mar 13, 2024

bors-openebs-mayastor bot commented Mar 13, 2024

niladrih commented Mar 13, 2024

bors-openebs-mayastor bot commented Mar 13, 2024

cmontemuino commented Mar 14, 2024

niladrih commented Mar 14, 2024

niladrih commented Mar 14, 2024

niladrih commented Mar 14, 2024

bors-openebs-mayastor bot commented Mar 14, 2024

niladrih commented Mar 14, 2024 •

edited

Loading

cmontemuino commented Mar 14, 2024

niladrih commented Mar 14, 2024

bors-openebs-mayastor bot commented Mar 14, 2024

tiagolobocastro commented Mar 26, 2024 •

edited

Loading

cmontemuino commented Mar 26, 2024 •

edited

Loading

tiagolobocastro commented Mar 26, 2024

cmontemuino commented Mar 26, 2024

cmontemuino commented Mar 26, 2024

refactor(chart): provide CRDs in a subchart #414

refactor(chart): provide CRDs in a subchart #414

Conversation

cmontemuino commented Feb 6, 2024

Description

Motivation and Context

Regression

How Has This Been Tested?

Types of changes

Checklist:

tiagolobocastro commented Feb 7, 2024

niladrih left a comment

Choose a reason for hiding this comment

cmontemuino commented Feb 13, 2024

cmontemuino commented Feb 13, 2024

cmontemuino commented Feb 13, 2024 • edited Loading

niladrih commented Feb 22, 2024 • edited Loading

cmontemuino commented Feb 23, 2024

niladrih commented Mar 1, 2024

niladrih commented Mar 8, 2024 • edited Loading

niladrih left a comment

Choose a reason for hiding this comment

cmontemuino commented Mar 12, 2024

niladrih left a comment

Choose a reason for hiding this comment

niladrih commented Mar 12, 2024

cmontemuino commented Mar 12, 2024

cmontemuino commented Mar 12, 2024

niladrih commented Mar 13, 2024

bors-openebs-mayastor bot commented Mar 13, 2024

niladrih commented Mar 13, 2024

bors-openebs-mayastor bot commented Mar 13, 2024

cmontemuino commented Mar 14, 2024

niladrih commented Mar 14, 2024

niladrih commented Mar 14, 2024

niladrih commented Mar 14, 2024

bors-openebs-mayastor bot commented Mar 14, 2024

niladrih commented Mar 14, 2024 • edited Loading

cmontemuino commented Mar 14, 2024

niladrih commented Mar 14, 2024

bors-openebs-mayastor bot commented Mar 14, 2024

tiagolobocastro commented Mar 26, 2024 • edited Loading

cmontemuino commented Mar 26, 2024 • edited Loading

tiagolobocastro commented Mar 26, 2024

cmontemuino commented Mar 26, 2024

cmontemuino commented Mar 26, 2024

cmontemuino commented Feb 13, 2024 •

edited

Loading

niladrih commented Feb 22, 2024 •

edited

Loading

niladrih commented Mar 8, 2024 •

edited

Loading

niladrih commented Mar 14, 2024 •

edited

Loading

tiagolobocastro commented Mar 26, 2024 •

edited

Loading

cmontemuino commented Mar 26, 2024 •

edited

Loading