iotests: 194: wait migration completion on target too

[PATCH] iotests: 194: wait migration completion on target too

Posted by Vladimir Sementsov-Ogievskiy 3 years, 11 months ago

It is possible, that shutdown on target occurs earlier than migration
finish. In this case we crash in bdrv_release_dirty_bitmap_locked()
on assertion "assert(!bdrv_dirty_bitmap_busy(bitmap));" as we do have
busy bitmap, as bitmap migration is ongoing.

We'll fix bitmap migration to gracefully cancel on early shutdown soon.
Now let's fix iotest 194 to wait migration completion before shutdown.

Note that in this test dest_vm.shutdown() is called implicitly, as vms
used as context-providers, see __exit__() method of QEMUMachine class.

Actually, not waiting migration finish is a wrong thing, but the test
started to crash after commit ae00aa239847682
"iotests: 194: test also migration of dirty bitmap", which added dirty
bitmaps here. So, Fixes: tag won't hurt.

Fixes: ae00aa2398476824f0eca80461da215e7cdc1c3b
Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/194     | 10 ++++++++++
 tests/qemu-iotests/194.out |  5 +++++
 2 files changed, 15 insertions(+)

diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 3fad7c6c1a..6dc2bc94d7 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -87,4 +87,14 @@ with iotests.FilePath('source.img') as source_img_path, \
             iotests.log(dest_vm.qmp('nbd-server-stop'))
             break
 
+    iotests.log('Wait migration completion on target...')
+    migr_events = (('MIGRATION', {'data': {'status': 'completed'}}),
+                   ('MIGRATION', {'data': {'status': 'failed'}}))
+    event = dest_vm.events_wait(migr_events)
+    iotests.log(event, filters=[iotests.filter_qmp_event])
+
+    iotests.log('Check bitmaps on source:')
     iotests.log(source_vm.qmp('query-block')['return'][0]['dirty-bitmaps'])
