Re: [RFC PATCH 3/4] tests/qtest/migration: Add support for simple device tests

[Peter Xu <peterx@redhat.com>]

On Mon, May 27, 2024 at 07:59:50PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, May 23, 2024 at 05:19:21PM -0300, Fabiano Rosas wrote:
> >> The current migration-tests are almost entirely focused on catching
> >> bugs on the migration code itself, not on the device migration
> >> infrastructure (vmstate). That means we miss catching some low hanging
> >> fruits that would show up immediately if only we had the device in
> >> question present in the VM.
> >> 
> >> Add a list of devices to include by default in the migration-tests,
> >> starting with one that recently had issues, virtio-gpu. Also add an
> >> environment variable QTEST_DEVICE_OPTS to allow test users to
> >> experiment with different devices or device options.
> >> 
> >> Do not run every migration test with the devices because that would
> >> increase the complexity of the command lines and, as mentioned, the
> >> migration-tests are mostly used to test the core migration code, not
> >> the device migration. Add a special value QTEST_DEVICE_OPTS=all that
> >> enables testing with devices.
> >> 
> >> Notes on usage:
> >> 
> >> For this new testing mode, it's not useful to run all the migration
> >> tests, a single test would probably suffice to catch any issues, so
> >> provide the -p option to migration-test and the test of your choice.
> >> 
> >> Like with the cross-version compatibility tests in CI and the recently
> >> introduced vmstate-static-checker test, to be of any use, a test with
> >> devices needs to be run against a different QEMU version, like so:
> >> 
> >> $ cd build
> >> $ QTEST_DEVICE_OPTS=all \
> >>  QTEST_QEMU_BINARY=./qemu-system-x86_64 \
> >>  QTEST_QEMU_BINARY_DST=../build-previous/qemu-system-x86_64 \
> >>  ./tests/qtest/migration-test -p /x86_64/migration/precopy/tcp/plain
> >> 
> >> $ cd build
> >> $ QTEST_DEVICE_OPTS='-device virtio-net' \
> >>  QTEST_QEMU_BINARY=./qemu-system-x86_64 \
> >>  QTEST_QEMU_BINARY_DST=../build-previous/qemu-system-x86_64 \
> >>  ./tests/qtest/migration-test -p /x86_64/migration/precopy/tcp/plain
> >> 
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> ---
> >>  tests/qtest/migration-test.c | 19 +++++++++++++++++--
> >>  1 file changed, 17 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> >> index 2253e0fc5b..35bb224d18 100644
> >> --- a/tests/qtest/migration-test.c
> >> +++ b/tests/qtest/migration-test.c
> >> @@ -71,6 +71,13 @@ static QTestMigrationState dst_state;
> >>  #define QEMU_ENV_SRC "QTEST_QEMU_BINARY_SRC"
> >>  #define QEMU_ENV_DST "QTEST_QEMU_BINARY_DST"
> >>  
> >> +/*
> >> + * The tests using DEFAULT_DEVICES need a special invocation and
> >> + * cannot be reached from make check, so don't bother with the
> >> + * --without-default-devices build.
> >
> > What's this "--without-default-devices"?
> 
> A configure option. It removes from the build any devices that are
> marked as default. It's an endless source of bugs because it is supposed
> to be paired with a config file that adds back some of the removed
> devices, but there's nothing enforcing that so we always run it as is
> and generate a broken QEMU binary.
> 
> So anything in the tests that refer to devices should first check if
> that QEMU binary even has the device present. I'm saying here that we're
> not going to do that because this test cannot be accidentally reached
> via make check. Realistically, most people will consume this test
> through the CI job only.

Ah I didn't expect that is an existing configure option.. then it is all
fine.

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu