From 728abfcc37dadced78e72a5c9f49c3e8f3600c15 Mon Sep 17 00:00:00 2001 From: Israel Date: Thu, 14 Sep 2023 10:27:23 -0300 Subject: [PATCH] Fix bug in `patronictl query` command (#2859) Previous to this commit `patronictl query` was working only if `-r` argument was provided to the command. Otherwise it would face issues: * If neither `-r` nor `-m` were provided: ``` PGPASSWORD=zalando patronictl -c postgres0.yml query -U postgres -c "SHOW PORT" 2023-09-12 17:45:38 No connection to role=None is available ``` * If only `-m` was provided: ``` $ PGPASSWORD=zalando patronictl -c postgres0.yml query -U postgres -c "SHOW PORT" -m postgresql0 2023-09-12 17:46:15 No connection to member postgresql0 is available ``` This issue was a regression introduced by `4c3e0b9382820524239d2aa4d6b95379ef1291db` through PR #2687. Through that PR we decided to move the common logic used to check mutually exclusiveness of `--role` and `--member` arguments to `get_any_member` function. However, previous to that change `role` variable would assume the default value of `any` in `query` method, before `get_any_member` was called, which was not the case after the change. This commit fixes that issue by adding a handler in `get_cursor` function to `role=None`. As `role` defaulting to `any` is handled in a sub-call to `get_any_member`, we are apparently safe in `get_cursor` to return the cursor if `role=None`. Unit tests were updated accordingly. References: PAT-204. --- patroni/ctl.py | 11 +++++++---- tests/test_ctl.py | 27 ++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/patroni/ctl.py b/patroni/ctl.py index d32440dc..5fc14336 100644 --- a/patroni/ctl.py +++ b/patroni/ctl.py @@ -561,9 +561,10 @@ def get_cursor(obj: Dict[str, Any], cluster: Cluster, group: Optional[int], conn from . import psycopg conn = psycopg.connect(**params) cursor = conn.cursor() - # If we want ``any`` node we are fine to return the cursor + # If we want ``any`` node we are fine to return the cursor. ``None`` is similar to ``any`` at this point, as it's + # been dealt with through :func:`get_any_member`. # If we want the Patroni leader node, :func:`get_any_member` already checks that for us - if role in ('any', 'leader'): + if role in (None, 'any', 'leader'): return cursor # If we want something other than ``any`` or ``leader``, then we do not rely only on the DCS information about @@ -857,9 +858,11 @@ def query_member(obj: Dict[str, Any], cluster: Cluster, group: Optional[int], if cursor is None: if member is not None: - message = 'No connection to member {0} is available'.format(member) + message = f'No connection to member {member} is available' + elif role is not None: + message = f'No connection to role {role} is available' else: - message = 'No connection to role={0} is available'.format(role) + message = 'No connection is available' logging.debug(message) return [[timestamp(0), message]], None diff --git a/tests/test_ctl.py b/tests/test_ctl.py index 7cf542b5..85691812 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -75,6 +75,21 @@ class TestCtl(unittest.TestCase): self.assertIsNotNone(get_cursor({}, get_cluster_initialized_with_leader(), None, {'dbname': 'foo'}, role='any')) + # Mutually exclusive options + with self.assertRaises(PatroniCtlException) as e: + get_cursor({}, get_cluster_initialized_with_leader(), None, {'dbname': 'foo'}, member_name='other', + role='replica') + + self.assertEqual(str(e.exception), '--role and --member are mutually exclusive options') + + # Invalid member provided + self.assertIsNone(get_cursor({}, get_cluster_initialized_with_leader(), None, {'dbname': 'foo'}, + member_name='invalid')) + + # Valid member provided + self.assertIsNotNone(get_cursor({}, get_cluster_initialized_with_leader(), None, {'dbname': 'foo'}, + member_name='other')) + def test_parse_dcs(self): assert parse_dcs(None) is None assert parse_dcs('localhost') == {'etcd': {'host': 'localhost:2379'}} @@ -278,11 +293,17 @@ class TestCtl(unittest.TestCase): rows = query_member({}, None, None, None, None, 'replica', 'SELECT pg_catalog.pg_is_in_recovery()', {}) with patch('patroni.ctl.get_cursor', Mock(return_value=None)): + # No role nor member given -- generic message rows = query_member({}, None, None, None, None, None, 'SELECT pg_catalog.pg_is_in_recovery()', {}) - self.assertTrue('No connection to' in str(rows)) + self.assertTrue('No connection is available' in str(rows)) - rows = query_member({}, None, None, None, 'foo', 'replica', 'SELECT pg_catalog.pg_is_in_recovery()', {}) - self.assertTrue('No connection to' in str(rows)) + # Member given -- message pointing to member + rows = query_member({}, None, None, None, 'foo', None, 'SELECT pg_catalog.pg_is_in_recovery()', {}) + self.assertTrue('No connection to member foo' in str(rows)) + + # Role given -- message pointing to role + rows = query_member({}, None, None, None, None, 'replica', 'SELECT pg_catalog.pg_is_in_recovery()', {}) + self.assertTrue('No connection to role replica' in str(rows)) with patch('patroni.ctl.get_cursor', Mock(side_effect=OperationalError('bla'))): rows = query_member({}, None, None, None, None, 'replica', 'SELECT pg_catalog.pg_is_in_recovery()', {})