Re: [PATCH] tests/qemu_iotests: Minimize usage of used ports

[Eric Blake <eblake@redhat.com>]

On 2/3/20 1:59 AM, Lukáš Doktor wrote:
> Using a range of ports from 32768 to 65538 is dangerous as some
> application might already be listening there and interfere with the
> testing. There is no way to reserve ports, but let's decrease the chance
> of interactions by only using ports that were free at the time of
> importing this module.
> 
> Without this patch CI occasionally fails with used ports. Additionally I
> tried listening on the first port to be tried via "nc -l localhost
> $port" and no matter how many other ports were available it always
> hanged for infinity.
> 
> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
> ---
>   tests/qemu-iotests/147        | 43 ++++++++++++++++-------
>   tests/qemu-iotests/iotests.py | 64 +++++++++++++++++++++++++++++++++++
>   2 files changed, 94 insertions(+), 13 deletions(-)

Is it worth sharing the logic already present in common.nbd's 
nbd_server_start_tcp_socket shell function?  (Oh right, that's shell, 
this is python).  It seems like we keep reinventing logic to pick a safe 
port.

> 
> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
> index 2b6f859a09..4d0e1418bb 100755
> --- a/tests/qemu-iotests/147
> +++ b/tests/qemu-iotests/147
> @@ -26,10 +26,8 @@ import time
>   import iotests
>   from iotests import cachemode, aiomode, imgfmt, qemu_img, qemu_nbd, qemu_nbd_early_pipe
>   
> -NBD_PORT_START      = 32768
> -NBD_PORT_END        = NBD_PORT_START + 1024
> -NBD_IPV6_PORT_START = NBD_PORT_END
> -NBD_IPV6_PORT_END   = NBD_IPV6_PORT_START + 1024
> +NBD_PORTS = iotests.find_free_ports(32768, 65536, 1024)
> +NBD_IPV6_PORTS = iotests.find_free_ports(NBD_PORTS[-1] + 1, 65536, 1024)

The changes here look sane...

> +++ b/tests/qemu-iotests/iotests.py
> @@ -20,6 +20,7 @@ from __future__ import print_function
>   import errno
>   import os
>   import re
> +import socket
>   import subprocess
>   import string
>   import unittest
> @@ -75,6 +76,69 @@ luks_default_secret_object = 'secret,id=keysec0,data=' + \
>   luks_default_key_secret_opt = 'key-secret=keysec0'
>   
>   
> +def is_port_free(port, address):

...and I'm glad you're adding a reusable method here.

> +    """
> +    Return True if the given port is available for use.
> +
> +    Currently we only check for TCP/UDP connections on IPv4/6
> +    Backported from avocado.utils.network
> +
> +    :param port: Port number
> +    :param address: Socket address to bind or connect
> +    """
> +    families = (socket.AF_INET, socket.AF_INET6)
> +    if address == "localhost" or not address:
> +        localhost = True
> +        protocols = (socket.SOCK_STREAM, socket.SOCK_DGRAM)
> +    else:
> +        localhost = False
> +        # sock.connect always connects for UDP
> +        protocols = (socket.SOCK_STREAM, )
> +    sock = None
> +    try:
> +        for family in families:
> +            for protocol in protocols:
> +                try:
> +                    sock = socket.socket(family, protocol)
> +                    if localhost:
> +                        sock.bind(("", port))
> +                    else:
> +                        sock.connect((address, port))
> +                        return False
> +                except socket.error as exc:
> +                    if exc.errno in (93, 94):   # Unsupported combinations

Ouch - that seems rather hard-coded (not all the world uses the same 
errno values as Linux).  Does python have symbolic names for 
EPROTONOSUPPORT and ESOCKTNOSUPPORT?

> +                        continue
> +                    if localhost:
> +                        return False
> +                sock.close()
> +        return True
> +    finally:
> +        if sock is not None:
> +            sock.close()
> +
> +
> +def find_free_ports(start_port, end_port, count, address="localhost"):
> +    """
> +    Return count of host free ports in the range [start_port, end_port].
> +
> +    Backported from avocado.utils.network
> +
> +    :param start_port: header of candidate port range
> +    :param end_port: ender of candidate port range

s/ender/end/

> +    :param count: Initial number of ports known to be free in the range.
> +    :param address: Socket address to bind or connect
> +    """
> +    ports = []
> +    port_range = range(start_port, end_port)
> +    for i in port_range:
> +        if is_port_free(i, address):
> +            ports.append(i)
> +        if len(ports) >= count:
> +            break
> +
> +    return ports
> +
> +
>   def qemu_img(*args):
>       '''Run qemu-img and return the exit code'''
>       devnull = open('/dev/null', 'r+')
> 

I'd like a second review on this (my python is tolerable, but not 
strong), but I'm happy to take it through my NBD tree if no one else 
speaks up first.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org