| Message ID | 20250110000752.81062-1-alessandro.zanni87@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/net/forwarding: teamd command not found | expand |
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()?
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.
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>
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?
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.
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 }
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(+)