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

[collector] Fix: Enable external configmap checksum #1302

Merged

Conversation

bixu
Copy link
Contributor

@bixu bixu commented Aug 13, 2024

Fixes "[collector] No collector reload when using external configmap" (#1194)

This PR ships a new config option, configMap.existingPath, which is unset by default. When enabled, it takes a relative path to a custom ConfigMap that can be bundled in a wrapper chart. A new helper template calculates a checksum annotation from the custom ConfigMap file.

Fixes [collector] No collector reload when using external configmap open-telemetry#1194
@bixu bixu requested a review from a team August 13, 2024 16:00
@bixu bixu changed the title Enable external configmap checksum [collector] Fix: Enable external configmap checksum Aug 13, 2024
@bixu bixu requested a review from puckpuck as a code owner August 14, 2024 08:08
@bixu

This comment was marked as outdated.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the demo examples got removed. If you only want to generate the collector examples you can do make generate-examples CHARTS=opentlemetry-collector.

charts/opentelemetry-collector/values.yaml Outdated Show resolved Hide resolved
@bixu bixu requested a review from TylerHelmuth August 14, 2024 17:39
@bixu
Copy link
Contributor Author

bixu commented Aug 14, 2024

It looks like the demo examples got removed. If you only want to generate the collector examples you can do make generate-examples CHARTS=opentlemetry-collector.

I've re-run the example generator for the Collector after reverting the previous generation (which seemed to be the cause of deleted files). The diff is much smaller now.

I'm not sure how to best handle merge conflicts from upstream around generated examples, though -- tried a few merge flows locally but was not happy with the results.

@TylerHelmuth
Copy link
Member

@bixu the best way to handle merging the examples is to take mains, bump the chart version to the appropriate value, and regenerate.

There doesn't seem to be a way to get at rendered template contents in Helm. This key allows a user to specify the path to template file that will be used to create `configMap.existingName`, in cases like mine where we want to wrap this chart in a chart that feeds in custom configmap contents.
@bixu bixu requested a review from TylerHelmuth August 15, 2024 16:01
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
@TylerHelmuth TylerHelmuth merged commit 634745a into open-telemetry:main Aug 16, 2024
3 checks passed
@bixu bixu deleted the enable-external-configmap-checksum branch August 16, 2024 15:58
@onematchfox
Copy link

Hey @bixu,

Are you actively using this feature and if so, how? I am trying to deploy this chart as a subchart whilst using existingPath but can't seem to find a working configuration and keep getting an error like:

foo/charts/opentelemetry-collector/templates/_helpers.tpl:234:22: executing "opentelemetry-collector.configTemplateChecksumAnnotation" at <include (print $.Template.BasePath "/" .Values.configMap.existingPath) .>: error calling include: template: no template "foo/charts/opentelemetry-collector/templates/../../../../otel/config.yaml" associated with template "gotpl"

According to the docs

Files outside of a Helm application subchart, including those of the parent, cannot be accessed

so it doesn't look like what I'm trying to do is possible.

I noticed that in the description you mentioned "can be bundled in a wrapper chart". What exactly is a "wrapper chart". Perhaps I'm approaching this wrong?

@bixu
Copy link
Contributor Author

bixu commented Oct 1, 2024

Hey @bixu,

Are you actively using this feature and if so, how? I am trying to deploy this chart as a subchart whilst using existingPath but can't seem to find a working configuration and keep getting an error like:

foo/charts/opentelemetry-collector/templates/_helpers.tpl:234:22: executing "opentelemetry-collector.configTemplateChecksumAnnotation" at <include (print $.Template.BasePath "/" .Values.configMap.existingPath) .>: error calling include: template: no template "foo/charts/opentelemetry-collector/templates/../../../../otel/config.yaml" associated with template "gotpl"

According to the docs

Files outside of a Helm application subchart, including those of the parent, cannot be accessed

so it doesn't look like what I'm trying to do is possible.

I noticed that in the description you mentioned "can be bundled in a wrapper chart". What exactly is a "wrapper chart". Perhaps I'm approaching this wrong?

@onematchfox, I actually ran in to the same issue the other day, but hadn't gotten around to thinking about a fix. I'm afraid this config option is broken and I'd like to remove it, or do you have a better idea?

@onematchfox
Copy link

or do you have a better idea?

Nope, sorry, I don't. I was hoping that I was just approaching it wrong.

@bixu
Copy link
Contributor Author

bixu commented Oct 1, 2024

or do you have a better idea?

Nope, sorry, I don't. I was hoping that I was just approaching it wrong.

@onematchfox, are you in the CNCF slack for the Collector? I'm @bixu there; would love to chat more about your use-case and what our options are.

@onematchfox
Copy link

@onematchfox, are you in the CNCF slack for the Collector? I'm @bixu there; would love to chat more about your use-case and what our options are.

I am. Will drop you a msg a bit later this afternoon when I have some spare time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants