[PATCH v3 0/4] migration & CI: Add a CI job for migration compat testing

[PATCH v3 0/4] migration & CI: Add a CI job for migration compat testing

[Fabiano Rosas]

Here's the second half of adding a migration compatibility test to CI.

We've already added support for running the full set of migration
tests with two QEMU binaries since commit 5050ad2a380
("tests/qtest/migration: Support more than one QEMU binary"), now
what's left is adding it to the CI.

I included patches that solve the problem of testing older QEMUs with
new test code. The old QEMU might lack features, bug fixes, etc. that
the tests expect to be present. After this series we can specify a
minimal QEMU version that a specific test supports.

changes since v2:
 - fixed version comparison once again

 - removed the 8.2 fixes. We don't need them anymore because we're now
   testing two 8.2 streams (8.2.0 vs. 8.2.50).

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1128168149

v2:
https://lore.kernel.org/r/20240104171857.20108-1-farosas@suse.de

v1:
https://lore.kernel.org/r/20231207155809.25673-1-farosas@suse.de

Fabiano Rosas (4):
  tests/qtest: Add a helper to query the QEMU version
  tests/qtest/migration: Add infrastructure to skip tests on older QEMUs
  ci: Add a migration compatibility test job
  [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

 tests/qtest/libqtest.h          | 10 ++++++
 tests/qtest/migration-helpers.h |  1 +
 tests/qtest/libqtest.c          | 24 +++++++++++++
 tests/qtest/migration-helpers.c | 48 +++++++++++++++++++++++++
 tests/qtest/migration-test.c    | 63 +++++++++++++++++++++++++++++----
 .gitlab-ci.d/buildtest.yml      | 53 +++++++++++++++++++++++++++
 6 files changed, 192 insertions(+), 7 deletions(-)

-- 
2.35.3

[PATCH v3 1/4] tests/qtest: Add a helper to query the QEMU version

[Fabiano Rosas]

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.h | 10 ++++++++++
 tests/qtest/libqtest.c | 24 ++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index 6e3d3525bf..5ec758242b 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -1085,4 +1085,14 @@ bool have_qemu_img(void);
  */
 bool mkimg(const char *file, const char *fmt, unsigned size_mb);
 
+/**
+ * qtest_query_version:
+ * @qts: QTestState instance to operate on
+ * @major: Pointer to where to store the major version number
+ * @minor: Pointer to where to store the minor version number
+ * @micro: Pointer to where to store the micro version number
+ *
+ */
+void qtest_query_version(QTestState *qts, int *major, int *minor, int *micro);
+
 #endif
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index f33a210861..af779a3248 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -337,6 +337,30 @@ void qtest_remove_abrt_handler(void *data)
     }
 }
 
+void qtest_query_version(QTestState *qts, int *major, int *minor, int *micro)
+{
+    QDict *rsp, *version;
+
+    rsp = qtest_qmp_assert_success_ref(qts, "{ 'execute': 'query-version' }");
+    g_assert(rsp);
+
+    version = qdict_get_qdict(rsp, "qemu");
+
+    if (major) {
+        *major = qdict_get_int(version, "major");
+    }
+
+    if (minor) {
+        *minor = qdict_get_int(version, "minor");
+    }
+
+    if (micro) {
+        *micro = qdict_get_int(version, "micro");
+    }
+
+    qobject_unref(rsp);
+}
+
 static const char *qtest_qemu_binary(const char *var)
 {
     const char *qemu_bin;
-- 
2.35.3

[PATCH v3 2/4] tests/qtest/migration: Add infrastructure to skip tests on older QEMUs

[Fabiano Rosas]

We can run the migration tests with two different QEMU binaries to
test migration compatibility between QEMU versions. This means we'll
be running the tests with an older QEMU in either source or
destination.

We need to avoid trying to test functionality that is unknown to the
older QEMU. This could mean new features, bug fixes, error message
changes, QEMU command line changes, migration API changes, etc.

Add a 'since' argument to the tests that inform when the functionality
that is being test has been added to QEMU so we can skip the test on
older versions.

Also add a version comparison function so we can adapt test code
depending on the QEMU binary version being used.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.h |  1 +
 tests/qtest/migration-helpers.c | 48 +++++++++++++++++++++++++++++++++
 tests/qtest/migration-test.c    | 29 ++++++++++++++++++++
 3 files changed, 78 insertions(+)

diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index e31dc85cc7..7b4f8e851e 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -47,4 +47,5 @@ char *find_common_machine_version(const char *mtype, const char *var1,
                                   const char *var2);
 char *resolve_machine_version(const char *alias, const char *var1,
                               const char *var2);
+int migration_vercmp(QTestState *who, const char *tgt_version);
 #endif /* MIGRATION_HELPERS_H */
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 24fb7b3525..bc4fd93e8c 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -292,3 +292,51 @@ char *resolve_machine_version(const char *alias, const char *var1,
 
     return find_common_machine_version(machine_name, var1, var2);
 }
+
+int migration_vercmp(QTestState *who, const char *tgt_version)
+{
+    g_autofree char *version = g_strdup(tgt_version);
+    int major = 0, minor = 0, micro = 0;
+    int tgt_major = 0, tgt_minor = 0, tgt_micro = 0;
+    const char *delim = ".";
+    char *tok;
+
+    qtest_query_version(who, &major, &minor, &micro);
+
+    tok = strtok(version, delim);
+    assert(tok);
+    tgt_major = atoi(tok);
+    assert(tgt_major);
+
+    if (major > tgt_major) {
+        return -1;
+    }
+    if (major < tgt_major) {
+        return 1;
+    }
+
+    tok = strtok(NULL, delim);
+    assert(tok);
+    tgt_minor = atoi(tok);
+
+    if (minor > tgt_minor) {
+        return -1;
+    }
+    if (minor < tgt_minor) {
+        return 1;
+    }
+
+    tok = strtok(NULL, delim);
+    if (tok) {
+        tgt_micro = atoi(tok);
+    }
+
+    if (micro > tgt_micro) {
+        return -1;
+    }
+    if (micro < tgt_micro) {
+        return 1;
+    }
+
+    return 0;
+}
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d520c587f7..001470238b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -637,6 +637,12 @@ typedef struct {
     bool use_dirty_ring;
     const char *opts_source;
     const char *opts_target;
+    /*
+     * If a test checks against new functionality that might not be
+     * present in older QEMUs this needs to be set so we can skip
+     * running it when doing compatibility testing.
+     */
+    const char *since;
 } MigrateStart;
 
 /*
@@ -850,6 +856,17 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         qtest_qmp_set_event_callback(*from,
                                      migrate_watch_for_stop,
                                      &got_src_stop);
+
+        if (args->since && migration_vercmp(*from, args->since) < 0) {
+            g_autofree char *msg = NULL;
+
+            msg = g_strdup_printf("Test requires at least QEMU version %s",
+                                  args->since);
+            g_test_skip(msg);
+            qtest_quit(*from);
+
+            return -1;
+        }
     }
 
     cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
@@ -872,6 +889,18 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  migrate_watch_for_resume,
                                  &got_dst_resume);
 
+    if (args->since && migration_vercmp(*to, args->since) < 0) {
+        g_autofree char *msg = NULL;
+
+        msg = g_strdup_printf("Test requires at least QEMU version %s",
+                              args->since);
+        g_test_skip(msg);
+        qtest_quit(*to);
+        qtest_quit(*from);
+
+        return -1;
+    }
+
     /*
      * Remove shmem file immediately to avoid memory leak in test failed case.
      * It's valid because QEMU has already opened this file
-- 
2.35.3

[PATCH v3 3/4] ci: Add a migration compatibility test job

[Fabiano Rosas]

The migration tests have support for being passed two QEMU binaries to
test migration compatibility.

Add a CI job that builds the lastest release of QEMU and another job
that uses that version plus an already present build of the current
version and run the migration tests with the two, both as source and
destination. I.e.:

 old QEMU (n-1) -> current QEMU (development tree)
 current QEMU (development tree) -> old QEMU (n-1)

The purpose of this CI job is to ensure the code we're about to merge
will not cause a migration compatibility problem when migrating the
next release (which will contain that code) to/from the previous
release.

I'm leaving the jobs as manual for now because using an older QEMU in
tests could hit bugs that were already fixed in the current
development tree and we need to handle those case-by-case.

Note: for user forks, the version tags need to be pushed to gitlab
otherwise it won't be able to checkout a different version.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 .gitlab-ci.d/buildtest.yml | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index 91663946de..81163a3f6a 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -167,6 +167,59 @@ build-system-centos:
       x86_64-softmmu rx-softmmu sh4-softmmu nios2-softmmu
     MAKE_CHECK_ARGS: check-build
 
+build-previous-qemu:
+  extends: .native_build_job_template
+  artifacts:
+    when: on_success
+    expire_in: 2 days
+    paths:
+      - build-previous
+    exclude:
+      - build-previous/**/*.p
+      - build-previous/**/*.a.p
+      - build-previous/**/*.fa.p
+      - build-previous/**/*.c.o
+      - build-previous/**/*.c.o.d
+      - build-previous/**/*.fa
+  needs:
+    job: amd64-opensuse-leap-container
+  variables:
+    QEMU_JOB_OPTIONAL: 1
+    IMAGE: opensuse-leap
+    TARGETS: x86_64-softmmu aarch64-softmmu
+  before_script:
+    - export QEMU_PREV_VERSION="$(sed 's/\([0-9.]*\)\.[0-9]*/v\1.0/' VERSION)"
+    - git checkout $QEMU_PREV_VERSION
+  after_script:
+    - mv build build-previous
+
+.migration-compat-common:
+  extends: .common_test_job_template
+  needs:
+    - job: build-previous-qemu
+    - job: build-system-opensuse
+  allow_failure: true
+  variables:
+    QEMU_JOB_OPTIONAL: 1
+    IMAGE: opensuse-leap
+    MAKE_CHECK_ARGS: check-build
+  script:
+    - cd build
+    - QTEST_QEMU_BINARY_SRC=../build-previous/qemu-system-${TARGET}
+          QTEST_QEMU_BINARY=./qemu-system-${TARGET} ./tests/qtest/migration-test
+    - QTEST_QEMU_BINARY_DST=../build-previous/qemu-system-${TARGET}
+          QTEST_QEMU_BINARY=./qemu-system-${TARGET} ./tests/qtest/migration-test
+
+migration-compat-aarch64:
+  extends: .migration-compat-common
+  variables:
+    TARGET: aarch64
+
+migration-compat-x86_64:
+  extends: .migration-compat-common
+  variables:
+    TARGET: x86_64
+
 check-system-centos:
   extends: .native_test_job_template
   needs:
-- 
2.35.3

[PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Fabiano Rosas]

[This patch is not necessary anymore after 8.2 has been released]

Add the 'since' annotations to recently added tests and adapt the
postcopy test to use the older "uri" API when needed.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 001470238b..599f51f978 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1338,14 +1338,21 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     migrate_ensure_non_converge(from);
 
     migrate_prepare_for_dirty_mem(from);
-    qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
-                             "  'arguments': { "
-                             "      'channels': [ { 'channel-type': 'main',"
-                             "      'addr': { 'transport': 'socket',"
-                             "                'type': 'inet',"
-                             "                'host': '127.0.0.1',"
-                             "                'port': '0' } } ] } }");
 
+    /* New syntax was introduced in 8.2 */
+    if (migration_vercmp(to, "8.2") < 0) {
+        qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
+                                 "  'arguments': { "
+                                 "      'uri': 'tcp:127.0.0.1:0' } }");
+    } else {
+        qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
+                                 "  'arguments': { "
+                                 "      'channels': [ { 'channel-type': 'main',"
+                                 "      'addr': { 'transport': 'socket',"
+                                 "                'type': 'inet',"
+                                 "                'host': '127.0.0.1',"
+                                 "                'port': '0' } } ] } }");
+    }
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
@@ -1603,6 +1610,9 @@ static void test_postcopy_recovery_double_fail(void)
 {
     MigrateCommon args = {
         .postcopy_recovery_test_fail = true,
+        .start = {
+            .since = "8.2",
+        },
     };
 
     test_postcopy_recovery_common(&args);
@@ -1665,6 +1675,7 @@ static void test_analyze_script(void)
 {
     MigrateStart args = {
         .opts_source = "-uuid 11111111-1111-1111-1111-111111111111",
+        .since = "8.2",
     };
     QTestState *from, *to;
     g_autofree char *uri = NULL;
@@ -2090,6 +2101,9 @@ static void test_precopy_file(void)
     MigrateCommon args = {
         .connect_uri = uri,
         .listen_uri = "defer",
+        .start = {
+            .since = "8.2"
+        },
     };
 
     test_file_common(&args, true);
@@ -2134,6 +2148,9 @@ static void test_precopy_file_offset(void)
         .connect_uri = uri,
         .listen_uri = "defer",
         .finish_hook = file_offset_finish_hook,
+        .start = {
+            .since = "8.2"
+        },
     };
 
     test_file_common(&args, false);
@@ -2148,6 +2165,9 @@ static void test_precopy_file_offset_bad(void)
         .connect_uri = uri,
         .listen_uri = "defer",
         .result = MIG_TEST_QMP_ERROR,
+        .start = {
+            .since = "8.2"
+        },
     };
 
     test_file_common(&args, false);
-- 
2.35.3

Re: [PATCH v3 2/4] tests/qtest/migration: Add infrastructure to skip tests on older QEMUs

[Peter Xu]

On Fri, Jan 05, 2024 at 03:04:47PM -0300, Fabiano Rosas wrote:
> We can run the migration tests with two different QEMU binaries to
> test migration compatibility between QEMU versions. This means we'll
> be running the tests with an older QEMU in either source or
> destination.
> 
> We need to avoid trying to test functionality that is unknown to the
> older QEMU. This could mean new features, bug fixes, error message
> changes, QEMU command line changes, migration API changes, etc.
> 
> Add a 'since' argument to the tests that inform when the functionality
> that is being test has been added to QEMU so we can skip the test on
> older versions.
> 
> Also add a version comparison function so we can adapt test code
> depending on the QEMU binary version being used.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-helpers.h |  1 +
>  tests/qtest/migration-helpers.c | 48 +++++++++++++++++++++++++++++++++
>  tests/qtest/migration-test.c    | 29 ++++++++++++++++++++
>  3 files changed, 78 insertions(+)
> 
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index e31dc85cc7..7b4f8e851e 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -47,4 +47,5 @@ char *find_common_machine_version(const char *mtype, const char *var1,
>                                    const char *var2);
>  char *resolve_machine_version(const char *alias, const char *var1,
>                                const char *var2);
> +int migration_vercmp(QTestState *who, const char *tgt_version);
>  #endif /* MIGRATION_HELPERS_H */
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 24fb7b3525..bc4fd93e8c 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -292,3 +292,51 @@ char *resolve_machine_version(const char *alias, const char *var1,
>  
>      return find_common_machine_version(machine_name, var1, var2);
>  }
> +
> +int migration_vercmp(QTestState *who, const char *tgt_version)
> +{
> +    g_autofree char *version = g_strdup(tgt_version);
> +    int major = 0, minor = 0, micro = 0;
> +    int tgt_major = 0, tgt_minor = 0, tgt_micro = 0;
> +    const char *delim = ".";
> +    char *tok;
> +
> +    qtest_query_version(who, &major, &minor, &micro);
> +
> +    tok = strtok(version, delim);
> +    assert(tok);
> +    tgt_major = atoi(tok);
> +    assert(tgt_major);

I'd consider dropping this.  But I don't think "since: 0.8" is valid. :)
More like a nitpick.

> +
> +    if (major > tgt_major) {
> +        return -1;

This means the QEMU version is newer, the function will return negative.
Is this what we want?  It seems it's inverted.

In all cases, document this function with retval would be helpful too.

> +    }
> +    if (major < tgt_major) {
> +        return 1;
> +    }

Instead of all these, I'm wondering whether we can allow "since" to be an
array of integers, like [8, 2, 0].  Would that be much easier?

> +
> +    tok = strtok(NULL, delim);
> +    assert(tok);
> +    tgt_minor = atoi(tok);
> +
> +    if (minor > tgt_minor) {
> +        return -1;
> +    }
> +    if (minor < tgt_minor) {
> +        return 1;
> +    }
> +
> +    tok = strtok(NULL, delim);
> +    if (tok) {
> +        tgt_micro = atoi(tok);
> +    }
> +
> +    if (micro > tgt_micro) {
> +        return -1;
> +    }
> +    if (micro < tgt_micro) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index d520c587f7..001470238b 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -637,6 +637,12 @@ typedef struct {
>      bool use_dirty_ring;
>      const char *opts_source;
>      const char *opts_target;
> +    /*
> +     * If a test checks against new functionality that might not be
> +     * present in older QEMUs this needs to be set so we can skip
> +     * running it when doing compatibility testing.
> +     */
> +    const char *since;
>  } MigrateStart;
>  
>  /*
> @@ -850,6 +856,17 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>          qtest_qmp_set_event_callback(*from,
>                                       migrate_watch_for_stop,
>                                       &got_src_stop);
> +
> +        if (args->since && migration_vercmp(*from, args->since) < 0) {
> +            g_autofree char *msg = NULL;
> +
> +            msg = g_strdup_printf("Test requires at least QEMU version %s",
> +                                  args->since);
> +            g_test_skip(msg);
> +            qtest_quit(*from);
> +
> +            return -1;
> +        }
>      }
>  
>      cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
> @@ -872,6 +889,18 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>                                   migrate_watch_for_resume,
>                                   &got_dst_resume);
>  
> +    if (args->since && migration_vercmp(*to, args->since) < 0) {
> +        g_autofree char *msg = NULL;
> +
> +        msg = g_strdup_printf("Test requires at least QEMU version %s",
> +                              args->since);
> +        g_test_skip(msg);
> +        qtest_quit(*to);
> +        qtest_quit(*from);
> +
> +        return -1;

Nitpick: you can do both check here, then avoid duplicating some code, and
always free from+to.

> +    }
> +
>      /*
>       * Remove shmem file immediately to avoid memory leak in test failed case.
>       * It's valid because QEMU has already opened this file
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH v3 1/4] tests/qtest: Add a helper to query the QEMU version

[Peter Xu]

On Fri, Jan 05, 2024 at 03:04:46PM -0300, Fabiano Rosas wrote:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Peter Xu

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Peter Xu]

On Fri, Jan 05, 2024 at 03:04:49PM -0300, Fabiano Rosas wrote:
> [This patch is not necessary anymore after 8.2 has been released]
> 
> Add the 'since' annotations to recently added tests and adapt the
> postcopy test to use the older "uri" API when needed.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

You marked this as not-for-merge.  Would something like this still be
useful in the future?  IIUC it's a matter of whether we'd still want to
test those old binaries.

> ---
>  tests/qtest/migration-test.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 001470238b..599f51f978 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1338,14 +1338,21 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>      migrate_ensure_non_converge(from);
>  
>      migrate_prepare_for_dirty_mem(from);
> -    qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
> -                             "  'arguments': { "
> -                             "      'channels': [ { 'channel-type': 'main',"
> -                             "      'addr': { 'transport': 'socket',"
> -                             "                'type': 'inet',"
> -                             "                'host': '127.0.0.1',"
> -                             "                'port': '0' } } ] } }");
>  
> +    /* New syntax was introduced in 8.2 */
> +    if (migration_vercmp(to, "8.2") < 0) {
> +        qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
> +                                 "  'arguments': { "
> +                                 "      'uri': 'tcp:127.0.0.1:0' } }");
> +    } else {
> +        qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
> +                                 "  'arguments': { "
> +                                 "      'channels': [ { 'channel-type': 'main',"
> +                                 "      'addr': { 'transport': 'socket',"
> +                                 "                'type': 'inet',"
> +                                 "                'host': '127.0.0.1',"
> +                                 "                'port': '0' } } ] } }");
> +    }
>      /* Wait for the first serial output from the source */
>      wait_for_serial("src_serial");
>  
> @@ -1603,6 +1610,9 @@ static void test_postcopy_recovery_double_fail(void)
>  {
>      MigrateCommon args = {
>          .postcopy_recovery_test_fail = true,
> +        .start = {
> +            .since = "8.2",
> +        },
>      };
>  
>      test_postcopy_recovery_common(&args);
> @@ -1665,6 +1675,7 @@ static void test_analyze_script(void)
>  {
>      MigrateStart args = {
>          .opts_source = "-uuid 11111111-1111-1111-1111-111111111111",
> +        .since = "8.2",
>      };
>      QTestState *from, *to;
>      g_autofree char *uri = NULL;
> @@ -2090,6 +2101,9 @@ static void test_precopy_file(void)
>      MigrateCommon args = {
>          .connect_uri = uri,
>          .listen_uri = "defer",
> +        .start = {
> +            .since = "8.2"
> +        },
>      };
>  
>      test_file_common(&args, true);
> @@ -2134,6 +2148,9 @@ static void test_precopy_file_offset(void)
>          .connect_uri = uri,
>          .listen_uri = "defer",
>          .finish_hook = file_offset_finish_hook,
> +        .start = {
> +            .since = "8.2"
> +        },
>      };
>  
>      test_file_common(&args, false);
> @@ -2148,6 +2165,9 @@ static void test_precopy_file_offset_bad(void)
>          .connect_uri = uri,
>          .listen_uri = "defer",
>          .result = MIG_TEST_QMP_ERROR,
> +        .start = {
> +            .since = "8.2"
> +        },
>      };
>  
>      test_file_common(&args, false);
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH v3 2/4] tests/qtest/migration: Add infrastructure to skip tests on older QEMUs

[Peter Xu]

On Mon, Jan 08, 2024 at 04:13:10PM +0800, Peter Xu wrote:
> On Fri, Jan 05, 2024 at 03:04:47PM -0300, Fabiano Rosas wrote:
> > We can run the migration tests with two different QEMU binaries to
> > test migration compatibility between QEMU versions. This means we'll
> > be running the tests with an older QEMU in either source or
> > destination.
> > 
> > We need to avoid trying to test functionality that is unknown to the
> > older QEMU. This could mean new features, bug fixes, error message
> > changes, QEMU command line changes, migration API changes, etc.
> > 
> > Add a 'since' argument to the tests that inform when the functionality
> > that is being test has been added to QEMU so we can skip the test on
> > older versions.
> > 
> > Also add a version comparison function so we can adapt test code
> > depending on the QEMU binary version being used.
> > 
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> >  tests/qtest/migration-helpers.h |  1 +
> >  tests/qtest/migration-helpers.c | 48 +++++++++++++++++++++++++++++++++
> >  tests/qtest/migration-test.c    | 29 ++++++++++++++++++++
> >  3 files changed, 78 insertions(+)
> > 
> > diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> > index e31dc85cc7..7b4f8e851e 100644
> > --- a/tests/qtest/migration-helpers.h
> > +++ b/tests/qtest/migration-helpers.h
> > @@ -47,4 +47,5 @@ char *find_common_machine_version(const char *mtype, const char *var1,
> >                                    const char *var2);
> >  char *resolve_machine_version(const char *alias, const char *var1,
> >                                const char *var2);
> > +int migration_vercmp(QTestState *who, const char *tgt_version);
> >  #endif /* MIGRATION_HELPERS_H */
> > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> > index 24fb7b3525..bc4fd93e8c 100644
> > --- a/tests/qtest/migration-helpers.c
> > +++ b/tests/qtest/migration-helpers.c
> > @@ -292,3 +292,51 @@ char *resolve_machine_version(const char *alias, const char *var1,
> >  
> >      return find_common_machine_version(machine_name, var1, var2);
> >  }
> > +
> > +int migration_vercmp(QTestState *who, const char *tgt_version)
> > +{
> > +    g_autofree char *version = g_strdup(tgt_version);
> > +    int major = 0, minor = 0, micro = 0;
> > +    int tgt_major = 0, tgt_minor = 0, tgt_micro = 0;
> > +    const char *delim = ".";
> > +    char *tok;
> > +
> > +    qtest_query_version(who, &major, &minor, &micro);
> > +
> > +    tok = strtok(version, delim);
> > +    assert(tok);
> > +    tgt_major = atoi(tok);
> > +    assert(tgt_major);
> 
> I'd consider dropping this.  But I don't think "since: 0.8" is valid. :)
> More like a nitpick.
> 
> > +
> > +    if (major > tgt_major) {
> > +        return -1;
> 
> This means the QEMU version is newer, the function will return negative.
> Is this what we want?  It seems it's inverted.
> 
> In all cases, document this function with retval would be helpful too.
> 
> > +    }
> > +    if (major < tgt_major) {
> > +        return 1;
> > +    }
> 
> Instead of all these, I'm wondering whether we can allow "since" to be an
> array of integers, like [8, 2, 0].  Would that be much easier?

