Add a new migration test to be ran as smoke test for each QEMU
target. This test will run only when the QEMU binary being used has no
KVM support, i.e. smoke tests run only on TCG.

Also modify the existing migration-test to run only when KVM is
present, i.e. the full set of tests will only run on a KVM host. To
still enable the full set to be ran anywhere for debug, ignore the
accel restriction when -m thorough is used.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/meson.build                   |  7 +++-
 tests/qtest/migration-test-smoke.c        | 39 +++++++++++++++++++++++
 tests/qtest/migration-test.c              | 12 +++++++
 tests/qtest/migration/compression-tests.c |  8 ++++-
 tests/qtest/migration/cpr-tests.c         |  8 ++++-
 tests/qtest/migration/file-tests.c        |  8 ++++-
 tests/qtest/migration/misc-tests.c        |  8 ++++-
 tests/qtest/migration/postcopy-tests.c    |  7 ++++
 tests/qtest/migration/precopy-tests.c     |  8 ++++-
 tests/qtest/migration/test-framework.h    |  8 +++++
 tests/qtest/migration/tls-tests.c         |  7 +++-
 11 files changed, 113 insertions(+), 7 deletions(-)
 create mode 100644 tests/qtest/migration-test-smoke.c

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 2b12a4d263..811117d264 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -5,6 +5,7 @@ slow_qtests = {
   'cdrom-test' : 610,
   'device-introspect-test' : 720,
   'ide-test' : 120,
+  'migration-test-smoke' : 480,
   'migration-test' : 480,
   'npcm7xx_pwm-test': 300,
   'npcm7xx_watchdog_timer-test': 120,
@@ -111,6 +112,7 @@ qtests_i386 = \
    'device-plug-test',
    'drive_del-test',
    'cpu-plug-test',
+   'migration-test-smoke',
    'migration-test',
   ]
 
@@ -185,7 +187,7 @@ qtests_ppc64 = \
   (slirp.found() ? ['pxe-test'] : []) +              \
   (config_all_devices.has_key('CONFIG_USB_UHCI') ? ['usb-hcd-uhci-test'] : []) +             \
   (config_all_devices.has_key('CONFIG_USB_XHCI_NEC') ? ['usb-hcd-xhci-test'] : []) +         \
-  qtests_pci + ['migration-test', 'cpu-plug-test', 'drive_del-test']
+  qtests_pci + ['migration-test-smoke', 'migration-test', 'cpu-plug-test', 'drive_del-test']
 
 qtests_sh4 = (config_all_devices.has_key('CONFIG_ISA_TESTDEV') ? ['endianness-test'] : [])
 qtests_sh4eb = (config_all_devices.has_key('CONFIG_ISA_TESTDEV') ? ['endianness-test'] : [])
@@ -257,6 +259,7 @@ qtests_aarch64 = \
   ['arm-cpu-features',
    'numa-test',
    'boot-serial-test',
+   'migration-test-smoke',
    'migration-test']
 
 qtests_s390x = \
@@ -266,6 +269,7 @@ qtests_s390x = \
    'device-plug-test',
    'virtio-ccw-test',
    'cpu-plug-test',
+   'migration-test-smoke',
    'migration-test']
 
 qtests_riscv32 = \
@@ -359,6 +363,7 @@ qtests = {
   'dbus-vmstate-test': files('migration/migration-qmp.c', 'migration/migration-util.c') + dbus_vmstate1,
   'erst-test': files('erst-test.c'),
   'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
+  'migration-test-smoke': migration_files + migration_tls_files,
   'migration-test': migration_files + migration_tls_files,
   'pxe-test': files('boot-sector.c'),
   'pnv-xive2-test': files('pnv-xive2-common.c', 'pnv-xive2-flush-sync.c'),
diff --git a/tests/qtest/migration-test-smoke.c b/tests/qtest/migration-test-smoke.c
new file mode 100644
index 0000000000..ff2d72881f
--- /dev/null
+++ b/tests/qtest/migration-test-smoke.c
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "migration/test-framework.h"
+#include "qemu/module.h"
+
+int main(int argc, char **argv)
+{
+    MigrationTestEnv *env;
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+    env = migration_get_env();
+    module_call_init(MODULE_INIT_QOM);
+
+    if (env->has_kvm) {
+        g_test_message(
+            "Smoke tests already run as part of the full suite on KVM hosts");
+        goto out;
+    }
+
+    migration_test_add_tls_smoke(env);
+    migration_test_add_compression_smoke(env);
+    migration_test_add_postcopy_smoke(env);
+    migration_test_add_file_smoke(env);
+    migration_test_add_precopy_smoke(env);
+    migration_test_add_cpr_smoke(env);
+    migration_test_add_misc_smoke(env);
+
+out:
+    ret = g_test_run();
+
+    g_assert_cmpint(ret, ==, 0);
+
+    ret = migration_env_clean(env);
+
+    return ret;
+}
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 4c8ea397ec..73cb0bdfe6 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -26,6 +26,17 @@ int main(int argc, char **argv)
     env = migration_get_env();
     module_call_init(MODULE_INIT_QOM);
 
+    /*
+     * Restrict the full set of tests to KVM hosts only. For tests
+     * that run in all platforms, see migration-test-smoke.c. Ignore
+     * the restriction if -m thorough was passed in the command line.
+     */
+    if (!g_test_thorough() && !env->has_kvm) {
+        g_test_message("Full test suite only runs on KVM hosts "
+                       "(override with -m thorough)");
+        goto out;
+    }
+
     migration_test_add_tls(env);
     migration_test_add_compression(env);
     migration_test_add_postcopy(env);
@@ -34,6 +45,7 @@ int main(int argc, char **argv)
     migration_test_add_cpr(env);
     migration_test_add_misc(env);
 
+out:
     ret = g_test_run();
 
     g_assert_cmpint(ret, ==, 0);
diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c
index 1b4c59338c..b48dc87239 100644
--- a/tests/qtest/migration/compression-tests.c
+++ b/tests/qtest/migration/compression-tests.c
@@ -205,9 +205,15 @@ static void test_multifd_tcp_zlib(void)
     test_precopy_common(&args);
 }
 
