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

Builder cache needs to be cleared #186

Open
mattfarina opened this issue Aug 3, 2021 · 5 comments
Open

Builder cache needs to be cleared #186

mattfarina opened this issue Aug 3, 2021 · 5 comments
Labels
bug Something isn't working
Projects

Comments

@mattfarina
Copy link
Collaborator

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"
@mattfarina mattfarina added the bug Something isn't working label Aug 3, 2021
@mattfarina mattfarina added this to To do in Shit It via automation Aug 3, 2021
@dragonchaser dragonchaser moved this from To do to In progress in Shit It Aug 23, 2021
@dragonchaser dragonchaser self-assigned this Aug 23, 2021
@dragonchaser
Copy link
Contributor

@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
Copy link
Member

viccuad commented Sep 13, 2021

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):

clientInstall := NewInstall(i.Config)
// we need to automatically satisfy all install options (i.CreateNamespace,
// i.DryRun, etc) when we are installing the dep using clientInstall. Doing
// a shallow copy sounds like asking for trouble when the install struct
// changes, so let's do a deep copy instead:
if err := copier.Copy(&clientInstall, &i); err != nil {
return nil, err
}
.

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
Copy link
Member

viccuad commented Nov 15, 2021

I cannot reproduce here.

With #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
Copy link
Member

viccuad commented Nov 15, 2021

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
Copy link
Member

viccuad commented Nov 15, 2021

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.

@dragonchaser dragonchaser removed their assignment Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Shit It
In progress
Development

No branches or pull requests

3 participants