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.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 <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?

This seems to fix the problem:

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?

  Thomas