-void migration_test_add_compression(MigrationTestEnv *env)
+void migration_test_add_compression_smoke(MigrationTestEnv *env)
 {
     tmpfs = env->tmpfs;
+    /* TODO: add smoke tests */
+}
+
+void migration_test_add_compression(MigrationTestEnv *env)
+{
+    migration_test_add_compression_smoke(env);
 
 #ifdef CONFIG_ZSTD
     migration_test_add("/migration/multifd/tcp/plain/zstd",
diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
index 92bd42e61a..4fe6eefe86 100644
--- a/tests/qtest/migration/cpr-tests.c
+++ b/tests/qtest/migration/cpr-tests.c
@@ -44,9 +44,15 @@ static void test_mode_reboot(void)
     test_file_common(&args, true);
 }
 
-void migration_test_add_cpr(MigrationTestEnv *env)
+void migration_test_add_cpr_smoke(MigrationTestEnv *env)
 {
     tmpfs = env->tmpfs;
+    /* TODO: add smoke tests */
+}
+
+void migration_test_add_cpr(MigrationTestEnv *env)
+{
+    migration_test_add_cpr_smoke(env);
 
     /*
      * Our CI system has problems with shared memory.
diff --git a/tests/qtest/migration/file-tests.c b/tests/qtest/migration/file-tests.c
index 90b0386f58..10a5cb648d 100644
--- a/tests/qtest/migration/file-tests.c
+++ b/tests/qtest/migration/file-tests.c
@@ -294,9 +294,15 @@ static void test_multifd_file_mapped_ram_fdset_dio(void)
 }
 #endif /* !_WIN32 */
 
-void migration_test_add_file(MigrationTestEnv *env)
+void migration_test_add_file_smoke(MigrationTestEnv *env)
 {
     tmpfs = env->tmpfs;
+    /* TODO: add smoke tests */
+}
+
+void migration_test_add_file(MigrationTestEnv *env)
+{
+    migration_test_add_file_smoke(env);
 
     migration_test_add("/migration/precopy/file",
                        test_precopy_file);
diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
index 6f2bc5cca1..480fbda1c9 100644
--- a/tests/qtest/migration/misc-tests.c
+++ b/tests/qtest/migration/misc-tests.c
@@ -250,9 +250,15 @@ static void test_validate_uri_channels_none_set(void)
     do_test_validate_uri_channel(&args);
 }
 
-void migration_test_add_misc(MigrationTestEnv *env)
+void migration_test_add_misc_smoke(MigrationTestEnv *env)
 {
     tmpfs = env->tmpfs;
+    /* TODO: add smoke tests */
+}
+
+void migration_test_add_misc(MigrationTestEnv *env)
+{
+    migration_test_add_misc_smoke(env);
 
     migration_test_add("/migration/bad_dest", test_baddest);
 #ifndef _WIN32
diff --git a/tests/qtest/migration/postcopy-tests.c b/tests/qtest/migration/postcopy-tests.c
index 9e2032bbf3..90d2d0820c 100644
--- a/tests/qtest/migration/postcopy-tests.c
+++ b/tests/qtest/migration/postcopy-tests.c
@@ -79,8 +79,15 @@ static void test_postcopy_preempt_recovery(void)
     test_postcopy_recovery_common(&args);
 }
 
+void migration_test_add_postcopy_smoke(MigrationTestEnv *env)
+{
+    /* TODO: add smoke tests */
+}
+
 void migration_test_add_postcopy(MigrationTestEnv *env)
 {
+    migration_test_add_postcopy_smoke(env);
+
     if (env->has_uffd) {
         migration_test_add("/migration/postcopy/plain", test_postcopy);
         migration_test_add("/migration/postcopy/recovery/plain",
diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
index c7c802f812..393c7e226a 100644
--- a/tests/qtest/migration/precopy-tests.c
+++ b/tests/qtest/migration/precopy-tests.c
@@ -946,9 +946,15 @@ static void test_dirty_limit(void)
     migrate_end(from, to, true);
 }
 
-void migration_test_add_precopy(MigrationTestEnv *env)
+void migration_test_add_precopy_smoke(MigrationTestEnv *env)
 {
     tmpfs = env->tmpfs;
+    /* TODO: add smoke tests */
+}
+
+void migration_test_add_precopy(MigrationTestEnv *env)
+{
+    migration_test_add_precopy_smoke(env);
 
     if (env->is_x86) {
         migration_test_add("/migration/precopy/unix/suspend/live",
diff --git a/tests/qtest/migration/test-framework.h b/tests/qtest/migration/test-framework.h
index 207a11edb9..10cc4e524c 100644
--- a/tests/qtest/migration/test-framework.h
+++ b/tests/qtest/migration/test-framework.h
@@ -215,14 +215,22 @@ QTestMigrationState *get_src(void);
 
 #ifdef CONFIG_GNUTLS
 void migration_test_add_tls(MigrationTestEnv *env);
+void migration_test_add_tls_smoke(MigrationTestEnv *env);
 #else
 static inline void migration_test_add_tls(MigrationTestEnv *env) {};
+static inline void migration_test_add_tls_smoke(MigrationTestEnv *env) {}
 #endif
 void migration_test_add_compression(MigrationTestEnv *env);
+void migration_test_add_compression_smoke(MigrationTestEnv *env);
 void migration_test_add_postcopy(MigrationTestEnv *env);
+void migration_test_add_postcopy_smoke(MigrationTestEnv *env);
 void migration_test_add_file(MigrationTestEnv *env);
+void migration_test_add_file_smoke(MigrationTestEnv *env);
 void migration_test_add_precopy(MigrationTestEnv *env);
+void migration_test_add_precopy_smoke(MigrationTestEnv *env);
 void migration_test_add_cpr(MigrationTestEnv *env);
+void migration_test_add_cpr_smoke(MigrationTestEnv *env);
 void migration_test_add_misc(MigrationTestEnv *env);
+void migration_test_add_misc_smoke(MigrationTestEnv *env);
 
 #endif /* TEST_FRAMEWORK_H */
diff --git a/tests/qtest/migration/tls-tests.c b/tests/qtest/migration/tls-tests.c
index 7609183474..264b54f352 100644
--- a/tests/qtest/migration/tls-tests.c
+++ b/tests/qtest/migration/tls-tests.c
@@ -722,10 +722,15 @@ static void test_multifd_tcp_tls_x509_reject_anon_client(void)
 }
 #endif /* CONFIG_TASN1 */
 
-void migration_test_add_tls(MigrationTestEnv *env)
+void migration_test_add_tls_smoke(MigrationTestEnv *env)
 {
     tmpfs = env->tmpfs;
+    /* TODO: add smoke tests */
+}
 
+void migration_test_add_tls(MigrationTestEnv *env)
+{
+    migration_test_add_tls_smoke(env);
     migration_test_add("/migration/precopy/unix/tls/psk",
                        test_precopy_unix_tls_psk);
 
-- 
2.35.3

On Wed, Nov 13, 2024 at 04:46:27PM -0300, Fabiano Rosas wrote:
> diff --git a/tests/qtest/migration-test-smoke.c b/tests/qtest/migration-test-smoke.c
> new file mode 100644
> index 0000000000..ff2d72881f
> --- /dev/null
> +++ b/tests/qtest/migration-test-smoke.c
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "migration/test-framework.h"
> +#include "qemu/module.h"
> +
> +int main(int argc, char **argv)
> +{
> +    MigrationTestEnv *env;
> +    int ret;
> +
> +    g_test_init(&argc, &argv, NULL);
> +    env = migration_get_env();
> +    module_call_init(MODULE_INIT_QOM);
> +
> +    if (env->has_kvm) {
> +        g_test_message(
> +            "Smoke tests already run as part of the full suite on KVM hosts");
> +        goto out;
> +    }

So the "smoke" here is almost "tcg".. and if i want to run a smoke test on
a kvm-enabled host, it's noop.. which isn't easy to understand why.

If to rethink our goal, we have two requirements:

  (1) We want to categorize migration tests, so some are quick, some are
      slow, some might be flacky.  Maybe more, but it's about putting one
      test into only one bucket, and there're >1 buckets.

  (2) We want to run only a small portion of tests on tcg, more tests on
      kvm.

Ideally, we don't need two separate main test files, do we?

I mean, we can do (1) with the existing migration-test.c, with the help of
either gtest's "-m" or something we invent.  The only unfortunate part is
qtest only have quick/slow, afaiu the "thorough" mode is the same as
"slow".. while we don't yet have real "perf" tests.  It means we only have
two buckets if we want to reuse gtest's "-m".

Maybe it's enough?  If not, we can implement >2 categories in whatever
form, either custom argv/argc cmdline, or env variable.

Then, if we always categorize one test (let me try to not reuse glib's
terms to be clear) into any of: FAST|NORMAL|SLOW|..., then we have a single
migration-test that have different level of tests.  We can invoke
"migration-test --mode FAST" if kvm is not supported, and invoke the same
"migration-test --mode SLOW" if kvm is supported.

Would this be nicer?  At least we can still run a pretty fast smoke / FAST
test even on kvm. Basically, untangle accel v.s. "test category".

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Wed, Nov 13, 2024 at 04:46:27PM -0300, Fabiano Rosas wrote:
>> diff --git a/tests/qtest/migration-test-smoke.c b/tests/qtest/migration-test-smoke.c
>> new file mode 100644
>> index 0000000000..ff2d72881f
>> --- /dev/null
>> +++ b/tests/qtest/migration-test-smoke.c
>> @@ -0,0 +1,39 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
>> +#include "qemu/osdep.h"
>> +#include "libqtest.h"
>> +#include "migration/test-framework.h"
>> +#include "qemu/module.h"
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    MigrationTestEnv *env;
>> +    int ret;
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +    env = migration_get_env();
>> +    module_call_init(MODULE_INIT_QOM);
>> +
>> +    if (env->has_kvm) {
>> +        g_test_message(
>> +            "Smoke tests already run as part of the full suite on KVM hosts");
>> +        goto out;
>> +    }
>
> So the "smoke" here is almost "tcg".. and if i want to run a smoke test on
> a kvm-enabled host, it's noop.. which isn't easy to understand why.
>
> If to rethink our goal, we have two requirements:
>
>   (1) We want to categorize migration tests, so some are quick, some are
>       slow, some might be flacky.  Maybe more, but it's about putting one
>       test into only one bucket, and there're >1 buckets.

It's true that the smoke test should never have slow or flaky tests, but
we can't use this categorization for anything else. IOW, what you
describe here is not a goal. If a test is found to be slow we put it
under slow and it will only run with -m slow/thorough, that's it. We can
just ignore this.

>
>   (2) We want to run only a small portion of tests on tcg, more tests on
>       kvm.

Yes. Guests are fast with KVM and slow with TCG (generally) and the KVM
hosts are the ones where it's actually important to ensure all migration
features work OK. Non-KVM will only care about save/restore of
snapshots. Therefore we don't need to have all tests running with TCG,
only the smoke set.

And "smoke set" is arbitrary, not tied to speed, but of course no slow
tests please (which already happens because we don't pass -m slow to
migration-test-smoke).

>
> Ideally, we don't need two separate main test files, do we?
>
> I mean, we can do (1) with the existing migration-test.c, with the help of
> either gtest's "-m" or something we invent.  The only unfortunate part is
> qtest only have quick/slow, afaiu the "thorough" mode is the same as
> "slow".. while we don't yet have real "perf" tests.  It means we only have
> two buckets if we want to reuse gtest's "-m".
>
> Maybe it's enough?  If not, we can implement >2 categories in whatever
> form, either custom argv/argc cmdline, or env variable.
>
> Then, if we always categorize one test (let me try to not reuse glib's
> terms to be clear) into any of: FAST|NORMAL|SLOW|..., then we have a single

It's either normal or slow. Because we only know a test is only after it
bothers us.

> migration-test that have different level of tests.  We can invoke
> "migration-test --mode FAST" if kvm is not supported, and invoke the same
> "migration-test --mode SLOW" if kvm is supported.

This is messy due to how qtest/meson.build works. Having two tests is
the clean change. Otherwise we'll have to add "if migration-test" or
create artificial test names to be able to restrict the arguments that
are passed to the test per arch.

I also *think* we cannot have anything extra in argv because gtester
expects to be able to parse those.

>
> Would this be nicer?  At least we can still run a pretty fast smoke / FAST
> test even on kvm. Basically, untangle accel v.s. "test category".

We could just remove the restriction from migration-test-smoke if that's
an issue.

On Wed, Dec 18, 2024 at 03:13:08PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Nov 13, 2024 at 04:46:27PM -0300, Fabiano Rosas wrote:
> >> diff --git a/tests/qtest/migration-test-smoke.c b/tests/qtest/migration-test-smoke.c
> >> new file mode 100644
> >> index 0000000000..ff2d72881f
> >> --- /dev/null
> >> +++ b/tests/qtest/migration-test-smoke.c
> >> @@ -0,0 +1,39 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "libqtest.h"
> >> +#include "migration/test-framework.h"
> >> +#include "qemu/module.h"
> >> +
> >> +int main(int argc, char **argv)
> >> +{
> >> +    MigrationTestEnv *env;
> >> +    int ret;
> >> +
> >> +    g_test_init(&argc, &argv, NULL);
> >> +    env = migration_get_env();
> >> +    module_call_init(MODULE_INIT_QOM);
> >> +
> >> +    if (env->has_kvm) {
> >> +        g_test_message(
> >> +            "Smoke tests already run as part of the full suite on KVM hosts");
> >> +        goto out;
> >> +    }
> >
> > So the "smoke" here is almost "tcg".. and if i want to run a smoke test on
> > a kvm-enabled host, it's noop.. which isn't easy to understand why.
> >
> > If to rethink our goal, we have two requirements:
> >
> >   (1) We want to categorize migration tests, so some are quick, some are
> >       slow, some might be flacky.  Maybe more, but it's about putting one
> >       test into only one bucket, and there're >1 buckets.
> 
> It's true that the smoke test should never have slow or flaky tests, but
> we can't use this categorization for anything else. IOW, what you
> describe here is not a goal. If a test is found to be slow we put it
> under slow and it will only run with -m slow/thorough, that's it. We can
> just ignore this.

I could have missed something, but I still think it's the same issue.  In
general, I think we want to provide different levels of tests, like:

  - Level 1: the minimum set of tests (aka, the "smoke" idea here)
  - Level 2: normal set of tests (aka, whatever we used to run by default)
  - Level 3: slow tests (aka, only ran with '-m slow' before)
  - Level 4: flaky tests (aka, only ran when QEMU_TEST_FLAKY_TESTS set)

Then we want to run level1 test only in tcg, and level1+2 in kvm.  We can
only trigger level 1-3 or level 1-4 in manual tests.

We used to have different way to provide the level idea, now I think we can
consider provide that level in migration-test in one shot.  Obviously it's
more than quick/slow so I don't think we can reuse "-m", but we can add our
own test level "--level" parameter, so --level N means run all tests lower
than level N, for example.

> 
> >
> >   (2) We want to run only a small portion of tests on tcg, more tests on
> >       kvm.
> 
> Yes. Guests are fast with KVM and slow with TCG (generally) and the KVM
> hosts are the ones where it's actually important to ensure all migration
> features work OK. Non-KVM will only care about save/restore of
> snapshots. Therefore we don't need to have all tests running with TCG,
> only the smoke set.
> 
> And "smoke set" is arbitrary, not tied to speed, but of course no slow
> tests please (which already happens because we don't pass -m slow to
> migration-test-smoke).
> 
> >
> > Ideally, we don't need two separate main test files, do we?
> >
> > I mean, we can do (1) with the existing migration-test.c, with the help of
> > either gtest's "-m" or something we invent.  The only unfortunate part is
> > qtest only have quick/slow, afaiu the "thorough" mode is the same as
> > "slow".. while we don't yet have real "perf" tests.  It means we only have
> > two buckets if we want to reuse gtest's "-m".
> >
> > Maybe it's enough?  If not, we can implement >2 categories in whatever
> > form, either custom argv/argc cmdline, or env variable.
> >
> > Then, if we always categorize one test (let me try to not reuse glib's
> > terms to be clear) into any of: FAST|NORMAL|SLOW|..., then we have a single
> 
> It's either normal or slow. Because we only know a test is only after it
> bothers us.

So I wonder if we can provide four levels, as above.. and define it for
each test in migration-test.

> 
> > migration-test that have different level of tests.  We can invoke
> > "migration-test --mode FAST" if kvm is not supported, and invoke the same
> > "migration-test --mode SLOW" if kvm is supported.
> 
> This is messy due to how qtest/meson.build works. Having two tests is
> the clean change. Otherwise we'll have to add "if migration-test" or
> create artificial test names to be able to restrict the arguments that
> are passed to the test per arch.

Indeed it'll need a few extra lines in meson, but it doesn't look too bad,
but yeah if anyone is not happy with it we can rethink.  I just want to
know whether it's still acceptable.

I tried to code it up, it looks like this:

====8<====
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index c5a70021c5..5bec33b627 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -392,6 +392,12 @@ if dbus_display
   qtests += {'dbus-display-test': [dbus_display1, gio]}
 endif
 
+if run_command('test', '-e', '/dev/kvm', check: false).returncode() == 0
+  has_kvm = true
+else
+  has_kvm =false
+endif
+
 qtest_executables = {}
 foreach dir : target_dirs
   if not dir.endswith('-softmmu')
@@ -434,11 +440,21 @@ foreach dir : target_dirs
         test: executable(test, src, dependencies: deps)
       }
     endif
+    test_args = ['--tap', '-k']
+    if test == 'migration-test'
+      if host_os == 'linux' and cpu == target_base and has_kvm
+        # Only run full migration test if host kvm supported
+        test_args += ['-m', 'thorough']
+      else
+        test_args += ['-m', 'quick']
+      endif
+    endif
+
     test('qtest-@0@/@1@'.format(target_base, test),
          qtest_executables[test],
          depends: [test_deps, qtest_emulator, emulator_modules],
          env: qtest_env,
-         args: ['--tap', '-k'],
+         args: test_args,
          protocol: 'tap',
          timeout: slow_qtests.get(test, 60),
          priority: slow_qtests.get(test, 60),
====8<====

I still used "-m" but just to show the idea.  I also wonder whether other
tests would have similar demands.. otherwise are we destined to not be able
to use qtest cmdline at all as long as we use meson?

> 
> I also *think* we cannot have anything extra in argv because gtester
> expects to be able to parse those.

We can definitely hijack argv/argc before passing it over to glib.

> 
> >
> > Would this be nicer?  At least we can still run a pretty fast smoke / FAST
> > test even on kvm. Basically, untangle accel v.s. "test category".
> 
> We could just remove the restriction from migration-test-smoke if that's
> an issue.

Not the only issue, but the idea of it. In general, IMHO it'll be good we
don't attach host info to a test program.

IOW, I want to keep the test in a way so that we'll be able to run whatever
level of test on whatever host, at least when when I run migration-test
manually.

So e.g. I also want to be able to run full set of tests on TCG too whenever
needed.  So I still feel like we mangled these two issues together which
might be unnecessarily.

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Wed, Dec 18, 2024 at 03:13:08PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Wed, Nov 13, 2024 at 04:46:27PM -0300, Fabiano Rosas wrote:
>> >> diff --git a/tests/qtest/migration-test-smoke.c b/tests/qtest/migration-test-smoke.c
>> >> new file mode 100644
>> >> index 0000000000..ff2d72881f
>> >> --- /dev/null
>> >> +++ b/tests/qtest/migration-test-smoke.c
>> >> @@ -0,0 +1,39 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> >> +
>> >> +#include "qemu/osdep.h"
>> >> +#include "libqtest.h"
>> >> +#include "migration/test-framework.h"
>> >> +#include "qemu/module.h"
>> >> +
>> >> +int main(int argc, char **argv)
>> >> +{
>> >> +    MigrationTestEnv *env;
>> >> +    int ret;
>> >> +
>> >> +    g_test_init(&argc, &argv, NULL);
>> >> +    env = migration_get_env();
>> >> +    module_call_init(MODULE_INIT_QOM);
>> >> +
>> >> +    if (env->has_kvm) {
>> >> +        g_test_message(
>> >> +            "Smoke tests already run as part of the full suite on KVM hosts");
>> >> +        goto out;
>> >> +    }
>> >
>> > So the "smoke" here is almost "tcg".. and if i want to run a smoke test on
>> > a kvm-enabled host, it's noop.. which isn't easy to understand why.
>> >
>> > If to rethink our goal, we have two requirements:
>> >
>> >   (1) We want to categorize migration tests, so some are quick, some are
>> >       slow, some might be flacky.  Maybe more, but it's about putting one
>> >       test into only one bucket, and there're >1 buckets.
>> 
>> It's true that the smoke test should never have slow or flaky tests, but
>> we can't use this categorization for anything else. IOW, what you
>> describe here is not a goal. If a test is found to be slow we put it
>> under slow and it will only run with -m slow/thorough, that's it. We can
>> just ignore this.
>
> I could have missed something, but I still think it's the same issue.  In
> general, I think we want to provide different levels of tests, like:
>
>   - Level 1: the minimum set of tests (aka, the "smoke" idea here)
>   - Level 2: normal set of tests (aka, whatever we used to run by default)
>   - Level 3: slow tests (aka, only ran with '-m slow' before)

How are you going to make this one work? 'migration-test --level 3'
vs. 'migration-test --level 3 -m slow' vs. 'migration-test -m slow'

The only way I can see is to not have a level 3 at all and just use -m
slow.

>   - Level 4: flaky tests (aka, only ran when QEMU_TEST_FLAKY_TESTS set)
>
> Then we want to run level1 test only in tcg, and level1+2 in kvm.  We can
> only trigger level 1-3 or level 1-4 in manual tests.
>
> We used to have different way to provide the level idea, now I think we can
> consider provide that level in migration-test in one shot.  Obviously it's
> more than quick/slow so I don't think we can reuse "-m", but we can add our
>
> own test level "--level" parameter, so --level N means run all tests lower
> than level N, for example.
>

I'm not sure that works semantically for level 4. Because the reason one
runs flaky tests is different from the reason one runs the other
tests. So we probably don't want to run a bunch of tests just to get to
the broken ones.

But we don't need to spend too much time on this. I hate the idea of
flaky tests anyway. Whatever we choose they'll just sit there doing
nothing.

>> 
>> >
>> >   (2) We want to run only a small portion of tests on tcg, more tests on
>> >       kvm.
>> 
>> Yes. Guests are fast with KVM and slow with TCG (generally) and the KVM
>> hosts are the ones where it's actually important to ensure all migration
>> features work OK. Non-KVM will only care about save/restore of
>> snapshots. Therefore we don't need to have all tests running with TCG,
>> only the smoke set.
>> 
>> And "smoke set" is arbitrary, not tied to speed, but of course no slow
>> tests please (which already happens because we don't pass -m slow to
>> migration-test-smoke).
>> 
>> >
>> > Ideally, we don't need two separate main test files, do we?
>> >
>> > I mean, we can do (1) with the existing migration-test.c, with the help of
>> > either gtest's "-m" or something we invent.  The only unfortunate part is
>> > qtest only have quick/slow, afaiu the "thorough" mode is the same as
>> > "slow".. while we don't yet have real "perf" tests.  It means we only have
>> > two buckets if we want to reuse gtest's "-m".
>> >
>> > Maybe it's enough?  If not, we can implement >2 categories in whatever
>> > form, either custom argv/argc cmdline, or env variable.
>> >
>> > Then, if we always categorize one test (let me try to not reuse glib's
>> > terms to be clear) into any of: FAST|NORMAL|SLOW|..., then we have a single
>> 
>> It's either normal or slow. Because we only know a test is only after it
>> bothers us.
>
> So I wonder if we can provide four levels, as above.. and define it for
> each test in migration-test.
>
>> 
>> > migration-test that have different level of tests.  We can invoke
>> > "migration-test --mode FAST" if kvm is not supported, and invoke the same
>> > "migration-test --mode SLOW" if kvm is supported.
>> 
>> This is messy due to how qtest/meson.build works. Having two tests is
>> the clean change. Otherwise we'll have to add "if migration-test" or
>> create artificial test names to be able to restrict the arguments that
>> are passed to the test per arch.
>
> Indeed it'll need a few extra lines in meson, but it doesn't look too bad,
> but yeah if anyone is not happy with it we can rethink.  I just want to
> know whether it's still acceptable.
>
> I tried to code it up, it looks like this:
>
> ====8<====
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index c5a70021c5..5bec33b627 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -392,6 +392,12 @@ if dbus_display
>    qtests += {'dbus-display-test': [dbus_display1, gio]}
>  endif
>  
> +if run_command('test', '-e', '/dev/kvm', check: false).returncode() == 0
> +  has_kvm = true
> +else
> +  has_kvm =false
> +endif

This is not right. Checking /dev/kvm at configure time doesn't ensure it
will be present at test runtime. It also doesn't account for builds with
CONFIG_KVM=n or builds without both KVM and TCG. This needs to be done
inside the test.

I think the best we can do is have a qtest_migration_level_<ARCH> and
set it for every arch.

Also note that we must keep plain 'migration-test' invocation working
because of the compat test.

> +
>  qtest_executables = {}
>  foreach dir : target_dirs
>    if not dir.endswith('-softmmu')
> @@ -434,11 +440,21 @@ foreach dir : target_dirs
>          test: executable(test, src, dependencies: deps)
>        }
>      endif
> +    test_args = ['--tap', '-k']
> +    if test == 'migration-test'
> +      if host_os == 'linux' and cpu == target_base and has_kvm
> +        # Only run full migration test if host kvm supported
> +        test_args += ['-m', 'thorough']
> +      else
> +        test_args += ['-m', 'quick']
> +      endif
> +    endif
> +
>      test('qtest-@0@/@1@'.format(target_base, test),
>           qtest_executables[test],
>           depends: [test_deps, qtest_emulator, emulator_modules],
>           env: qtest_env,
> -         args: ['--tap', '-k'],
> +         args: test_args,
>           protocol: 'tap',
>           timeout: slow_qtests.get(test, 60),
>           priority: slow_qtests.get(test, 60),
> ====8<====
>
> I still used "-m" but just to show the idea.  I also wonder whether other
> tests would have similar demands.. otherwise are we destined to not be able
> to use qtest cmdline at all as long as we use meson?
>
>> 
>> I also *think* we cannot have anything extra in argv because gtester
>> expects to be able to parse those.
>
> We can definitely hijack argv/argc before passing it over to glib.
>
>> 
>> >
>> > Would this be nicer?  At least we can still run a pretty fast smoke / FAST
>> > test even on kvm. Basically, untangle accel v.s. "test category".
>> 
>> We could just remove the restriction from migration-test-smoke if that's
>> an issue.
>
> Not the only issue, but the idea of it. In general, IMHO it'll be good we
> don't attach host info to a test program.
>
> IOW, I want to keep the test in a way so that we'll be able to run whatever
> level of test on whatever host, at least when when I run migration-test
> manually.
>
> So e.g. I also want to be able to run full set of tests on TCG too whenever
> needed.  So I still feel like we mangled these two issues together which
> might be unnecessarily.

On Wed, Dec 18, 2024 at 06:08:01PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Dec 18, 2024 at 03:13:08PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Wed, Nov 13, 2024 at 04:46:27PM -0300, Fabiano Rosas wrote:
> >> >> diff --git a/tests/qtest/migration-test-smoke.c b/tests/qtest/migration-test-smoke.c
> >> >> new file mode 100644
> >> >> index 0000000000..ff2d72881f
> >> >> --- /dev/null
> >> >> +++ b/tests/qtest/migration-test-smoke.c
> >> >> @@ -0,0 +1,39 @@
> >> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> >> +
> >> >> +#include "qemu/osdep.h"
> >> >> +#include "libqtest.h"
> >> >> +#include "migration/test-framework.h"
> >> >> +#include "qemu/module.h"
> >> >> +
> >> >> +int main(int argc, char **argv)
> >> >> +{
> >> >> +    MigrationTestEnv *env;
> >> >> +    int ret;
> >> >> +
> >> >> +    g_test_init(&argc, &argv, NULL);
> >> >> +    env = migration_get_env();
> >> >> +    module_call_init(MODULE_INIT_QOM);
> >> >> +
> >> >> +    if (env->has_kvm) {
> >> >> +        g_test_message(
> >> >> +            "Smoke tests already run as part of the full suite on KVM hosts");
> >> >> +        goto out;
> >> >> +    }
> >> >
> >> > So the "smoke" here is almost "tcg".. and if i want to run a smoke test on
> >> > a kvm-enabled host, it's noop.. which isn't easy to understand why.
> >> >
> >> > If to rethink our goal, we have two requirements:
> >> >
> >> >   (1) We want to categorize migration tests, so some are quick, some are
> >> >       slow, some might be flacky.  Maybe more, but it's about putting one
> >> >       test into only one bucket, and there're >1 buckets.
> >> 
> >> It's true that the smoke test should never have slow or flaky tests, but
> >> we can't use this categorization for anything else. IOW, what you
> >> describe here is not a goal. If a test is found to be slow we put it
> >> under slow and it will only run with -m slow/thorough, that's it. We can
> >> just ignore this.
> >
> > I could have missed something, but I still think it's the same issue.  In
> > general, I think we want to provide different levels of tests, like:
> >
> >   - Level 1: the minimum set of tests (aka, the "smoke" idea here)
> >   - Level 2: normal set of tests (aka, whatever we used to run by default)
> >   - Level 3: slow tests (aka, only ran with '-m slow' before)
> 
> How are you going to make this one work? 'migration-test --level 3'
> vs. 'migration-test --level 3 -m slow' vs. 'migration-test -m slow'
> 
> The only way I can see is to not have a level 3 at all and just use -m
> slow.

I meant remove "-m" and remove QEMU_TEST_FLAKY_TESTS, instead replacing all
of them using --level.  Then migration-test ignores '-m' in the future
because it's simply not enough for us.

> 
> >   - Level 4: flaky tests (aka, only ran when QEMU_TEST_FLAKY_TESTS set)
> >
> > Then we want to run level1 test only in tcg, and level1+2 in kvm.  We can
> > only trigger level 1-3 or level 1-4 in manual tests.
> >
> > We used to have different way to provide the level idea, now I think we can
> > consider provide that level in migration-test in one shot.  Obviously it's
> > more than quick/slow so I don't think we can reuse "-m", but we can add our
> >
> > own test level "--level" parameter, so --level N means run all tests lower
> > than level N, for example.
> >
> 
> I'm not sure that works semantically for level 4. Because the reason one
> runs flaky tests is different from the reason one runs the other
> tests. So we probably don't want to run a bunch of tests just to get to
> the broken ones.
> 
> But we don't need to spend too much time on this. I hate the idea of
> flaky tests anyway. Whatever we choose they'll just sit there doing
> nothing.

Yes how to treat flaky tests isn't important yet.  If we don't care about
QEMU_TEST_FLAKY_TESTS then we make it three levels.  The idea is the same.

> 
> >> 
> >> >
> >> >   (2) We want to run only a small portion of tests on tcg, more tests on
> >> >       kvm.
> >> 
> >> Yes. Guests are fast with KVM and slow with TCG (generally) and the KVM
> >> hosts are the ones where it's actually important to ensure all migration
> >> features work OK. Non-KVM will only care about save/restore of
> >> snapshots. Therefore we don't need to have all tests running with TCG,
> >> only the smoke set.
> >> 
> >> And "smoke set" is arbitrary, not tied to speed, but of course no slow
> >> tests please (which already happens because we don't pass -m slow to
> >> migration-test-smoke).
> >> 
> >> >
> >> > Ideally, we don't need two separate main test files, do we?
> >> >
> >> > I mean, we can do (1) with the existing migration-test.c, with the help of
> >> > either gtest's "-m" or something we invent.  The only unfortunate part is
> >> > qtest only have quick/slow, afaiu the "thorough" mode is the same as
> >> > "slow".. while we don't yet have real "perf" tests.  It means we only have
> >> > two buckets if we want to reuse gtest's "-m".
> >> >
> >> > Maybe it's enough?  If not, we can implement >2 categories in whatever
> >> > form, either custom argv/argc cmdline, or env variable.
> >> >
> >> > Then, if we always categorize one test (let me try to not reuse glib's
> >> > terms to be clear) into any of: FAST|NORMAL|SLOW|..., then we have a single
> >> 
> >> It's either normal or slow. Because we only know a test is only after it
> >> bothers us.
> >
> > So I wonder if we can provide four levels, as above.. and define it for
> > each test in migration-test.
> >
> >> 
> >> > migration-test that have different level of tests.  We can invoke
> >> > "migration-test --mode FAST" if kvm is not supported, and invoke the same
> >> > "migration-test --mode SLOW" if kvm is supported.
> >> 
> >> This is messy due to how qtest/meson.build works. Having two tests is
> >> the clean change. Otherwise we'll have to add "if migration-test" or
> >> create artificial test names to be able to restrict the arguments that
> >> are passed to the test per arch.
> >
> > Indeed it'll need a few extra lines in meson, but it doesn't look too bad,
> > but yeah if anyone is not happy with it we can rethink.  I just want to
> > know whether it's still acceptable.
> >
> > I tried to code it up, it looks like this:
> >
> > ====8<====
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > index c5a70021c5..5bec33b627 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -392,6 +392,12 @@ if dbus_display
> >    qtests += {'dbus-display-test': [dbus_display1, gio]}
> >  endif
> >  
> > +if run_command('test', '-e', '/dev/kvm', check: false).returncode() == 0
> > +  has_kvm = true
> > +else
> > +  has_kvm =false
> > +endif
> 
> This is not right. Checking /dev/kvm at configure time doesn't ensure it
> will be present at test runtime. It also doesn't account for builds with

Why the test runtime would be a different host versus whoever setup the
meson build?

> CONFIG_KVM=n or builds without both KVM and TCG. This needs to be done
> inside the test.

This is true, but IIUC that's not a blocker, as we can use (btw, I found
fs.exists() a better alternative than my previous hack):

if fs.exists('/dev/kvm') and 'CONFIG_KVM' in config_all_accel
  has_kvm = true
else
  has_kvm = false
endif

> 
> I think the best we can do is have a qtest_migration_level_<ARCH> and
> set it for every arch.
> 
> Also note that we must keep plain 'migration-test' invocation working
> because of the compat test.

We won't break it if we only switch to levels, right?

Btw, I also don't know why we need to.  IIRC the compat test runs the test
in previous release (but only feeds the new QEMU binary to the old
migration-test)?  I think that's one reason why we decided to use the old
migration-test (so we won't have new tests ran on compat tests, which is a
loss), just to avoid any change in migration-test will break the compat
test.. so I assume that should be fine regardless..

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Wed, Dec 18, 2024 at 06:08:01PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Wed, Dec 18, 2024 at 03:13:08PM -0300, Fabiano Rosas wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > On Wed, Nov 13, 2024 at 04:46:27PM -0300, Fabiano Rosas wrote:
>> >> >> diff --git a/tests/qtest/migration-test-smoke.c b/tests/qtest/migration-test-smoke.c
>> >> >> new file mode 100644
>> >> >> index 0000000000..ff2d72881f
>> >> >> --- /dev/null
>> >> >> +++ b/tests/qtest/migration-test-smoke.c
>> >> >> @@ -0,0 +1,39 @@
>> >> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> >> >> +
>> >> >> +#include "qemu/osdep.h"
>> >> >> +#include "libqtest.h"
>> >> >> +#include "migration/test-framework.h"
>> >> >> +#include "qemu/module.h"
>> >> >> +
>> >> >> +int main(int argc, char **argv)
>> >> >> +{
>> >> >> +    MigrationTestEnv *env;
>> >> >> +    int ret;
>> >> >> +
>> >> >> +    g_test_init(&argc, &argv, NULL);
>> >> >> +    env = migration_get_env();
>> >> >> +    module_call_init(MODULE_INIT_QOM);
>> >> >> +
>> >> >> +    if (env->has_kvm) {
>> >> >> +        g_test_message(
>> >> >> +            "Smoke tests already run as part of the full suite on KVM hosts");
>> >> >> +        goto out;
>> >> >> +    }
>> >> >
>> >> > So the "smoke" here is almost "tcg".. and if i want to run a smoke test on
>> >> > a kvm-enabled host, it's noop.. which isn't easy to understand why.
>> >> >
>> >> > If to rethink our goal, we have two requirements:
>> >> >
>> >> >   (1) We want to categorize migration tests, so some are quick, some are
>> >> >       slow, some might be flacky.  Maybe more, but it's about putting one
>> >> >       test into only one bucket, and there're >1 buckets.
>> >> 
>> >> It's true that the smoke test should never have slow or flaky tests, but
>> >> we can't use this categorization for anything else. IOW, what you
>> >> describe here is not a goal. If a test is found to be slow we put it
>> >> under slow and it will only run with -m slow/thorough, that's it. We can
>> >> just ignore this.
>> >
>> > I could have missed something, but I still think it's the same issue.  In
>> > general, I think we want to provide different levels of tests, like:
>> >
>> >   - Level 1: the minimum set of tests (aka, the "smoke" idea here)
>> >   - Level 2: normal set of tests (aka, whatever we used to run by default)
>> >   - Level 3: slow tests (aka, only ran with '-m slow' before)
>> 
>> How are you going to make this one work? 'migration-test --level 3'
>> vs. 'migration-test --level 3 -m slow' vs. 'migration-test -m slow'
>> 
>> The only way I can see is to not have a level 3 at all and just use -m
>> slow.
>
> I meant remove "-m" and remove QEMU_TEST_FLAKY_TESTS, instead replacing all
> of them using --level.  Then migration-test ignores '-m' in the future
> because it's simply not enough for us.
>

Even if we remove -m, it will still be passed in when people run make
SPEED=slow check and people who know about glib/qtests will continue to
try to use it.

Ignoring -m and FLAKY will create a diversion from the rest of qtest and
QEMU tests in general. I think the best approach is to drop levels 3 and
4 from this proposal and just ignore those options altogether.

If we're going to have a single binary as you suggest, then there's no
harm still using -m slow and FLAKY as usual. We call level 1: "smoke
tests", level 2: "full set" and we put in some code to prevent
registering a smoke test as slow or flaky and that's it.

... and of course, if there's only two levels, then we only need a
boolean flag: --full

IOW, I still don't think slow and flaky have anything to do with what
we're trying to do here. I do appreciate that a single binary is better
than two.

>> 
>> >   - Level 4: flaky tests (aka, only ran when QEMU_TEST_FLAKY_TESTS set)
>> >
>> > Then we want to run level1 test only in tcg, and level1+2 in kvm.  We can
>> > only trigger level 1-3 or level 1-4 in manual tests.
>> >
>> > We used to have different way to provide the level idea, now I think we can
>> > consider provide that level in migration-test in one shot.  Obviously it's
>> > more than quick/slow so I don't think we can reuse "-m", but we can add our
>> >
>> > own test level "--level" parameter, so --level N means run all tests lower
>> > than level N, for example.
>> >
>> 
>> I'm not sure that works semantically for level 4. Because the reason one
>> runs flaky tests is different from the reason one runs the other
>> tests. So we probably don't want to run a bunch of tests just to get to
>> the broken ones.
>> 
>> But we don't need to spend too much time on this. I hate the idea of
>> flaky tests anyway. Whatever we choose they'll just sit there doing
>> nothing.
>
> Yes how to treat flaky tests isn't important yet.  If we don't care about
> QEMU_TEST_FLAKY_TESTS then we make it three levels.  The idea is the same.
>
>> 
>> >> 
>> >> >
>> >> >   (2) We want to run only a small portion of tests on tcg, more tests on
>> >> >       kvm.
>> >> 
>> >> Yes. Guests are fast with KVM and slow with TCG (generally) and the KVM
>> >> hosts are the ones where it's actually important to ensure all migration
>> >> features work OK. Non-KVM will only care about save/restore of
>> >> snapshots. Therefore we don't need to have all tests running with TCG,
>> >> only the smoke set.
>> >> 
>> >> And "smoke set" is arbitrary, not tied to speed, but of course no slow
>> >> tests please (which already happens because we don't pass -m slow to
>> >> migration-test-smoke).
>> >> 
>> >> >
>> >> > Ideally, we don't need two separate main test files, do we?
>> >> >
>> >> > I mean, we can do (1) with the existing migration-test.c, with the help of
>> >> > either gtest's "-m" or something we invent.  The only unfortunate part is
>> >> > qtest only have quick/slow, afaiu the "thorough" mode is the same as
>> >> > "slow".. while we don't yet have real "perf" tests.  It means we only have
>> >> > two buckets if we want to reuse gtest's "-m".
>> >> >
>> >> > Maybe it's enough?  If not, we can implement >2 categories in whatever
>> >> > form, either custom argv/argc cmdline, or env variable.
>> >> >
>> >> > Then, if we always categorize one test (let me try to not reuse glib's
>> >> > terms to be clear) into any of: FAST|NORMAL|SLOW|..., then we have a single
>> >> 
>> >> It's either normal or slow. Because we only know a test is only after it
>> >> bothers us.
>> >
>> > So I wonder if we can provide four levels, as above.. and define it for
>> > each test in migration-test.
>> >
>> >> 
>> >> > migration-test that have different level of tests.  We can invoke
>> >> > "migration-test --mode FAST" if kvm is not supported, and invoke the same
>> >> > "migration-test --mode SLOW" if kvm is supported.
>> >> 
>> >> This is messy due to how qtest/meson.build works. Having two tests is
>> >> the clean change. Otherwise we'll have to add "if migration-test" or
>> >> create artificial test names to be able to restrict the arguments that
>> >> are passed to the test per arch.
>> >
>> > Indeed it'll need a few extra lines in meson, but it doesn't look too bad,
>> > but yeah if anyone is not happy with it we can rethink.  I just want to
>> > know whether it's still acceptable.
>> >
>> > I tried to code it up, it looks like this:
>> >
>> > ====8<====
>> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> > index c5a70021c5..5bec33b627 100644
>> > --- a/tests/qtest/meson.build
>> > +++ b/tests/qtest/meson.build
>> > @@ -392,6 +392,12 @@ if dbus_display
>> >    qtests += {'dbus-display-test': [dbus_display1, gio]}
>> >  endif
>> >  
>> > +if run_command('test', '-e', '/dev/kvm', check: false).returncode() == 0
>> > +  has_kvm = true
>> > +else
>> > +  has_kvm =false
>> > +endif
>> 
>> This is not right. Checking /dev/kvm at configure time doesn't ensure it
>> will be present at test runtime. It also doesn't account for builds with
>
> Why the test runtime would be a different host versus whoever setup the
> meson build?
>

User permissions, containers, configuration changes in between build
time and runtime, etc.

Also, it's quite convenient to be able to pass any QEMU binary to any
version of the tests, I do that with downstream builds sometimes.

>> CONFIG_KVM=n or builds without both KVM and TCG. This needs to be done
>> inside the test.
>
> This is true, but IIUC that's not a blocker, as we can use (btw, I found
> fs.exists() a better alternative than my previous hack):
>
> if fs.exists('/dev/kvm') and 'CONFIG_KVM' in config_all_accel
>   has_kvm = true
> else
>   has_kvm = false
> endif
>

I dislike this, however I'm thinking what's the worse that could happen
if there's a mismatch between configure and runtime? We'd just run a
different set of tests.

Can we make it:

meson.build:
  # If there's KVM support, run the full set of migration tests as KVM
  # hosts tend to use more migration features than just save/restore.
  if fs.exists('/dev/kvm')
    migration_test_args = "--full"
  endif

cmdline invocations:
  ./migration-test                 # runs smoke, i.e. level 1
  ./migration-test -m slow         # runs smoke only, no slow tests in the smoke set
  FLAKY=1 ./migration-test         # runs smoke only, no flaky tests in the smoke set

  ./migration-test --full          # runs full set, i.e. level 2
  ./migration-test --full -m slow  # runs full set + slow tests
  FLAKY=1 ./migration-test --full  # runs full set + flaky tests

I made the first one like that so the compat tests in CI now run less
tests. We don't need full set during compat because that job is about
catching changes in device code. It would also make the argument easier
to enable the compat job for all migration-test-supported archs.

>> 
>> I think the best we can do is have a qtest_migration_level_<ARCH> and
>> set it for every arch.
>> 
>> Also note that we must keep plain 'migration-test' invocation working
>> because of the compat test.
>
> We won't break it if we only switch to levels, right?
>
> Btw, I also don't know why we need to.  IIRC the compat test runs the test
> in previous release (but only feeds the new QEMU binary to the old
> migration-test)?  I think that's one reason why we decided to use the old
> migration-test (so we won't have new tests ran on compat tests, which is a
> loss), just to avoid any change in migration-test will break the compat
> test.. so I assume that should be fine regardless..

I meant we shouldn't break the command line invocation:

./tests/qtest/migration-test -p <test_name>

As in, we cannot change the test name or add mandatory flags. Otherwise
we have a discrepancy betweem what the CI job is calling vs. what the
build actually provides. We run the tests from the previous build, but
the CI job is current.

Another point that is more of an annoyance is that the migration-test
invocation not being stable affectes debug, bisect, etc. When debugging
the recent multifd regression I already had to keep changing from
/multifd/tcp/... to /multifd/tcp/uri/... when changing QEMU versions.

On Thu, Dec 19, 2024 at 12:38:05PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Dec 18, 2024 at 06:08:01PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Wed, Dec 18, 2024 at 03:13:08PM -0300, Fabiano Rosas wrote:
> >> >> Peter Xu <peterx@redhat.com> writes:
> >> >> 
> >> >> > On Wed, Nov 13, 2024 at 04:46:27PM -0300, Fabiano Rosas wrote:
> >> >> >> diff --git a/tests/qtest/migration-test-smoke.c b/tests/qtest/migration-test-smoke.c
> >> >> >> new file mode 100644
> >> >> >> index 0000000000..ff2d72881f
> >> >> >> --- /dev/null
> >> >> >> +++ b/tests/qtest/migration-test-smoke.c
> >> >> >> @@ -0,0 +1,39 @@
> >> >> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> >> >> +
> >> >> >> +#include "qemu/osdep.h"
> >> >> >> +#include "libqtest.h"
> >> >> >> +#include "migration/test-framework.h"
> >> >> >> +#include "qemu/module.h"
> >> >> >> +
> >> >> >> +int main(int argc, char **argv)
> >> >> >> +{
> >> >> >> +    MigrationTestEnv *env;
> >> >> >> +    int ret;
> >> >> >> +
> >> >> >> +    g_test_init(&argc, &argv, NULL);
> >> >> >> +    env = migration_get_env();
> >> >> >> +    module_call_init(MODULE_INIT_QOM);
> >> >> >> +
> >> >> >> +    if (env->has_kvm) {
> >> >> >> +        g_test_message(
> >> >> >> +            "Smoke tests already run as part of the full suite on KVM hosts");
> >> >> >> +        goto out;
> >> >> >> +    }
> >> >> >
> >> >> > So the "smoke" here is almost "tcg".. and if i want to run a smoke test on
> >> >> > a kvm-enabled host, it's noop.. which isn't easy to understand why.
> >> >> >
> >> >> > If to rethink our goal, we have two requirements:
> >> >> >
> >> >> >   (1) We want to categorize migration tests, so some are quick, some are
> >> >> >       slow, some might be flacky.  Maybe more, but it's about putting one
> >> >> >       test into only one bucket, and there're >1 buckets.
> >> >> 
> >> >> It's true that the smoke test should never have slow or flaky tests, but
> >> >> we can't use this categorization for anything else. IOW, what you
> >> >> describe here is not a goal. If a test is found to be slow we put it
> >> >> under slow and it will only run with -m slow/thorough, that's it. We can
> >> >> just ignore this.
> >> >
> >> > I could have missed something, but I still think it's the same issue.  In
> >> > general, I think we want to provide different levels of tests, like:
> >> >
> >> >   - Level 1: the minimum set of tests (aka, the "smoke" idea here)
> >> >   - Level 2: normal set of tests (aka, whatever we used to run by default)
> >> >   - Level 3: slow tests (aka, only ran with '-m slow' before)
> >> 
> >> How are you going to make this one work? 'migration-test --level 3'
> >> vs. 'migration-test --level 3 -m slow' vs. 'migration-test -m slow'
> >> 
> >> The only way I can see is to not have a level 3 at all and just use -m
> >> slow.
> >
> > I meant remove "-m" and remove QEMU_TEST_FLAKY_TESTS, instead replacing all
> > of them using --level.  Then migration-test ignores '-m' in the future
> > because it's simply not enough for us.
> >
> 
> Even if we remove -m, it will still be passed in when people run make
> SPEED=slow check and people who know about glib/qtests will continue to
> try to use it.
> 
> Ignoring -m and FLAKY will create a diversion from the rest of qtest and
> QEMU tests in general. I think the best approach is to drop levels 3 and
> 4 from this proposal and just ignore those options altogether.
> 
> If we're going to have a single binary as you suggest, then there's no
> harm still using -m slow and FLAKY as usual. We call level 1: "smoke
> tests", level 2: "full set" and we put in some code to prevent
> registering a smoke test as slow or flaky and that's it.
> 
> ... and of course, if there's only two levels, then we only need a
> boolean flag: --full
> 
> IOW, I still don't think slow and flaky have anything to do with what
> we're trying to do here. I do appreciate that a single binary is better
> than two.

Yes, the major goal is to keep it a single binary.

We can make it two levels.  However if so I'd rather reuse -m, then we keep
"-m quick" for tcg, "-m slow" for tcg+kvm.  We can move the current "slow
tests" into another env var: basically those ones (dirtylimit, auto
converge, xbzrle, iirc) and FLAKY ones won't be run by anyone but us.

> 
> >> 
> >> >   - Level 4: flaky tests (aka, only ran when QEMU_TEST_FLAKY_TESTS set)
> >> >
> >> > Then we want to run level1 test only in tcg, and level1+2 in kvm.  We can
> >> > only trigger level 1-3 or level 1-4 in manual tests.
> >> >
> >> > We used to have different way to provide the level idea, now I think we can
> >> > consider provide that level in migration-test in one shot.  Obviously it's
> >> > more than quick/slow so I don't think we can reuse "-m", but we can add our
> >> >
> >> > own test level "--level" parameter, so --level N means run all tests lower
> >> > than level N, for example.
> >> >
> >> 
> >> I'm not sure that works semantically for level 4. Because the reason one
> >> runs flaky tests is different from the reason one runs the other
> >> tests. So we probably don't want to run a bunch of tests just to get to
> >> the broken ones.
> >> 
> >> But we don't need to spend too much time on this. I hate the idea of
> >> flaky tests anyway. Whatever we choose they'll just sit there doing
> >> nothing.
> >
> > Yes how to treat flaky tests isn't important yet.  If we don't care about
> > QEMU_TEST_FLAKY_TESTS then we make it three levels.  The idea is the same.
> >
> >> 
> >> >> 
> >> >> >
> >> >> >   (2) We want to run only a small portion of tests on tcg, more tests on
> >> >> >       kvm.
> >> >> 
> >> >> Yes. Guests are fast with KVM and slow with TCG (generally) and the KVM
> >> >> hosts are the ones where it's actually important to ensure all migration
> >> >> features work OK. Non-KVM will only care about save/restore of
> >> >> snapshots. Therefore we don't need to have all tests running with TCG,
> >> >> only the smoke set.
> >> >> 
> >> >> And "smoke set" is arbitrary, not tied to speed, but of course no slow
> >> >> tests please (which already happens because we don't pass -m slow to
> >> >> migration-test-smoke).
> >> >> 
> >> >> >
> >> >> > Ideally, we don't need two separate main test files, do we?
> >> >> >
> >> >> > I mean, we can do (1) with the existing migration-test.c, with the help of
> >> >> > either gtest's "-m" or something we invent.  The only unfortunate part is
> >> >> > qtest only have quick/slow, afaiu the "thorough" mode is the same as
> >> >> > "slow".. while we don't yet have real "perf" tests.  It means we only have
> >> >> > two buckets if we want to reuse gtest's "-m".
> >> >> >
> >> >> > Maybe it's enough?  If not, we can implement >2 categories in whatever
> >> >> > form, either custom argv/argc cmdline, or env variable.
> >> >> >
> >> >> > Then, if we always categorize one test (let me try to not reuse glib's
> >> >> > terms to be clear) into any of: FAST|NORMAL|SLOW|..., then we have a single
> >> >> 
> >> >> It's either normal or slow. Because we only know a test is only after it
> >> >> bothers us.
> >> >
> >> > So I wonder if we can provide four levels, as above.. and define it for
> >> > each test in migration-test.
> >> >
> >> >> 
> >> >> > migration-test that have different level of tests.  We can invoke
> >> >> > "migration-test --mode FAST" if kvm is not supported, and invoke the same
> >> >> > "migration-test --mode SLOW" if kvm is supported.
> >> >> 
> >> >> This is messy due to how qtest/meson.build works. Having two tests is
> >> >> the clean change. Otherwise we'll have to add "if migration-test" or
> >> >> create artificial test names to be able to restrict the arguments that
> >> >> are passed to the test per arch.
> >> >
> >> > Indeed it'll need a few extra lines in meson, but it doesn't look too bad,
> >> > but yeah if anyone is not happy with it we can rethink.  I just want to
> >> > know whether it's still acceptable.
> >> >
> >> > I tried to code it up, it looks like this:
> >> >
> >> > ====8<====
> >> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> >> > index c5a70021c5..5bec33b627 100644
> >> > --- a/tests/qtest/meson.build
> >> > +++ b/tests/qtest/meson.build
> >> > @@ -392,6 +392,12 @@ if dbus_display
> >> >    qtests += {'dbus-display-test': [dbus_display1, gio]}
> >> >  endif
> >> >  
> >> > +if run_command('test', '-e', '/dev/kvm', check: false).returncode() == 0
> >> > +  has_kvm = true
> >> > +else
> >> > +  has_kvm =false
> >> > +endif
> >> 
> >> This is not right. Checking /dev/kvm at configure time doesn't ensure it
> >> will be present at test runtime. It also doesn't account for builds with
> >
> > Why the test runtime would be a different host versus whoever setup the
> > meson build?
> >
> 
> User permissions, containers, configuration changes in between build
> time and runtime, etc.

I was thinking there's no way in CI that it can be done separately.  But
yeah I don't know well on CI.. so if it can happen easily that's a problem.

> 
> Also, it's quite convenient to be able to pass any QEMU binary to any
> version of the tests, I do that with downstream builds sometimes.

IIUC we're only talking about "meson test" behavior, I assume
migration-test should be mostly unaffected on how it'll be used, except
that indeed we're changing "how to define quick/slow tests".  More below.

> 
> >> CONFIG_KVM=n or builds without both KVM and TCG. This needs to be done
> >> inside the test.
> >
> > This is true, but IIUC that's not a blocker, as we can use (btw, I found
> > fs.exists() a better alternative than my previous hack):
> >
> > if fs.exists('/dev/kvm') and 'CONFIG_KVM' in config_all_accel
> >   has_kvm = true
> > else
> >   has_kvm = false
> > endif
> >
> 
> I dislike this, however I'm thinking what's the worse that could happen
> if there's a mismatch between configure and runtime? We'd just run a
> different set of tests.
> 
> Can we make it:
> 
> meson.build:
>   # If there's KVM support, run the full set of migration tests as KVM
>   # hosts tend to use more migration features than just save/restore.
>   if fs.exists('/dev/kvm')
>     migration_test_args = "--full"

So IMHO we could go "-m slow | -m thorough" here, as mentioned above, just
to avoid making it a matrix of (--full, -m) combinations.

Then move all old slow tests into (just to avoid using "slow" as a word):

  if (getenv("QEMU_MIG_TEST_EXTRA")) ...

And rename FLAKY into:

  if (getenv("QEMU_MIG_TEST_FLAKY")) ...

Then we test it with QEMU_MIG_TEST_FLAKY=1 and QEMU_MIG_TEST_EXTRA=1 if we
want all tests.

>   endif
> 
> cmdline invocations:
>   ./migration-test                 # runs smoke, i.e. level 1
>   ./migration-test -m slow         # runs smoke only, no slow tests in the smoke set
>   FLAKY=1 ./migration-test         # runs smoke only, no flaky tests in the smoke set
> 
>   ./migration-test --full          # runs full set, i.e. level 2
>   ./migration-test --full -m slow  # runs full set + slow tests
>   FLAKY=1 ./migration-test --full  # runs full set + flaky tests
> 
> I made the first one like that so the compat tests in CI now run less
> tests. We don't need full set during compat because that job is about
> catching changes in device code. It would also make the argument easier
> to enable the compat job for all migration-test-supported archs.
> 
> >> 
> >> I think the best we can do is have a qtest_migration_level_<ARCH> and
> >> set it for every arch.
> >> 
> >> Also note that we must keep plain 'migration-test' invocation working
> >> because of the compat test.
> >
> > We won't break it if we only switch to levels, right?
> >
> > Btw, I also don't know why we need to.  IIRC the compat test runs the test
> > in previous release (but only feeds the new QEMU binary to the old
> > migration-test)?  I think that's one reason why we decided to use the old
> > migration-test (so we won't have new tests ran on compat tests, which is a
> > loss), just to avoid any change in migration-test will break the compat
> > test.. so I assume that should be fine regardless..
> 
> I meant we shouldn't break the command line invocation:
> 
> ./tests/qtest/migration-test -p <test_name>
> 
> As in, we cannot change the test name or add mandatory flags. Otherwise
> we have a discrepancy betweem what the CI job is calling vs. what the
> build actually provides. We run the tests from the previous build, but
> the CI job is current.

I failed to follow here.  Our CI doesn't hardcode any <test_name>, right?
It should invoke "migration-test" in the old build, feeding new QEMU binary
to it, run whatever are there in the old test.

> 
> Another point that is more of an annoyance is that the migration-test
> invocation not being stable affectes debug, bisect, etc. When debugging
> the recent multifd regression I already had to keep changing from
> /multifd/tcp/... to /multifd/tcp/uri/... when changing QEMU versions.

So I could have missed something above, and I can understand this adds
burden to bisections.  However I do prefer we can change test path any way
we want (even if in most cases we shouldn't ever touch them, and we should
still try to not change them as frequent).  IOW we also need to consider
the overhead of keeping test paths to be part of ABI as well.

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Thu, Dec 19, 2024 at 12:38:05PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Wed, Dec 18, 2024 at 06:08:01PM -0300, Fabiano Rosas wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > On Wed, Dec 18, 2024 at 03:13:08PM -0300, Fabiano Rosas wrote:
>> >> >> Peter Xu <peterx@redhat.com> writes:
>> >> >> 
>> >> >> > On Wed, Nov 13, 2024 at 04:46:27PM -0300, Fabiano Rosas wrote:
>> >> >> >> diff --git a/tests/qtest/migration-test-smoke.c b/tests/qtest/migration-test-smoke.c
>> >> >> >> new file mode 100644
>> >> >> >> index 0000000000..ff2d72881f
>> >> >> >> --- /dev/null
>> >> >> >> +++ b/tests/qtest/migration-test-smoke.c
>> >> >> >> @@ -0,0 +1,39 @@
>> >> >> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> >> >> >> +
>> >> >> >> +#include "qemu/osdep.h"
>> >> >> >> +#include "libqtest.h"
>> >> >> >> +#include "migration/test-framework.h"
>> >> >> >> +#include "qemu/module.h"
>> >> >> >> +
>> >> >> >> +int main(int argc, char **argv)
>> >> >> >> +{
>> >> >> >> +    MigrationTestEnv *env;
>> >> >> >> +    int ret;
>> >> >> >> +
>> >> >> >> +    g_test_init(&argc, &argv, NULL);
>> >> >> >> +    env = migration_get_env();
>> >> >> >> +    module_call_init(MODULE_INIT_QOM);
>> >> >> >> +
>> >> >> >> +    if (env->has_kvm) {
>> >> >> >> +        g_test_message(
>> >> >> >> +            "Smoke tests already run as part of the full suite on KVM hosts");
>> >> >> >> +        goto out;
>> >> >> >> +    }
>> >> >> >
>> >> >> > So the "smoke" here is almost "tcg".. and if i want to run a smoke test on
>> >> >> > a kvm-enabled host, it's noop.. which isn't easy to understand why.
>> >> >> >
>> >> >> > If to rethink our goal, we have two requirements:
>> >> >> >
>> >> >> >   (1) We want to categorize migration tests, so some are quick, some are
>> >> >> >       slow, some might be flacky.  Maybe more, but it's about putting one
>> >> >> >       test into only one bucket, and there're >1 buckets.
>> >> >> 
>> >> >> It's true that the smoke test should never have slow or flaky tests, but
>> >> >> we can't use this categorization for anything else. IOW, what you
>> >> >> describe here is not a goal. If a test is found to be slow we put it
>> >> >> under slow and it will only run with -m slow/thorough, that's it. We can
>> >> >> just ignore this.
>> >> >
>> >> > I could have missed something, but I still think it's the same issue.  In
>> >> > general, I think we want to provide different levels of tests, like:
>> >> >
>> >> >   - Level 1: the minimum set of tests (aka, the "smoke" idea here)
>> >> >   - Level 2: normal set of tests (aka, whatever we used to run by default)
>> >> >   - Level 3: slow tests (aka, only ran with '-m slow' before)
>> >> 
>> >> How are you going to make this one work? 'migration-test --level 3'
>> >> vs. 'migration-test --level 3 -m slow' vs. 'migration-test -m slow'
>> >> 
>> >> The only way I can see is to not have a level 3 at all and just use -m
>> >> slow.
>> >
>> > I meant remove "-m" and remove QEMU_TEST_FLAKY_TESTS, instead replacing all
>> > of them using --level.  Then migration-test ignores '-m' in the future
>> > because it's simply not enough for us.
>> >
>> 
>> Even if we remove -m, it will still be passed in when people run make
>> SPEED=slow check and people who know about glib/qtests will continue to
>> try to use it.
>> 
>> Ignoring -m and FLAKY will create a diversion from the rest of qtest and
>> QEMU tests in general. I think the best approach is to drop levels 3 and
>> 4 from this proposal and just ignore those options altogether.
>> 
>> If we're going to have a single binary as you suggest, then there's no
>> harm still using -m slow and FLAKY as usual. We call level 1: "smoke
>> tests", level 2: "full set" and we put in some code to prevent
>> registering a smoke test as slow or flaky and that's it.
>> 
>> ... and of course, if there's only two levels, then we only need a
>> boolean flag: --full
>> 
>> IOW, I still don't think slow and flaky have anything to do with what
>> we're trying to do here. I do appreciate that a single binary is better
>> than two.
>
> Yes, the major goal is to keep it a single binary.
>
> We can make it two levels.  However if so I'd rather reuse -m, then we keep
> "-m quick" for tcg, "-m slow" for tcg+kvm.  We can move the current "slow

I don't understand why you insist in bringing slow/quick into this. '-m
quick' is for quick tests and '-m slow' is for slow tests. What we're
doing here is: "small set" vs. "large set". Except for our own sanity
we're excluding slow tests from the "small set", otherwise the
perception of smallness is counteracted by the perception of slowness.

Then, we're determining arbitrarily that the small set is to be run only
on TCG hosts and the large set is to be run in the KVM hosts. And via
cmdline we want to be able to run anything basically.

> tests" into another env var: basically those ones (dirtylimit, auto
> converge, xbzrle, iirc) and FLAKY ones won't be run by anyone but us.
>
>> 
>> >> 
>> >> >
>> >> > Indeed it'll need a few extra lines in meson, but it doesn't look too bad,
>> >> > but yeah if anyone is not happy with it we can rethink.  I just want to
>> >> > know whether it's still acceptable.
>> >> >
>> >> > I tried to code it up, it looks like this:
>> >> >
>> >> > ====8<====
>> >> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> >> > index c5a70021c5..5bec33b627 100644
>> >> > --- a/tests/qtest/meson.build
>> >> > +++ b/tests/qtest/meson.build
>> >> > @@ -392,6 +392,12 @@ if dbus_display
>> >> >    qtests += {'dbus-display-test': [dbus_display1, gio]}
>> >> >  endif
>> >> >  
>> >> > +if run_command('test', '-e', '/dev/kvm', check: false).returncode() == 0
>> >> > +  has_kvm = true
>> >> > +else
>> >> > +  has_kvm =false
>> >> > +endif
>> >> 
>> >> This is not right. Checking /dev/kvm at configure time doesn't ensure it
>> >> will be present at test runtime. It also doesn't account for builds with
>> >
>> > Why the test runtime would be a different host versus whoever setup the
>> > meson build?
>> >
>> 
>> User permissions, containers, configuration changes in between build
>> time and runtime, etc.
>
> I was thinking there's no way in CI that it can be done separately.  But
> yeah I don't know well on CI.. so if it can happen easily that's a problem.
>

I was thinking more of make check in peoples machines. Who knows what
sort of setups QEMU developers have... As for CI, I don't think it
should change and as I said, if it does then the test itself should be
able to cope.

>> 
>> Also, it's quite convenient to be able to pass any QEMU binary to any
>> version of the tests, I do that with downstream builds sometimes.
>
> IIUC we're only talking about "meson test" behavior, I assume
> migration-test should be mostly unaffected on how it'll be used, except
> that indeed we're changing "how to define quick/slow tests".  More below.
>

Yes, indeed.

>> 
>> >> CONFIG_KVM=n or builds without both KVM and TCG. This needs to be done
>> >> inside the test.
>> >
>> > This is true, but IIUC that's not a blocker, as we can use (btw, I found
>> > fs.exists() a better alternative than my previous hack):
>> >
>> > if fs.exists('/dev/kvm') and 'CONFIG_KVM' in config_all_accel
>> >   has_kvm = true
>> > else
>> >   has_kvm = false
>> > endif
>> >
>> 
>> I dislike this, however I'm thinking what's the worse that could happen
>> if there's a mismatch between configure and runtime? We'd just run a
>> different set of tests.
>> 
>> Can we make it:
>> 
>> meson.build:
>>   # If there's KVM support, run the full set of migration tests as KVM
>>   # hosts tend to use more migration features than just save/restore.
>>   if fs.exists('/dev/kvm')
>>     migration_test_args = "--full"
>
> So IMHO we could go "-m slow | -m thorough" here, as mentioned above, just
> to avoid making it a matrix of (--full, -m) combinations.
>
> Then move all old slow tests into (just to avoid using "slow" as a word):
>
>   if (getenv("QEMU_MIG_TEST_EXTRA")) ...
>
> And rename FLAKY into:
>
>   if (getenv("QEMU_MIG_TEST_FLAKY")) ...
>
> Then we test it with QEMU_MIG_TEST_FLAKY=1 and QEMU_MIG_TEST_EXTRA=1 if we
> want all tests.

We shouldn't change stuff that's also used by the rest of the
community. People know about QEMU_TEST_FLAKY_TESTS and -m slow. These
must continue to work the same.

We can say: "Internally we don't allow slow and flaky to be in the smoke
set".

We cannot say: "In migration-test QEMU_TEST_FLAKY_TESTS is actually
QEMU_MIG_TEST_FLAKY, -m slow is actually QEMU_MIG_TEST_EXTRA, -m slow
just implies KVM and -m quick implies TCG. Easy peasy!"

>
>>   endif
>> 
>> cmdline invocations:
>>   ./migration-test                 # runs smoke, i.e. level 1
>>   ./migration-test -m slow         # runs smoke only, no slow tests in the smoke set
>>   FLAKY=1 ./migration-test         # runs smoke only, no flaky tests in the smoke set
>> 
>>   ./migration-test --full          # runs full set, i.e. level 2
>>   ./migration-test --full -m slow  # runs full set + slow tests
>>   FLAKY=1 ./migration-test --full  # runs full set + flaky tests

Don't see this^ as a matrix of --full and -m. This is identical to what
we have today, with the addition of a flag that determines the amount of
tests run. We could call it other names if we want:

--size small/large
--testset smoke/full

>> 
>> I made the first one like that so the compat tests in CI now run less
>> tests. We don't need full set during compat because that job is about
>> catching changes in device code. It would also make the argument easier
>> to enable the compat job for all migration-test-supported archs.
>> 
>> >> 
>> >> I think the best we can do is have a qtest_migration_level_<ARCH> and
>> >> set it for every arch.
>> >> 
>> >> Also note that we must keep plain 'migration-test' invocation working
>> >> because of the compat test.
>> >
>> > We won't break it if we only switch to levels, right?
>> >
>> > Btw, I also don't know why we need to.  IIRC the compat test runs the test
>> > in previous release (but only feeds the new QEMU binary to the old
>> > migration-test)?  I think that's one reason why we decided to use the old
>> > migration-test (so we won't have new tests ran on compat tests, which is a
>> > loss), just to avoid any change in migration-test will break the compat
>> > test.. so I assume that should be fine regardless..
>> 
>> I meant we shouldn't break the command line invocation:
>> 
>> ./tests/qtest/migration-test -p <test_name>
>> 
>> As in, we cannot change the test name or add mandatory flags. Otherwise
>> we have a discrepancy betweem what the CI job is calling vs. what the
>> build actually provides. We run the tests from the previous build, but
>> the CI job is current.
>
> I failed to follow here.  Our CI doesn't hardcode any <test_name>, right?
> It should invoke "migration-test" in the old build, feeding new QEMU binary
> to it, run whatever are there in the old test.

Yes, but we have the words "migration-test" in the CI .yaml *today*. It
doesn't matter if it invokes the tests from the last release. If we
changed the name to "migration-foobar", then the CI job continues to
work up until 10.0 is released. It then breaks immediately in the first
commit of 10.0.50 because the previous release will now have the
"migration-foobar" name while the CI still calls for "migration-test".

Basically the point is that CI .yaml changes take effect immediately
while test cmdline changes only take effect (in CI) on the next release.

>
>> 
>> Another point that is more of an annoyance is that the migration-test
>> invocation not being stable affectes debug, bisect, etc. When debugging
>> the recent multifd regression I already had to keep changing from
>> /multifd/tcp/... to /multifd/tcp/uri/... when changing QEMU versions.
>
> So I could have missed something above, and I can understand this adds
> burden to bisections.  However I do prefer we can change test path any way
> we want (even if in most cases we shouldn't ever touch them, and we should
> still try to not change them as frequent).  IOW we also need to consider
> the overhead of keeping test paths to be part of ABI as well.

It's annoying, that's all. Makes me 1% more grumpy.

On Thu, Dec 19, 2024 at 04:31:04PM -0300, Fabiano Rosas wrote:
> We shouldn't change stuff that's also used by the rest of the
> community. People know about QEMU_TEST_FLAKY_TESTS and -m slow. These
> must continue to work the same.

I see what I overlook; it's used much more than I thought in qtest and we
also have a CI for it.. So ok, let's keep at least QEMU_TEST_FLAKY_TESTS.

But again, I don't think it matters much even if we rename it, it means the
flaky CI test won't run these two migration tests, but that's not the end
of the world either, if you see what I meant.  CI relies on the normal
tests rather than flaky tests to present.

We should be able to move in / take out FLAKY tests at will, as that's not
what CI is really relying on.  Here renaming the macro in migration test
almost means we take both out.

> 
> We can say: "Internally we don't allow slow and flaky to be in the smoke
> set".
> 
> We cannot say: "In migration-test QEMU_TEST_FLAKY_TESTS is actually
> QEMU_MIG_TEST_FLAKY, -m slow is actually QEMU_MIG_TEST_EXTRA, -m slow
> just implies KVM and -m quick implies TCG. Easy peasy!"
> 
> >
> >>   endif
> >> 
> >> cmdline invocations:
> >>   ./migration-test                 # runs smoke, i.e. level 1
> >>   ./migration-test -m slow         # runs smoke only, no slow tests in the smoke set
> >>   FLAKY=1 ./migration-test         # runs smoke only, no flaky tests in the smoke set
> >> 
> >>   ./migration-test --full          # runs full set, i.e. level 2
> >>   ./migration-test --full -m slow  # runs full set + slow tests
> >>   FLAKY=1 ./migration-test --full  # runs full set + flaky tests
> 
> Don't see this^ as a matrix of --full and -m. This is identical to what
> we have today, with the addition of a flag that determines the amount of
> tests run. We could call it other names if we want:
> 
> --size small/large
> --testset smoke/full
> 
> >> 
> >> I made the first one like that so the compat tests in CI now run less
> >> tests. We don't need full set during compat because that job is about
> >> catching changes in device code. It would also make the argument easier
> >> to enable the compat job for all migration-test-supported archs.
> >> 
> >> >> 
> >> >> I think the best we can do is have a qtest_migration_level_<ARCH> and
> >> >> set it for every arch.
> >> >> 
> >> >> Also note that we must keep plain 'migration-test' invocation working
> >> >> because of the compat test.
> >> >
> >> > We won't break it if we only switch to levels, right?
> >> >
> >> > Btw, I also don't know why we need to.  IIRC the compat test runs the test
> >> > in previous release (but only feeds the new QEMU binary to the old
> >> > migration-test)?  I think that's one reason why we decided to use the old
> >> > migration-test (so we won't have new tests ran on compat tests, which is a
> >> > loss), just to avoid any change in migration-test will break the compat
> >> > test.. so I assume that should be fine regardless..
> >> 
> >> I meant we shouldn't break the command line invocation:
> >> 
> >> ./tests/qtest/migration-test -p <test_name>
> >> 
> >> As in, we cannot change the test name or add mandatory flags. Otherwise
> >> we have a discrepancy betweem what the CI job is calling vs. what the
> >> build actually provides. We run the tests from the previous build, but
> >> the CI job is current.
> >
> > I failed to follow here.  Our CI doesn't hardcode any <test_name>, right?
> > It should invoke "migration-test" in the old build, feeding new QEMU binary
> > to it, run whatever are there in the old test.
> 
> Yes, but we have the words "migration-test" in the CI .yaml *today*. It
> doesn't matter if it invokes the tests from the last release. If we
> changed the name to "migration-foobar", then the CI job continues to
> work up until 10.0 is released. It then breaks immediately in the first
> commit of 10.0.50 because the previous release will now have the
> "migration-foobar" name while the CI still calls for "migration-test".

Not fair at all: nobody suggested to rename the test!

> 
> Basically the point is that CI .yaml changes take effect immediately
> while test cmdline changes only take effect (in CI) on the next release.

Yes, but so far the "API" is the test name only (and actually not.. more
below), and at least no path involved.  That's why I want to make sure
we're on the same page.  So looks like at least "what tests to run by
default", and "full path of each of the test case" can still change.

Here's the "more below" part: logically if we want we can change the name
of migration-test.  We need to teach the CI on which version of QEMU to use
which program to test in the compat tests.  It isn't really hard (a git-tag
-> prog-name hash), it's just unnecessary to change the test name at all.

> 
> >
> >> 
> >> Another point that is more of an annoyance is that the migration-test
> >> invocation not being stable affectes debug, bisect, etc. When debugging
> >> the recent multifd regression I already had to keep changing from
> >> /multifd/tcp/... to /multifd/tcp/uri/... when changing QEMU versions.
> >
> > So I could have missed something above, and I can understand this adds
> > burden to bisections.  However I do prefer we can change test path any way
> > we want (even if in most cases we shouldn't ever touch them, and we should
> > still try to not change them as frequent).  IOW we also need to consider
> > the overhead of keeping test paths to be part of ABI as well.
> 
> It's annoying, that's all. Makes me 1% more grumpy.

I'm not going to change any.. but keeping it a protocol is another thing.

So to summarize..

My plan (after adjustment, keeping the name of QEMU_TEST_FLAKY_TESTS) is to
introduce QEMU_TEST_EXTRA_TESTS (renamed following FLAKY) and cover the
three current "slow" tests.  Then we stick with -m for the new quick/slow,
which maps to tcg/kvm directly.  That saves the extra --full parameter.

The diff v.s. your plan is, afaict, you prefer introducing yet another
--full parameter.

Yours is good that we stick with the whole test in compat tests with no
extra change, which is a benefit indeed.

If go with my plan, the default compat behavior will start to change after
10.0 released.  We need to do one more step if we still prefer the wholeset
run in compat test, which is at the start of 10.1, we send a patch to
change compat test's parameter from none to "-m slow".

Feel free to go ahead with whatever you prefer.  To me, the more important
bit is whether we both think that one program is better than two, and also
that means decouple host setup v.s. tests. I don't have a strong opinion
otherwise..

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Thu, Dec 19, 2024 at 04:31:04PM -0300, Fabiano Rosas wrote:
>> We shouldn't change stuff that's also used by the rest of the
>> community. People know about QEMU_TEST_FLAKY_TESTS and -m slow. These
>> must continue to work the same.
>
> I see what I overlook; it's used much more than I thought in qtest and we
> also have a CI for it.. So ok, let's keep at least QEMU_TEST_FLAKY_TESTS.
>
> But again, I don't think it matters much even if we rename it, it means the
> flaky CI test won't run these two migration tests, but that's not the end
> of the world either, if you see what I meant.  CI relies on the normal
> tests rather than flaky tests to present.
>
> We should be able to move in / take out FLAKY tests at will, as that's not
> what CI is really relying on.  Here renaming the macro in migration test
> almost means we take both out.
>
>> 
>> We can say: "Internally we don't allow slow and flaky to be in the smoke
>> set".
>> 
>> We cannot say: "In migration-test QEMU_TEST_FLAKY_TESTS is actually
>> QEMU_MIG_TEST_FLAKY, -m slow is actually QEMU_MIG_TEST_EXTRA, -m slow
>> just implies KVM and -m quick implies TCG. Easy peasy!"
>> 
>> >
>> >>   endif
>> >> 
>> >> cmdline invocations:
>> >>   ./migration-test                 # runs smoke, i.e. level 1
>> >>   ./migration-test -m slow         # runs smoke only, no slow tests in the smoke set
>> >>   FLAKY=1 ./migration-test         # runs smoke only, no flaky tests in the smoke set
>> >> 
>> >>   ./migration-test --full          # runs full set, i.e. level 2
>> >>   ./migration-test --full -m slow  # runs full set + slow tests
>> >>   FLAKY=1 ./migration-test --full  # runs full set + flaky tests
>> 
>> Don't see this^ as a matrix of --full and -m. This is identical to what
>> we have today, with the addition of a flag that determines the amount of
>> tests run. We could call it other names if we want:
>> 
>> --size small/large
>> --testset smoke/full
>> 
>> >> 
>> >> I made the first one like that so the compat tests in CI now run less
>> >> tests. We don't need full set during compat because that job is about
>> >> catching changes in device code. It would also make the argument easier
>> >> to enable the compat job for all migration-test-supported archs.
>> >> 
>> >> >> 
>> >> >> I think the best we can do is have a qtest_migration_level_<ARCH> and
>> >> >> set it for every arch.
>> >> >> 
>> >> >> Also note that we must keep plain 'migration-test' invocation working
>> >> >> because of the compat test.
>> >> >
>> >> > We won't break it if we only switch to levels, right?
>> >> >
>> >> > Btw, I also don't know why we need to.  IIRC the compat test runs the test
>> >> > in previous release (but only feeds the new QEMU binary to the old
>> >> > migration-test)?  I think that's one reason why we decided to use the old
>> >> > migration-test (so we won't have new tests ran on compat tests, which is a
>> >> > loss), just to avoid any change in migration-test will break the compat
>> >> > test.. so I assume that should be fine regardless..
>> >> 
>> >> I meant we shouldn't break the command line invocation:
>> >> 
>> >> ./tests/qtest/migration-test -p <test_name>
>> >> 
>> >> As in, we cannot change the test name or add mandatory flags. Otherwise
>> >> we have a discrepancy betweem what the CI job is calling vs. what the
>> >> build actually provides. We run the tests from the previous build, but
>> >> the CI job is current.
>> >
>> > I failed to follow here.  Our CI doesn't hardcode any <test_name>, right?
>> > It should invoke "migration-test" in the old build, feeding new QEMU binary
>> > to it, run whatever are there in the old test.
>> 
>> Yes, but we have the words "migration-test" in the CI .yaml *today*. It
>> doesn't matter if it invokes the tests from the last release. If we
>> changed the name to "migration-foobar", then the CI job continues to
>> work up until 10.0 is released. It then breaks immediately in the first
>> commit of 10.0.50 because the previous release will now have the
>> "migration-foobar" name while the CI still calls for "migration-test".
>
> Not fair at all: nobody suggested to rename the test!
>

Right, you suggested new comand line options (level) and I simply
*mentioned* that we should pay attention to not affect that CI job. The
rest of the exchange is just me trying to clarify. I was not using this
as an argument against your idea.

>> 
>> Basically the point is that CI .yaml changes take effect immediately
>> while test cmdline changes only take effect (in CI) on the next release.
>
> Yes, but so far the "API" is the test name only (and actually not.. more
> below), and at least no path involved.  That's why I want to make sure
> we're on the same page.  So looks like at least "what tests to run by
> default", and "full path of each of the test case" can still change.
>
> Here's the "more below" part: logically if we want we can change the name
> of migration-test.  We need to teach the CI on which version of QEMU to use
> which program to test in the compat tests.  It isn't really hard (a git-tag
> -> prog-name hash), it's just unnecessary to change the test name at all.
>

Sure, we could add code around it if we wanted indeed.

>> 
>> >
>> >> 
>> >> Another point that is more of an annoyance is that the migration-test
>> >> invocation not being stable affectes debug, bisect, etc. When debugging
>> >> the recent multifd regression I already had to keep changing from
>> >> /multifd/tcp/... to /multifd/tcp/uri/... when changing QEMU versions.
>> >
>> > So I could have missed something above, and I can understand this adds
>> > burden to bisections.  However I do prefer we can change test path any way
>> > we want (even if in most cases we shouldn't ever touch them, and we should
>> > still try to not change them as frequent).  IOW we also need to consider
>> > the overhead of keeping test paths to be part of ABI as well.
>> 
>> It's annoying, that's all. Makes me 1% more grumpy.
>
> I'm not going to change any.. but keeping it a protocol is another thing.
>
> So to summarize..
>
> My plan (after adjustment, keeping the name of QEMU_TEST_FLAKY_TESTS) is to
> introduce QEMU_TEST_EXTRA_TESTS (renamed following FLAKY) and cover the
> three current "slow" tests.  Then we stick with -m for the new quick/slow,
> which maps to tcg/kvm directly.  That saves the extra --full parameter.
>
> The diff v.s. your plan is, afaict, you prefer introducing yet another
> --full parameter.
>
> Yours is good that we stick with the whole test in compat tests with no
> extra change, which is a benefit indeed.

It's the other way around. If we add --full, then compat will be !full,
so it will start running less tests. This is a "breaking change" of
sorts, but since the whole point of the series is to run less stuff in
general, then I think it's aligned with the plan.

>
> If go with my plan, the default compat behavior will start to change after
> 10.0 released.  We need to do one more step if we still prefer the wholeset
> run in compat test, which is at the start of 10.1, we send a patch to
> change compat test's parameter from none to "-m slow".
>
> Feel free to go ahead with whatever you prefer.  To me, the more important
> bit is whether we both think that one program is better than two, and also
> that means decouple host setup v.s. tests. I don't have a strong opinion
> otherwise..

Yeah, I can't deal with subverting -m slow. My initial version had this
and it didn't felt right. Maybe in 2025 I'll be a new man and we can do
differently. Let's see =)

On Fri, Dec 20, 2024 at 10:18:37AM -0500, Peter Xu wrote:
> On Thu, Dec 19, 2024 at 04:31:04PM -0300, Fabiano Rosas wrote:
> > We shouldn't change stuff that's also used by the rest of the
> > community. People know about QEMU_TEST_FLAKY_TESTS and -m slow. These
> > must continue to work the same.
> 
> I see what I overlook; it's used much more than I thought in qtest and we
> also have a CI for it.. So ok, let's keep at least QEMU_TEST_FLAKY_TESTS.
> 
> But again, I don't think it matters much even if we rename it, it means the
> flaky CI test won't run these two migration tests, but that's not the end
> of the world either, if you see what I meant.  CI relies on the normal
> tests rather than flaky tests to present.
> 
> We should be able to move in / take out FLAKY tests at will, as that's not
> what CI is really relying on.  Here renaming the macro in migration test
> almost means we take both ou.t

Side-note - QEMU_TEST_FLAKY_TESTS is something we should apply
consistently across all types of tests - unit, qtest, functional,
and across all environments - CI and local developer execution.

In recent changes to functional testing, I've set the expectation[1]
that any use of QEMU_TEST_FLAKY_TESTS *must* be accompanied by a
link to the gitlab.com/qemu/qemu-project  issue that describes
the flaky behaviour seen. We've got too many places with flaky
tests where we don't quiet remember what was flaky, so don't
know if we can remove it or not

With regards,
Daniel

[1]https://gitlab.com/qemu-project/qemu/-/blob/master/tests/functional/qemu_test/decorators.py?ref_type=heads#L42

-- 
|: 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 :|

On Fri, Dec 20, 2024 at 03:34:32PM +0000, Daniel P. Berrangé wrote:
> On Fri, Dec 20, 2024 at 10:18:37AM -0500, Peter Xu wrote:
> > On Thu, Dec 19, 2024 at 04:31:04PM -0300, Fabiano Rosas wrote:
> > > We shouldn't change stuff that's also used by the rest of the
> > > community. People know about QEMU_TEST_FLAKY_TESTS and -m slow. These
> > > must continue to work the same.
> > 
> > I see what I overlook; it's used much more than I thought in qtest and we
> > also have a CI for it.. So ok, let's keep at least QEMU_TEST_FLAKY_TESTS.
> > 
> > But again, I don't think it matters much even if we rename it, it means the
> > flaky CI test won't run these two migration tests, but that's not the end
> > of the world either, if you see what I meant.  CI relies on the normal
> > tests rather than flaky tests to present.
> > 
> > We should be able to move in / take out FLAKY tests at will, as that's not
> > what CI is really relying on.  Here renaming the macro in migration test
> > almost means we take both ou.t
> 
> Side-note - QEMU_TEST_FLAKY_TESTS is something we should apply
> consistently across all types of tests - unit, qtest, functional,
> and across all environments - CI and local developer execution.
> 
> In recent changes to functional testing, I've set the expectation[1]
> that any use of QEMU_TEST_FLAKY_TESTS *must* be accompanied by a
> link to the gitlab.com/qemu/qemu-project  issue that describes
> the flaky behaviour seen. We've got too many places with flaky
> tests where we don't quiet remember what was flaky, so don't
> know if we can remove it or not

Thanks for the heads up.  Yes it makes sense to document it explicitly in
the code.  For now we can still dig into git log, but not as easy as
inline.

Aside that, IIUC the major challenge will still be there though on the
justification itself.  It could depend on how easily reproduceable the
issue is on the developers' hosts.. If there can be some link that not only
report where does it came from, but also to verify a fix that'll be even
nicer.

-- 
Peter Xu