Re: [PATCH 08/23] common: fix pkill by running test program in a separate session

[Dave Chinner <david@fromorbit.com>]

On Wed, Jan 22, 2025 at 01:46:09PM -0800, Darrick J. Wong wrote:
> On Wed, Jan 22, 2025 at 08:42:49PM +1100, Dave Chinner wrote:
> > On Tue, Jan 21, 2025 at 11:05:20PM -0800, Darrick J. Wong wrote:
> > > On Wed, Jan 22, 2025 at 05:08:17PM +1100, Dave Chinner wrote:
> > > > On Tue, Jan 21, 2025 at 08:24:00PM -0800, Darrick J. Wong wrote:
> > > > > On Tue, Jan 21, 2025 at 02:28:26PM +1100, Dave Chinner wrote:
> > > > > > On Thu, Jan 16, 2025 at 03:27:15PM -0800, Darrick J. Wong wrote:
> > > > > > > c) Putting test subprocesses in a systemd sub-scope and telling systemd
> > > > > > > to kill the sub-scope could work because ./check can already use it to
> > > > > > > ensure that all child processes of a test are killed.  However, this is
> > > > > > > an *optional* feature, which means that we'd have to require systemd.
> > > > > > 
> > > > > > ... requiring systemd was somewhat of a show-stopper for testing
> > > > > > older distros.
> > > > > 
> > > > > Isn't RHEL7 the oldest one at this point?  And it does systemd.  At this
> > > > > point the only reason I didn't go full systemd is out of consideration
> > > > > for Devuan, since they probably need QA.
> > > > 
> > > > I have no idea what is out there in distro land vs what fstests
> > > > "supports". All I know is that there are distros out there that
> > > > don't use systemd.
> > > > 
> > > > It feels like poor form to prevent generic filesystem QA
> > > > infrastructure from running on those distros by making an avoidable
> > > > choice to tie the infrastructure exclusively to systemd-based
> > > > functionality....
> > > 
> > > Agreed, though at some point after these bugfixes are merged I'll see if
> > > I can build on the existing "if you have systemd then ___ else here's
> > > your shabby opencoded version" logic in fstests to isolate the ./checks
> > > from each other a little better.  It'd be kinda nice if we could
> > > actually just put them in something resembling a modernish container,
> > > albeit with the same underlying fs.
> > 
> > Agreed, but I don't think we need to depend on systemd for that,
> > either.
> > 
> > > <shrug> Anyone else interested in that?
> > 
> > check-parallel has already started down that road with the
> > mount namespace isolation it uses for the runner tasks via
> > src/nsexec.c.
> > 
> > My plan has been to factor more of the check test running code
> > (similar to what I did with the test list parsing) so that the
> > check-parallel can iterate sections itself and runners can execute
> > individual tests directly, rather than bouncing them through check
> > to execute a set of tests serially. Then check-parallel could do
> > whatever it needed to isolate individual tests from each other and
> > nothing in check would need to change.
> > 
> > Now I'm wondering if I can just run each runner's check instance
> > in it's own private PID namespace as easily as I'm running them in
> > their own private mount namespace...
> > 
> > Hmmm - looks like src/nsexec.c can create new PID namespaces via
> > the "-p" option. I haven't used that before - I wonder if that's a
> > better solution that using per-test session IDs to solve the pkill
> > --parent problem? Something to look into in the morning....
> 
> I tried that -- it appears to work, but then:
> 
> # ./src/nsexec -p bash
> Current time: Wed Jan 22 13:43:33 PST 2025; Terminal: /dev/pts/0
> # ps
> fatal library error, lookup self
> # 

That looks like a bug in whatever distro you are using - it works
as it should here on a recent debian unstable userspace.

Note, however, that ps will show all processes in both the parent
and the child namespace the shell is running on because the contents
of /proc are the same for both.

However, because we are also using private mount namespaces for the
check process, pid_namespaces(7) tells us:

/proc and PID namespaces

       A /proc filesystem shows (in the /proc/pid directories) only
       processes visible in the PID namespace of the process that
       performed the mount, even if the /proc filesystem is viewed
       from processes in other namespaces.

       After creating a new PID namespace, it is useful for the
       child to change its root directory and mount a new procfs
       instance at /proc so that tools  such  as  ps(1) work
>>>    correctly.  If a new mount namespace is simultaneously
>>>    created by including CLONE_NEWNS in the flags argument of
>>>    clone(2) or unshare(2), then it isn't necessary to change the
>>>    root directory: a new procfs instance can be mounted directly
>>>    over /proc.

       From a shell, the command to mount /proc is:

           $ mount -t proc proc /proc

       Calling readlink(2) on the path /proc/self yields the process
       ID of the caller in the PID namespace of the procfs mount
       (i.e., the PID name‐ space of the process that mounted the
       procfs).  This can be useful for introspection purposes, when
       a process wants to discover its  PID  in other namespaces.

This appears to give us an environment that only shows the processes
within the current PID namespace:

$ sudo src/nsexec -p -m bash
# mount -t proc proc /proc
# ps waux
USER         PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root           1  0.0  0.0   7384  3744 pts/1    S    11:55   0:00 bash
root          72  0.0  0.0   8300  3736 pts/1    R+   12:04   0:00 ps waux
# pstree -N pid
[4026538173]
bash───pstree
#

Yup, there we go - we have full PID isolation for this shell.

OK, I suspect this is a better way to proceed for check-parallel than
to need session IDs for individual tests and wrappers for
pgrep/pkill/pidwait. Let me see what breaks when I use this.....

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