Re: [RFC PATCH 1/1] qtest/migration: Support more than one QEMU binary

[Fabiano Rosas <farosas@suse.de>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Oct 03, 2023 at 05:24:50PM +0200, Philippe Mathieu-Daudé wrote:
>> Hi Fabiano,
>> 
>> [+Alex & Daniel]
>> 
>> On 3/10/23 16:19, Fabiano Rosas wrote:
>> > We have strict rules around migration compatibility between different
>> > QEMU versions but not a single test that validates the migration state
>> > between different binaries.
>> > 
>> > Add some infrastructure to allow running the migration tests with two
>> > different QEMU binaries as migration source and destination.
>> > 
>> > The code now recognizes two new environment variables QTEST_QEMU_SRC
>> > and QTEST_QEMU_DST. Only one of the two is expected to be used along
>> > with the existing QTEST_QEMU_BINARY, which will automatically be used
>> > for the other side of migration.
>> > 
>> > Usage:
>> > QTEST_QEMU_DST=./build-8.2.0/qemu-system-x86_64 \
>> > QTEST_QEMU_BINARY=../build-8.1.0/qemu-system-x86_64 \
>> > ./tests/qtest/migration-test
>> 
>> I like it as a first step, but I'd rather run $QTEST_QEMU_SRC
>> directly from a docker image, i.e.:
>> 
>> $ docker run -it opensuse/leap
>> # zypper update -y && zypper install -y qemu-x86
>> $ docker run opensuse/leap qemu-system-x86_64 ...
>
> In theory this does not need any support in libqtest at all
>
> $ cat myqemu.dkr 
> FROM fedora:38
>
> RUN dnf -y install qemu-kvm
>
> $ podman build -f myqemu.dkr --tag myqemu .
>
> $ cat > myqemu <<EOF
> #!/bin/sh
> exec podman run --volume /tmp=/tmp --security-opt label=disable myqemu qemu-system-x86_64 "$@"
>
> $ chmod +x myqemu
>
> $ QTEST_QEMU_BINARY=./myqemu.sh  ./build/tests/qtest/rtc-test

I'm favor of this. I usually set that variable to something like 'gdb
--args ...' and it works just fine.

> Except we fail on the last step, because bind mounts don't make UNIX domain
> sockets accessible. So we can see the /tmp/qtest-$PID.sock in the container,
> but it can't be used.
>
> UNIX domain sockets in the filesystem are tied to the mount namespace, and
> podman/docker inherantly creates a new mount namespace making the UNIX
> domani socket inaccessible.
>
> UNIX domain sockets in the abstract namespace, however, are tied to the
> network namespace, so if you used podman --network host, they should be
> accessible.
>
> libqtest could be changed to use abstract UNIX domain sockets on Linux
> only, and likely unlock the use of podman for QEMU.
>
>> 
>> > This code also works for when debugging with GDB to pass the same
>> > binary, but different GDB options for src and dst.
>> > 
>> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> > ---
>> >   tests/qtest/migration-helpers.c | 168 ++++++++++++++++++++++++++++++++
>> >   tests/qtest/migration-helpers.h |   3 +
>> >   tests/qtest/migration-test.c    |  52 ++++++++--
>> >   3 files changed, 216 insertions(+), 7 deletions(-)
>> > 
>> > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> > index be00c52d00..e84360c3b3 100644
>> > --- a/tests/qtest/migration-helpers.c
>> > +++ b/tests/qtest/migration-helpers.c
>> > @@ -12,6 +12,8 @@
>> >   #include "qemu/osdep.h"
>> >   #include "qapi/qmp/qjson.h"
>> > +#include "qapi/qmp/qlist.h"
>> > +#include "qapi/qmp/qstring.h"
>> >   #include "migration-helpers.h"
>> > @@ -180,3 +182,169 @@ void wait_for_migration_fail(QTestState *from, bool allow_active)
>> >       g_assert(qdict_get_bool(rsp_return, "running"));
>> >       qobject_unref(rsp_return);
>> >   }
>> > +
>> > +static char *query_pkg_version(QTestState *who)
>> > +{
>> > +    QDict *rsp;
>> > +    char *pkg;
>> > +
>> > +    rsp = qtest_qmp_assert_success_ref(who, "{ 'execute': 'query-version' }");
>> > +    g_assert(rsp);
>> > +
>> > +    pkg = g_strdup(qdict_get_str(rsp, "package"));
>> > +    qobject_unref(rsp);
>> > +
>> > +    return pkg;
>> > +}
>> > +
>> > +static QList *query_machines(void)
>> > +{
>> > +    QDict *response;
>> > +    QList *list;
>> > +    QTestState *qts;
>> > +
>> > +    qts = qtest_init("-machine none");
>> > +    response = qtest_qmp(qts, "{ 'execute': 'query-machines' }");
>> > +    g_assert(response);
>> > +    list = qdict_get_qlist(response, "return");
>> > +    g_assert(list);
>> > +
>> > +    qtest_quit(qts);
>> > +    return list;
>> > +}
>> > +
>> > +static char *get_default_machine(QList *list)
>> > +{
>> > +    QDict *info;
>> > +    QListEntry *entry;
>> > +    QString *qstr;
>> > +    char *name = NULL;
>> > +
>> > +    QLIST_FOREACH_ENTRY(list, entry) {
>> > +        info = qobject_to(QDict, qlist_entry_obj(entry));
>> > +        g_assert(info);
>> > +
>> > +        if (qdict_get(info, "is-default")) {
>> > +            qstr = qobject_to(QString, qdict_get(info, "name"));
>> > +            g_assert(qstr);
>> > +            name = g_strdup(qstring_get_str(qstr));
>> > +            break;
>> > +        }
>> > +    }
>> > +
>> > +    g_assert(name);
>> > +    return name;
>> > +}
>> > +
>> > +static bool search_default_machine(QList *list, const char *theirs)
>> > +{
>> > +    QDict *info;
>> > +    QListEntry *entry;
>> > +    QString *qstr;
>> > +
>> > +    if (!theirs) {
>> > +        return false;
>> > +    }
>> > +
>> > +    QLIST_FOREACH_ENTRY(list, entry) {
>> > +        info = qobject_to(QDict, qlist_entry_obj(entry));
>> > +        g_assert(info);
>> > +
>> > +        qstr = qobject_to(QString, qdict_get(info, "name"));
>> > +        g_assert(qstr);
>> > +
>> > +        if (g_str_equal(qstring_get_str(qstr), theirs)) {
>> > +            return true;
>> > +        }
>> > +    }
>> > +    return false;
>> > +}
>> > +
>> > +/*
>> > + * We need to ensure that both QEMU instances set via the QTEST_QEMU_*
>> > + * vars will use the same machine type. Use a custom query_machines
>> > + * function because the generic one in libqtest has a cache that would
>> > + * return the same machines for both binaries.
>> > + */
>> > +char *find_common_machine_type(const char *bin)
>> > +{
>> > +    QList *m1, *m2;
>> > +    g_autofree char *def1 = NULL;
>> > +    g_autofree char *def2 = NULL;
>> > +    const char *qemu_bin = getenv("QTEST_QEMU_BINARY");
>> > +
>> > +    m1 = query_machines();
>> > +
>> > +    g_setenv("QTEST_QEMU_BINARY", bin, true);
>> > +    m2 = query_machines();
>> > +    g_setenv("QTEST_QEMU_BINARY", qemu_bin, true);
>> > +
>> > +    def1 = get_default_machine(m1);
>> > +    def2 = get_default_machine(m2);
>> > +
>> > +    if (g_str_equal(def1, def2)) {
>> > +        /* either can be used */
>> > +        return g_strdup(def1);
>> > +    }
>> > +
>> > +    if (search_default_machine(m1, def2)) {
>> > +        return g_strdup(def2);
>> > +    }
>> > +
>> > +    if (search_default_machine(m2, def1)) {
>> > +        return g_strdup(def1);
>> > +    }
>> > +
>> > +    g_assert_not_reached();
>> > +}
>> > +
>> > +/*
>> > + * Init a guest for migration tests using an alternate QEMU binary for
>> > + * either the source or destination, depending on @var. The other
>> > + * binary should be set as usual via QTEST_QEMU_BINARY.
>> > + *
>> > + * Expected values:
>> > + *   QTEST_QEMU_SRC
>> > + *   QTEST_QEMU_DST
>> > + *
>> > + * Warning: The generic parts of qtest could be using
>> > + * QTEST_QEMU_BINARY to query for properties before we reach the
>> > + * migration code. If the alternate binary is too dissimilar that
>> > + * could cause issues.
>> > + */
>> > +static QTestState *init_vm(const char *extra_args, const char *var)
>> > +{
>> > +    const char *alt_bin = getenv(var);
>> > +    const char *qemu_bin = getenv("QTEST_QEMU_BINARY");
>> > +    g_autofree char *pkg = NULL;
>> > +    bool src = !!strstr(var, "SRC");
>> > +    QTestState *qts;
>> > +
>> > +    if (alt_bin) {
>> > +        g_setenv("QTEST_QEMU_BINARY", alt_bin, true);
>> > +    }
>> > +
>> > +    qts = qtest_init(extra_args);
>> > +    pkg = query_pkg_version(qts);
>> > +
>> > +    g_test_message("Using %s (%s) as migration %s",
>> > +                   alt_bin ? alt_bin : qemu_bin,
>> > +                   pkg,
>> > +                   src ? "source" : "destination");
>> > +
>> > +    if (alt_bin) {
>> > +        /* restore the original */
>> > +        g_setenv("QTEST_QEMU_BINARY", qemu_bin, true);
>> > +    }
>> > +    return qts;
>> > +}
>
> IMHO, we should just create a new qtest_init_env variant, that is the
> same as qtest_init, but accepts an environment variable name to use as
> an override.
>
> eg
>
>    qtest_init_env("QTEST_QEMU_BINARY_SRC", extra_args)
>
> it would look for $QTEST_QEMU_BINARY_SRC and if not found automatically
> fallback to $QTEST_QEMU_BINARY.
>

This was initially intended to be an off-tree patch that I could use to
test migration compatibility, so I avoided touching libqtest. Now that I
learned this might be of interest we can make it less hackish.

> I don't think there's any need to explicitly forbid setting both
> QTEST_QEMU_BINARY_SRC and QTEST_QEMU_BINARY_DST at the same time.
>

This is a little biased on my usage of migration-test from the command
line. Adding one of SRC|DST is easier when coming/going from a test that
already uses QTEST_QEMU_BINARY. No big deal, of course.

We'll just have to do something about qtest_get_machines(),
qtest_has_device() and qtest_get_arch(), which expect QTEST_QEMU_BINARY
to be present.