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

[Fabiano Rosas <farosas@suse.de>]

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.