| Message ID | 20231012151052.154106-1-het.gala@nutanix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration | expand |
Fabiano, would your below commits impact this patchset 'make check' tests ? Because you have added tests for file based migration, which is still not included in this patchset. tests/qtest: migration-test: Add tests for file-based migration tests/qtest/migration: Add a test for the analyze-migration script I have tried to address all the g_autoptr() issues observed eariler, and make check tests failing as a result. All the tests were passing (even -qcow2 181) when the patch was posted for review. What can be the next steps here for us? Do we need to add support for file based migration in these patches or as you said eariler, you will introduce those patches on top of my patches. Please let me know. On 12/10/23 8:40 pm, Het Gala wrote: > This is v13 patchset of modified 'migrate' and 'migrate-incoming' QAPI design > for upstream review. > > Would like to thank all the maintainers that actively participated in the v12 > 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 > v5: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04845.html > v6: https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg01251.html > v7: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02027.html > v8: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02770.html > v9: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg04216.html > v10: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg05022.html > v11: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg00740.html > v12: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg02422.html > > v12 -> v13 changelog: > ------------------- > - qcow2 181 test was only failing. Fixed the code to pass that test > - Added HMP side error propagation in case user entered invalid or wrong uri > for migration. > > 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 (10): > migration: New QAPI type 'MigrateAddress' > migration: convert migration 'uri' into 'MigrateAddress' > migration: convert socket backend to accept MigrateAddress > migration: convert rdma backend to accept MigrateAddress > migration: convert exec backend to accept MigrateAddress. > migration: New migrate and migrate-incoming argument 'channels' > migration: modify migration_channels_and_uri_compatible() for new QAPI > syntax > migration: Implement MigrateChannelList to qmp migration flow. > migration: Implement MigrateChannelList to hmp migration flow. > migration: modify test_multifd_tcp_none() to use new QAPI syntax. > > migration/exec.c | 74 +++++++++---- > migration/exec.h | 8 +- > migration/migration-hmp-cmds.c | 27 ++++- > migration/migration.c | 189 +++++++++++++++++++++++++++------ > migration/migration.h | 3 +- > migration/rdma.c | 33 ++---- > migration/rdma.h | 6 +- > migration/socket.c | 39 ++----- > migration/socket.h | 7 +- > qapi/migration.json | 150 +++++++++++++++++++++++++- > system/vl.c | 2 +- > tests/qtest/migration-test.c | 7 +- > 12 files changed, 419 insertions(+), 126 deletions(-) > Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > Fabiano, would your below commits impact this patchset 'make check' > tests ? Because you have added tests for file based migration, which is > still not included in this patchset. AFAICS, the tests shouldn't break. > tests/qtest: migration-test: Add tests for file-based migration > tests/qtest/migration: Add a test for the analyze-migration script > > I have tried to address all the g_autoptr() issues observed eariler, and > make check tests failing as a result. All the tests were passing (even > -qcow2 181) when the patch was posted for review. What can be the next > steps here for us? Do we need to add support for file based migration in > these patches or as you said eariler, you will introduce those patches > on top of my patches. Please let me know. Right, your series is next on my queue for reviewing. I'll get to it soon. Here's what I was intending to send on top of it: -->8-- From 0fc533989366a1a1e19737916d65938f64426c9b Mon Sep 17 00:00:00 2001 From: Fabiano Rosas <farosas@suse.de> Date: Tue, 10 Oct 2023 11:01:32 -0300 Subject: [PATCH] migration: Convert the file transport to new migration api Convert the file: URI to support the new QAPI syntax. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/file.c | 22 +++++++--------------- migration/file.h | 8 ++++++-- migration/migration.c | 17 +++++++++++------ qapi/migration.json | 20 ++++++++++++++++++-- 4 files changed, 42 insertions(+), 25 deletions(-) diff --git a/migration/file.c b/migration/file.c index cf5b1bf365..e67c81dd2c 100644 --- a/migration/file.c +++ b/migration/file.c @@ -19,7 +19,7 @@ /* Remove the offset option from @filespec and return it in @offsetp. */ -static int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) +int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) { char *option = strstr(filespec, OFFSET_OPTION); int ret; @@ -36,20 +36,16 @@ static int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp) return 0; } -void file_start_outgoing_migration(MigrationState *s, const char *filespec, +void file_start_outgoing_migration(MigrationState *s, FileMigrationArgs *file_args, Error **errp) { - g_autofree char *filename = g_strdup(filespec); g_autoptr(QIOChannelFile) fioc = NULL; - uint64_t offset = 0; + g_autofree char *filename = g_strdup(file_args->path); + uint64_t offset = file_args->offset; QIOChannel *ioc; trace_migration_file_outgoing(filename); - if (file_parse_offset(filename, &offset, errp)) { - return; - } - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, 0600, errp); if (!fioc) { @@ -73,19 +69,15 @@ static gboolean file_accept_incoming_migration(QIOChannel *ioc, return G_SOURCE_REMOVE; } -void file_start_incoming_migration(const char *filespec, Error **errp) +void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp) { - g_autofree char *filename = g_strdup(filespec); + g_autofree char *filename = g_strdup(file_args->path); QIOChannelFile *fioc = NULL; - uint64_t offset = 0; + uint64_t offset = file_args->offset; QIOChannel *ioc; trace_migration_file_incoming(filename); - if (file_parse_offset(filename, &offset, errp)) { - return; - } - fioc = qio_channel_file_new_path(filename, O_RDONLY, 0, errp); if (!fioc) { return; diff --git a/migration/file.h b/migration/file.h index 90fa4849e0..155f6aab45 100644 --- a/migration/file.h +++ b/migration/file.h @@ -7,8 +7,12 @@ #ifndef QEMU_MIGRATION_FILE_H #define QEMU_MIGRATION_FILE_H -void file_start_incoming_migration(const char *filename, Error **errp); -void file_start_outgoing_migration(MigrationState *s, const char *filename, +#include "qapi/qapi-types-migration.h" + +void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp); + +void file_start_outgoing_migration(MigrationState *s, FileMigrationArgs *file_args, Error **errp); +int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp); #endif diff --git a/migration/migration.c b/migration/migration.c index a651106bff..0b07b7343b 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -468,6 +468,13 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel, } addr->u.socket.type = saddr->type; addr->u.socket.u = saddr->u; + } else if (strstart(uri, "file:", NULL)) { + addr->transport = MIGRATION_ADDRESS_TYPE_FILE; + addr->u.file.path = g_strdup(uri + strlen("file:")); + if (file_parse_offset(addr->u.file.path, &addr->u.file.offset, errp)) { + g_free(addr->u.file.path); + return false; + } } else { error_setg(errp, "unknown migration protocol: %s", uri); return false; @@ -483,7 +490,6 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, MigrationChannelList *channels, Error **errp) { - const char *p = NULL; MigrationChannel *channel = NULL; MigrationAddress *addr = NULL; @@ -535,8 +541,8 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, #endif } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) { exec_start_incoming_migration(addr->u.exec.args, errp); - } else if (strstart(uri, "file:", &p)) { - file_start_incoming_migration(p, errp); + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) { + file_start_incoming_migration(&addr->u.file, errp); } else { error_setg(errp, "unknown migration protocol: %s", uri); } @@ -1760,7 +1766,6 @@ void qmp_migrate(const char *uri, bool has_channels, bool resume_requested; Error *local_err = NULL; MigrationState *s = migrate_get_current(); - const char *p = NULL; MigrationChannel *channel = NULL; MigrationAddress *addr = NULL; @@ -1824,8 +1829,8 @@ void qmp_migrate(const char *uri, bool has_channels, #endif } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) { exec_start_outgoing_migration(s, addr->u.exec.args, &local_err); - } else if (strstart(uri, "file:", &p)) { - file_start_outgoing_migration(s, p, &local_err); + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) { + file_start_outgoing_migration(s, &addr->u.file, &local_err); } else { error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol"); diff --git a/qapi/migration.json b/qapi/migration.json index 7b84c04617..bb0639b9d6 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1489,10 +1489,25 @@ # # @rdma: Migrate via RDMA. # +# @file: Direct the migration stream to a file. +# # Since 8.2 ## { 'enum': 'MigrationAddressType', - 'data': ['socket', 'exec', 'rdma'] } + 'data': ['socket', 'exec', 'rdma', 'file'] } + +## +# @FileMigrationArgs: +# +# @path: file path +# +# @offset: initial offset for the file +# +# Since 8.2 +## +{ 'struct': 'FileMigrationArgs', + 'data': {'path': 'str', + 'offset': 'uint64' } } ## # @MigrationExecCommand: @@ -1517,7 +1532,8 @@ 'data': { 'socket': 'SocketAddress', 'exec': 'MigrationExecCommand', - 'rdma': 'InetSocketAddress' } } + 'rdma': 'InetSocketAddress', + 'file': 'FileMigrationArgs' } } ## # @MigrationChannelType:
On 18/10/23 7:58 pm, Fabiano Rosas wrote: > Het Gala <het.gala@nutanix.com> writes: > >> Fabiano, would your below commits impact this patchset 'make check' >> tests ? Because you have added tests for file based migration, which is >> still not included in this patchset. > AFAICS, the tests shouldn't break. > I tried two builds to run on Qemu CI/CD pipeline, Build without those two commits which involved file based migration - https://gitlab.com/galahet/Qemu/-/pipelines/1041554569 Build with those two commits and only 2 patch of the current patchset - https://gitlab.com/galahet/Qemu/-/pipelines/1041215154 I am not 100% certain but because the 'qtest/migration-test' test failed, I was of the view that might be because of those two commits. Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 18/10/23 7:58 pm, Fabiano Rosas wrote: >> Het Gala <het.gala@nutanix.com> writes: >> >>> Fabiano, would your below commits impact this patchset 'make check' >>> tests ? Because you have added tests for file based migration, which is >>> still not included in this patchset. >> AFAICS, the tests shouldn't break. >> > I tried two builds to run on Qemu CI/CD pipeline, > > Build without those two commits which involved file based migration - > https://gitlab.com/galahet/Qemu/-/pipelines/1041554569 > > Build with those two commits and only 2 patch of the current patchset - > https://gitlab.com/galahet/Qemu/-/pipelines/1041215154 > > I am not 100% certain but because the 'qtest/migration-test' test > failed, I was of the view that might be because of those two commits. I just sent a series on top of this one containing the additions for the file transport support. Let's move the discussion there.
This is v13 patchset of modified 'migrate' and 'migrate-incoming' QAPI design for upstream review. Would like to thank all the maintainers that actively participated in the v12 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 v5: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04845.html v6: https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg01251.html v7: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02027.html v8: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02770.html v9: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg04216.html v10: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg05022.html v11: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg00740.html v12: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg02422.html v12 -> v13 changelog: ------------------- - qcow2 181 test was only failing. Fixed the code to pass that test - Added HMP side error propagation in case user entered invalid or wrong uri for migration. 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 (10): migration: New QAPI type 'MigrateAddress' migration: convert migration 'uri' into 'MigrateAddress' migration: convert socket backend to accept MigrateAddress migration: convert rdma backend to accept MigrateAddress migration: convert exec backend to accept MigrateAddress. migration: New migrate and migrate-incoming argument 'channels' migration: modify migration_channels_and_uri_compatible() for new QAPI syntax migration: Implement MigrateChannelList to qmp migration flow. migration: Implement MigrateChannelList to hmp migration flow. migration: modify test_multifd_tcp_none() to use new QAPI syntax. migration/exec.c | 74 +++++++++---- migration/exec.h | 8 +- migration/migration-hmp-cmds.c | 27 ++++- migration/migration.c | 189 +++++++++++++++++++++++++++------ migration/migration.h | 3 +- migration/rdma.c | 33 ++---- migration/rdma.h | 6 +- migration/socket.c | 39 ++----- migration/socket.h | 7 +- qapi/migration.json | 150 +++++++++++++++++++++++++- system/vl.c | 2 +- tests/qtest/migration-test.c | 7 +- 12 files changed, 419 insertions(+), 126 deletions(-)