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.0On 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 - 2026 Red Hat, Inc.