[v1] selftests/net/forwarding: teamd command not found

[PATCH] selftests/net/forwarding: teamd command not found

Posted by Alessandro Zanni 11 months, 1 week ago

Running "make kselftest TARGETS=net/forwarding" results in several
occurrences of the same error:
 ./lib.sh: line 787: teamd: command not found

Since many tests depends on teamd, this fix stops the tests if the
teamd command is not installed.

Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
---
 tools/testing/selftests/net/forwarding/lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 7337f398f9cc..a6a74a4be4bf 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -784,6 +784,7 @@ team_destroy()
 {
 	local if_name=$1; shift
 
+	require_command $TEAMD
 	$TEAMD -t $if_name -k
 }
 
-- 
2.43.0

Re: [PATCH] selftests/net/forwarding: teamd command not found

Posted by Hangbin Liu 11 months, 1 week ago

On Fri, Jan 10, 2025 at 01:07:44AM +0100, Alessandro Zanni wrote:
> Running "make kselftest TARGETS=net/forwarding" results in several
> occurrences of the same error:
>  ./lib.sh: line 787: teamd: command not found
> 
> Since many tests depends on teamd, this fix stops the tests if the
> teamd command is not installed.
> 
> Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
> ---
>  tools/testing/selftests/net/forwarding/lib.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 7337f398f9cc..a6a74a4be4bf 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -784,6 +784,7 @@ team_destroy()
>  {
>  	local if_name=$1; shift
>  
> +	require_command $TEAMD
>  	$TEAMD -t $if_name -k
>  }

I saw team_create() has `require_command $TEAMD`. Is some test called
team_destroy() before team_create()?

Re: [PATCH] selftests/net/forwarding: teamd command not found

Posted by Alessandro Zanni 11 months, 1 week ago

On Fri, Jan 10, 2025 at 04:09:07AM +0000, Hangbin Liu wrote:
> On Fri, Jan 10, 2025 at 01:07:44AM +0100, Alessandro Zanni wrote:
> > Running "make kselftest TARGETS=net/forwarding" results in several
> > occurrences of the same error:
> >  ./lib.sh: line 787: teamd: command not found
> > 
> > Since many tests depends on teamd, this fix stops the tests if the
> > teamd command is not installed.
> > 
> > Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
> > ---
> >  tools/testing/selftests/net/forwarding/lib.sh | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> > index 7337f398f9cc..a6a74a4be4bf 100644
> > --- a/tools/testing/selftests/net/forwarding/lib.sh
> > +++ b/tools/testing/selftests/net/forwarding/lib.sh
> > @@ -784,6 +784,7 @@ team_destroy()
> >  {
> >  	local if_name=$1; shift
> >  
> > +	require_command $TEAMD
> >  	$TEAMD -t $if_name -k
> >  }
> 
> I saw team_create() has `require_command $TEAMD`. Is some test called
> team_destroy() before team_create()?

Actually, the errors seem to raise in the "cleanup()" method.

So, first, during the test preparation the "team_create()" method is called
and checks if teamd is installed. 
When it fails and skips the test, the "cleanup()" method calls
the "team_destroy()" that raises the errors.

Re: [PATCH] selftests/net/forwarding: teamd command not found

Posted by Jakub Kicinski 11 months, 1 week ago

On Fri, 10 Jan 2025 15:35:23 +0100 Alessandro Zanni wrote:
> On Fri, Jan 10, 2025 at 04:09:07AM +0000, Hangbin Liu wrote:
> > On Fri, Jan 10, 2025 at 01:07:44AM +0100, Alessandro Zanni wrote:  
> > > Running "make kselftest TARGETS=net/forwarding" results in several
> > > occurrences of the same error:
> > >  ./lib.sh: line 787: teamd: command not found
> > > 
> > > Since many tests depends on teamd, this fix stops the tests if the
> > > teamd command is not installed.
> > > 
> > > Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
> > > ---
> > >  tools/testing/selftests/net/forwarding/lib.sh | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> > > index 7337f398f9cc..a6a74a4be4bf 100644
> > > --- a/tools/testing/selftests/net/forwarding/lib.sh
> > > +++ b/tools/testing/selftests/net/forwarding/lib.sh
> > > @@ -784,6 +784,7 @@ team_destroy()
> > >  {
> > >  	local if_name=$1; shift
> > >  
> > > +	require_command $TEAMD
> > >  	$TEAMD -t $if_name -k
> > >  }  
> > 
> > I saw team_create() has `require_command $TEAMD`. Is some test called
> > team_destroy() before team_create()?  
> 
> Actually, the errors seem to raise in the "cleanup()" method.
> 
> So, first, during the test preparation the "team_create()" method is called
> and checks if teamd is installed. 
> When it fails and skips the test, the "cleanup()" method calls
> the "team_destroy()" that raises the errors.

Actually, maybe we're better off switching the team handling to the way
REQUIRE_JQ REQUIRE_MZ etc are handled? That way we'll validate and exit 
when lib is imported / sourced.

Petr, do you have a preference?
-- 
pw-bot: cr

Re: [PATCH] selftests/net/forwarding: teamd command not found

Posted by Petr Machata 11 months, 1 week ago

Jakub Kicinski <kuba@kernel.org> writes:

> On Fri, 10 Jan 2025 15:35:23 +0100 Alessandro Zanni wrote:
>> On Fri, Jan 10, 2025 at 04:09:07AM +0000, Hangbin Liu wrote:
>> > On Fri, Jan 10, 2025 at 01:07:44AM +0100, Alessandro Zanni wrote:  
>> > > Running "make kselftest TARGETS=net/forwarding" results in several
>> > > occurrences of the same error:
>> > >  ./lib.sh: line 787: teamd: command not found
>> > > 
>> > > Since many tests depends on teamd, this fix stops the tests if the
>> > > teamd command is not installed.
>> > > 
>> > > Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
>> > > ---
>> > >  tools/testing/selftests/net/forwarding/lib.sh | 1 +
>> > >  1 file changed, 1 insertion(+)
>> > > 
>> > > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>> > > index 7337f398f9cc..a6a74a4be4bf 100644
>> > > --- a/tools/testing/selftests/net/forwarding/lib.sh
>> > > +++ b/tools/testing/selftests/net/forwarding/lib.sh
>> > > @@ -784,6 +784,7 @@ team_destroy()
>> > >  {
>> > >  	local if_name=$1; shift
>> > >  
>> > > +	require_command $TEAMD
>> > >  	$TEAMD -t $if_name -k
>> > >  }  
>> > 
>> > I saw team_create() has `require_command $TEAMD`. Is some test called
>> > team_destroy() before team_create()?  
>> 
>> Actually, the errors seem to raise in the "cleanup()" method.
>> 
>> So, first, during the test preparation the "team_create()" method is called
>> and checks if teamd is installed. 
>> When it fails and skips the test, the "cleanup()" method calls
>> the "team_destroy()" that raises the errors.
>
> Actually, maybe we're better off switching the team handling to the way
> REQUIRE_JQ REQUIRE_MZ etc are handled? That way we'll validate and exit 
> when lib is imported / sourced.
>
> Petr, do you have a preference?

Yeah, that would be better.

My first though was that this is fine, because it's just for tests that
don't use defer. (With defer, if the creation bails out, you never
schedule the cleanup.)

But if you bail out early from team_destroy(), the rest of the cleanups
will be skipped, which is bad.

So maybe we need something like this:

	$TEAMD -t $if_name -k 2>/dev/null ||:

... i.e. to just quietly tolerate failures. But that's no good either,
because then you never catch issues with the cleanup itself even in
valid runs. So maybe team_create() should note whether $TEAMD was found
and do this conditionally, but at that point, going with REQUIRE_TEAMD
is the cleaner solution.

It looks like we have 7 scripts that use team_create(), so it's not
crazy to just fix them all.

Re: [PATCH] selftests/net/forwarding: teamd command not found

Posted by Hangbin Liu 11 months, 1 week ago

On Fri, Jan 10, 2025 at 03:35:23PM +0100, Alessandro Zanni wrote:
> On Fri, Jan 10, 2025 at 04:09:07AM +0000, Hangbin Liu wrote:
> > On Fri, Jan 10, 2025 at 01:07:44AM +0100, Alessandro Zanni wrote:
> > > Running "make kselftest TARGETS=net/forwarding" results in several
> > > occurrences of the same error:
> > >  ./lib.sh: line 787: teamd: command not found
> > > 
> > > Since many tests depends on teamd, this fix stops the tests if the
> > > teamd command is not installed.
> > > 
> > > Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
> > > ---
> > >  tools/testing/selftests/net/forwarding/lib.sh | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> > > index 7337f398f9cc..a6a74a4be4bf 100644
> > > --- a/tools/testing/selftests/net/forwarding/lib.sh
> > > +++ b/tools/testing/selftests/net/forwarding/lib.sh
> > > @@ -784,6 +784,7 @@ team_destroy()
> > >  {
> > >  	local if_name=$1; shift
> > >  
> > > +	require_command $TEAMD
> > >  	$TEAMD -t $if_name -k
> > >  }
> > 
> > I saw team_create() has `require_command $TEAMD`. Is some test called
> > team_destroy() before team_create()?
> 
> Actually, the errors seem to raise in the "cleanup()" method.
> 
> So, first, during the test preparation the "team_create()" method is called
> and checks if teamd is installed. 
> When it fails and skips the test, the "cleanup()" method calls
> the "team_destroy()" that raises the errors.
> 

Thanks for the clarify.

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>