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");
--
2.22.3Het 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
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.
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.
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 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
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");
© 2016 - 2024 Red Hat, Inc.