Re: [PATCH] qtest: migration: Add failure test for 'uri' and 'channels' combination in 'migrate' QAPI

[Fabiano Rosas]

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

> On 09/02/24 1:33 pm, Het Gala wrote:

Hi Het,

>> I wanted to share an update regarding the patch I've been working on. 
>> It seems that the patch is not yet fully ready as it encountered some 
>> issues during the check-qtest builds.
>
> Test fails with error:
>
> 55/59 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test                 
> ERROR           25.77s   killed by signal 6 SIGABRT
>>>> G_TEST_DBUS_DAEMON=/rpmbuild/SOURCES/qemu/tests/dbus-vmstate-daemon.sh 
>>>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> QTEST_QEMU_IMG=./qemu-img 
> PYTHON=/rpmbuild/SOURCES/qemu/build/pyvenv/bin/python3 
> QTEST_QEMU_BINARY=./qemu-system-x86_64
> MALLOC_PERTURB_=71 /rpmbuild/SOURCES/qemu/build/tests/qtest/migration-test 
> --tap -k

Run this again with:

QTEST_LOG=1 QTEST_QEMU_BINARY=./qemu-system-x86_64 \
/rpmbuild/SOURCES/qemu/build/tests/qtest/migration-test -p \
/x86_64/migration/validate_uri_channel_both_set

The QTEST_LOG option should allow you to see if the guest has printed
any error message (you might need to adjust hide_stderr as well).

> ――――――――――――――――――――――――――――――――――――――――――――――― ✀  
> ――――――――――――――――――――――――――――――――――――――――――――――――
> stderr:
> Could not access KVM kernel module: No such file or directory
> Could not access KVM kernel module: No such file or directory
> Could not access KVM kernel module: No such file or directory
> Could not access KVM kernel module: No such file or directory
> Could not access KVM kernel module: No such file or directory
> Could not access KVM kernel module: No such file or directory
> Broken pipe
> ../tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process 
> but encountered exit status 1 (expected 0)
>
> (test program exited with status code -6)
>
> TAP parsing error: Too few tests run (expected 21, got 7)
>
>>
>> This is my first attempt at writing a test case related to migration, 
>> and I'm aware that there may be areas where I could use some guidance. 
>> If there are any gaps in my understanding of how to properly mock a 
>> migration or if there are any other issues with the test case, I would 
>> greatly appreciate your assistance. I'm also struggling to understand 
>> why the test is failing. If anyone could provide some insight or 
>> assistance with troubleshooting, it would be greatly appreciated.
>>
>> I've cc'd Fabino, Daniel, as I believe they may have expertise in 
>> migration testing and could offer some valuable insights.
>>
>> Thank you for your help with this, and I look forward to any feedback 
>> or assistance you can provide.
>>
>> On 09/02/24 1:21 pm, Het Gala wrote:
>>> Ensure failure occurs while adding validation test for 'uri' and 
>>> 'channels' arguments
>>> used simultaneously in the 'migrate' QAPI command.
>>>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>> ---
>>>   tests/qtest/migration-helpers.c | 14 ++++++--
>>>   tests/qtest/migration-helpers.h |  5 +--
>>>   tests/qtest/migration-test.c    | 60 +++++++++++++++++++++++++++++++--
>>>   3 files changed, 72 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tests/qtest/migration-helpers.c 
>>> b/tests/qtest/migration-helpers.c
>>> index e451dbdbed..2dbb01e413 100644
>>> --- a/tests/qtest/migration-helpers.c
>>> +++ b/tests/qtest/migration-helpers.c
>>> @@ -43,7 +43,8 @@ bool migrate_watch_for_events(QTestState *who, 
>>> const char *name,
>>>       return false;
>>>   }
>>>   -void migrate_qmp_fail(QTestState *who, const char *uri, const char 
>>> *fmt, ...)
>>> +void migrate_qmp_fail(QTestState *who, const char *uri,
>>> +                      const char *channels, const char *fmt, ...)
>>>   {
>>>       va_list ap;
>>>       QDict *args, *err;
>>> @@ -52,8 +53,15 @@ void migrate_qmp_fail(QTestState *who, const char 
>>> *uri, const char *fmt, ...)
>>>       args = qdict_from_vjsonf_nofail(fmt, ap);
>>>       va_end(ap);
>>>   -    g_assert(!qdict_haskey(args, "uri"));
>>> -    qdict_put_str(args, "uri", uri);
>>> +    if (uri) {
>>> +        g_assert(!qdict_haskey(args, "uri"));
>>> +        qdict_put_str(args, "uri", uri);
>>> +    }
>>> +
>>> +    if (channels) {
>>> +        g_assert(!qdict_haskey(args, "channels"));
>>> +        qdict_put_str(args, "channels", channels);
>>> +    }
>>>         err = qtest_qmp_assert_failure_ref(
>>>           who, "{ 'execute': 'migrate', 'arguments': %p}", args);
>>> diff --git a/tests/qtest/migration-helpers.h 
>>> b/tests/qtest/migration-helpers.h
>>> index 3bf7ded1b9..d49e289c51 100644
>>> --- a/tests/qtest/migration-helpers.h
>>> +++ b/tests/qtest/migration-helpers.h
>>> @@ -32,8 +32,9 @@ G_GNUC_PRINTF(3, 4)
>>>   void migrate_incoming_qmp(QTestState *who, const char *uri,
>>>                             const char *fmt, ...);
>>>   -G_GNUC_PRINTF(3, 4)
>>> -void migrate_qmp_fail(QTestState *who, const char *uri, const char 
>>> *fmt, ...);
>>> +G_GNUC_PRINTF(4, 5)
>>> +void migrate_qmp_fail(QTestState *who, const char *uri,
>>> +                      const char *channels, const char *fmt, ...);
>>>     void migrate_set_capability(QTestState *who, const char *capability,
>>>                               bool value);
>>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>>> index d3066e119f..3aaffc2667 100644
>>> --- a/tests/qtest/migration-test.c
>>> +++ b/tests/qtest/migration-test.c
>>> @@ -18,6 +18,7 @@
>>>   #include "qemu/module.h"
>>>   #include "qemu/option.h"
>>>   #include "qemu/range.h"
>>> +#include "migration/migration.h"
>>>   #include "qemu/sockets.h"
>>>   #include "chardev/char.h"
>>>   #include "qapi/qapi-visit-sockets.h"
>>> @@ -1773,7 +1774,7 @@ static void test_precopy_common(MigrateCommon 
>>> *args)
>>>       }
>>>         if (args->result == MIG_TEST_QMP_ERROR) {
>>> -        migrate_qmp_fail(from, connect_uri, "{}");
>>> +        migrate_qmp_fail(from, connect_uri, NULL, "{}");
>>>           goto finish;
>>>       }
>>>   @@ -1869,7 +1870,7 @@ static void test_file_common(MigrateCommon 
>>> *args, bool stop_src)
>>>       }
>>>         if (args->result == MIG_TEST_QMP_ERROR) {
>>> -        migrate_qmp_fail(from, connect_uri, "{}");
>>> +        migrate_qmp_fail(from, connect_uri, NULL, "{}");
>>>           goto finish;
>>>       }
>>>   @@ -2508,6 +2509,59 @@ static void 
>>> test_validate_uuid_dst_not_set(void)
>>>       do_test_validate_uuid(&args, false);
>>>   }
>>>   +static void do_test_validate_uri_channel(MigrateCommon *args, bool 
>>> should_fail)
>> Not sure if should_fail is of any value here. The test ideally should 
>> not enter migration also. Should just fail even before making the 
>> connection, at the QMP level itself. I added it here, by taking the 
>> reference of validate_uuid tests.

It might be if you decide to add positive tests as well.

>>> +{
>>> +    g_autofree const char *uri = "127.0.0.1:0";
>>> +    g_autofree const char *channels = "{"
>>> +               "   'channels': [ { 'channel-type': 'main',"
>>> +               "                   'addr': { 'transport': 'socket',"
>>> +               "                             'type': 'inet',"
>>> +               "                             'host': '127.0.0.1',"
>>> +               "                             'port': '0' } } ] }";
>>> +    QTestState *from, *to;
>>> +
>>> +    if (test_migrate_start(&from, &to, uri, &args->start)) {
>>> +        return;
>>> +    }
>>> +
>>> +    /* Wait for the first serial output from the source */
>>> +    wait_for_serial("src_serial");
>>> +
>>> +    /*
>>> +     * 'uri' and 'channels' validation is checked even before the 
>>> migration
>>> +     * starts.
>>> +     */
>>> +    if (args->result == MIG_TEST_QMP_ERROR) {
>>> +        migrate_qmp_fail(from, uri, channels, "{}");
>>> +        goto finish;
>>> +    }
>>> +
>>> +    migrate_qmp(from, uri, "{}");
>>> +
>>> +    if (should_fail) {
>>> +        qtest_set_expected_status(to, EXIT_FAILURE);
>>> +        wait_for_migration_fail(from, false);

This is probably not useful if the QMP command has failed already. See
test_precopy_file_offset_bad as an example.

>>> +    } else {
>>> +        wait_for_migration_complete(from);
>>> +    }
>>> +
>>> +finish:
>>> +    test_migrate_end(from, to, args->result == MIG_TEST_QMP_ERROR);
>>> +}
>>> +
>>> +static void
>>> +test_validate_uri_channel_both_set(void)
>>> +{
>>> +    MigrateCommon args = {
>>> +        .start = {
>>> +            .hide_stderr = true,
>>> +        },
>>> +        .result = MIG_TEST_QMP_ERROR,
>>> +    };
>>> +
>>> +    do_test_validate_uri_channel(&args, true);
>>> +}
>>> +
>>>   /*
>>>    * The way auto_converge works, we need to do too many passes to
>>>    * run this test.  Auto_converge logic is only run once every
>>> @@ -3536,6 +3590,8 @@ int main(int argc, char **argv)
>>>                          test_validate_uuid_src_not_set);
>>>       migration_test_add("/migration/validate_uuid_dst_not_set",
>>>                          test_validate_uuid_dst_not_set);
>>> + migration_test_add("/migration/validate_uri_channel_both_set",
>>> +                       test_validate_uri_channel_both_set);

Here I'd add some subdivisions so we can in the future add more tests
for this:

migration_test_add("/migration/validate_uri/channel", ...);
migration_test_add("/migration/validate_uri/channel/both_set",
                   test_validate_uri_channel_both_set);

The first one could be a positive test for instance. It's not required,
just a suggestion.

>>>       /*
>>>        * See explanation why this test is slow on function definition
>>>        */