[PATCH] qtest/migration: Add a test for the analyze-migration script

[PATCH] qtest/migration: Add a test for the analyze-migration script

[Fabiano Rosas]

Add a smoke test that migrates to a file and gives it to the
script. It should catch the most annoying errors such as changes in
the ram flags.

After code has been merged it becomes way harder to figure out what is
causing the script to fail, the person making the change is the most
likely to know right away what the problem is.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
I know this adds a python dependency to qtests and I'm not sure how
much we care about this script, but on the other hand it would be nice
to catch these errors early on.

This would also help with future work that touches the migration
stream (moving multifd out of ram.c and fixed-ram).

Let me know what you think.
---
 tests/qtest/meson.build      |  6 +++++
 tests/qtest/migration-test.c | 51 ++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 1fba07f4ed..d2511b3227 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -301,6 +301,10 @@ if gnutls.found()
   endif
 endif
 
+configure_file(input: meson.project_source_root() / 'scripts/analyze-migration.py',
+               output: 'analyze-migration.py',
+               configuration: configuration_data())
+
 qtests = {
   'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
   'cdrom-test': files('boot-sector.c'),
@@ -356,6 +360,8 @@ foreach dir : target_dirs
     test_deps += [qsd]
   endif
 
+  qtest_env.set('PYTHON', python.full_path())
+
   foreach test : target_qtests
     # Executables are shared across targets, declare them only the first time we
     # encounter them
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 1b43df5ca7..122089522f 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -66,6 +66,8 @@ static bool got_dst_resume;
  */
 #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
 
+#define ANALYZE_SCRIPT "tests/qtest/analyze-migration.py"
+
 #if defined(__linux__)
 #include <sys/syscall.h>
 #include <sys/vfs.h>
@@ -1486,6 +1488,52 @@ static void test_baddest(void)
     test_migrate_end(from, to, false);
 }
 
+#ifndef _WIN32
+static void test_analyze_script(void)
+{
+    MigrateStart args = {};
+    QTestState *from, *to;
+    g_autofree char *uri = NULL;
+    g_autofree char *file = NULL;
+    int pid, wstatus;
+    const char *python = g_getenv("PYTHON");
+
+    if (!python) {
+        g_test_skip("PYTHON variable not set");
+        return;
+    }
+
+    /* dummy url */
+    if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
+        return;
+    }
+
+    file = g_strdup_printf("%s/migfile", tmpfs);
+    uri = g_strdup_printf("exec:cat > %s", file);
+
+    migrate_ensure_converge(from);
+    migrate_qmp(from, uri, "{}");
+    wait_for_migration_complete(from);
+
+    pid = fork();
+    if (!pid) {
+        close(1);
+        open("/dev/null", O_WRONLY);
+        execl(python, python, ANALYZE_SCRIPT,
+              "-f", file, NULL);
+        g_assert_not_reached();
+    }
+
+    assert(waitpid(pid, &wstatus, 0) == pid);
+    if (WIFEXITED(wstatus) && WEXITSTATUS(wstatus) != 0) {
+        g_test_message("Failed to analyze the migration stream");
+        g_test_fail();
+    }
+    test_migrate_end(from, to, false);
+    cleanup("migfile");
+}
+#endif
+
 static void test_precopy_common(MigrateCommon *args)
 {
     QTestState *from, *to;
@@ -2828,6 +2876,9 @@ int main(int argc, char **argv)
     }
 
     qtest_add_func("/migration/bad_dest", test_baddest);
+#ifndef _WIN32
+    qtest_add_func("/migration/analyze-script", test_analyze_script);
+#endif
     qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
     qtest_add_func("/migration/precopy/unix/xbzrle", test_precopy_unix_xbzrle);
     /*
-- 
2.35.3

Re: [PATCH] qtest/migration: Add a test for the analyze-migration script

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Add a smoke test that migrates to a file and gives it to the
> script. It should catch the most annoying errors such as changes in
> the ram flags.
>
> After code has been merged it becomes way harder to figure out what is
> causing the script to fail, the person making the change is the most
> likely to know right away what the problem is.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> I know this adds a python dependency to qtests and I'm not sure how
> much we care about this script, but on the other hand it would be nice
> to catch these errors early on.
>
> This would also help with future work that touches the migration
> stream (moving multifd out of ram.c and fixed-ram).
>
> Let me know what you think.
> ---
>  tests/qtest/meson.build      |  6 +++++
>  tests/qtest/migration-test.c | 51 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
>
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 1fba07f4ed..d2511b3227 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -301,6 +301,10 @@ if gnutls.found()
>    endif
>  endif
>  
> +configure_file(input: meson.project_source_root() / 'scripts/analyze-migration.py',
> +               output: 'analyze-migration.py',
> +               configuration: configuration_data())
> +
>  qtests = {
>    'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
>    'cdrom-test': files('boot-sector.c'),
> @@ -356,6 +360,8 @@ foreach dir : target_dirs
>      test_deps += [qsd]
>    endif
>  
> +  qtest_env.set('PYTHON', python.full_path())
> +
>    foreach test : target_qtests
>      # Executables are shared across targets, declare them only the first time we
>      # encounter them
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 1b43df5ca7..122089522f 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -66,6 +66,8 @@ static bool got_dst_resume;
>   */
>  #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
>  
> +#define ANALYZE_SCRIPT "tests/qtest/analyze-migration.py"
> +
>  #if defined(__linux__)
>  #include <sys/syscall.h>
>  #include <sys/vfs.h>
> @@ -1486,6 +1488,52 @@ static void test_baddest(void)
>      test_migrate_end(from, to, false);
>  }
>  
> +#ifndef _WIN32
> +static void test_analyze_script(void)
> +{
> +    MigrateStart args = {};
> +    QTestState *from, *to;
> +    g_autofree char *uri = NULL;
> +    g_autofree char *file = NULL;
> +    int pid, wstatus;
> +    const char *python = g_getenv("PYTHON");
> +
> +    if (!python) {
> +        g_test_skip("PYTHON variable not set");
> +        return;
> +    }
> +
> +    /* dummy url */
> +    if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
> +        return;
> +    }
> +
> +    file = g_strdup_printf("%s/migfile", tmpfs);
> +    uri = g_strdup_printf("exec:cat > %s", file);
> +
> +    migrate_ensure_converge(from);
> +    migrate_qmp(from, uri, "{}");
> +    wait_for_migration_complete(from);
> +
> +    pid = fork();
> +    if (!pid) {
> +        close(1);
> +        open("/dev/null", O_WRONLY);
> +        execl(python, python, ANALYZE_SCRIPT,
> +              "-f", file, NULL);
> +        g_assert_not_reached();
> +    }
> +
> +    assert(waitpid(pid, &wstatus, 0) == pid);
> +    if (WIFEXITED(wstatus) && WEXITSTATUS(wstatus) != 0) {
> +        g_test_message("Failed to analyze the migration stream");
> +        g_test_fail();

I just noticed that this is really nice because it fails the test
without aborting, so we get a nice line in the output like this:

▶  44/355 /x86_64/migration/analyze-script  FAIL

which means that if we could replace some asserts with g_test_fail in
the migration code we would actually see what failed without having to
look at the "ok" line in the log and guess what the next test was.

> +    }
> +    test_migrate_end(from, to, false);
> +    cleanup("migfile");
> +}
> +#endif
> +
>  static void test_precopy_common(MigrateCommon *args)
>  {
>      QTestState *from, *to;
> @@ -2828,6 +2876,9 @@ int main(int argc, char **argv)
>      }
>  
>      qtest_add_func("/migration/bad_dest", test_baddest);
> +#ifndef _WIN32
> +    qtest_add_func("/migration/analyze-script", test_analyze_script);
> +#endif
>      qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
>      qtest_add_func("/migration/precopy/unix/xbzrle", test_precopy_unix_xbzrle);
>      /*

Re: [PATCH] qtest/migration: Add a test for the analyze-migration script

[Thomas Huth]

On 27/09/2023 23.47, Fabiano Rosas wrote:
> Add a smoke test that migrates to a file and gives it to the
> script. It should catch the most annoying errors such as changes in
> the ram flags.
> 
> After code has been merged it becomes way harder to figure out what is
> causing the script to fail, the person making the change is the most
> likely to know right away what the problem is.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> I know this adds a python dependency to qtests and I'm not sure how
> much we care about this script, but on the other hand it would be nice
> to catch these errors early on.
> 
> This would also help with future work that touches the migration
> stream (moving multifd out of ram.c and fixed-ram).
> 
> Let me know what you think.

Without looking at this too closely, my first thought was: This sounds 
rather like a good candidate for an avocado test instead. It's using Python, 
so tests/avocado/ sounds like a better fit. Have you considered adding it as 
an avocado test already?

 >+#define ANALYZE_SCRIPT "tests/qtest/analyze-migration.py"

Why can't you use scripts/analyze-migration.py directly?

 >+    file = g_strdup_printf("%s/migfile", tmpfs);

Please, no static file names for temporary files - tests might be running in 
parallel, and then you get race conditions! Use something like 
g_file_open_tmp() instead to create a file with a random name.

  Thomas

Re: [PATCH] qtest/migration: Add a test for the analyze-migration script

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 27/09/2023 23.47, Fabiano Rosas wrote:
>> Add a smoke test that migrates to a file and gives it to the
>> script. It should catch the most annoying errors such as changes in
>> the ram flags.
>> 
>> After code has been merged it becomes way harder to figure out what is
>> causing the script to fail, the person making the change is the most
>> likely to know right away what the problem is.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> I know this adds a python dependency to qtests and I'm not sure how
>> much we care about this script, but on the other hand it would be nice
>> to catch these errors early on.
>> 
>> This would also help with future work that touches the migration
>> stream (moving multifd out of ram.c and fixed-ram).
>> 
>> Let me know what you think.
>
> Without looking at this too closely, my first thought was: This sounds 
> rather like a good candidate for an avocado test instead. It's using Python, 
> so tests/avocado/ sounds like a better fit. Have you considered adding it as 
> an avocado test already?
>

I intended to keep all migration tests at the same place. And well, to
be honest, I have given up on avocado. Too unmaintained, incrutable
logging and last time I tried to use it locally, it was leaving stale
processes behind upon failure.

Of course, if that's the preferred place to put python tests I could do
it, but I don't find it too compelling.

>  >+#define ANALYZE_SCRIPT "tests/qtest/analyze-migration.py"
>
> Why can't you use scripts/analyze-migration.py directly?
>

I'm not entirely sure that's the case with QEMU, but generally build
directories can move/not be directly under the source tree. The test
wouldn't know from where to fetch the script.

>  >+    file = g_strdup_printf("%s/migfile", tmpfs);
>
> Please, no static file names for temporary files - tests might be running in 
> parallel, and then you get race conditions! Use something like 
> g_file_open_tmp() instead to create a file with a random name.
>

Ok, I can do that. However, if you look for "tmpfs" in migration-test.c
you'll see that's done all over the place. I'm thinking individual tests
under glib are never run in parallel. At least for the migration suite.

Re: [PATCH] qtest/migration: Add a test for the analyze-migration script

[Thomas Huth]

On 28/09/2023 15.32, Fabiano Rosas wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 27/09/2023 23.47, Fabiano Rosas wrote:
>>> Add a smoke test that migrates to a file and gives it to the
>>> script. It should catch the most annoying errors such as changes in
>>> the ram flags.
>>>
>>> After code has been merged it becomes way harder to figure out what is
>>> causing the script to fail, the person making the change is the most
>>> likely to know right away what the problem is.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>> I know this adds a python dependency to qtests and I'm not sure how
>>> much we care about this script, but on the other hand it would be nice
>>> to catch these errors early on.
>>>
>>> This would also help with future work that touches the migration
>>> stream (moving multifd out of ram.c and fixed-ram).
>>>
>>> Let me know what you think.
>>
>> Without looking at this too closely, my first thought was: This sounds
>> rather like a good candidate for an avocado test instead. It's using Python,
>> so tests/avocado/ sounds like a better fit. Have you considered adding it as
>> an avocado test already?
>>
> 
> I intended to keep all migration tests at the same place. And well, to
> be honest, I have given up on avocado. Too unmaintained, incrutable
> logging and last time I tried to use it locally, it was leaving stale
> processes behind upon failure.
> 
> Of course, if that's the preferred place to put python tests I could do
> it, but I don't find it too compelling.

Well, I guess this test here is kind of borderline, since it does not 
introduce new Python code, but just calls a pre-existing python script...
maybe that's still ok for the qtests ... what do other people think?

>>   >+#define ANALYZE_SCRIPT "tests/qtest/analyze-migration.py"
>>
>> Why can't you use scripts/analyze-migration.py directly?
>>
> 
> I'm not entirely sure that's the case with QEMU, but generally build
> directories can move/not be directly under the source tree. The test
> wouldn't know from where to fetch the script.

AFAIK the build system puts a symlink for the scripts folder into the build 
directory, at least I have one in mine.

>>   >+    file = g_strdup_printf("%s/migfile", tmpfs);
>>
>> Please, no static file names for temporary files - tests might be running in
>> parallel, and then you get race conditions! Use something like
>> g_file_open_tmp() instead to create a file with a random name.
>>
> 
> Ok, I can do that. However, if you look for "tmpfs" in migration-test.c
> you'll see that's done all over the place. I'm thinking individual tests
> under glib are never run in parallel. At least for the migration suite.

Ah, sorry, my bad, I thought that tmpfs was simply pointing to "/tmp", but 
in fact it already contains a randomized name. So never mind, I think it 
should be fine what you're doing here.

  Thomas

Re: [PATCH] qtest/migration: Add a test for the analyze-migration script

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 28/09/2023 15.32, Fabiano Rosas wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 27/09/2023 23.47, Fabiano Rosas wrote:
>>>> Add a smoke test that migrates to a file and gives it to the
>>>> script. It should catch the most annoying errors such as changes in
>>>> the ram flags.
>>>>
>>>> After code has been merged it becomes way harder to figure out what is
>>>> causing the script to fail, the person making the change is the most
>>>> likely to know right away what the problem is.
>>>>
>>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>>> ---
>>>> I know this adds a python dependency to qtests and I'm not sure how
>>>> much we care about this script, but on the other hand it would be nice
>>>> to catch these errors early on.
>>>>
>>>> This would also help with future work that touches the migration
>>>> stream (moving multifd out of ram.c and fixed-ram).
>>>>
>>>> Let me know what you think.
>>>
>>> Without looking at this too closely, my first thought was: This sounds
>>> rather like a good candidate for an avocado test instead. It's using Python,
>>> so tests/avocado/ sounds like a better fit. Have you considered adding it as
>>> an avocado test already?
>>>
>> 
>> I intended to keep all migration tests at the same place. And well, to
>> be honest, I have given up on avocado. Too unmaintained, incrutable
>> logging and last time I tried to use it locally, it was leaving stale
>> processes behind upon failure.
>> 
>> Of course, if that's the preferred place to put python tests I could do
>> it, but I don't find it too compelling.
>
> Well, I guess this test here is kind of borderline, since it does not 
> introduce new Python code, but just calls a pre-existing python script...
> maybe that's still ok for the qtests ... what do other people think?
>
>>>   >+#define ANALYZE_SCRIPT "tests/qtest/analyze-migration.py"
>>>
>>> Why can't you use scripts/analyze-migration.py directly?
>>>
>> 
>> I'm not entirely sure that's the case with QEMU, but generally build
>> directories can move/not be directly under the source tree. The test
>> wouldn't know from where to fetch the script.
>
> AFAIK the build system puts a symlink for the scripts folder into the build 
> directory, at least I have one in mine.
>

Oh, I have one too. It never crossed my mind that it could already be
there! I should be able to use it then.

Thanks!

Re: [PATCH] qtest/migration: Add a test for the analyze-migration script

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 28/09/2023 15.32, Fabiano Rosas wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 27/09/2023 23.47, Fabiano Rosas wrote:
>>>> Add a smoke test that migrates to a file and gives it to the
>>>> script. It should catch the most annoying errors such as changes in
>>>> the ram flags.
>>>>
>>>> After code has been merged it becomes way harder to figure out what is
>>>> causing the script to fail, the person making the change is the most
>>>> likely to know right away what the problem is.
>>>>
>>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>>> ---
>>>> I know this adds a python dependency to qtests and I'm not sure how
>>>> much we care about this script, but on the other hand it would be nice
>>>> to catch these errors early on.
>>>>
>>>> This would also help with future work that touches the migration
>>>> stream (moving multifd out of ram.c and fixed-ram).
>>>>
>>>> Let me know what you think.
>>>
>>> Without looking at this too closely, my first thought was: This sounds
>>> rather like a good candidate for an avocado test instead. It's using Python,
>>> so tests/avocado/ sounds like a better fit. Have you considered adding it as
>>> an avocado test already?
>>>
>> I intended to keep all migration tests at the same place. And well,
>> to
>> be honest, I have given up on avocado. Too unmaintained, incrutable
>> logging and last time I tried to use it locally, it was leaving stale
>> processes behind upon failure.
>> Of course, if that's the preferred place to put python tests I could
>> do
>> it, but I don't find it too compelling.
>
> Well, I guess this test here is kind of borderline, since it does not
> introduce new Python code, but just calls a pre-existing python
> script...
> maybe that's still ok for the qtests ... what do other people think?

I agree with adding it to migration-tests.
We can create a different test if necessary.

Reason for that is that this script is very useful (not enough) to find
interversion compatibilities.

We never found that it is failing after the release is done,  So testing
it continously is much better in my humble opinion.

>>>   >+#define ANALYZE_SCRIPT "tests/qtest/analyze-migration.py"
>>>
>>> Why can't you use scripts/analyze-migration.py directly?
>>>
>> I'm not entirely sure that's the case with QEMU, but generally build
>> directories can move/not be directly under the source tree. The test
>> wouldn't know from where to fetch the script.
>
> AFAIK the build system puts a symlink for the scripts folder into the
> build directory, at least I have one in mine.
>
>>>   >+    file = g_strdup_printf("%s/migfile", tmpfs);
>>>
>>> Please, no static file names for temporary files - tests might be running in
>>> parallel, and then you get race conditions! Use something like
>>> g_file_open_tmp() instead to create a file with a random name.
>>>
>> Ok, I can do that. However, if you look for "tmpfs" in
>> migration-test.c
>> you'll see that's done all over the place. I'm thinking individual tests
>> under glib are never run in parallel. At least for the migration suite.
>
> Ah, sorry, my bad, I thought that tmpfs was simply pointing to "/tmp",
> but in fact it already contains a randomized name. So never mind, I
> think it should be fine what you're doing here.

I am changing that in a series that I have to rebase.

Stay tuned.

Later, Juan.

Re: [PATCH] qtest/migration: Add a test for the analyze-migration script

[Peter Xu]

On Wed, Sep 27, 2023 at 06:47:56PM -0300, Fabiano Rosas wrote:
> I know this adds a python dependency to qtests and I'm not sure how
> much we care about this script, but on the other hand it would be nice
> to catch these errors early on.
> 
> This would also help with future work that touches the migration
> stream (moving multifd out of ram.c and fixed-ram).
> 
> Let me know what you think.

The test is good, but I think it'll be definitely less burden and cleaner
if it can be written just in shell scripts.. that can even be put in a
single line..

$ (echo "migrate exec:cat>$IMAGE"; echo "quit") | $QEMU_BIN -monitor stdio; $DIR/scripts/analyze-migration.py -f $IMAGE

Any chance to hook that in?

Thanks,

-- 
Peter Xu

Re: [PATCH] qtest/migration: Add a test for the analyze-migration script

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Sep 27, 2023 at 06:47:56PM -0300, Fabiano Rosas wrote:
>> I know this adds a python dependency to qtests and I'm not sure how
>> much we care about this script, but on the other hand it would be nice
>> to catch these errors early on.
>> 
>> This would also help with future work that touches the migration
>> stream (moving multifd out of ram.c and fixed-ram).
>> 
>> Let me know what you think.
>
> The test is good, but I think it'll be definitely less burden and cleaner
> if it can be written just in shell scripts.. that can even be put in a
> single line..
>
> $ (echo "migrate exec:cat>$IMAGE"; echo "quit") | $QEMU_BIN -monitor stdio; $DIR/scripts/analyze-migration.py -f $IMAGE
>
> Any chance to hook that in?

I tried but it's way worse.

We need to add the script to meson, pass the Python and QEMU binaries
in, make it be invoked for each architecture, but skip s390x and ppc
when using TCG only.

Then we need to add code to map target vs. machine type because arm has
no default machine.

We also need to produce TAP output for meson. And the migration script
outputs a ton of lines so Python barfs due to EAGAIN so we need to
handle that as well.

All of that is already there in migration-test.

I'll send a v2 of this patch with the improvements Thomas suggested and
try to dig out a fix for the script on ppc64 I made about 3 years ago
that seemed to have not been merged.