Steven Sistare <steven.sist...@oracle.com> writes:
> On 12/5/2024 10:23 AM, Markus Armbruster wrote:
>> Steve Sistare <steven.sist...@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.sist...@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.
Documentation needs to be clarified then.
> 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?
A quick grep for the usual parser qobject_input_visitor_new() finds
-audiodev, -blockdev, -compat, -display, and -netdev. Beware, the
latter two come with backward compatibility gunk. There's also -device
and -object, also with backward compatibility gunk.
Simple example:
JSON -compat '{"deprecated-input": "reject", "deprecated-output":
"hide"}
dotted keys -compat deprecated-input=reject,deprecated-output=hide
Slightly more interesting:
JSON -audiodev '{"id": "audiodev0", "driver": "wav", "in":
{"voices": 4}}'
dotted keys -audiodev id=audiodev0,driver=wav,in.voices=4
>> How can a management application detect that -incoming supports
>> modern?
>
> How does mgmt detect when other arguments support JSON?
Easy when an option supports it from the start: -audiodev, -blockdev,
-compat. Awkward when we extend an existing option to support it:
-display, -netdev, -device, -object.
As far as I can tell at a glance, libvirt
* Remains unaware of -display JSON arguments
* Assumes -netdev accepts JSON when QMP netdev-add supports backend type
"dgram", see commit 697e26fac66 (qemu: capabilities: Detect support
for JSON args for -netdev) v8.10.0
* Assumes -device accepts JSON when QMP device_add has feature
json-cli-hotplug, see commit 1a691fe1c84 (qemu: capabilities:
Re-enable JSON syntax for -device) v8.1.0
* Assumes -object accepts JSON when object-add supports object type
"secret", see commit f763b6e4390 (qemu: capabilities: Enable detection
of QEMU_CAPS_OBJECT_QAPIFIED) v7.2.0
In theory, such indirect probing can fall apart when somebody backports
JSON syntax *without* the thing libvirt probes for. They then get to
adjust libvirt's detection logic, too. Hasn't been an issue in practice
as far as I know.
> The presence of cpr-transfer mode implies -incoming JSON support, though
> that is indirect.
Might be good enough.
> 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.
'feature' is best used sparingly. But when it's needed, using it is
*fine*.
>> 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.
Yes, but it'll then have to support single channels both as list of one
channel object, and channel object, unlike migrate-incoming.
Syntactical differences between CLI and QMP for things that are
semantically identical add unnecessary complexity, don't you think?
>>> +
>>> 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.
I'm not sure. Peter, what do you think?
