[PATCH 0/4] tests/qtest/migration: Add postcopy qtests for introducing 'channels' argument with new QAPI syntax

[PATCH 0/4] tests/qtest/migration: Add postcopy qtests for introducing 'channels' argument with new QAPI syntax

[Het Gala]

Add postcopy migration qtests with new QAPI syntax, having 'channels' as
the starting argument.
Also, introduce 'channels' to migrate_incoming_qmp function so as to
call migration with the new QAPI syntax from src as well as dest.

Patch 1:
--------
Revert back commit which temporarily introduced 'migrate-incoming' QAPI
with the new 'channels' syntax.

Patch 2-3:
---------
Introduce channels arg to migrate_incoming_qmp

Patch 4:
-------
Introduce postcopy qtests with new QAPI syntax

Het Gala (4):
  Revert "migration: modify test_multifd_tcp_none() to use new QAPI
    syntax"
  tests/qtest/migration: Replace 'migrate-incoming'
    qtest_qmp_assert_success with migrate_incoming_qmp
  tests/qtest/migration: Add channels parameter in migrate_incoming_qmp
  tests/qtest/migration: Add postcopy migration qtests to use 'channels'
    argument instead of uri

 tests/qtest/migration-helpers.c   | 13 ++++++--
 tests/qtest/migration-helpers.h   |  4 +--
 tests/qtest/migration-test.c      | 54 +++++++++++++++++++++++--------
 tests/qtest/virtio-net-failover.c |  8 ++---
 4 files changed, 58 insertions(+), 21 deletions(-)

-- 
2.22.3

[PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Het Gala]

This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb

After addition of 'channels' as the starting argument of new QAPI
syntax inside postcopy test, even if the user entered the old QAPI
syntax, test used the new syntax.
It was a temporary patch added to have some presence of the new syntax
since the migration qtest framework lacked any logic for introducing
'channels' argument.

Now that the qtest framework has logic to introduce uri and channel
argument separately, we can remove this temporary change.

Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/migration-test.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 5d6d8cd634..27a1066ae4 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1297,12 +1297,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
 
     migrate_prepare_for_dirty_mem(from);
     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' } } ] } }");
+                             "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
 
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
-- 
2.22.3

[PATCH 2/4] tests/qtest/migration: Replace 'migrate-incoming' qtest_qmp_assert_success with migrate_incoming_qmp

[Het Gala]

Already have a migrate_incoming_qmp helper function to initiate
'migrate-incoming' QMP command with some additional checks.
Replace 'migrate-incoming' qtest_qmp_assert_success command with
calling migrate_incoming_qmp helper function for postcopy qtests.

Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/migration-test.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 27a1066ae4..f2c27d611c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,8 +1296,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     migrate_ensure_non_converge(from);
 
     migrate_prepare_for_dirty_mem(from);
-    qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
-                             "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
 
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
-- 
2.22.3

[PATCH 3/4] tests/qtest/migration: Add channels parameter in migrate_incoming_qmp

[Het Gala]

Alter migrate_incoming_qmp() to allow both uri and channels
independently. For channels, convert string to a QDict.

Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/migration-helpers.c   | 13 +++++++++++--
 tests/qtest/migration-helpers.h   |  4 ++--
 tests/qtest/migration-test.c      | 12 ++++++------
 tests/qtest/virtio-net-failover.c |  8 ++++----
 4 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3b72cad6c1..cbd91719ae 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -245,7 +245,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,
+                          const char *channels, const char *fmt, ...)
 {
     va_list ap;
     QDict *args, *rsp, *data;
@@ -255,7 +256,15 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
     va_end(ap);
 
     g_assert(!qdict_haskey(args, "uri"));
-    qdict_put_str(args, "uri", uri);
+    if (uri) {
+        qdict_put_str(args, "uri", uri);
+    }
+
+    g_assert(!qdict_haskey(args, "channels"));
+    if (channels) {
+        QObject *channels_obj = qobject_from_json(channels, &error_abort);
+        qdict_put_obj(args, "channels", channels_obj);
+    }
 
     migrate_set_capability(to, "events", true);
 
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 1339835698..9f74281aea 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -29,9 +29,9 @@ G_GNUC_PRINTF(5, 6)
 void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
                  const char *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, ...);
+                          const char *channels, const char *fmt, ...);
 
 G_GNUC_PRINTF(4, 5)
 void migrate_qmp_fail(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f2c27d611c..fa8a860811 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,7 +1296,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     migrate_ensure_non_converge(from);
 
     migrate_prepare_for_dirty_mem(from);
-    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");
@@ -1824,7 +1824,7 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
      * We need to wait for the source to finish before starting the
      * destination.
      */
-    migrate_incoming_qmp(to, args->connect_uri, "{}");
+    migrate_incoming_qmp(to, args->connect_uri, NULL, "{}");
     wait_for_migration_complete(to);
 
     if (stop_src) {
@@ -2405,7 +2405,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,
@@ -2715,7 +2715,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;
 }
@@ -3040,7 +3040,7 @@ 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");
@@ -3068,7 +3068,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, "{}");
 
     wait_for_migration_status(from, "cancelled", NULL);
 
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.3

[PATCH 4/4] tests/qtest/migration: Add postcopy migration qtests to use 'channels' argument instead of uri

[Het Gala]

Add qtests to perform postcopy live migration by having list of
'channels' argument as the starting point instead of uri string.
(Note: length of the list is restricted to 1 for now)

Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/migration-test.c | 38 ++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index fa8a860811..599018baa0 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,13 +1296,17 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     migrate_ensure_non_converge(from);
 
     migrate_prepare_for_dirty_mem(from);
-    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
+    if (args->connect_channels) {
+        migrate_incoming_qmp(to, NULL, args->connect_channels, "{}");
+    } else {
+        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");
     wait_for_suspend(from, &src_state);
 
-    migrate_qmp(from, to, NULL, NULL, "{}");
+    migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
 
     migrate_wait_for_dirty_mem(from, to);
 
@@ -1355,6 +1359,20 @@ static void test_postcopy(void)
     test_postcopy_common(&args);
 }
 
+static void test_postcopy_channels(void)
+{
+    MigrateCommon args = {
+        .listen_uri = "defer",
+        .connect_channels = "[ { 'channel-type': 'main',"
+                            "    'addr': { 'transport': 'socket',"
+                            "              'type': 'inet',"
+                            "              'host': '127.0.0.1',"
+                            "              'port': '0' } } ]",
+    };
+
+    test_postcopy_common(&args);
+}
+
 static void test_postcopy_suspend(void)
 {
     MigrateCommon args = {
@@ -1555,6 +1573,18 @@ static void test_postcopy_recovery(void)
     test_postcopy_recovery_common(&args);
 }
 
+static void test_postcopy_recovery_channels(void)
+{
+    MigrateCommon args = {
+        .connect_channels = "[ { 'channel-type': 'main',"
+                            "    'addr': { 'transport': 'socket',"
+                            "              'type': 'inet',"
+                            "              'host': '127.0.0.1',"
+                            "              'port': '0' } } ]",
+    };
+
+    test_postcopy_recovery_common(&args);
+}
 static void test_postcopy_recovery_compress(void)
 {
     MigrateCommon args = {
@@ -3585,8 +3615,12 @@ int main(int argc, char **argv)
 
     if (has_uffd) {
         migration_test_add("/migration/postcopy/plain", test_postcopy);
+        migration_test_add("/migration/postcopy/channels/plain",
+                           test_postcopy_channels);
         migration_test_add("/migration/postcopy/recovery/plain",
                            test_postcopy_recovery);
+        migration_test_add("/migration/postcopy/recovery/channels/plain",
+                           test_postcopy_recovery_channels);
         migration_test_add("/migration/postcopy/preempt/plain",
                            test_postcopy_preempt);
         migration_test_add("/migration/postcopy/preempt/recovery/plain",
-- 
2.22.3

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb
>
> After addition of 'channels' as the starting argument of new QAPI
> syntax inside postcopy test, even if the user entered the old QAPI
> syntax, test used the new syntax.
> It was a temporary patch added to have some presence of the new syntax
> since the migration qtest framework lacked any logic for introducing
> 'channels' argument.

That wasn't clear to me when we merged that. Was that really the case?

>
> Now that the qtest framework has logic to introduce uri and channel
> argument separately, we can remove this temporary change.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>

Anyway, I can see the point of this.

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 2/4] tests/qtest/migration: Replace 'migrate-incoming' qtest_qmp_assert_success with migrate_incoming_qmp

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> Already have a migrate_incoming_qmp helper function to initiate
> 'migrate-incoming' QMP command with some additional checks.
> Replace 'migrate-incoming' qtest_qmp_assert_success command with
> calling migrate_incoming_qmp helper function for postcopy qtests.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 3/4] tests/qtest/migration: Add channels parameter in migrate_incoming_qmp

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> Alter migrate_incoming_qmp() to allow both uri and channels
> independently. For channels, convert string to a QDict.
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  tests/qtest/migration-helpers.c   | 13 +++++++++++--
>  tests/qtest/migration-helpers.h   |  4 ++--
>  tests/qtest/migration-test.c      | 12 ++++++------
>  tests/qtest/virtio-net-failover.c |  8 ++++----
>  4 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 3b72cad6c1..cbd91719ae 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -245,7 +245,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,
> +                          const char *channels, const char *fmt, ...)
>  {
>      va_list ap;
>      QDict *args, *rsp, *data;
> @@ -255,7 +256,15 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
>      va_end(ap);
>  
>      g_assert(!qdict_haskey(args, "uri"));
> -    qdict_put_str(args, "uri", uri);
> +    if (uri) {
> +        qdict_put_str(args, "uri", uri);
> +    }
> +
> +    g_assert(!qdict_haskey(args, "channels"));
> +    if (channels) {
> +        QObject *channels_obj = qobject_from_json(channels, &error_abort);
> +        qdict_put_obj(args, "channels", channels_obj);
> +    }

Do you think it makes sense to have channels take precedence here? It
would make the next patch cleaner without having to check for channels
presence. I don't think we need a 'both' test for incoming.

>  
>      migrate_set_capability(to, "events", true);
>  
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index 1339835698..9f74281aea 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -29,9 +29,9 @@ G_GNUC_PRINTF(5, 6)
>  void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>                   const char *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, ...);
> +                          const char *channels, const char *fmt, ...);
>  
>  G_GNUC_PRINTF(4, 5)
>  void migrate_qmp_fail(QTestState *who, const char *uri,
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index f2c27d611c..fa8a860811 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1296,7 +1296,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>      migrate_ensure_non_converge(from);
>  
>      migrate_prepare_for_dirty_mem(from);
> -    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");
> @@ -1824,7 +1824,7 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
>       * We need to wait for the source to finish before starting the
>       * destination.
>       */
> -    migrate_incoming_qmp(to, args->connect_uri, "{}");
> +    migrate_incoming_qmp(to, args->connect_uri, NULL, "{}");
>      wait_for_migration_complete(to);
>  
>      if (stop_src) {
> @@ -2405,7 +2405,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,
> @@ -2715,7 +2715,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;
>  }
> @@ -3040,7 +3040,7 @@ 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");
> @@ -3068,7 +3068,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, "{}");
>  
>      wait_for_migration_status(from, "cancelled", NULL);
>  
> 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");

Re: [PATCH 4/4] tests/qtest/migration: Add postcopy migration qtests to use 'channels' argument instead of uri

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> Add qtests to perform postcopy live migration by having list of
> 'channels' argument as the starting point instead of uri string.
> (Note: length of the list is restricted to 1 for now)
>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  tests/qtest/migration-test.c | 38 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index fa8a860811..599018baa0 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1296,13 +1296,17 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>      migrate_ensure_non_converge(from);
>  
>      migrate_prepare_for_dirty_mem(from);
> -    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
> +    if (args->connect_channels) {
> +        migrate_incoming_qmp(to, NULL, args->connect_channels, "{}");
> +    } else {
> +        migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
> +    }

From an interface perspective it's a bit unexpected to need this
conditional when the migrate_qmp below doesn't need it.

>  
>      /* Wait for the first serial output from the source */
>      wait_for_serial("src_serial");
>      wait_for_suspend(from, &src_state);
>  
> -    migrate_qmp(from, to, NULL, NULL, "{}");
> +    migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
>  
>      migrate_wait_for_dirty_mem(from, to);
>  
> @@ -1355,6 +1359,20 @@ static void test_postcopy(void)
>      test_postcopy_common(&args);
>  }
>  
> +static void test_postcopy_channels(void)
> +{
> +    MigrateCommon args = {
> +        .listen_uri = "defer",
> +        .connect_channels = "[ { 'channel-type': 'main',"
> +                            "    'addr': { 'transport': 'socket',"
> +                            "              'type': 'inet',"
> +                            "              'host': '127.0.0.1',"
> +                            "              'port': '0' } } ]",
> +    };
> +
> +    test_postcopy_common(&args);
> +}
> +
>  static void test_postcopy_suspend(void)
>  {
>      MigrateCommon args = {
> @@ -1555,6 +1573,18 @@ static void test_postcopy_recovery(void)
>      test_postcopy_recovery_common(&args);
>  }
>  
> +static void test_postcopy_recovery_channels(void)
> +{
> +    MigrateCommon args = {
> +        .connect_channels = "[ { 'channel-type': 'main',"
> +                            "    'addr': { 'transport': 'socket',"
> +                            "              'type': 'inet',"
> +                            "              'host': '127.0.0.1',"
> +                            "              'port': '0' } } ]",
> +    };
> +
> +    test_postcopy_recovery_common(&args);
> +}
>  static void test_postcopy_recovery_compress(void)
>  {
>      MigrateCommon args = {
> @@ -3585,8 +3615,12 @@ int main(int argc, char **argv)
>  
>      if (has_uffd) {
>          migration_test_add("/migration/postcopy/plain", test_postcopy);
> +        migration_test_add("/migration/postcopy/channels/plain",
> +                           test_postcopy_channels);
>          migration_test_add("/migration/postcopy/recovery/plain",
>                             test_postcopy_recovery);
> +        migration_test_add("/migration/postcopy/recovery/channels/plain",
> +                           test_postcopy_recovery_channels);
>          migration_test_add("/migration/postcopy/preempt/plain",
>                             test_postcopy_preempt);
>          migration_test_add("/migration/postcopy/preempt/recovery/plain",

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Peter Xu]

On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote:
> Het Gala <het.gala@nutanix.com> writes:
> 
> > This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb
> >
> > After addition of 'channels' as the starting argument of new QAPI
> > syntax inside postcopy test, even if the user entered the old QAPI
> > syntax, test used the new syntax.
> > It was a temporary patch added to have some presence of the new syntax
> > since the migration qtest framework lacked any logic for introducing
> > 'channels' argument.
> 
> That wasn't clear to me when we merged that. Was that really the case?

Yeah these look all a bit confusing..

I'm wondering whether do we need the new interface to cover both precopy
and postcopy, or one would suffice?

Both should share the same interface.  I think it means if we covered the
channels interface in precopy, then perhaps we don't need to test anywhere
else, as we got the code paths all covered.

We actually do the same already for all kinds of channels for postcopy,
where we stick with either tcp/unix but don't cover the rest.

-- 
Peter Xu

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]


On 10/04/24 6:34 pm, Fabiano Rosas wrote:
> !-------------------------------------------------------------------|
>    CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> Het Gala<het.gala@nutanix.com>  writes:
>
>> This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb
>>
>> After addition of 'channels' as the starting argument of new QAPI
>> syntax inside postcopy test, even if the user entered the old QAPI
>> syntax, test used the new syntax.
>> It was a temporary patch added to have some presence of the new syntax
>> since the migration qtest framework lacked any logic for introducing
>> 'channels' argument.
> That wasn't clear to me when we merged that. Was that really the case?

Yes, I had little to no experience on how to introduce 'channels' as a new
argument in the migration QAPIs back then.
IIRC, while trying to merge the series (migration: Modify 'migrate' and
'migrate-incoming' QAPI commands for migration), I was adviced to just 
modify
one of the qtest with the new QAPI syntax rather than writing a new qtest
altogether. So, I just renamed the old syntax with the new one.

>> Now that the qtest framework has logic to introduce uri and channel
>> argument separately, we can remove this temporary change.
>>
>> Signed-off-by: Het Gala<het.gala@nutanix.com>
> Anyway, I can see the point of this.
Thanks for the support for building that framework.
Today we can independently call channel or uri easily for 'migrate' QAPI
> Reviewed-by: Fabiano Rosas <farosas@suse.de

Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 2899 bytes --]

Re: [PATCH 3/4] tests/qtest/migration: Add channels parameter in migrate_incoming_qmp

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 7831 bytes --]


On 10/04/24 6:44 pm, Fabiano Rosas wrote:
> !-------------------------------------------------------------------|
>    CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> Het Gala<het.gala@nutanix.com>  writes:
>
>> Alter migrate_incoming_qmp() to allow both uri and channels
>> independently. For channels, convert string to a QDict.
>>
>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>> ---
>>   tests/qtest/migration-helpers.c   | 13 +++++++++++--
>>   tests/qtest/migration-helpers.h   |  4 ++--
>>   tests/qtest/migration-test.c      | 12 ++++++------
>>   tests/qtest/virtio-net-failover.c |  8 ++++----
>>   4 files changed, 23 insertions(+), 14 deletions(-)
>>
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index 3b72cad6c1..cbd91719ae 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -245,7 +245,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,
>> +                          const char *channels, const char *fmt, ...)
>>   {
>>       va_list ap;
>>       QDict *args, *rsp, *data;
>> @@ -255,7 +256,15 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
>>       va_end(ap);
>>   
>>       g_assert(!qdict_haskey(args, "uri"));
>> -    qdict_put_str(args, "uri", uri);
>> +    if (uri) {
>> +        qdict_put_str(args, "uri", uri);
>> +    }
>> +
>> +    g_assert(!qdict_haskey(args, "channels"));
>> +    if (channels) {
>> +        QObject *channels_obj = qobject_from_json(channels, &error_abort);
>> +        qdict_put_obj(args, "channels", channels_obj);
>> +    }
> Do you think it makes sense to have channels take precedence here? It
> would make the next patch cleaner without having to check for channels
> presence. I don't think we need a 'both' test for incoming.

Yes, this hack would silently solve, avoiding the above check.

IMO, it is good to have like to like QAPIs while running a qtest.
If the qtest uses the new syntax then, both 'migrate' and 'migrate-incoming'
QAPIs should use the new syntax. Even though implementation wise, it makes
no difference (qtests run successfully) but it would be good to ensure, 
we have
grammatical correctness.

>>   
>>       migrate_set_capability(to, "events", true);
>>   
>> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
>> index 1339835698..9f74281aea 100644
>> --- a/tests/qtest/migration-helpers.h
>> +++ b/tests/qtest/migration-helpers.h
>> @@ -29,9 +29,9 @@ G_GNUC_PRINTF(5, 6)
>>   void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>                    const char *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, ...);
>> +                          const char *channels, const char *fmt, ...);
>>   
>>   G_GNUC_PRINTF(4, 5)
>>   void migrate_qmp_fail(QTestState *who, const char *uri,
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index f2c27d611c..fa8a860811 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -1296,7 +1296,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>>       migrate_ensure_non_converge(from);
>>   
>>       migrate_prepare_for_dirty_mem(from);
>> -    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");
>> @@ -1824,7 +1824,7 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
>>        * We need to wait for the source to finish before starting the
>>        * destination.
>>        */
>> -    migrate_incoming_qmp(to, args->connect_uri, "{}");
>> +    migrate_incoming_qmp(to, args->connect_uri, NULL, "{}");
>>       wait_for_migration_complete(to);
>>   
>>       if (stop_src) {
>> @@ -2405,7 +2405,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,
>> @@ -2715,7 +2715,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;
>>   }
>> @@ -3040,7 +3040,7 @@ 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");
>> @@ -3068,7 +3068,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, "{}");
>>   
>>       wait_for_migration_status(from, "cancelled", NULL);
>>   
>> 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");
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 8940 bytes --]

Re: [PATCH 4/4] tests/qtest/migration: Add postcopy migration qtests to use 'channels' argument instead of uri

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 4930 bytes --]


On 10/04/24 6:45 pm, Fabiano Rosas wrote:
> !-------------------------------------------------------------------|
>    CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> Het Gala<het.gala@nutanix.com>  writes:
>
>> Add qtests to perform postcopy live migration by having list of
>> 'channels' argument as the starting point instead of uri string.
>> (Note: length of the list is restricted to 1 for now)
>>
>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>> ---
>>   tests/qtest/migration-test.c | 38 ++++++++++++++++++++++++++++++++++--
>>   1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index fa8a860811..599018baa0 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -1296,13 +1296,17 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>>       migrate_ensure_non_converge(from);
>>   
>>       migrate_prepare_for_dirty_mem(from);
>> -    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
>> +    if (args->connect_channels) {
>> +        migrate_incoming_qmp(to, NULL, args->connect_channels, "{}");
>> +    } else {
>> +        migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
>> +    }
>  From an interface perspective it's a bit unexpected to need this
> conditional when the migrate_qmp below doesn't need it.
I think, migrate_qmp has an unique advantage here. Please correct me If 
my understanding
is not correct.
1. test_migrate_start starts the qemu cmd line with either --incoming 
tcp:127.0.0.1:0
    or "defer". If tcp uri is provided, then migrate_incoming_qmp is not 
necessary
2. If "defer" is passed, then only migrate_incoming_qmp is required with 
tcp uri string
3. migrate_qmp can get the uri anyway either from
        test_migrate_start -> defer + migrate_incoming_qmp -> 
tcp:127.0.0.1:0
test_migrate_start -> tcp:127.0.0.1:0
    with the help of migrate_get_connect_uri() with the correct 
migration port.
    Even if we always pass NULL, we are okay, But this is just for tcp 
transport, and
    not unix

We can't have the unique situation for migrate_incoming_qmp, hence 
avoiding conditional
earlier seemed to be necessary for me. But, with the hack suggested by 
you in previous
patch, will prevent us from using conditional if else
>>   
>>       /* Wait for the first serial output from the source */
>>       wait_for_serial("src_serial");
>>       wait_for_suspend(from, &src_state);
>>   
>> -    migrate_qmp(from, to, NULL, NULL, "{}");
>> +    migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
>>   
>>       migrate_wait_for_dirty_mem(from, to);
>>   
>> @@ -1355,6 +1359,20 @@ static void test_postcopy(void)
>>       test_postcopy_common(&args);
>>   }
>>   
>> +static void test_postcopy_channels(void)
>> +{
>> +    MigrateCommon args = {
>> +        .listen_uri = "defer",
>> +        .connect_channels = "[ { 'channel-type': 'main',"
>> +                            "    'addr': { 'transport': 'socket',"
>> +                            "              'type': 'inet',"
>> +                            "              'host': '127.0.0.1',"
>> +                            "              'port': '0' } } ]",
>> +    };
>> +
>> +    test_postcopy_common(&args);
>> +}
>> +
>>   static void test_postcopy_suspend(void)
>>   {
>>       MigrateCommon args = {
>> @@ -1555,6 +1573,18 @@ static void test_postcopy_recovery(void)
>>       test_postcopy_recovery_common(&args);
>>   }
>>   
>> +static void test_postcopy_recovery_channels(void)
>> +{
>> +    MigrateCommon args = {
>> +        .connect_channels = "[ { 'channel-type': 'main',"
>> +                            "    'addr': { 'transport': 'socket',"
>> +                            "              'type': 'inet',"
>> +                            "              'host': '127.0.0.1',"
>> +                            "              'port': '0' } } ]",
>> +    };
>> +
>> +    test_postcopy_recovery_common(&args);
>> +}
>>   static void test_postcopy_recovery_compress(void)
>>   {
>>       MigrateCommon args = {
>> @@ -3585,8 +3615,12 @@ int main(int argc, char **argv)
>>   
>>       if (has_uffd) {
>>           migration_test_add("/migration/postcopy/plain", test_postcopy);
>> +        migration_test_add("/migration/postcopy/channels/plain",
>> +                           test_postcopy_channels);
>>           migration_test_add("/migration/postcopy/recovery/plain",
>>                              test_postcopy_recovery);
>> +        migration_test_add("/migration/postcopy/recovery/channels/plain",
>> +                           test_postcopy_recovery_channels);
>>           migration_test_add("/migration/postcopy/preempt/plain",
>>                              test_postcopy_preempt);
>>           migration_test_add("/migration/postcopy/preempt/recovery/plain",
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 6277 bytes --]

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Het Gala]

On 10/04/24 8:23 pm, Peter Xu wrote:
> !-------------------------------------------------------------------|
>    CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb
>>>
>>> After addition of 'channels' as the starting argument of new QAPI
>>> syntax inside postcopy test, even if the user entered the old QAPI
>>> syntax, test used the new syntax.
>>> It was a temporary patch added to have some presence of the new syntax
>>> since the migration qtest framework lacked any logic for introducing
>>> 'channels' argument.
>> That wasn't clear to me when we merged that. Was that really the case?
> Yeah these look all a bit confusing..
>
> I'm wondering whether do we need the new interface to cover both precopy
> and postcopy, or one would suffice?
>
> Both should share the same interface.  I think it means if we covered the
> channels interface in precopy, then perhaps we don't need to test anywhere
> else, as we got the code paths all covered.
>
> We actually do the same already for all kinds of channels for postcopy,
> where we stick with either tcp/unix but don't cover the rest.
Do we want to add other transports too (vsock, exec, rdma) with the new 
interface ?
I believe we have tests for fd based migration

Regards,
Het Gala

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Peter Xu]

On Thu, Apr 11, 2024 at 07:45:21PM +0530, Het Gala wrote:
> 
> On 10/04/24 8:23 pm, Peter Xu wrote:
> > !-------------------------------------------------------------------|
> >    CAUTION: External Email
> > 
> > |-------------------------------------------------------------------!
> > 
> > On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote:
> > > Het Gala <het.gala@nutanix.com> writes:
> > > 
> > > > This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb
> > > > 
> > > > After addition of 'channels' as the starting argument of new QAPI
> > > > syntax inside postcopy test, even if the user entered the old QAPI
> > > > syntax, test used the new syntax.
> > > > It was a temporary patch added to have some presence of the new syntax
> > > > since the migration qtest framework lacked any logic for introducing
> > > > 'channels' argument.
> > > That wasn't clear to me when we merged that. Was that really the case?
> > Yeah these look all a bit confusing..
> > 
> > I'm wondering whether do we need the new interface to cover both precopy
> > and postcopy, or one would suffice?
> > 
> > Both should share the same interface.  I think it means if we covered the
> > channels interface in precopy, then perhaps we don't need to test anywhere
> > else, as we got the code paths all covered.
> > 
> > We actually do the same already for all kinds of channels for postcopy,
> > where we stick with either tcp/unix but don't cover the rest.
> Do we want to add other transports too (vsock, exec, rdma) with the new
> interface ?
> I believe we have tests for fd based migration

Het,

What I meant is we used to do white box testing for migration, trying to
cover all the code paths would suffice for us in that case.

It means maybe we don't need the postcopy test to cover the channels
interface as long as precopy has covered that and also if that covered all
the "channels" abi then we should be safe.

What I worry is we keep extending the test matrix but we're actually
testing the same code paths.  Then the test runs slower each time, we burn
more cpus for each CI kick, but without a real beneift.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Het Gala]

On 11/04/24 7:56 pm, Peter Xu wrote:
> !-------------------------------------------------------------------|
>    CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> On Thu, Apr 11, 2024 at 07:45:21PM +0530, Het Gala wrote:
>> On 10/04/24 8:23 pm, Peter Xu wrote:
>>> !-------------------------------------------------------------------|
>>>     CAUTION: External Email
>>>
>>> |-------------------------------------------------------------------!
>>>
>>> On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote:
>>>> Het Gala <het.gala@nutanix.com> writes:
>>>>
>>>>> This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb
>>>>>
>>>>> After addition of 'channels' as the starting argument of new QAPI
>>>>> syntax inside postcopy test, even if the user entered the old QAPI
>>>>> syntax, test used the new syntax.
>>>>> It was a temporary patch added to have some presence of the new syntax
>>>>> since the migration qtest framework lacked any logic for introducing
>>>>> 'channels' argument.
>>>> That wasn't clear to me when we merged that. Was that really the case?
>>> Yeah these look all a bit confusing..
>>>
>>> I'm wondering whether do we need the new interface to cover both precopy
>>> and postcopy, or one would suffice?
>>>
>>> Both should share the same interface.  I think it means if we covered the
>>> channels interface in precopy, then perhaps we don't need to test anywhere
>>> else, as we got the code paths all covered.
>>>
>>> We actually do the same already for all kinds of channels for postcopy,
>>> where we stick with either tcp/unix but don't cover the rest.
>> Do we want to add other transports too (vsock, exec, rdma) with the new
>> interface ?
>> I believe we have tests for fd based migration
> Het,
>
> What I meant is we used to do white box testing for migration, trying to
> cover all the code paths would suffice for us in that case.
>
> It means maybe we don't need the postcopy test to cover the channels
> interface as long as precopy has covered that and also if that covered all
> the "channels" abi then we should be safe.
>
> What I worry is we keep extending the test matrix but we're actually
> testing the same code paths.  Then the test runs slower each time, we burn
> more cpus for each CI kick, but without a real beneift.


Yeah, I understood your concern Peter. Yes, since precopy and postcopy tests
cover the same code path, adding 'channels' postcopy qtests does not make
much sense after already having introduced in precopy qtests.

I just wanted to highlight couple of pointers:
1. though we are using 'channels' in the precopy tests for 'migrate' 
QAPI, we
    use the old uri for 'migrate-incoming' QAPI.
2. We do not cover other 'channels' abi, only have tcp path tested.

So, the TO-DOs could be:
1. Omit the 4th patch here, which introduced postcopy qtests with 'channels'
    interface OR have 'channels' interface with other than tcp transport
    (file, exec, vsock, etc) so as to cover different code paths.
2. Extend channels interface to migrate-incoming QAPI for precopy qtests


Regards,
Het Gala

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Peter Xu]

On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote:
> I just wanted to highlight couple of pointers:
> 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI,
> we
>    use the old uri for 'migrate-incoming' QAPI.
> 2. We do not cover other 'channels' abi, only have tcp path tested.
> 
> So, the TO-DOs could be:
> 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels'
>    interface OR have 'channels' interface with other than tcp transport
>    (file, exec, vsock, etc) so as to cover different code paths.
> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests

You can see whether Fabiano has anything to say, but what you proposed
looks good to me.

Thanks!

-- 
Peter Xu

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote:
>> I just wanted to highlight couple of pointers:
>> 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI,
>> we
>>    use the old uri for 'migrate-incoming' QAPI.
>> 2. We do not cover other 'channels' abi, only have tcp path tested.
>> 
>> So, the TO-DOs could be:
>> 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels'
>>    interface OR have 'channels' interface with other than tcp transport
>>    (file, exec, vsock, etc) so as to cover different code paths.
>> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests
>
> You can see whether Fabiano has anything to say, but what you proposed
> looks good to me.

Ok, so what about we convert some of the 'plain' tests into channels to
cover all transports?

- tcp: test_multifd_tcp_none  (this one we already did)
- file: test_precopy_file
- unix: test_precopy_unix_plain
- exec: test_analyze_script
- fd: test_migrate_precopy_fd_socket

Those^, plus the validate_uri that's already in next should cover
everything.

We don't need to do this at once, by the way.

Moreover:

- leave all test strings untouched to preserve bisecting;

- let's not bother adding "channels" and "uri" to the test string
  anymore. The channels API should be taken for granted at this point, I
  don't expect we start hitting bugs that will require us to run either
  foo/uri/plain or foo/channels/plain, so there's not much point in
  making the distinction.

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Peter Xu]

On Thu, Apr 11, 2024 at 04:41:16PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote:
> >> I just wanted to highlight couple of pointers:
> >> 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI,
> >> we
> >>    use the old uri for 'migrate-incoming' QAPI.
> >> 2. We do not cover other 'channels' abi, only have tcp path tested.
> >> 
> >> So, the TO-DOs could be:
> >> 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels'
> >>    interface OR have 'channels' interface with other than tcp transport
> >>    (file, exec, vsock, etc) so as to cover different code paths.
> >> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests
> >
> > You can see whether Fabiano has anything to say, but what you proposed
> > looks good to me.
> 
> Ok, so what about we convert some of the 'plain' tests into channels to
> cover all transports?
> 
> - tcp: test_multifd_tcp_none  (this one we already did)
> - file: test_precopy_file
> - unix: test_precopy_unix_plain
> - exec: test_analyze_script
> - fd: test_migrate_precopy_fd_socket
> 
> Those^, plus the validate_uri that's already in next should cover
> everything.
> 
> We don't need to do this at once, by the way.
> 
> Moreover:
> 
> - leave all test strings untouched to preserve bisecting;
> 
> - let's not bother adding "channels" and "uri" to the test string
>   anymore. The channels API should be taken for granted at this point, I
>   don't expect we start hitting bugs that will require us to run either
>   foo/uri/plain or foo/channels/plain, so there's not much point in
>   making the distinction.

Do you mean we can put "uri:" aside?  Maybe I misunderstood..

The matrix previously was (I think.. when this series posted):

  [tcp, unix, file, exec, fd] x [uri, channels] x [precopy, postcopy]

Drop postcopy as doesn't seem to have any special paths:

  [tcp, unix, file, exec, fd] x [uri, channels]

So logically we should still cover these, right?

Thanks,

-- 
Peter Xu

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Apr 11, 2024 at 04:41:16PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote:
>> >> I just wanted to highlight couple of pointers:
>> >> 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI,
>> >> we
>> >>    use the old uri for 'migrate-incoming' QAPI.
>> >> 2. We do not cover other 'channels' abi, only have tcp path tested.
>> >> 
>> >> So, the TO-DOs could be:
>> >> 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels'
>> >>    interface OR have 'channels' interface with other than tcp transport
>> >>    (file, exec, vsock, etc) so as to cover different code paths.
>> >> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests
>> >
>> > You can see whether Fabiano has anything to say, but what you proposed
>> > looks good to me.
>> 
>> Ok, so what about we convert some of the 'plain' tests into channels to
>> cover all transports?
>> 
>> - tcp: test_multifd_tcp_none  (this one we already did)
>> - file: test_precopy_file
>> - unix: test_precopy_unix_plain
>> - exec: test_analyze_script
>> - fd: test_migrate_precopy_fd_socket
>> 
>> Those^, plus the validate_uri that's already in next should cover
>> everything.
>> 
>> We don't need to do this at once, by the way.
>> 
>> Moreover:
>> 
>> - leave all test strings untouched to preserve bisecting;
>> 
>> - let's not bother adding "channels" and "uri" to the test string
>>   anymore. The channels API should be taken for granted at this point, I
>>   don't expect we start hitting bugs that will require us to run either
>>   foo/uri/plain or foo/channels/plain, so there's not much point in
>>   making the distinction.
>
> Do you mean we can put "uri:" aside?  Maybe I misunderstood..

I mean the test name does not need to specify "channels" vs. "uri"
because that should never be broken to the point that we actually need
to go fetch those tests by name. We'd still have at least 1 test for
each transport with channels and (existing) at least 1 test for each
transport with uri.

>
> The matrix previously was (I think.. when this series posted):
>
>   [tcp, unix, file, exec, fd] x [uri, channels] x [precopy, postcopy]
>
> Drop postcopy as doesn't seem to have any special paths:
>
>   [tcp, unix, file, exec, fd] x [uri, channels]
>
> So logically we should still cover these, right?

Right, I'm just suggesting we convert some tests to use channels, one
for each transport, to test the channels API in full. The rest of the
existing tests as well as future tests need not have a uri (or channel)
variant. Just one of them is enough.

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Peter Xu]

On Fri, Apr 12, 2024 at 11:58:43AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, Apr 11, 2024 at 04:41:16PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote:
> >> >> I just wanted to highlight couple of pointers:
> >> >> 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI,
> >> >> we
> >> >>    use the old uri for 'migrate-incoming' QAPI.
> >> >> 2. We do not cover other 'channels' abi, only have tcp path tested.
> >> >> 
> >> >> So, the TO-DOs could be:
> >> >> 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels'
> >> >>    interface OR have 'channels' interface with other than tcp transport
> >> >>    (file, exec, vsock, etc) so as to cover different code paths.
> >> >> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests
> >> >
> >> > You can see whether Fabiano has anything to say, but what you proposed
> >> > looks good to me.
> >> 
> >> Ok, so what about we convert some of the 'plain' tests into channels to
> >> cover all transports?
> >> 
> >> - tcp: test_multifd_tcp_none  (this one we already did)
> >> - file: test_precopy_file
> >> - unix: test_precopy_unix_plain
> >> - exec: test_analyze_script
> >> - fd: test_migrate_precopy_fd_socket
> >> 
> >> Those^, plus the validate_uri that's already in next should cover
> >> everything.
> >> 
> >> We don't need to do this at once, by the way.
> >> 
> >> Moreover:
> >> 
> >> - leave all test strings untouched to preserve bisecting;
> >> 
> >> - let's not bother adding "channels" and "uri" to the test string
> >>   anymore. The channels API should be taken for granted at this point, I
> >>   don't expect we start hitting bugs that will require us to run either
> >>   foo/uri/plain or foo/channels/plain, so there's not much point in
> >>   making the distinction.
> >
> > Do you mean we can put "uri:" aside?  Maybe I misunderstood..
> 
> I mean the test name does not need to specify "channels" vs. "uri"
> because that should never be broken to the point that we actually need
> to go fetch those tests by name. We'd still have at least 1 test for
> each transport with channels and (existing) at least 1 test for each
> transport with uri.
> 
> >
> > The matrix previously was (I think.. when this series posted):
> >
> >   [tcp, unix, file, exec, fd] x [uri, channels] x [precopy, postcopy]
> >
> > Drop postcopy as doesn't seem to have any special paths:
> >
> >   [tcp, unix, file, exec, fd] x [uri, channels]
> >
> > So logically we should still cover these, right?
> 
> Right, I'm just suggesting we convert some tests to use channels, one
> for each transport, to test the channels API in full. The rest of the
> existing tests as well as future tests need not have a uri (or channel)
> variant. Just one of them is enough.

Ah so that's the "test string"; sounds all good.

-- 
Peter Xu