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

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>>> On Tue, Oct 03, 2023 at 05:24:50PM +0200, Philippe Mathieu-Daudé wrote:
> [...]
>
>>> $ 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.
>
> That is one idea, but why can't we convince a container to compile
> _both_ qemus?
>
> I am not familiar with containers, but:
> - We already know how to compile a qemu inside a container
> - We can teach it to compile $HEAD and v8.0.0 (or whatever)
>
> And do the test inside, right?
>
> On the other hand, your approach has the advantage that one can test
> opensuse qemu against fedora qemu, and similar.  Not sure how useful is
> that, though.
>
> [lots of code to find common machine types]

I'm working on a cleanup of this patch to make it more integrated with
libqtest. If we teach qtest_get_machines() to sometimes refresh the list
of machines then it becomes way less code.

> I think that it is just easier to pass the machine type we want to test
> to whatever script we have.  Specially where [sane] architectures like
> arm don't have a default machine type (no, I haven't double checked if
> that has changed lately).

We still need to enforce the same machine type for both binaries and a
sane range of QEMU versions. I think our docs state that we only support
migration from QEMU n->n+1 and vice versa? If the test will know what
combinations are allowed, it could just go ahead and use those.

>>> 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)
>
> That was going to be my suggestion.
>
>>> 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.
>
> I think we can do the same trick here:
>
> qtest_has_device_env("VARIABLE") and let qemu_has_device() just call it
> with QTEST_QEMU_BINARY.  Same for the others.
>
> It is more, if we can do it easy, we can do that qtest_init_env() checks
> in the case that variable is not QTEST_QEMU_BINARY that this one is also
> set, and assume in the rest of the code that options are compatible
> between whatever we passed an QTEST_QEMU_BINARY, that way we don't need
> to change anything else.  Well, just put a big warning in a comment
> saying that using qtest_init_env() means that you know what you are
> doing.
>
> Later, Juan.