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

Added support for external BTF #320

Closed
wants to merge 0 commits into from

Conversation

havefuncoding1
Copy link

  1. Introduced new command line argument '--btfpath' to support external BTF so the tool can be used on system without vmlinx. External BTFs are available here.
  2. Updated kernel version check for biolatency. Without it, in my environment running 5.10.178 kernel, the exporter runtime fails with 'Invalid argument'. @ntnx-aleksa just wanted to bring this to your awareness since you made initial update in commit 5511eae. Hope changes here don't break anything.

@havefuncoding1
Copy link
Author

Hi @bobrik I noticed there is no reviewer automatically assigned to open PRs. Is there a way to get someone to review this? Thanks.

@bobrik
Copy link
Contributor

bobrik commented Nov 15, 2023

Introduced new command line argument '--btfpath' to support external BTF so the tool can be used on system without vmlinx.

Do you plan on using this? If it unlocks older kernels, you might want to add some docs into README.md.

Updated kernel version check for biolatency. Without it, in my environment running 5.10.178 kernel, the exporter runtime fails with 'Invalid argument'.

Could you open a separate PR for this?

Is there a way to get someone to review this?

I get notified for everything, but I'm also short on time. I review when I get a chance.

exporter/exporter.go Outdated Show resolved Hide resolved
exporter/exporter.go Outdated Show resolved Hide resolved
exporter/exporter.go Outdated Show resolved Hide resolved
@havefuncoding1
Copy link
Author

Thanks a lot @bobrik for reviewing this. Made all changes per your comments and will send a separate PR for kernel version update in biolatency.btf.c.

@bobrik bobrik changed the title Added support for external BTF; Updated kernel version check in biolatency Added support for external BTF Nov 16, 2023
@bobrik
Copy link
Contributor

bobrik commented Nov 16, 2023

I'm still interested in the answer to this:

Do you plan on using this?

@havefuncoding1
Copy link
Author

havefuncoding1 commented Nov 16, 2023

Yes I will have to use it for my target environments. Otherwise, I wouldn't have implemented the solution.

@bobrik
Copy link
Contributor

bobrik commented Nov 16, 2023

Please squash the commits into one.

@havefuncoding1
Copy link
Author

Done. Thanks a lot for reviewing this @bobrik

@bobrik
Copy link
Contributor

bobrik commented Nov 16, 2023

Could you remove protections your master branch in your fork? I have a few small changes that I want to add before merging, but I can't push them:

To github.com:havefuncoding1/ebpf_exporter.git
 ! [remote rejected] havefuncoding1/master -> havefuncoding1/master (permission denied)
error: failed to push some refs to 'github.com:havefuncoding1/ebpf_exporter.git'

@havefuncoding1
Copy link
Author

I did a quick search to remove repo protection. The results all points to configure protection rules which I have none. Not sure how to best proceed :(. If you have a pointer, it will be really great. Otherwise, I added you as a collaborator so you can push changes. Can't assign you write access yet as invitation is pending. Another option is to share the changes with me, and I can push them to the PR. I am fine with whichever method you think is most convenient. Apology for lack of github experience.

@bobrik
Copy link
Contributor

bobrik commented Nov 17, 2023

Somehow I messed it up, so I opened #323 as a replacement.

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.

2 participants