Miscellaneous fixes to allow running integration tests in bpf-linker #641

tamird · 2023-07-09T23:08:00Z

See individual commits and aya-rs/bpf-linker#69.

netlify · 2023-07-09T23:08:06Z

✅ Deploy Preview for aya-rs-docs ready!

Name	Link
🔨 Latest commit	`b8252f4`
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/64ac69048267c1000899b6b8
😎 Deploy Preview	https://deploy-preview-641--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tamird · 2023-07-09T23:50:34Z

cc @ajwerner

ajwerner

LGTM

ajwerner

Reviewed 16 of 29 files at r4, all commit messages.
Reviewable status: 16 of 30 files reviewed, all discussions resolved (waiting on @alessandrod)

test/integration-test/tests/btf_relocations.rs

xtask/src/build_ebpf.rs

xtask/src/build_test.rs

xtask/src/docs/mod.rs

test/integration-test/tests/smoke.rs

dave-tucker

Some things to clarify here before merge

dave-tucker · 2023-07-10T18:14:44Z

aya/Cargo.toml

 tokio = { version = "1.24.0", features = ["macros", "rt", "rt-multi-thread", "net"], optional = true }
-async-io = { version = "1.3", optional = true }
-log = "0.4"
+procfs = "0.15.1"


Suggested change

procfs = "0.15.1"

procfs = { version: "0.15.1", default-features = false }

This avoids pulling in chrono as a dep.

Also how well maintained is this crate? and why should we switch from what we have.
Adding this to the commit message would be awesome.

Done. Looks pretty active to me.

dave-tucker · 2023-07-10T18:26:39Z

aya/src/programs/mod.rs

