[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager
|
From: |
Nir Soffer |
|
Subject: |
Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager |
|
Date: |
Wed, 5 Aug 2020 11:47:18 +0300 |
On Wed, Aug 5, 2020 at 10:38 AM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 28.07.2020 19:05, Nir Soffer wrote:
> > On Tue, Jul 28, 2020 at 4:43 PM Vladimir Sementsov-Ogievskiy
> > <vsementsov@virtuozzo.com> wrote:
> >>
> >> 28.07.2020 00:58, Nir Soffer wrote:
> >>> Instead of duplicating the code to wait until the server is ready and
> >>> remember to terminate the server and wait for it, make it possible to
> >>> use like this:
> >>>
> >>> with qemu_nbd_popen('-k', sock, image):
> >>> # Access image via qemu-nbd socket...
> >>>
> >>> Only test 264 used this helper, but I had to modify the output since it
> >>> did not consistently when starting and stopping qemu-nbd.
> >>>
> >>> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> >>> ---
> >>> tests/qemu-iotests/264 | 76 +++++++++++++----------------------
> >>> tests/qemu-iotests/264.out | 2 +
> >>> tests/qemu-iotests/iotests.py | 28 ++++++++++++-
> >>> 3 files changed, 56 insertions(+), 50 deletions(-)
> >>>
> >>> diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
> >>> index 304a7443d7..666f164ed8 100755
> >>> --- a/tests/qemu-iotests/264
> >>> +++ b/tests/qemu-iotests/264
> >>> @@ -36,48 +36,32 @@ wait_step = 0.2
> >>>
> >>> qemu_img_create('-f', iotests.imgfmt, disk_a, str(size))
> >>> qemu_img_create('-f', iotests.imgfmt, disk_b, str(size))
> >>> -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
> >>>
> >>> -# Wait for NBD server availability
> >>> -t = 0
> >>> -ok = False
> >>> -while t < wait_limit:
> >>> - ok = qemu_io_silent_check('-f', 'raw', '-c', 'read 0 512', nbd_uri)
> >>> - if ok:
> >>> - break
> >>> - time.sleep(wait_step)
> >>> - t += wait_step
> >>> +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
> >>> + vm = iotests.VM().add_drive(disk_a)
> >>> + vm.launch()
> >>> + vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
> >>> +
> >>> + 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))
> >>> +
> >>> + # Wait for some progress
> >>> + t = 0
> >>> + while t < wait_limit:
> >>> + jobs = vm.qmp('query-block-jobs')['return']
> >>> + if jobs and jobs[0]['offset'] > 0:
> >>> + break
> >>> + time.sleep(wait_step)
> >>> + t += wait_step
> >>>
> >>> -assert ok
> >>> -
> >>> -vm = iotests.VM().add_drive(disk_a)
> >>> -vm.launch()
> >>> -vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
> >>> -
> >>> -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))
> >>> -
> >>> -# Wait for some progress
> >>> -t = 0
> >>> -while t < wait_limit:
> >>> - jobs = vm.qmp('query-block-jobs')['return']
> >>> if jobs and jobs[0]['offset'] > 0:
> >>> - break
> >>> - time.sleep(wait_step)
> >>> - t += wait_step
> >>> -
> >>> -if jobs and jobs[0]['offset'] > 0:
> >>> - log('Backup job is started')
> >>> -
> >>> -log('Kill NBD server')
> >>> -srv.kill()
> >>> -srv.wait()
> >>> + log('Backup job is started')
> >>>
> >>> jobs = vm.qmp('query-block-jobs')['return']
> >>> if jobs and jobs[0]['offset'] < jobs[0]['len']:
> >>> @@ -88,12 +72,8 @@ vm.qmp_log('block-job-set-speed', device='drive0',
> >>> speed=0)
> >>> # Emulate server down time for 1 second
> >>> time.sleep(1)
> >>>
> >>> -log('Start NBD server')
> >>> -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
> >>> -
> >>> -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()
> >>> +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
> >>> + e = vm.event_wait('BLOCK_JOB_COMPLETED')
> >>> + log('Backup completed: {}'.format(e['data']['offset']))
> >>> + vm.qmp_log('blockdev-del', node_name='backup0')
> >>> + vm.shutdown()
> >>> diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out
> >>> index 3000944b09..c45b1e81ef 100644
> >>> --- a/tests/qemu-iotests/264.out
> >>> +++ b/tests/qemu-iotests/264.out
> >>> @@ -1,3 +1,4 @@
> >>> +Start NBD server
> >>> {"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"}}
> >>> @@ -11,3 +12,4 @@ Start NBD server
> >>> Backup completed: 5242880
> >>> {"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
> >>> {"return": {}}
> >>> +Kill NBD server
> >>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >>> index 3590ed78a0..8f79668435 100644
> >>> --- a/tests/qemu-iotests/iotests.py
> >>> +++ b/tests/qemu-iotests/iotests.py
> >>> @@ -28,10 +28,13 @@ import signal
> >>> import struct
> >>> import subprocess
> >>> import sys
> >>> +import time
> >>> from typing import (Any, Callable, Dict, Iterable,
> >>> List, Optional, Sequence, Tuple, TypeVar)
> >>> import unittest
> >>>
> >>> +from contextlib import contextmanager
> >>> +
> >>> # pylint: disable=import-error, wrong-import-position
> >>> sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
> >>> 'python'))
> >>> from qemu import qtest
> >>> @@ -270,9 +273,30 @@ def qemu_nbd_early_pipe(*args):
> >>>
> >>> return subp.returncode, output if subp.returncode else ''
> >>>
> >>> +@contextmanager
> >>> 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))
> >>> + '''Context manager running qemu-nbd within the context'''
> >>
> >> PEP8 (or some another PEP referenced in PEP8) asks to use """ for
> >> doc-strings
> >
> > Both are valid, but I agree that """ is nicer.
>
> PEP 8:
>
> For triple-quoted strings, always use double quote characters to be
> consistent with the docstring convention in PEP 257
>
> PEP 257:
>
> For consistency, always use """triple double quotes""" around docstrings
These are good recommendations, but they are not enforced by the tools
like flake8.
Python treats ''' and """ as the same thing:
>>> def foo():
... '''
... This is my docstring.
... '''
...
>>> foo.__doc__
'\n\tThis is my docstring.\n\t'
We should use the recommended """, but this kind of change should be done for
the entire module.
I can send a patch for this later.
> > This module needs more work:
> >
> > $ flake8 --statistics --quiet tests/qemu-iotests/iotests.py
> > tests/qemu-iotests/iotests.py
> > 1 E261 at least two spaces before inline comment
> > 3 E301 expected 1 blank line, found 0
> > 64 E302 expected 2 blank lines, found 1
> > 1 E303 too many blank lines (2)
> > 5 E305 expected 2 blank lines after class or function definition, found
> > 1
> > 2 E402 module level import not at top of file
> >
> >>> + pid_file = file_path("pid")
> >>> +
> >>> + cmd = list(qemu_nbd_args)
> >>> + cmd.extend(('--persistent', '--pid-file', pid_file))
> >>> + cmd.extend(args)
> >>> +
> >>> + log('Start NBD server')
> >>> + p = subprocess.Popen(cmd)
> >>> + try:
> >>> + while not os.path.exists(pid_file):
> >>> + if p.poll() is not None:
> >>> + raise RuntimeError(
> >>> + "qemu-nbd terminated with exit code {}: {}"
> >>> + .format(p.returncode, ' '.join(cmd)))
> >>> +
> >>> + time.sleep(0.01)
> >>> + yield
> >>> + finally:
> >>> + log('Kill NBD server')
> >>> + p.kill()
> >>> + p.wait()
> >>
> >> why do we need try-finally? I think, the only possible exception is your
> >> "raise RuntimeError", and in this case the process is alredy dead, no need
> >> to kill it (and print the log message)
> >
> > The try-finally is needed for errors in user code like:
> >
> > with qemu_nbd_popen():
> > assert 0
> >
>
> Ahh, so this "finally" will work, on some exception during yield? OK then.
>
> > Without try-finally qemu-nbd will continue to run after the assert
> > fails, which may
> > cause trouble, depending on the test.
> >
> > In the case of the RuntimeError inside the try, the cleanup in finally is
> > not needed but harmless.
> >
> > Looking in python source:
> >
> > def send_signal(self, sig):
> > """Send a signal to the process."""
> > # Skip signalling a process that we know has already died.
> > if self.returncode is None:
> > os.kill(self.pid, sig)
> >
> > def kill(self):
> > """Kill the process with SIGKILL
> > """
> > self.send_signal(signal.SIGKILL)
> >
> > So the kill() will do nothing, and wait() will return immediately.
> >
> >>> def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
> >>> '''Return True if two image files are identical'''
> >>>
> >>
> >> anyway:
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>
> >> --
> >> Best regards,
> >> Vladimir
> >>
> >
>
>
> --
> Best regards,
> Vladimir
>