| Message ID | 20231006094226.711628-1-thaller@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft,1/3] tests/shell: mount all of "/var/run" in "test-wrapper.sh" | expand |
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 >
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
Series applied, thanks And thanks for explaining.
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
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(-)