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

[Thomas Huth <thuth@redhat.com>]

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 <thuth@redhat.com>
>>>> Acked-by: Andrew Jones <andrew.jones@linux.dev>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>> ---
>>>>    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?

  Thomas