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
+ 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[@]}")
}
--
2.43.0
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 -- Sponsored by the NGI0 Core fund.
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 -- Sponsored by the NGI0 Core fund.
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
© 2016 - 2024 Red Hat, Inc.