I meant something like:

struct Version { int major, minor, micro; };

#define QEMU_VER(major, minor, micro) \
  ((struct Verion){.major = major, .minor = minor, .micro = micro})

Then:

   ...
   struct Version since;
   ...

   ...
   .since = QEMU_VER(8, 2, 0),
   ...

> 
> > +
> > +    tok = strtok(NULL, delim);
> > +    assert(tok);
> > +    tgt_minor = atoi(tok);
> > +
> > +    if (minor > tgt_minor) {
> > +        return -1;
> > +    }
> > +    if (minor < tgt_minor) {
> > +        return 1;
> > +    }
> > +
> > +    tok = strtok(NULL, delim);
> > +    if (tok) {
> > +        tgt_micro = atoi(tok);
> > +    }
> > +
> > +    if (micro > tgt_micro) {
> > +        return -1;
> > +    }
> > +    if (micro < tgt_micro) {
> > +        return 1;
> > +    }
> > +
> > +    return 0;
> > +}
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index d520c587f7..001470238b 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -637,6 +637,12 @@ typedef struct {
> >      bool use_dirty_ring;
> >      const char *opts_source;
> >      const char *opts_target;
> > +    /*
> > +     * If a test checks against new functionality that might not be
> > +     * present in older QEMUs this needs to be set so we can skip
> > +     * running it when doing compatibility testing.
> > +     */
> > +    const char *since;
> >  } MigrateStart;
> >  
> >  /*
> > @@ -850,6 +856,17 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >          qtest_qmp_set_event_callback(*from,
> >                                       migrate_watch_for_stop,
> >                                       &got_src_stop);
> > +
> > +        if (args->since && migration_vercmp(*from, args->since) < 0) {
> > +            g_autofree char *msg = NULL;
> > +
> > +            msg = g_strdup_printf("Test requires at least QEMU version %s",
> > +                                  args->since);
> > +            g_test_skip(msg);
> > +            qtest_quit(*from);
> > +
> > +            return -1;
> > +        }
> >      }
> >  
> >      cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
> > @@ -872,6 +889,18 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >                                   migrate_watch_for_resume,
> >                                   &got_dst_resume);
> >  
> > +    if (args->since && migration_vercmp(*to, args->since) < 0) {
> > +        g_autofree char *msg = NULL;
> > +
> > +        msg = g_strdup_printf("Test requires at least QEMU version %s",
> > +                              args->since);
> > +        g_test_skip(msg);
> > +        qtest_quit(*to);
> > +        qtest_quit(*from);
> > +
> > +        return -1;
> 
> Nitpick: you can do both check here, then avoid duplicating some code, and
> always free from+to.
> 
> > +    }
> > +
> >      /*
> >       * Remove shmem file immediately to avoid memory leak in test failed case.
> >       * It's valid because QEMU has already opened this file
> > -- 
> > 2.35.3
> > 
> 
> -- 
> Peter Xu

-- 
Peter Xu

Re: [PATCH v3 2/4] tests/qtest/migration: Add infrastructure to skip tests on older QEMUs

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jan 05, 2024 at 03:04:47PM -0300, Fabiano Rosas wrote:
>> We can run the migration tests with two different QEMU binaries to
>> test migration compatibility between QEMU versions. This means we'll
>> be running the tests with an older QEMU in either source or
>> destination.
>> 
>> We need to avoid trying to test functionality that is unknown to the
>> older QEMU. This could mean new features, bug fixes, error message
>> changes, QEMU command line changes, migration API changes, etc.
>> 
>> Add a 'since' argument to the tests that inform when the functionality
>> that is being test has been added to QEMU so we can skip the test on
>> older versions.
>> 
>> Also add a version comparison function so we can adapt test code
>> depending on the QEMU binary version being used.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/qtest/migration-helpers.h |  1 +
>>  tests/qtest/migration-helpers.c | 48 +++++++++++++++++++++++++++++++++
>>  tests/qtest/migration-test.c    | 29 ++++++++++++++++++++
>>  3 files changed, 78 insertions(+)
>> 
>> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
>> index e31dc85cc7..7b4f8e851e 100644
>> --- a/tests/qtest/migration-helpers.h
>> +++ b/tests/qtest/migration-helpers.h
>> @@ -47,4 +47,5 @@ char *find_common_machine_version(const char *mtype, const char *var1,
>>                                    const char *var2);
>>  char *resolve_machine_version(const char *alias, const char *var1,
>>                                const char *var2);
>> +int migration_vercmp(QTestState *who, const char *tgt_version);
>>  #endif /* MIGRATION_HELPERS_H */
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index 24fb7b3525..bc4fd93e8c 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -292,3 +292,51 @@ char *resolve_machine_version(const char *alias, const char *var1,
>>  
>>      return find_common_machine_version(machine_name, var1, var2);
>>  }
>> +
>> +int migration_vercmp(QTestState *who, const char *tgt_version)
>> +{
>> +    g_autofree char *version = g_strdup(tgt_version);
>> +    int major = 0, minor = 0, micro = 0;
>> +    int tgt_major = 0, tgt_minor = 0, tgt_micro = 0;
>> +    const char *delim = ".";
>> +    char *tok;
>> +
>> +    qtest_query_version(who, &major, &minor, &micro);
>> +
>> +    tok = strtok(version, delim);
>> +    assert(tok);
>> +    tgt_major = atoi(tok);
>> +    assert(tgt_major);
>
> I'd consider dropping this.  But I don't think "since: 0.8" is valid. :)
> More like a nitpick.
>
>> +
>> +    if (major > tgt_major) {
>> +        return -1;
>
> This means the QEMU version is newer, the function will return negative.
> Is this what we want?  It seems it's inverted.

The return "points" to which once is the more recent:

QEMU version | since: version
-1           0         1

> In all cases, document this function with retval would be helpful too.
>

Ok.

>> +    }
>> +    if (major < tgt_major) {
>> +        return 1;
>> +    }
>
> Instead of all these, I'm wondering whether we can allow "since" to be an
> array of integers, like [8, 2, 0].  Would that be much easier?

I don't see why push the complexity towards the person writing the
tests. The string is much more natural to specify.

>> +
>> +    tok = strtok(NULL, delim);
>> +    assert(tok);
>> +    tgt_minor = atoi(tok);
>> +
>> +    if (minor > tgt_minor) {
>> +        return -1;
>> +    }
>> +    if (minor < tgt_minor) {
>> +        return 1;
>> +    }
>> +
>> +    tok = strtok(NULL, delim);
>> +    if (tok) {
>> +        tgt_micro = atoi(tok);
>> +    }
>> +
>> +    if (micro > tgt_micro) {
>> +        return -1;
>> +    }
>> +    if (micro < tgt_micro) {
>> +        return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index d520c587f7..001470238b 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -637,6 +637,12 @@ typedef struct {
>>      bool use_dirty_ring;
>>      const char *opts_source;
>>      const char *opts_target;
>> +    /*
>> +     * If a test checks against new functionality that might not be
>> +     * present in older QEMUs this needs to be set so we can skip
>> +     * running it when doing compatibility testing.
>> +     */
>> +    const char *since;
>>  } MigrateStart;
>>  
>>  /*
>> @@ -850,6 +856,17 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>          qtest_qmp_set_event_callback(*from,
>>                                       migrate_watch_for_stop,
>>                                       &got_src_stop);
>> +
>> +        if (args->since && migration_vercmp(*from, args->since) < 0) {
>> +            g_autofree char *msg = NULL;
>> +
>> +            msg = g_strdup_printf("Test requires at least QEMU version %s",
>> +                                  args->since);
>> +            g_test_skip(msg);
>> +            qtest_quit(*from);
>> +
>> +            return -1;
>> +        }
>>      }
>>  
>>      cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
>> @@ -872,6 +889,18 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>                                   migrate_watch_for_resume,
>>                                   &got_dst_resume);
>>  
>> +    if (args->since && migration_vercmp(*to, args->since) < 0) {
>> +        g_autofree char *msg = NULL;
>> +
>> +        msg = g_strdup_printf("Test requires at least QEMU version %s",
>> +                              args->since);
>> +        g_test_skip(msg);
>> +        qtest_quit(*to);
>> +        qtest_quit(*from);
>> +
>> +        return -1;
>
> Nitpick: you can do both check here, then avoid duplicating some code, and
> always free from+to.
>

Hm.. I think I had a good reason for doing it this way. I don't remember
now. I'll double check and apply your suggestion if it works.

>> +    }
>> +
>>      /*
>>       * Remove shmem file immediately to avoid memory leak in test failed case.
>>       * It's valid because QEMU has already opened this file
>> -- 
>> 2.35.3
>> 

Re: [PATCH v3 2/4] tests/qtest/migration: Add infrastructure to skip tests on older QEMUs

[Daniel P. Berrangé]

On Fri, Jan 05, 2024 at 03:04:47PM -0300, Fabiano Rosas wrote:
> We can run the migration tests with two different QEMU binaries to
> test migration compatibility between QEMU versions. This means we'll
> be running the tests with an older QEMU in either source or
> destination.
> 
> We need to avoid trying to test functionality that is unknown to the
> older QEMU. This could mean new features, bug fixes, error message
> changes, QEMU command line changes, migration API changes, etc.
> 
> Add a 'since' argument to the tests that inform when the functionality
> that is being test has been added to QEMU so we can skip the test on
> older versions.
> 
> Also add a version comparison function so we can adapt test code
> depending on the QEMU binary version being used.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-helpers.h |  1 +
>  tests/qtest/migration-helpers.c | 48 +++++++++++++++++++++++++++++++++
>  tests/qtest/migration-test.c    | 29 ++++++++++++++++++++
>  3 files changed, 78 insertions(+)
> 
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index e31dc85cc7..7b4f8e851e 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -47,4 +47,5 @@ char *find_common_machine_version(const char *mtype, const char *var1,
>                                    const char *var2);
>  char *resolve_machine_version(const char *alias, const char *var1,
>                                const char *var2);
> +int migration_vercmp(QTestState *who, const char *tgt_version);
>  #endif /* MIGRATION_HELPERS_H */
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 24fb7b3525..bc4fd93e8c 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -292,3 +292,51 @@ char *resolve_machine_version(const char *alias, const char *var1,
>  
>      return find_common_machine_version(machine_name, var1, var2);
>  }

If we provide a macro to encode the version to a single integer:

  #define VERSION_NUMBER(maj, min, mic) ((maj * 1000000) + (min * 1000)  + mic)

> +
> +int migration_vercmp(QTestState *who, const char *tgt_version)

This could be an "int tgt_version"

> +{
> +    g_autofree char *version = g_strdup(tgt_version);
> +    int major = 0, minor = 0, micro = 0;
> +    int tgt_major = 0, tgt_minor = 0, tgt_micro = 0;
> +    const char *delim = ".";
> +    char *tok;
> +
> +    qtest_query_version(who, &major, &minor, &micro);


This could be nothing more than

  return  (tgt_version >= VERSION_NUMBER(major, minor, micro));


> +
> +    tok = strtok(version, delim);
> +    assert(tok);
> +    tgt_major = atoi(tok);
> +    assert(tgt_major);
> +
> +    if (major > tgt_major) {
> +        return -1;
> +    }
> +    if (major < tgt_major) {
> +        return 1;
> +    }
> +
> +    tok = strtok(NULL, delim);
> +    assert(tok);
> +    tgt_minor = atoi(tok);
> +
> +    if (minor > tgt_minor) {
> +        return -1;
> +    }
> +    if (minor < tgt_minor) {
> +        return 1;
> +    }
> +
> +    tok = strtok(NULL, delim);
> +    if (tok) {
> +        tgt_micro = atoi(tok);
> +    }
> +
> +    if (micro > tgt_micro) {
> +        return -1;
> +    }
> +    if (micro < tgt_micro) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index d520c587f7..001470238b 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -637,6 +637,12 @@ typedef struct {
>      bool use_dirty_ring;
>      const char *opts_source;
>      const char *opts_target;
> +    /*
> +     * If a test checks against new functionality that might not be
> +     * present in older QEMUs this needs to be set so we can skip
> +     * running it when doing compatibility testing.
> +     */
> +    const char *since;
>  } MigrateStart;

This could be an 'int' too.

>  
>  /*
> @@ -850,6 +856,17 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>          qtest_qmp_set_event_callback(*from,
>                                       migrate_watch_for_stop,
>                                       &got_src_stop);
> +
> +        if (args->since && migration_vercmp(*from, args->since) < 0) {
> +            g_autofree char *msg = NULL;
> +
> +            msg = g_strdup_printf("Test requires at least QEMU version %s",
> +                                  args->since);
> +            g_test_skip(msg);
> +            qtest_quit(*from);

Hmm, the g_test_skip stuff is repeated in several places, which we could
avoid if we put it into a helper in libqtest.h

    "qtest_require_version(QTestState *test, int version)"

> +
> +            return -1;
> +        }
>      }
>  
>      cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
> @@ -872,6 +889,18 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>                                   migrate_watch_for_resume,
>                                   &got_dst_resume);
>  
> +    if (args->since && migration_vercmp(*to, args->since) < 0) {
> +        g_autofree char *msg = NULL;
> +
> +        msg = g_strdup_printf("Test requires at least QEMU version %s",
> +                              args->since);
> +        g_test_skip(msg);
> +        qtest_quit(*to);
> +        qtest_quit(*from);
> +
> +        return -1;
> +    }
> +
>      /*
>       * Remove shmem file immediately to avoid memory leak in test failed case.
>       * It's valid because QEMU has already opened this file
> -- 
> 2.35.3
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jan 05, 2024 at 03:04:49PM -0300, Fabiano Rosas wrote:
>> [This patch is not necessary anymore after 8.2 has been released]
>> 
>> Add the 'since' annotations to recently added tests and adapt the
>> postcopy test to use the older "uri" API when needed.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> You marked this as not-for-merge.  Would something like this still be
> useful in the future?  IIUC it's a matter of whether we'd still want to
> test those old binaries.
>

Technically yes, but I fail to see what benefit testing old binaries
would bring us. I'm thinking maybe it could be useful for bisecting
compatibility issues, but I can't think of a scenario where we'd like to
change the older QEMU instead of the newer.

I'm of course open to suggestions if you or anyone else has an use case
that you'd like to keep viable.

So far, my idea is that once a new QEMU is released, all the "since:"
annotations become obsolete. We could even remove them. This series is
just infrastructure to make our life easier if a change is ever
introduced that is incompatible with the n-1 QEMU. IMO we cannot have
compatibility testing if a random change might break a test and make it
more difficult to run the remaining tests. So we'd use 'since' or the
vercmp function to skip/adapt the offending tests until the next QEMU is
released.

I'm basing myself on this loosely worded support statement from our
docs:

  "In general QEMU tries to maintain forward migration compatibility
  (i.e. migrating from QEMU n->n+1) and there are users who benefit from
  backward compatibility as well."

Re: [PATCH v3 2/4] tests/qtest/migration: Add infrastructure to skip tests on older QEMUs

[Peter Xu]

On Mon, Jan 08, 2024 at 11:49:45AM -0300, Fabiano Rosas wrote:
> >> +
> >> +    if (major > tgt_major) {
> >> +        return -1;
> >
> > This means the QEMU version is newer, the function will return negative.
> > Is this what we want?  It seems it's inverted.
> 
> The return "points" to which once is the more recent:
> 
> QEMU version | since: version
> -1           0         1

Here if returns -1, then below [1] will skip the test?

> 
> > In all cases, document this function with retval would be helpful too.
> >
> 
> Ok.
> 
> >> +    }
> >> +    if (major < tgt_major) {
> >> +        return 1;
> >> +    }
> >
> > Instead of all these, I'm wondering whether we can allow "since" to be an
> > array of integers, like [8, 2, 0].  Would that be much easier?
> 
> I don't see why push the complexity towards the person writing the
> tests. The string is much more natural to specify.

To me QEMU_VER(8,2,0) is as easy to write and read, too.  What Dan proposed
looks also good in the other thread.

I don't really have a strong opinion here especially for the test case. But
imho it'll be still nice to avoid string <-> int if the string is not required.

[...]

> >> @@ -850,6 +856,17 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >>          qtest_qmp_set_event_callback(*from,
> >>                                       migrate_watch_for_stop,
> >>                                       &got_src_stop);
> >> +
> >> +        if (args->since && migration_vercmp(*from, args->since) < 0) {

[1]

> >> +            g_autofree char *msg = NULL;
> >> +
> >> +            msg = g_strdup_printf("Test requires at least QEMU version %s",
> >> +                                  args->since);
> >> +            g_test_skip(msg);
> >> +            qtest_quit(*from);
> >> +
> >> +            return -1;
> >> +        }

-- 
Peter Xu

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Peter Xu]

On Mon, Jan 08, 2024 at 12:37:46PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Fri, Jan 05, 2024 at 03:04:49PM -0300, Fabiano Rosas wrote:
> >> [This patch is not necessary anymore after 8.2 has been released]
> >> 
> >> Add the 'since' annotations to recently added tests and adapt the
> >> postcopy test to use the older "uri" API when needed.
> >> 
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >
> > You marked this as not-for-merge.  Would something like this still be
> > useful in the future?  IIUC it's a matter of whether we'd still want to
> > test those old binaries.
> >
> 
> Technically yes, but I fail to see what benefit testing old binaries
> would bring us. I'm thinking maybe it could be useful for bisecting
> compatibility issues, but I can't think of a scenario where we'd like to
> change the older QEMU instead of the newer.
> 
> I'm of course open to suggestions if you or anyone else has an use case
> that you'd like to keep viable.
> 
> So far, my idea is that once a new QEMU is released, all the "since:"
> annotations become obsolete. We could even remove them. This series is
> just infrastructure to make our life easier if a change is ever
> introduced that is incompatible with the n-1 QEMU. IMO we cannot have
> compatibility testing if a random change might break a test and make it
> more difficult to run the remaining tests. So we'd use 'since' or the
> vercmp function to skip/adapt the offending tests until the next QEMU is
> released.
> 
> I'm basing myself on this loosely worded support statement from our
> docs:
> 
>   "In general QEMU tries to maintain forward migration compatibility
>   (i.e. migrating from QEMU n->n+1) and there are users who benefit from
>   backward compatibility as well."

I think we could still have users migrating from e.g. 8.0 -> 9.0 as long as
with the same machine type, especially when upgrading upper level stack
(e.g. an openstack cluster upgrade), where IIUC can jump a few qemu major
versions.  That does sound like a common use case, and I suspect the doc
was only taking one example on why compatibility needs to be maintained,
rather than emphasizing "+1 only".

However then the question is whether those old binaries needs to be
convered.

Then I noticed that taking all these "since: XXX" and cmdline changes along
with migration-test may be yet another burden even if we want to cover old
binaries for whatever reason.  I am now more convinced myself that we
should try to get rid of as much burden as we can for migration, because we
already have enough, and it's not ideal to keep growing that unnecessarily.

One good thing with CI in this case (I still don't have enough knowledge on
CI, so I am hoping some CI people can review that patch, though) is that if
we can always guarantee n-1 -> n works for the test cases we enabled, it
most probably means when n boosts again to n+1, we keep making sure n ->
n+1 works perfectly, then n-1 -> n+1 should not fail either, considering
that we're testing the stream protocol matching each other.  There might be
outliers (especially if not described with VMSDs) but should be corner
cases.

So I tend to agree with you on that we drop this patch, keep it simple
until we're much more clear what we can get from that.

But then if so - do we need "since" at all to be expressed in versions?

Basically we keep qtest always be valid only on the latest qemu binary as
before (which actually works the same as Linux v.s. kselftests, which makes
sense), there's one exception now with "n-1" due to the CI we plan to add.
Dropping this patch means we don't yet plan to support n-2.  Then maybe
instead of a "since" we only need a boolean showing "whether one test needs
to be covered by a cross-binary test"?  Then we set it in incompatible
binaries (skip all cross-binary tests directly, rather than relying on any
qemu versions, no compare needed), and can also drop that when a new
release starts.

Thanks,

-- 
Peter Xu

Re: [PATCH v3 3/4] ci: Add a migration compatibility test job

[Peter Xu]

On Fri, Jan 05, 2024 at 03:04:48PM -0300, Fabiano Rosas wrote:
> The migration tests have support for being passed two QEMU binaries to
> test migration compatibility.
> 
> Add a CI job that builds the lastest release of QEMU and another job
> that uses that version plus an already present build of the current
> version and run the migration tests with the two, both as source and
> destination. I.e.:
> 
>  old QEMU (n-1) -> current QEMU (development tree)
>  current QEMU (development tree) -> old QEMU (n-1)
> 
> The purpose of this CI job is to ensure the code we're about to merge
> will not cause a migration compatibility problem when migrating the
> next release (which will contain that code) to/from the previous
> release.
> 
> I'm leaving the jobs as manual for now because using an older QEMU in
> tests could hit bugs that were already fixed in the current
> development tree and we need to handle those case-by-case.

Can we opt-out those broken tests using either your "since:" thing or
anything similar?

I hope we can start to run something by default in the CI in 9.0 to cover
n-1 -> n, even if starting with a subset of tests.  Is it possible?

Thanks,

-- 
Peter Xu

Re: [PATCH v3 3/4] ci: Add a migration compatibility test job

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jan 05, 2024 at 03:04:48PM -0300, Fabiano Rosas wrote:
>> The migration tests have support for being passed two QEMU binaries to
>> test migration compatibility.
>> 
>> Add a CI job that builds the lastest release of QEMU and another job
>> that uses that version plus an already present build of the current
>> version and run the migration tests with the two, both as source and
>> destination. I.e.:
>> 
>>  old QEMU (n-1) -> current QEMU (development tree)
>>  current QEMU (development tree) -> old QEMU (n-1)
>> 
>> The purpose of this CI job is to ensure the code we're about to merge
>> will not cause a migration compatibility problem when migrating the
>> next release (which will contain that code) to/from the previous
>> release.
>> 
>> I'm leaving the jobs as manual for now because using an older QEMU in
>> tests could hit bugs that were already fixed in the current
>> development tree and we need to handle those case-by-case.
>
> Can we opt-out those broken tests using either your "since:" thing or
> anything similar?

If it's something migration related, then yes. But there might be other
types of breakages that have nothing to do with migration. Our tests are
not resilent enough (nor they should) to detect when QEMU aborted for
other reasons. Think about the -audio issue: the old QEMU would just say
"there's no -audio option, abort" and that's a test failure of course.

> I hope we can start to run something by default in the CI in 9.0 to cover
> n-1 -> n, even if starting with a subset of tests.  Is it possible?

We could maybe have it enabled with "allow_failure" set. The important
thing here is that we don't want to get reports of "flaky test". These
tests are kind of flaky by definition, there's no way to backport a fix
to the older QEMU, so there's always the chance that this test will be
broken for a whole release cycle. We should act fast in adding the
"since" annotation or other workaround, but that depends on our
availability and the type of bug that we hit.

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Jan 08, 2024 at 12:37:46PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Fri, Jan 05, 2024 at 03:04:49PM -0300, Fabiano Rosas wrote:
>> >> [This patch is not necessary anymore after 8.2 has been released]
>> >> 
>> >> Add the 'since' annotations to recently added tests and adapt the
>> >> postcopy test to use the older "uri" API when needed.
>> >> 
>> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> >
>> > You marked this as not-for-merge.  Would something like this still be
>> > useful in the future?  IIUC it's a matter of whether we'd still want to
>> > test those old binaries.
>> >
>> 
>> Technically yes, but I fail to see what benefit testing old binaries
>> would bring us. I'm thinking maybe it could be useful for bisecting
>> compatibility issues, but I can't think of a scenario where we'd like to
>> change the older QEMU instead of the newer.
>> 
>> I'm of course open to suggestions if you or anyone else has an use case
>> that you'd like to keep viable.
>> 
>> So far, my idea is that once a new QEMU is released, all the "since:"
>> annotations become obsolete. We could even remove them. This series is
>> just infrastructure to make our life easier if a change is ever
>> introduced that is incompatible with the n-1 QEMU. IMO we cannot have
>> compatibility testing if a random change might break a test and make it
>> more difficult to run the remaining tests. So we'd use 'since' or the
>> vercmp function to skip/adapt the offending tests until the next QEMU is
>> released.
>> 
>> I'm basing myself on this loosely worded support statement from our
>> docs:
>> 
>>   "In general QEMU tries to maintain forward migration compatibility
>>   (i.e. migrating from QEMU n->n+1) and there are users who benefit from
>>   backward compatibility as well."
>
> I think we could still have users migrating from e.g. 8.0 -> 9.0 as long as
> with the same machine type, especially when upgrading upper level stack
> (e.g. an openstack cluster upgrade), where IIUC can jump a few qemu major
> versions.  That does sound like a common use case, and I suspect the doc
> was only taking one example on why compatibility needs to be maintained,
> rather than emphasizing "+1 only".

Oh, I would expect people to be migrating in all sorts of ways. But we
need to think in terms of what upstream QEMU supports so we can guide
the development. And hopefully have a test for everything we actually
support and everyone that touches migration code having the same view on
this.

I can barely think about n->n+1 to be honest, that's why I was writing
this compatibility test even before Juan asked for it.

You raise a good point about a cloud provider or distro jumping major
versions. That's a tricky situation. Because then their support
statement would potentially cover something that's completely different
from what we're testing upstream.

> However then the question is whether those old binaries needs to be
> convered.
>
> Then I noticed that taking all these "since: XXX" and cmdline changes along
> with migration-test may be yet another burden even if we want to cover old
> binaries for whatever reason.  I am now more convinced myself that we
> should try to get rid of as much burden as we can for migration, because we
> already have enough, and it's not ideal to keep growing that unnecessarily.
>
> One good thing with CI in this case (I still don't have enough knowledge on
> CI, so I am hoping some CI people can review that patch, though) is that if
> we can always guarantee n-1 -> n works for the test cases we enabled, it
> most probably means when n boosts again to n+1, we keep making sure n ->
> n+1 works perfectly, then n-1 -> n+1 should not fail either, considering
> that we're testing the stream protocol matching each other.  There might be
> outliers (especially if not described with VMSDs) but should be corner
> cases.

I agree that the transitivity should be preserved. If we could override
the QEMU_PREV_VERSION variable in the CI script, that would be an easy
way of running a sanity check every once in a while.

> So I tend to agree with you on that we drop this patch, keep it simple
> until we're much more clear what we can get from that.
>
> But then if so - do we need "since" at all to be expressed in versions?

I agree that we don't need "since" semantics.

> Basically we keep qtest always be valid only on the latest qemu binary as
> before (which actually works the same as Linux v.s. kselftests, which makes
> sense), there's one exception now with "n-1" due to the CI we plan to add.
> Dropping this patch means we don't yet plan to support n-2.  Then maybe
> instead of a "since" we only need a boolean showing "whether one test needs
> to be covered by a cross-binary test"?  Then we set it in incompatible
> binaries (skip all cross-binary tests directly, rather than relying on any
> qemu versions, no compare needed), and can also drop that when a new
> release starts.

Hm, it would be better to avoid the extra maintenance task at the start
of every release, no? It also blocks us from doing n-2 even
experimentally.

Re: [PATCH v3 2/4] tests/qtest/migration: Add infrastructure to skip tests on older QEMUs

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Jan 08, 2024 at 11:49:45AM -0300, Fabiano Rosas wrote:
>> >> +
>> >> +    if (major > tgt_major) {
>> >> +        return -1;
>> >
>> > This means the QEMU version is newer, the function will return negative.
>> > Is this what we want?  It seems it's inverted.
>> 
>> The return "points" to which once is the more recent:
>> 
>> QEMU version | since: version
>> -1           0         1
>
> Here if returns -1, then below [1] will skip the test?
>

Ah ok, code below is wrong.

>> 
>> > In all cases, document this function with retval would be helpful too.
>> >
>> 
>> Ok.
>> 
>> >> +    }
>> >> +    if (major < tgt_major) {
>> >> +        return 1;
>> >> +    }
>> >
>> > Instead of all these, I'm wondering whether we can allow "since" to be an
>> > array of integers, like [8, 2, 0].  Would that be much easier?
>> 
>> I don't see why push the complexity towards the person writing the
>> tests. The string is much more natural to specify.
>
> To me QEMU_VER(8,2,0) is as easy to write and read, too.  What Dan proposed
> looks also good in the other thread.
>
> I don't really have a strong opinion here especially for the test case. But
> imho it'll be still nice to avoid string <-> int if the string is not required.

Ok, I'll change it to something else.

>
> [...]
>
>> >> @@ -850,6 +856,17 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>> >>          qtest_qmp_set_event_callback(*from,
>> >>                                       migrate_watch_for_stop,
>> >>                                       &got_src_stop);
>> >> +
>> >> +        if (args->since && migration_vercmp(*from, args->since) < 0) {
>
> [1]
>
>> >> +            g_autofree char *msg = NULL;
>> >> +
>> >> +            msg = g_strdup_printf("Test requires at least QEMU version %s",
>> >> +                                  args->since);
>> >> +            g_test_skip(msg);
>> >> +            qtest_quit(*from);
>> >> +
>> >> +            return -1;
>> >> +        }

Re: [PATCH v3 3/4] ci: Add a migration compatibility test job

[Cédric Le Goater]

On 1/5/24 19:04, Fabiano Rosas wrote:
> The migration tests have support for being passed two QEMU binaries to
> test migration compatibility.
> 
> Add a CI job that builds the lastest release of QEMU and another job
> that uses that version plus an already present build of the current
> version and run the migration tests with the two, both as source and
> destination. I.e.:
> 
>   old QEMU (n-1) -> current QEMU (development tree)
>   current QEMU (development tree) -> old QEMU (n-1)
> 
> The purpose of this CI job is to ensure the code we're about to merge
> will not cause a migration compatibility problem when migrating the
> next release (which will contain that code) to/from the previous
> release.
> 
> I'm leaving the jobs as manual for now because using an older QEMU in
> tests could hit bugs that were already fixed in the current
> development tree and we need to handle those case-by-case.
> 
> Note: for user forks, the version tags need to be pushed to gitlab
> otherwise it won't be able to checkout a different version.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   .gitlab-ci.d/buildtest.yml | 53 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
> 
> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> index 91663946de..81163a3f6a 100644
> --- a/.gitlab-ci.d/buildtest.yml
> +++ b/.gitlab-ci.d/buildtest.yml
> @@ -167,6 +167,59 @@ build-system-centos:
>         x86_64-softmmu rx-softmmu sh4-softmmu nios2-softmmu
>       MAKE_CHECK_ARGS: check-build
>   
> +build-previous-qemu:
> +  extends: .native_build_job_template
> +  artifacts:
> +    when: on_success
> +    expire_in: 2 days
> +    paths:
> +      - build-previous
> +    exclude:
> +      - build-previous/**/*.p
> +      - build-previous/**/*.a.p
> +      - build-previous/**/*.fa.p
> +      - build-previous/**/*.c.o
> +      - build-previous/**/*.c.o.d
> +      - build-previous/**/*.fa
> +  needs:
> +    job: amd64-opensuse-leap-container
> +  variables:
> +    QEMU_JOB_OPTIONAL: 1
> +    IMAGE: opensuse-leap
> +    TARGETS: x86_64-softmmu aarch64-softmmu
> +  before_script:
> +    - export QEMU_PREV_VERSION="$(sed 's/\([0-9.]*\)\.[0-9]*/v\1.0/' VERSION)"
> +    - git checkout $QEMU_PREV_VERSION
> +  after_script:
> +    - mv build build-previous
> +
> +.migration-compat-common:
> +  extends: .common_test_job_template
> +  needs:
> +    - job: build-previous-qemu
> +    - job: build-system-opensuse
> +  allow_failure: true
> +  variables:
> +    QEMU_JOB_OPTIONAL: 1
> +    IMAGE: opensuse-leap
> +    MAKE_CHECK_ARGS: check-build
> +  script:
> +    - cd build
> +    - QTEST_QEMU_BINARY_SRC=../build-previous/qemu-system-${TARGET}
> +          QTEST_QEMU_BINARY=./qemu-system-${TARGET} ./tests/qtest/migration-test
> +    - QTEST_QEMU_BINARY_DST=../build-previous/qemu-system-${TARGET}
> +          QTEST_QEMU_BINARY=./qemu-system-${TARGET} ./tests/qtest/migration-test
> +
> +migration-compat-aarch64:
> +  extends: .migration-compat-common
> +  variables:
> +    TARGET: aarch64
> +
> +migration-compat-x86_64:
> +  extends: .migration-compat-common
> +  variables:
> +    TARGET: x86_64


What about the others archs, s390x and ppc ? Do you lack the resources
or are there any problems to address ?

Thanks,

C.

Re: [PATCH v3 3/4] ci: Add a migration compatibility test job

[Fabiano Rosas]

Cédric Le Goater <clg@redhat.com> writes:

> On 1/5/24 19:04, Fabiano Rosas wrote:
>> The migration tests have support for being passed two QEMU binaries to
>> test migration compatibility.
>> 
>> Add a CI job that builds the lastest release of QEMU and another job
>> that uses that version plus an already present build of the current
>> version and run the migration tests with the two, both as source and
>> destination. I.e.:
>> 
>>   old QEMU (n-1) -> current QEMU (development tree)
>>   current QEMU (development tree) -> old QEMU (n-1)
>> 
>> The purpose of this CI job is to ensure the code we're about to merge
>> will not cause a migration compatibility problem when migrating the
>> next release (which will contain that code) to/from the previous
>> release.
>> 
>> I'm leaving the jobs as manual for now because using an older QEMU in
>> tests could hit bugs that were already fixed in the current
>> development tree and we need to handle those case-by-case.
>> 
>> Note: for user forks, the version tags need to be pushed to gitlab
>> otherwise it won't be able to checkout a different version.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   .gitlab-ci.d/buildtest.yml | 53 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>> 
>> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
>> index 91663946de..81163a3f6a 100644
>> --- a/.gitlab-ci.d/buildtest.yml
>> +++ b/.gitlab-ci.d/buildtest.yml
>> @@ -167,6 +167,59 @@ build-system-centos:
>>         x86_64-softmmu rx-softmmu sh4-softmmu nios2-softmmu
>>       MAKE_CHECK_ARGS: check-build
>>   
>> +build-previous-qemu:
>> +  extends: .native_build_job_template
>> +  artifacts:
>> +    when: on_success
>> +    expire_in: 2 days
>> +    paths:
>> +      - build-previous
>> +    exclude:
>> +      - build-previous/**/*.p
>> +      - build-previous/**/*.a.p
>> +      - build-previous/**/*.fa.p
>> +      - build-previous/**/*.c.o
>> +      - build-previous/**/*.c.o.d
>> +      - build-previous/**/*.fa
>> +  needs:
>> +    job: amd64-opensuse-leap-container
>> +  variables:
>> +    QEMU_JOB_OPTIONAL: 1
>> +    IMAGE: opensuse-leap
>> +    TARGETS: x86_64-softmmu aarch64-softmmu
>> +  before_script:
>> +    - export QEMU_PREV_VERSION="$(sed 's/\([0-9.]*\)\.[0-9]*/v\1.0/' VERSION)"
>> +    - git checkout $QEMU_PREV_VERSION
>> +  after_script:
>> +    - mv build build-previous
>> +
>> +.migration-compat-common:
>> +  extends: .common_test_job_template
>> +  needs:
>> +    - job: build-previous-qemu
>> +    - job: build-system-opensuse
>> +  allow_failure: true
>> +  variables:
>> +    QEMU_JOB_OPTIONAL: 1
>> +    IMAGE: opensuse-leap
>> +    MAKE_CHECK_ARGS: check-build
>> +  script:
>> +    - cd build
>> +    - QTEST_QEMU_BINARY_SRC=../build-previous/qemu-system-${TARGET}
>> +          QTEST_QEMU_BINARY=./qemu-system-${TARGET} ./tests/qtest/migration-test
>> +    - QTEST_QEMU_BINARY_DST=../build-previous/qemu-system-${TARGET}
>> +          QTEST_QEMU_BINARY=./qemu-system-${TARGET} ./tests/qtest/migration-test
>> +
>> +migration-compat-aarch64:
>> +  extends: .migration-compat-common
>> +  variables:
>> +    TARGET: aarch64
>> +
>> +migration-compat-x86_64:
>> +  extends: .migration-compat-common
>> +  variables:
>> +    TARGET: x86_64
>
>
> What about the others archs, s390x and ppc ? Do you lack the resources
> or are there any problems to address ?

Currently s390x and ppc are only tested on KVM. Which means they are not
tested at all unless someone runs migration-test on a custom runner. The
same is true for this test.

The TCG tests have been disabled:
    /*
     * On ppc64, the test only works with kvm-hv, but not with kvm-pr and TCG
     * is touchy due to race conditions on dirty bits (especially on PPC for
     * some reason)
     */

    /*
     * Similar to ppc64, s390x seems to be touchy with TCG, so disable it
     * there until the problems are resolved
     */

It would be great if we could figure out what these issues are and fix
them so we can at least test with TCG like we do for aarch64.

Doing a TCG run of migration-test with both archs (one binary only, not
this series):

- ppc survived one run, taking 6 minutes longer than x86/Aarch64.
- s390x survived one run, taking 40s less than x86/aarch64.

I'll leave them enabled on my machine and do some runs here and there,
see if I spot something. If not, we can consider re-enabling them once
we figure out why ppc takes so long.

Re: [PATCH v3 3/4] ci: Add a migration compatibility test job

[Peter Xu]

On Tue, Jan 09, 2024 at 10:00:17AM -0300, Fabiano Rosas wrote:
> > Can we opt-out those broken tests using either your "since:" thing or
> > anything similar?
> 
> If it's something migration related, then yes. But there might be other
> types of breakages that have nothing to do with migration. Our tests are
> not resilent enough (nor they should) to detect when QEMU aborted for
> other reasons. Think about the -audio issue: the old QEMU would just say
> "there's no -audio option, abort" and that's a test failure of course.

I'm wondering whether we can more or less remedy that by running
migration-test under the build-previous directory for cross-binary tests.
We don't necessarily need to cross-test anything new happening anyway.

IOW, we use both old QEMU / migration-test for "n-1", and we only use "n"
for the new QEMU binary?

-- 
Peter Xu

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Peter Xu]

On Tue, Jan 09, 2024 at 11:46:32AM -0300, Fabiano Rosas wrote:
> Hm, it would be better to avoid the extra maintenance task at the start
> of every release, no? It also blocks us from doing n-2 even
> experimentally.

See my other reply, on whether we can use "n-1" for migration-test.  If
that can work for us, then IIUC we can avoid either "since:" or any
relevant flag, neither do we need to unmask tests after each releases.  All
old tests should always "just work" with a new qemu binary.

One drawback I can think of is, new tests (even if applicable to old qemu
binaries) will only start to take effect on cross-binary test until the
next release, but that's not so bad I assume.

Since the QTEST_QEMU_BINARY_SRC|DST function is already merged in 8.2, I
think we can already start kicking them and enable them for 9.0 if it works.

-- 
Peter Xu

Re: [PATCH v3 3/4] ci: Add a migration compatibility test job

[Thomas Huth]

On 09/01/2024 21.58, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> On 1/5/24 19:04, Fabiano Rosas wrote:
>>> The migration tests have support for being passed two QEMU binaries to
>>> test migration compatibility.
>>>
>>> Add a CI job that builds the lastest release of QEMU and another job
>>> that uses that version plus an already present build of the current
>>> version and run the migration tests with the two, both as source and
>>> destination. I.e.:
>>>
>>>    old QEMU (n-1) -> current QEMU (development tree)
>>>    current QEMU (development tree) -> old QEMU (n-1)
>>>
>>> The purpose of this CI job is to ensure the code we're about to merge
>>> will not cause a migration compatibility problem when migrating the
>>> next release (which will contain that code) to/from the previous
>>> release.
>>>
>>> I'm leaving the jobs as manual for now because using an older QEMU in
>>> tests could hit bugs that were already fixed in the current
>>> development tree and we need to handle those case-by-case.
>>>
>>> Note: for user forks, the version tags need to be pushed to gitlab
>>> otherwise it won't be able to checkout a different version.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>    .gitlab-ci.d/buildtest.yml | 53 ++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 53 insertions(+)
>>>
>>> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
>>> index 91663946de..81163a3f6a 100644
>>> --- a/.gitlab-ci.d/buildtest.yml
>>> +++ b/.gitlab-ci.d/buildtest.yml
>>> @@ -167,6 +167,59 @@ build-system-centos:
>>>          x86_64-softmmu rx-softmmu sh4-softmmu nios2-softmmu
>>>        MAKE_CHECK_ARGS: check-build
>>>    
>>> +build-previous-qemu:
>>> +  extends: .native_build_job_template
>>> +  artifacts:
>>> +    when: on_success
>>> +    expire_in: 2 days
>>> +    paths:
>>> +      - build-previous
>>> +    exclude:
>>> +      - build-previous/**/*.p
>>> +      - build-previous/**/*.a.p
>>> +      - build-previous/**/*.fa.p
>>> +      - build-previous/**/*.c.o
>>> +      - build-previous/**/*.c.o.d
>>> +      - build-previous/**/*.fa
>>> +  needs:
>>> +    job: amd64-opensuse-leap-container
>>> +  variables:
>>> +    QEMU_JOB_OPTIONAL: 1
>>> +    IMAGE: opensuse-leap
>>> +    TARGETS: x86_64-softmmu aarch64-softmmu
>>> +  before_script:
>>> +    - export QEMU_PREV_VERSION="$(sed 's/\([0-9.]*\)\.[0-9]*/v\1.0/' VERSION)"
>>> +    - git checkout $QEMU_PREV_VERSION
>>> +  after_script:
>>> +    - mv build build-previous
>>> +
>>> +.migration-compat-common:
>>> +  extends: .common_test_job_template
>>> +  needs:
>>> +    - job: build-previous-qemu
>>> +    - job: build-system-opensuse
>>> +  allow_failure: true
>>> +  variables:
>>> +    QEMU_JOB_OPTIONAL: 1
>>> +    IMAGE: opensuse-leap
>>> +    MAKE_CHECK_ARGS: check-build
>>> +  script:
>>> +    - cd build
>>> +    - QTEST_QEMU_BINARY_SRC=../build-previous/qemu-system-${TARGET}
>>> +          QTEST_QEMU_BINARY=./qemu-system-${TARGET} ./tests/qtest/migration-test
>>> +    - QTEST_QEMU_BINARY_DST=../build-previous/qemu-system-${TARGET}
>>> +          QTEST_QEMU_BINARY=./qemu-system-${TARGET} ./tests/qtest/migration-test
>>> +
>>> +migration-compat-aarch64:
>>> +  extends: .migration-compat-common
>>> +  variables:
>>> +    TARGET: aarch64
>>> +
>>> +migration-compat-x86_64:
>>> +  extends: .migration-compat-common
>>> +  variables:
>>> +    TARGET: x86_64
>>
>>
>> What about the others archs, s390x and ppc ? Do you lack the resources
>> or are there any problems to address ?
> 
> Currently s390x and ppc are only tested on KVM. Which means they are not
> tested at all unless someone runs migration-test on a custom runner. The
> same is true for this test.
> 
> The TCG tests have been disabled:
>      /*
>       * On ppc64, the test only works with kvm-hv, but not with kvm-pr and TCG
>       * is touchy due to race conditions on dirty bits (especially on PPC for
>       * some reason)
>       */
> 
>      /*
>       * Similar to ppc64, s390x seems to be touchy with TCG, so disable it
>       * there until the problems are resolved
>       */
> 
> It would be great if we could figure out what these issues are and fix
> them so we can at least test with TCG like we do for aarch64.
> 
> Doing a TCG run of migration-test with both archs (one binary only, not
> this series):
> 
> - ppc survived one run, taking 6 minutes longer than x86/Aarch64.
> - s390x survived one run, taking 40s less than x86/aarch64.
> 
> I'll leave them enabled on my machine and do some runs here and there,
> see if I spot something. If not, we can consider re-enabling them once
> we figure out why ppc takes so long.

I was curious and re-enabled the ppc64 and s390x migration tests with TCG on 
my laptop here, running "make check-tcg -j$(nproc)" in a loop. s390x 
unfortunately hang after the second iteration already, but ppc64 survived 25 
runs (then I stopped it).

So we might want to try to re-enable ppc64 at least. But we might need to 
cut the run time for ppc64 with TCG a little bit, it is currently the 
longest test on my system (it takes 240s to finish, while all other tests 
finish within 150s).

  Thomas

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Jan 09, 2024 at 11:46:32AM -0300, Fabiano Rosas wrote:
>> Hm, it would be better to avoid the extra maintenance task at the start
>> of every release, no? It also blocks us from doing n-2 even
>> experimentally.
>
> See my other reply, on whether we can use "n-1" for migration-test.  If
> that can work for us, then IIUC we can avoid either "since:" or any
> relevant flag, neither do we need to unmask tests after each releases.  All
> old tests should always "just work" with a new qemu binary.

Hmm.. There are some assumptions here:

1) New code will always be compatible with old tests. E.g. some
   patchseries changed code and changed a test to match the new
   code. Then we'd need a flag like 'since' anyway to mark that the new
   QEMU cannot be used with the old test.

   (if new QEMU is not compatible with old tests without any good
   reason, then that's just a regression I think)

2) There would not be issues when fixing bugs/refactoring
   tests. E.g. old tests had a bug that is now fixed, but since we're
   not using the new tests, the bug is always there until next
   release. This could block the entire test suite, specially with
   concurrency bugs which can start triggering due to changes in timing.

3) New code that can only be reached via new tests cannot cause
   regressions. E.g. new code is added but is kept under a machine
   property or migration capability. That code will only show the
   regression after the new test enables that cap/property. At that
   point it's too late because it was already released.

In general I like the simplicity of your approach, but it would be
annoying to change this series only to find out we still need some sort
of flag later. Even worse, #3 would miss the point of this kind of
testing entirely.

#1 could be mitigated by a "no changes to tests rule". We'd start
requiring that new tests be written and an existing test is never
altered. For #2 and #3 I don't have a solution.

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Peter Xu]

On Wed, Jan 10, 2024 at 11:42:18AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Jan 09, 2024 at 11:46:32AM -0300, Fabiano Rosas wrote:
> >> Hm, it would be better to avoid the extra maintenance task at the start
> >> of every release, no? It also blocks us from doing n-2 even
> >> experimentally.
> >
> > See my other reply, on whether we can use "n-1" for migration-test.  If
> > that can work for us, then IIUC we can avoid either "since:" or any
> > relevant flag, neither do we need to unmask tests after each releases.  All
> > old tests should always "just work" with a new qemu binary.
> 
> Hmm.. There are some assumptions here:
> 
> 1) New code will always be compatible with old tests. E.g. some
>    patchseries changed code and changed a test to match the new
>    code. Then we'd need a flag like 'since' anyway to mark that the new
>    QEMU cannot be used with the old test.
> 
>    (if new QEMU is not compatible with old tests without any good
>    reason, then that's just a regression I think)

Exactly what you are saying here.  We can't make new QEMU not working on
old tests.

One way to simplify the understanding is, we can imagine the old tests as
"some user currently using the old QEMU, and who would like to migrate to
the master QEMU binary".  Such user only uses exactly the same cmdline we
used for testing migration-test in exactly that n-1 qemu release binary.

If we fail that old test, it means we can already fail such an user.
That's destined a regression to me, no?  Or, do you have a solid example?

The only thing I can think of is, when we want to e.g. obsolete a QEMU
cmdline that is used in migration-test.  But then that cmdline needs to be
declared obsolete first for a few releases (let's say, 4), and before that
deadline we should already rewrite migration-test to not use it, and as
long as we do it in 3 releases I suppose nothing will be affected.

> 
> 2) There would not be issues when fixing bugs/refactoring
>    tests. E.g. old tests had a bug that is now fixed, but since we're
>    not using the new tests, the bug is always there until next
>    release. This could block the entire test suite, specially with
>    concurrency bugs which can start triggering due to changes in timing.

Yes this might be a problem.  Note that the old tests we're using will be
exactly the same test we released previous QEMU.  I am "assuming" that the
test case is as stable as the released QEMU, since we kept running it for
all pulls in CI runs.  If we see anything flaky, we should mark it
especially right before the release, then the released tests will be
considerably stable.

The worst case is we still keep a knob in the CI file, and we can turn off
n-1 -> n tests for the CI for some release if there's some unfortunate
accident.  But I hope in reality that can be avoided.

> 
> 3) New code that can only be reached via new tests cannot cause
>    regressions. E.g. new code is added but is kept under a machine
>    property or migration capability. That code will only show the
>    regression after the new test enables that cap/property. At that
>    point it's too late because it was already released.

I can't say I fully get the point here.  New code, if with a new cap with
it, should run exactly like the old code if the cap is not turned on.  I
suppose that's the case for when we only run n-1 version of migration-test.
IMHO it's the same issue as 1) above, that we just should not break it, and
if we do, that's exactly what we want to capture and fix in master, not n-1
branch.

But as I said, perhaps I didn't really get the issue you wanted to describe..

> 
> In general I like the simplicity of your approach, but it would be
> annoying to change this series only to find out we still need some sort
> of flag later. Even worse, #3 would miss the point of this kind of
> testing entirely.
> 
> #1 could be mitigated by a "no changes to tests rule". We'd start
> requiring that new tests be written and an existing test is never
> altered. For #2 and #3 I don't have a solution.
> 

-- 
Peter Xu

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Jan 10, 2024 at 11:42:18AM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Tue, Jan 09, 2024 at 11:46:32AM -0300, Fabiano Rosas wrote:
>> >> Hm, it would be better to avoid the extra maintenance task at the start
>> >> of every release, no? It also blocks us from doing n-2 even
>> >> experimentally.
>> >
>> > See my other reply, on whether we can use "n-1" for migration-test.  If
>> > that can work for us, then IIUC we can avoid either "since:" or any
>> > relevant flag, neither do we need to unmask tests after each releases.  All
>> > old tests should always "just work" with a new qemu binary.
>> 
>> Hmm.. There are some assumptions here:
>> 
>> 1) New code will always be compatible with old tests. E.g. some
>>    patchseries changed code and changed a test to match the new
>>    code. Then we'd need a flag like 'since' anyway to mark that the new
>>    QEMU cannot be used with the old test.
>> 
>>    (if new QEMU is not compatible with old tests without any good
>>    reason, then that's just a regression I think)
>
> Exactly what you are saying here.  We can't make new QEMU not working on
> old tests.

Ok, so we need to forbid breaking changes to tests from now on. I'll try
to add some words in the docs about this.

>
> One way to simplify the understanding is, we can imagine the old tests as
> "some user currently using the old QEMU, and who would like to migrate to
> the master QEMU binary".  Such user only uses exactly the same cmdline we
> used for testing migration-test in exactly that n-1 qemu release binary.
>
> If we fail that old test, it means we can already fail such an user.
> That's destined a regression to me, no?  Or, do you have a solid example?

For instance, we used to not issue the SETUP event on incoming. If a
test (or user app) expected to see the ACTIVE or FAILED states, then
would it be a regression to now start issuing the SETUP event at the
proper place?

Anyway, it's pointless to give examples because we either allow old
tests to be changed or we don't. If we don't then that's solved. If we
do, we'll always have space for the situation I mentioned in 1) above.

> The only thing I can think of is, when we want to e.g. obsolete a QEMU
> cmdline that is used in migration-test.  But then that cmdline needs to be
> declared obsolete first for a few releases (let's say, 4), and before that
> deadline we should already rewrite migration-test to not use it, and as
> long as we do it in 3 releases I suppose nothing will be affected.
>
>> 
>> 2) There would not be issues when fixing bugs/refactoring
>>    tests. E.g. old tests had a bug that is now fixed, but since we're
>>    not using the new tests, the bug is always there until next
>>    release. This could block the entire test suite, specially with
>>    concurrency bugs which can start triggering due to changes in timing.
>
> Yes this might be a problem.  Note that the old tests we're using will be
> exactly the same test we released previous QEMU.  I am "assuming" that the
> test case is as stable as the released QEMU, since we kept running it for
> all pulls in CI runs.  If we see anything flaky, we should mark it
> especially right before the release, then the released tests will be
> considerably stable.

It's not just the test case. The whole test infrastructure could change
entirely. But let's maybe cross that bridge when we get to it.

>
> The worst case is we still keep a knob in the CI file, and we can turn off
> n-1 -> n tests for the CI for some release if there's some unfortunate
> accident. But I hope in reality that can be avoided.
>
>> 
>> 3) New code that can only be reached via new tests cannot cause
>>    regressions. E.g. new code is added but is kept under a machine
>>    property or migration capability. That code will only show the
>>    regression after the new test enables that cap/property. At that
>>    point it's too late because it was already released.
>
> I can't say I fully get the point here.  New code, if with a new cap with
> it, should run exactly like the old code if the cap is not turned on.  I
> suppose that's the case for when we only run n-1 version of migration-test.
> IMHO it's the same issue as 1) above, that we just should not break it, and
> if we do, that's exactly what we want to capture and fix in master, not n-1
> branch.
>
> But as I said, perhaps I didn't really get the issue you wanted to describe..

if (cap_foo()) {
   <do something bad>
}

This^ only executes once we have a test that enables cap_foo. If the
"something bad" is something that breaks compatibility, then we'll miss
it when using n-1 migration-test.

Now that I think about it, should we parameterize the CI so we can
actually switch between old migration-tests and new migration-tests? So
we make the default what you suggest, but still have the ability to
trigger a job every once in a while that uses the new tests.

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Peter Xu]

On Thu, Jan 11, 2024 at 10:58:49AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Jan 10, 2024 at 11:42:18AM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Tue, Jan 09, 2024 at 11:46:32AM -0300, Fabiano Rosas wrote:
> >> >> Hm, it would be better to avoid the extra maintenance task at the start
> >> >> of every release, no? It also blocks us from doing n-2 even
> >> >> experimentally.
> >> >
> >> > See my other reply, on whether we can use "n-1" for migration-test.  If
> >> > that can work for us, then IIUC we can avoid either "since:" or any
> >> > relevant flag, neither do we need to unmask tests after each releases.  All
> >> > old tests should always "just work" with a new qemu binary.
> >> 
> >> Hmm.. There are some assumptions here:
> >> 
> >> 1) New code will always be compatible with old tests. E.g. some
> >>    patchseries changed code and changed a test to match the new
> >>    code. Then we'd need a flag like 'since' anyway to mark that the new
> >>    QEMU cannot be used with the old test.
> >> 
> >>    (if new QEMU is not compatible with old tests without any good
> >>    reason, then that's just a regression I think)
> >
> > Exactly what you are saying here.  We can't make new QEMU not working on
> > old tests.
> 
> Ok, so we need to forbid breaking changes to tests from now on. I'll try
> to add some words in the docs about this.
> 
> >
> > One way to simplify the understanding is, we can imagine the old tests as
> > "some user currently using the old QEMU, and who would like to migrate to
> > the master QEMU binary".  Such user only uses exactly the same cmdline we
> > used for testing migration-test in exactly that n-1 qemu release binary.
> >
> > If we fail that old test, it means we can already fail such an user.
> > That's destined a regression to me, no?  Or, do you have a solid example?
> 
> For instance, we used to not issue the SETUP event on incoming. If a
> test (or user app) expected to see the ACTIVE or FAILED states, then
> would it be a regression to now start issuing the SETUP event at the
> proper place?

Valid example.  And it's a tricky example in that it actually breaks the
ABI even though slightly, however events are just normally more flexible in
this case, so we didn't care.

I think it means we didn't care any program expecting no SETUP before
ACTIVE, or such user already crashes.

Our migration-test is compatible with such change, right?

I think the trick here is we shouldn't make migration-test to ever contain
any "assumption" of the internals of QEMU.  It should only behave strictly
as what an user can use QEMU, and that should always be guaranteed to work
on newer qemu binaries.  Then breaking old migration-test will be the same
as breaking an user, and it'll naturally fit in this model too of using n-1
version of migration-test.

> 
> Anyway, it's pointless to give examples because we either allow old
> tests to be changed or we don't. If we don't then that's solved. If we
> do, we'll always have space for the situation I mentioned in 1) above.

IMHO we should allow any changes to old tests, IMHO.  It won't apply to n-1
test anyway, not until the next release.  It may depend on how you define
"changed" in this case.

> 
> > The only thing I can think of is, when we want to e.g. obsolete a QEMU
> > cmdline that is used in migration-test.  But then that cmdline needs to be
> > declared obsolete first for a few releases (let's say, 4), and before that
> > deadline we should already rewrite migration-test to not use it, and as
> > long as we do it in 3 releases I suppose nothing will be affected.
> >
> >> 
> >> 2) There would not be issues when fixing bugs/refactoring
> >>    tests. E.g. old tests had a bug that is now fixed, but since we're
> >>    not using the new tests, the bug is always there until next
> >>    release. This could block the entire test suite, specially with
> >>    concurrency bugs which can start triggering due to changes in timing.
> >
> > Yes this might be a problem.  Note that the old tests we're using will be
> > exactly the same test we released previous QEMU.  I am "assuming" that the
> > test case is as stable as the released QEMU, since we kept running it for
> > all pulls in CI runs.  If we see anything flaky, we should mark it
> > especially right before the release, then the released tests will be
> > considerably stable.
> 
> It's not just the test case. The whole test infrastructure could change
> entirely. But let's maybe cross that bridge when we get to it.
> 
> >
> > The worst case is we still keep a knob in the CI file, and we can turn off
> > n-1 -> n tests for the CI for some release if there's some unfortunate
> > accident. But I hope in reality that can be avoided.
> >
> >> 
> >> 3) New code that can only be reached via new tests cannot cause
> >>    regressions. E.g. new code is added but is kept under a machine
> >>    property or migration capability. That code will only show the
> >>    regression after the new test enables that cap/property. At that
> >>    point it's too late because it was already released.
> >
> > I can't say I fully get the point here.  New code, if with a new cap with
> > it, should run exactly like the old code if the cap is not turned on.  I
> > suppose that's the case for when we only run n-1 version of migration-test.
> > IMHO it's the same issue as 1) above, that we just should not break it, and
> > if we do, that's exactly what we want to capture and fix in master, not n-1
> > branch.
> >
> > But as I said, perhaps I didn't really get the issue you wanted to describe..
> 
> if (cap_foo()) {
>    <do something bad>
> }
> 
> This^ only executes once we have a test that enables cap_foo. If the
> "something bad" is something that breaks compatibility, then we'll miss
> it when using n-1 migration-test.

IMHO the n-1 tests are not for this.  The new FOO cap can only be enabled
in n+ versions anyway, so something like above should be covered by the
normal migration test that anyone would like to propose the new FOO cap.
The n-1 test we're discussing is extra tests on top of that.  So:

  - Same binary test: we (of course) keep running migration-test for
    master, covers FOO

  - Cross binary testA: we (hopefully since 9.0?) runs n-1 migration-test
    for previous release

Then after n boosts, the new FOO test (that will enable FOO) will become
part of n-1 tests.

> 
> Now that I think about it, should we parameterize the CI so we can
> actually switch between old migration-tests and new migration-tests? So
> we make the default what you suggest, but still have the ability to
> trigger a job every once in a while that uses the new tests.

Certainly. Such a knob will never hurt, I assume.  It's just that I'd
expect new migration-test could constantly fail the cross-binary test as
long as we introduce new features.  Maybe it's a matter of whether we would
like migration-test itself to understand the "version" idea.

What I was saying above is trying to reduce our burden to teach
migration-test to understand any version concept.  So migration-test always
applies only to the master branch (and newer; due to migration's strict
ABI), no need to detect any cap as long as master supports it.

-- 
Peter Xu

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Jan 11, 2024 at 10:58:49AM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Wed, Jan 10, 2024 at 11:42:18AM -0300, Fabiano Rosas wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > On Tue, Jan 09, 2024 at 11:46:32AM -0300, Fabiano Rosas wrote:
>> >> >> Hm, it would be better to avoid the extra maintenance task at the start
>> >> >> of every release, no? It also blocks us from doing n-2 even
>> >> >> experimentally.
>> >> >
>> >> > See my other reply, on whether we can use "n-1" for migration-test.  If
>> >> > that can work for us, then IIUC we can avoid either "since:" or any
>> >> > relevant flag, neither do we need to unmask tests after each releases.  All
>> >> > old tests should always "just work" with a new qemu binary.
>> >> 
>> >> Hmm.. There are some assumptions here:
>> >> 
>> >> 1) New code will always be compatible with old tests. E.g. some
>> >>    patchseries changed code and changed a test to match the new
>> >>    code. Then we'd need a flag like 'since' anyway to mark that the new
>> >>    QEMU cannot be used with the old test.
>> >> 
>> >>    (if new QEMU is not compatible with old tests without any good
>> >>    reason, then that's just a regression I think)
>> >
>> > Exactly what you are saying here.  We can't make new QEMU not working on
>> > old tests.
>> 
>> Ok, so we need to forbid breaking changes to tests from now on. I'll try
>> to add some words in the docs about this.
>> 
>> >
>> > One way to simplify the understanding is, we can imagine the old tests as
>> > "some user currently using the old QEMU, and who would like to migrate to
>> > the master QEMU binary".  Such user only uses exactly the same cmdline we
>> > used for testing migration-test in exactly that n-1 qemu release binary.
>> >
>> > If we fail that old test, it means we can already fail such an user.
>> > That's destined a regression to me, no?  Or, do you have a solid example?
>> 
>> For instance, we used to not issue the SETUP event on incoming. If a
>> test (or user app) expected to see the ACTIVE or FAILED states, then
>> would it be a regression to now start issuing the SETUP event at the
>> proper place?
>
> Valid example.  And it's a tricky example in that it actually breaks the
> ABI even though slightly, however events are just normally more flexible in
> this case, so we didn't care.
>
> I think it means we didn't care any program expecting no SETUP before
> ACTIVE, or such user already crashes.
>
> Our migration-test is compatible with such change, right?
>
> I think the trick here is we shouldn't make migration-test to ever contain
> any "assumption" of the internals of QEMU.  It should only behave strictly
> as what an user can use QEMU, and that should always be guaranteed to work
> on newer qemu binaries.  Then breaking old migration-test will be the same
> as breaking an user, and it'll naturally fit in this model too of using n-1
> version of migration-test.
>
>> 
>> Anyway, it's pointless to give examples because we either allow old
>> tests to be changed or we don't. If we don't then that's solved. If we
>> do, we'll always have space for the situation I mentioned in 1) above.
>
> IMHO we should allow any changes to old tests, IMHO.  It won't apply to n-1
> test anyway, not until the next release.  It may depend on how you define
> "changed" in this case.
>

I mean introducing a piece of code in QEMU which requires a change in a
test. That shouldn't be allowed. Because the n-1 tests will still have
the old behavior which could potentially clash with what the new QEMU is
doing.

>> 
>> > The only thing I can think of is, when we want to e.g. obsolete a QEMU
>> > cmdline that is used in migration-test.  But then that cmdline needs to be
>> > declared obsolete first for a few releases (let's say, 4), and before that
>> > deadline we should already rewrite migration-test to not use it, and as
>> > long as we do it in 3 releases I suppose nothing will be affected.
>> >
>> >> 
>> >> 2) There would not be issues when fixing bugs/refactoring
>> >>    tests. E.g. old tests had a bug that is now fixed, but since we're
>> >>    not using the new tests, the bug is always there until next
>> >>    release. This could block the entire test suite, specially with
>> >>    concurrency bugs which can start triggering due to changes in timing.
>> >
>> > Yes this might be a problem.  Note that the old tests we're using will be
>> > exactly the same test we released previous QEMU.  I am "assuming" that the
>> > test case is as stable as the released QEMU, since we kept running it for
>> > all pulls in CI runs.  If we see anything flaky, we should mark it
>> > especially right before the release, then the released tests will be
>> > considerably stable.
>> 
>> It's not just the test case. The whole test infrastructure could change
>> entirely. But let's maybe cross that bridge when we get to it.
>> 
>> >
>> > The worst case is we still keep a knob in the CI file, and we can turn off
>> > n-1 -> n tests for the CI for some release if there's some unfortunate
>> > accident. But I hope in reality that can be avoided.
>> >
>> >> 
>> >> 3) New code that can only be reached via new tests cannot cause
>> >>    regressions. E.g. new code is added but is kept under a machine
>> >>    property or migration capability. That code will only show the
>> >>    regression after the new test enables that cap/property. At that
>> >>    point it's too late because it was already released.
>> >
>> > I can't say I fully get the point here.  New code, if with a new cap with
>> > it, should run exactly like the old code if the cap is not turned on.  I
>> > suppose that's the case for when we only run n-1 version of migration-test.
>> > IMHO it's the same issue as 1) above, that we just should not break it, and
>> > if we do, that's exactly what we want to capture and fix in master, not n-1
>> > branch.
>> >
>> > But as I said, perhaps I didn't really get the issue you wanted to describe..
>> 
>> if (cap_foo()) {
>>    <do something bad>
>> }
>> 
>> This^ only executes once we have a test that enables cap_foo. If the
>> "something bad" is something that breaks compatibility, then we'll miss
>> it when using n-1 migration-test.
>
> IMHO the n-1 tests are not for this.  The new FOO cap can only be enabled
> in n+ versions anyway, so something like above should be covered by the
> normal migration test that anyone would like to propose the new FOO cap.

You're being too generous in thinking new code will always restrict
itself to implementing new functionality and never have a bug that
affects a completly different part of the code. There could be an
innocent refactoring along with cap FOO that breaks the migration only
when FOO is enabled.

But fine. We can't predict every scenario. Let's get this series out the
door.

Thanks for the comments so far. I'll spin another version.

> The n-1 test we're discussing is extra tests on top of that.  So:
>
>   - Same binary test: we (of course) keep running migration-test for
>     master, covers FOO
>
>   - Cross binary testA: we (hopefully since 9.0?) runs n-1 migration-test
>     for previous release
>
> Then after n boosts, the new FOO test (that will enable FOO) will become
> part of n-1 tests.
>
>> 
>> Now that I think about it, should we parameterize the CI so we can
>> actually switch between old migration-tests and new migration-tests? So
>> we make the default what you suggest, but still have the ability to
>> trigger a job every once in a while that uses the new tests.
>
> Certainly. Such a knob will never hurt, I assume.  It's just that I'd
> expect new migration-test could constantly fail the cross-binary test as
> long as we introduce new features.  Maybe it's a matter of whether we would
> like migration-test itself to understand the "version" idea.
>
> What I was saying above is trying to reduce our burden to teach
> migration-test to understand any version concept.  So migration-test always
> applies only to the master branch (and newer; due to migration's strict
> ABI), no need to detect any cap as long as master supports it.

Re: [PATCH v3 4/4] [NOT FOR MERGE] tests/qtest/migration: Adapt tests to use older QEMUs

[Peter Xu]

On Mon, Jan 15, 2024 at 10:45:33AM -0300, Fabiano Rosas wrote:
> > IMHO the n-1 tests are not for this.  The new FOO cap can only be enabled
> > in n+ versions anyway, so something like above should be covered by the
> > normal migration test that anyone would like to propose the new FOO cap.
> 
> You're being too generous in thinking new code will always restrict
> itself to implementing new functionality and never have a bug that
> affects a completly different part of the code. There could be an
> innocent refactoring along with cap FOO that breaks the migration only
> when FOO is enabled.

The question is even if we run cross-binary migration-test with current
version ("n") we can't detect such issue, right?  Because afaiu with that
we need to let migration-test always understand qemu versions, and it
should skip the new test that will enable FOO for cross-binary test since
it should detect the old binary doesn't support it.

> 
> But fine. We can't predict every scenario. Let's get this series out the
> door.
> 
> Thanks for the comments so far. I'll spin another version.

Yes if you think that is a good start point, we can start from simple.
That's so far the only solution I can think of that has mostly zero
maintanence burden for the tests meanwhile hopefully start to cover some
spots for us.  Said that, the discussion can keep going no matter what.

-- 
Peter Xu