From a844920489425496c4553fd174df6472e028859b Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Fri, 16 Oct 2015 16:14:45 +0200 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 90c738d83a4897f1292d38c59e99a4e80945b578 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 19 Oct 2015 16:03:21 +0200 Subject: [PATCH 5/7] 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 6/7] 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 44a73982d4dda64618345142f0a3381aaafa539a Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 21 Oct 2015 12:00:03 +0200 Subject: [PATCH 7/7] 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 []