[PATCH v2 0/2] migration-test: Allow test to run without uffd

[PATCH v2 0/2] migration-test: Allow test to run without uffd

[Peter Xu]

Compare to v1, this added a new patch as reported by Thomas to (hopefully)
allow auto-converge test to pass on some MacOS testbeds.

Please review, thanks.

Peter Xu (2):
  migration-test: Use migrate_ensure_converge() for auto-converge
  migration-test: Allow test to run without uffd

 tests/qtest/migration-test.c | 65 +++++++++++++++---------------------
 1 file changed, 26 insertions(+), 39 deletions(-)

-- 
2.32.0

[PATCH v2 1/2] migration-test: Use migrate_ensure_converge() for auto-converge

[Peter Xu]

Thomas reported that auto-converge test will timeout on MacOS CI gatings.
Use the migrate_ensure_converge() helper too in the auto-converge as when
Daniel reworked the other test cases.

Since both max_bandwidth / downtime_limit will not be used for converge
calculations, make it simple by removing the remaining check, then we can
completely remove both variables altogether, since migrate_ensure_converge
is used the remaining check won't make much sense anyway.

Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qtest/migration-test.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 71595a74fd..dd50aa600c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1776,14 +1776,6 @@ static void test_migrate_auto_converge(void)
      * so we need to decrease a bandwidth.
      */
     const int64_t init_pct = 5, inc_pct = 50, max_pct = 95;
-    const int64_t max_bandwidth = 400000000; /* ~400Mb/s */
-    const int64_t downtime_limit = 250; /* 250ms */
-    /*
-     * We migrate through unix-socket (> 500Mb/s).
-     * Thus, expected migration speed ~= bandwidth limit (< 500Mb/s).
-     * So, we can predict expected_threshold
-     */
-    const int64_t expected_threshold = max_bandwidth * downtime_limit / 1000;
 
     if (test_migrate_start(&from, &to, uri, &args)) {
         return;
@@ -1818,8 +1810,7 @@ static void test_migrate_auto_converge(void)
     /* The first percentage of throttling should be equal to init_pct */
     g_assert_cmpint(percentage, ==, init_pct);
     /* Now, when we tested that throttling works, let it converge */
-    migrate_set_parameter_int(from, "downtime-limit", downtime_limit);
-    migrate_set_parameter_int(from, "max-bandwidth", max_bandwidth);
+    migrate_ensure_converge(from);
 
     /*
      * Wait for pre-switchover status to check last throttle percentage
@@ -1830,11 +1821,6 @@ static void test_migrate_auto_converge(void)
     /* The final percentage of throttling shouldn't be greater than max_pct */
     percentage = read_migrate_property_int(from, "cpu-throttle-percentage");
     g_assert_cmpint(percentage, <=, max_pct);
-
-    remaining = read_ram_property_int(from, "remaining");
-    g_assert_cmpint(remaining, <,
-                    (expected_threshold + expected_threshold / 100));
-
     migrate_continue(from, "pre-switchover");
 
     qtest_qmp_eventwait(to, "RESUME");
@@ -1842,7 +1828,6 @@ static void test_migrate_auto_converge(void)
     wait_for_serial("dest_serial");
     wait_for_migration_complete(from);
 
-
     test_migrate_end(from, to, true);
 }
 
-- 
2.32.0

Re: [PATCH v2 1/2] migration-test: Use migrate_ensure_converge() for auto-converge

[Daniel P. Berrangé]

On Fri, Jul 22, 2022 at 10:56:53AM -0400, Peter Xu wrote:
> Thomas reported that auto-converge test will timeout on MacOS CI gatings.
> Use the migrate_ensure_converge() helper too in the auto-converge as when
> Daniel reworked the other test cases.
> 
> Since both max_bandwidth / downtime_limit will not be used for converge
> calculations, make it simple by removing the remaining check, then we can
> completely remove both variables altogether, since migrate_ensure_converge
> is used the remaining check won't make much sense anyway.
> 
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tests/qtest/migration-test.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 v2 1/2] migration-test: Use migrate_ensure_converge() for auto-converge

[Thomas Huth]

On 22/07/2022 16.56, Peter Xu wrote:
> Thomas reported that auto-converge test will timeout on MacOS CI gatings.
> Use the migrate_ensure_converge() helper too in the auto-converge as when
> Daniel reworked the other test cases.
> 
> Since both max_bandwidth / downtime_limit will not be used for converge
> calculations, make it simple by removing the remaining check, then we can
> completely remove both variables altogether, since migrate_ensure_converge
> is used the remaining check won't make much sense anyway.
> 
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   tests/qtest/migration-test.c | 17 +----------------
>   1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 71595a74fd..dd50aa600c 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1776,14 +1776,6 @@ static void test_migrate_auto_converge(void)
>        * so we need to decrease a bandwidth.
>        */
>       const int64_t init_pct = 5, inc_pct = 50, max_pct = 95;
> -    const int64_t max_bandwidth = 400000000; /* ~400Mb/s */
> -    const int64_t downtime_limit = 250; /* 250ms */
> -    /*
> -     * We migrate through unix-socket (> 500Mb/s).
> -     * Thus, expected migration speed ~= bandwidth limit (< 500Mb/s).
> -     * So, we can predict expected_threshold
> -     */
> -    const int64_t expected_threshold = max_bandwidth * downtime_limit / 1000;
>   
>       if (test_migrate_start(&from, &to, uri, &args)) {
>           return;
> @@ -1818,8 +1810,7 @@ static void test_migrate_auto_converge(void)
>       /* The first percentage of throttling should be equal to init_pct */
>       g_assert_cmpint(percentage, ==, init_pct);
>       /* Now, when we tested that throttling works, let it converge */
> -    migrate_set_parameter_int(from, "downtime-limit", downtime_limit);
> -    migrate_set_parameter_int(from, "max-bandwidth", max_bandwidth);
> +    migrate_ensure_converge(from);
>   
>       /*
>        * Wait for pre-switchover status to check last throttle percentage
> @@ -1830,11 +1821,6 @@ static void test_migrate_auto_converge(void)
>       /* The final percentage of throttling shouldn't be greater than max_pct */
>       percentage = read_migrate_property_int(from, "cpu-throttle-percentage");
>       g_assert_cmpint(percentage, <=, max_pct);
> -
> -    remaining = read_ram_property_int(from, "remaining");
> -    g_assert_cmpint(remaining, <,
> -                    (expected_threshold + expected_threshold / 100));
> -

I'm getting this on FreeBSD:

../tests/qtest/migration-test.c:1771:13: error: unused variable 'remaining' 
[-Werror,-Wunused-variable]
     int64_t remaining, percentage;
             ^
1 error generated.

Did you try this with -Werror ?

  Thomas

Re: [PATCH v2 1/2] migration-test: Use migrate_ensure_converge() for auto-converge

[Peter Xu]

On Thu, Jul 28, 2022 at 03:04:27PM +0200, Thomas Huth wrote:
> On 22/07/2022 16.56, Peter Xu wrote:
> > Thomas reported that auto-converge test will timeout on MacOS CI gatings.
> > Use the migrate_ensure_converge() helper too in the auto-converge as when
> > Daniel reworked the other test cases.
> > 
> > Since both max_bandwidth / downtime_limit will not be used for converge
> > calculations, make it simple by removing the remaining check, then we can
> > completely remove both variables altogether, since migrate_ensure_converge
> > is used the remaining check won't make much sense anyway.
> > 
> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   tests/qtest/migration-test.c | 17 +----------------
> >   1 file changed, 1 insertion(+), 16 deletions(-)
> > 
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index 71595a74fd..dd50aa600c 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -1776,14 +1776,6 @@ static void test_migrate_auto_converge(void)
> >        * so we need to decrease a bandwidth.
> >        */
> >       const int64_t init_pct = 5, inc_pct = 50, max_pct = 95;
> > -    const int64_t max_bandwidth = 400000000; /* ~400Mb/s */
> > -    const int64_t downtime_limit = 250; /* 250ms */
> > -    /*
> > -     * We migrate through unix-socket (> 500Mb/s).
> > -     * Thus, expected migration speed ~= bandwidth limit (< 500Mb/s).
> > -     * So, we can predict expected_threshold
> > -     */
> > -    const int64_t expected_threshold = max_bandwidth * downtime_limit / 1000;
> >       if (test_migrate_start(&from, &to, uri, &args)) {
> >           return;
> > @@ -1818,8 +1810,7 @@ static void test_migrate_auto_converge(void)
> >       /* The first percentage of throttling should be equal to init_pct */
> >       g_assert_cmpint(percentage, ==, init_pct);
> >       /* Now, when we tested that throttling works, let it converge */
> > -    migrate_set_parameter_int(from, "downtime-limit", downtime_limit);
> > -    migrate_set_parameter_int(from, "max-bandwidth", max_bandwidth);
> > +    migrate_ensure_converge(from);
> >       /*
> >        * Wait for pre-switchover status to check last throttle percentage
> > @@ -1830,11 +1821,6 @@ static void test_migrate_auto_converge(void)
> >       /* The final percentage of throttling shouldn't be greater than max_pct */
> >       percentage = read_migrate_property_int(from, "cpu-throttle-percentage");
> >       g_assert_cmpint(percentage, <=, max_pct);
> > -
> > -    remaining = read_ram_property_int(from, "remaining");
> > -    g_assert_cmpint(remaining, <,
> > -                    (expected_threshold + expected_threshold / 100));
> > -
> 
> I'm getting this on FreeBSD:
> 
> ../tests/qtest/migration-test.c:1771:13: error: unused variable 'remaining'
> [-Werror,-Wunused-variable]
>     int64_t remaining, percentage;
>             ^
> 1 error generated.
> 
> Did you try this with -Werror ?

Thanks for catching that, I'll start to do so and repost.

-- 
Peter Xu

[PATCH v2 2/2] migration-test: Allow test to run without uffd

[Peter Xu]

We used to stop running all tests if uffd is not detected.  However
logically that's only needed for postcopy not the rest of tests.

Keep running the rest when still possible.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qtest/migration-test.c | 48 +++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index dd50aa600c..8826ee4be4 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2424,14 +2424,11 @@ int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
     const bool has_kvm = qtest_has_accel("kvm");
+    const bool has_uffd = ufd_version_check();
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
-    if (!ufd_version_check()) {
-        return g_test_run();
-    }
-
     /*
      * 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
@@ -2460,13 +2457,15 @@ int main(int argc, char **argv)
 
     module_call_init(MODULE_INIT_QOM);
 
-    qtest_add_func("/migration/postcopy/unix", test_postcopy);
-    qtest_add_func("/migration/postcopy/plain", test_postcopy);
-    qtest_add_func("/migration/postcopy/recovery/plain",
-                   test_postcopy_recovery);
-    qtest_add_func("/migration/postcopy/preempt/plain", test_postcopy_preempt);
-    qtest_add_func("/migration/postcopy/preempt/recovery/plain",
-                    test_postcopy_preempt_recovery);
+    if (has_uffd) {
+        qtest_add_func("/migration/postcopy/unix", test_postcopy);
+        qtest_add_func("/migration/postcopy/plain", test_postcopy);
+        qtest_add_func("/migration/postcopy/recovery/plain",
+                       test_postcopy_recovery);
+        qtest_add_func("/migration/postcopy/preempt/plain", test_postcopy_preempt);
+        qtest_add_func("/migration/postcopy/preempt/recovery/plain",
+                       test_postcopy_preempt_recovery);
+    }
 
     qtest_add_func("/migration/bad_dest", test_baddest);
     qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
@@ -2474,18 +2473,21 @@ int main(int argc, char **argv)
 #ifdef CONFIG_GNUTLS
     qtest_add_func("/migration/precopy/unix/tls/psk",
                    test_precopy_unix_tls_psk);
-    /*
-     * NOTE: psk test is enough for postcopy, as other types of TLS
-     * channels are tested under precopy.  Here what we want to test is the
-     * general postcopy path that has TLS channel enabled.
-     */
-    qtest_add_func("/migration/postcopy/tls/psk", test_postcopy_tls_psk);
-    qtest_add_func("/migration/postcopy/recovery/tls/psk",
-                   test_postcopy_recovery_tls_psk);
-    qtest_add_func("/migration/postcopy/preempt/tls/psk",
-                   test_postcopy_preempt_tls_psk);
-    qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
-                   test_postcopy_preempt_all);
+
+    if (has_uffd) {
+        /*
+         * NOTE: psk test is enough for postcopy, as other types of TLS
+         * channels are tested under precopy.  Here what we want to test is the
+         * general postcopy path that has TLS channel enabled.
+         */
+        qtest_add_func("/migration/postcopy/tls/psk", test_postcopy_tls_psk);
+        qtest_add_func("/migration/postcopy/recovery/tls/psk",
+                       test_postcopy_recovery_tls_psk);
+        qtest_add_func("/migration/postcopy/preempt/tls/psk",
+                       test_postcopy_preempt_tls_psk);
+        qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
+                       test_postcopy_preempt_all);
+    }
 #ifdef CONFIG_TASN1
     qtest_add_func("/migration/precopy/unix/tls/x509/default-host",
                    test_precopy_unix_tls_x509_default_host);
-- 
2.32.0

Re: [PATCH v2 2/2] migration-test: Allow test to run without uffd

[Daniel P. Berrangé]

On Fri, Jul 22, 2022 at 10:56:54AM -0400, Peter Xu wrote:
> We used to stop running all tests if uffd is not detected.  However
> logically that's only needed for postcopy not the rest of tests.
> 
> Keep running the rest when still possible.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tests/qtest/migration-test.c | 48 +++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 23 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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 :|