[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V4 09/19] migration: incoming channel
|
From: |
Steven Sistare |
|
Subject: |
Re: [PATCH V4 09/19] migration: incoming channel |
|
Date: |
Wed, 11 Dec 2024 13:58:36 -0500 |
|
User-agent: |
Mozilla Thunderbird |
On 12/9/2024 7:12 AM, Markus Armbruster wrote:
Steven Sistare <steven.sistare@oracle.com> writes:
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.
Documentation needs to be clarified then.
Maybe. Depends whether we want/need to take a stand on whether the current
behavior is an accident of the implementation, or part of the specification.
The current behavior is not documented.
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
Thank you (and for the correction to qobject_input_visitor_new_str). I did not
grok that this visitor handles both json and dotted keys, as so far I had only
seen examples with keys but not dotted keys. I can easily support those as
well
as the legacy uri, by returning a new parameter from migrate_uri_parse
indicating
that no uri is recognized (as opposed to recognized, but with some other error):
if (!strcmp(str, "defer")) {
channel = NULL;
} else if (!migrate_uri_parse(str, &channel, &no_uri, &err)) {
if (no_uri) {
qobject_input_visitor_new_str()
visit_type_MigrationChannel()
} else {
report error
}
}
I implemented this and tested for all formats.
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.
I'll keep it simple and go with that unless someone objects.
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?
Agreed on both. I am just pointing out that if we implement '-incoming
uri|channel'
now, but in the future want to add '-incoming channel-list', then we can parse
it
unambiguously and be backwards compatible. I don't foresee ever needing the
latter,
as multiple '-incoming uri|channel' arguments are logically equivalent to a
list.
To reiterate, I prefer '-incoming uri|channel' because it avoids the need
to add a 'defer' property to the channel specification and implementation.
+
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?
Peter likes -incoming, so I will continue with it.
- Steve
- Re: [PATCH V4 06/19] physmem: preserve ram blocks for cpr, (continued)
[PATCH V4 09/19] migration: incoming channel, Steve Sistare, 2024/12/02
Re: [PATCH V4 09/19] migration: incoming channel, Markus Armbruster, 2024/12/10
[PATCH V4 17/19] tests/migration-test: defer connection, Steve Sistare, 2024/12/02