| Message ID | 20231202020110.362433-2-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 25ae948b447881bf689d459cd5bd4629d9c04b20 |
| Headers | show |
| Series | Conver net selftests to run in unique namespace (Part 1) | expand |
On Sat, 2023-12-02 at 10:00 +0800, Hangbin Liu wrote: > Add a lib.sh for net selftests. This file can be used to define commonly > used variables and functions. Some commonly used functions can be moved > from forwarding/lib.sh to this lib file. e.g. busywait(). > > Add function setup_ns() for user to create unique namespaces with given > prefix name. > > Reviewed-by: Petr Machata <petrm@nvidia.com> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > tools/testing/selftests/net/Makefile | 2 +- > tools/testing/selftests/net/forwarding/lib.sh | 27 +----- > tools/testing/selftests/net/lib.sh | 85 +++++++++++++++++++ > 3 files changed, 87 insertions(+), 27 deletions(-) > 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/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh > index e37a15eda6c2..8f6ca458af9a 100755 > --- a/tools/testing/selftests/net/forwarding/lib.sh > +++ b/tools/testing/selftests/net/forwarding/lib.sh > @@ -4,9 +4,6 @@ > ############################################################################## > # Defines > > -# Kselftest framework requirement - SKIP code is 4. > -ksft_skip=4 > - > # Can be overridden by the configuration file. > PING=${PING:=ping} > PING6=${PING6:=ping6} > @@ -41,6 +38,7 @@ if [[ -f $relative_path/forwarding.config ]]; then > source "$relative_path/forwarding.config" > fi > > +source ../lib.sh > ############################################################################## > # Sanity checks > > @@ -395,29 +393,6 @@ log_info() > echo "INFO: $msg" > } > > -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 > -} > - > not() > { > "$@" > diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh > new file mode 100644 > index 000000000000..518eca57b815 > --- /dev/null > +++ b/tools/testing/selftests/net/lib.sh > @@ -0,0 +1,85 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0 > + > +############################################################################## > +# Defines > + > +# Kselftest framework requirement - SKIP code is 4. > +ksft_skip=4 > + > +############################################################################## > +# 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 > + local ret=0 > + > + # disable errexit temporary > + if [[ $- =~ "e" ]]; then > + errexit=1 > + set +e > + fi > + > + for ns in "$@"; do > + ip netns delete "${ns}" &> /dev/null > + if ! busywait 2 ip netns list \| grep -vq "^$ns$" &> /dev/null; then > + echo "Warn: Failed to remove namespace $ns" > + ret=1 > + fi > + done > + > + [ $errexit -eq 1 ] && set -e > + return $ret > +} > + > +# setup netns with given names as prefix. e.g > +# setup_ns local remote > +setup_ns() > +{ > + local ns="" > + local ns_name="" > + local ns_list="" > + for ns_name in "$@"; do > + # Some test may setup/remove same netns multi times > + if unset ${ns_name} 2> /dev/null; then > + ns="${ns_name,,}-$(mktemp -u XXXXXX)" > + eval readonly ${ns_name}="$ns" > + else > + eval ns='$'${ns_name} > + cleanup_ns "$ns" > + > + fi > + > + if ! ip netns add "$ns"; then > + echo "Failed to create namespace $ns_name" > + cleanup_ns "$ns_list" > + return $ksft_skip > + fi > + ip -n "$ns" link set lo up > + ns_list="$ns_list $ns" Side note for a possible follow-up: if you maintain $ns_list as global variable, and remove from such list the ns deleted by cleanup_ns, you could remove the cleanup trap from the individual test with something alike: final_cleanup_ns() { cleanup_ns $ns_list } trap final_cleanup_ns EXIT No respin needed for the above, could be a follow-up if agreed upon. Cheers, Paolo
On Tue, Dec 05, 2023 at 01:00:29PM +0100, Paolo Abeni wrote: > > +cleanup_ns() > > +{ > > + local ns="" > > + local errexit=0 > > + local ret=0 > > + > > + # disable errexit temporary > > + if [[ $- =~ "e" ]]; then > > + errexit=1 > > + set +e > > + fi > > + > > + for ns in "$@"; do > > + ip netns delete "${ns}" &> /dev/null > > + if ! busywait 2 ip netns list \| grep -vq "^$ns$" &> /dev/null; then > > + echo "Warn: Failed to remove namespace $ns" > > + ret=1 > > + fi > > + done > > + > > + [ $errexit -eq 1 ] && set -e > > + return $ret > > +} > > + > > +# setup netns with given names as prefix. e.g > > +# setup_ns local remote > > +setup_ns() > > +{ > > + local ns="" > > + local ns_name="" > > + local ns_list="" > > + for ns_name in "$@"; do > > + # Some test may setup/remove same netns multi times > > + if unset ${ns_name} 2> /dev/null; then > > + ns="${ns_name,,}-$(mktemp -u XXXXXX)" > > + eval readonly ${ns_name}="$ns" > > + else > > + eval ns='$'${ns_name} > > + cleanup_ns "$ns" > > + > > + fi > > + > > + if ! ip netns add "$ns"; then > > + echo "Failed to create namespace $ns_name" > > + cleanup_ns "$ns_list" > > + return $ksft_skip > > + fi > > + ip -n "$ns" link set lo up > > + ns_list="$ns_list $ns" > > Side note for a possible follow-up: if you maintain $ns_list as global > variable, and remove from such list the ns deleted by cleanup_ns, you > could remove the cleanup trap from the individual test with something > alike: > > final_cleanup_ns() > { > cleanup_ns $ns_list > } > > trap final_cleanup_ns EXIT > > No respin needed for the above, could be a follow-up if agreed upon. Hi Paolo, I did similar in the first version. But Petr said[1] we should let the client do cleanup specifically. I agree that we should let client script keep this in mind. On the other hand, maybe we can add this final cleanup and let client call it directly. What do you think? [1] https://lore.kernel.org/netdev/878r6nf9x5.fsf@nvidia.com/ Thanks Hangbin
Paolo Abeni <pabeni@redhat.com> writes: > Side note for a possible follow-up: if you maintain $ns_list as global > variable, and remove from such list the ns deleted by cleanup_ns, you > could remove the cleanup trap from the individual test with something > alike: > > final_cleanup_ns() > { > cleanup_ns $ns_list > } > > trap final_cleanup_ns EXIT > > No respin needed for the above, could be a follow-up if agreed upon. If you propose this for the library then I'm against it. The exit trap is a global resource that the client scripts sometimes need to use as well, to do topology teardowns or just general cleanups. So either the library would have to provide APIs for cleanup management, or the trap is for exclusive use by clients. The latter is IMHO simpler. It also puts the cleanups at the same place where the acquisition is prompted: the client allocates the NS, the client should prompt its cleanup.
On Wed, 2023-12-06 at 13:32 +0100, Petr Machata wrote: > Paolo Abeni <pabeni@redhat.com> writes: > > > Side note for a possible follow-up: if you maintain $ns_list as global > > variable, and remove from such list the ns deleted by cleanup_ns, you > > could remove the cleanup trap from the individual test with something > > alike: > > > > final_cleanup_ns() > > { > > cleanup_ns $ns_list > > } > > > > trap final_cleanup_ns EXIT > > > > No respin needed for the above, could be a follow-up if agreed upon. > > If you propose this for the library then I'm against it. The exit trap > is a global resource that the client scripts sometimes need to use as > well, to do topology teardowns or just general cleanups. > So either the library would have to provide APIs for cleanup management, or the trap > is for exclusive use by clients. The latter is IMHO simpler. Even the former would not be very complex: TRAPS="" do_at_exit() { TRAPS="${TRAPS}$@;" trap "${TRAPS}" EXIT } And then use "do_at_exit <whatever>" instead of "trap <whatever> EXIT" > It also puts the cleanups at the same place where the acquisition is > prompted: the client allocates the NS, the client should prompt its > cleanup. I guess I could argue that the the script is asking the library to allocate the namespaces, and the library could take care of disposing them. But I'm not pushing the proposed option, if there is no agreement no need for additional work ;) Cheers, Paolo
Paolo Abeni <pabeni@redhat.com> writes: > On Wed, 2023-12-06 at 13:32 +0100, Petr Machata wrote: >> Paolo Abeni <pabeni@redhat.com> writes: >> >> > Side note for a possible follow-up: if you maintain $ns_list as global >> > variable, and remove from such list the ns deleted by cleanup_ns, you >> > could remove the cleanup trap from the individual test with something >> > alike: >> > >> > final_cleanup_ns() >> > { >> > cleanup_ns $ns_list >> > } >> > >> > trap final_cleanup_ns EXIT >> > >> > No respin needed for the above, could be a follow-up if agreed upon. >> >> If you propose this for the library then I'm against it. The exit trap >> is a global resource that the client scripts sometimes need to use as >> well, to do topology teardowns or just general cleanups. >> So either the library would have to provide APIs for cleanup management, or the trap >> is for exclusive use by clients. The latter is IMHO simpler. > > Even the former would not be very complex: > > TRAPS="" > do_at_exit() { > TRAPS="${TRAPS}$@;" > > trap "${TRAPS}" EXIT > } > > And then use "do_at_exit <whatever>" instead of "trap <whatever> EXIT" Yep. I mentioned this during v2 review: https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L13 Not much code at all, though you need to convert all EXIT trap users to this contraption. Again, a mechanical process, just needs to be done. >> It also puts the cleanups at the same place where the acquisition is >> prompted: the client allocates the NS, the client should prompt its >> cleanup. > > I guess I could argue that the the script is asking the library to > allocate the namespaces, and the library could take care of disposing > them. It could also be said that since the script asked for NS creation, the script should ask for NS disposal :) But what I object against is that the library uses trap without having a way for user scripts to schedule at-exit work, because that's used literally everywhere in forwarding tests. If people are willing to do the conversion, I'm OK with that. > But I'm not pushing the proposed option, if there is no agreement no > need for additional work ;) > > Cheers, > > Paolo
On 12/7/23 3:34 AM, Petr Machata wrote: > But what I object against is that the library uses trap without having a > way for user scripts to schedule at-exit work, because that's used > literally everywhere in forwarding tests. +1
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/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh index e37a15eda6c2..8f6ca458af9a 100755 --- a/tools/testing/selftests/net/forwarding/lib.sh +++ b/tools/testing/selftests/net/forwarding/lib.sh @@ -4,9 +4,6 @@ ############################################################################## # Defines -# Kselftest framework requirement - SKIP code is 4. -ksft_skip=4 - # Can be overridden by the configuration file. PING=${PING:=ping} PING6=${PING6:=ping6} @@ -41,6 +38,7 @@ if [[ -f $relative_path/forwarding.config ]]; then source "$relative_path/forwarding.config" fi +source ../lib.sh ############################################################################## # Sanity checks @@ -395,29 +393,6 @@ log_info() echo "INFO: $msg" } -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 -} - not() { "$@" diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh new file mode 100644 index 000000000000..518eca57b815 --- /dev/null +++ b/tools/testing/selftests/net/lib.sh @@ -0,0 +1,85 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 + +############################################################################## +# Defines + +# Kselftest framework requirement - SKIP code is 4. +ksft_skip=4 + +############################################################################## +# 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 + local ret=0 + + # disable errexit temporary + if [[ $- =~ "e" ]]; then + errexit=1 + set +e + fi + + for ns in "$@"; do + ip netns delete "${ns}" &> /dev/null + if ! busywait 2 ip netns list \| grep -vq "^$ns$" &> /dev/null; then + echo "Warn: Failed to remove namespace $ns" + ret=1 + fi + done + + [ $errexit -eq 1 ] && set -e + return $ret +} + +# setup netns with given names as prefix. e.g +# setup_ns local remote +setup_ns() +{ + local ns="" + local ns_name="" + local ns_list="" + for ns_name in "$@"; do + # Some test may setup/remove same netns multi times + if unset ${ns_name} 2> /dev/null; then + ns="${ns_name,,}-$(mktemp -u XXXXXX)" + eval readonly ${ns_name}="$ns" + else + eval ns='$'${ns_name} + cleanup_ns "$ns" + + fi + + if ! ip netns add "$ns"; then + echo "Failed to create namespace $ns_name" + cleanup_ns "$ns_list" + return $ksft_skip + fi + ip -n "$ns" link set lo up + ns_list="$ns_list $ns" + done +}