[PATCH v2 0/5] tests/functional: Convert tests with find_free_ports()

[PATCH v2 0/5] tests/functional: Convert tests with find_free_ports()

[Thomas Huth]

Convert the vnc and migration test that use a find_free_ports()
function to look for a free port. Since the functional tests can
be run in parallel with other tests that might look for free ports,
we extract the find_free_ports() function into a helper and provide
a locking mechanism so that the tests don't race for the free ports.

v2:
- Use "-vnc ...,to=XYZ" in the "Do not use a hard-coded VNC port"
  to avoid that we have to take the lock of find_free_ports() here.
- Put the lock file into the build directory (as suggested by Daniel)
- Limit the port range to dynamic ports (see RFC 6335) and
  "randomize" the start address via the PID of the test program
  ==> This should make clashes much less likely

Thomas Huth (5):
  tests/functional: Convert the vnc test
  tests/functional/test_vnc: Remove the test_no_vnc test
  tests/functional/test_vnc: Do not use a hard-coded VNC port
  tests/functional: Extract the find_free_ports() function into a helper
    file
  tests/functional: Convert the migration avocado test

 MAINTAINERS                                   |   1 +
 tests/functional/meson.build                  |  20 +++
 tests/functional/qemu_test/ports.py           |  56 ++++++++
 .../test_migration.py}                        | 121 +++++-------------
 .../vnc.py => functional/test_vnc.py}         |  55 +++-----
 5 files changed, 127 insertions(+), 126 deletions(-)
 create mode 100644 tests/functional/qemu_test/ports.py
 rename tests/{avocado/migration.py => functional/test_migration.py} (41%)
 mode change 100644 => 100755
 rename tests/{avocado/vnc.py => functional/test_vnc.py} (74%)
 mode change 100644 => 100755

-- 
2.47.1

[PATCH v2 1/5] tests/functional: Convert the vnc test

[Thomas Huth]

Nothing thrilling in here, it's just a straight forward conversion.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/meson.build                     |  1 +
 tests/{avocado/vnc.py => functional/test_vnc.py} | 12 +++++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)
 rename tests/{avocado/vnc.py => functional/test_vnc.py} (97%)
 mode change 100644 => 100755

diff --git a/tests/functional/meson.build b/tests/functional/meson.build
index 09a6a97afc..781bd7eae6 100644
--- a/tests/functional/meson.build
+++ b/tests/functional/meson.build
@@ -225,6 +225,7 @@ tests_x86_64_system_quick = [
   'pc_cpu_hotplug_props',
   'virtio_version',
   'x86_cpu_model_versions',
+  'vnc',
 ]
 
 tests_x86_64_system_thorough = [
diff --git a/tests/avocado/vnc.py b/tests/functional/test_vnc.py
old mode 100644
new mode 100755
similarity index 97%
rename from tests/avocado/vnc.py
rename to tests/functional/test_vnc.py
index 862c8996a8..b769d3b268
--- a/tests/avocado/vnc.py
+++ b/tests/functional/test_vnc.py
@@ -1,3 +1,5 @@
+#!/usr/bin/env python3
+#
 # Simple functional tests for VNC functionality
 #
 # Copyright (c) 2018 Red Hat, Inc.
@@ -11,7 +13,7 @@
 import socket
 from typing import List
 
-from avocado_qemu import QemuSystemTest
+from qemu_test import QemuSystemTest
 
 
 VNC_ADDR = '127.0.0.1'
@@ -51,10 +53,7 @@ def find_free_ports(count: int) -> List[int]:
 
 
 class Vnc(QemuSystemTest):
-    """
-    :avocado: tags=vnc,quick
-    :avocado: tags=machine:none
-    """
+
     def test_no_vnc(self):
         self.vm.add_args('-nodefaults', '-S')
         self.vm.launch()
@@ -113,3 +112,6 @@ def test_change_listen(self):
         self.assertFalse(check_connect(a))
         self.assertTrue(check_connect(b))
         self.assertTrue(check_connect(c))
+
+if __name__ == '__main__':
+    QemuSystemTest.main()
-- 
2.47.1

[PATCH v2 2/5] tests/functional/test_vnc: Remove the test_no_vnc test

[Thomas Huth]

This test matches exactly the first three lines of the following
test_no_vnc_change_password test, so there is exactly zero additional
test coverage in here.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/test_vnc.py | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/tests/functional/test_vnc.py b/tests/functional/test_vnc.py
index b769d3b268..42ffcec217 100755
--- a/tests/functional/test_vnc.py
+++ b/tests/functional/test_vnc.py
@@ -54,11 +54,6 @@ def find_free_ports(count: int) -> List[int]:
 
 class Vnc(QemuSystemTest):
 
-    def test_no_vnc(self):
-        self.vm.add_args('-nodefaults', '-S')
-        self.vm.launch()
-        self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled'])
-
     def test_no_vnc_change_password(self):
         self.vm.add_args('-nodefaults', '-S')
         self.vm.launch()
-- 
2.47.1

[PATCH v2 3/5] tests/functional/test_vnc: Do not use a hard-coded VNC port

[Thomas Huth]

Two tests here are using the hard-coded VNC port :0 ... if there
is already a QEMU or other program running that is using this
port, the tests will be failing. Fortunately, QEMU can also
auto-detect a free port with the "to=..." parameter, so let's
use that for the tests to avoid the problem.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/test_vnc.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/functional/test_vnc.py b/tests/functional/test_vnc.py
index 42ffcec217..e600d75234 100755
--- a/tests/functional/test_vnc.py
+++ b/tests/functional/test_vnc.py
@@ -67,7 +67,7 @@ def test_no_vnc_change_password(self):
                          'Could not set password')
 
     def test_change_password_requires_a_password(self):
