Re: [PATCH v1] tests/qtest/migration: Fix slow test dirty_limit

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Mar 26, 2026 at 01:44:05PM -0300, Fabiano Rosas wrote:
>> After the referenced commit, the incoming side doesn't exit
>> automatically after a failure. Tests that expect the destination to
>> fail should use -incoming defer, issue QMP migrate-incoming, wait for
>> the failure event and issue QMP quit.
>> 
>> Fix the dirty_limit test which wasn't updated properly.
>> 
>> Fixes: 4e8c4dda97 ("tests/qtest/migration: Force exit-on-error=false")
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> Highly likely I missed something, but could you remind me what is the
> problem before the change, and why "defer" fixes it?
>

The original issue was that on failure the incoming side exits abruptly
and leaves resources in the io/channel.c and io/task.c without freeing
and that triggers ASAN (when in use). The fix was to set
exit-on-error=false always. -incoming defer is needed because -incoming
uri has no means of setting the exit-on-error flag, we need an explicit
call to migrate-incoming to be able to set the flag.

The dirty_limit test was not updated properly because it's under g_slow
and I forgot to test it. I removed the qtest_set_expected_status(to,
EXIT_FAILURE) line, but didn't make sure exit-on-error is set to false
for this test. So it's now aborting with "expected 0, got 1".

> The current failure path in this test is:
>
>     /* Now cancel migrate and wait for dirty limit throttle switch off */
>     migrate_cancel(from);
>     wait_for_migration_status(from, "cancelled", NULL);
>
>     /* destination always fails after cancel */
>     migration_event_wait(to, "failed");
>     qtest_set_expected_status(to, EXIT_FAILURE);
>     qtest_quit(to);
>
> It waits for "failed", so IIUC after switching to exit-on-error it'll make
> sure things got gracefully shutdown before qtest_quit().  So I'm a bit lost
> on what we're fixing.
>
> PS: a nitpick is if we could always attach a "Link:" to the commit message,
> it might help a bit on knowing the original report too.
>
>> ---
>>  tests/qtest/migration/precopy-tests.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
>> index c6c8ae3004..a0e3ff0547 100644
>> --- a/tests/qtest/migration/precopy-tests.c
>> +++ b/tests/qtest/migration/precopy-tests.c
>> @@ -1070,11 +1070,10 @@ static void test_dirty_limit(char *name, MigrateCommon *args)
>>      args->start.hide_stderr = true;
>>      args->start.use_dirty_ring = true;
>>  
>> -    args->listen_uri = uri;
>>      args->connect_uri = uri;
>>  
>>      /* Start src, dst vm */
>> -    if (migrate_start(&from, &to, args->listen_uri, &args->start)) {
>> +    if (migrate_start(&from, &to, "defer", &args->start)) {
>>          return;
>>      }
>>  
>> @@ -1082,6 +1081,7 @@ static void test_dirty_limit(char *name, MigrateCommon *args)
>>      migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value);
>>  
>>      /* Start migrate */
>> +    migrate_incoming_qmp(to, args->connect_uri, NULL, "{}");
>>      migrate_qmp(from, to, args->connect_uri, NULL, "{}");
>>  
>>      /* Wait for dirty limit throttle begin */
>> -- 
>> 2.51.0
>>