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

helm chart CRD installation first when installed as dependency #226

Open
agnewp opened this issue Jan 10, 2022 · 34 comments
Open

helm chart CRD installation first when installed as dependency #226

agnewp opened this issue Jan 10, 2022 · 34 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@agnewp
Copy link

agnewp commented Jan 10, 2022

A clear and concise description of what the bug is.

Expected Behavior

Keda can be installed from its chart as a dependency of another custom chart that uses Keda CRDs

Actual Behavior

Helm expects that CRDs need to be installed BEFORE other resources that potentially use these CRDS. when KEDA is a dependent chart of another one that attempts to use KEDA CRDs, the helm installation fails.

Steps to Reproduce the Problem

  1. make a simple chart that depends on the keda chart. this simple chart should have a keda CRD in it at minimum, like a TriggerAuthentication object for example
  2. use helm to attempt installation of the top-level custom chart
  3. watch in horror as you get this error:

installation failed: unable to build kubernetes objects from release manifest: unable to recognize \"\": no matches for kind \"TriggerAuthentication\" in version \"keda.sh/v1alpha1\"

Specifications

  • KEDA Version: 2.5.1
  • Platform & Version:
  • Kubernetes Version: 1.20 eks
  • Scaler(s): N/A

the good news is that you aren't the first people to run into this problem, i see someone made a crude attempt at enforcing installation order by making the file names look like linux init.d scripts in some kind of assumption that helm doesn't do things in parallel. check this out: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/

the fix is supper simple; put the crd definitions into a folder called 'crds' and boom! helm will install the CRD defs first every time!

@agnewp agnewp added the bug Something isn't working label Jan 10, 2022
@tomkerkhove
Copy link
Member

the fix is supper simple; put the crd definitions into a folder called 'crds' and boom! helm will install the CRD defs first every time!

This is what we used to have, but the CRDs were not being updated then. Helm does not provide a good story here.

@tomkerkhove tomkerkhove added the help wanted Extra attention is needed label Jan 11, 2022
@agnewp
Copy link
Author

agnewp commented Jan 11, 2022

well then its a trade-off. We have experienced that the current behavior for a new installation is quite flakey. the helm install command actually misses installation of some of the CRDs some of the time. also is there a point to how the files are named? does this actually work as a way to define installation order?

@zroubalik
Copy link
Member

@agnewp I can see that you are reporting KEDA version 1.5.1, but that error mentiones:

no matches for kind "TriggerAuthentication" in version "keda.sh/v1alpha1"

which is a version of a resource used on KEDA 2.X.

I definitely recommend you to use 2.x version, 1.x is deprecated.

@agnewp
Copy link
Author

agnewp commented Jan 11, 2022

my mistake, i am using 2.5.1

@tomkerkhove
Copy link
Member

well then its a trade-off. We have experienced that the current behavior for a new installation is quite flakey

The alternative approach will silently continue giving you incomplete CRDs.

However, I will take a look when I have time but we might want to use both approaches as a hack: crd/ for new installations and the current approach for updates.

@tomkerkhove
Copy link
Member

Let me try the chart again tomorrow though because last time I checked I didn't have issues.

@tomkerkhove
Copy link
Member

So what I did was:

  1. Create a fresh cluster (using AKS)
  2. Check CRDs:
~  k get crds
NAME                                             CREATED AT
volumesnapshotclasses.snapshot.storage.k8s.io    2022-01-12T12:24:40Z
volumesnapshotcontents.snapshot.storage.k8s.io   2022-01-12T12:24:40Z
volumesnapshots.snapshot.storage.k8s.io          2022-01-12T12:24:40Z
  1. Install KEDA
~  helm install keda --version 2.5.1 --namespace keda-system kedacore/keda
NAME: keda
LAST DEPLOYED: Wed Jan 12 13:37:00 2022
NAMESPACE: keda-system
STATUS: deployed
REVISION: 1
TEST SUITE: None
  1. Check CRDs:
~  k get crds
NAME                                             CREATED AT
clustertriggerauthentications.keda.sh            2022-01-12T12:37:05Z
scaledjobs.keda.sh                               2022-01-12T12:37:05Z
scaledobjects.keda.sh                            2022-01-12T12:37:05Z
triggerauthentications.keda.sh                   2022-01-12T12:37:05Z
volumesnapshotclasses.snapshot.storage.k8s.io    2022-01-12T12:24:40Z
volumesnapshotcontents.snapshot.storage.k8s.io   2022-01-12T12:24:40Z
volumesnapshots.snapshot.storage.k8s.io          2022-01-12T12:24:40Z

So everything seems fine.

Can you provide some more information about your environment? Keeping in mind that you use it as a Helm dependency of course, I'll test that next.

@tomkerkhove
Copy link
Member

Ok I can reproduce this:

~  helm install metachart .
Error: INSTALLATION FAILED: unable to build kubernetes objects from release manifest: unable to recognize "": no matches for kind "TriggerAuthentication" in version "keda.sh/v1alpha1"

Will take a stab at a fix later this week/next week. (will do my best)

@tomkerkhove
Copy link
Member

@tomkerkhove tomkerkhove changed the title helm chart CRD installation first helm chart CRD installation first when installed as dependency Jan 12, 2022
@tomkerkhove
Copy link
Member

Workaround: Install KEDA outside of the current Helm chart.

@agnewp
Copy link
Author

agnewp commented Jan 12, 2022

This is indeed what we did to work around the issue. however, even installing Keda stand-alone with the current chart, we ran into inconsistent issues with the chart failing to install all of the CRDs. very strange. i wonder if some of the CRDs depend on eachother?

@tomkerkhove
Copy link
Member

We do not have knowledge about that issue. Can you still reproduce this with the latest version?

@tomkerkhove
Copy link
Member

Would it help if there is a Helm chart with solely the CRDs that you can install first?

@simonlsk
Copy link

simonlsk commented Mar 9, 2022

This is what we used to have, but the CRDs were not being updated then. Helm does not provide a good story here.

Is there an open issue for that in helm?

Would it help if there is a Helm chart with solely the CRDs that you can install first?

Do you mean that we would add two dependencies in the chart like this:

- name: keda
  version: 2.5.1
  repository: https://kedacore.github.io/charts
- name: keda-crds
  version: 2.5.1
  repository: https://kedacore.github.io/charts

I think that could work for me.

@tomkerkhove
Copy link
Member

Is there an open issue for that in helm?

Yes, see above but link is helm/helm#10585

Do you mean that we would add two dependencies in the chart like this:

- name: keda
  version: 2.5.1
  repository: https://kedacore.github.io/charts
- name: keda-crds
  version: 2.5.1
  repository: https://kedacore.github.io/charts

I think that could work for me.

No, so you can deploy the CRDs before the application. Having it as two dependencies would have same issue.

@toniopelo
Copy link

I would add that for some of us that are using skaffold with helm, this is even worst.
Breaking this in two separate charts to first install the keda chart with all the crds and after install the other chart that uses these crds as stated above doesn't work (stated here: #226 (comment)).

  • skaffold dev works fine as it first install the keda chart and then the other chart.
  • But when uninstalling it fails as it first uninstall the keda chart with all the crds (they are in templates/, so uninstalled as any other resource) and then when trying to uninstall the other chart that rely on the keda crds that doesn't exist anymore, it fails to uninstall it.

Then I tried to extract the crds elsewhere to handle them separately (a command that will apply them before installing the rest via helm, easily integrated with the skaffold flow thanks to the hooks commands).
There it would theoretically work and allow me to put back all the chart under one roof.
But it doesn't work because the keda chart include the crds under the template/ folder and these are conflicting with crds I already installed.

