| Message ID | 20240523201922.28007-4-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration-test: Device migration smoke tests | expand |
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"? Other than this it looks all good.. thanks.
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.
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,
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. + */ +#define DEFAULT_DEVICES "-device virtio-gpu" + #if defined(__linux__) #include <sys/syscall.h> #include <sys/vfs.h> @@ -701,6 +708,7 @@ static int test_migrate_start(QTestState **from, QTestState **to, const char *memory_size; const char *machine_alias, *machine_opts = ""; g_autofree char *machine = NULL; + g_autofree gchar *device_opts = NULL; if (args->use_shmem) { if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) { @@ -793,12 +801,17 @@ static int test_migrate_start(QTestState **from, QTestState **to, g_test_message("Using machine type: %s", machine); + device_opts = g_strdup(getenv("QTEST_DEVICE_OPTS")); + if (g_str_equal(device_opts, "all")) { + device_opts = g_strdup(DEFAULT_DEVICES); + } + cmd_source = g_strdup_printf("-accel kvm%s -accel tcg " "-machine %s,%s " "-name source,debug-threads=on " "-m %s " "-serial file:%s/src_serial " - "%s %s %s %s %s", + "%s %s %s %s %s %s", kvm_opts ? kvm_opts : "", machine, machine_opts, memory_size, tmpfs, @@ -806,6 +819,7 @@ static int test_migrate_start(QTestState **from, QTestState **to, arch_source ? arch_source : "", shmem_opts ? shmem_opts : "", args->opts_source ? args->opts_source : "", + device_opts ? device_opts : "", ignore_stderr); if (!args->only_target) { *from = qtest_init_with_env(QEMU_ENV_SRC, cmd_source); @@ -820,7 +834,7 @@ static int test_migrate_start(QTestState **from, QTestState **to, "-m %s " "-serial file:%s/dest_serial " "-incoming %s " - "%s %s %s %s %s", + "%s %s %s %s %s %s", kvm_opts ? kvm_opts : "", machine, machine_opts, memory_size, tmpfs, uri, @@ -828,6 +842,7 @@ static int test_migrate_start(QTestState **from, QTestState **to, arch_target ? arch_target : "", shmem_opts ? shmem_opts : "", args->opts_target ? args->opts_target : "", + device_opts ? device_opts : "", ignore_stderr); *to = qtest_init_with_env(QEMU_ENV_DST, cmd_target); qtest_qmp_set_event_callback(*to,
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(-)