Don't reserve a new port if port was set up

Author: vshepard

testgres/node.py

@@ -96,7 +96,6 @@
 
 from .operations.os_ops import ConnectionParams
 from .operations.local_ops import LocalOperations
-from .operations.remote_ops import RemoteOperations
 
 InternalError = pglib.InternalError
 ProgrammingError = pglib.ProgrammingError
@@ -487,7 +486,7 @@ def init(self, initdb_params=None, cached=True, **kwargs):
             os_ops=self.os_ops,
             params=initdb_params,
             bin_path=self.bin_dir,
-            cached=False)
+            cached=cached)
 
         # initialize default config files
         self.default_conf(**kwargs)
@@ -717,9 +716,9 @@ def slow_start(self, replica=False, dbname='template1', username=None, max_attem
                                         OperationalError},
                               max_attempts=max_attempts)
 
-    def start(self, params=[], wait=True):
+    def start(self, params=None, wait: bool = True) -> 'PostgresNode':
         """
-        Starts the PostgreSQL node using pg_ctl if node has not been started.
+        Starts the PostgreSQL node using pg_ctl if the node has not been started.
         By default, it waits for the operation to complete before returning.
         Optionally, it can return immediately without waiting for the start operation
         to complete by setting the `wait` parameter to False.
@@ -731,45 +730,62 @@ def start(self, params=[], wait=True):
         Returns:
             This instance of :class:`.PostgresNode`.
         """
+        if params is None:
+            params = []
         if self.is_started:
             return self
 
         _params = [
-            self._get_bin_path("pg_ctl"),
-            "-D", self.data_dir,
-            "-l", self.pg_log_file,
-            "-w" if wait else '-W',  # --wait or --no-wait
-            "start"
-        ] + params  # yapf: disable
+                      self._get_bin_path("pg_ctl"),
+                      "-D", self.data_dir,
+                      "-l", self.pg_log_file,
+                      "-w" if wait else '-W',  # --wait or --no-wait
+                      "start"
+                  ] + params  # yapf: disable
 
-        startup_retries = 5
-        while True:
+        max_retries = 5
+        sleep_interval = 5  # seconds
+
+        for attempt in range(max_retries):
             try:
                 exit_status, out, error = execute_utility(_params, self.utils_log_file, verbose=True)
                 if error and 'does not exist' in error:
                     raise Exception
+                break  # Exit the loop if successful
             except Exception as e:
-                files = self._collect_special_files()
-                if any(len(file) > 1 and 'Is another postmaster already '
-                                         'running on port' in file[1].decode() for
-                       file in files):
-                    logging.warning("Detected an issue with connecting to port {0}. "
-                                    "Trying another port after a 5-second sleep...".format(self.port))
-                    self.port = reserve_port()
-                    options = {'port': str(self.port)}
-                    self.set_auto_conf(options)
-                    startup_retries -= 1
-                    time.sleep(5)
-                    continue
-
-                msg = 'Cannot start node'
-                raise_from(StartNodeException(msg, files), e)
-            break
+                if self._handle_port_conflict():
+                    if attempt < max_retries - 1:
+                        logging.info(f"Retrying start operation (Attempt {attempt + 2}/{max_retries})...")
+                        time.sleep(sleep_interval)
+                        continue
+                    else:
+                        logging.error("Reached maximum retry attempts. Unable to start node.")
+                        raise StartNodeException("Cannot start node after multiple attempts",
+                                                 self._collect_special_files()) from e
+                raise StartNodeException("Cannot start node", self._collect_special_files()) from e
+
         self._maybe_start_logger()
         self.is_started = True
         return self
 
-    def stop(self, params=[], wait=True):
+    def _handle_port_conflict(self) -> bool:
+        """
+        Checks for a port conflict and attempts to resolve it by changing the port.
+        Returns True if the port was changed, False otherwise.
+        """
+        files = self._collect_special_files()
+        if any(len(file) > 1 and 'Is another postmaster already running on port' in file[1].decode() for file in files):
+            logging.warning(f"Port conflict detected on port {self.port}.")
+            if self._should_free_port:
+                logging.warning("Port reservation skipped due to _should_free_port setting.")
+                return False
+            self.port = reserve_port()
+            self.set_auto_conf({'port': str(self.port)})
+            logging.info(f"Port changed to {self.port}.")
+            return True
+        return False
+
+    def stop(self, params=None, wait=True):
         """
         Stops the PostgreSQL node using pg_ctl if the node has been started.
 
@@ -780,6 +796,8 @@ def stop(self, params=[], wait=True):
         Returns:
             This instance of :class:`.PostgresNode`.
         """
+        if params is None:
+            params = []
         if not self.is_started:
             return self
 
@@ -812,7 +830,7 @@ def kill(self, someone=None):
                 os.kill(self.auxiliary_pids[someone][0], sig)
             self.is_started = False
 
-    def restart(self, params=[]):
+    def restart(self, params=None):
         """
         Restart this node using pg_ctl.
 
@@ -823,6 +841,8 @@ def restart(self, params=[]):
             This instance of :class:`.PostgresNode`.
         """
 
+        if params is None:
+            params = []
         _params = [
             self._get_bin_path("pg_ctl"),
             "-D", self.data_dir,
@@ -844,7 +864,7 @@ def restart(self, params=[]):
 
         return self
 
-    def reload(self, params=[]):
+    def reload(self, params=None):
         """
         Asynchronously reload config files using pg_ctl.
 
@@ -855,6 +875,8 @@ def reload(self, params=[]):
             This instance of :class:`.PostgresNode`.
         """
 
+        if params is None:
+            params = []
         _params = [
             self._get_bin_path("pg_ctl"),
             "-D", self.data_dir,
@@ -1587,7 +1609,7 @@ def pgbench_table_checksums(self, dbname="postgres",
         return {(table, self.table_checksum(table, dbname))
                 for table in pgbench_tables}
 
-    def set_auto_conf(self, options, config='postgresql.auto.conf', rm_options={}):
+    def set_auto_conf(self, options, config='postgresql.auto.conf', rm_options=None):
         """
         Update or remove configuration options in the specified configuration file,
         updates the options specified in the options dictionary, removes any options
@@ -1603,6 +1625,8 @@ def set_auto_conf(self, options, config='postgresql.auto.conf', rm_options={}):
                                          Defaults to an empty set.
         """
         # parse postgresql.auto.conf
+        if rm_options is None:
+            rm_options = {}
         path = os.path.join(self.data_dir, config)
 
         lines = self.os_ops.readlines(path)