From nobody Thu Mar 21 02:59:21 2024 X-Original-To: freebsd-hackers@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4V0VYH08gvz5FcLp for ; Thu, 21 Mar 2024 02:59:31 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4V0VYG6sSLz4GSk; Thu, 21 Mar 2024 02:59:30 +0000 (UTC) (envelope-from kp@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1710989970; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=aGT3H/lfq77F/WFc73hFgYSg77PzsCg4a0pAyckQssY=; b=qGFd3sdEvNGadpPEvspCWlBLOrp4lZaRztqGQLb0J/vMhKqlskizJTA9Qt41s20KSZDNC8 DKG/4++0NOL5jGf9hiqfcgDNPVaF2KteTrs2YQxHOpXoB28VXhFQynS2CfeDORoehNZ1ii nBKcVBkb2Pj6vBAcS9n/7jQDOU6en/olfFR+xswnUvRPk246WtHtR1SXnXsIDQLR4dadkn eWvGb0tmLvV8FWpYYHz8eV/03h02YFKtCZKPcFnIihHAiXwPLCOi6zMx6b9Icl9RuYOrkX XP8MkG/r3HAc+1/f0z7FAFGKt21JEq2rt7jDqXD+ju4/ze3DpEg3/00Zpp/sPQ== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1710989970; a=rsa-sha256; cv=none; b=JlyC9SOUEc6OxGFdPnj/KIUBKLPpVjZmh6tXXAXaLsFxz5ByoTLiT3a+BYTZ+6cPXb0tDL g2OT7jL4aTulr4oJMikBPsRjxgT//5RaWBrdvfrEglsFn9wEPgNHg4FW1kYyaqJsQ5lcU7 9GsitGFYWu+HaGiaQgz7hUWlbF6nzcz7jLagZooepPo39VjBJ5txPHaGYCmtf9tRIQJHLj bsDyZMV42m/TlAebRLotrlLHFZ2/FJKng2CtrZv6gR4bJShf+cFPQKUeJNemU7oLY0xr8+ kJ5C7XNs2HbUPhex0E2Rb1oNEMM3yIrO3ocijcEGDLVWvXryl5lZ+2DsBKD7ag== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1710989970; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=aGT3H/lfq77F/WFc73hFgYSg77PzsCg4a0pAyckQssY=; b=vIMMzDpMYXHdBYoleXCFVAgGISJI5dNyLarmJnVQSDTCghhbem4aR3c3M4PVnyMYZ93j5Z z+8IZdMamWC2dt7ZLBMohELnkRkh5/cFOh40UY1XHmnfcvndB+M51xahDtPdT5fnnfQQfj 3SAPi7OvgrWNYTHsMtQdDVQE75UFDWpNOPg3dOpyvSr5MEDIbfxQx5sDrFhYX4j70RnHSr nlfW0JEqubGSN7a5MNDnUr2zaPSJpQRMPE9MIm4N/fNPDzxxRcFC8PwyY4DOaB7/aQD5Xi Czw1v1rQ3osymzdHx5BuoDIMQ2ugL6e3qhDaXmH+nAEc5WHEK8NXsvlGzHmSKA== Received: from venus.codepro.be (venus.codepro.be [5.9.86.228]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "mx1.codepro.be", Issuer "R3" (verified OK)) (Authenticated sender: kp) by smtp.freebsd.org (Postfix) with ESMTPSA id 4V0VYG4XczzXsV; Thu, 21 Mar 2024 02:59:30 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: by venus.codepro.be (Postfix, authenticated sender kp) id 5A96E4F8C6; Thu, 21 Mar 2024 03:59:27 +0100 (CET) From: Kristof Provost To: Igor Ostapenko Cc: freebsd-hackers@freebsd.org Subject: Re: Add jail execution environment support to the FreeBSD test suite Date: Thu, 21 Mar 2024 10:59:21 +0800 X-Mailer: MailMate (1.14r5937) Message-ID: In-Reply-To: References: <2bjQNp1msrv-_AqyamMun6kY-SCqbgPm3Q7DqVQHAYlqvFkiE1i85svfIT-QQdUG1cg3cKippyTyv8Z-5nbLu4WaMutgZQ7KT-YYo_5Pbro=@pm.me> List-Id: Technical discussions relating to FreeBSD List-Archive: https://lists.freebsd.org/archives/freebsd-hackers List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-hackers@freebsd.org MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=_MailMate_F71C8465-E854-4AF9-9342-2B9110ED52D5_=" Content-Transfer-Encoding: 8bit --=_MailMate_F71C8465-E854-4AF9-9342-2B9110ED52D5_= Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > 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. --=_MailMate_F71C8465-E854-4AF9-9342-2B9110ED52D5_= Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

<Picking this mail to resurrect the thread>

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:

di=
ff --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+=3D    frag6.py
 ATF_TESTS_PYTEST+=3D     nat66.py
 ATF_TESTS_PYTEST+=3D     sctp.py

-# 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"

 PROGS=3D divapp

That gets the test time, with parallelism=3D5, down from = 22 minutes to 5m40s.
So I=E2=80=99m rather keen to see this work land.

My read from the reactions here is that people are genera= lly 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 la= nd 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 functiona= lity, from bottom to
top:

1 ATF based tests

- The new "execenv" metadata property can be set to expli= citly 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 optiona= lly 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 Kyua= file level:
"execenv" and "execenv_jail".

- Note that historically ATF uses dotted style of metadat= a 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 lis= ted
environments are skipped.

- By default, this variable lists all execution environme= nts supported by a
Kyua binary, e.g. execenv=3D"host jail".

- This variable can be changed via "kyua.conf" or via kyu= a CLI's "-v"
parameter. For example, "kyua -v execenv=3Dhost test" will run only
host-based tests and skip jail-based ones.

- Current value of this variable can be examined with "ky= ua config".

The patch is https://reviews.freebsd.org/D42350.

Any help with review and testing is welcome. Its test pla= n 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.ostape= nko@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. Th= e latter supports
running tests in parallel. However, some tests cannot be run in parallel = and
are marked with is_exclusive=3D"true" metadata, which makes Kyua run such= tests
in sequence.

Many tests are not meant to be exclusive conceptually, th= ey are so for very
simple technical reasons. For instance, some network related tests are ba= sed
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.<= /p>

The project seeks more tests, and it's kind of a trend fo= r 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 test= s
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 ru= n in parallel and
decrease the test time for CI runs and for the runs within the developmen= t
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 executi= on 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=3D"jail" then Kyua will create a temporary jail, run such test wi= thin
it, and remove the jail. If execenv=3D"host" is provided or execenv metad= ata is
undefined then Kyua will run such test as it does today.

execenv_jail metadata takes effect only in case of execen= v=3D"jail". It allows a
test to request specific parameters for its jail. These parameters are si= mply
arguments to jail(8), e.g. execenv_jail=3D"vnet allow.raw_sockets".

4 The Adoption

ATF based tests can easily define this new metadata via K= yuafile 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 suit= e does it through a
Makefile:

TEST_METADATA+=3D execenv=3D"jail"
TEST_METADATA+=3D execenv_jail=3D"vnet allow.raw_sockets"

The patch got some little evolution, I started with a sin= gle 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 t= ricky
interface such way. The evolution reasoning can be found in the history o= f 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 u= se
execenv=3D"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 fi= le '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 b= e
able to run back ported tests based on execenv=3D"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 ide= a)

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 core= s,
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 cov= er
the new feature
- [ ] igoro: Provide the respective documentation updates
- [ ] igoro: Migrate some of the existing tests for the start, e.g. netpf= il/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 proje= ct. 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.

--=_MailMate_F71C8465-E854-4AF9-9342-2B9110ED52D5_=--