Skip to content
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.

Sign up for GitHub

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

feat: install CRDs from a subchart #1922

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: install CRDs from a subchart #1922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c60d355
helm: install CRDs from a subchart
cmontemuino Oct 22, 2024
e5ed797
helm: adopt main chart example style
cmontemuino Oct 24, 2024
cb5d03a
helm: Merge branch 'master' into crds-in-subchart
cmontemuino Oct 24, 2024
3f3ab44
Merge branch 'kubernetes-sigs:master' into crds-in-subchart
cmontemuino Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions deployment/helm/node-feature-discovery/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,7 @@ keywords:
- node-labels
type: application
version: 0.2.1
dependencies:
- name: crds
version: 0.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we want to put a version here not 0.0.0

Copy link
Contributor Author

@cmontemuino cmontemuino Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

condition: crds.install
3 changes: 3 additions & 0 deletions deployment/helm/node-feature-discovery/charts/crds/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
apiVersion: v2
name: crds
version: 0.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

9 changes: 9 additions & 0 deletions deployment/helm/node-feature-discovery/charts/crds/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# crds

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here


## Values

| Key | Type | Default | Description |
|-----|------|---------|-------------|
| annotations | object | `{}` | Define annotations for CRDs |
719 changes: 719 additions & 0 deletions deployment/helm/node-feature-discovery/charts/crds/templates/nfd-api-crds.yaml
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions deployment/helm/node-feature-discovery/charts/crds/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# -- Define annotations for CRDs
annotations:
# example: ...
7 changes: 7 additions & 0 deletions deployment/helm/node-feature-discovery/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ featureGates:

priorityClassName: ""

## Custom resource configuration defined as a dependency chart
crds:
# -- 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)
install: false
# -- Annotations to be added to all CRDs
annotations: {}

master:
enable: true
extraArgs: []
Expand Down
6 changes: 4 additions & 2 deletions docs/deployment/helm.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,10 @@ Chart parameters are available.

### General parameters

| Name | Type | Default | Description |
|-----------------------------------------------------|--------|-----------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| 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) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| `crds.annotations` | object | {} | Annotations to be added to all CRDs |
| `image.repository` | string | `{{ site.container_image \| split: ":" \| first }}` | NFD image repository |
| `image.tag` | string | `{{ site.release }}` | NFD image tag |
| `image.pullPolicy` | string | `Always` | Image pull policy |
Expand Down
3 changes: 3 additions & 0 deletions hack/crds.annotations.snippet.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{{- with .Values.annotations }}
{{- toYaml . | nindent 4 }}
{{- end }}
4 changes: 4 additions & 0 deletions hack/update_codegen.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ NFD_ROOT=$(realpath $(dirname ${BASH_SOURCE[0]})/..)
controller-gen object crd output:crd:stdout paths=./api/... > deployment/base/nfd-crds/nfd-api-crds.yaml
mkdir -p deployment/helm/node-feature-discovery/crds
cp deployment/base/nfd-crds/nfd-api-crds.yaml deployment/helm/node-feature-discovery/crds
mkdir -p deployment/helm/node-feature-discovery/charts/crds/templates
for f in deployment/base/nfd-crds/*-crds.yaml; do
sed '/controller-gen.kubebuilder.io\/version/ r hack/crds.annotations.snippet.txt' ${f} > deployment/helm/node-feature-discovery/charts/crds/templates/${f##*/}
done

# Generate clientset and informers
mv vendor ${TMP_VENDOR_DIR}
Expand Down