Repository: postgrespro/testgres

Issue:
PostgresNode and ConnectionParams
Hello All,

I offer to do not use ConnectionParams object in PostgresNode.

PostgresNode must use os_ops instead.

What does it mean?

It means that if you want to use PostgresNode with RemoteOperations object, you have to explicitly create RemoteOperations object and use it to create PostgresNode object.

It allows us to create more clear/exact/simple code. Now  we can to create PostgresNode to work with remote host by two ways - through conn_params and through using of prepared os_ops object. New code will allow only one way - through os_ops.

To backward compatibility we can save conn_params parameter in PostgresNode constructor with None default value and verification that it is None. In the future conn_params will be removed at all.

Also, if we want to pass port_manager to PostgresNode constructor, we have to pass valid os_ops parameter, too. This will be checked in new code.

Discussion:

Solution patch:
diff --git a/setup.py b/setup.py
index 2c44b18..0b20918 100755
--- a/setup.py
+++ b/setup.py
@@ -29,7 +29,7 @@ with open('README.md', 'r') as f:
 setup(
     version='1.11.0',
     name='testgres',
-    packages=['testgres', 'testgres.operations'],
+    packages=['testgres', 'testgres.operations', 'testgres.impl'],
     description='Testing utility for PostgreSQL and its extensions',
     url='https://github.com/postgrespro/testgres',
     long_description=readme,
diff --git a/testgres/impl/port_manager__generic.py b/testgres/impl/port_manager__generic.py
new file mode 100755
index 0000000..a51af2b
--- /dev/null
+++ b/testgres/impl/port_manager__generic.py
@@ -0,0 +1,64 @@
+from ..operations.os_ops import OsOperations
+
+from ..port_manager import PortManager
+from ..exceptions import PortForException
+
+import threading
+import random
+import typing
+
+
+class PortManager__Generic(PortManager):
+    _os_ops: OsOperations
+    _guard: object
+    # TODO: is there better to use bitmap fot _available_ports?
+    _available_ports: typing.Set[int]
+    _reserved_ports: typing.Set[int]
+
+    def __init__(self, os_ops: OsOperations):
+        assert os_ops is not None
+        assert isinstance(os_ops, OsOperations)
+        self._os_ops = os_ops
+        self._guard = threading.Lock()
+        self._available_ports: typing.Set[int] = set(range(1024, 65535))
+        self._reserved_ports: typing.Set[int] = set()
+
+    def reserve_port(self) -> int:
+        assert self._guard is not None
+        assert type(self._available_ports) == set  # noqa: E721t
+        assert type(self._reserved_ports) == set  # noqa: E721
+
+        with self._guard:
+            t = tuple(self._available_ports)
+            assert len(t) == len(self._available_ports)
+            sampled_ports = random.sample(t, min(len(t), 100))
+            t = None
+
+            for port in sampled_ports:
+                assert not (port in self._reserved_ports)
+                assert port in self._available_ports
+
+                if not self._os_ops.is_port_free(port):
+                    continue
+
+                self._reserved_ports.add(port)
+                self._available_ports.discard(port)
+                assert port in self._reserved_ports
+                assert not (port in self._available_ports)
+                return port
+
+        raise PortForException("Can't select a port.")
+
+    def release_port(self, number: int) -> None:
+        assert type(number) == int  # noqa: E721
+
+        assert self._guard is not None
+        assert type(self._reserved_ports) == set  # noqa: E721
+
+        with self._guard:
+            assert number in self._reserved_ports
+            assert not (number in self._available_ports)
+            self._available_ports.add(number)
+            self._reserved_ports.discard(number)
+            assert not (number in self._reserved_ports)
+            assert number in self._available_ports
diff --git a/testgres/impl/port_manager__this_host.py b/testgres/impl/port_manager__this_host.py
new file mode 100755
index 0000000..0d56f35
--- /dev/null
+++ b/testgres/impl/port_manager__this_host.py
@@ -0,0 +1,33 @@
+from ..port_manager import PortManager
+
+from .. import utils
+
+import threading
+
+
+class PortManager__ThisHost(PortManager):
+    sm_single_instance: PortManager = None
+    sm_single_instance_guard = threading.Lock()
+
+    @staticmethod
+    def get_single_instance() -> PortManager:
+        assert __class__ == PortManager__ThisHost
+        assert __class__.sm_single_instance_guard is not None
+
+        if __class__.sm_single_instance is not None:
+            assert type(__class__.sm_single_instance) == __class__  # noqa: E721
+            return __class__.sm_single_instance
+
+        with __class__.sm_single_instance_guard:
+            if __class__.sm_single_instance is None:
+                __class__.sm_single_instance = __class__()
+        assert __class__.sm_single_instance is not None
+        assert type(__class__.sm_single_instance) == __class__  # noqa: E721
+        return __class__.sm_single_instance
+
+    def reserve_port(self) -> int:
+        return utils.reserve_port()
+
+    def release_port(self, number: int) -> None:
+        assert type(number) == int  # noqa: E721
+        return utils.release_port(number)
diff --git a/testgres/node.py b/testgres/node.py
index defc0b4..80ac5ee 100644
--- a/testgres/node.py
+++ b/testgres/node.py
@@ -84,8 +84,8 @@ from .exceptions import \
     InvalidOperationException
 
 from .port_manager import PortManager
-from .port_manager import PortManager__ThisHost
-from .port_manager import PortManager__Generic
+from .impl.port_manager__this_host import PortManager__ThisHost
+from .impl.port_manager__generic import PortManager__Generic
 
 from .logger import TestgresLogger
 
@@ -107,7 +107,6 @@ from .backup import NodeBackup
 from .operations.os_ops import ConnectionParams
 from .operations.os_ops import OsOperations
 from .operations.local_ops import LocalOperations
-from .operations.remote_ops import RemoteOperations
 
 InternalError = pglib.InternalError
 ProgrammingError = pglib.ProgrammingError
@@ -151,7 +150,7 @@ class PostgresNode(object):
                  name=None,
                  base_dir=None,
                  port: typing.Optional[int] = None,
-                 conn_params: ConnectionParams = ConnectionParams(),
+                 conn_params: ConnectionParams = None,
                  bin_dir=None,
                  prefix=None,
                  os_ops: typing.Optional[OsOperations] = None,
@@ -171,11 +170,15 @@ class PostgresNode(object):
         assert os_ops is None or isinstance(os_ops, OsOperations)
         assert port_manager is None or isinstance(port_manager, PortManager)
 
+        if conn_params is not None:
+            assert type(conn_params) == ConnectionParams  # noqa: E721
+
+            raise InvalidOperationException("conn_params is deprecated, please use os_ops parameter instead.")
+
         # private
         if os_ops is None:
-            self._os_ops = __class__._get_os_ops(conn_params)
+            self._os_ops = __class__._get_os_ops()
         else:
-            assert conn_params is None
             assert isinstance(os_ops, OsOperations)
             self._os_ops = os_ops
             pass
@@ -200,11 +203,14 @@ class PostgresNode(object):
             self._should_free_port = False
             self._port_manager = None
         else:
-            if port_manager is not None:
+            if port_manager is None:
+                self._port_manager = __class__._get_port_manager(self._os_ops)
+            elif os_ops is None:
+                raise InvalidOperationException("When port_manager is not None you have to define os_ops, too.")
+            else:
                 assert isinstance(port_manager, PortManager)
+                assert self._os_ops is os_ops
                 self._port_manager = port_manager
-            else:
-                self._port_manager = __class__._get_port_manager(self._os_ops)
 
             assert self._port_manager is not None
             assert isinstance(self._port_manager, PortManager)
@@ -255,16 +261,11 @@ class PostgresNode(object):
         )
 
     @staticmethod
-    def _get_os_ops(conn_params: ConnectionParams) -> OsOperations:
+    def _get_os_ops() -> OsOperations:
         if testgres_config.os_ops:
             return testgres_config.os_ops
 
-        assert type(conn_params) == ConnectionParams  # noqa: E721
-
-        if conn_params.ssh_key:
-            return RemoteOperations(conn_params)
-
-        return LocalOperations(conn_params)
+        return LocalOperations()
 
     @staticmethod
     def _get_port_manager(os_ops: OsOperations) -> PortManager:
@@ -272,7 +273,7 @@ class PostgresNode(object):
         assert isinstance(os_ops, OsOperations)
 
         if isinstance(os_ops, LocalOperations):
-            return PortManager__ThisHost()
+            return PortManager__ThisHost.get_single_instance()
 
         # TODO: Throw the exception "Please define a port manager." ?
         return PortManager__Generic(os_ops)
@@ -294,7 +295,6 @@ class PostgresNode(object):
         node = PostgresNode(
             name=name,
             base_dir=base_dir,
-            conn_params=None,
             bin_dir=self._bin_dir,
             prefix=self._prefix,
             os_ops=self._os_ops,
diff --git a/testgres/port_manager.py b/testgres/port_manager.py
index e253047..1ae696c 100644
--- a/testgres/port_manager.py
+++ b/testgres/port_manager.py
@@ -1,14 +1,3 @@
-from .operations.os_ops import OsOperations
-
-from .exceptions import PortForException
-
-from . import utils
-
-import threading
-import random
-import typing
-
-
 class PortManager:
     def __init__(self):
         super().__init__()
@@ -19,85 +8,3 @@ class PortManager:
     def release_port(self, number: int) -> None:
         assert type(number) == int  # noqa: E721
         raise NotImplementedError("PortManager::release_port is not implemented.")
-
-
-class PortManager__ThisHost(PortManager):
-    sm_single_instance: PortManager = None
-    sm_single_instance_guard = threading.Lock()
-
-    def __init__(self):
-        pass
-
-    def __new__(cls) -> PortManager:
-        assert __class__ == PortManager__ThisHost
-        assert __class__.sm_single_instance_guard is not None
-
-        if __class__.sm_single_instance is None:
-            with __class__.sm_single_instance_guard:
-                __class__.sm_single_instance = super().__new__(cls)
-        assert __class__.sm_single_instance
-        assert type(__class__.sm_single_instance) == __class__  # noqa: E721
-        return __class__.sm_single_instance
-
-    def reserve_port(self) -> int:
-        return utils.reserve_port()
-
-    def release_port(self, number: int) -> None:
-        assert type(number) == int  # noqa: E721
-        return utils.release_port(number)
-
-
-class PortManager__Generic(PortManager):
-    _os_ops: OsOperations
-    _guard: object
-    # TODO: is there better to use bitmap fot _available_ports?
-    _available_ports: typing.Set[int]
-    _reserved_ports: typing.Set[int]
-
-    def __init__(self, os_ops: OsOperations):
-        assert os_ops is not None
-        assert isinstance(os_ops, OsOperations)
-        self._os_ops = os_ops
-        self._guard = threading.Lock()
-        self._available_ports: typing.Set[int] = set(range(1024, 65535))
-        self._reserved_ports: typing.Set[int] = set()
-
-    def reserve_port(self) -> int:
-        assert self._guard is not None
-        assert type(self._available_ports) == set  # noqa: E721t
-        assert type(self._reserved_ports) == set  # noqa: E721
-
-        with self._guard:
-            t = tuple(self._available_ports)
-            assert len(t) == len(self._available_ports)
-            sampled_ports = random.sample(t, min(len(t), 100))
-            t = None
-
-            for port in sampled_ports:
-                assert not (port in self._reserved_ports)
-                assert port in self._available_ports
-
-                if not self._os_ops.is_port_free(port):
-                    continue
-
-                self._reserved_ports.add(port)
-                self._available_ports.discard(port)
-                assert port in self._reserved_ports
-                assert not (port in self._available_ports)
-                return port
-
-        raise PortForException("Can't select a port.")
-
-    def release_port(self, number: int) -> None:
-        assert type(number) == int  # noqa: E721
-
-        assert self._guard is not None
-        assert type(self._reserved_ports) == set  # noqa: E721
-
-        with self._guard:
-            assert number in self._reserved_ports
-            assert not (number in self._available_ports)
-            self._available_ports.add(number)
-            self._reserved_ports.discard(number)
-            assert not (number in self._reserved_ports)
-            assert number in self._available_ports
diff --git a/testgres/utils.py b/testgres/utils.py
index 2ff6f2a..6603c92 100644
--- a/testgres/utils.py
+++ b/testgres/utils.py
@@ -6,8 +6,6 @@ from __future__ import print_function
 import os
 
 import sys
-import socket
-import random
 
 from contextlib import contextmanager
 from packaging.version import Version, InvalidVersion
@@ -15,18 +13,27 @@ import re
 
 from six import iteritems
 
-from .exceptions import PortForException
 from .exceptions import ExecUtilException
 from .config import testgres_config as tconf
 from .operations.os_ops import OsOperations
 from .operations.remote_ops import RemoteOperations
+from .operations.local_ops import LocalOperations
 from .operations.helpers import Helpers as OsHelpers
 
+from .impl.port_manager__generic import PortManager__Generic
+
 # rows returned by PG_CONFIG
 _pg_config_data = {}
 
+_local_operations = LocalOperations()
+
+#
+# The old, global "port manager" always worked with LOCAL system
+#
+_old_port_manager = PortManager__Generic(_local_operations)
+
 # ports used by nodes
-bound_ports = set()
+bound_ports = _old_port_manager._reserved_ports
 
 
 # re-export version type
@@ -43,28 +50,7 @@ def internal__reserve_port():
     """
     Generate a new port and add it to 'bound_ports'.
     """
-    def LOCAL__is_port_free(port: int) -> bool:
-        """Check if a port is free to use."""
-        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
-            try:
-                s.bind(("", port))
-                return True
-            except OSError:
-                return False
-
-    ports = set(range(1024, 65535))
-    assert type(ports) == set  # noqa: E721
-    assert type(bound_ports) == set  # noqa: E721
-    ports.difference_update(bound_ports)
-
-    sampled_ports = random.sample(tuple(ports), min(len(ports), 100))
-
-    for port in sampled_ports:
-        if LOCAL__is_port_free(port):
-            bound_ports.add(port)
-            return port
-
-    raise PortForException("Can't select a port")
+    return _old_port_manager.reserve_port()
 
 
 def internal__release_port(port):
@@ -73,9 +59,7 @@ def internal__release_port(port):
     """
 
     assert type(port) == int  # noqa: E721
-    assert port in bound_ports
-
-    bound_ports.discard(port)
+    return _old_port_manager.release_port(port)
 
 
 reserve_port = internal__reserve_port


Test patch:
diff --git a/tests/helpers/global_data.py b/tests/helpers/global_data.py
index 51bf448..07ac083 100644
--- a/tests/helpers/global_data.py
+++ b/tests/helpers/global_data.py
@@ -39,7 +39,7 @@ class OsOpsDescrs:
 class PortManagers:
     sm_remote_port_manager = PortManager__Generic(OsOpsDescrs.sm_remote_os_ops)
 
-    sm_local_port_manager = PortManager__ThisHost()
+    sm_local_port_manager = PortManager__ThisHost.get_single_instance()
 
     sm_local2_port_manager = PortManager__Generic(OsOpsDescrs.sm_local_os_ops)
 
diff --git a/tests/test_testgres_common.py b/tests/test_testgres_common.py
index b71dc5a..5b926bc 100644
--- a/tests/test_testgres_common.py
+++ b/tests/test_testgres_common.py
@@ -1487,7 +1487,6 @@ class TestTestgresCommon:
         return PostgresNode(
             name,
             port=port,
-            conn_params=None,
             os_ops=node_svc.os_ops,
             port_manager=port_manager if port is None else None
         )
diff --git a/tests/test_testgres_remote.py b/tests/test_testgres_remote.py
index 87cc026..6a8d068 100755
--- a/tests/test_testgres_remote.py
+++ b/tests/test_testgres_remote.py
@@ -173,7 +173,6 @@ class TestTestgresRemote:
 
         return testgres.PostgresNode(
             name,
-            conn_params=None,
             os_ops=svc.os_ops,
             port_manager=svc.port_manager)