diff mbox series

[v4,4/6] migration-test: Make sure that multifd and cancel works

Message ID 20200122111517.33223-5-quintela@redhat.com
State New
Headers show
Series Fix multifd + cancel + multifd | expand

Commit Message

Juan Quintela Jan. 22, 2020, 11:15 a.m. UTC
Test that this sequerce works:

- launch source
- launch target
- start migration
- cancel migration
- relaunch target
- do migration again

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>

---

- Wait for 1st trhead to move to cancelled before launching second
  migration
- Add 'to2' parameter to diferentiate 1st and second target.
- Use g_free() instead of free()
---
 tests/qtest/migration-test.c | 112 ++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

Comments

Peter Maydell April 30, 2021, 10:57 p.m. UTC | #1
On Wed, 22 Jan 2020 at 11:20, Juan Quintela <quintela@redhat.com> wrote:
>
> Test that this sequerce works:
>
> - launch source
> - launch target
> - start migration
> - cancel migration
> - relaunch target
> - do migration again
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

A year-old commit, but we've just got around to running Coverity
on the tests/ directory, and it spotted this one:

>  static void migrate_set_capability(QTestState *who, const char *capability,
>                                     bool value)

The 3rd argument to migrate_set_capability() is a bool...


> +static void test_multifd_tcp_cancel(void)
> +{

> +    migrate_set_parameter_int(from, "downtime-limit", 1);
> +    /* 300MB/s */
> +    migrate_set_parameter_int(from, "max-bandwidth", 30000000);
> +
> +    migrate_set_parameter_int(from, "multifd-channels", 16);
> +    migrate_set_parameter_int(to, "multifd-channels", 16);
> +
> +    migrate_set_capability(from, "multifd", "true");
> +    migrate_set_capability(to, "multifd", "true");

...but here you pass it the character string '"true"' rather than
the boolean value 'true'.

This works by fluke since the implicit comparison of the literal string
against NULL will evaluate to true, but it isn't really right :-)

CID 1432373, 1432292, 1432288.

There seem to be 7 uses of the string "true" when the boolean
was intended; I don't know why Coverity only found 3 issues.

thanks
-- PMM
Dr. David Alan Gilbert May 4, 2021, 9:41 a.m. UTC | #2
* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Wed, 22 Jan 2020 at 11:20, Juan Quintela <quintela@redhat.com> wrote:
> >
> > Test that this sequerce works:
> >
> > - launch source
> > - launch target
> > - start migration
> > - cancel migration
> > - relaunch target
> > - do migration again
> >
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> A year-old commit, but we've just got around to running Coverity
> on the tests/ directory, and it spotted this one:
> 
> >  static void migrate_set_capability(QTestState *who, const char *capability,
> >                                     bool value)
> 
> The 3rd argument to migrate_set_capability() is a bool...

oops

> 
> > +static void test_multifd_tcp_cancel(void)
> > +{
> 
> > +    migrate_set_parameter_int(from, "downtime-limit", 1);
> > +    /* 300MB/s */
> > +    migrate_set_parameter_int(from, "max-bandwidth", 30000000);
> > +
> > +    migrate_set_parameter_int(from, "multifd-channels", 16);
> > +    migrate_set_parameter_int(to, "multifd-channels", 16);
> > +
> > +    migrate_set_capability(from, "multifd", "true");
> > +    migrate_set_capability(to, "multifd", "true");
> 
> ...but here you pass it the character string '"true"' rather than
> the boolean value 'true'.
> 
> This works by fluke since the implicit comparison of the literal string
> against NULL will evaluate to true, but it isn't really right :-)
> 
> CID 1432373, 1432292, 1432288.
> 
> There seem to be 7 uses of the string "true" when the boolean
> was intended; I don't know why Coverity only found 3 issues.

I'll send a patch.

Dave

> thanks
> -- PMM
>
diff mbox series

Patch

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b6a74a05ce..cf27ebbc9d 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -424,6 +424,14 @@  static void migrate_recover(QTestState *who, const char *uri)
     qobject_unref(rsp);
 }
 
+static void migrate_cancel(QTestState *who)
+{
+    QDict *rsp;
+
+    rsp = wait_command(who, "{ 'execute': 'migrate_cancel' }");
+    qobject_unref(rsp);
+}
+
 static void migrate_set_capability(QTestState *who, const char *capability,
                                    bool value)
 {
@@ -456,6 +464,8 @@  static void migrate_postcopy_start(QTestState *from, QTestState *to)
 typedef struct {
     bool hide_stderr;
     bool use_shmem;
+    /* only launch the target process */
+    bool only_target;
     char *opts_source;
     char *opts_target;
 } MigrateStart;
@@ -571,7 +581,9 @@  static int test_migrate_start(QTestState **from, QTestState **to,
                                  arch_source, shmem_opts, args->opts_source,
                                  ignore_stderr);
     g_free(arch_source);
-    *from = qtest_init(cmd_source);
+    if (!args->only_target) {
+        *from = qtest_init(cmd_source);
+    }
     g_free(cmd_source);
 
     cmd_target = g_strdup_printf("-accel kvm -accel tcg%s%s "
@@ -1294,6 +1306,103 @@  static void test_multifd_tcp(void)
     g_free(uri);
 }
 
+/*
+ * This test does:
+ *  source               target
+ *                       migrate_incoming
+ *     migrate
+ *     migrate_cancel
+ *                       launch another target
+ *     migrate
+ *
+ *  And see that it works
+ */
+
+static void test_multifd_tcp_cancel(void)
+{
+    MigrateStart *args = migrate_start_new();
+    QTestState *from, *to, *to2;
+    QDict *rsp;
+    char *uri;
+
+    args->hide_stderr = true;
+
+    if (test_migrate_start(&from, &to, "defer", args)) {
+        return;
+    }
+
+    /*
+     * We want to pick a speed slow enough that the test completes
+     * quickly, but that it doesn't complete precopy even on a slow
+     * machine, so also set the downtime.
+     */
+    /* 1 ms should make it not converge*/
+    migrate_set_parameter_int(from, "downtime-limit", 1);
+    /* 300MB/s */
+    migrate_set_parameter_int(from, "max-bandwidth", 30000000);
+
+    migrate_set_parameter_int(from, "multifd-channels", 16);
+    migrate_set_parameter_int(to, "multifd-channels", 16);
+
+    migrate_set_capability(from, "multifd", "true");
+    migrate_set_capability(to, "multifd", "true");
+
+    /* Start incoming migration from the 1st socket */
+    rsp = wait_command(to, "{ 'execute': 'migrate-incoming',"
+                           "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+    qobject_unref(rsp);
+
+    /* 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, "{}");
+
+    wait_for_migration_pass(from);
+
+    migrate_cancel(from);
+
+    args = migrate_start_new();
+    args->only_target = true;
+
+    if (test_migrate_start(&from, &to2, "defer", args)) {
+        return;
+    }
+
+    migrate_set_parameter_int(to2, "multifd-channels", 16);
+
+    migrate_set_capability(to2, "multifd", "true");
+
+    /* Start incoming migration from the 1st socket */
+    rsp = wait_command(to2, "{ 'execute': 'migrate-incoming',"
+                            "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+    qobject_unref(rsp);
+
+    uri = migrate_get_socket_address(to2, "socket-address");
+
+    wait_for_migration_status(from, "cancelled", NULL);
+
+    /* 300ms it should converge */
+    migrate_set_parameter_int(from, "downtime-limit", 300);
+    /* 1GB/s */
+    migrate_set_parameter_int(from, "max-bandwidth", 1000000000);
+
+    migrate_qmp(from, uri, "{}");
+
+    wait_for_migration_pass(from);
+
+    if (!got_stop) {
+        qtest_qmp_eventwait(from, "STOP");
+    }
+    qtest_qmp_eventwait(to2, "RESUME");
+
+    wait_for_serial("dest_serial");
+    wait_for_migration_complete(from);
+    test_migrate_end(from, to2, true);
+    g_free(uri);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -1359,6 +1468,7 @@  int main(int argc, char **argv)
 
     qtest_add_func("/migration/auto_converge", test_migrate_auto_converge);
     qtest_add_func("/migration/multifd/tcp", test_multifd_tcp);
+    qtest_add_func("/migration/multifd/tcp/cancel", test_multifd_tcp_cancel);
 
     ret = g_test_run();