| Message ID | 20231124092736.3673263-2-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Conver all net selftests to run in unique namespace | expand |
Hangbin Liu <liuhangbin@gmail.com> writes: > Add a lib.sh for net selftests. This file can be used to define commonly > used variables and functions. > > Add function setup_ns() for user to create unique namespaces with given > prefix name. > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > tools/testing/selftests/net/Makefile | 2 +- > tools/testing/selftests/net/lib.sh | 98 ++++++++++++++++++++++++++++ > 2 files changed, 99 insertions(+), 1 deletion(-) > create mode 100644 tools/testing/selftests/net/lib.sh > > diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile > index 9274edfb76ff..14bd68da7466 100644 > --- a/tools/testing/selftests/net/Makefile > +++ b/tools/testing/selftests/net/Makefile > @@ -54,7 +54,7 @@ TEST_PROGS += ip_local_port_range.sh > TEST_PROGS += rps_default_mask.sh > TEST_PROGS += big_tcp.sh > TEST_PROGS_EXTENDED := in_netns.sh setup_loopback.sh setup_veth.sh > -TEST_PROGS_EXTENDED += toeplitz_client.sh toeplitz.sh > +TEST_PROGS_EXTENDED += toeplitz_client.sh toeplitz.sh lib.sh > TEST_GEN_FILES = socket nettest > TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any > TEST_GEN_FILES += tcp_mmap tcp_inq psock_snd txring_overwrite > diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh > new file mode 100644 > index 000000000000..239ab2beb438 > --- /dev/null > +++ b/tools/testing/selftests/net/lib.sh > @@ -0,0 +1,98 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0 > + > +############################################################################## > +# Defines > + > +# Kselftest framework requirement - SKIP code is 4. > +ksft_skip=4 > +# namespace list created by setup_ns > +NS_LIST="" > + > +############################################################################## > +# Helpers > +busywait() > +{ > + local timeout=$1; shift > + > + local start_time="$(date -u +%s%3N)" > + while true > + do > + local out > + out=$($@) > + local ret=$? > + if ((!ret)); then > + echo -n "$out" > + return 0 > + fi > + > + local current_time="$(date -u +%s%3N)" > + if ((current_time - start_time > timeout)); then > + echo -n "$out" > + return 1 > + fi > + done > +} This is lifted from forwarding/lib.sh, right? Would it make sense to just source this new file from forwarding/lib.sh instead of copying stuff around? I imagine there will eventually be more commonality, and when that pops up, we can just shuffle the forwarding code to net/lib.sh.
Hangbin Liu <liuhangbin@gmail.com> writes: > +cleanup_ns() > +{ > + local ns="" > + local errexit=0 > + > + # disable errexit temporary > + if [[ $- =~ "e" ]]; then > + errexit=1 > + set +e > + fi > + > + for ns in "$@"; do > + ip netns delete "${ns}" &> /dev/null > + busywait 2 "ip netns list | grep -vq $1" &> /dev/null The grep would get confused by substrings of other names. This should be grep -vq "^$ns$". > + if ip netns list | grep -q $1; then Busywait returns != 0 when the wait condition is not reached within a given time. So it should be possible to roll the duplicated if-grep into the busywait line like so: if ! busywait 2 "ip netns etc."; then > + echo "Failed to remove namespace $1" > + return $ksft_skip This does not restore the errexit. I think it might be clearest to have this function as a helper, say __cleanup_ns, and then have a wrapper that does the errexit management: cleanup_ns() { local errexit local rc # disable errexit temporarily if [[ $- =~ "e" ]]; then errexit=1 set +e fi __cleanup_ns "$@" rc=$? [ $errexit -eq 1 ] && set -e return $rc } If this comes up more often, we can have a helper like with_disabled_errexit or whatever, that does this management and dispatches to "$@", so cleanup_ns() would become: cleanup_ns() { with_disabled_errexit __cleanup_ns "$@" } > + fi > + done > + > + [ $errexit -eq 1 ] && set -e > + return 0 > +} > + > +# By default, remove all netns before EXIT. > +cleanup_all_ns() > +{ > + cleanup_ns $NS_LIST > +} > +trap cleanup_all_ns EXIT Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh, because basically all users of forwarding/lib.sh use the EXIT trap. I wonder if we need something like these push_cleanup / on_exit helpers: https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L15 But I don't want to force this on your already large patchset :) So just ignore the bit about including from forwarding/lib.sh. > +# setup netns with given names as prefix. e.g > +# setup_ns local remote > +setup_ns() > +{ > + local ns="" > + # the ns list we created in this call > + local ns_list="" > + while [ -n "$1" ]; do I would find it more readable if this used the same iteration approach as the 'for ns in "$@"' above. The $1/shift approach used here is somewhat confusing. > + # Some test may setup/remove same netns multi times > + if unset $1 2> /dev/null; then > + ns="${1,,}-$(mktemp -u XXXXXX)" > + eval readonly $1=$ns > + else > + eval ns='$'$1 > + cleanup_ns $ns > + > + fi > + > + ip netns add $ns > + if ! ip netns list | grep -q $ns; then As above, the grep could get confused. But in fact wouldn't just checking the exit code of ip netns add be enough? > + echo "Failed to create namespace $1" > + cleanup_ns $ns_list > + return $ksft_skip > + fi > + ip -n $ns link set lo up > + ns_list="$ns_list $ns" > + > + shift > + done > + NS_LIST="$NS_LIST $ns_list" > +}
Petr Machata <petrm@nvidia.com> writes: > Hangbin Liu <liuhangbin@gmail.com> writes: > >> +# By default, remove all netns before EXIT. >> +cleanup_all_ns() >> +{ >> + cleanup_ns $NS_LIST >> +} >> +trap cleanup_all_ns EXIT > > Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh, > because basically all users of forwarding/lib.sh use the EXIT trap. [...] > So just ignore the bit about including from forwarding/lib.sh. Actually I take this back. The cleanup should be invoked from where the init was called. I don't think the library should be auto-invoking it, the client scripts should. Whether through a trap or otherwise.
On Fri, Nov 24, 2023 at 03:05:18PM +0100, Petr Machata wrote: > > Hangbin Liu <liuhangbin@gmail.com> writes: > > > Add a lib.sh for net selftests. This file can be used to define commonly > > used variables and functions. > > > > Add function setup_ns() for user to create unique namespaces with given > > prefix name. > > > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > --- > > diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh > > new file mode 100644 > > index 000000000000..239ab2beb438 > > --- /dev/null > > +++ b/tools/testing/selftests/net/lib.sh > > @@ -0,0 +1,98 @@ > > +#!/bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +############################################################################## > > +# Defines > > + > > +# Kselftest framework requirement - SKIP code is 4. > > +ksft_skip=4 > > +# namespace list created by setup_ns > > +NS_LIST="" > > + > > +############################################################################## > > +# Helpers > > +busywait() > > +{ > > + local timeout=$1; shift > > + > > + local start_time="$(date -u +%s%3N)" > > + while true > > + do > > + local out > > + out=$($@) > > + local ret=$? > > + if ((!ret)); then > > + echo -n "$out" > > + return 0 > > + fi > > + > > + local current_time="$(date -u +%s%3N)" > > + if ((current_time - start_time > timeout)); then > > + echo -n "$out" > > + return 1 > > + fi > > + done > > +} > > This is lifted from forwarding/lib.sh, right? Would it make sense to Yes. > just source this new file from forwarding/lib.sh instead of copying Do you mean let net/forwarding/lib.sh source net.lib, and let other net tests source the net/forwarding/lib.sh? Or move the busywait() function from net/forwarding/lib.sh to net.lib. Then let net/forwarding/lib.sh source net.lib? > stuff around? I imagine there will eventually be more commonality, and > when that pops up, we can just shuffle the forwarding code to > net/lib.sh. Yes, make sense. Thanks Hangbin
On Fri, Nov 24, 2023 at 03:35:51PM +0100, Petr Machata wrote: > > Hangbin Liu <liuhangbin@gmail.com> writes: > > > +cleanup_ns() > > +{ > > + local ns="" > > + local errexit=0 > > + > > + # disable errexit temporary > > + if [[ $- =~ "e" ]]; then > > + errexit=1 > > + set +e > > + fi > > + > > + for ns in "$@"; do > > + ip netns delete "${ns}" &> /dev/null > > + busywait 2 "ip netns list | grep -vq $1" &> /dev/null > > The grep would get confused by substrings of other names. > This should be grep -vq "^$ns$". Thanks. I just thought the ns name would like foo-xxxxxxx, but I forgot this is a common function, which maybe called with normal ns name. > > > + if ip netns list | grep -q $1; then > > Busywait returns != 0 when the wait condition is not reached within a > given time. So it should be possible to roll the duplicated if-grep into > the busywait line like so: > > if ! busywait 2 "ip netns etc."; then You are right. > > > + echo "Failed to remove namespace $1" > > + return $ksft_skip > > This does not restore the errexit. > > I think it might be clearest to have this function as a helper, say > __cleanup_ns, and then have a wrapper that does the errexit management: > > cleanup_ns() > { > local errexit > local rc > > # disable errexit temporarily > if [[ $- =~ "e" ]]; then > errexit=1 > set +e > fi > > __cleanup_ns "$@" > rc=$? > > [ $errexit -eq 1 ] && set -e > return $rc > } > > If this comes up more often, we can have a helper like > with_disabled_errexit or whatever, that does this management and > dispatches to "$@", so cleanup_ns() would become: > > cleanup_ns() > { > with_disabled_errexit __cleanup_ns "$@" > } Thanks for your suggestion. > > > + fi > > + done > > + > > + [ $errexit -eq 1 ] && set -e > > + return 0 > > +} > > + > > +# By default, remove all netns before EXIT. > > +cleanup_all_ns() > > +{ > > + cleanup_ns $NS_LIST > > +} > > +trap cleanup_all_ns EXIT > > Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh, > because basically all users of forwarding/lib.sh use the EXIT trap. > > I wonder if we need something like these push_cleanup / on_exit helpers: > > https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L15 When I added this, I just want to make sure the netns are cleaned up if the client script forgot. I think the client script trap function should cover this one, no? > > But I don't want to force this on your already large patchset :) Yes, Paolo also told me that this is too large. I will break it to 2 path set or merge some small patches together for next version. > So just ignore the bit about including from forwarding/lib.sh. > Actually I take this back. The cleanup should be invoked from where the > init was called. I don't think the library should be auto-invoking it, > the client scripts should. Whether through a trap or otherwise. OK, also makes sense. I will remove this trap. Thanks for all your comments. Hangbin
Hangbin Liu <liuhangbin@gmail.com> writes: > On Fri, Nov 24, 2023 at 03:05:18PM +0100, Petr Machata wrote: >> >> Hangbin Liu <liuhangbin@gmail.com> writes: >> >> > +# Helpers >> > +busywait() >> > +{ >> > + local timeout=$1; shift >> > + >> > + local start_time="$(date -u +%s%3N)" >> > + while true >> > + do >> > + local out >> > + out=$($@) >> > + local ret=$? >> > + if ((!ret)); then >> > + echo -n "$out" >> > + return 0 >> > + fi >> > + >> > + local current_time="$(date -u +%s%3N)" >> > + if ((current_time - start_time > timeout)); then >> > + echo -n "$out" >> > + return 1 >> > + fi >> > + done >> > +} >> >> This is lifted from forwarding/lib.sh, right? Would it make sense to > > Yes. > >> just source this new file from forwarding/lib.sh instead of copying > > Do you mean let net/forwarding/lib.sh source net.lib, and let other net > tests source the net/forwarding/lib.sh? > > Or move the busywait() function from net/forwarding/lib.sh to net.lib. > Then let net/forwarding/lib.sh source net.lib? This. >> stuff around? I imagine there will eventually be more commonality, and >> when that pops up, we can just shuffle the forwarding code to >> net/lib.sh. > > Yes, make sense. > > Thanks > Hangbin
Hangbin Liu <liuhangbin@gmail.com> writes: > On Fri, Nov 24, 2023 at 03:35:51PM +0100, Petr Machata wrote: >> >> Hangbin Liu <liuhangbin@gmail.com> writes: >> >> > + fi >> > + done >> > + >> > + [ $errexit -eq 1 ] && set -e >> > + return 0 >> > +} >> > + >> > +# By default, remove all netns before EXIT. >> > +cleanup_all_ns() >> > +{ >> > + cleanup_ns $NS_LIST >> > +} >> > +trap cleanup_all_ns EXIT >> >> Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh, >> because basically all users of forwarding/lib.sh use the EXIT trap. >> >> I wonder if we need something like these push_cleanup / on_exit helpers: >> >> https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L15 > > When I added this, I just want to make sure the netns are cleaned up if the > client script forgot. I think the client script trap function should > cover this one, no? So the motivation makes sense. But in general, invoking cleanup from the same abstraction layer that acquired the resource, makes the code easier to analyze. And in particular here that we are talking about traps, which are a global resource, and one that the client might well want to use for their own management. The client should be using the trap instead of the framework. The framework might expose APIs to allow clients to register cleanups etc., which the framework itself is then free to use of course, for resources that it itself has acquired. But even with these APIs in place I think it would be better if the client that acquires a resource also schedules its release. (Though it's not as clear-cut in that case.) >> >> But I don't want to force this on your already large patchset :) > > Yes, Paolo also told me that this is too large. I will break it to > 2 path set or merge some small patches together for next version. > >> So just ignore the bit about including from forwarding/lib.sh. > >> Actually I take this back. The cleanup should be invoked from where the >> init was called. I don't think the library should be auto-invoking it, >> the client scripts should. Whether through a trap or otherwise. > > OK, also makes sense. I will remove this trap. > > Thanks for all your comments. > Hangbin
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile index 9274edfb76ff..14bd68da7466 100644 --- a/tools/testing/selftests/net/Makefile +++ b/tools/testing/selftests/net/Makefile @@ -54,7 +54,7 @@ TEST_PROGS += ip_local_port_range.sh TEST_PROGS += rps_default_mask.sh TEST_PROGS += big_tcp.sh TEST_PROGS_EXTENDED := in_netns.sh setup_loopback.sh setup_veth.sh -TEST_PROGS_EXTENDED += toeplitz_client.sh toeplitz.sh +TEST_PROGS_EXTENDED += toeplitz_client.sh toeplitz.sh lib.sh TEST_GEN_FILES = socket nettest TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any TEST_GEN_FILES += tcp_mmap tcp_inq psock_snd txring_overwrite diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh new file mode 100644 index 000000000000..239ab2beb438 --- /dev/null +++ b/tools/testing/selftests/net/lib.sh @@ -0,0 +1,98 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 + +############################################################################## +# Defines + +# Kselftest framework requirement - SKIP code is 4. +ksft_skip=4 +# namespace list created by setup_ns +NS_LIST="" + +############################################################################## +# Helpers +busywait() +{ + local timeout=$1; shift + + local start_time="$(date -u +%s%3N)" + while true + do + local out + out=$($@) + local ret=$? + if ((!ret)); then + echo -n "$out" + return 0 + fi + + local current_time="$(date -u +%s%3N)" + if ((current_time - start_time > timeout)); then + echo -n "$out" + return 1 + fi + done +} + +cleanup_ns() +{ + local ns="" + local errexit=0 + + # disable errexit temporary + if [[ $- =~ "e" ]]; then + errexit=1 + set +e + fi + + for ns in "$@"; do + ip netns delete "${ns}" &> /dev/null + busywait 2 "ip netns list | grep -vq $1" &> /dev/null + if ip netns list | grep -q $1; then + echo "Failed to remove namespace $1" + return $ksft_skip + fi + done + + [ $errexit -eq 1 ] && set -e + return 0 +} + +# By default, remove all netns before EXIT. +cleanup_all_ns() +{ + cleanup_ns $NS_LIST +} +trap cleanup_all_ns EXIT + +# setup netns with given names as prefix. e.g +# setup_ns local remote +setup_ns() +{ + local ns="" + # the ns list we created in this call + local ns_list="" + while [ -n "$1" ]; do + # Some test may setup/remove same netns multi times + if unset $1 2> /dev/null; then + ns="${1,,}-$(mktemp -u XXXXXX)" + eval readonly $1=$ns + else + eval ns='$'$1 + cleanup_ns $ns + + fi + + ip netns add $ns + if ! ip netns list | grep -q $ns; then + echo "Failed to create namespace $1" + cleanup_ns $ns_list + return $ksft_skip + fi + ip -n $ns link set lo up + ns_list="$ns_list $ns" + + shift + done + NS_LIST="$NS_LIST $ns_list" +}
Add a lib.sh for net selftests. This file can be used to define commonly used variables and functions. Add function setup_ns() for user to create unique namespaces with given prefix name. Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- tools/testing/selftests/net/Makefile | 2 +- tools/testing/selftests/net/lib.sh | 98 ++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/net/lib.sh