-        self.vm.add_args('-nodefaults', '-S', '-vnc', ':0')
+        self.vm.add_args('-nodefaults', '-S', '-vnc', ':1,to=999')
         self.vm.launch()
         self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
         set_password_response = self.vm.qmp('change-vnc-password',
@@ -79,7 +79,7 @@ def test_change_password_requires_a_password(self):
                          'Could not set password')
 
     def test_change_password(self):
-        self.vm.add_args('-nodefaults', '-S', '-vnc', ':0,password=on')
+        self.vm.add_args('-nodefaults', '-S', '-vnc', ':1,to=999,password=on')
         self.vm.launch()
         self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
         self.vm.cmd('change-vnc-password',
-- 
2.47.1

[PATCH v2 4/5] tests/functional: Extract the find_free_ports() function into a helper file

[Thomas Huth]

We'll need this functionality in other functional tests, too, so
let's extract it into the qemu_test module.
Also add  an __enter__ and __exit__ function that can be used for
using this functionality in a locked context, so that tests that
are running in parallel don't try to compete for the same ports
later.
Also make sure to only use ports in the "Dynamic Ports" range
(see https://www.rfc-editor.org/rfc/rfc6335) and "randomize" the
start of the probed range with the PID of the test process to
further avoid possible clashes with other competing processes.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/qemu_test/ports.py | 56 +++++++++++++++++++++++++++++
 tests/functional/test_vnc.py        | 36 +++++--------------
 2 files changed, 64 insertions(+), 28 deletions(-)
 create mode 100644 tests/functional/qemu_test/ports.py

diff --git a/tests/functional/qemu_test/ports.py b/tests/functional/qemu_test/ports.py
new file mode 100644
index 0000000000..cc39939d48
--- /dev/null
+++ b/tests/functional/qemu_test/ports.py
@@ -0,0 +1,56 @@
+#!/usr/bin/env python3
+#
+# Simple functional tests for VNC functionality
+#
+# Copyright 2018, 2024 Red Hat, Inc.
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+import fcntl
+import os
+import socket
+import sys
+import tempfile
+
+from .config import BUILD_DIR
+from typing import List
+
+class Ports():
+
+    PORTS_ADDR = '127.0.0.1'
+    PORTS_RANGE_SIZE = 1024
+    PORTS_START = 49152 + ((os.getpid() * PORTS_RANGE_SIZE) % 16384)
+    PORTS_END = PORTS_START + PORTS_RANGE_SIZE
+
+    def __enter__(self):
+        lock_file = os.path.join(BUILD_DIR, "tests", "functional", "port_lock")
+        self.lock_fh = os.open(lock_file, os.O_CREAT)
+        fcntl.flock(self.lock_fh, fcntl.LOCK_EX)
+        return self
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        fcntl.flock(self.lock_fh, fcntl.LOCK_UN)
+        os.close(self.lock_fh)
+
+    def check_bind(self, port: int) -> bool:
+        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
+            try:
+                sock.bind((self.PORTS_ADDR, port))
+            except OSError:
+                return False
+
+        return True
+
+    def find_free_ports(self, count: int) -> List[int]:
+        result = []
+        for port in range(self.PORTS_START, self.PORTS_END):
+            if self.check_bind(port):
+                result.append(port)
+                if len(result) >= count:
+                    break
+        assert len(result) == count
+        return result
+
+    def find_free_port(self) -> int:
+        return self.find_free_ports(1)[0]
diff --git a/tests/functional/test_vnc.py b/tests/functional/test_vnc.py
index e600d75234..1916be0103 100755
--- a/tests/functional/test_vnc.py
+++ b/tests/functional/test_vnc.py
@@ -14,22 +14,9 @@
 from typing import List
 
 from qemu_test import QemuSystemTest
-
+from qemu_test.ports import Ports
 
 VNC_ADDR = '127.0.0.1'
-VNC_PORT_START = 32768
-VNC_PORT_END = VNC_PORT_START + 1024
-
-
-def check_bind(port: int) -> bool:
-    with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
-        try:
-            sock.bind((VNC_ADDR, port))
-        except OSError:
-            return False
-
-    return True
-
 
 def check_connect(port: int) -> bool:
     with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
@@ -40,18 +27,6 @@ def check_connect(port: int) -> bool:
 
     return True
 
-
-def find_free_ports(count: int) -> List[int]:
-    result = []
-    for port in range(VNC_PORT_START, VNC_PORT_END):
-        if check_bind(port):
-            result.append(port)
-            if len(result) >= count:
-                break
-    assert len(result) == count
-    return result
-
-
 class Vnc(QemuSystemTest):
 
     def test_no_vnc_change_password(self):
@@ -85,8 +60,7 @@ def test_change_password(self):
         self.vm.cmd('change-vnc-password',
                     password='new_password')
 
-    def test_change_listen(self):
-        a, b, c = find_free_ports(3)
+    def do_test_change_listen(self, a, b, c):
         self.assertFalse(check_connect(a))
         self.assertFalse(check_connect(b))
         self.assertFalse(check_connect(c))
@@ -108,5 +82,11 @@ def test_change_listen(self):
         self.assertTrue(check_connect(b))
         self.assertTrue(check_connect(c))
 
+    def test_change_listen(self):
+        with Ports() as ports:
+            a, b, c = ports.find_free_ports(3)
+            self.do_test_change_listen(a, b, c)
+
+
 if __name__ == '__main__':
     QemuSystemTest.main()
-- 
2.47.1

[PATCH v2 5/5] tests/functional: Convert the migration avocado test

[Thomas Huth]

Now that we've got a find_free_port() function in the functional
test framework, we can convert the migration test, too.
While the original avocado test was only meant to run on aarch64,
ppc64 and x86, we can turn this into a more generic test by now
and run it on all architectures that have a default machine that
ships with a working firmware.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 MAINTAINERS                                   |   1 +
 tests/functional/meson.build                  |  19 +++
 .../test_migration.py}                        | 121 +++++-------------
 3 files changed, 54 insertions(+), 87 deletions(-)
 rename tests/{avocado/migration.py => functional/test_migration.py} (41%)
 mode change 100644 => 100755

diff --git a/MAINTAINERS b/MAINTAINERS
index 389b390de1..704648d57a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3429,6 +3429,7 @@ F: include/migration/
 F: include/qemu/userfaultfd.h
 F: migration/
 F: scripts/vmstate-static-checker.py
+F: tests/functional/test_migration.py
 F: tests/vmstate-static-checker-data/
 F: tests/qtest/migration/
 F: tests/qtest/migration-*
diff --git a/tests/functional/meson.build b/tests/functional/meson.build
index 781bd7eae6..8c3d1c26da 100644
--- a/tests/functional/meson.build
+++ b/tests/functional/meson.build
@@ -70,6 +70,10 @@ tests_aarch64_system_thorough = [
   'multiprocess',
 ]
 
+tests_alpha_system_quick = [
+  'migration',
+]
+
 tests_alpha_system_thorough = [
   'alpha_clipper',
 ]
@@ -167,6 +171,10 @@ tests_ppc_system_thorough = [
   'ppc_virtex_ml507',
 ]
 
+tests_ppc64_system_quick = [
+  'migration',
+]
+
 tests_ppc64_system_thorough = [
   'ppc64_e500',
   'ppc64_hv',
@@ -176,6 +184,7 @@ tests_ppc64_system_thorough = [
 ]
 
 tests_riscv32_system_quick = [
+  'migration',
   'riscv_opensbi',
 ]
 
@@ -184,6 +193,7 @@ tests_riscv32_system_thorough = [
 ]
 
 tests_riscv64_system_quick = [
+  'migration',
   'riscv_opensbi',
 ]
 
@@ -210,10 +220,18 @@ tests_sh4eb_system_thorough = [
   'sh4eb_r2d',
 ]
 
+tests_sparc_system_quick = [
+  'migration',
+]
+
 tests_sparc_system_thorough = [
   'sparc_sun4m',
 ]
 
+tests_sparc64_system_quick = [
+  'migration',
+]
+
 tests_sparc64_system_thorough = [
   'sparc64_sun4u',
   'sparc64_tuxrun',
@@ -222,6 +240,7 @@ tests_sparc64_system_thorough = [
 tests_x86_64_system_quick = [
   'cpu_queries',
   'mem_addr_space',
+  'migration',
   'pc_cpu_hotplug_props',
   'virtio_version',
   'x86_cpu_model_versions',
diff --git a/tests/avocado/migration.py b/tests/functional/test_migration.py
old mode 100644
new mode 100755
similarity index 41%
rename from tests/avocado/migration.py
rename to tests/functional/test_migration.py
index be6234b3c2..5607d601eb
--- a/tests/avocado/migration.py
+++ b/tests/functional/test_migration.py
@@ -1,3 +1,5 @@
+#!/usr/bin/env python3
+#
 # Migration test
 #
 # Copyright (c) 2019 Red Hat, Inc.
@@ -9,22 +11,14 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or
 # later.  See the COPYING file in the top-level directory.
 
-
-import tempfile
 import os
+import tempfile
+import time
 
-from avocado_qemu import QemuSystemTest
-from avocado import skipUnless
-
-from avocado.utils.network import ports
-from avocado.utils import wait
-from avocado.utils.path import find_command
-
+from qemu_test import QemuSystemTest, skipIfMissingCommands
+from qemu_test.ports import Ports
 
 class MigrationTest(QemuSystemTest):
-    """
-    :avocado: tags=migration
-    """
 
     timeout = 10
 
@@ -33,103 +27,56 @@ def migration_finished(vm):
         return vm.cmd('query-migrate')['status'] in ('completed', 'failed')
 
     def assert_migration(self, src_vm, dst_vm):
-        wait.wait_for(self.migration_finished,
-                      timeout=self.timeout,
-                      step=0.1,
-                      args=(src_vm,))
-        wait.wait_for(self.migration_finished,
-                      timeout=self.timeout,
-                      step=0.1,
-                      args=(dst_vm,))
+
+        end = time.monotonic() + self.timeout
+        while time.monotonic() < end and not self.migration_finished(src_vm):
+           time.sleep(0.1)
+
+        end = time.monotonic() + self.timeout
+        while time.monotonic() < end and not self.migration_finished(dst_vm):
+           time.sleep(0.1)
+
         self.assertEqual(src_vm.cmd('query-migrate')['status'], 'completed')
         self.assertEqual(dst_vm.cmd('query-migrate')['status'], 'completed')
         self.assertEqual(dst_vm.cmd('query-status')['status'], 'running')
         self.assertEqual(src_vm.cmd('query-status')['status'],'postmigrate')
 
     def do_migrate(self, dest_uri, src_uri=None):
-        dest_vm = self.get_vm('-incoming', dest_uri)
+        dest_vm = self.get_vm('-incoming', dest_uri, name="dest-qemu")
         dest_vm.add_args('-nodefaults')
         dest_vm.launch()
         if src_uri is None:
             src_uri = dest_uri
-        source_vm = self.get_vm()
+        source_vm = self.get_vm(name="source-qemu")
         source_vm.add_args('-nodefaults')
         source_vm.launch()
         source_vm.qmp('migrate', uri=src_uri)
         self.assert_migration(source_vm, dest_vm)
 
-    def _get_free_port(self):
+    def _get_free_port(self, ports):
         port = ports.find_free_port()
         if port is None:
-            self.cancel('Failed to find a free port')
+            self.skipTest('Failed to find a free port')
         return port
 
-    def migration_with_tcp_localhost(self):
-        dest_uri = 'tcp:localhost:%u' % self._get_free_port()
-        self.do_migrate(dest_uri)
+    def test_migration_with_tcp_localhost(self):
+        with Ports() as ports:
+            dest_uri = 'tcp:localhost:%u' % self._get_free_port(ports)
+            self.do_migrate(dest_uri)
 
-    def migration_with_unix(self):
+    def test_migration_with_unix(self):
         with tempfile.TemporaryDirectory(prefix='socket_') as socket_path:
             dest_uri = 'unix:%s/qemu-test.sock' % socket_path
             self.do_migrate(dest_uri)
 
-    @skipUnless(find_command('nc', default=False), "'nc' command not found")
-    def migration_with_exec(self):
-        """The test works for both netcat-traditional and netcat-openbsd packages."""
-        free_port = self._get_free_port()
-        dest_uri = 'exec:nc -l localhost %u' % free_port
-        src_uri = 'exec:nc localhost %u' % free_port
-        self.do_migrate(dest_uri, src_uri)
-
-
-@skipUnless('aarch64' in os.uname()[4], "host != target")
-class Aarch64(MigrationTest):
-    """
-    :avocado: tags=arch:aarch64
-    :avocado: tags=machine:virt
-    :avocado: tags=cpu:max
-    """
-
-    def test_migration_with_tcp_localhost(self):
-        self.migration_with_tcp_localhost()
-
-    def test_migration_with_unix(self):
-        self.migration_with_unix()
-
-    def test_migration_with_exec(self):
-        self.migration_with_exec()
-
-
-@skipUnless('x86_64' in os.uname()[4], "host != target")
-class X86_64(MigrationTest):
-    """
-    :avocado: tags=arch:x86_64
-    :avocado: tags=machine:pc
-    :avocado: tags=cpu:qemu64
-    """
-
-    def test_migration_with_tcp_localhost(self):
-        self.migration_with_tcp_localhost()
-
-    def test_migration_with_unix(self):
-        self.migration_with_unix()
-
+    @skipIfMissingCommands('nc')
     def test_migration_with_exec(self):
-        self.migration_with_exec()
-
-
-@skipUnless('ppc64le' in os.uname()[4], "host != target")
-class PPC64(MigrationTest):
-    """
-    :avocado: tags=arch:ppc64
-    :avocado: tags=machine:pseries
-    """
-
-    def test_migration_with_tcp_localhost(self):
-        self.migration_with_tcp_localhost()
-
-    def test_migration_with_unix(self):
-        self.migration_with_unix()
-
-    def test_migration_with_exec(self):
-        self.migration_with_exec()
+        """The test works for both netcat-traditional and netcat-openbsd packages."""
+        with Ports() as ports:
+            free_port = self._get_free_port(ports)
+            dest_uri = 'exec:nc -l localhost %u' % free_port
+            src_uri = 'exec:nc localhost %u' % free_port
+            self.do_migrate(dest_uri, src_uri)
+
+if __name__ == '__main__':
+    QemuSystemTest.main()
-- 
2.47.1

Re: [PATCH v2 5/5] tests/functional: Convert the migration avocado test

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> Now that we've got a find_free_port() function in the functional
> test framework, we can convert the migration test, too.
> While the original avocado test was only meant to run on aarch64,
> ppc64 and x86, we can turn this into a more generic test by now
> and run it on all architectures that have a default machine that
> ships with a working firmware.

I'd rather drop this test. I haven't looked at it in ages and it has
never been useful. I haven't been following the development of the
functional suite so this might not apply this time (fingers crossed),
but Python tests have always been a pain to work with.

About adding more architectures to the set, this is not simply enabling
more testing, it is also adding workload to maintain these other arches
that were never tested with migration. Is that something we want?

Also note that what is actually prone to break is compatibility between
versions, which is not covered by this test.

>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  MAINTAINERS                                   |   1 +
>  tests/functional/meson.build                  |  19 +++
>  .../test_migration.py}                        | 121 +++++-------------
>  3 files changed, 54 insertions(+), 87 deletions(-)
>  rename tests/{avocado/migration.py => functional/test_migration.py} (41%)
>  mode change 100644 => 100755
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 389b390de1..704648d57a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3429,6 +3429,7 @@ F: include/migration/
>  F: include/qemu/userfaultfd.h
>  F: migration/
>  F: scripts/vmstate-static-checker.py
> +F: tests/functional/test_migration.py
>  F: tests/vmstate-static-checker-data/
>  F: tests/qtest/migration/
>  F: tests/qtest/migration-*
> diff --git a/tests/functional/meson.build b/tests/functional/meson.build
> index 781bd7eae6..8c3d1c26da 100644
> --- a/tests/functional/meson.build
> +++ b/tests/functional/meson.build
> @@ -70,6 +70,10 @@ tests_aarch64_system_thorough = [
>    'multiprocess',
>  ]
>  
> +tests_alpha_system_quick = [
> +  'migration',
> +]
> +
>  tests_alpha_system_thorough = [
>    'alpha_clipper',
>  ]
> @@ -167,6 +171,10 @@ tests_ppc_system_thorough = [
>    'ppc_virtex_ml507',
>  ]
>  
> +tests_ppc64_system_quick = [
> +  'migration',
> +]
> +
>  tests_ppc64_system_thorough = [
>    'ppc64_e500',
>    'ppc64_hv',
> @@ -176,6 +184,7 @@ tests_ppc64_system_thorough = [
>  ]
>  
>  tests_riscv32_system_quick = [
> +  'migration',
>    'riscv_opensbi',
>  ]
>  
> @@ -184,6 +193,7 @@ tests_riscv32_system_thorough = [
>  ]
>  
>  tests_riscv64_system_quick = [
> +  'migration',
>    'riscv_opensbi',
>  ]
>  
> @@ -210,10 +220,18 @@ tests_sh4eb_system_thorough = [
>    'sh4eb_r2d',
>  ]
>  
> +tests_sparc_system_quick = [
> +  'migration',
> +]
> +
>  tests_sparc_system_thorough = [
>    'sparc_sun4m',
>  ]
>  
> +tests_sparc64_system_quick = [
> +  'migration',
> +]
> +
>  tests_sparc64_system_thorough = [
>    'sparc64_sun4u',
>    'sparc64_tuxrun',
> @@ -222,6 +240,7 @@ tests_sparc64_system_thorough = [
>  tests_x86_64_system_quick = [
>    'cpu_queries',
>    'mem_addr_space',
> +  'migration',
>    'pc_cpu_hotplug_props',
>    'virtio_version',
>    'x86_cpu_model_versions',
> diff --git a/tests/avocado/migration.py b/tests/functional/test_migration.py
> old mode 100644
> new mode 100755
> similarity index 41%
> rename from tests/avocado/migration.py
> rename to tests/functional/test_migration.py
> index be6234b3c2..5607d601eb
> --- a/tests/avocado/migration.py
> +++ b/tests/functional/test_migration.py
> @@ -1,3 +1,5 @@
> +#!/usr/bin/env python3
> +#
>  # Migration test
>  #
>  # Copyright (c) 2019 Red Hat, Inc.
> @@ -9,22 +11,14 @@
>  # This work is licensed under the terms of the GNU GPL, version 2 or
>  # later.  See the COPYING file in the top-level directory.
>  
> -
> -import tempfile
>  import os
> +import tempfile
> +import time
>  
> -from avocado_qemu import QemuSystemTest
> -from avocado import skipUnless
> -
> -from avocado.utils.network import ports
> -from avocado.utils import wait
> -from avocado.utils.path import find_command
> -
> +from qemu_test import QemuSystemTest, skipIfMissingCommands
> +from qemu_test.ports import Ports
>  
>  class MigrationTest(QemuSystemTest):
> -    """
> -    :avocado: tags=migration
> -    """
>  
>      timeout = 10
>  
> @@ -33,103 +27,56 @@ def migration_finished(vm):
>          return vm.cmd('query-migrate')['status'] in ('completed', 'failed')
>  
>      def assert_migration(self, src_vm, dst_vm):
> -        wait.wait_for(self.migration_finished,
> -                      timeout=self.timeout,
> -                      step=0.1,
> -                      args=(src_vm,))
> -        wait.wait_for(self.migration_finished,
> -                      timeout=self.timeout,
> -                      step=0.1,
> -                      args=(dst_vm,))
> +
> +        end = time.monotonic() + self.timeout
> +        while time.monotonic() < end and not self.migration_finished(src_vm):
> +           time.sleep(0.1)
> +
> +        end = time.monotonic() + self.timeout
> +        while time.monotonic() < end and not self.migration_finished(dst_vm):
> +           time.sleep(0.1)
> +
>          self.assertEqual(src_vm.cmd('query-migrate')['status'], 'completed')
>          self.assertEqual(dst_vm.cmd('query-migrate')['status'], 'completed')
>          self.assertEqual(dst_vm.cmd('query-status')['status'], 'running')
>          self.assertEqual(src_vm.cmd('query-status')['status'],'postmigrate')
>  
>      def do_migrate(self, dest_uri, src_uri=None):
> -        dest_vm = self.get_vm('-incoming', dest_uri)
> +        dest_vm = self.get_vm('-incoming', dest_uri, name="dest-qemu")
>          dest_vm.add_args('-nodefaults')
>          dest_vm.launch()
>          if src_uri is None:
>              src_uri = dest_uri
> -        source_vm = self.get_vm()
> +        source_vm = self.get_vm(name="source-qemu")
>          source_vm.add_args('-nodefaults')
>          source_vm.launch()
>          source_vm.qmp('migrate', uri=src_uri)
>          self.assert_migration(source_vm, dest_vm)
>  
> -    def _get_free_port(self):
> +    def _get_free_port(self, ports):
>          port = ports.find_free_port()
>          if port is None:
> -            self.cancel('Failed to find a free port')
> +            self.skipTest('Failed to find a free port')
>          return port
>  
> -    def migration_with_tcp_localhost(self):
> -        dest_uri = 'tcp:localhost:%u' % self._get_free_port()
> -        self.do_migrate(dest_uri)
> +    def test_migration_with_tcp_localhost(self):
> +        with Ports() as ports:
> +            dest_uri = 'tcp:localhost:%u' % self._get_free_port(ports)
> +            self.do_migrate(dest_uri)
>  
> -    def migration_with_unix(self):
> +    def test_migration_with_unix(self):
>          with tempfile.TemporaryDirectory(prefix='socket_') as socket_path:
>              dest_uri = 'unix:%s/qemu-test.sock' % socket_path
>              self.do_migrate(dest_uri)
>  
> -    @skipUnless(find_command('nc', default=False), "'nc' command not found")
> -    def migration_with_exec(self):
> -        """The test works for both netcat-traditional and netcat-openbsd packages."""
> -        free_port = self._get_free_port()
> -        dest_uri = 'exec:nc -l localhost %u' % free_port
> -        src_uri = 'exec:nc localhost %u' % free_port
> -        self.do_migrate(dest_uri, src_uri)
> -
> -
> -@skipUnless('aarch64' in os.uname()[4], "host != target")
> -class Aarch64(MigrationTest):
> -    """
> -    :avocado: tags=arch:aarch64
> -    :avocado: tags=machine:virt
> -    :avocado: tags=cpu:max
> -    """
> -
> -    def test_migration_with_tcp_localhost(self):
> -        self.migration_with_tcp_localhost()
> -
> -    def test_migration_with_unix(self):
> -        self.migration_with_unix()
> -
> -    def test_migration_with_exec(self):
> -        self.migration_with_exec()
> -
> -
> -@skipUnless('x86_64' in os.uname()[4], "host != target")
> -class X86_64(MigrationTest):
> -    """
> -    :avocado: tags=arch:x86_64
> -    :avocado: tags=machine:pc
> -    :avocado: tags=cpu:qemu64
> -    """
> -
> -    def test_migration_with_tcp_localhost(self):
> -        self.migration_with_tcp_localhost()
> -
> -    def test_migration_with_unix(self):
> -        self.migration_with_unix()
> -
> +    @skipIfMissingCommands('nc')
>      def test_migration_with_exec(self):
> -        self.migration_with_exec()
> -
> -
> -@skipUnless('ppc64le' in os.uname()[4], "host != target")
> -class PPC64(MigrationTest):
> -    """
> -    :avocado: tags=arch:ppc64
> -    :avocado: tags=machine:pseries
> -    """
> -
> -    def test_migration_with_tcp_localhost(self):
> -        self.migration_with_tcp_localhost()
> -
> -    def test_migration_with_unix(self):
> -        self.migration_with_unix()
> -
> -    def test_migration_with_exec(self):
> -        self.migration_with_exec()
> +        """The test works for both netcat-traditional and netcat-openbsd packages."""
> +        with Ports() as ports:
> +            free_port = self._get_free_port(ports)
> +            dest_uri = 'exec:nc -l localhost %u' % free_port
> +            src_uri = 'exec:nc localhost %u' % free_port
> +            self.do_migrate(dest_uri, src_uri)
> +
> +if __name__ == '__main__':
> +    QemuSystemTest.main()

Re: [PATCH v2 5/5] tests/functional: Convert the migration avocado test

[Thomas Huth]

On 18/12/2024 14.51, Fabiano Rosas wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> Now that we've got a find_free_port() function in the functional
>> test framework, we can convert the migration test, too.
>> While the original avocado test was only meant to run on aarch64,
>> ppc64 and x86, we can turn this into a more generic test by now
>> and run it on all architectures that have a default machine that
>> ships with a working firmware.
> 
> I'd rather drop this test. I haven't looked at it in ages and it has
> never been useful.

I think I agree for the scope of the old avocado test - x86, ppc64 and 
aarch64 certainly have better test coverage by the qtest already... but we 
don't have any test coverage for other architectures at all yet, which is 
bad (see below).

So if you like I can change the patch so that the test is not run on x86, 
ppc64 and aarch64 anymore, just on the other architectures that do not have 
test coverage by the qtest yet?

> I haven't been following the development of the
> functional suite so this might not apply this time (fingers crossed),
> but Python tests have always been a pain to work with.

Well, one of the motivations with the functional test framework was to 
simplify things. You can now run the individual tests without any test 
runner at all, what makes debugging way easier (see 
docs/devel/testing/functional.rst for details)!

> About adding more architectures to the set, this is not simply enabling
> more testing, it is also adding workload to maintain these other arches
> that were never tested with migration. Is that something we want?

I think yes. Otherwise the bugs are just dormant until someone hits the 
issue, making bisection way more complicated later.
Remember this one for example:

  https://mail.gnu.org/archive/html/qemu-commits/2023-02/msg00030.html

?

It would have been good to have a migration test for alpha in the CI, then 
we could have prevented that bug from being merged.

> Also note that what is actually prone to break is compatibility between
> versions, which is not covered by this test.

I think it should be possible to add such a check later.

  Thomas

Re: [PATCH v2 5/5] tests/functional: Convert the migration avocado test

[Daniel P. Berrangé]

On Wed, Dec 18, 2024 at 04:51:24PM +0100, Thomas Huth wrote:
> On 18/12/2024 14.51, Fabiano Rosas wrote:
> > Thomas Huth <thuth@redhat.com> writes:
> > 
> > > Now that we've got a find_free_port() function in the functional
> > > test framework, we can convert the migration test, too.
> > > While the original avocado test was only meant to run on aarch64,
> > > ppc64 and x86, we can turn this into a more generic test by now
> > > and run it on all architectures that have a default machine that
> > > ships with a working firmware.
> > 
> > I'd rather drop this test. I haven't looked at it in ages and it has
> > never been useful.
> 
> I think I agree for the scope of the old avocado test - x86, ppc64 and
> aarch64 certainly have better test coverage by the qtest already... but we
> don't have any test coverage for other architectures at all yet, which is
> bad (see below).
> 
> So if you like I can change the patch so that the test is not run on x86,
> ppc64 and aarch64 anymore, just on the other architectures that do not have
> test coverage by the qtest yet?
> 
> > I haven't been following the development of the
> > functional suite so this might not apply this time (fingers crossed),
> > but Python tests have always been a pain to work with.
> 
> Well, one of the motivations with the functional test framework was to
> simplify things. You can now run the individual tests without any test
> runner at all, what makes debugging way easier (see
> docs/devel/testing/functional.rst for details)!
> 
> > About adding more architectures to the set, this is not simply enabling
> > more testing, it is also adding workload to maintain these other arches
> > that were never tested with migration. Is that something we want?
> 
> I think yes. Otherwise the bugs are just dormant until someone hits the
> issue, making bisection way more complicated later.
> Remember this one for example:
> 
>  https://mail.gnu.org/archive/html/qemu-commits/2023-02/msg00030.html
> 
> ?
> 
> It would have been good to have a migration test for alpha in the CI, then
> we could have prevented that bug from being merged.

IIUC, we run the migration-test  qtest for *every* softmmu target.

So, assuming you're referring to alpha guest, we were already
exercising it.

The migration qtest as it exists today is pushing the boundaries of
what a qtest is. I'd actually call the migration qtest a functional
test that happens to use the qtest framework for historical reasons.

The only slight thing that makes it not a functional test is that it
is using a specialized guest, which is just the custom boot sector
that dirties ram.

A true migration functional test is conceptually interesting, as we
have had bugs in the past which only hit when using real guest OS
in certain ways. The trouble is that worst bugs have been pretty
niche, such that its unlikely we would have pre-empatively have
a test combination that would have hit them.


Anyway, I think a true functional test for migration is relevant
to keep, as long as we make it clearly different from the qtest.
A simple smoke test using a real Linux guest is different enough
from our hand crafted boot sector that I think it is valuable
coverage. Even better if we make the functional test add *lots*
of different devices.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 5/5] tests/functional: Convert the migration avocado test

[Thomas Huth]

On 18/12/2024 17.00, Daniel P. Berrangé wrote:
> On Wed, Dec 18, 2024 at 04:51:24PM +0100, Thomas Huth wrote:
>> On 18/12/2024 14.51, Fabiano Rosas wrote:
>>> Thomas Huth <thuth@redhat.com> writes:
>>>
>>>> Now that we've got a find_free_port() function in the functional
>>>> test framework, we can convert the migration test, too.
>>>> While the original avocado test was only meant to run on aarch64,
>>>> ppc64 and x86, we can turn this into a more generic test by now
>>>> and run it on all architectures that have a default machine that
>>>> ships with a working firmware.
>>>
>>> I'd rather drop this test. I haven't looked at it in ages and it has
>>> never been useful.
>>
>> I think I agree for the scope of the old avocado test - x86, ppc64 and
>> aarch64 certainly have better test coverage by the qtest already... but we
>> don't have any test coverage for other architectures at all yet, which is
>> bad (see below).
>>
>> So if you like I can change the patch so that the test is not run on x86,
>> ppc64 and aarch64 anymore, just on the other architectures that do not have
>> test coverage by the qtest yet?
>>
>>> I haven't been following the development of the
>>> functional suite so this might not apply this time (fingers crossed),
>>> but Python tests have always been a pain to work with.
>>
>> Well, one of the motivations with the functional test framework was to
>> simplify things. You can now run the individual tests without any test
>> runner at all, what makes debugging way easier (see
>> docs/devel/testing/functional.rst for details)!
>>
>>> About adding more architectures to the set, this is not simply enabling
>>> more testing, it is also adding workload to maintain these other arches
>>> that were never tested with migration. Is that something we want?
>>
>> I think yes. Otherwise the bugs are just dormant until someone hits the
>> issue, making bisection way more complicated later.
>> Remember this one for example:
>>
>>   https://mail.gnu.org/archive/html/qemu-commits/2023-02/msg00030.html
>>
>> ?
>>
>> It would have been good to have a migration test for alpha in the CI, then
>> we could have prevented that bug from being merged.
> 
> IIUC, we run the migration-test  qtest for *every* softmmu target.
> 
> So, assuming you're referring to alpha guest, we were already
> exercising it.

Unless I missed something, you got that wrong. Have a look at 
tests/qtest/meson.build: The migration-test is only added for i386/x86_64, 
ppc64, aarch64 and s390x (since you need a special boot / guest code for 
this test).

> Anyway, I think a true functional test for migration is relevant
> to keep, as long as we make it clearly different from the qtest.
> A simple smoke test using a real Linux guest is different enough
> from our hand crafted boot sector that I think it is valuable
> coverage. Even better if we make the functional test add *lots*
> of different devices.

Agreed, that's a good idea. But for a start, I'd first like to convert the 
avocado test so that we finally can clean the tests/avocado folder.

  Thomas