integration-test: Implement running on VMs #638
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. |
|
Probably we should get this into CI. It'll need a way to mark certain tests as not working in this environment or a way to plumb in the necessary dependencies. |
There was a problem hiding this comment.
We should probably subsume the outer wrapper (test.sh) with this.
Reviewed 3 of 3 files at r1, 4 of 6 files at r2, all commit messages.
Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @ajwerner)
kerneltest/.gitkeep line 0 at r2 (raw file):
remove? only need the deepest one
xtask/Cargo.toml line 17 at r2 (raw file):
lazy_static = "1" serde_json = "1" reqwest = { version = "0.11.18", default-features = false, features = [
maybe go all the way and newline every attribute?
foo = {
bar = baz,
features = [
...
],
}
xtask/src/integration_test.rs line 44 at r1 (raw file):
crate::build_ebpf::BuildEbpfOptions { target: *bpf_target, libbpf_dir: PathBuf::from(libbpf_dir),
can we make Options::libbpf_dir a PathBuf?
xtask/src/integration_test.rs line 52 at r1 (raw file):
pub struct BuildOptions { pub release: bool, pub target: Option<String>,
This field is never not None in this commit. Remove?
xtask/src/integration_test.rs line 78 at r1 (raw file):
let bin_path = format!("target/{target_path}{profile}/integration-test"); Ok(PathBuf::from(bin_path))
attempting to live in String and PathBuf at the same time is a recipe for pain. See the fallible conversion you now have to do about 10 lines down.
Either everything is PathBuf (and by implication OsString) or everything is String.
There was a problem hiding this comment.
I'm going to leave subsuming run.sh for later when I understand the world better. We'll need #639 and we'll need to rework the relocation tests to build their probes at compile-time instead of at runtime. I'll tackle that next.
Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @tamird)
kerneltest/.gitkeep line at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
remove? only need the deepest one
Done.
xtask/Cargo.toml line 17 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
maybe go all the way and newline every attribute?
foo = {
bar = baz,
features = [
...
],
}
Seems like that's not legal toml.
xtask/src/integration_test.rs line 44 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we make
Options::libbpf_diraPathBuf?
Done.
xtask/src/integration_test.rs line 52 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
This field is never not None in this commit. Remove?
Done.
xtask/src/integration_test.rs line 78 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
attempting to live in String and PathBuf at the same time is a recipe for pain. See the fallible conversion you now have to do about 10 lines down.
Either everything is PathBuf (and by implication OsString) or everything is String.
Okay, String for all the things.
ajwerner
force-pushed
the
kernel-test
branch
2 times, most recently
from
7b0216e
to
4db0eb4
Compare
There was a problem hiding this comment.
Agreed, but I think we should treat that work as a prerequisite of this work. We should limit the number of ways we run VMs to 1.
Reviewed 7 of 7 files at r3, 5 of 7 files at r4, all commit messages.
Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @ajwerner)
xtask/Cargo.toml line 8 at r4 (raw file):
[dependencies] reqwest = { version = "0.11.18", default-features = false, features = [
why did this move?
xtask/src/integration_test.rs line 78 at r1 (raw file):
Previously, ajwerner wrote…
Okay,
Stringfor all the things.
Per discussion, let's go the other way. See #640.
ajwerner
changed the title
ajwerner
changed the title
ajwerner
force-pushed
the
kernel-test
branch
7 times, most recently
from
7abee9e
to
cc26ffa
Compare
|
Hey @alessandrod, this pull request changes the Aya Public API and requires your review. |
|
@ajwerner, this pull request is now in conflict and requires a rebase. |
ajwerner
force-pushed
the
kernel-test
branch
4 times, most recently
from
580f6df
to
6b39f31
Compare
tamird
force-pushed
the
kernel-test
branch
3 times, most recently
from
373e692
to
f9550fa
Compare
There was a problem hiding this comment.
Took a look, seems like it's getting close.
Reviewable status: 46 of 64 files reviewed, 1 unresolved discussion (waiting on @dave-tucker and @tamird)
xtask/src/qemu-initramfs.desc line 1 at r27 (raw file):
dir /bin 0755 0 0
This file doesn't seem to be used anymore.
There was a problem hiding this comment.
Reviewable status: 46 of 64 files reviewed, all discussions resolved (waiting on @dave-tucker)
xtask/src/qemu-initramfs.desc line 1 at r27 (raw file):
Previously, ajwerner wrote…
This file doesn't seem to be used anymore.
Done.
tamird
force-pushed
the
kernel-test
branch
6 times, most recently
from
aef8d40
to
2e8c5b1
Compare
This should get the aya integration tests passing, which are not yet able to run on older kernels. See aya-rs/aya#725 and aya-rs/aya#638.
tamird
force-pushed
the
kernel-test
branch
8 times, most recently
from
248aec8
to
252f5c9
Compare
Wait for at least one log and increase the wait time 10x.
tamird
changed the title
This allows integration tests to be run using qemu at an arbitrary kernel version. Note that before this change we were only testing fedora 38 which used 6.2. Now we're testing 6.1 kernels. The tests all pass. Older kernel versions were attempted, but the tests don't all pass. Later work can add more kernel versions to test.
|
This has been through too many iterations to be merged in this PR. @tamird to open a new PR. |
|
#733. |
No description provided.