Re: [PATCH V4 18/19] migration-test: cpr-transfer

[Steven Sistare]

On 12/19/2024 11:56 AM, Peter Xu wrote:
> On Mon, Dec 02, 2024 at 05:20:10AM -0800, Steve Sistare wrote:
> > Add a migration test for cpr-transfer mode.  Defer the connection to the
> > target monitor, else the test hangs because in cpr-transfer mode QEMU does
> > not listen for monitor connections until we send the migrate command to
> > source QEMU.
> >
> > To test -incoming defer, send a migrate incoming command to the target,
> > after sending the migrate command to the source, as required by
> > cpr-transfer mode.
> >
> > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > ---
> >   tests/qtest/migration-test.c | 72 +++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index 8bc665d..4eb641c 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -1729,6 +1729,7 @@ static void test_precopy_common(MigrateCommon *args)
> >   {
> >       QTestState *from, *to;
> >       void *data_hook = NULL;
> > +    const char *connect_uri;
> >   
> >       if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
> >           return;
> > @@ -1766,11 +1767,16 @@ static void test_precopy_common(MigrateCommon *args)
> >           goto finish;
> >       }
> >   
> > -    migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
> > +    /* If has channels, then connect_uri is only used for listen defer */
> > +    connect_uri = args->connect_channels ? NULL : args->connect_uri;
> > +    migrate_qmp(from, to, connect_uri, args->connect_channels, "{}");
>
> This smells like abuse.
>
> If the test case sets connect_uri only because of below...
>
> >   
> >       if (args->start.defer_target_connect) {
> >           qtest_connect_deferred(to);
> >           qtest_qmp_handshake(to);
> > +        if (!strcmp(args->listen_uri, "defer")) {
> > +            migrate_incoming_qmp(to, args->connect_uri, "{}");
>
> ... here, then IMHO it's abusing connect_uri to start service incoming
> ports.
>
> We do have solution for "delay" incoming, right?   Shouldn't we use
> migrate_get_connect_uri() instead, then never set connect_uri in
> cpr-transfer tests?

We cannot use migrate_get_connect_uri() to get the URI to pass to
migrate_incoming_qmp, because the migrate_incoming_qmp sets the URI
returned by query-migrate.  chicken-and-egg problem.

I'll add channels support to migrate_incoming_qmp, like migrate_qmp.
The cpr-transfer test will set listen_uri and connect_channels but
will not set connect_uri.

> > +        }
> >       }
> >   
> >       if (args->result != MIG_TEST_SUCCEED) {
> > @@ -2415,6 +2421,66 @@ static void test_multifd_file_mapped_ram_fdset_dio(void)
> >   }
> >   #endif /* !_WIN32 */
> >   
> > +static void *test_mode_transfer_start(QTestState *from, QTestState *to)
> > +{
> > +    migrate_set_parameter_str(from, "mode", "cpr-transfer");
> > +    return NULL;
> > +}
> > +
> > +/*
> > + * cpr-transfer mode cannot use the target monitor prior to starting the
> > + * migration, and cannot connect synchronously to the monitor, so defer
> > + * the target connection.
> > + */
> > +static void test_mode_transfer_common(bool incoming_defer)
> > +{
> > +    g_autofree char *cpr_path = g_strdup_printf("%s/cpr.sock", tmpfs);
> > +    g_autofree char *mig_path = g_strdup_printf("%s/migsocket", tmpfs);
> > +    g_autofree char *uri = g_strdup_printf("unix:%s", mig_path);
> > +
> > +    const char *opts = "-machine aux-ram-share=on -nodefaults";
> > +    g_autofree char *opts_target = g_strdup_printf(
> > +        "-incoming \\{\\\'channel-type\\\':\\\'cpr\\\',"
> > +        "\\\'addr\\\':\\{\\\'transport\\\':\\\'socket\\\',"
> > +        "\\\'type\\\':\\\'unix\\\',\\\'path\\\':\\\'%s\\\'\\}\\} %s",
> > +        cpr_path, opts);
>
> Nobody will be able to change this easily.. Maybe use g_strescape()?

Agreed. Fortunately incoming now accepts dotted keys after a suggestion from
Markus, so this can be:
  -incoming cpr,addr.transport=socket,addr.type=unix,addr.path=%s

- Steve

> > +
> > +    g_autofree char *channels = g_strdup_printf(
> > +        "[ { 'channel-type': 'main',"
> > +        "    'addr': { 'transport': 'socket',"
> > +        "              'type': 'unix',"
> > +        "              'path': '%s' } },"
> > +        "  { 'channel-type': 'cpr',"
> > +        "    'addr': { 'transport': 'socket',"
> > +        "              'type': 'unix',"
> > +        "              'path': '%s' } } ]",
> > +        mig_path, cpr_path);
> > +
> > +    MigrateCommon args = {
> > +        .start.opts_source = opts,
> > +        .start.opts_target = opts_target,
> > +        .start.defer_target_connect = true,
> > +        .start.memory_backend = "-object 
> > memory-backend-memfd,id=pc.ram,size=%s"
> > +                                " -machine memory-backend=pc.ram",
> > +        .listen_uri = incoming_defer ? "defer" : uri,
> > +        .connect_uri = incoming_defer ? uri : NULL,
> > +        .connect_channels = channels,
> > +        .start_hook = test_mode_transfer_start,
> > +    };
> > +
> > +    test_precopy_common(&args);
> > +}
> > +
> > +static void test_mode_transfer(void)
> > +{
> > +    test_mode_transfer_common(NULL);
> > +}
> > +
> > +static void test_mode_transfer_defer(void)
> > +{
> > +    test_mode_transfer_common(true);
> > +}
> > +
> >   static void test_precopy_tcp_plain(void)
> >   {
> >       MigrateCommon args = {
> > @@ -3905,6 +3971,10 @@ int main(int argc, char **argv)
> >           migration_test_add("/migration/mode/reboot", test_mode_reboot);
> >       }
> >   
> > +    migration_test_add("/migration/mode/transfer", test_mode_transfer);
> > +    migration_test_add("/migration/mode/transfer/defer",
> > +                       test_mode_transfer_defer);
> > +
> >       migration_test_add("/migration/precopy/file/mapped-ram",
> >                          test_precopy_file_mapped_ram);
> >       migration_test_add("/migration/precopy/file/mapped-ram/live",
> > --
> > 1.8.3.1