[PATCH] tests/functional: Use more fine-grained locking when looking for free ports

[PATCH] tests/functional: Use more fine-grained locking when looking for free ports

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Currently, we have one lock that is held while a test is looking for
free ports. However, we are also using different ranges for looking
for free ports nowadays (PORTS_START is based on the PID of the process),
so instead of using only one lock, we should rather use a lock per
range instead. This should help to allow running more tests in parallel.

While we're at it, also create the lock files without executable bit
(mode is 0o777 by default).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/qemu_test/ports.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/functional/qemu_test/ports.py b/tests/functional/qemu_test/ports.py
index 631b77abf6b..81174a61532 100644
--- a/tests/functional/qemu_test/ports.py
+++ b/tests/functional/qemu_test/ports.py
@@ -23,8 +23,9 @@ class Ports():
     PORTS_END = PORTS_START + PORTS_RANGE_SIZE
 
     def __enter__(self):
-        lock_file = os.path.join(BUILD_DIR, "tests", "functional", "port_lock")
-        self.lock_fh = os.open(lock_file, os.O_CREAT)
+        lock_file = os.path.join(BUILD_DIR, "tests", "functional",
+                                 f".port_lock.{self.PORTS_START}")
+        self.lock_fh = os.open(lock_file, os.O_CREAT, mode=0o666)
         fcntl.flock(self.lock_fh, fcntl.LOCK_EX)
         return self
 
-- 
2.50.1

Re: [PATCH] tests/functional: Use more fine-grained locking when looking for free ports

[Daniel P. Berrangé]

On Thu, Aug 21, 2025 at 11:47:35AM +0200, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> Currently, we have one lock that is held while a test is looking for
> free ports. However, we are also using different ranges for looking
> for free ports nowadays (PORTS_START is based on the PID of the process),
> so instead of using only one lock, we should rather use a lock per
> range instead. This should help to allow running more tests in parallel.
> 
> While we're at it, also create the lock files without executable bit
> (mode is 0o777 by default).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/functional/qemu_test/ports.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

Re: [PATCH] tests/functional: Use more fine-grained locking when looking for free ports

[Manos Pitsidianakis]

On Thu, Aug 21, 2025 at 12:49 PM Thomas Huth <thuth@redhat.com> wrote:
>
> From: Thomas Huth <thuth@redhat.com>
>
> Currently, we have one lock that is held while a test is looking for
> free ports. However, we are also using different ranges for looking
> for free ports nowadays (PORTS_START is based on the PID of the process),
> so instead of using only one lock, we should rather use a lock per
> range instead. This should help to allow running more tests in parallel.
>
> While we're at it, also create the lock files without executable bit
> (mode is 0o777 by default).
>

(Unrelated to this patch but the file itself)

Hm. AF_INET supports binding to port 0 to connect to any available
port (see man 7 ip). Is this not portable?

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/functional/qemu_test/ports.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tests/functional/qemu_test/ports.py b/tests/functional/qemu_test/ports.py
> index 631b77abf6b..81174a61532 100644
> --- a/tests/functional/qemu_test/ports.py
> +++ b/tests/functional/qemu_test/ports.py
> @@ -23,8 +23,9 @@ class Ports():
>      PORTS_END = PORTS_START + PORTS_RANGE_SIZE
>
>      def __enter__(self):
> -        lock_file = os.path.join(BUILD_DIR, "tests", "functional", "port_lock")
> -        self.lock_fh = os.open(lock_file, os.O_CREAT)
> +        lock_file = os.path.join(BUILD_DIR, "tests", "functional",
> +                                 f".port_lock.{self.PORTS_START}")
> +        self.lock_fh = os.open(lock_file, os.O_CREAT, mode=0o666)
>          fcntl.flock(self.lock_fh, fcntl.LOCK_EX)
>          return self
>
> --
> 2.50.1
>
>

Re: [PATCH] tests/functional: Use more fine-grained locking when looking for free ports

[Thomas Huth]

On 25/08/2025 09.30, Manos Pitsidianakis wrote:
> On Thu, Aug 21, 2025 at 12:49 PM Thomas Huth <thuth@redhat.com> wrote:
>>
>> From: Thomas Huth <thuth@redhat.com>
>>
>> Currently, we have one lock that is held while a test is looking for
>> free ports. However, we are also using different ranges for looking
>> for free ports nowadays (PORTS_START is based on the PID of the process),
>> so instead of using only one lock, we should rather use a lock per
>> range instead. This should help to allow running more tests in parallel.
>>
>> While we're at it, also create the lock files without executable bit
>> (mode is 0o777 by default).
>>
> 
> (Unrelated to this patch but the file itself)
> 
> Hm. AF_INET supports binding to port 0 to connect to any available
> port (see man 7 ip). Is this not portable?

No clue ... but in that case, we'd need to go back to only use one lock for 
all tests that are running in parallel, so it might cause some more contention?

  Thomas

