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

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Het Gala <het.gala@nutanix.com> writes:
>
>> On 27/02/24 1:04 am, Het Gala wrote:
>>>
>>>
>>> On 26/02/24 6:31 pm, Fabiano Rosas wrote:
>>>> Het Gala<het.gala@nutanix.com>  writes:
>>>>
>>>>> On 24/02/24 1:42 am, Fabiano Rosas wrote:
>>>>> 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...
>>>
>>> I think, here in qtest_qmp_assert_success, we actually can pass the 
>>> whole QMP command, and it just asserts that return key is present in 
>>> the response, though I am not very much familiar with qtest codebase 
>>> to verify how swiftly we can convert string into an actual QObject.
>>>
>>> [...]
>>>
>> I tried with qobject_from_json type of utility functions and the error I 
>> got was this :
>>
>> migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126: 
>> qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type < 
>> QTYPE__MAX' failed.
>>
>> And I suppose this was the case because, there are only limited types of 
>> QTYPE available
>>
>> typedefenumQType{
>> QTYPE_NONE,
>> QTYPE_QNULL,
>> QTYPE_QNUM,
>> QTYPE_QSTRING,
>> QTYPE_QDICT,
>> QTYPE_QLIST,
>> QTYPE_QBOOL,
>> QTYPE__MAX,
>> } QType;
>>
>> And 'channels' is a mixture of QDICT and QLIST and hence it is not able 
>> to easily convert from string to json.
>>
>> Thoughts on this ?
>
> I'm not sure what you tried. This works:
>
>     g_assert(!qdict_haskey(args, "channels"));
>     if (channels) {
>         channels_obj = qobject_from_json(channels, errp);
>         qdict_put_obj(args, "channels", channels_obj);
>     }
>
> And in the test:
>
>         .connect_channels = "[ { 'channel-type': 'main',"
>                             "    'addr': { 'transport': 'socket',"
>                             "              'type': 'inet',"
>                             "              'host': '127.0.0.1',"
>                             "              'port': '0' } } ]",
>         .listen_uri = "tcp:127.0.0.1:0",
>         .result = MIG_TEST_QMP_ERROR
>
> However, the real issue is how to inject the port for the source
> migration. The example above only works for the tests that are expected
> to fail. For a test that should pass, 0 as a port does not work.
>
> I'm thinking it might be better to alter migrate_qmp like this:
>
>   void migrate_qmp(QTestState *from, QTestState *to, const char *channels,
>                    const char *fmt, ...)
>
> Invocations would be:
>
>   migrate_qmp(from, to, NULL, "{uri: %s}", connect_uri);
>   migrate_qmp(from, to, args->channels, "{}");
>
> In this last case, if the test provided a port, we use it, otherwise we
> resolve it from the 'to' instance and put it in the QDict directly.
>
> I'll play with this a bit more tomorrow, let me know what you think.

Ok, so here's what I think we should do:

1) Add the 'to' object into migrate_qmp(), so we can use
migrate_get_socket_address() to get the port. Leave the other
migrate_qmp* alone because they don't need the port;

2) Move the calls to migrate_get_socket_address() into migrate_qmp() and
get rid of that connect_uri, use args->connect_uri instead everywhere;

3) Add a new function SocketAddress_to_qdict() that does the same as
SocketAddress_to_str(), but fills in a QDict instead;

4) Add args->connect_channels and pass it to migrate_qmp() and
migrate_qmp_fail(). Convert the string to a dict;

    if (channels) {
        channels_obj = qobject_from_json(channels, &error_abort);
        qdict_put_obj(args, "channels", channels_obj);
    }

5) Add a migrate_set_ports() function that uses 3) and fills in the port
in case it was 0 in the test. Handle a list of channels so we can add a
negative test that passes two channels;

  addr = migrate_get_socket_address(to);
  for each channel:
      if channel["port"] && channel["port"] == "0":
          channel["port"] = SocketAddress_to_qdict(addr->value)["port"]

(optionally iterate over addr to be prepared for when we add more
channels)

6) Alter migrate_qmp_fail() to allow both uri and channels
independently. No dealing with migrate_get_socket_address() because we
will fail before starting the migration anyway;

    if (uri) {
        qdict_put_str(args, "uri", uri);
    }

    if (channels) {
        channels_obj = qobject_from_json(channels, &error_abort);
        qdict_put_obj(args, "channels", channels_obj);
    }

7) Alter migrate_qmp() to only fill the uri if there are no
channels. Here we don't want to allow the wrong cases of having both or
none;

    if (uri) {
        qdict_put_str(args, "uri", uri);
    } else if (!channels) {
        qdict_put_str(args, "uri", migrate_get_socket_address(to));
    }

    if (channels) {
        channels_obj = qobject_from_json(channels, &error_abort);
        migrate_set_ports(to, qobject_to(QList, channels_obj));            
        qdict_put_obj(args, "channels", channels_obj);
    }

8) Write the tests to pass the list of channels;

    .connect_channels = "[ { 'channel-type': 'main',"
                        "    'addr': { 'transport': 'socket',"
                        "              'type': 'inet',"
                        "              'host': '127.0.0.1',"
                        "              'port': '0' } } ]",

With this we can test multiple channels, test other types of channel,
add more channel types in the future, etc and not need to change the
test infra.