-
Notifications
You must be signed in to change notification settings - Fork 503
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
Create a separate crds helm chart #677
Comments
@andrewdinunzio it seems your issue has 2 proposals in it :
|
I don't have much experience with multi-tenant clusters to know much about that but I figured maybe in that case that 1) they would probably be isolated by namespace and within their tenant namespace they would bind a normal namespaced W.r.t. a crds chart, I can certainly help with that when I find time, though I never used operator sdk before so will need to familiarize myself with that and how it could be automated with the releases, etc. Also would need to see what everyone thinks with respect to versioning a CRDs chart. We could follow something like linkerd which follows proper semver for their CRDs chart as its own entity, or we could do what I'd prefer and pin the crds chart releases (and versions) with the main app chart. The only reason the CRDs need their own chart is because helm handles CRDs badly, but otherwise they would probably be packaged alongside the operator that, well, operates on them. I prefer this approach because if the versions are pinned, there would be no question as to which CRDs chart version to install (and it'd be super easy for pipelines to handle; just install the same version CRDs chart as the app chart) |
I see a few use cases:
If I'm reading this correct you're proposing that the crd chart to be a sub chart of he operator chart which makes sense to me as well. |
I see, so if I understand your point correctly, having an aggregate-to-view/aggregate-to-edit could be a useful addition but you would want it to be disabled by default? Or are you saying that if we were to go this route, we might as well just make use of the ClusterRoles already present in the chart?
Not a subchart, since there are still potential issues with that. Helm aggregates all resources from dependencies and the "main" chart together before applying anything, and then it applies it in order based on Kind. Unfortunately, there is an indeterminate amount of time between when CRDs are applied and when they can be used to create/update resources of that type. It seems like this is due to a stale cache, which helm v3 fixed by adding the I went down the rabbit hole of looking into helm hooks as possible solutions too, but every approach seems to have issues, except having a separate chart dedicated to CRDs and installed separately (not as a dependency). By pinning the crds chart releases and versions to the main app chart, I simply mean that the CRDs chart would get a version bump even if nothing in it changed, just so that it always maintains parity with the main app chart, since essentially we are installing one "component". The only scenario I can see where this version pinning would be a bad thing would be if multiple different charts had dependencies on the OTel CRDs, then it would be tougher for the non-operator chart to know which version to use. It could be argued that that is more of a reason to use semver for the CRDs charts, but I'm not sure... there's a few points I think that factor into this:
I don't know if this makes sense, but that's my 2c Edit* Counterpoint to the above: CRDs are meant to only specify an API anyway and not an implementation. It's possible that one could install OpenTelemetry CRDs but use a different implementation for managing those custom resources. I can't think of anything that does that except maybe the Gateway API, which is now a built-in resource with K8s. Err actually VictoriaMetrics does this with the Prometheus CRDs too... Idk, maybe semver for the CRDs chart is actually a good idea, and k8s just needs more tooling in the "package management" space. |
Renaming this issue to focus on the CRDs helm chart, and I can make a separate issue for the other if desired. |
I'm actually saying that the current clusterrole/binding and the knobs to turn it on/off should be enough for any user to achieve his goals. The aggregate-to-view/aggregate-to-edit are useful but they should be done on the user side as they are custom to their needs/use cases. This is usually done by creating an internal chart which sub-charts the upstream(which assumes you are cluster admin) and adds custom configs on top. On the crds part you really went down the rabbit hole(thanks for that) and my opinion might disappoint you :) but here goes: After reading all of your research I'm inclined to avoid creating a separate crds chart and follow the kube-prometheus-stack model basically pinning the helm version to operator/crds version manually. You either include the crds when installing the chart for the first time (default helm behaviour) or sideload them trough a different process which deploys cluster level resources (ci/cd or any other scripts) and pass --skip-crds when deploying the operator chart. For upgrading safely there is a step that manually upgrades the crds before major/breaking helm chart changes. |
I agree with @Allex1 on this one too, I would avoid CRD chart given described helm issues. |
I should make it clear that having the dedicated crds chart does fix the helm issues mentioned above, provided it is installed separately before the main chart. But I'm on board with otel following the kube-prometheus-stack approach. It does make using common templates in a pipeline a bit more difficult, but as long as all CRDs are included in all releases, even if they don't change, then the pipelines can just apply the entire folder and the CRDs would be in the expected state. For kp, I had to make a script that inspects the helm chart, pulls out the appVersion from Chart.yaml, then downloads all the CRDs for that app version (substituting that app version in the URLs in the README), and I can do something similar for this. |
Meanwhile kps separated the crds in the a sub-chart |
Ohh, they take advantage of the cache invalidation provided by the Edit* just tested, still can't add new CRDs if using the |
So we're running into a flavor of this right now with #1164. We need to be able to upgrade the CRD to contain the new conversion webhook, but that has an expectation that the operator is available in a specific namespace.
As a result of this, I propose that we create a new chart that contains the CRDs within the
Although this is a bit different than what other charts do today, this would allow for a much simpler installation overall. The operator chart would then have the CRD chart as a dependency. I would be interested in hearing potential concerns with this approach. Those concerns are explained well in the original helm proposal. We should make this explicit for the chart:
There may be some cases where a user wants more than a single otel operator (multi-tenant installation where WATCH_NAMESPACE is set for each operator installation) in which case that user would probably install three charts – the CRDs, the first operator, and the second operator. In the operator installations the user would disable the CRD installation by flipping a value controlling the dependencies. |
FWIW, we do this extensively internally at my current role. It also appears that the following groups does this too:
|
@open-telemetry/helm-approvers please review as this is currently blocking the operator chart upgrade. I am most curious about the Istio solution of optionally including the CRDs in a template file via |
Putting crds in the templates causes a race condition where on the first installation, it might try to create CRs before the CRDs are created (due to the stale cache issue linked above that was "fixed" by the crds folder). On upgrades the stale cache can cause similar issues. Unless you mean putting the crds in a configmap or something and applying them in a helm hook job like Istio used to do, but they switched away from that for some reason, I think. I guess it's not an issue if you don't install any CRs but if that ever becomes the case it would be an issue. It would also be an issue for anyone trying to add this chart as a subchart/dependency where they add the CRs as templates. |
The CRDs should always be created first no? This is the sort order for helm |
There are no plans to allow the operator chart to install CRs, that will be the work of the |
Also we'll need to think about how these changes will affect that chart |
The CRDs are created first but due to the stale discovery cache it still throws an error while creating the CRs sometimes. We add the otel operator chart as a dependency and create CRs in our own chart, so it would affect us 😅 |
@TylerHelmuth either way for the opentelemetry-kube-stack i was going to do this anyway and fail open. The CRDs should be installed first and I can test that there. |
Okay I've looked into this a bit more and I do believe having an isolated helm chart for the CRDs is the way to go here. Given the precedence I linked above has a few operators that embed the CRDs in the templates, there are also many that simply embed the CRDs in their own chart: There are a few benefits to having this separate chart:
I tested this locally by using the subchart in the opentelemetry-kube-stack's dependencies this resulted in a clean and quick installation without needing any other work. As far as the plan goes for the operator's chart to use this:
Does this seem reasonable? cc @swiatekm-sumo @TylerHelmuth @open-telemetry/helm-approvers |
@jaronoff97 that seems reasonable, but I'm confused about the comparison to Are you proposing a standalone CRDs chart so that both |
yes thats what i was recommending here... there's maybe a case to be made for all of these to exist only within the kube-stack chart but that seems like a challenging deprecation to me. I wanted a single crds definition spot that can be used in multiple charts owned by us. |
If we follow the pattern set by in I wonder is better:
Do we have an expected roadmap for how long that'll be? |
I would be fine doing 2., it feels like the best of these options for the short term without locking us into anything in the long term. I'm not sure how many releases until v1alpha1 support is dropped, we should discuss at sig next week. FWIW 2. is what I have in #1166 right now and it works well :) cc @swiatekm-sumo what do you think about these options? |
Does the nested chart technique let us do templating on the CRDs? If no, then I'd prefer to stay with the current approach in the operator chart. I'm in favor of doing 1, because we have some consumers of the operator chart who want to depend on it while also shipping otel CRs, and it would make life easier for them if they could depend on a crd chart instead of shipping the crds on their own. |
@swiatekm-sumo option 2 would also support this I believe - you could configure the values.yaml of |
That's a bit convoluted, but the use case is inherently advanced, so I'm ok with it. |
Is there any progress in this discussion about the direction this is going? I use the |
That is the direction we're headed in |
This issue shifted focus. This is now an issue for creating a dedicated helm chart for opentelemetry CRDs.
The text was updated successfully, but these errors were encountered: