Re: [PATCH v2 8/9] tests/qtest/migration: Add a cancel test

[Peter Xu <peterx@redhat.com>]

On Tue, Feb 11, 2025 at 12:01:35PM -0300, Fabiano Rosas wrote:
> The qmp_migrate_cancel() command is poorly tested and code inspection
> reveals that there might be concurrency issues with its usage. Add a
> test that runs a migration and calls qmp_migrate_cancel() at specific
> moments.
> 
> In order to make the test more deterministic, instead of calling
> qmp_migrate_cancel() at random moments during migration, do it after
> the migration status change events are seen.
> 
> The expected result is that qmp_migrate_cancel() on the source ends
> migration on the source with the "cancelled" state and ends migration
> on the destination with the "failed" state. The only exception is that
> a failed migration should continue in the failed state.
> 
> Cancelling is not allowed during postcopy (no test is added for this
> because it's a trivial check in the code).
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration/precopy-tests.c | 176 ++++++++++++++++++++++++++
>  1 file changed, 176 insertions(+)
> 
> diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
> index 162fa69531..ba273d10b9 100644
> --- a/tests/qtest/migration/precopy-tests.c
> +++ b/tests/qtest/migration/precopy-tests.c
> @@ -20,6 +20,7 @@
>  #include "migration/migration-util.h"
>  #include "ppc-util.h"
>  #include "qobject/qlist.h"
> +#include "qapi-types-migration.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
>  #include "qemu/range.h"
> @@ -536,6 +537,161 @@ static void test_multifd_tcp_cancel(void)
>      migrate_end(from, to2, true);
>  }
>  
> +static void test_cancel_src_after_failed(QTestState *from, QTestState *to,
> +                                         const char *uri, const char *phase)
> +{
> +    /*
> +     * No migrate_incoming_qmp() at the start to force source into
> +     * failed state during migrate_qmp().
> +     */
> +
> +    wait_for_serial("src_serial");
> +    migrate_ensure_converge(from);
> +
> +    migrate_qmp(from, to, uri, NULL, "{}");
> +
> +    migration_event_wait(from, phase);
> +    migrate_cancel(from);
> +
> +    /* cancelling will not move the migration out of 'failed' */
> +
> +    wait_for_migration_status(from, "failed",
> +                              (const char * []) { "completed", NULL });
> +
> +    /*
> +     * Not waiting for the destination because it never started
> +     * migration.
> +     */
> +}
> +
> +static void test_cancel_src_after_cancelled(QTestState *from, QTestState *to,
> +                                            const char *uri, const char *phase)
> +{
> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
> +
> +    wait_for_serial("src_serial");
> +    migrate_ensure_converge(from);
> +
> +    migrate_qmp(from, to, uri, NULL, "{}");
> +
> +    /* To move to cancelled/cancelling */
> +    migrate_cancel(from);
> +    migration_event_wait(from, phase);
> +
> +    /* The migrate_cancel under test */
> +    migrate_cancel(from);
> +
> +    wait_for_migration_status(from, "cancelled",
> +                              (const char * []) { "completed", NULL });
> +
> +    wait_for_migration_status(to, "failed",
> +                              (const char * []) { "completed", NULL });
> +}
> +
> +static void test_cancel_src_after_complete(QTestState *from, QTestState *to,
> +                                           const char *uri, const char *phase)
> +{
> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
> +
> +    wait_for_serial("src_serial");
> +    migrate_ensure_converge(from);
> +
> +    migrate_qmp(from, to, uri, NULL, "{}");
> +
> +    migration_event_wait(from, phase);
> +    migrate_cancel(from);
> +
> +    /*
> +     * qmp_migrate_cancel() exits early if migration is not running
> +     * anymore, the status will not change to cancelled.
> +     */
> +    wait_for_migration_complete(from);
> +    wait_for_migration_complete(to);
> +}
> +
> +static void test_cancel_src_after_none(QTestState *from, QTestState *to,
> +                                       const char *uri, const char *phase)
> +{
> +    /*
> +     * Test that cancelling without a migration happening does not
> +     * affect subsequent migrations
> +     */
> +    migrate_cancel(to);
> +
> +    wait_for_serial("src_serial");
> +    migrate_cancel(from);
> +
> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
> +
> +    migrate_ensure_converge(from);
> +    migrate_qmp(from, to, uri, NULL, "{}");
> +
> +    wait_for_migration_complete(from);
> +    wait_for_migration_complete(to);
> +}
> +
> +static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
> +                                           const char *uri, const char *phase)
> +{
> +    migrate_set_capability(from, "pause-before-switchover", true);
> +    migrate_set_capability(to, "pause-before-switchover", true);
> +
> +    migrate_set_capability(from, "multifd", true);
> +    migrate_set_capability(to, "multifd", true);
> +
> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
> +
> +    wait_for_serial("src_serial");
> +    migrate_ensure_converge(from);
> +
> +    migrate_qmp(from, to, uri, NULL, "{}");
> +
> +    migration_event_wait(from, phase);
> +    migrate_cancel(from);
> +    migration_event_wait(from, "cancelling");
> +
> +    wait_for_migration_status(from, "cancelled",
> +                              (const char * []) { "completed", NULL });
> +
> +    wait_for_migration_status(to, "failed",
> +                              (const char * []) { "completed", NULL });
> +}
> +
> +static void test_cancel_src_after_status(void *opaque)
> +{
> +    const char *test_path = opaque;
> +    g_autofree char *phase = g_path_get_basename(test_path);
> +    g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> +    QTestState *from, *to;
> +    MigrateStart args = {
> +        .hide_stderr = true,
> +    };
> +
> +    if (migrate_start(&from, &to, "defer", &args)) {
> +        return;
> +    }
> +
> +    if (g_str_equal(phase, "cancelling") ||
> +        g_str_equal(phase, "cancelled")) {
> +        test_cancel_src_after_cancelled(from, to, uri, phase);
> +
> +    } else if (g_str_equal(phase, "completed")) {
> +        test_cancel_src_after_complete(from, to, uri, phase);
> +
> +    } else if (g_str_equal(phase, "failed")) {
> +        test_cancel_src_after_failed(from, to, uri, phase);
> +
> +    } else if (g_str_equal(phase, "none")) {
> +        test_cancel_src_after_none(from, to, uri, phase);
> +
> +    } else {
> +        /* any state that comes before pre-switchover */
> +        test_cancel_src_pre_switchover(from, to, uri, phase);

[1]

> +    }
> +
> +    migrate_end(from, to, false);
> +}

