Add a target runner for running tests in VMs #5285
Conversation
There was a problem hiding this comment.
This is extremely rad, great work getting this set up. Took me seconds to check out this PR and get the VMM test up-and-running -- truly incredible!
- Right now, only one test can be run per test binary, although we can change this if we want.
Given the hoops we're trying to jump through, and the current limitations of nextest, this seems totally reasonable, but probably worthwhile adding some docs in the form of a README ("here is how you add a VMM test, here is how you run it, etc"), even if just to make it "easy to follow along" with the way to use this thing as it develops.
The "requires an environment variable to work" + "single-test-per-binary" constraints are tricky, so we should document them pretty explicitly.
- We don't actually check the error result of the test in the VM. We should do this, and then automatically leave the VM running if the test fails.
I'd consider this a hard-blocker before encouraging anyone to use this as a test harness. Even if we aren't extracting all the log info we want yet, this seems like it's "dangerous enough to omit" that it should probably be fixed first.
| //! Mechanisms to launch and control propolis VMs via falcon | ||
|
|
||
| #[cfg(test)] | ||
| mod test { |
There was a problem hiding this comment.
Idea, take it or leave it -- it might be kinda nice to have a way to validate "this test is being run from a VMM context, bail out if that isn't true?"
I dunno what we'd want to set in the falcon_runner, but just something to distinguish it from the host, since "getting the config right" is one of the trickiest bits to launch these VM tests right now
There was a problem hiding this comment.
I added a check for the hostname, which is quite unique. We can certainly inject a file though, which might work better once we start using different names for VMs. We'll want to use different names to run tests in parallel.
This still needs a symlink of the .falcon dir to work.
|
This is probably good enough for a real review now. I added a README to get started and also leave the VM up when a test fails. |
There are few significant limitations listed in the README, but they can be resolved fairly quickly. It would probably be good to get this in to let people play with it, then figure out how we want to perform setup for more sophisticated tests that require injecting data into VMs, and for running in parallel. |
test-utils/README.md
Outdated
| * The name of the test `Runner` and `VM` is currently hardcoded. Therefore, | ||
| tests can only be run serially for now. This is an easy limitation to lift, we |
There was a problem hiding this comment.
nit - the names are hard-coded, but these aren't the names used, right? It's the string including "launchpad_mcduck?"
test-utils/src/bin/falcon_runner.rs
Outdated
| let exit_code_index = out.rfind('\n').unwrap(); | ||
| let exit_code: u8 = (&out[exit_code_index + 1..]).parse().unwrap_or(255); |
There was a problem hiding this comment.
Couple nitpicks here:
- If the
\ncharacter didn't exist in the output we might want to get better feedback from the test (via an.expectcall, perhaps?) - The indexing in
outseems like it could easily go out-of-bounds -- what if\nwas the last character emitted?
andrewjstone
changed the title
This PR allows us to run tests inside falcon VMs via nextest. Due to the lack of nextest per-test target runners, we enable the target runner via environment variable and run a specific test at a time.
In order to run a test, do the following, where the trailing
--nocaptureis optional but encouraged at this moment.An example:
A few things remain to be done, although this is safe to merge without adding them in this PR.
soloexample in falcon for a demo, but we'll want one in Omicron for ease of use.execcall like<BIN> exec <VM_NAME> 'cat <FILE>' > <LOCALFILE>. However, a better mechanism would be to make the p9 filesystem read/write instead of read-only, or to inject ssh keys and pull the files that way.