[nft PATCH 1/3] tests/shell: mount all of "/var/run" in "test-wrapper.sh"

[nft PATCH 1/3] tests/shell: mount all of "/var/run" in "test-wrapper.sh"

[Thomas Haller]

After reboot, "/var/run/netns" does not exist before we run the first
`ip netns add` command. Previously, "test-wrapper.sh" would mount a
tmpfs on that directory, but that fails, if the directory doesn't exist.
You will notice this, by deleting /var/run/netns (which only root can
delete or create, and which is wiped on reboot).

Instead, mount all of "/var/run". Then we can also create /var/run/netns
directory.

This means, any other content from /var/run is hidden too. That's
probably desirable, because it means we don't depend on stuff that
happens to be there. If we would require other content in /var/run, then
the test runner needs to be aware of the requirement and ensure it's
present. But best is just to not require anything. It's only iproute2
which insists on /var/run/netns.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/helpers/test-wrapper.sh | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
index e10360c9b266..13b918f8b8e1 100755
--- a/tests/shell/helpers/test-wrapper.sh
+++ b/tests/shell/helpers/test-wrapper.sh
@@ -23,11 +23,11 @@ START_TIME="$(cut -d ' ' -f1 /proc/uptime)"
 
 export TMPDIR="$NFT_TEST_TESTTMPDIR"
 
-CLEANUP_UMOUNT_RUN_NETNS=n
+CLEANUP_UMOUNT_VAR_RUN=n
 
 cleanup() {
-	if [ "$CLEANUP_UMOUNT_RUN_NETNS" = y ] ; then
-		umount "/var/run/netns" || :
+	if [ "$CLEANUP_UMOUNT_VAR_RUN" = y ] ; then
+		umount "/var/run" &>/dev/null || :
 	fi
 }
 
@@ -38,16 +38,20 @@ printf '%s\n' "$TEST" > "$NFT_TEST_TESTTMPDIR/name"
 read tainted_before < /proc/sys/kernel/tainted
 
 if [ "$NFT_TEST_HAS_UNSHARED_MOUNT" = y ] ; then
-	# We have a private mount namespace. We will mount /run/netns as a tmpfs,
-	# this is useful because `ip netns add` wants to add files there.
+	# We have a private mount namespace. We will mount /var/run/ as a tmpfs.
 	#
-	# When running as rootless, this is necessary to get such tests to
-	# pass.  When running rootful, it's still useful to not touch the
-	# "real" /var/run/netns of the system.
-	mkdir -p /var/run/netns
-	if mount -t tmpfs --make-private "/var/run/netns" ; then
-		CLEANUP_UMOUNT_RUN_NETNS=y
+	# The main purpose is so that we can create /var/run/netns, which is
+	# required for `ip netns add` to work.  When running as rootless, this
+	# is necessary to get such tests to pass. When running rootful, it's
+	# still useful to not touch the "real" /var/run/netns of the system.
+	#
+	# Note that this also hides everything that might reside in /var/run.
+	# That is desirable, as tests should not depend on content there (or if
+	# they do, we need to explicitly handle it as appropriate).
+	if mount -t tmpfs --make-private "/var/run" ; then
+		CLEANUP_UMOUNT_VAR_RUN=y
 	fi
+	mkdir -p /var/run/netns
 fi
 
 TEST_TAGS_PARSED=0
-- 
2.41.0

[nft PATCH 2/3] tests/shell: preserve result directory with NFT_TEST_FAIL_ON_SKIP

[Thomas Haller]

On a successful run, the result directory will be deleted (unless run
with "-k|--keep-logs" option or NFT_TEST_KEEP_LOGS=y).

With NFT_TEST_FAIL_ON_SKIP=y, when there are no failures but skipped
tests, also preserve the result.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 4ff0b55ad7b1..7672b2fe5074 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -850,7 +850,12 @@ echo ""
 kmemleak_found=0
 check_kmemleak_force
 
-if [ "$failed" -gt 0 ] || [ "$NFT_TEST_FAIL_ON_SKIP" = y -a "$skipped" -gt 0 ] ; then
+failed_total="$failed"
+if [ "$NFT_TEST_FAIL_ON_SKIP" = y ] ; then
+	failed_total="$((failed_total + skipped))"
+fi
+
+if [ "$failed_total" -gt 0 ] ; then
 	RR="$RED"
 elif [ "$skipped" -gt 0 ] ; then
 	RR="$YELLOW"
@@ -875,7 +880,7 @@ END_TIME="$(cut -d ' ' -f1 /proc/uptime)"
 WALL_TIME="$(awk -v start="$START_TIME" -v end="$END_TIME" "BEGIN { print(end - start) }")"
 printf "%s\n" "$WALL_TIME" "$START_TIME" "$END_TIME" > "$NFT_TEST_TMPDIR/times"
 
