Re: Add jail execution environment support to the FreeBSD test suite

In reply to: Kristof Provost : "Re: Add jail execution environment support to the FreeBSD test suite"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Igor Ostapenko <igor.ostapenko_at_pm.me>
Date: Wed, 03 Apr 2024 14:13:56 UTC
Hi,

Thanks for testing.

My current vision of the action points is as follows:

The first phase:
- [x] igoro: Separate FreeBSD specifics and fix existing Kyua tests broken by
             the change
- [x] igoro: Migrate some of the existing tests for the start, e.g. netpfil/pf
- [x] igoro: Cover Paul's use case mentioned in this email thread
- [x] igoro: Cover Olivier's use case mentioned in this email thread
- [x] igoro: Provide the respective documentation updates (manual pages)
- [~] community: Review, testing, comments -- probably we want to change the
                 design
- [ ] committers: Help with the main commit -- it should hit freebsd/kyua
                  GitHub fork first, then vendor branch, and merge to main after

The next phases:
- [ ] igoro: Provide the PRs to add brand new tests of Kyua itself to cover
             the new feature
- [ ] igoro: Help with the Handbook(s) update to cover the new concept for
             test authors

The future phases:
- [ ] igoro: Work on the related improvements and ideas like required_klds etc


If there is nothing to change or add at this stage then the next step could be
to merge the GitHub PR:

        https://github.com/freebsd/kyua/pull/224


Thanks the community for your time invested, I hope it will be eventually
payed back with better test run time during development.


Best regards, Igor.



On Thursday, March 21st, 2024 at 4:59 AM, Kristof Provost <kp@FreeBSD.org> wrote:

> <Picking this mail to resurrect the thread>
> 
> I’ve been toying with this patch. Adding only the following patch I can get Kyua to run the pf tests in parallel:
> 
>     diff --git a/tests/sys/netpfil/pf/Makefile b/tests/sys/netpfil/pf/Makefile
>     index 867b98e5f6c2..c2f0f15fa798 100644
>     --- a/tests/sys/netpfil/pf/Makefile
>     +++ b/tests/sys/netpfil/pf/Makefile
>     @@ -51,8 +51,8 @@ ATF_TESTS_PYTEST+=    frag6.py
>      ATF_TESTS_PYTEST+=     nat66.py
>      ATF_TESTS_PYTEST+=     sctp.py
>     
>     -# Tests reuse jail names and so cannot run in parallel.
>     -TEST_METADATA+=        is_exclusive=true
>     +TEST_METADATA+= execenv="jail"
>     +TEST_METADATA+= execenv_jail="vnet allow.raw_sockets"
>     
>      PROGS= divapp
>     
> 
> That gets the test time, with parallelism=5, down from 22 minutes to 5m40s.
> So I’m rather keen to see this work land.
> 
> My read from the reactions here is that people are generally okay with the approach, especially (I assume) given that the current version lets us turn this on on a per-test basis.
> 
> Is there anything else anyone wants to raise before we land this?
> 
> Best regards,
> Kristof
> 
> On 28 Feb 2024, at 2:32, Igor Ostapenko wrote:
> 
> > Hi,
> > 
> > The patch was updated after the recent discussion.
> > 
> > Currently, the patch provides the following new functionality, 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 implicitly 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".
> > 
> > The patch is https://reviews.freebsd.org/D42350.
> > 
> > Any help with review and testing is welcome. Its test plan covers the
> > details and refers to the demo patch of how existing tests could be
> > converted to be run in a jail.
> > 
> > Best regards, Igor.
> > 
> > On Thursday, February 22nd, 2024 at 10:57 PM, igor.ostapenko@pm.me <igor.ostapenko@pm.me> wrote:
> > 
> > > Hi FreeBSD developers,
> > > 
> > > There is a proposal to improve the FreeBSD test suite.
> > > 
> > > 1 The Problem
> > > 
> > > The FreeBSD test suite is based on the Kyua framework. The latter supports
> > > running tests in parallel. However, some tests cannot be run in parallel and
> > > are marked with is_exclusive="true" metadata, which makes Kyua run such tests
> > > in sequence.
> > > 
> > > Many tests are not meant to be exclusive conceptually, they are so for very
> > > simple technical reasons. For instance, some network related tests are based
> > > on jail and vnet usage. It's convenient for such tests and it provides a lot
> > > of isolation already not to conflict with other tests. But they are still
> > > marked as exclusive due to the shared space of jail names, routing, etc.
> > > 
> > > The project seeks more tests, and it's kind of a trend for new tests like
> > > jail/vnet based ones to be created as is_exclusive="true" from the very
> > > beginning. It only piles up the suite with exclusive tests, e.g. new tests
> > > from my side faced a fair question from a reviewer whether they could be
> > > re-designed for a parallel run. [1]
> > > 
> > > If such tests were 100% isolated they would be able to run in parallel and
> > > decrease the test time for CI runs and for the runs within the development
> > > process.
> > > 
> > > And the problem is that trying to add more isolation by a test itself looks to
> > > be a doable task from a glance, but it would add a lot of complexity to a test
> > > code, or could be found as an impossible task in a specific case.
> > > 
> > > 2 The Idea
> > > 
> > > The idea is not new. A test could be running in a jail -- it provides the
> > > required isolation with minimum or zero effort from a test.
> > > 
> > > 3 The Implementation
> > > 
> > > There is a lot of work done already and the working patch passed the initial
> > > review (thanks to markj@ and ngie@). [2]
> > > 
> > > It adds a new concept to the Kyua framework -- an execution environment. Two
> > > new metadata were added for that: execenv and execenv_jail.
> > > 
> > > execenv is a switch to select an environment. If a test's metadata defines
> > > execenv="jail" then Kyua will create a temporary jail, run such test within
> > > it, and remove the jail. If execenv="host" is provided or execenv metadata is
> > > undefined then Kyua will run such test as it does today.
> > > 
> > > execenv_jail metadata takes effect only in case of execenv="jail". It allows a
> > > test to request specific parameters for its jail. These parameters are simply
> > > arguments to jail(8), e.g. execenv_jail="vnet allow.raw_sockets".
> > > 
> > > 4 The Adoption
> > > 
> > > ATF based tests can easily define this new metadata via Kyuafile or directly,
> > > e.g. for atf-sh based tests:
> > > 
> > > test_head()
> > > {
> > > atf_set descr "Test foo in case of bar"
> > > atf_set require.user root
> > > atf_set execenv jail
> > > atf_set execenv.jail vnet allow.raw_sockets
> > > }
> > > 
> > > Non-ATF based ones will do it via Kyuafile. Our test suite does it through a
> > > Makefile:
> > > 
> > > TEST_METADATA+= execenv="jail"
> > > TEST_METADATA+= execenv_jail="vnet allow.raw_sockets"
> > > 
> > > The patch got some little evolution, I started with a single execenv_jail
> > > metadata, and during the patch discussion and review, I ended up with two
> > > knobs: execenv and execenv_jail. It turned out to be a cleaner and less tricky
> > > interface such way. The evolution reasoning can be found in the history of the
> > > respective Differential. [2]
> > > 
> > > 5 MFC Concerns
> > > 
> > > For now, I see at least one issue from the usual project workflow perspective.
> > > Let's imagine that the Kyua framework got this execenv feature committed to
> > > 15-CURRENT, we started to convert existing tests and create new ones to use
> > > execenv="jail". If some feature or a bug fix needs to be ported back to
> > > 14-STABLE or 13-STABLE, then "old" Kyua without execenv feature will fail to
> > > run such tests:
> > > 
> > > kyua: E: Load of 'Kyuafile' failed: Failed to load Lua file 'Kyuafile': Kyuafile:9: Unknown metadata property execenv.
> > > 
> > > From a combinatorics perspective, the first three options pop up to deal with
> > > that:
> > > a) Patch Kyua the same way for the supported STABLE branches so it will be
> > > able to run back ported tests based on execenv="jail" (it's not system ABI
> > > change after all)
> > > b) Exclusively patch Kyua framework for the supported STABLE branches to
> > > simply skip such tests (does not look to provide much benefit)
> > > c) Do not back port tests, only the fix/feature itself (kind of a bad idea)
> > > 
> > > 6 The Demo
> > > 
> > > My test environment showed promising run time numbers for almost the whole
> > > test suite (ZFS excluded). One of the tests yielded 36 min with test
> > > parallelism improvement versus 1 h 25 min without. In my case with 8 cores,
> > > the suite runs about 2 times faster with the improvement. [3]
> > > 
> > > 7 Action Points
> > > 
> > > My current vision of the plan looks as follows:
> > > - [ ] community: Review, testing, comments -- probably we want to change the
> > > design
> > > - [ ] committers: Help with the main commit -- it should hit freebsd/kyua
> > > GitHub fork first [4], then vendor branch, and merge to
> > > main after
> > > - [ ] igoro: Provide the subsequent PRs to separate FreeBSD specifics and fix
> > > existing Kyua tests
> > > - [ ] igoro: Provide the PRs to add brand new tests of Kyua itself to cover
> > > the new feature
> > > - [ ] igoro: Provide the respective documentation updates
> > > - [ ] igoro: Migrate some of the existing tests for the start, e.g. netpfil/pf
> > > - [ ] committers: Help with review and respective commits/merges
> > > 
> > > The plan is not strict, it depends on the discussion and interest of
> > > volunteers.
> > > 
> > > I hope that this proposal is found valuable for the project. If so, any help
> > > is appreciated.
> > > 
> > > [1] New tests exclusivity concern: https://reviews.freebsd.org/D42314
> > > [2] The Kyua patch: https://reviews.freebsd.org/D42350
> > > [3] The whole test suite demo: https://reviews.freebsd.org/D42410
> > > [4] The respective PR to the fork: https://github.com/freebsd/kyua/pull/224
> 
> > > Best regards, Igor.