tests/qtest: failover: fix infinite loop

If the migration is over before we cancel it, we are
waiting in a loop a state that never comes because the state
is already "completed".

To avoid an infinite loop, skip the test if the migration
is "completed" before we were able to cancel it.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 tests/qtest/virtio-net-failover.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

On 29/03/2022 14.42, Laurent Vivier wrote:
> If the migration is over before we cancel it, we are
> waiting in a loop a state that never comes because the state
> is already "completed".
> 
> To avoid an infinite loop, skip the test if the migration
> is "completed" before we were able to cancel it.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>   tests/qtest/virtio-net-failover.c | 29 +++++++++++++++++++++++++----
>   1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c
> index 80292eecf65f..78811f1c9216 100644
> --- a/tests/qtest/virtio-net-failover.c
> +++ b/tests/qtest/virtio-net-failover.c
> @@ -1141,6 +1141,11 @@ static void test_migrate_guest_off_abort(gconstpointer opaque)
>           ret = migrate_status(qts);
>   
>           status = qdict_get_str(ret, "status");
> +        if (strcmp(status, "completed") == 0) {
> +            g_test_skip("Failed to cancel the migration");
> +            qobject_unref(ret);
> +            goto out;
> +        }
>           if (strcmp(status, "active") == 0) {
>               qobject_unref(ret);
>               break;
> @@ -1155,8 +1160,12 @@ static void test_migrate_guest_off_abort(gconstpointer opaque)
>   
>       while (true) {
>           ret = migrate_status(qts);
> -
>           status = qdict_get_str(ret, "status");
> +        if (strcmp(status, "completed") == 0) {
> +            g_test_skip("Failed to cancel the migration");
> +            qobject_unref(ret);
> +            goto out;
> +        }
>           if (strcmp(status, "cancelled") == 0) {
>               qobject_unref(ret);
>               break;
> @@ -1169,6 +1178,7 @@ static void test_migrate_guest_off_abort(gconstpointer opaque)
>       check_one_card(qts, true, "standby0", MAC_STANDBY0);
>       check_one_card(qts, false, "primary0", MAC_PRIMARY0);
>   
> +out:
>       qos_object_destroy((QOSGraphObject *)vdev);
>       machine_stop(qts);
>   }
> @@ -1251,8 +1261,7 @@ static void test_migrate_abort_wait_unplug(gconstpointer opaque)
>               qobject_unref(ret);
>               break;
>           }
> -        g_assert_cmpstr(status, !=, "failed");
> -        g_assert_cmpstr(status, !=, "active");
> +        g_assert_cmpstr(status, ==, "cancelling");
>           qobject_unref(ret);
>       }
>   
> @@ -1324,11 +1333,11 @@ static void test_migrate_abort_active(gconstpointer opaque)
>           ret = migrate_status(qts);
>   
>           status = qdict_get_str(ret, "status");
> +        g_assert_cmpstr(status, !=, "failed");
>           if (strcmp(status, "wait-unplug") != 0) {
>               qobject_unref(ret);
>               break;
>           }
> -        g_assert_cmpstr(status, !=, "failed");
>           qobject_unref(ret);
>       }
>   
> @@ -1340,6 +1349,11 @@ static void test_migrate_abort_active(gconstpointer opaque)
>           ret = migrate_status(qts);
>   
>           status = qdict_get_str(ret, "status");
> +        if (strcmp(status, "completed") == 0) {
> +            g_test_skip("Failed to cancel the migration");
> +            qobject_unref(ret);
> +            goto out;
> +        }
>           if (strcmp(status, "cancelled") == 0) {
>               qobject_unref(ret);
>               break;
> @@ -1352,6 +1366,7 @@ static void test_migrate_abort_active(gconstpointer opaque)
>       check_one_card(qts, true, "standby0", MAC_STANDBY0);
>       check_one_card(qts, true, "primary0", MAC_PRIMARY0);
>   
> +out:
>       qos_object_destroy((QOSGraphObject *)vdev);
>       machine_stop(qts);
>   }
> @@ -1425,6 +1440,11 @@ static void test_migrate_off_abort(gconstpointer opaque)
>           ret = migrate_status(qts);
>   
>           status = qdict_get_str(ret, "status");
> +        if (strcmp(status, "completed") == 0) {
> +            g_test_skip("Failed to cancel the migration");
> +            qobject_unref(ret);
> +            goto out;
> +        }
>           if (strcmp(status, "cancelled") == 0) {
>               qobject_unref(ret);
>               break;
> @@ -1437,6 +1457,7 @@ static void test_migrate_off_abort(gconstpointer opaque)
>       check_one_card(qts, true, "standby0", MAC_STANDBY0);
>       check_one_card(qts, true, "primary0", MAC_PRIMARY0);
>   
> +out:
>       qos_object_destroy((QOSGraphObject *)vdev);
>       machine_stop(qts);
>   }

Acked-by: Thomas Huth <thuth@redhat.com>

Is this still urgent for 7.0, or can it wait for the 7.1 cycle?

  Thomas

On Tue, 29 Mar 2022 at 13:47, Thomas Huth <thuth@redhat.com> wrote:
>
> On 29/03/2022 14.42, Laurent Vivier wrote:
> > If the migration is over before we cancel it, we are
> > waiting in a loop a state that never comes because the state
> > is already "completed".
> >
> > To avoid an infinite loop, skip the test if the migration
> > is "completed" before we were able to cancel it.

> Is this still urgent for 7.0, or can it wait for the 7.1 cycle?

It's a test case change that fixes at least one hang I've seen
in "make check". I prefer those to go in, at least before rc3,
because the CI loop being unreliable makes the whole release
process slower and more annoying.

thanks
-- PMM

On 29/03/2022 15.23, Peter Maydell wrote:
> On Tue, 29 Mar 2022 at 13:47, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 29/03/2022 14.42, Laurent Vivier wrote:
>>> If the migration is over before we cancel it, we are
>>> waiting in a loop a state that never comes because the state
>>> is already "completed".
>>>
>>> To avoid an infinite loop, skip the test if the migration
>>> is "completed" before we were able to cancel it.
> 
>> Is this still urgent for 7.0, or can it wait for the 7.1 cycle?
> 
> It's a test case change that fixes at least one hang I've seen
> in "make check". I prefer those to go in, at least before rc3,
> because the CI loop being unreliable makes the whole release
> process slower and more annoying.

Ok. Do you want to pick it directly, or shall I create a pull request for 
this? (I don't have much else queued right now, that's why I ask)

  Thomas

On Tue, 29 Mar 2022 at 14:25, Thomas Huth <thuth@redhat.com> wrote:
>
> On 29/03/2022 15.23, Peter Maydell wrote:
> > On Tue, 29 Mar 2022 at 13:47, Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> On 29/03/2022 14.42, Laurent Vivier wrote:
> >>> If the migration is over before we cancel it, we are
> >>> waiting in a loop a state that never comes because the state
> >>> is already "completed".
> >>>
> >>> To avoid an infinite loop, skip the test if the migration
> >>> is "completed" before we were able to cancel it.
> >
> >> Is this still urgent for 7.0, or can it wait for the 7.1 cycle?
> >
> > It's a test case change that fixes at least one hang I've seen
> > in "make check". I prefer those to go in, at least before rc3,
> > because the CI loop being unreliable makes the whole release
> > process slower and more annoying.
>
> Ok. Do you want to pick it directly, or shall I create a pull request for
> this? (I don't have much else queued right now, that's why I ask)

I can apply it directly if you don't have anything else you're
sending anyway.

thanks
-- PMM

On 29/03/2022 15.27, Peter Maydell wrote:
> On Tue, 29 Mar 2022 at 14:25, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 29/03/2022 15.23, Peter Maydell wrote:
>>> On Tue, 29 Mar 2022 at 13:47, Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> On 29/03/2022 14.42, Laurent Vivier wrote:
>>>>> If the migration is over before we cancel it, we are
>>>>> waiting in a loop a state that never comes because the state
>>>>> is already "completed".
>>>>>
>>>>> To avoid an infinite loop, skip the test if the migration
>>>>> is "completed" before we were able to cancel it.
>>>
>>>> Is this still urgent for 7.0, or can it wait for the 7.1 cycle?
>>>
>>> It's a test case change that fixes at least one hang I've seen
>>> in "make check". I prefer those to go in, at least before rc3,
>>> because the CI loop being unreliable makes the whole release
>>> process slower and more annoying.
>>
>> Ok. Do you want to pick it directly, or shall I create a pull request for
>> this? (I don't have much else queued right now, that's why I ask)
> 
> I can apply it directly if you don't have anything else you're
> sending anyway.

Ok, then please apply directly! Thanks!

  Thomas

On Tue, 29 Mar 2022 at 13:43, Laurent Vivier <lvivier@redhat.com> wrote:
>
> If the migration is over before we cancel it, we are
> waiting in a loop a state that never comes because the state
> is already "completed".
>
> To avoid an infinite loop, skip the test if the migration
> is "completed" before we were able to cancel it.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  tests/qtest/virtio-net-failover.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)

Applied to master, thanks.

-- PMM

* Laurent Vivier (lvivier@redhat.com) wrote:
> If the migration is over before we cancel it, we are
> waiting in a loop a state that never comes because the state
> is already "completed".
> 
> To avoid an infinite loop, skip the test if the migration
> is "completed" before we were able to cancel it.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

If you're finding it's skipping to often, you might try setting the
migration bandwidth really low right at the start (a few bytes/second)
to ensure it doesn't complete under your feet.

Dave

> ---
>  tests/qtest/virtio-net-failover.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c
> index 80292eecf65f..78811f1c9216 100644
> --- a/tests/qtest/virtio-net-failover.c
> +++ b/tests/qtest/virtio-net-failover.c
> @@ -1141,6 +1141,11 @@ static void test_migrate_guest_off_abort(gconstpointer opaque)
>          ret = migrate_status(qts);
>  
>          status = qdict_get_str(ret, "status");
> +        if (strcmp(status, "completed") == 0) {
> +            g_test_skip("Failed to cancel the migration");
> +            qobject_unref(ret);
> +            goto out;
> +        }
>          if (strcmp(status, "active") == 0) {
>              qobject_unref(ret);
>              break;
> @@ -1155,8 +1160,12 @@ static void test_migrate_guest_off_abort(gconstpointer opaque)
>  
>      while (true) {
>          ret = migrate_status(qts);
> -
>          status = qdict_get_str(ret, "status");
> +        if (strcmp(status, "completed") == 0) {
> +            g_test_skip("Failed to cancel the migration");
> +            qobject_unref(ret);
> +            goto out;
> +        }
>          if (strcmp(status, "cancelled") == 0) {
>              qobject_unref(ret);
>              break;
> @@ -1169,6 +1178,7 @@ static void test_migrate_guest_off_abort(gconstpointer opaque)
>      check_one_card(qts, true, "standby0", MAC_STANDBY0);
>      check_one_card(qts, false, "primary0", MAC_PRIMARY0);
>  
> +out:
>      qos_object_destroy((QOSGraphObject *)vdev);
>      machine_stop(qts);
>  }
> @@ -1251,8 +1261,7 @@ static void test_migrate_abort_wait_unplug(gconstpointer opaque)
>              qobject_unref(ret);
>              break;
>          }
> -        g_assert_cmpstr(status, !=, "failed");
> -        g_assert_cmpstr(status, !=, "active");
> +        g_assert_cmpstr(status, ==, "cancelling");
>          qobject_unref(ret);
>      }
>  
> @@ -1324,11 +1333,11 @@ static void test_migrate_abort_active(gconstpointer opaque)
>          ret = migrate_status(qts);
>  
>          status = qdict_get_str(ret, "status");
> +        g_assert_cmpstr(status, !=, "failed");
>          if (strcmp(status, "wait-unplug") != 0) {
>              qobject_unref(ret);
>              break;
>          }
> -        g_assert_cmpstr(status, !=, "failed");
>          qobject_unref(ret);
>      }
>  
> @@ -1340,6 +1349,11 @@ static void test_migrate_abort_active(gconstpointer opaque)
>          ret = migrate_status(qts);
>  
>          status = qdict_get_str(ret, "status");
> +        if (strcmp(status, "completed") == 0) {
> +            g_test_skip("Failed to cancel the migration");
> +            qobject_unref(ret);
> +            goto out;
> +        }
>          if (strcmp(status, "cancelled") == 0) {
>              qobject_unref(ret);
>              break;
> @@ -1352,6 +1366,7 @@ static void test_migrate_abort_active(gconstpointer opaque)
>      check_one_card(qts, true, "standby0", MAC_STANDBY0);
>      check_one_card(qts, true, "primary0", MAC_PRIMARY0);
>  
> +out:
>      qos_object_destroy((QOSGraphObject *)vdev);
>      machine_stop(qts);
>  }
> @@ -1425,6 +1440,11 @@ static void test_migrate_off_abort(gconstpointer opaque)
>          ret = migrate_status(qts);
>  
>          status = qdict_get_str(ret, "status");
> +        if (strcmp(status, "completed") == 0) {
> +            g_test_skip("Failed to cancel the migration");
> +            qobject_unref(ret);
> +            goto out;
> +        }
>          if (strcmp(status, "cancelled") == 0) {
>              qobject_unref(ret);
>              break;
> @@ -1437,6 +1457,7 @@ static void test_migrate_off_abort(gconstpointer opaque)
>      check_one_card(qts, true, "standby0", MAC_STANDBY0);
>      check_one_card(qts, true, "primary0", MAC_PRIMARY0);
>  
> +out:
>      qos_object_destroy((QOSGraphObject *)vdev);
>      machine_stop(qts);
>  }
> -- 
> 2.35.1
>