Enforce loop_wait/retry_timeout/ttl rule (#2869)

* hard-code minimal possible values
* make adjustments if values are lower or if the rule is violated and show warnings
* update documentation

docs/dynamic_configuration.rst

@@ -8,9 +8,18 @@ Dynamic configuration is stored in the DCS (Distributed Configuration Store) and
 
 In order to change the dynamic configuration you can use either :ref:`patronictl_edit_config` tool or Patroni :ref:`REST API <rest_api>`.
 
--  **loop\_wait**: the number of seconds the loop will sleep. Default value: 10
--  **ttl**: the TTL to acquire the leader lock (in seconds). Think of it as the length of time before initiation of the automatic failover process. Default value: 30
--  **retry\_timeout**: timeout for DCS and PostgreSQL operation retries (in seconds). DCS or network issues shorter than this will not cause Patroni to demote the leader. Default value: 10
+-  **loop\_wait**: the number of seconds the loop will sleep. Default value: 10, minimum possible value: 1
+-  **ttl**: the TTL to acquire the leader lock (in seconds). Think of it as the length of time before initiation of the automatic failover process. Default value: 30, minimum possible value: 20
+-  **retry\_timeout**: timeout for DCS and PostgreSQL operation retries (in seconds). DCS or network issues shorter than this will not cause Patroni to demote the leader. Default value: 10, minimum possible value: 3
+
+.. warning::
+    when changing values of **loop_wait**, **retry_timeout**, or **ttl** you have to follow the rule:
+
+    .. code-block:: python
+
+        loop_wait + 2 * retry_timeout <= ttl
+
+
 -  **maximum\_lag\_on\_failover**: the maximum bytes a follower may lag to be able to participate in leader election.
 -  **maximum\_lag\_on\_syncnode**: the maximum bytes a synchronous follower may lag before it is considered as an unhealthy candidate and swapped by healthy asynchronous follower. Patroni utilize the max replica lsn if there is more than one follower, otherwise it will use leader's current wal lsn. Default is -1, Patroni will not take action to swap synchronous unhealthy follower when the value is set to 0 or below. Please set the value high enough so Patroni won't swap synchrounous follower fequently during high transaction volume.
 -  **max\_timelines\_history**: maximum number of timeline history items kept in DCS.  Default value: 0. When set to 0, it keeps the full history in DCS.

patroni/config.py

@@ -400,6 +400,66 @@ class Config(object):
                     except Exception:
                         logger.error('Can not remove temporary file %s', tmpfile)
 
+    def __get_and_maybe_adjust_int_value(self, config: Dict[str, Any], param: str, min_value: int) -> int:
+        """Get, validate and maybe adjust a *param* integer value from the *config* :class:`dict`.
+
+        .. note:
+            If the value is smaller than provided *min_value* we update the *config*.
+
+            This method may raise an exception if value isn't :class:`int` or cannot be casted to :class:`int`.
+
+        :param config: :class:`dict` object with new global configuration.
+        :param param: name of the configuration parameter we want to read/validate/adjust.
+        :param min_value: the minimum possible value that a given *param* could have.
+
+        :returns: an integer value which corresponds to a provided *param*.
+        """
+        value = int(config.get(param, self.__DEFAULT_CONFIG[param]))
+        if value < min_value:
+            logger.warning("%s=%d can't be smaller than %d, adjusting...", param, value, min_value)
+            value = config[param] = min_value
+        return value
+
+    def _validate_and_adjust_timeouts(self, config: Dict[str, Any]) -> None:
+        """Validate and adjust ``loop_wait``, ``retry_timeout``, and ``ttl`` values if necessary.
+
+        Minimum values:
+
+            * ``loop_wait``: 1 second;
+            * ``retry_timeout``: 3 seconds.
+            * ``ttl``: 20 seconds;
+
+        Maximum values:
+        In case if values don't fulfill the following rule, ``retry_timeout`` and ``loop_wait``
+        are reduced so that the rule is fulfilled:
+
+            .. code-block:: python
+
+                loop_wait + 2 * retry_timeout <= ttl
+
+        .. note:
+            We prefer to reduce ``loop_wait`` and will reduce ``retry_timeout`` only if ``loop_wait``
+            is already set to a minimal possible value.
+
+        :param config: :class:`dict` object with new global configuration.
+        """
+
+        min_loop_wait = 1
+        loop_wait = self. __get_and_maybe_adjust_int_value(config, 'loop_wait', min_loop_wait)
+        retry_timeout = self. __get_and_maybe_adjust_int_value(config, 'retry_timeout', 3)
+        ttl = self. __get_and_maybe_adjust_int_value(config, 'ttl', 20)
+
+        if min_loop_wait + 2 * retry_timeout > ttl:
+            config['loop_wait'] = min_loop_wait
+            config['retry_timeout'] = (ttl - min_loop_wait) // 2
+            logger.warning('Violated the rule "loop_wait + 2*retry_timeout <= ttl", where ttl=%d. '
+                           'Adjusting loop_wait from %d to %d and retry_timeout from %d to %d',
+                           ttl, loop_wait, min_loop_wait, retry_timeout, config['retry_timeout'])
+        elif loop_wait + 2 * retry_timeout > ttl:
+            config['loop_wait'] = ttl - 2 * retry_timeout
+            logger.warning('Violated the rule "loop_wait + 2*retry_timeout <= ttl", where ttl=%d and retry_timeout=%d.'
+                           ' Adjusting loop_wait from %d to %d', ttl, retry_timeout, loop_wait, config['loop_wait'])
+
     # configuration could be either ClusterConfig or dict
     def set_dynamic_configuration(self, configuration: Union[ClusterConfig, Dict[str, Any]]) -> bool:
         """Set dynamic configuration values with given *configuration*.
@@ -417,6 +477,7 @@ class Config(object):
 
         if not deep_compare(self._dynamic_configuration, configuration):
             try:
+                self._validate_and_adjust_timeouts(configuration)
                 self.__effective_configuration = self._build_effective_configuration(configuration,
                                                                                      self._local_configuration)
                 self._dynamic_configuration = configuration

tests/test_config.py

@@ -173,3 +173,27 @@ class TestConfig(unittest.TestCase):
         input_params['max_connections'] = 10
         expected_params.pop('max_connections')
         self.assertEqual(self.config._process_postgresql_parameters(input_params), expected_params)
+
+    def test__validate_and_adjust_timeouts(self):
+        with patch('patroni.config.logger.warning') as mock_logger:
+            self.config._validate_and_adjust_timeouts({'ttl': 15})
+            self.assertEqual(mock_logger.call_args_list[0][0],
+                             ("%s=%d can't be smaller than %d, adjusting...", 'ttl', 15, 20))
+        with patch('patroni.config.logger.warning') as mock_logger:
+            self.config._validate_and_adjust_timeouts({'loop_wait': 0})
+            self.assertEqual(mock_logger.call_args_list[0][0],
+                             ("%s=%d can't be smaller than %d, adjusting...", 'loop_wait', 0, 1))
+        with patch('patroni.config.logger.warning') as mock_logger:
+            self.config._validate_and_adjust_timeouts({'retry_timeout': 1})
+            self.assertEqual(mock_logger.call_args_list[0][0],
+                             ("%s=%d can't be smaller than %d, adjusting...", 'retry_timeout', 1, 3))
+        with patch('patroni.config.logger.warning') as mock_logger:
+            self.config._validate_and_adjust_timeouts({'ttl': 20, 'loop_wait': 11, 'retry_timeout': 5})
+            self.assertEqual(mock_logger.call_args_list[0][0],
+                             ('Violated the rule "loop_wait + 2*retry_timeout <= ttl", where ttl=%d '
+                              'and retry_timeout=%d. Adjusting loop_wait from %d to %d', 20, 5, 11, 10))
+        with patch('patroni.config.logger.warning') as mock_logger:
+            self.config._validate_and_adjust_timeouts({'ttl': 20, 'loop_wait': 10, 'retry_timeout': 10})
+            self.assertEqual(mock_logger.call_args_list[0][0],
+                             ('Violated the rule "loop_wait + 2*retry_timeout <= ttl", where ttl=%d. Adjusting'
+                              ' loop_wait from %d to %d and retry_timeout from %d to %d', 20, 10, 1, 10, 9))