| Message ID | 20240903143553.16877-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tests/functional/test_vnc: Reduce raciness in find_free_ports() | expand |
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 ? With regards, Daniel
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. Thomas
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
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))
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(-)