Re: [PATCH] tests/functional: Use more fine-grained locking when looking for free ports

[Manos Pitsidianakis]

On Mon, Aug 25, 2025 at 11:47 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 25/08/2025 09.30, Manos Pitsidianakis wrote:
> > On Thu, Aug 21, 2025 at 12:49 PM Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> From: Thomas Huth <thuth@redhat.com>
> >>
> >> Currently, we have one lock that is held while a test is looking for
> >> free ports. However, we are also using different ranges for looking
> >> for free ports nowadays (PORTS_START is based on the PID of the process),
> >> so instead of using only one lock, we should rather use a lock per
> >> range instead. This should help to allow running more tests in parallel.
> >>
> >> While we're at it, also create the lock files without executable bit
> >> (mode is 0o777 by default).
> >>
> >
> > (Unrelated to this patch but the file itself)
> >
> > Hm. AF_INET supports binding to port 0 to connect to any available
> > port (see man 7 ip). Is this not portable?
>
> No clue ... but in that case, we'd need to go back to only use one lock for
> all tests that are running in parallel, so it might cause some more contention?

IIUC there would be no need for locking, since the kernel would return
a free port for each process.

I can submit a patch btw, but thought I could ask first.

-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd

Re: [PATCH] tests/functional: Use more fine-grained locking when looking for free ports

[Thomas Huth]

On 25/08/2025 10.51, Manos Pitsidianakis wrote:
> On Mon, Aug 25, 2025 at 11:47 AM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 25/08/2025 09.30, Manos Pitsidianakis wrote:
>>> On Thu, Aug 21, 2025 at 12:49 PM Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> From: Thomas Huth <thuth@redhat.com>
>>>>
>>>> Currently, we have one lock that is held while a test is looking for
>>>> free ports. However, we are also using different ranges for looking
>>>> for free ports nowadays (PORTS_START is based on the PID of the process),
>>>> so instead of using only one lock, we should rather use a lock per
>>>> range instead. This should help to allow running more tests in parallel.
>>>>
>>>> While we're at it, also create the lock files without executable bit
>>>> (mode is 0o777 by default).
>>>>
>>>
>>> (Unrelated to this patch but the file itself)
>>>
>>> Hm. AF_INET supports binding to port 0 to connect to any available
>>> port (see man 7 ip). Is this not portable?
>>
>> No clue ... but in that case, we'd need to go back to only use one lock for
>> all tests that are running in parallel, so it might cause some more contention?
> 
> IIUC there would be no need for locking, since the kernel would return
> a free port for each process.

That only works within a process, doesn't it? The problem here is that the 
test itself does not open the port, it just first needs to figure out a free 
port that will be used later.
The test then launches QEMU with that port number, which will then open this 
port. Then the test connects to that port number (which it must know) to 
interact with QEMU.
If QEMU is using port 0 for that, the test might not have a way to retrieve 
the final port number from QEMU in all cases.

  Thomas

Re: [PATCH] tests/functional: Use more fine-grained locking when looking for free ports

[Manos Pitsidianakis]

On Mon, Aug 25, 2025 at 12:04 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 25/08/2025 10.51, Manos Pitsidianakis wrote:
> > On Mon, Aug 25, 2025 at 11:47 AM Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> On 25/08/2025 09.30, Manos Pitsidianakis wrote:
> >>> On Thu, Aug 21, 2025 at 12:49 PM Thomas Huth <thuth@redhat.com> wrote:
> >>>>
> >>>> From: Thomas Huth <thuth@redhat.com>
> >>>>
> >>>> Currently, we have one lock that is held while a test is looking for
> >>>> free ports. However, we are also using different ranges for looking
> >>>> for free ports nowadays (PORTS_START is based on the PID of the process),
> >>>> so instead of using only one lock, we should rather use a lock per
> >>>> range instead. This should help to allow running more tests in parallel.
> >>>>
> >>>> While we're at it, also create the lock files without executable bit
> >>>> (mode is 0o777 by default).
> >>>>
> >>>
> >>> (Unrelated to this patch but the file itself)
> >>>
> >>> Hm. AF_INET supports binding to port 0 to connect to any available
> >>> port (see man 7 ip). Is this not portable?
> >>
> >> No clue ... but in that case, we'd need to go back to only use one lock for
> >> all tests that are running in parallel, so it might cause some more contention?
> >
> > IIUC there would be no need for locking, since the kernel would return
> > a free port for each process.
>
> That only works within a process, doesn't it? The problem here is that the
> test itself does not open the port, it just first needs to figure out a free
> port that will be used later.
> The test then launches QEMU with that port number, which will then open this
> port. Then the test connects to that port number (which it must know) to
> interact with QEMU.
> If QEMU is using port 0 for that, the test might not have a way to retrieve
> the final port number from QEMU in all cases.
>
>   Thomas
>

Ah yeah, correct. It depends on what the test wants to do with that port.

Thanks

-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd