-
Notifications
You must be signed in to change notification settings - Fork 286
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
Implement FdLink conversions #560
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hrm I don't think this error is related:
|
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, we already do this for XDP and I'd forgotten to do this for the PerfLinks 😓
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.
Thanks!
I agree that this should be documented. We could add docs here https://docs.aya-rs.dev/user/aya/programs/links/struct.fdlink? Explain that FdLink allows you to pin, and that many links can be converted to FdLink by using either into() or try_into(), giving examples of both.
9b55e05
to
faba265
Compare
Sorry It's been so long @alessandrod I fixed the issues and added integration testing for @dave-tucker PTAL at how I also changed the integration test tooling to verify that a program is loaded AND linked in certain cases. |
Shoot and I still need to do this. |
6e5d1a5
to
8307435
Compare
done |
Poke @alessandrod :) |
8307435
to
eff8f3b
Compare
Rebased 👍 |
2ecb017
to
0f4cb94
Compare
@tamird and I are working on redoing the integration test infrastructure such that the tests themselves do not rely on userspace to execute. Today the cycle time of compiling and running tests in the VM is less than wonderful. I chatted with @alessandrod about it here. The preferred solution would be to have aya provide the relevant APIs for reading these links. If it's easy to implement the functionality you need in aya as opposed to shelling out, would you be open to it? |
Hiya @ajwerner and yeah I totally agree, we need a I'd rather go ahead and get this bit in for now since I've been carrying it for quite some time 😄 , but in the meantime I'll start working on a basic |
047f302
to
8eef908
Compare
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.
Reviewed 7 of 8 files at r1, all commit messages.
Reviewable status: 7 of 8 files reviewed, 8 unresolved discussions (waiting on @alessandrod, @astoycos, and @dave-tucker)
aya/src/programs/kprobe.rs
line 140 at r1 (raw file):
fn try_from(fd_link: FdLink) -> Result<Self, Self::Error> { // unwrap of fd_link.fd will not panic since it's only None when being dropped.
there's no unwrap here?
aya/src/programs/links.rs
line 89 at r1 (raw file):
/// process closes. Additionally many program specific links can be converted into /// FdLinks by using either into() or try_into() implementations. For examples of such functions /// see [`TracePointLink::try_from()`](crate::programs::trace_point::TracePointLink::try_from()) and [`FdLink::try_from()`].
nit: long line
aya/src/programs/perf_event.rs
line 209 at r1 (raw file):
fn try_from(fd_link: FdLink) -> Result<Self, Self::Error> { // unwrap of fd_link.fd will not panic since it's only None when being dropped.
ditto?
aya/src/programs/trace_point.rs
line 135 at r1 (raw file):
fn try_from(fd_link: FdLink) -> Result<Self, Self::Error> { // unwrap of fd_link.fd will not panic since it's only None when being dropped.
ditto
aya/src/programs/uprobe.rs
line 180 at r1 (raw file):
fn try_from(fd_link: FdLink) -> Result<Self, Self::Error> { // unwrap of fd_link.fd will not panic since it's only None when being dropped.
here
test/integration-test/tests/load.rs
line 67 at r1 (raw file):
} macro_rules! assert_loaded_and_linked {
can this just be assert_loaded
followed by assert linked? i don't think we need to retry the bpftool invocation separately from waiting for loadedness
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.
Reviewed 1 of 8 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @alessandrod, @astoycos, and @dave-tucker)
test/integration-ebpf/src/test.rs
line 23 at r1 (raw file):
#[kprobe] // truncated name to match bpftool output
the names were previously truncated to 15 characters, this comment is detritus
70d667f
to
9ce2283
Compare
This is the only thing I didn't get to, simply because these functions are built as macros and if we were to reuse them I'd need to return the program id from |
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.
Reviewed 8 of 8 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @alessandrod, @astoycos, and @dave-tucker)
test/integration-test/tests/load.rs
line 59 at r3 (raw file):
fn is_linked(prog_id: &u32) -> bool { let output = Command::new("bpftool").args(["link"]).output(); let output = match output {
i think this is output.expect(...)
f1c085e
to
f9ff234
Compare
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.
Reviewed 1 of 1 files at r5, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alessandrod and @dave-tucker)
Implement TryFrom functions to convert Fdlink to PerfLink/TracePointLink/ KprobeLink, and UprobeLink and vice-versa. This allows us to pin taken links for perf programs, ultimately ensuring the link isn't dropped when the loading process exits. Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
We need bpftool to add tests for the link APIs since we don't yet have and aya API for listing links. Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
Add integration testing for link pinning and loading/unloading of tracepoint, kprobe, and uprobe programs. Redo how we utilize bpftool to verify that programs are loaded to be explicit with names. Also add a helper to verify that a program is loaded AND linked. Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
in the integration tests we recenctly switched to using our internal api to list programs. I was seeing times when this would race and panic internally (program fd was deleted by aya WHILE we were trying to get it). This ensures that the list succeeded without panicking. Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
f9ff234
to
94f554a
Compare
Whelp ended up having to rebase again :/ |
Implement TryFrom functions to convert Fdlink to PerfLink/TracePointLink/ KprobeLink, and UprobeLink and vice-versa.
This allows us to pin taken links for perf programs, ultimately ensuring the link isn't dropped when the loading process exits.
Required for bpfman/bpfman#306
TODO: It'd be nice to add some documentation around this API somewhere...