[v2,02/10] test/telemetry: fix test calling all commands

This test was doing nothing as it could not find the telemetry client
script following the test suite rework.

Caught while looking at UNH unit test logs:

/root/workspace/Generic-Unit-Test-DPDK/dpdk/app/test/suites/test_telemetry.sh:
18: /root/workspace/Generic-Unit-Test-DPDK/dpdk/app/usertools/dpdk-telemetry.py:
not found

Fixes: 9da71dc4f96e ("test: add test case for scripted telemetry commands")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/suites/test_telemetry.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Reviewed-by: Marat Khalili <marat.khalili@huawei.com>

Just an idea, in case you have another iteration: could we maybe add a small check that $telemetry_script actually exists and executable to avoid similar situations in the future? Can be as simple as:

test -f "$telemetry_script"
test -x "$telemetry_script"

Due to -e in the first line it should fail the script of any of these tests fail.

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday 23 June 2025 14:53
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Bruce Richardson <bruce.richardson@intel.com>
> Subject: [PATCH v2 02/10] test/telemetry: fix test calling all commands
> 
> This test was doing nothing as it could not find the telemetry client
> script following the test suite rework.
> 
> Caught while looking at UNH unit test logs:
> 
> /root/workspace/Generic-Unit-Test-
> DPDK/dpdk/app/test/suites/test_telemetry.sh:
> 18: /root/workspace/Generic-Unit-Test-DPDK/dpdk/app/usertools/dpdk-
> telemetry.py:
> not found
> 
> Fixes: 9da71dc4f96e ("test: add test case for scripted telemetry commands")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test/suites/test_telemetry.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/app/test/suites/test_telemetry.sh
> b/app/test/suites/test_telemetry.sh
> index ca6abe266e..20806b43e4 100755
> --- a/app/test/suites/test_telemetry.sh
> +++ b/app/test/suites/test_telemetry.sh
> @@ -7,7 +7,7 @@ which jq || {
>      exit 77
>  }
> 
> -rootdir=$(readlink -f $(dirname $(readlink -f $0))/../..)
> +rootdir=$(readlink -f $(dirname $(readlink -f $0))/../../..)
>  tmpoutput=$(mktemp -t dpdk.test_telemetry.XXXXXX)
>  trap "cat $tmpoutput; rm -f $tmpoutput" EXIT
> 
> --
> 2.49.0
  

On Tue, Jun 24, 2025 at 6:00 PM Marat Khalili <marat.khalili@huawei.com> wrote:
>
> Reviewed-by: Marat Khalili <marat.khalili@huawei.com>
>
> Just an idea, in case you have another iteration: could we maybe add a small check that $telemetry_script actually exists and executable to avoid similar situations in the future? Can be as simple as:
>
> test -f "$telemetry_script"
> test -x "$telemetry_script"
>
> Due to -e in the first line it should fail the script of any of these tests fail.

The problem lies in the use of subshell and pipes and that a failure
is not propagated.
Adding a test only the the telemetry script would not catch other
errors (like for example, if the jq command starts to spew errors).

The most elegant would be to use errtrace and pipefail options, but
the errtrace is a bashism (iow not POSIX), and pipefail is POSIX only
since 2022 and many shell (like dash in Ubuntu 22.04/24.04) don't
implement it.

We could try something like:

diff --git a/app/test/suites/test_telemetry.sh
b/app/test/suites/test_telemetry.sh
index ca6abe266e..a81b4add90 100755
--- a/app/test/suites/test_telemetry.sh
+++ b/app/test/suites/test_telemetry.sh
@@ -15,7 +15,7 @@ call_all_telemetry() {
     telemetry_script=$rootdir/usertools/dpdk-telemetry.py
     echo >$tmpoutput
     echo "Telemetry commands log:" >>$tmpoutput
-    for cmd in $(echo / | $telemetry_script | jq -r '.["/"][]')
+    echo / | $telemetry_script | jq -r '.["/"][]' | while read cmd
     do
         for input in $cmd $cmd,0 $cmd,z
         do
@@ -25,4 +25,5 @@ call_all_telemetry() {
     done
 }

-(sleep 1 && call_all_telemetry && echo quit) | $@
+! set -o | grep -q pipefail || set -o pipefail
+(set -e; ! set -o | grep -q pipefail || set -o pipefail; sleep 1 &&
call_all_telemetry && echo quit) | $@
  

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday 26 June 2025 09:33
> 
> The problem lies in the use of subshell and pipes and that a failure
> is not propagated.
> Adding a test only the the telemetry script would not catch other
> errors (like for example, if the jq command starts to spew errors).

Yes, I agree.

> The most elegant would be to use errtrace and pipefail options, but
> the errtrace is a bashism (iow not POSIX), and pipefail is POSIX only
> since 2022 and many shell (like dash in Ubuntu 22.04/24.04) don't
> implement it.

Yes, also true.

> We could try something like:
> 
> diff --git a/app/test/suites/test_telemetry.sh
> b/app/test/suites/test_telemetry.sh
> index ca6abe266e..a81b4add90 100755
> --- a/app/test/suites/test_telemetry.sh
> +++ b/app/test/suites/test_telemetry.sh
> @@ -15,7 +15,7 @@ call_all_telemetry() {
>      telemetry_script=$rootdir/usertools/dpdk-telemetry.py
>      echo >$tmpoutput
>      echo "Telemetry commands log:" >>$tmpoutput
> -    for cmd in $(echo / | $telemetry_script | jq -r '.["/"][]')
> +    echo / | $telemetry_script | jq -r '.["/"][]' | while read cmd
>      do
>          for input in $cmd $cmd,0 $cmd,z
>          do
> @@ -25,4 +25,5 @@ call_all_telemetry() {
>      done
>  }
> 
> -(sleep 1 && call_all_telemetry && echo quit) | $@
> +! set -o | grep -q pipefail || set -o pipefail
> +(set -e; ! set -o | grep -q pipefail || set -o pipefail; sleep 1 &&
> call_all_telemetry && echo quit) | $@

I 100% agree with the idea, but sadly I'm not familiar with shell scripting enough to suggest or review this diff. Is `for cmd in` always equivalent to `while read cmd`? Is CI ever executing it in bash for our attempt to set pipefail to be justified? Is it idiomatic? I hope someone else here can help with this.

Perhaps this should just be re-written in Python. It depends on a Python script anyway.
  

On Thu, Jun 26, 2025 at 11:53 AM Marat Khalili <marat.khalili@huawei.com> wrote:
> > @@ -15,7 +15,7 @@ call_all_telemetry() {
> >      telemetry_script=$rootdir/usertools/dpdk-telemetry.py
> >      echo >$tmpoutput
> >      echo "Telemetry commands log:" >>$tmpoutput
> > -    for cmd in $(echo / | $telemetry_script | jq -r '.["/"][]')
> > +    echo / | $telemetry_script | jq -r '.["/"][]' | while read cmd
> >      do
> >          for input in $cmd $cmd,0 $cmd,z
> >          do
> > @@ -25,4 +25,5 @@ call_all_telemetry() {
> >      done
> >  }
> >
> > -(sleep 1 && call_all_telemetry && echo quit) | $@
> > +! set -o | grep -q pipefail || set -o pipefail
> > +(set -e; ! set -o | grep -q pipefail || set -o pipefail; sleep 1 &&
> > call_all_telemetry && echo quit) | $@
>
> I 100% agree with the idea, but sadly I'm not familiar with shell scripting enough to suggest or review this diff. Is `for cmd in` always equivalent to `while read cmd`? Is CI ever executing it in bash for our attempt to set pipefail to be justified? Is it idiomatic? I hope someone else here can help with this.

- From my experiment, the difference between 'for cmd in $(xxx)' and
'xxx | while read cmd' is that an error is not propagated in the
former case.

I suppose it has to do with the for loop, as described in POSIX:
"""
Exit Status

If there is at least one item in the list of items, the exit status of
a for command shall be the exit status of the last compound-list
executed. If there are no items, the exit status shall be zero.
"""

On error of the command, there is no item in the list of the for loop,
so the loop is overall evaluated as a success.


- As far as the CI is concened, the unit tests are run on various
distributions, including Fedora at UNH.
Since the default shell for Fedora is bash, then an error would be caught there.


>
> Perhaps this should just be re-written in Python. It depends on a Python script anyway.

Maybe.
Though if we go that way, we would need some refactoring of the
telemetry client script, defining some python class etc...
  

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday 3 July 2025 15:10
> 
> On Thu, Jun 26, 2025 at 11:53 AM Marat Khalili <marat.khalili@huawei.com>
> wrote:
> > > @@ -15,7 +15,7 @@ call_all_telemetry() {
> > >      telemetry_script=$rootdir/usertools/dpdk-telemetry.py
> > >      echo >$tmpoutput
> > >      echo "Telemetry commands log:" >>$tmpoutput
> > > -    for cmd in $(echo / | $telemetry_script | jq -r '.["/"][]')
> > > +    echo / | $telemetry_script | jq -r '.["/"][]' | while read cmd
> > >      do
> > >          for input in $cmd $cmd,0 $cmd,z
> > >          do
> > > @@ -25,4 +25,5 @@ call_all_telemetry() {
> > >      done
> > >  }
> > >
> > > -(sleep 1 && call_all_telemetry && echo quit) | $@
> > > +! set -o | grep -q pipefail || set -o pipefail
> > > +(set -e; ! set -o | grep -q pipefail || set -o pipefail; sleep 1 &&
> > > call_all_telemetry && echo quit) | $@
> >
> > I 100% agree with the idea, but sadly I'm not familiar with shell scripting
> enough to suggest or review this diff. Is `for cmd in` always equivalent to
> `while read cmd`? Is CI ever executing it in bash for our attempt to set
> pipefail to be justified? Is it idiomatic? I hope someone else here can help
> with this.
> 
> - From my experiment, the difference between 'for cmd in $(xxx)' and
> 'xxx | while read cmd' is that an error is not propagated in the
> former case.
> 
> I suppose it has to do with the for loop, as described in POSIX:
> """
> Exit Status
> 
> If there is at least one item in the list of items, the exit status of
> a for command shall be the exit status of the last compound-list
> executed. If there are no items, the exit status shall be zero.
> """
> 
> On error of the command, there is no item in the list of the for loop,
> so the loop is overall evaluated as a success.

BTW what would be the behaviour of the new code for the empty input?

> - As far as the CI is concened, the unit tests are run on various
> distributions, including Fedora at UNH.
> Since the default shell for Fedora is bash, then an error would be caught
> there.
> 
> 
> >
> > Perhaps this should just be re-written in Python. It depends on a Python
> script anyway.
> 
> Maybe.
> Though if we go that way, we would need some refactoring of the
> telemetry client script, defining some python class etc...

I guess it's a balance between effort and gain, and we are not making it worse (it will still need to pass CI where we can double check the behaviour)... I now support this change, thanks for detailed explanations and research.

Acked-by: Marat Khalili <marat.khalili@huawei.com>