Re: [PATCH] tests/functional/test_vnc: Reduce raciness in find_free_ports()

[Daniel P . Berrangé]

On Wed, Sep 04, 2024 at 08:20:12AM +0200, Thomas Huth wrote:
> On 03/09/2024 16.50, Daniel P. Berrangé wrote:
> > On Tue, Sep 03, 2024 at 04:35:53PM +0200, Philippe Mathieu-Daudé wrote:
> > > Pass the port range as argument. In order to reduce races
> > > when looking for free ports, use a per-target per-process
> > > base port (based on the target built-in hash).
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > > Based-on: <20240830133841.142644-33-thuth@redhat.com>
> > > ---
> > >   tests/functional/test_vnc.py | 12 ++++++++----
> > >   1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tests/functional/test_vnc.py b/tests/functional/test_vnc.py
> > > index b769d3b268..508db0709d 100755
> > > --- a/tests/functional/test_vnc.py
> > > +++ b/tests/functional/test_vnc.py
> > > @@ -10,6 +10,7 @@
> > >   # This work is licensed under the terms of the GNU GPL, version 2 or
> > >   # later.  See the COPYING file in the top-level directory.
> > > +import os
> > >   import socket
> > >   from typing import List
> > > @@ -18,7 +19,6 @@
> > >   VNC_ADDR = '127.0.0.1'
> > >   VNC_PORT_START = 32768
> > > -VNC_PORT_END = VNC_PORT_START + 1024
> > >   def check_bind(port: int) -> bool:
> > > @@ -41,9 +41,10 @@ def check_connect(port: int) -> bool:
> > >       return True
> > > -def find_free_ports(count: int) -> List[int]:
> > > +# warning, racy function
> > > +def find_free_ports(portrange, count: int) -> List[int]:
> > >       result = []
> > > -    for port in range(VNC_PORT_START, VNC_PORT_END):
> > > +    for port in portrange:
> > >           if check_bind(port):
> > >               result.append(port)
> > >               if len(result) >= count:
> > > @@ -91,7 +92,10 @@ def test_change_password(self):
> > >                       password='new_password')
> > >       def test_change_listen(self):
> > > -        a, b, c = find_free_ports(3)
> > > +        per_arch_port_base = abs((os.getpid() + hash(self.arch)) % (10 
> > > ** 4))
> > > +        port_start = VNC_PORT_START + per_arch_port_base
> > > +        port_stop = port_start + 100
> > > +        a, b, c = find_free_ports(range(port_start, port_stop), 3)
> > >           self.assertFalse(check_connect(a))
> > >           self.assertFalse(check_connect(b))
> > >           self.assertFalse(check_connect(c))
> > 
> > As your comment says, this is still racey, and its also not too
> > nice to read & understand this logic. How about we just make
> > test_vnc.py be serialized wrt itself ?
> 
> We'll likely have more tests that need a free port in the future...
> tests/avocado/migration.py and tests/avocado/reverse_debugging.py use
> find_free_ports(), too, so we should maybe think of a logic that avoids
> clashes between different tests, too.

Create a context manager that holds a fcntl lockfile on disk, while
giving you a free port ?

   class FreePort:
     def __enter__(self):
        self.num = find_free_port()
        ..acquire fcntl lock...

     def __exit__(self):
        ...release fcntl lock..

Letting tests do

  with FreePort() as port:
      ..do some test that uses port.num....


With 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 :|