Merge branch 'master' of github.com:patroni/patroni into di/no-master…

…-take-two

docs/patronictl.rst

@@ -320,7 +320,6 @@ Synopsis
     failover
       [ CLUSTER_NAME ]
       [ --group CITUS_GROUP ]
-      [ { --leader | --primary } LEADER_NAME ]
       --candidate CANDIDATE_NAME
       [ --force ]
 
@@ -359,16 +358,6 @@ Parameters
 
     ``CITUS_GROUP`` is the ID of the Citus group.
 
-``--leader`` / ``--primary``
-    Indicate who is the expected leader at failover time.
-
-    If given, a switchover is performed instead of a failover.
-
-    ``LEADER_NAME`` should match the name of the current leader in the cluster.
-
-    .. warning::
-        This argument is deprecated and will be removed in a future release.
-
 ``--candidate``
     The node to be promoted on failover.
 

patroni/ctl.py

@@ -1191,9 +1191,9 @@ def reinit(cluster_name: str, group: Optional[int], member_names: List[str], for
                 wait_on_members.remove(member)
 
 
-def _do_failover_or_switchover(action: str, cluster_name: str, group: Optional[int],
-                               switchover_leader: Optional[str], candidate: Optional[str],
-                               force: bool, scheduled: Optional[str] = None) -> None:
+def _do_failover_or_switchover(action: str, cluster_name: str, group: Optional[int], candidate: Optional[str],
+                               force: bool, switchover_leader: Optional[str] = None,
+                               switchover_scheduled: Optional[str] = None) -> None:
     """Perform a failover or a switchover operation in the cluster.
 
     Informational messages are printed in the console during the operation, as well as the list of members before and
@@ -1206,10 +1206,11 @@ def _do_failover_or_switchover(action: str, cluster_name: str, group: Optional[i
     :param cluster_name: name of the Patroni cluster.
     :param group: filter Citus group within we should perform a failover or switchover. If ``None``, user will be
         prompted for filling it -- unless *force* is ``True``, in which case an exception is raised.
-    :param switchover_leader: name of the leader member passed as switchover option.
     :param candidate: name of a standby member to be promoted. Nodes that are tagged with ``nofailover`` cannot be used.
     :param force: perform the failover or switchover without asking for confirmations.
-    :param scheduled: timestamp when the switchover should be scheduled to occur. If ``now`` perform immediately.
+    :param switchover_leader: name of the leader passed to the switchover command if any.
+    :param switchover_scheduled: timestamp when the switchover should be scheduled to occur. If ``now``,
+        perform immediately.
 
     :raises:
         :class:`PatroniCtlException`: if:
@@ -1288,12 +1289,12 @@ def _do_failover_or_switchover(action: str, cluster_name: str, group: Optional[i
     scheduled_at = None
 
     if action == 'switchover':
-        if scheduled is None and not force:
+        if switchover_scheduled is None and not force:
             next_hour = (datetime.datetime.now() + datetime.timedelta(hours=1)).strftime('%Y-%m-%dT%H:%M')
-            scheduled = click.prompt('When should the switchover take place (e.g. ' + next_hour + ' ) ',
-                                     type=str, default='now')
+            switchover_scheduled = click.prompt('When should the switchover take place (e.g. ' + next_hour + ' ) ',
+                                                type=str, default='now')
 
-        scheduled_at = parse_scheduled(scheduled)
+        scheduled_at = parse_scheduled(switchover_scheduled)
         if scheduled_at:
             if config.is_paused:
                 raise PatroniCtlException("Can't schedule switchover in the paused state")
@@ -1351,38 +1352,23 @@ def _do_failover_or_switchover(action: str, cluster_name: str, group: Optional[i
 @ctl.command('failover', help='Failover to a replica')
 @arg_cluster_name
 @option_citus_group
-@click.option('--leader', '--primary', 'leader', help='The name of the current leader', default=None)
 @click.option('--candidate', help='The name of the candidate', default=None)
 @option_force
-def failover(cluster_name: str, group: Optional[int],
-             leader: Optional[str], candidate: Optional[str], force: bool) -> None:
+def failover(cluster_name: str, group: Optional[int], candidate: Optional[str], force: bool) -> None:
     """Process ``failover`` command of ``patronictl`` utility.
 
     Perform a failover operation immediately in the cluster.
-
-    .. note::
-        If *leader* is given perform a switchover instead of a failover.
-        This behavior is deprecated. ``--leader`` option support will be
-        removed in the next major release.
-
     .. seealso::
         Refer to :func:`_do_failover_or_switchover` for details.
 
     :param cluster_name: name of the Patroni cluster.
     :param group: filter Citus group within we should perform a failover or switchover. If ``None``, user will be
         prompted for filling it -- unless *force* is ``True``, in which case an exception is raised by
         :func:`_do_failover_or_switchover`.
-    :param leader: name of the current leader member.
     :param candidate: name of a standby member to be promoted. Nodes that are tagged with ``nofailover`` cannot be used.
     :param force: perform the failover or switchover without asking for confirmations.
     """
-    action = 'failover'
-    if leader:
-        action = 'switchover'
-        click.echo(click.style(
-            'Supplying a leader name using this command is deprecated and will be removed in a future version of'
-            ' Patroni, change your scripts to use `switchover` instead.\nExecuting switchover!', fg='red'))
-    _do_failover_or_switchover(action, cluster_name, group, leader, candidate, force)
+    _do_failover_or_switchover('failover', cluster_name, group, candidate, force)
 
 
 @ctl.command('switchover', help='Switchover to a replica')
@@ -1411,7 +1397,7 @@ def switchover(cluster_name: str, group: Optional[int], leader: Optional[str],
     :param force: perform the switchover without asking for confirmations.
     :param scheduled: timestamp when the switchover should be scheduled to occur. If ``now`` perform immediately.
     """
-    _do_failover_or_switchover('switchover', cluster_name, group, leader, candidate, force, scheduled)
+    _do_failover_or_switchover('switchover', cluster_name, group, candidate, force, leader, scheduled)
 
 
 def generate_topology(level: int, member: Dict[str, Any],

patroni/dcs/__init__.py

@@ -260,6 +260,19 @@ def conn_kwargs(self, auth: Union[Any, Dict[str, Any], None] = None) -> Dict[str
                 ret['user'] = ret.pop('username')
         return ret
 
+    def get_endpoint_url(self, endpoint: Optional[str] = None) -> str:
+        """Get URL from member :attr:`~Member.api_url` and endpoint.
+
+        :param endpoint: URL path of REST API.
+
+        :returns: full URL for this REST API.
+        """
+        url = self.api_url or ''
+        if endpoint:
+            scheme, netloc, _, _, _, _ = urlparse(url)
+            url = urlunparse((scheme, netloc, endpoint, '', '', ''))
+        return url
+
     @property
     def api_url(self) -> Optional[str]:
         """The ``api_url`` value from :attr:`~Member.data` if defined."""

patroni/ha.py

@@ -1174,15 +1174,17 @@ def call_failsafe_member(self, data: Dict[str, Any], member: Member) -> _Failsaf
 
         :returns: a :class:`_FailsafeResponse` object.
         """
+        endpoint = 'failsafe'
+        url = member.get_endpoint_url(endpoint)
         try:
-            response = self.patroni.request(member, 'post', 'failsafe', data, timeout=2, retries=1)
+            response = self.patroni.request(member, 'post', endpoint, data, timeout=2, retries=1)
             response_data = response.data.decode('utf-8')
-            logger.info('Got response from %s %s: %s', member.name, member.api_url, response_data)
+            logger.info('Got response from %s %s: %s', member.name, url, response_data)
             accepted = response.status == 200 and response_data == 'Accepted'
             # member may return its current received/replayed LSN in the "lsn" header.
             return _FailsafeResponse(member.name, accepted, parse_int(response.headers.get('lsn')))
         except Exception as e:
-            logger.warning("Request failed to %s: POST %s (%s)", member.name, member.api_url, e)
+            logger.warning("Request failed to %s: POST %s (%s)", member.name, url, e)
         return _FailsafeResponse(member.name, False, None)
 
     def check_failsafe_topology(self) -> bool:

patroni/postgresql/postmaster.py

@@ -70,7 +70,7 @@ def _is_postmaster_process(self) -> bool:
             start_time = int(self._postmaster_pid.get('start_time', 0))
             if start_time and abs(self.create_time() - start_time) > 3:
                 logger.info('Process %s is not postmaster, too much difference between PID file start time %s and '
-                            'process start time %s', self.pid, self.create_time(), start_time)
+                            'process start time %s', self.pid, start_time, self.create_time())
                 return False
         except ValueError:
             logger.warning('Garbage start time value in pid file: %r', self._postmaster_pid.get('start_time'))

patroni/request.py

@@ -2,7 +2,6 @@
 import json
 
 from typing import Any, Dict, Optional, Union
-from urllib.parse import urlparse, urlunparse
 
 import urllib3
 
@@ -164,10 +163,7 @@ def __call__(self, member: Member, method: str = 'GET', endpoint: Optional[str]
 
         :returns: the response returned upon request.
         """
-        url = member.api_url or ''
-        if endpoint:
-            scheme, netloc, _, _, _, _ = urlparse(url)
-            url = urlunparse((scheme, netloc, endpoint, '', '', ''))
+        url = member.get_endpoint_url(endpoint)
         return self.request(method, url, data, **kwargs)
 
 

tests/test_ctl.py

@@ -231,12 +231,6 @@ def test_failover(self):
         self.assertIn("Candidate ['other']", result.output)
         self.assertIn('Member leader is already the leader of cluster dummy', result.output)
 
-        # Temp test to check a fallback to switchover if leader is specified
-        with patch('patroni.ctl._do_failover_or_switchover') as failover_func_mock:
-            result = self.runner.invoke(ctl, ['failover', '--leader', 'leader', 'dummy'], input='0\n')
-            self.assertIn('Supplying a leader name using this command is deprecated', result.output)
-            failover_func_mock.assert_called_once_with('switchover', 'dummy', None, 'leader', None, False)
-
         cluster = get_cluster_initialized_with_leader(sync=('leader', 'other'))
         cluster.members.append(Member(0, 'async', 28, {'api_url': 'http://127.0.0.1:8012/patroni'}))
         cluster.config.data['synchronous_mode'] = True

tests/test_ha.py

@@ -577,6 +577,23 @@ def test_update_failsafe(self):
         self.p.set_role('primary')
         self.assertEqual(self.ha.update_failsafe({}), 'Running as a leader')
 
+    def test_call_failsafe_member(self):
+        member = Member(0, 'test', 1, {'api_url': 'http://localhost:8011/patroni'})
+        self.ha.patroni.request = Mock()
+        self.ha.patroni.request.return_value.data = b'Accepted'
+        self.ha.patroni.request.return_value.status = 200
+        with patch('patroni.ha.logger.info') as mock_logger:
+            ret = self.ha.call_failsafe_member({}, member)
+            self.assertEqual(mock_logger.call_args_list[0][0], ('Got response from %s %s: %s', 'test', 'http://localhost:8011/failsafe', 'Accepted'))
+            self.assertTrue(ret.accepted)
+
+        e = Exception('request failed')
+        self.ha.patroni.request.side_effect = e
+        with patch('patroni.ha.logger.warning') as mock_logger:
+            ret = self.ha.call_failsafe_member({}, member)
+            self.assertEqual(mock_logger.call_args_list[0][0], ('Request failed to %s: POST %s (%s)', 'test', 'http://localhost:8011/failsafe', e))
+            self.assertFalse(ret.accepted)
+
     @patch('time.sleep', Mock())
     def test_bootstrap_from_another_member(self):
         self.ha.cluster = get_cluster_initialized_with_leader()