| Message ID | 20240223152517.7834-2-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs | expand |
CI Run for the whole patchset series: https://gitlab.com/galahet/Qemu/-/pipelines/1188155391 On 23/02/24 8:55 pm, Het Gala wrote: > Introduce support for adding a 'channels' argument to migrate_qmp_fail, > migrate_incoming_qmp and migrate_qmp functions within the migration qtest > framework, enabling enhanced control over migration scenarios. > > Signed-off-by: Het Gala <het.gala@nutanix.com> > --- > tests/qtest/dbus-vmstate-test.c | 2 +- > tests/qtest/migration-helpers.c | 78 +++++++++++++++++++++++++++---- > tests/qtest/migration-helpers.h | 15 +++--- > tests/qtest/migration-test.c | 43 ++++++++--------- > tests/qtest/virtio-net-failover.c | 8 ++-- > 5 files changed, 105 insertions(+), 41 deletions(-) > > diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c > index 6c990864e3..0ca572e29b 100644 > --- a/tests/qtest/dbus-vmstate-test.c > +++ b/tests/qtest/dbus-vmstate-test.c > @@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test) > > thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop); > > - migrate_qmp(src_qemu, uri, "{}"); > + migrate_qmp(src_qemu, uri, NULL, "{}"); > test->src_qemu = src_qemu; > if (test->migrate_fail) { > wait_for_migration_fail(src_qemu, true); > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c > index e451dbdbed..15c3650b55 100644 > --- a/tests/qtest/migration-helpers.c > +++ b/tests/qtest/migration-helpers.c > @@ -13,6 +13,7 @@ > #include "qemu/osdep.h" > #include "qemu/ctype.h" > #include "qapi/qmp/qjson.h" > +#include "qapi/qmp/qlist.h" > > #include "migration-helpers.h" > > @@ -43,7 +44,67 @@ bool migrate_watch_for_events(QTestState *who, const char *name, > return false; > } > > -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...) > +static QList *MigrationChannelList_to_QList(MigrationChannelList *channels) > +{ > + MigrationChannel *channel = NULL; > + MigrationAddress *addr = NULL; > + SocketAddress *saddr = NULL; > + QList *channelList = qlist_new(); > + QDict *channelDict = qdict_new(); > + QDict *addrDict = qdict_new(); > + > + channel = channels->value; > + if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) { > + fprintf(stderr, "%s: Channel or its type is NULL\n", > + __func__); > + } > + g_assert(channel); > + if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) { > + qdict_put_str(channelDict, "channel-type", g_strdup("main")); > + } > + > + addr = channel->addr; > + if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) { > + fprintf(stderr, "%s: addr or its transport is NULL\n", > + __func__); > + } > + g_assert(addr); > + if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) { > + qdict_put_str(addrDict, "transport", g_strdup("socket")); > + } > + > + saddr = &addr->u.socket; > + if (!saddr) { > + fprintf(stderr, "%s: saddr is NULL\n", __func__); > + } > + g_assert(saddr); > + qdict_put_str(addrDict, "type", SocketAddressType_str(saddr->type)); > + qdict_put_str(addrDict, "port", saddr->u.inet.port); > + qdict_put_str(addrDict, "host", saddr->u.inet.host); > + > + qdict_put_obj(channelDict, "addr", QOBJECT(addrDict)); > + qlist_append_obj(channelList, QOBJECT(channelDict)); > + > + return channelList; > +} > + > +static void migrate_qmp_attach_ports(QDict *args, const char *uri, > + MigrationChannelList *channels) > +{ > + if (uri) { > + g_assert(!qdict_haskey(args, "uri")); > + qdict_put_str(args, "uri", uri); > + } > + > + if (channels) { > + g_assert(!qdict_haskey(args, "channels")); > + QList *channelList = MigrationChannelList_to_QList(channels); > + qdict_put_obj(args, "channels", QOBJECT(channelList)); > + } > +} > + > +void migrate_qmp_fail(QTestState *who, const char *uri, > + MigrationChannelList *channels, const char *fmt, ...) > { > va_list ap; > QDict *args, *err; > @@ -52,8 +113,7 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...) > args = qdict_from_vjsonf_nofail(fmt, ap); > va_end(ap); > > - g_assert(!qdict_haskey(args, "uri")); > - qdict_put_str(args, "uri", uri); > + migrate_qmp_attach_ports(args, uri, channels); > > err = qtest_qmp_assert_failure_ref( > who, "{ 'execute': 'migrate', 'arguments': %p}", args); > @@ -68,7 +128,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...) > * Arguments are built from @fmt... (formatted like > * qobject_from_jsonf_nofail()) with "uri": @uri spliced in. > */ > -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...) > +void migrate_qmp(QTestState *who, const char *uri, > + MigrationChannelList *channels, const char *fmt, ...) > { > va_list ap; > QDict *args; > @@ -77,8 +138,7 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...) > args = qdict_from_vjsonf_nofail(fmt, ap); > va_end(ap); > > - g_assert(!qdict_haskey(args, "uri")); > - qdict_put_str(args, "uri", uri); > + migrate_qmp_attach_ports(args, uri, channels); > > qtest_qmp_assert_success(who, > "{ 'execute': 'migrate', 'arguments': %p}", args); > @@ -95,7 +155,8 @@ void migrate_set_capability(QTestState *who, const char *capability, > capability, value); > } > > -void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...) > +void migrate_incoming_qmp(QTestState *to, const char *uri, > + MigrationChannelList *channels, const char *fmt, ...) > { > va_list ap; > QDict *args, *rsp, *data; > @@ -104,8 +165,7 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...) > args = qdict_from_vjsonf_nofail(fmt, ap); > va_end(ap); > > - g_assert(!qdict_haskey(args, "uri")); > - qdict_put_str(args, "uri", uri); > + migrate_qmp_attach_ports(args, uri, channels); > > migrate_set_capability(to, "events", true); > > diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h > index 3bf7ded1b9..390e386274 100644 > --- a/tests/qtest/migration-helpers.h > +++ b/tests/qtest/migration-helpers.h > @@ -14,6 +14,7 @@ > #define MIGRATION_HELPERS_H > > #include "libqtest.h" > +#include "migration/migration.h" > > typedef struct QTestMigrationState { > bool stop_seen; > @@ -25,15 +26,17 @@ typedef struct QTestMigrationState { > bool migrate_watch_for_events(QTestState *who, const char *name, > QDict *event, void *opaque); > > -G_GNUC_PRINTF(3, 4) > -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...); > +G_GNUC_PRINTF(4, 5) > +void migrate_qmp(QTestState *who, const char *uri, > + MigrationChannelList *channels, const char *fmt, ...); > > -G_GNUC_PRINTF(3, 4) > +G_GNUC_PRINTF(4, 5) > void migrate_incoming_qmp(QTestState *who, const char *uri, > - const char *fmt, ...); > + MigrationChannelList *channels, const char *fmt, ...); > > -G_GNUC_PRINTF(3, 4) > -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...); > +G_GNUC_PRINTF(4, 5) > +void migrate_qmp_fail(QTestState *who, const char *uri, > + MigrationChannelList *channels, const char *fmt, ...); > > void migrate_set_capability(QTestState *who, const char *capability, > bool value); > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > index 8a5bb1752e..e5547b8746 100644 > --- a/tests/qtest/migration-test.c > +++ b/tests/qtest/migration-test.c > @@ -18,6 +18,7 @@ > #include "qemu/module.h" > #include "qemu/option.h" > #include "qemu/range.h" > +#include "migration/migration.h" > #include "qemu/sockets.h" > #include "chardev/char.h" > #include "qapi/qapi-visit-sockets.h" > @@ -1350,7 +1351,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr, > wait_for_suspend(from, &src_state); > > g_autofree char *uri = migrate_get_socket_address(to, "socket-address"); > - migrate_qmp(from, uri, "{}"); > + migrate_qmp(from, uri, NULL, "{}"); > > migrate_wait_for_dirty_mem(from, to); > > @@ -1500,7 +1501,7 @@ static void postcopy_recover_fail(QTestState *from, QTestState *to) > g_assert_cmpint(ret, ==, 1); > > migrate_recover(to, "fd:fd-mig"); > - migrate_qmp(from, "fd:fd-mig", "{'resume': true}"); > + migrate_qmp(from, "fd:fd-mig", NULL, "{'resume': true}"); > > /* > * Make sure both QEMU instances will go into RECOVER stage, then test > @@ -1588,7 +1589,7 @@ static void test_postcopy_recovery_common(MigrateCommon *args) > * Try to rebuild the migration channel using the resume flag and > * the newly created channel > */ > - migrate_qmp(from, uri, "{'resume': true}"); > + migrate_qmp(from, uri, NULL, "{'resume': true}"); > > /* Restore the postcopy bandwidth to unlimited */ > migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0); > @@ -1669,7 +1670,7 @@ static void test_baddest(void) > if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) { > return; > } > - migrate_qmp(from, "tcp:127.0.0.1:0", "{}"); > + migrate_qmp(from, "tcp:127.0.0.1:0", NULL, "{}"); > wait_for_migration_fail(from, false); > test_migrate_end(from, to, false); > } > @@ -1708,7 +1709,7 @@ static void test_analyze_script(void) > uri = g_strdup_printf("exec:cat > %s", file); > > migrate_ensure_converge(from); > - migrate_qmp(from, uri, "{}"); > + migrate_qmp(from, uri, NULL, "{}"); > wait_for_migration_complete(from); > > pid = fork(); > @@ -1773,11 +1774,11 @@ static void test_precopy_common(MigrateCommon *args) > } > > if (args->result == MIG_TEST_QMP_ERROR) { > - migrate_qmp_fail(from, connect_uri, "{}"); > + migrate_qmp_fail(from, connect_uri, NULL, "{}"); > goto finish; > } > > - migrate_qmp(from, connect_uri, "{}"); > + migrate_qmp(from, connect_uri, NULL, "{}"); > > if (args->result != MIG_TEST_SUCCEED) { > bool allow_active = args->result == MIG_TEST_FAIL; > @@ -1869,18 +1870,18 @@ static void test_file_common(MigrateCommon *args, bool stop_src) > } > > if (args->result == MIG_TEST_QMP_ERROR) { > - migrate_qmp_fail(from, connect_uri, "{}"); > + migrate_qmp_fail(from, connect_uri, NULL, "{}"); > goto finish; > } > > - migrate_qmp(from, connect_uri, "{}"); > + migrate_qmp(from, connect_uri, NULL, "{}"); > wait_for_migration_complete(from); > > /* > * We need to wait for the source to finish before starting the > * destination. > */ > - migrate_incoming_qmp(to, connect_uri, "{}"); > + migrate_incoming_qmp(to, connect_uri, NULL, "{}"); > wait_for_migration_complete(to); > > if (stop_src) { > @@ -2029,7 +2030,7 @@ static void test_ignore_shared(void) > /* Wait for the first serial output from the source */ > wait_for_serial("src_serial"); > > - migrate_qmp(from, uri, "{}"); > + migrate_qmp(from, uri, NULL, "{}"); > > migrate_wait_for_dirty_mem(from, to); > > @@ -2387,7 +2388,7 @@ static void *test_migrate_fd_start_hook(QTestState *from, > close(pair[0]); > > /* Start incoming migration from the 1st socket */ > - migrate_incoming_qmp(to, "fd:fd-mig", "{}"); > + migrate_incoming_qmp(to, "fd:fd-mig", NULL, "{}"); > > /* Send the 2nd socket to the target */ > qtest_qmp_fds_assert_success(from, &pair[1], 1, > @@ -2455,7 +2456,7 @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail) > /* Wait for the first serial output from the source */ > wait_for_serial("src_serial"); > > - migrate_qmp(from, uri, "{}"); > + migrate_qmp(from, uri, NULL, "{}"); > > if (should_fail) { > qtest_set_expected_status(to, EXIT_FAILURE); > @@ -2558,7 +2559,7 @@ static void test_migrate_auto_converge(void) > /* Wait for the first serial output from the source */ > wait_for_serial("src_serial"); > > - migrate_qmp(from, uri, "{}"); > + migrate_qmp(from, uri, NULL, "{}"); > > /* Wait for throttling begins */ > percentage = 0; > @@ -2609,7 +2610,7 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState *from, > migrate_set_capability(to, "multifd", true); > > /* Start incoming migration from the 1st socket */ > - migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}"); > + migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}"); > > return NULL; > } > @@ -2862,14 +2863,14 @@ static void test_multifd_tcp_cancel(void) > migrate_set_capability(to, "multifd", true); > > /* Start incoming migration from the 1st socket */ > - migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}"); > + migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}"); > > /* Wait for the first serial output from the source */ > wait_for_serial("src_serial"); > > uri = migrate_get_socket_address(to, "socket-address"); > > - migrate_qmp(from, uri, "{}"); > + migrate_qmp(from, uri, NULL, "{}"); > > migrate_wait_for_dirty_mem(from, to); > > @@ -2892,7 +2893,7 @@ static void test_multifd_tcp_cancel(void) > migrate_set_capability(to2, "multifd", true); > > /* Start incoming migration from the 1st socket */ > - migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", "{}"); > + migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", NULL, "{}"); > > g_free(uri); > uri = migrate_get_socket_address(to2, "socket-address"); > @@ -2901,7 +2902,7 @@ static void test_multifd_tcp_cancel(void) > > migrate_ensure_non_converge(from); > > - migrate_qmp(from, uri, "{}"); > + migrate_qmp(from, uri, NULL, "{}"); > > migrate_wait_for_dirty_mem(from, to2); > > @@ -3234,7 +3235,7 @@ static void test_migrate_dirty_limit(void) > migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value); > > /* Start migrate */ > - migrate_qmp(from, uri, "{}"); > + migrate_qmp(from, uri, NULL, "{}"); > > /* Wait for dirty limit throttle begin */ > throttle_us_per_full = 0; > @@ -3275,7 +3276,7 @@ static void test_migrate_dirty_limit(void) > } > > /* Start migrate */ > - migrate_qmp(from, uri, "{}"); > + migrate_qmp(from, uri, NULL, "{}"); > > /* Wait for dirty limit throttle begin */ > throttle_us_per_full = 0; > diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c > index 73dfabc272..e263ecd80e 100644 > --- a/tests/qtest/virtio-net-failover.c > +++ b/tests/qtest/virtio-net-failover.c > @@ -772,7 +772,7 @@ static void test_migrate_in(gconstpointer opaque) > check_one_card(qts, true, "standby0", MAC_STANDBY0); > check_one_card(qts, false, "primary0", MAC_PRIMARY0); > > - migrate_incoming_qmp(qts, uri, "{}"); > + migrate_incoming_qmp(qts, uri, NULL, "{}"); > > resp = get_failover_negociated_event(qts); > g_assert_cmpstr(qdict_get_str(resp, "device-id"), ==, "standby0"); > @@ -894,7 +894,7 @@ static void test_off_migrate_in(gconstpointer opaque) > check_one_card(qts, true, "standby0", MAC_STANDBY0); > check_one_card(qts, true, "primary0", MAC_PRIMARY0); > > - migrate_incoming_qmp(qts, uri, "{}"); > + migrate_incoming_qmp(qts, uri, NULL, "{}"); > > check_one_card(qts, true, "standby0", MAC_STANDBY0); > check_one_card(qts, true, "primary0", MAC_PRIMARY0); > @@ -1021,7 +1021,7 @@ static void test_guest_off_migrate_in(gconstpointer opaque) > check_one_card(qts, true, "standby0", MAC_STANDBY0); > check_one_card(qts, false, "primary0", MAC_PRIMARY0); > > - migrate_incoming_qmp(qts, uri, "{}"); > + migrate_incoming_qmp(qts, uri, NULL, "{}"); > > check_one_card(qts, true, "standby0", MAC_STANDBY0); > check_one_card(qts, false, "primary0", MAC_PRIMARY0); > @@ -1746,7 +1746,7 @@ static void test_multi_in(gconstpointer opaque) > check_one_card(qts, true, "standby1", MAC_STANDBY1); > check_one_card(qts, false, "primary1", MAC_PRIMARY1); > > - migrate_incoming_qmp(qts, uri, "{}"); > + migrate_incoming_qmp(qts, uri, NULL, "{}"); > > resp = get_failover_negociated_event(qts); > g_assert_cmpstr(qdict_get_str(resp, "device-id"), ==, "standby0");
Het Gala <het.gala@nutanix.com> writes: > Introduce support for adding a 'channels' argument to migrate_qmp_fail, > migrate_incoming_qmp and migrate_qmp functions within the migration qtest > framework, enabling enhanced control over migration scenarios. Can't we just pass a channels string like you did in the original series with migrate_postcopy_prepare? We'd change migrate_* functions like this: void migrate_qmp(QTestState *who, const char *uri, const char *channels, const char *fmt, ...) { ... g_assert(!qdict_haskey(args, "uri")); if (uri) { qdict_put_str(args, "uri", uri); } g_assert(!qdict_haskey(args, "channels")); if (channels) { qdict_put_str(args, "channels", channels); } } Write the test like this: static void test_multifd_tcp_none_channels(void) { MigrateCommon args = { .listen_uri = "defer", .start_hook = test_migrate_precopy_tcp_multifd_start, .live = true, .connect_channels = "'channels': [ { 'channel-type': 'main'," " 'addr': { 'transport': 'socket'," " 'type': 'inet'," " 'host': '127.0.0.1'," " 'port': '0' } } ]", .connect_uri = NULL; }; test_precopy_common(&args); } static void do_test_validate_uri_channel(MigrateCommon *args) { QTestState *from, *to; g_autofree char *connect_uri = NULL; if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) { return; } wait_for_serial("src_serial"); if (args->result == MIG_TEST_QMP_ERROR) { migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}"); } else { migrate_qmp(from, args->connect_uri, args->connect_channels, "{}"); } test_migrate_end(from, to, false); } It's better to require test writers to pass in their own uri and channel strings. Otherwise any new transport added will require people to modify these conversion helpers. Also, using the same string as the user would use in QMP helps with development in general. One could refer to the tests to see how to invoke the migration or experiment with the string in the tests during development.
On 24/02/24 1:42 am, Fabiano Rosas wrote: > Het Gala<het.gala@nutanix.com> writes: > >> Introduce support for adding a 'channels' argument to migrate_qmp_fail, >> migrate_incoming_qmp and migrate_qmp functions within the migration qtest >> framework, enabling enhanced control over migration scenarios. > Can't we just pass a channels string like you did in the original series > with migrate_postcopy_prepare? > > We'd change migrate_* functions like this: > > void migrate_qmp(QTestState *who, const char *uri, const char *channels, > const char *fmt, ...) > { > ... > g_assert(!qdict_haskey(args, "uri")); > if (uri) { > qdict_put_str(args, "uri", uri); > } > > g_assert(!qdict_haskey(args, "channels")); > if (channels) { > qdict_put_str(args, "channels", channels); > } > } > > Write the test like this: > > static void test_multifd_tcp_none_channels(void) > { > MigrateCommon args = { > .listen_uri = "defer", > .start_hook = test_migrate_precopy_tcp_multifd_start, > .live = true, > .connect_channels = "'channels': [ { 'channel-type': 'main'," > " 'addr': { 'transport': 'socket'," > " 'type': 'inet'," > " 'host': '127.0.0.1'," > " 'port': '0' } } ]", > .connect_uri = NULL; > > }; > test_precopy_common(&args); > } this was the same first approach that I attempted. It won't work because The final 'migrate' QAPI with channels string would look like { "execute": "migrate", "arguments": { "channels": "[ { "channel-type": "main", "addr": { "transport": "socket", "type": "inet", "host": "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } } instead of { "execute": "migrate", "arguments": { "channels": [ { "channel-type": "main", "addr": { "transport": "socket", "type": "inet", "host": "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } } It would complain, that channels should be an *array* and not a string. So, that's the reason parsing was required in qtest too. I would be glad to hear if there are any ideas to convert /string -> json object -> add it inside qdict along with uri/ ? > static void do_test_validate_uri_channel(MigrateCommon *args) > { > QTestState *from, *to; > g_autofree char *connect_uri = NULL; > > if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) { > return; > } > > wait_for_serial("src_serial"); > > if (args->result == MIG_TEST_QMP_ERROR) { > migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}"); > } else { > migrate_qmp(from, args->connect_uri, args->connect_channels, "{}"); > } > > test_migrate_end(from, to, false); > } > > It's better to require test writers to pass in their own uri and channel > strings. Otherwise any new transport added will require people to modify > these conversion helpers. I agree with your point here. I was thinking to have a general but a hacky version of migrate_uri_parse() but that too seemed like a overkill. I don't have a better solution to this right now > Also, using the same string as the user would use in QMP helps with > development in general. One could refer to the tests to see how to > invoke the migration or experiment with the string in the tests during > development. For examples, I think - enough examples with 'channel' argument are provided where 'migrate' QAPI is defined. users can directly copy the qmp command from there itself. Regards, Het Gala
On 24/02/24 6:18 pm, Het Gala wrote: > > > On 24/02/24 1:42 am, Fabiano Rosas wrote: >> Het Gala<het.gala@nutanix.com> writes: >> >>> Introduce support for adding a 'channels' argument to migrate_qmp_fail, >>> migrate_incoming_qmp and migrate_qmp functions within the migration qtest >>> framework, enabling enhanced control over migration scenarios. >> [...] >> >> Write the test like this: >> >> static void test_multifd_tcp_none_channels(void) >> { >> MigrateCommon args = { >> .listen_uri = "defer", >> .start_hook = test_migrate_precopy_tcp_multifd_start, >> .live = true, >> .connect_channels = "'channels': [ { 'channel-type': 'main'," >> " 'addr': { 'transport': 'socket'," >> " 'type': 'inet'," >> " 'host': '127.0.0.1'," >> " 'port': '0' } } ]", >> .connect_uri = NULL; >> >> }; >> test_precopy_common(&args); >> } > > this was the same first approach that I attempted. It won't work because > > The final 'migrate' QAPI with channels string would look like > > { "execute": "migrate", "arguments": { "channels": "[ { > "channel-type": "main", "addr": { "transport": "socket", "type": > "inet", "host": "10.117.29.84", "port": "4000" }, "multifd-channels": > 2 } ]" } } > Sorry, In your example given above, the output looks like : {"execute": "migrate", "arguments": {"uri": "tcp:127.0.0.1:0", "channels": "'channels': [ { 'channel-type': 'main', 'addr': { 'transport': 'socket', 'type': 'inet', 'host': '127.0.0.1', 'port': '0' } } ]"}} > instead of > > { "execute": "migrate", "arguments": { "channels": [ { "channel-type": > "main", "addr": { "transport": "socket", "type": "inet", "host": > "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } } > > It would complain, that channels should be an *array* and not a string. > because of /"channels": "'channels': [ { 'channel-type': .... / > [...] > > Regards, > > Het Gala > Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 24/02/24 1:42 am, Fabiano Rosas wrote: >> Het Gala<het.gala@nutanix.com> writes: >> >>> Introduce support for adding a 'channels' argument to migrate_qmp_fail, >>> migrate_incoming_qmp and migrate_qmp functions within the migration qtest >>> framework, enabling enhanced control over migration scenarios. >> Can't we just pass a channels string like you did in the original series >> with migrate_postcopy_prepare? >> >> We'd change migrate_* functions like this: >> >> void migrate_qmp(QTestState *who, const char *uri, const char *channels, >> const char *fmt, ...) >> { >> ... >> g_assert(!qdict_haskey(args, "uri")); >> if (uri) { >> qdict_put_str(args, "uri", uri); >> } >> >> g_assert(!qdict_haskey(args, "channels")); >> if (channels) { >> qdict_put_str(args, "channels", channels); >> } >> } >> >> Write the test like this: >> >> static void test_multifd_tcp_none_channels(void) >> { >> MigrateCommon args = { >> .listen_uri = "defer", >> .start_hook = test_migrate_precopy_tcp_multifd_start, >> .live = true, >> .connect_channels = "'channels': [ { 'channel-type': 'main'," >> " 'addr': { 'transport': 'socket'," >> " 'type': 'inet'," >> " 'host': '127.0.0.1'," >> " 'port': '0' } } ]", >> .connect_uri = NULL; >> >> }; >> test_precopy_common(&args); >> } > > this was the same first approach that I attempted. It won't work because > > The final 'migrate' QAPI with channels string would look like > > { "execute": "migrate", "arguments": { "channels": "[ { "channel-type": > "main", "addr": { "transport": "socket", "type": "inet", "host": > "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } } > > instead of > > { "execute": "migrate", "arguments": { "channels": [ { "channel-type": > "main", "addr": { "transport": "socket", "type": "inet", "host": > "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } } > > It would complain, that channels should be an *array* and not a string. > > So, that's the reason parsing was required in qtest too. > > I would be glad to hear if there are any ideas to convert /string -> > json object -> add it inside qdict along with uri/ ? > Isn't this what the various qobject_from_json do? How does it work with the existing tests? qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming'," " 'arguments': { " " 'channels': [ { 'channel-type': 'main'," " 'addr': { 'transport': 'socket'," " 'type': 'inet'," " 'host': '127.0.0.1'," " 'port': '0' } } ] } }"); We can pass this^ string successfully to QMP somehow... >> static void do_test_validate_uri_channel(MigrateCommon *args) >> { >> QTestState *from, *to; >> g_autofree char *connect_uri = NULL; >> >> if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) { >> return; >> } >> >> wait_for_serial("src_serial"); >> >> if (args->result == MIG_TEST_QMP_ERROR) { >> migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}"); >> } else { >> migrate_qmp(from, args->connect_uri, args->connect_channels, "{}"); >> } >> >> test_migrate_end(from, to, false); >> } >> >> It's better to require test writers to pass in their own uri and channel >> strings. Otherwise any new transport added will require people to modify >> these conversion helpers. > I agree with your point here. I was thinking to have a general but a > hacky version of migrate_uri_parse() but that too seemed like a > overkill. I don't have a better solution to this right now >> Also, using the same string as the user would use in QMP helps with >> development in general. One could refer to the tests to see how to >> invoke the migration or experiment with the string in the tests during >> development. > > For examples, I think - enough examples with 'channel' argument are > provided where 'migrate' QAPI is defined. users can directly copy the > qmp command from there itself. > > > Regards, > > Het Gala
On 26/02/24 6:31 pm, Fabiano Rosas wrote: > Het Gala<het.gala@nutanix.com> writes: > >> On 24/02/24 1:42 am, Fabiano Rosas wrote: >> this was the same first approach that I attempted. It won't work because >> >> The final 'migrate' QAPI with channels string would look like >> >> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type": >> "main", "addr": { "transport": "socket", "type": "inet", "host": >> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } } >> >> instead of >> >> { "execute": "migrate", "arguments": { "channels": [ { "channel-type": >> "main", "addr": { "transport": "socket", "type": "inet", "host": >> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } } >> >> It would complain, that channels should be an *array* and not a string. >> >> So, that's the reason parsing was required in qtest too. >> >> I would be glad to hear if there are any ideas to convert /string -> >> json object -> add it inside qdict along with uri/ ? >> > Isn't this what the various qobject_from_json do? How does it work with > the existing tests? > > qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming'," > " 'arguments': { " > " 'channels': [ { 'channel-type': 'main'," > " 'addr': { 'transport': 'socket'," > " 'type': 'inet'," > " 'host': '127.0.0.1'," > " 'port': '0' } } ] } }"); > > We can pass this^ string successfully to QMP somehow... I think, here in qtest_qmp_assert_success, we actually can pass the whole QMP command, and it just asserts that return key is present in the response, though I am not very much familiar with qtest codebase to verify how swiftly we can convert string into an actual QObject. [...] >>> static void do_test_validate_uri_channel(MigrateCommon *args) >>> { >>> QTestState *from, *to; >>> g_autofree char *connect_uri = NULL; >>> >>> if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) { >>> return; >>> } >>> >>> >>> >>> Regards, >>> >>> Het Gala Regards, Het Gala
On 27/02/24 1:04 am, Het Gala wrote: > > > On 26/02/24 6:31 pm, Fabiano Rosas wrote: >> Het Gala<het.gala@nutanix.com> writes: >> >>> On 24/02/24 1:42 am, Fabiano Rosas wrote: >>> this was the same first approach that I attempted. It won't work because >>> >>> The final 'migrate' QAPI with channels string would look like >>> >>> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type": >>> "main", "addr": { "transport": "socket", "type": "inet", "host": >>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } } >>> >>> instead of >>> >>> { "execute": "migrate", "arguments": { "channels": [ { "channel-type": >>> "main", "addr": { "transport": "socket", "type": "inet", "host": >>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } } >>> >>> It would complain, that channels should be an *array* and not a string. >>> >>> So, that's the reason parsing was required in qtest too. >>> >>> I would be glad to hear if there are any ideas to convert /string -> >>> json object -> add it inside qdict along with uri/ ? >>> >> Isn't this what the various qobject_from_json do? How does it work with >> the existing tests? >> >> qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming'," >> " 'arguments': { " >> " 'channels': [ { 'channel-type': 'main'," >> " 'addr': { 'transport': 'socket'," >> " 'type': 'inet'," >> " 'host': '127.0.0.1'," >> " 'port': '0' } } ] } }"); >> >> We can pass this^ string successfully to QMP somehow... > > I think, here in qtest_qmp_assert_success, we actually can pass the > whole QMP command, and it just asserts that return key is present in > the response, though I am not very much familiar with qtest codebase > to verify how swiftly we can convert string into an actual QObject. > > [...] > I tried with qobject_from_json type of utility functions and the error I got was this : migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126: qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type < QTYPE__MAX' failed. And I suppose this was the case because, there are only limited types of QTYPE available typedefenumQType{ QTYPE_NONE, QTYPE_QNULL, QTYPE_QNUM, QTYPE_QSTRING, QTYPE_QDICT, QTYPE_QLIST, QTYPE_QBOOL, QTYPE__MAX, } QType; And 'channels' is a mixture of QDICT and QLIST and hence it is not able to easily convert from string to json. Thoughts on this ? >>>> static void do_test_validate_uri_channel(MigrateCommon *args) >>>> { >>>> QTestState *from, *to; >>>> g_autofree char *connect_uri = NULL; >>>> >>>> if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) { >>>> return; >>>> } >>>> >>>> >>>> >>>> Regards, >>>> >>>> Het Gala Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 27/02/24 1:04 am, Het Gala wrote: >> >> >> On 26/02/24 6:31 pm, Fabiano Rosas wrote: >>> Het Gala<het.gala@nutanix.com> writes: >>> >>>> On 24/02/24 1:42 am, Fabiano Rosas wrote: >>>> this was the same first approach that I attempted. It won't work because >>>> >>>> The final 'migrate' QAPI with channels string would look like >>>> >>>> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type": >>>> "main", "addr": { "transport": "socket", "type": "inet", "host": >>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } } >>>> >>>> instead of >>>> >>>> { "execute": "migrate", "arguments": { "channels": [ { "channel-type": >>>> "main", "addr": { "transport": "socket", "type": "inet", "host": >>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } } >>>> >>>> It would complain, that channels should be an *array* and not a string. >>>> >>>> So, that's the reason parsing was required in qtest too. >>>> >>>> I would be glad to hear if there are any ideas to convert /string -> >>>> json object -> add it inside qdict along with uri/ ? >>>> >>> Isn't this what the various qobject_from_json do? How does it work with >>> the existing tests? >>> >>> qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming'," >>> " 'arguments': { " >>> " 'channels': [ { 'channel-type': 'main'," >>> " 'addr': { 'transport': 'socket'," >>> " 'type': 'inet'," >>> " 'host': '127.0.0.1'," >>> " 'port': '0' } } ] } }"); >>> >>> We can pass this^ string successfully to QMP somehow... >> >> I think, here in qtest_qmp_assert_success, we actually can pass the >> whole QMP command, and it just asserts that return key is present in >> the response, though I am not very much familiar with qtest codebase >> to verify how swiftly we can convert string into an actual QObject. >> >> [...] >> > I tried with qobject_from_json type of utility functions and the error I > got was this : > > migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126: > qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type < > QTYPE__MAX' failed. > > And I suppose this was the case because, there are only limited types of > QTYPE available > > typedefenumQType{ > QTYPE_NONE, > QTYPE_QNULL, > QTYPE_QNUM, > QTYPE_QSTRING, > QTYPE_QDICT, > QTYPE_QLIST, > QTYPE_QBOOL, > QTYPE__MAX, > } QType; > > And 'channels' is a mixture of QDICT and QLIST and hence it is not able > to easily convert from string to json. > > Thoughts on this ? I'm not sure what you tried. This works: g_assert(!qdict_haskey(args, "channels")); if (channels) { channels_obj = qobject_from_json(channels, errp); qdict_put_obj(args, "channels", channels_obj); } And in the test: .connect_channels = "[ { 'channel-type': 'main'," " 'addr': { 'transport': 'socket'," " 'type': 'inet'," " 'host': '127.0.0.1'," " 'port': '0' } } ]", .listen_uri = "tcp:127.0.0.1:0", .result = MIG_TEST_QMP_ERROR However, the real issue is how to inject the port for the source migration. The example above only works for the tests that are expected to fail. For a test that should pass, 0 as a port does not work. I'm thinking it might be better to alter migrate_qmp like this: void migrate_qmp(QTestState *from, QTestState *to, const char *channels, const char *fmt, ...) Invocations would be: migrate_qmp(from, to, NULL, "{uri: %s}", connect_uri); migrate_qmp(from, to, args->channels, "{}"); In this last case, if the test provided a port, we use it, otherwise we resolve it from the 'to' instance and put it in the QDict directly. I'll play with this a bit more tomorrow, let me know what you think.
Fabiano Rosas <farosas@suse.de> writes: > Het Gala <het.gala@nutanix.com> writes: > >> On 27/02/24 1:04 am, Het Gala wrote: >>> >>> >>> On 26/02/24 6:31 pm, Fabiano Rosas wrote: >>>> Het Gala<het.gala@nutanix.com> writes: >>>> >>>>> On 24/02/24 1:42 am, Fabiano Rosas wrote: >>>>> this was the same first approach that I attempted. It won't work because >>>>> >>>>> The final 'migrate' QAPI with channels string would look like >>>>> >>>>> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type": >>>>> "main", "addr": { "transport": "socket", "type": "inet", "host": >>>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } } >>>>> >>>>> instead of >>>>> >>>>> { "execute": "migrate", "arguments": { "channels": [ { "channel-type": >>>>> "main", "addr": { "transport": "socket", "type": "inet", "host": >>>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } } >>>>> >>>>> It would complain, that channels should be an *array* and not a string. >>>>> >>>>> So, that's the reason parsing was required in qtest too. >>>>> >>>>> I would be glad to hear if there are any ideas to convert /string -> >>>>> json object -> add it inside qdict along with uri/ ? >>>>> >>>> Isn't this what the various qobject_from_json do? How does it work with >>>> the existing tests? >>>> >>>> qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming'," >>>> " 'arguments': { " >>>> " 'channels': [ { 'channel-type': 'main'," >>>> " 'addr': { 'transport': 'socket'," >>>> " 'type': 'inet'," >>>> " 'host': '127.0.0.1'," >>>> " 'port': '0' } } ] } }"); >>>> >>>> We can pass this^ string successfully to QMP somehow... >>> >>> I think, here in qtest_qmp_assert_success, we actually can pass the >>> whole QMP command, and it just asserts that return key is present in >>> the response, though I am not very much familiar with qtest codebase >>> to verify how swiftly we can convert string into an actual QObject. >>> >>> [...] >>> >> I tried with qobject_from_json type of utility functions and the error I >> got was this : >> >> migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126: >> qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type < >> QTYPE__MAX' failed. >> >> And I suppose this was the case because, there are only limited types of >> QTYPE available >> >> typedefenumQType{ >> QTYPE_NONE, >> QTYPE_QNULL, >> QTYPE_QNUM, >> QTYPE_QSTRING, >> QTYPE_QDICT, >> QTYPE_QLIST, >> QTYPE_QBOOL, >> QTYPE__MAX, >> } QType; >> >> And 'channels' is a mixture of QDICT and QLIST and hence it is not able >> to easily convert from string to json. >> >> Thoughts on this ? > > I'm not sure what you tried. This works: > > g_assert(!qdict_haskey(args, "channels")); > if (channels) { > channels_obj = qobject_from_json(channels, errp); > qdict_put_obj(args, "channels", channels_obj); > } > > And in the test: > > .connect_channels = "[ { 'channel-type': 'main'," > " 'addr': { 'transport': 'socket'," > " 'type': 'inet'," > " 'host': '127.0.0.1'," > " 'port': '0' } } ]", > .listen_uri = "tcp:127.0.0.1:0", > .result = MIG_TEST_QMP_ERROR > > However, the real issue is how to inject the port for the source > migration. The example above only works for the tests that are expected > to fail. For a test that should pass, 0 as a port does not work. > > I'm thinking it might be better to alter migrate_qmp like this: > > void migrate_qmp(QTestState *from, QTestState *to, const char *channels, > const char *fmt, ...) > > Invocations would be: > > migrate_qmp(from, to, NULL, "{uri: %s}", connect_uri); > migrate_qmp(from, to, args->channels, "{}"); > > In this last case, if the test provided a port, we use it, otherwise we > resolve it from the 'to' instance and put it in the QDict directly. > > I'll play with this a bit more tomorrow, let me know what you think. Ok, so here's what I think we should do: 1) Add the 'to' object into migrate_qmp(), so we can use migrate_get_socket_address() to get the port. Leave the other migrate_qmp* alone because they don't need the port; 2) Move the calls to migrate_get_socket_address() into migrate_qmp() and get rid of that connect_uri, use args->connect_uri instead everywhere; 3) Add a new function SocketAddress_to_qdict() that does the same as SocketAddress_to_str(), but fills in a QDict instead; 4) Add args->connect_channels and pass it to migrate_qmp() and migrate_qmp_fail(). Convert the string to a dict; if (channels) { channels_obj = qobject_from_json(channels, &error_abort); qdict_put_obj(args, "channels", channels_obj); } 5) Add a migrate_set_ports() function that uses 3) and fills in the port in case it was 0 in the test. Handle a list of channels so we can add a negative test that passes two channels; addr = migrate_get_socket_address(to); for each channel: if channel["port"] && channel["port"] == "0": channel["port"] = SocketAddress_to_qdict(addr->value)["port"] (optionally iterate over addr to be prepared for when we add more channels) 6) Alter migrate_qmp_fail() to allow both uri and channels independently. No dealing with migrate_get_socket_address() because we will fail before starting the migration anyway; if (uri) { qdict_put_str(args, "uri", uri); } if (channels) { channels_obj = qobject_from_json(channels, &error_abort); qdict_put_obj(args, "channels", channels_obj); } 7) Alter migrate_qmp() to only fill the uri if there are no channels. Here we don't want to allow the wrong cases of having both or none; if (uri) { qdict_put_str(args, "uri", uri); } else if (!channels) { qdict_put_str(args, "uri", migrate_get_socket_address(to)); } if (channels) { channels_obj = qobject_from_json(channels, &error_abort); migrate_set_ports(to, qobject_to(QList, channels_obj)); qdict_put_obj(args, "channels", channels_obj); } 8) Write the tests to pass the list of channels; .connect_channels = "[ { 'channel-type': 'main'," " 'addr': { 'transport': 'socket'," " 'type': 'inet'," " 'host': '127.0.0.1'," " 'port': '0' } } ]", With this we can test multiple channels, test other types of channel, add more channel types in the future, etc and not need to change the test infra.
On 29/02/24 6:47 am, Fabiano Rosas wrote: > Het Gala <het.gala@nutanix.com> writes: > >> On 27/02/24 1:04 am, Het Gala wrote: >>> >>> On 26/02/24 6:31 pm, Fabiano Rosas wrote: >>>> Het Gala<het.gala@nutanix.com> writes: >>>> >>>>> On 24/02/24 1:42 am, Fabiano Rosas wrote: >>>>> this was the same first approach that I attempted. It won't work because >>>>> >>>>> The final 'migrate' QAPI with channels string would look like >>>>> >>>>> { "execute": "migrate", "arguments": { "channels": "[ { "channel-type": >>>>> "main", "addr": { "transport": "socket", "type": "inet", "host": >>>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } } >>>>> >>>>> instead of >>>>> >>>>> { "execute": "migrate", "arguments": { "channels": [ { "channel-type": >>>>> "main", "addr": { "transport": "socket", "type": "inet", "host": >>>>> "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } } >>>>> >>>>> It would complain, that channels should be an *array* and not a string. >>>>> >>>>> So, that's the reason parsing was required in qtest too. >>>>> >>>>> I would be glad to hear if there are any ideas to convert /string -> >>>>> json object -> add it inside qdict along with uri/ ? >>>>> >>>> Isn't this what the various qobject_from_json do? How does it work with >>>> the existing tests? >>>> >>>> qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming'," >>>> " 'arguments': { " >>>> " 'channels': [ { 'channel-type': 'main'," >>>> " 'addr': { 'transport': 'socket'," >>>> " 'type': 'inet'," >>>> " 'host': '127.0.0.1'," >>>> " 'port': '0' } } ] } }"); >>>> >>>> We can pass this^ string successfully to QMP somehow... >>> I think, here in qtest_qmp_assert_success, we actually can pass the >>> whole QMP command, and it just asserts that return key is present in >>> the response, though I am not very much familiar with qtest codebase >>> to verify how swiftly we can convert string into an actual QObject. >>> >>> [...] >>> >> I tried with qobject_from_json type of utility functions and the error I >> got was this : >> >> migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126: >> qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type < >> QTYPE__MAX' failed. >> >> And I suppose this was the case because, there are only limited types of >> QTYPE available >> >> typedefenumQType{ >> QTYPE_NONE, >> QTYPE_QNULL, >> QTYPE_QNUM, >> QTYPE_QSTRING, >> QTYPE_QDICT, >> QTYPE_QLIST, >> QTYPE_QBOOL, >> QTYPE__MAX, >> } QType; >> >> And 'channels' is a mixture of QDICT and QLIST and hence it is not able >> to easily convert from string to json. >> >> Thoughts on this ? > I'm not sure what you tried. This works: > > g_assert(!qdict_haskey(args, "channels")); > if (channels) { > channels_obj = qobject_from_json(channels, errp); > qdict_put_obj(args, "channels", channels_obj); > } Are you sure the above works ? Sorry I want to get this doubt cleared first, Let's me take a step back and just focus on the above part and not focus on port issue for now. Adding a validation test for uri and channels both used together migration_test_add("/migration/validate_uri/channels/both_set", test_validate_uri_channels_both_set); static void test_validate_uri_channels_both_set(void) { QTestState *from, *to; MigrateCommon args = { .connect_channels = "'channels': [ { 'channel-type': 'main'," " 'addr': { 'transport': 'socket'," " 'type': 'inet'," " 'host': '127.0.0.1'," " 'port': '0' } } ]", .listen_uri = "tcp:127.0.0.1:0", .result = MIG_TEST_QMP_ERROR, }; if (test_migrate_start(&from, &to, args.listen_uri, &args.start)) { return; } wait_for_serial("src_serial"); if (args.result == MIG_TEST_QMP_ERROR) { migrate_qmp_fail(from, args.connect_uri, args.connect_channels, "{}"); } else { migrate_qmp(from, args.connect_uri, args.connect_channels, "{}"); } test_migrate_end(from, to, false); } In the migration-helpers.c file, inside migrate_qmp_fail function: void migrate_qmp_fail(QTestState *who, const char *uri, const char *channels, const char *fmt, ...) { [...] Error **errp = NULL; QObject *channels_obj = NULL; [...] g_assert(!qdict_haskey(args, "channels")); if (channels) { channels_obj = qobject_from_json(channels, errp); if (!channels_obj) { fprintf(stdout, "Everything is right till here also ?\n"); goto cleanup; } qdict_put_obj(args, "channels", channels_obj); } err = qtest_qmp_assert_failure_ref( who, "{ 'execute': 'migrate', 'arguments': %p}", args); [...] cleanup: qobject_unref(channels_obj); } When I ran the test alone, test passed, but instead of giving output as {"error": {"class": "GenericError", "desc": "need either 'uri' or 'channels' argument"}} It just passed, i.e. channels_obj for me was NULL and hence just ended the test. What wrong am I doing here according to you ? Because, IMO if qobject_from_json() is not working properly, this approach of passing channels as string might not work at all. Regards, Het Gala
On 01/03/24 2:19 pm, Het Gala wrote: > > On 29/02/24 6:47 am, Fabiano Rosas wrote: >> Het Gala <het.gala@nutanix.com> writes: >> >>> On 27/02/24 1:04 am, Het Gala wrote: >>>> >>>> On 26/02/24 6:31 pm, Fabiano Rosas wrote: >>>>> Het Gala<het.gala@nutanix.com> writes: >>>>> >>>>>> On 24/02/24 1:42 am, Fabiano Rosas wrote: >>>>>> this was the same first approach that I attempted. It won't work >>>>>> because >>>>>> >>>>>> The final 'migrate' QAPI with channels string would look like >>>>>> [...] >> I'm not sure what you tried. This works: >> >> g_assert(!qdict_haskey(args, "channels")); >> if (channels) { >> channels_obj = qobject_from_json(channels, errp); >> qdict_put_obj(args, "channels", channels_obj); >> } > > Are you sure the above works ? Sorry, please ignore the below doubt. I understood what silly mistakes I was doing. you were right, qobject_from_json() works fine and converts string to json object. Will follow your latest reply and send out the patch really soon. Thank you for unblocking me here quickly :) > [...] Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 01/03/24 2:19 pm, Het Gala wrote: >> >> On 29/02/24 6:47 am, Fabiano Rosas wrote: >>> Het Gala <het.gala@nutanix.com> writes: >>> >>>> On 27/02/24 1:04 am, Het Gala wrote: >>>>> >>>>> On 26/02/24 6:31 pm, Fabiano Rosas wrote: >>>>>> Het Gala<het.gala@nutanix.com> writes: >>>>>> >>>>>>> On 24/02/24 1:42 am, Fabiano Rosas wrote: >>>>>>> this was the same first approach that I attempted. It won't work >>>>>>> because >>>>>>> >>>>>>> The final 'migrate' QAPI with channels string would look like >>>>>>> > [...] >>> I'm not sure what you tried. This works: >>> >>> g_assert(!qdict_haskey(args, "channels")); >>> if (channels) { >>> channels_obj = qobject_from_json(channels, errp); >>> qdict_put_obj(args, "channels", channels_obj); >>> } >> >> Are you sure the above works ? > > Sorry, please ignore the below doubt. I understood what silly mistakes I > was doing. you were right, qobject_from_json() works fine and converts > string to json object. > > Will follow your latest reply and send out the patch really soon. Thank > you for unblocking me here quickly :) Just please don't make it all one patch. Each of those steps could be a separate patch.
diff --git a/tests/qtest/dbus-vmstate-test.c b/tests/qtest/dbus-vmstate-test.c index 6c990864e3..0ca572e29b 100644 --- a/tests/qtest/dbus-vmstate-test.c +++ b/tests/qtest/dbus-vmstate-test.c @@ -229,7 +229,7 @@ test_dbus_vmstate(Test *test) thread = g_thread_new("dbus-vmstate-thread", dbus_vmstate_thread, loop); - migrate_qmp(src_qemu, uri, "{}"); + migrate_qmp(src_qemu, uri, NULL, "{}"); test->src_qemu = src_qemu; if (test->migrate_fail) { wait_for_migration_fail(src_qemu, true); diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c index e451dbdbed..15c3650b55 100644 --- a/tests/qtest/migration-helpers.c +++ b/tests/qtest/migration-helpers.c @@ -13,6 +13,7 @@ #include "qemu/osdep.h" #include "qemu/ctype.h" #include "qapi/qmp/qjson.h" +#include "qapi/qmp/qlist.h" #include "migration-helpers.h" @@ -43,7 +44,67 @@ bool migrate_watch_for_events(QTestState *who, const char *name, return false; } -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...) +static QList *MigrationChannelList_to_QList(MigrationChannelList *channels) +{ + MigrationChannel *channel = NULL; + MigrationAddress *addr = NULL; + SocketAddress *saddr = NULL; + QList *channelList = qlist_new(); + QDict *channelDict = qdict_new(); + QDict *addrDict = qdict_new(); + + channel = channels->value; + if (!channel || channel->channel_type == MIGRATION_CHANNEL_TYPE__MAX) { + fprintf(stderr, "%s: Channel or its type is NULL\n", + __func__); + } + g_assert(channel); + if (channel->channel_type == MIGRATION_CHANNEL_TYPE_MAIN) { + qdict_put_str(channelDict, "channel-type", g_strdup("main")); + } + + addr = channel->addr; + if (!addr || addr->transport == MIGRATION_ADDRESS_TYPE__MAX) { + fprintf(stderr, "%s: addr or its transport is NULL\n", + __func__); + } + g_assert(addr); + if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) { + qdict_put_str(addrDict, "transport", g_strdup("socket")); + } + + saddr = &addr->u.socket; + if (!saddr) { + fprintf(stderr, "%s: saddr is NULL\n", __func__); + } + g_assert(saddr); + qdict_put_str(addrDict, "type", SocketAddressType_str(saddr->type)); + qdict_put_str(addrDict, "port", saddr->u.inet.port); + qdict_put_str(addrDict, "host", saddr->u.inet.host); + + qdict_put_obj(channelDict, "addr", QOBJECT(addrDict)); + qlist_append_obj(channelList, QOBJECT(channelDict)); + + return channelList; +} + +static void migrate_qmp_attach_ports(QDict *args, const char *uri, + MigrationChannelList *channels) +{ + if (uri) { + g_assert(!qdict_haskey(args, "uri")); + qdict_put_str(args, "uri", uri); + } + + if (channels) { + g_assert(!qdict_haskey(args, "channels")); + QList *channelList = MigrationChannelList_to_QList(channels); + qdict_put_obj(args, "channels", QOBJECT(channelList)); + } +} + +void migrate_qmp_fail(QTestState *who, const char *uri, + MigrationChannelList *channels, const char *fmt, ...) { va_list ap; QDict *args, *err; @@ -52,8 +113,7 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...) args = qdict_from_vjsonf_nofail(fmt, ap); va_end(ap); - g_assert(!qdict_haskey(args, "uri")); - qdict_put_str(args, "uri", uri); + migrate_qmp_attach_ports(args, uri, channels); err = qtest_qmp_assert_failure_ref( who, "{ 'execute': 'migrate', 'arguments': %p}", args); @@ -68,7 +128,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...) * Arguments are built from @fmt... (formatted like * qobject_from_jsonf_nofail()) with "uri": @uri spliced in. */ -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...) +void migrate_qmp(QTestState *who, const char *uri, + MigrationChannelList *channels, const char *fmt, ...) { va_list ap; QDict *args; @@ -77,8 +138,7 @@ void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...) args = qdict_from_vjsonf_nofail(fmt, ap); va_end(ap); - g_assert(!qdict_haskey(args, "uri")); - qdict_put_str(args, "uri", uri); + migrate_qmp_attach_ports(args, uri, channels); qtest_qmp_assert_success(who, "{ 'execute': 'migrate', 'arguments': %p}", args); @@ -95,7 +155,8 @@ void migrate_set_capability(QTestState *who, const char *capability, capability, value); } -void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...) +void migrate_incoming_qmp(QTestState *to, const char *uri, + MigrationChannelList *channels, const char *fmt, ...) { va_list ap; QDict *args, *rsp, *data; @@ -104,8 +165,7 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...) args = qdict_from_vjsonf_nofail(fmt, ap); va_end(ap); - g_assert(!qdict_haskey(args, "uri")); - qdict_put_str(args, "uri", uri); + migrate_qmp_attach_ports(args, uri, channels); migrate_set_capability(to, "events", true); diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h index 3bf7ded1b9..390e386274 100644 --- a/tests/qtest/migration-helpers.h +++ b/tests/qtest/migration-helpers.h @@ -14,6 +14,7 @@ #define MIGRATION_HELPERS_H #include "libqtest.h" +#include "migration/migration.h" typedef struct QTestMigrationState { bool stop_seen; @@ -25,15 +26,17 @@ typedef struct QTestMigrationState { bool migrate_watch_for_events(QTestState *who, const char *name, QDict *event, void *opaque); -G_GNUC_PRINTF(3, 4) -void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...); +G_GNUC_PRINTF(4, 5) +void migrate_qmp(QTestState *who, const char *uri, + MigrationChannelList *channels, const char *fmt, ...); -G_GNUC_PRINTF(3, 4) +G_GNUC_PRINTF(4, 5) void migrate_incoming_qmp(QTestState *who, const char *uri, - const char *fmt, ...); + MigrationChannelList *channels, const char *fmt, ...); -G_GNUC_PRINTF(3, 4) -void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...); +G_GNUC_PRINTF(4, 5) +void migrate_qmp_fail(QTestState *who, const char *uri, + MigrationChannelList *channels, const char *fmt, ...); void migrate_set_capability(QTestState *who, const char *capability, bool value); diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 8a5bb1752e..e5547b8746 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -18,6 +18,7 @@ #include "qemu/module.h" #include "qemu/option.h" #include "qemu/range.h" +#include "migration/migration.h" #include "qemu/sockets.h" #include "chardev/char.h" #include "qapi/qapi-visit-sockets.h" @@ -1350,7 +1351,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr, wait_for_suspend(from, &src_state); g_autofree char *uri = migrate_get_socket_address(to, "socket-address"); - migrate_qmp(from, uri, "{}"); + migrate_qmp(from, uri, NULL, "{}"); migrate_wait_for_dirty_mem(from, to); @@ -1500,7 +1501,7 @@ static void postcopy_recover_fail(QTestState *from, QTestState *to) g_assert_cmpint(ret, ==, 1); migrate_recover(to, "fd:fd-mig"); - migrate_qmp(from, "fd:fd-mig", "{'resume': true}"); + migrate_qmp(from, "fd:fd-mig", NULL, "{'resume': true}"); /* * Make sure both QEMU instances will go into RECOVER stage, then test @@ -1588,7 +1589,7 @@ static void test_postcopy_recovery_common(MigrateCommon *args) * Try to rebuild the migration channel using the resume flag and * the newly created channel */ - migrate_qmp(from, uri, "{'resume': true}"); + migrate_qmp(from, uri, NULL, "{'resume': true}"); /* Restore the postcopy bandwidth to unlimited */ migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0); @@ -1669,7 +1670,7 @@ static void test_baddest(void) if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) { return; } - migrate_qmp(from, "tcp:127.0.0.1:0", "{}"); + migrate_qmp(from, "tcp:127.0.0.1:0", NULL, "{}"); wait_for_migration_fail(from, false); test_migrate_end(from, to, false); } @@ -1708,7 +1709,7 @@ static void test_analyze_script(void) uri = g_strdup_printf("exec:cat > %s", file); migrate_ensure_converge(from); - migrate_qmp(from, uri, "{}"); + migrate_qmp(from, uri, NULL, "{}"); wait_for_migration_complete(from); pid = fork(); @@ -1773,11 +1774,11 @@ static void test_precopy_common(MigrateCommon *args) } if (args->result == MIG_TEST_QMP_ERROR) { - migrate_qmp_fail(from, connect_uri, "{}"); + migrate_qmp_fail(from, connect_uri, NULL, "{}"); goto finish; } - migrate_qmp(from, connect_uri, "{}"); + migrate_qmp(from, connect_uri, NULL, "{}"); if (args->result != MIG_TEST_SUCCEED) { bool allow_active = args->result == MIG_TEST_FAIL; @@ -1869,18 +1870,18 @@ static void test_file_common(MigrateCommon *args, bool stop_src) } if (args->result == MIG_TEST_QMP_ERROR) { - migrate_qmp_fail(from, connect_uri, "{}"); + migrate_qmp_fail(from, connect_uri, NULL, "{}"); goto finish; } - migrate_qmp(from, connect_uri, "{}"); + migrate_qmp(from, connect_uri, NULL, "{}"); wait_for_migration_complete(from); /* * We need to wait for the source to finish before starting the * destination. */ - migrate_incoming_qmp(to, connect_uri, "{}"); + migrate_incoming_qmp(to, connect_uri, NULL, "{}"); wait_for_migration_complete(to); if (stop_src) { @@ -2029,7 +2030,7 @@ static void test_ignore_shared(void) /* Wait for the first serial output from the source */ wait_for_serial("src_serial"); - migrate_qmp(from, uri, "{}"); + migrate_qmp(from, uri, NULL, "{}"); migrate_wait_for_dirty_mem(from, to); @@ -2387,7 +2388,7 @@ static void *test_migrate_fd_start_hook(QTestState *from, close(pair[0]); /* Start incoming migration from the 1st socket */ - migrate_incoming_qmp(to, "fd:fd-mig", "{}"); + migrate_incoming_qmp(to, "fd:fd-mig", NULL, "{}"); /* Send the 2nd socket to the target */ qtest_qmp_fds_assert_success(from, &pair[1], 1, @@ -2455,7 +2456,7 @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail) /* Wait for the first serial output from the source */ wait_for_serial("src_serial"); - migrate_qmp(from, uri, "{}"); + migrate_qmp(from, uri, NULL, "{}"); if (should_fail) { qtest_set_expected_status(to, EXIT_FAILURE); @@ -2558,7 +2559,7 @@ static void test_migrate_auto_converge(void) /* Wait for the first serial output from the source */ wait_for_serial("src_serial"); - migrate_qmp(from, uri, "{}"); + migrate_qmp(from, uri, NULL, "{}"); /* Wait for throttling begins */ percentage = 0; @@ -2609,7 +2610,7 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState *from, migrate_set_capability(to, "multifd", true); /* Start incoming migration from the 1st socket */ - migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}"); + migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}"); return NULL; } @@ -2862,14 +2863,14 @@ static void test_multifd_tcp_cancel(void) migrate_set_capability(to, "multifd", true); /* Start incoming migration from the 1st socket */ - migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}"); + migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}"); /* Wait for the first serial output from the source */ wait_for_serial("src_serial"); uri = migrate_get_socket_address(to, "socket-address"); - migrate_qmp(from, uri, "{}"); + migrate_qmp(from, uri, NULL, "{}"); migrate_wait_for_dirty_mem(from, to); @@ -2892,7 +2893,7 @@ static void test_multifd_tcp_cancel(void) migrate_set_capability(to2, "multifd", true); /* Start incoming migration from the 1st socket */ - migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", "{}"); + migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", NULL, "{}"); g_free(uri); uri = migrate_get_socket_address(to2, "socket-address"); @@ -2901,7 +2902,7 @@ static void test_multifd_tcp_cancel(void) migrate_ensure_non_converge(from); - migrate_qmp(from, uri, "{}"); + migrate_qmp(from, uri, NULL, "{}"); migrate_wait_for_dirty_mem(from, to2); @@ -3234,7 +3235,7 @@ static void test_migrate_dirty_limit(void) migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value); /* Start migrate */ - migrate_qmp(from, uri, "{}"); + migrate_qmp(from, uri, NULL, "{}"); /* Wait for dirty limit throttle begin */ throttle_us_per_full = 0; @@ -3275,7 +3276,7 @@ static void test_migrate_dirty_limit(void) } /* Start migrate */ - migrate_qmp(from, uri, "{}"); + migrate_qmp(from, uri, NULL, "{}"); /* Wait for dirty limit throttle begin */ throttle_us_per_full = 0; diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c index 73dfabc272..e263ecd80e 100644 --- a/tests/qtest/virtio-net-failover.c +++ b/tests/qtest/virtio-net-failover.c @@ -772,7 +772,7 @@ static void test_migrate_in(gconstpointer opaque) check_one_card(qts, true, "standby0", MAC_STANDBY0); check_one_card(qts, false, "primary0", MAC_PRIMARY0); - migrate_incoming_qmp(qts, uri, "{}"); + migrate_incoming_qmp(qts, uri, NULL, "{}"); resp = get_failover_negociated_event(qts); g_assert_cmpstr(qdict_get_str(resp, "device-id"), ==, "standby0"); @@ -894,7 +894,7 @@ static void test_off_migrate_in(gconstpointer opaque) check_one_card(qts, true, "standby0", MAC_STANDBY0); check_one_card(qts, true, "primary0", MAC_PRIMARY0); - migrate_incoming_qmp(qts, uri, "{}"); + migrate_incoming_qmp(qts, uri, NULL, "{}"); check_one_card(qts, true, "standby0", MAC_STANDBY0); check_one_card(qts, true, "primary0", MAC_PRIMARY0); @@ -1021,7 +1021,7 @@ static void test_guest_off_migrate_in(gconstpointer opaque) check_one_card(qts, true, "standby0", MAC_STANDBY0); check_one_card(qts, false, "primary0", MAC_PRIMARY0); - migrate_incoming_qmp(qts, uri, "{}"); + migrate_incoming_qmp(qts, uri, NULL, "{}"); check_one_card(qts, true, "standby0", MAC_STANDBY0); check_one_card(qts, false, "primary0", MAC_PRIMARY0); @@ -1746,7 +1746,7 @@ static void test_multi_in(gconstpointer opaque) check_one_card(qts, true, "standby1", MAC_STANDBY1); check_one_card(qts, false, "primary1", MAC_PRIMARY1); - migrate_incoming_qmp(qts, uri, "{}"); + migrate_incoming_qmp(qts, uri, NULL, "{}"); resp = get_failover_negociated_event(qts); g_assert_cmpstr(qdict_get_str(resp, "device-id"), ==, "standby0");
Introduce support for adding a 'channels' argument to migrate_qmp_fail, migrate_incoming_qmp and migrate_qmp functions within the migration qtest framework, enabling enhanced control over migration scenarios. Signed-off-by: Het Gala <het.gala@nutanix.com> --- tests/qtest/dbus-vmstate-test.c | 2 +- tests/qtest/migration-helpers.c | 78 +++++++++++++++++++++++++++---- tests/qtest/migration-helpers.h | 15 +++--- tests/qtest/migration-test.c | 43 ++++++++--------- tests/qtest/virtio-net-failover.c | 8 ++-- 5 files changed, 105 insertions(+), 41 deletions(-)