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

CI: add CI for runtime tests #338

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

CI: add CI for runtime tests #338

wants to merge 28 commits into from

Conversation

yunwei37
Copy link
Member

Description

Fixes #332

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

@hp77-creator
Copy link
Contributor

@yunwei37 can we also add macOS machine tests in this?
Also for the code coverage, I have shared a secret with you in personal chat, please add those then I can configure codecov

@pull-request-size pull-request-size bot added size/M and removed size/S labels Aug 21, 2024
@yunwei37
Copy link
Member Author

@hp77-creator Can you help fix the build in github actions? We can test in Linux first.

@yunwei37
Copy link
Member Author

Seems there are github actions that can run on MacOS: https://github.com/actions/runner-images/tree/main/images/macos

You can pick one and try to add them.

@yunwei37
Copy link
Member Author

I also create a issue in #340

@hp77-creator
Copy link
Contributor

Shall I add my changes in this PR only @yunwei37 ?

@hp77-creator
Copy link
Contributor

Seems there are github actions that can run on MacOS: https://github.com/actions/runner-images/tree/main/images/macos

You can pick one and try to add them.

I will check these.

@yunwei37
Copy link
Member Author

You can add change to this pr by pushing to the branch. (I think you have access to that?

Thanks!

@hp77-creator
Copy link
Contributor

very strange thing is happening, I tried running the commands that you have used for without-libbpf
It is working fine in both MacOS and Linux on MacOS.
I think this is some dependency from the errors but I see that you have not changed any configuration as such. I will check.

@yunwei37
Copy link
Member Author

Maybe you have installed libbpf locally in Linux? I also get similar results before

@hp77-creator
Copy link
Contributor

Maybe you have installed libbpf locally in Linux? I also get similar results before

yea, so on my VM, there is already libbpf present, so its working fine, on MacOS, that is not required so its not using it. Hence tests are passing on my systems but are failing in CI. I will add the guards where there is use of libbpf

@hp77-creator
Copy link
Contributor

Fedora is working now but Ubuntu is failing because bpf_attr struct doesn't have map_extra in the ubuntu version.
I checked on my system for Ubuntu:22.04 and could see the same, Is there any way we can add the updated <linux/bpf.h> file in ubuntu?

Copy link
Contributor

@hp77-creator hp77-creator left a comment

Choose a reason for hiding this comment

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

LGTM. @Officeyutong can you also check once on the guards that I have added.

@@ -17,7 +17,7 @@ jobs:
- ubuntu-2204
- fedora-39
container:
image: "manjusakalza/bpftime-base-image:${{matrix.container}}"
image: "hp77creator/bpftime-base-image:${{matrix.container}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

What about getting these images to account eunomia-bpf? Do you have permission to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

for the ghci images, will have to configure i guess, For the time being, I focussed more on making the existing setup pass. We can create an issue to migrate these to eunomia ones. Works @Officeyutong ?

Copy link
Contributor

Choose a reason for hiding this comment

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

#341 addresses this. You can add more requirements on the issue if I am missing something.

runtime/syscall-server/syscall_context.cpp Outdated Show resolved Hide resolved
#if __linux__ && !BPFTIME_BUILD_WITH_LIBBPF
#define offsetofend(type, member) (offsetof(type, member) + sizeof(((type *)0)->member))
static void *libc_handle = dlopen(LIBC_SO, RTLD_LAZY);
static auto libc_syscall =
Copy link
Contributor

Choose a reason for hiding this comment

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

Original syscall from libc was already stored at

syscall_fn orig_syscall_fn = nullptr;

Copy link
Contributor

Choose a reason for hiding this comment

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

above definition is to support the bpf_get_obj_by_fd function. I see that there is a place where original syscall is being stored, but it is defined inside a member function. Do you want me to replace libc_handle with syscall_fun ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Where was bpf_get_obj_by_fd called?

Copy link
Contributor

Choose a reason for hiding this comment

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

wrong function at my end. It was actually bpf_obj_get_info_by_fd

Copy link
Contributor

Choose a reason for hiding this comment

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

bpf_obj_get_info_by_fd

bpf_obj_get_info_by_fd is only called by member function of syscall_context, so it's ok to use functions stored in syscall_context

@pull-request-size pull-request-size bot added size/M and removed size/L labels Aug 30, 2024
@hp77-creator
Copy link
Contributor

hp77-creator commented Aug 30, 2024

@yunwei37 @Officeyutong build is failing now becaues of this:
gmake[3]: *** [vm/compat/llvm-vm/llvm-jit/CMakeFiles/llvmbpf_vm.dir/build.make:90: vm/compat/llvm-vm/llvm-jit/CMakeFiles/llvmbpf_vm.dir/src/compiler.cpp.o] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:958: vm/compat/llvm-vm/llvm-jit/CMakeFiles/llvmbpf_vm.dir/all] Error 2
gmake[2]: *** Waiting for unfinished jobs....

any idea?

I have run the exact cmake commands in my fedora VM, its building fine there

@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 30, 2024
@Officeyutong
Copy link
Contributor

Officeyutong commented Aug 30, 2024

@yunwei37 @Officeyutong build is failing now becaues of this: gmake[3]: *** [vm/compat/llvm-vm/llvm-jit/CMakeFiles/llvmbpf_vm.dir/build.make:90: vm/compat/llvm-vm/llvm-jit/CMakeFiles/llvmbpf_vm.dir/src/compiler.cpp.o] Error 1 gmake[2]: *** [CMakeFiles/Makefile2:958: vm/compat/llvm-vm/llvm-jit/CMakeFiles/llvmbpf_vm.dir/all] Error 2 gmake[2]: *** Waiting for unfinished jobs....

any idea?

I have run the exact cmake commands in my fedora VM, its building fine there

Quote reply

It fails because of another recent PR, which changed the definition of struct ebpf_inst, the exact error is at https://github.com/eunomia-bpf/bpftime/actions/runs/10626245841/job/29457492213?pr=338#step:4:336

Field off was renamed to offset by eunomia-bpf/llvmbpf#9

You may need to update submodule, since llvmbpf is now in a submodule

@hp77-creator
Copy link
Contributor

Now more things are breaking, somewhere code is being used instead of opcode, somewhere some virtual are used in place of optional, Which version to upgrade it too? @yunwei37 @Officeyutong

@yunwei37
Copy link
Member Author

No worries, I will help fix that

@hp77-creator
Copy link
Contributor

No worries, I will help fix that

@yunwei37 for that CI codecov, shall I raise one or will this be fixed before?

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@0d8402e). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #338   +/-   ##
=========================================
  Coverage          ?   97.34%           
=========================================
  Files             ?       10           
  Lines             ?      377           
  Branches          ?       12           
=========================================
  Hits              ?      367           
  Misses            ?       10           
  Partials          ?        0           
Flag Coverage Δ
attach tests (uprobe & syscall trace) 97.34% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yunwei37 and others added 8 commits September 30, 2024 18:24
* add xdp-counter example

* add build in ci
* fix link dl and cmake vesion
* add dev container
* docs: include PATH in the README
* bump upload to v4
* fix hidden files
* feat:add lcov support for test

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

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

Successfully merging this pull request may close these issues.

[FEATURE] Add runtime options in CI
4 participants