From a4af9f2a4cb7670c4d1881366804cc47f682925e Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Fri, 11 Dec 2015 18:54:03 +0100 Subject: [PATCH 01/16] Add replicafrom tag. --- patroni/__init__.py | 4 ++++ patroni/dcs.py | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/patroni/__init__.py b/patroni/__init__.py index 045d0d59..028142e9 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -29,6 +29,10 @@ class Patroni: def nofailover(self): return self.tags.get('nofailover', False) + @property + def replicatefrom(self): + return self.tags.get('replicatefrom') + @staticmethod def get_dcs(name, config): if 'etcd' in config: diff --git a/patroni/dcs.py b/patroni/dcs.py index f640787c..85fda68c 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -67,6 +67,10 @@ class Member(namedtuple('Member', 'index,name,session,data')): def nofailover(self): return self.data.get('tags', {}).get('nofailover', False) + @property + def replicatefrom(self): + return self.data.get('tags', {}).get('replicatefrom') + class Leader(namedtuple('Leader', 'index,session,member')): From 39cbd5f1d69d34eb870cd8d365ff6f269c0dd954 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Fri, 11 Dec 2015 18:54:38 +0100 Subject: [PATCH 02/16] Unify all follow the leader calls from eventloop. Call normal follow the leader method from HA even during recovery. This provides a single place that changes recovery.conf, making it easier to plug in a cascading replica in the future. Remove an obsolete demote function from PostreSQL module, modified the tests. --- patroni/ha.py | 45 ++++++++++++++++++++++------------------ patroni/postgresql.py | 3 --- tests/test_ha.py | 8 ++++++- tests/test_postgresql.py | 2 -- 4 files changed, 32 insertions(+), 26 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 3a8d46d7..0a2072aa 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -19,6 +19,7 @@ class Ha: self.dcs = patroni.dcs self.cluster = None self.old_cluster = None + self.recovering = False self._async_executor = AsyncExecutor() def load_cluster_from_dcs(self): @@ -94,33 +95,21 @@ class Ha: return 'waiting for leader to bootstrap' def recover(self): - has_lock = self.has_lock() - # 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 and\ + if (self.state_handler.role == 'master') and pg_controldata and\ pg_controldata.get('Database cluster state', '') == 'in production': # crashed master self.state_handler.require_rewind() + self.recovering = True + return self.follow_the_leader("started as readonly because i had the session lock", + "started as a secondary", + refresh=True, recovery=True) - # XXX: follow the leader calls stop, which might take quite some time. - # perhaps we should run sync asynchronously - # (we still need the exit code from follow_the_leader) - ret = self.state_handler.follow_the_leader(None if has_lock else self.cluster.leader, recovery=True) - if not ret: - 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' - 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): + def follow_the_leader(self, demote_reason, follow_reason, refresh=True, recovery=False): refresh and self.load_cluster_from_dcs() - ret = demote_reason if self.state_handler.is_leader() else follow_reason + ret = demote_reason if (not recovery and self.state_handler.is_leader() + or recovery and self.state_handler.role == 'master') else follow_reason 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): @@ -382,6 +371,15 @@ class Ha: # so even 1 << 32 would have 10 digits. return str(sysid) and len(str(sysid)) >= 10 and str(sysid).isdigit() + def post_recover(self): + if not self.state_handler.is_running(): + if self.has_lock(): + self.dcs.delete_leader() + self.dcs.reset_cluster() + return 'removed leader key after trying and failing to start postgres' + return 'failed to start postgres' + return None + def _run_cycle(self): try: self.load_cluster_from_dcs() @@ -395,6 +393,13 @@ class Ha: if self._async_executor.busy: return self.handle_long_action_in_progress() + # we've go here, so async action has finished. Check if we tried to recover and failed + if self.recovering: + self.recovering = False + msg = self.post_recover() + if msg is not None: + return msg + # currently it can trigger only reinitialize msg = self.process_scheduled_action() if msg is not None: diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 45b871c2..84223675 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -596,9 +596,6 @@ recovery_target_timeline = 'latest' self.call_nowait(ACTION_ON_ROLE_CHANGE) return ret - def demote(self): - self.follow_the_leader(None) - def create_or_update_role(self, name, password, options): self.query("""DO $$ BEGIN diff --git a/tests/test_ha.py b/tests/test_ha.py index d9a408a4..bcc5c93d 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -127,13 +127,18 @@ class TestHa(unittest.TestCase): def test_recover_replica_failed(self): self.p.controldata = lambda: {'Database cluster state': 'in production'} self.p.is_healthy = false + self.p.is_running = false self.p.follow_the_leader = false + self.assertEquals(self.ha.run_cycle(), 'started as a secondary') self.assertEquals(self.ha.run_cycle(), 'failed to start postgres') def test_recover_master_failed(self): self.p.follow_the_leader = false self.p.is_healthy = false + self.p.is_running = false self.ha.has_lock = true + self.p.role = 'master' + self.assertEquals(self.ha.run_cycle(), 'started as readonly because i had the session lock') self.assertEquals(self.ha.run_cycle(), 'removed leader key after trying and failing to start postgres') @patch('sys.exit', return_value=1) @@ -144,7 +149,8 @@ class TestHa(unittest.TestCase): @patch.object(Cluster, 'is_unlocked', Mock(return_value=False)) def test_start_as_readonly(self): - self.p.is_leader = self.p.is_healthy = false + self.p.is_leader = false + self.p.is_healthy = true self.ha.has_lock = true self.assertEquals(self.ha.run_cycle(), 'promoted self to leader because i had the session lock') diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 2097bcb7..a4dae962 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -243,9 +243,7 @@ class TestPostgresql(unittest.TestCase): @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) - self.p.demote() self.p.follow_the_leader(self.leader) self.p.follow_the_leader(Leader(-1, 28, self.other)) self.p.rewind = mock_pg_rewind From bf52fa6f570208f5ddc5b2c0779c2eb43ffac732 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 30 Dec 2015 16:05:06 +0100 Subject: [PATCH 03/16] follow_the_leader unconditionally during recovery. Otherwise, we may 'forget' to start the crashed node. This fixes the regression from the former behavior introduced in the previous commit. --- patroni/ha.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 0a2072aa..af1b62b1 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -112,9 +112,9 @@ class Ha: or recovery and self.state_handler.role == 'master') else follow_reason 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): + if not self.state_handler.check_recovery_conf(leader) or recovery: self._async_executor.schedule('changing primary_conninfo and restarting') - self._async_executor.run_async(self.state_handler.follow_the_leader, (leader, )) + self._async_executor.run_async(self.state_handler.follow_the_leader, (leader, recovery)) return ret def enforce_master_role(self, message, promote_message): From c650dc092e4e46909baf8cb64d3071884e049cbe Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 30 Dec 2015 18:33:23 +0100 Subject: [PATCH 04/16] Follow the node in the replicatefrom if present. Rename the follow_the_leader to just follow, since the node to be followed is not necessary a leader anymore. Extend the code that manages replication slots to the non-master nodes if they are mentioned in at least one replicatefrom tag. Add the 3rd configuration in order to be able to run cascading replicas. --- patroni/ha.py | 44 ++++++++++------- patroni/postgresql.py | 8 +++- postgres0.yml | 5 +- postgres1.yml | 5 +- postgres2.yml | 101 +++++++++++++++++++++++++++++++++++++++ tests/test_ha.py | 9 ++-- tests/test_postgresql.py | 16 +++---- 7 files changed, 152 insertions(+), 36 deletions(-) create mode 100644 postgres2.yml diff --git a/patroni/ha.py b/patroni/ha.py index af1b62b1..3856366d 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -102,19 +102,25 @@ class Ha: pg_controldata.get('Database cluster state', '') == 'in production': # crashed master self.state_handler.require_rewind() self.recovering = True - return self.follow_the_leader("started as readonly because i had the session lock", - "started as a secondary", - refresh=True, recovery=True) + return self.follow("started as readonly because i had the session lock", + "started as a secondary", + refresh=True, recovery=True) - def follow_the_leader(self, demote_reason, follow_reason, refresh=True, recovery=False): + def follow(self, demote_reason, follow_reason, refresh=True, recovery=False): refresh and self.load_cluster_from_dcs() ret = demote_reason if (not recovery and self.state_handler.is_leader() or recovery and self.state_handler.role == 'master') else follow_reason - 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) or recovery: + # determine the node to follow. If replicatefrom tag is set, + # try to follow the node mentioned there, otherwise, follow the leader. + if self.patroni.replicatefrom: + node_to_follow = [m for m in self.cluster.members if m.name == self.patroni.replicatefrom] + node_to_follow = node_to_follow[0] if node_to_follow else self.cluster.leader + else: + node_to_follow = self.cluster.leader + node_to_follow = None if (node_to_follow and node_to_follow.name) == self.state_handler.name else node_to_follow + if not self.state_handler.check_recovery_conf(node_to_follow) or recovery: self._async_executor.schedule('changing primary_conninfo and restarting') - self._async_executor.run_async(self.state_handler.follow_the_leader, (leader, recovery)) + self._async_executor.run_async(self.state_handler.follow, (node_to_follow, recovery)) return ret def enforce_master_role(self, message, promote_message): @@ -287,14 +293,14 @@ class Ha: return self.enforce_master_role('acquired session lock as a leader', 'promoted self to leader by acquiring session lock') else: - return self.follow_the_leader('demoted self due after trying and failing to obtain lock', - 'following new leader after trying and failing to obtain lock') + return self.follow('demoted self due after trying and failing to obtain lock', + 'following new leader after trying and failing to obtain lock') else: if self.patroni.nofailover: - return self.follow_the_leader('demoting self because I am not allowed to become master', - 'following a different leader because I am not allowed to promote') - return self.follow_the_leader('demoting self because i am not the healthiest node', - 'following a different leader because i am not the healthiest node') + return self.follow('demoting self because I am not allowed to become master', + 'following a different leader because I am not allowed to promote') + return self.follow('demoting self because i am not the healthiest node', + 'following a different leader because i am not the healthiest node') def process_healthy_cluster(self): if self.has_lock(): @@ -312,8 +318,8 @@ class Ha: self.load_cluster_from_dcs() else: logger.info('does not have lock') - 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) + return self.follow('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(self, action): with self._async_executor: @@ -430,7 +436,11 @@ class Ha: else: return self.process_healthy_cluster() finally: - self.state_handler.sync_replication_slots(self.cluster) + # we might not have a valid PostgreSQL connection here if another thread + # stops PostgreSQL, therefore, we only reload replication slots if no + # asyncrhonous processes are running (should be always the case for the master) + if not self._async_executor.busy: + 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/postgresql.py b/patroni/postgresql.py index 84223675..db47cbff 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -524,7 +524,7 @@ recovery_target_timeline = 'latest' except: logger.exception("Unable to remove {}".format(path)) - def follow_the_leader(self, leader, recovery=False): + def follow(self, leader, recovery=False): if not self.check_recovery_conf(leader) or recovery: change_role = (self.role == 'master') @@ -634,7 +634,11 @@ $$""".format(name, options), name, password, password) if self.use_slots: try: self.load_replication_slots() - slots = [m.name for m in cluster.members if m.name != self.name] if self.role == 'master' else [] + if self.role == 'master': + slots = [m.name for m in cluster.members if m.name != self.name] + else: + # only manage slots for replicas that want to replicate from this one + slots = [m.name for m in cluster.members if m.replicatefrom == self.name] # drop unused slots for slot in set(self.replication_slots) - set(slots): self.query("""SELECT pg_drop_replication_slot(%s) diff --git a/postgres0.yml b/postgres0.yml index 36018ad5..f29e1fee 100644 --- a/postgres0.yml +++ b/postgres0.yml @@ -88,14 +88,13 @@ postgresql: archive_mode: "on" wal_level: hot_standby archive_command: mkdir -p ../wal_archive && test ! -f ../wal_archive/%f && cp %p ../wal_archive/%f - max_wal_senders: 5 + max_wal_senders: 10 wal_keep_segments: 8 archive_timeout: 1800s - max_replication_slots: 5 + max_replication_slots: 10 hot_standby: "on" wal_log_hints: "on" tags: nofailover: False noloadbalance: False clonefrom: False - replicatefrom: 127.0.0.1 diff --git a/postgres1.yml b/postgres1.yml index e1b61b3b..1e7a7045 100644 --- a/postgres1.yml +++ b/postgres1.yml @@ -88,14 +88,13 @@ postgresql: archive_mode: "on" wal_level: hot_standby archive_command: mkdir -p ../wal_archive && test ! -f ../wal_archive/%f && cp %p ../wal_archive/%f - max_wal_senders: 5 + max_wal_senders: 10 wal_keep_segments: 8 archive_timeout: 1800s - max_replication_slots: 5 + max_replication_slots: 10 hot_standby: "on" wal_log_hints: "on" tags: nofailover: False noloadbalance: False clonefrom: False - replicatefrom: 127.0.0.1 diff --git a/postgres2.yml b/postgres2.yml new file mode 100644 index 00000000..99e8b47e --- /dev/null +++ b/postgres2.yml @@ -0,0 +1,101 @@ +ttl: &ttl 30 +loop_wait: &loop_wait 10 +scope: &scope batman +restapi: + listen: 127.0.0.1:8010 + connect_address: 127.0.0.1:8010 + auth: 'username:password' +# certfile: /etc/ssl/certs/ssl-cert-snakeoil.pem +# keyfile: /etc/ssl/private/ssl-cert-snakeoil.key +etcd: + scope: *scope + ttl: *ttl + host: 127.0.0.1:4001 + #discovery_srv: my-etcd.domain +#zookeeper: +# scope: *scope +# session_timeout: *ttl +# reconnect_timeout: *loop_wait +# hosts: +# - 127.0.0.1:2181 +# - 127.0.0.2:2181 +# exhibitor: +# poll_interval: 300 +# port: 8181 +# hosts: +# - host1 +# - host2 +# - host3 +postgresql: + name: postgresql2 + scope: *scope + listen: 127.0.0.1:5434 + connect_address: 127.0.0.1:5434 + data_dir: data/postgresql2 + maximum_lag_on_failover: 1048576 # 1 megabyte in bytes + use_slots: True + pgpass: /tmp/pgpass2 + initdb: ## We allow the following options to be passed on to initdb + # - auth: authmethod + # - auth-host: authmethod + # - auth-local: authmethod + - encoding: UTF8 + # - data-checksums # When pg_rewind is needed on 9.3, this needs to be enabled + # - locale: locale + # - lc-collate: locale + # - lc-ctype: locale + # - lc-messages: locale + # - lc-monetary: locale + # - lc-numeric: locale + # - lc-time: locale + # - text-search-config: CFG + # - xlogdir: directory + # - debug + # - noclean + 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 + replication: + username: replicator + password: rep-pass + network: 127.0.0.1/32 + superuser: + user: postgres + password: zalando + admin: + username: admin + password: admin +# commented-out example for wal-e provisioning + create_replica_method: + - basebackup +# - wal_e +# commented-out example for wal-e provisioning + #wal_e: + #command: /patroni/scripts/wale_restore.py + #env_dir: /home/postgres/etc/wal-e.d/env + #threshold_megabytes: 10240 + #threshold_backup_size_percentage: 30 + #retries: 2 + #use_iam: 1 + #recovery_conf: + #restore_command: envdir /etc/wal-e.d/env wal-e wal-fetch "%f" "%p" -p 1 + recovery_conf: + restore_command: cp ../wal_archive/%f %p + parameters: + archive_mode: "on" + wal_level: hot_standby + archive_command: mkdir -p ../wal_archive && test ! -f ../wal_archive/%f && cp %p ../wal_archive/%f + max_wal_senders: 10 + wal_keep_segments: 8 + archive_timeout: 1800s + max_replication_slots: 10 + hot_standby: "on" + wal_log_hints: "on" +tags: + nofailover: False + noloadbalance: False + clonefrom: False + replicatefrom: postgresql1 diff --git a/tests/test_ha.py b/tests/test_ha.py index bcc5c93d..336b78a3 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -88,6 +88,7 @@ class MockPatroni: self.api = Mock() self.tags = {} self.nofailover = None + self.replicatefrom = None self.api.connection_string = 'http://127.0.0.1:8008' @@ -128,12 +129,12 @@ class TestHa(unittest.TestCase): self.p.controldata = lambda: {'Database cluster state': 'in production'} self.p.is_healthy = false self.p.is_running = false - self.p.follow_the_leader = false + self.p.follow = false self.assertEquals(self.ha.run_cycle(), 'started as a secondary') self.assertEquals(self.ha.run_cycle(), 'failed to start postgres') def test_recover_master_failed(self): - self.p.follow_the_leader = false + self.p.follow = false self.p.is_healthy = false self.p.is_running = false self.ha.has_lock = true @@ -202,10 +203,12 @@ class TestHa(unittest.TestCase): 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): + def test_follow(self): 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') + self.ha.patroni.replicatefrom = "foo" + self.assertEquals(self.ha.run_cycle(), 'no action. i am a secondary and i am following a leader') def test_no_etcd_connection_master_demote(self): self.ha.load_cluster_from_dcs = Mock(side_effect=DCSError('Etcd is not responding properly')) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index a4dae962..8114272b 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -242,23 +242,23 @@ class TestPostgresql(unittest.TestCase): @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.follow_the_leader(None) - self.p.follow_the_leader(self.leader) - self.p.follow_the_leader(Leader(-1, 28, self.other)) + def test_follow(self, mock_pg_rewind): + self.p.follow(None) + self.p.follow(self.leader) + self.p.follow(Leader(-1, 28, self.other)) self.p.rewind = mock_pg_rewind - self.p.follow_the_leader(self.leader) + self.p.follow(self.leader) self.p.require_rewind() with mock.patch('os.path.islink', MagicMock(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.follow(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) + self.p.follow(self.leader, recovery=True) self.p.rewind.return_value = False - self.p.follow_the_leader(self.leader, recovery=True) + self.p.follow(self.leader, recovery=True) def test_can_rewind(self): tmp = self.p.pg_rewind From 15bec1e28c0ca2ca82015fa8fa06b3ae5c8a9c94 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Tue, 26 Jan 2016 15:24:32 +0100 Subject: [PATCH 05/16] Support new replicas without the master. The replication method should have no_master flag set and support getting the base backup from some external storage (i.e. S3). At the moment we only support initialization of replicas in the already existing cluster when no master is present, since there is no 'one fits all' way to decide whether to run initidb or wait for the replica data storage to become available when dealing with the new cluster. --- patroni/ha.py | 9 ++++-- patroni/postgresql.py | 57 ++++++++++++++++++++++++--------- patroni/scripts/wale_restore.py | 37 ++++++++++++--------- tests/test_ha.py | 16 +++++++++ tests/test_postgresql.py | 23 ++++++++++++- tests/test_wale_restore.py | 4 ++- 6 files changed, 111 insertions(+), 35 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 3856366d..0feb172b 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -63,12 +63,12 @@ class Ha: self.dcs.touch_member(json.dumps(data, separators=(',', ':'))) def copy_backup_from_leader(self, leader): - if self.state_handler.bootstrap(leader): - logger.info('bootstrapped from leader') + if self.state_handler.bootstrap(True, leader): + logger.info('bootstrapped from leader' if leader else 'bootstrapped without leader') else: self.state_handler.stop('immediate') self.state_handler.remove_data_directory() - logger.error('failed to bootstrap from leader') + logger.error('failed to bootstrap from leader' if leader else 'failed to bootstrap (without leader)') def bootstrap(self): if not self.cluster.is_unlocked(): # cluster already has leader @@ -92,6 +92,9 @@ class Ha: else: return 'failed to acquire initialize lock' else: + if self.state_handler.can_create_replica_without_leader(): + self._async_executor.run_async(self.copy_backup_from_leader, args=(None, )) + return "trying to bootstrap without leader" return 'waiting for leader to bootstrap' def recover(self): diff --git a/patroni/postgresql.py b/patroni/postgresql.py index db47cbff..f329fad5 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -220,9 +220,9 @@ class Postgresql: return env def sync_from_leader(self, leader): - r = parseurl(leader.conn_url) - - env = self.write_pgpass(r) + if leader: + r = parseurl(leader.conn_url) + env = self.write_pgpass(r) if leader else os.environ.copy() ret = self.create_replica(leader, env) == 0 ret and self.delete_trigger_file() return ret @@ -235,14 +235,30 @@ class Postgresql: """ return ' '.join('{}={}'.format(param, val) for param, val in sorted(conn.items())) + def replica_method_can_work_without_leader(self, method): + return method != 'basebackup' and self.config and self.config.get(method, {}).get('no_master') + + def can_create_replica_without_leader(self): + """ go through the replication methods to see if there are ones + that does not require a running leader to create the replica. + """ + replica_methods = self.config.get('create_replica_method', []) + for replica_method in replica_methods: + if self.replica_method_can_work_without_leader(replica_method): + return True + return False + def create_replica(self, leader, env): # create the replica according to the replica_method # defined by the user. this is a list, so we need to # loop through all methods the user supplies - connstring = leader.conn_url + connstring = leader.conn_url if leader else "" # get list of replica methods from config. # If there is no configuration key, or no value is specified, use basebackup replica_methods = self.config.get('create_replica_method') or ['basebackup'] + # if we don't have any leader, leave only replica methods that work without it + replica_methods = [r for r in replica_methods if self.replica_method_can_work_without_leader(r)] if not leader \ + else replica_methods # go through them in priority order ret = 1 for replica_method in replica_methods: @@ -435,7 +451,7 @@ class Postgresql: return pattern and (pattern in line) return not pattern - def write_recovery_conf(self, leader): + def write_recovery_conf(self, leader, bootstrap=False): with open(self.recovery_conf, 'w') as f: f.write("""standby_mode = 'on' recovery_target_timeline = 'latest' @@ -444,6 +460,7 @@ recovery_target_timeline = 'latest' f.write("""primary_conninfo = '{}'\n""".format(self.primary_conninfo(leader.conn_url))) if self.use_slots: f.write("""primary_slot_name = '{}'\n""".format(self.name)) + if (leader and leader.conn_url) or bootstrap: for name, value in self.config.get('recovery_conf', {}).items(): f.write("{} = '{}'\n".format(name, value)) @@ -658,18 +675,28 @@ $$""".format(name, options), name, password, password) def last_operation(self): return str(self.xlog_position()) - def bootstrap(self, current_leader=None): + def bootstrap(self, initialize=False, current_leader=None): """ - Initially bootstrap PostgreSQL, either by creating a data - directory with initdb, or by initalizing a replica from an - exiting leader. Failure in the first case always leads to - exception, since there is no point in continuing if initdb failed. - In the second case, however, a False is returned on failure, since - it is normal for the replica to retry a failed attempt to initialize - from the master. + Populate PostgreSQL data directory by doing one of the following: + - create with initdb if there is no master. + - initialize the replica from an existing master + - initialize the replica using the replica creation method that + works without the master (i.e. restore from on-disk base backup) + + The choice between the last 2 is triggered by the initialize flag. + We should never try to initdb an already initialized cluster, nor + try to bootstrap the cluster that lacks the initialize key from from + the master-less replica creation method (in the latter case, there is + no clear inidicator of the moment we should abandon our attempts and + swich to initdb). + + Failure during initdb always leads to an exception, since there is + no point in continuing if initdb fails. For the rest of the cases, + the function returns False in order to inidicate a failed attempt + that should be retried in the future. """ ret = False - if not current_leader: + if not (initialize or current_leader): ret = self.initialize() and self.start() if ret: self.create_replication_user() @@ -679,7 +706,7 @@ $$""".format(name, options), name, password, password) else: if self.sync_from_leader(current_leader): self.restore_configuration_files() - self.write_recovery_conf(current_leader) + self.write_recovery_conf(current_leader, True) ret = self.start() return ret diff --git a/patroni/scripts/wale_restore.py b/patroni/scripts/wale_restore.py index f54c37a3..58fdd420 100755 --- a/patroni/scripts/wale_restore.py +++ b/patroni/scripts/wale_restore.py @@ -42,7 +42,7 @@ logger = logging.getLogger(__name__) class WALERestore(object): - def __init__(self, scope, datadir, connstring, env_dir, threshold_mb, threshold_pct, use_iam): + def __init__(self, scope, datadir, connstring, env_dir, threshold_mb, threshold_pct, use_iam, no_master): self.scope = scope self.master_connection = connstring self.data_dir = datadir @@ -51,6 +51,7 @@ class WALERestore(object): self.wal_e.threshold_mb = threshold_mb self.wal_e.threshold_pct = threshold_pct self.wal_e.iam_string = ' --aws-instance-profile ' if use_iam == 1 else '' + self.no_master = no_master self.wal_e.cmd = 'envdir {0} wal-e {1} '.format(self.wal_e.dir, self.wal_e.iam_string) self.init_error = (not os.path.exists(self.wal_e.dir)) @@ -109,19 +110,23 @@ class WALERestore(object): conn = None cursor = None diff_in_bytes = long(backup_size) - try: - # get the difference in bytes between the current WAL location and the backup start offset - conn = psycopg2.connect(self.master_connection) - conn.autocommit = True - cursor = conn.cursor() - cursor.execute("SELECT pg_xlog_location_diff(pg_current_xlog_location(), %s)", (backup_start_lsn,)) - diff_in_bytes = long(cursor.fetchone()[0]) - except psycopg2.Error as e: - logger.error('could not determine difference with the master location: {}'.format(e)) - return False - finally: - cursor and cursor.close() - conn and conn.close() + if not self.no_master: + try: + # get the difference in bytes between the current WAL location and the backup start offset + conn = psycopg2.connect(self.master_connection) + conn.autocommit = True + cursor = conn.cursor() + cursor.execute("SELECT pg_xlog_location_diff(pg_current_xlog_location(), %s)", (backup_start_lsn,)) + diff_in_bytes = long(cursor.fetchone()[0]) + except psycopg2.Error as e: + logger.error('could not determine difference with the master location: {}'.format(e)) + return False + finally: + cursor and cursor.close() + conn and conn.close() + else: + # always try to use WAL-E if base backup is available + diff_in_bytes = 0 # if the size of the accumulated WAL segments is more than a certan percentage of the backup size # or exceeds the pre-determined size - pg_basebackup is chosen instead. @@ -150,13 +155,15 @@ def main(): parser.add_argument('--threshold_megabytes', type=int, default=10240) parser.add_argument('--threshold_backup_size_percentage', type=int, default=30) parser.add_argument('--use_iam', type=int, default=0) + parser.add_argument('--no_master', type=int, default=0) args = parser.parse_args() # retry cloning in a loop for retry in range(0, args.retries + 1): restore = WALERestore(scope=args.scope, datadir=args.datadir, connstring=args.connstring, env_dir=args.envdir, threshold_mb=args.threshold_megabytes, - threshold_pct=args.threshold_backup_size_percentage, use_iam=args.use_iam) + threshold_pct=args.threshold_backup_size_percentage, use_iam=args.use_iam, + no_master=args.no_master) ret = restore.run() if ret == 0: break diff --git a/tests/test_ha.py b/tests/test_ha.py index 336b78a3..4c8500f4 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -103,6 +103,7 @@ class TestHa(unittest.TestCase): def setUp(self, mock_machines): mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) self.p = MockPostgresql() + self.p.can_create_replica_without_leader = MagicMock(return_value=False) self.e = Etcd('foo', {'ttl': 30, 'host': 'ok:2379', 'scope': 'test'}) self.e.client.read = etcd_read self.e.client.write = etcd_write @@ -139,6 +140,7 @@ class TestHa(unittest.TestCase): self.p.is_running = false self.ha.has_lock = true self.p.role = 'master' + self.p.controldata = lambda: {'Database cluster state': 'in production'} self.assertEquals(self.ha.run_cycle(), 'started as readonly because i had the session lock') self.assertEquals(self.ha.run_cycle(), 'removed leader key after trying and failing to start postgres') @@ -223,6 +225,11 @@ class TestHa(unittest.TestCase): self.ha.cluster = get_cluster_initialized_without_leader() self.assertEquals(self.ha.bootstrap(), 'waiting for leader to bootstrap') + def test_bootstrap_without_leader(self): + self.ha.cluster = get_cluster_initialized_without_leader() + self.p.can_create_replica_without_leader = MagicMock(return_value=True) + self.assertEquals(self.ha.bootstrap(), "trying to bootstrap without leader") + def test_bootstrap_initialize_lock_failed(self): self.ha.cluster = get_cluster_not_initialized_without_leader() self.assertEquals(self.ha.bootstrap(), 'failed to acquire initialize lock') @@ -342,3 +349,12 @@ class TestHa(unittest.TestCase): self.ha.fetch_node_status(member) member = Member(0, 'test', 1, {'api_url': 'http://localhost:8011/patroni'}) self.ha.fetch_node_status(member) + + def test_post_recover(self): + self.p.is_running = false + self.ha.has_lock = true + self.assertEqual(self.ha.post_recover(), 'removed leader key after trying and failing to start postgres') + self.ha.has_lock = false + self.assertEqual(self.ha.post_recover(), 'failed to start postgres') + self.p.is_running = true + self.assertIsNone(self.ha.post_recover()) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 8114272b..3681562f 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -259,6 +259,8 @@ class TestPostgresql(unittest.TestCase): self.p.follow(self.leader, recovery=True) self.p.rewind.return_value = False self.p.follow(self.leader, recovery=True) + with mock.patch('patroni.postgresql.Postgresql.check_recovery_conf', MagicMock(return_value=True)): + self.assertTrue(self.p.follow(None)) def test_can_rewind(self): tmp = self.p.pg_rewind @@ -305,6 +307,9 @@ class TestPostgresql(unittest.TestCase): self.p.query = Mock(side_effect=psycopg2.OperationalError) self.p.schedule_load_slots = True self.p.sync_replication_slots(cluster) + self.p.schedule_load_slots = False + with mock.patch('patroni.postgresql.Postgresql.role', new_callable=PropertyMock(return_value='replica')): + self.p.sync_replication_slots(cluster) @patch.object(MockConnect, 'closed', 2) def test__query(self): @@ -364,7 +369,8 @@ 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) + with patch('patroni.postgresql.Postgresql.sync_from_leader', MagicMock(return_value=True)): + self.p.bootstrap(self.leader) def test_remove_data_directory(self): self.p.data_dir = 'data_dir' @@ -478,3 +484,18 @@ class TestPostgresql(unittest.TestCase): def test_restore_configuration_files(self, mock_copy): shutil.copy = mock_copy self.p.restore_configuration_files() + + def test_can_create_replica_without_leader(self): + self.p.config['create_replica_method'] = [] + self.assertFalse(self.p.can_create_replica_without_leader()) + self.p.config['create_replica_method'] = ['wale', 'basebackup'] + self.p.config['wale'] = {'command': 'foo', 'no_master': 1} + self.assertTrue(self.p.can_create_replica_without_leader()) + + def test_replica_method_can_work_without_leader(self): + self.assertFalse(self.p.replica_method_can_work_without_leader('basebackup')) + self.assertFalse(self.p.replica_method_can_work_without_leader('foobar')) + self.p.config['foo'] = {'command': 'bar', 'no_master': 1} + self.assertTrue(self.p.replica_method_can_work_without_leader('foo')) + self.p.config['foo'] = {'command': 'bar'} + self.assertFalse(self.p.replica_method_can_work_without_leader('foo')) diff --git a/tests/test_wale_restore.py b/tests/test_wale_restore.py index 05f34187..ba4c3bea 100644 --- a/tests/test_wale_restore.py +++ b/tests/test_wale_restore.py @@ -58,7 +58,7 @@ class TestWALERestore(unittest.TestCase): def setUp(self): self.wale_restore = WALERestore("batman", "/data", - "host=batman port=5432 user=batman", "/etc", 100, 100, 1) + "host=batman port=5432 user=batman", "/etc", 100, 100, 1, 0) def tearDown(self): pass @@ -76,6 +76,8 @@ class TestWALERestore(unittest.TestCase): self.assertFalse(self.wale_restore.should_use_s3_to_create_replica()) self.wale_restore.should_use_s3_to_create_replica() + self.wale_restore.no_master = 1 + self.assertTrue(self.wale_restore.should_use_s3_to_create_replica()) def test_create_replica_with_s3(self): with patch('subprocess.call', MagicMock(return_value=0)): From 34437550d41013236ceabbb6d7cff75231b1e75b Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Tue, 26 Jan 2016 15:40:53 +0100 Subject: [PATCH 06/16] Fix a new flake8 warning (line break before the binary operator) --- patroni/ha.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 0feb172b..453ec03e 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -111,8 +111,8 @@ class Ha: def follow(self, demote_reason, follow_reason, refresh=True, recovery=False): refresh and self.load_cluster_from_dcs() - ret = demote_reason if (not recovery and self.state_handler.is_leader() - or recovery and self.state_handler.role == 'master') else follow_reason + ret = demote_reason if (not recovery and self.state_handler.is_leader() or + recovery and self.state_handler.role == 'master') else follow_reason # determine the node to follow. If replicatefrom tag is set, # try to follow the node mentioned there, otherwise, follow the leader. if self.patroni.replicatefrom: From aa350b71394fe24ddd8691f16d035f1e4b1ae3e6 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Tue, 26 Jan 2016 15:57:23 +0100 Subject: [PATCH 07/16] Increase the tests coverage. --- tests/test_patroni.py | 5 +++++ tests/test_wale_restore.py | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 18f5c14b..d1db99cb 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -80,3 +80,8 @@ class TestPatroni(unittest.TestCase): self.assertTrue(self.p.nofailover) self.p.tags['nofailover'] = None self.assertFalse(self.p.nofailover) + + def test_replicatefrom(self): + self.assertIsNone(self.p.replicatefrom) + self.p.tags['replicatefrom'] = 'foo' + self.assertEqual(self.p.replicatefrom, 'foo') diff --git a/tests/test_wale_restore.py b/tests/test_wale_restore.py index ba4c3bea..f353a4c9 100644 --- a/tests/test_wale_restore.py +++ b/tests/test_wale_restore.py @@ -3,7 +3,7 @@ from mock import MagicMock, patch, PropertyMock import os import psycopg2 import subprocess -from patroni.scripts.wale_restore import WALERestore +from patroni.scripts.wale_restore import WALERestore, main def fake_cursor_fetchone(*args, **kwargs): @@ -91,3 +91,7 @@ class TestWALERestore(unittest.TestCase): with patch.object(self.wale_restore, 'should_use_s3_to_create_replica', MagicMock(return_value=True)): with patch.object(self.wale_restore, 'create_replica_with_s3', MagicMock(return_value=0)): self.assertEqual(self.wale_restore.run(), 0) + + def test_main(self): + with patch('sys.exit', MagicMock(return_value=0)): + main() From c9de062ef5de8121b40e930b5f5b46e6f670e992 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Tue, 26 Jan 2016 16:07:17 +0100 Subject: [PATCH 08/16] declare the test as a static method to make the code analyzing tool happy. --- tests/test_wale_restore.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_wale_restore.py b/tests/test_wale_restore.py index f353a4c9..e922a9db 100644 --- a/tests/test_wale_restore.py +++ b/tests/test_wale_restore.py @@ -92,6 +92,7 @@ class TestWALERestore(unittest.TestCase): with patch.object(self.wale_restore, 'create_replica_with_s3', MagicMock(return_value=0)): self.assertEqual(self.wale_restore.run(), 0) + @staticmethod def test_main(self): with patch('sys.exit', MagicMock(return_value=0)): main() From 72d30974ad2cdbbf11b332a437b369640c266bff Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Tue, 26 Jan 2016 16:22:28 +0100 Subject: [PATCH 09/16] Another attempt at making the Quantifiedcode happy. --- tests/test_wale_restore.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_wale_restore.py b/tests/test_wale_restore.py index e922a9db..aa834b96 100644 --- a/tests/test_wale_restore.py +++ b/tests/test_wale_restore.py @@ -92,7 +92,6 @@ class TestWALERestore(unittest.TestCase): with patch.object(self.wale_restore, 'create_replica_with_s3', MagicMock(return_value=0)): self.assertEqual(self.wale_restore.run(), 0) - @staticmethod def test_main(self): with patch('sys.exit', MagicMock(return_value=0)): - main() + self.assertEqual(main(), None) From d1e54174c718133352e5b98a7d4e4f80f4b3a29f Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 27 Jan 2016 13:13:52 +0100 Subject: [PATCH 10/16] Make the code slightly more readable. --- patroni/ha.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/ha.py b/patroni/ha.py index 453ec03e..4f35cd8c 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -63,7 +63,7 @@ class Ha: self.dcs.touch_member(json.dumps(data, separators=(',', ':'))) def copy_backup_from_leader(self, leader): - if self.state_handler.bootstrap(True, leader): + if self.state_handler.bootstrap(initialize=True, current_leader=leader): logger.info('bootstrapped from leader' if leader else 'bootstrapped without leader') else: self.state_handler.stop('immediate') From 1d689d1e27c894bb68518701c6381781b2a7ed1a Mon Sep 17 00:00:00 2001 From: Feike Steenbergen Date: Mon, 1 Feb 2016 12:54:23 +0100 Subject: [PATCH 11/16] Spelling --- patroni/ha.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 4f35cd8c..8974b815 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -25,7 +25,7 @@ class Ha: def load_cluster_from_dcs(self): cluster = self.dcs.get_cluster() - # We want to keep the state of cluster when it was healhy + # We want to keep the state of cluster when it was healthy if not cluster.is_unlocked() or not self.old_cluster: self.old_cluster = cluster self.cluster = cluster @@ -296,7 +296,7 @@ class Ha: return self.enforce_master_role('acquired session lock as a leader', 'promoted self to leader by acquiring session lock') else: - return self.follow('demoted self due after trying and failing to obtain lock', + return self.follow('demoted self after trying and failing to obtain lock', 'following new leader after trying and failing to obtain lock') else: if self.patroni.nofailover: @@ -402,7 +402,7 @@ class Ha: if self._async_executor.busy: return self.handle_long_action_in_progress() - # we've go here, so async action has finished. Check if we tried to recover and failed + # we've got here, so any async action has finished. Check if we tried to recover and failed if self.recovering: self.recovering = False msg = self.post_recover() @@ -441,7 +441,7 @@ class Ha: finally: # we might not have a valid PostgreSQL connection here if another thread # stops PostgreSQL, therefore, we only reload replication slots if no - # asyncrhonous processes are running (should be always the case for the master) + # asynchronous processes are running (should be always the case for the master) if not self._async_executor.busy: self.state_handler.sync_replication_slots(self.cluster) except DCSError: @@ -450,7 +450,7 @@ class Ha: 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 later') + logger.exception('Error communicating with PostgreSQL. Will try again later') def run_cycle(self): with self._async_executor: From 1a8eaf8b936241c8b19ce4e1cfa09c69d835f5d2 Mon Sep 17 00:00:00 2001 From: Feike Steenbergen Date: Mon, 1 Feb 2016 13:02:21 +0100 Subject: [PATCH 12/16] Spelling: Even spelling can be tested --- tests/test_ha.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_ha.py b/tests/test_ha.py index 4c8500f4..ec02de97 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -167,7 +167,7 @@ class TestHa(unittest.TestCase): def test_demote_after_failing_to_obtain_lock(self): self.ha.acquire_lock = false - self.assertEquals(self.ha.run_cycle(), 'demoted self due after trying and failing to obtain lock') + self.assertEquals(self.ha.run_cycle(), 'demoted self after trying and failing to obtain lock') def test_follow_new_leader_after_failing_to_obtain_lock(self): self.ha.is_healthiest_node = true From 458f12f8a25deb7ccb512027fd32ad81e69afe8b Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Thu, 4 Feb 2016 12:24:31 +0100 Subject: [PATCH 13/16] Rename the badly named parameter. --- patroni/ha.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/ha.py b/patroni/ha.py index 8974b815..7ceac620 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -63,7 +63,7 @@ class Ha: self.dcs.touch_member(json.dumps(data, separators=(',', ':'))) def copy_backup_from_leader(self, leader): - if self.state_handler.bootstrap(initialize=True, current_leader=leader): + if self.state_handler.bootstrap(cluster_initialized=True, current_leader=leader): logger.info('bootstrapped from leader' if leader else 'bootstrapped without leader') else: self.state_handler.stop('immediate') From 1a87bbd830849ebe0b2062356e1ca3e1e65858f1 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Thu, 4 Feb 2016 15:31:22 +0100 Subject: [PATCH 14/16] Fix handling of replication slots on the master. Master shouldn't keep a replication slot for the members that replicate from other members instead of the master (replicatefrom). Otherwise, the master will keep collecting WAL segments that won't be requested ever. Of course, if the destination of replicatefrom is not part of the cluster, master should create the slot. --- patroni/dcs.py | 3 +++ patroni/postgresql.py | 13 ++++++++++--- tests/test_postgresql.py | 3 ++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/patroni/dcs.py b/patroni/dcs.py index 85fda68c..62e5a654 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -111,6 +111,9 @@ class Cluster(namedtuple('Cluster', 'initialize,leader,last_leader_operation,mem def is_unlocked(self): return not (self.leader and self.leader.name) + def has_member(self, member_name): + return len([m for m in self.members if m.name == member_name]) > 0 + class AbstractDCS: diff --git a/patroni/postgresql.py b/patroni/postgresql.py index f329fad5..293f7b51 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -651,11 +651,18 @@ $$""".format(name, options), name, password, password) if self.use_slots: try: self.load_replication_slots() + # if the replicatefrom tag is set on the member - we should not create the replication slot for it on + # the current master, because that member would replicate from elsewhere. We still create the slot if + # the replicatefrom destination member is currently not a member of the cluster (fallback to the + # master), or if replicatefrom destination member happens to be the current master if self.role == 'master': - slots = [m.name for m in cluster.members if m.name != self.name] + slots = [m.name for m in cluster.members if m.name != self.name and + (not cluster.has_member(m.replicatefrom) + if m.replicatefrom and m.replicatefrom != self.name else True)] else: # only manage slots for replicas that want to replicate from this one slots = [m.name for m in cluster.members if m.replicatefrom == self.name] + logger.info("setting replication slots for members {0}".format(slots)) # drop unused slots for slot in set(self.replication_slots) - set(slots): self.query("""SELECT pg_drop_replication_slot(%s) @@ -675,7 +682,7 @@ $$""".format(name, options), name, password, password) def last_operation(self): return str(self.xlog_position()) - def bootstrap(self, initialize=False, current_leader=None): + def bootstrap(self, cluster_initialized=False, current_leader=None): """ Populate PostgreSQL data directory by doing one of the following: - create with initdb if there is no master. @@ -696,7 +703,7 @@ $$""".format(name, options), name, password, password) that should be retried in the future. """ ret = False - if not (initialize or current_leader): + if not (cluster_initialized or current_leader): ret = self.initialize() and self.start() if ret: self.create_replication_user() diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 3681562f..a82bde99 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -181,7 +181,8 @@ class TestPostgresql(unittest.TestCase): os.makedirs(self.p.data_dir) 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.other = Member(0, 'test1', 28, {'conn_url': 'postgres://replicator:rep-pass@127.0.0.1:5433/postgres', + 'tags': {'replicatefrom': 'leader'}}) self.me = Member(0, 'test0', 28, {'conn_url': 'postgres://replicator:rep-pass@127.0.0.1:5434/postgres'}) def tearDown(self): From 03b56ae5b90ece76c572ff313e8e7b718aec3848 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Thu, 4 Feb 2016 19:14:22 +0100 Subject: [PATCH 15/16] Code refactoring per review by Alex Shulgin. In particular, rename most of the functions that have leader in the name if they can be called in the context where the leader is None. --- patroni/dcs.py | 2 +- patroni/ha.py | 10 +++++----- patroni/postgresql.py | 8 ++++---- tests/test_postgresql.py | 6 +++--- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/patroni/dcs.py b/patroni/dcs.py index 62e5a654..a6b9d856 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -112,7 +112,7 @@ class Cluster(namedtuple('Cluster', 'initialize,leader,last_leader_operation,mem return not (self.leader and self.leader.name) def has_member(self, member_name): - return len([m for m in self.members if m.name == member_name]) > 0 + return any(m for m in self.members if m.name == member_name) class AbstractDCS: diff --git a/patroni/ha.py b/patroni/ha.py index 7ceac620..7e78eb77 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -62,7 +62,7 @@ class Ha: pass self.dcs.touch_member(json.dumps(data, separators=(',', ':'))) - def copy_backup_from_leader(self, leader): + def clone(self, leader): if self.state_handler.bootstrap(cluster_initialized=True, current_leader=leader): logger.info('bootstrapped from leader' if leader else 'bootstrapped without leader') else: @@ -73,7 +73,7 @@ class Ha: def bootstrap(self): 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, )) + self._async_executor.run_async(self.clone, args=(self.cluster.leader, )) return 'trying to bootstrap from leader' elif not self.cluster.initialize and not self.patroni.nofailover: # no initialize key if self.dcs.initialize(create_new=True): # race for initialization @@ -93,7 +93,7 @@ class Ha: return 'failed to acquire initialize lock' else: if self.state_handler.can_create_replica_without_leader(): - self._async_executor.run_async(self.copy_backup_from_leader, args=(None, )) + self._async_executor.run_async(self.clone, args=(None, )) return "trying to bootstrap without leader" return 'waiting for leader to bootstrap' @@ -120,7 +120,7 @@ class Ha: node_to_follow = node_to_follow[0] if node_to_follow else self.cluster.leader else: node_to_follow = self.cluster.leader - node_to_follow = None if (node_to_follow and node_to_follow.name) == self.state_handler.name else node_to_follow + node_to_follow = None if node_to_follow and node_to_follow.name == self.state_handler.name else node_to_follow if not self.state_handler.check_recovery_conf(node_to_follow) or recovery: self._async_executor.schedule('changing primary_conninfo and restarting') self._async_executor.run_async(self.state_handler.follow, (node_to_follow, recovery)) @@ -350,7 +350,7 @@ class Ha: def reinitialize(self, cluster): self.state_handler.stop('immediate') self.state_handler.remove_data_directory() - self.copy_backup_from_leader(cluster.leader) + self.clone(cluster.leader) def process_scheduled_action(self): if self.reinitialize_scheduled(): diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 293f7b51..113d8e31 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -219,7 +219,7 @@ class Postgresql: env['PGPASSFILE'] = self.pgpass return env - def sync_from_leader(self, leader): + def sync_replica(self, leader): if leader: r = parseurl(leader.conn_url) env = self.write_pgpass(r) if leader else os.environ.copy() @@ -657,8 +657,8 @@ $$""".format(name, options), name, password, password) # master), or if replicatefrom destination member happens to be the current master if self.role == 'master': slots = [m.name for m in cluster.members if m.name != self.name and - (not cluster.has_member(m.replicatefrom) - if m.replicatefrom and m.replicatefrom != self.name else True)] + (m.replicatefrom is None or m.replicatefrom == self.name or + not cluster.has_member(m.replicatefrom))] else: # only manage slots for replicas that want to replicate from this one slots = [m.name for m in cluster.members if m.replicatefrom == self.name] @@ -711,7 +711,7 @@ $$""".format(name, options), name, password, password) else: raise PostgresException("Could not bootstrap master PostgreSQL") else: - if self.sync_from_leader(current_leader): + if self.sync_replica(current_leader): self.restore_configuration_files() self.write_recovery_conf(current_leader, True) ret = self.start() diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index a82bde99..a88f150c 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -229,8 +229,8 @@ class TestPostgresql(unittest.TestCase): 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)) + def test_sync_replica(self): + self.assertTrue(self.p.sync_replica(self.leader)) @patch('subprocess.call', side_effect=Exception("Test")) @patch('patroni.postgresql.Postgresql.write_pgpass', MagicMock(return_value=dict())) @@ -370,7 +370,7 @@ class TestPostgresql(unittest.TestCase): with patch('subprocess.call', Mock(return_value=1)): self.assertRaises(PostgresException, self.p.bootstrap) self.p.bootstrap() - with patch('patroni.postgresql.Postgresql.sync_from_leader', MagicMock(return_value=True)): + with patch('patroni.postgresql.Postgresql.sync_replica', MagicMock(return_value=True)): self.p.bootstrap(self.leader) def test_remove_data_directory(self): From 09ecd1cbece954beab765c772b9628eb1d673ffe Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Thu, 4 Feb 2016 19:27:26 +0100 Subject: [PATCH 16/16] Fix another issue with replication slots. Do not try to create replication slots on the replica for the member that wants to replicate from it if the member's currently holds the master role. Remove a debug message. --- patroni/postgresql.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 113d8e31..44d5acc3 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -660,9 +660,9 @@ $$""".format(name, options), name, password, password) (m.replicatefrom is None or m.replicatefrom == self.name or not cluster.has_member(m.replicatefrom))] else: - # only manage slots for replicas that want to replicate from this one - slots = [m.name for m in cluster.members if m.replicatefrom == self.name] - logger.info("setting replication slots for members {0}".format(slots)) + # only manage slots for replicas that replicate from this one, except for the leader among them + slots = [m.name for m in cluster.members if m.replicatefrom == self.name and + m.name != cluster.leader.name] # drop unused slots for slot in set(self.replication_slots) - set(slots): self.query("""SELECT pg_drop_replication_slot(%s)