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

fix: EBPF_NET_CRASH_METRIC_PORT container env in EBPF chart not being quoted issue. #1347

Merged

Conversation

hyfj44255
Copy link
Contributor

@hyfj44255 hyfj44255 commented Sep 15, 2024

Description

Currently the containers' EBPF_NET_CRASH_METRIC_PORT env in EBPF chart are not quoted, which will lead EBPF installation failure

Fixes # (issue)
#1348

Type of change

  • bug fix (non-breaking change which adds functionality)

How Has This Been Tested?

Go to repo root directory.

cd opentelemetry-helm-charts

Create a kind cluster same as the one in repo's Github action

kind create cluster --name ebpf-cluster --config ./.github/kind-1.24.yaml

Run CT to preform the test

ct install --charts charts/opentelemetry-ebpf.

@hyfj44255
Copy link
Contributor Author

Hi @nicolastakashi would it be okay if you take a look at this pr when you got chance?

@hyfj44255
Copy link
Contributor Author

Hi @TylerHelmuth could you take a look this PR ? it's related to containers' env EBPF_NET_CRASH_METRIC_PORT in EBPF chart are not quoted, which will lead eBPF installation failure #1348

@hyfj44255 hyfj44255 requested a review from a team as a code owner September 20, 2024 07:12
@@ -10,4 +10,4 @@ log:
debug:
enabled: true
storeMinidump: true
sendUnplannedExitMetric: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we revert it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@hyfj44255 I was wondering if it should be another ci testing, since this one is related to debug logs.

Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @nicolastakashi let me revert it and create another -values.yaml file for testing it

@nicolastakashi
Copy link
Contributor

@hyfj44255 minor comment, besides that LGTM

@hyfj44255
Copy link
Contributor Author

@hyfj44255 minor comment, besides that LGTM

Hi@nicolastakashi thank you so much for your reivew, IMHO, we might want to set it to true since we can test out whether containers' env EBPF_NET_CRASHL_METRIC_PORT has been quoted

@hyfj44255
Copy link
Contributor Author

Hi @nicolastakashi I added a other *-values.yaml file to test containers env quote issue , I was wondering if you could take a look charts/opentelemetry-ebpf/ci/send-unplanned-exit-metric-values.yaml

@nicolastakashi
Copy link
Contributor

@TylerHelmuth can you help us merging this, if it's looking good for you?

@TylerHelmuth TylerHelmuth merged commit 623c987 into open-telemetry:main Sep 25, 2024
3 checks passed
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.

3 participants