Tcx #921
Tcx #921
Conversation
|
cc @dave-tucker For some eyes |
✅ Deploy Preview for aya-rs-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
astoycos
force-pushed
the
tcx
branch
6 times, most recently
from
73ad791
to
43b67e4
Compare
|
Hey @alessandrod, this pull request changes the Aya Public API and requires your review. |
astoycos
force-pushed
the
tcx
branch
3 times, most recently
from
bfe585e
to
fc9f947
Compare
Thanks so much for doing all this work!
This seems fine, except the current definition isn't type safe: This shouldn't be possible. We should try to make the API as type safe as possible. Among other things we should probably have a wrapper type for program ids, so that
This also seems fine
I don't think we can do this without giving up on a lot of type safety. If you have only one Link type, then all operations become possilble on all links: xdp operations on tc links, kprobe operations on xdp links, etc. But if you have something in mind that would work I'd love to see it!
As an user of aya, I probably don't know anything about netlink or TCX and I shouldn't really learn anything about it. I think we should make this as transparent as possible to users. Like we do for xdp, we should probably detect whether we can attach as TCX or fallback to netlink, and in the default/common case, have sensible defaults for LinkOrdering and everything else. |
|
Hiya @alessandrod i'm FINALLY getting back to this :) Everything looks good on your comments above except I do have some questions on...
So the main problem with making this truly "transparent" is that the tcx requires new return codes https://github.com/torvalds/linux/blob/master/include/uapi/linux/bpf.h#L6413 /* (Simplified) user return codes for tcx prog type.
* A valid tcx program must return one of these defined values. All other
* return codes are reserved for future use. Must remain compatible with
* their TC_ACT_* counter-parts. For compatibility in behavior, unknown
* return codes are mapped to TCX_NEXT.
*/
enum tcx_action_base {
TCX_NEXT = -1,
TCX_PASS = 0,
TCX_DROP = 2,
TCX_REDIRECT = 7,
};vs #define TC_ACT_UNSPEC (-1)
#define TC_ACT_OK 0
#define TC_ACT_RECLASSIFY 1
#define TC_ACT_SHOT 2
#define TC_ACT_PIPE 3
#define TC_ACT_STOLEN 4
#define TC_ACT_QUEUED 5
#define TC_ACT_REPEAT 6
#define TC_ACT_REDIRECT 7
#define TC_ACT_TRAP 8 /* For hw path, this means "trap to cpu"
* and don't further process the frame
* in hardware. For sw path, this is
* equivalent of TC_ACT_STOLEN - drop
* the skb and act like everything
* is alright.
*/
#define TC_ACT_VALUE_MAX TC_ACT_TRAPSo therefore technically the bytecode AND the attach mechanism has to change for TCX to work properly... However this does seem to be backwards compatible, i.e the old return codes will still work as expected so we should be implement "Use TCX if available, if not fall back to netlink" Let me take a stab |
astoycos
force-pushed
the
tcx
branch
7 times, most recently
from
b5d4989
to
1b9fd57
Compare
|
@alessandrod Can you PTAL another look here? I changed up the API a bit making it much more type safe :) LMK what you think and if you're alright with it I'll continue by adding some integration tests before marking as ready for review |
There was a problem hiding this comment.
Reviewed 2 of 7 files at r8, 16 of 16 files at r12, all commit messages.
Dismissed @anfredette from a discussion.
Reviewable status: 22 of 23 files reviewed, 43 unresolved discussions (waiting on @alessandrod, @anfredette, @astoycos, and @dave-tucker)
aya/src/programs/links.rs line 407 at r1 (raw file):
Previously, anfredette (Andre Fredette) wrote…
bpfman doesn't need it in our first implementation, but it may be needed in the future. I'm okay taking it out for now and adding it back when there's a requirement.
OK. please do.
aya/src/programs/mod.rs line 225 at r12 (raw file):
ConversionError { /// The type being converted from. from: String,
this should definitely not be an owned string.
aya/src/programs/tc.rs line 275 at r12 (raw file):
.try_into() .map_err(|_| ProgramError::ConversionError { from: "u32".to_string(),
yikes. can we put the values here instead? also, why would you need owned strings here?
aya/src/sys/bpf.rs line 411 at r12 (raw file):
attr.link_create.flags = flags; // since kernel 6.6
you have orphaned this comment
test/integration-test/src/tests/tcx.rs line 148 at r6 (raw file):
Previously, anfredette (Andre Fredette) wrote…
Timestamp-based ordering tests have been removed.
Are you intending to replace these tests with anything? Currently we have way more complexity in the BPF side than on the userspace side.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r13, all commit messages.
Dismissed @dave-tucker, and @dave-tucker from 32 discussions.
Reviewable status: 22 of 23 files reviewed, 10 unresolved discussions (waiting on @alessandrod, @anfredette, @astoycos, and @dave-tucker)
aya/src/programs/tc.rs line 272 at r13 (raw file):
let (priority, handle) = unsafe { netlink_qdisc_attach( if_index.try_into().map_err(ProgramError::TryFromIntError)?,
this map_err should no longer be needed (because you have #[from] in the error variant)
aya/src/programs/tc.rs line 367 at r13 (raw file):
.if_index .try_into() .map_err(ProgramError::TryFromIntError)?;
this map_err should no longer be needed (because you have #[from] in the error variant)
There was a problem hiding this comment.
Reviewable status: 22 of 23 files reviewed, 10 unresolved discussions (waiting on @alessandrod, @astoycos, @dave-tucker, and @tamird)
aya/src/programs/links.rs line 407 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
OK. please do.
Done.
aya/src/programs/mod.rs line 225 at r12 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this should definitely not be an owned string.
I didn't like this either. I changed it to use #[error(transparent)] like many of the other ProgramErrors.
aya/src/programs/tc.rs line 275 at r12 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
yikes. can we put the values here instead? also, why would you need owned strings here?
I changed it as described on your comment on the error definition.
aya/src/sys/bpf.rs line 411 at r12 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
you have orphaned this comment
It applies to the LinkRef's below too, so I left it.
test/integration-test/src/tests/tcx.rs line 148 at r6 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Are you intending to replace these tests with anything? Currently we have way more complexity in the BPF side than on the userspace side.
Andrew was using userspace logging to try to test the BPF side, so we'll need to come up with a different approach. I wasn't planning to add it to this pr though. I was planning to use the new API for BPF_PROG_QUERY that we've discussed to validate that the right programs were installed in the right order.
Once I added a small delay to Andrews test after the programs were attached and before the test was run, the test was solid both upstream and on my local machine. I could add that back if you want.
|
FYI, I've kept my last few commits separate until I have some consensus in case I need to back them out. I'll squash them after we converge a little more. |
There was a problem hiding this comment.
Reviewable status: 22 of 23 files reviewed, 9 unresolved discussions (waiting on @alessandrod, @anfredette, @astoycos, and @dave-tucker)
test/integration-test/src/tests/tcx.rs line 148 at r6 (raw file):
Previously, anfredette (Andre Fredette) wrote…
Andrew was using userspace logging to try to test the BPF side, so we'll need to come up with a different approach. I wasn't planning to add it to this pr though. I was planning to use the new API for BPF_PROG_QUERY that we've discussed to validate that the right programs were installed in the right order.
Once I added a small delay to Andrews test after the programs were attached and before the test was run, the test was solid both upstream and on my local machine. I could add that back if you want.
The current state is unsatisfying:
- There is significant untested API surface and there is no documentation (i.e. TODOs) of that fact
- There is significant complexity in the test BPF programs that is not used
Could you resolve both please? The solution to the latter is to strip those test programs down to the minimum needed to run this test.
There was a problem hiding this comment.
Reviewable status: 22 of 23 files reviewed, 9 unresolved discussions (waiting on @alessandrod, @astoycos, and @dave-tucker)
test/integration-test/src/tests/tcx.rs line 148 at r6 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
The current state is unsatisfying:
- There is significant untested API surface and there is no documentation (i.e. TODOs) of that fact
- There is significant complexity in the test BPF programs that is not used
Could you resolve both please? The solution to the latter is to strip those test programs down to the minimum needed to run this test.
Will do.
- Removed LinkId and associated constructor functions - Changed ConversionError to a TryFromError - Remove map_err() from if_index try_into() calls Signed-off-by: Andre Fredette <afredette@redhat.com>
|
Issue #1032 can be assigned to me if someone wants to do it. I don't seem to be able to assign it to myself. |
There was a problem hiding this comment.
Reviewable status: 18 of 23 files reviewed, 8 unresolved discussions (waiting on @alessandrod, @astoycos, @dave-tucker, and @tamird)
aya/src/programs/tc.rs line 272 at r13 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this map_err should no longer be needed (because you have
#[from]in the error variant)
Removed.
aya/src/programs/tc.rs line 367 at r13 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this map_err should no longer be needed (because you have
#[from]in the error variant)
Removed.
test/integration-test/src/tests/tcx.rs line 148 at r6 (raw file):
Previously, anfredette (Andre Fredette) wrote…
Will do.
Done. See latest commit & issue #1032.
There was a problem hiding this comment.
Reviewed 4 of 5 files at r15, all commit messages.
Reviewable status: 22 of 23 files reviewed, 10 unresolved discussions (waiting on @alessandrod, @astoycos, and @dave-tucker)
test/integration-ebpf/src/tcx.rs line 18 at r15 (raw file):
} fn try_tcxtest(_ctx: TcContext) -> Result<i32, u32> {
what is the point of this? can it just be inline?
test/integration-test/src/tests/tcx.rs line 23 at r15 (raw file):
let _netns = NetNsGuard::new(); // Create 9 programs for testing the 9 different LinkOrder constructors.
don't repeat code in comments
test/integration-test/src/tests/tcx.rs line 26 at r15 (raw file):
let mut programs: Vec<Ebpf> = vec![]; for _ in 0..9 { let program = EbpfLoader::new().load(crate::TCX).unwrap();
nit: create the loader just once
test/integration-test/src/tests/tcx.rs line 30 at r15 (raw file):
} let mut tcx_programs: Vec<&mut SchedClassifier> = vec![];
instead of these collections could you write a macro to do the load and conversion? all the indexing here makes it rather fragile
test/integration-test/src/tests/tcx.rs line 137 at r15 (raw file):
// on the BPF_PROG_QUERY syscall is implemented. // Detach all links
why is this careful teardown required?
Also removed unused code in the tcx eBPF program. I've manually verified that all the programs get attached in the correct order. TODO: Add code to the integration test to automatically verify that the programs are attached in the correct order after the API based on the BPF_PROG_QUERY syscall has been implemented. Signed-off-by: Andre Fredette <afredette@redhat.com>
There was a problem hiding this comment.
Reviewable status: 21 of 24 files reviewed, 10 unresolved discussions (waiting on @alessandrod, @astoycos, @dave-tucker, and @tamird)
test/integration-ebpf/src/tcx.rs line 18 at r15 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
what is the point of this? can it just be inline?
Yes it can. I thought about doing it that way but decided to follow the pattern from the xdp pass.rs program which seems to be common.
However, I changed it as suggested.
test/integration-test/src/tests/tcx.rs line 23 at r15 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
don't repeat code in comments
Done
test/integration-test/src/tests/tcx.rs line 26 at r15 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
nit: create the loader just once
In general, I designed the test case this way to replicate a common use case in which multiple different user applications each need to attach a TCX program to the same attach point. Each applicaiton would have it's own loader, attach it's own program, and often detach/unload when the program is no longer needed due to a change in config or as part of cleanup when uninstalled. This is also a use case when applications use bpfman. However, I talked it over with @dave-tucker and we decided to strip it down as you have suggested here. That said, we still need multiple loaders because TCX does not allow the same program ID to be attached multiple times to the same interface/direction.
test/integration-test/src/tests/tcx.rs line 30 at r15 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
instead of these collections could you write a macro to do the load and conversion? all the indexing here makes it rather fragile
Done
test/integration-test/src/tests/tcx.rs line 137 at r15 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why is this careful teardown required?
Reason given above, but it's gone now.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r17, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @alessandrod, @astoycos, and @dave-tucker)
macro.patch line 0 at r17 (raw file):
what's this?
test/integration-test/src/tests/tcx.rs line 28 at r17 (raw file):
// since TCX does not allow the same program ID to be attached multiple // times to the same interface/direction. let mut attached_programs: HashMap<&str, (Ebpf, SchedClassifierLink)> = HashMap::new();
I think there's a misunderstanding. Why do you need this hash map? Could you write the macro like so:
let loader = EbpfLoader::new();
macro_rules! attach_program_with_linkorder {
($name:literal,$link_order:expr) => {{
let mut ebpf = loader.load(crate::TCX).unwrap();
let program: &mut SchedClassifier =
ebpf.program_mut("tcx_next").unwrap().try_into().unwrap();
program.load().unwrap();
let options = TcAttachOptions::TcxOrder($link_order);
let link_id = program
.attach_with_options("lo", TcAttachType::Ingress, options)
.unwrap();
let link = program.take_link(link_id).unwrap();
let name = (program, link);
}};
}Then every time you use this macro you end up with a variable called {name} on the stack that you can use directly, without this hash map.
There was a problem hiding this comment.
Andre is out for a few days but has asked me to carry this. I've updated the patchset with another commit to address outstanding comments. Once we're okay with the code and API, I'll go ahead and squash the history as it's getting pretty wild.
Reviewable status: 18 of 25 files reviewed, 7 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)
test/integration-test/src/tests/tcx.rs line 28 at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I think there's a misunderstanding. Why do you need this hash map? Could you write the macro like so:
let loader = EbpfLoader::new(); macro_rules! attach_program_with_linkorder { ($name:literal,$link_order:expr) => {{ let mut ebpf = loader.load(crate::TCX).unwrap(); let program: &mut SchedClassifier = ebpf.program_mut("tcx_next").unwrap().try_into().unwrap(); program.load().unwrap(); let options = TcAttachOptions::TcxOrder($link_order); let link_id = program .attach_with_options("lo", TcAttachType::Ingress, options) .unwrap(); let link = program.take_link(link_id).unwrap(); let name = (program, link); }}; }Then every time you use this macro you end up with a variable called
{name}on the stack that you can use directly, without this hash map.
Adjusted the macro to remove the hashmap (this was previously being used to keep both the loader and link in scope so they weren't dropped). The new implementation yields a tuple of (Ebpf, LinkId) which is much nicer to work with. In addition, I went ahead and made the necessary API changes for us to actually assert on the observed order since this was pretty small... and avoided more hacks to ensure these variables weren't dropped (which would trigger detach/unload).
macro.patch line at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
what's this?
mistakenly checked in. now removed.
There was a problem hiding this comment.
Reviewed 5 of 7 files at r18, all commit messages.
Reviewable status: 23 of 25 files reviewed, 14 unresolved discussions (waiting on @alessandrod and @astoycos)
aya/src/programs/mod.rs line 697 at r18 (raw file):
attach_flags: &mut Option<u32>, ) -> Result<(u64, Vec<u32>), ProgramError> { assert!(target_fd.is_none() || target_ifindex.is_none());
please do this at compile time
aya/src/programs/mod.rs line 705 at r18 (raw file):
let target_ifindex = target_ifindex.map(|name| { let c_interface = CString::new(name).unwrap();
this unwrap is unfortunate. see other comment about passing in CStr
aya/src/programs/tc.rs line 360 at r18 (raw file):
/// ``` pub fn query_tcx( interface: &str,
should it take a CStr to expose the fact that it can't contain nul bytes?
aya/src/programs/tc.rs line 363 at r18 (raw file):
attach_type: TcAttachType, ) -> Result<(u64, Vec<ProgramInfo>), ProgramError> { let prog_ids = query(
let (foo, bar) = ... to avoid .1 and .0 below
aya/src/programs/tc.rs line 379 at r18 (raw file):
Ok(prog_info) }) .collect();
nit: collect::<Result<_, _>>()? to avoid doing the error check below
test/integration-test/src/tests/tcx.rs line 27 at r18 (raw file):
// variable name conflicts. // // Yields a tuple of the `EbpfLoader` and the `SchedClassifierLink` as both
no it doesn't
Code quote:
EbpfLoader`test/integration-test/src/tests/tcx.rs line 31 at r18 (raw file):
macro_rules! attach_program_with_linkorder { ($link_order:expr) => {{ let mut loader = EbpfLoader::new().load(crate::TCX).unwrap();
Surely we can extract EbpfLoader::new()? this loader is also not a loader at all, it's a Ebpf, so this name is misleading.
test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):
) .unwrap(); (loader, link_id)
why not yield out program as well? that would remove the need for
let default_prog: &SchedClassifier = default.program("tcx_next").unwrap().try_into().unwrap();
below
test/integration-test/src/tests/tcx.rs line 118 at r18 (raw file):
.zip(expected_order.iter()) .for_each(|(got, expected)| { assert_eq!(
prefer to do the assertion on the outside so failures produce more information - as written this will stop after the first mismatch.
There was a problem hiding this comment.
Reviewable status: 23 of 25 files reviewed, 14 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)
aya/src/programs/mod.rs line 697 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
please do this at compile time
do you have a suggestion on how to do that?
the situation is the same with aya:sys::bpf_prog_query also.
the only way to ensure compile time safety I can see would be to specialize by
splitting this into bpf_prog_query_by_fd and bpf_prog_query_by_ifindex.
i decided against it as the implementation was 99% identical in both functions...
aya/src/programs/mod.rs line 705 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this unwrap is unfortunate. see other comment about passing in CStr
ack. i'll make query (or query_by_ifindex pending resolution of the other comment) take a u32 to avoid the conversion here. conversion can be handled by the caller in the same way as it is for SchedClassifier::attach.
aya/src/programs/tc.rs line 360 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
should it take a CStr to expose the fact that it can't contain nul bytes?
no as this wouldn't be consistent with other Aya APIs that take an interface as a parameter. For example, aya::programs::SchedClassifier::attach
There was a problem hiding this comment.
Reviewable status: 23 of 25 files reviewed, 14 unresolved discussions (waiting on @alessandrod, @astoycos, and @dave-tucker)
aya/src/programs/mod.rs line 697 at r18 (raw file):
Previously, dave-tucker (Dave Tucker) wrote…
do you have a suggestion on how to do that?
the situation is the same withaya:sys::bpf_prog_queryalso.the only way to ensure compile time safety I can see would be to specialize by
splitting this intobpf_prog_query_by_fdandbpf_prog_query_by_ifindex.i decided against it as the implementation was 99% identical in both functions...
I was thinking an enum (none, FD, if index)
dave-tucker
force-pushed
the
tcx
branch
2 times, most recently
from
920167c
to
a66e2d9
Compare
There was a problem hiding this comment.
Reviewable status: 19 of 25 files reviewed, 13 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)
aya/src/programs/mod.rs line 697 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I was thinking an enum (none, FD, if index)
Done
aya/src/programs/mod.rs line 705 at r18 (raw file):
Previously, dave-tucker (Dave Tucker) wrote…
ack. i'll make
query(orquery_by_ifindexpending resolution of the other comment) take a u32 to avoid the conversion here. conversion can be handled by the caller in the same way as it is for SchedClassifier::attach.
Done
aya/src/programs/tc.rs line 360 at r18 (raw file):
Previously, dave-tucker (Dave Tucker) wrote…
no as this wouldn't be consistent with other Aya APIs that take an interface as a parameter. For example,
aya::programs::SchedClassifier::attach
Done
aya/src/programs/tc.rs line 363 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
let (foo, bar) = ...to avoid.1and.0below
Done
aya/src/programs/tc.rs line 379 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
nit:
collect::<Result<_, _>>()?to avoid doing the error check below
Done
test/integration-test/src/tests/tcx.rs line 27 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
no it doesn't
Done
test/integration-test/src/tests/tcx.rs line 31 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Surely we can extract
EbpfLoader::new()? thisloaderis also not a loader at all, it's aEbpf, so this name is misleading.
Done
test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why not yield out
programas well? that would remove the need forlet default_prog: &SchedClassifier = default.program("tcx_next").unwrap().try_into().unwrap();below
This isn't possible since program is a borrowed from loader.
test/integration-test/src/tests/tcx.rs line 118 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
prefer to do the assertion on the outside so failures produce more information - as written this will stop after the first mismatch.
Done
There was a problem hiding this comment.
Reviewed 5 of 6 files at r19, 1 of 1 files at r20, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @alessandrod and @astoycos)
aya/src/sys/bpf.rs line 501 at r20 (raw file):
} #[derive(Debug, Clone)]
does it need clone?
test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):
Previously, dave-tucker (Dave Tucker) wrote…
This isn't possible since
programis a borrowed fromloader.
I think you mean it is borrowed from ebpf. Fine. Then can we yield out only program? That seems to be the only thing we actually need.
test/integration-test/src/tests/tcx.rs line 118 at r18 (raw file):
Previously, dave-tucker (Dave Tucker) wrote…
Done
why the string conversion? can't you write
assert_eq!(
got_order.iter().map(|p| p.id()).collect::<Vec<_>>(),
expected_order
);
?
test/integration-test/src/tests/tcx.rs line 111 at r20 (raw file):
let (revision, got_order) = SchedClassifier::query_tcx("lo", TcAttachType::Ingress).unwrap(); assert_eq!(got_order.len(), expected_order.len());
this is a pointless assertion now
test/integration-test/src/tests/tcx.rs line 112 at r20 (raw file):
let (revision, got_order) = SchedClassifier::query_tcx("lo", TcAttachType::Ingress).unwrap(); assert_eq!(got_order.len(), expected_order.len()); assert_eq!(revision, 10);
magic number
This adds SchedClassifier::query_tcx() API that is able to return an ordered list of ProgramInfo (and revision) of an interface that has TCX programs attached. This is useful to verify the ordering of the programs in our integration tests, but also to for aya-rs#1032. Additionally tidies up the macro used for TCX testing and adds assertions using this new API. Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
There was a problem hiding this comment.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @alessandrod, @astoycos, and @tamird)
aya/src/sys/bpf.rs line 501 at r20 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
does it need clone?
Done
test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I think you mean it is borrowed from
ebpf. Fine. Then can we yield out onlyprogram? That seems to be the only thing we actually need.
We still need to yield the link_id as it's used to test the LinkOrder::before_link and LinkOrder::after_link variants.
Granted, we only need one link_id in particular, but I couldn't reason an easier way to do this.
test/integration-test/src/tests/tcx.rs line 118 at r18 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why the string conversion? can't you write
assert_eq!( got_order.iter().map(|p| p.id()).collect::<Vec<_>>(), expected_order );?
Done
There was a problem hiding this comment.
Reviewed 2 of 2 files at r21, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alessandrod and @astoycos)
test/integration-test/src/tests/tcx.rs line 42 at r18 (raw file):
Previously, dave-tucker (Dave Tucker) wrote…
We still need to yield the
link_idas it's used to test theLinkOrder::before_linkandLinkOrder::after_linkvariants.
Granted, we only need one link_id in particular, but I couldn't reason an easier way to do this.
Right, but link_id is a scalar, no? Perhaps you took me too literally, I meant "yield program instead of ebpf".
Fixes #918
This initial attempt works with the following example -> https://github.com/astoycos/tcxtest
Please see integration tests for example of how the new API works
This change is