Re: [PATCH v3.1 15/34] check: run tests in a private pid/mount namespace

[Dave Chinner <david@fromorbit.com>]

On Tue, Feb 25, 2025 at 07:49:10AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 25, 2025 at 11:27:19AM +0000, Shinichiro Kawasaki wrote:
> > On Feb 14, 2025 / 13:13, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > As mentioned in the previous patch, trying to isolate processes from
> > > separate test instances through the use of distinct Unix process
> > > sessions is annoying due to the many complications with signal handling.
> > > 
> > > Instead, we could just use nsexec to run the test program with a private
> > > pid namespace so that each test instance can only see its own processes;
> > > and private mount namespace so that tests writing to /tmp cannot clobber
> > > other tests or the stuff running on the main system.  Further, the
> > > process created by the clone(CLONE_NEWPID) call is considered the init
> > > process of that pid namespace, so all processes will be SIGKILL'd when
> > > the init process terminates, so we no longer need systemd scopes for
> > > externally enforced cleanup.
> > > 
> > > However, it's not guaranteed that a particular kernel has pid and mount
> > > namespaces enabled.  Mount (2.4.19) and pid (2.6.24) namespaces have
> > > been around for a long time, but there's no hard requirement for the
> > > latter to be enabled in the kernel.  Therefore, this bugfix slips
> > > namespace support in alongside the session id thing.
> > > 
> > > Declaring CONFIG_PID_NS=n a deprecated configuration and removing
> > > support should be a separate conversation, not something that I have to
> > > do in a bug fix to get mainline QA back up.
> > > 
> > > Note that the new helper cannot unmount the /proc it inherits before
> > > mounting a pidns-specific /proc because generic/504 relies on being able
> > > to read the init_pid_ns (aka systemwide) version of /proc/locks to find
> > > a file lock that was taken and leaked by a process.
> > 
> > Hello Darrick,
> > 
> > I ran fstests for zoned btrfs using the latest fstests tag v2025.02.23, and
> > observed all test cases failed with my set up. I bisected and found that this
> > commit is the trigger. Let me share my observations.
> > 
> > For example, btrfs/001.out.bad contents are as follows:
> > 
> >   QA output created by 001
> >   mount: bad usage
> >   Try 'mount --help' for more information.
> >   common/rc: retrying test device mount with external set
> >   mount: bad usage
> >   Try 'mount --help' for more information.
> >   common/rc: could not mount /dev/sda on common/config: TEST_DIR (/tmp/test) is not a directory
> > 
> > As the last line above shows, fstests failed to find out TEST_DIR, /tmp/test.
> > 
> > My set up uses mount point directories in tmpfs, /tmp/*:
> > 
> >   export TEST_DIR=/tmp/test
> >   export SCRATCH_MNT=/tmp/scratch
> > 
> > I guessed that tmpfs might be a cause. As a trial, I modified these to,
> > 
> >   export TEST_DIR=/var/test
> >   export SCRATCH_MNT=/var/scratch
> > 
> > then I observed the failures disappeared. I guess this implies that the commit
> > for the private pid/mount namespace makes tmpfs unique to each namespace. Then,
> > the the mount points in tmpfs were not found in the private namespaces context,
> > probably.
> 
> Yes, /tmp is now private to the test program (e.g. tests/btrfs/001) so
> that tests run in parallel cannot interfere with each other.
> 
> > If this guess is correct, in indicates that tmpfs can no longer be used for
> > fstests mount points. Is this expected?
> 
> Expected, yes.  But still broken for you. :(
> 
> I can think of a few solutions:
> 
> 1. Perhaps run_privatens could detect that TEST_DIR/SCRATCH_MNT start
> with "/tmp" and bind mount them into the private /tmp before it starts
> the test.

Which then makes it specific to test running, and that makes it
less suited to use from check-parallel (or any other generic test
context).

> 2. fstests could take care of defining and mkdir'ing the
> TEST_DIR/SCRATCH_MNT directories and users no longer have to create
> them.  It might, however, be useful to have them accessible to someone
> who has ssh'd in to look around after a failure.

check-parallel already does this, and it leaves them around after
the test, so....

4. use check-parallel.

> 3. Everyone rewrites their fstests configs to choose something outside
> of /tmp (e.g. /var/tmp/{test,scratch})?

How many people actually use /tmp for fstests mount points?

IMO, it's better for ongoing maintenance to drop support for /tmp
based mount points (less complexity in infrastructure setup). If
there are relatively few ppl who do this, perhaps it would be best
to treat this setup the same as the setsid encapsulation. i.e. works
for now, but is deprecated and is going to be removed in a years
time....

Then we can simply add a /tmp test to the HAVE_PRIVATENS setting and
avoid using a private ns for these setups for now. This gives
everyone who does use these setups time to migrate to a different
setup that will work with private namespaces correctly, whilst
keeping the long term maintenance burden of running tests in private
namespaces down to a minimum.

-Dave.
-- 
Dave Chinner
david@fromorbit.com