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

[Fabiano Rosas <farosas@suse.de>]

Het Gala <het.gala@nutanix.com> writes:

> 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/ ?
>

Isn't this what the various qobject_from_json do? How does it work with
the existing tests?

    qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
                             "  'arguments': { "
                             "      'channels': [ { 'channel-type': 'main',"
                             "      'addr': { 'transport': 'socket',"
                             "                'type': 'inet',"
                             "                'host': '127.0.0.1',"
                             "                'port': '0' } } ] } }");

We can pass this^ string successfully to QMP somehow...

>>    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