Subject: Re: [kvm-unit-tests PATCH v9 07/31] scripts: allow machine option to be specified in unittests.cfg

From: "Nicholas Piggin" <[email-masked]>
Date: Thu, 09 May 2024 15:44:27 +1000

On Wed May 8, 2024 at 11:36 PM AEST, Thomas Huth wrote:
> On 08/05/2024 14.58, Thomas Huth wrote:
> > On 08/05/2024 14.55, Thomas Huth wrote:
> >> On 08/05/2024 14.27, Nicholas Piggin wrote:
> >>> On Wed May 8, 2024 at 1:08 AM AEST, Thomas Huth wrote:
> >>>> On 04/05/2024 14.28, Nicholas Piggin wrote:
> >>>>> This allows different machines with different requirements to be
> >>>>> supported by run_tests.sh, similarly to how different accelerators
> >>>>> are handled.
> >>>>>
> >>>>> Acked-by: Thomas Huth <[email-masked]>
> >>>>> Acked-by: Andrew Jones <[email-masked]>
> >>>>> Signed-off-by: Nicholas Piggin <[email-masked]>
> >>>>> ---
> >>>>>    docs/unittests.txt   |  7 +++++++
> >>>>>    scripts/common.bash  |  8 ++++++--
> >>>>>    scripts/runtime.bash | 16 ++++++++++++----
> >>>>>    3 files changed, 25 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/docs/unittests.txt b/docs/unittests.txt
> >>>>> index 7cf2c55ad..6449efd78 100644
> >>>>> --- a/docs/unittests.txt
> >>>>> +++ b/docs/unittests.txt
> >>>>> @@ -42,6 +42,13 @@ For <arch>/ directories that support multiple 
> >>>>> architectures, this restricts
> >>>>>    the test to the specified arch. By default, the test will run on any
> >>>>>    architecture.
> >>>>> +machine
> >>>>> +-------
> >>>>> +For those architectures that support multiple machine types, this 
> >>>>> restricts
> >>>>> +the test to the specified machine. By default, the test will run on
> >>>>> +any machine type. (Note, the machine can be specified with the MACHINE=
> >>>>> +environment variable, and defaults to the architecture's default.)
> >>>>> +
> >>>>>    smp
> >>>>>    ---
> >>>>>    smp = <number>
> >>>>> diff --git a/scripts/common.bash b/scripts/common.bash
> >>>>> index 5e9ad53e2..3aa557c8c 100644
> >>>>> --- a/scripts/common.bash
> >>>>> +++ b/scripts/common.bash
> >>>>> @@ -10,6 +10,7 @@ function for_each_unittest()
> >>>>>        local opts
> >>>>>        local groups
> >>>>>        local arch
> >>>>> +    local machine
> >>>>>        local check
> >>>>>        local accel
> >>>>>        local timeout
> >>>>> @@ -21,7 +22,7 @@ function for_each_unittest()
> >>>>>            if [[ "$line" =~ ^\[(.*)\]$ ]]; then
> >>>>>                rematch=${BASH_REMATCH[1]}
> >>>>>                if [ -n "${testname}" ]; then
> >>>>> -                $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" 
> >>>>> "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> >>>>> +                $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" 
> >>>>> "$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout"
> >>>>>                fi
> >>>>>                testname=$rematch
> >>>>>                smp=1
> >>>>> @@ -29,6 +30,7 @@ function for_each_unittest()
> >>>>>                opts=""
> >>>>>                groups=""
> >>>>>                arch=""
> >>>>> +            machine=""
> >>>>>                check=""
> >>>>>                accel=""
> >>>>>                timeout=""
> >>>>> @@ -58,6 +60,8 @@ function for_each_unittest()
> >>>>>                groups=${BASH_REMATCH[1]}
> >>>>>            elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
> >>>>>                arch=${BASH_REMATCH[1]}
> >>>>> +        elif [[ $line =~ ^machine\ *=\ *(.*)$ ]]; then
> >>>>> +            machine=${BASH_REMATCH[1]}
> >>>>>            elif [[ $line =~ ^check\ *=\ *(.*)$ ]]; then
> >>>>>                check=${BASH_REMATCH[1]}
> >>>>>            elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then
> >>>>> @@ -67,7 +71,7 @@ function for_each_unittest()
> >>>>>            fi
> >>>>>        done
> >>>>>        if [ -n "${testname}" ]; then
> >>>>> -        $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" 
> >>>>> "$opts" "$arch" "$check" "$accel" "$timeout"
> >>>>> +        $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" 
> >>>>> "$opts" "$arch" "$machine" "$check" "$accel" "$timeout"
> >>>>>        fi
> >>>>>        exec {fd}<&-
> >>>>>    }
> >>>>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> >>>>> index 177b62166..0c96d6ea2 100644
> >>>>> --- a/scripts/runtime.bash
> >>>>> +++ b/scripts/runtime.bash
> >>>>> @@ -32,7 +32,7 @@ premature_failure()
> >>>>>    get_cmdline()
> >>>>>    {
> >>>>>        local kernel=$1
> >>>>> -    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel 
> >>>>> $RUNTIME_arch_run $kernel -smp $smp $opts"
> >>>>> +    echo "TESTNAME=$testname TIMEOUT=$timeout MACHINE=$machine 
> >>>>> ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
> >>>>>    }
> >>>>>    skip_nodefault()
> >>>>> @@ -80,9 +80,10 @@ function run()
> >>>>>        local kernel="$4"
> >>>>>        local opts="$5"
> >>>>>        local arch="$6"
> >>>>> -    local check="${CHECK:-$7}"
> >>>>> -    local accel="$8"
> >>>>> -    local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default
> >>>>> +    local machine="$7"
> >>>>> +    local check="${CHECK:-$8}"
> >>>>> +    local accel="$9"
> >>>>> +    local timeout="${10:-$TIMEOUT}" # unittests.cfg overrides the default
> >>>>>        if [ "${CONFIG_EFI}" == "y" ]; then
> >>>>>            kernel=${kernel/%.flat/.efi}
> >>>>> @@ -116,6 +117,13 @@ function run()
> >>>>>            return 2
> >>>>>        fi
> >>>>> +    if [ -n "$machine" ] && [ -n "$MACHINE" ] && [ "$machine" != 
> >>>>> "$MACHINE" ]; then
> >>>>> +        print_result "SKIP" $testname "" "$machine only"
> >>>>> +        return 2
> >>>>> +    elif [ -n "$MACHINE" ]; then
> >>>>> +        machine="$MACHINE"
> >>>>> +    fi
> >>>>> +
> >>>>>        if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" 
> >>>>> ]; then
> >>>>>            print_result "SKIP" $testname "" "$accel only, but 
> >>>>> ACCEL=$ACCEL"
> >>>>>            return 2
> >>>>
> >>>> For some reasons that I don't quite understand yet, this patch causes the
> >>>> "sieve" test to always timeout on the s390x runner, see e.g.:
> >>>>
> >>>>    https://gitlab.com/thuth/kvm-unit-tests/-/jobs/6798954987
> >>>
> >>> How do you use the s390x runner?
> >>>
> >>>>
> >>>> Everything is fine in the previous patches (I pushed now the previous 5
> >>>> patches to the repo):
> >>>>
> >>>>    https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/pipelines/1281919104
> >>>>
> >>>> Could it be that he TIMEOUT gets messed up in certain cases?
> >>>
> >>> Hmm not sure yet. At least it got timeout right for the duration=90s
> >>> message.
> >>
> >> That seems to be wrong, the test is declared like this in 
> >> s390x/unittests.cfg :
> >>
> >> [sieve]
> >> file = sieve.elf
> >> groups = selftest
> >> # can take fairly long when KVM is nested inside z/VM
> >> timeout = 600
> >>
> >> And indeed, it takes way longer than 90 seconds on that CI machine, so the 
> >> timeout after 90 seconds should not occur here...
> > 
> > I guess you need to adjust arch_cmd_s390x in scripts/s390x/func.bash to be 
> > aware of the new parameter, too?
>
> This seems to fix the problem:

Thanks, that looks good.

> diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
> index fa47d019..6b817727 100644
> --- a/scripts/s390x/func.bash
> +++ b/scripts/s390x/func.bash
> @@ -13,12 +13,13 @@ function arch_cmd_s390x()
>          local kernel=$5
>          local opts=$6
>          local arch=$7
> -       local check=$8
> -       local accel=$9
> -       local timeout=${10}
> +       local machine=$8
> +       local check=$9
> +       local accel=${10}
> +       local timeout=${11}
>   
>          # run the normal test case
> -       "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
> +       "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout"
>   
>          # run PV test case
>          if [ "$accel" = 'tcg' ] || grep -q "migration" <<< "$groups"; then
>
> If you don't like to respin, I can add it to the patch while picking it up?

Yeah I shouldn't resend the full series just for this. If you're happy
to squash it in that would be good.

Thanks,
Nick