| Message ID | 20230508153223.133081-2-tejus.gk@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Update error description whenever migration fails | expand |
Hi! On 08/05/2023 17.32, tejus.gk wrote: > There are places in the code where the migration is marked failed with > MIGRATION_STATUS_FAILED, but the failiure reason is never updated. Hence s/failiure/failure/ > libvirt doesn't know why the migration failed when it queries for it. > > Signed-off-by: tejus.gk <tejus.gk@nutanix.com> The Signed-off-by line should contain the proper name... Is "tejus.gk" really the correct spelling of your name (with only lowercase letters and a dot in it)? If not, please update the line, thanks! Thomas
"tejus.gk" <tejus.gk@nutanix.com> wrote: > There are places in the code where the migration is marked failed with > MIGRATION_STATUS_FAILED, but the failiure reason is never updated. Hence > libvirt doesn't know why the migration failed when it queries for it. > > Signed-off-by: tejus.gk <tejus.gk@nutanix.com> > --- > migration/migration.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 232e387109..87101eed5c 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1660,15 +1660,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > } else if (strstart(uri, "fd:", &p)) { > fd_start_outgoing_migration(s, p, &local_err); > } else { > - if (!(has_resume && resume)) { > - yank_unregister_instance(MIGRATION_YANK_INSTANCE); > - } Why are you removing this yank_unregister()? > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri", > + error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri", > "a valid migration protocol"); > - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, > - MIGRATION_STATUS_FAILED); > block_cleanup_parameters(); > - return; > } > > if (local_err) { > @@ -2050,7 +2044,7 @@ migration_wait_main_channel(MigrationState *ms) > * Switch from normal iteration to postcopy > * Returns non-0 on error > */ > -static int postcopy_start(MigrationState *ms) > +static int postcopy_start(MigrationState *ms, Error **errp) > { > int ret; > QIOChannelBuffer *bioc; > @@ -2165,7 +2159,7 @@ static int postcopy_start(MigrationState *ms) > */ > ret = qemu_file_get_error(ms->to_dst_file); > if (ret) { > - error_report("postcopy_start: Migration stream errored (pre package)"); > + error_setg(errp, "postcopy_start: Migration stream errored (pre package)"); > goto fail_closefb; > } > > @@ -2202,7 +2196,7 @@ static int postcopy_start(MigrationState *ms) > > ret = qemu_file_get_error(ms->to_dst_file); > if (ret) { > - error_report("postcopy_start: Migration stream errored"); > + error_setg(errp, "postcopy_start: Migration stream errored"); > migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE, > MIGRATION_STATUS_FAILED); > } > @@ -2719,6 +2713,7 @@ typedef enum { > static MigIterateState migration_iteration_run(MigrationState *s) > { > uint64_t must_precopy, can_postcopy; > + Error *local_err = NULL; > bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; > > qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); > @@ -2741,8 +2736,9 @@ static MigIterateState migration_iteration_run(MigrationState *s) > /* Still a significant amount to transfer */ > if (!in_postcopy && must_precopy <= s->threshold_size && > qatomic_read(&s->start_postcopy)) { > - if (postcopy_start(s)) { > - error_report("%s: postcopy failed to start", __func__); > + if (postcopy_start(s, &local_err)) { > + migrate_set_error(s, local_err); > + error_report_err(local_err); > } > return MIG_ITERATE_SKIP; > } > @@ -3232,8 +3228,10 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) > */ > if (migrate_postcopy_ram() || migrate_return_path()) { > if (open_return_path_on_source(s, !resume)) { > - error_report("Unable to open return-path for postcopy"); > + error_setg(&local_err, "Unable to open return-path for postcopy"); > migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); > + migrate_set_error(s, local_err); > + error_report_err(local_err); > migrate_fd_cleanup(s); > return; > } The rest of the patch looks right to me. Later, Juan.
On 08/05/23 10:19 pm, Thomas Huth wrote: > Hi! > > On 08/05/2023 17.32, tejus.gk wrote: >> There are places in the code where the migration is marked failed with >> MIGRATION_STATUS_FAILED, but the failiure reason is never updated. Hence > > s/failiure/failure/ Ack > >> libvirt doesn't know why the migration failed when it queries for it. >> >> Signed-off-by: tejus.gk <tejus.gk@nutanix.com> > > The Signed-off-by line should contain the proper name... > Is "tejus.gk" really the correct spelling of your name (with only lowercase letters and a dot in it)? If not, please update the line, thanks! > > Thomas > My bad. My git config seemed to be configure improperly, will fix in the next revision. Regards, Tejus
On 09/05/23 3:46 pm, Juan Quintela wrote: > "tejus.gk" <tejus.gk@nutanix.com> wrote: >> There are places in the code where the migration is marked failed with >> MIGRATION_STATUS_FAILED, but the failiure reason is never updated. Hence >> libvirt doesn't know why the migration failed when it queries for it. >> >> Signed-off-by: tejus.gk <tejus.gk@nutanix.com> >> --- >> migration/migration.c | 24 +++++++++++------------- >> 1 file changed, 11 insertions(+), 13 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 232e387109..87101eed5c 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -1660,15 +1660,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >> } else if (strstart(uri, "fd:", &p)) { >> fd_start_outgoing_migration(s, p, &local_err); >> } else { >> - if (!(has_resume && resume)) { >> - yank_unregister_instance(MIGRATION_YANK_INSTANCE); >> - } > > Why are you removing this yank_unregister()? As recommended by Daniel in the previous patchset, most of the stuff in this else block is duplicating the contents of the "if (local_error)" block below if (local_err) { if (!(has_resume && resume)) { yank_unregister_instance(MIGRATION_YANK_INSTANCE); } So now, after local_error gets set through error_setg(), it falls to this block where yank_unregister() will be called as before. > >> - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri", >> + error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri", >> "a valid migration protocol"); >> - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, >> - MIGRATION_STATUS_FAILED); >> block_cleanup_parameters(); >> - return; >> } >> >> if (local_err) { >> @@ -2050,7 +2044,7 @@ migration_wait_main_channel(MigrationState *ms) >> * Switch from normal iteration to postcopy >> * Returns non-0 on error >> */ >> -static int postcopy_start(MigrationState *ms) >> +static int postcopy_start(MigrationState *ms, Error **errp) >> { >> int ret; >> QIOChannelBuffer *bioc; >> @@ -2165,7 +2159,7 @@ static int postcopy_start(MigrationState *ms) >> */ >> ret = qemu_file_get_error(ms->to_dst_file); >> if (ret) { >> - error_report("postcopy_start: Migration stream errored (pre package)"); >> + error_setg(errp, "postcopy_start: Migration stream errored (pre package)"); >> goto fail_closefb; >> } >> >> @@ -2202,7 +2196,7 @@ static int postcopy_start(MigrationState *ms) >> >> ret = qemu_file_get_error(ms->to_dst_file); >> if (ret) { >> - error_report("postcopy_start: Migration stream errored"); >> + error_setg(errp, "postcopy_start: Migration stream errored"); >> migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE, >> MIGRATION_STATUS_FAILED); >> } >> @@ -2719,6 +2713,7 @@ typedef enum { >> static MigIterateState migration_iteration_run(MigrationState *s) >> { >> uint64_t must_precopy, can_postcopy; >> + Error *local_err = NULL; >> bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; >> >> qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); >> @@ -2741,8 +2736,9 @@ static MigIterateState migration_iteration_run(MigrationState *s) >> /* Still a significant amount to transfer */ >> if (!in_postcopy && must_precopy <= s->threshold_size && >> qatomic_read(&s->start_postcopy)) { >> - if (postcopy_start(s)) { >> - error_report("%s: postcopy failed to start", __func__); >> + if (postcopy_start(s, &local_err)) { >> + migrate_set_error(s, local_err); >> + error_report_err(local_err); >> } >> return MIG_ITERATE_SKIP; >> } >> @@ -3232,8 +3228,10 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) >> */ >> if (migrate_postcopy_ram() || migrate_return_path()) { >> if (open_return_path_on_source(s, !resume)) { >> - error_report("Unable to open return-path for postcopy"); >> + error_setg(&local_err, "Unable to open return-path for postcopy"); >> migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); >> + migrate_set_error(s, local_err); >> + error_report_err(local_err); >> migrate_fd_cleanup(s); >> return; >> } > > The rest of the patch looks right to me. > > Later, Juan. >
diff --git a/migration/migration.c b/migration/migration.c index 232e387109..87101eed5c 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1660,15 +1660,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, } else if (strstart(uri, "fd:", &p)) { fd_start_outgoing_migration(s, p, &local_err); } else { - if (!(has_resume && resume)) { - yank_unregister_instance(MIGRATION_YANK_INSTANCE); - } - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri", + error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol"); - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, - MIGRATION_STATUS_FAILED); block_cleanup_parameters(); - return; } if (local_err) { @@ -2050,7 +2044,7 @@ migration_wait_main_channel(MigrationState *ms) * Switch from normal iteration to postcopy * Returns non-0 on error */ -static int postcopy_start(MigrationState *ms) +static int postcopy_start(MigrationState *ms, Error **errp) { int ret; QIOChannelBuffer *bioc; @@ -2165,7 +2159,7 @@ static int postcopy_start(MigrationState *ms) */ ret = qemu_file_get_error(ms->to_dst_file); if (ret) { - error_report("postcopy_start: Migration stream errored (pre package)"); + error_setg(errp, "postcopy_start: Migration stream errored (pre package)"); goto fail_closefb; } @@ -2202,7 +2196,7 @@ static int postcopy_start(MigrationState *ms) ret = qemu_file_get_error(ms->to_dst_file); if (ret) { - error_report("postcopy_start: Migration stream errored"); + error_setg(errp, "postcopy_start: Migration stream errored"); migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE, MIGRATION_STATUS_FAILED); } @@ -2719,6 +2713,7 @@ typedef enum { static MigIterateState migration_iteration_run(MigrationState *s) { uint64_t must_precopy, can_postcopy; + Error *local_err = NULL; bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); @@ -2741,8 +2736,9 @@ static MigIterateState migration_iteration_run(MigrationState *s) /* Still a significant amount to transfer */ if (!in_postcopy && must_precopy <= s->threshold_size && qatomic_read(&s->start_postcopy)) { - if (postcopy_start(s)) { - error_report("%s: postcopy failed to start", __func__); + if (postcopy_start(s, &local_err)) { + migrate_set_error(s, local_err); + error_report_err(local_err); } return MIG_ITERATE_SKIP; } @@ -3232,8 +3228,10 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) */ if (migrate_postcopy_ram() || migrate_return_path()) { if (open_return_path_on_source(s, !resume)) { - error_report("Unable to open return-path for postcopy"); + error_setg(&local_err, "Unable to open return-path for postcopy"); migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); + migrate_set_error(s, local_err); + error_report_err(local_err); migrate_fd_cleanup(s); return; }
There are places in the code where the migration is marked failed with MIGRATION_STATUS_FAILED, but the failiure reason is never updated. Hence libvirt doesn't know why the migration failed when it queries for it. Signed-off-by: tejus.gk <tejus.gk@nutanix.com> --- migration/migration.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)