Re: [PATCH V5 22/23] migration-test: cpr-transfer

[Peter Xu <peterx@redhat.com>]

On Thu, Jan 02, 2025 at 01:35:01PM -0500, Steven Sistare wrote:
> On 12/24/2024 3:01 PM, Peter Xu wrote:
> > On Tue, Dec 24, 2024 at 08:17:07AM -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/cpr-tests.c | 60 +++++++++++++++++++++++++++++++++++++++
> > >   tests/qtest/migration/framework.c | 19 +++++++++++++
> > >   tests/qtest/migration/framework.h |  3 ++
> > >   3 files changed, 82 insertions(+)
> > > 
> > > diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
> > > index 44ce89a..b221980 100644
> > > --- a/tests/qtest/migration/cpr-tests.c
> > > +++ b/tests/qtest/migration/cpr-tests.c
> > > @@ -44,6 +44,62 @@ static void test_mode_reboot(void)
> > >       test_file_common(&args, true);
> > >   }
> > > +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 const char *cpr_channel = g_strdup_printf(
> > > +        "cpr,addr.transport=socket,addr.type=unix,addr.path=%s",
> > > +        cpr_path);
> > > +    g_autofree char *opts_target = g_strdup_printf("-incoming %s %s",
> > > +                                                   cpr_channel, opts);
> > > +
> > > +    g_autofree char *connect_channels = g_strdup_printf(
> > > +        "[ { 'channel-type': 'main',"
> > > +        "    'addr': { 'transport': 'socket',"
> > > +        "              'type': 'unix',"
> > > +        "              'path': '%s' } } ]",
> > > +        mig_path);
> > 
> > So this test already uses json-format channels, IMHO we probably don't need
> > MigrateCommon.cpr_channel anymore?  We could put them all here. Then..
> 
> The previous version of this patch did that, and did not define cpr_channel,
> but you did not like how, in addition to using args->connect_channels, I
> defined the main uri in args->connect_uri and passed it to migrate_incoming_qmp.
> 
> The constraint I must satisfy is that main + cpr channels must be passed to
> migrate_qmp, but only the main channel can be passed to migrate_incoming_qmp.

Hmm true, I see now.

> 
> I could instead define an optional args->incoming_channels, passed to
> migrate_incoming_qmp, and only set by cpr.  Then the channel parsing extensions
> could be eliminated.
> 
> This current patch with its channel parsing support does make
> test_mode_transfer_common nicer in one way: the cpr channel is only described
> once, in dotted keys format.  A V6 that defines args->incoming_channels will
> need to define it once using dotted keys, to be embedded in startup args, and
> again in json in args->connect_uri.
> 
> I don't feel strongly either way: keep V5, or define incoming_channels.

Me too, please choose whatever easier for you.

Thanks,

-- 
Peter Xu