[Qemu-devel] [PATCH 0/3] iotests: Allow 147 to be run concurrently

[Qemu-devel] [PATCH 0/3] iotests: Allow 147 to be run concurrently

[Max Reitz]

147 currently cannot be run concurrently for two reasons:

(1) It creates TCP/IP NBD servers on a fixed port.

(2) It uses a mix of "0.0.0.0", "localhost", and "::1" as host addresses
    to bind to.  As explained in the commit messages of patches 2 and 3,
    this results in it potentially actually being able to set up two
    servers on the same port at the same time -- but connecting to one
    will always lead to the IPv6 server (regardless of whether you use
    "localhost" or "::1").  Therefore, even if you get two servers
    running concurrently, one of the tests will still break because it
    connects to the wrong one.

This series fixes these issues.


Note that even if you do not care about concurrency, it still is a good
idea to make 147 not use a fixed port to create NBD servers on, as it
may always be already in use by a totally different application.


Max Reitz (3):
  iotests.py: Add qemu_nbd_pipe()
  iotests: Bind qemu-nbd to localhost in 147
  iotests: Allow 147 to be run concurrently

 tests/qemu-iotests/147        | 98 ++++++++++++++++++++++++-----------
 tests/qemu-iotests/iotests.py | 14 +++++
 2 files changed, 82 insertions(+), 30 deletions(-)

-- 
2.19.2

[Qemu-devel] [PATCH 1/3] iotests.py: Add qemu_nbd_pipe()

[Max Reitz]

In some cases, we may want to deal with qemu-nbd errors (e.g. by
launching it in a different configuration until it no longer throws
any).  In that case, we do not want its output ending up in the test
output.

It may still be useful for handling the error, though, so add a new
function that works basically like qemu_nbd(), only that it returns the
qemu-nbd output instead of making it end up in the log.  In contrast to
qemu_img_pipe(), it does still return the exit code as well, though,
because that is even more important for error handling.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index d537538ba0..9c3eb9e2f8 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -190,6 +190,20 @@ def qemu_nbd(*args):
     '''Run qemu-nbd in daemon mode and return the parent's exit code'''
     return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
 
+def qemu_nbd_pipe(*args):
+    '''Run qemu-nbd in daemon mode and return both the parent's exit code
+       and its output'''
+    subp = subprocess.Popen(qemu_nbd_args + ['--fork'] + list(args),
+                            stdout=subprocess.PIPE,
+                            stderr=subprocess.STDOUT,
+                            universal_newlines=True)
+    exitcode = subp.wait()
+    if exitcode < 0:
+        sys.stderr.write('qemu-nbd received signal %i: %s\n' %
+                         (-exitcode,
+                          ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
+    return exitcode, subp.communicate()[0]
+
 def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
     '''Return True if two image files are identical'''
     return qemu_img('compare', '-f', fmt1,
-- 
2.19.2

[Qemu-devel] [PATCH 2/3] iotests: Bind qemu-nbd to localhost in 147

[Max Reitz]

By default, qemu-nbd binds to 0.0.0.0.  However, we then proceed to
connect to "localhost".  Usually, this works out fine; but if this test
is run concurrently, some other test function may have bound a different
server to ::1 (on the same port -- you can bind different serves to the
same port, as long as one is on IPv4 and the other on IPv6).

So running qemu-nbd works, it can bind to 0.0.0.0:NBD_PORT.  But
potentially a concurrent test has successfully taken [::1]:NBD_PORT.  In
this case, trying to connect to "localhost" will lead us to the IPv6
instance, where we do not want to end up.

Fix this by just binding to "localhost".  This will make qemu-nbd error
out immediately and not give us cryptic errors later.

(Also, it will allow us to just try a different port as of a future
patch.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/147 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index 05b374b7d3..3e10a9969e 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -92,7 +92,7 @@ class QemuNBD(NBDBlockdevAddBase):
         self.assertEqual(qemu_nbd('-f', imgfmt, test_img, *args), 0)
 
     def test_inet(self):
-        self._server_up('-p', str(NBD_PORT))
+        self._server_up('-b', 'localhost', '-p', str(NBD_PORT))
         address = { 'type': 'inet',
                     'data': {
                         'host': 'localhost',
-- 
2.19.2

[Qemu-devel] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[Max Reitz]

To do this, we need to allow creating the NBD server on various ports
instead of a single one (which may not even work if you run just one
instance, because something entirely else might be using that port).

So we just pick a random port in [32768, 32768 + 1024) and try to create
a server there.  If that fails, we just retry until something sticks.

For the IPv6 test, we need a different range, though (just above that
one).  This is because "localhost" resolves to both 127.0.0.1 and ::1.
This means that if you bind to it, it will bind to both, if possible, or
just one if the other is already in use.  Therefore, if the IPv6 test
has already taken [::1]:some_port and we then try to take
localhost:some_port, that will work -- only the second server will be
bound to 127.0.0.1:some_port alone and not [::1]:some_port in addition.
So we have two different servers on the same port, one for IPv4 and one
for IPv6.

But when we then try to connect to the server through
localhost:some_port, we will always end up at the IPv6 one (as long as
it is up), and this may not be the one we want.

Thus, we must make sure not to create an IPv6-only NBD server on the
same port as a normal "dual-stack" NBD server -- which is done by using
distinct port ranges, as explained above.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/147 | 98 +++++++++++++++++++++++++++++-------------
 1 file changed, 68 insertions(+), 30 deletions(-)

diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index 3e10a9969e..82513279b0 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -19,13 +19,17 @@
 #
 
 import os
+import random
 import socket
 import stat
 import time
 import iotests
-from iotests import cachemode, imgfmt, qemu_img, qemu_nbd
+from iotests import cachemode, imgfmt, qemu_img, qemu_nbd, qemu_nbd_pipe
 
-NBD_PORT = 10811
+NBD_PORT_START      = 32768
+NBD_PORT_END        = NBD_PORT_START + 1024
+NBD_IPV6_PORT_START = NBD_PORT_END
+NBD_IPV6_PORT_END   = NBD_IPV6_PORT_START + 1024
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
 unix_socket = os.path.join(iotests.test_dir, 'nbd.socket')
@@ -88,17 +92,29 @@ class QemuNBD(NBDBlockdevAddBase):
         except OSError:
             pass
 
+    def _try_server_up(self, *args):
+        status, msg = qemu_nbd_pipe('-f', imgfmt, test_img, *args)
+        if status == 0:
+            return True
+        if 'Address already in use' in msg:
+            return False
+        self.fail(msg)
+
     def _server_up(self, *args):
-        self.assertEqual(qemu_nbd('-f', imgfmt, test_img, *args), 0)
+        self.assertTrue(self._try_server_up(*args))
 
     def test_inet(self):
-        self._server_up('-b', 'localhost', '-p', str(NBD_PORT))
+        while True:
+            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
+            if self._try_server_up('-b', 'localhost', '-p', str(nbd_port)):
+                break
+
         address = { 'type': 'inet',
                     'data': {
                         'host': 'localhost',
-                        'port': str(NBD_PORT)
+                        'port': str(nbd_port)
                     } }
-        self.client_test('nbd://localhost:%i' % NBD_PORT,
+        self.client_test('nbd://localhost:%i' % nbd_port,
                          flatten_sock_addr(address))
 
     def test_unix(self):
@@ -130,8 +146,13 @@ class BuiltinNBD(NBDBlockdevAddBase):
         except OSError:
             pass
 
-    def _server_up(self, address, export_name=None, export_name2=None):
+    # Returns False on EADDRINUSE; fails an assertion on other errors.
+    # Returns True on success.
+    def _try_server_up(self, address, export_name=None, export_name2=None):
         result = self.server.qmp('nbd-server-start', addr=address)
+        if 'error' in result and \
+           'Address already in use' in result['error']['desc']:
+            return False
         self.assert_qmp(result, 'return', {})
 
         if export_name is None:
@@ -146,20 +167,28 @@ class BuiltinNBD(NBDBlockdevAddBase):
                                      name=export_name2)
             self.assert_qmp(result, 'return', {})
 
+        return True
+
+    def _server_up(self, address, export_name=None, export_name2=None):
+        self.assertTrue(self._try_server_up(address, export_name, export_name2))
 
     def _server_down(self):
         result = self.server.qmp('nbd-server-stop')
         self.assert_qmp(result, 'return', {})
 
     def do_test_inet(self, export_name=None):
-        address = { 'type': 'inet',
-                    'data': {
-                        'host': 'localhost',
-                        'port': str(NBD_PORT)
-                    } }
-        self._server_up(address, export_name)
+        while True:
+            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
+            address = { 'type': 'inet',
+                        'data': {
+                            'host': 'localhost',
+                            'port': str(nbd_port)
+                        } }
+            if self._try_server_up(address, export_name):
+                break
+
         export_name = export_name or 'nbd-export'
-        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, export_name),
+        self.client_test('nbd://localhost:%i/%s' % (nbd_port, export_name),
                          flatten_sock_addr(address), export_name)
         self._server_down()
 
@@ -173,15 +202,19 @@ class BuiltinNBD(NBDBlockdevAddBase):
         self.do_test_inet('shadow')
 
     def test_inet_two_exports(self):
-        address = { 'type': 'inet',
-                    'data': {
-                        'host': 'localhost',
-                        'port': str(NBD_PORT)
-                    } }
-        self._server_up(address, 'exp1', 'exp2')
-        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, 'exp1'),
+        while True:
+            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
+            address = { 'type': 'inet',
+                        'data': {
+                            'host': 'localhost',
+                            'port': str(nbd_port)
+                        } }
+            if self._try_server_up(address, 'exp1', 'exp2'):
+                break
+
+        self.client_test('nbd://localhost:%i/%s' % (nbd_port, 'exp1'),
                          flatten_sock_addr(address), 'exp1', 'node1', False)
-        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, 'exp2'),
+        self.client_test('nbd://localhost:%i/%s' % (nbd_port, 'exp2'),
                          flatten_sock_addr(address), 'exp2', 'node2', False)
         result = self.vm.qmp('blockdev-del', node_name='node1')
         self.assert_qmp(result, 'return', {})
@@ -197,20 +230,25 @@ class BuiltinNBD(NBDBlockdevAddBase):
         except socket.gaierror:
             # IPv6 not available, skip
             return
-        address = { 'type': 'inet',
-                    'data': {
-                        'host': '::1',
-                        'port': str(NBD_PORT),
-                        'ipv4': False,
-                        'ipv6': True
-                    } }
+
+        while True:
+            nbd_port = random.randrange(NBD_IPV6_PORT_START, NBD_IPV6_PORT_END)
+            address = { 'type': 'inet',
+                        'data': {
+                            'host': '::1',
+                            'port': str(nbd_port),
+                            'ipv4': False,
+                            'ipv6': True
+                        } }
+            if self._try_server_up(address):
+                break
+
         filename = { 'driver': 'raw',
                      'file': {
                          'driver': 'nbd',
                          'export': 'nbd-export',
                          'server': flatten_sock_addr(address)
                      } }
-        self._server_up(address)
         self.client_test(filename, flatten_sock_addr(address), 'nbd-export')
         self._server_down()
 
-- 
2.19.2

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[John Snow]

On 12/21/18 6:47 PM, Max Reitz wrote:
> To do this, we need to allow creating the NBD server on various ports
> instead of a single one (which may not even work if you run just one
> instance, because something entirely else might be using that port).
> 
> So we just pick a random port in [32768, 32768 + 1024) and try to create
> a server there.  If that fails, we just retry until something sticks.
> 
> For the IPv6 test, we need a different range, though (just above that
> one).  This is because "localhost" resolves to both 127.0.0.1 and ::1.
> This means that if you bind to it, it will bind to both, if possible, or
> just one if the other is already in use.  Therefore, if the IPv6 test
> has already taken [::1]:some_port and we then try to take
> localhost:some_port, that will work -- only the second server will be
> bound to 127.0.0.1:some_port alone and not [::1]:some_port in addition.
> So we have two different servers on the same port, one for IPv4 and one
> for IPv6.
> 
> But when we then try to connect to the server through
> localhost:some_port, we will always end up at the IPv6 one (as long as
> it is up), and this may not be the one we want.
> 
> Thus, we must make sure not to create an IPv6-only NBD server on the
> same port as a normal "dual-stack" NBD server -- which is done by using
> distinct port ranges, as explained above.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/147 | 98 +++++++++++++++++++++++++++++-------------
>  1 file changed, 68 insertions(+), 30 deletions(-)
> 
> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
> index 3e10a9969e..82513279b0 100755
> --- a/tests/qemu-iotests/147
> +++ b/tests/qemu-iotests/147
> @@ -19,13 +19,17 @@
>  #
>  
>  import os
> +import random
>  import socket
>  import stat
>  import time
>  import iotests
> -from iotests import cachemode, imgfmt, qemu_img, qemu_nbd
> +from iotests import cachemode, imgfmt, qemu_img, qemu_nbd, qemu_nbd_pipe
>  
> -NBD_PORT = 10811
> +NBD_PORT_START      = 32768
> +NBD_PORT_END        = NBD_PORT_START + 1024
> +NBD_IPV6_PORT_START = NBD_PORT_END
> +NBD_IPV6_PORT_END   = NBD_IPV6_PORT_START + 1024
>  
>  test_img = os.path.join(iotests.test_dir, 'test.img')
>  unix_socket = os.path.join(iotests.test_dir, 'nbd.socket')
> @@ -88,17 +92,29 @@ class QemuNBD(NBDBlockdevAddBase):
>          except OSError:
>              pass
>  
> +    def _try_server_up(self, *args):
> +        status, msg = qemu_nbd_pipe('-f', imgfmt, test_img, *args)
> +        if status == 0:
> +            return True
> +        if 'Address already in use' in msg:
> +            return False

My only half-empty question is if we are guaranteed to have a locale and
environment such that this string will always be present when we
encounter that specific error?

I suppose even if not, that it'll just fail hard and it's no worse than
what we had before.

> +        self.fail(msg)
> +
>      def _server_up(self, *args):
> -        self.assertEqual(qemu_nbd('-f', imgfmt, test_img, *args), 0)
> +        self.assertTrue(self._try_server_up(*args))
>  
>      def test_inet(self):
> -        self._server_up('-b', 'localhost', '-p', str(NBD_PORT))
> +        while True:
> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
> +            if self._try_server_up('-b', 'localhost', '-p', str(nbd_port)):
> +                break

Why not just iterate over the range so that once the range is exhausted
we're guaranteed to fail?

> +
>          address = { 'type': 'inet',
>                      'data': {
>                          'host': 'localhost',
> -                        'port': str(NBD_PORT)
> +                        'port': str(nbd_port)
>                      } }
> -        self.client_test('nbd://localhost:%i' % NBD_PORT,
> +        self.client_test('nbd://localhost:%i' % nbd_port,
>                           flatten_sock_addr(address))
>  
>      def test_unix(self):
> @@ -130,8 +146,13 @@ class BuiltinNBD(NBDBlockdevAddBase):
>          except OSError:
>              pass
>  
> -    def _server_up(self, address, export_name=None, export_name2=None):
> +    # Returns False on EADDRINUSE; fails an assertion on other errors.
> +    # Returns True on success.
> +    def _try_server_up(self, address, export_name=None, export_name2=None):
>          result = self.server.qmp('nbd-server-start', addr=address)
> +        if 'error' in result and \
> +           'Address already in use' in result['error']['desc']:
> +            return False
>          self.assert_qmp(result, 'return', {})
>  
>          if export_name is None:
> @@ -146,20 +167,28 @@ class BuiltinNBD(NBDBlockdevAddBase):
>                                       name=export_name2)
>              self.assert_qmp(result, 'return', {})
>  
> +        return True
> +
> +    def _server_up(self, address, export_name=None, export_name2=None):
> +        self.assertTrue(self._try_server_up(address, export_name, export_name2))
>  
>      def _server_down(self):
>          result = self.server.qmp('nbd-server-stop')
>          self.assert_qmp(result, 'return', {})
>  
>      def do_test_inet(self, export_name=None):
> -        address = { 'type': 'inet',
> -                    'data': {
> -                        'host': 'localhost',
> -                        'port': str(NBD_PORT)
> -                    } }
> -        self._server_up(address, export_name)
> +        while True:
> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
> +            address = { 'type': 'inet',
> +                        'data': {
> +                            'host': 'localhost',
> +                            'port': str(nbd_port)
> +                        } }
> +            if self._try_server_up(address, export_name):
> +                break
> +
>          export_name = export_name or 'nbd-export'
> -        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, export_name),
> +        self.client_test('nbd://localhost:%i/%s' % (nbd_port, export_name),
>                           flatten_sock_addr(address), export_name)
>          self._server_down()
>  
> @@ -173,15 +202,19 @@ class BuiltinNBD(NBDBlockdevAddBase):
>          self.do_test_inet('shadow')
>  
>      def test_inet_two_exports(self):
> -        address = { 'type': 'inet',
> -                    'data': {
> -                        'host': 'localhost',
> -                        'port': str(NBD_PORT)
> -                    } }
> -        self._server_up(address, 'exp1', 'exp2')
> -        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, 'exp1'),
> +        while True:
> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
> +            address = { 'type': 'inet',
> +                        'data': {
> +                            'host': 'localhost',
> +                            'port': str(nbd_port)
> +                        } }
> +            if self._try_server_up(address, 'exp1', 'exp2'):
> +                break
> +
> +        self.client_test('nbd://localhost:%i/%s' % (nbd_port, 'exp1'),
>                           flatten_sock_addr(address), 'exp1', 'node1', False)
> -        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, 'exp2'),
> +        self.client_test('nbd://localhost:%i/%s' % (nbd_port, 'exp2'),
>                           flatten_sock_addr(address), 'exp2', 'node2', False)
>          result = self.vm.qmp('blockdev-del', node_name='node1')
>          self.assert_qmp(result, 'return', {})
> @@ -197,20 +230,25 @@ class BuiltinNBD(NBDBlockdevAddBase):
>          except socket.gaierror:
>              # IPv6 not available, skip
>              return
> -        address = { 'type': 'inet',
> -                    'data': {
> -                        'host': '::1',
> -                        'port': str(NBD_PORT),
> -                        'ipv4': False,
> -                        'ipv6': True
> -                    } }
> +
> +        while True:
> +            nbd_port = random.randrange(NBD_IPV6_PORT_START, NBD_IPV6_PORT_END)
> +            address = { 'type': 'inet',
> +                        'data': {
> +                            'host': '::1',
> +                            'port': str(nbd_port),
> +                            'ipv4': False,
> +                            'ipv6': True
> +                        } }
> +            if self._try_server_up(address):
> +                break
> +
>          filename = { 'driver': 'raw',
>                       'file': {
>                           'driver': 'nbd',
>                           'export': 'nbd-export',
>                           'server': flatten_sock_addr(address)
>                       } }
> -        self._server_up(address)
>          self.client_test(filename, flatten_sock_addr(address), 'nbd-export')
>          self._server_down()
>  
> 

I guess my only other observation is that we have a lot of "while True"
loops now, is it worth creating some kind of helper that does the dirty
work of finding a serviceable port or nah?


If none of my observations spark joy:

1-3: Reviewed-By: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] iotests.py: Add qemu_nbd_pipe()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]

On 12/21/18 5:47 PM, Max Reitz wrote:
> In some cases, we may want to deal with qemu-nbd errors (e.g. by
> launching it in a different configuration until it no longer throws
> any).  In that case, we do not want its output ending up in the test
> output.
> 
> It may still be useful for handling the error, though, so add a new
> function that works basically like qemu_nbd(), only that it returns the
> qemu-nbd output instead of making it end up in the log.  In contrast to
> qemu_img_pipe(), it does still return the exit code as well, though,

In contrast to qemu_nbd(),

> because that is even more important for error handling.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index d537538ba0..9c3eb9e2f8 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -190,6 +190,20 @@ def qemu_nbd(*args):
>      '''Run qemu-nbd in daemon mode and return the parent's exit code'''
>      return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
>  
> +def qemu_nbd_pipe(*args):
> +    '''Run qemu-nbd in daemon mode and return both the parent's exit code
> +       and its output'''
> +    subp = subprocess.Popen(qemu_nbd_args + ['--fork'] + list(args),
> +                            stdout=subprocess.PIPE,
> +                            stderr=subprocess.STDOUT,
> +                            universal_newlines=True)
> +    exitcode = subp.wait()
> +    if exitcode < 0:
> +        sys.stderr.write('qemu-nbd received signal %i: %s\n' %
> +                         (-exitcode,
> +                          ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
> +    return exitcode, subp.communicate()[0]
> +

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/3] iotests: Bind qemu-nbd to localhost in 147

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

On 12/21/18 5:47 PM, Max Reitz wrote:
> By default, qemu-nbd binds to 0.0.0.0.  However, we then proceed to
> connect to "localhost".  Usually, this works out fine; but if this test
> is run concurrently, some other test function may have bound a different
> server to ::1 (on the same port -- you can bind different serves to the

s/serves/servers/

> same port, as long as one is on IPv4 and the other on IPv6).
> 
> So running qemu-nbd works, it can bind to 0.0.0.0:NBD_PORT.  But
> potentially a concurrent test has successfully taken [::1]:NBD_PORT.  In
> this case, trying to connect to "localhost" will lead us to the IPv6
> instance, where we do not want to end up.
> 
> Fix this by just binding to "localhost".  This will make qemu-nbd error
> out immediately and not give us cryptic errors later.
> 
> (Also, it will allow us to just try a different port as of a future
> patch.)
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/147 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3025 bytes --]