I'm OK with the current status, considering it at least enlarge our cancel
testcases so definitely good to have:

Reviewed-by: Peter Xu <peterx@redhat.com>

Though one thing to mention is the new "test_full()" abstraction doesn't
yet look like to benefit us a huge lot, IMHO.

These are the new tests:

# /x86_64/migration/cancel/src/after/none
# /x86_64/migration/cancel/src/after/setup             [*]
# /x86_64/migration/cancel/src/after/cancelling
# /x86_64/migration/cancel/src/after/cancelled
# /x86_64/migration/cancel/src/after/active
# /x86_64/migration/cancel/src/after/completed
# /x86_64/migration/cancel/src/after/failed
# /x86_64/migration/cancel/src/after/pre-switchover    [*]

We have only one abstracted path [1] to test random status, but that so far
only covers two cases marked with [*].  It is hard to say whether the
abstraction is necessary, or maybe it's easier we always register separate
test cases.  So it's still slightly debatable whether we could make all
above "if .. if else .. if else ... else" into separate tests.

> +
>  static void calc_dirty_rate(QTestState *who, uint64_t calc_time)
>  {
>      qtest_qmp_assert_success(who,
> @@ -1018,4 +1174,24 @@ void migration_test_add_precopy(MigrationTestEnv *env)
>                                 test_vcpu_dirty_limit);
>          }
>      }
> +
> +    /* ensure new status don't go unnoticed */
> +    assert(MIGRATION_STATUS__MAX == 15);
> +
> +    for (int i = MIGRATION_STATUS_NONE; i < MIGRATION_STATUS__MAX; i++) {
> +        switch (i) {
> +        case MIGRATION_STATUS_DEVICE: /* happens too fast */
> +        case MIGRATION_STATUS_WAIT_UNPLUG: /* no support in tests */
> +        case MIGRATION_STATUS_COLO: /* no support in tests */
> +        case MIGRATION_STATUS_POSTCOPY_ACTIVE: /* postcopy can't be cancelled */
> +        case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +        case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> +        case MIGRATION_STATUS_POSTCOPY_RECOVER:
> +            continue;
> +        default:
> +            migration_test_add_suffix("/migration/cancel/src/after/",
> +                                      MigrationStatus_str(i),
> +                                      test_cancel_src_after_status);
> +        }
> +    }
>  }
> -- 
> 2.35.3
> 

-- 
Peter Xu