Implement sync_priority tag (patroni#3223)

Author: hughcapet

docs/yaml_configuration.rst

@@ -401,8 +401,9 @@ Tags
 -  **noloadbalance**: ``true`` or ``false``. If set to ``true`` the node will return HTTP Status Code 503 for the ``GET /replica`` REST API health-check and therefore will be excluded from the load-balancing. Defaults to ``false``.
 -  **replicatefrom**: The name of another replica to replicate from. Used to support cascading replication.
 -  **nosync**: ``true`` or ``false``. If set to ``true`` the node will never be selected as a synchronous replica.
+-  **sync_priority**: integer, controls the priority this node should have during synchronous replica selection when ``synchronous_mode`` is set to ``on``. Nodes with higher priority will be preferred over lower-priority nodes. If the ``sync_priority`` is 0 or negative - such node is not allowed to be written to ``synchronous_standby_names`` PostgreSQL parameter (similar to ``nosync: true``). Keep in mind, that this parameter has the opposite meaning to ``sync_priority`` value reported in ``pg_stat_replication`` view.
 -  **nofailover**: ``true`` or ``false``, controls whether this node is allowed to participate in the leader race and become a leader. Defaults to ``false``, meaning this node _can_ participate in leader races. 
--  **failover_priority**: integer, controls the priority that this node should have during failover. Nodes with higher priority will be preferred over lower priority nodes if they received/replayed the same amount of WAL. However, nodes with higher values of receive/replay LSN are preferred regardless of their priority. If the ``failover_priority`` is 0 or negative - such node is not allowed to participate in the leader race and to become a leader (similar to ``nofailover: true``).
+-  **failover_priority**: integer, controls the priority this node should have during failover. Nodes with higher priority will be preferred over lower-priority nodes if they received/replayed the same amount of WAL. However, nodes with higher values of receive/replay LSN are preferred regardless of their priority. If the ``failover_priority`` is 0 or negative - such node is not allowed to participate in the leader race and to become a leader (similar to ``nofailover: true``).
 -  **nostream**: ``true`` or ``false``. If set to ``true`` the node will not use replication protocol to stream WAL. It will rely instead on archive recovery (if ``restore_command`` is configured) and ``pg_wal``/``pg_xlog`` polling. It also disables copying and synchronization of permanent logical replication slots on the node itself and all its cascading replicas. Setting this tag on primary node has no effect.
 
 .. warning::

features/priority_sync.feature

@@ -0,0 +1,36 @@
+Feature: synchronous replicas priority
+    We should check that we can give nodes priority for becoming synchronous replicas
+
+  Scenario: check replica with sync_priority=0 does not become a synchronous replica
+     Given I start postgres-0
+     When I issue a PATCH request to http://127.0.0.1:8008/config with {"ttl": 20, "synchronous_mode": true}
+     Then I receive a response code 200
+     When I configure and start postgres-1 with a tag sync_priority 0
+     Then sync key in DCS has leader=postgres-0 after 20 seconds
+     And sync key in DCS has sync_standby=None after 5 seconds
+
+  Scenario: check higher synchronous replicas priority is respected
+    Given I configure and start postgres-2 with a tag sync_priority 1
+    And I configure and start postgres-3 with a tag sync_priority 2
+    Then replication works from postgres-0 to postgres-2 after 20 seconds
+    And replication works from postgres-0 to postgres-3 after 20 seconds
+    When I issue a PATCH request to http://127.0.0.1:8008/config with {"synchronous_node_count": 1}
+    Then I receive a response code 200
+    And sync key in DCS has sync_standby=postgres-3 after 10 seconds
+
+
+ Scenario: check conflicting configuration handling
+    When I set nosync tag in postgres-3 config
+    And I issue an empty POST request to http://127.0.0.1:8011/reload
+    Then I receive a response code 202
+    And there is one of ["Conflicting configuration between nosync: True and sync_priority: 2. Defaulting to nosync: True"] WARNING in the postgres-3 patroni log after 5 seconds
+    And "members/postgres-3" key in DCS has tags={'nosync': True, 'sync_priority': '2'} after 10 seconds
+    And "sync" key in DCS has sync_standby=postgres-2 after 10 seconds
+    When I reset nosync tag in postgres-1 config
+    And I issue an empty POST request to http://127.0.0.1:8009/reload
+    Then I receive a response code 202
+    And there is one of ["Conflicting configuration between nosync: False and sync_priority: 0. Defaulting to nosync: False"] WARNING in the postgres-1 patroni log after 5 seconds
+    And "members/postgres-1" key in DCS has tags={'nosync': False, 'sync_priority': '0'} after 10 seconds
+    When I shut down postgres-2
+    And "sync" key in DCS has sync_standby=postgres-1 after 3 seconds
+

patroni/config.py

@@ -147,7 +147,7 @@ def __init__(self, configfile: str,
         self._cache_file = os.path.join(self._data_dir, self.__CACHE_FILENAME)
         if validator:  # patronictl uses validator=None
             self._load_cache()  # we don't want to load anything from local cache for ctl
-            self._validate_failover_tags()  # irrelevant for ctl
+            self._validate_contradictory_tags()  # irrelevant for ctl
         self._cache_needs_saving = False
 
     @property
@@ -359,7 +359,7 @@ def reload_local_configuration(self) -> Optional[bool]:
                     new_configuration = self._build_effective_configuration(self._dynamic_configuration, configuration)
                     self._local_configuration = configuration
                     self.__effective_configuration = new_configuration
-                    self._validate_failover_tags()
+                    self._validate_contradictory_tags()
                     return True
                 else:
                     logger.info('No local configuration items changed.')
@@ -798,25 +798,31 @@ def copy(self) -> Dict[str, Any]:
         """
         return deepcopy(self.__effective_configuration)
 
-    def _validate_failover_tags(self) -> None:
-        """Check ``nofailover``/``failover_priority`` config and warn user if it's contradictory.
+    def _validate_contradictory_tags(self) -> None:
+        """Check boolean/priority tags' config and warn user if it's contradictory.
 
         .. note::
-          To preserve sanity (and backwards compatibility) the ``nofailover`` tag will still exist. A contradictory
-          configuration is one where ``nofailover`` is ``True`` but ``failover_priority > 0``, or where
-          ``nofailover`` is ``False``, but ``failover_priority <= 0``. Essentially, ``nofailover`` and
-          ``failover_priority`` are communicating different things.
+          To preserve sanity (and backwards compatibility) the ``nofailover``/``nosync`` tag will still exist.
+          A contradictory configuration is one where ``nofailover``/``nosync`` is ``True`` but
+          ``failover_priority > 0``/``sync_priority > 0``, or where ``nofailover``/``nosync`` is ``False``,
+          but ``failover_priority <= 0``/``sync_priority <= 0``. Essentially, ``nofailover``/``nosync`` and
+          ``failover_priority``/``sync_priority`` are communicating different things.
           This checks for this edge case (which is a misconfiguration on the part of the user) and warns them.
-          The behaviour is as if ``failover_priority`` were not provided (i.e ``nofailover`` is the
-          bedrock source of truth)
+          The behaviour is as if ``failover_priority``/``sync_priority`` were not provided
+          (i.e ``nofailover``/``nosync`` is the bedrock source of truth).
         """
         tags = self.get('tags', {})
-        if 'nofailover' not in tags:
-            return
-        nofailover_tag = tags.get('nofailover')
-        failover_priority_tag = parse_int(tags.get('failover_priority'))
-        if failover_priority_tag is not None \
-                and (bool(nofailover_tag) is True and failover_priority_tag > 0
-                     or bool(nofailover_tag) is False and failover_priority_tag <= 0):
-            logger.warning('Conflicting configuration between nofailover: %s and failover_priority: %s. '
-                           'Defaulting to nofailover: %s', nofailover_tag, failover_priority_tag, nofailover_tag)
+
+        def validate_tag(bool_name: str, priority_name: str) -> None:
+            if bool_name not in tags:
+                return
+            bool_tag = tags.get(bool_name)
+            priority_tag = parse_int(tags.get(priority_name))
+            if priority_tag is not None \
+                    and (bool(bool_tag) is True and priority_tag > 0
+                         or bool(bool_tag) is False and priority_tag <= 0):
+                logger.warning('Conflicting configuration between %s: %s and %s: %s. Defaulting to %s: %s',
+                               bool_name, bool_tag, priority_name, priority_tag, bool_name, bool_tag)
+
+        validate_tag('nofailover', 'failover_priority')
+        validate_tag('nosync', 'sync_priority')

patroni/config_generator.py

@@ -126,6 +126,7 @@ def get_template_config(cls) -> Dict[str, Any]:
             },
             'tags': {
                 'failover_priority': 1,
+                'sync_priority': 1,
                 'noloadbalance': False,
                 'clonefrom': True,
                 'nosync': False,

patroni/ha.py

@@ -410,6 +410,7 @@ def get_effective_tags(self) -> Dict[str, Any]:
         # _disable_sync could be modified concurrently, but we don't care as attribute get and set are atomic.
         if self._disable_sync > 0:
             tags['nosync'] = True
+            tags['sync_priority'] = 0
         return tags
 
     def notify_mpp_coordinator(self, event: str) -> None:

patroni/postgresql/sync.py

@@ -196,6 +196,7 @@ class _Replica(NamedTuple):
     sync_state: str
     lsn: int
     nofailover: bool
+    sync_priority: int
 
 
 class _ReplicaList(List[_Replica]):
@@ -238,10 +239,12 @@ def __init__(self, postgresql: 'Postgresql', cluster: Cluster) -> None:
             #   b. PostgreSQL on the member is known to be running and accepting client connections.
             if member and row[sort_col] is not None and member.is_running and not member.nosync:
                 self.append(_Replica(row['pid'], row['application_name'],
-                                     row['sync_state'], row[sort_col], bool(member.nofailover)))
+                                     row['sync_state'], row[sort_col],
+                                     bool(member.nofailover), member.sync_priority))
 
