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

[Fabiano Rosas]

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 =)