Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager

[Nir Soffer]

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.

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

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
>