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

feat: grant ebpf k8s collector job watching permission. #1346

Conversation

hyfj44255
Copy link

@hyfj44255 hyfj44255 commented Sep 15, 2024

Description

opentelemetry-network added k8s collector handle pods owned by a CronJob feature so we need to grant EBPF k8s collector job watching permission in k8s-collector-clusterrole.

Fixes # (issue)

#1349

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Go to open-telemetry-opentelemetry-helm-chartsrepo 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

Go to EBPF chart

cd charts/opentelemetry-ebpf

Run helm install to install the EBPF chart

helm install ebpf-chart-test . -f ci/requests-limits-values.yaml -n ebpf-chart-test

Run command to get ebpf k8s collector container - k8s-watcher's log, and search 'jobs.batch is forbidden' , if not able to search any error related to "not able to list job" means this PR's changing is working

kubectl logs `kubectl get pod -n ebpf-chart-test |grep ebpf-chart-test-opentelemetry-ebpf-k8s-collector | \
awk -F ' ' '{print $1}'` -n ebpf-chart-test -c k8s-watcher | \
grep 'jobs.batch is forbidden'

@hyfj44255
Copy link
Author

Hi @TylerHelmuth could you take a look this PR when you get chance ,it's related to adjust the helm chart according to the change of Opentelemetry eBPF. #1349. BTW the I'm working on fixing the CI failure

@hyfj44255 hyfj44255 requested a review from a team as a code owner September 20, 2024 07:12
@kayhern kayhern force-pushed the feature/grant-ebpf-k8s-collector-job-watching-permission branch from 3a1734b to 323ff85 Compare September 20, 2024 13:28
@hyfj44255
Copy link
Author

Hi @TylerHelmuth the previous lint -test issue was fixed would it be okay if you help trigger the lint-test again?

@hyfj44255
Copy link
Author

Hi @nicolastakashi I was wonder if you could take a look this PR, sorry that it contains some other change from my other PR, I will fix it latter, but would it be okay if you take a look it in advance? thanks!

@@ -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.

I think we should use same idea I mentioned on #1347
let's make it a different test file

@kayhern kayhern force-pushed the feature/grant-ebpf-k8s-collector-job-watching-permission branch from 0c45ad5 to ac63121 Compare September 24, 2024 17:58
@hyfj44255
Copy link
Author

Hi @nicolastakashi I've got rid of the changes not related to the issue this PR is trying to fix , I was wondering if you could take a look again, thanks!

@nicolastakashi
Copy link
Contributor

LGTM 🥳
Although I don't have merge permissions.
@tylerbenson can you help merging it, if it's looking good for you?

@TylerHelmuth
Copy link
Member

@hyfj44255 I merged the other PR, can you check if this needs a rebase? I dont have permission to update your branch with the latest from main

@hyfj44255
Copy link
Author

Hi @TylerHelmuth I've updated this branch with the latest from main, would it be okay if you take a look again?

@TylerHelmuth TylerHelmuth merged commit b7407c4 into open-telemetry:main Sep 26, 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