[PATCH v2 0/2] Field 'reason' for MIGRATION event

Roman Khapov posted 2 patches 9 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240215122759.1438581-1-rkhapov@yandex-team.ru
Maintainers: Hailiang Zhang <zhanghailiang@xfusion.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>
migration/colo.c      |   6 +-
migration/migration.c | 128 ++++++++++++++++++++++++++++--------------
migration/migration.h |   5 +-
migration/multifd.c   |  10 ++--
migration/savevm.c    |  24 ++++----
qapi/migration.json   |   3 +-
6 files changed, 112 insertions(+), 64 deletions(-)
[PATCH v2 0/2] Field 'reason' for MIGRATION event
Posted by Roman Khapov 9 months, 2 weeks ago
This is resending of series 20240215082659.1378342-1-rkhapov@yandex-team.ru,
where patch subjects numbers were broken in patch 2/2.

Sometimes, when migration fails, it is hard to find out
the cause of the problems: you have to grep qemu logs.
At the same time, there is MIGRATION event, which looks like
suitable place to hold such error descriptions.

To handle situation like this (maybe one day it will be useful
for other MIGRATION statuses to have additional 'reason' strings),
the general optional field 'reason' can be added.

The series proposes next changes:

1. Adding optional 'reason' field of type str into
   qapi/migration.json MIGRATION event

2. Passing some error description as reason for every place, which
   sets migration state to MIGRATION_STATUS_FAILED

After the series, MIGRATION event will looks like this:
{"execute": "qmp_capabilities"}
{"return": {}}
{"event": "MIGRATION", "data": {"status": "setup"}}
{"event": "MIGRATION", "data": {"status": "failed", "reason": "Failed to connect to '/tmp/sock.sock': No such file or directory"}}

Roman Khapov (2):
  qapi/migration.json: add reason to MIGRATION event
  migration: add error reason for failed MIGRATION events

 migration/colo.c      |   6 +-
 migration/migration.c | 128 ++++++++++++++++++++++++++++--------------
 migration/migration.h |   5 +-
 migration/multifd.c   |  10 ++--
 migration/savevm.c    |  24 ++++----
 qapi/migration.json   |   3 +-
 6 files changed, 112 insertions(+), 64 deletions(-)

-- 
2.34.1
Re: [PATCH v2 0/2] Field 'reason' for MIGRATION event
Posted by Fabiano Rosas 9 months, 1 week ago
Roman Khapov <rkhapov@yandex-team.ru> writes:

Hi Roman,

> This is resending of series 20240215082659.1378342-1-rkhapov@yandex-team.ru,
> where patch subjects numbers were broken in patch 2/2.
>
> Sometimes, when migration fails, it is hard to find out
> the cause of the problems: you have to grep qemu logs.
> At the same time, there is MIGRATION event, which looks like
> suitable place to hold such error descriptions.

query-migrate after the event is received should be enough for giving
you the failure reason. We have that in error-desc. See commit
c94143e587 ("migration: Display error in query-migrate irrelevant of
status").

>
> To handle situation like this (maybe one day it will be useful
> for other MIGRATION statuses to have additional 'reason' strings),

I find it unlikely. There's no "reason" for making progress except
that's how things work. Only the exceptional (i.e. failure) statuses
would have a reason. Today that's FAILED only, maybe also
POSTCOPY_PAUSED.

> the general optional field 'reason' can be added.
>
> The series proposes next changes:
>
> 1. Adding optional 'reason' field of type str into
>    qapi/migration.json MIGRATION event
>
> 2. Passing some error description as reason for every place, which
>    sets migration state to MIGRATION_STATUS_FAILED
>
> After the series, MIGRATION event will looks like this:
> {"execute": "qmp_capabilities"}
> {"return": {}}
> {"event": "MIGRATION", "data": {"status": "setup"}}
> {"event": "MIGRATION", "data": {"status": "failed", "reason": "Failed to connect to '/tmp/sock.sock': No such file or directory"}}
>
> Roman Khapov (2):
>   qapi/migration.json: add reason to MIGRATION event
>   migration: add error reason for failed MIGRATION events
>
>  migration/colo.c      |   6 +-
>  migration/migration.c | 128 ++++++++++++++++++++++++++++--------------
>  migration/migration.h |   5 +-
>  migration/multifd.c   |  10 ++--
>  migration/savevm.c    |  24 ++++----
>  qapi/migration.json   |   3 +-
>  6 files changed, 112 insertions(+), 64 deletions(-)

Please remember to run make check:

380/383 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test ERROR
104.77s killed by signal 6 SIGABRT
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
stderr: Broken pipe ../tests/qtest/libqtest.c:204: kill_qemu() detected
QEMU death from signal 11 (Segmentation fault) (core dumped)


Most likely one of the new error_setg has broken postcopy recovery. Some
of those paths are not intended to trigger cleanup.
Re: [PATCH v2 0/2] Field 'reason' for MIGRATION event
Posted by Markus Armbruster 9 months, 1 week ago
Fabiano Rosas <farosas@suse.de> writes:

> Roman Khapov <rkhapov@yandex-team.ru> writes:
>
> Hi Roman,
>
>> This is resending of series 20240215082659.1378342-1-rkhapov@yandex-team.ru,
>> where patch subjects numbers were broken in patch 2/2.
>>
>> Sometimes, when migration fails, it is hard to find out
>> the cause of the problems: you have to grep qemu logs.
>> At the same time, there is MIGRATION event, which looks like
>> suitable place to hold such error descriptions.
>
> query-migrate after the event is received should be enough for giving
> you the failure reason. We have that in error-desc. See commit
> c94143e587 ("migration: Display error in query-migrate irrelevant of
> status").
>
>>
>> To handle situation like this (maybe one day it will be useful
>> for other MIGRATION statuses to have additional 'reason' strings),
>
> I find it unlikely. There's no "reason" for making progress except
> that's how things work. Only the exceptional (i.e. failure) statuses
> would have a reason. Today that's FAILED only, maybe also
> POSTCOPY_PAUSED.

I can't see a need for the proposed feature then.

>> the general optional field 'reason' can be added.

[...]