Re: [RFC PATCH 2/4] tests/qtest/migration: Add a test that runs vmstate-static-checker

[Peter Xu <peterx@redhat.com>]

On Mon, May 27, 2024 at 07:52:28PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, May 23, 2024 at 05:19:20PM -0300, Fabiano Rosas wrote:
> >> We have the vmstate-static-checker script that takes the output of:
> >> '$QEMU -M $machine -dump-vmstate' for two different QEMU versions and
> >> compares them to check for compatibility breakages. This is just too
> >> simple and useful for us to pass on it. Add a test that runs the
> >> script.
> >> 
> >> Since this needs to use two different QEMU versions, the test is
> >> skipped if only one QEMU is provided. The infrastructure for passing
> >> more than one binary is already in place:
> >> 
> >> $ PYTHON=$(which python3.11) \
> >>  QTEST_QEMU_BINARY_SRC=../build-previous/qemu-system-x86_64 \
> >>  QTEST_QEMU_BINARY=./qemu-system-x86_64 \
> >>  ./tests/qtest/migration-test -p /x86_64/migration/vmstate-checker-script
> >> 
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> ---
> >> some code duplication for now, just so we can reason about this
> >> without too much noise
> >> ---
> >>  tests/qtest/migration-test.c | 82 ++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 82 insertions(+)
> >> 
> >> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> >> index e8d3555f56..2253e0fc5b 100644
> >> --- a/tests/qtest/migration-test.c
> >> +++ b/tests/qtest/migration-test.c
> >> @@ -63,6 +63,7 @@ static QTestMigrationState dst_state;
> >>  #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
> >>  
> >>  #define ANALYZE_SCRIPT "scripts/analyze-migration.py"
> >> +#define VMSTATE_CHECKER_SCRIPT "scripts/vmstate-static-checker.py"
> >>  
> >>  #define QEMU_VM_FILE_MAGIC 0x5145564d
> >>  #define FILE_TEST_FILENAME "migfile"
> >> @@ -1611,6 +1612,85 @@ static void test_analyze_script(void)
> >>      test_migrate_end(from, to, false);
> >>      cleanup("migfile");
> >>  }
> >> +
> >> +static void test_vmstate_checker_script(void)
> >> +{
> >> +    g_autofree gchar *cmd_src = NULL;
> >> +    g_autofree gchar *cmd_dst = NULL;
> >> +    g_autofree gchar *vmstate_src = NULL;
> >> +    g_autofree gchar *vmstate_dst = NULL;
> >> +    const char *machine_alias, *machine_opts = "";
> >> +    g_autofree char *machine = NULL;
> >> +    const char *arch = qtest_get_arch();
> >> +    int pid, wstatus;
> >> +    const char *python = g_getenv("PYTHON");
> >> +
> >> +    if (!getenv(QEMU_ENV_SRC) && !getenv(QEMU_ENV_DST)) {
> >> +        g_test_skip("Test needs two different QEMU versions");
> >> +        return;
> >> +    }
> >> +
> >> +    if (!python) {
> >> +        g_test_skip("PYTHON variable not set");
> >> +        return;
> >> +    }
> >> +
> >> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> >> +        if (g_str_equal(arch, "i386")) {
> >> +            machine_alias = "pc";
> >> +        } else {
> >> +            machine_alias = "q35";
> >> +        }
> >> +    } else if (g_str_equal(arch, "s390x")) {
> >> +        machine_alias = "s390-ccw-virtio";
> >> +    } else if (strcmp(arch, "ppc64") == 0) {
> >> +        machine_alias = "pseries";
> >> +    } else if (strcmp(arch, "aarch64") == 0) {
> >> +        machine_alias = "virt";
> >> +    } else {
> >> +        g_assert_not_reached();
> >> +    }
> >> +
> >> +    if (!qtest_has_machine(machine_alias)) {
> >> +        g_autofree char *msg = g_strdup_printf("machine %s not supported", machine_alias);
> >> +        g_test_skip(msg);
> >> +        return;
> >> +    }
> >> +
> >> +    machine = resolve_machine_version(machine_alias, QEMU_ENV_SRC,
> >> +                                      QEMU_ENV_DST);
> >> +
> >> +    vmstate_src = g_strdup_printf("%s/vmstate-src", tmpfs);
> >> +    vmstate_dst = g_strdup_printf("%s/vmstate-dst", tmpfs);
> >> +
> >> +    cmd_dst = g_strdup_printf("-machine %s,%s -dump-vmstate %s",
> >> +                              machine, machine_opts, vmstate_dst);
> >> +    cmd_src = g_strdup_printf("-machine %s,%s -dump-vmstate %s",
> >> +                              machine, machine_opts, vmstate_src);
> >> +
> >> +    qtest_init_with_env_no_handshake(QEMU_ENV_SRC, cmd_src);
> >> +    qtest_init_with_env_no_handshake(QEMU_ENV_DST, cmd_dst);
> >> +
> >> +    pid = fork();
> >> +    if (!pid) {
> >> +        close(1);
> >> +        open("/dev/null", O_WRONLY);
> >> +        execl(python, python, VMSTATE_CHECKER_SCRIPT,
> >> +              "-s", vmstate_src,
> >> +              "-d", vmstate_dst,
> >> +              NULL);
> >> +        g_assert_not_reached();
> >> +    }
> >> +
> >> +    g_assert(waitpid(pid, &wstatus, 0) == pid);
> >> +    if (!WIFEXITED(wstatus) || WEXITSTATUS(wstatus) != 0) {
> >> +        g_test_message("Failed to run vmstate-static-checker.py");
> >> +        g_test_fail();
> >> +    }
> >> +
> >> +    cleanup("vmstate-src");
> >> +    cleanup("vmstate-dst");
> >> +}
> >
> > Did I ask before on whether this can be written without C?
> 
> If you did I forgot about it, sorry.

Nah it's really about me forgetting things sometimes... nothing else.

> 
> > I think this and also the analyze-script are more suitable to be written in
> > other ways, e.g., bash or python, no?
> >
> 
> I would prefer not to fragment the test framework. There's a bunch of
> infra already present in migration-test/libqtest that we would end up
> having to rewrite in the other languages.

It's ok; I don't feel strongly on this one, but I want us to figure out
whether we should have such test enforced in CI.  Let's discuss that, and
I'm ok with this in C at least for now.

Note that one worst case scenario (if you prefer running this in pulls for
our submaintainer CI) is we can always introduce this test but only kick it
with QEMU_CI_MIGRATION=1.  As you said we lost the chance to fail exactly
on the pull of another maintainer as they won't normally set this bit, but
that goes back to the false negative issue, then we can cover this test
more frequently than "softfreeze-only", and no need for manual triggers.

-- 
Peter Xu