* [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration
@ 2023-05-19 9:46 Het Gala
2023-05-19 9:46 ` [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol Het Gala
` (8 more replies)
0 siblings, 9 replies; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran, Het Gala
This is v5 patchset of modified 'migrate' and 'migrate-incoming' QAPI design
for upstream review.
Would like to thank all the maintainers that actively participated in the v4
patchset discussion and gave insightful suggestions to improve the patches.
Link to previous upstream community patchset links:
v1: https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg04339.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02106.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02473.html
v4: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03064.html
v4 -> v5 changelog:
-------------------
- Improved majorly on cleanly freeing objects across the patches by using
g_auto(), g_autoptr(), gnew0() macros wherever necessary and preventing
unecessary use goto statements.
- Simplified error statement propogation in migration code flow.
- qapi: changed version form 8.0 -> 8.1
- For HMP commands, decided to keep using URI syntax forever, so depricated
implementation of migrate_channel_from_qdict() for converting URI ->
MigrateChannel struct, as "char *uri" is already getting converted into
modified struct using another API in the workflow.
- Additional tcp test case for multifd with modified QAPI syntax in qtests.
Abstract:
---------
Current QAPI 'migrate' command design (for initiating a migration
stream) contains information regarding different migrate transport mechanism
(tcp / unix / exec), dest-host IP address, and binding port number in form of
a string. Thus the design does seem to have some design issues. Some of the
issues, stated below are:
1. Use of string URIs is a data encoding scheme within a data encoding scheme.
QEMU code should directly be able to work with the results from QAPI,
without resorting to do a second level of parsing (eg. socket_parse()).
2. For features / parameters related to migration, the migration tunables needs
to be defined and updated upfront. For example, 'migrate-set-capability'
and 'migrate-set-parameter' is required to enable multifd capability and
multifd-number of channels respectively. Instead, 'Multifd-channels' can
directly be represented as a single additional parameter to 'migrate'
QAPI. 'migrate-set-capability' and 'migrate-set-parameter' commands could
be used for runtime tunables that need setting after migration has already
started.
The current patchset focuses on solving the first problem of multi-level
encoding of URIs. The patch defines 'migrate' command as a QAPI discriminated
union for the various transport backends (like socket, exec and rdma), and on
basis of transport backends, different migration parameters are defined.
(uri) string --> (channel) Channel-type
Transport-type
Migration parameters based on transport type
------------------------------------------------------------------------------
Het Gala (9):
migration: introduced 'MigrateAddress' in QAPI for migration wire
protocol.
migration: convert uri parameter into 'MigrateAddress' struct
migration: convert socket backend to accept MigrateAddress struct
migration: convert rdma backend to accept MigrateAddress struct
migration: convert exec backend to accept MigrateAddress struct.
migration: modified migration QAPIs to accept 'channels' argument for
migration
migration: modify migration_channels_and_uri_compatible() to
incorporate newer migration QAPI syntax
migration: Introduced MigrateChannelList struct to migration code
flow.
migration: adding test case for modified QAPI syntax
migration/exec.c | 62 ++++++++---
migration/exec.h | 8 +-
migration/migration-hmp-cmds.c | 16 ++-
migration/migration.c | 183 ++++++++++++++++++++++++++-------
migration/rdma.c | 34 +++---
migration/rdma.h | 6 +-
migration/socket.c | 39 ++-----
migration/socket.h | 7 +-
qapi/migration.json | 145 +++++++++++++++++++++++++-
softmmu/vl.c | 2 +-
tests/qtest/migration-test.c | 47 +++++++++
11 files changed, 426 insertions(+), 123 deletions(-)
--
2.22.3
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-19 9:46 [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Het Gala
@ 2023-05-19 9:46 ` Het Gala
2023-05-25 17:34 ` Markus Armbruster
2023-05-19 9:46 ` [PATCH v5 2/9] migration: convert uri parameter into 'MigrateAddress' struct Het Gala
` (7 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran, Het Gala
This patch introduces well defined MigrateAddress struct and its related child
objects.
The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
string type. The current migration flow follows double encoding scheme for
fetching migration parameters such as 'uri' and this is not an ideal design.
Motive for intoducing struct level design is to prevent double encoding of QAPI
arguments, as Qemu should be able to directly use the QAPI arguments without
any level of encoding.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/qapi/migration.json b/qapi/migration.json
index 179af0c4d8..c500744bb7 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1407,6 +1407,47 @@
##
{ 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
+##
+# @MigrateTransport:
+#
+# The supported communication transport mechanisms for migration
+#
+# @socket: Supported communication type between two devices for migration.
+# Socket is able to cover all of 'tcp', 'unix', 'vsock' and
+# 'fd' already
+#
+# @exec: Supported communication type to redirect migration stream into file.
+#
+# @rdma: Supported communication type to redirect rdma type migration stream.
+#
+# Since 8.1
+##
+{ 'enum': 'MigrateTransport',
+ 'data': ['socket', 'exec', 'rdma'] }
+
+##
+# @MigrateExecCommand:
+ #
+ # Since 8.1
+ ##
+{ 'struct': 'MigrateExecCommand',
+ 'data': {'args': [ 'str' ] } }
+
+##
+# @MigrateAddress:
+#
+# The options available for communication transport mechanisms for migration
+#
+# Since 8.1
+##
+{ 'union': 'MigrateAddress',
+ 'base': { 'transport' : 'MigrateTransport'},
+ 'discriminator': 'transport',
+ 'data': {
+ 'socket': 'SocketAddress',
+ 'exec': 'MigrateExecCommand',
+ 'rdma': 'InetSocketAddress' } }
+
##
# @migrate:
#
--
2.22.3
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 2/9] migration: convert uri parameter into 'MigrateAddress' struct
2023-05-19 9:46 [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Het Gala
2023-05-19 9:46 ` [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol Het Gala
@ 2023-05-19 9:46 ` Het Gala
2023-05-19 9:46 ` [PATCH v5 3/9] migration: convert socket backend to accept MigrateAddress struct Het Gala
` (6 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran, Het Gala
This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri' parameter
with all the migration connection related information and stores them
inside well defined 'MigrateAddress' struct.
Misc: limit line width in exec.c to 80 characters recommended by Qemu.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/exec.c | 4 ++--
migration/exec.h | 4 ++++
migration/migration.c | 53 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/migration/exec.c b/migration/exec.c
index 2bf882bbe1..c4a3293246 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -27,7 +27,6 @@
#include "qemu/cutils.h"
#ifdef WIN32
-const char *exec_get_cmd_path(void);
const char *exec_get_cmd_path(void)
{
g_autofree char *detected_path = g_new(char, MAX_PATH);
@@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
}
#endif
-void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
+void exec_start_outgoing_migration(MigrationState *s, const char *command,
+ Error **errp)
{
QIOChannel *ioc;
diff --git a/migration/exec.h b/migration/exec.h
index b210ffde7a..736cd71028 100644
--- a/migration/exec.h
+++ b/migration/exec.h
@@ -19,6 +19,10 @@
#ifndef QEMU_MIGRATION_EXEC_H
#define QEMU_MIGRATION_EXEC_H
+
+#ifdef WIN32
+const char *exec_get_cmd_path(void);
+#endif
void exec_start_incoming_migration(const char *host_port, Error **errp);
void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
diff --git a/migration/migration.c b/migration/migration.c
index 5de7f734b9..4c9ecd521b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -64,6 +64,7 @@
#include "yank_functions.h"
#include "sysemu/qtest.h"
#include "options.h"
+#include "qemu/sockets.h"
static NotifierList migration_state_notifiers =
NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -420,15 +421,62 @@ void migrate_add_address(SocketAddress *address)
QAPI_CLONE(SocketAddress, address));
}
+static bool migrate_uri_parse(const char *uri, MigrateAddress **channel,
+ Error **errp)
+{
+ g_autoptr(MigrateAddress) addrs = g_new0(MigrateAddress, 1);
+ SocketAddress *saddr = &addrs->u.socket;
+ InetSocketAddress *isock = &addrs->u.rdma;
+ strList **tail = &addrs->u.exec.args;
+
+ if (strstart(uri, "exec:", NULL)) {
+ addrs->transport = MIGRATE_TRANSPORT_EXEC;
+#ifdef WIN32
+ QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
+#else
+ QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
+#endif
+ QAPI_LIST_APPEND(tail, g_strdup("-c"));
+ QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
+ } else if (strstart(uri, "rdma:", NULL)) {
+ if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
+ qapi_free_InetSocketAddress(isock);
+ return false;
+ }
+ addrs->transport = MIGRATE_TRANSPORT_RDMA;
+ } else if (strstart(uri, "tcp:", NULL) ||
+ strstart(uri, "unix:", NULL) ||
+ strstart(uri, "vsock:", NULL) ||
+ strstart(uri, "fd:", NULL)) {
+ addrs->transport = MIGRATE_TRANSPORT_SOCKET;
+ saddr = socket_parse(uri, errp);
+ if (!saddr) {
+ qapi_free_SocketAddress(saddr);
+ return false;
+ }
+ } else {
+ error_setg(errp, "unknown migration protocol: %s", uri);
+ return false;
+ }
+
+ *channel = addrs;
+ return true;
+}
+
static void qemu_start_incoming_migration(const char *uri, Error **errp)
{
const char *p = NULL;
+ g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
/* URI is not suitable for migration? */
if (!migration_channels_and_uri_compatible(uri, errp)) {
return;
}
+ if (uri && !migrate_uri_parse(uri, &channel, errp)) {
+ return;
+ }
+
qapi_event_send_migration(MIGRATION_STATUS_SETUP);
if (strstart(uri, "tcp:", &p) ||
strstart(uri, "unix:", NULL) ||
@@ -1632,12 +1680,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
Error *local_err = NULL;
MigrationState *s = migrate_get_current();
const char *p = NULL;
+ g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
/* URI is not suitable for migration? */
if (!migration_channels_and_uri_compatible(uri, errp)) {
return;
}
+ if (!migrate_uri_parse(uri, &channel, &local_err)) {
+ return;
+ }
+
if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
has_resume && resume, errp)) {
/* Error detected, put into errp */
--
2.22.3
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 3/9] migration: convert socket backend to accept MigrateAddress struct
2023-05-19 9:46 [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Het Gala
2023-05-19 9:46 ` [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol Het Gala
2023-05-19 9:46 ` [PATCH v5 2/9] migration: convert uri parameter into 'MigrateAddress' struct Het Gala
@ 2023-05-19 9:46 ` Het Gala
2023-05-19 9:46 ` [PATCH v5 4/9] migration: convert rdma " Het Gala
` (5 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran, Het Gala
Socket transport backend for 'migrate'/'migrate-incoming' QAPIs accept
new wire protocol of MigrateAddress struct.
It is achived by parsing 'uri' string and storing migration parameters
required for socket connection into well defined SocketAddress struct.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/migration.c | 32 +++++++++++++++++++-------------
migration/socket.c | 34 +++++-----------------------------
migration/socket.h | 7 ++++---
3 files changed, 28 insertions(+), 45 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 4c9ecd521b..3724de7edd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -478,18 +478,21 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
}
qapi_event_send_migration(MIGRATION_STATUS_SETUP);
- if (strstart(uri, "tcp:", &p) ||
- strstart(uri, "unix:", NULL) ||
- strstart(uri, "vsock:", NULL)) {
- socket_start_incoming_migration(p ? p : uri, errp);
+ if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
+ SocketAddress *saddr = &channel->u.socket;
+ if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+ saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+ saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
+ socket_start_incoming_migration(saddr, errp);
+ } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+ fd_start_incoming_migration(saddr->u.fd.str, errp);
+ }
#ifdef CONFIG_RDMA
} else if (strstart(uri, "rdma:", &p)) {
rdma_start_incoming_migration(p, errp);
#endif
} else if (strstart(uri, "exec:", &p)) {
exec_start_incoming_migration(p, errp);
- } else if (strstart(uri, "fd:", &p)) {
- fd_start_incoming_migration(p, errp);
} else {
error_setg(errp, "unknown migration protocol: %s", uri);
}
@@ -1687,7 +1690,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
return;
}
- if (!migrate_uri_parse(uri, &channel, &local_err)) {
+ if (!migrate_uri_parse(uri, &channel, errp)) {
return;
}
@@ -1703,18 +1706,21 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
}
}
- if (strstart(uri, "tcp:", &p) ||
- strstart(uri, "unix:", NULL) ||
- strstart(uri, "vsock:", NULL)) {
- socket_start_outgoing_migration(s, p ? p : uri, &local_err);
+ if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
+ SocketAddress *saddr = &channel->u.socket;
+ if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+ saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+ saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
+ socket_start_outgoing_migration(s, saddr, &local_err);
+ } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+ fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
+ }
#ifdef CONFIG_RDMA
} else if (strstart(uri, "rdma:", &p)) {
rdma_start_outgoing_migration(s, p, &local_err);
#endif
} else if (strstart(uri, "exec:", &p)) {
exec_start_outgoing_migration(s, p, &local_err);
- } else if (strstart(uri, "fd:", &p)) {
- fd_start_outgoing_migration(s, p, &local_err);
} else {
if (!(has_resume && resume)) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
diff --git a/migration/socket.c b/migration/socket.c
index 1b6f5baefb..8e7430b266 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -108,10 +108,9 @@ out:
object_unref(OBJECT(sioc));
}
-static void
-socket_start_outgoing_migration_internal(MigrationState *s,
- SocketAddress *saddr,
- Error **errp)
+void socket_start_outgoing_migration(MigrationState *s,
+ SocketAddress *saddr,
+ Error **errp)
{
QIOChannelSocket *sioc = qio_channel_socket_new();
struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
@@ -135,18 +134,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
NULL);
}
-void socket_start_outgoing_migration(MigrationState *s,
- const char *str,
- Error **errp)
-{
- Error *err = NULL;
- SocketAddress *saddr = socket_parse(str, &err);
- if (!err) {
- socket_start_outgoing_migration_internal(s, saddr, &err);
- }
- error_propagate(errp, err);
-}
-
static void socket_accept_incoming_migration(QIONetListener *listener,
QIOChannelSocket *cioc,
gpointer opaque)
@@ -172,9 +159,8 @@ socket_incoming_migration_end(void *opaque)
object_unref(OBJECT(listener));
}
-static void
-socket_start_incoming_migration_internal(SocketAddress *saddr,
- Error **errp)
+void socket_start_incoming_migration(SocketAddress *saddr,
+ Error **errp)
{
QIONetListener *listener = qio_net_listener_new();
MigrationIncomingState *mis = migration_incoming_get_current();
@@ -213,13 +199,3 @@ socket_start_incoming_migration_internal(SocketAddress *saddr,
}
}
-void socket_start_incoming_migration(const char *str, Error **errp)
-{
- Error *err = NULL;
- SocketAddress *saddr = socket_parse(str, &err);
- if (!err) {
- socket_start_incoming_migration_internal(saddr, &err);
- }
- qapi_free_SocketAddress(saddr);
- error_propagate(errp, err);
-}
diff --git a/migration/socket.h b/migration/socket.h
index dc54df4e6c..5e4c33b8ea 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -19,13 +19,14 @@
#include "io/channel.h"
#include "io/task.h"
+#include "qemu/sockets.h"
void socket_send_channel_create(QIOTaskFunc f, void *data);
QIOChannel *socket_send_channel_create_sync(Error **errp);
int socket_send_channel_destroy(QIOChannel *send);
-void socket_start_incoming_migration(const char *str, Error **errp);
+void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
-void socket_start_outgoing_migration(MigrationState *s, const char *str,
- Error **errp);
+void socket_start_outgoing_migration(MigrationState *s,
+ SocketAddress *saddr, Error **errp);
#endif
--
2.22.3
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 4/9] migration: convert rdma backend to accept MigrateAddress struct
2023-05-19 9:46 [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Het Gala
` (2 preceding siblings ...)
2023-05-19 9:46 ` [PATCH v5 3/9] migration: convert socket backend to accept MigrateAddress struct Het Gala
@ 2023-05-19 9:46 ` Het Gala
2023-05-19 9:46 ` [PATCH v5 5/9] migration: convert exec " Het Gala
` (4 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran, Het Gala
RDMA based transport backend for 'migrate'/'migrate-incoming' QAPIs
accept new wire protocol of MigrateAddress struct.
It is achived by parsing 'uri' string and storing migration parameters
required for RDMA connection into well defined InetSocketAddress struct.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/migration.c | 8 ++++----
migration/rdma.c | 34 ++++++++++++----------------------
migration/rdma.h | 6 ++++--
3 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 3724de7edd..b7c72fafc9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -488,8 +488,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
fd_start_incoming_migration(saddr->u.fd.str, errp);
}
#ifdef CONFIG_RDMA
- } else if (strstart(uri, "rdma:", &p)) {
- rdma_start_incoming_migration(p, errp);
+ } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
+ rdma_start_incoming_migration(&channel->u.rdma, errp);
#endif
} else if (strstart(uri, "exec:", &p)) {
exec_start_incoming_migration(p, errp);
@@ -1716,8 +1716,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
}
#ifdef CONFIG_RDMA
- } else if (strstart(uri, "rdma:", &p)) {
- rdma_start_outgoing_migration(s, p, &local_err);
+ } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
+ rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
#endif
} else if (strstart(uri, "exec:", &p)) {
exec_start_outgoing_migration(s, p, &local_err);
diff --git a/migration/rdma.c b/migration/rdma.c
index 2e4dcff1c9..966a21151c 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -319,7 +319,6 @@ typedef struct RDMALocalBlocks {
typedef struct RDMAContext {
char *host;
int port;
- char *host_port;
RDMAWorkRequestData wr_data[RDMA_WRID_MAX];
@@ -2455,9 +2454,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
rdma->channel = NULL;
}
g_free(rdma->host);
- g_free(rdma->host_port);
rdma->host = NULL;
- rdma->host_port = NULL;
}
@@ -2739,28 +2736,17 @@ static void qemu_rdma_return_path_dest_init(RDMAContext *rdma_return_path,
rdma_return_path->is_return_path = true;
}
-static void *qemu_rdma_data_init(const char *host_port, Error **errp)
+static void *qemu_rdma_data_init(InetSocketAddress *saddr, Error **errp)
{
RDMAContext *rdma = NULL;
- InetSocketAddress *addr;
- if (host_port) {
+ if (saddr) {
rdma = g_new0(RDMAContext, 1);
rdma->current_index = -1;
rdma->current_chunk = -1;
- addr = g_new(InetSocketAddress, 1);
- if (!inet_parse(addr, host_port, NULL)) {
- rdma->port = atoi(addr->port);
- rdma->host = g_strdup(addr->host);
- rdma->host_port = g_strdup(host_port);
- } else {
- ERROR(errp, "bad RDMA migration address '%s'", host_port);
- g_free(rdma);
- rdma = NULL;
- }
-
- qapi_free_InetSocketAddress(addr);
+ rdma->host = g_strdup(saddr->host);
+ rdma->port = atoi(saddr->port);
}
return rdma;
@@ -3359,6 +3345,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
.private_data_len = sizeof(cap),
};
RDMAContext *rdma_return_path = NULL;
+ g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
struct rdma_cm_event *cm_event;
struct ibv_context *verbs;
int ret = -EINVAL;
@@ -3374,13 +3361,16 @@ static int qemu_rdma_accept(RDMAContext *rdma)
goto err_rdma_dest_wait;
}
+ isock->host = rdma->host;
+ isock->port = g_strdup_printf("%d", rdma->port);
+
/*
* initialize the RDMAContext for return path for postcopy after first
* connection request reached.
*/
if ((migrate_postcopy() || migrate_return_path())
&& !rdma->is_return_path) {
- rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
+ rdma_return_path = qemu_rdma_data_init(isock, NULL);
if (rdma_return_path == NULL) {
rdma_ack_cm_event(cm_event);
goto err_rdma_dest_wait;
@@ -4113,7 +4103,8 @@ static void rdma_accept_incoming_migration(void *opaque)
}
}
-void rdma_start_incoming_migration(const char *host_port, Error **errp)
+void rdma_start_incoming_migration(InetSocketAddress *host_port,
+ Error **errp)
{
int ret;
RDMAContext *rdma;
@@ -4159,13 +4150,12 @@ err:
error_propagate(errp, local_err);
if (rdma) {
g_free(rdma->host);
- g_free(rdma->host_port);
}
g_free(rdma);
}
void rdma_start_outgoing_migration(void *opaque,
- const char *host_port, Error **errp)
+ InetSocketAddress *host_port, Error **errp)
{
MigrationState *s = opaque;
RDMAContext *rdma_return_path = NULL;
diff --git a/migration/rdma.h b/migration/rdma.h
index de2ba09dc5..ee89296555 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -14,12 +14,14 @@
*
*/
+#include "qemu/sockets.h"
+
#ifndef QEMU_MIGRATION_RDMA_H
#define QEMU_MIGRATION_RDMA_H
-void rdma_start_outgoing_migration(void *opaque, const char *host_port,
+void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *host_port,
Error **errp);
-void rdma_start_incoming_migration(const char *host_port, Error **errp);
+void rdma_start_incoming_migration(InetSocketAddress *host_port, Error **errp);
#endif
--
2.22.3
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 5/9] migration: convert exec backend to accept MigrateAddress struct.
2023-05-19 9:46 [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Het Gala
` (3 preceding siblings ...)
2023-05-19 9:46 ` [PATCH v5 4/9] migration: convert rdma " Het Gala
@ 2023-05-19 9:46 ` Het Gala
2023-05-19 9:52 ` Het Gala
2023-05-19 9:46 ` [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration Het Gala
` (3 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran, Het Gala
Exec transport backend for 'migrate'/'migrate-incoming' QAPIs accept
new wire protocol of MigrateAddress struct.
It is achived by parsing 'uri' string and storing migration parameters
required for exec connection into strList struct.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/exec.c | 60 +++++++++++++++++++++++++++++++------------
migration/exec.h | 4 +--
migration/migration.c | 10 +++-----
3 files changed, 50 insertions(+), 24 deletions(-)
diff --git a/migration/exec.c b/migration/exec.c
index c4a3293246..a4f02b207f 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -39,19 +39,47 @@ const char *exec_get_cmd_path(void)
}
#endif
-void exec_start_outgoing_migration(MigrationState *s, const char *command,
+/* provides the length of strList */
+static int
+str_list_length(strList *list)
+{
+ int len = 0;
+ strList *elem;
+
+ for (elem = list; elem != NULL; elem = elem->next) {
+ len++;
+ }
+
+ return len;
+}
+
+static void
+init_exec_array(strList *command, char **argv, Error **errp)
+{
+ int i = 0;
+ strList *lst;
+
+ for (lst = command; lst; lst = lst->next) {
+ argv[i++] = lst->value;
+ }
+
+ argv[i] = NULL;
+ return;
+}
+
+void exec_start_outgoing_migration(MigrationState *s, strList *command,
Error **errp)
{
QIOChannel *ioc;
-#ifdef WIN32
- const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
-#else
- const char *argv[] = { "/bin/sh", "-c", command, NULL };
-#endif
+ int length = str_list_length(command);
+ g_auto(GStrv) argv = (char **) g_new0(const char *, length);
- trace_migration_exec_outgoing(command);
- ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
+ init_exec_array(command, argv, errp);
+ g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
+
+ trace_migration_exec_outgoing(new_command);
+ ioc = QIO_CHANNEL(qio_channel_command_new_spawn((const char * const*) argv,
O_RDWR,
errp));
if (!ioc) {
@@ -72,18 +100,18 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
return G_SOURCE_REMOVE;
}
-void exec_start_incoming_migration(const char *command, Error **errp)
+void exec_start_incoming_migration(strList *command, Error **errp)
{
QIOChannel *ioc;
-#ifdef WIN32
- const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
-#else
- const char *argv[] = { "/bin/sh", "-c", command, NULL };
-#endif
+ int length = str_list_length(command);
+ g_auto(GStrv) argv = (char **) g_new0(const char *, length);
+
+ init_exec_array(command, argv, errp);
+ g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
- trace_migration_exec_incoming(command);
- ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
+ trace_migration_exec_incoming(new_command);
+ ioc = QIO_CHANNEL(qio_channel_command_new_spawn((const char * const*) argv,
O_RDWR,
errp));
if (!ioc) {
diff --git a/migration/exec.h b/migration/exec.h
index 736cd71028..3107f205e3 100644
--- a/migration/exec.h
+++ b/migration/exec.h
@@ -23,8 +23,8 @@
#ifdef WIN32
const char *exec_get_cmd_path(void);
#endif
-void exec_start_incoming_migration(const char *host_port, Error **errp);
+void exec_start_incoming_migration(strList *host_port, Error **errp);
-void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
+void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
Error **errp);
#endif
diff --git a/migration/migration.c b/migration/migration.c
index b7c72fafc9..0a6ab9229b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -465,7 +465,6 @@ static bool migrate_uri_parse(const char *uri, MigrateAddress **channel,
static void qemu_start_incoming_migration(const char *uri, Error **errp)
{
- const char *p = NULL;
g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
/* URI is not suitable for migration? */
@@ -491,8 +490,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
} else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
rdma_start_incoming_migration(&channel->u.rdma, errp);
#endif
- } else if (strstart(uri, "exec:", &p)) {
- exec_start_incoming_migration(p, errp);
+ } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) {
+ exec_start_incoming_migration(channel->u.exec.args, errp);
} else {
error_setg(errp, "unknown migration protocol: %s", uri);
}
@@ -1682,7 +1681,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
{
Error *local_err = NULL;
MigrationState *s = migrate_get_current();
- const char *p = NULL;
g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
/* URI is not suitable for migration? */
@@ -1719,8 +1717,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
} else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
#endif
- } else if (strstart(uri, "exec:", &p)) {
- exec_start_outgoing_migration(s, p, &local_err);
+ } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) {
+ exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
} else {
if (!(has_resume && resume)) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
--
2.22.3
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration
2023-05-19 9:46 [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Het Gala
` (4 preceding siblings ...)
2023-05-19 9:46 ` [PATCH v5 5/9] migration: convert exec " Het Gala
@ 2023-05-19 9:46 ` Het Gala
2023-05-25 17:50 ` Markus Armbruster
2023-05-19 9:46 ` [PATCH v5 7/9] migration: modify migration_channels_and_uri_compatible() to incorporate newer migration QAPI syntax Het Gala
` (2 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran, Het Gala
MigrateChannelList allows to connect accross multiple interfaces. Added
MigrateChannelList struct as argument to migration QAPIs.
Future patchset series plans to include multiple MigrateChannels
for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
is the preferred choice of argument over 'MigrateChannel' and making
migration QAPIs future proof.
For current patchset series, have limited the size of the list to single
element (single interface) as runtime check.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/migration-hmp-cmds.c | 16 +++--
migration/migration.c | 38 +++++++++---
qapi/migration.json | 104 ++++++++++++++++++++++++++++++++-
softmmu/vl.c | 2 +-
4 files changed, 144 insertions(+), 16 deletions(-)
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 9885d7c9f7..8ddfa258ad 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -423,10 +423,12 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
const char *uri = qdict_get_str(qdict, "uri");
+ MigrateChannelList *caps = NULL;
+ g_autoptr(MigrateChannel) channel = g_new0(MigrateChannel, 1);
- qmp_migrate_incoming(uri, &err);
-
- hmp_handle_error(mon, err);
+ QAPI_LIST_PREPEND(caps, channel);
+ qmp_migrate_incoming(uri, false, caps, &err);
+ qapi_free_MigrateChannelList(caps);
}
void hmp_migrate_recover(Monitor *mon, const QDict *qdict)
@@ -704,9 +706,13 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
bool resume = qdict_get_try_bool(qdict, "resume", false);
const char *uri = qdict_get_str(qdict, "uri");
Error *err = NULL;
+ MigrateChannelList *caps = NULL;
+ g_autoptr(MigrateChannel) channel = g_new0(MigrateChannel, 1);
- qmp_migrate(uri, !!blk, blk, !!inc, inc,
- false, false, true, resume, &err);
+ QAPI_LIST_PREPEND(caps, channel);
+ qmp_migrate(uri, false, caps, !!blk, blk, !!inc, inc,
+ false, false, true, resume, &err);
+ qapi_free_MigrateChannelList(caps);
if (hmp_handle_error(mon, err)) {
return;
}
diff --git a/migration/migration.c b/migration/migration.c
index 0a6ab9229b..abccc6bf26 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -463,10 +463,22 @@ static bool migrate_uri_parse(const char *uri, MigrateAddress **channel,
return true;
}
-static void qemu_start_incoming_migration(const char *uri, Error **errp)
+static void qemu_start_incoming_migration(const char *uri, bool has_channels,
+ MigrateChannelList *channels,
+ Error **errp)
{
g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
+ /*
+ * Having preliminary checks for uri and channel
+ */
+ if (uri && has_channels) {
+ error_setg(errp, "'uri' and 'channels' arguments are mutually "
+ "exclusive; exactly one of the two should be present in "
+ "'migrate-incoming' qmp command ");
+ return;
+ }
+
/* URI is not suitable for migration? */
if (!migration_channels_and_uri_compatible(uri, errp)) {
return;
@@ -1488,7 +1500,8 @@ void migrate_del_blocker(Error *reason)
migration_blockers = g_slist_remove(migration_blockers, reason);
}
-void qmp_migrate_incoming(const char *uri, Error **errp)
+void qmp_migrate_incoming(const char *uri, bool has_channels,
+ MigrateChannelList *channels, Error **errp)
{
Error *local_err = NULL;
static bool once = true;
@@ -1506,7 +1519,7 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
return;
}
- qemu_start_incoming_migration(uri, &local_err);
+ qemu_start_incoming_migration(uri, has_channels, channels, &local_err);
if (local_err) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
@@ -1542,7 +1555,7 @@ void qmp_migrate_recover(const char *uri, Error **errp)
* only re-setup the migration stream and poke existing migration
* to continue using that newly established channel.
*/
- qemu_start_incoming_migration(uri, errp);
+ qemu_start_incoming_migration(uri, false, NULL, errp);
}
void qmp_migrate_pause(Error **errp)
@@ -1675,14 +1688,25 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
return true;
}
-void qmp_migrate(const char *uri, bool has_blk, bool blk,
- bool has_inc, bool inc, bool has_detach, bool detach,
- bool has_resume, bool resume, Error **errp)
+void qmp_migrate(const char *uri, bool has_channels,
+ MigrateChannelList *channels, bool has_blk, bool blk,
+ bool has_inc, bool inc, bool has_detach,
+ bool detach, bool has_resume, bool resume, Error **errp)
{
Error *local_err = NULL;
MigrationState *s = migrate_get_current();
g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
+ /*
+ * Having preliminary checks for uri and channel
+ */
+ if (uri && has_channels) {
+ error_setg(errp, "'uri' and 'channels' arguments are mutually "
+ "exclusive; exactly one of the two should be present in "
+ "'migrate' qmp command ");
+ return;
+ }
+
/* URI is not suitable for migration? */
if (!migration_channels_and_uri_compatible(uri, errp)) {
return;
diff --git a/qapi/migration.json b/qapi/migration.json
index c500744bb7..86bbc916d1 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1448,12 +1448,47 @@
'exec': 'MigrateExecCommand',
'rdma': 'InetSocketAddress' } }
+##
+# @MigrateChannelType:
+#
+# The supported options for migration channel type requests
+#
+# @main: Support request for main outbound migration control channel
+#
+# Since 8.1
+##
+{ 'enum': 'MigrateChannelType',
+ 'data': [ 'main' ] }
+
+##
+# @MigrateChannel:
+#
+# Information regarding migration Channel-type for transferring packets,
+# source and corresponding destination interface for socket connection
+# and number of multifd channels over the interface.
+#
+# @channeltype: Name of Channel type for transfering packet information
+#
+# @addr: Information regarding migration parameters of destination interface
+#
+# Since 8.1
+##
+{ 'struct': 'MigrateChannel',
+ 'data': {
+ 'channeltype': 'MigrateChannelType',
+ 'addr': 'MigrateAddress' } }
+
##
# @migrate:
#
# Migrates the current running guest to another Virtual Machine.
#
# @uri: the Uniform Resource Identifier of the destination VM
+# for migration thread
+#
+# @channels: Struct containing list of migration channel types, with all
+# the information regarding destination interfaces required for
+# initiating a migration stream.
#
# @blk: do block migration (full disk copy)
#
@@ -1479,14 +1514,44 @@
# 3. The user Monitor's "detach" argument is invalid in QMP and should
# not be used
#
+# 4. The uri argument should have the Uniform Resource Identifier of default
+# destination VM. This connection will be bound to default network
+#
+# 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
+# of the two should be present.
+#
# Example:
#
# -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
# <- { "return": {} }
+# -> { "execute": "migrate",
+# "arguments": {
+# "channels": [ { "channeltype": "main",
+# "addr": { "transport": "socket", "type": "inet",
+# "host": "10.12.34.9",
+# "port": "1050" } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+# "arguments": {
+# "channels": [ { "channeltype": "main",
+# "addr": { "transport": "exec",
+# "args": [ "/bin/nc", "-p", "6000",
+# "/some/sock" ] } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+# "arguments": {
+# "channels": [ { "channeltype": "main",
+# "addr": { "transport": "rdma",
+# "host": "10.12.34.9",
+# "port": "1050" } } ] } }
+# <- { "return": {} }
+#
##
{ 'command': 'migrate',
- 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
- '*detach': 'bool', '*resume': 'bool' } }
+ 'data': {'*uri': 'str', '*channels': [ 'MigrateChannel' ], '*blk': 'bool',
+ '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
##
# @migrate-incoming:
@@ -1497,6 +1562,10 @@
# @uri: The Uniform Resource Identifier identifying the source or
# address to listen on
#
+# @channels: Struct containing list of migration channel types, with all
+# the information regarding destination interfaces required for
+# initiating a migration stream.
+#
# Returns: nothing on success
#
# Since: 2.3
@@ -1512,13 +1581,42 @@
#
# 3. The uri format is the same as for -incoming
#
+# 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
+# of the two should be present.
+#
# Example:
#
# -> { "execute": "migrate-incoming",
# "arguments": { "uri": "tcp::4446" } }
# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+# "arguments": {
+# "channels": [ { "channeltype": "main",
+# "addr": { "transport": "socket", "type": "inet",
+# "host": "10.12.34.9",
+# "port": "1050" } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+# "arguments": {
+# "channels": [ { "channeltype": "main",
+# "addr": { "transport": "exec",
+# "args": [ "/bin/nc", "-p", "6000",
+# "/some/sock" ] } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+# "arguments": {
+# "channels": [ { "channeltype": "main",
+# "addr": { "transport": "rdma",
+# "host": "10.12.34.9",
+# "port": "1050" } } ] } }
+# <- { "return": {} }
##
-{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
+{ 'command': 'migrate-incoming',
+ 'data': {'*uri': 'str',
+ '*channels': [ 'MigrateChannel' ] } }
##
# @xen-save-devices-state:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 6c2427262b..ade411eb4f 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2633,7 +2633,7 @@ void qmp_x_exit_preconfig(Error **errp)
if (incoming) {
Error *local_err = NULL;
if (strcmp(incoming, "defer") != 0) {
- qmp_migrate_incoming(incoming, &local_err);
+ qmp_migrate_incoming(incoming, false, NULL, &local_err);
if (local_err) {
error_reportf_err(local_err, "-incoming %s: ", incoming);
exit(1);
--
2.22.3
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 7/9] migration: modify migration_channels_and_uri_compatible() to incorporate newer migration QAPI syntax
2023-05-19 9:46 [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Het Gala
` (5 preceding siblings ...)
2023-05-19 9:46 ` [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration Het Gala
@ 2023-05-19 9:46 ` Het Gala
2023-05-19 9:46 ` [PATCH v5 8/9] migration: Introduced MigrateChannelList struct to migration code flow Het Gala
2023-05-19 9:46 ` [PATCH v5 9/9] migration: adding test case for modified QAPI syntax Het Gala
8 siblings, 0 replies; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran, Het Gala
migration_channels_and_uri_compatible() check for transport mechanism
suitable for multifd migration gets executed when the caller calls old
uri syntax. It needs it to be run when using the mordern MigrateCHannel
QAPI syntax too.
After URI -> 'MigrateChannel' object conversion,
migration_channels_and_uri_compatible() ->
migration_channels_and_transport_compatible() passes object as argument
and checks for valid transport mechanism.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/migration.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index abccc6bf26..8afdea4f91 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -103,17 +103,20 @@ static bool migration_needs_multiple_sockets(void)
return migrate_multifd() || migrate_postcopy_preempt();
}
-static bool uri_supports_multi_channels(const char *uri)
+static bool transport_supports_multi_channels(SocketAddress *saddr)
{
- return strstart(uri, "tcp:", NULL) || strstart(uri, "unix:", NULL) ||
- strstart(uri, "vsock:", NULL);
+ return saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+ saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+ saddr->type == SOCKET_ADDRESS_TYPE_VSOCK;
}
static bool
-migration_channels_and_uri_compatible(const char *uri, Error **errp)
+migration_channels_and_transport_compatible(MigrateAddress *addrs,
+ Error **errp)
{
if (migration_needs_multiple_sockets() &&
- !uri_supports_multi_channels(uri)) {
+ (addrs->transport == MIGRATE_TRANSPORT_SOCKET) &&
+ !transport_supports_multi_channels(&addrs->u.socket)) {
error_setg(errp, "Migration requires multi-channel URIs (e.g. tcp)");
return false;
}
@@ -479,12 +482,12 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
return;
}
- /* URI is not suitable for migration? */
- if (!migration_channels_and_uri_compatible(uri, errp)) {
+ if (uri && !migrate_uri_parse(uri, &channel, errp)) {
return;
}
- if (uri && !migrate_uri_parse(uri, &channel, errp)) {
+ /* transport mechanism not suitable for migration? */
+ if (!migration_channels_and_transport_compatible(channel, errp)) {
return;
}
@@ -1707,12 +1710,12 @@ void qmp_migrate(const char *uri, bool has_channels,
return;
}
- /* URI is not suitable for migration? */
- if (!migration_channels_and_uri_compatible(uri, errp)) {
+ if (!migrate_uri_parse(uri, &channel, errp)) {
return;
}
- if (!migrate_uri_parse(uri, &channel, errp)) {
+ /* transport mechanism not suitable for migration? */
+ if (!migration_channels_and_transport_compatible(channel, errp)) {
return;
}
--
2.22.3
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 8/9] migration: Introduced MigrateChannelList struct to migration code flow.
2023-05-19 9:46 [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Het Gala
` (6 preceding siblings ...)
2023-05-19 9:46 ` [PATCH v5 7/9] migration: modify migration_channels_and_uri_compatible() to incorporate newer migration QAPI syntax Het Gala
@ 2023-05-19 9:46 ` Het Gala
2023-05-25 18:02 ` Markus Armbruster
2023-05-19 9:46 ` [PATCH v5 9/9] migration: adding test case for modified QAPI syntax Het Gala
8 siblings, 1 reply; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran, Het Gala
Integrated MigrateChannelList with all transport backends (socket, exec
and rdma) for both source and destination migration code flow.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/migration.c | 75 ++++++++++++++++++++++++++++---------------
migration/socket.c | 5 ++-
2 files changed, 53 insertions(+), 27 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 8afdea4f91..57085753f4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -424,9 +424,10 @@ void migrate_add_address(SocketAddress *address)
QAPI_CLONE(SocketAddress, address));
}
-static bool migrate_uri_parse(const char *uri, MigrateAddress **channel,
+static bool migrate_uri_parse(const char *uri, MigrateChannel **channel,
Error **errp)
{
+ g_autoptr(MigrateChannel) val = g_new0(MigrateChannel, 1);
g_autoptr(MigrateAddress) addrs = g_new0(MigrateAddress, 1);
SocketAddress *saddr = &addrs->u.socket;
InetSocketAddress *isock = &addrs->u.rdma;
@@ -462,7 +463,9 @@ static bool migrate_uri_parse(const char *uri, MigrateAddress **channel,
return false;
}
- *channel = addrs;
+ val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
+ val->addr = addrs;
+ *channel = val;
return true;
}
@@ -470,7 +473,8 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
MigrateChannelList *channels,
Error **errp)
{
- g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
+ g_autoptr(MigrateChannel) channel = g_new0(MigrateChannel, 1);
+ g_autoptr(MigrateAddress) addrs;
/*
* Having preliminary checks for uri and channel
@@ -480,20 +484,29 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
"exclusive; exactly one of the two should be present in "
"'migrate-incoming' qmp command ");
return;
- }
-
- if (uri && !migrate_uri_parse(uri, &channel, errp)) {
- return;
+ } else if (channels) {
+ /* To verify that Migrate channel list has only item */
+ if (channels->next) {
+ error_setg(errp, "Channel list has more than one entries");
+ return;
+ }
+ channel = channels->value;
+ addrs = channel->addr;
+ } else {
+ /* caller uses the old URI syntax */
+ if (uri && !migrate_uri_parse(uri, &channel, errp)) {
+ return;
+ }
}
/* transport mechanism not suitable for migration? */
- if (!migration_channels_and_transport_compatible(channel, errp)) {
+ if (!migration_channels_and_transport_compatible(addrs, errp)) {
return;
}
qapi_event_send_migration(MIGRATION_STATUS_SETUP);
- if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
- SocketAddress *saddr = &channel->u.socket;
+ if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
+ SocketAddress *saddr = &addrs->u.socket;
if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
@@ -502,11 +515,11 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
fd_start_incoming_migration(saddr->u.fd.str, errp);
}
#ifdef CONFIG_RDMA
- } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
- rdma_start_incoming_migration(&channel->u.rdma, errp);
+ } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
+ rdma_start_incoming_migration(&addrs->u.rdma, errp);
#endif
- } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) {
- exec_start_incoming_migration(channel->u.exec.args, errp);
+ } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
+ exec_start_incoming_migration(addrs->u.exec.args, errp);
} else {
error_setg(errp, "unknown migration protocol: %s", uri);
}
@@ -1698,7 +1711,8 @@ void qmp_migrate(const char *uri, bool has_channels,
{
Error *local_err = NULL;
MigrationState *s = migrate_get_current();
- g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
+ g_autoptr(MigrateChannel) channel = g_new0(MigrateChannel, 1);
+ g_autoptr(MigrateAddress) addrs;
/*
* Having preliminary checks for uri and channel
@@ -1708,14 +1722,23 @@ void qmp_migrate(const char *uri, bool has_channels,
"exclusive; exactly one of the two should be present in "
"'migrate' qmp command ");
return;
- }
-
- if (!migrate_uri_parse(uri, &channel, errp)) {
- return;
+ } else if (channels) {
+ /* To verify that Migrate channel list has only item */
+ if (channels->next) {
+ error_setg(errp, "Channel list has more than one entries");
+ return;
+ }
+ channel = channels->value;
+ addrs = channel->addr;
+ } else {
+ /* caller uses the old URI syntax */
+ if (uri && !migrate_uri_parse(uri, &channel, errp)) {
+ return;
+ }
}
/* transport mechanism not suitable for migration? */
- if (!migration_channels_and_transport_compatible(channel, errp)) {
+ if (!migration_channels_and_transport_compatible(addrs, errp)) {
return;
}
@@ -1731,8 +1754,8 @@ void qmp_migrate(const char *uri, bool has_channels,
}
}
- if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
- SocketAddress *saddr = &channel->u.socket;
+ if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
+ SocketAddress *saddr = &addrs->u.socket;
if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
@@ -1741,11 +1764,11 @@ void qmp_migrate(const char *uri, bool has_channels,
fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
}
#ifdef CONFIG_RDMA
- } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
- rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
+ } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
+ rdma_start_outgoing_migration(s, &addrs->u.rdma, &local_err);
#endif
- } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) {
- exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
+ } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
+ exec_start_outgoing_migration(s, addrs->u.exec.args, &local_err);
} else {
if (!(has_resume && resume)) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
diff --git a/migration/socket.c b/migration/socket.c
index 8e7430b266..98e3ea1514 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -28,6 +28,8 @@
#include "trace.h"
#include "postcopy-ram.h"
#include "options.h"
+#include "qapi/clone-visitor.h"
+#include "qapi/qapi-visit-sockets.h"
struct SocketOutgoingArgs {
SocketAddress *saddr;
@@ -114,12 +116,13 @@ void socket_start_outgoing_migration(MigrationState *s,
{
QIOChannelSocket *sioc = qio_channel_socket_new();
struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
+ SocketAddress *addr = QAPI_CLONE(SocketAddress, saddr);
data->s = s;
/* in case previous migration leaked it */
qapi_free_SocketAddress(outgoing_args.saddr);
- outgoing_args.saddr = saddr;
+ outgoing_args.saddr = addr;
if (saddr->type == SOCKET_ADDRESS_TYPE_INET) {
data->hostname = g_strdup(saddr->u.inet.host);
--
2.22.3
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 9/9] migration: adding test case for modified QAPI syntax
2023-05-19 9:46 [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Het Gala
` (7 preceding siblings ...)
2023-05-19 9:46 ` [PATCH v5 8/9] migration: Introduced MigrateChannelList struct to migration code flow Het Gala
@ 2023-05-19 9:46 ` Het Gala
2023-05-19 9:49 ` Het Gala
8 siblings, 1 reply; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran, Het Gala
Adding multifd tcp common test case for modified QAPI syntax defined.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
tests/qtest/migration-test.c | 47 ++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b99b49a314..ef6f9181da 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2021,6 +2021,34 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
return NULL;
}
+static void *
+test_migrate_precopy_tcp_multifd_start_new_syntax_common(QTestState *from,
+ QTestState *to,
+ const char *method)
+{
+ QDict *rsp;
+
+ migrate_set_parameter_int(from, "multifd-channels", 16);
+ migrate_set_parameter_int(to, "multifd-channels", 16);
+
+ migrate_set_parameter_str(from, "multifd-compression", method);
+ migrate_set_parameter_str(to, "multifd-compression", method);
+
+ migrate_set_capability(from, "multifd", true);
+ migrate_set_capability(to, "multifd", true);
+
+ /* Start incoming migration from the 1st socket */
+ rsp = wait_command(to, "{ 'execute': 'migrate-incoming',"
+ " 'arguments': { "
+ " 'channels': [ { 'channeltype': 'main',"
+ " 'addr': { 'transport': 'socket',"
+ " 'type': 'inet','host': '127.0.0.1',"
+ " 'port': '0' } } ] } }");
+ qobject_unref(rsp);
+
+ return NULL;
+}
+
static void *
test_migrate_precopy_tcp_multifd_start(QTestState *from,
QTestState *to)
@@ -2028,6 +2056,14 @@ test_migrate_precopy_tcp_multifd_start(QTestState *from,
return test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
}
+static void *
+test_migrate_precopy_tcp_multifd_new_syntax_start(QTestState *from,
+ QTestState *to)
+{
+ return test_migrate_precopy_tcp_multifd_start_new_syntax_common(from,
+ to, "none");
+}
+
static void *
test_migrate_precopy_tcp_multifd_zlib_start(QTestState *from,
QTestState *to)
@@ -2053,6 +2089,15 @@ static void test_multifd_tcp_none(void)
test_precopy_common(&args);
}
+static void test_multifd_tcp_new_syntax_none(void)
+{
+ MigrateCommon args = {
+ .listen_uri = "defer",
+ .start_hook = test_migrate_precopy_tcp_multifd_new_syntax_start,
+ };
+ test_precopy_common(&args);
+}
+
static void test_multifd_tcp_zlib(void)
{
MigrateCommon args = {
@@ -2736,6 +2781,8 @@ int main(int argc, char **argv)
}
qtest_add_func("/migration/multifd/tcp/plain/none",
test_multifd_tcp_none);
+ qtest_add_func("/migration/multifd/tcp/plain/none",
+ test_multifd_tcp_new_syntax_none);
/*
* This test is flaky and sometimes fails in CI and otherwise:
* don't run unless user opts in via environment variable.
--
2.22.3
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v5 9/9] migration: adding test case for modified QAPI syntax
2023-05-19 9:46 ` [PATCH v5 9/9] migration: adding test case for modified QAPI syntax Het Gala
@ 2023-05-19 9:49 ` Het Gala
0 siblings, 0 replies; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:49 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran
On 19/05/23 3:16 pm, Het Gala wrote:
> Adding multifd tcp common test case for modified QAPI syntax defined.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
> tests/qtest/migration-test.c | 47 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index b99b49a314..ef6f9181da 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -2021,6 +2021,34 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
> return NULL;
> }
>
> +static void *
> +test_migrate_precopy_tcp_multifd_start_new_syntax_common(QTestState *from,
> + QTestState *to,
> + const char *method)
> +{
> + QDict *rsp;
> +
> + migrate_set_parameter_int(from, "multifd-channels", 16);
> + migrate_set_parameter_int(to, "multifd-channels", 16);
> +
> + migrate_set_parameter_str(from, "multifd-compression", method);
> + migrate_set_parameter_str(to, "multifd-compression", method);
> +
> + migrate_set_capability(from, "multifd", true);
> + migrate_set_capability(to, "multifd", true);
> +
> + /* Start incoming migration from the 1st socket */
> + rsp = wait_command(to, "{ 'execute': 'migrate-incoming',"
> + " 'arguments': { "
> + " 'channels': [ { 'channeltype': 'main',"
> + " 'addr': { 'transport': 'socket',"
> + " 'type': 'inet','host': '127.0.0.1',"
> + " 'port': '0' } } ] } }");
> + qobject_unref(rsp);
> +
> + return NULL;
> +}
> +
> static void *
> test_migrate_precopy_tcp_multifd_start(QTestState *from,
> QTestState *to)
> @@ -2028,6 +2056,14 @@ test_migrate_precopy_tcp_multifd_start(QTestState *from,
> return test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
> }
>
> +static void *
> +test_migrate_precopy_tcp_multifd_new_syntax_start(QTestState *from,
> + QTestState *to)
> +{
> + return test_migrate_precopy_tcp_multifd_start_new_syntax_common(from,
> + to, "none");
> +}
> +
> static void *
> test_migrate_precopy_tcp_multifd_zlib_start(QTestState *from,
> QTestState *to)
> @@ -2053,6 +2089,15 @@ static void test_multifd_tcp_none(void)
> test_precopy_common(&args);
> }
>
> +static void test_multifd_tcp_new_syntax_none(void)
> +{
> + MigrateCommon args = {
> + .listen_uri = "defer",
> + .start_hook = test_migrate_precopy_tcp_multifd_new_syntax_start,
> + };
> + test_precopy_common(&args);
> +}
> +
> static void test_multifd_tcp_zlib(void)
> {
> MigrateCommon args = {
> @@ -2736,6 +2781,8 @@ int main(int argc, char **argv)
> }
> qtest_add_func("/migration/multifd/tcp/plain/none",
> test_multifd_tcp_none);
> + qtest_add_func("/migration/multifd/tcp/plain/none",
> + test_multifd_tcp_new_syntax_none);
> /*
> * This test is flaky and sometimes fails in CI and otherwise:
> * don't run unless user opts in via environment variable.
Maintainers, though I have added a tcp test with new QAPI syntax for
multifd here. I feel it is incomplete because it seems like
test_precopy_common() finally sees the arguments in form of char *uri ->
string form. Please advice, do we need to totally revamp the test case
functions here for the modified syntax ?
Regards,
Het Gala
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 5/9] migration: convert exec backend to accept MigrateAddress struct.
2023-05-19 9:46 ` [PATCH v5 5/9] migration: convert exec " Het Gala
@ 2023-05-19 9:52 ` Het Gala
0 siblings, 0 replies; 31+ messages in thread
From: Het Gala @ 2023-05-19 9:52 UTC (permalink / raw)
To: qemu-devel
Cc: prerna.saxena, quintela, dgilbert, pbonzini, berrange, armbru,
eblake, manish.mishra, aravind.retnakaran
On 19/05/23 3:16 pm, Het Gala wrote:
> Exec transport backend for 'migrate'/'migrate-incoming' QAPIs accept
> new wire protocol of MigrateAddress struct.
>
> It is achived by parsing 'uri' string and storing migration parameters
> required for exec connection into strList struct.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
> migration/exec.c | 60 +++++++++++++++++++++++++++++++------------
> migration/exec.h | 4 +--
> migration/migration.c | 10 +++-----
> 3 files changed, 50 insertions(+), 24 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index c4a3293246..a4f02b207f 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -39,19 +39,47 @@ const char *exec_get_cmd_path(void)
> }
> #endif
>
> -void exec_start_outgoing_migration(MigrationState *s, const char *command,
> +/* provides the length of strList */
> +static int
> +str_list_length(strList *list)
> +{
> + int len = 0;
> + strList *elem;
> +
> + for (elem = list; elem != NULL; elem = elem->next) {
> + len++;
> + }
> +
> + return len;
> +}
> +
> +static void
> +init_exec_array(strList *command, char **argv, Error **errp)
> +{
> + int i = 0;
> + strList *lst;
> +
> + for (lst = command; lst; lst = lst->next) {
> + argv[i++] = lst->value;
> + }
> +
> + argv[i] = NULL;
> + return;
> +}
> +
> +void exec_start_outgoing_migration(MigrationState *s, strList *command,
> Error **errp)
> {
> QIOChannel *ioc;
>
> -#ifdef WIN32
> - const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
> -#else
> - const char *argv[] = { "/bin/sh", "-c", command, NULL };
> -#endif
> + int length = str_list_length(command);
> + g_auto(GStrv) argv = (char **) g_new0(const char *, length);
>
> - trace_migration_exec_outgoing(command);
> - ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> + init_exec_array(command, argv, errp);
> + g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
> +
> + trace_migration_exec_outgoing(new_command);
> + ioc = QIO_CHANNEL(qio_channel_command_new_spawn((const char * const*) argv,
> O_RDWR,
> errp));
> if (!ioc) {
> @@ -72,18 +100,18 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
> return G_SOURCE_REMOVE;
> }
>
> -void exec_start_incoming_migration(const char *command, Error **errp)
> +void exec_start_incoming_migration(strList *command, Error **errp)
> {
> QIOChannel *ioc;
>
> -#ifdef WIN32
> - const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
> -#else
> - const char *argv[] = { "/bin/sh", "-c", command, NULL };
> -#endif
> + int length = str_list_length(command);
> + g_auto(GStrv) argv = (char **) g_new0(const char *, length);
> +
> + init_exec_array(command, argv, errp);
> + g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
>
> - trace_migration_exec_incoming(command);
> - ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> + trace_migration_exec_incoming(new_command);
> + ioc = QIO_CHANNEL(qio_channel_command_new_spawn((const char * const*) argv,
> O_RDWR,
> errp));
Not sure if the type conversions are 100% correct, maintainers please
advice here on the patch if I am missing something or something is sort
of a bad practise for Qemu ?
> if (!ioc) {
> diff --git a/migration/exec.h b/migration/exec.h
> index 736cd71028..3107f205e3 100644
> --- a/migration/exec.h
> +++ b/migration/exec.h
> @@ -23,8 +23,8 @@
> #ifdef WIN32
> const char *exec_get_cmd_path(void);
> #endif
> -void exec_start_incoming_migration(const char *host_port, Error **errp);
> +void exec_start_incoming_migration(strList *host_port, Error **errp);
>
> -void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> +void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
> Error **errp);
> #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index b7c72fafc9..0a6ab9229b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -465,7 +465,6 @@ static bool migrate_uri_parse(const char *uri, MigrateAddress **channel,
>
> static void qemu_start_incoming_migration(const char *uri, Error **errp)
> {
> - const char *p = NULL;
> g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
>
Regards,
Het Gala
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-19 9:46 ` [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol Het Gala
@ 2023-05-25 17:34 ` Markus Armbruster
2023-05-29 9:37 ` Het Gala
0 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2023-05-25 17:34 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
Het Gala <het.gala@nutanix.com> writes:
> This patch introduces well defined MigrateAddress struct and its related child
> objects.
>
> The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
> string type. The current migration flow follows double encoding scheme for
> fetching migration parameters such as 'uri' and this is not an ideal design.
>
> Motive for intoducing struct level design is to prevent double encoding of QAPI
> arguments, as Qemu should be able to directly use the QAPI arguments without
> any level of encoding.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 179af0c4d8..c500744bb7 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1407,6 +1407,47 @@
> ##
> { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>
> +##
> +# @MigrateTransport:
I'd prefer MigrationTransport, because "migration" is a noun, while
migrate is a verb. Verbs are for commands. For types we use nouns.
More of the same below, not noting it again.
Actually, I'd prefer MigrationAddressType, because it's purpose is to
serve as discriminator type in union MigrationAddress.
> +#
> +# The supported communication transport mechanisms for migration
> +#
> +# @socket: Supported communication type between two devices for migration.
> +# Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> +# 'fd' already
Migration is between hosts, not "two devices".
The second sentence confuses me. What are you trying to say?
Also, missing period at the end.
> +#
> +# @exec: Supported communication type to redirect migration stream into file.
> +#
> +# @rdma: Supported communication type to redirect rdma type migration stream.
What about:
##
# @MigrationTransport:
#
# The migration stream transport mechanisms
#
# @socket: Migrate via socket
#
# @rdma: Migrate via RDMA
#
# @file: Direct the migration stream to a file
> +#
> +# Since 8.1
> +##
> +{ 'enum': 'MigrateTransport',
> + 'data': ['socket', 'exec', 'rdma'] }
> +
> +##
> +# @MigrateExecCommand:
Documentation of @args is missing.
> + #
> + # Since 8.1
> + ##
Unwanted indentation.
> +{ 'struct': 'MigrateExecCommand',
> + 'data': {'args': [ 'str' ] } }
> +
> +##
> +# @MigrateAddress:
> +#
> +# The options available for communication transport mechanisms for migration
Not happy with this sentence (writing good documentation is hard).
Is the address used for the destination only, or for the source as well?
If destination only, could it be used for the source at least in theory?
I'm asking because I need to understand more about intended use to be
able to suggest doc improvements.
> +#
> +# Since 8.1
> +##
> +{ 'union': 'MigrateAddress',
> + 'base': { 'transport' : 'MigrateTransport'},
> + 'discriminator': 'transport',
> + 'data': {
> + 'socket': 'SocketAddress',
> + 'exec': 'MigrateExecCommand',
> + 'rdma': 'InetSocketAddress' } }
> +
Aside: a more powerful type system would let us extend SocketAddress
with additional variants instead of wrapping it in a union.
> ##
> # @migrate:
> #
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration
2023-05-19 9:46 ` [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration Het Gala
@ 2023-05-25 17:50 ` Markus Armbruster
2023-05-29 11:33 ` Het Gala
0 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2023-05-25 17:50 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
Het Gala <het.gala@nutanix.com> writes:
> MigrateChannelList allows to connect accross multiple interfaces. Added
> MigrateChannelList struct as argument to migration QAPIs.
>
> Future patchset series plans to include multiple MigrateChannels
> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
> is the preferred choice of argument over 'MigrateChannel' and making
> migration QAPIs future proof.
>
> For current patchset series, have limited the size of the list to single
> element (single interface) as runtime check.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
[...]
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c500744bb7..86bbc916d1 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1448,12 +1448,47 @@
> 'exec': 'MigrateExecCommand',
> 'rdma': 'InetSocketAddress' } }
>
> +##
> +# @MigrateChannelType:
As mentioned in my review of PATCH 1, I prefer nouns to verbs for types,
i.e. Migration, not Migrate. More of the same below, not flagging it
again.
> +#
> +# The supported options for migration channel type requests
> +#
> +# @main: Support request for main outbound migration control channel
> +#
> +# Since 8.1
> +##
> +{ 'enum': 'MigrateChannelType',
> + 'data': [ 'main' ] }
> +
> +##
> +# @MigrateChannel:
> +#
> +# Information regarding migration Channel-type for transferring packets,
> +# source and corresponding destination interface for socket connection
> +# and number of multifd channels over the interface.
> +#
> +# @channeltype: Name of Channel type for transfering packet information
@channel-type, because "channeltype" is not a word.
> +#
> +# @addr: Information regarding migration parameters of destination interface
> +#
> +# Since 8.1
> +##
> +{ 'struct': 'MigrateChannel',
> + 'data': {
> + 'channeltype': 'MigrateChannelType',
> + 'addr': 'MigrateAddress' } }
> +
> ##
> # @migrate:
> #
> # Migrates the current running guest to another Virtual Machine.
> #
> # @uri: the Uniform Resource Identifier of the destination VM
> +# for migration thread
> +#
> +# @channels: Struct containing list of migration channel types, with all
> +# the information regarding destination interfaces required for
> +# initiating a migration stream.
Please format like
# @uri: the Uniform Resource Identifier of the destination VM for
# migration thread
#
# @channels: Struct containing list of migration channel types, with
# all the information regarding destination interfaces required
# for initiating a migration stream.
to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).
> #
> # @blk: do block migration (full disk copy)
> #
> @@ -1479,14 +1514,44 @@
> # 3. The user Monitor's "detach" argument is invalid in QMP and should
> # not be used
> #
> +# 4. The uri argument should have the Uniform Resource Identifier of default
> +# destination VM. This connection will be bound to default network
> +#
> +# 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
> +# of the two should be present.
> +#
Long lines. Better:
# 4. The uri argument should have the Uniform Resource Identifier of
# default destination VM. This connection will be bound to default
# network
#
# 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly
# one of the two should be present.
> # Example:
> #
> # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
> # <- { "return": {} }
> +# -> { "execute": "migrate",
> +# "arguments": {
> +# "channels": [ { "channeltype": "main",
> +# "addr": { "transport": "socket", "type": "inet",
> +# "host": "10.12.34.9",
> +# "port": "1050" } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +# "arguments": {
> +# "channels": [ { "channeltype": "main",
> +# "addr": { "transport": "exec",
> +# "args": [ "/bin/nc", "-p", "6000",
> +# "/some/sock" ] } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +# "arguments": {
> +# "channels": [ { "channeltype": "main",
> +# "addr": { "transport": "rdma",
> +# "host": "10.12.34.9",
> +# "port": "1050" } } ] } }
> +# <- { "return": {} }
> +#
> ##
> { 'command': 'migrate',
> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> - '*detach': 'bool', '*resume': 'bool' } }
> + 'data': {'*uri': 'str', '*channels': [ 'MigrateChannel' ], '*blk': 'bool',
> + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>
> ##
> # @migrate-incoming:
> @@ -1497,6 +1562,10 @@
> # @uri: The Uniform Resource Identifier identifying the source or
> # address to listen on
> #
> +# @channels: Struct containing list of migration channel types, with all
> +# the information regarding destination interfaces required for
> +# initiating a migration stream.
> +#
The list doesn't contain migration channel types, it contains migration
channels.
I'm not sure what you're trying to say by "with all the information ..."
What does it mean to have multiple channels?
Please format like
# @channels: Struct containing list of migration channel types, with
# all the information regarding destination interfaces required
# for initiating a migration stream.
> # Returns: nothing on success
> #
> # Since: 2.3
> @@ -1512,13 +1581,42 @@
> #
> # 3. The uri format is the same as for -incoming
> #
> +# 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
> +# of the two should be present.
> +#
Long line. Better:
# 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly
# one of the two should be present.
> # Example:
> #
> # -> { "execute": "migrate-incoming",
> # "arguments": { "uri": "tcp::4446" } }
> # <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +# "arguments": {
> +# "channels": [ { "channeltype": "main",
> +# "addr": { "transport": "socket", "type": "inet",
> +# "host": "10.12.34.9",
> +# "port": "1050" } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +# "arguments": {
> +# "channels": [ { "channeltype": "main",
> +# "addr": { "transport": "exec",
> +# "args": [ "/bin/nc", "-p", "6000",
> +# "/some/sock" ] } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +# "arguments": {
> +# "channels": [ { "channeltype": "main",
> +# "addr": { "transport": "rdma",
> +# "host": "10.12.34.9",
> +# "port": "1050" } } ] } }
> +# <- { "return": {} }
> ##
> -{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
> +{ 'command': 'migrate-incoming',
> + 'data': {'*uri': 'str',
> + '*channels': [ 'MigrateChannel' ] } }
>
> ##
> # @xen-save-devices-state:
The text feels cumbersome. Writing good prose is hard, especially when
you're not a native speaker. Eric, would you like to try your hand at
polishing this?
[...]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 8/9] migration: Introduced MigrateChannelList struct to migration code flow.
2023-05-19 9:46 ` [PATCH v5 8/9] migration: Introduced MigrateChannelList struct to migration code flow Het Gala
@ 2023-05-25 18:02 ` Markus Armbruster
2023-05-29 11:35 ` Het Gala
0 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2023-05-25 18:02 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
Het Gala <het.gala@nutanix.com> writes:
> Integrated MigrateChannelList with all transport backends (socket, exec
> and rdma) for both source and destination migration code flow.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
clang warns for me:
../migration/migration.c:497:13: warning: variable 'addrs' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (uri && !migrate_uri_parse(uri, &channel, errp)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../migration/migration.c:503:54: note: uninitialized use occurs here
if (!migration_channels_and_transport_compatible(addrs, errp)) {
^~~~~
../migration/migration.c:497:9: note: remove the 'if' if its condition is always true
if (uri && !migrate_uri_parse(uri, &channel, errp)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../migration/migration.c:497:13: warning: variable 'addrs' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
if (uri && !migrate_uri_parse(uri, &channel, errp)) {
^~~
../migration/migration.c:503:54: note: uninitialized use occurs here
if (!migration_channels_and_transport_compatible(addrs, errp)) {
^~~~~
../migration/migration.c:497:13: note: remove the '&&' if its condition is always true
if (uri && !migrate_uri_parse(uri, &channel, errp)) {
^~~~~~
../migration/migration.c:477:36: note: initialize the variable 'addrs' to silence this warning
g_autoptr(MigrateAddress) addrs;
^
= NULL
../migration/migration.c:1735:13: warning: variable 'addrs' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (uri && !migrate_uri_parse(uri, &channel, errp)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../migration/migration.c:1741:54: note: uninitialized use occurs here
if (!migration_channels_and_transport_compatible(addrs, errp)) {
^~~~~
../migration/migration.c:1735:9: note: remove the 'if' if its condition is always true
if (uri && !migrate_uri_parse(uri, &channel, errp)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../migration/migration.c:1735:13: warning: variable 'addrs' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
if (uri && !migrate_uri_parse(uri, &channel, errp)) {
^~~
../migration/migration.c:1741:54: note: uninitialized use occurs here
if (!migration_channels_and_transport_compatible(addrs, errp)) {
^~~~~
../migration/migration.c:1735:13: note: remove the '&&' if its condition is always true
if (uri && !migrate_uri_parse(uri, &channel, errp)) {
^~~~~~
../migration/migration.c:1715:36: note: initialize the variable 'addrs' to silence this warning
g_autoptr(MigrateAddress) addrs;
^
= NULL
4 warnings generated.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-25 17:34 ` Markus Armbruster
@ 2023-05-29 9:37 ` Het Gala
2023-05-30 6:58 ` Markus Armbruster
0 siblings, 1 reply; 31+ messages in thread
From: Het Gala @ 2023-05-29 9:37 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
On 25/05/23 11:04 pm, Markus Armbruster wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> This patch introduces well defined MigrateAddress struct and its related child
>> objects.
>>
>> The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
>> string type. The current migration flow follows double encoding scheme for
>> fetching migration parameters such as 'uri' and this is not an ideal design.
>>
>> Motive for intoducing struct level design is to prevent double encoding of QAPI
>> arguments, as Qemu should be able to directly use the QAPI arguments without
>> any level of encoding.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>> qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 41 insertions(+)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 179af0c4d8..c500744bb7 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1407,6 +1407,47 @@
>> ##
>> { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>
>> +##
>> +# @MigrateTransport:
> I'd prefer MigrationTransport, because "migration" is a noun, while
> migrate is a verb. Verbs are for commands. For types we use nouns.
> More of the same below, not noting it again.
>
> Actually, I'd prefer MigrationAddressType, because it's purpose is to
> serve as discriminator type in union MigrationAddress.
Okay got it. I kept it Transport as they are different transport
mechanisms. But 'MigrationAddressType' looks cleaner and comaptible with
'MigrateAddress' union too. Will change that
>> +#
>> +# The supported communication transport mechanisms for migration
>> +#
>> +# @socket: Supported communication type between two devices for migration.
>> +# Socket is able to cover all of 'tcp', 'unix', 'vsock' and
>> +# 'fd' already
> Migration is between hosts, not "two devices".
Here we are just talking about socket communication right ? So I thought
devices might also work.
Will change that to 'hosts' as this is in context of migration i.e.
MigrattionAddressType
> The second sentence confuses me. What are you trying to say?
I am trying to say that socket is a union in itslef right, so it covers
communication transport mechanisms like tcp, unix, vsock and fd already
in it.
> Also, missing period at the end.
Ack.
>> +#
>> +# @exec: Supported communication type to redirect migration stream into file.
>> +#
>> +# @rdma: Supported communication type to redirect rdma type migration stream.
> What about:
>
> ##
> # @MigrationTransport:
> #
> # The migration stream transport mechanisms
> #
> # @socket: Migrate via socket
> #
> # @rdma: Migrate via RDMA
> #
> # @file: Direct the migration stream to a file
Should I change from '@exec' to '@file' ?
Other than that, it looks better than what I proposed. Will change it.
>> +#
>> +# Since 8.1
>> +##
>> +{ 'enum': 'MigrateTransport',
>> + 'data': ['socket', 'exec', 'rdma'] }
>> +
>> +##
>> +# @MigrateExecCommand:
> Documentation of @args is missing.
Ack. Should the naming '@args' be replaced by '@filepath' or @path' or
something similar ?
>> + #
>> + # Since 8.1
>> + ##
> Unwanted indentation.
Not able to see any unwanted indentation here ?
>> +{ 'struct': 'MigrateExecCommand',
>> + 'data': {'args': [ 'str' ] } }
>> +
>> +##
>> +# @MigrateAddress:
>> +#
>> +# The options available for communication transport mechanisms for migration
> Not happy with this sentence (writing good documentation is hard).
>
> Is the address used for the destination only, or for the source as well?
>
> If destination only, could it be used for the source at least in theory?
>
> I'm asking because I need to understand more about intended use to be
> able to suggest doc improvements.
This address will be used on both destination and source. In code flow,
in later patches, changes on destination as well as source have been
made to incorporate same definition.
>> +#
>> +# Since 8.1
>> +##
>> +{ 'union': 'MigrateAddress',
>> + 'base': { 'transport' : 'MigrateTransport'},
>> + 'discriminator': 'transport',
>> + 'data': {
>> + 'socket': 'SocketAddress',
>> + 'exec': 'MigrateExecCommand',
>> + 'rdma': 'InetSocketAddress' } }
>> +
> Aside: a more powerful type system would let us extend SocketAddress
> with additional variants instead of wrapping it in a union.
Markus, what do you mean by additional variants here in context of
socket? Can you give a small example.
>> ##
>> # @migrate:
>> #
Regards,
Het Gala
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration
2023-05-25 17:50 ` Markus Armbruster
@ 2023-05-29 11:33 ` Het Gala
2023-05-30 7:13 ` Markus Armbruster
0 siblings, 1 reply; 31+ messages in thread
From: Het Gala @ 2023-05-29 11:33 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
On 25/05/23 11:20 pm, Markus Armbruster wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> MigrateChannelList allows to connect accross multiple interfaces. Added
>> MigrateChannelList struct as argument to migration QAPIs.
>>
>> Future patchset series plans to include multiple MigrateChannels
>> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
>> is the preferred choice of argument over 'MigrateChannel' and making
>> migration QAPIs future proof.
>>
>> For current patchset series, have limited the size of the list to single
>> element (single interface) as runtime check.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
> [...]
>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index c500744bb7..86bbc916d1 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1448,12 +1448,47 @@
>> 'exec': 'MigrateExecCommand',
>> 'rdma': 'InetSocketAddress' } }
>>
>> +##
>> +# @MigrateChannelType:
> As mentioned in my review of PATCH 1, I prefer nouns to verbs for types,
> i.e. Migration, not Migrate. More of the same below, not flagging it
> again.
>
Ack.
Also, I forgot ot mention in the 1st patch - even for union and struct
namings - nouns are preffered over verbs ? or its just for enum types ?
We use structs like - MigrateExecCommand, MigrateChannel -->
MigrationExecCommand, MigrationChannel ?
and union like - MigrateAddress --> MigrationAddress ?
>> +#
>> +# The supported options for migration channel type requests
>> +#
>> +# @main: Support request for main outbound migration control channel
>> +#
>> +# Since 8.1
>> +##
>> +{ 'enum': 'MigrateChannelType',
>> + 'data': [ 'main' ] }
>> +
>> +##
>> +# @MigrateChannel:
>> +#
>> +# Information regarding migration Channel-type for transferring packets,
>> +# source and corresponding destination interface for socket connection
>> +# and number of multifd channels over the interface.
>> +#
>> +# @channeltype: Name of Channel type for transfering packet information
> @channel-type, because "channeltype" is not a word.
Ack.
>> +#
>> +# @addr: Information regarding migration parameters of destination interface
>> +#
>> +# Since 8.1
>> +##
>> +{ 'struct': 'MigrateChannel',
>> + 'data': {
>> + 'channeltype': 'MigrateChannelType',
>> + 'addr': 'MigrateAddress' } }
>> +
>> ##
>> # @migrate:
>> #
>> # Migrates the current running guest to another Virtual Machine.
>> #
>> # @uri: the Uniform Resource Identifier of the destination VM
>> +# for migration thread
>> +#
>> +# @channels: Struct containing list of migration channel types, with all
>> +# the information regarding destination interfaces required for
>> +# initiating a migration stream.
> Please format like
>
> # @uri: the Uniform Resource Identifier of the destination VM for
> # migration thread
> #
> # @channels: Struct containing list of migration channel types, with
> # all the information regarding destination interfaces required
> # for initiating a migration stream.
>
> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
> to conform to current conventions).
Ack. Will change that in the previous patch and will take care in future
patches too. Thanks for informing regarding qapi documentation changes!
>> #
>> # @blk: do block migration (full disk copy)
>> #
>> @@ -1479,14 +1514,44 @@
>> # 3. The user Monitor's "detach" argument is invalid in QMP and should
>> # not be used
>> #
>> +# 4. The uri argument should have the Uniform Resource Identifier of default
>> +# destination VM. This connection will be bound to default network
>> +#
>> +# 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
>> +# of the two should be present.
>> +#
> Long lines. Better:
>
> # 4. The uri argument should have the Uniform Resource Identifier of
> # default destination VM. This connection will be bound to default
> # network
> #
> # 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly
> # one of the two should be present.
Ack.
>> # Example:
>> #
>> # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>> # <- { "return": {} }
>> +# -> { "execute": "migrate",
>> +# "arguments": {
>> +# "channels": [ { "channeltype": "main",
>> +# "addr": { "transport": "socket", "type": "inet",
>> +# "host": "10.12.34.9",
>> +# "port": "1050" } } ] } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +# "arguments": {
>> +# "channels": [ { "channeltype": "main",
>> +# "addr": { "transport": "exec",
>> +# "args": [ "/bin/nc", "-p", "6000",
>> +# "/some/sock" ] } } ] } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +# "arguments": {
>> +# "channels": [ { "channeltype": "main",
>> +# "addr": { "transport": "rdma",
>> +# "host": "10.12.34.9",
>> +# "port": "1050" } } ] } }
>> +# <- { "return": {} }
>> +#
>> ##
>> { 'command': 'migrate',
>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>> - '*detach': 'bool', '*resume': 'bool' } }
>> + 'data': {'*uri': 'str', '*channels': [ 'MigrateChannel' ], '*blk': 'bool',
>> + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>
>> ##
>> # @migrate-incoming:
>> @@ -1497,6 +1562,10 @@
>> # @uri: The Uniform Resource Identifier identifying the source or
>> # address to listen on
>> #
>> +# @channels: Struct containing list of migration channel types, with all
>> +# the information regarding destination interfaces required for
>> +# initiating a migration stream.
>> +#
> The list doesn't contain migration channel types, it contains migration
> channels.
Yes, my bad. Will update it.
> I'm not sure what you're trying to say by "with all the information ..."
>
> What does it mean to have multiple channels?
In future patchset series, we will be introducing channels over
different interfaces (src-dest pair), with each channel having multiple
multifd channels. For now we will restrict the size of the list to 1.
> Please format like
>
> # @channels: Struct containing list of migration channel types, with
> # all the information regarding destination interfaces required
> # for initiating a migration stream.
Ack.
>> # Returns: nothing on success
>> #
>> # Since: 2.3
>> @@ -1512,13 +1581,42 @@
>> #
>> # 3. The uri format is the same as for -incoming
>> #
>> +# 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
>> +# of the two should be present.
>> +#
> Long line. Better:
>
> # 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly
> # one of the two should be present.
Ack.
>> # Example:
>> #
>> # -> { "execute": "migrate-incoming",
>> # "arguments": { "uri": "tcp::4446" } }
>> # <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +# "arguments": {
>> +# "channels": [ { "channeltype": "main",
>> +# "addr": { "transport": "socket", "type": "inet",
>> +# "host": "10.12.34.9",
>> +# "port": "1050" } } ] } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +# "arguments": {
>> +# "channels": [ { "channeltype": "main",
>> +# "addr": { "transport": "exec",
>> +# "args": [ "/bin/nc", "-p", "6000",
>> +# "/some/sock" ] } } ] } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +# "arguments": {
>> +# "channels": [ { "channeltype": "main",
>> +# "addr": { "transport": "rdma",
>> +# "host": "10.12.34.9",
>> +# "port": "1050" } } ] } }
>> +# <- { "return": {} }
>> ##
>> -{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
>> +{ 'command': 'migrate-incoming',
>> + 'data': {'*uri': 'str',
>> + '*channels': [ 'MigrateChannel' ] } }
>>
>> ##
>> # @xen-save-devices-state:
> The text feels cumbersome. Writing good prose is hard, especially when
> you're not a native speaker. Eric, would you like to try your hand at
> polishing this?
> [...]
Regards,
Het Gala
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 8/9] migration: Introduced MigrateChannelList struct to migration code flow.
2023-05-25 18:02 ` Markus Armbruster
@ 2023-05-29 11:35 ` Het Gala
0 siblings, 0 replies; 31+ messages in thread
From: Het Gala @ 2023-05-29 11:35 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
On 25/05/23 11:32 pm, Markus Armbruster wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> Integrated MigrateChannelList with all transport backends (socket, exec
>> and rdma) for both source and destination migration code flow.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
> clang warns for me:
>
> ../migration/migration.c:497:13: warning: variable 'addrs' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../migration/migration.c:503:54: note: uninitialized use occurs here
> if (!migration_channels_and_transport_compatible(addrs, errp)) {
> ^~~~~
> ../migration/migration.c:497:9: note: remove the 'if' if its condition is always true
> if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../migration/migration.c:497:13: warning: variable 'addrs' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
> if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> ^~~
> ../migration/migration.c:503:54: note: uninitialized use occurs here
> if (!migration_channels_and_transport_compatible(addrs, errp)) {
> ^~~~~
> ../migration/migration.c:497:13: note: remove the '&&' if its condition is always true
> if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> ^~~~~~
> ../migration/migration.c:477:36: note: initialize the variable 'addrs' to silence this warning
> g_autoptr(MigrateAddress) addrs;
> ^
> = NULL
> ../migration/migration.c:1735:13: warning: variable 'addrs' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../migration/migration.c:1741:54: note: uninitialized use occurs here
> if (!migration_channels_and_transport_compatible(addrs, errp)) {
> ^~~~~
> ../migration/migration.c:1735:9: note: remove the 'if' if its condition is always true
> if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../migration/migration.c:1735:13: warning: variable 'addrs' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
> if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> ^~~
> ../migration/migration.c:1741:54: note: uninitialized use occurs here
> if (!migration_channels_and_transport_compatible(addrs, errp)) {
> ^~~~~
> ../migration/migration.c:1735:13: note: remove the '&&' if its condition is always true
> if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> ^~~~~~
> ../migration/migration.c:1715:36: note: initialize the variable 'addrs' to silence this warning
> g_autoptr(MigrateAddress) addrs;
> ^
> = NULL
> 4 warnings generated.
Thankyou Markus. Will look into it once again, and make changes in the
next patchset version.
Regards,
Het Gala
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-29 9:37 ` Het Gala
@ 2023-05-30 6:58 ` Markus Armbruster
2023-05-30 7:32 ` Het Gala
0 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2023-05-30 6:58 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
Het Gala <het.gala@nutanix.com> writes:
> On 25/05/23 11:04 pm, Markus Armbruster wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> This patch introduces well defined MigrateAddress struct and its related child
>>> objects.
>>>
>>> The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
>>> string type. The current migration flow follows double encoding scheme for
>>> fetching migration parameters such as 'uri' and this is not an ideal design.
>>>
>>> Motive for intoducing struct level design is to prevent double encoding of QAPI
>>> arguments, as Qemu should be able to directly use the QAPI arguments without
>>> any level of encoding.
>>>
>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>> qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 41 insertions(+)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 179af0c4d8..c500744bb7 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1407,6 +1407,47 @@
>>> ##
>>> { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>> +##
>>> +# @MigrateTransport:
>>
>> I'd prefer MigrationTransport, because "migration" is a noun, while
>> migrate is a verb. Verbs are for commands. For types we use nouns.
>> More of the same below, not noting it again.
>>
>> Actually, I'd prefer MigrationAddressType, because it's purpose is to
>> serve as discriminator type in union MigrationAddress.
>>
> Okay got it. I kept it Transport as they are different transport mechanisms. But 'MigrationAddressType' looks cleaner and comaptible with 'MigrateAddress' union too. Will change that
Transport isn't bad, but I think a type that is only used for a union
discriminator is best named after the union type.
>>> +#
>>> +# The supported communication transport mechanisms for migration
>>> +#
>>> +# @socket: Supported communication type between two devices for migration.
>>> +# Socket is able to cover all of 'tcp', 'unix', 'vsock' and
>>> +# 'fd' already
>>
>> Migration is between hosts, not "two devices".
>
> Here we are just talking about socket communication right ? So I thought devices might also work.
In QEMU, "devices" are the things you create with device_add.
Sockets connect "endpoints". Also called "peers".
> Will change that to 'hosts' as this is in context of migration i.e. MigrattionAddressType
>
>> The second sentence confuses me. What are you trying to say?
>
> I am trying to say that socket is a union in itslef right, so it covers communication transport mechanisms like tcp, unix, vsock and fd already in it.
>
>> Also, missing period at the end.
>
> Ack.
>
>>> +#
>>> +# @exec: Supported communication type to redirect migration stream into file.
>>> +#
>>> +# @rdma: Supported communication type to redirect rdma type migration stream.
>> What about:
>>
>> ##
>> # @MigrationTransport:
>> #
>> # The migration stream transport mechanisms
>> #
>> # @socket: Migrate via socket
>> #
>> # @rdma: Migrate via RDMA
>> #
>> # @file: Direct the migration stream to a file
>
> Should I change from '@exec' to '@file' ?
Uh, that change happened somewhere between my conscious thought process
and the keyboard ;)
What about
# @exec: Direct the migration stream to another process
> Other than that, it looks better than what I proposed. Will change it.
>
>>> +#
>>> +# Since 8.1
>>> +##
>>> +{ 'enum': 'MigrateTransport',
>>> + 'data': ['socket', 'exec', 'rdma'] }
>>> +
>>> +##
>>> +# @MigrateExecCommand:
>
>> Documentation of @args is missing.
>
> Ack. Should the naming '@args' be replaced by '@filepath' or @path' or something similar ?
Depends on what @args means.
I guess its [program, arg1, arg2, ...].
You could split off the program:
'program: 'str',
'args': [ 'str' ]
Try to write clear documentation for both alternatives. Such an
exercise tends to lead me to the one I prefer.
>>> + #
>>> + # Since 8.1
>>> + ##
>>
>> Unwanted indentation.
>
> Not able to see any unwanted indentation here ?
Looks like it got eaten on the way. The last three lines of the doc
comment are indented:
+##
+# @MigrateExecCommand:
+ #
+ # Since 8.1
+ ##
+{ 'struct': 'MigrateExecCommand',
+ 'data': {'args': [ 'str' ] } }
>>> +{ 'struct': 'MigrateExecCommand',
>>> + 'data': {'args': [ 'str' ] } }
>>> +
>>> +##
>>> +# @MigrateAddress:
>>> +#
>>> +# The options available for communication transport mechanisms for migration
>> Not happy with this sentence (writing good documentation is hard).
>>
>> Is the address used for the destination only, or for the source as well?
>>
>> If destination only, could it be used for the source at least in theory?
>>
>> I'm asking because I need to understand more about intended use to be
>> able to suggest doc improvements.
>
> This address will be used on both destination and source. In code flow, in later patches, changes on destination as well as source have been made to incorporate same definition.
Does @exec work as source?
Maybe:
# Endpoint address for migration
or
# Migration endpoint configuration
>>> +#
>>> +# Since 8.1
>>> +##
>>> +{ 'union': 'MigrateAddress',
>>> + 'base': { 'transport' : 'MigrateTransport'},
>>> + 'discriminator': 'transport',
>>> + 'data': {
>>> + 'socket': 'SocketAddress',
>>> + 'exec': 'MigrateExecCommand',
>>> + 'rdma': 'InetSocketAddress' } }
>>> +
>> Aside: a more powerful type system would let us extend SocketAddress
>> with additional variants instead of wrapping it in a union.
>
> Markus, what do you mean by additional variants here in context of socket? Can you give a small example.
As is, we have a nest of two unions:
* The outer union has branches @socket, @exec, @rdma.
* Branch @socket is the inner union, it has branches @inet, @unix, ...
A more powerful type system would let us extend SocketAddress instead,
so MigrateAddress has everything SocketAddress has, plus additional
branches @exec, @rdma. Naturally, the type of the discriminator also
needs to be extended, so that it has everything SocketAddress's
discriminator type has, plus additional members @exec, @rdma.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration
2023-05-29 11:33 ` Het Gala
@ 2023-05-30 7:13 ` Markus Armbruster
2023-05-30 8:45 ` Het Gala
0 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2023-05-30 7:13 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
Het Gala <het.gala@nutanix.com> writes:
> On 25/05/23 11:20 pm, Markus Armbruster wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> MigrateChannelList allows to connect accross multiple interfaces. Added
>>> MigrateChannelList struct as argument to migration QAPIs.
>>>
>>> Future patchset series plans to include multiple MigrateChannels
>>> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
>>> is the preferred choice of argument over 'MigrateChannel' and making
>>> migration QAPIs future proof.
>>>
>>> For current patchset series, have limited the size of the list to single
>>> element (single interface) as runtime check.
>>>
>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>> ---
>> [...]
>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index c500744bb7..86bbc916d1 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1448,12 +1448,47 @@
>>> 'exec': 'MigrateExecCommand',
>>> 'rdma': 'InetSocketAddress' } }
>>> +##
>>> +# @MigrateChannelType:
>>
>> As mentioned in my review of PATCH 1, I prefer nouns to verbs for types,
>> i.e. Migration, not Migrate. More of the same below, not flagging it
>> again.
>>
> Ack.
>
> Also, I forgot ot mention in the 1st patch - even for union and struct namings - nouns are preffered over verbs ? or its just for enum types ?
> We use structs like - MigrateExecCommand, MigrateChannel --> MigrationExecCommand, MigrationChannel ?
> and union like - MigrateAddress --> MigrationAddress ?
The "types are things, and names of things are nouns" argument applies
to all types.
Yes, existing names use verbs in places. Mildly annoying.
Renaming them would not create compatibility problems, as types are not
part of the external interface. Up to migration maintainers.
>>> +#
>>> +# The supported options for migration channel type requests
>>> +#
>>> +# @main: Support request for main outbound migration control channel
>>> +#
>>> +# Since 8.1
>>> +##
>>> +{ 'enum': 'MigrateChannelType',
>>> + 'data': [ 'main' ] }
>>> +
>>> +##
>>> +# @MigrateChannel:
>>> +#
>>> +# Information regarding migration Channel-type for transferring packets,
>>> +# source and corresponding destination interface for socket connection
>>> +# and number of multifd channels over the interface.
>>> +#
>>> +# @channeltype: Name of Channel type for transfering packet information
>>
>> @channel-type, because "channeltype" is not a word.
>
> Ack.
>
>>> +#
>>> +# @addr: Information regarding migration parameters of destination interface
>>> +#
>>> +# Since 8.1
>>> +##
>>> +{ 'struct': 'MigrateChannel',
>>> + 'data': {
>>> + 'channeltype': 'MigrateChannelType',
>>> + 'addr': 'MigrateAddress' } }
>>> +
>>> ##
>>> # @migrate:
>>> #
>>> # Migrates the current running guest to another Virtual Machine.
>>> #
>>> # @uri: the Uniform Resource Identifier of the destination VM
>>> +# for migration thread
>>> +#
>>> +# @channels: Struct containing list of migration channel types, with all
>>> +# the information regarding destination interfaces required for
>>> +# initiating a migration stream.
>>
>> Please format like
>>
>> # @uri: the Uniform Resource Identifier of the destination VM for
>> # migration thread
>> #
>> # @channels: Struct containing list of migration channel types, with
>> # all the information regarding destination interfaces required
>> # for initiating a migration stream.
>>
>> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
>> to conform to current conventions).
>
> Ack. Will change that in the previous patch and will take care in future patches too. Thanks for informing regarding qapi documentation changes!
Gladly! It's the only way to make the nicer formatting stick :)
>>> #
>>> # @blk: do block migration (full disk copy)
>>> #
>>> @@ -1479,14 +1514,44 @@
>>> # 3. The user Monitor's "detach" argument is invalid in QMP and should
>>> # not be used
>>> #
>>> +# 4. The uri argument should have the Uniform Resource Identifier of default
>>> +# destination VM. This connection will be bound to default network
>>> +#
>>> +# 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
>>> +# of the two should be present.
>>> +#
>> Long lines. Better:
>>
>> # 4. The uri argument should have the Uniform Resource Identifier of
>> # default destination VM. This connection will be bound to default
>> # network
>> #
>> # 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly
>> # one of the two should be present.
> Ack.
>>> # Example:
>>> #
>>> # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>>> # <- { "return": {} }
>>> +# -> { "execute": "migrate",
>>> +# "arguments": {
>>> +# "channels": [ { "channeltype": "main",
>>> +# "addr": { "transport": "socket", "type": "inet",
>>> +# "host": "10.12.34.9",
>>> +# "port": "1050" } } ] } }
>>> +# <- { "return": {} }
>>> +#
>>> +# -> { "execute": "migrate",
>>> +# "arguments": {
>>> +# "channels": [ { "channeltype": "main",
>>> +# "addr": { "transport": "exec",
>>> +# "args": [ "/bin/nc", "-p", "6000",
>>> +# "/some/sock" ] } } ] } }
>>> +# <- { "return": {} }
>>> +#
>>> +# -> { "execute": "migrate",
>>> +# "arguments": {
>>> +# "channels": [ { "channeltype": "main",
>>> +# "addr": { "transport": "rdma",
>>> +# "host": "10.12.34.9",
>>> +# "port": "1050" } } ] } }
>>> +# <- { "return": {} }
>>> +#
>>> ##
>>> { 'command': 'migrate',
>>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>>> - '*detach': 'bool', '*resume': 'bool' } }
>>> + 'data': {'*uri': 'str', '*channels': [ 'MigrateChannel' ], '*blk': 'bool',
>>> + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>> ##
>>> # @migrate-incoming:
>>> @@ -1497,6 +1562,10 @@
>>> # @uri: The Uniform Resource Identifier identifying the source or
>>> # address to listen on
>>> #
>>> +# @channels: Struct containing list of migration channel types, with all
>>> +# the information regarding destination interfaces required for
>>> +# initiating a migration stream.
>>> +#
>>
>> The list doesn't contain migration channel types, it contains migration
>> channels.
>
> Yes, my bad. Will update it.
Writing good documentation is hard!
>> I'm not sure what you're trying to say by "with all the information ..."
>>
>> What does it mean to have multiple channels?
>
> In future patchset series, we will be introducing channels over different interfaces (src-dest pair), with each channel having multiple multifd channels. For now we will restrict the size of the list to 1.
Please document this restriction right here.
When you add support for multiple channels, will each channel have a
unique channel type?
[...]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-30 6:58 ` Markus Armbruster
@ 2023-05-30 7:32 ` Het Gala
2023-05-30 7:56 ` Markus Armbruster
` (3 more replies)
0 siblings, 4 replies; 31+ messages in thread
From: Het Gala @ 2023-05-30 7:32 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
[-- Attachment #1: Type: text/plain, Size: 8187 bytes --]
On 30/05/23 12:28 pm, Markus Armbruster wrote:
> Het Gala<het.gala@nutanix.com> writes:
>
>> On 25/05/23 11:04 pm, Markus Armbruster wrote:
>>> Het Gala<het.gala@nutanix.com> writes:
>>>
>>>> This patch introduces well defined MigrateAddress struct and its related child
>>>> objects.
>>>>
>>>> The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
>>>> string type. The current migration flow follows double encoding scheme for
>>>> fetching migration parameters such as 'uri' and this is not an ideal design.
>>>>
>>>> Motive for intoducing struct level design is to prevent double encoding of QAPI
>>>> arguments, as Qemu should be able to directly use the QAPI arguments without
>>>> any level of encoding.
>>>>
>>>> Suggested-by: Aravind Retnakaran<aravind.retnakaran@nutanix.com>
>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>> Reviewed-by: Juan Quintela<quintela@redhat.com>
>>>> Reviewed-by: Daniel P. Berrangé<berrange@redhat.com>
>>>> ---
>>>> qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 41 insertions(+)
>>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index 179af0c4d8..c500744bb7 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -1407,6 +1407,47 @@
>>>> ##
>>>> { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>>> +##
>>>> +# @MigrateTransport:
>>> I'd prefer MigrationTransport, because "migration" is a noun, while
>>> migrate is a verb. Verbs are for commands. For types we use nouns.
>>> More of the same below, not noting it again.
>>>
>>> Actually, I'd prefer MigrationAddressType, because it's purpose is to
>>> serve as discriminator type in union MigrationAddress.
>>>
>> Okay got it. I kept it Transport as they are different transport mechanisms. But 'MigrationAddressType' looks cleaner and comaptible with 'MigrateAddress' union too. Will change that
> Transport isn't bad, but I think a type that is only used for a union
> discriminator is best named after the union type.
Yes I agree with your approach too. Will change it to
'MigrationAddressType' in the next patchset.
>>>> +#
>>>> +# The supported communication transport mechanisms for migration
>>>> +#
>>>> +# @socket: Supported communication type between two devices for migration.
>>>> +# Socket is able to cover all of 'tcp', 'unix', 'vsock' and
>>>> +# 'fd' already
>>> Migration is between hosts, not "two devices".
>> Here we are just talking about socket communication right ? So I thought devices might also work.
> In QEMU, "devices" are the things you create with device_add.
>
> Sockets connect "endpoints". Also called "peers".
Ack. 'endpoints' sounds very appropriate to me.
>> Will change that to 'hosts' as this is in context of migration i.e. MigrattionAddressType
>>
>>> The second sentence confuses me. What are you trying to say?
>> I am trying to say that socket is a union in itslef right, so it covers communication transport mechanisms like tcp, unix, vsock and fd already in it.
>>
>>> Also, missing period at the end.
>> Ack.
>>
>>>> +#
>>>> +# @exec: Supported communication type to redirect migration stream into file.
>>>> +#
>>>> +# @rdma: Supported communication type to redirect rdma type migration stream.
>>> What about:
>>>
>>> ##
>>> # @MigrationTransport:
>>> #
>>> # The migration stream transport mechanisms
>>> #
>>> # @socket: Migrate via socket
>>> #
>>> # @rdma: Migrate via RDMA
>>> #
>>> # @file: Direct the migration stream to a file
>> Should I change from '@exec' to '@file' ?
> Uh, that change happened somewhere between my conscious thought process
> and the keyboard ;)
>
> What about
>
> # @exec: Direct the migration stream to another process
No worries Markus. Seems okay.
>> Other than that, it looks better than what I proposed. Will change it.
>>
>>>> +#
>>>> +# Since 8.1
>>>> +##
>>>> +{ 'enum': 'MigrateTransport',
>>>> + 'data': ['socket', 'exec', 'rdma'] }
>>>> +
>>>> +##
>>>> +# @MigrateExecCommand:
>>> Documentation of @args is missing.
>> Ack. Should the naming '@args' be replaced by '@filepath' or @path' or something similar ?
> Depends on what @args means.
>
> I guess its [program, arg1, arg2, ...].
>
> You could split off the program:
>
> 'program: 'str',
> 'args': [ 'str' ]
>
> Try to write clear documentation for both alternatives. Such an
> exercise tends to lead me to the one I prefer.
Hmm, basically here the @args means, for example ['/bin/bash', args1,
args2, ..., <command>], where command -> /some/file/path.
Does it even make sense now to break into 3 different parts ?
'program': 'str'
'args': [ 'str' ]
'command': 'str'
This might probably just need to tewak something in the exec file I guess.
>>>> + #
>>>> + # Since 8.1
>>>> + ##
>>> Unwanted indentation.
>> Not able to see any unwanted indentation here ?
> Looks like it got eaten on the way. The last three lines of the doc
> comment are indented:
>
> +##
> +# @MigrateExecCommand:
> + #
> + # Since 8.1
> + ##
> +{ 'struct': 'MigrateExecCommand',
> + 'data': {'args': [ 'str' ] } }
Yes, you are right. I figured out after replying to you and was looking
at the code. Thanks for noticing it out! Will change spurious
indentation in the v6.
>>>> +{ 'struct': 'MigrateExecCommand',
>>>> + 'data': {'args': [ 'str' ] } }
>>>> +
>>>> +##
>>>> +# @MigrateAddress:
>>>> +#
>>>> +# The options available for communication transport mechanisms for migration
>>> Not happy with this sentence (writing good documentation is hard).
>>>
>>> Is the address used for the destination only, or for the source as well?
>>>
>>> If destination only, could it be used for the source at least in theory?
>>>
>>> I'm asking because I need to understand more about intended use to be
>>> able to suggest doc improvements.
>> This address will be used on both destination and source. In code flow, in later patches, changes on destination as well as source have been made to incorporate same definition.
> Does @exec work as source?
>
> Maybe:
>
> # Endpoint address for migration
>
> or
>
> # Migration endpoint configuration
I think @exec work as source too, because in exec.c file, there are
calls for souce as well as destination.
I would like to go with "Migration endpoint configuration" because I
feel 'migrate' and 'migrate-incoming' QAPIs are defined in context of
live migration.
>>>> +#
>>>> +# Since 8.1
>>>> +##
>>>> +{ 'union': 'MigrateAddress',
>>>> + 'base': { 'transport' : 'MigrateTransport'},
>>>> + 'discriminator': 'transport',
>>>> + 'data': {
>>>> + 'socket': 'SocketAddress',
>>>> + 'exec': 'MigrateExecCommand',
>>>> + 'rdma': 'InetSocketAddress' } }
>>>> +
>>> Aside: a more powerful type system would let us extend SocketAddress
>>> with additional variants instead of wrapping it in a union.
>> Markus, what do you mean by additional variants here in context of socket? Can you give a small example.
> As is, we have a nest of two unions:
>
> * The outer union has branches @socket, @exec, @rdma.
>
> * Branch @socket is the inner union, it has branches @inet, @unix, ...
>
> A more powerful type system would let us extend SocketAddress instead,
> so MigrateAddress has everything SocketAddress has, plus additional
> branches @exec, @rdma. Naturally, the type of the discriminator also
> needs to be extended, so that it has everything SocketAddress's
> discriminator type has, plus additional members @exec, @rdma.
>
Okay, so you mean something like :
+# Since 8.1
+##
+{ 'union': 'MigrateAddress',
+ 'base': { 'transport' : 'MigrateTransport'},
+ 'discriminator': 'transport',
+ 'data': {
+ 'inet': 'InetSocketAddress',
+ 'unix': 'UnixSocketAddress',
+ 'vsock': 'VsockSocketAddress',
+ 'fd': 'str',
+ 'exec': 'MigrateExecCommand',
+ 'rdma': 'InetSocketAddress' } }
Even I agree that directly leveraging this is the best option, but then
wouldn't it introduce redundancy ? we would not be able to leverage
socket union right in that case or am I missing something.
Regards,
Het Gala
[-- Attachment #2: Type: text/html, Size: 12447 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-30 7:32 ` Het Gala
@ 2023-05-30 7:56 ` Markus Armbruster
2023-05-30 8:56 ` Het Gala
2023-05-30 8:57 ` Daniel P. Berrangé
` (2 subsequent siblings)
3 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2023-05-30 7:56 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
Het Gala <het.gala@nutanix.com> writes:
> On 30/05/23 12:28 pm, Markus Armbruster wrote:
>> Het Gala<het.gala@nutanix.com> writes:
>>
>>> On 25/05/23 11:04 pm, Markus Armbruster wrote:
[...]
>>>> Aside: a more powerful type system would let us extend SocketAddress
>>>> with additional variants instead of wrapping it in a union.
>>>
>>> Markus, what do you mean by additional variants here in context of socket? Can you give a small example.
>>
>> As is, we have a nest of two unions:
>>
>> * The outer union has branches @socket, @exec, @rdma.
>>
>> * Branch @socket is the inner union, it has branches @inet, @unix, ...
>>
>> A more powerful type system would let us extend SocketAddress instead,
>> so MigrateAddress has everything SocketAddress has, plus additional
>> branches @exec, @rdma. Naturally, the type of the discriminator also
>> needs to be extended, so that it has everything SocketAddress's
>> discriminator type has, plus additional members @exec, @rdma.
>
> Okay, so you mean something like :
>
> +# Since 8.1
> +##
> +{ 'union': 'MigrateAddress',
> + 'base': { 'transport' : 'MigrateTransport'},
> + 'discriminator': 'transport',
> + 'data': {
> + 'inet': 'InetSocketAddress',
> + 'unix': 'UnixSocketAddress',
> + 'vsock': 'VsockSocketAddress',
> + 'fd': 'str',
> + 'exec': 'MigrateExecCommand',
> + 'rdma': 'InetSocketAddress' } }
>
> Even I agree that directly leveraging this is the best option, but then wouldn't it introduce redundancy ? we would not be able to leverage socket union right in that case or am I missing something.
Yes, there's redundancy, due to QAPI's insufficient expressive power.
Is the cleaner external interface worth the (internal) redundancy, and
possibly coding complications that go with it?
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration
2023-05-30 7:13 ` Markus Armbruster
@ 2023-05-30 8:45 ` Het Gala
2023-05-30 9:17 ` Markus Armbruster
0 siblings, 1 reply; 31+ messages in thread
From: Het Gala @ 2023-05-30 8:45 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
On 30/05/23 12:43 pm, Markus Armbruster wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> On 25/05/23 11:20 pm, Markus Armbruster wrote:
>>> Het Gala <het.gala@nutanix.com> writes:
>>>
>>>> MigrateChannelList allows to connect accross multiple interfaces. Added
>>>> MigrateChannelList struct as argument to migration QAPIs.
>>>>
>>>> Future patchset series plans to include multiple MigrateChannels
>>>> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
>>>> is the preferred choice of argument over 'MigrateChannel' and making
>>>> migration QAPIs future proof.
>>>>
>>>> For current patchset series, have limited the size of the list to single
>>>> element (single interface) as runtime check.
>>>>
>>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>>> ---
>>> [...]
>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index c500744bb7..86bbc916d1 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -1448,12 +1448,47 @@
>>>> 'exec': 'MigrateExecCommand',
>>>> 'rdma': 'InetSocketAddress' } }
>>>> +##
>>>> +# @MigrateChannelType:
>>> As mentioned in my review of PATCH 1, I prefer nouns to verbs for types,
>>> i.e. Migration, not Migrate. More of the same below, not flagging it
>>> again.
>>>
>> Ack.
>>
>> Also, I forgot ot mention in the 1st patch - even for union and struct namings - nouns are preffered over verbs ? or its just for enum types ?
>> We use structs like - MigrateExecCommand, MigrateChannel --> MigrationExecCommand, MigrationChannel ?
>> and union like - MigrateAddress --> MigrationAddress ?
> The "types are things, and names of things are nouns" argument applies
> to all types.
>
> Yes, existing names use verbs in places. Mildly annoying.
>
> Renaming them would not create compatibility problems, as types are not
> part of the external interface. Up to migration maintainers.
Yes, I got your point. Will change naming convention for all 'types' as
nouns other than 'command' which will be a verb. Will kep this in mind
in future too. Thanks!
>>>> +#
>>>> +# The supported options for migration channel type requests
>>>> +#
>>>> +# @main: Support request for main outbound migration control channel
>>>> +#
>>>> +# Since 8.1
>>>> +##
>>>> +{ 'enum': 'MigrateChannelType',
>>>> + 'data': [ 'main' ] }
>>>> +
>>>> +##
>>>> +# @MigrateChannel:
>>>> +#
>>>> +# Information regarding migration Channel-type for transferring packets,
>>>> +# source and corresponding destination interface for socket connection
>>>> +# and number of multifd channels over the interface.
>>>> +#
>>>> +# @channeltype: Name of Channel type for transfering packet information
>>> @channel-type, because "channeltype" is not a word.
>> Ack.
>>
>>>> +#
>>>> +# @addr: Information regarding migration parameters of destination interface
>>>> +#
>>>> +# Since 8.1
>>>> +##
>>>> +{ 'struct': 'MigrateChannel',
>>>> + 'data': {
>>>> + 'channeltype': 'MigrateChannelType',
>>>> + 'addr': 'MigrateAddress' } }
>>>> +
>>>> ##
>>>> # @migrate:
>>>> #
>>>> # Migrates the current running guest to another Virtual Machine.
>>>> #
>>>> # @uri: the Uniform Resource Identifier of the destination VM
>>>> +# for migration thread
>>>> +#
>>>> +# @channels: Struct containing list of migration channel types, with all
>>>> +# the information regarding destination interfaces required for
>>>> +# initiating a migration stream.
>>> Please format like
>>>
>>> # @uri: the Uniform Resource Identifier of the destination VM for
>>> # migration thread
>>> #
>>> # @channels: Struct containing list of migration channel types, with
>>> # all the information regarding destination interfaces required
>>> # for initiating a migration stream.
>>>
>>> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
>>> to conform to current conventions).
>> Ack. Will change that in the previous patch and will take care in future patches too. Thanks for informing regarding qapi documentation changes!
> Gladly! It's the only way to make the nicer formatting stick :)
Yes 😅
>
>>>> #
>>>> # @blk: do block migration (full disk copy)
>>>> #
>>>> @@ -1479,14 +1514,44 @@
>>>> # 3. The user Monitor's "detach" argument is invalid in QMP and should
>>>> # not be used
>>>> #
>>>> +# 4. The uri argument should have the Uniform Resource Identifier of default
>>>> +# destination VM. This connection will be bound to default network
>>>> +#
>>>> +# 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
>>>> +# of the two should be present.
>>>> +#
>>> Long lines. Better:
>>>
>>> # 4. The uri argument should have the Uniform Resource Identifier of
>>> # default destination VM. This connection will be bound to default
>>> # network
>>> #
>>> # 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly
>>> # one of the two should be present.
>> Ack.
>>>> # Example:
>>>> #
>>>> # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>>>> # <- { "return": {} }
>>>> +# -> { "execute": "migrate",
>>>> +# "arguments": {
>>>> +# "channels": [ { "channeltype": "main",
>>>> +# "addr": { "transport": "socket", "type": "inet",
>>>> +# "host": "10.12.34.9",
>>>> +# "port": "1050" } } ] } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +# -> { "execute": "migrate",
>>>> +# "arguments": {
>>>> +# "channels": [ { "channeltype": "main",
>>>> +# "addr": { "transport": "exec",
>>>> +# "args": [ "/bin/nc", "-p", "6000",
>>>> +# "/some/sock" ] } } ] } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +# -> { "execute": "migrate",
>>>> +# "arguments": {
>>>> +# "channels": [ { "channeltype": "main",
>>>> +# "addr": { "transport": "rdma",
>>>> +# "host": "10.12.34.9",
>>>> +# "port": "1050" } } ] } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> ##
>>>> { 'command': 'migrate',
>>>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>>>> - '*detach': 'bool', '*resume': 'bool' } }
>>>> + 'data': {'*uri': 'str', '*channels': [ 'MigrateChannel' ], '*blk': 'bool',
>>>> + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>>> ##
>>>> # @migrate-incoming:
>>>> @@ -1497,6 +1562,10 @@
>>>> # @uri: The Uniform Resource Identifier identifying the source or
>>>> # address to listen on
>>>> #
>>>> +# @channels: Struct containing list of migration channel types, with all
>>>> +# the information regarding destination interfaces required for
>>>> +# initiating a migration stream.
>>>> +#
>>> The list doesn't contain migration channel types, it contains migration
>>> channels.
>> Yes, my bad. Will update it.
> Writing good documentation is hard!
>
>>> I'm not sure what you're trying to say by "with all the information ..."
>>>
>>> What does it mean to have multiple channels?
>> In future patchset series, we will be introducing channels over different interfaces (src-dest pair), with each channel having multiple multifd channels. For now we will restrict the size of the list to 1.
> Please document this restriction right here.
Ack. But it is mainly an implementation point that's the reason I did
not mention it here but have mentioned it in the migration code flow
path. Will add one more point to note down.
> When you add support for multiple channels, will each channel have a
> unique channel type?
Not every channel will have a unique channel type but mixture like, for
multifd in future : (main, data, data, data) --> the first connection
will be migration main connection, other three will be multifd connections.
> [...]
Regards,
Het Gala
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-30 7:56 ` Markus Armbruster
@ 2023-05-30 8:56 ` Het Gala
2023-05-30 10:34 ` Daniel P. Berrangé
0 siblings, 1 reply; 31+ messages in thread
From: Het Gala @ 2023-05-30 8:56 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
On 30/05/23 1:26 pm, Markus Armbruster wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> On 30/05/23 12:28 pm, Markus Armbruster wrote:
>>> Het Gala<het.gala@nutanix.com> writes:
>>>
>>>> On 25/05/23 11:04 pm, Markus Armbruster wrote:
> [...]
>
>>>>> Aside: a more powerful type system would let us extend SocketAddress
>>>>> with additional variants instead of wrapping it in a union.
>>>> Markus, what do you mean by additional variants here in context of socket? Can you give a small example.
>>> As is, we have a nest of two unions:
>>>
>>> * The outer union has branches @socket, @exec, @rdma.
>>>
>>> * Branch @socket is the inner union, it has branches @inet, @unix, ...
>>>
>>> A more powerful type system would let us extend SocketAddress instead,
>>> so MigrateAddress has everything SocketAddress has, plus additional
>>> branches @exec, @rdma. Naturally, the type of the discriminator also
>>> needs to be extended, so that it has everything SocketAddress's
>>> discriminator type has, plus additional members @exec, @rdma.
>> Okay, so you mean something like :
>>
>> +# Since 8.1
>> +##
>> +{ 'union': 'MigrateAddress',
>> + 'base': { 'transport' : 'MigrateTransport'},
>> + 'discriminator': 'transport',
>> + 'data': {
>> + 'inet': 'InetSocketAddress',
>> + 'unix': 'UnixSocketAddress',
>> + 'vsock': 'VsockSocketAddress',
>> + 'fd': 'str',
>> + 'exec': 'MigrateExecCommand',
>> + 'rdma': 'InetSocketAddress' } }
>>
>> Even I agree that directly leveraging this is the best option, but then wouldn't it introduce redundancy ? we would not be able to leverage socket union right in that case or am I missing something.
> Yes, there's redundancy, due to QAPI's insufficient expressive power.
>
> Is the cleaner external interface worth the (internal) redundancy, and
> possibly coding complications that go with it?
Honestly, I would like to have it this way, where the user is aware of
all the transport mechanisms available. But I guess for external
interface problem statement, the migration code flow has similar path
for SocketAddress and non-socketAddress (exec and rdma). So even if on
the QAPI side we express explicitly all the transport mechanisms, while
implementing it we either would need to brinf them under a single
umbrella. For now, I would keep the implementation as it is (union
inside a union) but would want to have a more powerful and better
appraoch out there if possible.
I would like to have Migration maintainers - Juan, Peter Xu and others
to comment on what approach from the above two is a better one.
Regards,
Het Gala
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-30 7:32 ` Het Gala
2023-05-30 7:56 ` Markus Armbruster
@ 2023-05-30 8:57 ` Daniel P. Berrangé
2023-05-30 9:02 ` Het Gala
2023-05-30 11:38 ` Het Gala
2023-05-30 12:10 ` Daniel P. Berrangé
3 siblings, 1 reply; 31+ messages in thread
From: Daniel P. Berrangé @ 2023-05-30 8:57 UTC (permalink / raw)
To: Het Gala
Cc: Markus Armbruster, qemu-devel, prerna.saxena, quintela, dgilbert,
pbonzini, eblake, manish.mishra, aravind.retnakaran
On Tue, May 30, 2023 at 01:02:27PM +0530, Het Gala wrote:
>
> On 30/05/23 12:28 pm, Markus Armbruster wrote:
> > Het Gala<het.gala@nutanix.com> writes:
> >
> > > On 25/05/23 11:04 pm, Markus Armbruster wrote:
> > > > Het Gala<het.gala@nutanix.com> writes:
> > > >
> > > > > This patch introduces well defined MigrateAddress struct and its related child
> > > > > objects.
> > > > >
> > > > > The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
> > > > > string type. The current migration flow follows double encoding scheme for
> > > > > fetching migration parameters such as 'uri' and this is not an ideal design.
> > > > >
> > > > > Motive for intoducing struct level design is to prevent double encoding of QAPI
> > > > > arguments, as Qemu should be able to directly use the QAPI arguments without
> > > > > any level of encoding.
> > > > >
> > > > > Suggested-by: Aravind Retnakaran<aravind.retnakaran@nutanix.com>
> > > > > Signed-off-by: Het Gala<het.gala@nutanix.com>
> > > > > Reviewed-by: Juan Quintela<quintela@redhat.com>
> > > > > Reviewed-by: Daniel P. Berrangé<berrange@redhat.com>
> > > > > ---
> > > > > qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 41 insertions(+)
> > > > >
> > > > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > > > index 179af0c4d8..c500744bb7 100644
> > > > > --- a/qapi/migration.json
> > > > > +++ b/qapi/migration.json
> > > > > @@ -1407,6 +1407,47 @@
> > > > > ##
> > > > > { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
> > > > > +##
> > > > > +# @MigrateTransport:
> > > > I'd prefer MigrationTransport, because "migration" is a noun, while
> > > > migrate is a verb. Verbs are for commands. For types we use nouns.
> > > > More of the same below, not noting it again.
> > > >
> > > > Actually, I'd prefer MigrationAddressType, because it's purpose is to
> > > > serve as discriminator type in union MigrationAddress.
> > > >
> > > Okay got it. I kept it Transport as they are different transport mechanisms. But 'MigrationAddressType' looks cleaner and comaptible with 'MigrateAddress' union too. Will change that
> > Transport isn't bad, but I think a type that is only used for a union
> > discriminator is best named after the union type.
> Yes I agree with your approach too. Will change it to 'MigrationAddressType'
> in the next patchset.
> > > > > +#
> > > > > +# The supported communication transport mechanisms for migration
> > > > > +#
> > > > > +# @socket: Supported communication type between two devices for migration.
> > > > > +# Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> > > > > +# 'fd' already
> > > > Migration is between hosts, not "two devices".
> > > Here we are just talking about socket communication right ? So I thought devices might also work.
> > In QEMU, "devices" are the things you create with device_add.
> >
> > Sockets connect "endpoints". Also called "peers".
> Ack. 'endpoints' sounds very appropriate to me.
> > > Will change that to 'hosts' as this is in context of migration i.e. MigrattionAddressType
> > >
> > > > The second sentence confuses me. What are you trying to say?
> > > I am trying to say that socket is a union in itslef right, so it covers communication transport mechanisms like tcp, unix, vsock and fd already in it.
> > >
> > > > Also, missing period at the end.
> > > Ack.
> > >
> > > > > +#
> > > > > +# @exec: Supported communication type to redirect migration stream into file.
> > > > > +#
> > > > > +# @rdma: Supported communication type to redirect rdma type migration stream.
> > > > What about:
> > > >
> > > > ##
> > > > # @MigrationTransport:
> > > > #
> > > > # The migration stream transport mechanisms
> > > > #
> > > > # @socket: Migrate via socket
> > > > #
> > > > # @rdma: Migrate via RDMA
> > > > #
> > > > # @file: Direct the migration stream to a file
> > > Should I change from '@exec' to '@file' ?
> > Uh, that change happened somewhere between my conscious thought process
> > and the keyboard ;)
> >
> > What about
> >
> > # @exec: Direct the migration stream to another process
> No worries Markus. Seems okay.
> > > Other than that, it looks better than what I proposed. Will change it.
> > >
> > > > > +#
> > > > > +# Since 8.1
> > > > > +##
> > > > > +{ 'enum': 'MigrateTransport',
> > > > > + 'data': ['socket', 'exec', 'rdma'] }
> > > > > +
> > > > > +##
> > > > > +# @MigrateExecCommand:
> > > > Documentation of @args is missing.
> > > Ack. Should the naming '@args' be replaced by '@filepath' or @path' or something similar ?
> > Depends on what @args means.
> >
> > I guess its [program, arg1, arg2, ...].
> >
> > You could split off the program:
> >
> > 'program: 'str',
> > 'args': [ 'str' ]
> >
> > Try to write clear documentation for both alternatives. Such an
> > exercise tends to lead me to the one I prefer.
>
> Hmm, basically here the @args means, for example ['/bin/bash', args1, args2,
> ..., <command>], where command -> /some/file/path.
>
> Does it even make sense now to break into 3 different parts ?
>
> 'program': 'str'
> 'args': [ 'str' ]
> 'command': 'str'
>
> This might probably just need to tewak something in the exec file I guess.
>
> > > > > + #
> > > > > + # Since 8.1
> > > > > + ##
> > > > Unwanted indentation.
> > > Not able to see any unwanted indentation here ?
> > Looks like it got eaten on the way. The last three lines of the doc
> > comment are indented:
> >
> > +##
> > +# @MigrateExecCommand:
> > + #
> > + # Since 8.1
> > + ##
> > +{ 'struct': 'MigrateExecCommand',
> > + 'data': {'args': [ 'str' ] } }
> Yes, you are right. I figured out after replying to you and was looking at
> the code. Thanks for noticing it out! Will change spurious indentation in
> the v6.
> > > > > +{ 'struct': 'MigrateExecCommand',
> > > > > + 'data': {'args': [ 'str' ] } }
> > > > > +
> > > > > +##
> > > > > +# @MigrateAddress:
> > > > > +#
> > > > > +# The options available for communication transport mechanisms for migration
> > > > Not happy with this sentence (writing good documentation is hard).
> > > >
> > > > Is the address used for the destination only, or for the source as well?
> > > >
> > > > If destination only, could it be used for the source at least in theory?
> > > >
> > > > I'm asking because I need to understand more about intended use to be
> > > > able to suggest doc improvements.
> > > This address will be used on both destination and source. In code flow, in later patches, changes on destination as well as source have been made to incorporate same definition.
> > Does @exec work as source?
> >
> > Maybe:
> >
> > # Endpoint address for migration
> >
> > or
> >
> > # Migration endpoint configuration
>
> I think @exec work as source too, because in exec.c file, there are calls
> for souce as well as destination.
>
> I would like to go with "Migration endpoint configuration" because I feel
> 'migrate' and 'migrate-incoming' QAPIs are defined in context of live
> migration.
>
> > > > > +#
> > > > > +# Since 8.1
> > > > > +##
> > > > > +{ 'union': 'MigrateAddress',
> > > > > + 'base': { 'transport' : 'MigrateTransport'},
> > > > > + 'discriminator': 'transport',
> > > > > + 'data': {
> > > > > + 'socket': 'SocketAddress',
> > > > > + 'exec': 'MigrateExecCommand',
> > > > > + 'rdma': 'InetSocketAddress' } }
> > > > > +
> > > > Aside: a more powerful type system would let us extend SocketAddress
> > > > with additional variants instead of wrapping it in a union.
> > > Markus, what do you mean by additional variants here in context of socket? Can you give a small example.
> > As is, we have a nest of two unions:
> >
> > * The outer union has branches @socket, @exec, @rdma.
> >
> > * Branch @socket is the inner union, it has branches @inet, @unix, ...
> >
> > A more powerful type system would let us extend SocketAddress instead,
> > so MigrateAddress has everything SocketAddress has, plus additional
> > branches @exec, @rdma. Naturally, the type of the discriminator also
> > needs to be extended, so that it has everything SocketAddress's
> > discriminator type has, plus additional members @exec, @rdma.
> >
> Okay, so you mean something like :
>
> +# Since 8.1
> +##
> +{ 'union': 'MigrateAddress',
> + 'base': { 'transport' : 'MigrateTransport'},
> + 'discriminator': 'transport',
> + 'data': {
> + 'inet': 'InetSocketAddress',
> + 'unix': 'UnixSocketAddress',
> + 'vsock': 'VsockSocketAddress',
> + 'fd': 'str',
> + 'exec': 'MigrateExecCommand',
> + 'rdma': 'InetSocketAddress' } }
>
> Even I agree that directly leveraging this is the best option, but then
> wouldn't it introduce redundancy ? we would not be able to leverage socket
> union right in that case or am I missing something.
The first four are going to have to be packed back into a SocketAddress
struct immediately, as the internal migration APIs all work in terms of
a SocketAddress for the inet/unix/vsock/fd case.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-30 8:57 ` Daniel P. Berrangé
@ 2023-05-30 9:02 ` Het Gala
0 siblings, 0 replies; 31+ messages in thread
From: Het Gala @ 2023-05-30 9:02 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Markus Armbruster, qemu-devel, prerna.saxena, quintela, dgilbert,
pbonzini, eblake, manish.mishra, aravind.retnakaran
On 30/05/23 2:27 pm, Daniel P. Berrangé wrote:
> On Tue, May 30, 2023 at 01:02:27PM +0530, Het Gala wrote:
>> On 30/05/23 12:28 pm, Markus Armbruster wrote:
>>> Het Gala<het.gala@nutanix.com> writes:
>>>
>>>> On 25/05/23 11:04 pm, Markus Armbruster wrote:
>>>>> Het Gala<het.gala@nutanix.com> writes:
>>>>>
>>>>>> This patch introduces well defined MigrateAddress struct and its related child
>>>>>> objects.
>>>>>>
>>>>>> The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
>>>>>> string type. The current migration flow follows double encoding scheme for
>>>>>> fetching migration parameters such as 'uri' and this is not an ideal design.
>>>>>>
>>>>>> Motive for intoducing struct level design is to prevent double encoding of QAPI
>>>>>> arguments, as Qemu should be able to directly use the QAPI arguments without
>>>>>> any level of encoding.
>>>>>>
>>>>>> Suggested-by: Aravind Retnakaran<aravind.retnakaran@nutanix.com>
>>>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>>>> Reviewed-by: Juan Quintela<quintela@redhat.com>
>>>>>> Reviewed-by: Daniel P. Berrangé<berrange@redhat.com>
>>>>>> ---
>>>>>> qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 41 insertions(+)
>>>>>>
>>>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>>>> index 179af0c4d8..c500744bb7 100644
>>>>>> --- a/qapi/migration.json
>>>>>> +++ b/qapi/migration.json
>>>>>> @@ -1407,6 +1407,47 @@
>>>>>> ##
>>>>>> { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>>>>> +##
>>>>>> +# @MigrateTransport:
>>>>> I'd prefer MigrationTransport, because "migration" is a noun, while
>>>>> migrate is a verb. Verbs are for commands. For types we use nouns.
>>>>> More of the same below, not noting it again.
>>>>>
>>>>> Actually, I'd prefer MigrationAddressType, because it's purpose is to
>>>>> serve as discriminator type in union MigrationAddress.
>>>>>
>>>> Okay got it. I kept it Transport as they are different transport mechanisms. But 'MigrationAddressType' looks cleaner and comaptible with 'MigrateAddress' union too. Will change that
>>> Transport isn't bad, but I think a type that is only used for a union
>>> discriminator is best named after the union type.
>> Yes I agree with your approach too. Will change it to 'MigrationAddressType'
>> in the next patchset.
>>>>>> +#
>>>>>> +# The supported communication transport mechanisms for migration
>>>>>> +#
>>>>>> +# @socket: Supported communication type between two devices for migration.
>>>>>> +# Socket is able to cover all of 'tcp', 'unix', 'vsock' and
>>>>>> +# 'fd' already
>>>>> Migration is between hosts, not "two devices".
>>>> Here we are just talking about socket communication right ? So I thought devices might also work.
>>> In QEMU, "devices" are the things you create with device_add.
>>>
>>> Sockets connect "endpoints". Also called "peers".
>> Ack. 'endpoints' sounds very appropriate to me.
>>>> Will change that to 'hosts' as this is in context of migration i.e. MigrattionAddressType
>>>>
>>>>> The second sentence confuses me. What are you trying to say?
>>>> I am trying to say that socket is a union in itslef right, so it covers communication transport mechanisms like tcp, unix, vsock and fd already in it.
>>>>
>>>>> Also, missing period at the end.
>>>> Ack.
>>>>
>>>>>> +#
>>>>>> +# @exec: Supported communication type to redirect migration stream into file.
>>>>>> +#
>>>>>> +# @rdma: Supported communication type to redirect rdma type migration stream.
>>>>> What about:
>>>>>
>>>>> ##
>>>>> # @MigrationTransport:
>>>>> #
>>>>> # The migration stream transport mechanisms
>>>>> #
>>>>> # @socket: Migrate via socket
>>>>> #
>>>>> # @rdma: Migrate via RDMA
>>>>> #
>>>>> # @file: Direct the migration stream to a file
>>>> Should I change from '@exec' to '@file' ?
>>> Uh, that change happened somewhere between my conscious thought process
>>> and the keyboard ;)
>>>
>>> What about
>>>
>>> # @exec: Direct the migration stream to another process
>> No worries Markus. Seems okay.
>>>> Other than that, it looks better than what I proposed. Will change it.
>>>>
>>>>>> +#
>>>>>> +# Since 8.1
>>>>>> +##
>>>>>> +{ 'enum': 'MigrateTransport',
>>>>>> + 'data': ['socket', 'exec', 'rdma'] }
>>>>>> +
>>>>>> +##
>>>>>> +# @MigrateExecCommand:
>>>>> Documentation of @args is missing.
>>>> Ack. Should the naming '@args' be replaced by '@filepath' or @path' or something similar ?
>>> Depends on what @args means.
>>>
>>> I guess its [program, arg1, arg2, ...].
>>>
>>> You could split off the program:
>>>
>>> 'program: 'str',
>>> 'args': [ 'str' ]
>>>
>>> Try to write clear documentation for both alternatives. Such an
>>> exercise tends to lead me to the one I prefer.
>> Hmm, basically here the @args means, for example ['/bin/bash', args1, args2,
>> ..., <command>], where command -> /some/file/path.
>>
>> Does it even make sense now to break into 3 different parts ?
>>
>> 'program': 'str'
>> 'args': [ 'str' ]
>> 'command': 'str'
>>
>> This might probably just need to tewak something in the exec file I guess.
>>
>>>>>> + #
>>>>>> + # Since 8.1
>>>>>> + ##
>>>>> Unwanted indentation.
>>>> Not able to see any unwanted indentation here ?
>>> Looks like it got eaten on the way. The last three lines of the doc
>>> comment are indented:
>>>
>>> +##
>>> +# @MigrateExecCommand:
>>> + #
>>> + # Since 8.1
>>> + ##
>>> +{ 'struct': 'MigrateExecCommand',
>>> + 'data': {'args': [ 'str' ] } }
>> Yes, you are right. I figured out after replying to you and was looking at
>> the code. Thanks for noticing it out! Will change spurious indentation in
>> the v6.
>>>>>> +{ 'struct': 'MigrateExecCommand',
>>>>>> + 'data': {'args': [ 'str' ] } }
>>>>>> +
>>>>>> +##
>>>>>> +# @MigrateAddress:
>>>>>> +#
>>>>>> +# The options available for communication transport mechanisms for migration
>>>>> Not happy with this sentence (writing good documentation is hard).
>>>>>
>>>>> Is the address used for the destination only, or for the source as well?
>>>>>
>>>>> If destination only, could it be used for the source at least in theory?
>>>>>
>>>>> I'm asking because I need to understand more about intended use to be
>>>>> able to suggest doc improvements.
>>>> This address will be used on both destination and source. In code flow, in later patches, changes on destination as well as source have been made to incorporate same definition.
>>> Does @exec work as source?
>>>
>>> Maybe:
>>>
>>> # Endpoint address for migration
>>>
>>> or
>>>
>>> # Migration endpoint configuration
>> I think @exec work as source too, because in exec.c file, there are calls
>> for souce as well as destination.
>>
>> I would like to go with "Migration endpoint configuration" because I feel
>> 'migrate' and 'migrate-incoming' QAPIs are defined in context of live
>> migration.
>>
>>>>>> +#
>>>>>> +# Since 8.1
>>>>>> +##
>>>>>> +{ 'union': 'MigrateAddress',
>>>>>> + 'base': { 'transport' : 'MigrateTransport'},
>>>>>> + 'discriminator': 'transport',
>>>>>> + 'data': {
>>>>>> + 'socket': 'SocketAddress',
>>>>>> + 'exec': 'MigrateExecCommand',
>>>>>> + 'rdma': 'InetSocketAddress' } }
>>>>>> +
>>>>> Aside: a more powerful type system would let us extend SocketAddress
>>>>> with additional variants instead of wrapping it in a union.
>>>> Markus, what do you mean by additional variants here in context of socket? Can you give a small example.
>>> As is, we have a nest of two unions:
>>>
>>> * The outer union has branches @socket, @exec, @rdma.
>>>
>>> * Branch @socket is the inner union, it has branches @inet, @unix, ...
>>>
>>> A more powerful type system would let us extend SocketAddress instead,
>>> so MigrateAddress has everything SocketAddress has, plus additional
>>> branches @exec, @rdma. Naturally, the type of the discriminator also
>>> needs to be extended, so that it has everything SocketAddress's
>>> discriminator type has, plus additional members @exec, @rdma.
>>>
>> Okay, so you mean something like :
>>
>> +# Since 8.1
>> +##
>> +{ 'union': 'MigrateAddress',
>> + 'base': { 'transport' : 'MigrateTransport'},
>> + 'discriminator': 'transport',
>> + 'data': {
>> + 'inet': 'InetSocketAddress',
>> + 'unix': 'UnixSocketAddress',
>> + 'vsock': 'VsockSocketAddress',
>> + 'fd': 'str',
>> + 'exec': 'MigrateExecCommand',
>> + 'rdma': 'InetSocketAddress' } }
>>
>> Even I agree that directly leveraging this is the best option, but then
>> wouldn't it introduce redundancy ? we would not be able to leverage socket
>> union right in that case or am I missing something.
> The first four are going to have to be packed back into a SocketAddress
> struct immediately, as the internal migration APIs all work in terms of
> a SocketAddress for the inet/unix/vsock/fd case.
Concur, that's what I mentioned in just earlier reply, Daniel.
> With regards,
> Daniel
Regards,
Het Gala
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration
2023-05-30 8:45 ` Het Gala
@ 2023-05-30 9:17 ` Markus Armbruster
0 siblings, 0 replies; 31+ messages in thread
From: Markus Armbruster @ 2023-05-30 9:17 UTC (permalink / raw)
To: Het Gala
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
Het Gala <het.gala@nutanix.com> writes:
> On 30/05/23 12:43 pm, Markus Armbruster wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> On 25/05/23 11:20 pm, Markus Armbruster wrote:
>>>> Het Gala <het.gala@nutanix.com> writes:
>>>>
>>>>> MigrateChannelList allows to connect accross multiple interfaces. Added
>>>>> MigrateChannelList struct as argument to migration QAPIs.
>>>>>
>>>>> Future patchset series plans to include multiple MigrateChannels
>>>>> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
>>>>> is the preferred choice of argument over 'MigrateChannel' and making
>>>>> migration QAPIs future proof.
>>>>>
>>>>> For current patchset series, have limited the size of the list to single
>>>>> element (single interface) as runtime check.
>>>>>
>>>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
[...]
>>>>> @@ -1497,6 +1562,10 @@
>>>>> # @uri: The Uniform Resource Identifier identifying the source or
>>>>> # address to listen on
>>>>> #
>>>>> +# @channels: Struct containing list of migration channel types, with all
>>>>> +# the information regarding destination interfaces required for
>>>>> +# initiating a migration stream.
>>>>> +#
>>>> The list doesn't contain migration channel types, it contains migration
>>>> channels.
>>>
>>> Yes, my bad. Will update it.
>>
>> Writing good documentation is hard!
>>
>>>> I'm not sure what you're trying to say by "with all the information ..."
>>>>
>>>> What does it mean to have multiple channels?
>>>
>>> In future patchset series, we will be introducing channels over different interfaces (src-dest pair), with each channel having multiple multifd channels. For now we will restrict the size of the list to 1.
>>
>> Please document this restriction right here.
>
> Ack. But it is mainly an implementation point that's the reason I did not mention it here but have mentioned it in the migration code flow path. Will add one more point to note down.
Documenting temporary restrictions is work that's useful only
temporarily.
When a restriction goes away within the same patch series, not doing
that work can make sense. I'd still want it mentioned in the commit
message.
It's tempting treat restrictions expected to go away before we release
the same. But when lifting the restriction misses the release, it's
easy to forget we still have a restriction to document. Better document
it right away.
>> When you add support for multiple channels, will each channel have a
>> unique channel type?
>
> Not every channel will have a unique channel type but mixture like, for multifd in future : (main, data, data, data) --> the first connection will be migration main connection, other three will be multifd connections.
Got it, thanks!
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-30 8:56 ` Het Gala
@ 2023-05-30 10:34 ` Daniel P. Berrangé
0 siblings, 0 replies; 31+ messages in thread
From: Daniel P. Berrangé @ 2023-05-30 10:34 UTC (permalink / raw)
To: Het Gala
Cc: Markus Armbruster, qemu-devel, prerna.saxena, quintela, dgilbert,
pbonzini, eblake, manish.mishra, aravind.retnakaran
On Tue, May 30, 2023 at 02:26:23PM +0530, Het Gala wrote:
>
> On 30/05/23 1:26 pm, Markus Armbruster wrote:
> > Het Gala <het.gala@nutanix.com> writes:
> >
> > > On 30/05/23 12:28 pm, Markus Armbruster wrote:
> > > > Het Gala<het.gala@nutanix.com> writes:
> > > >
> > > > > On 25/05/23 11:04 pm, Markus Armbruster wrote:
> > [...]
> >
> > > > > > Aside: a more powerful type system would let us extend SocketAddress
> > > > > > with additional variants instead of wrapping it in a union.
> > > > > Markus, what do you mean by additional variants here in context of socket? Can you give a small example.
> > > > As is, we have a nest of two unions:
> > > >
> > > > * The outer union has branches @socket, @exec, @rdma.
> > > >
> > > > * Branch @socket is the inner union, it has branches @inet, @unix, ...
> > > >
> > > > A more powerful type system would let us extend SocketAddress instead,
> > > > so MigrateAddress has everything SocketAddress has, plus additional
> > > > branches @exec, @rdma. Naturally, the type of the discriminator also
> > > > needs to be extended, so that it has everything SocketAddress's
> > > > discriminator type has, plus additional members @exec, @rdma.
> > > Okay, so you mean something like :
> > >
> > > +# Since 8.1
> > > +##
> > > +{ 'union': 'MigrateAddress',
> > > + 'base': { 'transport' : 'MigrateTransport'},
> > > + 'discriminator': 'transport',
> > > + 'data': {
> > > + 'inet': 'InetSocketAddress',
> > > + 'unix': 'UnixSocketAddress',
> > > + 'vsock': 'VsockSocketAddress',
> > > + 'fd': 'str',
> > > + 'exec': 'MigrateExecCommand',
> > > + 'rdma': 'InetSocketAddress' } }
> > >
> > > Even I agree that directly leveraging this is the best option, but then wouldn't it introduce redundancy ? we would not be able to leverage socket union right in that case or am I missing something.
> > Yes, there's redundancy, due to QAPI's insufficient expressive power.
> >
> > Is the cleaner external interface worth the (internal) redundancy, and
> > possibly coding complications that go with it?
>
> Honestly, I would like to have it this way, where the user is aware of all
> the transport mechanisms available. But I guess for external interface
> problem statement, the migration code flow has similar path for
> SocketAddress and non-socketAddress (exec and rdma). So even if on the QAPI
> side we express explicitly all the transport mechanisms, while implementing
> it we either would need to brinf them under a single umbrella. For now, I
> would keep the implementation as it is (union inside a union) but would want
> to have a more powerful and better appraoch out there if possible.
IMHO, unpacking the SocketAddress types into the two level is not something
we need to do. The main (perhaps even only) reason it arises as a design
question is because historically the migration public interface has NOT
used SocketAddress, and exposed unix/inet/vsock as distinct top level
options, and this taints our thoughts.
If the "migrate" command didn't already exist and we were adding it now
IMHO we would just expose 'socket': 'SocketAdress' as a top level type,
in parallel with 'exec' and 'rdma', and not even discuss the idea of
unpacking 'SocketAddress'.
IMHO the compelling reason to NOT unpack 'SocketAddress' is that it lets
the migration code seemlessly benefit from any new 'SocketAddress' types
that gets implemented. eg consider when 'vsock' was added to QEMU. Any
command that took a 'SocketAddress' parameter needed no QAPI changes
and usually no code changes either.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-30 7:32 ` Het Gala
2023-05-30 7:56 ` Markus Armbruster
2023-05-30 8:57 ` Daniel P. Berrangé
@ 2023-05-30 11:38 ` Het Gala
2023-05-30 12:10 ` Daniel P. Berrangé
3 siblings, 0 replies; 31+ messages in thread
From: Het Gala @ 2023-05-30 11:38 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, prerna.saxena, quintela, dgilbert, pbonzini, berrange,
eblake, manish.mishra, aravind.retnakaran
[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]
On 30/05/23 1:02 pm, Het Gala wrote:
>
>
> On 30/05/23 12:28 pm, Markus Armbruster wrote:
>> Het Gala<het.gala@nutanix.com> writes:
>>
>>> On 25/05/23 11:04 pm, Markus Armbruster wrote:
>>>> Het Gala<het.gala@nutanix.com> writes:
>>>>
>>>>> This patch introduces well defined MigrateAddress struct and its related child
>>>>> objects.
>>>>>
>>>>> The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
>>>>> string type. The current migration flow follows double encoding scheme for
>>>>> fetching migration parameters such as 'uri' and this is not an ideal design.
>>>>>
>>>>> Motive for intoducing struct level design is to prevent double encoding of QAPI
>>>>> arguments, as Qemu should be able to directly use the QAPI arguments without
>>>>> any level of encoding.
>>>>>
>>>>> Other than that, it looks better than what I proposed. Will change it.
[...]
>>>>> +# Since 8.1
>>>>> +##
>>>>> +{ 'enum': 'MigrateTransport',
>>>>> + 'data': ['socket', 'exec', 'rdma'] }
>>>>> +
>>>>> +##
>>>>> +# @MigrateExecCommand:
>>>> Documentation of @args is missing.
>>> Ack. Should the naming '@args' be replaced by '@filepath' or @path' or something similar ?
>> Depends on what @args means.
>>
>> I guess its [program, arg1, arg2, ...].
>>
>> You could split off the program:
>>
>> 'program: 'str',
>> 'args': [ 'str' ]
>>
>> Try to write clear documentation for both alternatives. Such an
>> exercise tends to lead me to the one I prefer.
>
> Hmm, basically here the @args means, for example ['/bin/bash', args1,
> args2, ..., <command>], where command -> /some/file/path.
>
> Does it even make sense now to break into 3 different parts ?
>
> 'program': 'str'
> 'args': [ 'str' ]
> 'command': 'str'
>
> This might probably just need to tewak something in the exec file I
> guess.
>
Markus, Daniel - any comments on this ?
>>>>> + #
>>>>> + # Since 8.1
>>>>> + ##
>>>> Unwanted indentation.
>>> Not able to see any unwanted indentation here ?
>> Looks like it got eaten on the way. The last three lines of the doc
>> comment are indented:
>>
>> +##
>> +# @MigrateExecCommand:
>> + #
>> + # Since 8.1
>> + ##
>> +{ 'struct': 'MigrateExecCommand',
>> + 'data': {'args': [ 'str' ] } }
[...]
Regards,
Het Gala
[-- Attachment #2: Type: text/html, Size: 4451 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-30 7:32 ` Het Gala
` (2 preceding siblings ...)
2023-05-30 11:38 ` Het Gala
@ 2023-05-30 12:10 ` Daniel P. Berrangé
2023-06-01 8:56 ` Het Gala
3 siblings, 1 reply; 31+ messages in thread
From: Daniel P. Berrangé @ 2023-05-30 12:10 UTC (permalink / raw)
To: Het Gala
Cc: Markus Armbruster, qemu-devel, prerna.saxena, quintela, dgilbert,
pbonzini, eblake, manish.mishra, aravind.retnakaran
On Tue, May 30, 2023 at 01:02:27PM +0530, Het Gala wrote:
>
> On 30/05/23 12:28 pm, Markus Armbruster wrote:
> > Het Gala<het.gala@nutanix.com> writes:
> >
> > > On 25/05/23 11:04 pm, Markus Armbruster wrote:
> > > > Het Gala<het.gala@nutanix.com> writes:
> > > >
> > > > > This patch introduces well defined MigrateAddress struct and its related child
> > > > > objects.
> > > > >
> > > > > The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
> > > > > string type. The current migration flow follows double encoding scheme for
> > > > > fetching migration parameters such as 'uri' and this is not an ideal design.
> > > > >
> > > > > Motive for intoducing struct level design is to prevent double encoding of QAPI
> > > > > arguments, as Qemu should be able to directly use the QAPI arguments without
> > > > > any level of encoding.
> > > > >
> > > > > Suggested-by: Aravind Retnakaran<aravind.retnakaran@nutanix.com>
> > > > > Signed-off-by: Het Gala<het.gala@nutanix.com>
> > > > > Reviewed-by: Juan Quintela<quintela@redhat.com>
> > > > > Reviewed-by: Daniel P. Berrangé<berrange@redhat.com>
> > > > > ---
> > > > > qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 41 insertions(+)
> > > > >
> > > > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > > > index 179af0c4d8..c500744bb7 100644
> > > > > --- a/qapi/migration.json
> > > > > +++ b/qapi/migration.json
> > > > > @@ -1407,6 +1407,47 @@
> > > > > ##
> > > > > { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
> > > > > +##
> > > > > +# @MigrateTransport:
> > > > I'd prefer MigrationTransport, because "migration" is a noun, while
> > > > migrate is a verb. Verbs are for commands. For types we use nouns.
> > > > More of the same below, not noting it again.
> > > >
> > > > Actually, I'd prefer MigrationAddressType, because it's purpose is to
> > > > serve as discriminator type in union MigrationAddress.
> > > >
> > > Okay got it. I kept it Transport as they are different transport mechanisms. But 'MigrationAddressType' looks cleaner and comaptible with 'MigrateAddress' union too. Will change that
> > Transport isn't bad, but I think a type that is only used for a union
> > discriminator is best named after the union type.
> Yes I agree with your approach too. Will change it to 'MigrationAddressType'
> in the next patchset.
> > > > > +#
> > > > > +# The supported communication transport mechanisms for migration
> > > > > +#
> > > > > +# @socket: Supported communication type between two devices for migration.
> > > > > +# Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> > > > > +# 'fd' already
> > > > Migration is between hosts, not "two devices".
> > > Here we are just talking about socket communication right ? So I thought devices might also work.
> > In QEMU, "devices" are the things you create with device_add.
> >
> > Sockets connect "endpoints". Also called "peers".
> Ack. 'endpoints' sounds very appropriate to me.
> > > Will change that to 'hosts' as this is in context of migration i.e. MigrattionAddressType
> > >
> > > > The second sentence confuses me. What are you trying to say?
> > > I am trying to say that socket is a union in itslef right, so it covers communication transport mechanisms like tcp, unix, vsock and fd already in it.
> > >
> > > > Also, missing period at the end.
> > > Ack.
> > >
> > > > > +#
> > > > > +# @exec: Supported communication type to redirect migration stream into file.
> > > > > +#
> > > > > +# @rdma: Supported communication type to redirect rdma type migration stream.
> > > > What about:
> > > >
> > > > ##
> > > > # @MigrationTransport:
> > > > #
> > > > # The migration stream transport mechanisms
> > > > #
> > > > # @socket: Migrate via socket
> > > > #
> > > > # @rdma: Migrate via RDMA
> > > > #
> > > > # @file: Direct the migration stream to a file
> > > Should I change from '@exec' to '@file' ?
> > Uh, that change happened somewhere between my conscious thought process
> > and the keyboard ;)
> >
> > What about
> >
> > # @exec: Direct the migration stream to another process
> No worries Markus. Seems okay.
> > > Other than that, it looks better than what I proposed. Will change it.
> > >
> > > > > +#
> > > > > +# Since 8.1
> > > > > +##
> > > > > +{ 'enum': 'MigrateTransport',
> > > > > + 'data': ['socket', 'exec', 'rdma'] }
> > > > > +
> > > > > +##
> > > > > +# @MigrateExecCommand:
> > > > Documentation of @args is missing.
> > > Ack. Should the naming '@args' be replaced by '@filepath' or @path' or something similar ?
> > Depends on what @args means.
> >
> > I guess its [program, arg1, arg2, ...].
Yes, that is correct. Essentially this ends up calling
execve(@args[0], @args)
> > You could split off the program:
> >
> > 'program: 'str',
> > 'args': [ 'str' ]
Now you also have to declare whether '@args' includes or excludes
'program' as @args[0]. execve() style would be to have '@args[0]'
duplicate 'program'. Personally I think that's overkill for QEMU's
needs. If we don't include 'program' in @args, then we have to
document this, as it isn't discoverable from the QAPI design.
Not separating 'program' and 'args' in QAPI makes it unambiguous
that 'args' must include everything.
> > Try to write clear documentation for both alternatives. Such an
> > exercise tends to lead me to the one I prefer.
>
> Hmm, basically here the @args means, for example ['/bin/bash', args1, args2,
> ..., <command>], where command -> /some/file/path.
>
> Does it even make sense now to break into 3 different parts ?
>
> 'program': 'str'
> 'args': [ 'str' ]
> 'command': 'str'
Definitely not. This encodes an assumption that we're spawning via a
shell. The intent with the new design is that it lets mgmt apps fully
eliminate use of shell, and directly invoke the program, thus eliminating
potential (security) pitfalls with shell metacharacters.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.
2023-05-30 12:10 ` Daniel P. Berrangé
@ 2023-06-01 8:56 ` Het Gala
0 siblings, 0 replies; 31+ messages in thread
From: Het Gala @ 2023-06-01 8:56 UTC (permalink / raw)
To: Daniel P. Berrangé, Markus Armbruster
Cc: Markus Armbruster, qemu-devel, prerna.saxena, quintela, dgilbert,
pbonzini, eblake, manish.mishra, aravind.retnakaran
On 30/05/23 5:40 pm, Daniel P. Berrangé wrote:
> On Tue, May 30, 2023 at 01:02:27PM +0530, Het Gala wrote:
>> On 30/05/23 12:28 pm, Markus Armbruster wrote:
>>> Het Gala<het.gala@nutanix.com> writes:
>>>
>>>
[...]
>>>>>> +##
>>>>>> +{ 'enum': 'MigrateTransport',
>>>>>> + 'data': ['socket', 'exec', 'rdma'] }
>>>>>> +
>>>>>> +##
>>>>>> +# @MigrateExecCommand:
>>>>> Documentation of @args is missing.
>>>> Ack. Should the naming '@args' be replaced by '@filepath' or @path' or something similar ?
>>> Depends on what @args means.
>>>
>>> I guess its [program, arg1, arg2, ...].
> Yes, that is correct. Essentially this ends up calling
>
> execve(@args[0], @args)
>
>>> You could split off the program:
>>>
>>> 'program: 'str',
>>> 'args': [ 'str' ]
> Now you also have to declare whether '@args' includes or excludes
> 'program' as @args[0]. execve() style would be to have '@args[0]'
> duplicate 'program'. Personally I think that's overkill for QEMU's
> needs. If we don't include 'program' in @args, then we have to
> document this, as it isn't discoverable from the QAPI design.
>
> Not separating 'program' and 'args' in QAPI makes it unambiguous
> that 'args' must include everything.
Yes, we need to make user aware of args[0], so keeping it @args along
with adding a note that @args[0] - path to the new program ? is the best
alternative here ? - Markus, Daniel
>>> Try to write clear documentation for both alternatives. Such an
>>> exercise tends to lead me to the one I prefer.
>> Hmm, basically here the @args means, for example ['/bin/bash', args1, args2,
>> ..., <command>], where command -> /some/file/path.
>>
>> Does it even make sense now to break into 3 different parts ?
>>
>> 'program': 'str'
>> 'args': [ 'str' ]
>> 'command': 'str'
> Definitely not. This encodes an assumption that we're spawning via a
> shell. The intent with the new design is that it lets mgmt apps fully
> eliminate use of shell, and directly invoke the program, thus eliminating
> potential (security) pitfalls with shell metacharacters.
Got your point Daniel. Thanks.
> With regards,
> Daniel
Regards,
Het Gala
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2023-06-01 8:57 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-19 9:46 [PATCH v5 0/9] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Het Gala
2023-05-19 9:46 ` [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol Het Gala
2023-05-25 17:34 ` Markus Armbruster
2023-05-29 9:37 ` Het Gala
2023-05-30 6:58 ` Markus Armbruster
2023-05-30 7:32 ` Het Gala
2023-05-30 7:56 ` Markus Armbruster
2023-05-30 8:56 ` Het Gala
2023-05-30 10:34 ` Daniel P. Berrangé
2023-05-30 8:57 ` Daniel P. Berrangé
2023-05-30 9:02 ` Het Gala
2023-05-30 11:38 ` Het Gala
2023-05-30 12:10 ` Daniel P. Berrangé
2023-06-01 8:56 ` Het Gala
2023-05-19 9:46 ` [PATCH v5 2/9] migration: convert uri parameter into 'MigrateAddress' struct Het Gala
2023-05-19 9:46 ` [PATCH v5 3/9] migration: convert socket backend to accept MigrateAddress struct Het Gala
2023-05-19 9:46 ` [PATCH v5 4/9] migration: convert rdma " Het Gala
2023-05-19 9:46 ` [PATCH v5 5/9] migration: convert exec " Het Gala
2023-05-19 9:52 ` Het Gala
2023-05-19 9:46 ` [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration Het Gala
2023-05-25 17:50 ` Markus Armbruster
2023-05-29 11:33 ` Het Gala
2023-05-30 7:13 ` Markus Armbruster
2023-05-30 8:45 ` Het Gala
2023-05-30 9:17 ` Markus Armbruster
2023-05-19 9:46 ` [PATCH v5 7/9] migration: modify migration_channels_and_uri_compatible() to incorporate newer migration QAPI syntax Het Gala
2023-05-19 9:46 ` [PATCH v5 8/9] migration: Introduced MigrateChannelList struct to migration code flow Het Gala
2023-05-25 18:02 ` Markus Armbruster
2023-05-29 11:35 ` Het Gala
2023-05-19 9:46 ` [PATCH v5 9/9] migration: adding test case for modified QAPI syntax Het Gala
2023-05-19 9:49 ` Het Gala
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).