On 12/21/18 5:47 PM, Max Reitz wrote:
> To do this, we need to allow creating the NBD server on various ports
> instead of a single one (which may not even work if you run just one
> instance, because something entirely else might be using that port).

Can you instead reuse the ideas from nbd_server_set_tcp_port() from
qemu-iotests/common.nbd?

> 
> So we just pick a random port in [32768, 32768 + 1024) and try to create
> a server there.  If that fails, we just retry until something sticks.

That has the advantage of checking whether a port is actually in use
(using 'ss' - although it does limit the test to Linux-only; perhaps
using socat instead of ss could make the test portable to non-Linux?)

> 
> For the IPv6 test, we need a different range, though (just above that
> one).  This is because "localhost" resolves to both 127.0.0.1 and ::1.
> This means that if you bind to it, it will bind to both, if possible, or
> just one if the other is already in use.  Therefore, if the IPv6 test
> has already taken [::1]:some_port and we then try to take
> localhost:some_port, that will work -- only the second server will be
> bound to 127.0.0.1:some_port alone and not [::1]:some_port in addition.
> So we have two different servers on the same port, one for IPv4 and one
> for IPv6.
> 
> But when we then try to connect to the server through
> localhost:some_port, we will always end up at the IPv6 one (as long as
> it is up), and this may not be the one we want.
> 
> Thus, we must make sure not to create an IPv6-only NBD server on the
> same port as a normal "dual-stack" NBD server -- which is done by using
> distinct port ranges, as explained above.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/147 | 98 +++++++++++++++++++++++++++++-------------
>  1 file changed, 68 insertions(+), 30 deletions(-)
> 

> @@ -88,17 +92,29 @@ class QemuNBD(NBDBlockdevAddBase):
>          except OSError:
>              pass
>  
> +    def _try_server_up(self, *args):
> +        status, msg = qemu_nbd_pipe('-f', imgfmt, test_img, *args)
> +        if status == 0:
> +            return True
> +        if 'Address already in use' in msg:
> +            return False
> +        self.fail(msg)

Do you actually need to attempt a qemu-nbd process, if you take my
suggestion of using ss to probe for an unused port?  And if not, do we
still need qemu_nbd_pipe() added earlier in the series?


> -        address = { 'type': 'inet',
> -                    'data': {
> -                        'host': 'localhost',
> -                        'port': str(NBD_PORT)
> -                    } }
> -        self._server_up(address, export_name)
> +        while True:
> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)

common.nbd just iterates, instead of trying random ports.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 10128 bytes --]

On 21.01.19 21:50, John Snow wrote:
> 
> 
> On 12/21/18 6:47 PM, Max Reitz wrote:
>> To do this, we need to allow creating the NBD server on various ports
>> instead of a single one (which may not even work if you run just one
>> instance, because something entirely else might be using that port).
>>
>> So we just pick a random port in [32768, 32768 + 1024) and try to create
>> a server there.  If that fails, we just retry until something sticks.
>>
>> For the IPv6 test, we need a different range, though (just above that
>> one).  This is because "localhost" resolves to both 127.0.0.1 and ::1.
>> This means that if you bind to it, it will bind to both, if possible, or
>> just one if the other is already in use.  Therefore, if the IPv6 test
>> has already taken [::1]:some_port and we then try to take
>> localhost:some_port, that will work -- only the second server will be
>> bound to 127.0.0.1:some_port alone and not [::1]:some_port in addition.
>> So we have two different servers on the same port, one for IPv4 and one
>> for IPv6.
>>
>> But when we then try to connect to the server through
>> localhost:some_port, we will always end up at the IPv6 one (as long as
>> it is up), and this may not be the one we want.
>>
>> Thus, we must make sure not to create an IPv6-only NBD server on the
>> same port as a normal "dual-stack" NBD server -- which is done by using
>> distinct port ranges, as explained above.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/147 | 98 +++++++++++++++++++++++++++++-------------
>>  1 file changed, 68 insertions(+), 30 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
>> index 3e10a9969e..82513279b0 100755
>> --- a/tests/qemu-iotests/147
>> +++ b/tests/qemu-iotests/147
>> @@ -19,13 +19,17 @@
>>  #
>>  
>>  import os
>> +import random
>>  import socket
>>  import stat
>>  import time
>>  import iotests
>> -from iotests import cachemode, imgfmt, qemu_img, qemu_nbd
>> +from iotests import cachemode, imgfmt, qemu_img, qemu_nbd, qemu_nbd_pipe
>>  
>> -NBD_PORT = 10811
>> +NBD_PORT_START      = 32768
>> +NBD_PORT_END        = NBD_PORT_START + 1024
>> +NBD_IPV6_PORT_START = NBD_PORT_END
>> +NBD_IPV6_PORT_END   = NBD_IPV6_PORT_START + 1024
>>  
>>  test_img = os.path.join(iotests.test_dir, 'test.img')
>>  unix_socket = os.path.join(iotests.test_dir, 'nbd.socket')
>> @@ -88,17 +92,29 @@ class QemuNBD(NBDBlockdevAddBase):
>>          except OSError:
>>              pass
>>  
>> +    def _try_server_up(self, *args):
>> +        status, msg = qemu_nbd_pipe('-f', imgfmt, test_img, *args)
>> +        if status == 0:
>> +            return True
>> +        if 'Address already in use' in msg:
>> +            return False
> 
> My only half-empty question is if we are guaranteed to have a locale and
> environment such that this string will always be present when we
> encounter that specific error?
> 
> I suppose even if not, that it'll just fail hard and it's no worse than
> what we had before.

I can tell you at least that my locale is de_DE.UTF-8.

>> +        self.fail(msg)
>> +
>>      def _server_up(self, *args):
>> -        self.assertEqual(qemu_nbd('-f', imgfmt, test_img, *args), 0)
>> +        self.assertTrue(self._try_server_up(*args))
>>  
>>      def test_inet(self):
>> -        self._server_up('-b', 'localhost', '-p', str(NBD_PORT))
>> +        while True:
>> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
>> +            if self._try_server_up('-b', 'localhost', '-p', str(nbd_port)):
>> +                break
> 
> Why not just iterate over the range so that once the range is exhausted
> we're guaranteed to fail?

Hm.  With this approach chances are higher that the first port you try
works, so it does have a benefit.  I'm not sure whether it's actually a
problem that this will pretty much break down if everything in the range
is allocated, because I can't see that happening.

Iterating over a range wouldn't make the code easier because I would
have to account for the error case. O:-)

>> +
>>          address = { 'type': 'inet',
>>                      'data': {
>>                          'host': 'localhost',
>> -                        'port': str(NBD_PORT)
>> +                        'port': str(nbd_port)
>>                      } }
>> -        self.client_test('nbd://localhost:%i' % NBD_PORT,
>> +        self.client_test('nbd://localhost:%i' % nbd_port,
>>                           flatten_sock_addr(address))
>>  
>>      def test_unix(self):
>> @@ -130,8 +146,13 @@ class BuiltinNBD(NBDBlockdevAddBase):
>>          except OSError:
>>              pass
>>  
>> -    def _server_up(self, address, export_name=None, export_name2=None):
>> +    # Returns False on EADDRINUSE; fails an assertion on other errors.
>> +    # Returns True on success.
>> +    def _try_server_up(self, address, export_name=None, export_name2=None):
>>          result = self.server.qmp('nbd-server-start', addr=address)
>> +        if 'error' in result and \
>> +           'Address already in use' in result['error']['desc']:
>> +            return False
>>          self.assert_qmp(result, 'return', {})
>>  
>>          if export_name is None:
>> @@ -146,20 +167,28 @@ class BuiltinNBD(NBDBlockdevAddBase):
>>                                       name=export_name2)
>>              self.assert_qmp(result, 'return', {})
>>  
>> +        return True
>> +
>> +    def _server_up(self, address, export_name=None, export_name2=None):
>> +        self.assertTrue(self._try_server_up(address, export_name, export_name2))
>>  
>>      def _server_down(self):
>>          result = self.server.qmp('nbd-server-stop')
>>          self.assert_qmp(result, 'return', {})
>>  
>>      def do_test_inet(self, export_name=None):
>> -        address = { 'type': 'inet',
>> -                    'data': {
>> -                        'host': 'localhost',
>> -                        'port': str(NBD_PORT)
>> -                    } }
>> -        self._server_up(address, export_name)
>> +        while True:
>> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
>> +            address = { 'type': 'inet',
>> +                        'data': {
>> +                            'host': 'localhost',
>> +                            'port': str(nbd_port)
>> +                        } }
>> +            if self._try_server_up(address, export_name):
>> +                break
>> +
>>          export_name = export_name or 'nbd-export'
>> -        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, export_name),
>> +        self.client_test('nbd://localhost:%i/%s' % (nbd_port, export_name),
>>                           flatten_sock_addr(address), export_name)
>>          self._server_down()
>>  
>> @@ -173,15 +202,19 @@ class BuiltinNBD(NBDBlockdevAddBase):
>>          self.do_test_inet('shadow')
>>  
>>      def test_inet_two_exports(self):
>> -        address = { 'type': 'inet',
>> -                    'data': {
>> -                        'host': 'localhost',
>> -                        'port': str(NBD_PORT)
>> -                    } }
>> -        self._server_up(address, 'exp1', 'exp2')
>> -        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, 'exp1'),
>> +        while True:
>> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
>> +            address = { 'type': 'inet',
>> +                        'data': {
>> +                            'host': 'localhost',
>> +                            'port': str(nbd_port)
>> +                        } }
>> +            if self._try_server_up(address, 'exp1', 'exp2'):
>> +                break
>> +
>> +        self.client_test('nbd://localhost:%i/%s' % (nbd_port, 'exp1'),
>>                           flatten_sock_addr(address), 'exp1', 'node1', False)
>> -        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, 'exp2'),
>> +        self.client_test('nbd://localhost:%i/%s' % (nbd_port, 'exp2'),
>>                           flatten_sock_addr(address), 'exp2', 'node2', False)
>>          result = self.vm.qmp('blockdev-del', node_name='node1')
>>          self.assert_qmp(result, 'return', {})
>> @@ -197,20 +230,25 @@ class BuiltinNBD(NBDBlockdevAddBase):
>>          except socket.gaierror:
>>              # IPv6 not available, skip
>>              return
>> -        address = { 'type': 'inet',
>> -                    'data': {
>> -                        'host': '::1',
>> -                        'port': str(NBD_PORT),
>> -                        'ipv4': False,
>> -                        'ipv6': True
>> -                    } }
>> +
>> +        while True:
>> +            nbd_port = random.randrange(NBD_IPV6_PORT_START, NBD_IPV6_PORT_END)
>> +            address = { 'type': 'inet',
>> +                        'data': {
>> +                            'host': '::1',
>> +                            'port': str(nbd_port),
>> +                            'ipv4': False,
>> +                            'ipv6': True
>> +                        } }
>> +            if self._try_server_up(address):
>> +                break
>> +
>>          filename = { 'driver': 'raw',
>>                       'file': {
>>                           'driver': 'nbd',
>>                           'export': 'nbd-export',
>>                           'server': flatten_sock_addr(address)
>>                       } }
>> -        self._server_up(address)
>>          self.client_test(filename, flatten_sock_addr(address), 'nbd-export')
>>          self._server_down()
>>  
>>
> 
> I guess my only other observation is that we have a lot of "while True"
> loops now, is it worth creating some kind of helper that does the dirty
> work of finding a serviceable port or nah?

Seems reasonable, I'll see how it looks.

> If none of my observations spark joy:
> 
> 1-3: Reviewed-By: John Snow <jsnow@redhat.com>

Thanks!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 1/3] iotests.py: Add qemu_nbd_pipe()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

On 21.01.19 21:55, Eric Blake wrote:
> On 12/21/18 5:47 PM, Max Reitz wrote:
>> In some cases, we may want to deal with qemu-nbd errors (e.g. by
>> launching it in a different configuration until it no longer throws
>> any).  In that case, we do not want its output ending up in the test
>> output.
>>
>> It may still be useful for handling the error, though, so add a new
>> function that works basically like qemu_nbd(), only that it returns the
>> qemu-nbd output instead of making it end up in the log.  In contrast to
>> qemu_img_pipe(), it does still return the exit code as well, though,
> 
> In contrast to qemu_nbd(),

But qemu_nbd() does return the exit code.  qemu_img_pipe() doesn't.

>> because that is even more important for error handling.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/iotests.py | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index d537538ba0..9c3eb9e2f8 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -190,6 +190,20 @@ def qemu_nbd(*args):
>>      '''Run qemu-nbd in daemon mode and return the parent's exit code'''
>>      return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
>>  
>> +def qemu_nbd_pipe(*args):
>> +    '''Run qemu-nbd in daemon mode and return both the parent's exit code
>> +       and its output'''
>> +    subp = subprocess.Popen(qemu_nbd_args + ['--fork'] + list(args),
>> +                            stdout=subprocess.PIPE,
>> +                            stderr=subprocess.STDOUT,
>> +                            universal_newlines=True)
>> +    exitcode = subp.wait()
>> +    if exitcode < 0:
>> +        sys.stderr.write('qemu-nbd received signal %i: %s\n' %
>> +                         (-exitcode,
>> +                          ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
>> +    return exitcode, subp.communicate()[0]
>> +
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks for reviewing!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3293 bytes --]

On 21.01.19 22:02, Eric Blake wrote:
> On 12/21/18 5:47 PM, Max Reitz wrote:
>> To do this, we need to allow creating the NBD server on various ports
>> instead of a single one (which may not even work if you run just one
>> instance, because something entirely else might be using that port).
> 
> Can you instead reuse the ideas from nbd_server_set_tcp_port() from
> qemu-iotests/common.nbd?
> 
>>
>> So we just pick a random port in [32768, 32768 + 1024) and try to create
>> a server there.  If that fails, we just retry until something sticks.
> 
> That has the advantage of checking whether a port is actually in use
> (using 'ss' - although it does limit the test to Linux-only; perhaps
> using socat instead of ss could make the test portable to non-Linux?)

But doesn't that give you race conditions?  That's the point of this
series, so you can run multiple instances of 147 concurrently.

>> For the IPv6 test, we need a different range, though (just above that
>> one).  This is because "localhost" resolves to both 127.0.0.1 and ::1.
>> This means that if you bind to it, it will bind to both, if possible, or
>> just one if the other is already in use.  Therefore, if the IPv6 test
>> has already taken [::1]:some_port and we then try to take
>> localhost:some_port, that will work -- only the second server will be
>> bound to 127.0.0.1:some_port alone and not [::1]:some_port in addition.
>> So we have two different servers on the same port, one for IPv4 and one
>> for IPv6.
>>
>> But when we then try to connect to the server through
>> localhost:some_port, we will always end up at the IPv6 one (as long as
>> it is up), and this may not be the one we want.
>>
>> Thus, we must make sure not to create an IPv6-only NBD server on the
>> same port as a normal "dual-stack" NBD server -- which is done by using
>> distinct port ranges, as explained above.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/147 | 98 +++++++++++++++++++++++++++++-------------
>>  1 file changed, 68 insertions(+), 30 deletions(-)
>>
> 
>> @@ -88,17 +92,29 @@ class QemuNBD(NBDBlockdevAddBase):
>>          except OSError:
>>              pass
>>  
>> +    def _try_server_up(self, *args):
>> +        status, msg = qemu_nbd_pipe('-f', imgfmt, test_img, *args)
>> +        if status == 0:
>> +            return True
>> +        if 'Address already in use' in msg:
>> +            return False
>> +        self.fail(msg)
> 
> Do you actually need to attempt a qemu-nbd process, if you take my
> suggestion of using ss to probe for an unused port?  And if not, do we
> still need qemu_nbd_pipe() added earlier in the series?
> 
> 
>> -        address = { 'type': 'inet',
>> -                    'data': {
>> -                        'host': 'localhost',
>> -                        'port': str(NBD_PORT)
>> -                    } }
>> -        self._server_up(address, export_name)
>> +        while True:
>> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
> 
> common.nbd just iterates, instead of trying random ports.

I'm not sure which is better.  Iterating gives guaranteed termination,
trying random ports means the first one you try will usually work.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 1/3] iotests.py: Add qemu_nbd_pipe()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

On 1/23/19 7:06 AM, Max Reitz wrote:
> On 21.01.19 21:55, Eric Blake wrote:
>> On 12/21/18 5:47 PM, Max Reitz wrote:
>>> In some cases, we may want to deal with qemu-nbd errors (e.g. by
>>> launching it in a different configuration until it no longer throws
>>> any).  In that case, we do not want its output ending up in the test
>>> output.
>>>
>>> It may still be useful for handling the error, though, so add a new
>>> function that works basically like qemu_nbd(), only that it returns the
>>> qemu-nbd output instead of making it end up in the log.  In contrast to
>>> qemu_img_pipe(), it does still return the exit code as well, though,
>>
>> In contrast to qemu_nbd(),
> 
> But qemu_nbd() does return the exit code.  qemu_img_pipe() doesn't.
> 

Oh, I see where I got confused. I thought you were comparing the new
code [qemu_nbd_pipe] to itself; not to the pre-existing qemu_img_pipe
that is not touched by this patch, but which served as the model you
copied after.

>>> because that is even more important for error handling.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>  tests/qemu-iotests/iotests.py | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>

>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Thanks for reviewing!

Although my questions on patch 3 call into question whether you even
need this patch, if your only use of it was to find a free port.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2693 bytes --]

On 1/23/19 7:12 AM, Max Reitz wrote:
> On 21.01.19 22:02, Eric Blake wrote:
>> On 12/21/18 5:47 PM, Max Reitz wrote:
>>> To do this, we need to allow creating the NBD server on various ports
>>> instead of a single one (which may not even work if you run just one
>>> instance, because something entirely else might be using that port).
>>
>> Can you instead reuse the ideas from nbd_server_set_tcp_port() from
>> qemu-iotests/common.nbd?
>>
>>>
>>> So we just pick a random port in [32768, 32768 + 1024) and try to create
>>> a server there.  If that fails, we just retry until something sticks.
>>
>> That has the advantage of checking whether a port is actually in use
>> (using 'ss' - although it does limit the test to Linux-only; perhaps
>> using socat instead of ss could make the test portable to non-Linux?)
> 
> But doesn't that give you race conditions?  That's the point of this
> series, so you can run multiple instances of 147 concurrently.

Hmm - that does imply that common.nbd's use of ss IS racy because it
checks in linear fashion and has a TOCTTOU window (affects at least
iotest 233). Your observation that random probes within a range are less
susceptible (although not immune) to the race is correct.

>> Do you actually need to attempt a qemu-nbd process, if you take my
>> suggestion of using ss to probe for an unused port?  And if not, do we
>> still need qemu_nbd_pipe() added earlier in the series?
>>
>>
>>> -        address = { 'type': 'inet',
>>> -                    'data': {
>>> -                        'host': 'localhost',
>>> -                        'port': str(NBD_PORT)
>>> -                    } }
>>> -        self._server_up(address, export_name)
>>> +        while True:
>>> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
>>
>> common.nbd just iterates, instead of trying random ports.
> 
> I'm not sure which is better.  Iterating gives guaranteed termination,
> trying random ports means the first one you try will usually work.

Is there any other way we can make the test more robust, perhaps by
using socket activation (that is, pre-open the port prior to starting
qemu_nbd, so that our code for finding a free socket is more easily
reusable), or by using Unix sockets for test 147 (that test seems to be
using TCP sockets only as a means to get to the real feature under test,
and not as the actual thing being tested)?

Hmm, and you made me realize that socket activation is NOT documented in
'man qemu-nbd'; I ought to fix that.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3044 bytes --]

On 23.01.19 15:33, Eric Blake wrote:
> On 1/23/19 7:12 AM, Max Reitz wrote:
>> On 21.01.19 22:02, Eric Blake wrote:
>>> On 12/21/18 5:47 PM, Max Reitz wrote:
>>>> To do this, we need to allow creating the NBD server on various ports
>>>> instead of a single one (which may not even work if you run just one
>>>> instance, because something entirely else might be using that port).
>>>
>>> Can you instead reuse the ideas from nbd_server_set_tcp_port() from
>>> qemu-iotests/common.nbd?
>>>
>>>>
>>>> So we just pick a random port in [32768, 32768 + 1024) and try to create
>>>> a server there.  If that fails, we just retry until something sticks.
>>>
>>> That has the advantage of checking whether a port is actually in use
>>> (using 'ss' - although it does limit the test to Linux-only; perhaps
>>> using socat instead of ss could make the test portable to non-Linux?)
>>
>> But doesn't that give you race conditions?  That's the point of this
>> series, so you can run multiple instances of 147 concurrently.
> 
> Hmm - that does imply that common.nbd's use of ss IS racy because it
> checks in linear fashion and has a TOCTTOU window (affects at least
> iotest 233). Your observation that random probes within a range are less
> susceptible (although not immune) to the race is correct.
> 
>>> Do you actually need to attempt a qemu-nbd process, if you take my
>>> suggestion of using ss to probe for an unused port?  And if not, do we
>>> still need qemu_nbd_pipe() added earlier in the series?
>>>
>>>
>>>> -        address = { 'type': 'inet',
>>>> -                    'data': {
>>>> -                        'host': 'localhost',
>>>> -                        'port': str(NBD_PORT)
>>>> -                    } }
>>>> -        self._server_up(address, export_name)
>>>> +        while True:
>>>> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
>>>
>>> common.nbd just iterates, instead of trying random ports.
>>
>> I'm not sure which is better.  Iterating gives guaranteed termination,
>> trying random ports means the first one you try will usually work.
> 
> Is there any other way we can make the test more robust, perhaps by
> using socket activation (that is, pre-open the port prior to starting
> qemu_nbd, so that our code for finding a free socket is more easily
> reusable), or by using Unix sockets for test 147 (that test seems to be
> using TCP sockets only as a means to get to the real feature under test,
> and not as the actual thing being tested)?

147 needs TCP sockets because that interface is tested.

Making the code reusable is not too high of a priority to me, as
normally NBD tests should just use Unix sockets.  This test is just a
special case.  But for this test, I can try to put the while loop into
an own function (that gets fed an address object without data.port), as
John has proposed.

Max

> Hmm, and you made me realize that socket activation is NOT documented in
> 'man qemu-nbd'; I ought to fix that.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[Daniel P. Berrangé]

On Wed, Jan 23, 2019 at 08:33:41AM -0600, Eric Blake wrote:
> On 1/23/19 7:12 AM, Max Reitz wrote:
> > On 21.01.19 22:02, Eric Blake wrote:
> >> On 12/21/18 5:47 PM, Max Reitz wrote:
> >>> To do this, we need to allow creating the NBD server on various ports
> >>> instead of a single one (which may not even work if you run just one
> >>> instance, because something entirely else might be using that port).
> >>
> >> Can you instead reuse the ideas from nbd_server_set_tcp_port() from
> >> qemu-iotests/common.nbd?
> >>
> >>>
> >>> So we just pick a random port in [32768, 32768 + 1024) and try to create
> >>> a server there.  If that fails, we just retry until something sticks.
> >>
> >> That has the advantage of checking whether a port is actually in use
> >> (using 'ss' - although it does limit the test to Linux-only; perhaps
> >> using socat instead of ss could make the test portable to non-Linux?)
> > 
> > But doesn't that give you race conditions?  That's the point of this
> > series, so you can run multiple instances of 147 concurrently.
> 
> Hmm - that does imply that common.nbd's use of ss IS racy because it
> checks in linear fashion and has a TOCTTOU window (affects at least
> iotest 233). Your observation that random probes within a range are less
> susceptible (although not immune) to the race is correct.
> 
> >> Do you actually need to attempt a qemu-nbd process, if you take my
> >> suggestion of using ss to probe for an unused port?  And if not, do we
> >> still need qemu_nbd_pipe() added earlier in the series?
> >>
> >>
> >>> -        address = { 'type': 'inet',
> >>> -                    'data': {
> >>> -                        'host': 'localhost',
> >>> -                        'port': str(NBD_PORT)
> >>> -                    } }
> >>> -        self._server_up(address, export_name)
> >>> +        while True:
> >>> +            nbd_port = random.randrange(NBD_PORT_START, NBD_PORT_END)
> >>
> >> common.nbd just iterates, instead of trying random ports.
> > 
> > I'm not sure which is better.  Iterating gives guaranteed termination,
> > trying random ports means the first one you try will usually work.
> 
> Is there any other way we can make the test more robust, perhaps by
> using socket activation (that is, pre-open the port prior to starting
> qemu_nbd, so that our code for finding a free socket is more easily
> reusable), or by using Unix sockets for test 147 (that test seems to be
> using TCP sockets only as a means to get to the real feature under test,
> and not as the actual thing being tested)?

The problem with using socket activation is that you then are not getting
test coverage of the non-activation code paths which are quite significant
things we really want to be testing.

I do wonder if there's a case to be made for having iotests run inside a
container with private network namespace such that they then have a
predictable environment.  You could then simply declare that if a test
needs a TCP port, it should use  "port 9000 + $TEST_NUM". So every
test can safely run in parallel.

If the entire test harness needs to be run multiple in parallel each
run woudl be a separate container, and so again avoid clashing.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 415 bytes --]

On 23.01.19 14:05, Max Reitz wrote:
> On 21.01.19 21:50, John Snow wrote:

[...]

>> I guess my only other observation is that we have a lot of "while True"
>> loops now, is it worth creating some kind of helper that does the dirty
>> work of finding a serviceable port or nah?
> 
> Seems reasonable, I'll see how it looks.

If I do that, the diff stat looks to be 40+, 33-.  Is that worth it?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[John Snow]

On 1/23/19 12:34 PM, Max Reitz wrote:
> On 23.01.19 14:05, Max Reitz wrote:
>> On 21.01.19 21:50, John Snow wrote:
> 
> [...]
> 
>>> I guess my only other observation is that we have a lot of "while True"
>>> loops now, is it worth creating some kind of helper that does the dirty
>>> work of finding a serviceable port or nah?
>>
>> Seems reasonable, I'll see how it looks.
> 
> If I do that, the diff stat looks to be 40+, 33-.  Is that worth it?
> 
> Max
> 

Instead of:
 1 file changed, 68 insertions(+), 30 deletions(-)

Or in addition to?

Use your own judg[e?]ment on it, I guess it's local to this one iotest
for now anyway, so if it's not an obvious win just skip it.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] iotests: Allow 147 to be run concurrently

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

On 23.01.19 18:47, John Snow wrote:
> 
> 
> On 1/23/19 12:34 PM, Max Reitz wrote:
>> On 23.01.19 14:05, Max Reitz wrote:
>>> On 21.01.19 21:50, John Snow wrote:
>>
>> [...]
>>
>>>> I guess my only other observation is that we have a lot of "while True"
>>>> loops now, is it worth creating some kind of helper that does the dirty
>>>> work of finding a serviceable port or nah?
>>>
>>> Seems reasonable, I'll see how it looks.
>>
>> If I do that, the diff stat looks to be 40+, 33-.  Is that worth it?
>>
>> Max
>>
> 
> Instead of:
>  1 file changed, 68 insertions(+), 30 deletions(-)
> 
> Or in addition to?

In addition to.

> Use your own judg[e?]ment on it, I guess it's local to this one iotest
> for now anyway, so if it's not an obvious win just skip it.

OK.  I guess I'll skip it then, but let it be noted that your criticism
was listened to. :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/3] iotests: Allow 147 to be run concurrently

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

On 22.12.18 00:47, Max Reitz wrote:
> 147 currently cannot be run concurrently for two reasons:
> 
> (1) It creates TCP/IP NBD servers on a fixed port.
> 
> (2) It uses a mix of "0.0.0.0", "localhost", and "::1" as host addresses
>     to bind to.  As explained in the commit messages of patches 2 and 3,
>     this results in it potentially actually being able to set up two
>     servers on the same port at the same time -- but connecting to one
>     will always lead to the IPv6 server (regardless of whether you use
>     "localhost" or "::1").  Therefore, even if you get two servers
>     running concurrently, one of the tests will still break because it
>     connects to the wrong one.
> 
> This series fixes these issues.
> 
> 
> Note that even if you do not care about concurrency, it still is a good
> idea to make 147 not use a fixed port to create NBD servers on, as it
> may always be already in use by a totally different application.
> 
> 
> Max Reitz (3):
>   iotests.py: Add qemu_nbd_pipe()
>   iotests: Bind qemu-nbd to localhost in 147
>   iotests: Allow 147 to be run concurrently
> 
>  tests/qemu-iotests/147        | 98 ++++++++++++++++++++++++-----------
>  tests/qemu-iotests/iotests.py | 14 +++++
>  2 files changed, 82 insertions(+), 30 deletions(-)

Applied to my block branch.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]