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

[Peter Xu]

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