kyua: add jail execution environment

[igoro]

Summary

The goal is to possibly improve the speed of the FreeBSD test suite by having additional approach of test running in parallel without additional complexity on a test case side. For example, some network related tests already spawn jails to isolate many details but still have clashing jail naming, routing entries, etc.

It introduces a new general concept of an execution environment. By default this is current approach without changes, i.e. a test case is running within a so called "host" environment. The second execution environment is added to run a test case within a temporary jail, it allows to isolate it more and run in parallel with other tests (of course, if conceptually it's possible for a specific test).

The changes of Kyua/ATF interfaces, from bottom to top:

1 ATF based tests

• The new execenv metadata property can be set to explicitly ask for an execution environment: "host" or "jail". If it’s not defined, as all existing tests do, then it means "host".
• The new execenv.jail metadata property can be optionally defined to ask Kyua to use specific jail(8) parameters during creation of a temporary jail. An example is "vnet allow.raw_sockets".

2 Kyuafile

• The same new metadata properties can be defined on Kyuafile level: execenv and execenv_jail.
• Note that historically ATF uses dotted style of metadata naming, while Kyua uses underscore style. Hence execenv.jail vs. execenv_jail.

3 kyua.conf, kyua CLI

• The new execenv engine configuration variable can be set to a list of execution environments to run only tests designed for. Tests of not listed environments are skipped.
• By default, this variable lists all execution environments supported by a Kyua binary, e.g. execenv="host jail".
• This variable can be changed via kyua.conf or via kyua CLI's -v parameter. For example, kyua -v execenv=host test will run only host-based tests and skip jail-based ones.
• Current value of this variable can be examined with kyua config.

Test Plan

In order to test Kyua jail support against main:

• apply kyua patch: https://reviews.freebsd.org/D42350
• apply jailed tests demo patch: https://reviews.freebsd.org/D44087
• full build
• kldload pf if_epair
• (jailed parallelism test only) kyua -v parallelism=$(sysctl -n hw.ncpu) test -k /usr/tests/sys/netpfil/pf/Kyuafile pass_block
  • optionally it can be run without parallelism to compare timings
• (full suite test) kyua test -k /usr/tests/Kyuafile
• (full suite test in parallel) kyua -v parallelism=$(sysctl -n hw.ncpu) test -k /usr/tests/Kyuafile

Extra testing for the cases without jail feature:

• apply kyua patch: https://reviews.freebsd.org/D42350
• apply jailed tests demo patch: https://reviews.freebsd.org/D44087
• full build WITHOUT_JAIL
• (jailed parallelism test only) kyua -v parallelism=$(sysctl -n hw.ncpu) test -k /usr/tests/sys/netpfil/pf/Kyuafile pass_block
  • it should report what those tests are skipped due to execenv=jail is not supported
• (full suite test) kyua test -k /usr/tests/Kyuafile

[igoro]

contrib/kyua/engine/execenv/jail.cpp

50 ↗

(On Diff #129548)

Agree, the comment is missing.

Yes, the simplest thing is implemented for now, currently it's based on program full path like /usr/tests/foo/bar plus a case name if presented. If required it's shortened down to the jail name limit, keeping the tail of the string, assuming it is the most unique part.

contrib/kyua/engine/plain.cpp

111

Yes, such situation is the most common one -- when usual so called "host" execenv is used, then init() does nothing. I think I left it as a separate thing for the sake of more obvious interface, and probably for the simpler responsibility of the execenv::exec() itself not to make it determine whether initialization is requied or it's done already (and probably keep additional state to maintain, just for that). For instance, an execution of the cleanup phase may make execenv::exec() life harder. Also, it could be important for possible new execenv implementations, I could think of execenv="bhyve" just as an example. What do you think?

contrib/kyua/utils/process/executor.cpp

694

Probably, it could be a separate patch. I've found a defect in the interrupt handling logic. Kyua already has logic to run cleanup for the spawned child processes (tests). But it does not collect all of them correctly and Kyua fails its expectations as a result. I guess it's hard to see it if no parallelism is used, but it's quite easy to reproduce it otherwise. And I needed to correctly run execenv cleanup phase (the new one, introduced by this patch) -- not to get dangling jails after Ctrl+C or some other abrupt termination. I think the respective code comment could be easily added here.

contrib/kyua/utils/process/jail.cpp

156 ↗

(On Diff #129548)

Yes, I added it to trigger this discussion due to it looks obvious that execenv="jail" based tests could soon enough start asking for some useful default not to set this for every single test case. Current implementation allows to override it by a test through execenv_jail metadata. Sure, we can use JAIL_MAX here as long as there are no technical objections.

178 ↗

(On Diff #129548)

I guess my idea was to have some limit anyway. Just in case. Not to let issues of third parties to flood Kyua logs. What do you think?

A code comment could be added here to make it clear.

190 ↗

(On Diff #129548)

Yes, they do hit the heap. Actually, the comment is a copy-pase of the existing process::exec() mechanism above the jail one. But the reality is that we use usual fork(2). Thanks, it needs comment update.

228 ↗

(On Diff #129548)

I understand that this is an expedient approach, but I don't like this method of generating and executing a script on the fly for each test execution.

I do not like it either :) Just the simplest and scoped (not to touch other things around as much as possible for now) solution to get the working prototype as soon as possible and reach the review quicker -- to optimize the time budget.

Can we have a standalone parameterized script that gets checked in and installed to /usr/libexec or so?

Oh, sounds like a bit better option if it's allowed to "pollute" the base such way :). This is where my incompleteness of global vision of the project does not allow to spawn such ideas and needs advices from the community.

Taking a further step back, why exactly do we need a script and jexec at all? Isn't it possible to use the jail system calls directly to do everything we want? I have done this one or twice when writing test cases and it's not too painful.

It sounds interesting.

(a little off-topic)
To be honest, my initial idea was to make it even simpler -- just add jail -cm <options> command= in front, without explicit execenv initialization and cleanup process. But the established jail(8) interface does not allow to do it easily and nowadays it's too late to break the POLA (e.g. see my patch https://reviews.freebsd.org/D42675), and there are other questions to the jail(8) like correct exit code communication in this context etc. But, probably for the sake of the execenv abstraction itself it's a good thing that it has explicit phases: execenv init, test exec, test cleanup exec, execenv cleanup.

Looking at D42410 I see that you do some things which require jexec, like set exec.prepare=kldload if_epair, but IMO this is not the right mechanism to load KLDs. Tests can easily load kernel modules themselves (and really this should be handled by the test framework).

Yes, currently this is the only working solution I have due to non-prison0 ones cannot load kernel modules. Here, I have not dug deeper yet to find alternatives or provide tests with the needed priv. Probably, you have some suggestions already.

[igoro]

In D42350#988878, @glebius wrote:

One thing that I really miss in our jail/vnet testing suite is memory leak control. An automated snapshot of 'vmstat -m' and 'vmstat -z', run test in a jail, destroy the jail and compare with new vmstat snapshot. At first approach this may lead to lots of false positive, which aren't real leaks, but with fine filtering of which types/zones we really expect not to change that it going to work. That will bring a lot of value to the testing automation.

Sounds like an interesting idea for the subsequent Kyua contribution. And it looks like I'm in the context already, I was thinking about similar options after I dealt with a memleak in pf upon vnet destroy.

I guess the most complex part here could be to brainstorm and agree on a good long-standing interface for test authors not to break it later when extension is wanted or something. Well, a classic problem anyway.

[markj]

In D42350#988878, @glebius wrote:

One thing that I really miss in our jail/vnet testing suite is memory leak control. An automated snapshot of 'vmstat -m' and 'vmstat -z', run test in a jail, destroy the jail and compare with new vmstat snapshot. At first approach this may lead to lots of false positive, which aren't real leaks, but with fine filtering of which types/zones we really expect not to change that it going to work. That will bring a lot of value to the testing automation.

I have a feeling that diagnosing false positives will be too difficult. I would try to implement something like Linux's kmemleak instead.

contrib/kyua/engine/execenv/execenv.cpp

79

Yes, I think an else would make this more clear.

contrib/kyua/engine/execenv/jail.cpp

50 ↗

(On Diff #129548)

So in general, it's impossible to guarantee that this will always work? :(

I agree that it's probably ok in practice, but it's hard to be sure.

contrib/kyua/engine/plain.cpp

111

How would execenv=bhyve work? What image would it boot?

It's definitely useful to have a framework for managing VMs which run tests; I have some lua scripts which do this, so one can build a src tree, create a VM image, boot and run all tests with a single command. IMO, kyua is probably not the right vehicle for that kind of functionality, but I don't know what you have in mind.

contrib/kyua/utils/process/jail.cpp

156 ↗

(On Diff #129548)

Hmm. I really wish it would be possible to specify this stuff in lua.

I cannot see a reason to avoid making children.max=JAIL_MAX the default. If individual tests want to override this for some reason, they are free to do so.

178 ↗

(On Diff #129548)

I don't understand this logic. If I'm using kyua to run test programs, then I definitely want to get the full output to understand what went wrong.

228 ↗

(On Diff #129548)

Can we just have a new test property, i.e., required_klds="if_epair pf", etc., and then kyua can handle this internally?

There is some inconsistency in the existing tests regarding KLDs. Some tests will load KLDs automatically (if they are running as root), some tests expect the user to load them first. I think the latter approach is a bit silly - if I'm running the tests, then of course I want them to do whatever they need to do. And, it makes the test suite as a whole less useful: I can't just run kyua test, I have to first do kldload carp dummynet if_ovpn if_stf ipdivert ipsec mac_bsdextended mqueuefs pf pflog pfsync sctp and then run kyua test. It's silly.

So, IMO tests should just load whatever modules they need. It would be even better if kyua can handle that, so that the requirements of each test can be declared.

Regarding the implementation here, my vote would be to try to avoid having a script at all, i.e., use the jail_* system calls directly and avoid relying on jexec features.

[igoro]

contrib/kyua/engine/execenv/jail.cpp

50 ↗

(On Diff #129548)

Yeah, IT frequently is about some trade-offs. Here it has some benefits being stateless and depend on input only, like SYN cookie as an example. The Ostrich algorithm looks to be fine here due to uniqueness of the program path is provided by a file system and uniqueness of a test case name is provided by ATF design.

But now I'm a bit worry about cases when a test author uses quite long descriptive test case naming, considering the jail name limit. There is a room for a little improvement, e.g. to shorten path and case name separately to preserve as much uniqueness as possible. But it looks like an additional complexity for no much win in return. What if we add the simplest hashing here? Probably you would like an idea to add something like CRC32 computed over the original path+case string before it's cut. Then a jail name could look like the following:
kyuaA1B2C3D4_usr_tests_sys_netpfil_pf_pass_block_nested_inline. And a shortened one could be like this: kyuaA1B2C305_ry_long_shortened_program_path_plus_case_name_string.

What do you think?

contrib/kyua/engine/plain.cpp

111

It was merely an example to reason about the separation of init and exec :) Thus, I try not to make execenv concept to be about jails only. If I have ideas about bhyve specifically then I will post a prototype patch to discuss it.

Probably I referred to bhyve as an example due to I had some quick "visions" months ago that probably execenv="bhyve" could be useful for some very specific tests like kernel crashing related or specific hardware behavior related, etc. But, for now it does not have any technical form to discuss, just high level quick thoughts to work on in future if I still find them interesting.

contrib/kyua/utils/process/jail.cpp

178 ↗

(On Diff #129548)

Fair enough. Anyway, jail(8) is not expected to be very talky.

[igoro]

contrib/kyua/engine/execenv/jail.cpp

50 ↗

(On Diff #129548)

We have 255 bytes for a jail name, as I remember. Probably we could even go with something more interesting like md5 or so, and additionally compact it into the name with some baseX. And we could still have enough space for the actual name. It looks to be useful to keep the name (path+case) as much as possible for the troubleshooting situations. Just thinking out loud.

[igoro]

Most of the agreed changes should be covered by this version of the patch.
Open topics left:

• jail naming
• required_klds

[igoro]

contrib/kyua/utils/process/executor.cpp

694

It already has the respective code commented on the caller side -- executor::executor_handle::reap().

contrib/kyua/utils/process/jail.cpp

156 ↗

(On Diff #129548)

Now it's based on JAIL_MAX.

(off-topic)
I've added to my TODO to propose a separate patch for <sys/jail.h> in order to move the JAIL_MAX constant out from internals (ifdef _KERNEL). It will be interesting to hear from jail system supervisors whether it's a good idea.

[igoro]

contrib/kyua/utils/process/jail.cpp

228 ↗

(On Diff #129548)

Yeah, the same here, for two decades I've been striving towards a single command to run a test suite(s) of a product.

But my recent study of Kyua and the FreeBSD test suite formed the vision that the project decided to be based on the Kyua philosophy -- infrastructure tests for both developers and users. It means to be able to run tests right on the battlefield, not only during R&D (e.g. FreeBSD's CI) and manufacture (e.g. FreeBSD's releng process). And Kyua puts it such way that if during manufacture we want to run everything with a possibility to blow the test environment, on the other hand we would like to allow a soldier to run some self-check of the weapon but without actual rocket launching by default. Thus, the test suite is configurable and "explosive" tests should run only if an operator asks for it explicitly. That's why we have options like allow_sysctl_side_effects and some tests are designed to be skipped if expecatations are not fulfilled.

And the first existing tests I was working with confirmed this vision. I hope a good example could be firewall testing. For instance, if a host does not use ipfw but a test silently loads it for testing needs then such host may end up being non-functional over network due to the default settings provide a closed firewall type. That's why such tests simply check whether such module is loaded and skip themselves with a message which describes the expectations from the host's operator.

Working deeper with the FreeBSD test suite I've got the idea that some modules are okay to be loaded silently, e.g. if_epair. It sounded for me like it was decided (or it's just happened so) that relatively non-harmful modules can be silently loaded as a runtime dependency of a test. Sure, it's disputable, but this is how it is today.

While I was working on https://reviews.freebsd.org/D42410 I got acquainted with the whole FreeBSD test suite and found the mentioned inconsistency.

As for me, the idea of explicit declaration of expectations and requirements sounds profitable. First of all, it's kind of a self-documentation, e.g. I had to research which sysctl is needed for specific tests to get the expected feature enabled, or which package should be installed, etc. If a test declares it more explicit way then it will be easier to provide it with the expectations or to automate this process.

Sorry for the tl;dr, I just wanted to explain my worries that if we implement something like required_klds that unconditionally loads the mentioned modules then we will break behavior of the existing tests.

From my understanding, some action committee of developers (as it usually happens) could decide on the design of the FreeBSD test suite if we the past years of its usage lead us to the idea that doing it for both developers and users is less-profitable than just "for developers".

To sum up, I personally see at least the following possible next steps:

1. For now, keep current implementation which allows to load modules via jail(8) interface.
2. Decide on the conceptual direction of the whole test suite and implement respective declarative approach. I guess it may keep this jail support unresolved for much longer. I have no rush here, just as I feel it :).
3. Other ideas to come from the discussion.

What do you think?

[igoro]

There is a follow-up idea.

If we go with the step #2, then there is a high chance a decision making group could end up with at least two options:

1. Okay, let's go "tests for developers only" way.

Here we could easily implement required_klds test metadata, and kyua could help to load such modules.

2. Let's keep it as is.

As long as we already have actual separation of klds within existing tests, we could introduce two metadata:

• expected_klds could list SUT (system under test) modules which are expected to be prepared by a host's operator. Here we could find ipfw, pf, pflog, ipdivert etc.
• required_klds could list runtime dependencies of a test. Here we could find things like if_epair.

And the idea itself is not to wait for this decision making and postpone it until it's really wanted. We could introduce expected_klds and required_klds today, but kyua would treat them according to the current state of the things. Thus, kyua would do nothing with expected modules, but will help to load required ones. And if in future it's decided to go with "for developers only" then existing tests will not require updating and only kyua could change its behavior and do load expected modules. Or there could be a knob to configure it.

If this idea is found interesting we could also think of other expectation types, e.g. expected_sysctl, expected_pkg, probably even something generic like expected_cmd (like any command, e.g. pip install scapy), and so on.
I think in future such metadata could be used for other useful automations, regardless the decision on the FreeBSD test suite philosophy.

Could such direction be found helpful?

P.S. The mentioned naming is just an example, we could think of something else.

[ngie]

I held off on this review for a while, but I wanted to chime in about a few things:

1. kyua reviews and proposed changes should be done on GitHub instead of Phabricator: https://github.com/freebsd/kyua/ .
2. Jails are a FreeBSDism; this unfortunately cannot be easily tested without using Cirrus CI on Linux or OSX hosts, which will discourage future contributions from folks in the Linux or Solaris communities (there has been some expressed interest in the past in maybe leveraging Kyua on those platforms). This would also discourage future contributions from NetBSD, etc. Some build support would need to be added, like conditional inclusion via autoconf, cmake, etc, to support those OSes.

contrib/kyua/engine/execenv/execenv.cpp

58–65

This needs to be wrapped in curly braces or dedented appropriately.

Depending on the toolchain/preprocessor/reformatter/human, I could see where this could result in incorrect behavior at a later date.

82–89

Again, this should be wrapped in curly braces or dedented appropriately.

108

For example...?

110–112

Again, this should be wrapped in curly braces or dedented appropriately.

contrib/kyua/engine/execenv/jail.cpp

58 ↗

(On Diff #132793)

• "jail name max" should be provided via a C++ or libjail-provided constant.
• Hardcoding strlen((const char*)"kyua") instead of computing it (and letting the C++ compiler optimize it away) could result in programming errors later if someone fails to modify either the hardcoded prefix or this value.
• "kyua" should be stored in a well-defined/named constexpr.

contrib/kyua/engine/scheduler.cpp

997–998

Why squelch this error?

1424–1430

Why is this being ignored?

1488–1489

Again, why is this being ignored?

1523–1524

This seems like a potential programming error... why is this being ignored?

contrib/kyua/utils/process/jail.cpp

59 ↗

(On Diff #132793)

This puts all of the functions in the anonymous namespace; this could cause conflicts at a later date.

201–202 ↗

(On Diff #132793)

I'd use std::filesystem::current_path() instead: https://stackoverflow.com/a/2203694 . This would change the minimum required C++ standard to C++-17, which would likely be ok, given that clang added this support back in 7.x: https://en.cppreference.com/w/cpp/compiler_support/17 .

[markj]

In D42350#991264, @igor.ostapenko_pm.me wrote:

There is a follow-up idea.

If we go with the step #2, then there is a high chance a decision making group could end up with at least two options:

1. Okay, let's go "tests for developers only" way.

Here we could easily implement required_klds test metadata, and kyua could help to load such modules.

2. Let's keep it as is.

As long as we already have actual separation of klds within existing tests, we could introduce two metadata:

• expected_klds could list SUT (system under test) modules which are expected to be prepared by a host's operator. Here we could find ipfw, pf, pflog, ipdivert etc.
• required_klds could list runtime dependencies of a test. Here we could find things like if_epair.

And the idea itself is not to wait for this decision making and postpone it until it's really wanted. We could introduce expected_klds and required_klds today, but kyua would treat them according to the current state of the things. Thus, kyua would do nothing with expected modules, but will help to load required ones. And if in future it's decided to go with "for developers only" then existing tests will not require updating and only kyua could change its behavior and do load expected modules. Or there could be a knob to configure it.

If this idea is found interesting we could also think of other expectation types, e.g. expected_sysctl, expected_pkg, probably even something generic like expected_cmd (like any command, e.g. pip install scapy), and so on.
I think in future such metadata could be used for other useful automations, regardless the decision on the FreeBSD test suite philosophy.

Could such direction be found helpful?

P.S. The mentioned naming is just an example, we could think of something else.

I agree with the points that you raised. The test suite is a large, growing, breathing thing, so we should try to proceed incrementally. For now, I would not worry about automatically loading kernel modules. To run the test suite today, one needs to preload a large set of modules to get good coverage. If one does not preload modules, tests should be skipped rather than failing. My proposal is merely to copy the declarative aspect of required_progs and use that for kernel modules as well. I am not sure yet whether kyua should be loading kernel modules at all; similarly, it does not install required_progs.

To be honest, I don't really like the idea of required_klds vs. expected_klds. It is a bit too confusing. Some tests will automatically load if_epair.ko. It's inconsistent, but not a huge problem really. If such tests run in a jail, then they need to be explicit about it, that's all. Today they can manually check whether if_epair is loaded using kldstat. A required_klds declaration would be nicer, but either way works.

I do not consider required_klds to be a prerequisite for jail execenv, by the way. Maybe that's already clear, but there's a lot of discussion so I want to say that explicitly.

In D42350#991331, @ngie wrote:

I held off on this review for a while, but I wanted to chime in about a few things:

1. kyua reviews and proposed changes should be done on GitHub instead of Phabricator: https://github.com/freebsd/kyua/ .

Well, the review's been open since October, might as well keep the discussion here until we've fully agreed on some direction.

1. Jails are a FreeBSDism; this unfortunately cannot be easily tested without using Cirrus CI on Linux or OSX hosts, which will discourage future contributions from folks in the Linux or Solaris communities (there has been some expressed interest in the past in maybe leveraging Kyua on those platforms). This would also discourage future contributions from NetBSD, etc. Some build support would need to be added, like conditional inclusion via autoconf, cmake, etc, to support those OSes.

The proposed feature is optional and simply wouldn't be available on other platforms. It would be a good idea, though, to structure code changes so that FreeBSD-specific features are easily separated, i.e., we should ensure that code relating to jails and FreeBSD kernel modules lives in a freebsd/ subdirectory. At this point in the review we are still fleshing out the shape of some new features though.

Given the anemic history of contributions to kyua in the past (the NetBSD project still doesn't use kyua despite it having been written by a NetBSD developer), and given that FreeBSD has a large test suite which relies on it (are there any other projects which use it?), I strongly prefer to focus on adding features that help make FreeBSD user and developer lives easier.

contrib/kyua/utils/process/jail.cpp

228 ↗

(On Diff #129548)

I don't think 1 is a good direction for now. We are not going to run all tests in jails by default (right?); we just need to make sure that any jailed tests are explicit about their dependencies so that kyua can decide whether or not to skip a test. I don't think kyua should be loading kernel modules.

[igoro]

contrib/kyua/utils/process/jail.cpp

228 ↗

(On Diff #129548)

Actually, the idea of the option 1 is not about jail(8), the only thing I saw behind this option is to keep existing tests unchanged and speed-up the whole test suite today -- my tests yielded ~x2 speedup (https://reviews.freebsd.org/D42410 has demo numbers). The exec.prepare parameter of jail(8) simply has been found handy for dealing with existing kld inconsistency without extra effort.

That patch is named "run all in jail by default" for short, but its actual idea is to enable execenv="jail" with a single knob for the whole suite and negate it with execenv="host" for the tests which are not capable to be run within a jail or they are not meant to do so like non-root tests (required_user="unprivileged"). And the test suite polishing process could slowly take place after, e.g.: alter some tests to be jail capable to improve parallelism and speedup the suite more, revise some tests to fix kld inconsistency and drop exec.prepare usage respectively, etc. The actual usage of exec.prepare is like a TODO marker of the tests which need fix of kld inconsistency.

So, the whole idea is to get the test suite run faster today w/o modification of the existing tests and CI configuration -- i.e. get benefit for the FreeBSD development process much sooner comparing to months or years of slow migration to execenv="jail" per test. And the logic of the tests are not altered, we simply run them faster. And that second patch shows that this idea works and can provide about x2 speed today, and I guess more CPU can yield more.

Another interesting feature of this approach demonstrated by that second patch is that tests which are capable to be run in a jail still can do that without being jailed. This kyua patch has a logic that if a test case has both is_exclusive="true" and execenv="jail" defined then it will be run in parallel, i.e. it's not practical to follow is_exclusive="true" flag and run such test sequentially. It means that if we do not need to run such test in a jail it still has is_exclusive="true" flag correctly defined, i.e. it needs to be run sequentially for its own reasons. But if we go with "opt-in" option per test as we agreed before then it means that we may end up nailing tests to be always run within a jail only, and if we want to turn it off for the whole test suite (who knows, maybe some troubleshooting case will require to avoid jails to find a bug) it will mean to alter metadata of every single test who opted-in -- what practically means it's impossible.

I guess now you have deeper context of the deal and probably find that second patch interesting by looking at its overall idea from another angle. Okay-okay, agree, now it looks like I'm trying to sell again after a failed cold call :)

Coming back to our previous agreements, we decided to go with "opt-in" per test and the good news is that this kyua patch does not depend on this decision -- it's about how to use kyua jail support not how to implement it. And, via the review process, we may encourage opted-in tests to fix kld inconsistency if any not to use exec.prepare, as an example pf tests will end up expecting both pf and if_epair being prepared by a host's operator (I guess all vnet based tests will end up to expect if_epair because of the common util functions). And we will update CI configuration to pre-load such unshadowed modules.

[igoro]

contrib/kyua/engine/execenv/jail.cpp

50 ↗

(On Diff #129548)

So in general, it's impossible to guarantee that this will always work? :(
I agree that it's probably ok in practice, but it's hard to be sure.

It looks that jail naming is the only open question left, from the overall design perspective. Do you have preferences here? Should we do some hashing today or it can be picked up later?

[igoro]

In D42350#991331, @ngie wrote:

I held off on this review for a while, but I wanted to chime in about a few things:

Thanks for your time and consideration.

1. kyua reviews and proposed changes should be done on GitHub instead of Phabricator: https://github.com/freebsd/kyua/ .

Yeah, through my study of Kyua I found the fork and the vendor branch. The initial summary of this patch was kind of asking for the guidance what is the best way to proceed, but we ended up doing it here in Phabricator. I hope it's okay if we finish the process here for consistency and when it's time to land it we could go through the required process: github fork first, vendor branch second, and merge to CURRENT after.

1. Jails are a FreeBSDism; this unfortunately cannot be easily tested without using Cirrus CI on Linux or OSX hosts, which will discourage future contributions from folks in the Linux or Solaris communities (there has been some expressed interest in the past in maybe leveraging Kyua on those platforms). This would also discourage future contributions from NetBSD, etc. Some build support would need to be added, like conditional inclusion via autoconf, cmake, etc, to support those OSes.

As @markj agrees we could allocate the final phase of the work to re-structure the change and collect FreeBSD specifics separately with conditional build or so.

[igoro]

contrib/kyua/engine/execenv/execenv.cpp

108

I guess execenv can be anything what is found practical, e.g. "bhyve", "qemu", or "docker" if we think outside of the FreeBSD project itself.

contrib/kyua/engine/execenv/jail.cpp

58 ↗

(On Diff #132793)

Sure, as long as the overall idea is accepted we now can invest time to polish the things.

contrib/kyua/engine/scheduler.cpp

997–998

It simply follows the existing style, e.g. tests_needing_cleanup(), when we pick only values of a specific type.

1523–1524

This whole function, wait_any(), is a giant if-else switch actually. The original code forms this branching around try-catch type casting and I decided to follow the same idea to simplify the diff and the review process respectively. There were two types only, and this patch adds the third one. Agree, it's not perfect, but refactoring along with the key changes (this is the cornerstone function for this patch) could be too much for the reviewers and minimize chances to attract attention to the path. And the desired refactoring may happen later separately. The whole approach of this patch is to follow the existing Kyua architecture and style as much as possible.

I think we could help the current state of the code with another comment instead of just "ignored" word to ease the reading. And our TODO lists could be extended with the idea to refactor it in future with typeid() or something else.

contrib/kyua/utils/process/jail.cpp

59 ↗

(On Diff #132793)

Just following the style of existing code. I guess there is no issue to avoid this.

201–202 ↗

(On Diff #132793)

Yeah, it would be great to stay within the C++ itself for the sake of Kyua as a separate cross-platform project, but I've tried to avoid modernism due to this: https://cgit.freebsd.org/src/commit/usr.bin/kyua?id=42fb28cef42e883d808c9efadd44016563248817.

Fortunately, it's not so bad due to getcwd() is from POSIX.

[igoro]

I hope this update covers most of the @ngie points raised.

[igoro]

This update gets wait_any() back to the working state after the recent manipulations with the if-else branching there.

[igoro]

Migrate from hard-coded JAIL_MAX to dynamic security.jail.children.max sysctl. The latter was recently added to the main: https://reviews.freebsd.org/D43565.

[igoro]

@markj @ngie , the current state of the patch seems to cover most of the comments and agreements. I guess we could land it and think of further priorities like FreeBSD specifics separation, jail naming improvements (if it's a must today), probably tests of the kyua itself, man updates, etc. So, the subsequent changes could be separate reviews to ease the cognitive load.

If it sounds like an applicable plan, then I have a question about the commit path: should I prepare GitHub PR for the freebsd/kyua fork? Or which path should it take?

[markj]

In D42350#994944, @igor.ostapenko_pm.me wrote:

@markj @ngie , the current state of the patch seems to cover most of the comments and agreements. I guess we could land it and think of further priorities like FreeBSD specifics separation, jail naming improvements (if it's a must today), probably tests of the kyua itself, man updates, etc. So, the subsequent changes could be separate reviews to ease the cognitive load.

I think to land this, we should have at least:

• documentation updates (e.g., a description in Kyuafile),
• some test cases converted to use the new feature.

These should be separate patches, as this one is already too large.

If it sounds like an applicable plan, then I have a question about the commit path: should I prepare GitHub PR for the freebsd/kyua fork? Or which path should it take?

It should go in via the freebsd/kyua github repo.

contrib/kyua/engine/execenv/jail.cpp

58 ↗

(On Diff #133448)

This function should have a comment explaining its limitations.

contrib/kyua/utils/process/jail.cpp

221 ↗

(On Diff #133448)

Shouldn't this be PATH_MAX?

[igoro]

Resolve the recent comments.

[igoro]

This comment has been deleted.

contrib/kyua/engine/execenv/jail.cpp

58 ↗

(On Diff #133448)

Yeah, the initial request for the comment was lost within the discussion of the name generation approach. As long as it's decided now it's time to add the comment.

contrib/kyua/utils/process/jail.cpp

221 ↗

(On Diff #133448)

Thanks, this part has been missed to get polished after the initial prototype. And 256 was not even close :)

[igoro]

Please, find the PR here: https://github.com/freebsd/kyua/pull/224.

[igoro]

This is a re-worked version of the patch after the recent discussion on freebsd-hackers@ list:

• As long as there are volunteers which potentially are ready to help with testing of the patch I decided to apply refactoring before that, not to ask community for re-testing. I had this refactoring in my TODO planned for future. It better separates execenv implementation and FreeBSD specifics from the main code. There is a room for more improvement, but current state looks to be good enough.
• is_exclusive is reverted back to its original state when it takes effect regardless execenv value. That's for the POLA. If a test opts in to be based on execenv=jail then it will have to explicitly turn off its exclusivity for parallel execution.
• A brand new execenv Kyua engine variable was introduced (aka user runtime configuration, aka kyua.conf, aka kyua -v key=value). It allows to ask the kyua engine to run only tests designed for the listed execution environments, e.g. kyua -v execenv=host test would not run jail based tests. By default, it lists all built-in and supported envs, e.g. execenv=host jail. The Kyua runtime configuration can be listed with kyua config. This change covers Paul's case.
• usr.bin/kyua build was improved to allow Kyua build WITHOUT_JAIL. This change covers Olivier's case.

[igoro]

Documentation update and extra tests of Kyua itself are still postponed until the code freeze.

@ngie , if you have time it would be appreciated to receive your review and comments, especially from C++ perspective.

[igoro]

Updated summary of this Differential to align it with the current implementation.

[igoro]

The respective documentation update is added. Two man pages are patched: kyua.conf.5 and kyuafile.5.

[kp]

contrib/kyua/doc/kyuafile.5.in

177 ↗

(On Diff #136096)

man pages want a new line for every new sentence.

[igoro]

Make mandoc -T lint and igor report nothing for kyuafile.5 and kyua.conf.5.

[igoro]

contrib/kyua/doc/kyuafile.5.in

177 ↗

(On Diff #136096)

Thanks for reminding about man linting.

[igoro]

I've updated the GitHub PR respectively: https://github.com/freebsd/kyua/pull/224.

[igoro]

Jail related error messages are corrected after FreeBSD based code separation. The GitHub PR has been updated respectively: https://github.com/freebsd/kyua/pull/224.

[igoro]

This has been resolved as a separate https://reviews.freebsd.org/D45865 review. The synchronization with the vendor is expected to be continued on GitHub: https://github.com/freebsd/kyua/pull/224.