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

[Thomas Huth <thuth@redhat.com>]

On 09/01/2024 03.12, Peter Xu wrote:
> On Mon, Jan 08, 2024 at 11:26:04AM -0300, Fabiano Rosas wrote:
>> 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>
> 
> Hmm, i see.

Sorry for that :-( Maybe it's better if we remove the migration-test from 
the qtest section in MAINTAINERS? Since the migration test is very well 
maintained already, there's IMHO no need for picking up the patches via the 
qtest tree, so something like this should prevent these problems:

diff --git a/MAINTAINERS b/MAINTAINERS
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3269,6 +3269,7 @@ F: tests/qtest/
  F: docs/devel/qgraph.rst
  F: docs/devel/qtest.rst
  X: tests/qtest/bios-tables-test*
+X: tests/qtest/migration-*

  Device Fuzzing
  M: Alexander Bulekov <alxndr@bu.edu>

(as you can see, we're doing it in a similar way for the bios tables test 
already)

If you agree, I can send out a proper patch for this later today.

  Thomas