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

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Feb 26, 2025 at 02:13:00AM +0000, Shinichiro Kawasaki wrote:
> On Feb 26, 2025 / 08:29, Dave Chinner wrote:
> > 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. :(
> 
> Darrick, thanks for the clarifications.
> 
> > > 
> > > 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.
> 
> I took a quick look in check-parallel. IIUC, it creates loop devices then it can
> not run with real storage devices. I run fstests with real zoned storage
> devices regularly, so I'm afraid that this solution does not fit my use case.
> 
> > 
> > > 3. Everyone rewrites their fstests configs to choose something outside
> > > of /tmp (e.g. /var/tmp/{test,scratch})?
> 
> I'm okay with this, since it is not a big deal to change the mount points in my
> test environments.
> 
> If this solution is chosen, I suggest to document the restriction and/or have
> the test script to check the restriction, to avoid the confusion.

I would certainly do that if everyone else agrees to the additional
restriction + adding the necessary checks to ./check?

--D

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