-        # Prefer replicas that are in state ``sync`` and with higher values of ``write``/``flush``/``replay`` LSN.
-        self.sort(key=lambda r: (r.sync_state, r.lsn), reverse=True)
+        # Prefer replicas with higher ``sync_priority`` value, in state ``sync``,
+        # and higher values of ``write``/``flush``/``replay`` LSN.
+        self.sort(key=lambda r: (r.sync_priority, r.sync_state, r.lsn), reverse=True)
 
         # When checking ``maximum_lag_on_syncnode`` we want to compare with the most
         # up-to-date replica otherwise with cluster LSN if there is only one replica.

patroni/tags.py

@@ -29,14 +29,16 @@ def _filter_tags(tags: Dict[str, Any]) -> Dict[str, Any]:
             they all are boolean values that default to disabled.
             However ``nofailover`` tag is always returned if ``failover_priority`` tag is defined. In this case, we need
             both values to see if they are contradictory and the ``nofailover`` value should be used.
+            The same rule applies for ``nosync`` and ``sync_priority`` tags.
 
         :returns: a dictionary of tags set for this node. The key is the tag name, and the value is the corresponding
             tag value.
         """
         return {tag: value for tag, value in tags.items()
                 if any((tag not in ('clonefrom', 'nofailover', 'noloadbalance', 'nosync', 'nostream'),
                         value,
-                        tag == 'nofailover' and 'failover_priority' in tags))}
+                        tag == 'nofailover' and 'failover_priority' in tags,
+                        tag == 'nosync' and 'sync_priority' in tags))}
 
     @property
     @abc.abstractmethod
@@ -52,31 +54,49 @@ def clonefrom(self) -> bool:
         """``True`` if ``clonefrom`` tag is ``True``, else ``False``."""
         return self.tags.get('clonefrom', False)
 
-    @property
-    def nofailover(self) -> bool:
-        """Common logic for obtaining the value of ``nofailover`` from ``tags`` if defined.
+    def _priority_tag(self, bool_name: str, priority_name: str) -> int:
+        """Common logic for obtaining the value of a priority tag from ``tags`` if defined.
+
+        If boolean tag is defined as ``True``, this will return ``0``. Otherwise, it will return the value of
+        the respective priority tag, defaulting to ``1`` if it's not defined or invalid.
+
+        :param bool_name: name of the boolean tag (``nofailover``. ``nosync``).
+        :param priority_name: name of the priority tag (``failover_priority``, ``sync_priority``).
 
-        If ``nofailover`` is not defined, this methods returns ``True`` if ``failover_priority`` is non-positive,
+        :returns: integer value based on the defined tags.
+        """
+        from_tags = self.tags.get(bool_name)
+        priority = parse_int(self.tags.get(priority_name))
+        priority = 1 if priority is None else priority
+        return 0 if from_tags else priority
+
+    def _bool_tag(self, bool_name: str, priority_name: str) -> bool:
+        """Common logic for obtaining the value of a boolean tag from ``tags`` if defined.
+
+        If boolean tag is not defined, this methods returns ``True`` if priority tag is non-positive,
         ``False`` otherwise.
+
+        :param bool_name: name of the boolean tag (``nofailover``. ``nosync``).
+        :param priority_name: name of the priority tag (``failover_priority``, ``sync_priority``).
+
+        :returns: boolean value based on the defined tags.
         """
-        from_tags = self.tags.get('nofailover')
+        from_tags = self.tags.get(bool_name)
         if from_tags is not None:
-            # Value of `nofailover` takes precedence over `failover_priority`
+            # Value of bool tag takes precedence over priority tag
             return bool(from_tags)
-        failover_priority = parse_int(self.tags.get('failover_priority'))
-        return failover_priority is not None and failover_priority <= 0
+        priority = parse_int(self.tags.get(priority_name))
+        return priority is not None and priority <= 0
 
     @property
-    def failover_priority(self) -> int:
-        """Common logic for obtaining the value of ``failover_priority`` from ``tags`` if defined.
+    def nofailover(self) -> bool:
+        """``True`` if node configuration doesn't allow it to become primary, ``False`` otherwise."""
+        return self._bool_tag('nofailover', 'failover_priority')
 
-        If ``nofailover`` is defined as ``True``, this will return ``0``. Otherwise, it will return the value of
-        ``failover_priority``, defaulting to ``1`` if it's not defined or invalid.
-        """
-        from_tags = self.tags.get('nofailover')
-        failover_priority = parse_int(self.tags.get('failover_priority'))
-        failover_priority = 1 if failover_priority is None else failover_priority
-        return 0 if from_tags else failover_priority
+    @property
+    def failover_priority(self) -> int:
+        """Value of ``failover_priority`` from ``tags`` if defined, otherwise derived from ``nofailover``."""
+        return self._priority_tag('nofailover', 'failover_priority')
 
     @property
     def noloadbalance(self) -> bool:
@@ -85,8 +105,13 @@ def noloadbalance(self) -> bool:
 
     @property
     def nosync(self) -> bool:
-        """``True`` if ``nosync`` is ``True``, else ``False``."""
-        return bool(self.tags.get('nosync', False))
+        """``True`` if node configuration doesn't allow it to become synchronous, ``False`` otherwise."""
+        return self._bool_tag('nosync', 'sync_priority')
+
+    @property
+    def sync_priority(self) -> int:
+        """Value of ``sync_priority`` from ``tags`` if defined, otherwise derived from ``nosync``."""
+        return self._priority_tag('nosync', 'sync_priority')
 
     @property
     def replicatefrom(self) -> Optional[str]:

patroni/validator.py

@@ -1182,7 +1182,10 @@ def validate_watchdog_mode(value: Any) -> None:
         Optional("clonefrom"): bool,
         Optional("noloadbalance"): bool,
         Optional("replicatefrom"): str,
-        Optional("nosync"): bool,
+        AtMostOne("nosync", "sync_priority"): Case({
+            "nosync": bool,
+            "sync_priority": IntValidator(min=0, expected_type=int, raise_assert=True),
+        }),
         Optional("nostream"): bool
     }
 })

postgres0.yml

@@ -133,7 +133,7 @@ postgresql:
 
 tags:
     # failover_priority: 1
+    # sync_priority: 1
     noloadbalance: false
     clonefrom: false
-    nosync: false
     nostream: false

postgres1.yml

@@ -125,5 +125,6 @@ postgresql:
 
 tags:
     # failover_priority: 1
+    # sync_priority: 1
     noloadbalance: false
     clonefrom: false