Re: [PATCH v3] iotests: Test NBD client reconnection

[Andrey Shinkevich]

On 08/11/2019 16:49, Vladimir Sementsov-Ogievskiy wrote:
> 01.11.2019 19:54, Andrey Shinkevich wrote:
>> The test for an NBD client. The NBD server is disconnected after the
>> client write request. The NBD client should reconnect and complete
>> the write operation.
>>
>> Suggested-by: Denis V. Lunev <address@hidden>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> Signed-off-by: Andrey Shinkevich <address@hidden>
>> ---
>>    tests/qemu-iotests/277                   | 102 
>> +++++++++++++++++++++++++++++++
>>    tests/qemu-iotests/277.out               |   6 ++
>>    tests/qemu-iotests/group                 |   1 +
>>    tests/qemu-iotests/iotests.py            |   5 ++
>>    tests/qemu-iotests/nbd-fault-injector.py |   3 +-
>>    5 files changed, 116 insertions(+), 1 deletion(-)
>>    create mode 100755 tests/qemu-iotests/277
>>    create mode 100644 tests/qemu-iotests/277.out
>>
>> diff --git a/tests/qemu-iotests/277 b/tests/qemu-iotests/277
>> new file mode 100755
>> index 0000000..e4e6730
>> --- /dev/null
>> +++ b/tests/qemu-iotests/277
>> @@ -0,0 +1,102 @@
>> +#!/usr/bin/env python
>> +#
>> +# Test NBD client reconnection
>> +#
>> +# Copyright (c) 2019 Virtuozzo International GmbH
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +import os
>> +import subprocess
>> +import iotests
>> +from iotests import file_path, log
>> +
>> +
>> +def make_conf_file(event):
>> +    """
>> +    Create configuration file for the nbd-fault-injector.py
>> +
>> +    :param event: which event the server should close a connection on
>> +    """
>> +    if os.path.exists(conf_file):
>> +        os.remove(conf_file)
> 
> a bit strange for my eyes to see global variable not defined higher in the 
> file than used..
> 
>> +
>> +    with open(conf_file, "w+") as conff:
>> +        conff.write("[inject-error]\nevent={}\nwhen=after".format(event))
> 
> better open with "w" and don't need to remove file before it.
> 
>> +
>> +
>> +def start_server_NBD(event):
>> +    make_conf_file(event)
>> +
>> +    srv = subprocess.Popen(["nbd-fault-injector.py", 
>> "--classic-negotiation",
>> +                           nbd_sock, conf_file], stdout=subprocess.PIPE,
>> +                           stderr=subprocess.STDOUT, 
>> universal_newlines=True)
>> +    line = srv.stdout.readline()
>> +    if "Listening on " in line:
>> +        log('NBD server: started')
>> +    else:
>> +        log('NBD server: ' + line.rstrip())
>> +
>> +    return srv
>> +
>> +
>> +def start_client_NBD():
>> +    log('NBD client: QEMU-IO write')
>> +    args = iotests.qemu_io_args_no_fmt + \
>> +        ['-c', 'write -P 0x7 0 3M', '--image-opts',
>> +         'driver=nbd,server.type=unix,server.path={},'
>> +         'reconnect-delay=7'.format(nbd_sock)]
>> +    clt = subprocess.Popen(args, stdout=subprocess.PIPE,
>> +                           stderr=subprocess.STDOUT,
>> +                           universal_newlines=True)
>> +    return clt
> 
> Could you reuse QemuIoInteractive as a client?
> 

Thanks for your review.
The class QemuIoInteractive uses the qemu_io_args variable that includes 
the format option which conflicts with the option 'image-opts'.

>> +
>> +
>> +def check_proc_NBD(proc, connector):
>> +    try:
>> +        exitcode = proc.wait(timeout=10)
>> +
>> +        if exitcode < 0:
>> +            log('NBD {}: EXIT SIGNAL {}\n'.format(connector, -exitcode))
>> +            log(proc.communicate()[0])
>> +        else:
>> +            line = proc.stdout.readline()
> 
> 
> could we use proc.communicate() for both cases, what is the difference?
> 
>> +            log('NBD {}: {}'.format(connector, line.rstrip()))
>> +
>> +    except subprocess.TimeoutExpired:
>> +        proc.kill()
>> +        log('NBD {}: ERROR timeout expired'.format(connector))
>> +    finally:
>> +        if connector == 'server':
>> +            os.remove(nbd_sock)
>> +            os.remove(conf_file)
>> +
>> +
>> +conf_file = os.path.join(iotests.test_dir, "nbd-fault-injector.conf")
> 
> use file_path here too.
> 
>> +nbd_sock = file_path('nbd-sock')
>> +nbd_uri = 'nbd+unix:///?socket=' + nbd_sock
> 
> unused variable
> 
>> +if os.path.exists(nbd_sock):
>> +    os.remove(nbd_sock)
> 
> I don't think we need this
> 
>> +
>> +srv = start_server_NBD('data')
>> +clt = start_client_NBD()
>> +# The server should close the connection after a client write request
>> +check_proc_NBD(srv, 'server')
>> +# Start the NBD server again
>> +srv = start_server_NBD('reply')
>> +# The client should reconnect and complete the write operation
>> +check_proc_NBD(clt, 'client')
>> +# Make it sure that server terminated
>> +check_proc_NBD(srv, 'server')
>> diff --git a/tests/qemu-iotests/277.out b/tests/qemu-iotests/277.out
>> new file mode 100644
>> index 0000000..45404b3
>> --- /dev/null
>> +++ b/tests/qemu-iotests/277.out
>> @@ -0,0 +1,6 @@
>> +NBD server: started
>> +NBD client: QEMU-IO write
>> +NBD server: Closing connection on rule match inject-error
>> +NBD server: started
>> +NBD client: wrote 3145728/3145728 bytes at offset 0
>> +NBD server: Closing connection on rule match inject-error
>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>> index af322af..22ef1b8 100644
>> --- a/tests/qemu-iotests/group
>> +++ b/tests/qemu-iotests/group
>> @@ -282,3 +282,4 @@
>>    267 rw auto quick snapshot
>>    268 rw auto quick
>>    270 rw backing quick
>> +277 rw
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 709def4..0d16303 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -47,6 +47,11 @@ qemu_io_args = [os.environ.get('QEMU_IO_PROG', 'qemu-io')]
>>    if os.environ.get('QEMU_IO_OPTIONS'):
>>        qemu_io_args += os.environ['QEMU_IO_OPTIONS'].strip().split(' ')
>>    
>> +qemu_io_args_no_fmt = [os.environ.get('QEMU_IO_PROG', 'qemu-io')]
>> +if os.environ.get('QEMU_IO_OPTIONS_NO_FMT'):
>> +    qemu_io_args_no_fmt += \
>> +        os.environ['QEMU_IO_OPTIONS_NO_FMT'].strip().split(' ')
>> +
>>    qemu_nbd_args = [os.environ.get('QEMU_NBD_PROG', 'qemu-nbd')]
>>    if os.environ.get('QEMU_NBD_OPTIONS'):
>>        qemu_nbd_args += os.environ['QEMU_NBD_OPTIONS'].strip().split(' ')
>> diff --git a/tests/qemu-iotests/nbd-fault-injector.py 
>> b/tests/qemu-iotests/nbd-fault-injector.py
>> index 6b2d659..7e2dab6 100755
>> --- a/tests/qemu-iotests/nbd-fault-injector.py
>> +++ b/tests/qemu-iotests/nbd-fault-injector.py
>> @@ -115,7 +115,8 @@ class FaultInjectionSocket(object):
>>                if rule.match(event, io):
>>                    if rule.when == 0 or bufsize is None:
>>                        print('Closing connection on rule match %s' % 
>> rule.name)
>> -                    self.sock.flush()
>> +                    self.sock.close()
>> +                    sys.stdout.flush()
> 
> Why do you need this? It should be mentioned in commit message I think.
> 

The socket object does not have the flush() method. I guess the author 
wanted to flush from the stdout.

>>                        sys.exit(0)
>>                    if rule.when != -1:
>>                        return rule.when
>>
> 
> 

-- 
With the best regards,
Andrey Shinkevich