Re: [PATCH 1/2] Add test for per-NS NFS client statistics

Re: [PATCH 1/2] Add test for per-NS NFS client statistics

[Petr Vorel]

Hi Martin,

> Check that /proc/net/rpc/nfs file exists in nested network namespaces.

> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---

> Only do the minimal check here. If the file exists in namespaces,
> nfsstat01.sh will take care of functional testing.

>  runtest/net.nfs                              |  2 ++
>  testcases/network/nfs/nfsstat01/nfsstat02.sh | 23 ++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>  create mode 100755 testcases/network/nfs/nfsstat01/nfsstat02.sh

> diff --git a/runtest/net.nfs b/runtest/net.nfs
> index 929868a7b..7f84457bc 100644
> --- a/runtest/net.nfs
> +++ b/runtest/net.nfs
> @@ -116,6 +116,8 @@ nfsstat01_v40_ip6t nfsstat01.sh -6 -v 4 -t tcp
>  nfsstat01_v41_ip6t nfsstat01.sh -6 -v 4.1 -t tcp
>  nfsstat01_v42_ip6t nfsstat01.sh -6 -v 4.2 -t tcp

> +nfsstat02 nfsstat02.sh
> +
>  fsx_v30_ip4u fsx.sh -v 3 -t udp
>  fsx_v30_ip4t fsx.sh -v 3 -t tcp
>  fsx_v40_ip4t fsx.sh -v 4 -t tcp
> diff --git a/testcases/network/nfs/nfsstat01/nfsstat02.sh b/testcases/network/nfs/nfsstat01/nfsstat02.sh
> new file mode 100755
> index 000000000..1e1bebe97
> --- /dev/null
> +++ b/testcases/network/nfs/nfsstat01/nfsstat02.sh
> @@ -0,0 +1,23 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2024 SUSE LLC <mdoucha@suse.cz>
> +
> +TST_TESTFUNC="do_test"
> +
> +# PURPOSE:  Check that /proc/net/rpc/nfs exists in nested network namespaces
I would point here a commit which added it or a patchset.

Shell API does not have functionality to point out missing kernel git commit,
but that might change. But even without it a comment is useful.

Maybe point out d47151b79e32 ("nfs: expose /proc/net/sunrpc/nfs in net namespaces")
and whole patchset
https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Tested on 6.8 and 6.11 mainline
Tested-by: Petr Vorel <pvorel@suse.cz>

Thanks for this test!

Kind regards,
Petr

> +do_test()
> +{
> +	local procfile="/proc/net/rpc/nfs"
> +
> +	if tst_rhost_run -c "test -e '$procfile'"; then
> +		tst_res TPASS "$procfile exists in net namespaces"
> +	else
> +		tst_res TFAIL "$procfile missing in net namespaces"
> +	fi
> +}
> +
> +# Force use of nested net namespace
> +unset RHOST
> +
> +. nfs_lib.sh
> +tst_run

Re: [PATCH 1/2] Add test for per-NS NFS client statistics

[Martin Doucha]

Hi,

On 30. 08. 24 22:09, Petr Vorel wrote:
> Hi Martin,
> 
>> +# PURPOSE:  Check that /proc/net/rpc/nfs exists in nested network namespaces
> I would point here a commit which added it or a patchset.
> 
> Shell API does not have functionality to point out missing kernel git commit,
> but that might change. But even without it a comment is useful.
> 
> Maybe point out d47151b79e32 ("nfs: expose /proc/net/sunrpc/nfs in net namespaces")
> and whole patchset
> https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/

Feel free to add the comment during merge.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

Re: [PATCH 2/2] nfsstat01: Check that RPC stats don't leak between net namespaces

[Petr Vorel]

> Hi all,

> > On Fri, Aug 30, 2024 at 04:13:40PM +0200, Martin Doucha wrote:
> > > When the NFS server and client run on the same host in different net
> > > namespaces, check that RPC calls from the client namespace don't
> > > change RPC statistics in the root namespace.

> > > Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> > > ---

> > > I've initially tried to test both NFS and RPC client stats but it appears
> > > that NFS client stats are still shared across all namespaces. Only RPC
> > > client stats are separate for each net namespace. The kernel patchset[1]
> > > which introduced per-NS stats confirms that only RPC stats have been changed.

> Yes, only RPC client stats needed to be fixed in LTP test.

OTOH there is also nfsd stats namespaced [2] as a second part of whole "Make nfs
and nfsd stats visible in network ns" patchset. I suppose we would need to
reverse client and server to detect this (IMHO worth of doing it).

Kind regards,
Petr

[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=author&q=4b14885411f74b2b0ce0eb2b39d0fffe54e5ca0d
[3] https://lore.kernel.org/linux-nfs/cover.1706212207.git.josef@toxicpanda.com/


> Kind regards,
> Petr

> > I believe that is correct, Josef changed only RPC counters. Which
> > counters did you expect also would be containerized, exactly?
> > Perhaps this issue should be raised on linux-nfs@vger, it could be
> > considered to be another information leak.


> > > If NFS client stats should be separate for each namespace as well, let
> > > me know and I'll return the second set of NS checks in patch v2.

> > > Tested on kernel v5.14 with Neil's backports.

> > > [1] https://lore.kernel.org/linux-nfs/cover.1708026931.git.josef@toxicpanda.com/

> > >  testcases/network/nfs/nfsstat01/nfsstat01.sh | 18 ++++++++++++++++--
> > >  1 file changed, 16 insertions(+), 2 deletions(-)

> > > diff --git a/testcases/network/nfs/nfsstat01/nfsstat01.sh b/testcases/network/nfs/nfsstat01/nfsstat01.sh
> > > index 8d7202cf3..3379c4d46 100755
> > > --- a/testcases/network/nfs/nfsstat01/nfsstat01.sh
> > > +++ b/testcases/network/nfs/nfsstat01/nfsstat01.sh
> > > @@ -22,6 +22,7 @@ get_calls()
> > >  	local name=$1
> > >  	local field=$2
> > >  	local nfs_f=$3
> > > +	local netns=${4:-rhost}
> > >  	local type="lhost"
> > >  	local calls opt

> > > @@ -30,7 +31,8 @@ get_calls()

> > >  	if tst_net_use_netns; then
> > >  		# In netns setup, rhost is the client
> > > -		[ "$nfs_f" = "nfs" ] && [ $NS_STAT_RHOST -ne 0 ] && type="rhost"
> > > +		[ "$nfs_f" = "nfs" ] && [ $NS_STAT_RHOST -ne 0 ] && \
> > > +			type="$netns"
> > >  	else
> > >  		[ "$nfs_f" != "nfs" ] && type="rhost"
> > >  	fi
> > > @@ -64,13 +66,14 @@ get_calls()
> > >  do_test()
> > >  {
> > >  	local client_calls server_calls new_server_calls new_client_calls
> > > -	local client_field server_field
> > > +	local client_field server_field root_calls new_root_calls
> > >  	local client_v=$VERSION server_v=$VERSION

> > >  	tst_res TINFO "checking RPC calls for server/client"

> > >  	server_calls="$(get_calls rpc 2 nfsd)"
> > >  	client_calls="$(get_calls rpc 2 nfs)"
> > > +	root_calls="$(get_calls rpc 2 nfs lhost)"

> > >  	tst_res TINFO "calls $server_calls/$client_calls"

> > > @@ -79,6 +82,7 @@ do_test()

> > >  	new_server_calls="$(get_calls rpc 2 nfsd)"
> > >  	new_client_calls="$(get_calls rpc 2 nfs)"
> > > +	new_root_calls="$(get_calls rpc 2 nfs lhost)"
> > >  	tst_res TINFO "new calls $new_server_calls/$new_client_calls"

> > >  	if [ "$new_server_calls" -le "$server_calls" ]; then
> > > @@ -93,6 +97,16 @@ do_test()
> > >  		tst_res TPASS "client RPC calls increased"
> > >  	fi

> > > +	if [ $NS_STAT_RHOST -ne 0 ]; then
> > > +		tst_res TINFO "Root NS client RPC calls: $root_calls => $new_root_calls"
> > > +
> > > +		if [ $root_calls -ne $new_root_calls ]; then
> > > +			tst_res TFAIL "RPC stats leaked between net namespaces"
> > > +		else
> > > +			tst_res TPASS "RPC stats stay within net namespaces"
> > > +		fi
> > > +	fi
> > > +
> > >  	tst_res TINFO "checking NFS calls for server/client"
> > >  	case $VERSION in
> > >  	2) client_field=13 server_field=13
> > > -- 
> > > 2.46.0