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.

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

Move CRDs back to root folder #1785

Closed
leviplj opened this issue Jan 20, 2023 · 8 comments · Fixed by #1857
Closed

Move CRDs back to root folder #1785

leviplj opened this issue Jan 20, 2023 · 8 comments · Fixed by #1857
Assignees
Labels
argo-workflows enhancement New feature or request

Comments

@leviplj
Copy link

leviplj commented Jan 20, 2023

Is your feature request related to a problem?

With crds in the templates folder, we can't install argo-workflows and resources that use its custom resources (eg. WorkflowTemplate) on a single helm install.
From what I understood, this was done to avoid problems with updates to the CRDs when the chart was already installed.

Related helm chart

argo-workflows

Describe the solution you'd like

According to Helm best practices, the custom resources should be in a folder called crds on the root directory of the chart.
That would allow the installation and usage of the CRDs on a single run.
To solve the problem with CRD updates, I'd suggest we create a new version of the CRD with the changes when needed.

Describe alternatives you've considered

No response

Additional context

When installing argo-workflows and a resource of kind WorkflowTemplate in a single run:

Error: unable to build kubernetes objects from release manifest: unable to recognize "": no matches for kind "WorkflowTemplate" in version "argoproj.io/v1alpha1"
helm.go:84: [debug] unable to recognize "": no matches for kind "WorkflowTemplate" in version "argoproj.io/v1alpha1"
@leviplj leviplj added the enhancement New feature or request label Jan 20, 2023
@jmeridth
Copy link
Member

@leviplj this has gone back and forth. They were moved out with #1472 due to the opposite requests in #1430 and #1468.

According to Helm best practices, the custom resources should be in a folder called crds on the root directory of the chart

Yep

I'd suggest we create a new version of the CRD with the changes when needed

We're still doing that with them in the templates folder. It does make it harder because we need to make sure we don't accidentally remove any go templating we put in place.

I'm going to play with a blank setup again and think on this a bit.

Thank you for filing your issue and your patience.

@jmeridth jmeridth self-assigned this Jan 21, 2023
@mkilchhofer
Copy link
Member

@leviplj how do you install the chart and the WorkflowTemplate at once? That is not a supported/provided use case on our helm charts here :-)

@leviplj
Copy link
Author

leviplj commented Jan 25, 2023

@mkilchhofer I was using argo-workflows version 0.15.0. As the CRDs were on the special folder, they were installed before helm did the templating, so I could install the CRDs and use them "at the same time".

This would be an example of the structure of the charts we have.

MainChart
├── argo-workflows
│   └── crds
│   └── ...
└── chart1
    ├── Chart.yaml
    └── templates
        ├── workflow-template-1.yaml
        └── workflow-template-2.yaml

@cyrilico
Copy link

+1 for this. As it stands, CRDs are not installed when Argo Workflows is installed as a dependency (I have a custom chart that builds on top of Argo Workflows to create some base workflow templates). Putting the crds folder at the chart root and removing templating is what Helm recommends. It works, of course, if we install Argo Workflows separately first, but that doesn't seem very ideal for CI testing, for example

@mkilchhofer
Copy link
Member

When the CRDs are placed in the root folder, helm cannot upgrade them. We had multiple issues where users had old CRDs installed.
I'd sugest to disable the installation of the CRDs and manage them outside your umbrella chart or copy them over to your umbrella chart.

@leviplj
Copy link
Author

leviplj commented Jan 27, 2023

Well, that's a workaround to fix the problem, we are using that for the time being.
I understand that changes to a CRD would result in creating a new version of that CRD. A CRD definition should be immutable as we might have resources on the cluster using it already.
To solve the problem of users having issues with old CRDs I'd suggest creating a new version of the CRD when argo needs an updated definition of that CRD.

@mkilchhofer
Copy link
Member

You forgot that updates to CRDs are sometimes due to kubernetes and not as applications extens them. Eg.:

  • how additional attrubutes are honoured (preserve unknown fields)
  • updates due to kubebuilder
  • updates due to kubernetes upgrades (api version of the CRD object itself)

@jmeridth
Copy link
Member

@leviplj I've been thinking on this one a lot lately as it actually just affected my day job. Unfortunately, even Helm knows this is an issue and states it in their docs:

Furthermore, there is currently no community consensus around how to handle CRDs and their lifecycle. As this evolves, Helm will add support for those use cases. from here

We understand the crds folder is the "standard" but even Helm admits it is not a good one re: upgrades.

There have been a few PRs to github.com/helm/helm that have stalled. Versioning CRDs is a choice but not an easy one, per se.

Our current choice is the approach with the most maintainable perspective. From a consumer perspective it is still not an intuitive or easy one.

I'm going to discuss this more with other maintainers. Please know we are not ignoring you.

jmeridth added a commit to jmeridth/argo-helm that referenced this issue Feb 23, 2023
…es folder

Closes argoproj#1785

Borrowed `Custom resource definition` from argo-cd helm chart README and added it to
argo-workflows helm chart README

Signed-off-by: jmeridth <jmeridth@gmail.com>
jmeridth added a commit that referenced this issue Feb 23, 2023
…es folder (#1857)

Closes #1785

Borrowed `Custom resource definition` from argo-cd helm chart README and added it to
argo-workflows helm chart README

Signed-off-by: jmeridth <jmeridth@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
argo-workflows enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants