diff mbox series

[RFC,4/7] selftests: openvswitch: delete previously allocated netns

Message ID 20240216152846.1850120-5-aconole@redhat.com (mailing list archive)
State New
Headers show
Series selftests: openvswitch: cleanups for running as selftests | expand

Commit Message

Aaron Conole Feb. 16, 2024, 3:28 p.m. UTC
Many openvswitch test cases reused netns and interface names.  This works
fine as long as the test case cleans up gracefully.  However, if there is
some kind of ungraceful termination (such as an external signal) the netns
or interfaces can be left lingering.  This happens when the selftest
timeout gets exceeded, while running under very slow debugging conditions.

The solution here is to cleanup the netns on executing the next test.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 tools/testing/selftests/net/openvswitch/openvswitch.sh | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Paolo Abeni Feb. 16, 2024, 4:31 p.m. UTC | #1
On Fri, 2024-02-16 at 10:28 -0500, Aaron Conole wrote:
> Many openvswitch test cases reused netns and interface names.  This works
> fine as long as the test case cleans up gracefully.  However, if there is
> some kind of ungraceful termination (such as an external signal) the netns
> or interfaces can be left lingering.  

It looks the openvswitch.sh test script is already trying quite hard to
delete the allocated resources on ungraceful termination via "trap...".

That is usually enough for other self-tests, could you please detail
when it fails here?

> This happens when the selftest
> timeout gets exceeded, while running under very slow debugging conditions.

'timeout' should send SIG_TERM, and the script already handle that
gracefully?

> The solution here is to cleanup the netns on executing the next test.

I suggest avoiding this, it could end up killing innocent alias netns.

You could consider using the 'setup_ns' helper from the
tools/testing/selftests/net/lib.sh library to always generate unique
netns names.

> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  tools/testing/selftests/net/openvswitch/openvswitch.sh | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 678a72ad47c1..8dc315585710 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -115,6 +115,10 @@ ovs_netns_spawn_daemon() {
>  
>  ovs_add_netns_and_veths () {
>  	info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
> +	ntns_e=`ip netns list | grep $3`
> +	[ "$ntns_e" != "" ] && ip netns del "$3"
> +	if4_e=`ip link show $4 2>/dev/null`

Minor unrelated note: $() is preferable to `` for sub-shells, as it's
more friendly to nesting, string expansing, quotes, etc.


Cheers,

Paolo
Aaron Conole Feb. 16, 2024, 6:50 p.m. UTC | #2
Paolo Abeni <pabeni@redhat.com> writes:

> On Fri, 2024-02-16 at 10:28 -0500, Aaron Conole wrote:
>> Many openvswitch test cases reused netns and interface names.  This works
>> fine as long as the test case cleans up gracefully.  However, if there is
>> some kind of ungraceful termination (such as an external signal) the netns
>> or interfaces can be left lingering.  
>
> It looks the openvswitch.sh test script is already trying quite hard to
> delete the allocated resources on ungraceful termination via "trap...".
>
> That is usually enough for other self-tests, could you please detail
> when it fails here?

I thought it should work - but at least what I observed is that when the
vng spawned VM was running the tests, it would TERM portions of the
subshell, but not the running openvswitch.sh script.  That left these
namespaces and interfaces lingering.

>> This happens when the selftest
>> timeout gets exceeded, while running under very slow debugging conditions.
>
> 'timeout' should send SIG_TERM, and the script already handle that
> gracefully?

At least, I didn't observe that to be the case when it got terminated.
I'll remove the timeout setting and try to reproduce it.

>> The solution here is to cleanup the netns on executing the next test.
>
> I suggest avoiding this, it could end up killing innocent alias netns.
>
> You could consider using the 'setup_ns' helper from the
> tools/testing/selftests/net/lib.sh library to always generate unique
> netns names.

Okay - I will look into that.

>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  tools/testing/selftests/net/openvswitch/openvswitch.sh | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> index 678a72ad47c1..8dc315585710 100755
>> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> @@ -115,6 +115,10 @@ ovs_netns_spawn_daemon() {
>>  
>>  ovs_add_netns_and_veths () {
>>  	info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
>> +	ntns_e=`ip netns list | grep $3`
>> +	[ "$ntns_e" != "" ] && ip netns del "$3"
>> +	if4_e=`ip link show $4 2>/dev/null`
>
> Minor unrelated note: $() is preferable to `` for sub-shells, as it's
> more friendly to nesting, string expansing, quotes, etc.

Okay - I'll prefer it in future.  I didn't know how much I should be
worrying about non-POSIX shells (I seem to remember that `` is accepted
in more shells).

> Cheers,
>
> Paolo
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 678a72ad47c1..8dc315585710 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -115,6 +115,10 @@  ovs_netns_spawn_daemon() {
 
 ovs_add_netns_and_veths () {
 	info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
+	ntns_e=`ip netns list | grep $3`
+	[ "$ntns_e" != "" ] && ip netns del "$3"
+	if4_e=`ip link show $4 2>/dev/null`
+	[ "$if4_e" != "" ] && ip link del "$4"
 	ovs_sbx "$1" ip netns add "$3" || return 1
 	on_exit "ovs_sbx $1 ip netns del $3"
 	ovs_sbx "$1" ip link add "$4" type veth peer name "$5" || return 1