[v3] iotests: Test NBD client reconnection

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 <den@openvz.org>
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 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

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 <den@openvz.org>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   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?

> +
> +
> +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.

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

On Fri, Nov 08, 2019 at 01:49:50PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 01.11.2019 19:54, Andrey Shinkevich wrote:
> > +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?

In fact if proc produces any non-trivial amount of output you are better
off using .communicate() otherwise your child may block on output and
never exit.  See
https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate
for how to express the above logic correctly.  The exit code *after*
.communicate is available in .returncode.

> 
> > +            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)

Roman.

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 <den@openvz.org>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>    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
>>
> 
>

On 08/11/2019 17:05, Roman Kagan wrote:
> On Fri, Nov 08, 2019 at 01:49:50PM +0000, Vladimir Sementsov-Ogievskiy wrote:
>> 01.11.2019 19:54, Andrey Shinkevich wrote:
>>> +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?
> 
> In fact if proc produces any non-trivial amount of output you are better
> off using .communicate() otherwise your child may block on output and
> never exit.  See
> https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate
> for how to express the above logic correctly.  The exit code *after*
> .communicate is available in .returncode.
> 

The pattern by the link above does not work (Python3):

proc = subprocess.Popen(...)
try:
     outs, errs = proc.communicate(timeout=15)
except TimeoutExpired:
     proc.kill()
     outs, errs = proc.communicate()

as 'proc' cannot be used for output after being killed. It results in 
another exception being raised.

Andrey

>>
>>> +            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)
> 
> Roman.
>

On Mon, Nov 11, 2019 at 12:18:48PM +0300, Andrey Shinkevich wrote:
> 
> 
> On 08/11/2019 17:05, Roman Kagan wrote:
> > On Fri, Nov 08, 2019 at 01:49:50PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> >> 01.11.2019 19:54, Andrey Shinkevich wrote:
> >>> +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?
> > 
> > In fact if proc produces any non-trivial amount of output you are better
> > off using .communicate() otherwise your child may block on output and
> > never exit.  See
> > https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate
> > for how to express the above logic correctly.  The exit code *after*
> > .communicate is available in .returncode.
> > 
> 
> The pattern by the link above does not work (Python3):
> 
> proc = subprocess.Popen(...)
> try:
>      outs, errs = proc.communicate(timeout=15)
> except TimeoutExpired:
>      proc.kill()
>      outs, errs = proc.communicate()
> 
> as 'proc' cannot be used for output after being killed. It results in 
> another exception being raised.

Of course it can't.  You need to use the strings returned by
.communicate().

Roman.