From 793325cb609010cfc195986f9cf1ea7f2c9fbc09 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 23 Sep 2015 18:38:17 +0200 Subject: [PATCH 01/59] add support for pg_rewind. --- patroni/postgresql.py | 41 +++++++++++++++++++++++++++++++++++++---- postgres0.yml | 5 +++++ postgres1.yml | 5 +++++ 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 532e65c8..b69099a9 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -47,6 +47,7 @@ class Postgresql: self.replication = config['replication'] self.superuser = config['superuser'] self.admin = config['admin'] + self.pg_rewind = config.get('pg_rewind', {}) self.callback = config.get('callbacks', {}) self.use_slots = config.get('use_slots', True) self.schedule_load_slots = self.use_slots @@ -69,6 +70,17 @@ class Postgresql: self._cursor_holder = None self.members = [] # list of already existing replication slots self.retry = Retry(max_tries=-1, deadline=10, max_delay=1, retry_exceptions=PostgresConnectionException) + try: + self._pg_rewind_present = ('username' in self.pg_rewind and + ('wal_log_hints' in self.config['parameters'] or + 'data_checksums' in self.config['parameters']) and + os.system("pg_rewind --version >/dev/null 2>&1") == 0) + if self._pg_rewind_present: + self.pg_rewind['user'] = self.pg_rewind['username'] + except: + self._pg_rewind_present = False + if self.pg_rewind and not self._pg_rewind_present: + logger.warning("pg_rewind support is disabled") def get_local_address(self): listen_addresses = self.listen_addresses.split(',') @@ -265,8 +277,10 @@ class Postgresql: f.write(line + '\n') @staticmethod - def primary_conninfo(leader_url): + def primary_conninfo(leader_url, replacement=None): r = parseurl(leader_url) + if replacement is not None: + r.update(replacement) return 'user={user} password={password} host={host} port={port} sslmode=prefer sslcompression=1'.format(**r) def check_recovery_conf(self, leader): @@ -299,9 +313,28 @@ recovery_target_timeline = 'latest' def follow_the_leader(self, leader): if not self.check_recovery_conf(leader): self.write_recovery_conf(leader) - run_callback = self.role == 'master' - self.restart() - run_callback and self.call_nowait(ACTION_ON_ROLE_CHANGE) + change_role = self.role == 'master' + + if leader and change_role and self._pg_rewind_present: + self.stop() + pc = self.primary_conninfo(leader.conn_url, + self.pg_rewind) + ' dbname=postgres' + logger.info("running pg_rewind from {}".format(pc)) + pg_rewind = ['pg_rewind', '-D', self.data_dir, '--source-server', pc] + try: + ret = (subprocess.call(pg_rewind) == 0) + except: + ret = False + # pg_rewind removes recovery.conf, we have to reinstate it. + if ret: + self.write_recovery_conf(leader) + self.start() + else: + self.remove_data_directory() + logger.error("unable to rewind the former leader") + else: + ret = self.restart() + change_role and ret and self.call_nowait(ACTION_ON_ROLE_CHANGE) def save_configuration_files(self): """ diff --git a/postgres0.yml b/postgres0.yml index f800183a..a155b1cd 100644 --- a/postgres0.yml +++ b/postgres0.yml @@ -34,6 +34,9 @@ postgresql: data_dir: data/postgresql0 maximum_lag_on_failover: 1048576 # 1 megabyte in bytes use_slots: True + pg_rewind: + username: postgres + password: zalando pg_hba: - host all all 0.0.0.0/0 md5 - hostssl all all 0.0.0.0/0 md5 @@ -42,6 +45,7 @@ postgresql: password: rep-pass network: 127.0.0.1/32 superuser: + username: postgres password: zalando admin: username: admin @@ -62,3 +66,4 @@ postgresql: archive_timeout: 1800s max_replication_slots: 5 hot_standby: "on" + wal_log_hints: "on" diff --git a/postgres1.yml b/postgres1.yml index e1c3e663..94e33a42 100644 --- a/postgres1.yml +++ b/postgres1.yml @@ -34,6 +34,9 @@ postgresql: data_dir: data/postgresql1 maximum_lag_on_failover: 1048576 # 1 megabyte in bytes use_slots: True + pg_rewind: + username: postgres + password: zalando pg_hba: - host all all 0.0.0.0/0 md5 - hostssl all all 0.0.0.0/0 md5 @@ -42,6 +45,7 @@ postgresql: password: rep-pass network: 127.0.0.1/32 superuser: + user: postgres password: zalando admin: username: admin @@ -62,3 +66,4 @@ postgresql: archive_timeout: 1800s max_replication_slots: 5 hot_standby: "on" + wal_log_hints: "on" From c8108f221e1f78162715cd4f70776e79b0365b38 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Thu, 24 Sep 2015 11:34:28 +0200 Subject: [PATCH 02/59] Check the exit code of the postgres start when determining whether to run the on_role_change callback. --- patroni/postgresql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index b69099a9..51c3db13 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -328,7 +328,7 @@ recovery_target_timeline = 'latest' # pg_rewind removes recovery.conf, we have to reinstate it. if ret: self.write_recovery_conf(leader) - self.start() + ret = self.start() else: self.remove_data_directory() logger.error("unable to rewind the former leader") From 027bcd39cede37524f788d0688bd3966c8e2259c Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Thu, 24 Sep 2015 12:46:36 +0200 Subject: [PATCH 03/59] Move pg_rewind call into a separate sub. Add a Postgresql method to call pg_rewind. Improve the test coverage. --- patroni/postgresql.py | 38 ++++++++++++++++++++++---------------- tests/test_postgresql.py | 23 ++++++++++++++++++++++- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 51c3db13..5d0e27da 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -47,7 +47,7 @@ class Postgresql: self.replication = config['replication'] self.superuser = config['superuser'] self.admin = config['admin'] - self.pg_rewind = config.get('pg_rewind', {}) + self._pg_rewind = config.get('pg_rewind', {}) self.callback = config.get('callbacks', {}) self.use_slots = config.get('use_slots', True) self.schedule_load_slots = self.use_slots @@ -70,16 +70,19 @@ class Postgresql: self._cursor_holder = None self.members = [] # list of already existing replication slots self.retry = Retry(max_tries=-1, deadline=10, max_delay=1, retry_exceptions=PostgresConnectionException) + self.init_pg_rewind() + + def init_pg_rewind(self): try: - self._pg_rewind_present = ('username' in self.pg_rewind and + self._pg_rewind_present = ('username' in self._pg_rewind and ('wal_log_hints' in self.config['parameters'] or 'data_checksums' in self.config['parameters']) and os.system("pg_rewind --version >/dev/null 2>&1") == 0) if self._pg_rewind_present: - self.pg_rewind['user'] = self.pg_rewind['username'] + self._pg_rewind['user'] = self._pg_rewind['username'] except: self._pg_rewind_present = False - if self.pg_rewind and not self._pg_rewind_present: + if self._pg_rewind and not self._pg_rewind_present: logger.warning("pg_rewind support is disabled") def get_local_address(self): @@ -310,6 +313,18 @@ recovery_target_timeline = 'latest' for name, value in self.config.get('recovery_conf', {}).items(): f.write("{} = '{}'\n".format(name, value)) + def pg_rewind(self, leader): + pc = self.primary_conninfo(leader.conn_url, self._pg_rewind) + ' dbname=postgres' + logger.info("running pg_rewind from {}".format(pc)) + pg_rewind = ['pg_rewind', '-D', self.data_dir, '--source-server', pc] + try: + ret = (subprocess.call(pg_rewind) == 0) + except: + ret = False + if ret: + self.write_recovery_conf(leader) + return ret + def follow_the_leader(self, leader): if not self.check_recovery_conf(leader): self.write_recovery_conf(leader) @@ -317,20 +332,11 @@ recovery_target_timeline = 'latest' if leader and change_role and self._pg_rewind_present: self.stop() - pc = self.primary_conninfo(leader.conn_url, - self.pg_rewind) + ' dbname=postgres' - logger.info("running pg_rewind from {}".format(pc)) - pg_rewind = ['pg_rewind', '-D', self.data_dir, '--source-server', pc] - try: - ret = (subprocess.call(pg_rewind) == 0) - except: - ret = False - # pg_rewind removes recovery.conf, we have to reinstate it. - if ret: - self.write_recovery_conf(leader) + if self.pg_rewind(leader): ret = self.start() else: - self.remove_data_directory() + ret = False + self.move_data_directory() logger.error("unable to rewind the former leader") else: ret = self.restart() diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index eaf84943..35019780 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -9,6 +9,7 @@ from patroni.exceptions import PostgresConnectionException from patroni.postgresql import Postgresql from patroni.utils import RetryFailedError from test_ha import false +import subprocess class MockCursor: @@ -132,12 +133,32 @@ class TestPostgresql(unittest.TestCase): def test_sync_from_leader(self): self.assertTrue(self.p.sync_from_leader(self.leader)) - def test_follow_the_leader(self): + @patch('os.system', side_effect=Exception("Test")) + def test_init_pg_rewind(self, mock_system): + self.p.init_pg_rewind() + # prepare parameters for pg_rewind + self.p._pg_rewind = {'username': 'foo'} + self.p.config['parameters']['data_checksums'] = 1 + os.system = mock_system + self.p.init_pg_rewind() + + @patch('subprocess.call', side_effect=Exception("Test")) + def test_pg_rewind(self, mock_call): + self.assertTrue(self.p.pg_rewind(self.leader)) + self.p + subprocess.call = mock_call + self.assertFalse(self.p.pg_rewind(self.leader)) + + @patch('patroni.postgresql.Postgresql.pg_rewind', return_value=False) + def test_follow_the_leader(self, mock_pg_rewind): self.p.demote(self.leader) self.p.follow_the_leader(None) + self.p._pg_rewind_present = True self.p.demote(self.leader) self.p.follow_the_leader(self.leader) self.p.follow_the_leader(Leader(-1, None, 28, self.other)) + self.p.pg_rewind = mock_pg_rewind + self.p.follow_the_leader(self.leader) def test_create_replica(self): self.p.delete_trigger_file = Mock(side_effect=OSError()) From 6e9cb60fd532dbd0f45d985d0f5841403de44931 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 24 Sep 2015 14:52:03 +0200 Subject: [PATCH 04/59] Restart and reinitialize via api POST /restart -- will restart postgres You you are restartung leader node, lock would be maintained during restart. POST /reinitialize -- will reinitialize node from the leader. It's not possible to reinitialize current leader. Command will fail when the leader is unknown. --- patroni/__init__.py | 4 +- patroni/api.py | 69 +++++++++++++++++++----- patroni/dcs.py | 11 ++-- patroni/etcd.py | 10 ++-- patroni/ha.py | 112 ++++++++++++++++++++++++++++++++++----- patroni/postgresql.py | 81 +++++++++++++++++++++------- patroni/zookeeper.py | 6 ++- tests/test_api.py | 59 ++++++++++++++++++--- tests/test_etcd.py | 5 +- tests/test_ha.py | 49 ++++++++++++++++- tests/test_postgresql.py | 32 ++++++++--- tests/test_zookeeper.py | 10 ++-- 12 files changed, 366 insertions(+), 82 deletions(-) diff --git a/patroni/__init__.py b/patroni/__init__.py index 10e70956..35f206df 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -48,8 +48,6 @@ class Patroni: logger.info('waiting on DCS') sleep(5) - self.postgresql.schedule_load_slots = self.postgresql.is_running() and self.postgresql.use_slots - def schedule_next_run(self): self.next_run += self.nap_time current_time = time.time() @@ -75,7 +73,7 @@ class Patroni: def main(): - logging.basicConfig(format='%(asctime)s %(levelname)s: %(message)s', level=logging.INFO) + logging.basicConfig(format='%(asctime)s %(levelname)s: %(message)s', level=logging.DEBUG) logging.getLogger('requests').setLevel(logging.WARNING) setup_signal_handlers() diff --git a/patroni/api.py b/patroni/api.py index 30bc8914..ebb732a4 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -44,23 +44,23 @@ class RestApiHandler(BaseHTTPRequestHandler): def do_GET(self): """Default method for processing all GET requests which can not be routed to other methods""" + path = '/master' if self.path == '/' else self.path response = self.get_postgresql_status() - path = '/master' if self.path == '/' else self.path - status_code = 200 if response['running'] and 'role' in response and response['role'] in path else 503 + patroni = self.server.patroni + if 'role' in response and response['role'] in path: + status_code = 200 + elif patroni.ha.restart_scheduled() and patroni.postgresql.role == 'master' and 'master' in path: + # exceptional case for master node when the postgres is being restarted via API + status_code = 200 + else: + status_code = 503 self.send_response(status_code) self.send_header('Content-Type', 'application/json') self.end_headers() self.wfile.write(json.dumps(response).encode('utf-8')) - @check_auth - def do_GET_sampleauth(self): - self.send_response(200) - self.send_header('Content-Type', 'text/html') - self.end_headers() - self.wfile.write(b'Hello!') - def do_GET_patroni(self): response = self.get_postgresql_status(True) @@ -69,6 +69,51 @@ class RestApiHandler(BaseHTTPRequestHandler): self.end_headers() self.wfile.write(json.dumps(response).encode('utf-8')) + @check_auth + def do_POST_restart(self): + action = self.server.patroni.ha.schedule_restart() + if action is not None: + status_code = 503 + data = (action + ' already in progress').encode('utf-8') + else: + status_code = 503 + data = b'restart failed' + try: + if self.server.patroni.ha.restart(): + status_code = 200 + data = b'restarted successfully' + except: + logger.exception('Exception during restart') + + self.send_response(status_code) + self.send_header('Content-Type', 'text/html') + self.end_headers() + self.wfile.write(data) + + @check_auth + def do_POST_reinitialize(self): + ha = self.server.patroni.ha + cluster = ha.dcs.get_cluster() + if cluster.is_unlocked(): + status_code = 503 + data = b'Cluster has no leader, can not reinitialize' + elif cluster.leader.name == ha.state_handler.name: + status_code = 503 + data = b'I am the leader, can not reinitialize' + else: + action = ha.schedule_reinitialize() + if action is not None: + status_code = 503 + data = (action + ' already in progress').encode('utf-8') + else: + status_code = 200 + data = b'reinitialize scheduled' + + self.send_response(status_code) + self.send_header('Content-Type', 'text/html') + self.end_headers() + self.wfile.write(data) + def parse_request(self): """Override parse_request method to enrich basic functionality of `BaseHTTPRequestHandler` class @@ -104,9 +149,9 @@ class RestApiHandler(BaseHTTPRequestHandler): pg_last_xlog_replay_location(), pg_is_in_recovery() AND pg_is_xlog_replay_paused()""", retry=retry)[0] return { - 'running': True, + 'state': self.server.patroni.postgresql.state, 'postmaster_start_time': row[0], - 'role': 'slave' if row[1] else 'master', + 'role': 'replica' if row[1] else 'master', 'xlog': ({ 'received_location': row[3], 'replayed_location': row[4], @@ -116,7 +161,7 @@ class RestApiHandler(BaseHTTPRequestHandler): } except (psycopg2.Error, RetryFailedError, PostgresConnectionException): logger.exception('get_postgresql_status') - return {'running': self.server.patroni.postgresql.is_running()} + return {'state': self.server.patroni.postgresql.state} class RestApiServer(ThreadingMixIn, HTTPServer, Thread): diff --git a/patroni/dcs.py b/patroni/dcs.py index 908386b5..ebf625d6 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -120,14 +120,17 @@ class AbstractDCS: running as a master and exception raised instance would be demoted.""" @abc.abstractmethod - def update_leader(self, state_handler): - """Update leader key (or session) ttl and `/optime/leader` key in DCS. + def write_leader_optime(self, last_operation): + """write current xlog location into `/optime/leader` key in DCS + :param last_operation: absolute xlog location in bytes""" + + @abc.abstractmethod + def update_leader(self): + """Update leader key (or session) ttl - :param state_handler: reference to `Postgresql` object :returns: `!True` if leader key (or session) has been updated successfully. If not, `!False` must be returned and current instance would be demoted. - If you failed to update `/optime/leader` this error is not critical and you can return `!True` You have to use CAS (Compare And Swap) operation in order to update leader key, for example for etcd `prevValue` parameter must be used.""" diff --git a/patroni/etcd.py b/patroni/etcd.py index fc6d5a97..b189c188 100644 --- a/patroni/etcd.py +++ b/patroni/etcd.py @@ -222,14 +222,12 @@ class Etcd(AbstractDCS): return False @catch_etcd_errors - def write_leader_optime(self, state_handler): - return self.client.set(self.leader_optime_path, state_handler.last_operation()) + def write_leader_optime(self, last_operation): + return self.client.set(self.leader_optime_path, last_operation) @catch_etcd_errors - def update_leader(self, state_handler): - ret = self.retry(self.client.test_and_set, self.leader_path, self._name, self._name, self.ttl) - ret and self.write_leader_optime(state_handler) - return ret + def update_leader(self): + return self.retry(self.client.test_and_set, self.leader_path, self._name, self._name, self.ttl) @catch_etcd_errors def initialize(self): diff --git a/patroni/ha.py b/patroni/ha.py index 0d95d372..64179a78 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -2,6 +2,7 @@ import logging import psycopg2 from patroni.exceptions import DCSError, PostgresConnectionException +from threading import Lock logger = logging.getLogger(__name__) @@ -13,6 +14,10 @@ class Ha: self.dcs = etcd self.cluster = None self.old_cluster = None + self.scheduled_action = None + self.scheduled_action_lock = Lock() + self.restart_in_progress = False + self.restart_thread_lock = Lock() def load_cluster_from_dcs(self): cluster = self.dcs.get_cluster() @@ -28,7 +33,13 @@ class Ha: return self.dcs.attempt_to_acquire_leader() def update_lock(self): - return self.dcs.update_leader(self.state_handler) + ret = self.dcs.update_leader() + if ret: + try: + self.dcs.write_leader_optime(self.state_handler.last_operation()) + except: + pass + return ret def has_lock(self): lock_owner = self.cluster.leader and self.cluster.leader.name @@ -37,8 +48,9 @@ class Ha: def bootstrap(self): if not self.cluster.is_unlocked(): # cluster already has leader - logger.info('trying to bootstrap from leader', ) + logger.info('trying to bootstrap from leader') if self.state_handler.bootstrap(self.cluster.leader): + self.reinitialize_scheduled() and self.reset_scheduled_action() return 'bootstrapped from leader' else: self.state_handler.stop('immediate') @@ -63,15 +75,17 @@ class Ha: return 'waiting for leader to bootstrap' def recover(self): - if self.state_handler.is_healthy(): - return False has_lock = self.has_lock() self.state_handler.write_recovery_conf(None if has_lock else self.cluster.leader) - self.state_handler.start() - if has_lock: - logger.info('started as readonly because i had the session lock') - self.load_cluster_from_dcs() - return True + if not self.state_handler.start(): + if not has_lock: + return 'failed to start postgres' + self.dcs.delete_leader() + return 'removed leader key after trying and failing to start postgres' + if not has_lock: + return 'started as a secondary' + logger.info('started as readonly because i had the session lock') + self.load_cluster_from_dcs() def follow_the_leader(self, demote_reason, follow_reason, refresh=True): refresh and self.load_cluster_from_dcs() @@ -112,7 +126,68 @@ class Ha: return self.follow_the_leader('demoting self because i do not have the lock and i was a leader', 'no action. i am a secondary and i am following a leader', False) - def run_cycle(self): + def schedule_action(self, action): + with self.scheduled_action_lock: + if self.scheduled_action is not None: + return self.scheduled_action + self.scheduled_action = action + return None + + def get_scheduled_action(self): + with self.scheduled_action_lock: + return self.scheduled_action + + def reset_scheduled_action(self): + with self.scheduled_action_lock: + self.scheduled_action = None + + def schedule_restart(self): + return self.schedule_action('restart') + + def restart_scheduled(self): + return self.get_scheduled_action() == 'restart' + + def schedule_reinitialize(self): + return self.schedule_action('reinitialize') + + def reinitialize_scheduled(self): + return self.get_scheduled_action() == 'reinitialize' + + def restart(self): + with self.restart_thread_lock: + self.restart_in_progress = True + try: + return self.state_handler.restart() + finally: + with self.restart_thread_lock: + self.restart_in_progress = False + self.reset_scheduled_action() + + def process_scheduled_action(self): + if self.reinitialize_scheduled(): + if self.cluster.is_unlocked(): + logger.error('Cluster has no leader, can not reinitialize') + self.reset_scheduled_action() + elif self.has_lock(): + logger.error('I am the leader, can not reinitialize') + self.reset_scheduled_action() + else: + self.state_handler.stop('immediate') + self.state_handler.remove_data_directory() + self.load_cluster_from_dcs() + + def handle_restart_in_progress(self): + if self.has_lock(): + if self.update_lock(): + return 'updated leader lock during restart' + else: + return 'failed to update leader lock during restart' + elif self.cluster.is_unlocked(): + return 'not healthy enough for leader race' + else: + return 'restart in progress' + + def _run_cycle(self): try: self.load_cluster_from_dcs() @@ -120,6 +195,9 @@ class Ha: if not self.cluster.is_unlocked() and not self.cluster.initialize: self.dcs.initialize() # fix it + # currently it can trigger only reinitialize + self.process_scheduled_action() + # is data directory empty? if self.state_handler.data_directory_empty(): return self.bootstrap() # new node @@ -127,10 +205,14 @@ class Ha: elif not self.cluster.initialize and self.cluster.is_unlocked(): self.dcs.initialize() + if self.restart_in_progress: + return self.handle_restart_in_progress() + # try to start dead postgres - if self.recover() and not self.has_lock(): - # no lock, do not try to promote immediately - return 'started as a secondary' + if not self.state_handler.is_healthy(): + msg = self.recover() + if msg is not None: + return msg if self.cluster.is_unlocked(): return self.process_unhealthy_cluster() @@ -143,3 +225,7 @@ class Ha: return 'demoted self because DCS is not accessible and i was a leader' except (psycopg2.Error, PostgresConnectionException): logger.exception('Error communicating with Postgresql. Will try again') + + def run_cycle(self): + with self.restart_thread_lock: + return self._run_cycle() diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 81f5c02e..2bd372cf 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -56,7 +56,6 @@ class Postgresql: self.postmaster_pid = os.path.join(self.data_dir, 'postmaster.pid') self.trigger_file = config.get('recovery_conf', {}).get('trigger_file', None) or 'promote' self.trigger_file = os.path.abspath(os.path.join(self.data_dir, self.trigger_file)) - self._role = 'replica' self._pg_ctl = ['pg_ctl', '-w', '-D', self.data_dir] @@ -67,9 +66,16 @@ class Postgresql: self._connection = None self._cursor_holder = None - self.members = [] # list of already existing replication slots + self.replication_slots = [] # list of already existing replication slots self.retry = Retry(max_tries=-1, deadline=10, max_delay=1, retry_exceptions=PostgresConnectionException) + self._state = 'stopped' + self._role = 'replica' + + if self.is_running(): + self._state = 'running' + self._role = 'master' if self.is_leader() else 'replica' + def get_local_address(self): listen_addresses = self.listen_addresses.split(',') local_address = listen_addresses[0].strip() # take first address from listen_addresses @@ -101,6 +107,8 @@ class Postgresql: except psycopg2.Error as e: if cursor and cursor.connection.closed == 0: raise e + if self.state == 'restarting': + raise RetryFailedError('cluster is being restarted') raise PostgresConnectionException('connection problems') def query(self, sql, *params): @@ -113,8 +121,12 @@ class Postgresql: return not os.path.exists(self.data_dir) or os.listdir(self.data_dir) == [] def initialize(self): + self._state = 'initalizing new cluster' ret = subprocess.call(self._pg_ctl + ['initdb', '-o', '--encoding=UTF8']) == 0 - ret and self.write_pg_hba() + if ret: + self.write_pg_hba() + else: + self._state = 'initdb failed' return ret def delete_trigger_file(self): @@ -137,6 +149,7 @@ class Postgresql: return "host={host} port={port} user={user}".format(**conn) def create_replica(self, master_connection, env): + self._state = 'building replica from {host}:{port}'.format(**master_connection) connstring = self.build_connstring(master_connection) cmd = self.config['restore'] try: @@ -144,7 +157,9 @@ class Postgresql: self.delete_trigger_file() except: logger.exception('Error when creating replica') - return 1 + ret = 1 + if ret != 0: + self._state = 'failed to build replica from {host}:{port}'.format(**master_connection) return ret def is_leader(self): @@ -169,38 +184,60 @@ class Postgresql: def role(self): return self._role + @property + def state(self): + return self._state + def start(self, block_callbacks=False): if self.is_running(): - self._role = 'master' if self.is_leader() else 'replica' - self.schedule_load_slots = self.use_slots logger.error('Cannot start PostgreSQL because one is already running.') - return False + return True self._role = 'replica' if os.path.exists(self.recovery_conf) else 'master' if os.path.exists(self.postmaster_pid): os.remove(self.postmaster_pid) logger.info('Removed %s', self.postmaster_pid) + if not block_callbacks: + self._state = 'starting' + ret = subprocess.call(self._pg_ctl + ['start', '-o', self.server_options()]) == 0 + + self._state = 'running' if ret else 'start failed' + self.schedule_load_slots = ret and self.use_slots self.save_configuration_files() # block_callbacks is used during restart to avoid # running start/stop callbacks in addition to restart ones - ret and not block_callbacks and ret and self.call_nowait(ACTION_ON_START) + ret and not block_callbacks and self.call_nowait(ACTION_ON_START) return ret + def checkpoint(self): + try: + self.query('SET statement_timeout TO 0') + self.query('CHECKPOINT') + except: + logging.exception('Exception diring CHECKPOINT') + def stop(self, mode='fast', block_callbacks=False): + if not self.is_running(): + if not block_callbacks: + self._state = 'stopped' + return True + if block_callbacks: - try: - self.query('SET statement_timeout TO 0') - self.query('CHECKPOINT') - except: - logging.exception('Exception diring CHECKPOINT') + self.checkpoint() + else: + self._state = 'stopping' ret = subprocess.call(self._pg_ctl + ['stop', '-m', mode]) == 0 # block_callbacks is used during restart to avoid # running start/stop callbacks in addition to restart ones - ret and not block_callbacks and self.call_nowait(ACTION_ON_STOP) + if not ret: + self._state = 'stop failed' + elif not block_callbacks: + self._state = 'stopped' + self.call_nowait(ACTION_ON_STOP) return ret def reload(self): @@ -209,8 +246,12 @@ class Postgresql: return ret def restart(self): + self._state = 'restarting' ret = self.stop(block_callbacks=True) and self.start(block_callbacks=True) - ret and self.call_nowait(ACTION_ON_RESTART) + if ret: + self.call_nowait(ACTION_ON_RESTART) + else: + self._state = 'restart failed ({})'.format(self._state) return ret def server_options(self): @@ -356,26 +397,26 @@ recovery_target_timeline = 'latest' def load_replication_slots(self): if self.use_slots and self.schedule_load_slots: cursor = self.query("SELECT slot_name FROM pg_replication_slots WHERE slot_type='physical'") - self.members = [r[0] for r in cursor] + self.replication_slots = [r[0] for r in cursor] self.schedule_load_slots = False def sync_replication_slots(self, cluster): if self.use_slots: self.load_replication_slots() - members = [m.name for m in cluster.members if m.name != self.name] if self.role == 'master' else [] + slots = [m.name for m in cluster.members if m.name != self.name] if self.role == 'master' else [] # drop unused slots - for slot in set(self.members) - set(members): + for slot in set(self.replication_slots) - set(slots): self.query("""SELECT pg_drop_replication_slot(%s) WHERE EXISTS(SELECT 1 FROM pg_replication_slots WHERE slot_name = %s)""", slot, slot) # create new slots - for slot in set(members) - set(self.members): + for slot in set(slots) - set(self.replication_slots): self.query("""SELECT pg_create_physical_replication_slot(%s) WHERE NOT EXISTS (SELECT 1 FROM pg_replication_slots WHERE slot_name = %s)""", slot, slot) - self.members = members + self.replication_slots = slots def last_operation(self): return str(self.xlog_position()) diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index 9cd15fbc..5d221ac6 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -211,8 +211,8 @@ class ZooKeeper(AbstractDCS): def take_leader(self): return self.attempt_to_acquire_leader() - def update_leader(self, state_handler): - last_operation = state_handler.last_operation().encode('utf-8') + def write_leader_optime(self, last_operation): + last_operation = last_operation.encode('utf-8') if last_operation != self.last_leader_operation: self.last_leader_operation = last_operation path = self.leader_optime_path @@ -225,6 +225,8 @@ class ZooKeeper(AbstractDCS): logger.exception('Failed to create %s', path) except: logger.exception('Failed to update %s', path) + + def update_leader(self): return True def delete_leader(self): diff --git a/tests/test_api.py b/tests/test_api.py index b8f9e038..4bb481bf 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -10,6 +10,10 @@ from test_postgresql import psycopg2_connect, MockCursor class MockPostgresql(Mock): + name = 'test' + state = 'running' + role = 'master' + def connection(self): return psycopg2_connect() @@ -17,9 +21,28 @@ class MockPostgresql(Mock): return True +class MockHa(Mock): + + dcs = Mock() + state_handler = MockPostgresql() + + def schedule_restart(self): + return 'restart' + + def schedule_reinitialize(self): + return 'reinitialize' + + def restart(self): + return True + + def restart_scheduled(self): + return False + + class MockPatroni: postgresql = MockPostgresql() + ha = MockHa() class MockRequest: @@ -47,18 +70,38 @@ class MockRestApiServer(RestApiServer): class TestRestApiHandler(unittest.TestCase): def test_do_GET(self): - MockRestApiServer(RestApiHandler, b'GET /') - with patch.object(RestApiServer, 'query', Mock(side_effect=psycopg2.OperationalError())): - MockRestApiServer(RestApiHandler, b'GET /') - - def test_do_GET_sampleauth(self): - MockRestApiServer(RestApiHandler, b'GET /sampleauth') - MockRestApiServer(RestApiHandler, b'GET /sampleauth\nAuthorization:') - MockRestApiServer(RestApiHandler, b'GET /sampleauth\nAuthorization: Basic dGVzdDp0ZXN0') + MockRestApiServer(RestApiHandler, b'GET /master') + MockRestApiServer(RestApiHandler, b'GET /replica') + with patch.object(MockHa, 'restart_scheduled', Mock(return_value=True)): + MockRestApiServer(RestApiHandler, b'GET /master') def test_do_GET_patroni(self): MockRestApiServer(RestApiHandler, b'GET /patroni') + def test_basicauth(self): + MockRestApiServer(RestApiHandler, b'POST /restart HTTP/1.0') + MockRestApiServer(RestApiHandler, b'POST /restart HTTP/1.0\nAuthorization:') + + def test_do_POST_restart(self): + request = b'POST /restart HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' + MockRestApiServer(RestApiHandler, request) + with patch.object(MockHa, 'schedule_restart', Mock(return_value=None)): + MockRestApiServer(RestApiHandler, request) + with patch.object(MockHa, 'restart', Mock(side_effect=Exception)): + MockRestApiServer(RestApiHandler, request) + + @patch.object(MockHa, 'dcs') + def test_do_POST_reinitialize(self, dcs): + cluster = dcs.get_cluster.return_value + request = b'POST /reinitialize HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' + MockRestApiServer(RestApiHandler, request) + cluster.is_unlocked.return_value = False + MockRestApiServer(RestApiHandler, request) + with patch.object(MockHa, 'schedule_reinitialize', Mock(return_value=None)): + MockRestApiServer(RestApiHandler, request) + cluster.leader.name = 'test' + MockRestApiServer(RestApiHandler, request) + @patch('time.sleep', Mock()) def test_RestApiServer_query(self): with patch.object(MockCursor, 'execute', Mock(side_effect=psycopg2.OperationalError)): diff --git a/tests/test_etcd.py b/tests/test_etcd.py index d2824699..18e8f22e 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -242,8 +242,11 @@ class TestEtcd(unittest.TestCase): self.etcd._base_path = '/service/failed' self.assertFalse(self.etcd.attempt_to_acquire_leader()) + def test_write_leader_optime(self): + self.etcd.write_leader_optime('0') + def test_update_leader(self): - self.assertTrue(self.etcd.update_leader(MockPostgresql())) + self.assertTrue(self.etcd.update_leader()) def test_initialize(self): self.assertFalse(self.etcd.initialize()) diff --git a/tests/test_ha.py b/tests/test_ha.py index 925beb33..70018672 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -82,10 +82,25 @@ class TestHa(unittest.TestCase): self.e.get_cluster = get_cluster_not_initialized_without_leader ha.load_cluster_from_dcs() - def test_start_as_slave(self): + def test_update_lock(self): + self.p.last_operation = Mock(side_effect=PostgresException('')) + self.assertTrue(self.ha.update_lock()) + + def test_start_as_replica(self): self.p.is_healthy = false self.assertEquals(self.ha.run_cycle(), 'started as a secondary') + def test_recover_replica_failed(self): + self.p.is_healthy = false + self.p.start = false + self.assertEquals(self.ha.run_cycle(), 'failed to start postgres') + + def test_recover_master_failed(self): + self.p.is_healthy = false + self.p.start = false + self.ha.has_lock = true + self.assertEquals(self.ha.run_cycle(), 'removed leader key after trying and failing to start postgres') + def test_start_as_readonly(self): self.ha.cluster.is_unlocked = false self.p.is_leader = self.p.is_healthy = false @@ -174,3 +189,35 @@ class TestHa(unittest.TestCase): self.e.initialize = true self.p.bootstrap = Mock(side_effect=PostgresException("Could not bootstrap master PostgreSQL")) self.assertRaises(PostgresException, self.ha.bootstrap) + + def test_reinitialize(self): + self.ha.schedule_reinitialize() + self.ha.run_cycle() + self.assertIsNone(self.ha.get_scheduled_action()) + + self.ha.cluster = get_cluster_initialized_with_leader() + self.ha.schedule_reinitialize() + self.ha.run_cycle() + + self.ha.has_lock = true + self.ha.schedule_reinitialize() + self.ha.run_cycle() + self.assertIsNone(self.ha.get_scheduled_action()) + + def test_restart(self): + self.ha.schedule_restart() + self.assertTrue(self.ha.restart_scheduled()) + self.ha.restart() + + def test_restart_in_progress(self): + self.ha.restart_in_progress = True + self.assertEquals(self.ha.run_cycle(), 'not healthy enough for leader race') + + self.ha.cluster = get_cluster_initialized_with_leader() + self.assertEquals(self.ha.run_cycle(), 'restart in progress') + + self.ha.has_lock = true + self.assertEquals(self.ha.run_cycle(), 'updated leader lock during restart') + + self.ha.update_lock = false + self.assertEquals(self.ha.run_cycle(), 'failed to update leader lock during restart') diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index b4988c25..d1ab7e8d 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -85,10 +85,12 @@ def psycopg2_connect(*args, **kwargs): @patch('subprocess.call', Mock(return_value=0)) -@patch('shutil.copy', Mock()) @patch('psycopg2.connect', psycopg2_connect) +@patch('shutil.copy', Mock()) class TestPostgresql(unittest.TestCase): + @patch('subprocess.call', Mock(return_value=0)) + @patch('psycopg2.connect', psycopg2_connect) def setUp(self): self.p = Postgresql({'name': 'test0', 'scope': 'batman', 'data_dir': 'data/test0', 'listen': '127.0.0.1, *:5432', 'connect_address': '127.0.0.2:5432', @@ -121,13 +123,24 @@ class TestPostgresql(unittest.TestCase): self.assertTrue(self.p.initialize()) self.assertTrue(os.path.exists(os.path.join(self.p.data_dir, 'pg_hba.conf'))) - def test_start_stop(self): - self.assertFalse(self.p.start()) - self.p.is_running = false - with open(os.path.join(self.p.data_dir, 'postmaster.pid'), 'w'): - pass + def test_start(self): self.assertTrue(self.p.start()) + self.p.is_running = false + open(os.path.join(self.p.data_dir, 'postmaster.pid'), 'w').close() + self.assertTrue(self.p.start()) + + def test_stop(self): self.assertTrue(self.p.stop()) + with patch('subprocess.call', Mock(return_value=1)): + self.assertTrue(self.p.stop()) + self.p.is_running = Mock(return_value=True) + self.assertFalse(self.p.stop()) + + def test_restart(self): + self.p.start = false + self.p.is_running = false + self.assertFalse(self.p.restart()) + self.assertEquals(self.p.state, 'restart failed (restarting)') def test_sync_from_leader(self): self.assertTrue(self.p.sync_from_leader(self.leader)) @@ -157,6 +170,8 @@ class TestPostgresql(unittest.TestCase): @patch.object(MockConnect, 'closed', 2) def test__query(self): self.assertRaises(PostgresConnectionException, self.p._query, 'blabla') + self.p._state = 'restarting' + self.assertRaises(RetryFailedError, self.p._query, 'blabla') def test_query(self): self.p.query('select 1') @@ -184,6 +199,7 @@ class TestPostgresql(unittest.TestCase): self.assertFalse(self.p.is_healthy()) def test_promote(self): + self.p._role = 'replica' self.assertTrue(self.p.promote()) self.assertTrue(self.p.promote()) @@ -211,8 +227,8 @@ class TestPostgresql(unittest.TestCase): self.p.move_data_directory() def test_bootstrap(self): - self.assertRaises(PostgresException, self.p.bootstrap) - self.p.start = Mock(return_value=True) + with patch('subprocess.call', Mock(return_value=1)): + self.assertRaises(PostgresException, self.p.bootstrap) self.p.bootstrap() def test_remove_data_directory(self): diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py index afbdad5f..b04f5b5a 100644 --- a/tests/test_zookeeper.py +++ b/tests/test_zookeeper.py @@ -134,11 +134,13 @@ class TestZooKeeper(unittest.TestCase): self.zk.take_leader() def test_update_leader(self): - self.zk.last_leader_operation = -1 - self.assertTrue(self.zk.update_leader(MockPostgresql())) + self.assertTrue(self.zk.update_leader()) + + def test_write_leader_optime(self): + self.zk.last_leader_operation = '0' + self.zk.write_leader_optime('1') self.zk._base_path = self.zk._base_path.replace('test', 'bla') - self.zk.last_leader_operation = -1 - self.assertTrue(self.zk.update_leader(MockPostgresql())) + self.zk.write_leader_optime('2') def test_watch(self): self.zk.watch(0) From 3b1b6ff448e104ac41bba963435bc67ce3d37ff5 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 24 Sep 2015 16:54:16 +0200 Subject: [PATCH 05/59] revert log level to INFO --- patroni/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/__init__.py b/patroni/__init__.py index 35f206df..cadf335a 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -73,7 +73,7 @@ class Patroni: def main(): - logging.basicConfig(format='%(asctime)s %(levelname)s: %(message)s', level=logging.DEBUG) + logging.basicConfig(format='%(asctime)s %(levelname)s: %(message)s', level=logging.INFO) logging.getLogger('requests').setLevel(logging.WARNING) setup_signal_handlers() From d6c8df45e149a21e883124e3c09f72f5bff3606b Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Fri, 25 Sep 2015 13:08:12 +0200 Subject: [PATCH 06/59] Write the pg_rewind password in pgpass instead of passing it in the command line. --- patroni/postgresql.py | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 5d0e27da..be82a6e4 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -135,14 +135,17 @@ class Postgresql: def delete_trigger_file(self): os.path.exists(self.trigger_file) and os.unlink(self.trigger_file) + def write_pgpass(self, record, append=False): + pgpass = 'pgpass' + with open(pgpass, 'w' if not append else 'a') as f: + os.fchmod(f.fileno(), 0o600) + f.write('{host}:{port}:*:{user}:{password}\n'.format(**record)) + return pgpass + def sync_from_leader(self, leader): r = parseurl(leader.conn_url) - pgpass = 'pgpass' - with open(pgpass, 'w') as f: - os.fchmod(f.fileno(), 0o600) - f.write('{host}:{port}:*:{user}:{password}\n'.format(**r)) - + pgpass = self.write_pgpass(r) env = os.environ.copy() env['PGPASSFILE'] = pgpass return self.create_replica(r, env) == 0 @@ -280,10 +283,8 @@ class Postgresql: f.write(line + '\n') @staticmethod - def primary_conninfo(leader_url, replacement=None): + def primary_conninfo(leader_url): r = parseurl(leader_url) - if replacement is not None: - r.update(replacement) return 'user={user} password={password} host={host} port={port} sslmode=prefer sslcompression=1'.format(**r) def check_recovery_conf(self, leader): @@ -313,8 +314,14 @@ recovery_target_timeline = 'latest' for name, value in self.config.get('recovery_conf', {}).items(): f.write("{} = '{}'\n".format(name, value)) + def prepare_pg_rewind_connection(self, leader_url, pg_rewind): + r = parseurl(leader_url) + r.update(pg_rewind) + self.write_pgpass(r, append=True) + return "user={user} host={host} port={port} dbname=postgres sslmode=prefer sslcompression=1".format(**r) + def pg_rewind(self, leader): - pc = self.primary_conninfo(leader.conn_url, self._pg_rewind) + ' dbname=postgres' + pc = self.prepare_pg_rewind_connection(leader.conn_url, self._pg_rewind) logger.info("running pg_rewind from {}".format(pc)) pg_rewind = ['pg_rewind', '-D', self.data_dir, '--source-server', pc] try: From d44a54628ad7191da16279f6ae1d85d95ceee8b2 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Fri, 25 Sep 2015 16:00:24 +0200 Subject: [PATCH 07/59] remove the data directory on an unsuccessfull rewind attempt. --- patroni/postgresql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 23047026..a7270460 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -343,7 +343,7 @@ recovery_target_timeline = 'latest' ret = self.start() else: ret = False - self.move_data_directory() + self.remove_data_directory() logger.error("unable to rewind the former leader") else: ret = self.restart() From e39d3187324a0e8dc08061bf0874c2fe026db836 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 28 Sep 2015 12:04:06 +0200 Subject: [PATCH 08/59] Eliminate os.system call. --- patroni/postgresql.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index a7270460..7e3e9088 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -77,7 +77,10 @@ class Postgresql: self._pg_rewind_present = ('username' in self._pg_rewind and ('wal_log_hints' in self.config['parameters'] or 'data_checksums' in self.config['parameters']) and - os.system("pg_rewind --version >/dev/null 2>&1") == 0) + subprocess.call(['pg_rewind', + '--version'], + stdout=open(os.devnull, 'w'), + stderr=subprocess.STDOUT) == 0) if self._pg_rewind_present: self._pg_rewind['user'] = self._pg_rewind['username'] except: From c218054d05e4ddbaee5a144ae439e7c6ffe72bdb Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 28 Sep 2015 17:00:42 +0200 Subject: [PATCH 09/59] Implement manual failover Implementation is done on top of feature/is-healthiest-via-api and feature/api branches. In order to trigger manual failover one has to create 'failover' key in a configuration store with the value in following format: 'leader_name:member_name' leader_name can be empty or should match with the name of current leader member_name can be empty or should match with the name one of cluster nodes Leader always checks that either desired member (if specified) or one of the memners is accessible and healthy before demote. After leader has deomted himself other nodes are performig checks that desired node is healthy. If it is not they are participating in a leader race. In some cases (when accidently there is no healthy nodes) former leader can also participate in a leader race. Current implementation does not provide REST API endpoint for a manual failover. --- patroni/__init__.py | 11 +-- patroni/api.py | 9 +-- patroni/dcs.py | 25 ++++++- patroni/etcd.py | 22 ++++-- patroni/ha.py | 147 +++++++++++++++++++++++++++++++++++++-- patroni/postgresql.py | 32 +-------- patroni/zookeeper.py | 35 +++++++--- tests/test_etcd.py | 4 ++ tests/test_ha.py | 88 +++++++++++++++++++---- tests/test_patroni.py | 7 +- tests/test_postgresql.py | 29 ++------ tests/test_zookeeper.py | 19 ++++- 12 files changed, 320 insertions(+), 108 deletions(-) diff --git a/patroni/__init__.py b/patroni/__init__.py index cadf335a..6ca96df0 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -8,7 +8,7 @@ from patroni.api import RestApiServer from patroni.etcd import Etcd from patroni.ha import Ha from patroni.postgresql import Postgresql -from patroni.utils import setup_signal_handlers, sleep, reap_children +from patroni.utils import setup_signal_handlers, reap_children from patroni.zookeeper import ZooKeeper logger = logging.getLogger(__name__) @@ -42,12 +42,6 @@ class Patroni: return True return self.ha.dcs.touch_member(connection_string, ttl) - def initialize(self): - # wait for etcd to be available - while not self.touch_member(): - logger.info('waiting on DCS') - sleep(5) - def schedule_next_run(self): self.next_run += self.nap_time current_time = time.time() @@ -62,12 +56,12 @@ class Patroni: self.next_run = time.time() while True: - self.touch_member() logger.info(self.ha.run_cycle()) try: self.ha.cluster and self.ha.state_handler.sync_replication_slots(self.ha.cluster) except: logger.exception('Exception when changing replication slots') + self.touch_member() reap_children() self.schedule_next_run() @@ -85,7 +79,6 @@ def main(): config = yaml.load(f) patroni = Patroni(config) - patroni.initialize() try: patroni.run() except KeyboardInterrupt: diff --git a/patroni/api.py b/patroni/api.py index ebb732a4..d507bfb3 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -143,10 +143,11 @@ class RestApiHandler(BaseHTTPRequestHandler): row = self.query("""SELECT to_char(pg_postmaster_start_time(), 'YYYY-MM-DD HH24:MI:SS.MS TZ'), pg_is_in_recovery(), CASE WHEN pg_is_in_recovery() - THEN null - ELSE pg_current_xlog_location() END, - pg_last_xlog_receive_location(), - pg_last_xlog_replay_location(), + THEN 0 + ELSE pg_xlog_location_diff(pg_current_xlog_location(), '0/0')::bigint + END, + pg_xlog_location_diff(pg_last_xlog_receive_location(), '0/0')::bigint, + pg_xlog_location_diff(pg_last_xlog_replay_location(), '0/0')::bigint, pg_is_in_recovery() AND pg_is_xlog_replay_paused()""", retry=retry)[0] return { 'state': self.server.patroni.postgresql.state, diff --git a/patroni/dcs.py b/patroni/dcs.py index ebf625d6..fb034322 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -56,7 +56,15 @@ class Leader(namedtuple('Leader', 'index,expiration,ttl,member')): return self.member.conn_url -class Cluster(namedtuple('Cluster', 'initialize,leader,last_leader_operation,members')): +class Failover(namedtuple('Failover', 'index,leader,member')): + + @staticmethod + def from_node(index, value): + t = [a.strip() for a in value.split(':')] + [''] + return Failover(index, t[0], t[1]) if t[0] or t[1] else None + + +class Cluster(namedtuple('Cluster', 'initialize,leader,last_leader_operation,members,failover')): """Immutable object (namedtuple) which represents PostgreSQL cluster. Consists of the following fields: @@ -64,7 +72,8 @@ class Cluster(namedtuple('Cluster', 'initialize,leader,last_leader_operation,mem :param leader: `Leader` object which represents current leader of the cluster :param last_leader_operation: int or long object containing position of last known leader operation. This value is stored in `/optime/leader` key - :param members: list of Member object, all PostgreSQL cluster members including leader""" + :param members: list of Member object, all PostgreSQL cluster members including leader + :param failover: reference to `Failover` object""" def is_unlocked(self): return not (self.leader and self.leader.name) @@ -76,6 +85,7 @@ class AbstractDCS: _INITIALIZE = 'initialize' _LEADER = 'leader' + _FAILOVER = 'failover' _MEMBERS = 'members/' _OPTIME = 'optime' _LEADER_OPTIME = _OPTIME + '/' + _LEADER @@ -109,6 +119,10 @@ class AbstractDCS: def leader_path(self): return self.client_path(self._LEADER) + @property + def failover_path(self): + return self.client_path(self._FAILOVER) + @property def leader_optime_path(self): return self.client_path(self._LEADER_OPTIME) @@ -143,6 +157,13 @@ class AbstractDCS: Key must be created atomically. In case if key already exists it should not be overwritten and `!False` must be returned""" + @abc.abstractmethod + def set_failover_value(self, value, index=None): + """Create or update `/failover` key""" + + def manual_failover(self, leader, member, index=None): + return self.set_failover_value(leader + (':' + member if member else ''), index) + def current_leader(self): try: cluster = self.get_cluster() diff --git a/patroni/etcd.py b/patroni/etcd.py index b189c188..c2995da9 100644 --- a/patroni/etcd.py +++ b/patroni/etcd.py @@ -10,7 +10,8 @@ import urllib3 from dns.exception import DNSException from dns import resolver -from patroni.dcs import AbstractDCS, Cluster, DCSError, Leader, Member, parse_connection_string +from patroni.dcs import AbstractDCS, Cluster, Failover, Leader, Member, parse_connection_string +from patroni.exceptions import DCSError from patroni.utils import Retry, RetryFailedError, sleep from requests.exceptions import RequestException @@ -80,7 +81,7 @@ class Client(etcd.Client): for host, port in self.get_srv_record(discovery_srv): url = '{}://{}:{}/members'.format(self._protocol, host, port) try: - response = requests.get(url) + response = requests.get(url, timeout=5) if response.ok: for member in response.json(): ret.extend(member['clientURLs']) @@ -195,9 +196,14 @@ class Etcd(AbstractDCS): member = ([m for m in members if m.name == leader.value] or [member])[0] leader = Leader(leader.modifiedIndex, leader.expiration, leader.ttl, member) - self.cluster = Cluster(initialize, leader, last_leader_operation, members) + # failover key + failover = nodes.get(self._FAILOVER, None) + if failover: + failover = Failover.from_node(failover.modifiedIndex, failover.value) + + self.cluster = Cluster(initialize, leader, last_leader_operation, members, failover) except etcd.EtcdKeyNotFound: - self.cluster = Cluster(False, None, None, []) + self.cluster = Cluster(False, None, None, [], None) except: self.cluster = None logger.exception('get_cluster') @@ -221,6 +227,10 @@ class Etcd(AbstractDCS): pass return False + @catch_etcd_errors + def set_failover_value(self, value, index=None): + return self.client.write(self.failover_path, value, prevIndex=index or 0) + @catch_etcd_errors def write_leader_optime(self, last_operation): return self.client.set(self.leader_optime_path, last_operation) @@ -231,7 +241,7 @@ class Etcd(AbstractDCS): @catch_etcd_errors def initialize(self): - return self.client.write(self.initialize_path, self._name, prevExist=False) + return self.retry(self.client.write, self.initialize_path, self._name, prevExist=False) @catch_etcd_errors def delete_leader(self): @@ -239,7 +249,7 @@ class Etcd(AbstractDCS): @catch_etcd_errors def cancel_initialization(self): - return self.client.delete(self.initialize_path, prevValue=self._name) + return self.retry(self.client.delete, self.initialize_path, prevValue=self._name) def watch(self, timeout): # watch on leader key changes if it is defined and current node is not lock owner diff --git a/patroni/ha.py b/patroni/ha.py index 64179a78..628b9c41 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -1,7 +1,9 @@ import logging import psycopg2 +import requests from patroni.exceptions import DCSError, PostgresConnectionException +from multiprocessing.pool import ThreadPool from threading import Lock logger = logging.getLogger(__name__) @@ -9,9 +11,9 @@ logger = logging.getLogger(__name__) class Ha: - def __init__(self, state_handler, etcd): + def __init__(self, state_handler, dcs): self.state_handler = state_handler - self.dcs = etcd + self.dcs = dcs self.cluster = None self.old_cluster = None self.scheduled_action = None @@ -100,9 +102,141 @@ class Ha: self.state_handler.promote() return promote_message + @staticmethod + def fetch_node_status(member): + """This function perform http get request on member.api_url and fetches its status + :returns: tuple(`member`, reachable, in_recovery, xlog_location) + + reachable - `!False` if the node is not reachable or is not responding with correct JSON + in_recovery - `!True` if pg_is_in_recovery() == true + xlog_location - value of `replayed_location` or `location` from JSON, dependin on its role.""" + + try: + response = requests.get(member.api_url, timeout=2, verify=False) + logger.info('Got response from %s %s: %s', member.name, member.api_url, response.content) + json = response.json() + is_master = json['role'] == 'master' + xlog_location = json['xlog']['location' if is_master else 'replayed_location'] + return (member, True, not is_master, xlog_location) + except: + logging.exception('request failed: GET %s', member.api_url) + return (member, False, None, 0) + + def fetch_nodes_statuses(self, members): + pool = ThreadPool(len(members)) + results = pool.map(self.fetch_node_status, members) # Run API calls on members in parallel + pool.close() + pool.join() + return results + + def _is_healthiest_node(self, members, check_replication_lag=True): + """This method tries to determine whether I am healthy enough to became a new leader candidate or not.""" + + if self.state_handler.is_leader(): + return True + + if check_replication_lag and not self.state_handler.check_replication_lag(self.cluster.last_leader_operation): + return False # Too far behind last reported xlog location on master + + # Prepare list of nodes to run check against + members = [m for m in members if m.name != self.state_handler.name and m.api_url] + + if members: + my_xlog_location = self.state_handler.xlog_position() + for member, reachable, in_recovery, xlog_location in self.fetch_nodes_statuses(members): + if reachable: # If the node is unreachable it's not healhy + if not in_recovery: + logger.warning('Master (%s) is still alive', member.name) + return False + if my_xlog_location < xlog_location: + return False + return True + + def is_failover_possible(self, members): + ret = False + members = [m for m in members if m.name != self.state_handler.name and m.api_url] + if members: + for member, reachable, in_recovery, xlog_location in self.fetch_nodes_statuses(members): + if reachable: + ret = True # TODO: check xlog_location + else: + logger.info('Member %s is not reachable', member.name) + else: + logger.warning('manual failover: members list is empty') + return ret + + def manual_failover_process_no_leader(self): + failover = self.cluster.failover + if failover.member: # manual failover to specific member + if failover.member == self.state_handler.name: # manual failover to me + return True + + # find specific node and check that it is healthy + members = [m for m in self.cluster.members if m.name == failover.member] + if members: + member, reachable, in_recovery, xlog_location = self.fetch_node_status(members[0]) + if reachable: # node is healthy + logger.info('manual failover: to %s, i am %s', member.name, self.state_handler.name) + return False + # we wanted to failover to specific member but it is not healthy + logger.warning('manual failover: member %s is unhealthy', member.name) + + # at this point we should consider all members as a candidates for failover + # i.e. we assume that failover.member is None + + # try to pick some other members to failover and check that they are healthy + if failover.leader: + if self.state_handler.name == failover.leader: # I was the leader + # exclude me and desired member which is unhealthy (failover.member can be None) + members = [m for m in self.cluster.members if m.name != failover.member] + if self.is_failover_possible(members): # check that there are healthy members + return False + else: # I was the leader and it looks like currently I am the only healthy member + return True + + # at this point we assume that our node is a candidate for a failover among all nodes except former leader + + # exclude former leader from the list (failover.leader can be None) + members = [m for m in self.cluster.members if m.name != failover.leader] + return self._is_healthiest_node(members, check_replication_lag=False) + + def is_healthiest_node(self): + if self.cluster.failover: + return self.manual_failover_process_no_leader() + + # run usual health check + members = {m.name: m for m in self.old_cluster.members + self.cluster.members} + return self._is_healthiest_node(members.values()) + + def process_manual_failover_from_leader(self): + failover = self.cluster.failover + if not failover.leader or failover.leader == self.state_handler.name: + if not failover.member or failover.member != self.state_handler.name: + members = [m for m in self.cluster.members if not failover.member or m.name == failover.member] + if self.is_failover_possible(members): # check that there are healthy members + self.state_handler.follow_the_leader(None) + self.cluster = None + if self.dcs.delete_leader(): + return 'manual failover: demoted self and released leader lock' + else: + return 'manual failover: demoted self but failed to release leader lock' + else: + logger.warning('manual failover: no healthy members found, failover is not possible') + else: + logger.warning('manual failover: I am already the leader, no need to failover') + else: + logger.warning('manual failover: leader name does not match: %s != %s', + self.cluster.failover.leader, self.state_handler.name) + + logger.info('Trying to clean up failover key') + self.dcs.manual_failover('', '', self.cluster.failover.index) + def process_unhealthy_cluster(self): - if self.state_handler.is_healthiest_node(self.old_cluster): + if self.is_healthiest_node(): if self.acquire_lock(): + if self.cluster.failover: + logger.info('Cleanning up failover key after acquiring leader lock...') + self.dcs.manual_failover('', '') return self.enforce_master_role('acquired session lock as a leader', 'promoted self to leader by acquiring session lock') else: @@ -114,6 +248,11 @@ class Ha: def process_healthy_cluster(self): if self.has_lock(): + if self.cluster.failover: + msg = self.process_manual_failover_from_leader() + if msg is not None: + return msg + if self.update_lock(): return self.enforce_master_role('no action. i am the leader with the lock', 'promoted self to leader because i had the session lock') @@ -220,7 +359,7 @@ class Ha: return self.process_healthy_cluster() except DCSError: logger.error('Error communicating with DCS') - if self.state_handler.is_leader(): + if self.state_handler.is_running() and self.state_handler.is_leader(): self.state_handler.demote(None) return 'demoted self because DCS is not accessible and i was a leader' except (psycopg2.Error, PostgresConnectionException): diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 2bd372cf..bc3188ed 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -266,36 +266,8 @@ class Postgresql: return False return True - def is_healthiest_node(self, cluster): - if self.is_leader(): - return True - - if cluster.last_leader_operation - self.xlog_position() > self.config.get('maximum_lag_on_failover', 0): - return False - - for member in cluster.members: - if member.name == self.name: - continue - try: - r = parseurl(member.conn_url) - member_conn = psycopg2.connect(**r) - member_conn.autocommit = True - member_cursor = member_conn.cursor() - member_cursor.execute( - "SELECT pg_is_in_recovery(), %s - pg_xlog_location_diff(pg_last_xlog_replay_location(), '0/0')", - (self.xlog_position(),)) - row = member_cursor.fetchone() - member_cursor.close() - member_conn.close() - logger.error([self.name, member.name, row]) - if not row[0]: - logger.warning('Master (%s) is still alive', member.name) - return False - if row[1] < 0: - return False - except psycopg2.Error: - continue - return True + def check_replication_lag(self, last_leader_operation): + return last_leader_operation - self.xlog_position() <= self.config.get('maximum_lag_on_failover', 0) def write_pg_hba(self): with open(os.path.join(self.data_dir, 'pg_hba.conf'), 'a') as f: diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index 5d221ac6..7458ae46 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -5,7 +5,8 @@ import time from kazoo.client import KazooClient, KazooState from kazoo.exceptions import NoNodeError, NodeExistsError -from patroni.dcs import AbstractDCS, Cluster, DCSError, Leader, Member, parse_connection_string +from patroni.dcs import AbstractDCS, Cluster, Failover, Leader, Member, parse_connection_string +from patroni.exceptions import DCSError from patroni.utils import sleep from requests.exceptions import RequestException @@ -134,7 +135,7 @@ class ZooKeeper(AbstractDCS): def _inner_load_cluster(self): self.cluster_event.clear() - nodes = set(self.get_children(self.client_path(''))) + nodes = set(self.get_children(self.client_path(''), self.cluster_watcher)) # get initialize flag initialize = self._INITIALIZE in nodes @@ -143,7 +144,7 @@ class ZooKeeper(AbstractDCS): members = self.load_members() if self._MEMBERS[:-1] in nodes else [] # get leader - leader = self.get_node(self.leader_path, self.cluster_watcher) if self._LEADER in nodes else None + leader = self.get_node(self.leader_path) if self._LEADER in nodes else None if leader: client_id = self.client.client_id if leader[0] == self._name and client_id is not None and client_id[0] != leader[1].ephemeralOwner: @@ -157,10 +158,15 @@ class ZooKeeper(AbstractDCS): leader = Leader(leader[1].version, None, None, member) self.fetch_cluster = member.index == -1 + # failover key + failover = self.get_node(self.failover_path, watch=self.cluster_watcher) if self._FAILOVER in nodes else None + if failover: + failover = Failover.from_node(failover[1].version, failover[0]) + # get last leader operation self.last_leader_operation = self.get_node(self.leader_optime_path) if self.fetch_cluster else None self.last_leader_operation = 0 if self.last_leader_operation is None else int(self.last_leader_operation[0]) - self.cluster = Cluster(initialize, leader, self.last_leader_operation, members) + self.cluster = Cluster(initialize, leader, self.last_leader_operation, members, failover) def get_cluster(self): if self.exhibitor and self.exhibitor.poll(): @@ -188,11 +194,21 @@ class ZooKeeper(AbstractDCS): ret or logger.info('Could not take out TTL lock') return ret + def set_failover_value(self, value, index=None): + try: + self.client.retry(self.client.set, self.failover_path, value.encode('utf-8'), version=index or -1) + return True + except NoNodeError: + return value == '' or (not index and self._create(self.failover_path, value.encode('utf-8'))) + except: + logging.exception('foo') + return False + def initialize(self): return self._create(self.initialize_path, self._name, makepath=True) def touch_member(self, connection_string, ttl=None): - if self.cluster and any(m.name == self._name for m in self.cluster.members): + if not self.fetch_cluster and self.cluster and any(m.name == self._name for m in self.cluster.members): return True path = self.member_path connection_string = connection_string.encode('utf-8') @@ -201,6 +217,9 @@ class ZooKeeper(AbstractDCS): return True except NodeExistsError: try: + node = self.get_node(path) + if node and self.client.client_id is not None and node[1].ephemeralOwner == self.client.client_id[0]: + return True self.client.retry(self.client.delete, path) self.client.retry(self.client.create, path, connection_string, makepath=True, ephemeral=True) return True @@ -230,8 +249,8 @@ class ZooKeeper(AbstractDCS): return True def delete_leader(self): - if isinstance(self.cluster, Cluster) and self.cluster.leader.name == self._name: - self.client.delete(self.leader_path, version=self.cluster.leader.index) + self.client.restart() + return True def _cancel_initialization(self): node = self.get_node(self.initialize_path) @@ -248,5 +267,5 @@ class ZooKeeper(AbstractDCS): self.cluster_event.wait(timeout) if self.cluster_event.isSet(): self.fetch_cluster = True - return not self.cluster or not self.cluster.leader or self.cluster.leader.name != self._name + return True return False diff --git a/tests/test_etcd.py b/tests/test_etcd.py index 18e8f22e..f55aa15e 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -50,6 +50,8 @@ def requests_get(url, **kwargs): response = MockResponse() if url.startswith('http://local'): raise requests.exceptions.RequestException() + elif ':8011/patroni' in url: + response.content = '{"role": "replica", "xlog": {"replayed_location": 0}}' elif url.endswith('/members'): if url.startswith('http://error'): response.content = '[{}]' @@ -92,6 +94,8 @@ def etcd_read(key, **kwargs): raise etcd.EtcdKeyNotFound response = {"action": "get", "node": {"key": "/service/batman5", "dir": True, "nodes": [ + {"key": "/service/batman5/failover", "value": "", + "modifiedIndex": 1582, "createdIndex": 1582}, {"key": "/service/batman5/initialize", "value": "postgresql0", "modifiedIndex": 1582, "createdIndex": 1582}, {"key": "/service/batman5/leader", "value": "postgresql1", diff --git a/tests/test_ha.py b/tests/test_ha.py index 70018672..6e50c629 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -1,11 +1,11 @@ import unittest from mock import Mock, patch -from patroni.dcs import Cluster, DCSError, Leader, Member +from patroni.dcs import Cluster, Failover, Leader, Member from patroni.etcd import Client, Etcd -from patroni.exceptions import PostgresException +from patroni.exceptions import DCSError, PostgresException from patroni.ha import Ha -from test_etcd import socket_getaddrinfo, etcd_read, etcd_write +from test_etcd import socket_getaddrinfo, etcd_read, etcd_write, requests_get def true(*args, **kwargs): @@ -16,22 +16,25 @@ def false(*args, **kwargs): return False -def get_cluster(initialize, leader): - return Cluster(initialize, leader, None, None) +def get_cluster(initialize, leader, members, failover): + return Cluster(initialize, leader, None, members, failover) def get_cluster_not_initialized_without_leader(): - return get_cluster(None, None) + return get_cluster(None, None, [], None) -def get_cluster_initialized_without_leader(): - return get_cluster(True, None) +def get_cluster_initialized_without_leader(leader=False, failover=None): + m = Member(0, 'leader', 'postgres://replicator:rep-pass@127.0.0.1:5435/postgres', + 'http://127.0.0.1:8008/patroni', None, 28) + l = Leader(0, 0, 0, m) if leader else None + o = Member(0, 'other', 'postgres://replicator:rep-pass@127.0.0.1:5436/postgres', + 'http://127.0.0.1:8011/patroni', None, 28) + return get_cluster(True, l, [m, o], failover) -def get_cluster_initialized_with_leader(): - return get_cluster(True, Leader(0, 0, 0, - Member(0, 'leader', 'postgres://replicator:rep-pass@127.0.0.1:5435/postgres', - None, None, 28))) +def get_cluster_initialized_with_leader(failover=None): + return get_cluster_initialized_without_leader(leader=True, failover=failover) class MockPostgresql(Mock): @@ -51,6 +54,9 @@ class MockPostgresql(Mock): def is_leader(self): return True + def xlog_position(self): + return 0 + def last_operation(self): return 0 @@ -60,6 +66,9 @@ class MockPostgresql(Mock): def bootstrap(self, *args, **kwargs): return True + def check_replication_lag(self, last_leader_operation): + return True + class TestHa(unittest.TestCase): @@ -111,6 +120,7 @@ class TestHa(unittest.TestCase): self.assertEquals(self.ha.run_cycle(), 'acquired session lock as a leader') def test_promoted_by_acquiring_lock(self): + self.ha.is_healthiest_node = true self.p.is_leader = false self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') @@ -119,16 +129,17 @@ class TestHa(unittest.TestCase): self.assertEquals(self.ha.run_cycle(), 'demoted self due after trying and failing to obtain lock') def test_follow_new_leader_after_failing_to_obtain_lock(self): + self.ha.is_healthiest_node = true self.ha.acquire_lock = false self.p.is_leader = false self.assertEquals(self.ha.run_cycle(), 'following new leader after trying and failing to obtain lock') def test_demote_because_not_healthiest(self): - self.p.is_healthiest_node = false + self.ha.is_healthiest_node = false self.assertEquals(self.ha.run_cycle(), 'demoting self because i am not the healthiest node') def test_follow_new_leader_because_not_healthiest(self): - self.p.is_healthiest_node = false + self.ha.is_healthiest_node = false self.p.is_leader = false self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') @@ -221,3 +232,52 @@ class TestHa(unittest.TestCase): self.ha.update_lock = false self.assertEquals(self.ha.run_cycle(), 'failed to update leader lock during restart') + + @patch('requests.get', requests_get) + def test_manual_failover_from_leader(self): + self.ha.has_lock = true + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', '')) + self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, '', MockPostgresql.name)) + self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, '', 'blabla')) + self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') + f = Failover(0, MockPostgresql.name, '') + self.ha.cluster = get_cluster_initialized_with_leader(f) + self.assertEquals(self.ha.run_cycle(), 'manual failover: demoted self but failed to release leader lock') + self.ha.cluster = get_cluster_initialized_with_leader(f) + self.e.client.delete = Mock(return_value=True) + self.assertEquals(self.ha.run_cycle(), 'manual failover: demoted self and released leader lock') + + @patch('requests.get', requests_get) + def test_manual_failover_process_no_leader(self): + self.p.is_leader = false + self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', MockPostgresql.name)) + self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') + self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', 'leader')) + self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') + self.ha.fetch_node_status = lambda e: (e, True, True, 0) # accessible, in_recovery + self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') + self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, MockPostgresql.name, '')) + self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') + self.ha.fetch_node_status = lambda e: (e, False, True, 0) # accessible, in_recovery + self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') + + def test__is_healthiest_node(self): + self.assertTrue(self.ha._is_healthiest_node(self.ha.old_cluster.members)) + self.p.is_leader = false + self.ha.fetch_node_status = lambda e: (e, True, True, 0) # accessible, in_recovery + self.assertTrue(self.ha._is_healthiest_node(self.ha.old_cluster.members)) + self.ha.fetch_node_status = lambda e: (e, True, False, 0) # accessible, not in_recovery + self.assertFalse(self.ha._is_healthiest_node(self.ha.old_cluster.members)) + self.ha.fetch_node_status = lambda e: (e, True, True, 1) # accessible, in_recovery, xlog location ahead + self.assertFalse(self.ha._is_healthiest_node(self.ha.old_cluster.members)) + self.p.check_replication_lag = false + self.assertFalse(self.ha._is_healthiest_node(self.ha.old_cluster.members)) + + @patch('requests.get', requests_get) + def test_fetch_node_status(self): + member = Member(0, 'test', '', 'http://127.0.0.1:8011/patroni', None, None) + self.ha.fetch_node_status(member) + member = Member(0, 'test', '', 'http://localhost:8011/patroni', None, None) + self.ha.fetch_node_status(member) diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 7cac2eba..8936d18b 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -48,7 +48,6 @@ class TestPatroni(unittest.TestCase): self.assertRaises(Exception, self.p.get_dcs, '', {}) @patch('time.sleep', Mock(side_effect=SleepException())) - @patch.object(Patroni, 'initialize', Mock()) @patch.object(Etcd, 'delete_leader', Mock()) @patch.object(Client, 'machines') def test_patroni_main(self, mock_machines): @@ -84,13 +83,9 @@ class TestPatroni(unittest.TestCase): now = datetime.datetime.utcnow() member = Member(0, self.p.postgresql.name, 'b', 'c', (now + datetime.timedelta( seconds=self.p.shutdown_member_ttl + 10)).strftime('%Y-%m-%dT%H:%M:%S.%fZ'), None) - self.p.ha.cluster = Cluster(True, member, 0, [member]) + self.p.ha.cluster = Cluster(True, member, 0, [member], None) self.p.touch_member() - def test_patroni_initialize(self): - self.p.touch_member = self.touch_member - self.p.initialize() - def test_schedule_next_run(self): self.p.ha.dcs.watch = Mock(return_value=True) self.p.schedule_next_run() diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index d1ab7e8d..9fe012a7 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -25,18 +25,9 @@ class MockCursor: raise RetryFailedError('retry') elif sql.startswith('SELECT slot_name'): self.results = [('blabla',), ('foobar',)] - elif sql.startswith('SELECT pg_current_xlog_location()'): - self.results = [(0,)] - elif sql.startswith('SELECT pg_is_in_recovery(), %s'): - if params[0][0] == 1: - raise psycopg2.OperationalError() - elif params[0][0] == 2: - self.results = [(True, -1)] - else: - self.results = [(False, 0)] elif sql.startswith('SELECT pg_xlog_location_diff'): self.results = [(0,)] - elif sql.startswith('SELECT pg_is_in_recovery()'): + elif sql == 'SELECT pg_is_in_recovery()': self.results = [(False, )] elif sql.startswith('SELECT to_char(pg_postmaster_start_time'): self.results = [('', True, '', '', '', False)] @@ -164,7 +155,7 @@ class TestPostgresql(unittest.TestCase): def test_sync_replication_slots(self): self.p.start() - cluster = Cluster(True, self.leader, 0, [self.me, self.other, self.leadermem]) + cluster = Cluster(True, self.leader, 0, [self.me, self.other, self.leadermem], None) self.p.sync_replication_slots(cluster) @patch.object(MockConnect, 'closed', 2) @@ -178,17 +169,8 @@ class TestPostgresql(unittest.TestCase): self.assertRaises(PostgresConnectionException, self.p.query, 'RetryFailedError') self.assertRaises(psycopg2.OperationalError, self.p.query, 'blabla') - def test_is_healthiest_node(self): - cluster = Cluster(True, self.leader, 0, [self.me, self.other, self.leadermem]) - self.assertTrue(self.p.is_healthiest_node(cluster)) - self.p.is_leader = false - self.assertFalse(self.p.is_healthiest_node(cluster)) - self.p.xlog_position = lambda: 1 - self.assertTrue(self.p.is_healthiest_node(cluster)) - self.p.xlog_position = lambda: 2 - self.assertFalse(self.p.is_healthiest_node(cluster)) - self.p.config['maximum_lag_on_failover'] = -3 - self.assertFalse(self.p.is_healthiest_node(cluster)) + def test_is_leader(self): + self.assertTrue(self.p.is_leader()) def test_reload(self): self.assertTrue(self.p.reload()) @@ -218,6 +200,9 @@ class TestPostgresql(unittest.TestCase): self.p.query = Mock(side_effect=psycopg2.OperationalError("not supported")) self.assertTrue(self.p.stop()) + def test_check_replication_lag(self): + self.assertTrue(self.p.check_replication_lag(0)) + @patch('os.rename', Mock()) @patch('os.path.isdir', Mock(return_value=True)) def test_move_data_directory(self): diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py index b04f5b5a..039faa58 100644 --- a/tests/test_zookeeper.py +++ b/tests/test_zookeeper.py @@ -7,7 +7,7 @@ from patroni.zookeeper import ExhibitorEnsembleProvider, ZooKeeper, ZooKeeperErr from kazoo.client import KazooState from kazoo.exceptions import NoNodeError, NodeExistsError from kazoo.protocol.states import ZnodeStat -from test_etcd import MockPostgresql, SleepException, requests_get +from test_etcd import SleepException, requests_get class MockKazooClient(Mock): @@ -31,7 +31,7 @@ class MockKazooClient(Mock): elif '/members/' in path: return ( b'postgres://repuser:rep-pass@localhost:5434/postgres?application_name=http://127.0.0.1:8009/patroni', - ZnodeStat(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) + ZnodeStat(0, 0, 0, 0, 0, 0, 0, 0 if self.exists else -1, 0, 0, 0) ) elif path.endswith('/optime/leader'): return (b'1', ZnodeStat(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)) @@ -41,6 +41,7 @@ class MockKazooClient(Mock): return (b'foo', ZnodeStat(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)) elif path.endswith('/initialize'): return (b'foo', ZnodeStat(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)) + return (b'', ZnodeStat(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)) def get_children(self, path, watch=None, include_data=False): if not isinstance(path, six.string_types): @@ -48,7 +49,7 @@ class MockKazooClient(Mock): if path == '/no_node': raise NoNodeError elif path in ['/service/bla/', '/service/test/']: - return ['initialize', 'leader', 'members', 'optime'] + return ['initialize', 'leader', 'members', 'optime', 'failover'] return ['foo', 'bar', 'buzz'] def create(self, path, value=b"", acl=None, ephemeral=False, sequence=False, makepath=False): @@ -68,6 +69,11 @@ class MockKazooClient(Mock): raise TypeError("Invalid type for 'value' (must be a byte string)") if path == '/service/bla/optime/leader': raise Exception + if path == '/service/test/failover': + if value == b'Exception': + raise Exception + elif value == b'ok': + return raise NoNodeError def delete(self, path, version=-1, recursive=False): @@ -119,6 +125,11 @@ class TestZooKeeper(unittest.TestCase): self.zk.touch_member('foo') self.zk.delete_leader() + def test_set_failover_value(self): + self.zk.set_failover_value('') + self.zk.set_failover_value('ok') + self.zk.set_failover_value('Exception') + def test_initialize(self): self.assertFalse(self.zk.initialize()) @@ -129,6 +140,8 @@ class TestZooKeeper(unittest.TestCase): self.zk.touch_member('new') self.zk.touch_member('exists') self.zk.touch_member('retry') + self.zk.client.exists = True + self.zk.touch_member('retry') def test_take_leader(self): self.zk.take_leader() From a25976445804df4f4f16f15827ab2026f5fe54e6 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 29 Sep 2015 08:39:05 +0200 Subject: [PATCH 10/59] Suppress logging from API when postgres is being bootstrapped/initialized --- patroni/api.py | 6 ++++-- patroni/postgresql.py | 45 ++++++++++++++++++++++++++++--------------- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/patroni/api.py b/patroni/api.py index d507bfb3..580d9fe8 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -161,8 +161,10 @@ class RestApiHandler(BaseHTTPRequestHandler): }) } except (psycopg2.Error, RetryFailedError, PostgresConnectionException): - logger.exception('get_postgresql_status') - return {'state': self.server.patroni.postgresql.state} + state = self.server.patroni.postgresql.state + if state in ['stopped', 'starting', 'stopping', 'restarting', 'running']: + logger.exception('get_postgresql_status') + return {'state': state} class RestApiServer(ThreadingMixIn, HTTPServer, Thread): diff --git a/patroni/postgresql.py b/patroni/postgresql.py index bc3188ed..97d37515 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -9,6 +9,7 @@ import time from patroni.exceptions import PostgresConnectionException, PostgresException from patroni.utils import Retry, RetryFailedError from six.moves.urllib_parse import urlparse +from threading import Lock logger = logging.getLogger(__name__) @@ -70,7 +71,9 @@ class Postgresql: self.retry = Retry(max_tries=-1, deadline=10, max_delay=1, retry_exceptions=PostgresConnectionException) self._state = 'stopped' + self._state_lock = Lock() self._role = 'replica' + self._role_lock = Lock() if self.is_running(): self._state = 'running' @@ -121,12 +124,12 @@ class Postgresql: return not os.path.exists(self.data_dir) or os.listdir(self.data_dir) == [] def initialize(self): - self._state = 'initalizing new cluster' + self.set_state('initalizing new cluster') ret = subprocess.call(self._pg_ctl + ['initdb', '-o', '--encoding=UTF8']) == 0 if ret: self.write_pg_hba() else: - self._state = 'initdb failed' + self.set_state('initdb failed') return ret def delete_trigger_file(self): @@ -149,7 +152,7 @@ class Postgresql: return "host={host} port={port} user={user}".format(**conn) def create_replica(self, master_connection, env): - self._state = 'building replica from {host}:{port}'.format(**master_connection) + self.set_state('building replica from {host}:{port}'.format(**master_connection)) connstring = self.build_connstring(master_connection) cmd = self.config['restore'] try: @@ -159,7 +162,7 @@ class Postgresql: logger.exception('Error when creating replica') ret = 1 if ret != 0: - self._state = 'failed to build replica from {host}:{port}'.format(**master_connection) + self.set_state('failed to build replica from {host}:{port}'.format(**master_connection)) return ret def is_leader(self): @@ -182,28 +185,38 @@ class Postgresql: @property def role(self): - return self._role + with self._role_lock: + return self._role + + def set_role(self, value): + with self._role_lock: + self._role = value @property def state(self): - return self._state + with self._state_lock: + return self._state + + def set_state(self, value): + with self._state_lock: + self._state = value def start(self, block_callbacks=False): if self.is_running(): logger.error('Cannot start PostgreSQL because one is already running.') return True - self._role = 'replica' if os.path.exists(self.recovery_conf) else 'master' + self.set_role('replica' if os.path.exists(self.recovery_conf) else 'master') if os.path.exists(self.postmaster_pid): os.remove(self.postmaster_pid) logger.info('Removed %s', self.postmaster_pid) if not block_callbacks: - self._state = 'starting' + self.set_state('starting') ret = subprocess.call(self._pg_ctl + ['start', '-o', self.server_options()]) == 0 - self._state = 'running' if ret else 'start failed' + self.set_state('running' if ret else 'start failed') self.schedule_load_slots = ret and self.use_slots self.save_configuration_files() @@ -222,21 +235,21 @@ class Postgresql: def stop(self, mode='fast', block_callbacks=False): if not self.is_running(): if not block_callbacks: - self._state = 'stopped' + self.set_state('stopped') return True if block_callbacks: self.checkpoint() else: - self._state = 'stopping' + self.set_state('stopping') ret = subprocess.call(self._pg_ctl + ['stop', '-m', mode]) == 0 # block_callbacks is used during restart to avoid # running start/stop callbacks in addition to restart ones if not ret: - self._state = 'stop failed' + self.set_state('stop failed') elif not block_callbacks: - self._state = 'stopped' + self.set_state('stopped') self.call_nowait(ACTION_ON_STOP) return ret @@ -246,12 +259,12 @@ class Postgresql: return ret def restart(self): - self._state = 'restarting' + self.set_state('restarting') ret = self.stop(block_callbacks=True) and self.start(block_callbacks=True) if ret: self.call_nowait(ACTION_ON_RESTART) else: - self._state = 'restart failed ({})'.format(self._state) + self.set_state('restart failed ({})'.format(self.state)) return ret def server_options(self): @@ -337,7 +350,7 @@ recovery_target_timeline = 'latest' return True ret = subprocess.call(self._pg_ctl + ['promote']) == 0 if ret: - self._role = 'master' + self.set_role('master') self.call_nowait(ACTION_ON_ROLE_CHANGE) return ret From 0572fec6a3b77137e1400adede3affddfae87257 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 29 Sep 2015 12:59:26 +0200 Subject: [PATCH 11/59] remove leader lock after stop of postgres to speed up failover --- patroni/ha.py | 12 +++++++----- patroni/postgresql.py | 4 ++-- tests/test_postgresql.py | 5 ++--- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 628b9c41..a5f68068 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -214,12 +214,14 @@ class Ha: if not failover.member or failover.member != self.state_handler.name: members = [m for m in self.cluster.members if not failover.member or m.name == failover.member] if self.is_failover_possible(members): # check that there are healthy members + self.state_handler.stop() + if self.dcs.delete_leader(): + ret = 'manual failover: demoted self and released leader lock' + else: + ret = 'manual failover: demoted self but failed to release leader lock' self.state_handler.follow_the_leader(None) self.cluster = None - if self.dcs.delete_leader(): - return 'manual failover: demoted self and released leader lock' - else: - return 'manual failover: demoted self but failed to release leader lock' + return ret else: logger.warning('manual failover: no healthy members found, failover is not possible') else: @@ -360,7 +362,7 @@ class Ha: except DCSError: logger.error('Error communicating with DCS') if self.state_handler.is_running() and self.state_handler.is_leader(): - self.state_handler.demote(None) + self.state_handler.demote() return 'demoted self because DCS is not accessible and i was a leader' except (psycopg2.Error, PostgresConnectionException): logger.exception('Error communicating with Postgresql. Will try again') diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 97d37515..b2b93f72 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -354,8 +354,8 @@ recovery_target_timeline = 'latest' self.call_nowait(ACTION_ON_ROLE_CHANGE) return ret - def demote(self, leader): - self.follow_the_leader(leader) + def demote(self): + self.follow_the_leader(None) def create_replication_user(self): self.query('CREATE USER "{}" WITH REPLICATION ENCRYPTED PASSWORD %s'.format( diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 9fe012a7..f1ddea8e 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -137,9 +137,8 @@ class TestPostgresql(unittest.TestCase): self.assertTrue(self.p.sync_from_leader(self.leader)) def test_follow_the_leader(self): - self.p.demote(self.leader) - self.p.follow_the_leader(None) - self.p.demote(self.leader) + self.p.follow_the_leader(self.leader) + self.p.demote() self.p.follow_the_leader(self.leader) self.p.follow_the_leader(Leader(-1, None, 28, self.other)) From a500781b6d9dca09c66d57af11920126203d3904 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 30 Sep 2015 16:32:56 +0200 Subject: [PATCH 12/59] Mock remove_data_directory in the pg_rewind test. --- tests/test_postgresql.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 3a14fd60..8551e02e 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -3,7 +3,7 @@ import psycopg2 import shutil import unittest -from mock import Mock, patch +from mock import Mock, MagicMock, patch from patroni.dcs import Cluster, Leader, Member from patroni.exceptions import PostgresException, PostgresConnectionException from patroni.postgresql import Postgresql @@ -150,6 +150,7 @@ class TestPostgresql(unittest.TestCase): self.assertFalse(self.p.pg_rewind(self.leader)) @patch('patroni.postgresql.Postgresql.pg_rewind', return_value=False) + @patch('patroni.postgresql.Postgresql.remove_data_directory', MagicMock(return_value=True)) def test_follow_the_leader(self, mock_pg_rewind): self.p.demote(self.leader) self.p.follow_the_leader(None) From 1997f15a7a845457f0a47a14c960ae0acb420c3f Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 30 Sep 2015 17:08:15 +0200 Subject: [PATCH 13/59] Run long time operations asynchronously i.e. restart, reinitialize, demote --- patroni/__init__.py | 6 +- patroni/api.py | 24 +++--- patroni/ha.py | 157 ++++++++++++++++++++++++--------------- patroni/postgresql.py | 10 ++- patroni/zookeeper.py | 2 + tests/test_api.py | 6 +- tests/test_ha.py | 50 +++++++++---- tests/test_patroni.py | 2 + tests/test_postgresql.py | 7 ++ 9 files changed, 161 insertions(+), 103 deletions(-) diff --git a/patroni/__init__.py b/patroni/__init__.py index 6ca96df0..c5a57a67 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -56,12 +56,8 @@ class Patroni: self.next_run = time.time() while True: - logger.info(self.ha.run_cycle()) - try: - self.ha.cluster and self.ha.state_handler.sync_replication_slots(self.ha.cluster) - except: - logger.exception('Exception when changing replication slots') self.touch_member() + logger.info(self.ha.run_cycle()) reap_children() self.schedule_next_run() diff --git a/patroni/api.py b/patroni/api.py index 580d9fe8..d1cee49f 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -71,19 +71,14 @@ class RestApiHandler(BaseHTTPRequestHandler): @check_auth def do_POST_restart(self): - action = self.server.patroni.ha.schedule_restart() - if action is not None: - status_code = 503 - data = (action + ' already in progress').encode('utf-8') - else: - status_code = 503 - data = b'restart failed' - try: - if self.server.patroni.ha.restart(): - status_code = 200 - data = b'restarted successfully' - except: - logger.exception('Exception during restart') + status_code = 503 + data = b'restart failed' + try: + status, msg = self.server.patroni.ha.restart() + status_code = 200 if status else 503 + data = msg.encode('utf-8') + except: + logger.exception('Exception during restart') self.send_response(status_code) self.send_header('Content-Type', 'text/html') @@ -135,7 +130,7 @@ class RestApiHandler(BaseHTTPRequestHandler): def query(self, sql, *params, **kwargs): if not kwargs.get('retry', False): return self.server.query(sql, *params) - retry = Retry(delay=2, retry_exceptions=PostgresConnectionException) + retry = Retry(delay=1, retry_exceptions=PostgresConnectionException) return retry(self.server.query, sql, *params) def get_postgresql_status(self, retry=False): @@ -164,6 +159,7 @@ class RestApiHandler(BaseHTTPRequestHandler): state = self.server.patroni.postgresql.state if state in ['stopped', 'starting', 'stopping', 'restarting', 'running']: logger.exception('get_postgresql_status') + state = 'unknown' if state == 'running' else state return {'state': state} diff --git a/patroni/ha.py b/patroni/ha.py index a5f68068..d791ea87 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -4,7 +4,7 @@ import requests from patroni.exceptions import DCSError, PostgresConnectionException from multiprocessing.pool import ThreadPool -from threading import Lock +from threading import Lock, Thread logger = logging.getLogger(__name__) @@ -16,10 +16,10 @@ class Ha: self.dcs = dcs self.cluster = None self.old_cluster = None - self.scheduled_action = None - self.scheduled_action_lock = Lock() - self.restart_in_progress = False - self.restart_thread_lock = Lock() + self._scheduled_action = None + self._scheduled_action_lock = Lock() + self._long_action_in_progress = False + self._long_action_thread_lock = Lock() def load_cluster_from_dcs(self): cluster = self.dcs.get_cluster() @@ -48,16 +48,37 @@ class Ha: logger.info('Lock owner: %s; I am %s', lock_owner, self.state_handler.name) return lock_owner == self.state_handler.name + def _run_async(self, func, args=()): + try: + return func(*args) if args else func() + except: + logger.exception('Exception during execution of long running task %s', self.get_scheduled_action()) + finally: + with self._long_action_thread_lock: + self._long_action_in_progress = False + self._reset_scheduled_action() + + def run_async(self, func, args=()): + self._long_action_in_progress = True + Thread(target=self._run_async, args=(func, args)).start() + + def copy_backup_from_leader(self): + if self.state_handler.bootstrap(self.cluster.leader): + logger.info('bootstrapped from leader') + else: + self.state_handler.stop('immediate') + self.state_handler.remove_data_directory() + logger.error('failed to bootstrap from leader') + def bootstrap(self): if not self.cluster.is_unlocked(): # cluster already has leader - logger.info('trying to bootstrap from leader') - if self.state_handler.bootstrap(self.cluster.leader): - self.reinitialize_scheduled() and self.reset_scheduled_action() - return 'bootstrapped from leader' + if self._long_action_in_progress: + self.copy_backup_from_leader() else: - self.state_handler.stop('immediate') - self.state_handler.remove_data_directory() - return 'failed to bootstrap from leader' + with self._scheduled_action_lock: + self._scheduled_action = 'bootstrap from leader' + self.run_async(self.copy_backup_from_leader) + return 'trying to bootstrap from leader' elif not self.cluster.initialize: # no initialize key if self.dcs.initialize(): # race for initialization try: @@ -92,7 +113,10 @@ class Ha: def follow_the_leader(self, demote_reason, follow_reason, refresh=True): refresh and self.load_cluster_from_dcs() ret = demote_reason if self.state_handler.is_leader() else follow_reason - self.state_handler.follow_the_leader(self.cluster.leader) + if not self.state_handler.check_recovery_conf(self.cluster.leader): + with self._scheduled_action_lock: + self._scheduled_action = 'changing primary_conninfo and restarting' + self.run_async(self.state_handler.follow_the_leader, (self.cluster.leader, )) return ret def enforce_master_role(self, message, promote_message): @@ -208,20 +232,22 @@ class Ha: members = {m.name: m for m in self.old_cluster.members + self.cluster.members} return self._is_healthiest_node(members.values()) + def demote(self, delete_leader=True): + if delete_leader: + self.state_handler.stop() + self.dcs.delete_leader() + self.state_handler.follow_the_leader(None) + def process_manual_failover_from_leader(self): failover = self.cluster.failover if not failover.leader or failover.leader == self.state_handler.name: if not failover.member or failover.member != self.state_handler.name: members = [m for m in self.cluster.members if not failover.member or m.name == failover.member] if self.is_failover_possible(members): # check that there are healthy members - self.state_handler.stop() - if self.dcs.delete_leader(): - ret = 'manual failover: demoted self and released leader lock' - else: - ret = 'manual failover: demoted self but failed to release leader lock' - self.state_handler.follow_the_leader(None) - self.cluster = None - return ret + with self._scheduled_action_lock: + self._scheduled_action = 'manual failover: demote' + self.run_async(self.demote) + return 'manual failover: demoting myself' else: logger.warning('manual failover: no healthy members found, failover is not possible') else: @@ -268,22 +294,20 @@ class Ha: 'no action. i am a secondary and i am following a leader', False) def schedule_action(self, action): - with self.scheduled_action_lock: - if self.scheduled_action is not None: - return self.scheduled_action - self.scheduled_action = action + with self._long_action_thread_lock: + with self._scheduled_action_lock: + if self._scheduled_action is not None: + return self._scheduled_action + self._scheduled_action = action return None def get_scheduled_action(self): - with self.scheduled_action_lock: - return self.scheduled_action + with self._scheduled_action_lock: + return self._scheduled_action - def reset_scheduled_action(self): - with self.scheduled_action_lock: - self.scheduled_action = None - - def schedule_restart(self): - return self.schedule_action('restart') + def _reset_scheduled_action(self): + with self._scheduled_action_lock: + self._scheduled_action = None def restart_scheduled(self): return self.get_scheduled_action() == 'restart' @@ -295,38 +319,45 @@ class Ha: return self.get_scheduled_action() == 'reinitialize' def restart(self): - with self.restart_thread_lock: - self.restart_in_progress = True - try: - return self.state_handler.restart() - finally: - with self.restart_thread_lock: - self.restart_in_progress = False - self.reset_scheduled_action() + with self._long_action_thread_lock: + with self._scheduled_action_lock: + if self._scheduled_action is not None: + return False, self._scheduled_action + ' already in progress' + self._scheduled_action = 'restart' + self._long_action_in_progress = True + if self._run_async(self.state_handler.restart): + return True, 'restarted successfully' + else: + return False, 'restart failed' + + def reinitialize(self): + self.state_handler.stop('immediate') + self.state_handler.remove_data_directory() + self.load_cluster_from_dcs() + self.bootstrap() def process_scheduled_action(self): if self.reinitialize_scheduled(): if self.cluster.is_unlocked(): logger.error('Cluster has no leader, can not reinitialize') - self.reset_scheduled_action() + self._reset_scheduled_action() elif self.has_lock(): logger.error('I am the leader, can not reinitialize') - self.reset_scheduled_action() + self._reset_scheduled_action() else: - self.state_handler.stop('immediate') - self.state_handler.remove_data_directory() - self.load_cluster_from_dcs() + self.run_async(self.reinitialize) + return True - def handle_restart_in_progress(self): + def handle_long_action_in_progress(self): if self.has_lock(): if self.update_lock(): - return 'updated leader lock during restart' + return 'updated leader lock during ' + self.get_scheduled_action() else: - return 'failed to update leader lock during restart' + return 'failed to update leader lock during ' + self.get_scheduled_action() elif self.cluster.is_unlocked(): return 'not healthy enough for leader race' else: - return 'restart in progress' + return self.get_scheduled_action() + ' in progress' def _run_cycle(self): try: @@ -336,8 +367,12 @@ class Ha: if not self.cluster.is_unlocked() and not self.cluster.initialize: self.dcs.initialize() # fix it + if self._long_action_in_progress: + return self.handle_long_action_in_progress() + # currently it can trigger only reinitialize - self.process_scheduled_action() + if self.process_scheduled_action(): + return 'reinitialize started' # is data directory empty? if self.state_handler.data_directory_empty(): @@ -346,27 +381,27 @@ class Ha: elif not self.cluster.initialize and self.cluster.is_unlocked(): self.dcs.initialize() - if self.restart_in_progress: - return self.handle_restart_in_progress() - # try to start dead postgres if not self.state_handler.is_healthy(): msg = self.recover() if msg is not None: return msg - if self.cluster.is_unlocked(): - return self.process_unhealthy_cluster() - else: - return self.process_healthy_cluster() + try: + if self.cluster.is_unlocked(): + return self.process_unhealthy_cluster() + else: + return self.process_healthy_cluster() + finally: + self.state_handler.sync_replication_slots(self.cluster) except DCSError: logger.error('Error communicating with DCS') if self.state_handler.is_running() and self.state_handler.is_leader(): - self.state_handler.demote() + self.demote(delete_leader=False) return 'demoted self because DCS is not accessible and i was a leader' except (psycopg2.Error, PostgresConnectionException): - logger.exception('Error communicating with Postgresql. Will try again') + logger.exception('Error communicating with Postgresql. Will try again later') def run_cycle(self): - with self.restart_thread_lock: + with self._long_action_thread_lock: return self._run_cycle() diff --git a/patroni/postgresql.py b/patroni/postgresql.py index b2b93f72..816474a3 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -227,10 +227,14 @@ class Postgresql: def checkpoint(self): try: - self.query('SET statement_timeout TO 0') - self.query('CHECKPOINT') + r = parseurl('postgres://{}/postgres'.format(self.local_address)) + r['options'] = '-c statement_timeout=0' + with psycopg2.connect(**r) as conn: + conn.autocommit = True + with conn.cursor() as cur: + cur.execute('CHECKPOINT') except: - logging.exception('Exception diring CHECKPOINT') + logging.exception('Exception during CHECKPOINT') def stop(self, mode='fast', block_callbacks=False): if not self.is_running(): diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index 7458ae46..4b3389e1 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -225,6 +225,8 @@ class ZooKeeper(AbstractDCS): return True except: logger.exception('touch_member') + except: + logger.exception('touch_member') return False def take_leader(self): diff --git a/tests/test_api.py b/tests/test_api.py index 4bb481bf..5003e43e 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -33,7 +33,7 @@ class MockHa(Mock): return 'reinitialize' def restart(self): - return True + return (True, '') def restart_scheduled(self): return False @@ -85,10 +85,8 @@ class TestRestApiHandler(unittest.TestCase): def test_do_POST_restart(self): request = b'POST /restart HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' MockRestApiServer(RestApiHandler, request) - with patch.object(MockHa, 'schedule_restart', Mock(return_value=None)): + with patch.object(MockHa, 'restart', Mock(side_effect=Exception)): MockRestApiServer(RestApiHandler, request) - with patch.object(MockHa, 'restart', Mock(side_effect=Exception)): - MockRestApiServer(RestApiHandler, request) @patch.object(MockHa, 'dcs') def test_do_POST_reinitialize(self, dcs): diff --git a/tests/test_ha.py b/tests/test_ha.py index 6e50c629..52faba35 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -6,6 +6,7 @@ from patroni.etcd import Client, Etcd from patroni.exceptions import DCSError, PostgresException from patroni.ha import Ha from test_etcd import socket_getaddrinfo, etcd_read, etcd_write, requests_get +from threading import Thread def true(*args, **kwargs): @@ -69,6 +70,13 @@ class MockPostgresql(Mock): def check_replication_lag(self, last_leader_operation): return True + def check_recovery_conf(self, leader): + return False + + +def run_async(func, args=()): + func(args) if args else func() + class TestHa(unittest.TestCase): @@ -81,6 +89,7 @@ class TestHa(unittest.TestCase): self.e.client.read = etcd_read self.e.client.write = etcd_write self.ha = Ha(self.p, self.e) + self.ha.run_async = run_async self.ha.load_cluster_from_dcs() self.ha.cluster = get_cluster_not_initialized_without_leader() self.ha.load_cluster_from_dcs = Mock() @@ -173,14 +182,20 @@ class TestHa(unittest.TestCase): self.ha.load_cluster_from_dcs = Mock(side_effect=DCSError('Etcd is not responding properly')) self.assertEquals(self.ha.run_cycle(), 'demoted self because DCS is not accessible and i was a leader') + def test__run_async(self): + self.ha._run_async(Mock(side_effect=Exception())) + + @patch.object(Thread, 'start', Mock()) + def test_run_async(self): + ha = Ha(self.p, self.e) + ha.run_async(true) + def test_bootstrap_from_leader(self): - self.ha.cluster = get_cluster_initialized_with_leader() - self.assertEquals(self.ha.bootstrap(), 'bootstrapped from leader') - - def test_bootstrap_from_leader_failed(self): self.ha.cluster = get_cluster_initialized_with_leader() self.p.bootstrap = false - self.assertEquals(self.ha.bootstrap(), 'failed to bootstrap from leader') + self.assertEquals(self.ha.bootstrap(), 'trying to bootstrap from leader') + self.ha._long_action_in_progress = True + self.assertEquals(self.ha.bootstrap(), 'trying to bootstrap from leader') def test_bootstrap_waiting_for_leader(self): self.ha.cluster = get_cluster_initialized_without_leader() @@ -202,26 +217,32 @@ class TestHa(unittest.TestCase): self.assertRaises(PostgresException, self.ha.bootstrap) def test_reinitialize(self): + self.ha.schedule_reinitialize() self.ha.schedule_reinitialize() self.ha.run_cycle() self.assertIsNone(self.ha.get_scheduled_action()) self.ha.cluster = get_cluster_initialized_with_leader() - self.ha.schedule_reinitialize() - self.ha.run_cycle() - self.ha.has_lock = true self.ha.schedule_reinitialize() self.ha.run_cycle() self.assertIsNone(self.ha.get_scheduled_action()) + self.ha.has_lock = false + self.ha.schedule_reinitialize() + self.ha.run_cycle() + def test_restart(self): - self.ha.schedule_restart() - self.assertTrue(self.ha.restart_scheduled()) - self.ha.restart() + self.assertEquals(self.ha.restart(), (True, 'restarted successfully')) + self.p.restart = false + self.assertEquals(self.ha.restart(), (False, 'restart failed')) + self.ha.schedule_reinitialize() + self.assertEquals(self.ha.restart(), (False, 'reinitialize already in progress')) def test_restart_in_progress(self): - self.ha.restart_in_progress = True + self.ha._long_action_in_progress = True + self.ha._scheduled_action = 'restart' + self.assertTrue(self.ha.restart_scheduled()) self.assertEquals(self.ha.run_cycle(), 'not healthy enough for leader race') self.ha.cluster = get_cluster_initialized_with_leader() @@ -244,10 +265,7 @@ class TestHa(unittest.TestCase): self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') f = Failover(0, MockPostgresql.name, '') self.ha.cluster = get_cluster_initialized_with_leader(f) - self.assertEquals(self.ha.run_cycle(), 'manual failover: demoted self but failed to release leader lock') - self.ha.cluster = get_cluster_initialized_with_leader(f) - self.e.client.delete = Mock(return_value=True) - self.assertEquals(self.ha.run_cycle(), 'manual failover: demoted self and released leader lock') + self.assertEquals(self.ha.run_cycle(), 'manual failover: demoting myself') @patch('requests.get', requests_get) def test_manual_failover_process_no_leader(self): diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 8936d18b..2c7998c4 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -8,6 +8,7 @@ from mock import Mock, patch from patroni.api import RestApiServer from patroni.dcs import Cluster, Member from patroni.etcd import Etcd +from patroni.ha import Ha from patroni import Patroni, main from patroni.zookeeper import ZooKeeper from six.moves import BaseHTTPServer @@ -26,6 +27,7 @@ def time_sleep(*args): @patch.object(Postgresql, 'write_pg_hba', Mock()) @patch.object(Postgresql, 'write_recovery_conf', Mock()) @patch.object(BaseHTTPServer.HTTPServer, '__init__', Mock()) +@patch.object(Ha, 'run_async', Mock()) class TestPatroni(unittest.TestCase): @patch.object(Client, 'machines') diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index f1ddea8e..e7e39594 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -70,6 +70,12 @@ class MockConnect(Mock): def cursor(self): return MockCursor(self) + def __enter__(self): + return self + + def __exit__(self, *args): + pass + def psycopg2_connect(*args, **kwargs): return MockConnect() @@ -214,6 +220,7 @@ class TestPostgresql(unittest.TestCase): with patch('subprocess.call', Mock(return_value=1)): self.assertRaises(PostgresException, self.p.bootstrap) self.p.bootstrap() + self.p.bootstrap(self.leader) def test_remove_data_directory(self): self.p.data_dir = 'data_dir' From b223319183d9a6813a6b0e37e7edd759daed94e0 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 30 Sep 2015 18:00:28 +0200 Subject: [PATCH 14/59] use the PATH to get the python interpreter path for the scripts. --- patroni/scripts/aws.py | 2 +- patroni/scripts/restore.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/patroni/scripts/aws.py b/patroni/scripts/aws.py index b172a8c3..a0622f15 100755 --- a/patroni/scripts/aws.py +++ b/patroni/scripts/aws.py @@ -1,4 +1,4 @@ -#!/usr/bin/python +#!/usr/bin/env python import logging import requests diff --git a/patroni/scripts/restore.py b/patroni/scripts/restore.py index 4ac091f3..6b20e3e8 100755 --- a/patroni/scripts/restore.py +++ b/patroni/scripts/restore.py @@ -1,4 +1,4 @@ -#!/usr/bin/python +#!/usr/bin/env python # arguments are: # - cluster scope # - cluster role From ea910a89878910939dd4271d19fd87dc00dbbdc6 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 30 Sep 2015 18:02:07 +0200 Subject: [PATCH 15/59] Make sure pgpass file name is also passed in the PGPASSFILE environment variable. --- patroni/postgresql.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 7e3e9088..4c0da705 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -143,14 +143,14 @@ class Postgresql: with open(pgpass, 'w' if not append else 'a') as f: os.fchmod(f.fileno(), 0o600) f.write('{host}:{port}:*:{user}:{password}\n'.format(**record)) - return pgpass + env = os.environ.copy() + env['PGPASSFILE'] = pgpass + return env def sync_from_leader(self, leader): r = parseurl(leader.conn_url) - pgpass = self.write_pgpass(r) - env = os.environ.copy() - env['PGPASSFILE'] = pgpass + env = self.write_pgpass(r) return self.create_replica(r, env) == 0 @staticmethod @@ -320,15 +320,15 @@ recovery_target_timeline = 'latest' def prepare_pg_rewind_connection(self, leader_url, pg_rewind): r = parseurl(leader_url) r.update(pg_rewind) - self.write_pgpass(r, append=True) - return "user={user} host={host} port={port} dbname=postgres sslmode=prefer sslcompression=1".format(**r) + env = self.write_pgpass(r, append=True) + return (env, "user={user} host={host} port={port} dbname=postgres sslmode=prefer sslcompression=1".format(**r)) def pg_rewind(self, leader): - pc = self.prepare_pg_rewind_connection(leader.conn_url, self._pg_rewind) + env, pc = self.prepare_pg_rewind_connection(leader.conn_url, self._pg_rewind) logger.info("running pg_rewind from {}".format(pc)) pg_rewind = ['pg_rewind', '-D', self.data_dir, '--source-server', pc] try: - ret = (subprocess.call(pg_rewind) == 0) + ret = (subprocess.call(pg_rewind, env=env) == 0) except: ret = False if ret: From a6cb7563e57178b76499ffd95ddb335181f396a0 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 1 Oct 2015 08:06:00 +0200 Subject: [PATCH 16/59] catch all exceptions in change_replication_slots method --- patroni/postgresql.py | 29 ++++++++++++++++------------- tests/test_patroni.py | 1 - tests/test_postgresql.py | 3 +++ 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 816474a3..e15a4996 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -391,21 +391,24 @@ recovery_target_timeline = 'latest' def sync_replication_slots(self, cluster): if self.use_slots: - self.load_replication_slots() - slots = [m.name for m in cluster.members if m.name != self.name] if self.role == 'master' else [] - # drop unused slots - for slot in set(self.replication_slots) - set(slots): - self.query("""SELECT pg_drop_replication_slot(%s) - WHERE EXISTS(SELECT 1 FROM pg_replication_slots - WHERE slot_name = %s)""", slot, slot) + try: + self.load_replication_slots() + slots = [m.name for m in cluster.members if m.name != self.name] if self.role == 'master' else [] + # drop unused slots + for slot in set(self.replication_slots) - set(slots): + self.query("""SELECT pg_drop_replication_slot(%s) + WHERE EXISTS(SELECT 1 FROM pg_replication_slots + WHERE slot_name = %s)""", slot, slot) - # create new slots - for slot in set(slots) - set(self.replication_slots): - self.query("""SELECT pg_create_physical_replication_slot(%s) - WHERE NOT EXISTS (SELECT 1 FROM pg_replication_slots - WHERE slot_name = %s)""", slot, slot) + # create new slots + for slot in set(slots) - set(self.replication_slots): + self.query("""SELECT pg_create_physical_replication_slot(%s) + WHERE NOT EXISTS (SELECT 1 FROM pg_replication_slots + WHERE slot_name = %s)""", slot, slot) - self.replication_slots = slots + self.replication_slots = slots + except: + logger.exception('Exception when changing replication slots') def last_operation(self): return str(self.xlog_position()) diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 2c7998c4..1c82799b 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -66,7 +66,6 @@ class TestPatroni(unittest.TestCase): @patch('time.sleep', Mock(side_effect=SleepException())) def test_run(self): self.p.touch_member = self.touch_member - self.p.ha.state_handler.sync_replication_slots = time_sleep self.p.ha.dcs.watch = time_sleep self.assertRaises(SleepException, self.p.run) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index e7e39594..90874288 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -162,6 +162,9 @@ class TestPostgresql(unittest.TestCase): self.p.start() cluster = Cluster(True, self.leader, 0, [self.me, self.other, self.leadermem], None) self.p.sync_replication_slots(cluster) + self.p.query = Mock(side_effect=psycopg2.OperationalError) + self.p.schedule_load_slots = True + self.p.sync_replication_slots(cluster) @patch.object(MockConnect, 'closed', 2) def test__query(self): From d09875a056bbbe4977150f84391f8c378978e8b2 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 1 Oct 2015 17:06:42 +0200 Subject: [PATCH 17/59] refactoring: 1. run touch_member from the main loop 2. move code which takes care about long tasks into separate class 3. change format of data stored in a DCS: use json instead of url 4. change Member class: from now it deserialize everything into data property 5. rework API: from now it takes into account state of the current node in a dcs --- patroni/__init__.py | 19 +--- patroni/api.py | 13 ++- patroni/async_executor.py | 55 ++++++++++++ patroni/dcs.py | 44 ++++++--- patroni/etcd.py | 10 +-- patroni/ha.py | 183 +++++++++++++++++--------------------- patroni/postgresql.py | 4 +- patroni/utils.py | 2 + patroni/zookeeper.py | 45 ++++++---- tests/test_api.py | 1 + tests/test_etcd.py | 9 -- tests/test_ha.py | 99 ++++++++++----------- tests/test_patroni.py | 27 ++---- tests/test_postgresql.py | 10 +-- tests/test_zookeeper.py | 17 +++- 15 files changed, 298 insertions(+), 240 deletions(-) create mode 100644 patroni/async_executor.py diff --git a/patroni/__init__.py b/patroni/__init__.py index c5a57a67..150334ba 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -19,11 +19,11 @@ class Patroni: def __init__(self, config): self.nap_time = config['loop_wait'] self.postgresql = Postgresql(config['postgresql']) - self.ha = Ha(self.postgresql, self.get_dcs(self.postgresql.name, config)) + self.dcs = self.get_dcs(self.postgresql.name, config) host, port = config['restapi']['listen'].split(':') self.api = RestApiServer(self, config['restapi']) + self.ha = Ha(self) self.next_run = time.time() - self.shutdown_member_ttl = 300 @staticmethod def get_dcs(name, config): @@ -33,22 +33,13 @@ class Patroni: return ZooKeeper(name, config['zookeeper']) raise Exception('Can not find sutable configuration of distributed configuration store') - def touch_member(self, ttl=None): - connection_string = self.postgresql.connection_string + '?application_name=' + self.api.connection_string - if self.ha.cluster: - for m in self.ha.cluster.members: - # Do not update member TTL when it is far from being expired - if m.name == self.postgresql.name and m.real_ttl() > self.shutdown_member_ttl: - return True - return self.ha.dcs.touch_member(connection_string, ttl) - def schedule_next_run(self): self.next_run += self.nap_time current_time = time.time() nap_time = self.next_run - current_time if nap_time <= 0: self.next_run = current_time - elif self.ha.dcs.watch(nap_time): + elif self.dcs.watch(nap_time): self.next_run = time.time() def run(self): @@ -56,7 +47,6 @@ class Patroni: self.next_run = time.time() while True: - self.touch_member() logger.info(self.ha.run_cycle()) reap_children() self.schedule_next_run() @@ -81,6 +71,5 @@ def main(): pass finally: patroni.api.shutdown() - patroni.touch_member(patroni.shutdown_member_ttl) # schedule member removal patroni.postgresql.stop() - patroni.ha.dcs.delete_leader() + patroni.dcs.delete_leader() diff --git a/patroni/api.py b/patroni/api.py index d1cee49f..0333e86c 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -48,7 +48,18 @@ class RestApiHandler(BaseHTTPRequestHandler): response = self.get_postgresql_status() patroni = self.server.patroni - if 'role' in response and response['role'] in path: + if patroni.dcs.cluster: # dcs available + if patroni.dcs.cluster.leader and patroni.dcs.cluster.leader.name == patroni.postgresql.name: # is_leader + status_code = 200 if 'master' in path else 503 + elif 'role' not in response: + status_code = 503 + elif response['role'] == 'master': # running as master but without leader lock!!!! + status_code = 503 + elif response['role'] in path: + status_code = 200 + else: + status_code = 503 + elif 'role' in response and response['role'] in path: status_code = 200 elif patroni.ha.restart_scheduled() and patroni.postgresql.role == 'master' and 'master' in path: # exceptional case for master node when the postgres is being restarted via API diff --git a/patroni/async_executor.py b/patroni/async_executor.py new file mode 100644 index 00000000..fc222202 --- /dev/null +++ b/patroni/async_executor.py @@ -0,0 +1,55 @@ +import logging +from threading import Lock, Thread + +logger = logging.getLogger(__name__) + + +class AsyncExecutor: + + def __init__(self): + Lock.__init__(self) + self._busy = False + self._thread_lock = Lock() + self._scheduled_action = None + self._scheduled_action_lock = Lock() + + @property + def busy(self): + return self._busy + + def schedule(self, action, immediately=False): + with self._scheduled_action_lock: + if self._scheduled_action is not None: + return self._scheduled_action + self._scheduled_action = action + self._busy = immediately + return None + + @property + def scheduled_action(self): + with self._scheduled_action_lock: + return self._scheduled_action + + def reset_scheduled_action(self): + with self._scheduled_action_lock: + self._scheduled_action = None + + def run(self, func, args=()): + try: + return func(*args) if args else func() + except: + logger.exception('Exception during execution of long running task %s', self.scheduled_action) + finally: + with self: + self._busy = False + self.reset_scheduled_action() + + def run_async(self, func, args=()): + self._busy = True + Thread(target=self.run, args=(func, args)).start() + + def __enter__(self): + self._thread_lock.acquire() + + def __exit__(self, type, value, traceback): + self._thread_lock.release() diff --git a/patroni/dcs.py b/patroni/dcs.py index fb034322..1bca092e 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -1,8 +1,9 @@ import abc +import json from collections import namedtuple from patroni.exceptions import DCSError -from patroni.utils import calculate_ttl, sleep +from patroni.utils import sleep from six.moves.urllib_parse import urlparse, urlunparse, parse_qsl @@ -23,28 +24,47 @@ def parse_connection_string(value): return conn_url, api_url -class Member(namedtuple('Member', 'index,name,conn_url,api_url,expiration,ttl')): +class Member(namedtuple('Member', 'index,name,session,data')): """Immutable object (namedtuple) which represents single member of PostgreSQL cluster. Consists of the following fields: :param index: modification index of a given member key in a Configuration Store :param name: name of PostgreSQL cluster member - :param conn_url: connection string containing host, user and password which could be used to access this member. - :param api_url: REST API url of patroni instance - :param expiration: expiration time of given member key - :param ttl: ttl of given member key in seconds""" + :param session: either session id or just ttl in seconds + :param data: arbitrary data i.e. conn_url, api_url, xlog location, state, role, tags, etc... - def real_ttl(self): - return calculate_ttl(self.expiration) or -1 + There are two mandatory keys in a data: + conn_url: connection string containing host, user and password which could be used to access this member. + api_url: REST API url of patroni instance""" + + @staticmethod + def from_node(index, name, session, data): + """ + >>> Member.from_node(-1, '', '', '{"conn_url": "postgres://foo@bar/postgres"}') is not None + True + """ + if data.startswith('postgres'): + conn_url, api_url = parse_connection_string(data) + data = {'conn_url': conn_url, 'api_url': api_url} + else: + data = json.loads(data) + return Member(index, name, session, data) + + @property + def conn_url(self): + return self.data.get('conn_url', None) + + @property + def api_url(self): + return self.data.get('api_url', None) -class Leader(namedtuple('Leader', 'index,expiration,ttl,member')): +class Leader(namedtuple('Leader', 'index,session,member')): """Immutable object (namedtuple) which represents leader key. Consists of the following fields: :param index: modification index of a leader key in a Configuration Store - :param expiration: expiration time of the leader key - :param ttl: ttl of the leader key + :param session: either session id or just ttl in seconds :param member: reference to a `Member` object which represents current leader (see `Cluster.members`)""" @property @@ -100,6 +120,8 @@ class AbstractDCS: self._scope = config['scope'] self._base_path = '/service/' + self._scope + self.cluster = None + def client_path(self, path): return '/'.join([self._base_path, path.lstrip('/')]) diff --git a/patroni/etcd.py b/patroni/etcd.py index c2995da9..f1ca5044 100644 --- a/patroni/etcd.py +++ b/patroni/etcd.py @@ -10,7 +10,7 @@ import urllib3 from dns.exception import DNSException from dns import resolver -from patroni.dcs import AbstractDCS, Cluster, Failover, Leader, Member, parse_connection_string +from patroni.dcs import AbstractDCS, Cluster, Failover, Leader, Member from patroni.exceptions import DCSError from patroni.utils import Retry, RetryFailedError, sleep from requests.exceptions import RequestException @@ -154,7 +154,6 @@ class Etcd(AbstractDCS): etcd.EtcdWatcherCleared, etcd.EtcdEventIndexCleared)) self.client = self.get_etcd_client(config) - self.cluster = None def retry(self, *args, **kwargs): return self._retry.copy()(*args, **kwargs) @@ -171,8 +170,7 @@ class Etcd(AbstractDCS): @staticmethod def member(node): - conn_url, api_url = parse_connection_string(node.value) - return Member(node.modifiedIndex, os.path.basename(node.key), conn_url, api_url, node.expiration, node.ttl) + return Member.from_node(node.modifiedIndex, os.path.basename(node.key), node.ttl, node.value) def get_cluster(self): try: @@ -192,9 +190,9 @@ class Etcd(AbstractDCS): # get leader leader = nodes.get(self._LEADER, None) if leader: - member = Member(-1, leader.value, None, None, None, None) + member = Member(-1, leader.value, None, {}) member = ([m for m in members if m.name == leader.value] or [member])[0] - leader = Leader(leader.modifiedIndex, leader.expiration, leader.ttl, member) + leader = Leader(leader.modifiedIndex, leader.ttl, member) # failover key failover = nodes.get(self._FAILOVER, None) diff --git a/patroni/ha.py b/patroni/ha.py index d791ea87..25a05c23 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -1,35 +1,28 @@ +import json import logging import psycopg2 import requests +from patroni.async_executor import AsyncExecutor from patroni.exceptions import DCSError, PostgresConnectionException from multiprocessing.pool import ThreadPool -from threading import Lock, Thread logger = logging.getLogger(__name__) class Ha: - def __init__(self, state_handler, dcs): - self.state_handler = state_handler - self.dcs = dcs - self.cluster = None + def __init__(self, patroni): + self.patroni = patroni + self.state_handler = patroni.postgresql + self.dcs = patroni.dcs self.old_cluster = None - self._scheduled_action = None - self._scheduled_action_lock = Lock() - self._long_action_in_progress = False - self._long_action_thread_lock = Lock() + self._async_executor = AsyncExecutor() def load_cluster_from_dcs(self): - cluster = self.dcs.get_cluster() - # We want to keep the state of cluster when it was healhy - if cluster.is_unlocked() and self.cluster and not self.cluster.is_unlocked(): - self.old_cluster = self.cluster - if not self.old_cluster: - self.old_cluster = cluster - self.cluster = cluster + if not self.dcs.get_cluster().is_unlocked() or not self.old_cluster: + self.old_cluster = self.dcs.cluster def acquire_lock(self): return self.dcs.attempt_to_acquire_leader() @@ -44,26 +37,26 @@ class Ha: return ret def has_lock(self): - lock_owner = self.cluster.leader and self.cluster.leader.name + lock_owner = self.dcs.cluster.leader and self.dcs.cluster.leader.name logger.info('Lock owner: %s; I am %s', lock_owner, self.state_handler.name) return lock_owner == self.state_handler.name - def _run_async(self, func, args=()): - try: - return func(*args) if args else func() - except: - logger.exception('Exception during execution of long running task %s', self.get_scheduled_action()) - finally: - with self._long_action_thread_lock: - self._long_action_in_progress = False - self._reset_scheduled_action() - - def run_async(self, func, args=()): - self._long_action_in_progress = True - Thread(target=self._run_async, args=(func, args)).start() + def touch_member(self): + data = { + 'conn_url': self.state_handler.connection_string, + 'api_url': self.patroni.api.connection_string, + 'state': self.state_handler.state, + 'role': self.state_handler.role + } + if data['state'] in ['running', 'restarting', 'starting']: + try: + data['xlog_location'] = self.state_handler.xlog_position() + except: + pass + self.dcs.touch_member(json.dumps(data, separators=(',', ':'))) def copy_backup_from_leader(self): - if self.state_handler.bootstrap(self.cluster.leader): + if self.state_handler.bootstrap(self.dcs.cluster.leader): logger.info('bootstrapped from leader') else: self.state_handler.stop('immediate') @@ -71,15 +64,14 @@ class Ha: logger.error('failed to bootstrap from leader') def bootstrap(self): - if not self.cluster.is_unlocked(): # cluster already has leader - if self._long_action_in_progress: + if not self.dcs.cluster.is_unlocked(): # cluster already has leader + if self._async_executor.busy: self.copy_backup_from_leader() else: - with self._scheduled_action_lock: - self._scheduled_action = 'bootstrap from leader' - self.run_async(self.copy_backup_from_leader) + self._async_executor.schedule('bootstrap from leader') + self._async_executor.run_async(self.copy_backup_from_leader) return 'trying to bootstrap from leader' - elif not self.cluster.initialize: # no initialize key + elif not self.dcs.cluster.initialize: # no initialize key if self.dcs.initialize(): # race for initialization try: self.state_handler.bootstrap() @@ -99,7 +91,7 @@ class Ha: def recover(self): has_lock = self.has_lock() - self.state_handler.write_recovery_conf(None if has_lock else self.cluster.leader) + self.state_handler.write_recovery_conf(None if has_lock else self.dcs.cluster.leader) if not self.state_handler.start(): if not has_lock: return 'failed to start postgres' @@ -113,10 +105,9 @@ class Ha: def follow_the_leader(self, demote_reason, follow_reason, refresh=True): refresh and self.load_cluster_from_dcs() ret = demote_reason if self.state_handler.is_leader() else follow_reason - if not self.state_handler.check_recovery_conf(self.cluster.leader): - with self._scheduled_action_lock: - self._scheduled_action = 'changing primary_conninfo and restarting' - self.run_async(self.state_handler.follow_the_leader, (self.cluster.leader, )) + if not self.state_handler.check_recovery_conf(self.dcs.cluster.leader): + self._async_executor.schedule('changing primary_conninfo and restarting') + self._async_executor.run_async(self.state_handler.follow_the_leader, (self.dcs.cluster.leader, )) return ret def enforce_master_role(self, message, promote_message): @@ -159,7 +150,7 @@ class Ha: if self.state_handler.is_leader(): return True - if check_replication_lag and not self.state_handler.check_replication_lag(self.cluster.last_leader_operation): + if check_replication_lag and not self.state_handler.check_replication_lag(self.dcs.cluster.last_leader_operation): return False # Too far behind last reported xlog location on master # Prepare list of nodes to run check against @@ -190,13 +181,13 @@ class Ha: return ret def manual_failover_process_no_leader(self): - failover = self.cluster.failover + failover = self.dcs.cluster.failover if failover.member: # manual failover to specific member if failover.member == self.state_handler.name: # manual failover to me return True # find specific node and check that it is healthy - members = [m for m in self.cluster.members if m.name == failover.member] + members = [m for m in self.dcs.cluster.members if m.name == failover.member] if members: member, reachable, in_recovery, xlog_location = self.fetch_node_status(members[0]) if reachable: # node is healthy @@ -212,7 +203,7 @@ class Ha: if failover.leader: if self.state_handler.name == failover.leader: # I was the leader # exclude me and desired member which is unhealthy (failover.member can be None) - members = [m for m in self.cluster.members if m.name != failover.member] + members = [m for m in self.dcs.cluster.members if m.name != failover.member] if self.is_failover_possible(members): # check that there are healthy members return False else: # I was the leader and it looks like currently I am the only healthy member @@ -221,15 +212,15 @@ class Ha: # at this point we assume that our node is a candidate for a failover among all nodes except former leader # exclude former leader from the list (failover.leader can be None) - members = [m for m in self.cluster.members if m.name != failover.leader] + members = [m for m in self.dcs.cluster.members if m.name != failover.leader] return self._is_healthiest_node(members, check_replication_lag=False) def is_healthiest_node(self): - if self.cluster.failover: + if self.dcs.cluster.failover: return self.manual_failover_process_no_leader() # run usual health check - members = {m.name: m for m in self.old_cluster.members + self.cluster.members} + members = {m.name: m for m in self.dcs.cluster.members + self.old_cluster.members} return self._is_healthiest_node(members.values()) def demote(self, delete_leader=True): @@ -239,14 +230,13 @@ class Ha: self.state_handler.follow_the_leader(None) def process_manual_failover_from_leader(self): - failover = self.cluster.failover + failover = self.dcs.cluster.failover if not failover.leader or failover.leader == self.state_handler.name: if not failover.member or failover.member != self.state_handler.name: - members = [m for m in self.cluster.members if not failover.member or m.name == failover.member] + members = [m for m in self.dcs.cluster.members if not failover.member or m.name == failover.member] if self.is_failover_possible(members): # check that there are healthy members - with self._scheduled_action_lock: - self._scheduled_action = 'manual failover: demote' - self.run_async(self.demote) + self._async_executor.schedule('manual failover: demote') + self._async_executor.run_async(self.demote) return 'manual failover: demoting myself' else: logger.warning('manual failover: no healthy members found, failover is not possible') @@ -254,15 +244,15 @@ class Ha: logger.warning('manual failover: I am already the leader, no need to failover') else: logger.warning('manual failover: leader name does not match: %s != %s', - self.cluster.failover.leader, self.state_handler.name) + self.dcs.cluster.failover.leader, self.state_handler.name) logger.info('Trying to clean up failover key') - self.dcs.manual_failover('', '', self.cluster.failover.index) + self.dcs.manual_failover('', '', self.dcs.cluster.failover.index) def process_unhealthy_cluster(self): if self.is_healthiest_node(): if self.acquire_lock(): - if self.cluster.failover: + if self.dcs.cluster.failover: logger.info('Cleanning up failover key after acquiring leader lock...') self.dcs.manual_failover('', '') return self.enforce_master_role('acquired session lock as a leader', @@ -276,7 +266,7 @@ class Ha: def process_healthy_cluster(self): if self.has_lock(): - if self.cluster.failover: + if self.dcs.cluster.failover: msg = self.process_manual_failover_from_leader() if msg is not None: return msg @@ -293,42 +283,28 @@ class Ha: return self.follow_the_leader('demoting self because i do not have the lock and i was a leader', 'no action. i am a secondary and i am following a leader', False) - def schedule_action(self, action): - with self._long_action_thread_lock: - with self._scheduled_action_lock: - if self._scheduled_action is not None: - return self._scheduled_action - self._scheduled_action = action - return None - - def get_scheduled_action(self): - with self._scheduled_action_lock: - return self._scheduled_action - - def _reset_scheduled_action(self): - with self._scheduled_action_lock: - self._scheduled_action = None + def schedule(self, action): + with self._async_executor: + return self._async_executor.schedule(action) def restart_scheduled(self): - return self.get_scheduled_action() == 'restart' + return self._async_executor.scheduled_action == 'restart' def schedule_reinitialize(self): - return self.schedule_action('reinitialize') + return self.schedule('reinitialize') def reinitialize_scheduled(self): - return self.get_scheduled_action() == 'reinitialize' + return self._async_executor.scheduled_action == 'reinitialize' def restart(self): - with self._long_action_thread_lock: - with self._scheduled_action_lock: - if self._scheduled_action is not None: - return False, self._scheduled_action + ' already in progress' - self._scheduled_action = 'restart' - self._long_action_in_progress = True - if self._run_async(self.state_handler.restart): - return True, 'restarted successfully' + with self._async_executor: + prev = self._async_executor.schedule('restart', True) + if prev is not None: + return (False, prev + ' already in progress') + if self._async_executor.run(self.state_handler.restart): + return (True, 'restarted successfully') else: - return False, 'restart failed' + return (False, 'restart failed') def reinitialize(self): self.state_handler.stop('immediate') @@ -338,47 +314,50 @@ class Ha: def process_scheduled_action(self): if self.reinitialize_scheduled(): - if self.cluster.is_unlocked(): + if self.dcs.cluster.is_unlocked(): logger.error('Cluster has no leader, can not reinitialize') - self._reset_scheduled_action() + self._async_executor.reset_scheduled_action() elif self.has_lock(): logger.error('I am the leader, can not reinitialize') - self._reset_scheduled_action() + self._async_executor.reset_scheduled_action() else: - self.run_async(self.reinitialize) - return True + self._async_executor.run_async(self.reinitialize) + return 'reinitialize started' def handle_long_action_in_progress(self): if self.has_lock(): if self.update_lock(): - return 'updated leader lock during ' + self.get_scheduled_action() + return 'updated leader lock during ' + self._async_executor.scheduled_action else: - return 'failed to update leader lock during ' + self.get_scheduled_action() - elif self.cluster.is_unlocked(): + return 'failed to update leader lock during ' + self._async_executor.scheduled_action + elif self.dcs.cluster.is_unlocked(): return 'not healthy enough for leader race' else: - return self.get_scheduled_action() + ' in progress' + return self._async_executor.scheduled_action + ' in progress' def _run_cycle(self): try: self.load_cluster_from_dcs() + self.touch_member() + # cluster has leader key but not initialize key - if not self.cluster.is_unlocked() and not self.cluster.initialize: + if not self.dcs.cluster.is_unlocked() and not self.dcs.cluster.initialize: self.dcs.initialize() # fix it - if self._long_action_in_progress: + if self._async_executor.busy: return self.handle_long_action_in_progress() # currently it can trigger only reinitialize - if self.process_scheduled_action(): - return 'reinitialize started' + msg = self.process_scheduled_action() + if msg is not None: + return msg # is data directory empty? if self.state_handler.data_directory_empty(): return self.bootstrap() # new node # "bootstrap", but data directory is not empty - elif not self.cluster.initialize and self.cluster.is_unlocked(): + elif not self.dcs.cluster.initialize and self.dcs.cluster.is_unlocked(): self.dcs.initialize() # try to start dead postgres @@ -388,12 +367,12 @@ class Ha: return msg try: - if self.cluster.is_unlocked(): + if self.dcs.cluster.is_unlocked(): return self.process_unhealthy_cluster() else: return self.process_healthy_cluster() finally: - self.state_handler.sync_replication_slots(self.cluster) + self.state_handler.sync_replication_slots(self.dcs.cluster) except DCSError: logger.error('Error communicating with DCS') if self.state_handler.is_running() and self.state_handler.is_leader(): @@ -403,5 +382,5 @@ class Ha: logger.exception('Error communicating with Postgresql. Will try again later') def run_cycle(self): - with self._long_action_thread_lock: + with self._async_executor: return self._run_cycle() diff --git a/patroni/postgresql.py b/patroni/postgresql.py index e15a4996..673f16c3 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -68,7 +68,7 @@ class Postgresql: self._connection = None self._cursor_holder = None self.replication_slots = [] # list of already existing replication slots - self.retry = Retry(max_tries=-1, deadline=10, max_delay=1, retry_exceptions=PostgresConnectionException) + self.retry = Retry(max_tries=-1, deadline=5, max_delay=1, retry_exceptions=PostgresConnectionException) self._state = 'stopped' self._state_lock = Lock() @@ -381,7 +381,7 @@ recovery_target_timeline = 'latest' return self.query("""SELECT pg_xlog_location_diff(CASE WHEN pg_is_in_recovery() THEN pg_last_xlog_replay_location() ELSE pg_current_xlog_location() - END, '0/0')""").fetchone()[0] + END, '0/0')::bigint""").fetchone()[0] def load_replication_slots(self): if self.use_slots and self.schedule_load_slots: diff --git a/patroni/utils.py b/patroni/utils.py index 681ca39c..9b040294 100644 --- a/patroni/utils.py +++ b/patroni/utils.py @@ -36,6 +36,8 @@ def calculate_ttl(expiration): """ >>> calculate_ttl(None) >>> calculate_ttl('2015-06-10 12:56:30.552539016Z') + >>> calculate_ttl('2015-06-10T12:56:30.552539016Z') < 0 + True """ if not expiration: return None diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index 4b3389e1..a5258eac 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -5,7 +5,7 @@ import time from kazoo.client import KazooClient, KazooState from kazoo.exceptions import NoNodeError, NodeExistsError -from patroni.dcs import AbstractDCS, Cluster, Failover, Leader, Member, parse_connection_string +from patroni.dcs import AbstractDCS, Cluster, Failover, Leader, Member from patroni.exceptions import DCSError from patroni.utils import sleep from requests.exceptions import RequestException @@ -93,7 +93,7 @@ class ZooKeeper(AbstractDCS): self.client.add_listener(self.session_listener) self.cluster_event = self.client.handler.event_object() - self.cluster = None + self._my_member_data = None self.fetch_cluster = True self.last_leader_operation = 0 @@ -116,8 +116,7 @@ class ZooKeeper(AbstractDCS): @staticmethod def member(name, value, znode): - conn_url, api_url = parse_connection_string(value) - return Member(znode.version, name, conn_url, api_url, None, None) + return Member.from_node(znode.version, name, znode.ephemeralOwner, value) def get_children(self, key, watch=None): try: @@ -153,9 +152,9 @@ class ZooKeeper(AbstractDCS): leader = None if leader: - member = Member(-1, leader[0], None, None, None, None) + member = Member(-1, leader[0], None, {}) member = ([m for m in members if m.name == leader[0]] or [member])[0] - leader = Leader(leader[1].version, None, None, member) + leader = Leader(leader[1].version, leader[1].ephemeralOwner, member) self.fetch_cluster = member.index == -1 # failover key @@ -207,21 +206,34 @@ class ZooKeeper(AbstractDCS): def initialize(self): return self._create(self.initialize_path, self._name, makepath=True) - def touch_member(self, connection_string, ttl=None): - if not self.fetch_cluster and self.cluster and any(m.name == self._name for m in self.cluster.members): - return True + def touch_member(self, data, ttl=None): + me = self.cluster and ([m for m in self.cluster.members if m.name == self._name] or [None])[0] path = self.member_path - connection_string = connection_string.encode('utf-8') + data = data.encode('utf-8') + create = not me + if me and self.client.client_id is not None and me.session != self.client.client_id[0]: + try: + self.client.retry(self.client.delete, path) + except NoNodeError: + pass + except: + return False + create = True + + if not create and data == self._my_member_data: + return True + try: - self.client.retry(self.client.create, path, connection_string, makepath=True, ephemeral=True) + if create: + self.client.retry(self.client.create, path, data, makepath=True, ephemeral=True) + else: + self.client.retry(self.client.set, path, data) + self._my_member_data = data return True except NodeExistsError: try: - node = self.get_node(path) - if node and self.client.client_id is not None and node[1].ephemeralOwner == self.client.client_id[0]: - return True - self.client.retry(self.client.delete, path) - self.client.retry(self.client.create, path, connection_string, makepath=True, ephemeral=True) + self.client.retry(self.client.set, path, data) + self._my_member_data = data return True except: logger.exception('touch_member') @@ -252,6 +264,7 @@ class ZooKeeper(AbstractDCS): def delete_leader(self): self.client.restart() + self._my_member_data = None return True def _cancel_initialization(self): diff --git a/tests/test_api.py b/tests/test_api.py index 5003e43e..1a8b59e2 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -43,6 +43,7 @@ class MockPatroni: postgresql = MockPostgresql() ha = MockHa() + dcs = Mock() class MockRequest: diff --git a/tests/test_etcd.py b/tests/test_etcd.py index f55aa15e..3d2535a7 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -149,15 +149,6 @@ def http_request(method, url, **kwargs): raise socket.error -class TestMember(unittest.TestCase): - - def test_real_ttl(self): - now = datetime.datetime.utcnow() - member = Member(0, 'a', 'b', 'c', (now + datetime.timedelta(seconds=2)).strftime('%Y-%m-%dT%H:%M:%S.%fZ'), None) - self.assertLess(member.real_ttl(), 2) - self.assertEquals(Member(0, 'a', 'b', 'c', '', None).real_ttl(), -1) - - @patch('dns.resolver.query', dns_query) @patch('socket.getaddrinfo', socket_getaddrinfo) @patch('requests.get', requests_get) diff --git a/tests/test_ha.py b/tests/test_ha.py index 52faba35..a0a43d3f 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -26,11 +26,11 @@ def get_cluster_not_initialized_without_leader(): def get_cluster_initialized_without_leader(leader=False, failover=None): - m = Member(0, 'leader', 'postgres://replicator:rep-pass@127.0.0.1:5435/postgres', - 'http://127.0.0.1:8008/patroni', None, 28) - l = Leader(0, 0, 0, m) if leader else None - o = Member(0, 'other', 'postgres://replicator:rep-pass@127.0.0.1:5436/postgres', - 'http://127.0.0.1:8011/patroni', None, 28) + m = Member(0, 'leader', 28, {'conn_url': 'postgres://replicator:rep-pass@127.0.0.1:5435/postgres', + 'api_url': 'http://127.0.0.1:8008/patroni'}) + l = Leader(0, 0, m) if leader else None + o = Member(0, 'other', 28, {'conn_url': 'postgres://replicator:rep-pass@127.0.0.1:5436/postgres', + 'api_url': 'http://127.0.0.1:8011/patroni'}) return get_cluster(True, l, [m, o], failover) @@ -42,6 +42,8 @@ class MockPostgresql(Mock): name = 'postgresql0' role = 'replica' + state = 'running' + connection_string = 'postgres://foo@bar/postgres' def is_healthy(self): return True @@ -74,6 +76,14 @@ class MockPostgresql(Mock): return False +class MockPatroni: + + def __init__(self, p, d): + self.postgresql = p + self.dcs = d + self.api = Mock() + self.api.connection_string = 'http://127.0.0.1:8008' + def run_async(func, args=()): func(args) if args else func() @@ -88,22 +98,20 @@ class TestHa(unittest.TestCase): self.e = Etcd('foo', {'ttl': 30, 'host': 'ok:2379', 'scope': 'test'}) self.e.client.read = etcd_read self.e.client.write = etcd_write - self.ha = Ha(self.p, self.e) - self.ha.run_async = run_async - self.ha.load_cluster_from_dcs() - self.ha.cluster = get_cluster_not_initialized_without_leader() + self.ha = Ha(MockPatroni(self.p, self.e)) + self.ha._async_executor.run_async = run_async + self.ha.old_cluster = self.e.get_cluster() + self.e.cluster = get_cluster_not_initialized_without_leader() self.ha.load_cluster_from_dcs = Mock() - def test_load_cluster_from_dcs(self): - ha = Ha(self.p, self.e) - ha.load_cluster_from_dcs() - self.e.get_cluster = get_cluster_not_initialized_without_leader - ha.load_cluster_from_dcs() - def test_update_lock(self): self.p.last_operation = Mock(side_effect=PostgresException('')) self.assertTrue(self.ha.update_lock()) + def test_touch_member(self): + self.p.xlog_position = Mock(side_effect=Exception) + self.ha.touch_member() + def test_start_as_replica(self): self.p.is_healthy = false self.assertEquals(self.ha.run_cycle(), 'started as a secondary') @@ -119,8 +127,8 @@ class TestHa(unittest.TestCase): self.ha.has_lock = true self.assertEquals(self.ha.run_cycle(), 'removed leader key after trying and failing to start postgres') + @patch.object(Cluster, 'is_unlocked', Mock(return_value=False)) def test_start_as_readonly(self): - self.ha.cluster.is_unlocked = false self.p.is_leader = self.p.is_healthy = false self.ha.has_lock = true self.assertEquals(self.ha.run_cycle(), 'promoted self to leader because i had the session lock') @@ -153,28 +161,28 @@ class TestHa(unittest.TestCase): self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') def test_promote_because_have_lock(self): - self.ha.cluster.is_unlocked = false + self.e.cluster.is_unlocked = false self.ha.has_lock = true self.p.is_leader = false self.assertEquals(self.ha.run_cycle(), 'promoted self to leader because i had the session lock') def test_leader_with_lock(self): - self.ha.cluster.is_unlocked = false + self.e.cluster.is_unlocked = false self.ha.has_lock = true self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') def test_demote_because_not_having_lock(self): - self.ha.cluster.is_unlocked = false + self.e.cluster.is_unlocked = false self.assertEquals(self.ha.run_cycle(), 'demoting self because i do not have the lock and i was a leader') def test_demote_because_update_lock_failed(self): - self.ha.cluster.is_unlocked = false + self.e.cluster.is_unlocked = false self.ha.has_lock = true self.ha.update_lock = false self.assertEquals(self.ha.run_cycle(), 'demoting self because i do not have the lock and i was a leader') def test_follow_the_leader(self): - self.ha.cluster.is_unlocked = false + self.e.cluster.is_unlocked = false self.p.is_leader = false self.assertEquals(self.ha.run_cycle(), 'no action. i am a secondary and i am following a leader') @@ -182,36 +190,28 @@ class TestHa(unittest.TestCase): self.ha.load_cluster_from_dcs = Mock(side_effect=DCSError('Etcd is not responding properly')) self.assertEquals(self.ha.run_cycle(), 'demoted self because DCS is not accessible and i was a leader') - def test__run_async(self): - self.ha._run_async(Mock(side_effect=Exception())) - - @patch.object(Thread, 'start', Mock()) - def test_run_async(self): - ha = Ha(self.p, self.e) - ha.run_async(true) - def test_bootstrap_from_leader(self): - self.ha.cluster = get_cluster_initialized_with_leader() + self.e.cluster = get_cluster_initialized_with_leader() self.p.bootstrap = false self.assertEquals(self.ha.bootstrap(), 'trying to bootstrap from leader') - self.ha._long_action_in_progress = True + self.ha._async_executor._busy = True self.assertEquals(self.ha.bootstrap(), 'trying to bootstrap from leader') def test_bootstrap_waiting_for_leader(self): - self.ha.cluster = get_cluster_initialized_without_leader() + self.e.cluster = get_cluster_initialized_without_leader() self.assertEquals(self.ha.bootstrap(), 'waiting for leader to bootstrap') def test_bootstrap_initialize_lock_failed(self): - self.ha.cluster = get_cluster_not_initialized_without_leader() + self.e.cluster = get_cluster_not_initialized_without_leader() self.assertEquals(self.ha.bootstrap(), 'failed to acquire initialize lock') def test_bootstrap_initialized_new_cluster(self): - self.ha.cluster = get_cluster_not_initialized_without_leader() + self.e.cluster = get_cluster_not_initialized_without_leader() self.e.initialize = true self.assertEquals(self.ha.bootstrap(), 'initialized a new cluster') def test_bootstrap_release_initialize_key_on_failure(self): - self.ha.cluster = get_cluster_not_initialized_without_leader() + self.e.cluster = get_cluster_not_initialized_without_leader() self.e.initialize = true self.p.bootstrap = Mock(side_effect=PostgresException("Could not bootstrap master PostgreSQL")) self.assertRaises(PostgresException, self.ha.bootstrap) @@ -220,13 +220,13 @@ class TestHa(unittest.TestCase): self.ha.schedule_reinitialize() self.ha.schedule_reinitialize() self.ha.run_cycle() - self.assertIsNone(self.ha.get_scheduled_action()) + self.assertIsNone(self.ha._async_executor.scheduled_action) - self.ha.cluster = get_cluster_initialized_with_leader() + self.e.cluster = get_cluster_initialized_with_leader() self.ha.has_lock = true self.ha.schedule_reinitialize() self.ha.run_cycle() - self.assertIsNone(self.ha.get_scheduled_action()) + self.assertIsNone(self.ha._async_executor.scheduled_action) self.ha.has_lock = false self.ha.schedule_reinitialize() @@ -240,12 +240,11 @@ class TestHa(unittest.TestCase): self.assertEquals(self.ha.restart(), (False, 'reinitialize already in progress')) def test_restart_in_progress(self): - self.ha._long_action_in_progress = True - self.ha._scheduled_action = 'restart' + self.ha._async_executor.schedule('restart', True) self.assertTrue(self.ha.restart_scheduled()) self.assertEquals(self.ha.run_cycle(), 'not healthy enough for leader race') - self.ha.cluster = get_cluster_initialized_with_leader() + self.e.cluster = get_cluster_initialized_with_leader() self.assertEquals(self.ha.run_cycle(), 'restart in progress') self.ha.has_lock = true @@ -257,26 +256,26 @@ class TestHa(unittest.TestCase): @patch('requests.get', requests_get) def test_manual_failover_from_leader(self): self.ha.has_lock = true - self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', '')) + self.e.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', '')) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') - self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, '', MockPostgresql.name)) + self.e.cluster = get_cluster_initialized_with_leader(Failover(0, '', MockPostgresql.name)) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') - self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, '', 'blabla')) + self.e.cluster = get_cluster_initialized_with_leader(Failover(0, '', 'blabla')) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') f = Failover(0, MockPostgresql.name, '') - self.ha.cluster = get_cluster_initialized_with_leader(f) + self.e.cluster = get_cluster_initialized_with_leader(f) self.assertEquals(self.ha.run_cycle(), 'manual failover: demoting myself') @patch('requests.get', requests_get) def test_manual_failover_process_no_leader(self): self.p.is_leader = false - self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', MockPostgresql.name)) + self.e.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', MockPostgresql.name)) self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') - self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', 'leader')) + self.e.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', 'leader')) self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') self.ha.fetch_node_status = lambda e: (e, True, True, 0) # accessible, in_recovery self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') - self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, MockPostgresql.name, '')) + self.e.cluster = get_cluster_initialized_without_leader(failover=Failover(0, MockPostgresql.name, '')) self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') self.ha.fetch_node_status = lambda e: (e, False, True, 0) # accessible, in_recovery self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') @@ -295,7 +294,7 @@ class TestHa(unittest.TestCase): @patch('requests.get', requests_get) def test_fetch_node_status(self): - member = Member(0, 'test', '', 'http://127.0.0.1:8011/patroni', None, None) + member = Member(0, 'test', 1, {'api_url': 'http://127.0.0.1:8011/patroni'}) self.ha.fetch_node_status(member) - member = Member(0, 'test', '', 'http://localhost:8011/patroni', None, None) + member = Member(0, 'test', 1, {'api_url': 'http://localhost:8011/patroni'}) self.ha.fetch_node_status(member) diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 1c82799b..b38f36ce 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -6,6 +6,7 @@ import yaml from mock import Mock, patch from patroni.api import RestApiServer +from patroni.async_executor import AsyncExecutor from patroni.dcs import Cluster, Member from patroni.etcd import Etcd from patroni.ha import Ha @@ -27,7 +28,7 @@ def time_sleep(*args): @patch.object(Postgresql, 'write_pg_hba', Mock()) @patch.object(Postgresql, 'write_recovery_conf', Mock()) @patch.object(BaseHTTPServer.HTTPServer, '__init__', Mock()) -@patch.object(Ha, 'run_async', Mock()) +@patch.object(AsyncExecutor, 'run', Mock()) class TestPatroni(unittest.TestCase): @patch.object(Client, 'machines') @@ -57,15 +58,13 @@ class TestPatroni(unittest.TestCase): sys.argv = ['patroni.py', 'postgres0.yml'] mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) - with patch.object(Patroni, 'touch_member', self.touch_member): - with patch.object(Patroni, 'run', Mock(side_effect=SleepException())): - self.assertRaises(SleepException, main) - with patch.object(Patroni, 'run', Mock(side_effect=KeyboardInterrupt())): - main() + with patch.object(Patroni, 'run', Mock(side_effect=SleepException())): + self.assertRaises(SleepException, main) + with patch.object(Patroni, 'run', Mock(side_effect=KeyboardInterrupt())): + main() @patch('time.sleep', Mock(side_effect=SleepException())) def test_run(self): - self.p.touch_member = self.touch_member self.p.ha.dcs.watch = time_sleep self.assertRaises(SleepException, self.p.run) @@ -73,20 +72,6 @@ class TestPatroni(unittest.TestCase): self.p.api.start = Mock() self.assertRaises(SleepException, self.p.run) - def touch_member(self, ttl=None): - if not self.touched: - self.touched = True - return False - return True - - def test_touch_member(self): - self.p.touch_member() - now = datetime.datetime.utcnow() - member = Member(0, self.p.postgresql.name, 'b', 'c', (now + datetime.timedelta( - seconds=self.p.shutdown_member_ttl + 10)).strftime('%Y-%m-%dT%H:%M:%S.%fZ'), None) - self.p.ha.cluster = Cluster(True, member, 0, [member], None) - self.p.touch_member() - def test_schedule_next_run(self): self.p.ha.dcs.watch = Mock(return_value=True) self.p.schedule_next_run() diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 90874288..007310a5 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -105,10 +105,10 @@ class TestPostgresql(unittest.TestCase): 'restore': 'true'}) if not os.path.exists(self.p.data_dir): os.makedirs(self.p.data_dir) - self.leadermem = Member(0, 'leader', 'postgres://replicator:rep-pass@127.0.0.1:5435/postgres', None, None, 28) - self.leader = Leader(-1, None, 28, self.leadermem) - self.other = Member(0, 'test1', 'postgres://replicator:rep-pass@127.0.0.1:5433/postgres', None, None, 28) - self.me = Member(0, 'test0', 'postgres://replicator:rep-pass@127.0.0.1:5434/postgres', None, None, 28) + self.leadermem = Member(0, 'leader', 28, {'conn_url': 'postgres://replicator:rep-pass@127.0.0.1:5435/postgres'}) + self.leader = Leader(-1, 28, self.leadermem) + self.other = Member(0, 'test1', 28, {'conn_url': 'postgres://replicator:rep-pass@127.0.0.1:5433/postgres'}) + self.me = Member(0, 'test0', 28, {'conn_url': 'postgres://replicator:rep-pass@127.0.0.1:5434/postgres'}) def tearDown(self): shutil.rmtree('data') @@ -146,7 +146,7 @@ class TestPostgresql(unittest.TestCase): self.p.follow_the_leader(self.leader) self.p.demote() self.p.follow_the_leader(self.leader) - self.p.follow_the_leader(Leader(-1, None, 28, self.other)) + self.p.follow_the_leader(Leader(-1, 28, self.other)) def test_create_replica(self): self.p.delete_trigger_file = Mock(side_effect=OSError()) diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py index 039faa58..9e14bec2 100644 --- a/tests/test_zookeeper.py +++ b/tests/test_zookeeper.py @@ -69,6 +69,9 @@ class MockKazooClient(Mock): raise TypeError("Invalid type for 'value' (must be a byte string)") if path == '/service/bla/optime/leader': raise Exception + if path == '/service/test/members/bar': + if value == b'retry': + return if path == '/service/test/failover': if value == b'Exception': raise Exception @@ -85,7 +88,9 @@ class MockKazooClient(Mock): return self.leader = True raise Exception - elif path.endswith('/initialize'): + elif path == '/service/test/members/buzz': + raise Exception + elif path.endswith('/initialize') or path == '/service/test/members/bar': raise NoNodeError @@ -137,10 +142,18 @@ class TestZooKeeper(unittest.TestCase): self.zk.cancel_initialization() def test_touch_member(self): + self.zk._name = 'buzz' + self.zk.get_cluster() self.zk.touch_member('new') + self.zk._name = 'bar' + self.zk.touch_member('new') + self.zk._name = 'na' + self.zk.client.exists = 1 self.zk.touch_member('exists') + self.zk._name = 'bar' self.zk.touch_member('retry') - self.zk.client.exists = True + self.zk.fetch_cluster = True + self.zk.get_cluster() self.zk.touch_member('retry') def test_take_leader(self): From d8f4b09478aaac13cbf3a983f686d05518131cd5 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 2 Oct 2015 10:26:48 +0200 Subject: [PATCH 18/59] use Event.wait instead of sleep it makes possible to break "sleep" for example from API plus small bugfix: catch ValueError exception from json.loads --- patroni/dcs.py | 14 ++++++++++---- patroni/etcd.py | 8 +++++--- patroni/ha.py | 3 ++- patroni/zookeeper.py | 12 ++++-------- tests/test_etcd.py | 6 ++---- tests/test_ha.py | 2 +- tests/test_patroni.py | 3 --- tests/test_zookeeper.py | 2 +- 8 files changed, 25 insertions(+), 25 deletions(-) diff --git a/patroni/dcs.py b/patroni/dcs.py index 1bca092e..d007b392 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -3,8 +3,8 @@ import json from collections import namedtuple from patroni.exceptions import DCSError -from patroni.utils import sleep from six.moves.urllib_parse import urlparse, urlunparse, parse_qsl +from threading import Event def parse_connection_string(value): @@ -42,12 +42,17 @@ class Member(namedtuple('Member', 'index,name,session,data')): """ >>> Member.from_node(-1, '', '', '{"conn_url": "postgres://foo@bar/postgres"}') is not None True + >>> Member.from_node(-1, '', '', '{') + Member(index=-1, name='', session='', data={}) """ if data.startswith('postgres'): conn_url, api_url = parse_connection_string(data) data = {'conn_url': conn_url, 'api_url': api_url} else: - data = json.loads(data) + try: + data = json.loads(data) + except: + data = {} return Member(index, name, session, data) @property @@ -121,6 +126,7 @@ class AbstractDCS: self._base_path = '/service/' + self._scope self.cluster = None + self.event = Event() def client_path(self, path): return '/'.join([self._base_path, path.lstrip('/')]) @@ -234,5 +240,5 @@ class AbstractDCS: :param timeout: timeout in seconds :returns: `!True` if you would like to reschedule the next run of ha cycle""" - sleep(timeout) - return False + self.event.wait(timeout) + return self.event.isSet() diff --git a/patroni/etcd.py b/patroni/etcd.py index f1ca5044..76e00554 100644 --- a/patroni/etcd.py +++ b/patroni/etcd.py @@ -147,7 +147,6 @@ class Etcd(AbstractDCS): def __init__(self, name, config): super(Etcd, self).__init__(name, config) self.ttl = config['ttl'] - self.member_ttl = config.get('member_ttl', 3600) self._retry = Retry(deadline=10, max_delay=1, max_tries=-1, retry_exceptions=(etcd.EtcdConnectionFailed, etcd.EtcdLeaderElectionInProgress, @@ -210,7 +209,7 @@ class Etcd(AbstractDCS): @catch_etcd_errors def touch_member(self, connection_string, ttl=None): - return self.retry(self.client.set, self.member_path, connection_string, ttl or self.member_ttl) + return self.retry(self.client.set, self.member_path, connection_string, ttl or self.ttl) @catch_etcd_errors def take_leader(self): @@ -269,4 +268,7 @@ class Etcd(AbstractDCS): timeout = end_time - time.time() - return timeout > 0 and super(Etcd, self).watch(timeout) + try: + return super(Etcd, self).watch(timeout) + finally: + self.event.clear() diff --git a/patroni/ha.py b/patroni/ha.py index 25a05c23..ca79d8e3 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -150,7 +150,8 @@ class Ha: if self.state_handler.is_leader(): return True - if check_replication_lag and not self.state_handler.check_replication_lag(self.dcs.cluster.last_leader_operation): + if check_replication_lag and \ + not self.state_handler.check_replication_lag(self.dcs.cluster.last_leader_operation): return False # Too far behind last reported xlog location on master # Prepare list of nodes to run check against diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index a5258eac..68786485 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -91,7 +91,6 @@ class ZooKeeper(AbstractDCS): 'max_tries': -1}, connection_retry={'max_delay': 1, 'max_tries': -1}) self.client.add_listener(self.session_listener) - self.cluster_event = self.client.handler.event_object() self._my_member_data = None self.fetch_cluster = True @@ -105,7 +104,7 @@ class ZooKeeper(AbstractDCS): def cluster_watcher(self, event): self.fetch_cluster = True - self.cluster_event.set() + self.event.set() def get_node(self, key, watch=None): try: @@ -133,7 +132,7 @@ class ZooKeeper(AbstractDCS): return members def _inner_load_cluster(self): - self.cluster_event.clear() + self.event.clear() nodes = set(self.get_children(self.client_path(''), self.cluster_watcher)) # get initialize flag @@ -279,8 +278,5 @@ class ZooKeeper(AbstractDCS): logger.exception("Unable to delete initialize key") def watch(self, timeout): - self.cluster_event.wait(timeout) - if self.cluster_event.isSet(): - self.fetch_cluster = True - return True - return False + self.fetch_cluster = super(ZooKeeper, self).watch(timeout) + return self.fetch_cluster diff --git a/tests/test_etcd.py b/tests/test_etcd.py index 3d2535a7..53c054e5 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -1,4 +1,3 @@ -import datetime import etcd import json import requests @@ -8,7 +7,7 @@ import unittest from dns.exception import DNSException from mock import Mock, patch -from patroni.dcs import Cluster, DCSError, Leader, Member +from patroni.dcs import Cluster, DCSError, Leader from patroni.etcd import Client, Etcd @@ -194,7 +193,6 @@ class TestClient(unittest.TestCase): self.assertRaises(etcd.EtcdException, self.client._load_machines_cache) -@patch('time.sleep', Mock()) @patch('requests.get', requests_get) class TestEtcd(unittest.TestCase): @@ -254,7 +252,7 @@ class TestEtcd(unittest.TestCase): def test_watch(self): self.etcd.client.watch = etcd_watch - self.etcd.watch(100) + self.etcd.watch(0) self.etcd.get_cluster() self.etcd.watch(1.5) self.etcd.watch(4.5) diff --git a/tests/test_ha.py b/tests/test_ha.py index a0a43d3f..392ad663 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -6,7 +6,6 @@ from patroni.etcd import Client, Etcd from patroni.exceptions import DCSError, PostgresException from patroni.ha import Ha from test_etcd import socket_getaddrinfo, etcd_read, etcd_write, requests_get -from threading import Thread def true(*args, **kwargs): @@ -84,6 +83,7 @@ class MockPatroni: self.api = Mock() self.api.connection_string = 'http://127.0.0.1:8008' + def run_async(func, args=()): func(args) if args else func() diff --git a/tests/test_patroni.py b/tests/test_patroni.py index b38f36ce..52b10d7a 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -1,4 +1,3 @@ -import datetime import sys import time import unittest @@ -7,9 +6,7 @@ import yaml from mock import Mock, patch from patroni.api import RestApiServer from patroni.async_executor import AsyncExecutor -from patroni.dcs import Cluster, Member from patroni.etcd import Etcd -from patroni.ha import Ha from patroni import Patroni, main from patroni.zookeeper import ZooKeeper from six.moves import BaseHTTPServer diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py index 9e14bec2..b4e004be 100644 --- a/tests/test_zookeeper.py +++ b/tests/test_zookeeper.py @@ -170,5 +170,5 @@ class TestZooKeeper(unittest.TestCase): def test_watch(self): self.zk.watch(0) - self.zk.cluster_event.isSet = lambda: False + self.zk.event.isSet = lambda: False self.zk.watch(0) From bad37a5a212ebaf81a0d282fa1133a51785ce183 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Fri, 2 Oct 2015 10:57:49 +0200 Subject: [PATCH 19/59] Always check that cluster is configured correctly right before running pg_rewind. --- patroni/postgresql.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 4c0da705..4fd0e13d 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -75,8 +75,6 @@ class Postgresql: def init_pg_rewind(self): try: self._pg_rewind_present = ('username' in self._pg_rewind and - ('wal_log_hints' in self.config['parameters'] or - 'data_checksums' in self.config['parameters']) and subprocess.call(['pg_rewind', '--version'], stdout=open(os.devnull, 'w'), @@ -317,14 +315,12 @@ recovery_target_timeline = 'latest' for name, value in self.config.get('recovery_conf', {}).items(): f.write("{} = '{}'\n".format(name, value)) - def prepare_pg_rewind_connection(self, leader_url, pg_rewind): - r = parseurl(leader_url) - r.update(pg_rewind) - env = self.write_pgpass(r, append=True) - return (env, "user={user} host={host} port={port} dbname=postgres sslmode=prefer sslcompression=1".format(**r)) - def pg_rewind(self, leader): - env, pc = self.prepare_pg_rewind_connection(leader.conn_url, self._pg_rewind) + # prepare pg_rewind connection + r = parseurl(leader.conn_url) + r.update(self._pg_rewind) + env = self.write_pgpass(r, append=True) + pc = "user={user} host={host} port={port} dbname=postgres sslmode=prefer sslcompression=1".format(**r) logger.info("running pg_rewind from {}".format(pc)) pg_rewind = ['pg_rewind', '-D', self.data_dir, '--source-server', pc] try: @@ -335,12 +331,21 @@ recovery_target_timeline = 'latest' self.write_recovery_conf(leader) return ret + def pg_rewind_verify_cluster(self): + """ check that pg_rewind can be used with the cluster """ + try: + return self.query("""SELECT bool_or(setting::boolean) + FROM pg_settings + WHERE name IN ( 'data_checksums', 'wal_log_hints')""").fetchone()[0] + except: + return False + def follow_the_leader(self, leader): if not self.check_recovery_conf(leader): self.write_recovery_conf(leader) change_role = self.role == 'master' - if leader and change_role and self._pg_rewind_present: + if leader and change_role and self._pg_rewind_present and self.pg_rewind_verify_cluster(): self.stop() if self.pg_rewind(leader): ret = self.start() From 4c444c943e60758f0f84500a5fcb88c752bcf2cc Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 2 Oct 2015 13:17:58 +0200 Subject: [PATCH 20/59] tests for Api.do_GET method --- tests/test_api.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/test_api.py b/tests/test_api.py index 1a8b59e2..e4b8f87e 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -71,10 +71,20 @@ class MockRestApiServer(RestApiServer): class TestRestApiHandler(unittest.TestCase): def test_do_GET(self): - MockRestApiServer(RestApiHandler, b'GET /master') MockRestApiServer(RestApiHandler, b'GET /replica') + with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={})): + MockRestApiServer(RestApiHandler, b'GET /replica') + with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={'role': 'master'})): + MockRestApiServer(RestApiHandler, b'GET /replica') + MockRestApiServer(RestApiHandler, b'GET /master') + MockPatroni.dcs.cluster.leader.name = MockPostgresql.name + MockRestApiServer(RestApiHandler, b'GET /replica') + MockPatroni.dcs.cluster = None + with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={'role': 'master'})): + MockRestApiServer(RestApiHandler, b'GET /master') with patch.object(MockHa, 'restart_scheduled', Mock(return_value=True)): MockRestApiServer(RestApiHandler, b'GET /master') + MockRestApiServer(RestApiHandler, b'GET /master') def test_do_GET_patroni(self): MockRestApiServer(RestApiHandler, b'GET /patroni') From 601ba7db8da5507f60a4b79df744b49bd64db597 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 5 Oct 2015 14:30:47 +0200 Subject: [PATCH 21/59] Make work with dcs.cluster thread-safe --- patroni/api.py | 5 +-- patroni/dcs.py | 33 +++++++++++++++--- patroni/etcd.py | 13 ++++--- patroni/ha.py | 81 +++++++++++++++++++++++--------------------- patroni/zookeeper.py | 9 +++-- tests/test_ha.py | 44 ++++++++++++------------ 6 files changed, 104 insertions(+), 81 deletions(-) diff --git a/patroni/api.py b/patroni/api.py index 0333e86c..dc83249f 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -48,8 +48,9 @@ class RestApiHandler(BaseHTTPRequestHandler): response = self.get_postgresql_status() patroni = self.server.patroni - if patroni.dcs.cluster: # dcs available - if patroni.dcs.cluster.leader and patroni.dcs.cluster.leader.name == patroni.postgresql.name: # is_leader + cluster = patroni.dcs.cluster + if cluster: # dcs available + if cluster.leader and cluster.leader.name == patroni.postgresql.name: # is_leader status_code = 200 if 'master' in path else 503 elif 'role' not in response: status_code = 503 diff --git a/patroni/dcs.py b/patroni/dcs.py index d007b392..25e44cd1 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -4,7 +4,7 @@ import json from collections import namedtuple from patroni.exceptions import DCSError from six.moves.urllib_parse import urlparse, urlunparse, parse_qsl -from threading import Event +from threading import Event, Lock def parse_connection_string(value): @@ -125,7 +125,8 @@ class AbstractDCS: self._scope = config['scope'] self._base_path = '/service/' + self._scope - self.cluster = None + self._cluster = None + self._cluster_thread_lock = Lock() self.event = Event() def client_path(self, path): @@ -156,10 +157,32 @@ class AbstractDCS: return self.client_path(self._LEADER_OPTIME) @abc.abstractmethod + def _load_cluster(self): + """Internally this method should build `Cluster` object which + represents current state and topology of the cluster in DCS. + this method supposed to be called only by `get_cluster` method. + + raise `~DCSError` in case of communication or other problems with DCS. + If the current node was running as a master and exception raised, + instance would be demoted.""" + def get_cluster(self): - """:returns: `Cluster` object which represent current state and topology of the cluster - raise `~DCSError` in case of communication or other problems with DCS. If current instance was - running as a master and exception raised instance would be demoted.""" + with self._cluster_thread_lock: + try: + self._load_cluster() + except: + self._cluster = None + raise + return self._cluster + + @property + def cluster(self): + with self._cluster_thread_lock: + return self._cluster + + def reset_cluster(self): + with self._cluster_thread_lock: + self._cluster = None @abc.abstractmethod def write_leader_optime(self, last_operation): diff --git a/patroni/etcd.py b/patroni/etcd.py index 76e00554..79379700 100644 --- a/patroni/etcd.py +++ b/patroni/etcd.py @@ -171,7 +171,7 @@ class Etcd(AbstractDCS): def member(node): return Member.from_node(node.modifiedIndex, os.path.basename(node.key), node.ttl, node.value) - def get_cluster(self): + def _load_cluster(self): try: result = self.retry(self.client.read, self.client_path(''), recursive=True) nodes = {os.path.relpath(node.key, result.key): node for node in result.leaves} @@ -198,14 +198,12 @@ class Etcd(AbstractDCS): if failover: failover = Failover.from_node(failover.modifiedIndex, failover.value) - self.cluster = Cluster(initialize, leader, last_leader_operation, members, failover) + self._cluster = Cluster(initialize, leader, last_leader_operation, members, failover) except etcd.EtcdKeyNotFound: - self.cluster = Cluster(False, None, None, [], None) + self._cluster = Cluster(False, None, None, [], None) except: - self.cluster = None logger.exception('get_cluster') raise EtcdError('Etcd is not responding properly') - return self.cluster @catch_etcd_errors def touch_member(self, connection_string, ttl=None): @@ -249,10 +247,11 @@ class Etcd(AbstractDCS): return self.retry(self.client.delete, self.initialize_path, prevValue=self._name) def watch(self, timeout): + cluster = self.cluster # watch on leader key changes if it is defined and current node is not lock owner - if self.cluster and self.cluster.leader and self.cluster.leader.name != self._name: + if cluster and cluster.leader and cluster.leader.name != self._name: end_time = time.time() + timeout - index = self.cluster.leader.index + index = cluster.leader.index while index and timeout >= 1: # when timeout is too small urllib3 doesn't have enough time to connect try: diff --git a/patroni/ha.py b/patroni/ha.py index ca79d8e3..9203e153 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -16,13 +16,17 @@ class Ha: self.patroni = patroni self.state_handler = patroni.postgresql self.dcs = patroni.dcs + self.cluster = None self.old_cluster = None self._async_executor = AsyncExecutor() def load_cluster_from_dcs(self): + cluster = self.dcs.get_cluster() + # We want to keep the state of cluster when it was healhy - if not self.dcs.get_cluster().is_unlocked() or not self.old_cluster: - self.old_cluster = self.dcs.cluster + if not cluster.is_unlocked() or not self.old_cluster: + self.old_cluster = cluster + self.cluster = cluster def acquire_lock(self): return self.dcs.attempt_to_acquire_leader() @@ -37,7 +41,7 @@ class Ha: return ret def has_lock(self): - lock_owner = self.dcs.cluster.leader and self.dcs.cluster.leader.name + lock_owner = self.cluster.leader and self.cluster.leader.name logger.info('Lock owner: %s; I am %s', lock_owner, self.state_handler.name) return lock_owner == self.state_handler.name @@ -55,8 +59,8 @@ class Ha: pass self.dcs.touch_member(json.dumps(data, separators=(',', ':'))) - def copy_backup_from_leader(self): - if self.state_handler.bootstrap(self.dcs.cluster.leader): + def copy_backup_from_leader(self, leader): + if self.state_handler.bootstrap(leader): logger.info('bootstrapped from leader') else: self.state_handler.stop('immediate') @@ -64,14 +68,11 @@ class Ha: logger.error('failed to bootstrap from leader') def bootstrap(self): - if not self.dcs.cluster.is_unlocked(): # cluster already has leader - if self._async_executor.busy: - self.copy_backup_from_leader() - else: - self._async_executor.schedule('bootstrap from leader') - self._async_executor.run_async(self.copy_backup_from_leader) + if not self.cluster.is_unlocked(): # cluster already has leader + self._async_executor.schedule('bootstrap from leader') + self._async_executor.run_async(self.copy_backup_from_leader, args=(self.cluster.leader, )) return 'trying to bootstrap from leader' - elif not self.dcs.cluster.initialize: # no initialize key + elif not self.cluster.initialize: # no initialize key if self.dcs.initialize(): # race for initialization try: self.state_handler.bootstrap() @@ -91,11 +92,12 @@ class Ha: def recover(self): has_lock = self.has_lock() - self.state_handler.write_recovery_conf(None if has_lock else self.dcs.cluster.leader) + self.state_handler.write_recovery_conf(None if has_lock else self.cluster.leader) if not self.state_handler.start(): if not has_lock: return 'failed to start postgres' self.dcs.delete_leader() + self.dcs.reset_cluster() return 'removed leader key after trying and failing to start postgres' if not has_lock: return 'started as a secondary' @@ -105,9 +107,11 @@ class Ha: def follow_the_leader(self, demote_reason, follow_reason, refresh=True): refresh and self.load_cluster_from_dcs() ret = demote_reason if self.state_handler.is_leader() else follow_reason - if not self.state_handler.check_recovery_conf(self.dcs.cluster.leader): + if not self.state_handler.check_recovery_conf(self.cluster.leader): self._async_executor.schedule('changing primary_conninfo and restarting') - self._async_executor.run_async(self.state_handler.follow_the_leader, (self.dcs.cluster.leader, )) + leader = self.cluster.leader + leader = None if (leader and leader.name) == self.state_handler.name else leader + self._async_executor.run_async(self.state_handler.follow_the_leader, (leader, )) return ret def enforce_master_role(self, message, promote_message): @@ -150,8 +154,7 @@ class Ha: if self.state_handler.is_leader(): return True - if check_replication_lag and \ - not self.state_handler.check_replication_lag(self.dcs.cluster.last_leader_operation): + if check_replication_lag and not self.state_handler.check_replication_lag(self.cluster.last_leader_operation): return False # Too far behind last reported xlog location on master # Prepare list of nodes to run check against @@ -182,13 +185,13 @@ class Ha: return ret def manual_failover_process_no_leader(self): - failover = self.dcs.cluster.failover + failover = self.cluster.failover if failover.member: # manual failover to specific member if failover.member == self.state_handler.name: # manual failover to me return True # find specific node and check that it is healthy - members = [m for m in self.dcs.cluster.members if m.name == failover.member] + members = [m for m in self.cluster.members if m.name == failover.member] if members: member, reachable, in_recovery, xlog_location = self.fetch_node_status(members[0]) if reachable: # node is healthy @@ -204,7 +207,7 @@ class Ha: if failover.leader: if self.state_handler.name == failover.leader: # I was the leader # exclude me and desired member which is unhealthy (failover.member can be None) - members = [m for m in self.dcs.cluster.members if m.name != failover.member] + members = [m for m in self.cluster.members if m.name != failover.member] if self.is_failover_possible(members): # check that there are healthy members return False else: # I was the leader and it looks like currently I am the only healthy member @@ -213,28 +216,29 @@ class Ha: # at this point we assume that our node is a candidate for a failover among all nodes except former leader # exclude former leader from the list (failover.leader can be None) - members = [m for m in self.dcs.cluster.members if m.name != failover.leader] + members = [m for m in self.cluster.members if m.name != failover.leader] return self._is_healthiest_node(members, check_replication_lag=False) def is_healthiest_node(self): - if self.dcs.cluster.failover: + if self.cluster.failover: return self.manual_failover_process_no_leader() # run usual health check - members = {m.name: m for m in self.dcs.cluster.members + self.old_cluster.members} + members = {m.name: m for m in self.cluster.members + self.old_cluster.members} return self._is_healthiest_node(members.values()) def demote(self, delete_leader=True): if delete_leader: self.state_handler.stop() self.dcs.delete_leader() + self.dcs.reset_cluster() self.state_handler.follow_the_leader(None) def process_manual_failover_from_leader(self): - failover = self.dcs.cluster.failover + failover = self.cluster.failover if not failover.leader or failover.leader == self.state_handler.name: if not failover.member or failover.member != self.state_handler.name: - members = [m for m in self.dcs.cluster.members if not failover.member or m.name == failover.member] + members = [m for m in self.cluster.members if not failover.member or m.name == failover.member] if self.is_failover_possible(members): # check that there are healthy members self._async_executor.schedule('manual failover: demote') self._async_executor.run_async(self.demote) @@ -245,15 +249,15 @@ class Ha: logger.warning('manual failover: I am already the leader, no need to failover') else: logger.warning('manual failover: leader name does not match: %s != %s', - self.dcs.cluster.failover.leader, self.state_handler.name) + self.cluster.failover.leader, self.state_handler.name) logger.info('Trying to clean up failover key') - self.dcs.manual_failover('', '', self.dcs.cluster.failover.index) + self.dcs.manual_failover('', '', self.cluster.failover.index) def process_unhealthy_cluster(self): if self.is_healthiest_node(): if self.acquire_lock(): - if self.dcs.cluster.failover: + if self.cluster.failover: logger.info('Cleanning up failover key after acquiring leader lock...') self.dcs.manual_failover('', '') return self.enforce_master_role('acquired session lock as a leader', @@ -267,7 +271,7 @@ class Ha: def process_healthy_cluster(self): if self.has_lock(): - if self.dcs.cluster.failover: + if self.cluster.failover: msg = self.process_manual_failover_from_leader() if msg is not None: return msg @@ -307,22 +311,21 @@ class Ha: else: return (False, 'restart failed') - def reinitialize(self): + def reinitialize(self, cluster): self.state_handler.stop('immediate') self.state_handler.remove_data_directory() - self.load_cluster_from_dcs() - self.bootstrap() + self.copy_backup_from_leader(cluster.leader) def process_scheduled_action(self): if self.reinitialize_scheduled(): - if self.dcs.cluster.is_unlocked(): + if self.cluster.is_unlocked(): logger.error('Cluster has no leader, can not reinitialize') self._async_executor.reset_scheduled_action() elif self.has_lock(): logger.error('I am the leader, can not reinitialize') self._async_executor.reset_scheduled_action() else: - self._async_executor.run_async(self.reinitialize) + self._async_executor.run_async(self.reinitialize, args=(self.cluster, )) return 'reinitialize started' def handle_long_action_in_progress(self): @@ -331,7 +334,7 @@ class Ha: return 'updated leader lock during ' + self._async_executor.scheduled_action else: return 'failed to update leader lock during ' + self._async_executor.scheduled_action - elif self.dcs.cluster.is_unlocked(): + elif self.cluster.is_unlocked(): return 'not healthy enough for leader race' else: return self._async_executor.scheduled_action + ' in progress' @@ -343,7 +346,7 @@ class Ha: self.touch_member() # cluster has leader key but not initialize key - if not self.dcs.cluster.is_unlocked() and not self.dcs.cluster.initialize: + if not self.cluster.is_unlocked() and not self.cluster.initialize: self.dcs.initialize() # fix it if self._async_executor.busy: @@ -358,7 +361,7 @@ class Ha: if self.state_handler.data_directory_empty(): return self.bootstrap() # new node # "bootstrap", but data directory is not empty - elif not self.dcs.cluster.initialize and self.dcs.cluster.is_unlocked(): + elif not self.cluster.initialize and self.cluster.is_unlocked(): self.dcs.initialize() # try to start dead postgres @@ -368,12 +371,12 @@ class Ha: return msg try: - if self.dcs.cluster.is_unlocked(): + if self.cluster.is_unlocked(): return self.process_unhealthy_cluster() else: return self.process_healthy_cluster() finally: - self.state_handler.sync_replication_slots(self.dcs.cluster) + self.state_handler.sync_replication_slots(self.cluster) except DCSError: logger.error('Error communicating with DCS') if self.state_handler.is_running() and self.state_handler.is_leader(): diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index 68786485..c328ae52 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -164,9 +164,9 @@ class ZooKeeper(AbstractDCS): # get last leader operation self.last_leader_operation = self.get_node(self.leader_optime_path) if self.fetch_cluster else None self.last_leader_operation = 0 if self.last_leader_operation is None else int(self.last_leader_operation[0]) - self.cluster = Cluster(initialize, leader, self.last_leader_operation, members, failover) + self._cluster = Cluster(initialize, leader, self.last_leader_operation, members, failover) - def get_cluster(self): + def _load_cluster(self): if self.exhibitor and self.exhibitor.poll(): self.client.set_hosts(self.exhibitor.zookeeper_hosts) @@ -174,11 +174,9 @@ class ZooKeeper(AbstractDCS): try: self.client.retry(self._inner_load_cluster) except: - self.cluster = None logger.exception('get_cluster') self.session_listener(KazooState.LOST) raise ZooKeeperError('ZooKeeper in not responding properly') - return self.cluster def _create(self, path, value, **kwargs): try: @@ -206,7 +204,8 @@ class ZooKeeper(AbstractDCS): return self._create(self.initialize_path, self._name, makepath=True) def touch_member(self, data, ttl=None): - me = self.cluster and ([m for m in self.cluster.members if m.name == self._name] or [None])[0] + cluster = self.cluster + me = cluster and ([m for m in cluster.members if m.name == self._name] or [None])[0] path = self.member_path data = data.encode('utf-8') create = not me diff --git a/tests/test_ha.py b/tests/test_ha.py index 392ad663..0a986280 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -85,7 +85,7 @@ class MockPatroni: def run_async(func, args=()): - func(args) if args else func() + func(*args) if args else func() class TestHa(unittest.TestCase): @@ -101,7 +101,7 @@ class TestHa(unittest.TestCase): self.ha = Ha(MockPatroni(self.p, self.e)) self.ha._async_executor.run_async = run_async self.ha.old_cluster = self.e.get_cluster() - self.e.cluster = get_cluster_not_initialized_without_leader() + self.ha.cluster = get_cluster_not_initialized_without_leader() self.ha.load_cluster_from_dcs = Mock() def test_update_lock(self): @@ -161,28 +161,28 @@ class TestHa(unittest.TestCase): self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') def test_promote_because_have_lock(self): - self.e.cluster.is_unlocked = false + self.ha.cluster.is_unlocked = false self.ha.has_lock = true self.p.is_leader = false self.assertEquals(self.ha.run_cycle(), 'promoted self to leader because i had the session lock') def test_leader_with_lock(self): - self.e.cluster.is_unlocked = false + self.ha.cluster.is_unlocked = false self.ha.has_lock = true self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') def test_demote_because_not_having_lock(self): - self.e.cluster.is_unlocked = false + self.ha.cluster.is_unlocked = false self.assertEquals(self.ha.run_cycle(), 'demoting self because i do not have the lock and i was a leader') def test_demote_because_update_lock_failed(self): - self.e.cluster.is_unlocked = false + self.ha.cluster.is_unlocked = false self.ha.has_lock = true self.ha.update_lock = false self.assertEquals(self.ha.run_cycle(), 'demoting self because i do not have the lock and i was a leader') def test_follow_the_leader(self): - self.e.cluster.is_unlocked = false + self.ha.cluster.is_unlocked = false self.p.is_leader = false self.assertEquals(self.ha.run_cycle(), 'no action. i am a secondary and i am following a leader') @@ -191,27 +191,25 @@ class TestHa(unittest.TestCase): self.assertEquals(self.ha.run_cycle(), 'demoted self because DCS is not accessible and i was a leader') def test_bootstrap_from_leader(self): - self.e.cluster = get_cluster_initialized_with_leader() + self.ha.cluster = get_cluster_initialized_with_leader() self.p.bootstrap = false self.assertEquals(self.ha.bootstrap(), 'trying to bootstrap from leader') - self.ha._async_executor._busy = True - self.assertEquals(self.ha.bootstrap(), 'trying to bootstrap from leader') def test_bootstrap_waiting_for_leader(self): - self.e.cluster = get_cluster_initialized_without_leader() + self.ha.cluster = get_cluster_initialized_without_leader() self.assertEquals(self.ha.bootstrap(), 'waiting for leader to bootstrap') def test_bootstrap_initialize_lock_failed(self): - self.e.cluster = get_cluster_not_initialized_without_leader() + self.ha.cluster = get_cluster_not_initialized_without_leader() self.assertEquals(self.ha.bootstrap(), 'failed to acquire initialize lock') def test_bootstrap_initialized_new_cluster(self): - self.e.cluster = get_cluster_not_initialized_without_leader() + self.ha.cluster = get_cluster_not_initialized_without_leader() self.e.initialize = true self.assertEquals(self.ha.bootstrap(), 'initialized a new cluster') def test_bootstrap_release_initialize_key_on_failure(self): - self.e.cluster = get_cluster_not_initialized_without_leader() + self.ha.cluster = get_cluster_not_initialized_without_leader() self.e.initialize = true self.p.bootstrap = Mock(side_effect=PostgresException("Could not bootstrap master PostgreSQL")) self.assertRaises(PostgresException, self.ha.bootstrap) @@ -222,7 +220,7 @@ class TestHa(unittest.TestCase): self.ha.run_cycle() self.assertIsNone(self.ha._async_executor.scheduled_action) - self.e.cluster = get_cluster_initialized_with_leader() + self.ha.cluster = get_cluster_initialized_with_leader() self.ha.has_lock = true self.ha.schedule_reinitialize() self.ha.run_cycle() @@ -244,7 +242,7 @@ class TestHa(unittest.TestCase): self.assertTrue(self.ha.restart_scheduled()) self.assertEquals(self.ha.run_cycle(), 'not healthy enough for leader race') - self.e.cluster = get_cluster_initialized_with_leader() + self.ha.cluster = get_cluster_initialized_with_leader() self.assertEquals(self.ha.run_cycle(), 'restart in progress') self.ha.has_lock = true @@ -256,26 +254,26 @@ class TestHa(unittest.TestCase): @patch('requests.get', requests_get) def test_manual_failover_from_leader(self): self.ha.has_lock = true - self.e.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', '')) + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', '')) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') - self.e.cluster = get_cluster_initialized_with_leader(Failover(0, '', MockPostgresql.name)) + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, '', MockPostgresql.name)) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') - self.e.cluster = get_cluster_initialized_with_leader(Failover(0, '', 'blabla')) + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, '', 'blabla')) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') f = Failover(0, MockPostgresql.name, '') - self.e.cluster = get_cluster_initialized_with_leader(f) + self.ha.cluster = get_cluster_initialized_with_leader(f) self.assertEquals(self.ha.run_cycle(), 'manual failover: demoting myself') @patch('requests.get', requests_get) def test_manual_failover_process_no_leader(self): self.p.is_leader = false - self.e.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', MockPostgresql.name)) + self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', MockPostgresql.name)) self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') - self.e.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', 'leader')) + self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', 'leader')) self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') self.ha.fetch_node_status = lambda e: (e, True, True, 0) # accessible, in_recovery self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') - self.e.cluster = get_cluster_initialized_without_leader(failover=Failover(0, MockPostgresql.name, '')) + self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, MockPostgresql.name, '')) self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') self.ha.fetch_node_status = lambda e: (e, False, True, 0) # accessible, in_recovery self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') From d48f8384ed3314e634ca26682aaf16f2965fc409 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 6 Oct 2015 10:05:19 +0200 Subject: [PATCH 22/59] leader variable should be None if the leader.name == my name. This check has to be performed even check_recovery_conf call --- patroni/ha.py | 6 +++--- patroni/zookeeper.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 9203e153..d4102e61 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -107,10 +107,10 @@ class Ha: def follow_the_leader(self, demote_reason, follow_reason, refresh=True): refresh and self.load_cluster_from_dcs() ret = demote_reason if self.state_handler.is_leader() else follow_reason - if not self.state_handler.check_recovery_conf(self.cluster.leader): + leader = self.cluster.leader + leader = None if (leader and leader.name) == self.state_handler.name else leader + if not self.state_handler.check_recovery_conf(leader): self._async_executor.schedule('changing primary_conninfo and restarting') - leader = self.cluster.leader - leader = None if (leader and leader.name) == self.state_handler.name else leader self._async_executor.run_async(self.state_handler.follow_the_leader, (leader, )) return ret diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index c328ae52..c6bfe639 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -197,7 +197,7 @@ class ZooKeeper(AbstractDCS): except NoNodeError: return value == '' or (not index and self._create(self.failover_path, value.encode('utf-8'))) except: - logging.exception('foo') + logging.exception('set_failover_value') return False def initialize(self): From 8a844285ff83c5c85c0884dae31f0664dd153022 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 7 Oct 2015 16:48:39 +0200 Subject: [PATCH 23/59] Set fetch_cluster flag to False when _inner_load_cluster called Set the same flag to True if the cluster does not yet exists in ZooKeeper --- patroni/zookeeper.py | 3 +++ tests/test_zookeeper.py | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index c6bfe639..c7b9a44c 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -132,8 +132,11 @@ class ZooKeeper(AbstractDCS): return members def _inner_load_cluster(self): + self.fetch_cluster = False self.event.clear() nodes = set(self.get_children(self.client_path(''), self.cluster_watcher)) + if not nodes: + self.fetch_cluster = True # get initialize flag initialize = self._INITIALIZE in nodes diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py index b4e004be..731eb53f 100644 --- a/tests/test_zookeeper.py +++ b/tests/test_zookeeper.py @@ -46,7 +46,7 @@ class MockKazooClient(Mock): def get_children(self, path, watch=None, include_data=False): if not isinstance(path, six.string_types): raise TypeError("Invalid type for 'path' (string expected)") - if path == '/no_node': + if path.startswith('/no_node'): raise NoNodeError elif path in ['/service/bla/', '/service/test/']: return ['initialize', 'leader', 'members', 'optime', 'failover'] @@ -121,6 +121,8 @@ class TestZooKeeper(unittest.TestCase): def test__inner_load_cluster(self): self.zk._base_path = self.zk._base_path.replace('test', 'bla') self.zk._inner_load_cluster() + self.zk._base_path = self.zk._base_path = '/no_node' + self.zk._inner_load_cluster() def test_get_cluster(self): self.assertRaises(ZooKeeperError, self.zk.get_cluster) From 52c4826569797cca41f2fe7a53bd8be81d49a5e4 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Thu, 8 Oct 2015 12:40:21 +0200 Subject: [PATCH 24/59] Reflect the renaming of os-registry.stups.zalan.do to registry.opensource.zalan.do --- docker/README.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docker/README.md b/docker/README.md index 74288a98..afadc4b6 100644 --- a/docker/README.md +++ b/docker/README.md @@ -1,7 +1,7 @@ # Patroni Dockerfile You can run Patroni in a docker container using this Dockerfile, or by using one of the Docker image at - https://os-registry.stups.zalan.do/v1/repositories/acid/patroni/tags + https://registry.opensource.zalan.do/v1/repositories/acid/patroni/tags This Dockerfile is meant in aiding development of Patroni and quick testing of features. It is not a production-worthy Dockerfile @@ -10,7 +10,7 @@ Dockerfile ## Standalone Patroni - docker run -d os-registry.stups.zalan.do/acid/patroni:1.0-SNAPSHOT + docker run -d registry.opensource.zalan.do/acid/patroni:1.0-SNAPSHOT ## Multiple Patroni's communicating with a standalone etcd inside Docker @@ -36,12 +36,12 @@ To automate this you can run the following script: Example session: - $ ./dev_patroni_cluster.sh --image os-registry.stups.zalan.do/acid/patroni:1.0-SNAPSHOT --members=2 --name=bravo + $ ./dev_patroni_cluster.sh --image registry.opensource.zalan.do/acid/patroni:1.0-SNAPSHOT --members=2 --name=bravo The etcd container is 6be871a11cb373406ca5ea1c6b39e1.0-SNAPSHOTfdde9fb1d6177212d6ad0c0d1bd9b563, ip=172.17.1.24 Started Patroni container 67e611f2eca7c40f9e6e0e24a4a8f2cba7e3e56d22a420e15ab9240a37a9d7a4, ip=172.17.1.25 Started Patroni container 47dd12ae635ab83b039f5889e250048b606ed5e48e3650b69e365e7e1d4acbcf, ip=172.17.1.26 $ docker ps CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES - 47dd12ae635a os-registry.stups.zalan.do/acid/patroni:1.0-SNAPSHOT "/bin/bash /entrypoi 10 seconds ago Up 8 seconds 4001/tcp, 5432/tcp, 2380/tcp bravo_OR64g8bx - 67e611f2eca7 os-registry.stups.zalan.do/acid/patroni:1.0-SNAPSHOT "/bin/bash /entrypoi 11 seconds ago Up 10 seconds 2380/tcp, 4001/tcp, 5432/tcp bravo_si9no8iz - 6be871a11cb3 os-registry.stups.zalan.do/acid/patroni:1.0-SNAPSHOT "/bin/bash /entrypoi 12 seconds ago Up 10 seconds 4001/tcp, 5432/tcp, 2380/tcp bravo_etcd + 47dd12ae635a registry.opensource.zalan.do/acid/patroni:1.0-SNAPSHOT "/bin/bash /entrypoi 10 seconds ago Up 8 seconds 4001/tcp, 5432/tcp, 2380/tcp bravo_OR64g8bx + 67e611f2eca7 registry.opensource.zalan.do/acid/patroni:1.0-SNAPSHOT "/bin/bash /entrypoi 11 seconds ago Up 10 seconds 2380/tcp, 4001/tcp, 5432/tcp bravo_si9no8iz + 6be871a11cb3 registry.opensource.zalan.do/acid/patroni:1.0-SNAPSHOT "/bin/bash /entrypoi 12 seconds ago Up 10 seconds 4001/tcp, 5432/tcp, 2380/tcp bravo_etcd From a6603e8b48647f28a57a45b38c904bb87f75ef47 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 8 Oct 2015 13:07:38 +0200 Subject: [PATCH 25/59] bugfix in zookeeper module: when master node was being attached to patroni/zookeeper (no cluster in zookeeper yet) patroni has never tried to "refetch" cluster from DCS. It was leeding to demote... --- patroni/zookeeper.py | 7 ++++--- tests/test_zookeeper.py | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index c7b9a44c..6f8ab981 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -165,8 +165,8 @@ class ZooKeeper(AbstractDCS): failover = Failover.from_node(failover[1].version, failover[0]) # get last leader operation - self.last_leader_operation = self.get_node(self.leader_optime_path) if self.fetch_cluster else None - self.last_leader_operation = 0 if self.last_leader_operation is None else int(self.last_leader_operation[0]) + optime = self.get_node(self.leader_optime_path) if self._OPTIME in nodes and self.fetch_cluster else None + self.last_leader_operation = 0 if optime is None else int(optime[0]) self._cluster = Cluster(initialize, leader, self.last_leader_operation, members, failover) def _load_cluster(self): @@ -280,5 +280,6 @@ class ZooKeeper(AbstractDCS): logger.exception("Unable to delete initialize key") def watch(self, timeout): - self.fetch_cluster = super(ZooKeeper, self).watch(timeout) + if super(ZooKeeper, self).watch(timeout): + self.fetch_cluster = True return self.fetch_cluster diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py index 731eb53f..84807270 100644 --- a/tests/test_zookeeper.py +++ b/tests/test_zookeeper.py @@ -172,5 +172,5 @@ class TestZooKeeper(unittest.TestCase): def test_watch(self): self.zk.watch(0) - self.zk.event.isSet = lambda: False + self.zk.event.isSet = lambda: True self.zk.watch(0) From cf6be5f58e72ba25ee6b904d90fce69a011fccf7 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 9 Oct 2015 16:02:34 +0200 Subject: [PATCH 26/59] add missing tests for async_executor --- tests/test_async_executor.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 tests/test_async_executor.py diff --git a/tests/test_async_executor.py b/tests/test_async_executor.py new file mode 100644 index 00000000..17d7f94a --- /dev/null +++ b/tests/test_async_executor.py @@ -0,0 +1,18 @@ +import unittest + +from mock import Mock, patch +from patroni.async_executor import AsyncExecutor +from threading import Thread + + +class TestAsyncExecutor(unittest.TestCase): + + def setUp(self): + self.a = AsyncExecutor() + + @patch.object(Thread, 'start', Mock()) + def test_run_async(self): + self.a.run_async(Mock(return_value=True)) + + def test_run(self): + self.a.run(Mock(side_effect=Exception())) From b629e0852f8f9d5a5072b24268e06f025c45a9d5 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 12 Oct 2015 08:34:08 +0200 Subject: [PATCH 27/59] Call pg_rewind in case of the master's unclean shutdown. If patroni detects the former master was killed, it runs it first in a single-user mode and then shuts down normally, to make sure pg_rewind will see a normal shut down status in pg_controldata. Add a flag need_rewind, since the point where it is detected that rewind might be necessary is moved out the code that runs rewind. --- patroni/ha.py | 11 ++- patroni/postgresql.py | 155 +++++++++++++++++++++++++++-------- tests/test_postgresql.py | 170 +++++++++++++++++++++++++++++++++++---- 3 files changed, 286 insertions(+), 50 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 0d95d372..41a2a56a 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -66,8 +66,15 @@ class Ha: if self.state_handler.is_healthy(): return False has_lock = self.has_lock() - self.state_handler.write_recovery_conf(None if has_lock else self.cluster.leader) - self.state_handler.start() + + # try to see if we are the former master that crashed. If so - we likely need to run pg_rewind + # in order to join the former standby being promoted. + pg_controldata = self.state_handler.controldata() + if not has_lock and pg_controldata.get('Database cluster state', '') == 'in production': # crashed master + self.state_handler.require_rewind() + + # XXX: should we call ha.follow_the_leader here instead? + ret = self.state_handler.follow_the_leader(None if has_lock else self.cluster.leader, recovery=True) if has_lock: logger.info('started as readonly because i had the session lock') self.load_cluster_from_dcs() diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 4fd0e13d..59ee4936 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -47,7 +47,7 @@ class Postgresql: self.replication = config['replication'] self.superuser = config['superuser'] self.admin = config['admin'] - self._pg_rewind = config.get('pg_rewind', {}) + self.pg_rewind = config.get('pg_rewind', {}) self.callback = config.get('callbacks', {}) self.use_slots = config.get('use_slots', True) self.schedule_load_slots = self.use_slots @@ -68,23 +68,36 @@ class Postgresql: self._connection = None self._cursor_holder = None + self._need_rewind = False self.members = [] # list of already existing replication slots self.retry = Retry(max_tries=-1, deadline=10, max_delay=1, retry_exceptions=PostgresConnectionException) - self.init_pg_rewind() - def init_pg_rewind(self): + @property + def can_rewind(self): + """ check if pg_rewind executable is there and that pg_controldata indicates + we have either wal_log_hints or checksums turned on + """ + # low-hanging fruit: check if pg_rewind configuration is there + if not self.pg_rewind or\ + not (self.pg_rewind.get('username', '') and self.pg_rewind.get('password', '')): + return False + + cmd = ['pg_rewind', '--help'] try: - self._pg_rewind_present = ('username' in self._pg_rewind and - subprocess.call(['pg_rewind', - '--version'], - stdout=open(os.devnull, 'w'), - stderr=subprocess.STDOUT) == 0) - if self._pg_rewind_present: - self._pg_rewind['user'] = self._pg_rewind['username'] - except: - self._pg_rewind_present = False - if self._pg_rewind and not self._pg_rewind_present: - logger.warning("pg_rewind support is disabled") + ret = subprocess.call(cmd, stdout=open(os.devnull, 'w'), stderr=subprocess.STDOUT) + if ret != 0: # pg_rewind is not there, close up the shop and go home + return False + except OSError: + return False + # check if the cluster's configuration permits pg_rewind + data = self.controldata() + if data: + return data.get('wal_log_hints setting', 'off') == 'on' or\ + data.get('Data page checksum version', '0') != '0' + return False + + def require_rewind(self): + self._need_rewind = True def get_local_address(self): listen_addresses = self.listen_addresses.split(',') @@ -209,6 +222,8 @@ class Postgresql: return ret def stop(self, mode='fast', block_callbacks=False): + if not self.is_running(): + return True if block_callbacks: try: self.query('SET statement_timeout TO 0') @@ -315,10 +330,11 @@ recovery_target_timeline = 'latest' for name, value in self.config.get('recovery_conf', {}).items(): f.write("{} = '{}'\n".format(name, value)) - def pg_rewind(self, leader): + def rewind(self, leader): # prepare pg_rewind connection r = parseurl(leader.conn_url) - r.update(self._pg_rewind) + r.update(self.pg_rewind) + r['user'] = r['username'] env = self.write_pgpass(r, append=True) pc = "user={user} host={host} port={port} dbname=postgres sslmode=prefer sslcompression=1".format(**r) logger.info("running pg_rewind from {}".format(pc)) @@ -331,30 +347,99 @@ recovery_target_timeline = 'latest' self.write_recovery_conf(leader) return ret - def pg_rewind_verify_cluster(self): - """ check that pg_rewind can be used with the cluster """ + def controldata(self): + """ return the contents of pg_controldata, or non-True value if pg_controldata call failed """ + result = None try: - return self.query("""SELECT bool_or(setting::boolean) - FROM pg_settings - WHERE name IN ( 'data_checksums', 'wal_log_hints')""").fetchone()[0] - except: - return False + data = subprocess.check_output(['pg_controldata', self.data_dir]) + if data: + data = data.splitlines() + result = {l.split(':')[0]: l.split(':')[1].strip() for l in data if l} + except subprocess.CalledProcessError: + logger.exception("Error when calling pg_controldata") + finally: + return result - def follow_the_leader(self, leader): - if not self.check_recovery_conf(leader): + def read_postmaster_opts(self): + """ returns the list of option names/values from postgres.opts, Empty dict if read failed or no file """ + result = {} + try: + with open(os.path.join(self.data_dir, "postmaster.opts")) as f: + data = f.read() + opts = [opt.strip('"\n') for opt in data.split(' "')] + for opt in opts: + if '=' in opt and opt.startswith('--'): + name, val = opt.split('=', 1) + name = name.strip('-') + result[name] = val + except IOError: + logger.exception('Error when reading postmaster.opts') + finally: + return result + + def single_user_mode(self, command=None, options={}): + """ run a given command in a single-user mode. If the command is empty - then just start and stop """ + cmd = ['postgres', '--single', '-D', self.data_dir] + for opt in sorted(options): + cmd.extend(['-c', '{0}={1}'.format(opt, options[opt])]) + # need a database name to connect + cmd.append('postgres') + p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=open(os.devnull, 'w'), stderr=subprocess.STDOUT) + if p: + command and p.communicate('{}\n'.format(command)) + p.stdin.close() + return p.wait() + return 1 + + def cleanup_archive_status(self): + status_dir = os.path.join(self.data_dir, 'pg_xlog', 'archive_status') + if os.path.isdir(status_dir): + for f in os.listdir(status_dir): + path = os.path.join(status_dir, f) + try: + if os.path.isfile(path): + os.remove(path) + elif os.path.islink(path): # should not happen, but just in case + os.unlink(path) + except: + logger.exception("Unable to remove {}".format(path)) + + def follow_the_leader(self, leader, recovery=False): + if not self.check_recovery_conf(leader) or recovery: + change_role = (self.role == 'master') + + self._need_rewind = (self._need_rewind or change_role) and self.can_rewind + if self._need_rewind: + logger.info("set the rewind flag after demote") self.write_recovery_conf(leader) - change_role = self.role == 'master' - - if leader and change_role and self._pg_rewind_present and self.pg_rewind_verify_cluster(): - self.stop() - if self.pg_rewind(leader): + if not leader or not self._need_rewind: # do not rewind until the leader becomes available + ret = self.restart() + else: # we have a leader and need to rewind + if self.is_running(): + self.stop() + # at present, pg_rewind only runs when the cluster is shut down cleanly + # and not shutdown in recovery. We have to remove the recovery.conf if present + # and start/shutdown in a single user mode to emulate this. + # XXX: if recovery.conf is linked, it will be written anew as a normal file. + if os.path.isfile(self.recovery_conf): + os.remove(self.recovery_conf) + else: + os.unlink(self.recovery_conf) + # Archived segments might be useful to pg_rewind, + # clean the flags that tell we should remove them. + self.cleanup_archive_status() + # Start in a single user mode and stop to produce a clean shutdown + opts = self.read_postmaster_opts() + opts['archive_mode'] = 'on' + opts['archive_command'] = 'false' + self.single_user_mode(options=opts) + if self.rewind(leader): ret = self.start() else: - ret = False + logger.error("unable to rewind the former master") self.remove_data_directory() - logger.error("unable to rewind the former leader") - else: - ret = self.restart() + ret = True + self._need_rewind = False change_role and ret and self.call_nowait(ACTION_ON_ROLE_CHANGE) def save_configuration_files(self): @@ -379,6 +464,8 @@ recovery_target_timeline = 'latest' ret = subprocess.call(self._pg_ctl + ['promote']) == 0 if ret: self._role = 'master' + logger.info("cleared rewind flag after becoming the leader") + self._need_rewind = False self.call_nowait(ACTION_ON_ROLE_CHANGE) return ret diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 8551e02e..96b1cbe4 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -1,8 +1,15 @@ +import mock # for the mock.call method, importing it without a namespace breaks python3 import os import psycopg2 import shutil import unittest +from sys import version_info +if version_info.major == 2: + import __builtin__ as builtins +else: + import builtins + from mock import Mock, MagicMock, patch from patroni.dcs import Cluster, Leader, Member from patroni.exceptions import PostgresException, PostgresConnectionException @@ -81,6 +88,68 @@ class MockConnect(Mock): return MockCursor(self) +def pg_controldata_string(*args, **kwargs): + return """ +pg_control version number: 942 +Catalog version number: 201509161 +Database system identifier: 6200971513092291716 +Database cluster state: shut down in recovery +pg_control last modified: Fri Oct 2 10:57:06 2015 +Latest checkpoint location: 0/30000C8 +Prior checkpoint location: 0/2000060 +Latest checkpoint's REDO location: 0/3000090 +Latest checkpoint's REDO WAL file: 000000020000000000000003 +Latest checkpoint's TimeLineID: 2 +Latest checkpoint's PrevTimeLineID: 2 +Latest checkpoint's full_page_writes: on +Latest checkpoint's NextXID: 0/943 +Latest checkpoint's NextOID: 24576 +Latest checkpoint's NextMultiXactId: 1 +Latest checkpoint's NextMultiOffset: 0 +Latest checkpoint's oldestXID: 931 +Latest checkpoint's oldestXID's DB: 1 +Latest checkpoint's oldestActiveXID: 943 +Latest checkpoint's oldestMultiXid: 1 +Latest checkpoint's oldestMulti's DB: 1 +Latest checkpoint's oldestCommitTs: 0 +Latest checkpoint's newestCommitTs: 0 +Time of latest checkpoint: Fri Oct 2 10:56:54 2015 +Fake LSN counter for unlogged rels: 0/1 +Minimum recovery ending location: 0/30241F8 +Min recovery ending loc's timeline: 2 +Backup start location: 0/0 +Backup end location: 0/0 +End-of-backup record required: no +wal_level setting: hot_standby +wal_log_hints setting: on +max_connections setting: 100 +max_worker_processes setting: 8 +max_prepared_xacts setting: 0 +max_locks_per_xact setting: 64 +track_commit_timestamp setting: off +Maximum data alignment: 8 +Database block size: 8192 +Blocks per segment of large relation: 131072 +WAL block size: 8192 +Bytes per WAL segment: 16777216 +Maximum length of identifiers: 64 +Maximum columns in an index: 32 +Maximum size of a TOAST chunk: 1996 +Size of a large-object chunk: 2048 +Date/time type storage: 64-bit integers +Float4 argument passing: by value +Float8 argument passing: by value +Data page checksum version: 0 +""" + + +def postmaster_opts_string(*args, **kwargs): + return '/usr/local/pgsql/bin/postgres "-D" "data/postgresql0" "--listen_addresses=127.0.0.1" "--port=5432"'\ + ' "--hot_standby=on" "--wal_keep_segments=8" "--wal_level=hot_standby" "--archive_command=mkdir -p ../wal_archive \n'\ + '&& cp %p ../wal_archive/%f" "--wal_log_hints=on" "--max_wal_senders=5" "--archive_timeout=1800s" "--archive_mode=on"'\ + ' "--max_replication_slots=5"\n' + + def psycopg2_connect(*args, **kwargs): return MockConnect() @@ -96,6 +165,7 @@ class TestPostgresql(unittest.TestCase): 'pg_hba': ['hostssl all all 0.0.0.0/0 md5', 'host all all 0.0.0.0/0 md5'], 'superuser': {'password': ''}, 'admin': {'username': 'admin', 'password': 'admin'}, + 'pg_rewind': {'username': 'admin', 'password': 'admin'}, 'replication': {'username': 'replicator', 'password': 'rep-pass', 'network': '127.0.0.1/32'}, @@ -133,32 +203,22 @@ class TestPostgresql(unittest.TestCase): def test_sync_from_leader(self): self.assertTrue(self.p.sync_from_leader(self.leader)) - @patch('os.system', side_effect=Exception("Test")) - def test_init_pg_rewind(self, mock_system): - self.p.init_pg_rewind() - # prepare parameters for pg_rewind - self.p._pg_rewind = {'username': 'foo'} - self.p.config['parameters']['data_checksums'] = 1 - os.system = mock_system - self.p.init_pg_rewind() - @patch('subprocess.call', side_effect=Exception("Test")) def test_pg_rewind(self, mock_call): - self.assertTrue(self.p.pg_rewind(self.leader)) + self.assertTrue(self.p.rewind(self.leader)) self.p subprocess.call = mock_call - self.assertFalse(self.p.pg_rewind(self.leader)) + self.assertFalse(self.p.rewind(self.leader)) - @patch('patroni.postgresql.Postgresql.pg_rewind', return_value=False) + @patch('patroni.postgresql.Postgresql.rewind', return_value=False) @patch('patroni.postgresql.Postgresql.remove_data_directory', MagicMock(return_value=True)) def test_follow_the_leader(self, mock_pg_rewind): self.p.demote(self.leader) self.p.follow_the_leader(None) - self.p._pg_rewind_present = True self.p.demote(self.leader) self.p.follow_the_leader(self.leader) self.p.follow_the_leader(Leader(-1, None, 28, self.other)) - self.p.pg_rewind = mock_pg_rewind + self.p.rewind = mock_pg_rewind self.p.follow_the_leader(self.leader) def test_create_replica(self): @@ -248,3 +308,85 @@ class TestPostgresql(unittest.TestCase): with patch('os.unlink', Mock(side_effect=Exception)): self.p.remove_data_directory() self.p.remove_data_directory() + + @patch('subprocess.check_output', MagicMock(return_value=0, side_effect=pg_controldata_string)) + @patch('subprocess.check_output', side_effect=subprocess.CalledProcessError) + @patch('subprocess.check_output', side_effect=Exception('Failed')) + def test_controldata(self, check_output_call_error, check_output_generic_exception): + data = self.p.controldata() + self.assertEquals(len(data), 50) + self.assertEquals(data['Database cluster state'], 'shut down in recovery') + self.assertEquals(data['wal_log_hints setting'], 'on') + self.assertEquals(int(data['Database block size']), 8192) + + subprocess.check_output = check_output_call_error + data = self.p.controldata() + self.assertIsNone(data) + + subprocess.check_output = check_output_generic_exception + self.assertRaises(Exception, self.p.controldata()) + + def test_read_postmaster_opts(self): + m = mock.mock_open(read_data=postmaster_opts_string()) + with patch.object(builtins, 'open', m): + data = self.p.read_postmaster_opts() + self.assertEquals(data['wal_level'], 'hot_standby') + self.assertEquals(int(data['max_replication_slots']), 5) + self.assertEqual(data.get('D'), None) + + m.side_effect = IOError("foo") + data = self.p.read_postmaster_opts() + self.assertEqual(data, dict()) + + m.side_effect = Exception("foo") + self.assertRaises(Exception, self.p.read_postmaster_opts()) + + @patch('subprocess.Popen') + @patch.object(builtins, 'open', MagicMock(return_value=42)) + def test_single_user_mode(self, subprocess_popen_mock): + subprocess_popen_mock.return_value.wait.return_value = 0 + self.assertEquals(self.p.single_user_mode(options=dict(archive_mode='on', archive_command='false')), 0) + subprocess_popen_mock.assert_called_once_with(['postgres', '--single', '-D', self.p.data_dir, + '-c', 'archive_command=false', '-c', 'archive_mode=on', + 'postgres'], stdin=subprocess.PIPE, + stdout=42, + stderr=subprocess.STDOUT) + subprocess_popen_mock.reset_mock() + self.assertEquals(self.p.single_user_mode(command="CHECKPOINT"), 0) + subprocess_popen_mock.assert_called_once_with(['postgres', '--single', '-D', self.p.data_dir, + 'postgres'], stdin=subprocess.PIPE, + stdout=42, + stderr=subprocess.STDOUT) + subprocess_popen_mock.return_value = None + self.assertEquals(self.p.single_user_mode(), 1) + + def fake_listdir(path): + if path.endswith(os.path.join('pg_xlog', 'archive_status')): + return ["a", "b", "c"] + return [] + + @patch('os.listdir', MagicMock(side_effect=fake_listdir)) + @patch('os.path.isdir', MagicMock(return_value=True)) + @patch('os.unlink', return_value=True) + @patch('os.remove', return_value=True) + @patch('os.path.islink', return_value=False) + @patch('os.path.isfile', return_value=True) + def test_cleanup_archive_status(self, mock_file, mock_link, mock_remove, mock_unlink): + ap = os.path.join(self.p.data_dir, 'pg_xlog', 'archive_status/') + self.p.cleanup_archive_status() + mock_remove.assert_has_calls([mock.call(ap+'a'), mock.call(ap+'b'), mock.call(ap+'c')]) + mock_unlink.assert_not_called() + + mock_remove.reset_mock() + mock_file.return_value = False + mock_link.return_value = True + self.p.cleanup_archive_status() + mock_unlink.assert_has_calls([mock.call(ap+'a'), mock.call(ap+'b'), mock.call(ap+'c')]) + mock_remove.assert_not_called() + + mock_unlink.reset_mock() + mock_remove.reset_mock() + mock_file.side_effect = Exception("foo") + self.p.cleanup_archive_status() + mock_unlink.assert_not_called() + mock_remove.assert_not_called() From ce7169f61df109427ec723519d24773bddc1b6fb Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 12 Oct 2015 15:29:47 +0200 Subject: [PATCH 28/59] Add new tests ha and postgresql. --- tests/test_ha.py | 1 + tests/test_postgresql.py | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_ha.py b/tests/test_ha.py index c55cbd90..38bb07a6 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -117,6 +117,7 @@ class TestHa(unittest.TestCase): self.assertEquals(self.ha.run_cycle(), 'started as a secondary') def test_recover_replica_failed(self): + self.p.controldata = lambda: {'Database cluster state': 'in production'} self.p.is_healthy = false self.p.follow_the_leader = false self.assertEquals(self.ha.run_cycle(), 'failed to start postgres') diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 0be4317e..060302c7 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -10,7 +10,7 @@ if version_info.major == 2: else: import builtins -from mock import Mock, MagicMock, patch +from mock import Mock, MagicMock, PropertyMock, patch from patroni.dcs import Cluster, Leader, Member from patroni.exceptions import PostgresException, PostgresConnectionException from patroni.postgresql import Postgresql @@ -216,7 +216,6 @@ class TestPostgresql(unittest.TestCase): @patch('subprocess.call', side_effect=Exception("Test")) def test_pg_rewind(self, mock_call): self.assertTrue(self.p.rewind(self.leader)) - self.p subprocess.call = mock_call self.assertFalse(self.p.rewind(self.leader)) @@ -230,6 +229,12 @@ class TestPostgresql(unittest.TestCase): self.p.follow_the_leader(Leader(-1, 28, self.other)) self.p.rewind = mock_pg_rewind self.p.follow_the_leader(self.leader) + self.p.require_rewind() + with mock.patch('patroni.postgresql.Postgresql.can_rewind', new_callable=PropertyMock(return_value=True)): + self.p.rewind.return_value = True + self.p.follow_the_leader(self.leader, recovery=True) + self.p.rewind.return_value = False + self.p.follow_the_leader(self.leader, recovery=True) def test_create_replica(self): self.p.delete_trigger_file = Mock(side_effect=OSError()) From d7988384d37a533bba8f945e60103d72a6de37a3 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 12 Oct 2015 16:24:02 +0200 Subject: [PATCH 29/59] Address the code review by Alex Kukushkin: - check the link before checking the file when deciding to remove it, as isfile follows symlinks and, therefore, may return True on them. - Remove append mode from write_pgpass, as it is always written anew before it is used. - make pg_controldata return an empty hash in case of an error, and check for the empty value return by this function before using it. some other minior fixed and test updates. --- patroni/ha.py | 3 ++- patroni/postgresql.py | 22 +++++++++++----------- tests/test_postgresql.py | 5 ++++- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 3bb75b78..0019253d 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -96,7 +96,8 @@ class Ha: # try to see if we are the former master that crashed. If so - we likely need to run pg_rewind # in order to join the former standby being promoted. pg_controldata = self.state_handler.controldata() - if not has_lock and pg_controldata.get('Database cluster state', '') == 'in production': # crashed master + if not has_lock and pg_controldata and\ + pg_controldata.get('Database cluster state', '') == 'in production': # crashed master self.state_handler.require_rewind() # XXX: follow the leader calls stop, which might take quite some time. diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 2051d819..3ab8b345 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -164,9 +164,9 @@ class Postgresql: def delete_trigger_file(self): os.path.exists(self.trigger_file) and os.unlink(self.trigger_file) - def write_pgpass(self, record, append=False): + def write_pgpass(self, record): pgpass = 'pgpass' - with open(pgpass, 'w' if not append else 'a') as f: + with open(pgpass, 'w') as f: os.fchmod(f.fileno(), 0o600) f.write('{host}:{port}:*:{user}:{password}\n'.format(**record)) env = os.environ.copy() @@ -363,7 +363,7 @@ recovery_target_timeline = 'latest' r = parseurl(leader.conn_url) r.update(self.pg_rewind) r['user'] = r['username'] - env = self.write_pgpass(r, append=True) + env = self.write_pgpass(r) pc = "user={user} host={host} port={port} dbname=postgres sslmode=prefer sslcompression=1".format(**r) logger.info("running pg_rewind from {}".format(pc)) pg_rewind = ['pg_rewind', '-D', self.data_dir, '--source-server', pc] @@ -377,7 +377,7 @@ recovery_target_timeline = 'latest' def controldata(self): """ return the contents of pg_controldata, or non-True value if pg_controldata call failed """ - result = None + result = {} try: data = subprocess.check_output(['pg_controldata', self.data_dir]) if data: @@ -425,10 +425,10 @@ recovery_target_timeline = 'latest' for f in os.listdir(status_dir): path = os.path.join(status_dir, f) try: - if os.path.isfile(path): - os.remove(path) - elif os.path.islink(path): # should not happen, but just in case + if os.path.islink(path): os.unlink(path) + elif os.path.isfile(path): + os.remove(path) except: logger.exception("Unable to remove {}".format(path)) @@ -449,10 +449,10 @@ recovery_target_timeline = 'latest' # and not shutdown in recovery. We have to remove the recovery.conf if present # and start/shutdown in a single user mode to emulate this. # XXX: if recovery.conf is linked, it will be written anew as a normal file. - if os.path.isfile(self.recovery_conf): - os.remove(self.recovery_conf) - else: + if os.path.islink(self.recovery_conf): os.unlink(self.recovery_conf) + else: + os.remove(self.recovery_conf) # Archived segments might be useful to pg_rewind, # clean the flags that tell we should remove them. self.cleanup_archive_status() @@ -494,7 +494,7 @@ recovery_target_timeline = 'latest' return True ret = subprocess.call(self._pg_ctl + ['promote']) == 0 if ret: - self._role = 'master' + self.set_role('master') logger.info("cleared rewind flag after becoming the leader") self._need_rewind = False self.call_nowait(ACTION_ON_ROLE_CHANGE) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 060302c7..53f41b98 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -337,7 +337,7 @@ class TestPostgresql(unittest.TestCase): subprocess.check_output = check_output_call_error data = self.p.controldata() - self.assertIsNone(data) + self.assertEquals(data, dict()) subprocess.check_output = check_output_generic_exception self.assertRaises(Exception, self.p.controldata()) @@ -394,6 +394,7 @@ class TestPostgresql(unittest.TestCase): mock_unlink.assert_not_called() mock_remove.reset_mock() + mock_file.return_value = False mock_link.return_value = True self.p.cleanup_archive_status() @@ -402,7 +403,9 @@ class TestPostgresql(unittest.TestCase): mock_unlink.reset_mock() mock_remove.reset_mock() + mock_file.side_effect = Exception("foo") + mock_link.side_effect = Exception("foo") self.p.cleanup_archive_status() mock_unlink.assert_not_called() mock_remove.assert_not_called() From 46f4788c28c9e0cd3052ba83273f0b704f5c1224 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 12 Oct 2015 17:06:13 +0200 Subject: [PATCH 30/59] Do not try to run postgres -D during unit tests. --- tests/test_postgresql.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 53f41b98..5677c638 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -221,6 +221,7 @@ class TestPostgresql(unittest.TestCase): @patch('patroni.postgresql.Postgresql.rewind', return_value=False) @patch('patroni.postgresql.Postgresql.remove_data_directory', MagicMock(return_value=True)) + @patch('patroni.postgresql.Postgresql.single_user_mode', MagicMock(return_value=1)) def test_follow_the_leader(self, mock_pg_rewind): self.p.demote() self.p.follow_the_leader(None) From 94aa6873f4b4e5eae460f6897d335559c07dc0f8 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Tue, 13 Oct 2015 08:19:44 +0200 Subject: [PATCH 31/59] Add more tests for the new postgresql methods. --- tests/test_postgresql.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 5677c638..2f1c75ac 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -237,6 +237,20 @@ class TestPostgresql(unittest.TestCase): self.p.rewind.return_value = False self.p.follow_the_leader(self.leader, recovery=True) + def test_can_rewind(self): + tmp = self.p.pg_rewind + self.p.pg_rewind = None + self.assertFalse(self.p.can_rewind) + self.p.pg_rewind = tmp + with mock.patch('subprocess.call', MagicMock(return_value=1)): + self.assertFalse(self.p.can_rewind) + with mock.patch('subprocess.call', side_effect=OSError("foo")): + self.assertFalse(self.p.can_rewind) + tmp = self.p.controldata() + self.p.controldata = lambda: {'wal_log_hints setting': 'on'} + self.assertTrue(self.p.can_rewind) + self.p.controldata = tmp + def test_create_replica(self): self.p.delete_trigger_file = Mock(side_effect=OSError()) self.assertEquals(self.p.create_replica({'host': '', 'port': '', 'user': ''}, ''), 1) From 101082fa3b0ef02a4369b1c373f99a2e3ad9b6f0 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Tue, 13 Oct 2015 09:08:27 +0200 Subject: [PATCH 32/59] more tests. --- patroni/postgresql.py | 5 +---- tests/test_postgresql.py | 9 +++++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 3ab8b345..fc7956e9 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -340,10 +340,7 @@ class Postgresql: with open(self.recovery_conf, 'r') as f: for line in f: if line.startswith('primary_conninfo'): - if not pattern: - return False - return pattern in line - + return pattern and (pattern in line) return not pattern def write_recovery_conf(self, leader): diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 2f1c75ac..e5e2dfd4 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -10,7 +10,7 @@ if version_info.major == 2: else: import builtins -from mock import Mock, MagicMock, PropertyMock, patch +from mock import Mock, MagicMock, PropertyMock, patch, mock_open from patroni.dcs import Cluster, Leader, Member from patroni.exceptions import PostgresException, PostgresConnectionException from patroni.postgresql import Postgresql @@ -231,6 +231,11 @@ class TestPostgresql(unittest.TestCase): self.p.rewind = mock_pg_rewind self.p.follow_the_leader(self.leader) self.p.require_rewind() + with mock.patch('os.path.islink', MagicMock(return_value=True)): + with mock.patch('os.unlink', MagicMock(return_value=True)): + with mock.patch('patroni.postgresql.Postgresql.can_rewind', new_callable=PropertyMock(return_value=True)): + self.p.follow_the_leader(self.leader, recovery=True) + self.p.require_rewind() with mock.patch('patroni.postgresql.Postgresql.can_rewind', new_callable=PropertyMock(return_value=True)): self.p.rewind.return_value = True self.p.follow_the_leader(self.leader, recovery=True) @@ -358,7 +363,7 @@ class TestPostgresql(unittest.TestCase): self.assertRaises(Exception, self.p.controldata()) def test_read_postmaster_opts(self): - m = mock.mock_open(read_data=postmaster_opts_string()) + m = mock_open(read_data=postmaster_opts_string()) with patch.object(builtins, 'open', m): data = self.p.read_postmaster_opts() self.assertEquals(data['wal_level'], 'hot_standby') From 953ea749bf902e91ea6f957d6fd68ec8fd580dea Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Tue, 13 Oct 2015 15:00:16 +0200 Subject: [PATCH 33/59] Make sure patroni is not using stale connections. After the PostgreSQL crash (i.e. with kill -9), the backend patroni connects to may still exist. In this case, patroni will get stale postgres role from this backend, preventing a restarted node with a leader lock from being promoted. Easily reproducible and also observed in a staging environment after the postgres crash due to out of disk space. --- patroni/postgresql.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index fc7956e9..a7f95a81 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -127,9 +127,15 @@ class Postgresql: def _cursor(self): if not self._cursor_holder or self._cursor_holder.closed or self._cursor_holder.connection.closed != 0: + logger.info("established a new patroni connection to the postgres cluster") self._cursor_holder = self.connection().cursor() return self._cursor_holder + def close_connection(self): + if self._cursor_holder and self._cursor_holder.connection and self._cursor_holder.connection.closed == 0: + self._cursor_holder.connection.close() + logger.info("closed patroni connection to the postgresql cluster") + def _query(self, sql, *params): cursor = None try: @@ -269,6 +275,12 @@ class Postgresql: logging.exception('Exception during CHECKPOINT') def stop(self, mode='fast', block_callbacks=False): + # make sure we close all connections established against + # the former node, otherwise, we might get a stalled one + # after kill -9, which would report incorrect data to + # patroni. + + self.close_connection() if not self.is_running(): if not block_callbacks: self.set_state('stopped') From c7246e48d9a4986d603bfba7413711ec54b5480c Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 14 Oct 2015 09:46:20 +0200 Subject: [PATCH 34/59] Work around the differences in pg_controldata names. --- patroni/postgresql.py | 2 +- tests/test_postgresql.py | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index fc7956e9..263b58ce 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -379,7 +379,7 @@ recovery_target_timeline = 'latest' data = subprocess.check_output(['pg_controldata', self.data_dir]) if data: data = data.splitlines() - result = {l.split(':')[0]: l.split(':')[1].strip() for l in data if l} + result = {l.split(':')[0].replace('Current ', '', 1): l.split(':')[1].strip() for l in data if l} except subprocess.CalledProcessError: logger.exception("Error when calling pg_controldata") finally: diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index e5e2dfd4..b402626a 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -118,12 +118,12 @@ Backup start location: 0/0 Backup end location: 0/0 End-of-backup record required: no wal_level setting: hot_standby -wal_log_hints setting: on -max_connections setting: 100 -max_worker_processes setting: 8 -max_prepared_xacts setting: 0 -max_locks_per_xact setting: 64 -track_commit_timestamp setting: off +Current wal_log_hints setting: on +Current max_connections setting: 100 +Current max_worker_processes setting: 8 +Current max_prepared_xacts setting: 0 +Current max_locks_per_xact setting: 64 +Current track_commit_timestamp setting: off Maximum data alignment: 8 Database block size: 8192 Blocks per segment of large relation: 131072 From 98b59354a9f4d72ec7bd9e668d67ea683a892e06 Mon Sep 17 00:00:00 2001 From: Feike Steenbergen Date: Wed, 14 Oct 2015 14:37:05 +0200 Subject: [PATCH 35/59] Exclude more files from git. --- .gitignore | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index df367db5..699794c2 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,11 @@ data/* *.pyc -helpers/*.pyc +*.egg/ +*.egg-info/ +.cache/ +.coverage +.eggs/ +build/ +coverage.xml +junit.xml +pgpass From 5c86b60cd2a96033c3475fd14e6207f0f5fea41a Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 14 Oct 2015 17:05:09 +0200 Subject: [PATCH 36/59] Fix an exception in the (rather unusual) case of attaching Patroni to an existing running replica. --- patroni/postgresql.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 263b58ce..5a857c1c 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -316,7 +316,8 @@ class Postgresql: return True def check_replication_lag(self, last_leader_operation): - return last_leader_operation - self.xlog_position() <= self.config.get('maximum_lag_on_failover', 0) + return (last_leader_operation if last_leader_operation else 0) - self.xlog_position() <=\ + self.config.get('maximum_lag_on_failover', 0) def write_pg_hba(self): with open(os.path.join(self.data_dir, 'pg_hba.conf'), 'a') as f: From 2f0cf1db06269d05c8ad30f865c8f788078bf16c Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 15 Oct 2015 09:08:16 +0200 Subject: [PATCH 37/59] Mock etcd client delete method --- tests/test_ha.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_ha.py b/tests/test_ha.py index 38bb07a6..a5a816da 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -1,3 +1,4 @@ +import etcd import unittest from mock import Mock, patch @@ -98,6 +99,7 @@ class TestHa(unittest.TestCase): self.e = Etcd('foo', {'ttl': 30, 'host': 'ok:2379', 'scope': 'test'}) self.e.client.read = etcd_read self.e.client.write = etcd_write + self.e.client.delete = Mock(side_effect=etcd.EtcdException()) self.ha = Ha(MockPatroni(self.p, self.e)) self.ha._async_executor.run_async = run_async self.ha.old_cluster = self.e.get_cluster() From 16a0a3481db76cda73207f78e19905b17ca3f4f2 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 15 Oct 2015 09:08:33 +0200 Subject: [PATCH 38/59] fix pep8 formatting --- tests/test_postgresql.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index b402626a..9a02ece6 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -232,8 +232,8 @@ class TestPostgresql(unittest.TestCase): self.p.follow_the_leader(self.leader) self.p.require_rewind() with mock.patch('os.path.islink', MagicMock(return_value=True)): - with mock.patch('os.unlink', MagicMock(return_value=True)): - with mock.patch('patroni.postgresql.Postgresql.can_rewind', new_callable=PropertyMock(return_value=True)): + with mock.patch('patroni.postgresql.Postgresql.can_rewind', new_callable=PropertyMock(return_value=True)): + with mock.patch('os.unlink', MagicMock(return_value=True)): self.p.follow_the_leader(self.leader, recovery=True) self.p.require_rewind() with mock.patch('patroni.postgresql.Postgresql.can_rewind', new_callable=PropertyMock(return_value=True)): From f35d1098102f484846f7eb10a15678e61646e822 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 15 Oct 2015 16:17:11 +0200 Subject: [PATCH 39/59] Bugfix: do not try to double encode data --- patroni/zookeeper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index 6f8ab981..9a2ee8bb 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -198,7 +198,7 @@ class ZooKeeper(AbstractDCS): self.client.retry(self.client.set, self.failover_path, value.encode('utf-8'), version=index or -1) return True except NoNodeError: - return value == '' or (not index and self._create(self.failover_path, value.encode('utf-8'))) + return value == '' or (not index and self._create(self.failover_path, value)) except: logging.exception('set_failover_value') return False From 3ed82ae22c5f3ec7c22ad944338e4ea5c09ace3e Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 15 Oct 2015 16:18:28 +0200 Subject: [PATCH 40/59] Manual failover via rest api curl -XPOST --data '{"leader": "leader_name", "member": "member_name"}' http://127.0.0.1:8008/failover It will execute some preliminary checks and write failover key into DCS. Afterward it will wait until new leader key will appear in a DCS. It's better to execute this request on the master node. It will send a signal to the main HA loop which makes possible to release leader key immidiately even if you are working with etcd. --- patroni/api.py | 49 +++++++++++++++++++++++++++++++++++++++++++++++ tests/test_api.py | 26 +++++++++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/patroni/api.py b/patroni/api.py index dc83249f..f673d1c0 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -3,6 +3,7 @@ import fcntl import json import logging import psycopg2 +import time from patroni.exceptions import PostgresConnectionException from patroni.utils import Retry, RetryFailedError @@ -121,6 +122,54 @@ class RestApiHandler(BaseHTTPRequestHandler): self.end_headers() self.wfile.write(data) + def poll_failover_result(self, leader, member): + for a in range(0, 15): + time.sleep(1) + try: + cluster = self.server.patroni.dcs.get_cluster() + if cluster.leader and cluster.leader.name != leader: + return 200, ('Successfully failed over to ' + cluster.leader.name).encode('utf-8') + except: + pass + return 503, b'Failover failed' + + def is_failover_possible(self, cluster, leader, member): + if leader and not cluster.leader or cluster.leader.name != leader: + return b'leader name does not match' + if member: + members = [m for m in cluster.members if m.name == member] + if not members: + return b'member does not exists' + else: + members = [m for m in cluster.members if m.name != cluster.leader.name and m.api_url] + if not members: + return b'failover is not possible: cluster does not have members except leader' + for member, reachable, in_recovery, xlog_location in self.server.patroni.ha.fetch_nodes_statuses(members): + if reachable: + return None + return b'failover is not possible: no good candidates have been found' + + @check_auth + def do_POST_failover(self): + content_length = int(self.headers.get('content-length', 0)) + request = json.loads(self.rfile.read(content_length).decode('utf-8')) + leader = request.get('leader', None) + member = request.get('member', None) + cluster = self.server.patroni.ha.dcs.get_cluster() + status_code = 503 + data = self.is_failover_possible(cluster, leader, member) + if not data: + if not self.server.patroni.dcs.manual_failover(leader, member): + data = b'failed to write failover key into DCS' + else: + self.server.patroni.dcs.event.set() + status_code, data = self.poll_failover_result(cluster.leader and cluster.leader.name, member) + + self.send_response(status_code) + self.send_header('Content-Type', 'text/html') + self.end_headers() + self.wfile.write(data) + def parse_request(self): """Override parse_request method to enrich basic functionality of `BaseHTTPRequestHandler` class diff --git a/tests/test_api.py b/tests/test_api.py index e4b8f87e..aa4608c4 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -3,6 +3,7 @@ import unittest from mock import Mock, patch from patroni.api import RestApiHandler, RestApiServer +from patroni.dcs import Member from six import BytesIO as IO from six.moves import BaseHTTPServer from test_postgresql import psycopg2_connect, MockCursor @@ -38,6 +39,9 @@ class MockHa(Mock): def restart_scheduled(self): return False + def fetch_nodes_statuses(self, members): + return [[None, True, None, None]] + class MockPatroni: @@ -117,3 +121,25 @@ class TestRestApiHandler(unittest.TestCase): MockRestApiServer(RestApiHandler, b'GET /patroni') with patch.object(MockPostgresql, 'connection', Mock(side_effect=psycopg2.OperationalError)): MockRestApiServer(RestApiHandler, b'GET /patroni') + + @patch('time.sleep', Mock()) + @patch.object(MockHa, 'dcs') + def test_do_POST_failover(self, dcs): + cluster = dcs.get_cluster.return_value + request = b'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\n' +\ + b'Content-Length: 25\n\n{"leader": "postgresql1"}' + MockRestApiServer(RestApiHandler, request) + cluster.leader.name = 'postgresql1' + MockRestApiServer(RestApiHandler, request) + cluster.members = [Member(0, 'postgresql0', 30, {'api_url': 'http'})] + MockRestApiServer(RestApiHandler, request) + with patch.object(MockPatroni, 'dcs') as d: + d.get_cluster = Mock(side_effect=Exception()) + MockRestApiServer(RestApiHandler, request) + d.manual_failover.return_value = False + MockRestApiServer(RestApiHandler, request) + with patch.object(MockHa, 'fetch_nodes_statuses', Mock(return_value=[])): + MockRestApiServer(RestApiHandler, request) + request = b'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\n' +\ + b'Content-Length: 50\n\n{"leader": "postgresql1", "member": "postgresql2"}' + MockRestApiServer(RestApiHandler, request) From 921e4fc32357d7b7fdecc57b76c5a7eb84aa6e5e Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 16 Oct 2015 10:28:15 +0200 Subject: [PATCH 41/59] psycopg2 should be not older than 2.6.1 --- requirements-py2.txt | 2 +- requirements-py3.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements-py2.txt b/requirements-py2.txt index d57df720..fde9c79a 100644 --- a/requirements-py2.txt +++ b/requirements-py2.txt @@ -1,7 +1,7 @@ boto dnspython mock -psycopg2 +psycopg2>=2.6.1 PyYAML requests six >= 1.7 diff --git a/requirements-py3.txt b/requirements-py3.txt index 0fd9dfb3..cc00965b 100644 --- a/requirements-py3.txt +++ b/requirements-py3.txt @@ -1,7 +1,7 @@ boto mock dnspython3 -psycopg2 +psycopg2>=2.6.1 PyYAML requests six From a844920489425496c4553fd174df6472e028859b Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Fri, 16 Oct 2015 16:14:45 +0200 Subject: [PATCH 42/59] Store the cluster sysid in the initialize flag. Make sure that the new PostgreSQL node will only join the cluster if its sysid matches the one stored in DCS. --- patroni/dcs.py | 5 ++++- patroni/etcd.py | 7 ++++--- patroni/ha.py | 20 +++++++++++++++----- patroni/postgresql.py | 8 ++++++++ patroni/zookeeper.py | 7 ++++--- 5 files changed, 35 insertions(+), 12 deletions(-) diff --git a/patroni/dcs.py b/patroni/dcs.py index 25e44cd1..48d49cf5 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -240,8 +240,11 @@ class AbstractDCS: overwriting the key if necessary.""" @abc.abstractmethod - def initialize(self): + def initialize(self, create_new=True, sysid=None): """Race for cluster initialization. + + :param create_new: False if the key should already exist (in the case we are setting the system_id) + :param sysid: PostgreSQL cluster system identifier, if specified, is written to the key :returns: `!True` if key has been created successfully. this method should create atomically initialize key and return `!True` diff --git a/patroni/etcd.py b/patroni/etcd.py index 79379700..c20d73ab 100644 --- a/patroni/etcd.py +++ b/patroni/etcd.py @@ -177,7 +177,8 @@ class Etcd(AbstractDCS): nodes = {os.path.relpath(node.key, result.key): node for node in result.leaves} # get initialize flag - initialize = bool(nodes.get(self._INITIALIZE, False)) + initialize = nodes.get(self._INITIALIZE, None) + initialize = initialize and initialize.value # get last leader operation last_leader_operation = nodes.get(self._LEADER_OPTIME, None) @@ -235,8 +236,8 @@ class Etcd(AbstractDCS): return self.retry(self.client.test_and_set, self.leader_path, self._name, self._name, self.ttl) @catch_etcd_errors - def initialize(self): - return self.retry(self.client.write, self.initialize_path, self._name, prevExist=False) + def initialize(self, create_new=True, sysid=None): + return self.retry(self.client.write, self.initialize_path, sysid or "", prevExist=(not create_new)) @catch_etcd_errors def delete_leader(self): diff --git a/patroni/ha.py b/patroni/ha.py index 0019253d..82921a74 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -73,9 +73,10 @@ class Ha: self._async_executor.run_async(self.copy_backup_from_leader, args=(self.cluster.leader, )) return 'trying to bootstrap from leader' elif not self.cluster.initialize: # no initialize key - if self.dcs.initialize(): # race for initialization + if self.dcs.initialize(create_new=True): # race for initialization try: self.state_handler.bootstrap() + self.dcs.initialize(create_new=False, sysid=self.state_handler.sysid) except: # initdb or start failed # remove initialization key and give a chance to other members logger.info("removing initialize key after failed attempt to initialize the cluster") @@ -350,6 +351,11 @@ class Ha: else: return self._async_executor.scheduled_action + ' in progress' + def sysid_valid(self, sysid): + # sysid does tv_sec << 32, where tv_sec is the number of seconds sine 1970, + # so even 1 << 32 would have 10 digits. + return str(sysid) and len(str(sysid)) >= 10 and str(sysid).isdigit() + def _run_cycle(self): try: self.load_cluster_from_dcs() @@ -357,8 +363,8 @@ class Ha: self.touch_member() # cluster has leader key but not initialize key - if not self.cluster.is_unlocked() and not self.cluster.initialize: - self.dcs.initialize() # fix it + if not self.cluster.is_unlocked() and not self.sysid_valid(self.cluster.initialize) and self.has_lock(): + self.dcs.initialize(create_new=(self.cluster.initialize is None), sysid=self.state_handler.sysid) if self._async_executor.busy: return self.handle_long_action_in_progress() @@ -372,8 +378,12 @@ class Ha: if self.state_handler.data_directory_empty(): return self.bootstrap() # new node # "bootstrap", but data directory is not empty - elif not self.cluster.initialize and self.cluster.is_unlocked(): - self.dcs.initialize() + elif not self.sysid_valid(self.cluster.initialize) and self.cluster.is_unlocked(): + self.dcs.initialize(create_new=(self.cluster.initialize is None), sysid=self.state_handler.sysid) + else: + # check if we are allowed to join + if self.sysid_valid(self.cluster.initialize) and self.cluster.initialize != self.state_handler.sysid: + return "system ID mismatch, node {0} belongs to a different cluster".format(self.state_handler.name) # try to start dead postgres if not self.state_handler.is_healthy(): diff --git a/patroni/postgresql.py b/patroni/postgresql.py index fc7956e9..07f62fdf 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -69,6 +69,7 @@ class Postgresql: self._connection = None self._cursor_holder = None self._need_rewind = False + self._sysid = None self.replication_slots = [] # list of already existing replication slots self.retry = Retry(max_tries=-1, deadline=5, max_delay=1, retry_exceptions=PostgresConnectionException) @@ -105,6 +106,13 @@ class Postgresql: data.get('Data page checksum version', '0') != '0' return False + @property + def sysid(self): + if not self._sysid: + data = self.controldata() + self._sysid = data and data.get('Database system identifier', None) + return self._sysid + def require_rewind(self): self._need_rewind = True diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index 6f8ab981..d3e4fd57 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -139,7 +139,7 @@ class ZooKeeper(AbstractDCS): self.fetch_cluster = True # get initialize flag - initialize = self._INITIALIZE in nodes + initialize = self.get_node(self._INITIALIZE)[0] if self._INITIALIZE in nodes else None # get list of members members = self.load_members() if self._MEMBERS[:-1] in nodes else [] @@ -203,8 +203,9 @@ class ZooKeeper(AbstractDCS): logging.exception('set_failover_value') return False - def initialize(self): - return self._create(self.initialize_path, self._name, makepath=True) + def initialize(self, create_new=True, sysid=None): + return self._create(self.initialize_path, sysid if sysid else "", makepath=True) if create_new \ + else self.client.retry(self.client.set, self.initialize_path, sysid.encode("utf-8") if sysid else "") def touch_member(self, data, ttl=None): cluster = self.cluster From 83662f71cba6c3af3fe7e6bdcbf1346585a3fc24 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Fri, 16 Oct 2015 16:38:05 +0200 Subject: [PATCH 43/59] Exit right away if the node sysid is different from the cluster's one --- patroni/ha.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/patroni/ha.py b/patroni/ha.py index 82921a74..283d3678 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -2,6 +2,7 @@ import json import logging import psycopg2 import requests +import sys from patroni.async_executor import AsyncExecutor from patroni.exceptions import DCSError, PostgresConnectionException @@ -383,7 +384,8 @@ class Ha: else: # check if we are allowed to join if self.sysid_valid(self.cluster.initialize) and self.cluster.initialize != self.state_handler.sysid: - return "system ID mismatch, node {0} belongs to a different cluster".format(self.state_handler.name) + logger.fatal("system ID mismatch, node {0} belongs to a different cluster".format(self.state_handler.name)) + sys.exit(1) # try to start dead postgres if not self.state_handler.is_healthy(): From a10b7248a6e92956b5d3201ae744f9f038bab60c Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 19 Oct 2015 09:19:25 +0200 Subject: [PATCH 44/59] Fix a flake8 warning --- patroni/ha.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/patroni/ha.py b/patroni/ha.py index 283d3678..8ed18b16 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -384,7 +384,8 @@ class Ha: else: # check if we are allowed to join if self.sysid_valid(self.cluster.initialize) and self.cluster.initialize != self.state_handler.sysid: - logger.fatal("system ID mismatch, node {0} belongs to a different cluster".format(self.state_handler.name)) + logger.fatal("system ID mismatch, node {0} belongs to a different cluster". + format(self.state_handler.name)) sys.exit(1) # try to start dead postgres From 4e448015f3fda0c80a5e59dde38633a82ca880d1 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 19 Oct 2015 10:13:14 +0200 Subject: [PATCH 45/59] Increase the test coverage. --- tests/test_ha.py | 8 +++++++- tests/test_postgresql.py | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/test_ha.py b/tests/test_ha.py index a5a816da..e34f9b8e 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -1,7 +1,7 @@ import etcd import unittest -from mock import Mock, patch +from mock import Mock, MagicMock, patch from patroni.dcs import Cluster, Failover, Leader, Member from patroni.etcd import Client, Etcd from patroni.exceptions import DCSError, PostgresException @@ -130,6 +130,12 @@ class TestHa(unittest.TestCase): self.ha.has_lock = true self.assertEquals(self.ha.run_cycle(), 'removed leader key after trying and failing to start postgres') + @patch('sys.exit', return_value=1) + @patch('patroni.ha.Ha.sysid_valid', MagicMock(return_value=True)) + def test_sysid_no_match(self, exit_mock): + self.ha.run_cycle() + exit_mock.assert_called_once_with(1) + @patch.object(Cluster, 'is_unlocked', Mock(return_value=False)) def test_start_as_readonly(self): self.p.is_leader = self.p.is_healthy = false diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 9a02ece6..544f8afd 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -429,3 +429,7 @@ class TestPostgresql(unittest.TestCase): self.p.cleanup_archive_status() mock_unlink.assert_not_called() mock_remove.assert_not_called() + + @patch('subprocess.check_output', MagicMock(return_value=0, side_effect=pg_controldata_string)) + def test_sysid(self): + self.assertEqual(self.p.sysid, "6200971513092291716") From 18eebdadaa7ef40613d129981e3c9c532d3ef25c Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 19 Oct 2015 15:00:06 +0200 Subject: [PATCH 46/59] Watch for change of failover key. If the value is empty and leader didn't changed, this probably means that failover failed. After 15 seconds timeout we will consider failover status = unknown --- patroni/api.py | 4 +++- tests/test_api.py | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/patroni/api.py b/patroni/api.py index f673d1c0..1fa5ca62 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -129,9 +129,11 @@ class RestApiHandler(BaseHTTPRequestHandler): cluster = self.server.patroni.dcs.get_cluster() if cluster.leader and cluster.leader.name != leader: return 200, ('Successfully failed over to ' + cluster.leader.name).encode('utf-8') + if not cluster.failover: + return 503, b'Failover failed' except: pass - return 503, b'Failover failed' + return 503, b'Failover status unknown' def is_failover_possible(self, cluster, leader, member): if leader and not cluster.leader or cluster.leader.name != leader: diff --git a/tests/test_api.py b/tests/test_api.py index aa4608c4..ae25964d 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -134,6 +134,10 @@ class TestRestApiHandler(unittest.TestCase): cluster.members = [Member(0, 'postgresql0', 30, {'api_url': 'http'})] MockRestApiServer(RestApiHandler, request) with patch.object(MockPatroni, 'dcs') as d: + cluster = d.get_cluster.return_value + cluster.leader.name = 'postgresql1' + cluster.failover = None + MockRestApiServer(RestApiHandler, request) d.get_cluster = Mock(side_effect=Exception()) MockRestApiServer(RestApiHandler, request) d.manual_failover.return_value = False From 8f606e4ff9a85f6f6d76db744a48880ec940f827 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 19 Oct 2015 15:13:24 +0200 Subject: [PATCH 47/59] Add a missing call to restore_configuration_files. I accidentially removed the call when moving the backup functions to the external script. It is intended to save the configuration, so that at the restore phase one can just copy backup files. Its primary intention was to save configuration files in the WAL-E case (WAL-E just omits everything with .conf), but it is also useful in the pg_basebackup case, which omits all symlinks, leaving the cluster with .conf files symlinked in the broken state. --- patroni/postgresql.py | 17 +++++++++++------ tests/test_postgresql.py | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index f75324e7..b4e8ff94 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -485,19 +485,23 @@ recovery_target_timeline = 'latest' def save_configuration_files(self): """ - copy postgresql.conf to postgresql.conf.backup to preserve it in the WAL-e backup. - see http://comments.gmane.org/gmane.comp.db.postgresql.wal-e/239 + copy postgresql.conf to postgresql.conf.backup to be able to retrive configuration files + - originally stored as symlinks, those are normally skipped by pg_basebackup + - in case of WAL-E basebackup (see http://comments.gmane.org/gmane.comp.db.postgresql.wal-e/239) """ - for f in self.configuration_to_save: - shutil.copy(f, f + '.backup') + try: + for f in self.configuration_to_save: + os.path.isfile(f) and shutil.copy(f, f + '.backup') + except: + logger.exception('unable to create backup copies of configuration files') def restore_configuration_files(self): """ restore a previously saved postgresql.conf """ try: for f in self.configuration_to_save: - shutil.copy(f + '.backup', f) + not os.path.isfile(f) and os.path.isfile(f+'.backup') and shutil.copy(f + '.backup', f) except: - logger.exception('unable to restore configuration from WAL-E backup') + logger.exception('unable to restore configuration files from backup') def promote(self): if self.role == 'master': @@ -585,6 +589,7 @@ recovery_target_timeline = 'latest' raise PostgresException("Could not bootstrap master PostgreSQL") else: if self.sync_from_leader(current_leader): + self.restore_configuration_files() self.write_recovery_conf(current_leader) ret = self.start() return ret diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 9a02ece6..ca60760f 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -19,6 +19,11 @@ from test_ha import false import subprocess +def is_file_raise_on_backup(*args, **kwargs): + if args[0].endswith('.backup'): + raise Exception("foo") + + class MockCursor: def __init__(self, connection): @@ -429,3 +434,15 @@ class TestPostgresql(unittest.TestCase): self.p.cleanup_archive_status() mock_unlink.assert_not_called() mock_remove.assert_not_called() + + @patch('os.path.isfile', MagicMock(return_value=True)) + @patch('shutil.copy', side_effect=Exception) + def test_save_configuration_files(self, mock_copy): + shutil.copy = mock_copy + self.p.save_configuration_files() + + @patch('os.path.isfile', MagicMock(side_effect=is_file_raise_on_backup)) + @patch('shutil.copy', side_effect=Exception) + def test_restore_configuration_files(self, mock_copy): + shutil.copy = mock_copy + self.p.restore_configuration_files() From 90c738d83a4897f1292d38c59e99a4e80945b578 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 19 Oct 2015 16:03:21 +0200 Subject: [PATCH 48/59] Address the code review by Alex. --- patroni/etcd.py | 4 ++-- patroni/postgresql.py | 9 +++------ patroni/zookeeper.py | 8 ++++---- tests/test_postgresql.py | 2 +- 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/patroni/etcd.py b/patroni/etcd.py index c20d73ab..93ef2a2c 100644 --- a/patroni/etcd.py +++ b/patroni/etcd.py @@ -236,8 +236,8 @@ class Etcd(AbstractDCS): return self.retry(self.client.test_and_set, self.leader_path, self._name, self._name, self.ttl) @catch_etcd_errors - def initialize(self, create_new=True, sysid=None): - return self.retry(self.client.write, self.initialize_path, sysid or "", prevExist=(not create_new)) + def initialize(self, create_new=True, sysid=""): + return self.retry(self.client.write, self.initialize_path, sysid, prevExist=(not create_new)) @catch_etcd_errors def delete_leader(self): diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 165057e6..9ac0d69a 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -101,16 +101,13 @@ class Postgresql: return False # check if the cluster's configuration permits pg_rewind data = self.controldata() - if data: - return data.get('wal_log_hints setting', 'off') == 'on' or\ - data.get('Data page checksum version', '0') != '0' - return False + return data.get('wal_log_hints setting', 'off') == 'on' or data.get('Data page checksum version', '0') != '0' @property def sysid(self): if not self._sysid: data = self.controldata() - self._sysid = data and data.get('Database system identifier', None) + self._sysid = data.get('Database system identifier', "") return self._sysid def require_rewind(self): @@ -399,7 +396,7 @@ recovery_target_timeline = 'latest' try: data = subprocess.check_output(['pg_controldata', self.data_dir]) if data: - data = data.splitlines() + data = data.decode().splitlines() result = {l.split(':')[0].replace('Current ', '', 1): l.split(':')[1].strip() for l in data if l} except subprocess.CalledProcessError: logger.exception("Error when calling pg_controldata") diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index d3e4fd57..ba31e756 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -139,7 +139,7 @@ class ZooKeeper(AbstractDCS): self.fetch_cluster = True # get initialize flag - initialize = self.get_node(self._INITIALIZE)[0] if self._INITIALIZE in nodes else None + initialize = self.get_node(self.initialize_path)[0] if self._INITIALIZE in nodes else None # get list of members members = self.load_members() if self._MEMBERS[:-1] in nodes else [] @@ -203,9 +203,9 @@ class ZooKeeper(AbstractDCS): logging.exception('set_failover_value') return False - def initialize(self, create_new=True, sysid=None): - return self._create(self.initialize_path, sysid if sysid else "", makepath=True) if create_new \ - else self.client.retry(self.client.set, self.initialize_path, sysid.encode("utf-8") if sysid else "") + def initialize(self, create_new=True, sysid=""): + return self._create(self.initialize_path, sysid, makepath=True) if create_new \ + else self.client.retry(self.client.set, self.initialize_path, sysid.encode("utf-8")) def touch_member(self, data, ttl=None): cluster = self.cluster diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 544f8afd..c4628135 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -86,7 +86,7 @@ class MockConnect(Mock): def pg_controldata_string(*args, **kwargs): - return """ + return b""" pg_control version number: 942 Catalog version number: 201509161 Database system identifier: 6200971513092291716 From 40c5d5e3516b225ffddb3cfd14bc6673004d960e Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 19 Oct 2015 16:08:52 +0200 Subject: [PATCH 49/59] Match default param in the abstract class definition with those from the implementation. --- patroni/dcs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/dcs.py b/patroni/dcs.py index 48d49cf5..a8afda06 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -240,7 +240,7 @@ class AbstractDCS: overwriting the key if necessary.""" @abc.abstractmethod - def initialize(self, create_new=True, sysid=None): + def initialize(self, create_new=True, sysid=""): """Race for cluster initialization. :param create_new: False if the key should already exist (in the case we are setting the system_id) From 92fe6a1de9c05dfc9946ec66691529a55af1571f Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Tue, 20 Oct 2015 11:28:26 +0200 Subject: [PATCH 50/59] Make pgpass location configurable. One can use pgpass configuration parameter in the postgres subsection of Patroni. By default pgpass is written in ~/. Mock actual writes to pgpass in the tests. --- patroni/postgresql.py | 9 ++++++--- tests/test_postgresql.py | 8 ++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index f75324e7..81758909 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -48,6 +48,7 @@ class Postgresql: self.replication = config['replication'] self.superuser = config['superuser'] self.admin = config['admin'] + self.pgpass = config.get('pgpass', None) self.pg_rewind = config.get('pg_rewind', {}) self.callback = config.get('callbacks', {}) self.use_slots = config.get('use_slots', True) @@ -171,12 +172,14 @@ class Postgresql: os.path.exists(self.trigger_file) and os.unlink(self.trigger_file) def write_pgpass(self, record): - pgpass = 'pgpass' - with open(pgpass, 'w') as f: + self.pgpass = self.pgpass or os.path.join(os.path.expanduser('~'), 'pgpass') + + with open(self.pgpass, 'w') as f: os.fchmod(f.fileno(), 0o600) f.write('{host}:{port}:*:{user}:{password}\n'.format(**record)) + env = os.environ.copy() - env['PGPASSFILE'] = pgpass + env['PGPASSFILE'] = self.pgpass return env def sync_from_leader(self, leader): diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 9a02ece6..5bd09e13 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -210,10 +210,16 @@ class TestPostgresql(unittest.TestCase): self.assertFalse(self.p.restart()) self.assertEquals(self.p.state, 'restart failed (restarting)') + @patch.object(builtins, 'open', MagicMock()) + def test_write_pgpass(self): + self.p.write_pgpass({'host': 'localhost', 'port': '5432', 'user': 'foo', 'password': 'bar'}) + + @patch('patroni.postgresql.Postgresql.write_pgpass', MagicMock(return_value=dict())) def test_sync_from_leader(self): self.assertTrue(self.p.sync_from_leader(self.leader)) @patch('subprocess.call', side_effect=Exception("Test")) + @patch('patroni.postgresql.Postgresql.write_pgpass', MagicMock(return_value=dict())) def test_pg_rewind(self, mock_call): self.assertTrue(self.p.rewind(self.leader)) subprocess.call = mock_call @@ -222,6 +228,7 @@ class TestPostgresql(unittest.TestCase): @patch('patroni.postgresql.Postgresql.rewind', return_value=False) @patch('patroni.postgresql.Postgresql.remove_data_directory', MagicMock(return_value=True)) @patch('patroni.postgresql.Postgresql.single_user_mode', MagicMock(return_value=1)) + @patch('patroni.postgresql.Postgresql.write_pgpass', MagicMock(return_value=dict())) def test_follow_the_leader(self, mock_pg_rewind): self.p.demote() self.p.follow_the_leader(None) @@ -327,6 +334,7 @@ class TestPostgresql(unittest.TestCase): with patch('os.rename', Mock(side_effect=OSError())): self.p.move_data_directory() + @patch('patroni.postgresql.Postgresql.write_pgpass', MagicMock(return_value=dict())) def test_bootstrap(self): with patch('subprocess.call', Mock(return_value=1)): self.assertRaises(PostgresException, self.p.bootstrap) From 35641ac0727f04f9527333a45a76e7ded1f55734 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Tue, 20 Oct 2015 11:40:52 +0200 Subject: [PATCH 51/59] Use distinct paths for pgpass from test nodes. --- postgres0.yml | 1 + postgres1.yml | 1 + 2 files changed, 2 insertions(+) diff --git a/postgres0.yml b/postgres0.yml index a155b1cd..8747a3af 100644 --- a/postgres0.yml +++ b/postgres0.yml @@ -34,6 +34,7 @@ postgresql: data_dir: data/postgresql0 maximum_lag_on_failover: 1048576 # 1 megabyte in bytes use_slots: True + pgpass: /tmp/pgpass0 pg_rewind: username: postgres password: zalando diff --git a/postgres1.yml b/postgres1.yml index 94e33a42..dcf2f0cf 100644 --- a/postgres1.yml +++ b/postgres1.yml @@ -34,6 +34,7 @@ postgresql: data_dir: data/postgresql1 maximum_lag_on_failover: 1048576 # 1 megabyte in bytes use_slots: True + pgpass: /tmp/pgpass1 pg_rewind: username: postgres password: zalando From f53c968d8b17fc526c0883af70dfe99797dbb1ca Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 20 Oct 2015 14:36:49 +0200 Subject: [PATCH 52/59] Improve tests --- tests/test_api.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_api.py b/tests/test_api.py index ae25964d..72d5c2b0 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -135,6 +135,8 @@ class TestRestApiHandler(unittest.TestCase): MockRestApiServer(RestApiHandler, request) with patch.object(MockPatroni, 'dcs') as d: cluster = d.get_cluster.return_value + cluster.leader.name = 'postgresql0' + MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'postgresql1' cluster.failover = None MockRestApiServer(RestApiHandler, request) From 5d7e4fe90afd03bc9be1a92328890fa16dc8ea3d Mon Sep 17 00:00:00 2001 From: Dr Nic Williams Date: Tue, 20 Oct 2015 14:32:59 -0500 Subject: [PATCH 53/59] allow $PATRONI_SCOPE to be set via 'docker run -e PATRONI_SCOPE=ironman' --- docker/entrypoint.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh index f4851a36..d94757b3 100755 --- a/docker/entrypoint.sh +++ b/docker/entrypoint.sh @@ -3,25 +3,25 @@ function usage() { cat <<__EOF__ -Usage: $0 +Usage: $0 Options: --etcd ETCD Provide an external etcd to connect to - --name NAME Give the cluster a specific name + --name NAME Give the cluster a specific name --etcd-only Do not run Patroni, run a standalone etcd Examples: $0 --etcd=127.17.0.84:4001 $0 --etcd-only - $0 + $0 $0 --name=true_scotsman __EOF__ } DOCKER_IP=$(hostname --ip-address) -PATRONI_SCOPE=batman +PATRONI_SCOPE=${PATRONI_SCOPE:-batman} optspec=":vh-:" while getopts "$optspec" optchar; do @@ -32,7 +32,7 @@ while getopts "$optspec" optchar; do exec etcd --data-dir /tmp/etcd.data \ -advertise-client-urls=http://${DOCKER_IP}:4001 \ -listen-client-urls=http://0.0.0.0:4001 \ - -listen-peer-urls=http://0.0.0.0:2380 + -listen-peer-urls=http://0.0.0.0:2380 exit 0 ;; cheat) From 0096b6b06fdb76d9b616fe08dcfa7e00f79b2c57 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 21 Oct 2015 10:56:43 +0200 Subject: [PATCH 54/59] Schedule update of machines cache when api_execute call has failed Such situation could happen if we replaced all etcd nodes except one which was used by patroni. After replacing the last node patroni will try to execute request on all other nodes from machines_cache but non of them are available. Michines cache would became empty and patroni will stick to the latest node which was available in the machines_cache and will never try to refresh machines_cache from dns for example. Currently machines cache is refreshed only when one request to the etcd cluster has failed, but probably it should be done periodically, for example every minute... --- patroni/etcd.py | 6 +++++- tests/test_etcd.py | 14 ++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/patroni/etcd.py b/patroni/etcd.py index 79379700..622eb747 100644 --- a/patroni/etcd.py +++ b/patroni/etcd.py @@ -52,7 +52,11 @@ class Client(etcd.Client): def api_execute(self, path, method, **kwargs): # Update machines_cache if previous attempt of update has failed self._update_machines_cache and self._load_machines_cache() - return super(Client, self).api_execute(path, method, **kwargs) + try: + return super(Client, self).api_execute(path, method, **kwargs) + except etcd.EtcdConnectionFailed: + self._update_machines_cache = True + raise @staticmethod def get_srv_record(host): diff --git a/tests/test_etcd.py b/tests/test_etcd.py index 53c054e5..6cd5441e 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -106,13 +106,13 @@ def etcd_read(key, **kwargs): "modifiedIndex": 20437, "createdIndex": 20437}, {"key": "/service/batman5/members", "dir": True, "nodes": [ {"key": "/service/batman5/members/postgresql1", - "value": "postgres://replicator:rep-pass@127.0.0.1:5434/postgres" - + "?application_name=http://127.0.0.1:8009/patroni", + "value": "postgres://replicator:rep-pass@127.0.0.1:5434/postgres" + + "?application_name=http://127.0.0.1:8009/patroni", "expiration": "2015-05-15T09:10:59.949384522Z", "ttl": 21, "modifiedIndex": 20727, "createdIndex": 20727}, {"key": "/service/batman5/members/postgresql0", - "value": "postgres://replicator:rep-pass@127.0.0.1:5433/postgres" - + "?application_name=http://127.0.0.1:8008/patroni", + "value": "postgres://replicator:rep-pass@127.0.0.1:5433/postgres" + + "?application_name=http://127.0.0.1:8008/patroni", "expiration": "2015-05-15T09:11:09.611860899Z", "ttl": 30, "modifiedIndex": 20730, "createdIndex": 20730}], "modifiedIndex": 1581, "createdIndex": 1581}], "modifiedIndex": 1581, "createdIndex": 1581}} @@ -143,6 +143,7 @@ def socket_getaddrinfo(*args): def http_request(method, url, **kwargs): + print('http_request', method, url, kwargs) if url == 'http://localhost:2379/': return MockResponse() raise socket.error @@ -165,6 +166,11 @@ class TestClient(unittest.TestCase): self.client._base_uri = 'http://localhost:4001' self.client._machines_cache = ['http://localhost:2379'] self.client.api_execute('/', 'GET') + self.client._update_machines_cache = False + self.client._base_uri = 'http://localhost:4001' + self.client._machines_cache = [] + self.assertRaises(etcd.EtcdConnectionFailed, self.client.api_execute, '/', 'GET') + self.assertTrue(self.client._update_machines_cache) def test_get_srv_record(self): self.assertEquals(self.client.get_srv_record('blabla'), []) From 8bd28507a93310cb8b37d830822900ce1d1417f2 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 21 Oct 2015 11:08:06 +0200 Subject: [PATCH 55/59] format tests according to the latest pep8 standards --- tests/test_postgresql.py | 15 +++++---------- tests/test_utils.py | 4 +++- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 9a02ece6..cc781fc8 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -4,12 +4,7 @@ import psycopg2 import shutil import unittest -from sys import version_info -if version_info.major == 2: - import __builtin__ as builtins -else: - import builtins - +from six.moves import builtins from mock import Mock, MagicMock, PropertyMock, patch, mock_open from patroni.dcs import Cluster, Leader, Member from patroni.exceptions import PostgresException, PostgresConnectionException @@ -141,10 +136,10 @@ Data page checksum version: 0 def postmaster_opts_string(*args, **kwargs): - return '/usr/local/pgsql/bin/postgres "-D" "data/postgresql0" "--listen_addresses=127.0.0.1" "--port=5432"'\ - ' "--hot_standby=on" "--wal_keep_segments=8" "--wal_level=hot_standby" "--archive_command=mkdir -p ../wal_archive \n'\ - '&& cp %p ../wal_archive/%f" "--wal_log_hints=on" "--max_wal_senders=5" "--archive_timeout=1800s" "--archive_mode=on"'\ - ' "--max_replication_slots=5"\n' + return '/usr/local/pgsql/bin/postgres "-D" "data/postgresql0" "--listen_addresses=127.0.0.1" \ +"--port=5432" "--hot_standby=on" "--wal_keep_segments=8" "--wal_level=hot_standby" \ +"--archive_command=mkdir -p ../wal_archive && cp %p ../wal_archive/%f" "--wal_log_hints=on" \ +"--max_wal_senders=5" "--archive_timeout=1800s" "--archive_mode=on" "--max_replication_slots=5"\n' def psycopg2_connect(*args, **kwargs): diff --git a/tests/test_utils.py b/tests/test_utils.py index 45b194dc..265f98ab 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -67,7 +67,9 @@ class TestRetrySleeper(unittest.TestCase): self.assertRaises(RetryFailedError, retry, self._fail(times=100)) def test_copy(self): - _sleep = lambda t: None + def _sleep(t): + None + retry = self._makeOne(sleep_func=_sleep) rcopy = retry.copy() self.assertTrue(rcopy.sleep_func is _sleep) From c4a6dd48d34b490971edc77e87bd466ad95d8988 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 21 Oct 2015 11:09:37 +0200 Subject: [PATCH 56/59] remove debug print statement --- tests/test_etcd.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_etcd.py b/tests/test_etcd.py index 6cd5441e..d0d01d71 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -143,7 +143,6 @@ def socket_getaddrinfo(*args): def http_request(method, url, **kwargs): - print('http_request', method, url, kwargs) if url == 'http://localhost:2379/': return MockResponse() raise socket.error From 44a73982d4dda64618345142f0a3381aaafa539a Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 21 Oct 2015 12:00:03 +0200 Subject: [PATCH 57/59] Do not try to fetch the element from the get_node result if the node is not there. --- patroni/zookeeper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index ba31e756..4cb73e0f 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -139,7 +139,7 @@ class ZooKeeper(AbstractDCS): self.fetch_cluster = True # get initialize flag - initialize = self.get_node(self.initialize_path)[0] if self._INITIALIZE in nodes else None + initialize = (self.get_node(self.initialize_path) or [None])[0] if self._INITIALIZE in nodes else None # get list of members members = self.load_members() if self._MEMBERS[:-1] in nodes else [] From 9130891029076f3bf0aadb3e54bdf829bcc4deec Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 21 Oct 2015 13:06:54 +0200 Subject: [PATCH 58/59] Move calculation of pgpass to the class constructor: better to fail fast in case of issues. --- patroni/postgresql.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 81758909..44406fe2 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -48,7 +48,7 @@ class Postgresql: self.replication = config['replication'] self.superuser = config['superuser'] self.admin = config['admin'] - self.pgpass = config.get('pgpass', None) + self.pgpass = config.get('pgpass', None) or os.path.join(os.path.expanduser('~'), 'pgpass') self.pg_rewind = config.get('pg_rewind', {}) self.callback = config.get('callbacks', {}) self.use_slots = config.get('use_slots', True) @@ -172,8 +172,6 @@ class Postgresql: os.path.exists(self.trigger_file) and os.unlink(self.trigger_file) def write_pgpass(self, record): - self.pgpass = self.pgpass or os.path.join(os.path.expanduser('~'), 'pgpass') - with open(self.pgpass, 'w') as f: os.fchmod(f.fileno(), 0o600) f.write('{host}:{port}:*:{user}:{password}\n'.format(**record)) From 5ae6f3a56c2d073d5c9543c6afcc8772e1e606a4 Mon Sep 17 00:00:00 2001 From: Feike Steenbergen Date: Thu, 22 Oct 2015 09:30:12 +0200 Subject: [PATCH 59/59] Change Docker registry --- docker/dev_patroni_cluster.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/dev_patroni_cluster.sh b/docker/dev_patroni_cluster.sh index e9a253dc..dcc18f87 100755 --- a/docker/dev_patroni_cluster.sh +++ b/docker/dev_patroni_cluster.sh @@ -1,6 +1,6 @@ #!/bin/bash -DOCKER_IMAGE="os-registry.stups.zalan.do/acid/patroni:1.0-SNAPSHOT" +DOCKER_IMAGE="registry.opensource.zalan.do/acid/patroni:1.0-SNAPSHOT" MEMBERS=3