diff mbox series

tests/functional/test_vnc: Reduce raciness in find_free_ports()

Message ID 20240903143553.16877-1-philmd@linaro.org (mailing list archive)
State New, archived
Headers show
Series tests/functional/test_vnc: Reduce raciness in find_free_ports() | expand

Commit Message

Philippe Mathieu-Daudé Sept. 3, 2024, 2:35 p.m. UTC 
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(-)

Comments

Daniel P. Berrangé Sept. 3, 2024, 2:50 p.m. UTC | #1 
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
Thomas Huth Sept. 4, 2024, 6:20 a.m. UTC | #2 
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
Daniel P. Berrangé Sept. 4, 2024, 7:13 a.m. UTC | #3 
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 mbox series

Patch

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