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

[Peter Xu <peterx@redhat.com>]

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