Re: [PATCH v2 19/22] tests/qtest/migration: Add migration-test-smoke

[Peter Xu <peterx@redhat.com>]

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