Re: [PATCH v9 3/3] iotests: test nbd reconnect

[Vladimir Sementsov-Ogievskiy]

23.09.2019 22:51, Eric Blake wrote:
> On 9/17/19 12:13 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Add test, which starts backup to nbd target and restarts nbd server
>> during backup.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   tests/qemu-iotests/264        | 65 +++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/264.out    | 12 +++++++
>>   tests/qemu-iotests/group      |  1 +
>>   tests/qemu-iotests/iotests.py |  4 +++
>>   4 files changed, 82 insertions(+)
>>   create mode 100755 tests/qemu-iotests/264
>>   create mode 100644 tests/qemu-iotests/264.out
>>
> 
>> +import time
>> +
>> +import iotests
>> +from iotests import qemu_img_create, file_path, qemu_nbd_popen, log
>> +
>> +disk_a, disk_b, nbd_sock = file_path('disk_a', 'disk_b', 'nbd-sock')
>> +
>> +qemu_img_create('-f', iotests.imgfmt, disk_a, '5M')
>> +qemu_img_create('-f', iotests.imgfmt, disk_b, '5M')
>> +srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
>> +time.sleep(1)
>> +
> 
> Hard-coded sleep times can be problematic (too long, and the test is no
> longer quick; too short, and heavy load can interfere); is there any way
> to poll for a completion event rather than just waiting one second?

Hmm, I think qemu-nbd don't send events.. Here I just want to give it a time
to start. Possibly we can enable tracing and wait for some trace-point, but I'm
not sure that this is good idea. And sleep is simpler.

> 
>> +vm = iotests.VM().add_drive(disk_a)
>> +vm.launch()
>> +vm.hmp_qemu_io('drive0', 'write 0 5M')
>> +
>> +vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
>> +           **{'node_name': 'backup0',
>> +              'driver': 'raw',
>> +              'file': {'driver': 'nbd',
>> +                       'server': {'type': 'unix', 'path': nbd_sock},
>> +                       'reconnect-delay': 10}})
>> +vm.qmp_log('blockdev-backup', device='drive0', sync='full', 
>> target='backup0',
>> +           speed=(1 * 1024 * 1024))
> 
> Throttling to make sure we are likely to still be going and thus test
> the reconnect logic.
> 
>> +
>> +time.sleep(1)
>> +log('Kill NBD server')
>> +srv.kill()
> 
> Again, that hard-coded sleep looks a bit risky.

Hmm, you are right it's definitely risky.. I forget, that in down-stream 
virtuozzo
branch this place is rewritten to be smarter, I'll resend updated test, replying
to this thread.

> 
>> +
>> +jobs = vm.qmp('query-block-jobs')['return']
>> +if jobs and jobs[0]['offset'] < jobs[0]['len']:
>> +    log('Backup job is still in progress')
>> +
>> +time.sleep(1)
> 
> Why do we need yet another sleep?

The idea is to emulate that server is down for some time. It may be better
to disable throttling before this sleep, to give backup more chance to create
requests during server down-time.

> 
>> +
>> +log('Start NBD server')
>> +srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
>> +
>> +vm.qmp_log('block-job-set-speed', device='drive0', speed=0)
> 
> So here, you release the throttle to finish the job.
> 
>> +e = vm.event_wait('BLOCK_JOB_COMPLETED')
>> +log('Backup completed: {}'.format(e['data']['offset']))
>> +
>> +vm.qmp_log('blockdev-del', node_name='backup0')
>> +srv.kill()
>> +vm.shutdown()
>> diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out
>> new file mode 100644
>> index 0000000000..4a2f4aa509
>> --- /dev/null
>> +++ b/tests/qemu-iotests/264.out
>> @@ -0,0 +1,12 @@
>> +{"execute": "blockdev-add", "arguments": {"driver": "raw", "file": 
>> {"driver": "nbd", "reconnect-delay": 10, "server": {"path": 
>> "TEST_DIR/PID-nbd-sock", "type": "unix"}}, "node-name": "backup0"}}
>> +{"return": {}}
>> +{"execute": "blockdev-backup", "arguments": {"device": "drive0", "speed": 
>> 1048576, "sync": "full", "target": "backup0"}}
>> +{"return": {}}
>> +Kill NBD server
>> +Backup job is still in progress
>> +Start NBD server
>> +{"execute": "block-job-set-speed", "arguments": {"device": "drive0", 
>> "speed": 0}}
>> +{"return": {}}
>> +Backup completed: 5242880
>> +{"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
>> +{"return": {}}
>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>> index 5d3da937e4..4f6dd6f153 100644
>> --- a/tests/qemu-iotests/group
>> +++ b/tests/qemu-iotests/group
>> @@ -275,5 +275,6 @@
>>   258 rw quick
>>   262 rw quick migration
>>   263 rw quick
>> +264 rw quick
> 
> With that many hard-coded sleeps, is it still quick?
> 
> /me applies the patch and runs it...
> 
> 264      pass       [14:49:55] [14:50:01]   6s
> 
> that's borderline enough that I would not call it quick.
> 
>>   
>> +def qemu_nbd_popen(*args):
>> +    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
>> +    return subprocess.Popen(qemu_nbd_args + ['--persistent'] + list(args))
>> +
> 
> Should you also use a pid file here, and wait for the existence of the
> pid file before returning (rather than hard-coding sleep(1))?

What do you mean / how to do it?

We want to wait until listening socket is prepared..

> 
>>   def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
>>       '''Return True if two image files are identical'''
>>       return qemu_img('compare', '-f', fmt1,
>>
> 
> At any rate,
> 
> Tested-by: Eric Blake <address@hidden>
> 


-- 
Best regards,
Vladimir