From 15b57c5bdc08f2b7a1b27c753029a23f274d0d2a Mon Sep 17 00:00:00 2001 From: Polina Bungina <27892524+hughcapet@users.noreply.github.com> Date: Wed, 20 Dec 2023 09:54:04 +0100 Subject: [PATCH] Exclude leader from failover candidates in ctl (#2983) Exclude actual leader (not the passed leader argument) from the candidates list in the `patronictl failover` prompt. Abort `patronictl failover` execution if candidate specified is the same as the current cluster leader --- patroni/ctl.py | 36 +++++++++++++++++++----------------- tests/test_ctl.py | 8 ++++++-- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/patroni/ctl.py b/patroni/ctl.py index 8ec4e646..3981bcae 100644 --- a/patroni/ctl.py +++ b/patroni/ctl.py @@ -1190,7 +1190,7 @@ def reinit(obj: Dict[str, Any], cluster_name: str, group: Optional[int], def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: str, - group: Optional[int], leader: Optional[str], candidate: Optional[str], + group: Optional[int], switchover_leader: Optional[str], candidate: Optional[str], force: bool, scheduled: Optional[str] = None) -> None: """Perform a failover or a switchover operation in the cluster. @@ -1205,7 +1205,7 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s :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 leader: name of the current leader member. + :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. @@ -1214,10 +1214,11 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s :class:`PatroniCtlException`: if: * Patroni is running on a Citus cluster, but no *group* was specified; or * a switchover was requested by the cluster has no leader; or - * *leader* does not match the current leader of the cluster; or + * *switchover_leader* does not match the current leader of the cluster; or * cluster has no candidates available for the operation; or * no *candidate* is given for a failover operation; or - * *leader* and *candidate* are the same; or + * current leader and *candidate* are the same; or + * *candidate* is tagged as nofailover; or * *candidate* is not a member of the cluster; or * trying to schedule a switchover in a cluster that is in maintenance mode; or * user aborts the operation. @@ -1237,23 +1238,24 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s global_config = get_global_config(cluster) + cluster_leader = cluster.leader and cluster.leader.name # leader has to be be defined for switchover only if action == 'switchover': - if cluster.leader is None or not cluster.leader.name: + if not cluster_leader: raise PatroniCtlException('This cluster has no leader') - if leader is None: + if switchover_leader is None: if force: - leader = cluster.leader.name + switchover_leader = cluster_leader else: prompt = 'Standby Leader' if global_config.is_standby_cluster else 'Primary' - leader = click.prompt(prompt, type=str, default=(cluster.leader and cluster.leader.name)) + switchover_leader = click.prompt(prompt, type=str, default=cluster_leader) - if cluster.leader.name != leader: - raise PatroniCtlException(f'Member {leader} is not the leader of cluster {cluster_name}') + if cluster_leader != switchover_leader: + raise PatroniCtlException(f'Member {switchover_leader} is not the leader of cluster {cluster_name}') # excluding members with nofailover tag - candidate_names = [str(m.name) for m in cluster.members if m.name != leader and not m.nofailover] + candidate_names = [str(m.name) for m in cluster.members if m.name != cluster_leader and not m.nofailover] # We sort the names for consistent output to the client candidate_names.sort() @@ -1266,10 +1268,10 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s if action == 'failover' and not candidate: raise PatroniCtlException('Failover could be performed only to a specific candidate') - if candidate == leader: - raise PatroniCtlException(action.title() + ' target and source are the same.') - if candidate and candidate not in candidate_names: + if candidate == cluster_leader: + raise PatroniCtlException( + f'Member {candidate} is already the leader of cluster {cluster_name}') raise PatroniCtlException( f'Member {candidate} does not exist in cluster {cluster_name} or is tagged as nofailover') @@ -1298,7 +1300,7 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s failover_value = {'candidate': candidate} if action == 'switchover': - failover_value['leader'] = leader + failover_value['leader'] = switchover_leader if scheduled_at_str: failover_value['scheduled_at'] = scheduled_at_str @@ -1306,7 +1308,7 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s # By now we have established that the leader exists and the candidate exists if not force: - demote_msg = f', demoting current leader {cluster.leader.name}' if cluster.leader else '' + demote_msg = f', demoting current leader {cluster_leader}' if cluster_leader else '' if scheduled_at_str: # only switchover can be scheduled if not click.confirm(f'Are you sure you want to schedule switchover of cluster ' @@ -1340,7 +1342,7 @@ def _do_failover_or_switchover(obj: Dict[str, Any], action: str, cluster_name: s logging.exception(r) logging.warning('Failing over to DCS') click.echo('{0} Could not {1} using Patroni api, falling back to DCS'.format(timestamp(), action)) - dcs.manual_failover(leader, candidate, scheduled_at=scheduled_at) + dcs.manual_failover(switchover_leader, candidate, scheduled_at=scheduled_at) output_members(obj, cluster, cluster_name, group=group) diff --git a/tests/test_ctl.py b/tests/test_ctl.py index 0f4744a5..c936dbff 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -156,7 +156,8 @@ class TestCtl(unittest.TestCase): # Target and source are equal result = self.runner.invoke(ctl, ['switchover', 'dummy', '--group', '0'], input='leader\nleader\n\ny') self.assertEqual(result.exit_code, 1) - self.assertIn('Switchover target and source are the same', result.output) + self.assertIn("Candidate ['other']", result.output) + self.assertIn('Member leader is already the leader of cluster dummy', result.output) # Candidate is not a member of the cluster result = self.runner.invoke(ctl, ['switchover', 'dummy', '--group', '0'], input='leader\nReality\n\ny') @@ -223,7 +224,10 @@ class TestCtl(unittest.TestCase): result = self.runner.invoke(ctl, ['failover', 'dummy'], input='0\n') self.assertIn('Failover could be performed only to a specific candidate', result.output) - cluster = get_cluster_initialized_with_leader(sync=('leader', 'other')) + # Candidate is the same as the leader + result = self.runner.invoke(ctl, ['failover', 'dummy', '--group', '0'], input='leader\n') + 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: