Paths

Table of Contentst

kyua: Add execution plan concept
Needs ReviewPublic
Actions

Authored by igoro on Sun, Aug 30, 9:23 PM.

Details

Reviewers

Summary

The idea is that kyua now uses wait6() to collect wrusage per test and save it into a file, after full run of kyua test command. Next time, having such test profile on the disk (usually beneath ~/.kyua/profiles/) it can schedule testing in a special way based on the facts from the profile. To apply such special scheduling Kyua must be run with a newly added configuration parameter (-v execplan=time).

Obviously, it does not provide magic multiple times speedup, it seems to potentially provide -10-20% of runtime or so. But the idea is that running many iterations is expected to bring a cumulative improvement in the total runtime.

The first run is the usual one to generate a profile:
/usr/tests> time -h kyua -v parallelism=N -v execplan=time test

From the 2nd run , having the profile, it should schedule the tests differently:
/usr/tests> time -h kyua -v parallelism=N -v execplan=time test

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 77200
Build 74083: arc lint + arc unit

Event Timeline

igoro created this revision.Sun, Aug 30, 9:23 PM

Herald added subscribers: ziaee, imp. · View Herald TranscriptSun, Aug 30, 9:23 PM

igoro requested review of this revision.Sun, Aug 30, 9:23 PM

Harbormaster completed remote builds in B76336: Diff 185447.Sun, Aug 30, 9:23 PM

That's an interesting idea.
Running the pf tests (at parallelism=4) that knocks the runtime down from 16 minutes to 10 minutes, which is pretty impressive.

That said, I find myself mostly running tests in bricoler these days, and it won't help there because that's a new single-use VM every time, without the profile information. (Though perhaps we could teach bricoler to import existing profile information from the host system?)

igoro edited the summary of this revision. (Show Details)Mon, Aug 31, 12:37 PM

In D59276#1360076, @kp wrote:

That's an interesting idea.

Yeah, I think this is a good idea. My only concern is that it might eliminate some randomness from the test scheduling that helps probabilistically catch problems. I have not looked at all at the patch yet, nor at the existing scheduler: how does the patch affect determinism in the test scheduler?

Running the pf tests (at parallelism=4) that knocks the runtime down from 16 minutes to 10 minutes, which is pretty impressive.

That said, I find myself mostly running tests in bricoler these days, and it won't help there because that's a new single-use VM every time, without the profile information. (Though perhaps we could teach bricoler to import existing profile information from the host system?)

Do you ever use the freebsd-regression-test-suite-ci target? It does a few things on top of freebsd-regression-test-suite:

it keeps track of results across successive runs, so you'll have persistent results for run #1, #2, ...
after the test suite finishes, it'll rerun failed and broken tests in a new VM, without parallelism enabled, so that we can better detect flakiness
it can send email summaries after the test run finishes

It'd be easy to change that task to save the profile as well, and when preparing to start a test run, it could use the previously saved profile to seed the next run.

Thinking a bit more, we could even integrate it into plain freebsd-regression-test-suite: just save the profile after a test run, like we already save the results db, and use it when starting a new run if available. So, I think this won't be a problem. I'll try to integrate it in the next day or so.

In D59276#1360539, @markj wrote:

In D59276#1360076, @kp wrote:

That said, I find myself mostly running tests in bricoler these days, and it won't help there because that's a new single-use VM every time, without the profile information. (Though perhaps we could teach bricoler to import existing profile information from the host system?)

Do you ever use the freebsd-regression-test-suite-ci target? It does a few things on top of freebsd-regression-test-suite:

it keeps track of results across successive runs, so you'll have persistent results for run #1, #2, ...

after the test suite finishes, it'll rerun failed and broken tests in a new VM, without parallelism enabled, so that we can better detect flakiness

it can send email summaries after the test run finishes

I've never used that, no. I don't really have a need for that, although I suppose the automatic re-running of failed tests is interesting.

My own use case for bricoler is mostly for a clean test of a patch (so I'm confident I'm testing what I think I'm testing) and a really easy way of getting the KASAN/KMSAN configs tested.

Remove debug code leftovers, make execplan=none do nothing with profiles, revise licensing

Harbormaster completed remote builds in B76581: Diff 185947.Sat, Sep 5, 5:52 PM

In D59276#1360539, @markj wrote:

My only concern is that it might eliminate some randomness from the test scheduling that helps probabilistically catch problems. I have not looked at all at the patch yet, nor at the existing scheduler: how does the patch affect determinism in the test scheduler?

The original scheduler does not randomize execution. I would say it's roughly the same run each time. It follows the list of tests represented by a Kyuafile and runs them one by one while respecting the limitation of parallel slots.

Using -v execplan=time also does not introduce randomization. It changes the order of execution and parallelism, but we could still say that each run is roughly the same.

Sorry it took me some time to come back to this. I integrated it into bricoler and hit a problem:

(gdb) run
Starting program: /usr/bin/kyua -v parallelism=16 -v execplan=time test -k /usr/tests/Kyuafile -r /usr/tests/kyua.db
*** /home/markj/sb/main/src/contrib/kyua/utils/fs/path.cpp:157: Precondition check failed: !is_absolute()

Program received signal SIGABRT, Aborted.
Sent by thr_kill() from pid 4608 and user 0.
thr_kill () at thr_kill.S:4
warning: 4      thr_kill.S: No such file or directory
(gdb) bt
#0  thr_kill () at thr_kill.S:4
#1  0x0000000801582c34 in __raise (s=s@entry=6) at /home/markj/sb/main/src/lib/libc/gen/raise.c:48
#2  0x0000000801635769 in abort () at /home/markj/sb/main/src/lib/libc/stdlib/abort.c:61
#3  0x0000000001081279 in utils::sanity_failure (type=type@entry=utils::precondition, file=0x10390fd "/home/markj/sb/main/src/contrib/kyua/utils/fs/path.cpp", line=line@entry=157, message=Python Exception <class 'gdb.error'>: There is no member or method named __r_.
) at /home/markj/sb/main/src/contrib/kyua/utils/sanity.cpp:171
#4  0x00000000010a3e8f in utils::fs::path::to_absolute (this=0x7fffffffe1c8) at /home/markj/sb/main/src/contrib/kyua/utils/fs/path.cpp:157
#5  0x0000000001102678 in engine::execplan::profile::get_path (this=this@entry=0x801c2b190) at /home/markj/sb/main/src/contrib/kyua/engine/execplan/profile.cpp:34
#6  0x0000000001103907 in engine::execplan::profile::load (this=0x801c2b190) at /home/markj/sb/main/src/contrib/kyua/engine/execplan/profile.cpp:163
#7  0x00000000010fde2e in engine::execplan::execplan_time::init (this=0x801c2b140) at /home/markj/sb/main/src/contrib/kyua/engine/execplan/execplan_time.cpp:38
#8  0x0000000001124342 in drivers::run_tests::drive (kyuafile_path=..., build_root=..., store_path=..., filters=..., user_config=..., hooks=...) at /home/markj/sb/main/src/contrib/kyua/drivers/run_tests.cpp:275
#9  0x000000000113a160 in cli::cmd_test::run (this=<optimized out>, ui=0x7fffffffe860, cmdline=..., user_config=...) at /home/markj/sb/main/src/contrib/kyua/cli/cmd_test.cpp:153
#10 0x000000000114366a in utils::cmdline::base_command<utils::config::tree>::main (this=this@entry=0x801c48c00, ui=ui@entry=0x7fffffffe860, args=std::vector of length 5 = {...}, data=...) at /home/markj/sb/main/src/contrib/kyua/utils/cmdline/base_command.ipp:96
#11 0x0000000001140f29 in (anonymous namespace)::run_subcommand (ui=0x7fffffffe860, command=0x801c48c00, args=std::vector of length 5 = {...}, user_config=...) at /home/markj/sb/main/src/contrib/kyua/cli/main.cpp:140
#12 (anonymous namespace)::safe_main (ui=<optimized out>, argc=<optimized out>, argv=<optimized out>, mock_command=...) at /home/markj/sb/main/src/contrib/kyua/cli/main.cpp:230
#13 cli::main (ui=ui@entry=0x7fffffffe860, argc=argc@entry=10, argv=argv@entry=0x7fffffffea78, mock_command=Python Exception <class 'gdb.error'>: There is no member or method named __value_.
) at /home/markj/sb/main/src/contrib/kyua/cli/main.cpp:282
#14 0x00000000011428ce in cli::main (argc=10, argv=0x7fffffffea78) at /home/markj/sb/main/src/contrib/kyua/cli/main.cpp:355
#15 0x00000008015553ff in __libc_start1 (argc=10, argv=0x7fffffffea78, env=0x7fffffffead0, cleanup=<optimized out>, mainX=0x107b1a0 <main(int, char const* const*)>) at /home/markj/sb/main/src/lib/libc/csu/libc_start1.c:180
#16 0x000000000107b121 in _start () at /home/markj/sb/main/src/lib/csu/amd64/crt1_s.S:80

It seems to be related to the use of -k, if I remove that then it works.

contrib/kyua/doc/kyua.conf.5.in
98	It would be helpful to be able to specify the location of the profile, like you can do for reports with `kyua test -r <report>`. Though, this case is a bit different since the profile is both an input file and an output file, so maybe we want separate options?

Herald added a reviewer: ngie. · View Herald TranscriptTue, Sep 22, 3:25 PM

I'm looking at this again.

To save time:

I recommend not making any huge changes or huge refactors until 0.15.0 lands.
- This ideally could be sometime later on this week--pending potential issues I might find when running my tests and any follow up work I need to do for the atf-0.26 release.
It would be extremely helpful to submit the change in PR form upstream to freebsd/kyua first, suss out any potential issues seen there on Linux and macOS, then pull the change back in to FreeBSD src proper. I know it's more work, but it makes life easier for me on the backend as the maintainer: I had to adjust/fix several changes to work with Linux/macOS after the fact because the code didn't work on those platforms, which could have been caught on the frontend, saving time for everyone.

Obey fs::path::to_absolute() behavior

Harbormaster completed remote builds in B77200: Diff 187475.Tue, Sep 22, 9:54 PM

In D59276#1374764, @markj wrote:

It seems to be related to the use of -k, if I remove that then it works.

It is. And, from my point of view, fs::path::to_absolute() failed the expectation coming from its name and const specification. I believe, it should provide an absolute path no matter what, i.e. it should do if (absolute) then (provide as is) else (construct an absolute path). I've checked the existing code around, and all its users do this if-else on their own, so I decided to follow the existing design.

igoro added inline comments.Tue, Sep 22, 10:08 PM

contrib/kyua/doc/kyua.conf.5.in
98	It would be helpful to be able to specify the location of the profile, like you can do for reports with `kyua test -r <report>`. Though, this case is a bit different since the profile is both an input file and an output file, so maybe we want separate options? Yes, it makes sense, having existing flags to direct Kyua to non-usual location for report, etc. And, as I see, bricoler needs non-default locations. I will add it with the next iteration. How is your testing going in general? Does it save you at least a minute in your testing environment and configuration?

In D59276#1374831, @ngie wrote:

I'm looking at this again.

To save time:

I recommend not making any huge changes or huge refactors until 0.15.0 lands.

This ideally could be sometime later on this week--pending potential issues I might find when running my tests and any follow up work I need to do for the atf-0.26 release.

No worries, I do not think it's going to be so quick. For instance, I still need to extract FreeBSD-specifics and make them conditional. Such polishing is left for later intentionally, as I wanted to discuss and test the idea first.

It would be extremely helpful to submit the change in PR form upstream to freebsd/kyua first, suss out any potential issues seen there on Linux and macOS, then pull the change back in to FreeBSD src proper. I know it's more work, but it makes life easier for me on the backend as the maintainer: I had to adjust/fix several changes to work with Linux/macOS after the fact because the code didn't work on those platforms, which could have been caught on the frontend, saving time for everyone.

Yep, why not. After approval here we can make a detour to let Kyua GitHub CI check the things before we land the patch on FreeBSD side. This is what I usually try to do post-factum, as Kyua upstream usually needs additional work and testing, e.g. it has another build system which is not used on FreeBSD side for obvious reasons.

markj added inline comments.Wed, Sep 23, 9:39 PM

contrib/kyua/doc/kyua.conf.5.in
98	With a KASAN kernel, the test suite runs in 41 minutes instead of 50+ minutes (typical test suite run times have quite high variance for some reason). This is with parallelism=16 and a bhyve VM with 16 vCPUs on a ryzen. That's just one run though, I will try to get a better picture in the coming days. Also interesting is that the interleaving of unrelated tests helps expose race conditions in the kernel: I had to fix four(!) different kernel panics in order to complete a single run of the test suite with your feature enabled. (One bug each in lockf, jail, zfs and geli code, I will post patches for review soon.) So, I think it's a very worthy feature!