search

rancher-sandbox / hypper

https://hypper.io
Apache License 2.0
66 stars 9 forks source link

Builder cache needs to be cleared #186

Open mattfarina opened 3 years ago

mattfarina commented 3 years ago

Take the case where hypper first installs a CRD chart and then installs a chart that creates resources based on those CRDs. In some cases, when hypper goes to install the custom resources it can't because it doesn't know of the type (i.e. it does not know about the CRD).

Than error like the following can occur:

ERROR: ❌  unable to build kubernetes objects from release manifest: unable to recognize "": no matches for kind "Logging" in version "logging.banzaicloud.io/v1beta1"
dragonchaser commented 3 years ago

@mattfarina I am having a bit of a hard time to track that down, can you give me a more detailed workflow example how this happens? Thx.

viccuad commented 3 years ago

From the daily, we extracted that this happens when one installs a chart with CRD, and then install a dependent chart of those CRDs. The kubeclient cache hasn't been updated to know about the CRDs, therefore the dependent install will have an error.

The kubeclient gets instantiated as part of the action.Install, in the action.Configuration, which reuses the action from the parent chart (inside the action there's Configuration.Kubeclient): https://github.com/rancher-sandbox/hypper/blob/1ac0a7588bf2bd5122218befba13d01e664b7de9/pkg/action/install.go#L354-L361.

We need to flush the kubeclient cache or create a new one, so it does a Get of all resources, and particularly of the newly installed CRDs. Maybe doing a Helm kubeclient.Update() is enough.

viccuad commented 2 years ago

I cannot reproduce here.

With https://github.com/rancher-sandbox/hypper/pull/191 (so I can do hypper install --wait), and with the kubewarden-controller and kubewarden-crds charts, annotated with Hypper annotations from https://github.com/viccuad/helm-charts/tree/add-hypper-annot.

Example:

  1. Install cert-manager shared-dep manually, as it needs a values for installCRDs=true, which we can't specify in Hypper annotations yet:
$ k3d cluster delete; k3d cluster create
$ hypper repo add jetstack https://charts.jetstack.io
$ hypper repo update
$ hypper install cert-manager jetstack/cert-manager --namespace cert-manager  --version v1.6.0 --set installCRDs=true
  1. Then, install kubewarden-controller, which installs kubewarden-crds:
$ hypper install --wait ./kubewarden-controller

Could it be that the chart that was failing was trying to install templates for the crds and templates that use them at the same time? (they could separate the CRDs into another chart, or use the hooks for ordering).

viccuad commented 2 years ago

The Bug is still valid. Note that the previous example succeeds because the CRDs get used in a post-install hook, which instantiates a new kube client:

kubeclient:     n1                     ->                                       -> n2
chart:          kubewarden-crds ->  kubewarden-controller -> post-install of kubewarden-controller that uses CRDs
viccuad commented 2 years ago

I still can't reproduce. Crafted a chart that installs crds, and a chart that depends on it and instantiates a CR, It succeds. See the commit in https://github.com/viccuad/hypper/tree/reproducer-crds.

Since that is an e2e test and Helm doesn't seem to have such a testsuite (closest seems to be https://github.com/helm/acceptance-testing), I haven't opened a PR for it. We could just do a simple bash testsuite with bats, for now.