Re: [PATCH] migration: do not exit on incoming failure

[Vladimir Sementsov-Ogievskiy]

On 18.04.24 17:27, Fabiano Rosas wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>
> > We do set MIGRATION_FAILED state, but don't give a chance to
> > orchestrator to query migration state and get the error.
> >
> > Let's report an error through QAPI like we do on outgoing migration.
> >
> > migration-test is updated correspondingly.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > ---
> >
> > Doubt: is exiting on failure a contract? Will this commit break
> > something in Libvirt? Finally, could we just change the logic, or I need
> > and additional migration-parameter for new behavior?
>
> It seems we depend on the non-zero value:
>
>    4aead69241 ("migration: reflect incoming failure to shell")
>    Author: Eric Blake <eblake@redhat.com>
>    Date:   Tue Apr 16 15:50:41 2013 -0600
>    
>        migration: reflect incoming failure to shell
>        
>        Management apps like libvirt don't know to pay attention to
>        stderr unless there is a non-zero exit status.
>        
>        * migration.c (process_incoming_migration_co): Exit with non-zero
>        status on failure.
>        
>        Signed-off-by: Eric Blake <eblake@redhat.com>
>        Message-id: 1366149041-626-1-git-send-email-eblake@redhat.com
>        Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> One idea would be to plumb the s->error somehow through
> migration_shutdown() and allow qemu_cleanup() to change the status
> value.

The idea is not to exit at all, and wait for 'quit' QMP command to exit. But I 
agree with Daniel that new behavior is good only for -incoming defer.

> >   migration/migration.c           | 22 +++++++---------------
> >   tests/qtest/migration-helpers.c | 13 ++++++++++---
> >   tests/qtest/migration-helpers.h |  3 ++-
> >   tests/qtest/migration-test.c    | 14 +++++++-------
> >   4 files changed, 26 insertions(+), 26 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 86bf76e925..3c203e767d 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -738,11 +738,12 @@ process_incoming_migration_co(void *opaque)
> >       MigrationIncomingState *mis = migration_incoming_get_current();
> >       PostcopyState ps;
> >       int ret;
> > +    Error *local_err = NULL;
> >   
> >       assert(mis->from_src_file);
> >   
> >       if (compress_threads_load_setup(mis->from_src_file)) {
> > -        error_report("Failed to setup decompress threads");
> > +        error_setg(&local_err, "Failed to setup decompress threads");
> >           goto fail;
> >       }
> >   
> > @@ -779,32 +780,23 @@ process_incoming_migration_co(void *opaque)
> >       }
> >   
> >       if (ret < 0) {
> > -        MigrationState *s = migrate_get_current();
> > -
> > -        if (migrate_has_error(s)) {
> > -            WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
> > -                error_report_err(s->error);
> > -            }
> > -        }
> > -        error_report("load of migration failed: %s", strerror(-ret));
> > +        error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
> >           goto fail;
> >       }
> >   
> >       if (colo_incoming_co() < 0) {
> > +        error_setg(&local_err, "colo incoming failed");
> >           goto fail;
> >       }
> >   
> >       migration_bh_schedule(process_incoming_migration_bh, mis);
> >       return;
> >   fail:
> > +    migrate_set_error(migrate_get_current(), local_err);
> > +    error_report_err(local_err);
>
> This will report an different error from the QMP one if s->error happens
> to be already set. Either use s->error here or prepend the "load of
> migration..." error to the s->error above.

I had another idea: first, modify migrate_set_error so that it always prints 
the error. This way we should not care to print it here.

> >       migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> >                         MIGRATION_STATUS_FAILED);
> > -    qemu_fclose(mis->from_src_file);
> > -
> > -    multifd_recv_cleanup();
> > -    compress_threads_load_cleanup();
> > -
> > -    exit(EXIT_FAILURE);
> > +    migration_incoming_state_destroy();
> >   }
> >   
> >   /**
> > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> > index e451dbdbed..91c13bd566 100644
> > --- a/tests/qtest/migration-helpers.c
> > +++ b/tests/qtest/migration-helpers.c
> > @@ -211,7 +211,8 @@ void wait_for_migration_complete(QTestState *who)
> >       wait_for_migration_status(who, "completed", NULL);
> >   }
> >   
> > -void wait_for_migration_fail(QTestState *from, bool allow_active)
> > +void wait_for_migration_fail(QTestState *from, bool allow_active,
> > +                             bool is_incoming)
> >   {
> >       g_test_timer_start();
> >       QDict *rsp_return;
> > @@ -236,8 +237,14 @@ void wait_for_migration_fail(QTestState *from, bool 
> > allow_active)
> >       /* Is the machine currently running? */
> >       rsp_return = qtest_qmp_assert_success_ref(from,
> >                                                 "{ 'execute': 'query-status' 
> > }");
> > -    g_assert(qdict_haskey(rsp_return, "running"));
> > -    g_assert(qdict_get_bool(rsp_return, "running"));
> > +    if (is_incoming) {
> > +        if (qdict_haskey(rsp_return, "running")) {
> > +            g_assert(!qdict_get_bool(rsp_return, "running"));
> > +        }
> > +    } else {
> > +        g_assert(qdict_haskey(rsp_return, "running"));
> > +        g_assert(qdict_get_bool(rsp_return, "running"));
> > +    }
> >       qobject_unref(rsp_return);
> >   }
> >   
> > diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> > index 3bf7ded1b9..7bd07059ae 100644
> > --- a/tests/qtest/migration-helpers.h
> > +++ b/tests/qtest/migration-helpers.h
> > @@ -46,7 +46,8 @@ void wait_for_migration_status(QTestState *who,
> >   
> >   void wait_for_migration_complete(QTestState *who);
> >   
> > -void wait_for_migration_fail(QTestState *from, bool allow_active);
> > +void wait_for_migration_fail(QTestState *from, bool allow_active,
> > +                             bool is_incoming);
> >   
> >   char *find_common_machine_version(const char *mtype, const char *var1,
> >                                     const char *var2);
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index 1d2cee87ea..e00b755f05 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -1670,7 +1670,7 @@ static void test_baddest(void)
> >           return;
> >       }
> >       migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
> > -    wait_for_migration_fail(from, false);
> > +    wait_for_migration_fail(from, false, false);
> >       test_migrate_end(from, to, false);
> >   }
> >   
> > @@ -1781,10 +1781,10 @@ static void test_precopy_common(MigrateCommon *args)
> >   
> >       if (args->result != MIG_TEST_SUCCEED) {
> >           bool allow_active = args->result == MIG_TEST_FAIL;
> > -        wait_for_migration_fail(from, allow_active);
> > +        wait_for_migration_fail(from, allow_active, false);
> >   
> >           if (args->result == MIG_TEST_FAIL_DEST_QUIT_ERR) {
> > -            qtest_set_expected_status(to, EXIT_FAILURE);
> > +            wait_for_migration_fail(to, true, true);
> >           }
> >       } else {
> >           if (args->live) {
> > @@ -2571,8 +2571,8 @@ static void do_test_validate_uuid(MigrateStart *args, 
> > bool should_fail)
> >       migrate_qmp(from, uri, "{}");
> >   
> >       if (should_fail) {
> > -        qtest_set_expected_status(to, EXIT_FAILURE);
> > -        wait_for_migration_fail(from, true);
> > +        wait_for_migration_fail(to, true, true);
> > +        wait_for_migration_fail(from, true, false);
> >       } else {
> >           wait_for_migration_complete(from);
> >       }
> > @@ -3047,8 +3047,8 @@ static void test_multifd_tcp_cancel(void)
> >       migrate_cancel(from);
> >   
> >       /* Make sure QEMU process "to" exited */
> > -    qtest_set_expected_status(to, EXIT_FAILURE);
> > -    qtest_wait_qemu(to);
> > +    wait_for_migration_fail(to, true, true);
> > +    qtest_quit(to);
> >   
> >       args = (MigrateStart){
> >           .only_target = true,

--
Best regards,
Vladimir