Re: [PATCH v2 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument

[Het Gala]

On 24/02/24 1:42 am, Fabiano Rosas wrote:

> Het Gala <het.gala@nutanix.com> writes:
>
> > Introduce support for adding a 'channels' argument to migrate_qmp_fail,
> > migrate_incoming_qmp and migrate_qmp functions within the migration qtest
> > framework, enabling enhanced control over migration scenarios.
> >       
> Can't we just pass a channels string like you did in the original series
> with migrate_postcopy_prepare?
>
> We'd change migrate_* functions like this:
>
>   void migrate_qmp(QTestState *who, const char *uri, const char *channels,
>                    const char *fmt, ...)
>   {
>   ...
>       g_assert(!qdict_haskey(args, "uri"));
>       if (uri) {
>           qdict_put_str(args, "uri", uri);
>       }
>   
>       g_assert(!qdict_haskey(args, "channels"));
>       if (channels) {
>           qdict_put_str(args, "channels", channels);
>       }
>   }
>
> Write the test like this:
>
>   static void test_multifd_tcp_none_channels(void)
>   {
>       MigrateCommon args = {
>           .listen_uri = "defer",
>           .start_hook = test_migrate_precopy_tcp_multifd_start,
>           .live = true,
>           .connect_channels = "'channels': [ { 'channel-type': 'main',"
>                               "      'addr': { 'transport': 'socket',"
>                               "                'type': 'inet',"
>                               "                'host': '127.0.0.1',"
>                               "                'port': '0' } } ]",
>           .connect_uri = NULL;
>                                
>       };
>       test_precopy_common(&args);
>   }
>

this was the same first approach that I attempted. It won't work because

The final 'migrate' QAPI with channels string would look like

{ "execute": "migrate", "arguments": { "channels": "[ { "channel-type": "main", "addr": { "transport": "socket", "type": "inet", "host": "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }

instead of

{ "execute": "migrate", "arguments": { "channels": [ { "channel-type": "main", "addr": { "transport": "socket", "type": "inet", "host": "10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }

It would complain, that channels should be an array and not a string.

So, that's the reason parsing was required in qtest too.

I would be glad to hear if there are any ideas to convert string -> json object -> add it inside qdict along with uri ?

>   static void do_test_validate_uri_channel(MigrateCommon *args)
>   {
>       QTestState *from, *to;
>       g_autofree char *connect_uri = NULL;
>   
>       if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
>           return;
>       }
>   
>       wait_for_serial("src_serial");
>   
>       if (args->result == MIG_TEST_QMP_ERROR) {
>           migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
>       } else {
>           migrate_qmp(from, args->connect_uri, args->connect_channels, "{}");
>       }
>   
>       test_migrate_end(from, to, false);
>   }
>
> It's better to require test writers to pass in their own uri and channel
> strings. Otherwise any new transport added will require people to modify
> these conversion helpers.
>     
    I agree with your point here. I was thinking
      to have a general but a hacky version of migrate_uri_parse() but
      that too seemed like a overkill. I don't have a better solution to
      this right now

> Also, using the same string as the user would use in QMP helps with
> development in general. One could refer to the tests to see how to
> invoke the migration or experiment with the string in the tests during
> development.
>

For examples, I think - enough examples with 'channel' argument are provided where 'migrate' QAPI is defined. users can directly copy the qmp command from there itself.

Regards,

Het Gala