+
+    iotests.log('Check bitmaps on target:')
+    iotests.log(dest_vm.qmp('query-block')['return'][0]['dirty-bitmaps'])
diff --git a/tests/qemu-iotests/194.out b/tests/qemu-iotests/194.out
index dd60dcc14f..f70cf7610e 100644
--- a/tests/qemu-iotests/194.out
+++ b/tests/qemu-iotests/194.out
@@ -21,4 +21,9 @@ Gracefully ending the `drive-mirror` job on source...
 {"data": {"device": "mirror-job0", "len": 1073741824, "offset": 1073741824, "speed": 0, "type": "mirror"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 Stopping the NBD server on destination...
 {"return": {}}
+Wait migration completion on target...
+{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+Check bitmaps on source:
+[{"busy": false, "count": 0, "granularity": 65536, "name": "bitmap0", "persistent": false, "recording": true, "status": "active"}]
+Check bitmaps on target:
 [{"busy": false, "count": 0, "granularity": 65536, "name": "bitmap0", "persistent": false, "recording": true, "status": "active"}]
-- 
2.21.0

Re: [PATCH] iotests: 194: wait migration completion on target too

Posted by Dr. David Alan Gilbert 3 years, 11 months ago

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> It is possible, that shutdown on target occurs earlier than migration
> finish. In this case we crash in bdrv_release_dirty_bitmap_locked()
> on assertion "assert(!bdrv_dirty_bitmap_busy(bitmap));" as we do have
> busy bitmap, as bitmap migration is ongoing.
> 
> We'll fix bitmap migration to gracefully cancel on early shutdown soon.
> Now let's fix iotest 194 to wait migration completion before shutdown.
> 
> Note that in this test dest_vm.shutdown() is called implicitly, as vms
> used as context-providers, see __exit__() method of QEMUMachine class.
> 
> Actually, not waiting migration finish is a wrong thing, but the test
> started to crash after commit ae00aa239847682
> "iotests: 194: test also migration of dirty bitmap", which added dirty
> bitmaps here. So, Fixes: tag won't hurt.

Without knowing the iotests framework, the actual problem sounds right;
I just fixed a similar bug in the acceptance test migration code.
Every migration test seems to make the same mistake!

Dave

> Fixes: ae00aa2398476824f0eca80461da215e7cdc1c3b
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/194     | 10 ++++++++++
>  tests/qemu-iotests/194.out |  5 +++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
> index 3fad7c6c1a..6dc2bc94d7 100755
> --- a/tests/qemu-iotests/194
> +++ b/tests/qemu-iotests/194
> @@ -87,4 +87,14 @@ with iotests.FilePath('source.img') as source_img_path, \
>              iotests.log(dest_vm.qmp('nbd-server-stop'))
>              break
>  
> +    iotests.log('Wait migration completion on target...')
> +    migr_events = (('MIGRATION', {'data': {'status': 'completed'}}),
> +                   ('MIGRATION', {'data': {'status': 'failed'}}))
> +    event = dest_vm.events_wait(migr_events)
> +    iotests.log(event, filters=[iotests.filter_qmp_event])
> +
> +    iotests.log('Check bitmaps on source:')
>      iotests.log(source_vm.qmp('query-block')['return'][0]['dirty-bitmaps'])
> +
> +    iotests.log('Check bitmaps on target:')
> +    iotests.log(dest_vm.qmp('query-block')['return'][0]['dirty-bitmaps'])
> diff --git a/tests/qemu-iotests/194.out b/tests/qemu-iotests/194.out
> index dd60dcc14f..f70cf7610e 100644
> --- a/tests/qemu-iotests/194.out
> +++ b/tests/qemu-iotests/194.out
> @@ -21,4 +21,9 @@ Gracefully ending the `drive-mirror` job on source...
>  {"data": {"device": "mirror-job0", "len": 1073741824, "offset": 1073741824, "speed": 0, "type": "mirror"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>  Stopping the NBD server on destination...
>  {"return": {}}
> +Wait migration completion on target...
> +{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +Check bitmaps on source:
> +[{"busy": false, "count": 0, "granularity": 65536, "name": "bitmap0", "persistent": false, "recording": true, "status": "active"}]
> +Check bitmaps on target:
>  [{"busy": false, "count": 0, "granularity": 65536, "name": "bitmap0", "persistent": false, "recording": true, "status": "active"}]
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] iotests: 194: wait migration completion on target too

Posted by Eric Blake 3 years, 11 months ago

On 6/4/20 3:33 AM, Vladimir Sementsov-Ogievskiy wrote:
> It is possible, that shutdown on target occurs earlier than migration
> finish. In this case we crash in bdrv_release_dirty_bitmap_locked()
> on assertion "assert(!bdrv_dirty_bitmap_busy(bitmap));" as we do have
> busy bitmap, as bitmap migration is ongoing.
> 
> We'll fix bitmap migration to gracefully cancel on early shutdown soon.
> Now let's fix iotest 194 to wait migration completion before shutdown.
> 
> Note that in this test dest_vm.shutdown() is called implicitly, as vms
> used as context-providers, see __exit__() method of QEMUMachine class.
> 
> Actually, not waiting migration finish is a wrong thing, but the test
> started to crash after commit ae00aa239847682
> "iotests: 194: test also migration of dirty bitmap", which added dirty
> bitmaps here. So, Fixes: tag won't hurt.
> 
> Fixes: ae00aa2398476824f0eca80461da215e7cdc1c3b
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/194     | 10 ++++++++++
>   tests/qemu-iotests/194.out |  5 +++++
>   2 files changed, 15 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

Will queue through my NBD tree.

> 
> diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
> index 3fad7c6c1a..6dc2bc94d7 100755
> --- a/tests/qemu-iotests/194
> +++ b/tests/qemu-iotests/194
> @@ -87,4 +87,14 @@ with iotests.FilePath('source.img') as source_img_path, \
>               iotests.log(dest_vm.qmp('nbd-server-stop'))
>               break
>   
> +    iotests.log('Wait migration completion on target...')

Do you mind if I s/Wait/Wait for/ here and in the .out?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] iotests: 194: wait migration completion on target too

Posted by Vladimir Sementsov-Ogievskiy 3 years, 11 months ago

04.06.2020 15:04, Eric Blake wrote:
> On 6/4/20 3:33 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It is possible, that shutdown on target occurs earlier than migration
>> finish. In this case we crash in bdrv_release_dirty_bitmap_locked()
>> on assertion "assert(!bdrv_dirty_bitmap_busy(bitmap));" as we do have
>> busy bitmap, as bitmap migration is ongoing.
>>
>> We'll fix bitmap migration to gracefully cancel on early shutdown soon.
>> Now let's fix iotest 194 to wait migration completion before shutdown.
>>
>> Note that in this test dest_vm.shutdown() is called implicitly, as vms
>> used as context-providers, see __exit__() method of QEMUMachine class.
>>
>> Actually, not waiting migration finish is a wrong thing, but the test
>> started to crash after commit ae00aa239847682
>> "iotests: 194: test also migration of dirty bitmap", which added dirty
>> bitmaps here. So, Fixes: tag won't hurt.
>>
>> Fixes: ae00aa2398476824f0eca80461da215e7cdc1c3b
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/194     | 10 ++++++++++
>>   tests/qemu-iotests/194.out |  5 +++++
>>   2 files changed, 15 insertions(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Will queue through my NBD tree.
> 
>>
>> diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
>> index 3fad7c6c1a..6dc2bc94d7 100755
>> --- a/tests/qemu-iotests/194
>> +++ b/tests/qemu-iotests/194
>> @@ -87,4 +87,14 @@ with iotests.FilePath('source.img') as source_img_path, \
>>               iotests.log(dest_vm.qmp('nbd-server-stop'))
>>               break
>> +    iotests.log('Wait migration completion on target...')
> 
> Do you mind if I s/Wait/Wait for/ here and in the .out?
> 

No objections of course


-- 
Best regards,
Vladimir

Re: [PATCH] iotests: 194: wait migration completion on target too

Posted by Thomas Huth 3 years, 11 months ago

On 04/06/2020 10.33, Vladimir Sementsov-Ogievskiy wrote:
> It is possible, that shutdown on target occurs earlier than migration
> finish. In this case we crash in bdrv_release_dirty_bitmap_locked()
> on assertion "assert(!bdrv_dirty_bitmap_busy(bitmap));" as we do have
> busy bitmap, as bitmap migration is ongoing.
> 
> We'll fix bitmap migration to gracefully cancel on early shutdown soon.
> Now let's fix iotest 194 to wait migration completion before shutdown.
> 
> Note that in this test dest_vm.shutdown() is called implicitly, as vms
> used as context-providers, see __exit__() method of QEMUMachine class.
> 
> Actually, not waiting migration finish is a wrong thing, but the test
> started to crash after commit ae00aa239847682
> "iotests: 194: test also migration of dirty bitmap", which added dirty
> bitmaps here. So, Fixes: tag won't hurt.
> 
> Fixes: ae00aa2398476824f0eca80461da215e7cdc1c3b
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/194     | 10 ++++++++++
>  tests/qemu-iotests/194.out |  5 +++++
>  2 files changed, 15 insertions(+)

Thanks, this fixes the issue in the gitlab CI:

 https://gitlab.com/huth/qemu/-/jobs/580902477#L3780

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