Date: Wed, 03 Apr 2024 14:13:56 +0000 From: Igor Ostapenko <igor.ostapenko@pm.me> To: freebsd-hackers@freebsd.org Cc: "freebsd-testing@FreeBSD.org" <freebsd-testing@FreeBSD.org>, Kristof Provost <kp@FreeBSD.org> Subject: Re: Add jail execution environment support to the FreeBSD test suite Message-ID: <8H_refsJfTozkldDQEkQFQY-RBauBRmKOJRWFfdSSZkxJ6KOmarYb8Fk97FjQ1yGgZhKGmkbZWgsgbaLgPhExtasz8OLJujk-54AO_erA5U=@pm.me> In-Reply-To: <B7E897BE-3DA2-4B95-9011-929E833B1A0B@FreeBSD.org> References: <2bjQNp1msrv-_AqyamMun6kY-SCqbgPm3Q7DqVQHAYlqvFkiE1i85svfIT-QQdUG1cg3cKippyTyv8Z-5nbLu4WaMutgZQ7KT-YYo_5Pbro=@pm.me> <BIsilmF1Kw7IA2Hn-S6XjAriQbMZf3VGECDEvbMD5tFgYIeyFvjLL-rEJyo-klhhSaqx9V9Hn2uZWLklKjbxC9MaM9jhlgwa4lQUfS3SGPY=@pm.me> <B7E897BE-3DA2-4B95-9011-929E833B1A0B@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 th=
e
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>
>=20
> I=E2=80=99ve been toying with this patch. Adding only the following patch=
I can get Kyua to run the pf tests in parallel:
>=20
> diff --git a/tests/sys/netpfil/pf/Makefile b/tests/sys/netpfil/pf/Mak=
efile
> index 867b98e5f6c2..c2f0f15fa798 100644
> --- a/tests/sys/netpfil/pf/Makefile
> +++ b/tests/sys/netpfil/pf/Makefile
> @@ -51,8 +51,8 @@ ATF_TESTS_PYTEST+=3D frag6.py
> ATF_TESTS_PYTEST+=3D nat66.py
> ATF_TESTS_PYTEST+=3D sctp.py
> =20
> -# Tests reuse jail names and so cannot run in parallel.
> -TEST_METADATA+=3D is_exclusive=3Dtrue
> +TEST_METADATA+=3D execenv=3D"jail"
> +TEST_METADATA+=3D execenv_jail=3D"vnet allow.raw_sockets"
> =20
> PROGS=3D divapp
> =20
>=20
> That gets the test time, with parallelism=3D5, down from 22 minutes to 5m=
40s.
> So I=E2=80=99m rather keen to see this work land.
>=20
> My read from the reactions here is that people are generally okay with th=
e approach, especially (I assume) given that the current version lets us tu=
rn this on on a per-test basis.
>=20
> Is there anything else anyone wants to raise before we land this?
>=20
> Best regards,
> Kristof
>=20
> On 28 Feb 2024, at 2:32, Igor Ostapenko wrote:
>=20
> > Hi,
> >=20
> > The patch was updated after the recent discussion.
> >=20
> > Currently, the patch provides the following new functionality, from bot=
tom to
> > top:
> >=20
> > 1 ATF based tests
> >=20
> > - 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".
> >=20
> > - 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".
> >=20
> > 2 Kyuafile
> >=20
> > - The same new metadata properties can be defined on Kyuafile level:
> > "execenv" and "execenv_jail".
> >=20
> > - Note that historically ATF uses dotted style of metadata naming, whil=
e
> > Kyua uses underscore style. Hence "execenv.jail" vs. "execenv_jail".
> >=20
> > 3 kyua.conf, kyua CLI
> >=20
> > - The new "execenv" engine configuration variable can be set to a list =
of
> > execution environments to run only tests designed for. Tests of not lis=
ted
> > environments are skipped.
> >=20
> > - By default, this variable lists all execution environments supported =
by a
> > Kyua binary, e.g. execenv=3D"host jail".
> >=20
> > - This variable can be changed via "kyua.conf" or via kyua CLI's "-v"
> > parameter. For example, "kyua -v execenv=3Dhost test" will run only
> > host-based tests and skip jail-based ones.
> >=20
> > - Current value of this variable can be examined with "kyua config".
> >=20
> > The patch is https://reviews.freebsd.org/D42350.
> >=20
> > 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.
> >=20
> > Best regards, Igor.
> >=20
> > On Thursday, February 22nd, 2024 at 10:57 PM, igor.ostapenko@pm.me <igo=
r.ostapenko@pm.me> wrote:
> >=20
> > > Hi FreeBSD developers,
> > >=20
> > > There is a proposal to improve the FreeBSD test suite.
> > >=20
> > > 1 The Problem
> > >=20
> > > The FreeBSD test suite is based on the Kyua framework. The latter sup=
ports
> > > running tests in parallel. However, some tests cannot be run in paral=
lel and
> > > are marked with is_exclusive=3D"true" metadata, which makes Kyua run =
such tests
> > > in sequence.
> > >=20
> > > Many tests are not meant to be exclusive conceptually, they are so fo=
r very
> > > simple technical reasons. For instance, some network related tests ar=
e based
> > > on jail and vnet usage. It's convenient for such tests and it provide=
s a lot
> > > of isolation already not to conflict with other tests. But they are s=
till
> > > marked as exclusive due to the shared space of jail names, routing, e=
tc.
> > >=20
> > > 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=3D"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]
> > >=20
> > > If such tests were 100% isolated they would be able to run in paralle=
l and
> > > decrease the test time for CI runs and for the runs within the develo=
pment
> > > process.
> > >=20
> > > 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.
> > >=20
> > > 2 The Idea
> > >=20
> > > 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.
> > >=20
> > > 3 The Implementation
> > >=20
> > > There is a lot of work done already and the working patch passed the =
initial
> > > review (thanks to markj@ and ngie@). [2]
> > >=20
> > > It adds a new concept to the Kyua framework -- an execution environme=
nt. Two
> > > new metadata were added for that: execenv and execenv_jail.
> > >=20
> > > execenv is a switch to select an environment. If a test's metadata de=
fines
> > > execenv=3D"jail" then Kyua will create a temporary jail, run such tes=
t within
> > > it, and remove the jail. If execenv=3D"host" is provided or execenv m=
etadata is
> > > undefined then Kyua will run such test as it does today.
> > >=20
> > > execenv_jail metadata takes effect only in case of execenv=3D"jail". =
It allows a
> > > test to request specific parameters for its jail. These parameters ar=
e simply
> > > arguments to jail(8), e.g. execenv_jail=3D"vnet allow.raw_sockets".
> > >=20
> > > 4 The Adoption
> > >=20
> > > ATF based tests can easily define this new metadata via Kyuafile or d=
irectly,
> > > e.g. for atf-sh based tests:
> > >=20
> > > 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
> > > }
> > >=20
> > > Non-ATF based ones will do it via Kyuafile. Our test suite does it th=
rough a
> > > Makefile:
> > >=20
> > > TEST_METADATA+=3D execenv=3D"jail"
> > > TEST_METADATA+=3D execenv_jail=3D"vnet allow.raw_sockets"
> > >=20
> > > 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 le=
ss tricky
> > > interface such way. The evolution reasoning can be found in the histo=
ry of the
> > > respective Differential. [2]
> > >=20
> > > 5 MFC Concerns
> > >=20
> > > For now, I see at least one issue from the usual project workflow per=
spective.
> > > Let's imagine that the Kyua framework got this execenv feature commit=
ted to
> > > 15-CURRENT, we started to convert existing tests and create new ones =
to use
> > > execenv=3D"jail". If some feature or a bug fix needs to be ported bac=
k to
> > > 14-STABLE or 13-STABLE, then "old" Kyua without execenv feature will =
fail to
> > > run such tests:
> > >=20
> > > kyua: E: Load of 'Kyuafile' failed: Failed to load Lua file 'Kyuafile=
': Kyuafile:9: Unknown metadata property execenv.
> > >=20
> > > From a combinatorics perspective, the first three options pop up to d=
eal with
> > > that:
> > > a) Patch Kyua the same way for the supported STABLE branches so it wi=
ll be
> > > able to run back ported tests based on execenv=3D"jail" (it's not sys=
tem 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)
> > >=20
> > > 6 The Demo
> > >=20
> > > 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]
> > >=20
> > > 7 Action Points
> > >=20
> > > My current vision of the plan looks as follows:
> > > - [ ] community: Review, testing, comments -- probably we want to cha=
nge 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. n=
etpfil/pf
> > > - [ ] committers: Help with review and respective commits/merges
> > >=20
> > > The plan is not strict, it depends on the discussion and interest of
> > > volunteers.
> > >=20
> > > I hope that this proposal is found valuable for the project. If so, a=
ny help
> > > is appreciated.
> > >=20
> > > [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/pu=
ll/224
>=20
> > > Best regards, Igor.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8H_refsJfTozkldDQEkQFQY-RBauBRmKOJRWFfdSSZkxJ6KOmarYb8Fk97FjQ1yGgZhKGmkbZWgsgbaLgPhExtasz8OLJujk-54AO_erA5U=>