-
Notifications
You must be signed in to change notification settings - Fork 633
add opentelemetry-ebpf-instrumentation helm chart #1704
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
base: main
Are you sure you want to change the base?
add opentelemetry-ebpf-instrumentation helm chart #1704
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to discuss this chart addition in the issue first
Hey @TylerHelmuth, just added a comment in the issue. Hope it clarifies everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
@mariomac iv'e added to k8s cache to the helm chart, so i would love a re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!! Thanks for the contribution
- Integrate the chart with the Makefile for consistent tooling support. - Add examples and render via `make generate-examples'.
9a6914d
to
f91fcfa
Compare
@TylerHelmuth i see that mario has approved |
I still need to do a real review but wont have time till July. But I dont want to block anymore.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
hey @TylerHelmuth, are you available for a review of this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the github workflows so that this chart is tested? chart-testing will do a default install if there is no ci
folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a section detailing requirements
- https://github.com/open-telemetry/opentelemetry-helm-charts | ||
icon: https://opentelemetry.io/img/logos/opentelemetry-logo-nav.png | ||
maintainers: | ||
- name: nimrodavni78 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- name: nimrodavni78 | |
- name: dmitryax | |
- name: jaronoff97 | |
- name: TylerHelmuth | |
- name: nimrodavni78 |
{{/* | ||
Expand the name of the chart. | ||
*/}} | ||
{{- define "obi.name" -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please prefix all the templates with the chart's name instead of obi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename file to daemonset.yaml please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to clusterrole.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename to clusterrolebinding.yaml
ebpf-instrument-config.yml: | | ||
{{- if eq .Values.preset "network" }} | ||
{{- if not .Values.config.data.network }} | ||
network: | ||
enable: true | ||
{{- end }} | ||
{{- end }} | ||
{{- if eq .Values.preset "application" }} | ||
{{- if not .Values.config.data.discovery }} | ||
discovery: | ||
services: | ||
- k8s_namespace: . | ||
exclude_services: | ||
- exe_path: ".*ebpf-instrument.*|.*otelcol.*" | ||
{{- end }} | ||
{{- end }} | ||
{{- toYaml .Values.config.data | nindent 4}} | ||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets move the configmap logic into a helper template like the other charts do
global: | ||
image: | ||
# -- Global image registry to use if it needs to be overridden for some specific use cases (e.g local registries, custom images, ...) | ||
registry: "" | ||
# -- Optional set of global image pull secrets. | ||
pullSecrets: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this and .Values.image
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are 2 components, the obi agent and the k8s cache, this allows for configuring the same registry for both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
global
is a keyword in helm, can we nest this in a different section that is specific to this chart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mariomac wdyt, this was copied from the beyla helm chart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine replacing global
by something more meaningful that doesn't collide with the keyword.
According to the block description, it seems something more related to the infrastructure setup (registry, pull secrets...). Maybe something like setup
, infrastructure
, login
?
TBH I don't know about anyone using this section in Beyla.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote we simplify then and remove this capability. Users can configure each image in its own section instead.
4c568f9
to
8ae443f
Compare
@TylerHelmuth fixed all comments except changing the chart name |
@NimrodAvni78 the name that we use for the executable and the docker image is
|
The pattern to match is to include |
f253362
to
e39a935
Compare
@@ -0,0 +1,327 @@ | |||
image: | |||
# -- Opentelemetry eBPF Instrumentation image registry (defaults to docker.io) | |||
registry: "docker.io" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think makes sense to default to "" or ghcr, this is what we recommend for collector;
helm install my-opentelemetry-collector open-telemetry/opentelemetry-collector --set mode= --set image.repository="ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector-k8s" --set command.name="otelcol-k8s"
We had a lot of issues with docker.io with publishing our images and rate-limiting
repository: otel/ebpf-instrument | ||
# -- (string) Opentelemetry eBPF Instrumentation image tag. When empty, the Chart's appVersion is | ||
# used. | ||
tag: main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not default to main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this a bit troublesome to maintain, as it's very hard to know if a specific commit removed a flag or did breaking change, so one day this helm chart will work another day it won't.
Maybe it makes sense to wait for proper versions, before releasing the chart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We absolutely should not release a chart that doesn't have an official image. We've had issues in the Collector SIG dealing with some dependencies that didn't have an official tag. I dont want that to happen here.
We should wait for the 0.1.0 release before merging this PR.
registry: "docker.io" | ||
# -- K8s Cache image repository. | ||
repository: otel/opentelemetry-ebpf-k8s-cache | ||
# -- (string) K8s Cache image tag. When empty, the Chart's appVersion is used. | ||
tag: main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as for main image / repository
@@ -0,0 +1,327 @@ | |||
image: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe also make sense to move config here under opentelemetry-ebpf-instrumentation
(maybe some better/ shorter name) ? since we have two workflows cache and daemonset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i understand what you are suggestion we discussed here its better to seperate them
Co-authored-by: Tyler Helmuth <[email protected]>
Helm chart for opentelemetry-ebpf-instrumentation, the helm chart is based on the original beyla helm chart
issue