[RFC v2 0/1] migration: Update error description whenever migration fails

[RFC v2 0/1] migration: Update error description whenever migration fails

[tejus.gk]

Hi everyone, 

Thanks for the reviews. This is the v2 patchset based on the reviews 
recieved for the previous one. 

Links to previous patchsets:
v1: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg00868.html


tejus.gk (1):
  migration: Update error description whenever migration fails

 migration/migration.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

-- 
2.22.3

[RFC v2 1/1] migration: Update error description whenever migration fails

[tejus.gk]

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);
-        }
-        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;
         }
-- 
2.22.3

Re: [RFC v2 1/1] migration: Update error description whenever migration fails

[Thomas Huth]

  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

Re: [RFC v2 1/1] migration: Update error description whenever migration fails

[Juan Quintela]

"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.

Re: [RFC v2 1/1] migration: Update error description whenever migration fails

[Tejus GK]

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

Re: [RFC v2 1/1] migration: Update error description whenever migration fails

[Tejus GK]

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.
>