Re: [PATCH] migration: add optional capabilities field to migrate and migrate-incoming

[Fabiano Rosas <farosas@suse.de>]

Trieu Huynh <vikingtc4@gmail.com> writes:

> From: Trieu Huynh <vikingtc4@gmail.com>
>
> When invoking migration, user must call migrate-set-capabilities as a
> separate QMP command before calling migrate or migrate-incoming.
>
> Add an optional 'capabilities' field to both commands. When provided,
> the capabilities are applied before the migration starts, in the same
> order as a migrate-set-capabilities call would apply them. Existing
> callers that do not pass the field are unaffected.
>
> This is particularly useful, eg. for snapshot-load workflows where a
> single migrate-incoming call should express the full migration
> configuration:
>
>   { "execute": "migrate-incoming",
>     "arguments": {
>       "uri": "file:/tmp/snapshot",
>       "capabilities": [{"capability": "mapped-ram", "state": true}]
>     }}
>
> Internal callers (HMP migrate-incoming, vl.c -incoming) pass
> has_capabilities=false so their behaviour is unchanged.
>
> Related-to: https://wiki.qemu.org/ToDo/LiveMigration#Allow_QMP_command_%22migrate[_incoming]%22_to_take_capabilities_and_parameters
>

Hi!

This is a change that comes with a lot of implications from the
maintenance perspective. We have an ongoing attempt at moving all
configurable migration options to be arguments of the two main migration
commands, but it seems the consensus is diverging a bit, see:

https://lore.kernel.org/r/20251215220041.12657-1-farosas@suse.de
https://lore.kernel.org/r/aURtVveE88n31AN_@x1.local

The main point is whether this change in design direction is worth the
maintenance burden. I do think it is, but it's not so simple, we'll have
practically another set of commands to test and document, ensure there's
no bad interactions with the old way of setting options, etc. Then,
there's the entire aspect of deprecating old commands, informing users,
switching to the new commands, etc.

Peter also mentions in the thread I linked that we haven't seen a real
need for this type of feature. There's the general consensus (I think)
that passing options via the migrate commands is a good design approach,
but that doesn't mean we currently _need_ to do it, there is nothing
technically requiring it. In which case the "more stuff to maintain"
argument becomes stronger.

Your patch makes me realise that we could divide the whole work in
steps, first add capabilities argument to migrate, then add parameters,
then deprecate set-capabilities, then deprecate set-parameters. This
would definitely ease the burden of the change, but we might get stuck
in the middle of the process and end up with an inconsistent interface.

Let's hear what others think on whether a simplified approach like this
is better and whether the whole endevour is still desirable.

(from my side, I'm in favor of continuing with the work I started. Time
investment has been a challenge recently, maybe Trieu could help with
it. To be clear, I'm definitely not against other approaches, as long as
we all agree and it gets done.)

> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
> ---
>  migration/migration-hmp-cmds.c |  4 ++--
>  migration/migration.c          | 19 +++++++++++++++++++
>  qapi/migration.json            | 12 ++++++++++++
>  system/vl.c                    |  3 ++-
>  4 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 0a193b8f54..32c338103d 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -522,7 +522,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
>      }
>      QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
>  
> -    qmp_migrate_incoming(NULL, true, caps, true, false, &err);
> +    qmp_migrate_incoming(NULL, true, caps, false, NULL, true, false, &err);
>      qapi_free_MigrationChannelList(caps);
>  
>  end:
> @@ -829,7 +829,7 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>      }
>      QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
>  
> -    qmp_migrate(NULL, true, caps, true, resume, &err);
> +    qmp_migrate(NULL, true, caps, false, NULL, true, resume, &err);
>      if (hmp_handle_error(mon, err)) {
>          return;
>      }
> diff --git a/migration/migration.c b/migration/migration.c
> index 5c9aaa6e58..eb1a734dd9 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1752,6 +1752,8 @@ void migrate_del_blocker(Error **reasonp)
>  
>  void qmp_migrate_incoming(const char *uri, bool has_channels,
>                            MigrationChannelList *channels,
> +                          bool has_capabilities,
> +                          MigrationCapabilityStatusList *capabilities,
>                            bool has_exit_on_error, bool exit_on_error,
>                            Error **errp)
>  {
> @@ -1772,6 +1774,14 @@ void qmp_migrate_incoming(const char *uri, bool has_channels,
>          return;
>      }
>  
> +    if (has_capabilities) {
> +        qmp_migrate_set_capabilities(capabilities, errp);
> +        if (*errp) {
> +            yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> +            return;
> +        }
> +    }
> +
>      mis->exit_on_error =
>          has_exit_on_error ? exit_on_error : INMIGRATE_DEFAULT_EXIT_ON_ERROR;
>  
> @@ -2020,6 +2030,8 @@ static gboolean migration_connect_outgoing_cb(QIOChannel *channel,
>  
>  void qmp_migrate(const char *uri, bool has_channels,
>                   MigrationChannelList *channels,
> +                 bool has_capabilities,
> +                 MigrationCapabilityStatusList *capabilities,
>                   bool has_resume, bool resume, Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
> @@ -2036,6 +2048,13 @@ void qmp_migrate(const char *uri, bool has_channels,
>          return;
>      }
>  
> +    if (has_capabilities) {
> +        qmp_migrate_set_capabilities(capabilities, errp);
> +        if (*errp) {
> +            return;
> +        }
> +    }
> +
>      if (!migrate_prepare(s, has_resume && resume, errp)) {
>          /* Error detected, put into errp */
>          return;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 7134d4ce47..58d934f8fe 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1388,6 +1388,11 @@
>  # @channels: list of migration stream channels with each stream in the
>  #     list connected to a destination interface endpoint.
>  #
> +# @capabilities: list of migration capabilities to set before
> +#     starting the migration.  Equivalent to calling
> +#     `migrate-set-capabilities` before `migrate`, but more
> +#     convenient.
> +#
>  # @resume: when set, use the new uri/channels specified to resume
>  #     paused postcopy migration.  This flag should only be used if
>  #     the previous postcopy migration was interrupted.  The command
> @@ -1453,6 +1458,7 @@
>  { 'command': 'migrate',
>    'data': {'*uri': 'str',
>             '*channels': [ 'MigrationChannel' ],
> +           '*capabilities': ['MigrationCapabilityStatus'],
>             '*resume': 'bool' } }
>  
>  ##
> @@ -1467,6 +1473,11 @@
>  # @channels: list of migration stream channels with each stream in the
>  #     list connected to a destination interface endpoint.
>  #
> +# @capabilities: list of migration capabilities to set before
> +#     starting the migration.  Equivalent to calling
> +#     `migrate-set-capabilities` before `migrate-incoming`, but more
> +#     convenient.
> +#
>  # @exit-on-error: Exit on incoming migration failure.  Default true.
>  #     When set to false, the failure triggers a
>  #     :qapi:event:`MIGRATION` event, and error details could be
> @@ -1525,6 +1536,7 @@
>  { 'command': 'migrate-incoming',
>               'data': {'*uri': 'str',
>                        '*channels': [ 'MigrationChannel' ],
> +                      '*capabilities': ['MigrationCapabilityStatus'],
>                        '*exit-on-error': 'bool' } }
>  
>  ##
> diff --git a/system/vl.c b/system/vl.c
> index 246623b319..ed569d75e7 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2826,7 +2826,8 @@ void qmp_x_exit_preconfig(Error **errp)
>                  g_new0(MigrationChannelList, 1);
>  
>              channels->value = incoming_channels[MIGRATION_CHANNEL_TYPE_MAIN];
> -            qmp_migrate_incoming(NULL, true, channels, true, true, &local_err);
> +            qmp_migrate_incoming(NULL, true, channels, false, NULL, true,
> +                                 true, &local_err);
>              if (local_err) {
>                  error_reportf_err(local_err, "-incoming %s: ", incoming);
>                  exit(1);