-if [ "$failed" -gt 0 -o "$NFT_TEST_KEEP_LOGS" = y ] ; then
+if [ "$failed_total" -gt 0 -o "$NFT_TEST_KEEP_LOGS" = y ] ; then
 	msg_info "check the temp directory \"$NFT_TEST_TMPDIR\" (\"$NFT_TEST_LATEST\")"
 	msg_info "   ls -lad \"$NFT_TEST_LATEST\"/*/*"
 	msg_info "   grep -R ^ \"$NFT_TEST_LATEST\"/"
-- 
2.41.0

[nft PATCH 3/3] tests/shell: add "-S|--setup-host" option to set sysctl for rootless tests

[Thomas Haller]

Most tests can run just fine without root. A few of them will fail if
/proc/sys/net/core/{wmem_max,rmem_max} is too small (as it is by default
on the host).

The easy workaround is to bump those limits once. This has to be
repeated after each reboot.

Doing that manually (every time) is cumbersome. Add a "--setup-host"
option for that.

Usage:

  $ sudo ./tests/shell/run-tests.sh -S
  Setting up host for running as rootless (requires root).
      echo 4096000 > /proc/sys/net/core/rmem_max (previous value 100000)
      echo 4096000 > /proc/sys/net/core/wmem_max (previous value 100000)

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 46 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 7672b2fe5074..22105c2e90e2 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -166,6 +166,9 @@ usage() {
 	echo " -s|--sequential : Sets NFT_TEST_JOBS=0, which also enables global cleanups."
 	echo "                   Also sets NFT_TEST_SHUFFLE_TESTS=n if left unspecified."
 	echo " -Q|--quick      : Sets NFT_TEST_SKIP_slow=y."
+	echo " -S|--setup-host : Modify the host to run as rootless. Otherwise, some tests will be"
+	echo "                   skipped. Basically, this bumps /proc/sys/net/core/{wmem_max,rmem_max}."
+	echo "                   Must run as root and this option must be specified alone."
 	echo " --              : Separate options from tests."
 	echo " [TESTS...]      : Other options are treated as test names,"
 	echo "                   that is, executables that are run by the runner."
@@ -302,10 +305,25 @@ export NFT_TEST_RANDOM_SEED
 
 TESTS=()
 
+SETUP_HOST=
+SETUP_HOST_OTHER=
+
+ARGV_ORIG=( "$@" )
+
 while [ $# -gt 0 ] ; do
 	A="$1"
 	shift
 	case "$A" in
+		-S|--setup-host)
+			;;
+		*)
+			SETUP_HOST_OTHER=y
+			;;
+	esac
+	case "$A" in
+		-S|--setup-host)
+			SETUP_HOST="$A"
+			;;
 		-v)
 			VERBOSE=y
 			;;
@@ -353,6 +371,34 @@ while [ $# -gt 0 ] ; do
 	esac
 done
 
+sysctl_bump() {
+	local sysctl="$1"
+	local val="$2"
+	local cur;
+
+	cur="$(cat "$sysctl" 2>/dev/null)" || :
+	if [ -n "$cur" -a "$cur" -ge "$val" ] ; then
+		echo "# Skip: echo $val > $sysctl (current value $cur)"
+		return 0
+	fi
+	echo "    echo $val > $sysctl (previous value $cur)"
+	echo "$val" > "$sysctl"
+}
+
+setup_host() {
+	echo "Setting up host for running as rootless (requires root)."
+	sysctl_bump /proc/sys/net/core/rmem_max $((4000*1024)) || return $?
+	sysctl_bump /proc/sys/net/core/wmem_max $((4000*1024)) || return $?
+}
+
+if [ -n "$SETUP_HOST" ] ; then
+	if [ "$SETUP_HOST_OTHER" = y ] ; then
+		msg_error "The $SETUP_HOST option must be specified alone."
+	fi
+	setup_host
+	exit $?
+fi
+
 find_tests() {
 	find "$1" -type f -executable | sort
 }
-- 
2.41.0

Re: [nft PATCH 1/3] tests/shell: mount all of "/var/run" in "test-wrapper.sh"

[Pablo Neira Ayuso]

Hi Thomas,

Series LGTM, one question below

On Fri, Oct 06, 2023 at 11:42:18AM +0200, Thomas Haller wrote:
> After reboot, "/var/run/netns" does not exist before we run the first
> `ip netns add` command. Previously, "test-wrapper.sh" would mount a
> tmpfs on that directory, but that fails, if the directory doesn't exist.
> You will notice this, by deleting /var/run/netns (which only root can
> delete or create, and which is wiped on reboot).
> 
> Instead, mount all of "/var/run". Then we can also create /var/run/netns
> directory.

Maybe create a specify mount point for this? This will be created
once, then it will remain there for those that run tests?

> This means, any other content from /var/run is hidden too. That's
> probably desirable, because it means we don't depend on stuff that
> happens to be there. If we would require other content in /var/run, then
> the test runner needs to be aware of the requirement and ensure it's
> present. But best is just to not require anything. It's only iproute2
> which insists on /var/run/netns.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  tests/shell/helpers/test-wrapper.sh | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
> index e10360c9b266..13b918f8b8e1 100755
> --- a/tests/shell/helpers/test-wrapper.sh
> +++ b/tests/shell/helpers/test-wrapper.sh
> @@ -23,11 +23,11 @@ START_TIME="$(cut -d ' ' -f1 /proc/uptime)"
>  
>  export TMPDIR="$NFT_TEST_TESTTMPDIR"
>  
> -CLEANUP_UMOUNT_RUN_NETNS=n
> +CLEANUP_UMOUNT_VAR_RUN=n
>  
>  cleanup() {
> -	if [ "$CLEANUP_UMOUNT_RUN_NETNS" = y ] ; then
> -		umount "/var/run/netns" || :
> +	if [ "$CLEANUP_UMOUNT_VAR_RUN" = y ] ; then
> +		umount "/var/run" &>/dev/null || :
>  	fi
>  }
>  
> @@ -38,16 +38,20 @@ printf '%s\n' "$TEST" > "$NFT_TEST_TESTTMPDIR/name"
>  read tainted_before < /proc/sys/kernel/tainted
>  
>  if [ "$NFT_TEST_HAS_UNSHARED_MOUNT" = y ] ; then
> -	# We have a private mount namespace. We will mount /run/netns as a tmpfs,
> -	# this is useful because `ip netns add` wants to add files there.
> +	# We have a private mount namespace. We will mount /var/run/ as a tmpfs.
>  	#
> -	# When running as rootless, this is necessary to get such tests to
> -	# pass.  When running rootful, it's still useful to not touch the
> -	# "real" /var/run/netns of the system.
> -	mkdir -p /var/run/netns
> -	if mount -t tmpfs --make-private "/var/run/netns" ; then
> -		CLEANUP_UMOUNT_RUN_NETNS=y
> +	# The main purpose is so that we can create /var/run/netns, which is
> +	# required for `ip netns add` to work.  When running as rootless, this
> +	# is necessary to get such tests to pass. When running rootful, it's
> +	# still useful to not touch the "real" /var/run/netns of the system.
> +	#
> +	# Note that this also hides everything that might reside in /var/run.
> +	# That is desirable, as tests should not depend on content there (or if
> +	# they do, we need to explicitly handle it as appropriate).
> +	if mount -t tmpfs --make-private "/var/run" ; then
> +		CLEANUP_UMOUNT_VAR_RUN=y
>  	fi
> +	mkdir -p /var/run/netns
>  fi
>  
>  TEST_TAGS_PARSED=0
> -- 
> 2.41.0
> 

Re: [nft PATCH 1/3] tests/shell: mount all of "/var/run" in "test-wrapper.sh"

[Thomas Haller]

On Fri, 2023-10-06 at 13:51 +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 06, 2023 at 11:42:18AM +0200, Thomas Haller wrote:
> > After reboot, "/var/run/netns" does not exist before we run the
> > first
> > `ip netns add` command. Previously, "test-wrapper.sh" would mount a
> > tmpfs on that directory, but that fails, if the directory doesn't
> > exist.
> > You will notice this, by deleting /var/run/netns (which only root
> > can
> > delete or create, and which is wiped on reboot).
> > 
> > Instead, mount all of "/var/run". Then we can also create
> > /var/run/netns
> > directory.
> 
> Maybe create a specify mount point for this? This will be created
> once, then it will remain there for those that run tests?

Hi Pablo,

I don't understand. Could you please elaborate?

Note that the last commit adds a "--setup-host" option. I considered
whether that should also `mkdir -p /var/run/netns`, but I decided
against it, because Patch 1/3 solves it better (and makes it
unnecessary).

As tests run in parallel, they all should use their own private
/var/run/netns to not interfere with each other. Hence, they test-
wrapper.sh would still (at least) remount /var/run/netns. While at it,
Patch 1/3 remounting /var/run is even preferable, because there should
be nothing in /var/run that is required by tests or that should be
shared (or if we ever learn that there would be something, than special
action could be taken).


Thomas

Re: [nft PATCH 1/3] tests/shell: mount all of "/var/run" in "test-wrapper.sh"

[Pablo Neira Ayuso]

Series applied, thanks

And thanks for explaining.