feat: install CRDs from a subchart #1922

cmontemuino · 2024-10-22T09:42:15Z

ROBLEM: Helm 3 only install CRDs during the chart installation. Both deletes and upgrades are not handled by Helm 3. It requires manual intervention. Additionally, it is not possible to template CRDs. This is especially important when installing the chart with an Argo CD application.

SOLUTION: add the possibility to install the CRDs from a dependency subchart. This allows clean uninstall, possibility for automated upgrades, and templating. This feature comes disabled by default.

closes #1637

k8s-ci-robot · 2024-10-22T09:42:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cmontemuino
Once this PR has been reviewed and has the lgtm label, please assign marquiz for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2024-10-22T09:42:25Z

Hi @cmontemuino. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

netlify · 2024-10-22T09:42:37Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`3f3ab44`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/6731f62f1bdb240008045216
😎 Deploy Preview	https://deploy-preview-1922--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

TessaIO · 2024-10-22T09:53:49Z

Thanks for the patch @cmontemuino. I saw, that you added the new CRD installation without removing the old CRDs manifests. Wouldn't that be an issue if we installed both Helm charts (the NFD and CRD) as it would try to create the same CRD twice and fail?

cmontemuino · 2024-10-22T10:13:10Z

Thanks for the patch @cmontemuino. I saw, that you added the new CRD installation without removing the old CRDs manifests. Wouldn't that be an issue if we installed both Helm charts (the NFD and CRD) as it would try to create the same CRD twice and fail?

Thanks for raising this point @TessaIO, as it requires more clarification.

My initial take was to make this feature completely optional. The assumption here is that there could be a use case for using Helm 3 CRDs approach.

This is what I've added in the values.yaml:

# -- Toggle to install and upgrade CRDs from a subchart. Make sure to use it with `--skip-crds` to avoid conflicts.

And also reflected it in the helm.md file -- general parameters section.

I'd say a section in helm.md detailing this new addition makes sense. That's where I'm struggling about how the documentation is generated. For example, should I add a new section in the same lines as in https://github.com/kubernetes-sigs/node-feature-discovery/blob/master/docs/deployment/helm.md#from-v014 ?

PROBLEM: Helm 3 only install CRDs during the chart installation. Both deletes and upgrades are not handled by Helm 3. It requires manual intervention. Additionally, it is not possible to template CRDs. This is especially important when installing the chart with an Argo CD application. SOLUTION: add the possibility to install the CRDs from a dependency subchart. This allows clean uninstall, possibility for automated upgrades, and templating. This feature comes disabled by default. closes kubernetes-sigs#1637 Signed-off-by: cmontemuino <[email protected]>

TessaIO

Thanks @cmontemuino for the clarifications. I put some comments, I'll review it one more time once this is ready.

TessaIO · 2024-10-22T15:57:01Z

deployment/helm/node-feature-discovery/Chart.yaml

@@ -13,3 +13,7 @@ keywords:
  - node-labels
 type: application
 version: 0.2.1
+dependencies:
+  - name: crds
+    version: 0.0.0


Probably we want to put a version here not 0.0.0

There's a drawback to use a version here: you need bump the version and helm dependency update with every release.

The versioning usually makes sense when adding charts from other repos, with different lifecycles.

In our case, CRDs are always generated and copied under crds folder: https://github.com/kubernetes-sigs/node-feature-discovery/blob/master/hack/update_codegen.sh#L44-L46

There's a 1-1 match in the lifecycle for this chart. Thus, the version: 0.0.0 makes the trick to avoid extra steps.

TessaIO · 2024-10-22T15:57:08Z

deployment/helm/node-feature-discovery/charts/crds/Chart.yaml

@@ -0,0 +1,3 @@
+apiVersion: v2
+name: crds
+version: 0.0.0


TessaIO · 2024-10-22T15:57:15Z

deployment/helm/node-feature-discovery/charts/crds/README.md

