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é <phi...@linaro.org>
> > > ---
> > > Based-on: <20240830133841.142644-33-th...@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 :|
