| Message ID | 20240527-selftests-net-lib-fixes-v6-5-72411ff2460e@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 74952f110c3dd47f60ebbdc5abd8729838fd44d6 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | use helpers in lib.sh and net_helpers.sh | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 53 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) wrote: > Instead of only appending items to the list, remove them when the > netns > has been deleted. > > By doing that, we can make sure 'cleanup_all_ns()' is not trying to > remove already deleted netns. > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > --- > tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/net/lib.sh > b/tools/testing/selftests/net/lib.sh > index b2572aff6286..c7a8cfb477cc 100644 > --- a/tools/testing/selftests/net/lib.sh > +++ b/tools/testing/selftests/net/lib.sh > @@ -125,6 +125,20 @@ slowwait_for_counter() > slowwait "$timeout" until_counter_is ">= $((base + delta))" > "$@" > } > > +remove_ns_list() > +{ > + local item=$1 > + local ns > + local ns_list=("${NS_LIST[@]}") > + NS_LIST=() > + > + for ns in "${ns_list[@]}"; do > + if [ "${ns}" != "${item}" ]; then > + NS_LIST+=("${ns}") > + fi > + done > +} > + > cleanup_ns() > { > local ns="" > @@ -136,6 +150,8 @@ cleanup_ns() > if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| > grep -vq "^$ns$" &> /dev/null; then > echo "Warn: Failed to remove namespace $ns" > ret=1 > + else nit: Should we also remove ns_list when "ip netns list" is busy? Or should we not delete ns when "ip netns list" is busy? I think we should do these two commands together: ip netns delete "${ns}" &> /dev/null || true remove_ns_list "${ns}" I'm not sure. WDYT? Thanks, -Geliang > + remove_ns_list "${ns}" > fi > done > > @@ -154,17 +170,14 @@ setup_ns() > local ns="" > local ns_name="" > local ns_list=() > - local ns_exist= > 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" > - ns_exist=false > else > eval ns='$'${ns_name} > cleanup_ns "$ns" > - ns_exist=true > fi > > if ! ip netns add "$ns"; then > @@ -173,7 +186,7 @@ setup_ns() > return $ksft_skip > fi > ip -n "$ns" link set lo up > - ! $ns_exist && ns_list+=("$ns") > + ns_list+=("$ns") > done > NS_LIST+=("${ns_list[@]}") > } >
Hi Geliang, Thank you for your review! On 28/05/2024 05:47, Geliang Tang wrote: > On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) wrote: >> Instead of only appending items to the list, remove them when the >> netns >> has been deleted. >> >> By doing that, we can make sure 'cleanup_all_ns()' is not trying to >> remove already deleted netns. >> >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >> --- >> tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++---- >> 1 file changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/tools/testing/selftests/net/lib.sh >> b/tools/testing/selftests/net/lib.sh >> index b2572aff6286..c7a8cfb477cc 100644 >> --- a/tools/testing/selftests/net/lib.sh >> +++ b/tools/testing/selftests/net/lib.sh >> @@ -125,6 +125,20 @@ slowwait_for_counter() >> slowwait "$timeout" until_counter_is ">= $((base + delta))" >> "$@" >> } >> >> +remove_ns_list() >> +{ >> + local item=$1 >> + local ns >> + local ns_list=("${NS_LIST[@]}") >> + NS_LIST=() >> + >> + for ns in "${ns_list[@]}"; do >> + if [ "${ns}" != "${item}" ]; then >> + NS_LIST+=("${ns}") >> + fi >> + done >> +} >> + >> cleanup_ns() >> { >> local ns="" >> @@ -136,6 +150,8 @@ cleanup_ns() >> if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| >> grep -vq "^$ns$" &> /dev/null; then >> echo "Warn: Failed to remove namespace $ns" >> ret=1 >> + else > > nit: Should we also remove ns_list when "ip netns list" is busy? Or > should we not delete ns when "ip netns list" is busy? > > I think we should do these two commands together: > > ip netns delete "${ns}" &> /dev/null || true > remove_ns_list "${ns}" > > I'm not sure. WDYT? Good point, I'm not sure either. I kept it in the list because in case of errors, the caller could still get the list of NS that have been created, but not deleted yet. To be honest, I don't think any actions should be done on the NS after having called cleanup: it would probably better to kill all attached processes before that, and not retry later after errors and a kill. But I didn't want to change this logic as there might be other impacts. So at least here, we let the responsibility to the caller, and it can call cleanup_all_ns() again in case of errors. WDYT? Cheers, Matt
On Tue, 2024-05-28 at 12:38 +0200, Matthieu Baerts wrote: > Hi Geliang, > > Thank you for your review! > > On 28/05/2024 05:47, Geliang Tang wrote: > > On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) wrote: > > > Instead of only appending items to the list, remove them when the > > > netns > > > has been deleted. > > > > > > By doing that, we can make sure 'cleanup_all_ns()' is not trying > > > to > > > remove already deleted netns. > > > > > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > > --- > > > tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++---- > > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > > > diff --git a/tools/testing/selftests/net/lib.sh > > > b/tools/testing/selftests/net/lib.sh > > > index b2572aff6286..c7a8cfb477cc 100644 > > > --- a/tools/testing/selftests/net/lib.sh > > > +++ b/tools/testing/selftests/net/lib.sh > > > @@ -125,6 +125,20 @@ slowwait_for_counter() > > > slowwait "$timeout" until_counter_is ">= $((base + > > > delta))" > > > "$@" > > > } > > > > > > +remove_ns_list() > > > +{ > > > + local item=$1 > > > + local ns > > > + local ns_list=("${NS_LIST[@]}") > > > + NS_LIST=() > > > + > > > + for ns in "${ns_list[@]}"; do > > > + if [ "${ns}" != "${item}" ]; then > > > + NS_LIST+=("${ns}") > > > + fi > > > + done > > > +} > > > + > > > cleanup_ns() > > > { > > > local ns="" > > > @@ -136,6 +150,8 @@ cleanup_ns() > > > if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| > > > grep -vq "^$ns$" &> /dev/null; then > > > echo "Warn: Failed to remove namespace > > > $ns" > > > ret=1 > > > + else > > > > nit: Should we also remove ns_list when "ip netns list" is busy? Or > > should we not delete ns when "ip netns list" is busy? > > > > I think we should do these two commands together: > > > > ip netns delete "${ns}" &> /dev/null || true > > remove_ns_list "${ns}" > > > > I'm not sure. WDYT? > > Good point, I'm not sure either. I kept it in the list because in > case > of errors, the caller could still get the list of NS that have been > created, but not deleted yet. > > To be honest, I don't think any actions should be done on the NS > after > having called cleanup: it would probably better to kill all attached > processes before that, and not retry later after errors and a kill. > But > I didn't want to change this logic as there might be other impacts. > So > at least here, we let the responsibility to the caller, and it can > call > cleanup_all_ns() again in case of errors. WDYT? If so, I think this is better: for ns in "$@"; do [ -z "${ns}" ] && continue ip netns delete "${ns}" &> /dev/null || true remove_ns_list "${ns}" if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep - vq "^$ns$" &> /dev/null; then echo "Warn: Failed to remove namespace $ns" ret=1 fi done If you agree, please update this when merging it. No need to send a v7. And please add my Reviewd-by tag. I'll repay it in the cover-letter. Thanks, -Geliang > > Cheers, > Matt
Hi Geliang, On 28/05/2024 15:18, Geliang Tang wrote: > On Tue, 2024-05-28 at 12:38 +0200, Matthieu Baerts wrote: >> Hi Geliang, >> >> Thank you for your review! >> >> On 28/05/2024 05:47, Geliang Tang wrote: >>> On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) wrote: >>>> Instead of only appending items to the list, remove them when the >>>> netns >>>> has been deleted. >>>> >>>> By doing that, we can make sure 'cleanup_all_ns()' is not trying >>>> to >>>> remove already deleted netns. >>>> >>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >>>> --- >>>> tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++---- >>>> 1 file changed, 17 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/net/lib.sh >>>> b/tools/testing/selftests/net/lib.sh >>>> index b2572aff6286..c7a8cfb477cc 100644 >>>> --- a/tools/testing/selftests/net/lib.sh >>>> +++ b/tools/testing/selftests/net/lib.sh >>>> @@ -125,6 +125,20 @@ slowwait_for_counter() >>>> slowwait "$timeout" until_counter_is ">= $((base + >>>> delta))" >>>> "$@" >>>> } >>>> >>>> +remove_ns_list() >>>> +{ >>>> + local item=$1 >>>> + local ns >>>> + local ns_list=("${NS_LIST[@]}") >>>> + NS_LIST=() >>>> + >>>> + for ns in "${ns_list[@]}"; do >>>> + if [ "${ns}" != "${item}" ]; then >>>> + NS_LIST+=("${ns}") >>>> + fi >>>> + done >>>> +} >>>> + >>>> cleanup_ns() >>>> { >>>> local ns="" >>>> @@ -136,6 +150,8 @@ cleanup_ns() >>>> if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| >>>> grep -vq "^$ns$" &> /dev/null; then >>>> echo "Warn: Failed to remove namespace >>>> $ns" >>>> ret=1 >>>> + else >>> >>> nit: Should we also remove ns_list when "ip netns list" is busy? Or >>> should we not delete ns when "ip netns list" is busy? >>> >>> I think we should do these two commands together: >>> >>> ip netns delete "${ns}" &> /dev/null || true >>> remove_ns_list "${ns}" >>> >>> I'm not sure. WDYT? >> >> Good point, I'm not sure either. I kept it in the list because in >> case >> of errors, the caller could still get the list of NS that have been >> created, but not deleted yet. >> >> To be honest, I don't think any actions should be done on the NS >> after >> having called cleanup: it would probably better to kill all attached >> processes before that, and not retry later after errors and a kill. >> But >> I didn't want to change this logic as there might be other impacts. >> So >> at least here, we let the responsibility to the caller, and it can >> call >> cleanup_all_ns() again in case of errors. WDYT? > > If so, I think this is better: > > for ns in "$@"; do > [ -z "${ns}" ] && continue > ip netns delete "${ns}" &> /dev/null || true > remove_ns_list "${ns}" > if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep - > vq "^$ns$" &> /dev/null; then > echo "Warn: Failed to remove namespace $ns" > ret=1 > fi > done Sorry, I was not clear enough: I wanted to say that I think it is better to keep "remove_ns_list" in the "else", only to remove the netns from the list if it has been deleted. By doing that, the caller can call cleanup_all_ns() again, after having stopped everything. (That's a detail, I guess the removal should not fail, and if it does, the caller should adapt the code to make sure the clean doesn't fail.) > If you agree, please update this when merging it. No need to send a v7. Thanks! I will wait for your feedback on what is above before applying this series (even if we can always have squash-to patches later). Cheers, Matt
On Tue, 2024-05-28 at 17:26 +0200, Matthieu Baerts wrote: > Hi Geliang, > > On 28/05/2024 15:18, Geliang Tang wrote: > > On Tue, 2024-05-28 at 12:38 +0200, Matthieu Baerts wrote: > > > Hi Geliang, > > > > > > Thank you for your review! > > > > > > On 28/05/2024 05:47, Geliang Tang wrote: > > > > On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) > > > > wrote: > > > > > Instead of only appending items to the list, remove them when > > > > > the > > > > > netns > > > > > has been deleted. > > > > > > > > > > By doing that, we can make sure 'cleanup_all_ns()' is not > > > > > trying > > > > > to > > > > > remove already deleted netns. > > > > > > > > > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > > > > --- > > > > > tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++--- > > > > > - > > > > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/tools/testing/selftests/net/lib.sh > > > > > b/tools/testing/selftests/net/lib.sh > > > > > index b2572aff6286..c7a8cfb477cc 100644 > > > > > --- a/tools/testing/selftests/net/lib.sh > > > > > +++ b/tools/testing/selftests/net/lib.sh > > > > > @@ -125,6 +125,20 @@ slowwait_for_counter() > > > > > slowwait "$timeout" until_counter_is ">= $((base + > > > > > delta))" > > > > > "$@" > > > > > } > > > > > > > > > > +remove_ns_list() > > > > > +{ > > > > > + local item=$1 > > > > > + local ns > > > > > + local ns_list=("${NS_LIST[@]}") > > > > > + NS_LIST=() > > > > > + > > > > > + for ns in "${ns_list[@]}"; do > > > > > + if [ "${ns}" != "${item}" ]; then > > > > > + NS_LIST+=("${ns}") > > > > > + fi > > > > > + done > > > > > +} > > > > > + > > > > > cleanup_ns() > > > > > { > > > > > local ns="" > > > > > @@ -136,6 +150,8 @@ cleanup_ns() > > > > > if ! busywait $BUSYWAIT_TIMEOUT ip netns > > > > > list \| > > > > > grep -vq "^$ns$" &> /dev/null; then > > > > > echo "Warn: Failed to remove > > > > > namespace > > > > > $ns" > > > > > ret=1 > > > > > + else > > > > > > > > nit: Should we also remove ns_list when "ip netns list" is > > > > busy? Or > > > > should we not delete ns when "ip netns list" is busy? > > > > > > > > I think we should do these two commands together: > > > > > > > > ip netns delete "${ns}" &> /dev/null || true > > > > remove_ns_list "${ns}" > > > > > > > > I'm not sure. WDYT? > > > > > > Good point, I'm not sure either. I kept it in the list because in > > > case > > > of errors, the caller could still get the list of NS that have > > > been > > > created, but not deleted yet. > > > > > > To be honest, I don't think any actions should be done on the NS > > > after > > > having called cleanup: it would probably better to kill all > > > attached > > > processes before that, and not retry later after errors and a > > > kill. > > > But > > > I didn't want to change this logic as there might be other > > > impacts. > > > So > > > at least here, we let the responsibility to the caller, and it > > > can > > > call > > > cleanup_all_ns() again in case of errors. WDYT? > > > > If so, I think this is better: > > > > for ns in "$@"; do > > [ -z "${ns}" ] && continue > > ip netns delete "${ns}" &> /dev/null || true > > remove_ns_list "${ns}" > > if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| > > grep - > > vq "^$ns$" &> /dev/null; then > > echo "Warn: Failed to remove namespace $ns" > > ret=1 > > fi > > done > > Sorry, I was not clear enough: I wanted to say that I think it is > better > to keep "remove_ns_list" in the "else", only to remove the netns from > the list if it has been deleted. By doing that, the caller can call > cleanup_all_ns() again, after having stopped everything. Yes, you're right, it's better to keep "remove_ns_list" in the "else". My bad, I misunderstood it. Please apply this patch as is. > > (That's a detail, I guess the removal should not fail, and if it > does, > the caller should adapt the code to make sure the clean doesn't > fail.) > > > If you agree, please update this when merging it. No need to send a > > v7. > > Thanks! I will wait for your feedback on what is above before > applying > this series (even if we can always have squash-to patches later). I changed this series as "Queued" on patchwork. Thanks, -Geliang > > Cheers, > Matt
diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh index b2572aff6286..c7a8cfb477cc 100644 --- a/tools/testing/selftests/net/lib.sh +++ b/tools/testing/selftests/net/lib.sh @@ -125,6 +125,20 @@ slowwait_for_counter() slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@" } +remove_ns_list() +{ + local item=$1 + local ns + local ns_list=("${NS_LIST[@]}") + NS_LIST=() + + for ns in "${ns_list[@]}"; do + if [ "${ns}" != "${item}" ]; then + NS_LIST+=("${ns}") + fi + done +} + cleanup_ns() { local ns="" @@ -136,6 +150,8 @@ cleanup_ns() if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep -vq "^$ns$" &> /dev/null; then echo "Warn: Failed to remove namespace $ns" ret=1 + else + remove_ns_list "${ns}" fi done @@ -154,17 +170,14 @@ setup_ns() local ns="" local ns_name="" local ns_list=() - local ns_exist= 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" - ns_exist=false else eval ns='$'${ns_name} cleanup_ns "$ns" - ns_exist=true fi if ! ip netns add "$ns"; then @@ -173,7 +186,7 @@ setup_ns() return $ksft_skip fi ip -n "$ns" link set lo up - ! $ns_exist && ns_list+=("$ns") + ns_list+=("$ns") done NS_LIST+=("${ns_list[@]}") }
Instead of only appending items to the list, remove them when the netns has been deleted. By doing that, we can make sure 'cleanup_all_ns()' is not trying to remove already deleted netns. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)