Re: [PATCH V4 09/19] migration: incoming channel

[Steven Sistare <steven.sistare@oracle.com>]

On 12/5/2024 10:23 AM, Markus Armbruster wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
> 
>> Extend the -incoming option to allow an @MigrationChannel to be specified.
>> This allows channels other than 'main' to be described on the command
>> line, which will be needed for CPR.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> 
> [...]
> 
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 02b9118..fab50ce 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -4937,10 +4937,17 @@ DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
>>       "-incoming exec:cmdline\n" \
>>       "                accept incoming migration on given file descriptor\n" \
>>       "                or from given external command\n" \
>> +    "-incoming @MigrationChannel\n" \
>> +    "                accept incoming migration on the channel\n" \
>>       "-incoming defer\n" \
>>       "                wait for the URI to be specified via migrate_incoming\n",
>>       QEMU_ARCH_ALL)
>>   SRST
>> +The -incoming option specifies the migration channel for an incoming
>> +migration.  It may be used multiple times to specify multiple
>> +migration channel types.
> 
> Really?  If I understand the code below correctly, the last -incoming
> wins, and any previous ones are silently ignored.

See patch "cpr-channel", where the cpr channel is saved separately.
Last wins, per channel type.
I did this to preserve the current behavior of -incoming in which last wins.

qemu_start_incoming_migration will need modification if more types are added.

>>                             The channel type is specified in @MigrationChannel,
>> +and is 'main' for all other forms of -incoming.
>> +
>>   ``-incoming tcp:[host]:port[,to=maxport][,ipv4=on|off][,ipv6=on|off]``
>>     \
>>   ``-incoming rdma:host:port[,ipv4=on|off][,ipv6=on|off]``
>> @@ -4960,6 +4967,16 @@ SRST
>>       Accept incoming migration as an output from specified external
>>       command.
>>   
>> +``-incoming @MigrationChannel``
>> +    Accept incoming migration on the channel.  See the QAPI documentation
>> +    for the syntax of the @MigrationChannel data element.  For example:
>> +    ::
> 
> I get what you're trying to express, but there's no precedence for
> referring to QAPI types like @TypeName in option documentation.  But
> let's ignore this until after we nailed down the actual interface, on
> which I have questions below.

Ack.

>> +
>> +        -incoming '{"channel-type": "main",
>> +                    "addr": { "transport": "socket",
>> +                              "type": "unix",
>> +                              "path": "my.sock" }}'
>> +
>>   ``-incoming defer``
>>       Wait for the URI to be specified via migrate\_incoming. The monitor
>>       can be used to change settings (such as migration parameters) prior
>> diff --git a/system/vl.c b/system/vl.c
>> index 4151a79..2c24c60 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -123,6 +123,7 @@
>>   #include "qapi/qapi-visit-block-core.h"
>>   #include "qapi/qapi-visit-compat.h"
>>   #include "qapi/qapi-visit-machine.h"
>> +#include "qapi/qapi-visit-migration.h"
>>   #include "qapi/qapi-visit-ui.h"
>>   #include "qapi/qapi-commands-block-core.h"
>>   #include "qapi/qapi-commands-migration.h"
>> @@ -159,6 +160,7 @@ typedef struct DeviceOption {
>>   static const char *cpu_option;
>>   static const char *mem_path;
>>   static const char *incoming;
>> +static MigrationChannelList *incoming_channels;
>>   static const char *loadvm;
>>   static const char *accelerators;
>>   static bool have_custom_ram_size;
>> @@ -1821,6 +1823,35 @@ static void object_option_add_visitor(Visitor *v)
>>       QTAILQ_INSERT_TAIL(&object_opts, opt, next);
>>   }
>>   
>> +static void incoming_option_parse(const char *str)
>> +{
>> +    MigrationChannel *channel;
>> +
>> +    if (str[0] == '{') {
>> +        QObject *obj = qobject_from_json(str, &error_fatal);
>> +        Visitor *v = qobject_input_visitor_new(obj);
>> +
>> +        qobject_unref(obj);
>> +        visit_type_MigrationChannel(v, "channel", &channel, &error_fatal);
>> +        visit_free(v);
>> +    } else if (!strcmp(str, "defer")) {
>> +        channel = NULL;
>> +    } else {
>> +        migrate_uri_parse(str, &channel, &error_fatal);
>> +    }
>> +
>> +    /* New incoming spec replaces the previous */
>> +
>> +    if (incoming_channels) {
>> +        qapi_free_MigrationChannelList(incoming_channels);
>> +    }
>> +    if (channel) {
>> +        incoming_channels = g_new0(MigrationChannelList, 1);
>> +        incoming_channels->value = channel;
>> +    }
>> +    incoming = str;
>> +}
> 
> @incoming is set to @optarg.
> 
> @incoming_channels is set to a MigrationChannelList of exactly one
> element, parsed from @incoming.  Except when @incoming is "defer", then
> @incoming_channels is set to null.
> 
> @incoming is only ever used as a flag.  Turn it into a bool?

The remembered incoming specifier is also used in an error message in
qmp_x_exit_preconfig:
     error_reportf_err(local_err, "-incoming %s: ", incoming);

> Oh, wait...  see my comment on the next hunk.
> 
> Option -incoming resembles QMP command migrate-incoming.  Differences:
> 
> * migrate-incoming keeps legacy URI and modern argument separate: there
>    are two named arguments, and exactly one of them must be passed.
>    -incoming overloads them: if @optarg starts with '{', it's modern,
>    else legacy URI.
> 
>    Because of that, -incoming *only* supports JSON syntax for modern, not
>    dotted keys.  Other JSON-capable arguments support both.

Not sure I follow.
Could you give me a dotted key example for a JSON-capable argument?
Do we care about dotted key for incoming, given the user can specify
a simple legacy URI?

>    How can a management application detect that -incoming supports
>    modern?

How does mgmt detect when other arguments support JSON?

The presence of cpr-transfer mode implies -incoming JSON support, though
that is indirect.

We could add a feature to the migrate-incoming command, like json-cli
for device_add.  Seems like overkill though.  'feature' is little used,
except for unstable and deprecated.

>    Sure overloading -incoming this way is a good idea?
> 
> * migrate-incoming takes a list of channels, currently restricted to a
>    single channel.  -incoming takes a channel.  If we lift the
>    restriction, -incoming syntax will become even messier: we'll have to
>    additionally overload list of channel.
> 
>    Should -incoming take a list from the start, like migrate-incoming
>    does?

That was my first try.  However, to support the equivalent of '-incoming deferred',
we need to add an 'defer' key to the channel, and when defer is true, the other
keys that are currently mandatory must be omitted.  The tweaks to the implementation
and specification seemed not worth worth it.

If we want -incoming to also support a channel list in the future, we can simply
check for an initial '[' token.

>> +
>>   static void object_option_parse(const char *str)
>>   {
>>       QemuOpts *opts;
>> @@ -2730,7 +2761,7 @@ void qmp_x_exit_preconfig(Error **errp)
>>       if (incoming) {
>>           Error *local_err = NULL;
>>           if (strcmp(incoming, "defer") != 0) {
>> -            qmp_migrate_incoming(incoming, false, NULL, true, true,
>> +            qmp_migrate_incoming(NULL, true, incoming_channels, true, true,
>>                                    &local_err);
> 
> You move the parsing of legacy URI from within qmp_migrate_incoming()
> into incoming_option_parse().
> 
> The alternative is not to parse it in incoming_option_parse(), but pass
> it to qmp_migrate_incoming() like this:
> 
>                 qmp_migrate_incoming(incoming, !incoming, incoming_channels,
>                                      true, true, &local_err);

Sure, I can tweak that, but I need to define an additional incoming_uri variable:
     qmp_migrate_incoming(incoming_uri, !!incoming_channels, incoming_channels, ...

Only one of incoming_uri and incoming_channels can be non-NULL (checked in
qemu_start_incoming_migration).

Would you prefer I continue down this path, or revert to the previous -cpr-uri
option?  I made this change to make the incoming interface look more like the
V4 outgoing interface, in which the user adds a cpr channel to the migrate command
channels.

- Steve