@@ -575,10 +575,14 @@ fn load_program<T: Link>(

    let target_kernel_version = match *kernel_version {
        KernelVersion::Any => {
-            let (major, minor, patch) = crate::sys::kernel_version().unwrap();
-            (major << 16) + (minor << 8) + patch
+            let procfs::KernelVersion {


It's a bit gross having an aya KernelVersion and procfs::KernelVersion here.
Do we need both still?

Done, replaced the aya KernelVersion with Option

dave-tucker · 2023-07-10T18:33:59Z

test/integration-ebpf/src/pass.rs

@@ -3,6 +3,8 @@

 use aya_bpf::{bindings::xdp_action, macros::xdp, programs::XdpContext};

+// Note: the `frags` attribute causes this probe to be incompatible with kernel versions < 5.18.0.


This is a bug.
This test should pass on older kernels too - we shouldn't set the flag for frags supported if isn't supported by the target kernel.

Sure, please fix in another PR.

dave-tucker · 2023-07-10T18:34:33Z

test/integration-test/tests/load.rs

@@ -168,8 +168,8 @@ fn pin_link() {
 #[test]
 fn pin_lifecycle() {
    let kernel_version = KernelVersion::current().unwrap();
-    if kernel_version < KernelVersion::new(5, 9, 0) {
-        eprintln!("skipping test on kernel {kernel_version:?}, XDP uses netlink");
+    if kernel_version < KernelVersion::new(5, 18, 0) {


This test has nothing to do with those flags - it's just an unfortunate property of sharing the same bytecode

Yes, I realize, but I chose not to fix this here. Please fix it in another PR.

dave-tucker · 2023-07-10T18:34:54Z

test/integration-test/tests/smoke.rs

@@ -8,6 +8,12 @@ use aya::{

 #[test]
 fn xdp() {
+    let kernel_version = KernelVersion::current().unwrap();
+    if kernel_version < KernelVersion::new(5, 18, 0) {
+        eprintln!("skipping test on kernel {kernel_version:?}, support for BPF_F_XDP_HAS_FRAGS was added in 5.18.0; see https://github.com/torvalds/linux/commit/c2f2cdb");


As above ☝️

dave-tucker · 2023-07-10T18:41:26Z

aya/src/bpf.rs

-    let ret = retry_with_verifier_logs(10, &mut logger, |logger| {
-        bpf_load_btf(raw_btf.as_slice(), logger)
+    let (ret, verifier_log) = retry_with_verifier_logs(10, |logger| {
+        bpf_load_btf(raw_btf.as_slice(), logger, VerifierLogLevel::default())


This should come from the BpfLoader instance, assuming default() here would override someone who's explicitly disabled verifier logs.

Good point, done.

dave-tucker · 2023-07-10T18:41:34Z

aya/src/programs/mod.rs

@@ -474,7 +475,7 @@ impl<T: Link> ProgramData<T> {
            attach_btf_id,
            attach_prog_fd: None,
            btf_fd: None,
-            verifier_log_level: 0,
+            verifier_log_level: VerifierLogLevel::default(),


This should come from the BpfLoader instance, assuming default() here would override someone who's explicitly disabled verifier logs.

There's no BpfLoader in hand here. Where do you suggest I get one?

I would suggest plumbing in the verifier_log_level: VerifierLogLevel as an argument to this function...
If BpfLoader is never available in the call stack, then setting the log level should probably be part of the public API.

The option that's less work is to keep the existing behaviour and use VerifierLogLevel::None

Plumbed it all the way to from_pin, which is a public API. There's no loader object in sight.

Would you like me to break the public API?

Ah okay, no need to expose that in from_pin, You can safely use VerifierLogLevel::None then, not default().
Programs are already loaded to the kernel so you won't get any verifier logs loading from pin in this case.

If it's all the same to you, I'd rather leave default here. If there aren't verifier logs to be had, then it doesn't matter. Agreed?

This allows the logic to be shared between aya and the integration tests without exposing additional public API surface.

Don't run `cargo build --verbose`; it's too noisy.

This type is really only used by one function.

dave-tucker

LGTM

tamird force-pushed the logger-messages-plz branch 3 times, most recently from ca4fa44 to 620b2c8 Compare July 9, 2023 23:49

tamird requested a review from alessandrod July 9, 2023 23:50

tamird marked this pull request as ready for review July 9, 2023 23:50

tamird mentioned this pull request Jul 9, 2023

Run aya integration tests aya-rs/bpf-linker#69

Merged

ajwerner approved these changes Jul 10, 2023

View reviewed changes

tamird force-pushed the logger-messages-plz branch from 620b2c8 to c883935 Compare July 10, 2023 13:59

ajwerner approved these changes Jul 10, 2023

View reviewed changes

tamird force-pushed the logger-messages-plz branch 2 times, most recently from 580e6d8 to 5a2d1fa Compare July 10, 2023 14:55

ajwerner reviewed Jul 10, 2023

View reviewed changes

Add missing test annotation

7f25956

vadorovsky reviewed Jul 10, 2023

View reviewed changes

tamird force-pushed the logger-messages-plz branch from 5a2d1fa to 7d50085 Compare July 10, 2023 18:18

vadorovsky approved these changes Jul 10, 2023

View reviewed changes

dave-tucker self-requested a review July 10, 2023 18:25

dave-tucker requested changes Jul 10, 2023

View reviewed changes

tamird added 3 commits July 10, 2023 15:25

Use procfs crate for kernel version parsing

b611038

This allows the logic to be shared between aya and the integration tests without exposing additional public API surface.

xtask: Standardize command logging

b0a4ab5

Don't run `cargo build --verbose`; it's too noisy.

Hide details of VerifierLog

6b94b20

This type is really only used by one function.

tamird force-pushed the logger-messages-plz branch from 7d50085 to cd15fbd Compare July 10, 2023 19:26

tamird added 3 commits July 10, 2023 15:43

Get verifier logs when loading programs

b45a5bb

Skip relocation tests on unsupported kernels

91a415f

Skip BPF_F_XDP_HAS_FRAGS tests on unsupported kernels

b8252f4

tamird force-pushed the logger-messages-plz branch 2 times, most recently from b1bb810 to b8252f4 Compare July 10, 2023 20:24

dave-tucker approved these changes Jul 10, 2023

View reviewed changes

tamird merged commit 4c0983b into main Jul 10, 2023

tamird deleted the logger-messages-plz branch July 10, 2023 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous fixes to allow running integration tests in bpf-linker #641

Miscellaneous fixes to allow running integration tests in bpf-linker #641

tamird commented Jul 9, 2023

netlify bot commented Jul 9, 2023 •

edited

Loading

tamird commented Jul 9, 2023

ajwerner left a comment

ajwerner left a comment

dave-tucker left a comment

dave-tucker Jul 10, 2023

tamird Jul 10, 2023

dave-tucker Jul 10, 2023

tamird Jul 10, 2023

dave-tucker Jul 10, 2023

tamird Jul 10, 2023

dave-tucker Jul 10, 2023

tamird Jul 10, 2023

dave-tucker Jul 10, 2023

tamird Jul 10, 2023

dave-tucker Jul 10, 2023

tamird Jul 10, 2023

dave-tucker Jul 10, 2023

tamird Jul 10, 2023

dave-tucker Jul 10, 2023 •

edited

Loading

tamird Jul 10, 2023

dave-tucker Jul 10, 2023

tamird Jul 10, 2023

dave-tucker left a comment

	procfs = "0.15.1"
	procfs = { version: "0.15.1", default-features = false }

		@@ -3,6 +3,8 @@

		use aya_bpf::{bindings::xdp_action, macros::xdp, programs::XdpContext};

		// Note: the `frags` attribute causes this probe to be incompatible with kernel versions < 5.18.0.

Miscellaneous fixes to allow running integration tests in bpf-linker #641

Miscellaneous fixes to allow running integration tests in bpf-linker #641

Conversation

tamird commented Jul 9, 2023

netlify bot commented Jul 9, 2023 • edited Loading

✅ Deploy Preview for aya-rs-docs ready!

tamird commented Jul 9, 2023

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

dave-tucker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dave-tucker Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dave-tucker left a comment

Choose a reason for hiding this comment

netlify bot commented Jul 9, 2023 •

edited

Loading

dave-tucker Jul 10, 2023 •

edited

Loading