ACK: [PATCH] [autotools-client-tests] UBUNTU: SAUCE: ubuntu_kernel_selftests: disable txtimestamp.sh

[Colin Ian King]

On 12/08/2020 12:59, Paolo Pisati wrote:
> The timestamp net selftests exercise the SOF_TIMESTAMPING* code in the network stack
> under a variety of conditions (ipv4/ipv6, tcp/udp/raw/raw_ipproto/pf_packet,
> data/nodata, setsockopt/cmsg), by sending packets round-trip over loopback (but
> through netem to simulate delays), and verifying that timestamps in pkts correspond
> to the expected delay (plus a small delta ~500us to accommodate "noise").
> 
> Unfortunately, txtimestamp has shown a very erratic behaviour for the entire
> Groovy dev cycle.
> 
> Originally, the test was merged upstream around v5.0 in
> toos/testing/selftest/networking/timestamping:
> 
> commit cda261f421ba2682e25f46beb229172706fc7fcd
> Author: Willem de Bruijn <willemb at google.com>
> Date:   Thu Dec 20 16:22:54 2018 -0500
> 
>     selftests: add txtimestamp kselftest
> 
> but disconnected from the rest of kernel selftests (and our
> autotools-client-tests). We started executing it (and seeing failures) in the
> Groovy cycle after it was moved to selftest/net and hooked to kernel selftests
> (commit 5ef5c90e3cb3 "selftests: move timestamping selftests to net folder"),
> but manually running it in a Focal environment shows the same inconsistencies.
> 
> By executing the test in a 10 iteration loop, the chances it doesn't complete
> successfully are very high (and running it in VM makes the failure ratio go up
> significantly):
> 
> $ cd linux/tools/testing/selftests/net
> $ make
> ...
> $ cat << EOF > test.sh
> #!/bin/bash
> 
> THRE=10;
> while /bin/true; do
>         cnt=0;
>         while [ $cnt -lt $THRE ]; do
>                 cnt=$((cnt+1));
>                 sudo ./txtimestamp.sh > /dev/null 2> ./timestamp.log;
>                 [ $? -ne 0 ] && echo "Fail at cycle $cnt" && break;
>         done;
>         [ $cnt -ge $THRE ] && echo "Successfully completed a $THRE-iteration cycle";
> done
> EOF
> 
> $ chmod +x test.sh
> $ ./test.sh > txtimestamp.log
> 
> and let it soak for a bit:
> 
> ...
> Fail at cycle 3
> Fail at cycle 1
> Fail at cycle 5
> Successfully completed a 10-iteration cycle
> Successfully completed a 10-iteration cycle
> Successfully completed a 10-iteration cycle
> Fail at cycle 6
> Successfully completed a 10-iteration cycle
> Fail at cycle 8
> Fail at cycle 9
> Successfully completed a 10-iteration cycle
> Fail at cycle 2
> Fail at cycle 10
> Successfully completed a 10-iteration cycle
> Successfully completed a 10-iteration cycle
> Fail at cycle 3
> Fail at cycle 9
> Fail at cycle 9
> Fail at cycle 1
> Fail at cycle 6
> Successfully completed a 10-iteration cycle
> Fail at cycle 7
> Fail at cycle 9
> Successfully completed a 10-iteration cycle
> Successfully completed a 10-iteration cycle
> Fail at cycle 6
> Fail at cycle 7
> Fail at cycle 6
> Fail at cycle 5
> Successfully completed a 10-iteration cycle
> Successfully completed a 10-iteration cycle
> Fail at cycle 1
> Fail at cycle 4
> ...
> 
> For all these reasons, i don't think we should mark the entire selftests/net as
> FAILED based on txtimestamp results and actually disconnect it from autotools.
> 
> Signed-off-by: Paolo Pisati <paolo.pisati at canonical.com>
> ---
>  ubuntu_kernel_selftests/ubuntu_kernel_selftests.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/ubuntu_kernel_selftests/ubuntu_kernel_selftests.py b/ubuntu_kernel_selftests/ubuntu_kernel_selftests.py
> index e958e537..dc75aec2 100644
> --- a/ubuntu_kernel_selftests/ubuntu_kernel_selftests.py
> +++ b/ubuntu_kernel_selftests/ubuntu_kernel_selftests.py
> @@ -195,6 +195,14 @@ class ubuntu_kernel_selftests(test.test):
>                  cmd = 'sed -i "s/ vmaccess//" ' + mk
>                  utils.system(cmd)
>  
> +            #
> +            # net/txtimestamp.sh is very fragile, disable it
> +            #
> +            fn = 'linux/tools/testing/selftests/net/Makefile'
> +            if os.path.exists(fn):
> +                cmd = 'sed -i "/^TEST_PROGS += txtimestamp.sh$/d" ' + fn
> +                utils.system(cmd)
> +
>      def run_once(self, test_name):
>          if test_name == 'setup':
>              return
> 

Yep, false positives because of fragile tests is a pain. It may be worth
reporting this upstream to see if they can get this working more
reliably in the future.

Acked-by: Colin Ian King <colin.king at canonical.com>