Having one chart keda-crds and another one keda without the crds like proposed by @tomkerkhove (#226 (comment)) would solve my problem as I could only install the keda one and avoid conflicts.
For now I'm stuck with unpacking the keda chart and removing the crds myself, this is a bit annoying, but this works.
This might be best of both world as you get to handle the keda-crds chart how you wish with the upgrade possibility and still allow the ones that would like to handle crds their way to use your keda chart.

@tomkerkhove
Copy link
Member

Thanks for bumping this thread again, unfortunately no change on the Helm side.

Having one chart keda-crds and another one keda without the crds like proposed by @tomkerkhove Tom Kerkhove FTE (#226 (comment)) would solve my problem as I could only install the keda one and avoid conflicts

If we do this, then you would be unblocked if I get it correctly?

@toniopelo
Copy link

Hi @tomkerkhove, thanks for the fast reply :)!

unfortunately no change on the Helm side.

Yeah, the Helm implementation is really not ideal...

If we do this, then you would be unblocked if I get it correctly?

Yes that's right. I would be unblocked in my case, and I think that having both charts separated would likely solve other cases as well.

  • you can still have them both (nothing change)
  • you can handle the crds in another way that suits your needs and still use the main chart without risking conflicts.

@tomkerkhove
Copy link
Member

tomkerkhove commented Sep 13, 2022

What are your thoughts on this @kedacore/keda-maintainers? I think if we do this we should do the following:

  • Introduce keda-crd Helm chart with solely our CRDs
  • Introduce keda-runtime Helm chart with everything, except for our CRDs
  • Continue maintaining keda Helm chart, but use keda-crd & keda-runtime as dependencies.

Thoughts?

@toniopelo
Copy link

@tomkerkhove I just realised that you can use the following in the values.yaml:

crds:
  install: false

This will prevent crds from being installed on the current chart.
So I can use the chart as-is without any conflict and still installing the crds my way.
I don't think any action needs to be taken on your side then :).

@tomkerkhove
Copy link
Member

Sounds good, thanks for letting us know!

@brianV
Copy link

brianV commented Dec 23, 2022

I just ran into the same issue.

I ended up taking the CRDs from the 2.8.2.yaml file and installing them. I then added the helm annotations and labels. Following that, the helm chart installed correctly, and I know the removal of the CRDs will be managed by helm.

wkalt added a commit to foxglove/helm-charts that referenced this issue Jan 11, 2023
Adds autoscalers for the inbox-listener and stream service. The inbox
listener autoscaler relies on KEDA. The stream service autoscaler is a
standard HPA.

KEDA has some unfortunate friction with helm. It cannot be added as a
dependency to the chart and installed in a reasonable way, because it
relies on CRDs, which helm will not be responsible for updating or
installing in order.

Based on conversation in kedacore/charts#226,
there are two alternative approaches open to us:

* We can instruct users to install KEDA on their cluster prior to
  installing our deployment. The command for this is,
    helm install keda --version 2.9.1 --namespace keda kedacore/keda --create-namespace

* We can vendor KEDA's CRDs and put them into our chart, under a crd
  directory. This should enable a single-command install, but comes with
  the downsides that,
  1. Helm will not update or manage the keda CRDs for us
  2. If users already have KEDA installed (certainly possible) we will
     encounter a conflict.

  Mainly due to the second item, this PR takes the first approach.
wkalt added a commit to foxglove/helm-charts that referenced this issue Jan 11, 2023
Adds autoscalers for the inbox-listener and stream service. The inbox
listener autoscaler relies on KEDA. The stream service autoscaler is a
standard HPA.

KEDA has some unfortunate friction with helm. It cannot be added as a
dependency to the chart and installed in a reasonable way, because it
relies on CRDs, which helm will not be responsible for updating or
installing in order.

Based on conversation in kedacore/charts#226,
there are two alternative approaches open to us:

* We can instruct users to install KEDA on their cluster prior to
  installing our deployment. The command for this is,
    helm install keda --version 2.9.1 --namespace keda kedacore/keda --create-namespace

* We can vendor KEDA's CRDs and put them into our chart, under a crd
  directory. This should enable a single-command install, but comes with
  the downsides that,
  1. Helm will not update or manage the keda CRDs for us
  2. If users already have KEDA installed (certainly possible) we will
     encounter a conflict.

  Mainly due to the second item, this PR takes the first approach.
@wmagon
Copy link

wmagon commented Feb 15, 2023

[TL;DR]
even though CRDs are not clear for Helm, the separate /crds location seems to be a standard, prepared to provide compatibility with different approaches and giving some choice.
For our project, where we use Terraform with helm_release resource, it's extremely useful to detach CRDs from Helm Charts,that provides us flexibility with recreating/destroying resources automated way many times, without removing CR objects. Detached CRDs are applied/updated with kubectl (in null_resource) before helm_release and it's convenient if that is separate single file, stored in different catalog, in Release assets, even in different repo would work, etc.

[Longer description]
I think it would still be very helpful if CRDs would have separate location (e.g. /crds). If not for skipping them with Helm flag, then for installing in separate step. Correct me if I'm wrong, but currently it seems to be tricky to install keda CRDs outside the Helm and not controlled by Helm (needed some kind of extracting from template the few .yaml files, generate manifest that could be applied after removing from it helm annotations/labels?). I think it's safer and quite common scenario, than someone would like to avoid removal of CRDs during Chart deletion and wants to pre-install them first.

Many projects are struggling with similar Helm challenges, but I guess I've everywhere seen recommendations and solutions that allows extracting CRDs from the Chart, e.g.:

@tomkerkhove
Copy link
Member

Correct me if I'm wrong, but currently it seems to be tricky to install keda CRDs outside the Helm and not controlled by Helm (needed some kind of extracting from template the few .yaml files, generate manifest that could be applied after removing from it helm annotations/labels?).

This is not 100% correct - If you use Helm, it will install/update them. For non-Helm scenarios we offer them as YAML in our GitHub release.

I'm happy to adapt and align with the /crd pattern, but does it actually install them? That is not clear to me.

@wmagon
Copy link

wmagon commented Mar 7, 2023

I meant Helm scenario and the whole point is to have any option to use Helm, but without CRDs and easy way to install CRDs separately.
Currently even if we would use crds.install = false, there is no simple way to find only CRDs manifests, as they are always part of the gathered config. Also mentioned yaml manifests gives only option to apply everything or nothing.
All examples mentioned above gives the option for that separation. Some yaml on Github releases with just CRDs (like cert-manager), or /crd with such yaml, both would work and provide that helpful flexibility.

@tomkerkhove
Copy link
Member

So having a YAML file with just CRDs in the releases would help?

@wmagon
Copy link

wmagon commented Mar 8, 2023

In our case yes. We would have an easy way to install/maintain only CRDs first and then Helm Chart without CRDs.

@daodennis-ri
Copy link

What is the problem if you don't skip CRDs with the installation of the main chart? I'm trying to understand why skipCRDs is a thing

@wmagon
Copy link

wmagon commented Aug 31, 2023

@daodennis-ri
In case of this chart the CRDs will be installed wrong way, with helm annotations, tracked by helm and removed during helm uninstall, what might remove Custom Resources related to those CRDs. That's why Helm decided to go with safer approach: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/.

Usually it's not big difference whether CRDs will be installed first time with Helm or without (e.g. kubectl) if can be later easily maintained (updated) outside and if are not tracked by Helm later on , something like Methond 1 in Helm docs. The Helm idea is to allow such installation (or skip) using manifests in /crds where "These CRDs are not templated".

With KEDA chart the issue and main difference is that /crds files are "templated" and I think /crds location doesn't work as designed and CRDs are annotated/labeled and tracked by helm. There is an option to disable CRDs with crds.install though. To install CRDs outside helm, instead of using /crds templated files and unpinning helm tracking by removing annotations, some manifests located in kedacore/keda should allow doing it much easier.

@edeustua
Copy link

edeustua commented Sep 5, 2023

I'm having a similar problem. I'm trying to install cloudnative-pg's helm chart as a dependency (set in Chart.yaml) and helm is not able to install the corresponding CRDs before attempting to install my chart.

@sidharthpunathil
Copy link

+1 Same Problem

@edeustua
Copy link

After fiddling with this for a while, I've reached the conclusion that the concept of Helm subcharts is fundamentally flawed. This is specially true for CRDs, or charts containing non-namespaced resources.

The solution we have arrived at is the same as the one proposed by most Helm charts containing CRDs. Have them as a separate Helm installation, decoupled for your target chart.

@rajpopat27
Copy link

So what is the recommended approach should we install keda out of the main helm chart and not add it as a dependency? Because I am still facing this issue where it fails saying CRD's doesn't exist

@ana-cafemedia
Copy link

@tomkerkhove Any update on your keda-crd chart? We are encountering the same issue and would like to be able to use that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests