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

Thomas Huth <thuth@xxxxxxxxxx> · Wed, 8 May 2024 15:36:13 +0200

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@xxxxxxxxxx>
Acked-by: Andrew Jones <andrew.jones@xxxxxxxxx>
Signed-off-by: Nicholas Piggin <npiggin@xxxxxxxxx>
---
   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








