| Message ID | 20241218131439.255841-6-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tests/functional: Convert tests with find_free_ports() | expand |
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()
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
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
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
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()
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