Re: [PATCH 3/3] tests/qtest: Re-enable multifd cancel test

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Jun 07, 2023 at 10:27:15AM +0200, Juan Quintela wrote:
>> Fabiano Rosas <farosas@suse.de> wrote:
>> > We've found the source of flakiness in this test, so re-enable it.
>> >
>> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> > ---
>> >  tests/qtest/migration-test.c | 10 ++--------
>> >  1 file changed, 2 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> > index b0c355bbd9..800ad23b75 100644
>> > --- a/tests/qtest/migration-test.c
>> > +++ b/tests/qtest/migration-test.c
>> > @@ -2778,14 +2778,8 @@ int main(int argc, char **argv)
>> >      }
>> >      qtest_add_func("/migration/multifd/tcp/plain/none",
>> >                     test_multifd_tcp_none);
>> > -    /*
>> > -     * This test is flaky and sometimes fails in CI and otherwise:
>> > -     * don't run unless user opts in via environment variable.
>> > -     */
>> > -    if (getenv("QEMU_TEST_FLAKY_TESTS")) {
>> > -        qtest_add_func("/migration/multifd/tcp/plain/cancel",
>> > -                       test_multifd_tcp_cancel);
>> > -    }
>> > +    qtest_add_func("/migration/multifd/tcp/plain/cancel",
>> > +                   test_multifd_tcp_cancel);
>> >      qtest_add_func("/migration/multifd/tcp/plain/zlib",
>> >                     test_multifd_tcp_zlib);
>> >  #ifdef CONFIG_ZSTD
>> 
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> 
>> 
>> There was another failure with migration test that I will post during
>> the rest of the day.  It needs both to get it right.
>
> This one didn't yet land upstream.  I'm not sure, but maybe Juan was saying
> about this change:
>
>         commit d2026ee117147893f8d80f060cede6d872ecbd7f
>         Author: Juan Quintela <quintela@trasno.org>
>         Date:   Wed Apr 26 12:20:36 2023 +0200
>
>         multifd: Fix the number of channels ready

That's not it. It was something in the test itself around the fact that
we use two sets of: from/to. There was supposed to be a situation where
we'd start 'to2' while 'to' was still running and that would cause
issues (possibly with sockets).

I think what might have happened is that someone merged a fix through
another tree and Juan didn't notice. I think this is the one:

  commit f2d063e61ee2026700ab44bef967f663e976bec8
  Author: Xuzhou Cheng <xuzhou.cheng@windriver.com>
  Date:   Fri Oct 28 12:57:32 2022 +0800
  
      tests/qtest: migration-test: Make sure QEMU process "to" exited after migration is canceled
      
      Make sure QEMU process "to" exited before launching another target
      for migration in the test_multifd_tcp_cancel case.
      
      Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
      Signed-off-by: Bin Meng <bin.meng@windriver.com>
      Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
      Message-Id: <20221028045736.679903-8-bin.meng@windriver.com>
      Signed-off-by: Thomas Huth <thuth@redhat.com>

> Fabiano, did you try to reproduce multifd-cancel with current master?  I'm
> wondering whether this test has already been completely fixed, then maybe
> we can pick up this patch now.

Yes, let's merge it. I have kept it enabled during testing of all of the
recent race conditions we've debugged and haven't seen it fail. Current
master also looks fine.