Re: [PATCH mptcp-next v6 5/9] selftests: net: lib: remove ns from list after clean-up

[Matthieu Baerts <matttbe@kernel.org>]

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.