@@ -0,0 +1,9 @@
+# crds
+
+![Version: 0.0.0](https://img.shields.io/badge/Version-0.0.0-informational?style=flat-square)


deployment/helm/node-feature-discovery/charts/crds/values.yaml

TessaIO · 2024-10-22T16:02:06Z

docs/deployment/helm.md

@@ -149,6 +149,8 @@ Chart parameters are available.

 | Name                                                | Type   | Default                                             | Description                                                                                                                                                                                                  |
 | --------------------------------------------------- | ------ | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `crds.install`                                      | bool   | false                                               | Toggle to install and upgrade CRDs from a subchart. Make sure to use it with `--skip-crds` to avoid conflicts. [More info about limitations on CRDs in Helm 3](https://helm.sh/docs/topics/charts/#limitations-on-crds) |


If i'm not mistaken, I think the --skip-crds flag is used for the crds folder and not the separate helm chart. so I don't think that they're related. Please correct if I'm wrong.

You're right about the flag 👍

The problem happens only when the chart is installed by the first time. In that scenario: helm install xxxx --skip-crds --set crds.install=true

(apologies for the oversimplification above)

Otherwise, Helm will install CRDs from both places. Again, this only happens the first time. Helm won't care about the CRDs folder during upgrades (nor uninstall)

Are you keeping an additional copy of the CRD in the /crds directory only for the initial installation of the chart?
I'm wondering if it might be too strict to always require using --skip-crds --set crds.install=true.

My take was make the new approach optional. That is, if you do nothing, then the CRDs get installed as they are now.
By the other hand, if you want to taste the new approach, then --skip-crds --set crds.install=true.

I wonder if this is a good compromise, or if we should go the sub-chart with CRDs approach in one single shot. WDYT @fmuyassarov

Regarding additional copies, all of them are automatically generated.

I was considering whether we should standardize on using only crds.install=true for all installations, including both first-time installs and upgrades. However, I'm concerned it might be too much to ask of users at this stage. For now, I think it’s better to keep it optional as you have it now.

Signed-off-by: cmontemuino <[email protected]>

k8s-ci-robot · 2024-11-11T12:18:56Z

Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

c60d355 helm: install CRDs from a subchart

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 22, 2024

k8s-ci-robot requested review from kad and PiotrProkop October 22, 2024 09:42

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 22, 2024

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 22, 2024

cmontemuino mentioned this pull request Oct 22, 2024

Install CRDs from a subchart instead of using Helm 3 crds directory #1637

Open

cmontemuino force-pushed the crds-in-subchart branch from 2ec2038 to c60d355 Compare October 22, 2024 10:45

TessaIO reviewed Oct 22, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2024

helm: adopt main chart example style

e5ed797

Signed-off-by: cmontemuino <[email protected]>

cmontemuino force-pushed the crds-in-subchart branch from 81a7c2b to e5ed797 Compare October 24, 2024 11:55

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024

helm: Merge branch 'master' into crds-in-subchart

cb5d03a

Signed-off-by: cmontemuino <[email protected]>

cmontemuino force-pushed the crds-in-subchart branch from 1588b58 to cb5d03a Compare October 24, 2024 14:40

Merge branch 'kubernetes-sigs:master' into crds-in-subchart

3f3ab44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: install CRDs from a subchart #1922

feat: install CRDs from a subchart #1922

cmontemuino commented Oct 22, 2024

k8s-ci-robot commented Oct 22, 2024

k8s-ci-robot commented Oct 22, 2024

netlify bot commented Oct 22, 2024 •

edited

Loading

TessaIO commented Oct 22, 2024

cmontemuino commented Oct 22, 2024

TessaIO left a comment

TessaIO Oct 22, 2024

cmontemuino Oct 23, 2024 •

edited

Loading

TessaIO Oct 22, 2024

TessaIO Oct 22, 2024

TessaIO Oct 22, 2024

cmontemuino Oct 24, 2024

fmuyassarov Nov 7, 2024

cmontemuino Nov 11, 2024

fmuyassarov Nov 11, 2024

k8s-ci-robot commented Nov 11, 2024

		@@ -0,0 +1,9 @@
		# crds

		![Version: 0.0.0](https://img.shields.io/badge/Version-0.0.0-informational?style=flat-square)

feat: install CRDs from a subchart #1922

Are you sure you want to change the base?

feat: install CRDs from a subchart #1922

Conversation

cmontemuino commented Oct 22, 2024

k8s-ci-robot commented Oct 22, 2024

k8s-ci-robot commented Oct 22, 2024

netlify bot commented Oct 22, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

TessaIO commented Oct 22, 2024

cmontemuino commented Oct 22, 2024

TessaIO left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmontemuino Oct 23, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 11, 2024

netlify bot commented Oct 22, 2024 •

edited

Loading

cmontemuino Oct 23, 2024 •

edited

Loading