[PATCH 0/2] migration test tweeks

[PATCH 0/2] migration test tweeks

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Alex noticed that some of the postcopy tests would occasionally
hang; this series adds some checks to make them more likely
to assert than hang in some failure cases, and changes
the migration bandwidth so that under load it's much more likely
to manage to land in postcopy.

Dr. David Alan Gilbert (2):
  tests/migration: Fail on unexpected migration states
  tests/migration/postcopy: trim migration bandwidth

 tests/migration-test.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

-- 
2.21.0

[PATCH 1/2] tests/migration: Fail on unexpected migration states

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

We've got various places where we wait for a migration to enter
a given state; but if we enter an unexpected state we tend to fail
in odd ways; add a mechanism for explicitly testing for any state
which we shouldn't be in.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tests/migration-test.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 258aa064d4..9c62ee5331 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -255,15 +255,19 @@ static void read_blocktime(QTestState *who)
 }
 
 static void wait_for_migration_status(QTestState *who,
-                                      const char *goal)
+                                      const char *goal,
+                                      const char **ungoals)
 {
     while (true) {
         bool completed;
         char *status;
+        const char **ungoal;
 
         status = migrate_query_status(who);
         completed = strcmp(status, goal) == 0;
-        g_assert_cmpstr(status, !=,  "failed");
+        for (ungoal = ungoals; *ungoal; ungoal++) {
+            g_assert_cmpstr(status, !=,  *ungoal);
+        }
         g_free(status);
         if (completed) {
             return;
@@ -274,7 +278,8 @@ static void wait_for_migration_status(QTestState *who,
 
 static void wait_for_migration_complete(QTestState *who)
 {
-    wait_for_migration_status(who, "completed");
+    wait_for_migration_status(who, "completed",
+                              (const char * []) { "failed", NULL });
 }
 
 static void wait_for_migration_pass(QTestState *who)
@@ -809,7 +814,9 @@ static void test_postcopy_recovery(void)
      * Wait until postcopy is really started; we can only run the
      * migrate-pause command during a postcopy
      */
-    wait_for_migration_status(from, "postcopy-active");
+    wait_for_migration_status(from, "postcopy-active",
+                              (const char * []) { "failed",
+                                                  "completed", NULL });
 
     /*
      * Manually stop the postcopy migration. This emulates a network
@@ -822,7 +829,9 @@ static void test_postcopy_recovery(void)
      * migrate-recover command can only succeed if destination machine
      * is in the paused state
      */
-    wait_for_migration_status(to, "postcopy-paused");
+    wait_for_migration_status(to, "postcopy-paused",
+                              (const char * []) { "failed", "active",
+                                                  "completed", NULL });
 
     /*
      * Create a new socket to emulate a new channel that is different
@@ -836,7 +845,9 @@ static void test_postcopy_recovery(void)
      * Try to rebuild the migration channel using the resume flag and
      * the newly created channel
      */
-    wait_for_migration_status(from, "postcopy-paused");
+    wait_for_migration_status(from, "postcopy-paused",
+                              (const char * []) { "failed", "active",
+                                                  "completed", NULL });
     migrate(from, uri, "{'resume': true}");
     g_free(uri);
 
-- 
2.21.0

Re: [PATCH 1/2] tests/migration: Fail on unexpected migration states

[Cleber Rosa]

On Mon, Sep 23, 2019 at 02:10:21PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> We've got various places where we wait for a migration to enter
> a given state; but if we enter an unexpected state we tend to fail
> in odd ways; add a mechanism for explicitly testing for any state
> which we shouldn't be in.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  tests/migration-test.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 258aa064d4..9c62ee5331 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -255,15 +255,19 @@ static void read_blocktime(QTestState *who)
>  }
>  
>  static void wait_for_migration_status(QTestState *who,
> -                                      const char *goal)
> +                                      const char *goal,
> +                                      const char **ungoals)
>  {
>      while (true) {
>          bool completed;
>          char *status;
> +        const char **ungoal;
>  
>          status = migrate_query_status(who);
>          completed = strcmp(status, goal) == 0;
> -        g_assert_cmpstr(status, !=,  "failed");
> +        for (ungoal = ungoals; *ungoal; ungoal++) {
> +            g_assert_cmpstr(status, !=,  *ungoal);
> +        }
>          g_free(status);
>          if (completed) {
>              return;
> @@ -274,7 +278,8 @@ static void wait_for_migration_status(QTestState *who,
>  
>  static void wait_for_migration_complete(QTestState *who)
>  {
> -    wait_for_migration_status(who, "completed");
> +    wait_for_migration_status(who, "completed",
> +                              (const char * []) { "failed", NULL });
>  }
>  
>  static void wait_for_migration_pass(QTestState *who)
> @@ -809,7 +814,9 @@ static void test_postcopy_recovery(void)
>       * Wait until postcopy is really started; we can only run the
>       * migrate-pause command during a postcopy
>       */
> -    wait_for_migration_status(from, "postcopy-active");
> +    wait_for_migration_status(from, "postcopy-active",
> +                              (const char * []) { "failed",
> +                                                  "completed", NULL });
>  
>      /*
>       * Manually stop the postcopy migration. This emulates a network
> @@ -822,7 +829,9 @@ static void test_postcopy_recovery(void)
>       * migrate-recover command can only succeed if destination machine
>       * is in the paused state
>       */
> -    wait_for_migration_status(to, "postcopy-paused");
> +    wait_for_migration_status(to, "postcopy-paused",
> +                              (const char * []) { "failed", "active",
> +                                                  "completed", NULL });
>  
>      /*
>       * Create a new socket to emulate a new channel that is different
> @@ -836,7 +845,9 @@ static void test_postcopy_recovery(void)
>       * Try to rebuild the migration channel using the resume flag and
>       * the newly created channel
>       */
> -    wait_for_migration_status(from, "postcopy-paused");
> +    wait_for_migration_status(from, "postcopy-paused",
> +                              (const char * []) { "failed", "active",
> +                                                  "completed", NULL });
>      migrate(from, uri, "{'resume': true}");
>      g_free(uri);
>  
> -- 
> 2.21.0
> 
>

LGTM (tested with x86_64 and aarch64 targets on an x86_64 host).

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

Re: [PATCH 1/2] tests/migration: Fail on unexpected migration states

[Alex Bennée]

Dr. David Alan Gilbert (git) <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> We've got various places where we wait for a migration to enter
> a given state; but if we enter an unexpected state we tend to fail
> in odd ways; add a mechanism for explicitly testing for any state
> which we shouldn't be in.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  tests/migration-test.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 258aa064d4..9c62ee5331 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -255,15 +255,19 @@ static void read_blocktime(QTestState *who)
>  }
>
>  static void wait_for_migration_status(QTestState *who,
> -                                      const char *goal)
> +                                      const char *goal,
> +                                      const char **ungoals)
>  {
>      while (true) {
>          bool completed;
>          char *status;
> +        const char **ungoal;
>
>          status = migrate_query_status(who);
>          completed = strcmp(status, goal) == 0;
> -        g_assert_cmpstr(status, !=,  "failed");
> +        for (ungoal = ungoals; *ungoal; ungoal++) {
> +            g_assert_cmpstr(status, !=,  *ungoal);
> +        }

You could use:

  g_assert(!g_strv_contains(ungoals, status))

if you wanted to be more gliby. But anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

>          g_free(status);
>          if (completed) {
>              return;
> @@ -274,7 +278,8 @@ static void wait_for_migration_status(QTestState *who,
>
>  static void wait_for_migration_complete(QTestState *who)
>  {
> -    wait_for_migration_status(who, "completed");
> +    wait_for_migration_status(who, "completed",
> +                              (const char * []) { "failed", NULL });
>  }
>
>  static void wait_for_migration_pass(QTestState *who)
> @@ -809,7 +814,9 @@ static void test_postcopy_recovery(void)
>       * Wait until postcopy is really started; we can only run the
>       * migrate-pause command during a postcopy
>       */
> -    wait_for_migration_status(from, "postcopy-active");
> +    wait_for_migration_status(from, "postcopy-active",
> +                              (const char * []) { "failed",
> +                                                  "completed", NULL });
>
>      /*
>       * Manually stop the postcopy migration. This emulates a network
> @@ -822,7 +829,9 @@ static void test_postcopy_recovery(void)
>       * migrate-recover command can only succeed if destination machine
>       * is in the paused state
>       */
> -    wait_for_migration_status(to, "postcopy-paused");
> +    wait_for_migration_status(to, "postcopy-paused",
> +                              (const char * []) { "failed", "active",
> +                                                  "completed", NULL });
>
>      /*
>       * Create a new socket to emulate a new channel that is different
> @@ -836,7 +845,9 @@ static void test_postcopy_recovery(void)
>       * Try to rebuild the migration channel using the resume flag and
>       * the newly created channel
>       */
> -    wait_for_migration_status(from, "postcopy-paused");
> +    wait_for_migration_status(from, "postcopy-paused",
> +                              (const char * []) { "failed", "active",
> +                                                  "completed", NULL });
>      migrate(from, uri, "{'resume': true}");
>      g_free(uri);


--
Alex Bennée

Re: [PATCH 1/2] tests/migration: Fail on unexpected migration states

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> We've got various places where we wait for a migration to enter
> a given state; but if we enter an unexpected state we tend to fail
> in odd ways; add a mechanism for explicitly testing for any state
> which we shouldn't be in.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

> -    wait_for_migration_status(from, "postcopy-active");
> +    wait_for_migration_status(from, "postcopy-active",
> +                              (const char * []) { "failed",
> +                                                  "completed", NULL });
>  

I really preffer not to use that kind of casts.

const char *ungoals[] = { "failed", "completed", NULL };

but as it is you who are doing the patches, I will not complain O:-)

[PATCH 2/2] tests/migration/postcopy: trim migration bandwidth

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

On slow hosts with tcg we were sometimes finding that the migration
would complete during precopy and never get into the postcopy test.
Trim back the bandwidth a bit to make that much less likely.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tests/migration-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 9c62ee5331..221a33d083 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -753,7 +753,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
      * quickly, but that it doesn't complete precopy even on a slow
      * machine, so also set the downtime.
      */
-    migrate_set_parameter_int(from, "max-bandwidth", 100000000);
+    migrate_set_parameter_int(from, "max-bandwidth", 30000000);
     migrate_set_parameter_int(from, "downtime-limit", 1);
 
     /* Wait for the first serial output from the source */
-- 
2.21.0

Re: [PATCH 2/2] tests/migration/postcopy: trim migration bandwidth

[Cleber Rosa]

On Mon, Sep 23, 2019 at 02:10:22PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> On slow hosts with tcg we were sometimes finding that the migration
> would complete during precopy and never get into the postcopy test.
> Trim back the bandwidth a bit to make that much less likely.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  tests/migration-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 9c62ee5331..221a33d083 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -753,7 +753,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>       * quickly, but that it doesn't complete precopy even on a slow
>       * machine, so also set the downtime.
>       */
> -    migrate_set_parameter_int(from, "max-bandwidth", 100000000);
> +    migrate_set_parameter_int(from, "max-bandwidth", 30000000);
>      migrate_set_parameter_int(from, "downtime-limit", 1);
>  
>      /* Wait for the first serial output from the source */
> -- 
> 2.21.0
> 
> 

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

Re: [PATCH 2/2] tests/migration/postcopy: trim migration bandwidth

[Alex Bennée]

Dr. David Alan Gilbert (git) <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> On slow hosts with tcg we were sometimes finding that the migration
> would complete during precopy and never get into the postcopy test.
> Trim back the bandwidth a bit to make that much less likely.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Acked-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  tests/migration-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 9c62ee5331..221a33d083 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -753,7 +753,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>       * quickly, but that it doesn't complete precopy even on a slow
>       * machine, so also set the downtime.
>       */
> -    migrate_set_parameter_int(from, "max-bandwidth", 100000000);
> +    migrate_set_parameter_int(from, "max-bandwidth", 30000000);
>      migrate_set_parameter_int(from, "downtime-limit", 1);
>
>      /* Wait for the first serial output from the source */


--
Alex Bennée

Re: [PATCH 2/2] tests/migration/postcopy: trim migration bandwidth

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> On slow hosts with tcg we were sometimes finding that the migration
> would complete during precopy and never get into the postcopy test.
> Trim back the bandwidth a bit to make that much less likely.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 0/2] migration test tweeks

[Peter Xu]

On Mon, Sep 23, 2019 at 02:10:20PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Alex noticed that some of the postcopy tests would occasionally
> hang; this series adds some checks to make them more likely
> to assert than hang in some failure cases, and changes
> the migration bandwidth so that under load it's much more likely
> to manage to land in postcopy.
> 
> Dr. David Alan Gilbert (2):
>   tests/migration: Fail on unexpected migration states
>   tests/migration/postcopy: trim migration bandwidth

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

I might be even more aggresive on patch 2 when turning down the
bandwidth. :)

Another thing I thought about on the hang issue is that maybe we can
give a timeout for the waits and when the timeout triggers before a
directly assert in the test case we send sigabrt to QEMU (just like
what kill_qemu does) then we could have a chance to see the cores.
Not sure whether that could help, though.

Regards,

-- 
Peter Xu

Re: [PATCH 0/2] migration test tweeks

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Mon, Sep 23, 2019 at 02:10:20PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Alex noticed that some of the postcopy tests would occasionally
> > hang; this series adds some checks to make them more likely
> > to assert than hang in some failure cases, and changes
> > the migration bandwidth so that under load it's much more likely
> > to manage to land in postcopy.
> > 
> > Dr. David Alan Gilbert (2):
> >   tests/migration: Fail on unexpected migration states
> >   tests/migration/postcopy: trim migration bandwidth
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> I might be even more aggresive on patch 2 when turning down the
> bandwidth. :)
> 
> Another thing I thought about on the hang issue is that maybe we can
> give a timeout for the waits and when the timeout triggers before a
> directly assert in the test case we send sigabrt to QEMU (just like
> what kill_qemu does) then we could have a chance to see the cores.
> Not sure whether that could help, though.

Yes I might need to add that; I can see there's another hang somewhere
even with these two patches.
(I *think* it's at the end while it's waiting for completion after
resumption - but I've not figured out what's going on yet).

Dave

> Regards,
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/2] migration test tweeks

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (git) (dgilbert@redhat.com) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Alex noticed that some of the postcopy tests would occasionally
> hang; this series adds some checks to make them more likely
> to assert than hang in some failure cases, and changes
> the migration bandwidth so that under load it's much more likely
> to manage to land in postcopy.

Queued

> 
> Dr. David Alan Gilbert (2):
>   tests/migration: Fail on unexpected migration states
>   tests/migration/postcopy: trim migration bandwidth
> 
>  tests/migration-test.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> -- 
> 2.21.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK