From d422e16aade677503c7cd3b9c05b6f5b0d4ca15e Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 13 May 2016 13:31:21 +0200 Subject: [PATCH 01/49] Implement reload of config.yaml on SIGHUP If some changes require restart of postgres patroni will expose `restart_pending` flag in DCS and via REST API --- patroni/__init__.py | 57 +++++++++++++++++++++++++----------- patroni/api.py | 62 +++++++++++++++++++++++++++------------- patroni/consul.py | 15 +++++++--- patroni/dcs.py | 10 +++++-- patroni/etcd.py | 9 ++++-- patroni/ha.py | 2 ++ patroni/postgresql.py | 49 ++++++++++++++++++++++++++----- patroni/zookeeper.py | 8 ++++++ requirements.txt | 2 +- tests/test_api.py | 5 +++- tests/test_patroni.py | 12 +++++--- tests/test_postgresql.py | 15 ++-------- tests/test_zookeeper.py | 3 ++ 13 files changed, 178 insertions(+), 71 deletions(-) diff --git a/patroni/__init__.py b/patroni/__init__.py index 161270cf..09604ca6 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -1,5 +1,6 @@ import logging import os +import signal import sys import time import yaml @@ -17,10 +18,12 @@ logger = logging.getLogger(__name__) class Patroni(object): PATRONI_CONFIG_VARIABLE = 'PATRONI_CONFIGURATION' - def __init__(self, config): + def __init__(self, config_file=None, config_env=None): + self._config_file = config_file + config = yaml.load(config_env) if config_env else self._load_config() + self.nap_time = config['loop_wait'] - self.tags = {tag: value for tag, value in config.get('tags', {}).items() - if tag not in ('clonefrom', 'nofailover', 'noloadbalance') or value} + self.tags = self.get_tags(config) self.postgresql = Postgresql(config['postgresql']) self.dcs = self.get_dcs(self.postgresql.name, config) self.version = __version__ @@ -28,6 +31,32 @@ class Patroni(object): self.ha = Ha(self) self.next_run = time.time() + self._reload_config_scheduled = False + + @staticmethod + def get_tags(config): + return {tag: value for tag, value in config.get('tags', {}).items() + if tag not in ('clonefrom', 'nofailover', 'noloadbalance') or value} + + def _load_config(self, fail=True): + with open(self._config_file) as f: + return yaml.load(f) + + def reload_config(self): + try: + config = self._load_config() + self.tags = self.get_tags(config) + self.nap_time = config['loop_wait'] + self.dcs.set_ttl(config.get('ttl') or 30) + self.api.reload_config(config['restapi']) + self.postgresql.reload_config(config['postgresql']) + except Exception: + logger.exception('Failed to reload config_file=%s', self._config_file) + self._reload_config_scheduled = False + + def sighup_handler(self, *args): + self._reload_config_scheduled = True + @property def noloadbalance(self): return self.tags.get('noloadbalance', False) @@ -64,38 +93,34 @@ class Patroni(object): def run(self): self.api.start() + signal.signal(signal.SIGHUP, self.sighup_handler) self.next_run = time.time() while True: + if self._reload_config_scheduled: + self.reload_config() logger.info(self.ha.run_cycle()) reap_children() self.schedule_next_run() 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() # Patroni reads the configuration from the command-line argument if it exists, and from the environment otherwise. - use_env = False - use_file = (len(sys.argv) >= 2 and os.path.isfile(sys.argv[1])) - if not use_file: + config_env = False + config_file = len(sys.argv) >= 2 and os.path.isfile(sys.argv[1]) and sys.argv[1] + if not config_file: config_env = os.environ.get(Patroni.PATRONI_CONFIG_VARIABLE) - use_env = config_env is not None - if not use_env: + if config_env is None: print('Usage: {0} config.yml'.format(sys.argv[0])) print('\tPatroni may also read the configuration from the {} environment variable'. format(Patroni.PATRONI_CONFIG_VARIABLE)) return - if use_file: - with open(sys.argv[1], 'r') as f: - config = yaml.load(f) - elif use_env: - config = yaml.load(config_env) - - patroni = Patroni(config) + patroni = Patroni(config_file, config_env) try: patroni.run() except KeyboardInterrupt: diff --git a/patroni/api.py b/patroni/api.py index 526da8b4..33404f28 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -71,6 +71,8 @@ class RestApiHandler(BaseHTTPRequestHandler): response.update({'tags': patroni.tags} if patroni.tags else {}) if patroni.postgresql.sysid: response['database_system_identifier'] = patroni.postgresql.sysid + if patroni.postgresql.restart_pending: + response['restart_pending'] = True response['patroni'] = {'version': patroni.version, 'scope': patroni.postgresql.scope} body = json.dumps(response) self._write_response(status_code, body, {'Content-Type': 'application/json'}) @@ -294,25 +296,9 @@ class RestApiHandler(BaseHTTPRequestHandler): class RestApiServer(ThreadingMixIn, HTTPServer, Thread): def __init__(self, patroni, config): - self._auth_key = base64.b64encode(config['auth'].encode('utf-8')).decode('utf-8') if 'auth' in config else None - host, port = config['listen'].split(':') - HTTPServer.__init__(self, (host, int(port)), RestApiHandler) - Thread.__init__(self, target=self.serve_forever) - self._set_fd_cloexec(self.socket) - - protocol = 'http' - - # wrap socket with ssl if 'certfile' is defined in a config.yaml - # Sometime it's also needed to pass reference to a 'keyfile'. - options = {option: config[option] for option in ['certfile', 'keyfile'] if option in config} - if options.get('certfile'): - import ssl - self.socket = ssl.wrap_socket(self.socket, server_side=True, **options) - protocol = 'https' - - self.connection_string = '{0}://{1}/patroni'.format(protocol, config.get('connect_address', config['listen'])) - self.patroni = patroni + self.__initialize(config) + self.__set_config_parameters(config) self.daemon = True def query(self, sql, *params): @@ -332,11 +318,47 @@ class RestApiServer(ThreadingMixIn, HTTPServer, Thread): fcntl.fcntl(fd, fcntl.F_SETFD, flags | fcntl.FD_CLOEXEC) def check_basic_auth_key(self, key): - return self._auth_key == key + return self.__auth_key == key def check_auth_header(self, auth_header): - if self._auth_key: + if self.__auth_key: if auth_header is None: return 'no auth header received' if not auth_header.startswith('Basic ') or not self.check_basic_auth_key(auth_header[6:]): return 'not authenticated' + + @staticmethod + def __get_ssl_options(config): + return {option: config[option] for option in ['certfile', 'keyfile'] if option in config} + + def __set_connection_string(self, connect_address): + self.connection_string = '{0}://{1}/patroni'.format(self.__protocol, connect_address or self.__listen) + + def __set_config_parameters(self, config): + self.__auth_key = base64.b64encode(config['auth'].encode('utf-8')).decode('utf-8') if 'auth' in config else None + self.__set_connection_string(config.get('connect_address')) + + def __initialize(self, config): + self.__ssl_options = self.__get_ssl_options(config) + self.__listen = config['listen'] + host, port = config['listen'].split(':') + HTTPServer.__init__(self, (host, int(port)), RestApiHandler) + Thread.__init__(self, target=self.serve_forever) + self._set_fd_cloexec(self.socket) + + self.__protocol = 'http' + + # wrap socket with ssl if 'certfile' is defined in a config.yaml + # Sometime it's also needed to pass reference to a 'keyfile'. + if self.__ssl_options.get('certfile'): + import ssl + self.socket = ssl.wrap_socket(self.socket, server_side=True, **self.__ssl_options) + self.__protocol = 'https' + self.__set_connection_string(config.get('connect_address')) + + def reload_config(self, config): + self.__set_config_parameters(config) + if self.__listen != config['listen'] or self.__ssl_options != self.__get_ssl_options(config): + self.shutdown() + self.__initialize(config) + self.start() diff --git a/patroni/consul.py b/patroni/consul.py index a033aeda..81f430bc 100644 --- a/patroni/consul.py +++ b/patroni/consul.py @@ -72,12 +72,13 @@ class Consul(AbstractDCS): def __init__(self, name, config): super(Consul, self).__init__(name, config) - self.ttl = int((config.get('ttl') or 30)/2) # My experiments have shown that session expires after 2*ttl time + self._ttl = None + self._session = None + self._my_member_data = None + self.set_ttl(config.get('ttl') or 30) host, port = config.get('host', '127.0.0.1:8500').split(':') self._client = ConsulClient(host=host, port=port) self._scope = config['scope'] - self._session = None - self._my_member_data = None self.create_or_restore_session() def create_or_restore_session(self): @@ -91,6 +92,12 @@ class Consul(AbstractDCS): logger.info('waiting on consul') sleep(5) + def set_ttl(self, ttl): + ttl = int(ttl/2) # My experiments have shown that session expires after 2*ttl time + if self._ttl != ttl: + self._session = None + self._ttl = ttl + def refresh_session(self): """:returns: `!True` if it had to create new session""" if self._session: @@ -101,7 +108,7 @@ class Consul(AbstractDCS): if not self._session: name = self._scope + '-' + self._name try: - self._session = self._client.session.create(name=name, lock_delay=0, behavior='delete', ttl=self.ttl) + self._session = self._client.session.create(name=name, lock_delay=0, behavior='delete', ttl=self._ttl) except (ConsulException, RequestException): logger.exception('session.create') if not self._session: diff --git a/patroni/dcs.py b/patroni/dcs.py index 7ae70269..957eccae 100644 --- a/patroni/dcs.py +++ b/patroni/dcs.py @@ -215,6 +215,10 @@ class AbstractDCS(object): def leader_optime_path(self): return self.client_path(self._LEADER_OPTIME) + @abc.abstractmethod + def set_ttl(self, ttl): + """Set the new ttl value for leader key""" + @abc.abstractmethod def _load_cluster(self): """Internally this method should build `Cluster` object which @@ -285,12 +289,12 @@ class AbstractDCS(object): return self.set_failover_value(json.dumps(failover_value), index) @abc.abstractmethod - def touch_member(self, connection_string, ttl=None): + def touch_member(self, data, ttl=None): """Update member key in DCS. This method should create or update key with the name = '/members/' + `~self._name` - and value = connection_string in a given DCS. + and value = data in a given DCS. - :param connection_string: how this instance can be accessed by other instances + :param data: json serialized information about instance (including connection strings) :param ttl: ttl for member key, optional parameter. If it is None `~self.member_ttl will be used` :returns: `!True` on success otherwise `!False` """ diff --git a/patroni/etcd.py b/patroni/etcd.py index 47f0b636..f06cca3d 100644 --- a/patroni/etcd.py +++ b/patroni/etcd.py @@ -193,7 +193,7 @@ class Etcd(AbstractDCS): def __init__(self, name, config): super(Etcd, self).__init__(name, config) - self.ttl = config.get('ttl', 30) + self.set_ttl(config.get('ttl', 30)) self._retry = Retry(deadline=10, max_delay=1, max_tries=-1, retry_exceptions=(etcd.EtcdConnectionFailed, etcd.EtcdLeaderElectionInProgress, @@ -215,6 +215,9 @@ class Etcd(AbstractDCS): sleep(5) return client + def set_ttl(self, ttl): + self.ttl = int(ttl) + @staticmethod def member(node): return Member.from_node(node.modifiedIndex, os.path.basename(node.key), node.ttl, node.value) @@ -255,8 +258,8 @@ class Etcd(AbstractDCS): raise EtcdError('Etcd is not responding properly') @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.ttl) + def touch_member(self, data, ttl=None): + return self.retry(self._client.set, self.member_path, data, ttl or self.ttl) @catch_etcd_errors def take_leader(self): diff --git a/patroni/ha.py b/patroni/ha.py index 1e825d4f..e1772eb5 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -59,6 +59,8 @@ class Ha(object): } if self.patroni.tags: data['tags'] = self.patroni.tags + if self.state_handler.restart_pending: + data['restart_pending'] = True if not self._async_executor.busy and data['state'] in ['running', 'restarting', 'starting']: try: data['xlog_location'] = self.state_handler.xlog_position() diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 060932c7..1197aa08 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -44,12 +44,15 @@ class Postgresql(object): def __init__(self, config): self.config = config self.name = config['name'] + self._restart_pending = False self._server_parameters = self.get_server_parameters(config) self._listen_addresses, self._port = (config['listen'] + ':5432').split(':')[:2] + self._connect_address = config.get('connect_address') + self.replication = config['replication'] + self.resolve_connection_addresses() self.scope = config['scope'] self._data_dir = config['data_dir'] - self.replication = config['replication'] self.superuser = config.get('superuser') or {} self.admin = config.get('admin') or {} @@ -71,11 +74,6 @@ class Postgresql(object): self._pg_ctl = ['pg_ctl', '-w', '-D', self._data_dir] - self.local_address = self.get_local_address() - connect_address = config.get('connect_address') or self.local_address - self.connection_string = 'postgres://{username}:{password}@{connect_address}/postgres'.format( - connect_address=connect_address, **self.replication) - self._connection = None self._cursor_holder = None self._sysid = None @@ -96,6 +94,40 @@ class Postgresql(object): def get_server_parameters(config): return {p: v for p, v in (config.get('parameters') or {}).items() if p not in ('listen_addresses', 'port')} + def resolve_connection_addresses(self): + self.local_address = self.get_local_address() + self.connection_string = 'postgres://{username}:{password}@{connect_address}/postgres'.format( + connect_address=self._connect_address or self.local_address, **self.replication) + + def reload_config(self, config): + server_parameters = self.get_server_parameters(config) + self._connect_address = config.get('connect_address') + listen_addresses, port = (config['listen'] + ':5432').split(':')[:2] + if self._listen_addresses == listen_addresses and self._port == port: + self.resolve_connection_addresses() + + self._listen_addresses = listen_addresses + self._port = port + if self.is_healthy(): + changes = server_parameters.copy() + changes.update({p: None for p, v in self._server_parameters.items() if p not in server_parameters}) + if changes: + for r in self.query("""SELECT name, setting + FROM pg_settings + WHERE context in ('internal', 'postmaster') + AND name IN (""" + ', '.join('%s' for _ in changes.keys()) + ')', + *(list(changes.keys()))): + if server_parameters[r[0]] is None or str(server_parameters[r[0]]) != str(r[1]): + self._restart_pending = True + break + self._server_parameters = server_parameters + self._write_postgresql_conf() + self.reload() + + @property + def restart_pending(self): + return self._restart_pending + @property def can_rewind(self): """ check if pg_rewind executable is there and that pg_controldata indicates @@ -378,10 +410,13 @@ class Postgresql(object): self._write_postgresql_conf() server_arguments = ['-o', "--listen_addresses='{0}' --port={1}".format(self._listen_addresses, self._port)] ret = subprocess.call(self._pg_ctl + ['start'] + server_arguments, env=env, preexec_fn=os.setsid) == 0 + self._restart_pending = False self.set_state('running' if ret else 'start failed') + if ret: + self.resolve_connection_addresses() + self._schedule_load_slots = self.use_slots - 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 diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index e245e5e6..22f2f5ad 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -103,6 +103,14 @@ class ZooKeeper(AbstractDCS): self._fetch_cluster = True self.event.set() + def set_ttl(self, ttl): + ttl = int(ttl * 1000) + # I know, it's weird to access private attributes and method + # but there is no other way to change session_timeout without losing session + if self._client._session_timeout != ttl: + self._client._session_timeout = ttl + self._client._connection._socket.close() + def get_node(self, key, watch=None): try: ret = self._client.get(key, watch) diff --git a/requirements.txt b/requirements.txt index 57ed4ea9..8b9b3c79 100644 --- a/requirements.txt +++ b/requirements.txt @@ -3,7 +3,7 @@ psycopg2>=2.6.1 PyYAML requests six >= 1.7 -kazoo>=2.2.1 +kazoo==2.2.1 python-etcd==0.4.3 python-consul==0.6.0 click>=4.1 diff --git a/tests/test_api.py b/tests/test_api.py index 5871d240..34fbc353 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -19,6 +19,7 @@ class MockPostgresql(object): server_version = '999999' sysid = 'dummysysid' scope = 'dummy' + restart_pending = True @staticmethod def connection(): @@ -73,8 +74,10 @@ class MockRestApiServer(RestApiServer): BaseHTTPServer.HTTPServer.__init__ = Mock() MockRestApiServer._BaseServer__is_shut_down = Mock() MockRestApiServer._BaseServer__shutdown_request = True - config = {'listen': '127.0.0.1:8008', 'auth': 'test:test', 'certfile': 'dumb'} + config = {'listen': '127.0.0.1:8008', 'auth': 'test:test'} super(MockRestApiServer, self).__init__(MockPatroni(), config) + config['certfile'] = 'dumb' + self.reload_config(config) Handler(MockRequest(request), ('0.0.0.0', 8080), self) diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 6f7e67fe..cf249a68 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -3,7 +3,6 @@ import os import sys import time import unittest -import yaml from mock import Mock, patch from patroni.api import RestApiServer @@ -22,6 +21,7 @@ from test_zookeeper import MockKazooClient @patch('subprocess.call', Mock(return_value=0)) @patch('psycopg2.connect', psycopg2_connect) @patch.object(Postgresql, 'write_pg_hba', Mock()) +@patch.object(Postgresql, '_write_postgresql_conf', Mock()) @patch.object(Postgresql, 'write_recovery_conf', Mock()) @patch.object(BaseHTTPServer.HTTPServer, '__init__', Mock()) @patch.object(AsyncExecutor, 'run', Mock()) @@ -37,9 +37,7 @@ class TestPatroni(unittest.TestCase): RestApiServer._BaseServer__is_shut_down = Mock() RestApiServer._BaseServer__shutdown_request = True RestApiServer.socket = 0 - with open('postgres0.yml', 'r') as f: - config = yaml.load(f) - self.p = Patroni(config) + self.p = Patroni('postgres0.yml') @patch('patroni.zookeeper.KazooClient', MockKazooClient()) @patch.object(Consul, 'create_or_restore_session', Mock()) @@ -71,6 +69,7 @@ class TestPatroni(unittest.TestCase): del os.environ[Patroni.PATRONI_CONFIG_VARIABLE] def test_run(self): + self.p.sighup_handler() self.p.ha.dcs.watch = Mock(side_effect=SleepException) self.p.api.start = Mock() self.assertRaises(SleepException, self.p.run) @@ -95,3 +94,8 @@ class TestPatroni(unittest.TestCase): self.assertIsNone(self.p.replicatefrom) self.p.tags['replicatefrom'] = 'foo' self.assertEqual(self.p.replicatefrom, 'foo') + + def test_reload_config(self): + self.p.reload_config() + with patch('yaml.load', Mock(side_effect=Exception)): + self.p.reload_config() diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index f4b34ec0..c6efa841 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -34,19 +34,10 @@ class MockCursor(object): self.results = [(False, )] elif sql.startswith('SELECT to_char(pg_postmaster_start_time'): self.results = [('', True, '', '', '', '', False)] + elif sql.startswith('SELECT name, setting'): + self.results = [('archive_mode', 'off')] else: - self.results = [( - None, - None, - None, - None, - None, - None, - None, - None, - None, - None, - )] + self.results = [(None, None, None, None, None, None, None, None, None, None)] def fetchone(self): return self.results[0] diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py index 5e9ebc82..79516267 100644 --- a/tests/test_zookeeper.py +++ b/tests/test_zookeeper.py @@ -109,6 +109,9 @@ class TestZooKeeper(unittest.TestCase): def test_session_listener(self): self.zk.session_listener(KazooState.SUSPENDED) + def test_set_ttl(self): + self.zk.set_ttl(20) + def test_get_node(self): self.assertIsNone(self.zk.get_node('/no_node')) From 98c505a16bb8adf10b24dcbd9fe46a311257d0c7 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 13 May 2016 16:12:46 +0200 Subject: [PATCH 02/49] Remove unused argument --- patroni/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/__init__.py b/patroni/__init__.py index 09604ca6..7a9f3bdc 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -38,7 +38,7 @@ class Patroni(object): return {tag: value for tag, value in config.get('tags', {}).items() if tag not in ('clonefrom', 'nofailover', 'noloadbalance') or value} - def _load_config(self, fail=True): + def _load_config(self): with open(self._config_file) as f: return yaml.load(f) From 7827951c8cfe36fa32cabad6495673c88e4cf1b9 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 25 May 2016 14:17:05 +0200 Subject: [PATCH 03/49] Dynamic configuration --- features/environment.py | 6 +- patroni/__init__.py | 72 ++++++++----- patroni/config.py | 168 +++++++++++++++++++++++++++++ patroni/ctl.py | 3 +- patroni/dcs/__init__.py | 42 ++++++-- patroni/dcs/consul.py | 18 +++- patroni/dcs/etcd.py | 18 +++- patroni/dcs/zookeeper.py | 27 +++-- patroni/ha.py | 11 +- patroni/postgresql.py | 225 ++++++++++++++++++++++++--------------- postgres0.yml | 148 ++++++++++--------------- tests/test_api.py | 3 +- tests/test_config.py | 30 ++++++ tests/test_consul.py | 6 +- tests/test_ctl.py | 2 +- tests/test_etcd.py | 5 +- tests/test_ha.py | 26 ++++- tests/test_patroni.py | 16 ++- tests/test_postgresql.py | 87 +++++++++------ tests/test_zookeeper.py | 10 +- 20 files changed, 642 insertions(+), 281 deletions(-) create mode 100644 patroni/config.py create mode 100644 tests/test_config.py diff --git a/features/environment.py b/features/environment.py index bc1106f4..bd03df46 100644 --- a/features/environment.py +++ b/features/environment.py @@ -116,11 +116,12 @@ class PatroniController(AbstractController): config['postgresql']['listen'] = config['postgresql']['connect_address'] = '{0}:{1}'.format(host, self.__PORT) - user = config['postgresql'].get('superuser', {}) + user = config['postgresql'].get('authentication', config['postgresql']).get('superuser', {}) self._connkwargs = {k: user[n] for n, k in [('username', 'user'), ('password', 'password')] if n in user} self._connkwargs.update({'host': host, 'port': self.__PORT, 'database': 'postgres'}) - config['postgresql'].update({'name': name, 'data_dir': self._data_dir}) + config['name'] = name + config['postgresql']['data_dir'] = self._data_dir config['postgresql']['parameters'].update({ 'logging_collector': 'on', 'log_destination': 'csvlog', 'log_directory': self._output_dir, 'log_filename': name + '.log', 'log_statement': 'all', 'log_min_messages': 'debug1'}) @@ -135,7 +136,6 @@ class PatroniController(AbstractController): if dcs == 'consul': config[dcs] = dcs_config else: - dcs_config.update({'session_timeout': dcs_config.pop('ttl'), 'reconnect_timeout': config['loop_wait']}) if dcs == 'exhibitor': dcs_config['exhibitor'] = {'hosts': ['127.0.0.1'], 'port': 8181} else: diff --git a/patroni/__init__.py b/patroni/__init__.py index 1f6a2ab1..2addc264 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -3,10 +3,11 @@ import os import signal import sys import time -import yaml from patroni.api import RestApiServer +from patroni.config import Config from patroni.dcs import get_dcs +from patroni.exceptions import DCSError from patroni.ha import Ha from patroni.postgresql import Postgresql from patroni.utils import reap_children, set_ignore_sigterm, setup_signal_handlers @@ -19,43 +20,50 @@ class Patroni(object): PATRONI_CONFIG_VARIABLE = 'PATRONI_CONFIGURATION' def __init__(self, config_file=None, config_env=None): - self._config_file = config_file - config = yaml.load(config_env) if config_env else self._load_config() - - self.nap_time = config['loop_wait'] - self.tags = self.get_tags(config) - self.postgresql = Postgresql(config['postgresql']) - self.dcs = get_dcs(self.postgresql.name, config) self.version = __version__ - self.api = RestApiServer(self, config['restapi']) + self.config = Config(config_file=config_file, config_env=config_env) + self.dcs = get_dcs(self.config) + self.load_dynamic_configuration() + + self.postgresql = Postgresql(self.config['postgresql']) + self.api = RestApiServer(self, self.config['restapi']) self.ha = Ha(self) + + self.tags = self.get_tags() + self.nap_time = self.config['loop_wait'] self.next_run = time.time() self._reload_config_scheduled = False + self._received_sighup = False - @staticmethod - def get_tags(config): - return {tag: value for tag, value in config.get('tags', {}).items() + def load_dynamic_configuration(self): + while True: + try: + cluster = self.dcs.get_cluster() + if cluster and cluster.config: + self.config.set_dynamic_configuration(cluster.config.data) + elif not self.config.dynamic_configuration and 'bootstrap' in self.config: + self.config.set_dynamic_configuration(self.config['bootstrap']['dcs']) + break + except DCSError: + logger.warning('Can not get cluster from dcs') + + def get_tags(self): + return {tag: value for tag, value in self.config.get('tags', {}).items() if tag not in ('clonefrom', 'nofailover', 'noloadbalance') or value} - def _load_config(self): - with open(self._config_file) as f: - return yaml.load(f) - def reload_config(self): try: - config = self._load_config() - self.tags = self.get_tags(config) - self.nap_time = config['loop_wait'] - self.dcs.set_ttl(config.get('ttl') or 30) - self.api.reload_config(config['restapi']) - self.postgresql.reload_config(config['postgresql']) + self.tags = self.get_tags() + self.nap_time = self.config['loop_wait'] + self.dcs.set_ttl(self.config.get('ttl') or 30) + self.api.reload_config(self.config['restapi']) + self.postgresql.reload_config(self.config['postgresql']) except Exception: - logger.exception('Failed to reload config_file=%s', self._config_file) - self._reload_config_scheduled = False + logger.exception('Failed to reload config_file=%s', self.config.config_file) def sighup_handler(self, *args): - self._reload_config_scheduled = True + self._received_sighup = True @property def noloadbalance(self): @@ -84,9 +92,21 @@ class Patroni(object): self.next_run = time.time() while True: - if self._reload_config_scheduled: + if self._received_sighup: + self._received_sighup = False + self.config.reload_local_configuration() self.reload_config() + logger.info(self.ha.run_cycle()) + + cluster = self.dcs.cluster + if cluster and cluster.config and cluster.config.data and \ + self.config.set_dynamic_configuration(cluster.config.data): + self.reload_config() + + if not self.postgresql.data_directory_empty(): + self.config.save_cache() + reap_children() self.schedule_next_run() diff --git a/patroni/config.py b/patroni/config.py new file mode 100644 index 00000000..cf5ba85d --- /dev/null +++ b/patroni/config.py @@ -0,0 +1,168 @@ +import json +import logging +import os +import tempfile +import yaml + +from copy import deepcopy +from patroni.postgresql import Postgresql + +logger = logging.getLogger(__name__) + + +class Config(object): + """ + This class is responsible for: + + 1) Building and giving access to `effective_configuration` from: + * `Config.__DEFAULT_CONFIG` -- some sane default values + * `dynamic_configuration` -- configuration stored in DCS + * `local_configuration` -- configuration from `config.yml` or environment + + 2) Saving and loading `dynamic_configuration` into 'patroni.dynamic.json' file + located in local_configuration['postgresql']['data_dir'] directory. + This is necessary to be able to restore `dynamic_configuration` + if DCS was accidentally wiped + + 3) Loading of configuration file in the old format and converting it into new format + + 4) Mimicking some of the `dict` interfaces to make it possible + to work with it as with the old `config` object. + """ + + __CACHE_FILENAME = 'patroni.dynamic.json' + __DEFAULT_CONFIG = { + 'ttl': 30, 'loop_wait': 10, 'retry_timeout': 5, + 'maximum_lag_on_failover': 1048576, + 'postgresql': { + 'parameters': Postgresql.CMDLINE_OPTIONS + } + } + + def __init__(self, config_file=None, config_env=None): + self._config_file = None if config_env else config_file + self._dynamic_configuration = {} + self._local_configuration = yaml.safe_load(config_env) if config_env else self._load_config_file() + self._build_effective_configuration() + self._data_dir = self.__effective_configuration['postgresql']['data_dir'] + self._cache_file = os.path.join(self._data_dir, self.__CACHE_FILENAME) + self._load_cache() + self._cache_needs_saving = False + + @property + def config_file(self): + return self._config_file + + @property + def dynamic_configuration(self): + return deepcopy(self._dynamic_configuration) + + def _load_config_file(self): + with open(self._config_file) as f: + return yaml.safe_load(f) + + def _load_cache(self): + if os.path.isfile(self._cache_file): + try: + with open(self._cache_file) as f: + self.set_dynamic_configuration(json.load(f)) + except Exception: + logger.exception('Exception when loading file: %s', self._cache_file) + + def save_cache(self): + if self._cache_needs_saving: + tmpfile = fd = None + try: + (fd, tmpfile) = tempfile.mkstemp(prefix=self.__CACHE_FILENAME, dir=self._data_dir) + with os.fdopen(fd, 'w') as f: + fd = None + json.dump(self.dynamic_configuration, f) + tmpfile = os.rename(tmpfile, self._cache_file) + self._cache_needs_saving = False + except Exception: + logger.exception('Exception when saving file: %s', self._cache_file) + if fd: + try: + os.close(fd) + except Exception: + logger.error('Can not close temporary file %s', tmpfile) + if tmpfile and os.path.exists(tmpfile): + try: + os.remove(tmpfile) + except Exception: + logger.error('Can not remove temporary file %s', tmpfile) + + def set_dynamic_configuration(self, configuration): + if configuration and self._dynamic_configuration != configuration: + self._dynamic_configuration = configuration + self._build_effective_configuration() + self._cache_needs_saving = True + return True + + def reload_local_configuration(self): + self._local_configuration = self._load_config_file() + self._build_effective_configuration() + + def _process_postgresql_parameters(self, parameters, is_local=False): + ret = {} + for name, value in (parameters or {}).items(): + if (is_local and name not in self.__DEFAULT_CONFIG['postgresql']['parameters']) \ + or not ((name == 'wal_level' and value not in ('hot_standby', 'logical')) or + (name in ('max_replication_slots', 'max_wal_senders', 'wal_keep_segments') and + int(value) < self.__DEFAULT_CONFIG['postgresql']['parameters'][name]) or + name in ('hot_standby', 'wal_log_hints')): + ret[name] = value + return ret + + def _safe_copy_dynamic_configuration(self): + config = deepcopy(self.__DEFAULT_CONFIG) + + for name, value in self._dynamic_configuration.items(): + if name == 'postgresql': + for name, value in (value or {}).items(): + if name == 'parameters': + config['postgresql'][name].update(self._process_postgresql_parameters(value)) + elif name not in ('connect_address', 'listen', 'data_dir', 'pgpass', 'authentication'): + config['postgresql'][name] = deepcopy(value) + elif name in config: + config[name] = value + return config + + def _build_effective_configuration(self): + config = self._safe_copy_dynamic_configuration() + for name, value in self._local_configuration.items(): + if name == 'postgresql': + for name, value in (value or {}).items(): + if name == 'parameters': + config['postgresql'][name].update(self._process_postgresql_parameters(value, True)) + else: + config['postgresql'][name] = deepcopy(value) + elif name not in config: + config[name] = deepcopy(value) if value else {} + + pg_config = config['postgresql'] + + # special treatment for old config + if 'authentication' not in pg_config: + pg_config['use_pg_rewind'] = 'pg_rewind' in pg_config + pg_config['authentication'] = {u: pg_config[u] for u in ('replication', 'superuser') if u in pg_config} + + if 'superuser' not in pg_config['authentication'] and 'pg_rewind' in pg_config: + pg_config['authentication']['superuser'] = pg_config['pg_rewind'] + + if 'name' not in config and 'name' in pg_config: + config['name'] = pg_config['name'] + + pg_config.update({p: config[p] for p in ('name', 'scope', 'retry_timeout', + 'maximum_lag_on_failover') if p in config}) + + self.__effective_configuration = config + + def get(self, key, default=None): + return self.__effective_configuration.get(key, default) + + def __contains__(self, key): + return key in self.__effective_configuration + + def __getitem__(self, key): + return self.__effective_configuration[key] diff --git a/patroni/ctl.py b/patroni/ctl.py index 19b48feb..08622086 100644 --- a/patroni/ctl.py +++ b/patroni/ctl.py @@ -95,8 +95,9 @@ def ctl(ctx): def get_dcs(config, scope): config.setdefault('scope', scope) + config.setdefault('name', scope) try: - return _get_dcs(scope, config) + return _get_dcs(config) except PatroniException as e: raise PatroniCtlException(str(e)) diff --git a/patroni/dcs/__init__.py b/patroni/dcs/__init__.py index 2b543ff4..fcf258b7 100644 --- a/patroni/dcs/__init__.py +++ b/patroni/dcs/__init__.py @@ -30,7 +30,7 @@ def parse_connection_string(value): return conn_url, api_url -def get_dcs(node_name, config): +def get_dcs(config): available_implementations = [] for name in os.listdir(os.path.dirname(__file__)): if name.endswith('.py') and not name.startswith('__'): # find module @@ -44,8 +44,9 @@ def get_dcs(node_name, config): available_implementations.append(name) if name in config: # which has configuration section in the config file # propagate some parameters - config[name].update({p: config[p] for p in ('namespace', 'scope', 'ttl') if p in config}) - return value(node_name, config[name]) + config[name].update({p: config[p] for p in ('namespace', 'name', + 'scope', 'ttl', 'retry_timeout') if p in config}) + return value(config[name]) raise PatroniException("""Can not find suitable configuration of distributed configuration store Available implementations: """ + ', '.join(available_implementations)) @@ -163,11 +164,28 @@ class Failover(namedtuple('Failover', 'index,leader,candidate,scheduled_at')): return Failover(index, data.get('leader'), data.get('member'), data.get('scheduled_at')) -class Cluster(namedtuple('Cluster', 'initialize,leader,last_leader_operation,members,failover')): +class ClusterConfig(namedtuple('ClusterConfig', 'index,data')): + + @staticmethod + def from_node(index, data): + """ + >>> ClusterConfig.from_node(1, '{').data + {} + """ + + try: + data = json.loads(data) + except (TypeError, ValueError): + data = {} + return ClusterConfig(index, data) + + +class Cluster(namedtuple('Cluster', 'initialize,config,leader,last_leader_operation,members,failover')): """Immutable object (namedtuple) which represents PostgreSQL cluster. Consists of the following fields: :param initialize: boolean, shows whether this cluster has initialization key stored in DC or not. + :param config: global dynamic configuration, reference to `ClusterConfig` object :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 @@ -192,19 +210,19 @@ class Cluster(namedtuple('Cluster', 'initialize,leader,last_leader_operation,mem class AbstractDCS(object): _INITIALIZE = 'initialize' + _CONFIG = 'config' _LEADER = 'leader' _FAILOVER = 'failover' _MEMBERS = 'members/' _OPTIME = 'optime' _LEADER_OPTIME = _OPTIME + '/' + _LEADER - def __init__(self, name, config): + def __init__(self, config): """ - :param name: name of current instance (the same value as `~Postgresql.name`) :param config: dict, reference to config section of selected DCS. i.e.: `zookeeper` for zookeeper, `etcd` for etcd, etc... """ - self._name = name + self._name = config['name'] self._namespace = '/{0}'.format(config.get('namespace', '/service/').strip('/')) self._base_path = '/'.join([self._namespace, config['scope']]) @@ -219,6 +237,10 @@ class AbstractDCS(object): def initialize_path(self): return self.client_path(self._INITIALIZE) + @property + def config_path(self): + return self.client_path(self._CONFIG) + @property def members_path(self): return self.client_path(self._MEMBERS) @@ -310,7 +332,11 @@ class AbstractDCS(object): if scheduled_at: failover_value['scheduled_at'] = scheduled_at.isoformat() - return self.set_failover_value(json.dumps(failover_value), index) + return self.set_failover_value(json.dumps(failover_value, separators=(',', ':')), index) + + @abc.abstractmethod + def set_config_value(self, value, index=None): + """Create or update `/config` key""" @abc.abstractmethod def touch_member(self, data, ttl=None): diff --git a/patroni/dcs/consul.py b/patroni/dcs/consul.py index 81f430bc..e941d025 100644 --- a/patroni/dcs/consul.py +++ b/patroni/dcs/consul.py @@ -5,7 +5,7 @@ import time import six from consul import ConsulException, NotFound, base, std -from patroni.dcs import AbstractDCS, Cluster, Failover, Leader, Member +from patroni.dcs import AbstractDCS, ClusterConfig, Cluster, Failover, Leader, Member from patroni.exceptions import DCSError from patroni.utils import sleep from requests.exceptions import RequestException @@ -70,8 +70,8 @@ def catch_consul_errors(func): class Consul(AbstractDCS): - def __init__(self, name, config): - super(Consul, self).__init__(name, config) + def __init__(self, config): + super(Consul, self).__init__(config) self._ttl = None self._session = None self._my_member_data = None @@ -139,6 +139,10 @@ class Consul(AbstractDCS): initialize = nodes.get(self._INITIALIZE) initialize = initialize and initialize['Value'] + # get global dynamic configuration + config = nodes.get(self._CONFIG) + config = config and ClusterConfig.from_node(config['ModifyIndex'], config['Value']) + # get last leader operation last_leader_operation = nodes.get(self._LEADER_OPTIME) last_leader_operation = 0 if last_leader_operation is None else int(last_leader_operation['Value']) @@ -163,9 +167,9 @@ class Consul(AbstractDCS): if failover: failover = Failover.from_node(failover['ModifyIndex'], failover['Value']) - self._cluster = Cluster(initialize, leader, last_leader_operation, members, failover) + self._cluster = Cluster(initialize, config, leader, last_leader_operation, members, failover) except NotFound: - self._cluster = Cluster(False, None, None, [], None) + self._cluster = Cluster(False, None, None, None, [], None) except: logger.exception('get_cluster') raise ConsulError('Consul is not responding properly') @@ -205,6 +209,10 @@ class Consul(AbstractDCS): def set_failover_value(self, value, index=None): return self._client.kv.put(self.failover_path, value, cas=index) + @catch_consul_errors + def set_config_value(self, value, index=None): + return self._client.kv.put(self.config_path, value, cas=index) + @catch_consul_errors def write_leader_optime(self, last_operation): return self._client.kv.put(self.leader_optime_path, last_operation) diff --git a/patroni/dcs/etcd.py b/patroni/dcs/etcd.py index f06cca3d..0c64cbfd 100644 --- a/patroni/dcs/etcd.py +++ b/patroni/dcs/etcd.py @@ -9,7 +9,7 @@ import time from dns.exception import DNSException from dns import resolver -from patroni.dcs import AbstractDCS, Cluster, Failover, Leader, Member +from patroni.dcs import AbstractDCS, ClusterConfig, Cluster, Failover, Leader, Member from patroni.exceptions import DCSError from patroni.utils import Retry, RetryFailedError, sleep from urllib3.exceptions import HTTPError, ReadTimeoutError @@ -191,8 +191,8 @@ def catch_etcd_errors(func): class Etcd(AbstractDCS): - def __init__(self, name, config): - super(Etcd, self).__init__(name, config) + def __init__(self, config): + super(Etcd, self).__init__(config) self.set_ttl(config.get('ttl', 30)) self._retry = Retry(deadline=10, max_delay=1, max_tries=-1, retry_exceptions=(etcd.EtcdConnectionFailed, @@ -231,6 +231,10 @@ class Etcd(AbstractDCS): initialize = nodes.get(self._INITIALIZE) initialize = initialize and initialize.value + # get global dynamic configuration + config = nodes.get(self._CONFIG) + config = config and ClusterConfig.from_node(config.modifiedIndex, config.value) + # get last leader operation last_leader_operation = nodes.get(self._LEADER_OPTIME) last_leader_operation = 0 if last_leader_operation is None else int(last_leader_operation.value) @@ -250,9 +254,9 @@ 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, config, leader, last_leader_operation, members, failover) except etcd.EtcdKeyNotFound: - self._cluster = Cluster(False, None, None, [], None) + self._cluster = Cluster(False, None, None, None, [], None) except: logger.exception('get_cluster') raise EtcdError('Etcd is not responding properly') @@ -278,6 +282,10 @@ class Etcd(AbstractDCS): def set_failover_value(self, value, index=None): return self._client.write(self.failover_path, value, prevIndex=index or 0) + @catch_etcd_errors + def set_config_value(self, value, index=None): + return self._client.write(self.config_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) diff --git a/patroni/dcs/zookeeper.py b/patroni/dcs/zookeeper.py index 22f2f5ad..b374ab51 100644 --- a/patroni/dcs/zookeeper.py +++ b/patroni/dcs/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 +from patroni.dcs import AbstractDCS, ClusterConfig, Cluster, Failover, Leader, Member from patroni.exceptions import DCSError from patroni.utils import sleep from requests.exceptions import RequestException @@ -69,8 +69,8 @@ class ExhibitorEnsembleProvider(object): class ZooKeeper(AbstractDCS): - def __init__(self, name, config): - super(ZooKeeper, self).__init__(name, config) + def __init__(self, config): + super(ZooKeeper, self).__init__(config) hosts = config.get('hosts', []) if isinstance(hosts, list): @@ -83,7 +83,7 @@ class ZooKeeper(AbstractDCS): self.exhibitor = ExhibitorEnsembleProvider(exhibitor['hosts'], exhibitor['port'], poll_interval=interval) hosts = self.exhibitor.zookeeper_hosts - self._client = KazooClient(hosts=hosts, timeout=(config.get('session_timeout') or 30), + self._client = KazooClient(hosts=hosts, timeout=(config.get('session_timeout') or config.get('ttl') or 30), command_retry={'deadline': (config.get('reconnect_timeout') or 10), 'max_delay': 1, 'max_tries': -1}, connection_retry={'max_delay': 1, 'max_tries': -1}) @@ -146,6 +146,10 @@ class ZooKeeper(AbstractDCS): # get initialize flag initialize = (self.get_node(self.initialize_path) or [None])[0] if self._INITIALIZE in nodes else None + # get global dynamic configuration + config = self.get_node(self.config_path, watch=self.cluster_watcher) if self._CONFIG in nodes else None + config = config and ClusterConfig.from_node(config[1].version, config[0]) + # get list of members members = self.load_members() if self._MEMBERS[:-1] in nodes else [] @@ -166,13 +170,12 @@ class ZooKeeper(AbstractDCS): # 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]) + failover = failover and Failover.from_node(failover[1].version, failover[0]) # get last leader operation 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) + self._cluster = Cluster(initialize, config, leader, self._last_leader_operation, members, failover) def _load_cluster(self): if self.exhibitor and self.exhibitor.poll(): @@ -209,6 +212,16 @@ class ZooKeeper(AbstractDCS): logging.exception('set_failover_value') return False + def set_config_value(self, value, index=None): + try: + self._client.retry(self._client.set, self.config_path, value.encode('utf-8'), version=index or -1) + return True + except NoNodeError: + return value == '' or (not index and self._create(self.config_path, value)) + except Exception: + logging.exception('set_config_value') + return False + 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")) diff --git a/patroni/ha.py b/patroni/ha.py index e1772eb5..ad791be5 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -86,10 +86,11 @@ class Ha(object): self._async_executor.schedule('bootstrap {0}'.format(msg)) self._async_executor.run_async(self.clone, args=(clone_member, msg)) return 'trying to bootstrap {0}'.format(msg) - elif not self.cluster.initialize and not self.patroni.nofailover: # no initialize key + # no initialize key and node is allowed to be master and has 'bootstrap' section in a configuration file + elif not (self.cluster.initialize or self.patroni.nofailover) and 'bootstrap' in self.patroni.config: if self.dcs.initialize(create_new=True): # race for initialization try: - self.state_handler.bootstrap() + self.state_handler.bootstrap(self.patroni.config['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 @@ -98,6 +99,7 @@ class Ha(object): self.state_handler.stop('immediate') self.state_handler.move_data_directory() raise + self.dcs.set_config_value(json.dumps(self.patroni.config.dynamic_configuration, separators=(',', ':'))) self.dcs.take_leader() self.load_cluster_from_dcs() return 'initialized a new cluster' @@ -440,9 +442,12 @@ class Ha(object): self.touch_member() # cluster has leader key but not initialize key - if not self.cluster.is_unlocked() and not self.sysid_valid(self.cluster.initialize) and self.has_lock(): + if not (self.cluster.is_unlocked() or 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 not (self.cluster.is_unlocked() or self.cluster.config and self.cluster.config.data) and self.has_lock(): + self.dcs.set_config_value(json.dumps(self.patroni.config.dynamic_configuration, separators=(',', ':'))) + if self._async_executor.busy: return self.handle_long_action_in_progress() diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 1197aa08..35aecae7 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -41,27 +41,44 @@ def parseurl(url): class Postgresql(object): + # List of parameters which must be always passed to postmaster as command line options + # to make it not possible to change them with 'ALTER SYSTEM'. + # Some of these parameters have sane default value assigned and Patroni doesn't allow + # to decrease this value. E.g. 'wal_level' can't be lower then 'hot_standby' and so on. + # These parameters could be changed only globally, i.e. via DCS. + # P.S. 'listen_addresses' and 'port' are added here just for convenience, to mark them + # as a parameters which should always be passed through command line. + CMDLINE_OPTIONS = { + 'listen_addresses': None, + 'port': None, + 'wal_level': 'hot_standby', + 'hot_standby': 'on', + 'max_wal_senders': 5, + 'wal_keep_segments': 8, + 'max_replication_slots': 5, + 'wal_log_hints': 'on' + } + def __init__(self, config): self.config = config self.name = config['name'] - self._restart_pending = False - self._server_parameters = self.get_server_parameters(config) - self._listen_addresses, self._port = (config['listen'] + ':5432').split(':')[:2] - self._connect_address = config.get('connect_address') - self.replication = config['replication'] - self.resolve_connection_addresses() - self.scope = config['scope'] self._data_dir = config['data_dir'] - self.superuser = config.get('superuser') or {} - self.admin = config.get('admin') or {} + self._restart_pending = False + self._server_parameters = self.get_server_parameters(config) - self.initdb_options = config.get('initdb') or [] - self.pgpass = config.get('pgpass') or os.path.join(os.path.expanduser('~'), 'pgpass') - self.pg_rewind = config.get('pg_rewind') or {} - self.callback = config.get('callbacks') or {} - self.use_slots = config.get('use_slots', True) + self._connect_address = config.get('connect_address') + self._superuser = config['authentication'].get('superuser', {}) + self._replication = config['authentication']['replication'] + self.resolve_connection_addresses() + + self._use_pg_rewind = config.get('use_pg_rewind', False) + self._use_slots = config.get('use_slots', True) + self._major_version = self.get_major_version() self._schedule_load_slots = self.use_slots + + self._pgpass = config.get('pgpass') or os.path.join(os.path.expanduser('~'), 'pgpass') + self.callback = config.get('callbacks') or {} self._postgresql_conf = os.path.join(self._data_dir, 'postgresql.conf') self._postgresql_base_conf_name = 'postgresql.base.conf' self._postgresql_base_conf = os.path.join(self._data_dir, self._postgresql_base_conf_name) @@ -90,39 +107,58 @@ class Postgresql(object): self.set_role('master' if self.is_leader() else 'replica') self._write_postgresql_conf() # we are "joining" already running postgres - @staticmethod - def get_server_parameters(config): - return {p: v for p, v in (config.get('parameters') or {}).items() if p not in ('listen_addresses', 'port')} + @property + def use_slots(self): + return self._use_slots and self._major_version >= 9.4 + + def get_major_version(self): + if not self.data_directory_empty(): + try: + with open(os.path.join(self._data_dir, 'PG_VERSION')) as f: + return float(f.read()) + except Exception: + logger.exception('Failed to read PG_VERSION from %s', self._data_dir) + return 0.0 + + def get_server_parameters(self, config): + parameters = config['parameters'].copy() + listen_addresses, port = (config['listen'] + ':5432').split(':')[:2] + parameters.update({'listen_addresses': listen_addresses, 'port': port}) + return parameters def resolve_connection_addresses(self): - self.local_address = self.get_local_address() + self._local_address = self.get_local_address() self.connection_string = 'postgres://{username}:{password}@{connect_address}/postgres'.format( - connect_address=self._connect_address or self.local_address, **self.replication) + connect_address=self._connect_address or self._local_address, **self._replication) def reload_config(self, config): server_parameters = self.get_server_parameters(config) - self._connect_address = config.get('connect_address') - listen_addresses, port = (config['listen'] + ':5432').split(':')[:2] - if self._listen_addresses == listen_addresses and self._port == port: - self.resolve_connection_addresses() - self._listen_addresses = listen_addresses - self._port = port + listen_address_changed = reload_pending = False if self.is_healthy(): changes = server_parameters.copy() changes.update({p: None for p, v in self._server_parameters.items() if p not in server_parameters}) if changes: - for r in self.query("""SELECT name, setting + for r in self.query("""SELECT name, setting, context FROM pg_settings - WHERE context in ('internal', 'postmaster') - AND name IN (""" + ', '.join('%s' for _ in changes.keys()) + ')', + WHERE name IN (""" + ', '.join('%s' for _ in changes.keys()) + ')', *(list(changes.keys()))): if server_parameters[r[0]] is None or str(server_parameters[r[0]]) != str(r[1]): - self._restart_pending = True - break + reload_pending = True + if r[2] in ('internal', 'postmaster'): + self._restart_pending = True + if r[0] in ('listen_addresses', 'port'): + listen_address_changed = True + self.config = config self._server_parameters = server_parameters - self._write_postgresql_conf() - self.reload() + self._connect_address = config.get('connect_address') + + if not listen_address_changed: + self.resolve_connection_addresses() + + if reload_pending: + self._write_postgresql_conf() + self.reload() @property def restart_pending(self): @@ -134,8 +170,7 @@ class Postgresql(object): 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', '')): + if not (self._use_pg_rewind and all(self._superuser.get(n) for n in ('username', 'password'))): return False cmd = ['pg_rewind', '--help'] @@ -157,14 +192,14 @@ class Postgresql(object): return self._sysid def get_local_address(self): - listen_addresses = self._listen_addresses.split(',') + listen_addresses = self._server_parameters['listen_addresses'].split(',') local_address = listen_addresses[0].strip() # take first address from listen_addresses for la in listen_addresses: - if la.strip() in ['*', '0.0.0.0']: # we are listening on * + if la.strip() in ('*', '0.0.0.0', '127.0.0.1', 'localhost'): # we are listening on '*' or localhost local_address = 'localhost' # connection via localhost is preferred break - return local_address + ':' + self._port + return local_address + ':' + self._server_parameters['port'] def get_postgres_role_from_data_directory(self): if self.data_directory_empty(): @@ -176,11 +211,11 @@ class Postgresql(object): @property def _connect_kwargs(self): - r = parseurl('postgres://{0}/postgres'.format(self.local_address)) - if 'username' in self.superuser: - r['user'] = self.superuser['username'] - if 'password' in self.superuser: - r['password'] = self.superuser['password'] + r = parseurl('postgres://{0}/postgres'.format(self._local_address)) + if 'username' in self._superuser: + r['user'] = self._superuser['username'] + if 'password' in self._superuser: + r['password'] = self._superuser['password'] return r def connection(self): @@ -229,9 +264,9 @@ class Postgresql(object): raise Exception('{0} option for initdb is not allowed'.format(name)) return True - def get_initdb_options(self): + def get_initdb_options(self, config): options = [] - for o in self.initdb_options: + for o in config: if isinstance(o, string_types) and self.initdb_allowed_option(o): options.append('--{0}'.format(o)) elif isinstance(o, dict): @@ -243,17 +278,17 @@ class Postgresql(object): raise Exception('Unknown type of initdb option: {0}'.format(o)) return options - def initialize(self): + def _initialize(self, config): self.set_state('initalizing new cluster') - options = self.get_initdb_options() + options = self.get_initdb_options(config.get('initdb') or []) pwfile = None - if self.superuser: - if 'username' in self.superuser: - options.append('--username={0}'.format(self.superuser['username'])) - if 'password' in self.superuser: + if self._superuser: + if 'username' in self._superuser: + options.append('--username={0}'.format(self._superuser['username'])) + if 'password' in self._superuser: (fd, pwfile) = tempfile.mkstemp() - os.write(fd, self.superuser['password'].encode('utf-8')) + os.write(fd, self._superuser['password'].encode('utf-8')) os.close(fd) options.append('--pwfile={0}'.format(pwfile)) @@ -261,7 +296,8 @@ class Postgresql(object): if pwfile: os.remove(pwfile) if ret: - self.write_pg_hba() + self.write_pg_hba(config.get('pg_hba', [])) + self._major_version = self.get_major_version() else: self.set_state('initdb failed') return ret @@ -271,12 +307,12 @@ class Postgresql(object): os.unlink(self._trigger_file) def write_pgpass(self, record): - with open(self.pgpass, 'w') as f: + 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'] = self.pgpass + env['PGPASSFILE'] = self._pgpass return env def replica_method_can_work_without_replication_connection(self, method): @@ -405,16 +441,20 @@ class Postgresql(object): env = {'PATH': os.environ.get('PATH')} # pg_ctl will write a FATAL if the username is incorrect. exporting PGUSER if necessary - if 'username' in self.superuser and self.superuser['username'] != os.environ.get('USER'): - env['PGUSER'] = self.superuser['username'] + if 'username' in self._superuser and self._superuser['username'] != os.environ.get('USER'): + env['PGUSER'] = self._superuser['username'] + self._write_postgresql_conf() - server_arguments = ['-o', "--listen_addresses='{0}' --port={1}".format(self._listen_addresses, self._port)] - ret = subprocess.call(self._pg_ctl + ['start'] + server_arguments, env=env, preexec_fn=os.setsid) == 0 + self.resolve_connection_addresses() + + options = ' '.join("--{0}='{1}'".format(p, self._server_parameters[p]) for p in self.CMDLINE_OPTIONS + if not (self._major_version < 9.4 and p in ('max_replication_slots', 'wal_log_hints'))) + + ret = subprocess.call(self._pg_ctl + ['start', '-o', options], env=env, preexec_fn=os.setsid) == 0 self._restart_pending = False self.set_state('running' if ret else 'start failed') if ret: - self.resolve_connection_addresses() self._schedule_load_slots = self.use_slots self.save_configuration_files() @@ -488,8 +528,9 @@ class Postgresql(object): with open(self._postgresql_conf, 'w') as f: f.write('# Do not edit this file manually!\n# It will be overwritten by Patroni!\n') f.write("include '{0}'\n\n".format(self._postgresql_base_conf_name)) - for setting, value in sorted(self._server_parameters.items()): - f.write("{0} = '{1}'\n".format(setting, value)) + for name, value in sorted(self._server_parameters.items()): + if name not in self.CMDLINE_OPTIONS: + f.write("{0} = '{1}'\n".format(name, value)) def is_healthy(self): if not self.is_running(): @@ -500,9 +541,9 @@ class Postgresql(object): def check_replication_lag(self, last_leader_operation): return (last_leader_operation or 0) - self.xlog_position() <= self.config.get('maximum_lag_on_failover', 0) - def write_pg_hba(self): + def write_pg_hba(self, config): with open(os.path.join(self._data_dir, 'pg_hba.conf'), 'a') as f: - f.write('\n{}\n'.format('\n'.join(self.config.get('pg_hba', [])))) + f.write('\n{}\n'.format('\n'.join(config))) def primary_conninfo(self, leader_url): r = parseurl(leader_url) @@ -536,7 +577,7 @@ class Postgresql(object): def rewind(self, leader): # prepare pg_rewind connection r = parseurl(leader.conn_url) - r.update(self.pg_rewind) + r.update(self._superuser) r['user'] = r.pop('username') env = self.write_pgpass(r) pc = "user={user} host={host} port={port} dbname=postgres sslmode=prefer sslcompression=1".format(**r) @@ -546,12 +587,9 @@ class Postgresql(object): logger.info("running pg_rewind from %s", pc) pg_rewind = ['pg_rewind', '-D', self._data_dir, '--source-server', pc] try: - ret = subprocess.call(pg_rewind, env=env) == 0 + return subprocess.call(pg_rewind, env=env) == 0 except OSError: - ret = False - if ret: - self.write_recovery_conf(leader) - return ret + return False def controldata(self): """ return the contents of pg_controldata, or non-True value if pg_controldata call failed """ @@ -566,6 +604,22 @@ class Postgresql(object): logger.exception("Error when calling pg_controldata") return result + 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') + return result + def single_user_mode(self, command=None, options=None): """ 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] @@ -603,7 +657,6 @@ class Postgresql(object): need_rewind = change_role and self.can_rewind if need_rewind: logger.info("set the rewind flag after demote") - self.write_recovery_conf(leader) if leader and need_rewind: # we have a leader and need to rewind if self.is_running(): self.stop() @@ -613,20 +666,24 @@ class Postgresql(object): # XXX: if recovery.conf is linked, it will be written anew as a normal file. if os.path.islink(self._recovery_conf): os.unlink(self._recovery_conf) - else: + elif os.path.isfile(self._recovery_conf): 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() # Start in a single user mode and stop to produce a clean shutdown - self.single_user_mode(options={'archive_mode': 'on', 'archive_command': 'false'}) + opts = self.read_postmaster_opts() + opts.update({'archive_mode': 'on', 'archive_command': 'false'}) + self.single_user_mode(options=opts) if self.rewind(leader): + self.write_recovery_conf(leader) ret = self.start() else: logger.error("unable to rewind the former master") self.remove_data_directory() ret = True else: # do not rewind until the leader becomes available + self.write_recovery_conf(leader) ret = self.restart() if change_role and ret: self.call_nowait(ACTION_ON_ROLE_CHANGE) @@ -664,26 +721,19 @@ class Postgresql(object): self.call_nowait(ACTION_ON_ROLE_CHANGE) return ret - def create_or_update_role(self, name, password, options): + def create_or_update_user(self, name, password, options): self.query("""DO $$ BEGIN SET local synchronous_commit = 'local'; PERFORM * FROM pg_authid WHERE rolname = %s; IF FOUND THEN - ALTER ROLE "{0}" WITH LOGIN {1} PASSWORD %s; + ALTER USER "{0}" WITH {1} PASSWORD %s; ELSE - CREATE ROLE "{0}" WITH LOGIN {1} PASSWORD %s; + CREATE USER "{0}" WITH {1} PASSWORD %s; END IF; END; $$""".format(name, options), name, password, password) - def create_replication_user(self): - self.create_or_update_role(self.replication['username'], self.replication['password'], 'REPLICATION') - - def create_connection_user(self): - if self.admin: - self.create_or_update_role(self.admin['username'], self.admin['password'], 'CREATEDB CREATEROLE') - def xlog_position(self): return self.query("""SELECT pg_xlog_location_diff(CASE WHEN pg_is_in_recovery() THEN pg_last_xlog_replay_location() @@ -741,15 +791,18 @@ $$""".format(name, options), name, password, password) ret = self.create_replica(clone_member) == 0 if ret: + self._major_version = self.get_major_version() self.delete_trigger_file() self.restore_configuration_files() return ret - def bootstrap(self): + def bootstrap(self, config): """ Initialize a new node from scratch and start it. """ - if self.initialize() and self.start(): - self.create_replication_user() - self.create_connection_user() + if self._initialize(config) and self.start(): + for name, value in config['users'].items(): + if name not in (self._superuser.get('username'), self._replication['username']): + self.create_or_update_user(name, value['password'], ' '.join(value.get('options', [])).upper()) + self.create_or_update_user(self._replication['username'], self._replication['password'], 'REPLICATION') else: raise PostgresException("Could not bootstrap master PostgreSQL") diff --git a/postgres0.yml b/postgres0.yml index 22ab5a73..f6c6ca07 100644 --- a/postgres0.yml +++ b/postgres0.yml @@ -1,104 +1,70 @@ -ttl: &ttl 30 -loop_wait: &loop_wait 10 -scope: &scope batman +scope: batman +#namespace: /service/ +name: postgresql0 + restapi: listen: 127.0.0.1:8008 connect_address: 127.0.0.1:8008 -# 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 -#consul: -# scope: *scope -# ttl: *ttl -# host: 127.0.0.1:8500 -#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 + +bootstrap: + # this section will be written into Etcd:///config after initializing new cluster + # and all other cluster members will use it as a `global configuration` + dcs: + ttl: 30 + loop_wait: 10 + retry_timeout: 5 + maximum_lag_on_failover: 1048576 + postgresql: + use_pg_rewind: true +# use_slots: true + parameters: +# wal_level: hot_standby +# hot_standby: "on" +# wal_keep_segments: 8 +# max_wal_senders: 5 +# max_replication_slots: 5 +# wal_log_hints: "on" + archive_mode: "on" + archive_timeout: 1800s + archive_command: mkdir -p ../wal_archive && test ! -f ../wal_archive/%f && cp %p ../wal_archive/%f + recovery_conf: + restore_command: cp ../wal_archive/%f %p + + # some desired options for 'initdb' + initdb: # Note: It needs to be a list (some options need values, others are switches) + - encoding: UTF8 + - data-checksums + + pg_hba: # Add following lines to pg_hba.conf after running 'initdb' + - host replication replicator 127.0.0.1/32 md5 + - host all all 0.0.0.0/0 md5 +# - hostssl all all 0.0.0.0/0 md5 + + # Some additional users users which needs to be created after initializing new cluster + users: + admin: + password: admin + options: + - createrole + - createdb postgresql: - name: postgresql0 - scope: *scope listen: 127.0.0.1:5432 connect_address: 127.0.0.1:5432 data_dir: data/postgresql0 - maximum_lag_on_failover: 1048576 # 1 megabyte in bytes - use_slots: True pgpass: /tmp/pgpass0 - 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 replication replicator 127.0.0.1/32 md5 - - host all all 0.0.0.0/0 md5 - # - hostssl all all 0.0.0.0/0 md5 - replication: - username: replicator - password: rep-pass - superuser: - username: postgres - password: zalando - admin: - username: admin - password: admin - create_replica_method: - - basebackup -# - wal_e -# commented-out example for wal-e provisioning - #wal_e: - #command: /patroni/scripts/wale_restore.py - #env_dir: /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 + authentication: + replication: + username: replicator + password: rep-pass + superuser: + username: postgres + password: zalando 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" unix_socket_directories: '.' tags: - nofailover: False - noloadbalance: False - clonefrom: False + nofailover: false + noloadbalance: false + clonefrom: false diff --git a/tests/test_api.py b/tests/test_api.py index 34fbc353..23541cec 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1,4 +1,5 @@ import psycopg2 +import socket import unittest from mock import Mock, patch @@ -7,7 +8,6 @@ from patroni.dcs import Member from six import BytesIO as IO from six.moves import BaseHTTPServer from six.moves.BaseHTTPServer import BaseHTTPRequestHandler -import socket from test_postgresql import psycopg2_connect, MockCursor @@ -71,6 +71,7 @@ class MockRestApiServer(RestApiServer): def __init__(self, Handler, request): self.socket = 0 + self.serve_forever = Mock() BaseHTTPServer.HTTPServer.__init__ = Mock() MockRestApiServer._BaseServer__is_shut_down = Mock() MockRestApiServer._BaseServer__shutdown_request = True diff --git a/tests/test_config.py b/tests/test_config.py new file mode 100644 index 00000000..9c259796 --- /dev/null +++ b/tests/test_config.py @@ -0,0 +1,30 @@ +import unittest + +from mock import MagicMock, Mock, patch +from patroni.config import Config +from six.moves import builtins + + +class TestConfig(unittest.TestCase): + + @patch('os.path.isfile', Mock(return_value=True)) + @patch('json.load', Mock(side_effect=Exception)) + @patch.object(builtins, 'open', MagicMock()) + def setUp(self): + self.config = Config(config_env='postgresql: {data_dir: foo}') + + def test_reload_local_configuration(self): + Config(config_file='postgres0.yml').reload_local_configuration() + + @patch('tempfile.mkstemp', Mock(return_value=[3000, 'blabla'])) + @patch('os.path.exists', Mock(return_value=True)) + @patch('os.remove', Mock(side_effect=IOError)) + @patch('os.close', Mock(side_effect=IOError)) + @patch('os.rename', Mock(return_value=None)) + @patch('json.dump', Mock()) + def test_save_cache(self): + self.config.set_dynamic_configuration({'ttl': 30, 'postgresql': {'foo': 'bar'}}) + with patch('os.fdopen', Mock(side_effect=IOError)): + self.config.save_cache() + with patch('os.fdopen', MagicMock()): + self.config.save_cache() diff --git a/tests/test_consul.py b/tests/test_consul.py index 22665174..b3c5ff16 100644 --- a/tests/test_consul.py +++ b/tests/test_consul.py @@ -51,7 +51,7 @@ class TestConsul(unittest.TestCase): @patch.object(consul.Consul.KV, 'get', kv_get) @patch.object(consul.Consul.KV, 'delete', Mock()) def setUp(self): - self.c = Consul('postgresql1', {'ttl': 30, 'scope': 'test', 'host': 'localhost:1'}) + self.c = Consul({'ttl': 30, 'scope': 'test', 'name': 'postgresql1', 'host': 'localhost:1'}) self.c._base_path = '/service/good' self.c._load_cluster() @@ -96,6 +96,10 @@ class TestConsul(unittest.TestCase): def test_set_failover_value(self): self.c.set_failover_value('') + @patch.object(consul.Consul.KV, 'put', Mock(return_value=True)) + def test_set_config_value(self): + self.c.set_config_value('') + @patch.object(consul.Consul.KV, 'put', Mock(side_effect=ConsulException)) def test_write_leader_optime(self): self.c.write_leader_optime('') diff --git a/tests/test_ctl.py b/tests/test_ctl.py index d7394db3..d8dbecb7 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -51,7 +51,7 @@ class TestCtl(unittest.TestCase): self.runner = CliRunner() with patch.object(etcd.Client, 'machines') as mock_machines: mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) - self.e = get_dcs({'etcd': {'ttl': 30, 'host': 'ok:2379', 'scope': 'test'}}, 'foo') + self.e = get_dcs({'etcd': {'ttl': 30, 'host': 'ok:2379'}}, 'foo') @patch('psycopg2.connect', psycopg2_connect) def test_get_cursor(self): diff --git a/tests/test_etcd.py b/tests/test_etcd.py index 508cd480..5631f817 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -78,6 +78,8 @@ def etcd_read(self, key, **kwargs): raise etcd.EtcdKeyNotFound response = {"action": "get", "node": {"key": "/service/batman5", "dir": True, "nodes": [ + {"key": "/service/batman5/config", "value": '{"foo": "bar"}', + "modifiedIndex": 1582, "createdIndex": 1582}, {"key": "/service/batman5/failover", "value": "", "modifiedIndex": 1582, "createdIndex": 1582}, {"key": "/service/batman5/initialize", "value": "postgresql0", @@ -191,7 +193,8 @@ class TestEtcd(unittest.TestCase): def setUp(self): with patch.object(Client, 'machines') as mock_machines: mock_machines.__get__ = Mock(return_value=['http://localhost:2379', 'http://localhost:4001']) - self.etcd = Etcd('foo', {'namespace': '/patroni/', 'ttl': 30, 'host': 'localhost:2379', 'scope': 'test'}) + self.etcd = Etcd({'namespace': '/patroni/', 'ttl': 30, + 'host': 'localhost:2379', 'scope': 'test', 'name': 'foo'}) def test_base_path(self): self.assertEquals(self.etcd._base_path, '/patroni/test') diff --git a/tests/test_ha.py b/tests/test_ha.py index 5ceb3392..f8f1f90c 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -4,6 +4,7 @@ import datetime import pytz from mock import Mock, MagicMock, patch +from patroni.config import Config from patroni.dcs import Cluster, Failover, Leader, Member, get_dcs from patroni.exceptions import DCSError, PostgresException from patroni.ha import Ha @@ -20,7 +21,7 @@ def false(*args, **kwargs): def get_cluster(initialize, leader, members, failover): - return Cluster(initialize, leader, 10, members, failover) + return Cluster(initialize, None, leader, 10, members, failover) def get_cluster_not_initialized_without_leader(): @@ -48,6 +49,20 @@ def get_cluster_initialized_with_only_leader(failover=None): class MockPatroni(object): def __init__(self, p, d): + self.config = Config(config_env=""" +bootstrap: + users: + replicator: + password: rep-pass + options: + - replication +postgresql: + name: foo + data_dir: data/postgresql0 + pg_rewind: + username: postgres + password: postgres +""") self.postgresql = p self.dcs = d self.api = Mock() @@ -87,13 +102,16 @@ class TestHa(unittest.TestCase): with patch.object(etcd.Client, 'machines') as mock_machines: mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) self.p = Postgresql({'name': 'postgresql0', 'scope': 'dummy', 'listen': '127.0.0.1:5432', - 'data_dir': 'data/postgresql0', 'superuser': {}, 'admin': {}, - 'replication': {'username': '', 'password': '', 'network': ''}}) + 'data_dir': 'data/postgresql0', + 'authentication': {'superuser': {'username': 'foo', 'password': 'bar'}, + 'replication': {'username': '', 'password': ''}}, + 'parameters': {'wal_level': 'hot_standby', 'max_replication_slots': 5, 'foo': 'bar', + 'hot_standby': 'on', 'max_wal_senders': 5, 'wal_keep_segments': 8}}) self.p.set_state('running') self.p.set_role('replica') self.p.check_replication_lag = true self.p.can_create_replica_without_replication_connection = MagicMock(return_value=False) - self.e = get_dcs('foo', {'etcd': {'ttl': 30, 'host': 'ok:2379', 'scope': 'test'}}) + self.e = get_dcs({'etcd': {'ttl': 30, 'host': 'ok:2379', 'scope': 'test', 'name': 'foo'}}) self.ha = Ha(MockPatroni(self.p, self.e)) self.ha._async_executor.run_async = run_async self.ha.old_cluster = self.e.get_cluster() diff --git a/tests/test_patroni.py b/tests/test_patroni.py index c4492a9f..1cff2611 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -7,6 +7,7 @@ import unittest from mock import Mock, patch from patroni.api import RestApiServer from patroni.async_executor import AsyncExecutor +from patroni.exceptions import DCSError from patroni import Patroni, main as _main from six.moves import BaseHTTPServer from test_etcd import SleepException, etcd_read, etcd_write @@ -25,6 +26,7 @@ from test_postgresql import Postgresql, psycopg2_connect @patch.object(etcd.Client, 'read', etcd_read) class TestPatroni(unittest.TestCase): + @patch.object(etcd.Client, 'read', etcd_read) def setUp(self): RestApiServer._BaseServer__is_shut_down = Mock() RestApiServer._BaseServer__shutdown_request = True @@ -33,6 +35,12 @@ class TestPatroni(unittest.TestCase): mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) self.p = Patroni('postgres0.yml') + @patch('patroni.dcs.AbstractDCS.get_cluster', Mock(side_effect=[None, DCSError('foo'), None])) + def test_load_dynamic_configuration(self): + self.p.config._dynamic_configuration = {} + self.p.load_dynamic_configuration() + self.p.load_dynamic_configuration() + @patch('time.sleep', Mock(side_effect=SleepException)) @patch.object(etcd.Client, 'delete', Mock()) @patch.object(etcd.Client, 'machines') @@ -55,11 +63,15 @@ class TestPatroni(unittest.TestCase): self.assertRaises(SleepException, _main) del os.environ[Patroni.PATRONI_CONFIG_VARIABLE] + @patch('patroni.config.Config.save_cache', Mock()) def test_run(self): self.p.sighup_handler() self.p.ha.dcs.watch = Mock(side_effect=SleepException) self.p.api.start = Mock() + self.p.config._dynamic_configuration = {} self.assertRaises(SleepException, self.p.run) + with patch('patroni.postgresql.Postgresql.data_directory_empty', Mock(return_value=False)): + self.assertRaises(SleepException, self.p.run) def test_schedule_next_run(self): self.p.ha.dcs.watch = Mock(return_value=True) @@ -84,5 +96,5 @@ class TestPatroni(unittest.TestCase): def test_reload_config(self): self.p.reload_config() - with patch('yaml.load', Mock(side_effect=Exception)): - self.p.reload_config() + self.p.get_tags = Mock(side_effect=Exception) + self.p.reload_config() diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index c6efa841..84f33e2e 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -5,7 +5,7 @@ import shutil import subprocess import unittest -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 @@ -35,7 +35,7 @@ class MockCursor(object): elif sql.startswith('SELECT to_char(pg_postmaster_start_time'): self.results = [('', True, '', '', '', '', False)] elif sql.startswith('SELECT name, setting'): - self.results = [('archive_mode', 'off')] + self.results = [('port', '5433', 'postmaster')] else: self.results = [(None, None, None, None, None, None, None, None, None, None)] @@ -131,6 +131,13 @@ 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 && 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() @@ -142,25 +149,24 @@ def fake_listdir(path): @patch('subprocess.call', Mock(return_value=0)) @patch('psycopg2.connect', psycopg2_connect) class TestPostgresql(unittest.TestCase): + _PARAMETERS = {'wal_level': 'hot_standby', 'max_replication_slots': 5, 'foo': 'bar', + 'hot_standby': 'on', 'max_wal_senders': 5, 'wal_keep_segments': 8, 'wal_log_hints': 'on'} @patch('subprocess.call', Mock(return_value=0)) @patch('psycopg2.connect', psycopg2_connect) @patch('os.rename', Mock()) + @patch.object(Postgresql, 'get_major_version', Mock(return_value=9.4)) def setUp(self): self.data_dir = 'data/test0' if not os.path.exists(self.data_dir): os.makedirs(self.data_dir) self.p = Postgresql({'name': 'test0', 'scope': 'batman', 'data_dir': self.data_dir, 'listen': '127.0.0.1, *:5432', 'connect_address': '127.0.0.2:5432', - 'pg_hba': ['host replication replicator 127.0.0.1/32 md5', - 'hostssl all all 0.0.0.0/0 md5', - 'host all all 0.0.0.0/0 md5'], - 'superuser': {'username': 'test', 'password': 'test'}, - 'admin': {'username': 'admin', 'password': 'admin'}, - 'pg_rewind': {'username': 'admin', 'password': 'admin'}, - 'replication': {'username': 'replicator', - 'password': 'rep-pass'}, - 'parameters': {'foo': 'bar'}, 'recovery_conf': {'foo': 'bar'}, + 'authentication': {'superuser': {'username': 'test', 'password': 'test'}, + 'replication': {'username': 'replicator', 'password': 'rep-pass'}}, + 'use_pg_rewind': True, + 'parameters': self._PARAMETERS, + 'recovery_conf': {'foo': 'bar'}, 'callbacks': {'on_start': 'true', 'on_stop': 'true', 'on_restart': 'true', 'on_role_change': 'true', 'on_reload': 'true' @@ -176,22 +182,11 @@ class TestPostgresql(unittest.TestCase): shutil.rmtree('data') def test_get_initdb_options(self): - self.p.initdb_options = [{'encoding': 'UTF8'}, 'data-checksums'] - self.assertEquals(self.p.get_initdb_options(), ['--encoding=UTF8', '--data-checksums']) - self.p.initdb_options = [{'pgdata': 'bar'}] - self.assertRaises(Exception, self.p.get_initdb_options) - self.p.initdb_options = [{'foo': 'bar', 1: 2}] - self.assertRaises(Exception, self.p.get_initdb_options) - self.p.initdb_options = [1] - self.assertRaises(Exception, self.p.get_initdb_options) - - def test_initialize(self): - self.assertTrue(self.p.initialize()) - - with open(os.path.join(self.data_dir, 'pg_hba.conf')) as f: - lines = f.readlines() - assert 'host replication replicator 127.0.0.1/32 md5\n' in lines - assert 'host all all 0.0.0.0/0 md5\n' in lines + self.assertEquals(self.p.get_initdb_options([{'encoding': 'UTF8'}, 'data-checksums']), + ['--encoding=UTF8', '--data-checksums']) + self.assertRaises(Exception, self.p.get_initdb_options, [{'pgdata': 'bar'}]) + self.assertRaises(Exception, self.p.get_initdb_options, [{'foo': 'bar', 1: 2}]) + self.assertRaises(Exception, self.p.get_initdb_options, [1]) @patch('os.path.exists', Mock(return_value=True)) @patch('os.unlink', Mock()) @@ -258,10 +253,6 @@ class TestPostgresql(unittest.TestCase): @patch('subprocess.check_output', Mock(return_value=0, side_effect=pg_controldata_string)) 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): @@ -291,7 +282,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], None) + cluster = Cluster(True, None, 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 @@ -355,8 +346,16 @@ class TestPostgresql(unittest.TestCase): def test_bootstrap(self): with patch('subprocess.call', Mock(return_value=1)): - self.assertRaises(PostgresException, self.p.bootstrap) - self.p.bootstrap() + self.assertRaises(PostgresException, self.p.bootstrap, {}) + + self.p.bootstrap({'users': {'replicator': {'password': 'rep-pass', 'options': ['replication']}}, + 'pg_hba': ['host replication replicator 127.0.0.1/32 md5', + 'hostssl all all 0.0.0.0/0 md5', + 'host all all 0.0.0.0/0 md5']}) + with open(os.path.join(self.data_dir, 'pg_hba.conf')) as f: + lines = f.readlines() + assert 'host replication replicator 127.0.0.1/32 md5\n' in lines + assert 'host all all 0.0.0.0/0 md5\n' in lines @patch('patroni.postgresql.Postgresql.create_replica', Mock(return_value=0)) def test_clone(self): @@ -387,6 +386,18 @@ class TestPostgresql(unittest.TestCase): with patch('subprocess.check_output', Mock(side_effect=subprocess.CalledProcessError(1, ''))): self.assertEquals(self.p.controldata(), {}) + def test_read_postmaster_opts(self): + 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') + self.assertEquals(int(data['max_replication_slots']), 5) + self.assertEqual(data.get('D'), None) + + m.side_effect = IOError + data = self.p.read_postmaster_opts() + self.assertEqual(data, dict()) + @patch('subprocess.Popen') @patch.object(builtins, 'open', MagicMock(return_value=42)) def test_single_user_mode(self, subprocess_popen_mock): @@ -462,3 +473,11 @@ class TestPostgresql(unittest.TestCase): self.assertTrue(self.p.replica_method_can_work_without_replication_connection('foo')) self.p.config['foo'] = {'command': 'bar'} self.assertFalse(self.p.replica_method_can_work_without_replication_connection('foo')) + + def test_reload_config(self): + self.p.reload_config({'listen': '*', 'parameters': self._PARAMETERS}) + self.p.reload_config({'listen': '*:5433', 'parameters': self._PARAMETERS}) + + @patch.object(builtins, 'open', mock_open(read_data='9.4')) + def test_get_major_version(self): + self.assertEquals(self.p.get_major_version(), 9.4) diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py index 3b5c9a6a..b2e6e57e 100644 --- a/tests/test_zookeeper.py +++ b/tests/test_zookeeper.py @@ -70,7 +70,7 @@ class MockKazooClient(Mock): raise Exception if path == '/service/test/members/bar' and value == b'retry': return - if path == '/service/test/failover': + if path in ('/service/test/failover', '/service/test/config'): if value == b'Exception': raise Exception elif value == b'ok': @@ -103,7 +103,8 @@ class TestZooKeeper(unittest.TestCase): @patch('requests.get', requests_get) @patch('patroni.dcs.zookeeper.KazooClient', MockKazooClient) def setUp(self): - self.zk = ZooKeeper('foo', {'exhibitor': {'hosts': ['localhost', 'exhibitor'], 'port': 8181}, 'scope': 'test'}) + self.zk = ZooKeeper({'exhibitor': {'hosts': ['localhost', 'exhibitor'], 'port': 8181}, + 'scope': 'test', 'name': 'foo'}) def test_session_listener(self): self.zk.session_listener(KazooState.SUSPENDED) @@ -136,6 +137,11 @@ class TestZooKeeper(unittest.TestCase): self.zk.set_failover_value('ok') self.zk.set_failover_value('Exception') + def test_set_config_value(self): + self.zk.set_config_value('') + self.zk.set_config_value('ok') + self.zk.set_config_value('Exception') + def test_initialize(self): self.assertFalse(self.zk.initialize()) From ceace0364689f8ac3ca7934eabff466378b92675 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 25 May 2016 14:49:33 +0200 Subject: [PATCH 04/49] Address codacy and travis issues --- features/environment.py | 4 ++-- patroni/postgresql.py | 3 ++- tests/test_config.py | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/features/environment.py b/features/environment.py index bd03df46..16c6a3fe 100644 --- a/features/environment.py +++ b/features/environment.py @@ -110,7 +110,7 @@ class PatroniController(AbstractController): patroni_config_path = os.path.join(self._output_dir, patroni_config_name) with open(patroni_config_name) as f: - config = yaml.load(f) + config = yaml.safe_load(f) host = config['postgresql']['listen'].split(':')[0] @@ -143,7 +143,7 @@ class PatroniController(AbstractController): config['zookeeper'] = dcs_config with open(patroni_config_path, 'w') as f: - yaml.dump(config, f, default_flow_style=False) + yaml.safe_dump(config, f, default_flow_style=False) return patroni_config_path diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 35aecae7..aeb229a1 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -120,7 +120,8 @@ class Postgresql(object): logger.exception('Failed to read PG_VERSION from %s', self._data_dir) return 0.0 - def get_server_parameters(self, config): + @staticmethod + def get_server_parameters(config): parameters = config['parameters'].copy() listen_addresses, port = (config['listen'] + ':5432').split(':')[:2] parameters.update({'listen_addresses': listen_addresses, 'port': port}) diff --git a/tests/test_config.py b/tests/test_config.py index 9c259796..ca2f2511 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -14,7 +14,7 @@ class TestConfig(unittest.TestCase): self.config = Config(config_env='postgresql: {data_dir: foo}') def test_reload_local_configuration(self): - Config(config_file='postgres0.yml').reload_local_configuration() + self.assertIsNone(Config(config_file='postgres0.yml').reload_local_configuration()) @patch('tempfile.mkstemp', Mock(return_value=[3000, 'blabla'])) @patch('os.path.exists', Mock(return_value=True)) From 89adc0717a8653bbef72aba36faf0e9fb345f01f Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 25 May 2016 15:02:42 +0200 Subject: [PATCH 05/49] Set loglevel back to INFO --- patroni/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/__init__.py b/patroni/__init__.py index 2addc264..34c7d84e 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -112,7 +112,7 @@ class Patroni(object): 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 45cbc8ca70ae460572a421ebc4d9db9c09591a39 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 26 May 2016 10:16:24 +0200 Subject: [PATCH 06/49] Implement acceptance test for dynamic configuration functionality and fix some bugs revealed by acceptance tests --- features/basic_replication.feature | 13 ++++++++---- features/environment.py | 33 +++++++++++++++++++++++------ features/steps/basic_replication.py | 30 ++++++++++++++++++++++++++ patroni/dcs/consul.py | 7 ++++++ patroni/dcs/etcd.py | 16 ++++++++------ patroni/dcs/zookeeper.py | 2 +- tests/test_consul.py | 4 ++++ tests/test_etcd.py | 3 +++ 8 files changed, 90 insertions(+), 18 deletions(-) diff --git a/features/basic_replication.feature b/features/basic_replication.feature index be192715..ba347f45 100644 --- a/features/basic_replication.feature +++ b/features/basic_replication.feature @@ -1,5 +1,5 @@ Feature: basic replication - We should check that the basic bootstrapping, replication and failover works. + We should check that the basic bootstrapping, replication, failover and dyncamic configuration works. Scenario: check replication of a single table Given I start postgres0 @@ -8,10 +8,15 @@ Feature: basic replication When I add the table foo to postgres0 Then table foo is present on postgres1 after 20 seconds + Scenario: check dynamic configuration change via DCS + When I patch global configuration with {"ttl": 20, "loop_wait": 5, "postgresql": {"parameters": {"max_connections": 101}}} + Then Response on GET http://127.0.0.1:8008/patroni contains restart_pending after 11 seconds + And Response on GET http://127.0.0.1:8009/patroni contains restart_pending after 11 seconds + Scenario: check the basic failover - When I kill postgres0 - Then postgres1 role is the primary after 32 seconds + And I kill postgres0 + Then postgres1 role is the primary after 22 seconds When I start postgres0 Then postgres0 role is the secondary after 20 seconds When I add the table bar to postgres1 - Then table bar is present on postgres0 after 20 seconds + Then table bar is present on postgres0 after 10 seconds diff --git a/features/environment.py b/features/environment.py index 16c6a3fe..01e94ce0 100644 --- a/features/environment.py +++ b/features/environment.py @@ -182,7 +182,7 @@ class PatroniController(AbstractController): class AbstractDcsController(AbstractController): - _CLUSTER_NODE = 'service/batman' + _CLUSTER_NODE = '/service/batman' def _is_accessible(self): return self._is_running() @@ -193,10 +193,17 @@ class AbstractDcsController(AbstractController): if self._work_directory: shutil.rmtree(self._work_directory) + def path(self, key=None): + return self._CLUSTER_NODE + (key and '/' + key or '') + @abc.abstractmethod def query(self, key): """ query for a value of a given key """ + @abc.abstractmethod + def set(self, key, value): + """ set a value to a given key """ + @abc.abstractmethod def cleanup_service_tree(self): """ clean all contents stored in the tree used for the tests """ @@ -218,12 +225,18 @@ class ConsulController(AbstractDcsController): except Exception: return False + def path(self, key=None): + return super(ConsulController, self).path(key)[1:] + def query(self, key): - _, value = self._client.kv.get('{0}/{1}'.format(self._CLUSTER_NODE, key)) + _, value = self._client.kv.get(self.path(key)) return value and value['Value'].decode('utf-8') + def set(self, key, value): + self._client.kv.put(self.path(key), value) + def cleanup_service_tree(self): - self._client.kv.delete(self._CLUSTER_NODE, recurse=True) + self._client.kv.delete(self.path(), recurse=True) class EtcdController(AbstractDcsController): @@ -240,13 +253,16 @@ class EtcdController(AbstractDcsController): def query(self, key): try: - return self._client.get('/{0}/{1}'.format(self._CLUSTER_NODE, key)).value + return self._client.get(self.path(key)).value except etcd.EtcdKeyNotFound: return None + def set(self, key, value): + self._client.set(self.path(key), value) + def cleanup_service_tree(self): try: - self._client.delete('/' + self._CLUSTER_NODE, recursive=True) + self._client.delete(self.path(), recursive=True) except (etcd.EtcdKeyNotFound, etcd.EtcdConnectionFailed): return except Exception as e: @@ -273,13 +289,16 @@ class ZooKeeperController(AbstractDcsController): def query(self, key): try: - return self._client.get('/{0}/{1}'.format(self._CLUSTER_NODE, key))[0].decode('utf-8') + return self._client.get(self.path(key))[0].decode('utf-8') except kazoo.exceptions.NoNodeError: return None + def set(self, key, value): + self._client.set(self.path(key), value.encode('utf-8')) + def cleanup_service_tree(self): try: - self._client.delete('/' + self._CLUSTER_NODE, recursive=True) + self._client.delete(self.path(), recursive=True) except (kazoo.exceptions.NoNodeError): return except Exception as e: diff --git a/features/steps/basic_replication.py b/features/steps/basic_replication.py index b59a7639..4f441dac 100644 --- a/features/steps/basic_replication.py +++ b/features/steps/basic_replication.py @@ -1,4 +1,6 @@ +import json import psycopg2 as pg +import requests from behave import step, then from time import sleep, time @@ -52,3 +54,31 @@ def replication_works(context, master, replica, time_limit): When I add the table test_{0} to {1} Then table test_{0} is present on {2} after {3} seconds """.format(int(time()), master, replica, time_limit)) + + +def patch_config_with_data(config, data): + for name, value in data.items(): + if isinstance(value, dict): + patch_config_with_data(config[name], value) + else: + config[name] = value + + +@step('I patch global configuration with {data}') +def patch_config(context, data): + data = json.loads(data) + config = json.loads(context.dcs_ctl.query('config')) + patch_config_with_data(config, data) + context.dcs_ctl.set('config', json.dumps(config)) + + +@then('Response on GET {url} contains {value} after {timeout:d} seconds') +def check_http_response(context, url, value, timeout): + for _ in range(int(timeout)): + r = requests.get(url) + if value in r.content.decode('utf-8'): + break + sleep(1) + else: + assert False,\ + "Value {0} is not present in response after {1} seconds".format(value, timeout) diff --git a/patroni/dcs/consul.py b/patroni/dcs/consul.py index e941d025..499c8ae1 100644 --- a/patroni/dcs/consul.py +++ b/patroni/dcs/consul.py @@ -95,7 +95,14 @@ class Consul(AbstractDCS): def set_ttl(self, ttl): ttl = int(ttl/2) # My experiments have shown that session expires after 2*ttl time if self._ttl != ttl: + if self._session: + try: + self._client.session.destroy(self._session) + except Exception: + logger.exception("Can not destroy session %s", self._session) self._session = None + self.reset_cluster() + self.event.set() self._ttl = ttl def refresh_session(self): diff --git a/patroni/dcs/etcd.py b/patroni/dcs/etcd.py index 0c64cbfd..beb494b5 100644 --- a/patroni/dcs/etcd.py +++ b/patroni/dcs/etcd.py @@ -193,7 +193,7 @@ class Etcd(AbstractDCS): def __init__(self, config): super(Etcd, self).__init__(config) - self.set_ttl(config.get('ttl', 30)) + self._ttl = int(config.get('ttl') or 30) self._retry = Retry(deadline=10, max_delay=1, max_tries=-1, retry_exceptions=(etcd.EtcdConnectionFailed, etcd.EtcdLeaderElectionInProgress, @@ -216,7 +216,11 @@ class Etcd(AbstractDCS): return client def set_ttl(self, ttl): - self.ttl = int(ttl) + ttl = int(ttl) + if self._ttl != ttl: + self.reset_cluster() + self.event.set() + self._ttl = ttl @staticmethod def member(node): @@ -263,15 +267,15 @@ class Etcd(AbstractDCS): @catch_etcd_errors def touch_member(self, data, ttl=None): - return self.retry(self._client.set, self.member_path, data, ttl or self.ttl) + return self.retry(self._client.set, self.member_path, data, ttl or self._ttl) @catch_etcd_errors def take_leader(self): - return self.retry(self._client.set, self.leader_path, self._name, self.ttl) + return self.retry(self._client.set, self.leader_path, self._name, self._ttl) def attempt_to_acquire_leader(self): try: - return bool(self.retry(self._client.write, self.leader_path, self._name, ttl=self.ttl, prevExist=False)) + return bool(self.retry(self._client.write, self.leader_path, self._name, ttl=self._ttl, prevExist=False)) except etcd.EtcdAlreadyExist: logger.info('Could not take out TTL lock') except (RetryFailedError, etcd.EtcdException): @@ -292,7 +296,7 @@ class Etcd(AbstractDCS): @catch_etcd_errors def update_leader(self): - return self.retry(self._client.test_and_set, self.leader_path, self._name, self._name, self.ttl) + 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=""): diff --git a/patroni/dcs/zookeeper.py b/patroni/dcs/zookeeper.py index b374ab51..d4d680ee 100644 --- a/patroni/dcs/zookeeper.py +++ b/patroni/dcs/zookeeper.py @@ -109,7 +109,7 @@ class ZooKeeper(AbstractDCS): # but there is no other way to change session_timeout without losing session if self._client._session_timeout != ttl: self._client._session_timeout = ttl - self._client._connection._socket.close() + self._client.restart() def get_node(self, key, watch=None): try: diff --git a/tests/test_consul.py b/tests/test_consul.py index b3c5ff16..2c5bb36d 100644 --- a/tests/test_consul.py +++ b/tests/test_consul.py @@ -129,3 +129,7 @@ class TestConsul(unittest.TestCase): self.c.watch(1) with patch.object(consul.Consul.KV, 'get', Mock(side_effect=ConsulException)): self.c.watch(1) + + @patch.object(consul.Consul.Session, 'destroy', Mock(side_effect=ConsulException)) + def test_set_ttl(self): + self.c.set_ttl(20) diff --git a/tests/test_etcd.py b/tests/test_etcd.py index 5631f817..4d248e11 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -257,3 +257,6 @@ class TestEtcd(unittest.TestCase): def test_other_exceptions(self): self.etcd.retry = Mock(side_effect=AttributeError('foo')) self.assertRaises(EtcdError, self.etcd.cancel_initialization) + + def test_set_ttl(self): + self.etcd.set_ttl(20) From 6700cd0aa67fa703413b61f54d33ba5643dc16dd Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 26 May 2016 17:09:40 +0200 Subject: [PATCH 07/49] Implement reload of config.yml with REST API call and acceptance tests for that --- features/basic_replication.feature | 8 ++++++ features/environment.py | 12 +++++++- features/steps/basic_replication.py | 5 ++++ patroni/__init__.py | 4 +-- patroni/api.py | 13 +++++++++ patroni/config.py | 43 ++++++++++++++++++++--------- tests/test_api.py | 9 ++++++ tests/test_config.py | 11 +++++++- tests/test_patroni.py | 1 + 9 files changed, 89 insertions(+), 17 deletions(-) diff --git a/features/basic_replication.feature b/features/basic_replication.feature index ba347f45..438b1dd7 100644 --- a/features/basic_replication.feature +++ b/features/basic_replication.feature @@ -8,10 +8,18 @@ Feature: basic replication When I add the table foo to postgres0 Then table foo is present on postgres1 after 20 seconds + Scenario: check local configuration reload + When I issue an empty POST request to http://127.0.0.1:8008/reload + Then I receive a response code 304 + When I add tag new_tag new_value to postgres0 config + And I issue an empty POST request to http://127.0.0.1:8008/reload + Then I receive a response code 200 + Scenario: check dynamic configuration change via DCS When I patch global configuration with {"ttl": 20, "loop_wait": 5, "postgresql": {"parameters": {"max_connections": 101}}} Then Response on GET http://127.0.0.1:8008/patroni contains restart_pending after 11 seconds And Response on GET http://127.0.0.1:8009/patroni contains restart_pending after 11 seconds + And Response on GET http://127.0.0.1:8008/patroni contains new_value after 1 seconds Scenario: check the basic failover And I kill postgres0 diff --git a/features/environment.py b/features/environment.py index 01e94ce0..40b2f377 100644 --- a/features/environment.py +++ b/features/environment.py @@ -98,6 +98,13 @@ class PatroniController(AbstractController): except IOError: return None + def add_tag_to_config(self, tag, value): + with open(self._config) as r: + config = yaml.safe_load(r) + config['tags']['tag'] = value + with open(self._config, 'w') as w: + yaml.safe_dump(config, w, default_flow_style=False) + def _start(self): return subprocess.Popen(['coverage', 'run', '--source=patroni', '-p', 'patroni.py', self._config], stdout=self._log, stderr=subprocess.STDOUT, cwd=self._work_directory) @@ -126,6 +133,9 @@ class PatroniController(AbstractController): 'logging_collector': 'on', 'log_destination': 'csvlog', 'log_directory': self._output_dir, 'log_filename': name + '.log', 'log_statement': 'all', 'log_min_messages': 'debug1'}) + if 'bootstrap' in config and 'initdb' in config['bootstrap']: + config['bootstrap']['initdb'].extend([{'auth': 'md5'}, {'auth-host': 'md5'}]) + if tags: config['tags'] = tags @@ -347,7 +357,7 @@ class PatroniPoolController(object): self._processes[pg_name].start(max_wait_limit) def __getattr__(self, func): - if func not in ['stop', 'query', 'write_label', 'read_label', 'check_role_has_changed_to']: + if func not in ['stop', 'query', 'write_label', 'read_label', 'check_role_has_changed_to', 'add_tag_to_config']: raise AttributeError("PatroniPoolController instance has no attribute '{0}'".format(func)) def wrapper(pg_name, *args, **kwargs): diff --git a/features/steps/basic_replication.py b/features/steps/basic_replication.py index 4f441dac..c3033627 100644 --- a/features/steps/basic_replication.py +++ b/features/steps/basic_replication.py @@ -82,3 +82,8 @@ def check_http_response(context, url, value, timeout): else: assert False,\ "Value {0} is not present in response after {1} seconds".format(value, timeout) + + +@step('I add tag {tag:w} {value:w} to {pg_name:w} config') +def add_tag_to_config(context, tag, value, pg_name): + context.pctl.add_tag_to_config(pg_name, tag, value) diff --git a/patroni/__init__.py b/patroni/__init__.py index 34c7d84e..b8df602e 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -94,8 +94,8 @@ class Patroni(object): while True: if self._received_sighup: self._received_sighup = False - self.config.reload_local_configuration() - self.reload_config() + if self.config.reload_local_configuration(): + self.reload_config() logger.info(self.ha.run_cycle()) diff --git a/patroni/api.py b/patroni/api.py index 33404f28..d84ce7ff 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -112,6 +112,19 @@ class RestApiHandler(BaseHTTPRequestHandler): response = self.get_postgresql_status(True) self._write_status_response(200, response) + @check_auth + def do_POST_reload(self): + try: + configuration_is_changed = self.server.patroni.config.reload_local_configuration(True) + status_code = configuration_is_changed and 200 or 304 + response = configuration_is_changed and 'reload scheduled' or '' + if configuration_is_changed: + self.server.patroni.sighup_handler() + except Exception as e: + status_code = 500 + response = str(e) + self._write_response(status_code, response) + @check_auth def do_POST_restart(self): status_code = 500 diff --git a/patroni/config.py b/patroni/config.py index cf5ba85d..8cb43458 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -43,7 +43,7 @@ class Config(object): self._config_file = None if config_env else config_file self._dynamic_configuration = {} self._local_configuration = yaml.safe_load(config_env) if config_env else self._load_config_file() - self._build_effective_configuration() + self._build_effective_configuration(self._dynamic_configuration, self._local_configuration) self._data_dir = self.__effective_configuration['postgresql']['data_dir'] self._cache_file = os.path.join(self._data_dir, self.__CACHE_FILENAME) self._load_cache() @@ -94,14 +94,31 @@ class Config(object): def set_dynamic_configuration(self, configuration): if configuration and self._dynamic_configuration != configuration: - self._dynamic_configuration = configuration - self._build_effective_configuration() - self._cache_needs_saving = True - return True + try: + self._build_effective_configuration(configuration, self._local_configuration) + self._dynamic_configuration = configuration + self._cache_needs_saving = True + return True + except Exception: + logger.exception('Exception when setting dynamic_configuration') - def reload_local_configuration(self): - self._local_configuration = self._load_config_file() - self._build_effective_configuration() + def reload_local_configuration(self, dry_run=False): + if self.config_file: + try: + configuration = self._load_config_file() + if self._local_configuration != configuration: + old_effective_configuration = self.__effective_configuration + self._build_effective_configuration(self._dynamic_configuration, configuration) + if dry_run: + ret = old_effective_configuration != self.__effective_configuration + self.__effective_configuration = old_effective_configuration + return ret + self._local_configuration = configuration + return True + except Exception: + logger.exception('Exception when reloading local configuration from %s', self.config_file) + if dry_run: + raise def _process_postgresql_parameters(self, parameters, is_local=False): ret = {} @@ -114,10 +131,10 @@ class Config(object): ret[name] = value return ret - def _safe_copy_dynamic_configuration(self): + def _safe_copy_dynamic_configuration(self, dynamic_configuration): config = deepcopy(self.__DEFAULT_CONFIG) - for name, value in self._dynamic_configuration.items(): + for name, value in dynamic_configuration.items(): if name == 'postgresql': for name, value in (value or {}).items(): if name == 'parameters': @@ -128,9 +145,9 @@ class Config(object): config[name] = value return config - def _build_effective_configuration(self): - config = self._safe_copy_dynamic_configuration() - for name, value in self._local_configuration.items(): + def _build_effective_configuration(self, dynamic_configuration, local_configuration): + config = self._safe_copy_dynamic_configuration(dynamic_configuration) + for name, value in local_configuration.items(): if name == 'postgresql': for name, value in (value or {}).items(): if name == 'parameters': diff --git a/tests/test_api.py b/tests/test_api.py index 23541cec..9cfdd4ec 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -50,6 +50,7 @@ class MockHa(object): class MockPatroni(object): + config = Mock() postgresql = MockPostgresql() ha = MockHa() dcs = Mock() @@ -57,6 +58,10 @@ class MockPatroni(object): version = '0.00' noloadbalance = Mock(return_value=False) + @staticmethod + def sighup_handler(): + pass + class MockRequest(object): @@ -122,6 +127,10 @@ class TestRestApiHandler(unittest.TestCase): self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'POST /restart HTTP/1.0')) MockRestApiServer(RestApiHandler, 'POST /restart HTTP/1.0\nAuthorization:') + @patch.object(MockPatroni, 'sighup_handler', Mock(side_effect=Exception)) + def test_do_POST_reload(self): + MockRestApiServer(RestApiHandler, 'POST /reload HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0') + def test_do_POST_restart(self): request = 'POST /restart HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' self.assertIsNotNone(MockRestApiServer(RestApiHandler, request)) diff --git a/tests/test_config.py b/tests/test_config.py index ca2f2511..6987fa67 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -13,8 +13,17 @@ class TestConfig(unittest.TestCase): def setUp(self): self.config = Config(config_env='postgresql: {data_dir: foo}') + @patch.object(Config, '_build_effective_configuration', Mock(side_effect=Exception)) + def test_set_dynamic_configuration(self): + self.assertIsNone(self.config.set_dynamic_configuration({'foo': 'bar'})) + def test_reload_local_configuration(self): - self.assertIsNone(Config(config_file='postgres0.yml').reload_local_configuration()) + config = Config(config_file='postgres0.yml') + with patch.object(Config, '_load_config_file', Mock(return_value={})): + with patch.object(Config, '_build_effective_configuration', Mock(side_effect=Exception)): + self.assertRaises(Exception, config.reload_local_configuration, True) + self.assertTrue(config.reload_local_configuration(True)) + self.assertTrue(config.reload_local_configuration()) @patch('tempfile.mkstemp', Mock(return_value=[3000, 'blabla'])) @patch('os.path.exists', Mock(return_value=True)) diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 1cff2611..71575e82 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -64,6 +64,7 @@ class TestPatroni(unittest.TestCase): del os.environ[Patroni.PATRONI_CONFIG_VARIABLE] @patch('patroni.config.Config.save_cache', Mock()) + @patch('patroni.config.Config.reload_local_configuration', Mock(return_value=True)) def test_run(self): self.p.sighup_handler() self.p.ha.dcs.watch = Mock(side_effect=SleepException) From 073ef3784f1068afa1e3094fed8045585a5e09d4 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 27 May 2016 16:29:33 +0200 Subject: [PATCH 08/49] Implement PATCH /config --- features/basic_replication.feature | 6 ++-- features/patroni_api.feature | 6 ++-- features/steps/basic_replication.py | 17 --------- features/steps/patroni_api.py | 29 ++++++--------- patroni/__init__.py | 3 +- patroni/api.py | 56 +++++++++++++++++++++-------- patroni/config.py | 18 +++++----- patroni/ctl.py | 2 +- patroni/dcs/__init__.py | 6 ++-- tests/test_api.py | 40 ++++++++++++++------- 10 files changed, 100 insertions(+), 83 deletions(-) diff --git a/features/basic_replication.feature b/features/basic_replication.feature index 438b1dd7..ea5176be 100644 --- a/features/basic_replication.feature +++ b/features/basic_replication.feature @@ -9,14 +9,14 @@ Feature: basic replication Then table foo is present on postgres1 after 20 seconds Scenario: check local configuration reload - When I issue an empty POST request to http://127.0.0.1:8008/reload + Given I issue an empty POST request to http://127.0.0.1:8008/reload Then I receive a response code 304 When I add tag new_tag new_value to postgres0 config And I issue an empty POST request to http://127.0.0.1:8008/reload - Then I receive a response code 200 + Then I receive a response code 202 Scenario: check dynamic configuration change via DCS - When I patch global configuration with {"ttl": 20, "loop_wait": 5, "postgresql": {"parameters": {"max_connections": 101}}} + Given I issue a PATCH request to http://127.0.0.1:8008/config with {"ttl": 20, "loop_wait": 5, "postgresql": {"parameters": {"max_connections": 101}}} Then Response on GET http://127.0.0.1:8008/patroni contains restart_pending after 11 seconds And Response on GET http://127.0.0.1:8009/patroni contains restart_pending after 11 seconds And Response on GET http://127.0.0.1:8008/patroni contains new_value after 1 seconds diff --git a/features/patroni_api.feature b/features/patroni_api.feature index 31bbbdb2..22dce74f 100644 --- a/features/patroni_api.feature +++ b/features/patroni_api.feature @@ -13,7 +13,7 @@ Scenario: check API requests on a stand-alone server When I issue an empty POST request to http://127.0.0.1:8008/reinitialize Then I receive a response code 503 And I receive a response text "I am the leader, can not reinitialize" - When I issue a POST request to http://127.0.0.1:8008/failover with leader=postgres0 + When I issue a POST request to http://127.0.0.1:8008/failover with {"leader": "postgres0"} Then I receive a response code 500 And I receive a response text "failover is not possible: cluster does not have members except leader" When I issue an empty POST request to http://127.0.0.1:8008/failover @@ -36,7 +36,7 @@ Scenario: check API requests for the primary-replica pair Then postgres1 role is the secondary after 15 seconds Scenario: check the failover via the API - Given I issue a POST request to http://127.0.0.1:8008/failover with leader=postgres0,candidate=postgres1 + Given I issue a POST request to http://127.0.0.1:8008/failover with {"leader": "postgres0", "candidate": "postgres1"} Then I receive a response code 200 And postgres1 is a leader after 5 seconds And postgres1 role is the primary after 5 seconds @@ -45,7 +45,7 @@ Scenario: check the failover via the API Scenario: check the scheduled failover Given I issue a scheduled failover at http://127.0.0.1:8009 from postgres1 to postgres0 in 1 seconds - Then I receive a response code 200 + Then I receive a response code 202 And postgres0 is a leader after 20 seconds And postgres0 role is the primary after 5 seconds And postgres1 role is the secondary after 10 seconds diff --git a/features/steps/basic_replication.py b/features/steps/basic_replication.py index c3033627..05acf288 100644 --- a/features/steps/basic_replication.py +++ b/features/steps/basic_replication.py @@ -1,4 +1,3 @@ -import json import psycopg2 as pg import requests @@ -56,22 +55,6 @@ def replication_works(context, master, replica, time_limit): """.format(int(time()), master, replica, time_limit)) -def patch_config_with_data(config, data): - for name, value in data.items(): - if isinstance(value, dict): - patch_config_with_data(config[name], value) - else: - config[name] = value - - -@step('I patch global configuration with {data}') -def patch_config(context, data): - data = json.loads(data) - config = json.loads(context.dcs_ctl.query('config')) - patch_config_with_data(config, data) - context.dcs_ctl.set('config', json.dumps(config)) - - @then('Response on GET {url} contains {value} after {timeout:d} seconds') def check_http_response(context, url, value, timeout): for _ in range(int(timeout)): diff --git a/features/steps/patroni_api.py b/features/steps/patroni_api.py index d94086b5..d41eb373 100644 --- a/features/steps/patroni_api.py +++ b/features/steps/patroni_api.py @@ -1,3 +1,4 @@ +import json import parse import pytz import requests @@ -12,12 +13,7 @@ def parse_url(text): return text -@parse.with_pattern(r'(?:\w+=(?:\w|\.|:|-|\+|\s)+,?)+') -def parse_data(text): - return text - - -register_type(url=parse_url, data=parse_data) +register_type(url=parse_url) # there is no way we can find out if the node has already @@ -56,20 +52,17 @@ def do_get(context, url): @step('I issue an empty POST request to {url:url}') def do_post_empty(context, url): - do_post(context, url, None) + do_request(context, 'POST', url, None) -@step('I issue a POST request to {url:url} with {data:data}') -def do_post(context, url, data): - post_data = {} - if data: - post_components = data.split(',') - for pc in post_components: - if '=' in pc: - k, v = pc.split('=', 2) - post_data[k.strip()] = v.strip() +@step('I issue a {request_method:w} request to {url:url} with {data}') +def do_request(context, request_method, url, data): + data = data and json.loads(data) or {} try: - r = requests.post(url, json=post_data) + if request_method == 'PATCH': + r = requests.patch(url, json=data) + else: + r = requests.post(url, json=data) except requests.exceptions.RequestException: context.status_code = None context.response = None @@ -96,5 +89,5 @@ def check_response(context, component, data): @step('I issue a scheduled failover at {at_url:url} from {from_host:w} to {to_host:w} in {in_seconds:d} seconds') def scheduled_failover(context, at_url, from_host, to_host, in_seconds): context.execute_steps(u""" - Given I issue a POST request to {0}/failover with leader={1},candidate={2},scheduled_at={3} + Given I issue a POST request to {0}/failover with {{"leader": "{1}", "candidate": "{2}", "scheduled_at": "{3}"}} """.format(at_url, from_host, to_host, datetime.now(pytz.utc) + timedelta(seconds=int(in_seconds)))) diff --git a/patroni/__init__.py b/patroni/__init__.py index b8df602e..a8107362 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -100,8 +100,7 @@ class Patroni(object): logger.info(self.ha.run_cycle()) cluster = self.dcs.cluster - if cluster and cluster.config and cluster.config.data and \ - self.config.set_dynamic_configuration(cluster.config.data): + if cluster and cluster.config and self.config.set_dynamic_configuration(cluster.config.data): self.reload_config() if not self.postgresql.data_directory_empty(): diff --git a/patroni/api.py b/patroni/api.py index d84ce7ff..8858a4f5 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -45,6 +45,9 @@ class RestApiHandler(BaseHTTPRequestHandler): self.end_headers() self.wfile.write(body.encode('utf-8')) + def _write_json_response(self, status_code, response): + self._write_response(status_code, json.dumps(response), {'Content-Type': 'application/json'}) + def send_auth_request(self, body): headers = {'WWW-Authenticate': 'Basic realm="' + self.server.patroni.__class__.__name__ + '"'} self._write_response(401, body, headers) @@ -65,17 +68,15 @@ class RestApiHandler(BaseHTTPRequestHandler): def _write_status_response(self, status_code, response, options=False): if options: - body = None - else: - patroni = self.server.patroni - response.update({'tags': patroni.tags} if patroni.tags else {}) - if patroni.postgresql.sysid: - response['database_system_identifier'] = patroni.postgresql.sysid - if patroni.postgresql.restart_pending: - response['restart_pending'] = True - response['patroni'] = {'version': patroni.version, 'scope': patroni.postgresql.scope} - body = json.dumps(response) - self._write_response(status_code, body, {'Content-Type': 'application/json'}) + return self._write_response(status_code, None) + patroni = self.server.patroni + response.update({'tags': patroni.tags} if patroni.tags else {}) + if patroni.postgresql.sysid: + response['database_system_identifier'] = patroni.postgresql.sysid + if patroni.postgresql.restart_pending: + response['restart_pending'] = True + response['patroni'] = {'version': patroni.version, 'scope': patroni.postgresql.scope} + self._write_json_response(status_code, response) def do_GET(self, options=False): """Default method for processing all GET requests which can not be routed to other methods""" @@ -112,12 +113,39 @@ class RestApiHandler(BaseHTTPRequestHandler): response = self.get_postgresql_status(True) self._write_status_response(200, response) + def do_GET_config(self): + self._write_json_response(200, self.server.patroni.config.dynamic_configuration) + + @staticmethod + def _patch_config(config, data): + for name, value in data.items(): + if isinstance(value, dict) and name in config: + RestApiHandler._patch_config(config[name], value) + elif value is None: + config.pop(name, None) + else: + config[name] = value + + @check_auth + def do_PATCH_config(self): + content_length = int(self.headers.get('content-length', 0)) + request = json.loads(self.rfile.read(content_length).decode('utf-8')) + cluster = self.server.patroni.ha.dcs.get_cluster() + data = cluster.config.data.copy() + RestApiHandler._patch_config(data, request) + response_code = data == cluster.config.data and 304 or 200 + if response_code == 200: + self.server.patroni.ha.dcs.set_config_value(json.dumps(data, separators=(',', ':')), cluster.config.index) + self._write_json_response(200, data) + else: + self._write_response(304, None) + @check_auth def do_POST_reload(self): try: configuration_is_changed = self.server.patroni.config.reload_local_configuration(True) - status_code = configuration_is_changed and 200 or 304 - response = configuration_is_changed and 'reload scheduled' or '' + status_code = configuration_is_changed and 202 or 304 + response = configuration_is_changed and 'reload scheduled' or None if configuration_is_changed: self.server.patroni.sighup_handler() except Exception as e: @@ -218,7 +246,7 @@ class RestApiHandler(BaseHTTPRequestHandler): elif self.server.patroni.dcs.manual_failover(leader, candidate, scheduled_at=scheduled_at): self.server.patroni.dcs.event.set() data = 'Failover scheduled' - status_code = 200 + status_code = 202 else: data = 'failed to write failover key into DCS' status_code = 503 diff --git a/patroni/config.py b/patroni/config.py index 8cb43458..2fb24135 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -43,7 +43,8 @@ class Config(object): self._config_file = None if config_env else config_file self._dynamic_configuration = {} self._local_configuration = yaml.safe_load(config_env) if config_env else self._load_config_file() - self._build_effective_configuration(self._dynamic_configuration, self._local_configuration) + self.__effective_configuration = self._build_effective_configuration(self._dynamic_configuration, + self._local_configuration) self._data_dir = self.__effective_configuration['postgresql']['data_dir'] self._cache_file = os.path.join(self._data_dir, self.__CACHE_FILENAME) self._load_cache() @@ -93,9 +94,10 @@ class Config(object): logger.error('Can not remove temporary file %s', tmpfile) def set_dynamic_configuration(self, configuration): - if configuration and self._dynamic_configuration != configuration: + if self._dynamic_configuration != configuration: try: - self._build_effective_configuration(configuration, self._local_configuration) + self.__effective_configuration = self._build_effective_configuration(configuration, + self._local_configuration) self._dynamic_configuration = configuration self._cache_needs_saving = True return True @@ -107,13 +109,11 @@ class Config(object): try: configuration = self._load_config_file() if self._local_configuration != configuration: - old_effective_configuration = self.__effective_configuration - self._build_effective_configuration(self._dynamic_configuration, configuration) + new_configuration = self._build_effective_configuration(self._dynamic_configuration, configuration) if dry_run: - ret = old_effective_configuration != self.__effective_configuration - self.__effective_configuration = old_effective_configuration - return ret + return new_configuration != self.__effective_configuration self._local_configuration = configuration + self.__effective_configuration = new_configuration return True except Exception: logger.exception('Exception when reloading local configuration from %s', self.config_file) @@ -173,7 +173,7 @@ class Config(object): pg_config.update({p: config[p] for p in ('name', 'scope', 'retry_timeout', 'maximum_lag_on_failover') if p in config}) - self.__effective_configuration = config + return config def get(self, key, default=None): return self.__effective_configuration.get(key, default) diff --git a/patroni/ctl.py b/patroni/ctl.py index 08622086..3d93d2fd 100644 --- a/patroni/ctl.py +++ b/patroni/ctl.py @@ -534,7 +534,7 @@ def failover(config_file, cluster_name, master, candidate, force, dcs, scheduled r = None try: r = post_patroni(cluster.leader.member, 'failover', failover_value) - if r.status_code == 200: + if r.status_code in (200, 202): logging.debug(r) cluster = dcs.get_cluster() logging.debug(cluster) diff --git a/patroni/dcs/__init__.py b/patroni/dcs/__init__.py index fcf258b7..209214c0 100644 --- a/patroni/dcs/__init__.py +++ b/patroni/dcs/__init__.py @@ -169,14 +169,14 @@ class ClusterConfig(namedtuple('ClusterConfig', 'index,data')): @staticmethod def from_node(index, data): """ - >>> ClusterConfig.from_node(1, '{').data - {} + >>> ClusterConfig.from_node(1, '{') is None + True """ try: data = json.loads(data) except (TypeError, ValueError): - data = {} + return None return ClusterConfig(index, data) diff --git a/tests/test_api.py b/tests/test_api.py index 9cfdd4ec..edc0ebc7 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -4,7 +4,7 @@ import unittest from mock import Mock, patch from patroni.api import RestApiHandler, RestApiServer -from patroni.dcs import Member +from patroni.dcs import ClusterConfig, Member from six import BytesIO as IO from six.moves import BaseHTTPServer from six.moves.BaseHTTPServer import BaseHTTPRequestHandler @@ -90,6 +90,8 @@ class MockRestApiServer(RestApiServer): @patch('ssl.wrap_socket', Mock(return_value=0)) class TestRestApiHandler(unittest.TestCase): + _authorization = '\nAuthorization: Basic dGVzdDp0ZXN0' + def test_do_GET(self): MockRestApiServer(RestApiHandler, 'GET /replica') with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={})): @@ -127,12 +129,25 @@ class TestRestApiHandler(unittest.TestCase): self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'POST /restart HTTP/1.0')) MockRestApiServer(RestApiHandler, 'POST /restart HTTP/1.0\nAuthorization:') + @patch.object(MockPatroni, 'config') + def test_do_GET_config(self, mock_config): + mock_config.dynamic_configuration = {} + self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'GET /config')) + + @patch.object(MockHa, 'dcs') + def test_do_PATCH_config(self, mock_dcs): + mock_dcs.get_cluster.return_value.config = \ + ClusterConfig.from_node(1, '{"postgresql": {"use_slots": false, "parameters": {"wal_level": "logical"}}}') + request = 'PATCH /config HTTP/1.0' + self._authorization + '\nContent-Length: ' + self.assertIsNotNone(MockRestApiServer(RestApiHandler, request + '2\n\n{}')) + MockRestApiServer(RestApiHandler, request + '59\n\n{"ttl":5,"use_slots":true,"postgresql":{"parameters":null}}') + @patch.object(MockPatroni, 'sighup_handler', Mock(side_effect=Exception)) def test_do_POST_reload(self): - MockRestApiServer(RestApiHandler, 'POST /reload HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0') + self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'POST /reload HTTP/1.0' + self._authorization)) def test_do_POST_restart(self): - request = 'POST /restart HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' + request = 'POST /restart HTTP/1.0' + self._authorization self.assertIsNotNone(MockRestApiServer(RestApiHandler, request)) with patch.object(MockHa, 'restart', Mock(side_effect=Exception)): MockRestApiServer(RestApiHandler, request) @@ -140,7 +155,7 @@ class TestRestApiHandler(unittest.TestCase): @patch.object(MockHa, 'dcs') def test_do_POST_reinitialize(self, dcs): cluster = dcs.get_cluster.return_value - request = 'POST /reinitialize HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' + request = 'POST /reinitialize HTTP/1.0' + self._authorization MockRestApiServer(RestApiHandler, request) cluster.is_unlocked.return_value = False MockRestApiServer(RestApiHandler, request) @@ -161,19 +176,18 @@ class TestRestApiHandler(unittest.TestCase): def test_do_POST_failover(self, dcs): cluster = dcs.get_cluster.return_value - request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\nContent-Length: 0\n\n' + request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 0\n\n' MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'postgresql1' MockRestApiServer(RestApiHandler, request) - request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\n' +\ - 'Content-Length: 25\n\n{"leader": "postgresql1"}' + request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 25\n\n{"leader": "postgresql1"}' MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'postgresql2' - request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\n' +\ - 'Content-Length: 53\n\n{"leader": "postgresql1", "candidate": "postgresql2"}' + request = 'POST /failover HTTP/1.0' + self._authorization +\ + '\nContent-Length: 53\n\n{"leader": "postgresql1", "candidate": "postgresql2"}' MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'postgresql1' @@ -199,7 +213,7 @@ class TestRestApiHandler(unittest.TestCase): MockRestApiServer(RestApiHandler, request) # Valid future date - request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\nContent-Length: 103\n\n{"leader": ' +\ + request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 103\n\n{"leader": ' +\ '"postgresql1", "member": "postgresql2", "scheduled_at": "6016-02-15T18:13:30.568224+01:00"}' MockRestApiServer(RestApiHandler, request) with patch.object(MockPatroni, 'dcs') as d: @@ -207,16 +221,16 @@ class TestRestApiHandler(unittest.TestCase): MockRestApiServer(RestApiHandler, request) # Exception: No timezone specified - request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\nContent-Length: 97\n\n{"leader": ' +\ + request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 97\n\n{"leader": ' +\ '"postgresql1", "member": "postgresql2", "scheduled_at": "6016-02-15T18:13:30.568224"}' MockRestApiServer(RestApiHandler, request) # Exception: Scheduled in the past - request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\nContent-Length: 103\n\n{"leader": ' +\ + request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 103\n\n{"leader": ' +\ '"postgresql1", "member": "postgresql2", "scheduled_at": "1016-02-15T18:13:30.568224+01:00"}' MockRestApiServer(RestApiHandler, request) # Invalid date - request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\nContent-Length: 103\n\n{"leader": ' +\ + request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 103\n\n{"leader": ' +\ '"postgresql1", "member": "postgresql2", "scheduled_at": "2010-02-29T18:13:30.568224+01:00"}' self.assertIsNotNone(MockRestApiServer(RestApiHandler, request)) From 8b5d6e83e7db385ffba6db32b41849fa9f67b83b Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 27 May 2016 17:38:19 +0200 Subject: [PATCH 09/49] fix some bugs revaled by acceptance tests --- patroni/api.py | 27 +++++++++++++-------------- tests/test_api.py | 3 +++ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/patroni/api.py b/patroni/api.py index 8858a4f5..c44df093 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -34,23 +34,23 @@ def check_auth(func): class RestApiHandler(BaseHTTPRequestHandler): - def _write_response(self, status_code, body, headers=None): + def _write_response(self, status_code, body, content_type='text/html', headers=None): self.send_response(status_code) if body is not None: headers = headers or {} - if 'Content-Type' not in headers: - headers['Content-Type'] = 'text/html' - for name, value in (headers or {}).items(): + if content_type: + headers['Content-Type'] = content_type + for name, value in headers.items(): self.send_header(name, value) self.end_headers() self.wfile.write(body.encode('utf-8')) def _write_json_response(self, status_code, response): - self._write_response(status_code, json.dumps(response), {'Content-Type': 'application/json'}) + self._write_response(status_code, json.dumps(response), content_type='application/json') def send_auth_request(self, body): headers = {'WWW-Authenticate': 'Basic realm="' + self.server.patroni.__class__.__name__ + '"'} - self._write_response(401, body, headers) + self._write_response(401, body, headers=headers) def finish(self): try: @@ -133,21 +133,20 @@ class RestApiHandler(BaseHTTPRequestHandler): cluster = self.server.patroni.ha.dcs.get_cluster() data = cluster.config.data.copy() RestApiHandler._patch_config(data, request) - response_code = data == cluster.config.data and 304 or 200 - if response_code == 200: + if data != cluster.config.data: self.server.patroni.ha.dcs.set_config_value(json.dumps(data, separators=(',', ':')), cluster.config.index) self._write_json_response(200, data) else: - self._write_response(304, None) + self._write_response(304, '', '') @check_auth def do_POST_reload(self): try: - configuration_is_changed = self.server.patroni.config.reload_local_configuration(True) - status_code = configuration_is_changed and 202 or 304 - response = configuration_is_changed and 'reload scheduled' or None - if configuration_is_changed: - self.server.patroni.sighup_handler() + if not self.server.patroni.config.reload_local_configuration(True): + return self._write_response(304, '', '') + status_code = 202 + response = 'reload scheduled' + self.server.patroni.sighup_handler() except Exception as e: status_code = 500 response = str(e) diff --git a/tests/test_api.py b/tests/test_api.py index edc0ebc7..d7b0ece3 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -144,6 +144,9 @@ class TestRestApiHandler(unittest.TestCase): @patch.object(MockPatroni, 'sighup_handler', Mock(side_effect=Exception)) def test_do_POST_reload(self): + with patch.object(MockPatroni, 'config') as mock_config: + mock_config.reload_local_configuration.return_value = False + MockRestApiServer(RestApiHandler, 'POST /reload HTTP/1.0' + self._authorization) self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'POST /reload HTTP/1.0' + self._authorization)) def test_do_POST_restart(self): From 33b6c88fd5f1c5d8c58ab00086e2aa611b7d4612 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 30 May 2016 11:59:58 +0200 Subject: [PATCH 10/49] state_handler.follow needs to know cluster.leader --- patroni/ha.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/ha.py b/patroni/ha.py index 503d09df..fc9cd847 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -282,7 +282,7 @@ class Ha(object): sleep(2) # Give a time to somebody to promote cluster = self.dcs.get_cluster() node_to_follow = self._get_node_to_follow(cluster) - self.state_handler.follow(node_to_follow, True) + self.state_handler.follow(node_to_follow, cluster.leader, True) else: self.state_handler.follow(None, None) From f7912991a85eef68d7dbc5bbb0d9bf88001f29b3 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 30 May 2016 12:37:14 +0200 Subject: [PATCH 11/49] Reshuffle acceptance tests one more time --- features/patroni_api.feature | 8 +++++--- features/steps/patroni_api.py | 12 ++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/features/patroni_api.feature b/features/patroni_api.feature index a94ad8bc..0702f308 100644 --- a/features/patroni_api.feature +++ b/features/patroni_api.feature @@ -29,17 +29,19 @@ Scenario: check local configuration reload Scenario: check dynamic configuration change via DCS Given I issue a PATCH request to http://127.0.0.1:8008/config with {"ttl": 20, "loop_wait": 1, "postgresql": {"parameters": {"max_connections": 101}}} - And I start postgres1 - And replication works from postgres0 to postgres1 after 20 seconds + Then I receive a response code 200 + And I receive a response loop_wait 1 + And Response on GET http://127.0.0.1:8008/patroni contains restart_pending after 11 seconds When I issue a GET request to http://127.0.0.1:8008/config Then I receive a response code 200 And I receive a response loop_wait 1 When I issue a GET request to http://127.0.0.1:8008/patroni Then I receive a response code 200 - And I receive a response restart_pending True And I receive a response tags {'tag': 'new_value'} Scenario: check API requests for the primary-replica pair + Given I start postgres1 + And replication works from postgres0 to postgres1 after 20 seconds When I issue a GET request to http://127.0.0.1:8009/replica Then I receive a response code 200 And I receive a response state running diff --git a/features/steps/patroni_api.py b/features/steps/patroni_api.py index 1ecd1af7..90c38564 100644 --- a/features/steps/patroni_api.py +++ b/features/steps/patroni_api.py @@ -106,3 +106,15 @@ def scheduled_failover(context, at_url, from_host, to_host, in_seconds): @step('I add tag {tag:w} {value:w} to {pg_name:w} config') def add_tag_to_config(context, tag, value, pg_name): context.pctl.add_tag_to_config(pg_name, tag, value) + + +@then('Response on GET {url} contains {value} after {timeout:d} seconds') +def check_http_response(context, url, value, timeout): + for _ in range(int(timeout)): + r = requests.get(url) + if value in r.content.decode('utf-8'): + break + time.sleep(1) + else: + assert False,\ + "Value {0} is not present in response after {1} seconds".format(value, timeout) From b7359e7b0df6c687193fed062376c60c822541e4 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 30 May 2016 12:40:52 +0200 Subject: [PATCH 12/49] Rollback all changes to basic_replication.feature since I moved all functionality to patroni_api.feature --- features/basic_replication.feature | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/features/basic_replication.feature b/features/basic_replication.feature index 0f5799e0..be192715 100644 --- a/features/basic_replication.feature +++ b/features/basic_replication.feature @@ -1,5 +1,5 @@ Feature: basic replication - We should check that the basic bootstrapping, replication, failover and dyncamic configuration works. + We should check that the basic bootstrapping, replication and failover works. Scenario: check replication of a single table Given I start postgres0 @@ -9,9 +9,9 @@ Feature: basic replication Then table foo is present on postgres1 after 20 seconds Scenario: check the basic failover - And I kill postgres0 + When I kill postgres0 Then postgres1 role is the primary after 32 seconds When I start postgres0 Then postgres0 role is the secondary after 20 seconds When I add the table bar to postgres1 - Then table bar is present on postgres0 after 10 seconds + Then table bar is present on postgres0 after 20 seconds From 9379c036d5af4bfe0b35552517b10e994cb54701 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 30 May 2016 17:02:29 +0200 Subject: [PATCH 13/49] Add comments to `set_ttl` method To explain how it's supposed to work and why it manupulates with the cache of `Cluster` object (calls `reset_cluster`) --- patroni/dcs/consul.py | 2 ++ patroni/dcs/etcd.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/patroni/dcs/consul.py b/patroni/dcs/consul.py index 499c8ae1..d20be351 100644 --- a/patroni/dcs/consul.py +++ b/patroni/dcs/consul.py @@ -101,7 +101,9 @@ class Consul(AbstractDCS): except Exception: logger.exception("Can not destroy session %s", self._session) self._session = None + # force `watch` method to call `AbstractDCS.watch` instead of watching for leader key self.reset_cluster() + # fire up an event to wake up from `watch` and immediately run HA loop (to create the new session) self.event.set() self._ttl = ttl diff --git a/patroni/dcs/etcd.py b/patroni/dcs/etcd.py index beb494b5..48afe914 100644 --- a/patroni/dcs/etcd.py +++ b/patroni/dcs/etcd.py @@ -218,7 +218,9 @@ class Etcd(AbstractDCS): def set_ttl(self, ttl): ttl = int(ttl) if self._ttl != ttl: + # force `watch` method to call `AbstractDCS.watch` instead of watching for leader key self.reset_cluster() + # fire up an event to wake up from `watch` and immediately run HA loop (to update TTL of leader and member) self.event.set() self._ttl = ttl From 515e9e34f4677ec7e45469258bc3be4b732c64af Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 31 May 2016 08:58:21 +0200 Subject: [PATCH 14/49] Update SETTINGS.rst accordingly to the new config --- SETTINGS.rst | 67 +++++++++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 30 deletions(-) diff --git a/SETTINGS.rst b/SETTINGS.rst index a5ba11b5..6b0d4153 100644 --- a/SETTINGS.rst +++ b/SETTINGS.rst @@ -4,26 +4,52 @@ YAML Configuration Settings Global/Universal ---------------- -- **loop\_wait**: the number of seconds the loop will sleep. -- **ttl**: the TTL to acquire the leader lock. Think of it as the length of time before initiation of the automatic failover process. +- **name**: the name of the host. Must be unique for the cluster. +- **namespace**: path within configuration store where Patroni will keep information about cluster. Default value: "/service" +- **scope**: cluster name + +Bootstrap configuration +----------------------- +- **dcs**: This section will be written into `///config` of a given configuration store after initializing of new cluster. This is the global configuration for the cluster. If you want to change some parameters for all cluster nodes - just do it in DCS (or via Patroni API) and all nodes will apply this configuration. + - **loop\_wait**: the number of seconds the loop will sleep. Default value: 10 + - **ttl**: the TTL to acquire the leader lock. Think of it as the length of time before initiation of the automatic failover process. Default value: 30 + - **maximum\_lag\_on\_failover**: the maximum bytes a follower may lag to be able to participate in leader election. + - **postgresql**: + - **use\_pg\_rewind**:whether or not to use pg_rewind + - **use\_slots**: whether or not to use replication_slots. Must be False for PostgreSQL 9.3. You should comment out max_replication_slots before it becomes ineligible for leader status. + - **recovery\_conf**: additional configuration settings written to recovery.conf when configuring follower. + - **parameters**: list of configuration settings for Postgres. Many of these are required for replication to work. +- **initdb**: List options to be passed on to initdb. + - **- data-checksums**: Must be enabled when pg_rewind is needed on 9.3. + - **- encoding: UTF8**: default encoding for new databases. + - **- locale: UTF8**: default locale for new databases. +- **pg\_hba**: list of lines that you should add to pg\_hba.conf. + - **- host all all 0.0.0.0/0 md5**. + - **- host replication replicator 127.0.0.1/32 md5**: A line like this is required for replication. +- **users**: Some additional users users which needs to be created after initializing new cluster + - **admin**: the name of user + - **password: zalando**: + - **options**: list of options for CREATE USER statement + - **- createrole** + - **- createdb** Consul ------ - **host**: the host:port for the Consul endpoint. -- **scope**: the relative path used on Consul's HTTP API for this deployment; makes it possible to run multiple HA deployments from a single Consul cluster. -- **ttl**: the TTL to acquire the leader lock. Think of it as the length of time before initiation of the automatic failover process. etcd ---- - **host**: the host:port for the etcd endpoint. -- **scope**: the relative path used on etcd's HTTP API for this deployment. Makes it possible to run multiple HA deployments from a single etcd cluster. -- **ttl**: the TTL to acquire the leader lock. Think of it as the length of time before initiation of the automatic failover process. PostgreSQL ---------------- -- **admin**: - - **password**: admin password; user is created during initialization. - - **username**: admin username; user is created during initialization. It will have CREATEDB and CREATEROLE privileges. +---------- +- **authentication**: + - **superuser**: + - **username**: name for the superuser, set during initialization (initdb) and later used by Patroni to connect to the postgres. + - **password**: password for the superuser, set during initialization (initdb). + - **replication**: + - **username**: replication username; user will be created during initialization. + - **password**: replication password; user will be created during initialization. - **callbacks**: callback scripts to run on certain actions. Patroni will pass the action, role and cluster name. (See scripts/aws.py as an example of how to write them.) - **on\_reload**: run this script when configuration reload is triggered. - **on\_restart**: run this script when the cluster restarts. @@ -33,25 +59,10 @@ PostgreSQL - **connect\_address**: IP address + port through which Postgres is accessible from other nodes and applications. - **create\_replica\_methods**: an ordered list of the create methods for turning a Patroni node into a new replica. "basebackup" is the default method; other methods are assumed to refer to scripts, each of which is configured as its own config item. - **data\_dir**: file path to initialize and store Postgres data files. -- **initdb**: List options to be passed on to initdb. - - **data-checksums**: Must be enabled when pg_rewind is needed on 9.3. - - **encoding**: default encoding for new databases. - - **locale**: default locale for new databases. - **listen**: IP address + port that Postgres listens to; must be accessible from other nodes in the cluster, if you're using streaming replication. Multiple comma-separated addresses are permitted, as long as the port component is appended after to the last one with a colon, i.e. ``listen: 127.0.0.1,127.0.0.2:5432``. Patroni will use the first address from this list to establish local connections to the PostgreSQL node. -- **maximum\_lag\_on\_failover**: the maximum bytes a follower may lag. -- **name**: the name of the Postgres host. Must be unique for the cluster. -- **pg\_hba**: list of lines that you should add to pg\_hba.conf. - - **- host all all 0.0.0.0/0 md5**. - - **- host replication replicator 127.0.0.1/32 md5**: A line like this is required for replication. - **recovery\_conf**: additional configuration settings written to recovery.conf when configuring follower. - - **parameters**: list of configuration settings for Postgres. Many of these are required for replication to work. +- **parameters**: list of configuration settings for Postgres. Many of these are required for replication to work. - **replica\_method** for each create_replica_method other than basebackup, you would add a configuration section of the same name. At a minimum, this should include "command" with a full path to the actual script to be executed. Other configuration parameters will be passed along to the script in the form "parameter=value". -- **replication**: - - **username**: replication username; user will be created during initialization. - - **password**: replication password; user will be created during initialization. -- **use\_slots**: whether or not to use replication_slots. Must be False for PostgreSQL 9.3. You should comment out max_replication_slots before it becomes ineligible for leader status. -- **superuser**: - - **password**: password for the Postgres user, set during initialization. REST API -------- @@ -65,10 +76,6 @@ REST API ZooKeeper ---------- - **hosts**: list of ZooKeeper cluster members in format: ['host1:port1', 'host2:port2', 'etc...']. -- **reconnect\_timeout**: how long you should try to reconnect to ZooKeeper after a connection loss. After this timeout, assume that you no longer have a lock and restart in read-only mode. -- **scope**: the relative path used on ZooKeeper for this deployment. Makes it possible to run multiple HA deployments from a single ZooKeeper cluster. -- **session\_timeout**: the TTL to acquire the leader lock. Think of it as the length of time before initiation of the automatic failover process. - - **exhibitor**: If you are running a ZooKeeper cluster under the Exhibitor supervisory, this section might interest you: - **hosts**: initial list of Exhibitor (ZooKeeper) nodes in format: ['host1', 'host2', 'etc...' ]. This list updates automatically whenever the Exhibitor (ZooKeeper) cluster topology changes. - **poll\_interval**: how often the list of ZooKeeper and Exhibitor nodes should be updated from Exhibitor From b3ada161cf6d67cbd1656fa0fba92c5b61ebb6a1 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 31 May 2016 10:30:53 +0200 Subject: [PATCH 15/49] Implement possibility to configure `retry_timeout` globally Previously it was hardcoded all over the place. --- patroni/__init__.py | 1 + patroni/config.py | 2 +- patroni/dcs/__init__.py | 4 ++++ patroni/dcs/consul.py | 9 ++++++--- patroni/dcs/etcd.py | 5 ++++- patroni/dcs/zookeeper.py | 12 +++++++----- patroni/postgresql.py | 4 +++- postgres0.yml | 2 +- tests/test_consul.py | 5 ++++- tests/test_ctl.py | 2 +- tests/test_etcd.py | 2 +- tests/test_ha.py | 5 +++-- tests/test_postgresql.py | 6 +++--- tests/test_zookeeper.py | 5 ++++- 14 files changed, 43 insertions(+), 21 deletions(-) diff --git a/patroni/__init__.py b/patroni/__init__.py index a8107362..9fc15f50 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -57,6 +57,7 @@ class Patroni(object): self.tags = self.get_tags() self.nap_time = self.config['loop_wait'] self.dcs.set_ttl(self.config.get('ttl') or 30) + self.dcs.set_retry_timeout(self.config.get('retry_timeout') or self.nap_time) self.api.reload_config(self.config['restapi']) self.postgresql.reload_config(self.config['postgresql']) except Exception: diff --git a/patroni/config.py b/patroni/config.py index 2fb24135..4e03b480 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -32,7 +32,7 @@ class Config(object): __CACHE_FILENAME = 'patroni.dynamic.json' __DEFAULT_CONFIG = { - 'ttl': 30, 'loop_wait': 10, 'retry_timeout': 5, + 'ttl': 30, 'loop_wait': 10, 'retry_timeout': 10, 'maximum_lag_on_failover': 1048576, 'postgresql': { 'parameters': Postgresql.CMDLINE_OPTIONS diff --git a/patroni/dcs/__init__.py b/patroni/dcs/__init__.py index 209214c0..d4da8e6b 100644 --- a/patroni/dcs/__init__.py +++ b/patroni/dcs/__init__.py @@ -265,6 +265,10 @@ class AbstractDCS(object): def set_ttl(self, ttl): """Set the new ttl value for leader key""" + @abc.abstractmethod + def set_retry_timeout(self, retry_timeout): + """Set the new value for retry_timeout""" + @abc.abstractmethod def _load_cluster(self): """Internally this method should build `Cluster` object which diff --git a/patroni/dcs/consul.py b/patroni/dcs/consul.py index d20be351..4c5a4bc1 100644 --- a/patroni/dcs/consul.py +++ b/patroni/dcs/consul.py @@ -21,9 +21,8 @@ class HTTPClient(std.HTTPClient): def __init__(self, *args, **kwargs): super(HTTPClient, self).__init__(*args, **kwargs) - self._patch_default_timeout() - def _patch_default_timeout(self): + def patch_default_timeout(self, timeout): # Set a default timeout for the `request.session.request` method, that is used # internally by the methods request.session.get, request.session.post and # others. We monkey-patch here to avoid reimplementing each individual method from @@ -78,6 +77,7 @@ class Consul(AbstractDCS): self.set_ttl(config.get('ttl') or 30) host, port = config.get('host', '127.0.0.1:8500').split(':') self._client = ConsulClient(host=host, port=port) + self._client.http.patch_default_timeout(config['retry_timeout']/2.0) self._scope = config['scope'] self.create_or_restore_session() @@ -93,7 +93,7 @@ class Consul(AbstractDCS): sleep(5) def set_ttl(self, ttl): - ttl = int(ttl/2) # My experiments have shown that session expires after 2*ttl time + ttl = ttl/2.0 # My experiments have shown that session expires after 2*ttl time if self._ttl != ttl: if self._session: try: @@ -107,6 +107,9 @@ class Consul(AbstractDCS): self.event.set() self._ttl = ttl + def set_retry_timeout(self, retry_timeout): + self._client.http.patch_default_timeout(retry_timeout/2.0) + def refresh_session(self): """:returns: `!True` if it had to create new session""" if self._session: diff --git a/patroni/dcs/etcd.py b/patroni/dcs/etcd.py index 48afe914..467e1ddd 100644 --- a/patroni/dcs/etcd.py +++ b/patroni/dcs/etcd.py @@ -194,7 +194,7 @@ class Etcd(AbstractDCS): def __init__(self, config): super(Etcd, self).__init__(config) self._ttl = int(config.get('ttl') or 30) - self._retry = Retry(deadline=10, max_delay=1, max_tries=-1, + self._retry = Retry(deadline=config['retry_timeout'], max_delay=1, max_tries=-1, retry_exceptions=(etcd.EtcdConnectionFailed, etcd.EtcdLeaderElectionInProgress, etcd.EtcdWatcherCleared, @@ -224,6 +224,9 @@ class Etcd(AbstractDCS): self.event.set() self._ttl = ttl + def set_retry_timeout(self, retry_timeout): + self._retry.deadline = retry_timeout + @staticmethod def member(node): return Member.from_node(node.modifiedIndex, os.path.basename(node.key), node.ttl, node.value) diff --git a/patroni/dcs/zookeeper.py b/patroni/dcs/zookeeper.py index d4d680ee..29367419 100644 --- a/patroni/dcs/zookeeper.py +++ b/patroni/dcs/zookeeper.py @@ -83,9 +83,8 @@ class ZooKeeper(AbstractDCS): self.exhibitor = ExhibitorEnsembleProvider(exhibitor['hosts'], exhibitor['port'], poll_interval=interval) hosts = self.exhibitor.zookeeper_hosts - self._client = KazooClient(hosts=hosts, timeout=(config.get('session_timeout') or config.get('ttl') or 30), - command_retry={'deadline': (config.get('reconnect_timeout') or 10), - 'max_delay': 1, 'max_tries': -1}, + self._client = KazooClient(hosts=hosts, timeout=config['ttl'], + command_retry={'deadline': config['retry_timeout'], 'max_delay': 1, 'max_tries': -1}, connection_retry={'max_delay': 1, 'max_tries': -1}) self._client.add_listener(self.session_listener) @@ -105,12 +104,15 @@ class ZooKeeper(AbstractDCS): def set_ttl(self, ttl): ttl = int(ttl * 1000) - # I know, it's weird to access private attributes and method - # but there is no other way to change session_timeout without losing session + # I know, it's weird to access private attributes, but there is + # no other way to change session_timeout without losing session if self._client._session_timeout != ttl: self._client._session_timeout = ttl self._client.restart() + def set_retry_timeout(self, retry_timeout): + self._client._retry.deadline = retry_timeout + def get_node(self, key, watch=None): try: ret = self._client.get(key, watch) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index fd8a3692..22916086 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -95,7 +95,8 @@ class Postgresql(object): self._cursor_holder = None 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) + self.retry = Retry(max_tries=-1, deadline=config['retry_timeout']/2.0, max_delay=1, + retry_exceptions=PostgresConnectionException) self._state_lock = Lock() self.set_state('stopped') @@ -160,6 +161,7 @@ class Postgresql(object): if reload_pending: self._write_postgresql_conf() self.reload() + self.retry.deadline = config['retry_timeout']/2.0 @property def restart_pending(self): diff --git a/postgres0.yml b/postgres0.yml index f6c6ca07..e09cb4f0 100644 --- a/postgres0.yml +++ b/postgres0.yml @@ -15,7 +15,7 @@ bootstrap: dcs: ttl: 30 loop_wait: 10 - retry_timeout: 5 + retry_timeout: 10 maximum_lag_on_failover: 1048576 postgresql: use_pg_rewind: true diff --git a/tests/test_consul.py b/tests/test_consul.py index 2c5bb36d..baf28719 100644 --- a/tests/test_consul.py +++ b/tests/test_consul.py @@ -51,7 +51,7 @@ class TestConsul(unittest.TestCase): @patch.object(consul.Consul.KV, 'get', kv_get) @patch.object(consul.Consul.KV, 'delete', Mock()) def setUp(self): - self.c = Consul({'ttl': 30, 'scope': 'test', 'name': 'postgresql1', 'host': 'localhost:1'}) + self.c = Consul({'ttl': 30, 'scope': 'test', 'name': 'postgresql1', 'host': 'localhost:1', 'retry_timeout': 10}) self.c._base_path = '/service/good' self.c._load_cluster() @@ -133,3 +133,6 @@ class TestConsul(unittest.TestCase): @patch.object(consul.Consul.Session, 'destroy', Mock(side_effect=ConsulException)) def test_set_ttl(self): self.c.set_ttl(20) + + def test_set_retry_timeout(self): + self.c.set_retry_timeout(10) diff --git a/tests/test_ctl.py b/tests/test_ctl.py index d8dbecb7..ff491ba6 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -51,7 +51,7 @@ class TestCtl(unittest.TestCase): self.runner = CliRunner() with patch.object(etcd.Client, 'machines') as mock_machines: mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) - self.e = get_dcs({'etcd': {'ttl': 30, 'host': 'ok:2379'}}, 'foo') + self.e = get_dcs({'etcd': {'ttl': 30, 'host': 'ok:2379', 'retry_timeout': 10}}, 'foo') @patch('psycopg2.connect', psycopg2_connect) def test_get_cursor(self): diff --git a/tests/test_etcd.py b/tests/test_etcd.py index 4d248e11..d675db43 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -193,7 +193,7 @@ class TestEtcd(unittest.TestCase): def setUp(self): with patch.object(Client, 'machines') as mock_machines: mock_machines.__get__ = Mock(return_value=['http://localhost:2379', 'http://localhost:4001']) - self.etcd = Etcd({'namespace': '/patroni/', 'ttl': 30, + self.etcd = Etcd({'namespace': '/patroni/', 'ttl': 30, 'retry_timeout': 10, 'host': 'localhost:2379', 'scope': 'test', 'name': 'foo'}) def test_base_path(self): diff --git a/tests/test_ha.py b/tests/test_ha.py index f8f1f90c..ee90eae0 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -102,7 +102,7 @@ class TestHa(unittest.TestCase): with patch.object(etcd.Client, 'machines') as mock_machines: mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) self.p = Postgresql({'name': 'postgresql0', 'scope': 'dummy', 'listen': '127.0.0.1:5432', - 'data_dir': 'data/postgresql0', + 'data_dir': 'data/postgresql0', 'retry_timeout': 10, 'authentication': {'superuser': {'username': 'foo', 'password': 'bar'}, 'replication': {'username': '', 'password': ''}}, 'parameters': {'wal_level': 'hot_standby', 'max_replication_slots': 5, 'foo': 'bar', @@ -111,7 +111,8 @@ class TestHa(unittest.TestCase): self.p.set_role('replica') self.p.check_replication_lag = true self.p.can_create_replica_without_replication_connection = MagicMock(return_value=False) - self.e = get_dcs({'etcd': {'ttl': 30, 'host': 'ok:2379', 'scope': 'test', 'name': 'foo'}}) + self.e = get_dcs({'etcd': {'ttl': 30, 'host': 'ok:2379', 'scope': 'test', + 'name': 'foo', 'retry_timeout': 10}}) self.ha = Ha(MockPatroni(self.p, self.e)) self.ha._async_executor.run_async = run_async self.ha.old_cluster = self.e.get_cluster() diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 38d89ed1..31ec0c60 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -160,7 +160,7 @@ class TestPostgresql(unittest.TestCase): self.data_dir = 'data/test0' if not os.path.exists(self.data_dir): os.makedirs(self.data_dir) - self.p = Postgresql({'name': 'test0', 'scope': 'batman', 'data_dir': self.data_dir, + self.p = Postgresql({'name': 'test0', 'scope': 'batman', 'data_dir': self.data_dir, 'retry_timeout': 10, 'listen': '127.0.0.1, *:5432', 'connect_address': '127.0.0.2:5432', 'authentication': {'superuser': {'username': 'test', 'password': 'test'}, 'replication': {'username': 'replicator', 'password': 'rep-pass'}}, @@ -475,8 +475,8 @@ class TestPostgresql(unittest.TestCase): self.assertFalse(self.p.replica_method_can_work_without_replication_connection('foo')) def test_reload_config(self): - self.p.reload_config({'listen': '*', 'parameters': self._PARAMETERS}) - self.p.reload_config({'listen': '*:5433', 'parameters': self._PARAMETERS}) + self.p.reload_config({'retry_timeout': 10, 'listen': '*', 'parameters': self._PARAMETERS}) + self.p.reload_config({'retry_timeout': 10, 'listen': '*:5433', 'parameters': self._PARAMETERS}) @patch.object(builtins, 'open', mock_open(read_data='9.4')) def test_get_major_version(self): diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py index b2e6e57e..71c9093f 100644 --- a/tests/test_zookeeper.py +++ b/tests/test_zookeeper.py @@ -104,7 +104,7 @@ class TestZooKeeper(unittest.TestCase): @patch('patroni.dcs.zookeeper.KazooClient', MockKazooClient) def setUp(self): self.zk = ZooKeeper({'exhibitor': {'hosts': ['localhost', 'exhibitor'], 'port': 8181}, - 'scope': 'test', 'name': 'foo'}) + 'scope': 'test', 'name': 'foo', 'ttl': 30, 'retry_timeout': 10}) def test_session_listener(self): self.zk.session_listener(KazooState.SUSPENDED) @@ -112,6 +112,9 @@ class TestZooKeeper(unittest.TestCase): def test_set_ttl(self): self.zk.set_ttl(20) + def test_set_retry_timeout(self): + self.zk.set_retry_timeout(10) + def test_get_node(self): self.assertIsNone(self.zk.get_node('/no_node')) From a40377fac13b1b3fa5f02d0fb0724ad99f2f9ea2 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 31 May 2016 10:34:21 +0200 Subject: [PATCH 16/49] rename options to only_status_code --- patroni/api.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/patroni/api.py b/patroni/api.py index c44df093..ef800753 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -66,8 +66,8 @@ class RestApiHandler(BaseHTTPRequestHandler): status = self.server.check_auth_header(auth_header) return not status or self.send_auth_request(status) - def _write_status_response(self, status_code, response, options=False): - if options: + def _write_status_response(self, status_code, response, only_status_code=False): + if only_status_code: return self._write_response(status_code, None) patroni = self.server.patroni response.update({'tags': patroni.tags} if patroni.tags else {}) @@ -78,7 +78,7 @@ class RestApiHandler(BaseHTTPRequestHandler): response['patroni'] = {'version': patroni.version, 'scope': patroni.postgresql.scope} self._write_json_response(status_code, response) - def do_GET(self, options=False): + def do_GET(self, only_status_code=False): """Default method for processing all GET requests which can not be routed to other methods""" path = '/master' if self.path == '/' else self.path @@ -104,10 +104,10 @@ class RestApiHandler(BaseHTTPRequestHandler): status_code = 200 else: status_code = 503 - self._write_status_response(status_code, response, options) + self._write_status_response(status_code, response, only_status_code) def do_OPTIONS(self): - self.do_GET(options=True) + self.do_GET(only_status_code=True) def do_GET_patroni(self): response = self.get_postgresql_status(True) From d47671e5b645544e1e407146e6f1b3f03bd042e8 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 31 May 2016 13:18:18 +0200 Subject: [PATCH 17/49] ALTER USER does not add LOGIN to the non-login role --- patroni/postgresql.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 22916086..452e2e6d 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -724,7 +724,11 @@ class Postgresql(object): self.call_nowait(ACTION_ON_ROLE_CHANGE) return ret - def create_or_update_user(self, name, password, options): + def create_or_update_role(self, name, password, options): + options = list(map(str.upper, options)) + if 'NOLOGIN' not in options and 'LOGIN' not in options: + options.append('LOGIN') + self.query("""DO $$ BEGIN SET local synchronous_commit = 'local'; @@ -735,7 +739,7 @@ BEGIN CREATE USER "{0}" WITH {1} PASSWORD %s; END IF; END; -$$""".format(name, options), name, password, password) +$$""".format(name, ' '.join(options)), name, password, password) def xlog_position(self): return self.query("""SELECT pg_xlog_location_diff(CASE WHEN pg_is_in_recovery() @@ -804,8 +808,8 @@ $$""".format(name, options), name, password, password) if self._initialize(config) and self.start(): for name, value in config['users'].items(): if name not in (self._superuser.get('username'), self._replication['username']): - self.create_or_update_user(name, value['password'], ' '.join(value.get('options', [])).upper()) - self.create_or_update_user(self._replication['username'], self._replication['password'], 'REPLICATION') + self.create_or_update_role(name, value['password'], value.get('options', [])) + self.create_or_update_role(self._replication['username'], self._replication['password'], ['REPLICATION']) else: raise PostgresException("Could not bootstrap master PostgreSQL") From 1cd42d4e47f9e373a17d107f9c489dda72f01252 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 31 May 2016 14:42:00 +0200 Subject: [PATCH 18/49] Get rid from some stupid logic with options=True/False And some other tricks with overriding handle_one_request and finish methods from the parent class which were necessary only to make OPTIONS request from haproxy work with python2, but in fact it was still not working with python3. Instead of doing all the magic we should simply give to haproxy what it wants to get: HTTP response code and nothing more. --- patroni/api.py | 46 ++++++++++++++++------------------------------ tests/test_api.py | 13 ------------- 2 files changed, 16 insertions(+), 43 deletions(-) diff --git a/patroni/api.py b/patroni/api.py index ef800753..ad688f57 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -3,7 +3,6 @@ import fcntl import json import logging import psycopg2 -import socket import time import dateutil import datetime @@ -36,14 +35,13 @@ class RestApiHandler(BaseHTTPRequestHandler): def _write_response(self, status_code, body, content_type='text/html', headers=None): self.send_response(status_code) - if body is not None: - headers = headers or {} - if content_type: - headers['Content-Type'] = content_type - for name, value in headers.items(): - self.send_header(name, value) - self.end_headers() - self.wfile.write(body.encode('utf-8')) + headers = headers or {} + if content_type: + headers['Content-Type'] = content_type + for name, value in headers.items(): + self.send_header(name, value) + self.end_headers() + self.wfile.write(body.encode('utf-8')) def _write_json_response(self, status_code, response): self._write_response(status_code, json.dumps(response), content_type='application/json') @@ -52,23 +50,12 @@ class RestApiHandler(BaseHTTPRequestHandler): headers = {'WWW-Authenticate': 'Basic realm="' + self.server.patroni.__class__.__name__ + '"'} self._write_response(401, body, headers=headers) - def finish(self): - try: - if not self.wfile.closed: - self.wfile.flush() - self.wfile.close() - except socket.error: - pass - self.rfile.close() - def check_auth_header(self): auth_header = self.headers.get('Authorization') status = self.server.check_auth_header(auth_header) return not status or self.send_auth_request(status) - def _write_status_response(self, status_code, response, only_status_code=False): - if only_status_code: - return self._write_response(status_code, None) + def _write_status_response(self, status_code, response): patroni = self.server.patroni response.update({'tags': patroni.tags} if patroni.tags else {}) if patroni.postgresql.sysid: @@ -78,7 +65,7 @@ class RestApiHandler(BaseHTTPRequestHandler): response['patroni'] = {'version': patroni.version, 'scope': patroni.postgresql.scope} self._write_json_response(status_code, response) - def do_GET(self, only_status_code=False): + def do_GET(self, write_status_code_only=False): """Default method for processing all GET requests which can not be routed to other methods""" path = '/master' if self.path == '/' else self.path @@ -104,10 +91,15 @@ class RestApiHandler(BaseHTTPRequestHandler): status_code = 200 else: status_code = 503 - self._write_status_response(status_code, response, only_status_code) + + if write_status_code_only: # when haproxy sends OPTIONS request it reads only statue code and nothing more + message = self.responses[status_code][0] + self.wfile.write(("%s %d %s\r\n" % (self.protocol_version, status_code, message)).encode('utf-8')) + else: + self._write_status_response(status_code, response) def do_OPTIONS(self): - self.do_GET(only_status_code=True) + self.do_GET(write_status_code_only=True) def do_GET_patroni(self): response = self.get_postgresql_status(True) @@ -285,12 +277,6 @@ class RestApiHandler(BaseHTTPRequestHandler): self.command = mname return ret - def handle_one_request(self): - try: - BaseHTTPRequestHandler.handle_one_request(self) - except socket.error: - pass - def query(self, sql, *params, **kwargs): if not kwargs.get('retry', False): return self.server.query(sql, *params) diff --git a/tests/test_api.py b/tests/test_api.py index d7b0ece3..44dfead1 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1,5 +1,4 @@ import psycopg2 -import socket import unittest from mock import Mock, patch @@ -7,7 +6,6 @@ from patroni.api import RestApiHandler, RestApiServer from patroni.dcs import ClusterConfig, Member from six import BytesIO as IO from six.moves import BaseHTTPServer -from six.moves.BaseHTTPServer import BaseHTTPRequestHandler from test_postgresql import psycopg2_connect, MockCursor @@ -111,17 +109,6 @@ class TestRestApiHandler(unittest.TestCase): def test_do_OPTIONS(self): self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'OPTIONS / HTTP/1.0')) - with patch.object(BaseHTTPRequestHandler, 'handle_one_request') as mock_handle_request: - mock_handle_request.side_effect = socket.error("foo") - MockRestApiServer(RestApiHandler, 'OPTIONS / HTTP/1.0') - - # make sure socket.error gets propagated via wfile object in finalize() - with patch.object(MockRequest, 'makefile') as makefile: - makefile.return_value.closed = False - makefile.return_value.readline = Mock(return_value=b'foo') - makefile.return_value.flush = Mock(side_effect=socket.error('foo')) - MockRestApiServer(RestApiHandler, 'OPTIONS / HTTP/1.0') - def test_do_GET_patroni(self): self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'GET /patroni')) From e10873dd9c09cd15ec4dcd7fce17cf8e53fc7ae9 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 31 May 2016 15:49:55 +0200 Subject: [PATCH 19/49] RestApiHandler._patch_config returns True if configuration was changed --- patroni/api.py | 20 ++++++++++++++------ tests/test_api.py | 10 +++++++--- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/patroni/api.py b/patroni/api.py index ad688f57..18eb21eb 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -110,13 +110,22 @@ class RestApiHandler(BaseHTTPRequestHandler): @staticmethod def _patch_config(config, data): + is_changed = False for name, value in data.items(): - if isinstance(value, dict) and name in config: - RestApiHandler._patch_config(config[name], value) - elif value is None: - config.pop(name, None) + if value is None: + if config.pop(name, None) is not None: + is_changed = True + elif name in config: + if isinstance(value, dict): + if RestApiHandler._patch_config(config[name], value): + is_changed = True + elif str(config[name]) != str(value): + config[name] = value + is_changed = True else: config[name] = value + is_changed = True + return is_changed @check_auth def do_PATCH_config(self): @@ -124,8 +133,7 @@ class RestApiHandler(BaseHTTPRequestHandler): request = json.loads(self.rfile.read(content_length).decode('utf-8')) cluster = self.server.patroni.ha.dcs.get_cluster() data = cluster.config.data.copy() - RestApiHandler._patch_config(data, request) - if data != cluster.config.data: + if RestApiHandler._patch_config(data, request): self.server.patroni.ha.dcs.set_config_value(json.dumps(data, separators=(',', ':')), cluster.config.index) self._write_json_response(200, data) else: diff --git a/tests/test_api.py b/tests/test_api.py index 44dfead1..fe569836 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1,3 +1,4 @@ +import json import psycopg2 import unittest @@ -123,11 +124,14 @@ class TestRestApiHandler(unittest.TestCase): @patch.object(MockHa, 'dcs') def test_do_PATCH_config(self, mock_dcs): - mock_dcs.get_cluster.return_value.config = \ - ClusterConfig.from_node(1, '{"postgresql": {"use_slots": false, "parameters": {"wal_level": "logical"}}}') + config = {'postgresql': {'use_slots': False, 'use_pg_rewind': True, 'parameters': {'wal_level': 'logical'}}} + mock_dcs.get_cluster.return_value.config = ClusterConfig.from_node(1, json.dumps(config)) request = 'PATCH /config HTTP/1.0' + self._authorization + '\nContent-Length: ' self.assertIsNotNone(MockRestApiServer(RestApiHandler, request + '2\n\n{}')) - MockRestApiServer(RestApiHandler, request + '59\n\n{"ttl":5,"use_slots":true,"postgresql":{"parameters":null}}') + config['ttl'] = 5 + config['postgresql'].update({'use_slots': True, "parameters": None}) + config = json.dumps(config) + MockRestApiServer(RestApiHandler, request + str(len(config)) + '\n\n' + config) @patch.object(MockPatroni, 'sighup_handler', Mock(side_effect=Exception)) def test_do_POST_reload(self): From 1c2e1755cb7ecbba50f4355e5ea7b72abb589781 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 31 May 2016 15:51:49 +0200 Subject: [PATCH 20/49] Explicitly cast some parameters from DCS to int (ttl, loop_wait, etc...) --- patroni/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/config.py b/patroni/config.py index 4e03b480..485aa335 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -142,7 +142,7 @@ class Config(object): elif name not in ('connect_address', 'listen', 'data_dir', 'pgpass', 'authentication'): config['postgresql'][name] = deepcopy(value) elif name in config: - config[name] = value + config[name] = int(value) return config def _build_effective_configuration(self, dynamic_configuration, local_configuration): From a55cbff865a6fcc808a629ba80eed3e4942a8204 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 31 May 2016 16:13:32 +0200 Subject: [PATCH 21/49] Compare configuration objects "smart" and "deep" --- patroni/config.py | 7 ++++--- patroni/utils.py | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/patroni/config.py b/patroni/config.py index 485aa335..d4a186b9 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -6,6 +6,7 @@ import yaml from copy import deepcopy from patroni.postgresql import Postgresql +from patroni.utils import deep_compare logger = logging.getLogger(__name__) @@ -94,7 +95,7 @@ class Config(object): logger.error('Can not remove temporary file %s', tmpfile) def set_dynamic_configuration(self, configuration): - if self._dynamic_configuration != configuration: + if not deep_compare(self._dynamic_configuration, configuration): try: self.__effective_configuration = self._build_effective_configuration(configuration, self._local_configuration) @@ -108,10 +109,10 @@ class Config(object): if self.config_file: try: configuration = self._load_config_file() - if self._local_configuration != configuration: + if not deep_compare(self._local_configuration, configuration): new_configuration = self._build_effective_configuration(self._dynamic_configuration, configuration) if dry_run: - return new_configuration != self.__effective_configuration + return not deep_compare(new_configuration, self.__effective_configuration) self._local_configuration = configuration self.__effective_configuration = new_configuration return True diff --git a/patroni/utils.py b/patroni/utils.py index 0969d458..d5e8bc41 100644 --- a/patroni/utils.py +++ b/patroni/utils.py @@ -33,6 +33,32 @@ def calculate_ttl(expiration): return int((expiration - now).total_seconds()) +def deep_compare(obj1, obj2): + """ + >>> deep_compare({'1': None}, {}) + False + >>> deep_compare({'1': {}}, {'1': None}) + False + >>> deep_compare({'1': [1]}, {'1': [2]}) + False + >>> deep_compare({'1': 2}, {'1': '2'}) + True + >>> deep_compare({'1': {'2': [3, 4]}}, {'1': {'2': [3, 4]}}) + True + """ + + if set(list(obj1.keys())) != set(list(obj2.keys())): # Objects have different sets of keys + return False + + for key, value in obj1.items(): + if isinstance(value, dict): + if not (isinstance(obj2[key], dict) and deep_compare(value, obj2[key])): + return False + elif str(value) != str(obj2[key]): + return False + return True + + def set_ignore_sigterm(value=True): global __ignore_sigterm __ignore_sigterm = value From 60f7759c5e3354eab501a15a7748fe172a140636 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 1 Jun 2016 09:21:42 +0200 Subject: [PATCH 22/49] Small optimization Don't compare values of configuration if modify_index didn't changed --- patroni/__init__.py | 4 ++-- patroni/config.py | 9 +++++++++ patroni/dcs/__init__.py | 6 +++--- patroni/dcs/consul.py | 2 +- patroni/dcs/zookeeper.py | 6 +++--- tests/test_patroni.py | 2 ++ 6 files changed, 20 insertions(+), 9 deletions(-) diff --git a/patroni/__init__.py b/patroni/__init__.py index 9fc15f50..d8494c23 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -41,7 +41,7 @@ class Patroni(object): try: cluster = self.dcs.get_cluster() if cluster and cluster.config: - self.config.set_dynamic_configuration(cluster.config.data) + self.config.set_dynamic_configuration(cluster.config) elif not self.config.dynamic_configuration and 'bootstrap' in self.config: self.config.set_dynamic_configuration(self.config['bootstrap']['dcs']) break @@ -101,7 +101,7 @@ class Patroni(object): logger.info(self.ha.run_cycle()) cluster = self.dcs.cluster - if cluster and cluster.config and self.config.set_dynamic_configuration(cluster.config.data): + if cluster and cluster.config and self.config.set_dynamic_configuration(cluster.config): self.reload_config() if not self.postgresql.data_directory_empty(): diff --git a/patroni/config.py b/patroni/config.py index d4a186b9..541905a0 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -5,6 +5,7 @@ import tempfile import yaml from copy import deepcopy +from patroni.dcs import ClusterConfig from patroni.postgresql import Postgresql from patroni.utils import deep_compare @@ -42,6 +43,7 @@ class Config(object): def __init__(self, config_file=None, config_env=None): self._config_file = None if config_env else config_file + self._modify_index = -1 self._dynamic_configuration = {} self._local_configuration = yaml.safe_load(config_env) if config_env else self._load_config_file() self.__effective_configuration = self._build_effective_configuration(self._dynamic_configuration, @@ -94,7 +96,14 @@ class Config(object): except Exception: logger.error('Can not remove temporary file %s', tmpfile) + # configuration could be either ClusterConfig or dict def set_dynamic_configuration(self, configuration): + if isinstance(configuration, ClusterConfig): + if self._modify_index == configuration.modify_index: + return False # If the index didn't changed there is nothing to do + self._modify_index = configuration.modify_index + configuration = configuration.data + if not deep_compare(self._dynamic_configuration, configuration): try: self.__effective_configuration = self._build_effective_configuration(configuration, diff --git a/patroni/dcs/__init__.py b/patroni/dcs/__init__.py index d4da8e6b..c068ef8e 100644 --- a/patroni/dcs/__init__.py +++ b/patroni/dcs/__init__.py @@ -164,10 +164,10 @@ class Failover(namedtuple('Failover', 'index,leader,candidate,scheduled_at')): return Failover(index, data.get('leader'), data.get('member'), data.get('scheduled_at')) -class ClusterConfig(namedtuple('ClusterConfig', 'index,data')): +class ClusterConfig(namedtuple('ClusterConfig', 'index,data,modify_index')): @staticmethod - def from_node(index, data): + def from_node(index, data, modify_index=None): """ >>> ClusterConfig.from_node(1, '{') is None True @@ -177,7 +177,7 @@ class ClusterConfig(namedtuple('ClusterConfig', 'index,data')): data = json.loads(data) except (TypeError, ValueError): return None - return ClusterConfig(index, data) + return ClusterConfig(index, data, modify_index or index) class Cluster(namedtuple('Cluster', 'initialize,config,leader,last_leader_operation,members,failover')): diff --git a/patroni/dcs/consul.py b/patroni/dcs/consul.py index 4c5a4bc1..f7736dfb 100644 --- a/patroni/dcs/consul.py +++ b/patroni/dcs/consul.py @@ -34,7 +34,7 @@ class HTTPClient(std.HTTPClient): defaults_attr_name = '__defaults__' if six.PY3 else 'func_defaults' defaults = list(getattr(request_func, defaults_attr_name)) code = request_func.__code__ if six.PY3 else request_func.func_code - defaults[code.co_varnames[code.co_argcount - len(defaults):code.co_argcount].index('timeout')] = 5 + defaults[code.co_varnames[code.co_argcount - len(defaults):code.co_argcount].index('timeout')] = timeout setattr(request_func, defaults_attr_name, tuple(defaults)) # monkeypatching def get(self, callback, path, params=None): diff --git a/patroni/dcs/zookeeper.py b/patroni/dcs/zookeeper.py index 29367419..618cc9bd 100644 --- a/patroni/dcs/zookeeper.py +++ b/patroni/dcs/zookeeper.py @@ -150,7 +150,7 @@ class ZooKeeper(AbstractDCS): # get global dynamic configuration config = self.get_node(self.config_path, watch=self.cluster_watcher) if self._CONFIG in nodes else None - config = config and ClusterConfig.from_node(config[1].version, config[0]) + config = config and ClusterConfig.from_node(config[1].version, config[0], config[1].mzxid) # get list of members members = self.load_members() if self._MEMBERS[:-1] in nodes else [] @@ -209,7 +209,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)) + return value == '' or (index is None and self._create(self.failover_path, value)) except: logging.exception('set_failover_value') return False @@ -219,7 +219,7 @@ class ZooKeeper(AbstractDCS): self._client.retry(self._client.set, self.config_path, value.encode('utf-8'), version=index or -1) return True except NoNodeError: - return value == '' or (not index and self._create(self.config_path, value)) + return index is None and self._create(self.config_path, value) except Exception: logging.exception('set_config_value') return False diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 71575e82..1e53e27b 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -71,6 +71,8 @@ class TestPatroni(unittest.TestCase): self.p.api.start = Mock() self.p.config._dynamic_configuration = {} self.assertRaises(SleepException, self.p.run) + with patch('patroni.config.Config.set_dynamic_configuration', Mock(return_value=True)): + self.assertRaises(SleepException, self.p.run) with patch('patroni.postgresql.Postgresql.data_directory_empty', Mock(return_value=False)): self.assertRaises(SleepException, self.p.run) From 140917ba37d240279b093aff777d6826bd299f6f Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 1 Jun 2016 09:26:34 +0200 Subject: [PATCH 23/49] Fix a typo --- patroni/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/api.py b/patroni/api.py index 18eb21eb..c8d52b27 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -92,7 +92,7 @@ class RestApiHandler(BaseHTTPRequestHandler): else: status_code = 503 - if write_status_code_only: # when haproxy sends OPTIONS request it reads only statue code and nothing more + if write_status_code_only: # when haproxy sends OPTIONS request it reads only status code and nothing more message = self.responses[status_code][0] self.wfile.write(("%s %d %s\r\n" % (self.protocol_version, status_code, message)).encode('utf-8')) else: From aad2433440af384ef2be4d7cfc90b7ab49da5540 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 1 Jun 2016 10:04:50 +0200 Subject: [PATCH 24/49] Make QuantifiedCode happier --- patroni/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/api.py b/patroni/api.py index c8d52b27..982a3569 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -94,7 +94,7 @@ class RestApiHandler(BaseHTTPRequestHandler): if write_status_code_only: # when haproxy sends OPTIONS request it reads only status code and nothing more message = self.responses[status_code][0] - self.wfile.write(("%s %d %s\r\n" % (self.protocol_version, status_code, message)).encode('utf-8')) + self.wfile.write('{0} {1} {2}\r\n'.format(self.protocol_version, status_code, message).encode('utf-8')) else: self._write_status_response(status_code, response) From c8b5003b86fa024090ea8836f5573bb97a644011 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 1 Jun 2016 13:41:49 +0200 Subject: [PATCH 25/49] Set __do_not_watch flag when ttl needs to be changed it's more readable comparing to `reset_cluster` --- patroni/dcs/consul.py | 31 +++++++++++++------------------ patroni/dcs/etcd.py | 11 ++++++----- patroni/dcs/zookeeper.py | 2 +- tests/test_consul.py | 6 ++++-- tests/test_etcd.py | 1 + 5 files changed, 25 insertions(+), 26 deletions(-) diff --git a/patroni/dcs/consul.py b/patroni/dcs/consul.py index f7736dfb..887787f2 100644 --- a/patroni/dcs/consul.py +++ b/patroni/dcs/consul.py @@ -79,32 +79,22 @@ class Consul(AbstractDCS): self._client = ConsulClient(host=host, port=port) self._client.http.patch_default_timeout(config['retry_timeout']/2.0) self._scope = config['scope'] - self.create_or_restore_session() + self.create_session() + self.__do_not_watch = False - def create_or_restore_session(self): + def create_session(self): while not self._session: try: - _, member = self._client.kv.get(self.member_path) - self._session = (member or {}).get('Session') - if self.refresh_session(): - self._client.kv.delete(self.member_path) - except (ConsulException, RequestException): + self.refresh_session() + except ConsulError: logger.info('waiting on consul') sleep(5) def set_ttl(self, ttl): ttl = ttl/2.0 # My experiments have shown that session expires after 2*ttl time if self._ttl != ttl: - if self._session: - try: - self._client.session.destroy(self._session) - except Exception: - logger.exception("Can not destroy session %s", self._session) self._session = None - # force `watch` method to call `AbstractDCS.watch` instead of watching for leader key - self.reset_cluster() - # fire up an event to wake up from `watch` and immediately run HA loop (to create the new session) - self.event.set() + self.__do_not_watch = True self._ttl = ttl def set_retry_timeout(self, retry_timeout): @@ -187,12 +177,13 @@ class Consul(AbstractDCS): raise ConsulError('Consul is not responding properly') def touch_member(self, data, **kwargs): - create_member = self.refresh_session() cluster = self.cluster member = cluster and ([m for m in cluster.members if m.name == self._name] or [None])[0] - if create_member and member: + create_member = self.refresh_session() + if member and (create_member or member.session != self._session): try: self._client.kv.delete(self.member_path) + create_member = True except Exception: return False @@ -253,6 +244,10 @@ class Consul(AbstractDCS): return self._client.kv.delete(self.leader_path, cas=cluster.leader.index) def watch(self, timeout): + if self.__do_not_watch: + self.__do_not_watch = False + return True + cluster = self.cluster if cluster and cluster.leader and cluster.leader.name != self._name and cluster.leader.index: end_time = time.time() + timeout diff --git a/patroni/dcs/etcd.py b/patroni/dcs/etcd.py index 467e1ddd..7b2b7477 100644 --- a/patroni/dcs/etcd.py +++ b/patroni/dcs/etcd.py @@ -200,6 +200,7 @@ class Etcd(AbstractDCS): etcd.EtcdWatcherCleared, etcd.EtcdEventIndexCleared)) self._client = self.get_etcd_client(config) + self.__do_not_watch = False def retry(self, *args, **kwargs): return self._retry.copy()(*args, **kwargs) @@ -217,11 +218,7 @@ class Etcd(AbstractDCS): def set_ttl(self, ttl): ttl = int(ttl) - if self._ttl != ttl: - # force `watch` method to call `AbstractDCS.watch` instead of watching for leader key - self.reset_cluster() - # fire up an event to wake up from `watch` and immediately run HA loop (to update TTL of leader and member) - self.event.set() + self.__do_not_watch = self._ttl != ttl self._ttl = ttl def set_retry_timeout(self, retry_timeout): @@ -320,6 +317,10 @@ class Etcd(AbstractDCS): return self.retry(self._client.delete, self.client_path(''), recursive=True) def watch(self, timeout): + if self.__do_not_watch: + self.__do_not_watch = False + return True + cluster = self.cluster # watch on leader key changes if it is defined and current node is not lock owner if cluster and cluster.leader and cluster.leader.name != self._name and cluster.leader.index: diff --git a/patroni/dcs/zookeeper.py b/patroni/dcs/zookeeper.py index 618cc9bd..e077f726 100644 --- a/patroni/dcs/zookeeper.py +++ b/patroni/dcs/zookeeper.py @@ -188,7 +188,7 @@ class ZooKeeper(AbstractDCS): self._client.retry(self._inner_load_cluster) except: logger.exception('get_cluster') - self.session_listener(KazooState.LOST) + self.cluster_watcher(None) raise ZooKeeperError('ZooKeeper in not responding properly') def _create(self, path, value, **kwargs): diff --git a/tests/test_consul.py b/tests/test_consul.py index baf28719..32e27462 100644 --- a/tests/test_consul.py +++ b/tests/test_consul.py @@ -56,9 +56,10 @@ class TestConsul(unittest.TestCase): self.c._load_cluster() @patch('time.sleep', Mock(side_effect=SleepException)) - def test_create_or_restore_session(self): + @patch.object(consul.Consul.Session, 'create', Mock(side_effect=ConsulException)) + def test_create_session(self): self.c._session = None - self.assertRaises(SleepException, self.c.create_or_restore_session) + self.assertRaises(SleepException, self.c.create_session) @patch.object(consul.Consul.Session, 'renew', Mock(side_effect=NotFound)) @patch.object(consul.Consul.Session, 'create', Mock(side_effect=ConsulException)) @@ -133,6 +134,7 @@ class TestConsul(unittest.TestCase): @patch.object(consul.Consul.Session, 'destroy', Mock(side_effect=ConsulException)) def test_set_ttl(self): self.c.set_ttl(20) + self.assertTrue(self.c.watch(1)) def test_set_retry_timeout(self): self.c.set_retry_timeout(10) diff --git a/tests/test_etcd.py b/tests/test_etcd.py index d675db43..ebdf6aa2 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -260,3 +260,4 @@ class TestEtcd(unittest.TestCase): def test_set_ttl(self): self.etcd.set_ttl(20) + self.assertTrue(self.etcd.watch(1)) From 1c30948ef9cb126b27c166c4309ef20fc9206982 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 1 Jun 2016 17:06:31 +0200 Subject: [PATCH 26/49] Implement PUT /config and enhance some checks --- features/patroni_api.feature | 2 ++ patroni/api.py | 61 ++++++++++++++++++++++++++---------- tests/test_api.py | 54 ++++++++++++++++++++----------- 3 files changed, 83 insertions(+), 34 deletions(-) diff --git a/features/patroni_api.feature b/features/patroni_api.feature index 0702f308..04d31765 100644 --- a/features/patroni_api.feature +++ b/features/patroni_api.feature @@ -18,6 +18,8 @@ Scenario: check API requests on a stand-alone server And I receive a response text failover is not possible: cluster does not have members except leader When I issue an empty POST request to http://127.0.0.1:8008/failover Then I receive a response code 400 + When I issue a POST request to http://127.0.0.1:8008/failover with {"foo": "bar"} + Then I receive a response code 400 And I receive a response text "No values given for required parameters leader and candidate" Scenario: check local configuration reload diff --git a/patroni/api.py b/patroni/api.py index 982a3569..73a74d1c 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -9,7 +9,7 @@ import datetime import pytz from patroni.exceptions import PostgresConnectionException -from patroni.utils import Retry, RetryFailedError +from patroni.utils import deep_compare, Retry, RetryFailedError from six.moves.BaseHTTPServer import BaseHTTPRequestHandler, HTTPServer from six.moves.socketserver import ThreadingMixIn from threading import Thread @@ -127,17 +127,46 @@ class RestApiHandler(BaseHTTPRequestHandler): is_changed = True return is_changed + def _read_json_content(self): + if 'content-length' not in self.headers: + return self.send_error(411) + try: + content_length = int(self.headers.get('content-length')) + request = json.loads(self.rfile.read(content_length).decode('utf-8')) + if isinstance(request, dict) and request: + return request + except Exception: + logger.exception('Bad request') + self.send_error(400) + @check_auth def do_PATCH_config(self): - content_length = int(self.headers.get('content-length', 0)) - request = json.loads(self.rfile.read(content_length).decode('utf-8')) - cluster = self.server.patroni.ha.dcs.get_cluster() - data = cluster.config.data.copy() - if RestApiHandler._patch_config(data, request): - self.server.patroni.ha.dcs.set_config_value(json.dumps(data, separators=(',', ':')), cluster.config.index) - self._write_json_response(200, data) - else: - self._write_response(304, '', '') + request = self._read_json_content() + if request: + cluster = self.server.patroni.ha.dcs.get_cluster() + data = cluster.config.data.copy() + if RestApiHandler._patch_config(data, request): + value = json.dumps(data, separators=(',', ':')) + if self.server.patroni.ha.dcs.set_config_value(value, cluster.config.index): + self._write_json_response(200, data) + else: + self.send_error(409) + else: + self.send_error(304) + + @check_auth + def do_PUT_config(self): + request = self._read_json_content() + if request: + cluster = self.server.patroni.ha.dcs.get_cluster() + if deep_compare(request, cluster.config.data): + self.send_error(304) + else: + value = json.dumps(request, separators=(',', ':')) + if self.server.patroni.ha.dcs.set_config_value(value): + self._write_json_response(200, request) + else: + self.send_error(502) @check_auth def do_POST_reload(self): @@ -184,7 +213,8 @@ class RestApiHandler(BaseHTTPRequestHandler): self._write_response(status_code, data) def poll_failover_result(self, leader, candidate): - for _ in range(0, 15): + timeout = 10 if self.server.patroni.nap_time < 10 else self.server.patroni.nap_time + for _ in range(0, timeout*2): time.sleep(1) try: cluster = self.server.patroni.dcs.get_cluster() @@ -217,11 +247,10 @@ class RestApiHandler(BaseHTTPRequestHandler): @check_auth def do_POST_failover(self): - content_length = int(self.headers.get('content-length', 0)) - try: - request = json.loads(self.rfile.read(content_length).decode('utf-8')) - except ValueError: - request = {} + request = self._read_json_content() + if not request: + return + leader = request.get('leader') candidate = request.get('candidate') or request.get('member') scheduled_at = request.get('scheduled_at') diff --git a/tests/test_api.py b/tests/test_api.py index fe569836..befa120e 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -49,6 +49,7 @@ class MockHa(object): class MockPatroni(object): + nap_time = 10 config = Mock() postgresql = MockPostgresql() ha = MockHa() @@ -126,12 +127,30 @@ class TestRestApiHandler(unittest.TestCase): def test_do_PATCH_config(self, mock_dcs): config = {'postgresql': {'use_slots': False, 'use_pg_rewind': True, 'parameters': {'wal_level': 'logical'}}} mock_dcs.get_cluster.return_value.config = ClusterConfig.from_node(1, json.dumps(config)) - request = 'PATCH /config HTTP/1.0' + self._authorization + '\nContent-Length: ' - self.assertIsNotNone(MockRestApiServer(RestApiHandler, request + '2\n\n{}')) + request = 'PATCH /config HTTP/1.0' + self._authorization + self.assertIsNotNone(MockRestApiServer(RestApiHandler, request)) + request += '\nContent-Length: ' + self.assertIsNotNone(MockRestApiServer(RestApiHandler, request + '34\n\n{"postgresql":{"use_slots":false}}')) config['ttl'] = 5 config['postgresql'].update({'use_slots': True, "parameters": None}) config = json.dumps(config) - MockRestApiServer(RestApiHandler, request + str(len(config)) + '\n\n' + config) + request += str(len(config)) + '\n\n' + config + MockRestApiServer(RestApiHandler, request) + mock_dcs.set_config_value.return_value = False + MockRestApiServer(RestApiHandler, request) + + @patch.object(MockHa, 'dcs') + def test_do_PUT_config(self, mock_dcs): + mock_dcs.get_cluster.return_value.config = ClusterConfig.from_node(1, '{}') + request = 'PUT /config HTTP/1.0' + self._authorization + '\nContent-Length: ' + self.assertIsNotNone(MockRestApiServer(RestApiHandler, request + '2\n\n{}')) + config = '{"foo": "bar"}' + request += str(len(config)) + '\n\n' + config + MockRestApiServer(RestApiHandler, request) + mock_dcs.set_config_value.return_value = False + MockRestApiServer(RestApiHandler, request) + mock_dcs.get_cluster.return_value.config = ClusterConfig.from_node(1, config) + MockRestApiServer(RestApiHandler, request) @patch.object(MockPatroni, 'sighup_handler', Mock(side_effect=Exception)) def test_do_POST_reload(self): @@ -170,18 +189,20 @@ class TestRestApiHandler(unittest.TestCase): def test_do_POST_failover(self, dcs): cluster = dcs.get_cluster.return_value - request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 0\n\n' + post = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: ' + + MockRestApiServer(RestApiHandler, post + '7\n\n{"1":2}') + + request = post + '0\n\n' MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'postgresql1' MockRestApiServer(RestApiHandler, request) - request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 25\n\n{"leader": "postgresql1"}' - MockRestApiServer(RestApiHandler, request) + MockRestApiServer(RestApiHandler, post + '25\n\n{"leader": "postgresql1"}') cluster.leader.name = 'postgresql2' - request = 'POST /failover HTTP/1.0' + self._authorization +\ - '\nContent-Length: 53\n\n{"leader": "postgresql1", "candidate": "postgresql2"}' + request = post + '53\n\n{"leader": "postgresql1", "candidate": "postgresql2"}' MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'postgresql1' @@ -207,24 +228,21 @@ class TestRestApiHandler(unittest.TestCase): MockRestApiServer(RestApiHandler, request) # Valid future date - request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 103\n\n{"leader": ' +\ - '"postgresql1", "member": "postgresql2", "scheduled_at": "6016-02-15T18:13:30.568224+01:00"}' + request = post + '103\n\n{"leader": "postgresql1", "member": "postgresql2",' +\ + ' "scheduled_at": "6016-02-15T18:13:30.568224+01:00"}' MockRestApiServer(RestApiHandler, request) with patch.object(MockPatroni, 'dcs') as d: d.manual_failover.return_value = False MockRestApiServer(RestApiHandler, request) # Exception: No timezone specified - request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 97\n\n{"leader": ' +\ - '"postgresql1", "member": "postgresql2", "scheduled_at": "6016-02-15T18:13:30.568224"}' + request = post + '97\n\n{"leader": "postgresql1", "member": "postgresql2",' +\ + ' "scheduled_at": "6016-02-15T18:13:30.568224"}' MockRestApiServer(RestApiHandler, request) # Exception: Scheduled in the past - request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 103\n\n{"leader": ' +\ - '"postgresql1", "member": "postgresql2", "scheduled_at": "1016-02-15T18:13:30.568224+01:00"}' - MockRestApiServer(RestApiHandler, request) + request = post + '103\n\n{"leader": "postgresql1", "member": "postgresql2", "scheduled_at": "' + MockRestApiServer(RestApiHandler, request + '1016-02-15T18:13:30.568224+01:00"}') # Invalid date - request = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: 103\n\n{"leader": ' +\ - '"postgresql1", "member": "postgresql2", "scheduled_at": "2010-02-29T18:13:30.568224+01:00"}' - self.assertIsNotNone(MockRestApiServer(RestApiHandler, request)) + self.assertIsNotNone(MockRestApiServer(RestApiHandler, request + '2010-02-29T18:13:30.568224+01:00"}')) From 2d78ef092220b2e700faaefb973442a0074af9dd Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 2 Jun 2016 09:28:11 +0200 Subject: [PATCH 27/49] CREATE/ALTER USER=>ROLE --- patroni/postgresql.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 452e2e6d..39a3bc1a 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -734,9 +734,9 @@ BEGIN SET local synchronous_commit = 'local'; PERFORM * FROM pg_authid WHERE rolname = %s; IF FOUND THEN - ALTER USER "{0}" WITH {1} PASSWORD %s; + ALTER ROLE "{0}" WITH {1} PASSWORD %s; ELSE - CREATE USER "{0}" WITH {1} PASSWORD %s; + CREATE ROLE "{0}" WITH {1} PASSWORD %s; END IF; END; $$""".format(name, ' '.join(options)), name, password, password) From ebb9e252d8bb73616d7cdabe8ae87337485c70cc Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 2 Jun 2016 09:31:30 +0200 Subject: [PATCH 28/49] Rename restart_pending to pending_restart for compatibility --- features/patroni_api.feature | 2 +- patroni/api.py | 4 ++-- patroni/ha.py | 4 ++-- patroni/postgresql.py | 10 +++++----- tests/test_api.py | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/features/patroni_api.feature b/features/patroni_api.feature index 04d31765..22e5d8b9 100644 --- a/features/patroni_api.feature +++ b/features/patroni_api.feature @@ -33,7 +33,7 @@ Scenario: check dynamic configuration change via DCS Given I issue a PATCH request to http://127.0.0.1:8008/config with {"ttl": 20, "loop_wait": 1, "postgresql": {"parameters": {"max_connections": 101}}} Then I receive a response code 200 And I receive a response loop_wait 1 - And Response on GET http://127.0.0.1:8008/patroni contains restart_pending after 11 seconds + And Response on GET http://127.0.0.1:8008/patroni contains pending_restart after 11 seconds When I issue a GET request to http://127.0.0.1:8008/config Then I receive a response code 200 And I receive a response loop_wait 1 diff --git a/patroni/api.py b/patroni/api.py index 73a74d1c..6dc038a5 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -60,8 +60,8 @@ class RestApiHandler(BaseHTTPRequestHandler): response.update({'tags': patroni.tags} if patroni.tags else {}) if patroni.postgresql.sysid: response['database_system_identifier'] = patroni.postgresql.sysid - if patroni.postgresql.restart_pending: - response['restart_pending'] = True + if patroni.postgresql.pending_restart: + response['pending_restart'] = True response['patroni'] = {'version': patroni.version, 'scope': patroni.postgresql.scope} self._write_json_response(status_code, response) diff --git a/patroni/ha.py b/patroni/ha.py index fc9cd847..0352b270 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -59,8 +59,8 @@ class Ha(object): } if self.patroni.tags: data['tags'] = self.patroni.tags - if self.state_handler.restart_pending: - data['restart_pending'] = True + if self.state_handler.pending_restart: + data['pending_restart'] = True if not self._async_executor.busy and data['state'] in ['running', 'restarting', 'starting']: try: data['xlog_location'] = self.state_handler.xlog_position() diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 39a3bc1a..b61ae77b 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -64,7 +64,7 @@ class Postgresql(object): self.name = config['name'] self.scope = config['scope'] self._data_dir = config['data_dir'] - self._restart_pending = False + self._pending_restart = False self._server_parameters = self.get_server_parameters(config) self._connect_address = config.get('connect_address') @@ -148,7 +148,7 @@ class Postgresql(object): if server_parameters[r[0]] is None or str(server_parameters[r[0]]) != str(r[1]): reload_pending = True if r[2] in ('internal', 'postmaster'): - self._restart_pending = True + self._pending_restart = True if r[0] in ('listen_addresses', 'port'): listen_address_changed = True self.config = config @@ -164,8 +164,8 @@ class Postgresql(object): self.retry.deadline = config['retry_timeout']/2.0 @property - def restart_pending(self): - return self._restart_pending + def pending_restart(self): + return self._pending_restart @property def can_rewind(self): @@ -454,7 +454,7 @@ class Postgresql(object): if not (self._major_version < 9.4 and p in ('max_replication_slots', 'wal_log_hints'))) ret = subprocess.call(self._pg_ctl + ['start', '-o', options], env=env, preexec_fn=os.setsid) == 0 - self._restart_pending = False + self._pending_restart = False self.set_state('running' if ret else 'start failed') if ret: diff --git a/tests/test_api.py b/tests/test_api.py index befa120e..079fa7f7 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -18,7 +18,7 @@ class MockPostgresql(object): server_version = '999999' sysid = 'dummysysid' scope = 'dummy' - restart_pending = True + pending_restart = True @staticmethod def connection(): From 2e5ce4a30351368eff785f05a312cb4c76462e41 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 2 Jun 2016 16:34:34 +0200 Subject: [PATCH 29/49] "Smart" compare of postgres parameters to decide do we need to reload/restart --- patroni/postgresql.py | 20 +++++---- patroni/utils.py | 90 ++++++++++++++++++++++++++++++++++++++++ tests/test_postgresql.py | 12 ++++-- 3 files changed, 110 insertions(+), 12 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index b61ae77b..d13b33f8 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -8,7 +8,7 @@ import tempfile import time from patroni.exceptions import PostgresConnectionException, PostgresException -from patroni.utils import Retry, RetryFailedError +from patroni.utils import compare_values, Retry, RetryFailedError from six import string_types from six.moves.urllib_parse import urlparse from threading import Lock @@ -51,6 +51,7 @@ class Postgresql(object): CMDLINE_OPTIONS = { 'listen_addresses': None, 'port': None, + 'config_file': None, 'wal_level': 'hot_standby', 'hot_standby': 'on', 'max_wal_senders': 5, @@ -136,21 +137,23 @@ class Postgresql(object): def reload_config(self, config): server_parameters = self.get_server_parameters(config) - listen_address_changed = reload_pending = False + listen_address_changed = pending_reload = False if self.is_healthy(): changes = server_parameters.copy() changes.update({p: None for p, v in self._server_parameters.items() if p not in server_parameters}) if changes: - for r in self.query("""SELECT name, setting, context + for r in self.query("""SELECT name, setting, unit, vartype, context FROM pg_settings WHERE name IN (""" + ', '.join('%s' for _ in changes.keys()) + ')', *(list(changes.keys()))): - if server_parameters[r[0]] is None or str(server_parameters[r[0]]) != str(r[1]): - reload_pending = True - if r[2] in ('internal', 'postmaster'): + unit = '16384kB' if r[0] in ('min_wal_size', 'max_wal_size') else r[2] + if server_parameters[r[0]] is None or not compare_values(r[3], unit, r[1], server_parameters[r[0]]): + if r[4] == 'postmaster': self._pending_restart = True if r[0] in ('listen_addresses', 'port'): listen_address_changed = True + elif r[4] != 'internal': + pending_reload = True self.config = config self._server_parameters = server_parameters self._connect_address = config.get('connect_address') @@ -158,7 +161,7 @@ class Postgresql(object): if not listen_address_changed: self.resolve_connection_addresses() - if reload_pending: + if pending_reload: self._write_postgresql_conf() self.reload() self.retry.deadline = config['retry_timeout']/2.0 @@ -451,7 +454,8 @@ class Postgresql(object): self.resolve_connection_addresses() options = ' '.join("--{0}='{1}'".format(p, self._server_parameters[p]) for p in self.CMDLINE_OPTIONS - if not (self._major_version < 9.4 and p in ('max_replication_slots', 'wal_log_hints'))) + if self._server_parameters[p] is not None and + not (self._major_version < 9.4 and p in ('max_replication_slots', 'wal_log_hints'))) ret = subprocess.call(self._pg_ctl + ['start', '-o', options], env=env, preexec_fn=os.setsid) == 0 self._pending_restart = False diff --git a/patroni/utils.py b/patroni/utils.py index d5e8bc41..f85ec56f 100644 --- a/patroni/utils.py +++ b/patroni/utils.py @@ -2,6 +2,7 @@ import datetime import os import random import signal +import six import sys import time import pytz @@ -59,6 +60,95 @@ def deep_compare(obj1, obj2): return True +def parse_bool(value): + """ + >>> parse_bool(1) + True + >>> parse_bool('off') + False + >>> parse_bool('foo') + """ + value = str(value).lower() + if value in ('on', 'true', 'yes', '1'): + return True + if value in ('off', 'false', 'no', '0'): + return False + + +def split_int_unit(value, strict=True): + value = str(value) + l = len(value) - 1 + while l >= 0 and not value[l].isdigit(): + l -= 1 + unit = value[l + 1:].strip() + try: + value = int(value[:l + 1], 0) if six.PY3 else long(value[:l + 1], 0) + except ValueError: + value = None if strict else 1 + return (value, unit) + + +def parse_int(value, base_unit=None): + """ + >>> parse_int('1') == 1 + True + >>> parse_int(' 0x400 MB ', '16384kB') == 64 + True + >>> parse_int('1MB', 'kB') == 1024 + True + >>> parse_int('1000 ms', 's') == 1 + True + >>> parse_int('1GB', 'MB') is None + True + """ + + convert = { + 'kB': {'kB': 1, 'MB': 1024, 'GB': 1024 * 1024, 'TB': 1024 * 1024 * 1024}, + 'ms': {'ms': 1, 's': 1000, 'min': 1000 * 60, 'h': 1000 * 60 * 60, 'd': 1000 * 60 * 60 * 24}, + 's': {'ms': -1000, 's': 1, 'min': 60, 'h': 60 * 60, 'd': 60 * 60 * 24}, + 'min': {'ms': -1000 * 60, 's': -60, 'min': 1, 'h': 60, 'd': 60 * 24} + } + + value, unit = split_int_unit(value) + if value is not None: + if not unit: + return value + + if base_unit and base_unit not in convert: + base_value, base_unit = split_int_unit(base_unit, False) + else: + base_value = 1 + if base_unit in convert and unit in convert[base_unit]: + multiplier = convert[base_unit][unit] + if multiplier < 0: + value /= -multiplier + else: + value *= multiplier + return int(value/base_value) + + +def compare_values(vartype, unit, old_value, new_value): + """ + >>> compare_values('enum', None, 'remote_write', 'REMOTE_WRITE') + True + >>> compare_values('real', None, '1.23', 1.23) + True + """ + + # if the integer or bool new_value is not correct this function will return False + if vartype == 'bool': + old_value = parse_bool(old_value) + new_value = parse_bool(new_value) + elif vartype == 'integer': + old_value = parse_int(old_value) + new_value = parse_int(new_value, unit) + elif vartype == 'enum': + return str(old_value).lower() == str(new_value).lower() + else: # ('string', 'real') + return str(old_value) == str(new_value) + return old_value is not None and new_value is not None and old_value == new_value + + def set_ignore_sigterm(value=True): global __ignore_sigterm __ignore_sigterm = value diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 31ec0c60..3107ed9b 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -35,7 +35,8 @@ class MockCursor(object): elif sql.startswith('SELECT to_char(pg_postmaster_start_time'): self.results = [('', True, '', '', '', '', False)] elif sql.startswith('SELECT name, setting'): - self.results = [('port', '5433', 'postmaster')] + self.results = [('port', '5433', None, 'integer', 'postmaster'), + ('autovacuum', 'on', None, 'bool', 'sighup')] else: self.results = [(None, None, None, None, None, None, None, None, None, None)] @@ -149,7 +150,7 @@ def fake_listdir(path): @patch('subprocess.call', Mock(return_value=0)) @patch('psycopg2.connect', psycopg2_connect) class TestPostgresql(unittest.TestCase): - _PARAMETERS = {'wal_level': 'hot_standby', 'max_replication_slots': 5, 'foo': 'bar', + _PARAMETERS = {'wal_level': 'hot_standby', 'max_replication_slots': 5, 'foo': 'bar', 'config_file': None, 'hot_standby': 'on', 'max_wal_senders': 5, 'wal_keep_segments': 8, 'wal_log_hints': 'on'} @patch('subprocess.call', Mock(return_value=0)) @@ -475,8 +476,11 @@ class TestPostgresql(unittest.TestCase): self.assertFalse(self.p.replica_method_can_work_without_replication_connection('foo')) def test_reload_config(self): - self.p.reload_config({'retry_timeout': 10, 'listen': '*', 'parameters': self._PARAMETERS}) - self.p.reload_config({'retry_timeout': 10, 'listen': '*:5433', 'parameters': self._PARAMETERS}) + parameters = self._PARAMETERS.copy() + parameters['autovacuum'] = 'on' + self.p.reload_config({'retry_timeout': 10, 'listen': '*', 'parameters': parameters}) + parameters['autovacuum'] = 'off' + self.p.reload_config({'retry_timeout': 10, 'listen': '*:5433', 'parameters': parameters}) @patch.object(builtins, 'open', mock_open(read_data='9.4')) def test_get_major_version(self): From d536b4b62ac291f07ffdd84e21437ce46d1a7978 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 2 Jun 2016 16:45:17 +0200 Subject: [PATCH 30/49] Rollback changes regarding config_file It could be set only on the postgres command line anyway. --- patroni/postgresql.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index d13b33f8..dd0409e8 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -51,7 +51,6 @@ class Postgresql(object): CMDLINE_OPTIONS = { 'listen_addresses': None, 'port': None, - 'config_file': None, 'wal_level': 'hot_standby', 'hot_standby': 'on', 'max_wal_senders': 5, @@ -454,8 +453,7 @@ class Postgresql(object): self.resolve_connection_addresses() options = ' '.join("--{0}='{1}'".format(p, self._server_parameters[p]) for p in self.CMDLINE_OPTIONS - if self._server_parameters[p] is not None and - not (self._major_version < 9.4 and p in ('max_replication_slots', 'wal_log_hints'))) + if not (self._major_version < 9.4 and p in ('max_replication_slots', 'wal_log_hints'))) ret = subprocess.call(self._pg_ctl + ['start', '-o', options], env=env, preexec_fn=os.setsid) == 0 self._pending_restart = False From 16771f37d57f8ca4542911fc44464ce6a3d89193 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 3 Jun 2016 08:17:09 +0200 Subject: [PATCH 31/49] Compare old and new user-defined-parameters to avoid reload when parameters didn't changed. Plus get wal_segment_size from pg_settings instead of hardcoding it's value. --- patroni/postgresql.py | 52 ++++++++++++++++++++++++++++++---------- tests/test_postgresql.py | 14 ++++++++--- 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index dd0409e8..7d06de11 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -8,7 +8,7 @@ import tempfile import time from patroni.exceptions import PostgresConnectionException, PostgresException -from patroni.utils import compare_values, Retry, RetryFailedError +from patroni.utils import compare_values, parse_int, Retry, RetryFailedError from six import string_types from six.moves.urllib_parse import urlparse from threading import Lock @@ -138,21 +138,49 @@ class Postgresql(object): listen_address_changed = pending_reload = False if self.is_healthy(): - changes = server_parameters.copy() - changes.update({p: None for p, v in self._server_parameters.items() if p not in server_parameters}) + changes = {p: v for p, v in server_parameters.items() if '.' not in p} + changes.update({p: None for p, v in self._server_parameters.items() if not ('.' in p or p in changes)}) if changes: + if 'wal_segment_size' not in changes: + changes['wal_segment_size'] = '16384kB' + # XXX: query can raise an exception for r in self.query("""SELECT name, setting, unit, vartype, context FROM pg_settings - WHERE name IN (""" + ', '.join('%s' for _ in changes.keys()) + ')', - *(list(changes.keys()))): - unit = '16384kB' if r[0] in ('min_wal_size', 'max_wal_size') else r[2] - if server_parameters[r[0]] is None or not compare_values(r[3], unit, r[1], server_parameters[r[0]]): - if r[4] == 'postmaster': - self._pending_restart = True - if r[0] in ('listen_addresses', 'port'): - listen_address_changed = True - elif r[4] != 'internal': + WHERE name IN (""" + ', '.join(['%s'] * len(changes)) + """) + ORDER BY 1 DESC""", *(list(changes.keys()))): + if r[4] == 'internal': + if r[0] == 'wal_segment_size': + server_parameters.pop(r[0], None) + wal_segment_size = parse_int(r[2], 'kB') + if wal_segment_size is not None: + changes['wal_segment_size'] = '{0}kB'.format(int(r[1]) * wal_segment_size) + elif r[0] in changes: + unit = changes['wal_segment_size'] if r[0] in ('min_wal_size', 'max_wal_size') else r[2] + new_value = changes.pop(r[0]) + if new_value is None or not compare_values(r[3], unit, r[1], new_value): + if r[4] == 'postmaster': + self._pending_restart = True + if r[0] in ('listen_addresses', 'port'): + listen_address_changed = True + else: + pending_reload = True + for param, value in changes.items(): + if param in server_parameters: + logger.warning('Removing invalid parameter `%s` from postgresql.parameters', param) + server_parameters.pop(param) + + # Check that user-defined-paramters have changed (parameters with period in name) + if not pending_reload: + for p, v in server_parameters.items(): + if '.' in p and (p not in self._server_parameters or str(v) != str(self._server_parameters[p])): + pending_reload = True + break + if not pending_reload: + for p, v in self._server_parameters.items(): + if '.' in p and (p not in server_parameters or str(v) != str(server_parameters[p])): pending_reload = True + break + self.config = config self._server_parameters = server_parameters self._connect_address = config.get('connect_address') diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 3107ed9b..92d2a3e8 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -35,7 +35,10 @@ class MockCursor(object): elif sql.startswith('SELECT to_char(pg_postmaster_start_time'): self.results = [('', True, '', '', '', '', False)] elif sql.startswith('SELECT name, setting'): - self.results = [('port', '5433', None, 'integer', 'postmaster'), + self.results = [('wal_segment_size', '2048', '8kB', 'integer', 'internal'), + ('search_path', 'public', None, 'string', 'user'), + ('port', '5433', None, 'integer', 'postmaster'), + ('listen_addresses', '*', None, 'string', 'postmaster'), ('autovacuum', 'on', None, 'bool', 'sighup')] else: self.results = [(None, None, None, None, None, None, None, None, None, None)] @@ -150,7 +153,7 @@ def fake_listdir(path): @patch('subprocess.call', Mock(return_value=0)) @patch('psycopg2.connect', psycopg2_connect) class TestPostgresql(unittest.TestCase): - _PARAMETERS = {'wal_level': 'hot_standby', 'max_replication_slots': 5, 'foo': 'bar', 'config_file': None, + _PARAMETERS = {'wal_level': 'hot_standby', 'max_replication_slots': 5, 'f.oo': 'bar', 'search_path': 'public', 'hot_standby': 'on', 'max_wal_senders': 5, 'wal_keep_segments': 8, 'wal_log_hints': 'on'} @patch('subprocess.call', Mock(return_value=0)) @@ -203,7 +206,7 @@ class TestPostgresql(unittest.TestCase): self.assertTrue(self.p.start()) with open(pg_conf) as f: lines = f.readlines() - self.assertTrue("foo = 'bar'\n" in lines) + self.assertTrue("f.oo = 'bar'\n" in lines) def test_stop(self): self.assertTrue(self.p.stop()) @@ -477,9 +480,14 @@ class TestPostgresql(unittest.TestCase): def test_reload_config(self): parameters = self._PARAMETERS.copy() + parameters.pop('f.oo') + self.p.reload_config({'retry_timeout': 10, 'listen': '*', 'parameters': parameters}) + parameters['b.ar'] = 'bar' + self.p.reload_config({'retry_timeout': 10, 'listen': '*', 'parameters': parameters}) parameters['autovacuum'] = 'on' self.p.reload_config({'retry_timeout': 10, 'listen': '*', 'parameters': parameters}) parameters['autovacuum'] = 'off' + parameters.pop('search_path') self.p.reload_config({'retry_timeout': 10, 'listen': '*:5433', 'parameters': parameters}) @patch.object(builtins, 'open', mock_open(read_data='9.4')) From 40529d718c17318d8793a742821b5ff3aeebb2f6 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 3 Jun 2016 12:28:31 +0200 Subject: [PATCH 32/49] Get rid from unused variable --- patroni/postgresql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 7d06de11..7dfd02cb 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -164,7 +164,7 @@ class Postgresql(object): listen_address_changed = True else: pending_reload = True - for param, value in changes.items(): + for param in changes: if param in server_parameters: logger.warning('Removing invalid parameter `%s` from postgresql.parameters', param) server_parameters.pop(param) From 24822bd9ac7d6814fb51fd197ae697cdf34081a7 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 6 Jun 2016 10:50:42 +0200 Subject: [PATCH 33/49] Returning 304 for POST, PATCH, PUT is not good idea --- features/patroni_api.feature | 3 ++- patroni/api.py | 32 ++++++++++++++------------------ 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/features/patroni_api.feature b/features/patroni_api.feature index 22e5d8b9..6bbdbeb1 100644 --- a/features/patroni_api.feature +++ b/features/patroni_api.feature @@ -24,7 +24,8 @@ Scenario: check API requests on a stand-alone server Scenario: check local configuration reload Given I issue an empty POST request to http://127.0.0.1:8008/reload - Then I receive a response code 304 + Then I receive a response code 200 + And I receive a response text nothing changed When I add tag new_tag new_value to postgres0 config And I issue an empty POST request to http://127.0.0.1:8008/reload Then I receive a response code 202 diff --git a/patroni/api.py b/patroni/api.py index 6dc038a5..c185c755 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -147,35 +147,31 @@ class RestApiHandler(BaseHTTPRequestHandler): data = cluster.config.data.copy() if RestApiHandler._patch_config(data, request): value = json.dumps(data, separators=(',', ':')) - if self.server.patroni.ha.dcs.set_config_value(value, cluster.config.index): - self._write_json_response(200, data) - else: - self.send_error(409) - else: - self.send_error(304) + if not self.server.patroni.ha.dcs.set_config_value(value, cluster.config.index): + return self.send_error(409) + self._write_json_response(200, data) @check_auth def do_PUT_config(self): request = self._read_json_content() if request: cluster = self.server.patroni.ha.dcs.get_cluster() - if deep_compare(request, cluster.config.data): - self.send_error(304) - else: + if not deep_compare(request, cluster.config.data): value = json.dumps(request, separators=(',', ':')) - if self.server.patroni.ha.dcs.set_config_value(value): - self._write_json_response(200, request) - else: - self.send_error(502) + if not self.server.patroni.ha.dcs.set_config_value(value): + return self.send_error(502) + self._write_json_response(200, request) @check_auth def do_POST_reload(self): try: - if not self.server.patroni.config.reload_local_configuration(True): - return self._write_response(304, '', '') - status_code = 202 - response = 'reload scheduled' - self.server.patroni.sighup_handler() + if self.server.patroni.config.reload_local_configuration(True): + status_code = 202 + response = 'reload scheduled' + self.server.patroni.sighup_handler() + else: + status_code = 200 + response = 'nothing changed' except Exception as e: status_code = 500 response = str(e) From b7d87f7d07486335428e4469007525816977f655 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 8 Jun 2016 10:15:24 +0200 Subject: [PATCH 34/49] Implement possibility to configure Patroni via environment --- README.rst | 8 ++- docs/ENVIRONMENT.rst | 52 +++++++++++++++++ SETTINGS.rst => docs/SETTINGS.rst | 0 patroni/__init__.py | 2 +- patroni/api.py | 23 +------- patroni/config.py | 97 +++++++++++++++++++++++++++++-- patroni/utils.py | 21 +++++++ postgres0.yml | 3 + tests/test_config.py | 30 +++++++++- tests/test_ha.py | 2 + tests/test_patroni.py | 1 - 11 files changed, 209 insertions(+), 30 deletions(-) create mode 100644 docs/ENVIRONMENT.rst rename SETTINGS.rst => docs/SETTINGS.rst (100%) diff --git a/README.rst b/README.rst index 922f1e25..edf0357e 100644 --- a/README.rst +++ b/README.rst @@ -75,7 +75,13 @@ run: YAML Configuration =============== -Go `here `__ for comprehensive information about settings for etcd, consul, and ZooKeeper. And for an example, see `postgres0.yml `__. +Go `here `__ for comprehensive information about settings for etcd, consul, and ZooKeeper. And for an example, see `postgres0.yml `__. + +========================= +Environment Configuration +========================= + +Go `here `__ for comprehensive information about configuring(overriding) settings via environment variables. =============== Replication Choices diff --git a/docs/ENVIRONMENT.rst b/docs/ENVIRONMENT.rst new file mode 100644 index 00000000..c8ac4528 --- /dev/null +++ b/docs/ENVIRONMENT.rst @@ -0,0 +1,52 @@ +================================== +Environment Configuration Settings +================================== + +Some of configuration parameters defined in the configuration file is possible to override via Environment variables +This document list all possible environment variables handled by Patroni. +Environment variable always takes precedence on configuration file. + +Global/Universal +---------------- +- **PATRONI\_NAME**: name of the node where the current instance of Patroni is running. Must be unique for the cluster. +- **PATRONI\_NAMESPACE**: path within configuration store where Patroni will keep information about cluster. Default value: "/service" +- **PATRONI\_SCOPE**: cluster name + +Bootstrap configuration +----------------------- +It is possible to define users which will be created right after initializing of a new cluster by defining following environment variables: +- **PATRONI\_\_PASSWORD=''** +- **PATRONI\_\_OPTIONS='list,of,options'** +Example: defining of `PATRONI\_admin\_PASSWORD=admin` `PATRONI\_admin\_OPTIONS='createrole,createdb'` will cause creation of `admin` user which is allowed to create other users and databases + +Consul +------ +- **PATRONI\_CONSUL\_HOST**: the host:port for the Consul endpoint. + +Etcd +---- +- **PATRONI\_ETCD\_HOST**: the host:port for the etcd endpoint. + +PostgreSQL +---------- +- **PATRONI\_POSTGRESQL\_LISTEN**: IP address + port that Postgres listens to. Multiple comma-separated addresses are permitted, as long as the port component is appended after to the last one with a colon, i.e. ``listen: 127.0.0.1,127.0.0.2:5432``. Patroni will use the first address from this list to establish local connections to the PostgreSQL node. +- **PATRONI\_POSTGRESQL\_CONNECT\_ADDRESS**: IP address + port through which Postgres is accessible from other nodes and applications. +- **PATRONI\_POSTGRESQL\_DATA\_DIR**: file path to initialize and store Postgres data files. +- **PATRONI\_POSTGRESQL\_PGPASS**: path to `pgpass` file which would be created by Patroni when it is necessary (for example, before executing pg\_basebackup). This locations must be accessible for writing by Patroni. +- **PATRONI\_REPLICATION\_USERNAME**: replication username; user will be created during initialization. Replicas will use this user to access master via streaming replication +- **PATRONI\_REPLICATION\_PASSWORD**: replication password; user will be created during initialization. +- **PATRONI\_SUPERUSER\_USERNAME**: name for the superuser, set during initialization (initdb) and later used by Patroni to connect to the postgres. Also this user is used by pg_rewind. +- **PATRONI\_SUPERUSER\_PASSWORD**: password for the superuser, set during initialization (initdb). + +REST API +-------- +- **PATRONI\_RESTAPI\_CONNECT\_ADDRESS**: IP address and port through which restapi is accessible. +- **PATRONI\_RESTAPI\_LISTEN**: IP address and port that Patroni will listen to, to provide health-check information for HAProxy. +- **PATRONI\_RESTAPI\_USERNAME**: Basic-auth username to protect dangerous REST API endpoints. +- **PATRONI\_RESTAPI\_PASSWORD**: Basic-auth password to protect dangerous REST API endpoints. +- **PATRONI\_RESTAPI\_CERTFILE**: Specifies a file with the certificate in the PEM format. If the certfile is not specified or is left empty, the API server will work without SSL. +- **PATRONI\_RESTAPI\_KEYFILE**: Specifies a file with the secret key in the PEM format. + +ZooKeeper +--------- +- **PATRONI\_ZOOKEEPER\_HOSTS**: comma separated list of ZooKeeper cluster members: 'host1:port1,host2:port2,etc...' diff --git a/SETTINGS.rst b/docs/SETTINGS.rst similarity index 100% rename from SETTINGS.rst rename to docs/SETTINGS.rst diff --git a/patroni/__init__.py b/patroni/__init__.py index d8494c23..84a1018d 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -120,7 +120,7 @@ def main(): config_env = False config_file = len(sys.argv) >= 2 and os.path.isfile(sys.argv[1]) and sys.argv[1] if not config_file: - config_env = os.environ.get(Patroni.PATRONI_CONFIG_VARIABLE) + config_env = os.environ.pop(Patroni.PATRONI_CONFIG_VARIABLE, None) if config_env is None: print('Usage: {0} config.yml'.format(sys.argv[0])) print('\tPatroni may also read the configuration from the {} environment variable'. diff --git a/patroni/api.py b/patroni/api.py index c185c755..420067ca 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -9,7 +9,7 @@ import datetime import pytz from patroni.exceptions import PostgresConnectionException -from patroni.utils import deep_compare, Retry, RetryFailedError +from patroni.utils import deep_compare, patch_config, Retry, RetryFailedError from six.moves.BaseHTTPServer import BaseHTTPRequestHandler, HTTPServer from six.moves.socketserver import ThreadingMixIn from threading import Thread @@ -108,25 +108,6 @@ class RestApiHandler(BaseHTTPRequestHandler): def do_GET_config(self): self._write_json_response(200, self.server.patroni.config.dynamic_configuration) - @staticmethod - def _patch_config(config, data): - is_changed = False - for name, value in data.items(): - if value is None: - if config.pop(name, None) is not None: - is_changed = True - elif name in config: - if isinstance(value, dict): - if RestApiHandler._patch_config(config[name], value): - is_changed = True - elif str(config[name]) != str(value): - config[name] = value - is_changed = True - else: - config[name] = value - is_changed = True - return is_changed - def _read_json_content(self): if 'content-length' not in self.headers: return self.send_error(411) @@ -145,7 +126,7 @@ class RestApiHandler(BaseHTTPRequestHandler): if request: cluster = self.server.patroni.ha.dcs.get_cluster() data = cluster.config.data.copy() - if RestApiHandler._patch_config(data, request): + if patch_config(data, request): value = json.dumps(data, separators=(',', ':')) if not self.server.patroni.ha.dcs.set_config_value(value, cluster.config.index): return self.send_error(409) diff --git a/patroni/config.py b/patroni/config.py index 541905a0..f8ec2686 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -4,10 +4,11 @@ import os import tempfile import yaml +from collections import defaultdict from copy import deepcopy from patroni.dcs import ClusterConfig from patroni.postgresql import Postgresql -from patroni.utils import deep_compare +from patroni.utils import deep_compare, patch_config logger = logging.getLogger(__name__) @@ -45,7 +46,11 @@ class Config(object): self._config_file = None if config_env else config_file self._modify_index = -1 self._dynamic_configuration = {} - self._local_configuration = yaml.safe_load(config_env) if config_env else self._load_config_file() + if config_env: + self._local_configuration = yaml.safe_load(config_env) + else: + self.__environment_configuration = self._build_environment_configuration() + self._local_configuration = self._load_config_file() self.__effective_configuration = self._build_effective_configuration(self._dynamic_configuration, self._local_configuration) self._data_dir = self.__effective_configuration['postgresql']['data_dir'] @@ -62,8 +67,11 @@ class Config(object): return deepcopy(self._dynamic_configuration) def _load_config_file(self): + """Loads config.yaml from filesystem and applies some values which were set via ENV""" with open(self._config_file) as f: - return yaml.safe_load(f) + config = yaml.safe_load(f) + patch_config(config, self.__environment_configuration) + return config def _load_cache(self): if os.path.isfile(self._cache_file): @@ -151,10 +159,86 @@ class Config(object): config['postgresql'][name].update(self._process_postgresql_parameters(value)) elif name not in ('connect_address', 'listen', 'data_dir', 'pgpass', 'authentication'): config['postgresql'][name] = deepcopy(value) - elif name in config: + elif name in config: # only variables present in __DEFAULT_CONFIG allowed to be overriden from DCS config[name] = int(value) return config + @staticmethod + def _build_environment_configuration(): + ret = defaultdict(dict) + def _popenv(name): + return os.environ.pop('PATRONI_' + name.upper(), None) + + for param in ('name', 'namespace', 'scope'): + value = _popenv(param) + if value: + ret[param] = value + + def _set_section_values(section, params): + for param in params: + value = _popenv(section + '_' + param) + if value: + ret[section][param] = value + + _set_section_values('restapi', ['listen', 'connect_address', 'certfile', 'keyfile']) + _set_section_values('postgresql', ['listen', 'connect_address', 'data_dir', 'pgpass']) + + def _get_auth(name): + ret = {} + for param in ('username', 'password'): + value = _popenv(name + '_' + param) + if value: + ret[param] = value + return len(ret) == 2 and ret or None + + restapi_auth = _get_auth('restapi') + if restapi_auth: + ret['restapi']['authentication'] = restapi_auth + + authentication = {} + for user_type in ('replication', 'superuser'): + entry = _get_auth(user_type) + if entry: + authentication[user_type] = entry + + if authentication: + ret['postgresql']['authentication'] = authentication + + users = {} + + def _parse_list(value): + if not (value.strip().startswith('-') or '[' in value): + value = '[{0}]'.format(value) + try: + return yaml.safe_load(value) + except Exception: + return None + + for param in list(os.environ.keys()): + if param.startswith('PATRONI_'): + name, suffix = (param[8:].rsplit('_', 1) + [''])[:2] + if name and suffix: + # PATRONI_(ETCD|CONSUL|ZOOKEEPER|...)_HOSTS? + if suffix in ('HOST', 'HOSTS') and '_' not in name: + value = os.environ.pop(param) + value = value if suffix == 'HOST' else value and _parse_list(value) + if value: + ret[name.lower()][suffix.lower()] = value + # PATRONI__PASSWORD=, PATRONI__OPTIONS= + # CREATE USER "" WITH PASSWORD '' + elif suffix == 'PASSWORD': + password = os.environ.pop(param) + if password: + users[name] = {'password': password} + options = os.environ.pop(param[:-9] + '_OPTIONS', None) + options = options and _parse_list(options) + if options: + users[name]['options'] = options + if users: + ret['bootstrap']['users'] = users + + return ret + def _build_effective_configuration(self, dynamic_configuration, local_configuration): config = self._safe_copy_dynamic_configuration(dynamic_configuration) for name, value in local_configuration.items(): @@ -167,6 +251,11 @@ class Config(object): elif name not in config: config[name] = deepcopy(value) if value else {} + if 'authentication' in config['restapi']: + restapi = config['restapi'] + auth = restapi['authentication'] + restapi['auth'] = '{0}:{1}'.format(auth['username'], auth['password']) + pg_config = config['postgresql'] # special treatment for old config diff --git a/patroni/utils.py b/patroni/utils.py index f85ec56f..fdeed596 100644 --- a/patroni/utils.py +++ b/patroni/utils.py @@ -60,6 +60,27 @@ def deep_compare(obj1, obj2): return True +def patch_config(config, data): + """recursively 'patch' `config` with `data` + :returns: `!True` if the `config` was changed""" + is_changed = False + for name, value in data.items(): + if value is None: + if config.pop(name, None) is not None: + is_changed = True + elif name in config: + if isinstance(value, dict): + if patch_config(config[name], value): + is_changed = True + elif str(config[name]) != str(value): + config[name] = value + is_changed = True + else: + config[name] = value + is_changed = True + return is_changed + + def parse_bool(value): """ >>> parse_bool(1) diff --git a/postgres0.yml b/postgres0.yml index e09cb4f0..c05788c4 100644 --- a/postgres0.yml +++ b/postgres0.yml @@ -4,6 +4,9 @@ name: postgresql0 restapi: listen: 127.0.0.1:8008 +# authentication: +# username: username +# password: password connect_address: 127.0.0.1:8008 etcd: diff --git a/tests/test_config.py b/tests/test_config.py index 6987fa67..d85d4f3a 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,3 +1,4 @@ +import os import unittest from mock import MagicMock, Mock, patch @@ -11,15 +12,40 @@ class TestConfig(unittest.TestCase): @patch('json.load', Mock(side_effect=Exception)) @patch.object(builtins, 'open', MagicMock()) def setUp(self): - self.config = Config(config_env='postgresql: {data_dir: foo}') + self.config = Config(config_env='restapi: {}\npostgresql: {data_dir: foo}') @patch.object(Config, '_build_effective_configuration', Mock(side_effect=Exception)) def test_set_dynamic_configuration(self): self.assertIsNone(self.config.set_dynamic_configuration({'foo': 'bar'})) def test_reload_local_configuration(self): + os.environ.update({ + 'PATRONI_NAME': 'postgres0', + 'PATRONI_NAMESPACE': '/patroni/', + 'PATRONI_SCOPE': 'batman2', + 'PATRONI_RESTAPI_USERNAME': 'username', + 'PATRONI_RESTAPI_PASSWORD': 'password', + 'PATRONI_RESTAPI_LISTEN': '0.0.0.0:8008', + 'PATRONI_RESTAPI_CONNECT_ADDRESS': '127.0.0.1:8008', + 'PATRONI_RESTAPI_CERTFILE': '/certfile', + 'PATRONI_RESTAPI_KEYFILE': '/keyfile', + 'PATRONI_POSTGRESQL_LISTEN': '0.0.0.0:5432', + 'PATRONI_POSTGRESQL_CONNECT_ADDRESS': '127.0.0.1:5432', + 'PATRONI_POSTGRESQL_DATA_DIR': 'data/postgres0', + 'PATRONI_POSTGRESQL_PGPASS': '/tmp/pgpass0', + 'PATRONI_ETCD_HOST': '127.0.0.1:2379', + 'PATRONI_CONSUL_HOST': '127.0.0.1:8500', + 'PATRONI_ZOOKEEPER_HOSTS': 'host1,host2', + 'PATRONI_foo_HOSTS': '[host1,host2', # Exception in parse_list + 'PATRONI_SUPERUSER_USERNAME': 'postgres', + 'PATRONI_SUPERUSER_PASSWORD': 'zalando', + 'PATRONI_REPLICATION_USERNAME': 'replicator', + 'PATRONI_REPLICATION_PASSWORD': 'rep-pass', + 'PATRONI_admin_PASSWORD': 'admin', + 'PATRONI_admin_OPTIONS': 'createrole,createdb' + }) config = Config(config_file='postgres0.yml') - with patch.object(Config, '_load_config_file', Mock(return_value={})): + with patch.object(Config, '_load_config_file', Mock(return_value={'restapi': {}})): with patch.object(Config, '_build_effective_configuration', Mock(side_effect=Exception)): self.assertRaises(Exception, config.reload_local_configuration, True) self.assertTrue(config.reload_local_configuration(True)) diff --git a/tests/test_ha.py b/tests/test_ha.py index ee90eae0..911401da 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -50,6 +50,8 @@ class MockPatroni(object): def __init__(self, p, d): self.config = Config(config_env=""" +restapi: + listen: 0.0.0.0:8008 bootstrap: users: replicator: diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 1e53e27b..a16b611d 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -61,7 +61,6 @@ class TestPatroni(unittest.TestCase): os.environ[Patroni.PATRONI_CONFIG_VARIABLE] = f.read() with patch.object(Patroni, 'run', Mock(side_effect=SleepException())): self.assertRaises(SleepException, _main) - del os.environ[Patroni.PATRONI_CONFIG_VARIABLE] @patch('patroni.config.Config.save_cache', Mock()) @patch('patroni.config.Config.reload_local_configuration', Mock(return_value=True)) From b65dc9a82715ec08d9f5085c0808a3afc18ad98c Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 8 Jun 2016 10:29:37 +0200 Subject: [PATCH 35/49] Update ENVIRONMENT.rst --- docs/ENVIRONMENT.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/ENVIRONMENT.rst b/docs/ENVIRONMENT.rst index c8ac4528..9fc3e398 100644 --- a/docs/ENVIRONMENT.rst +++ b/docs/ENVIRONMENT.rst @@ -15,9 +15,11 @@ Global/Universal Bootstrap configuration ----------------------- It is possible to define users which will be created right after initializing of a new cluster by defining following environment variables: + - **PATRONI\_\_PASSWORD=''** - **PATRONI\_\_OPTIONS='list,of,options'** -Example: defining of `PATRONI\_admin\_PASSWORD=admin` `PATRONI\_admin\_OPTIONS='createrole,createdb'` will cause creation of `admin` user which is allowed to create other users and databases + +Example: defining of ``PATRONI_admin_PASSWORD=strongpasswd`` and ``PATRONI_admin_OPTIONS='createrole,createdb'`` will cause creation of user **admin** with the password **strongpasswd**, which is allowed to create other users and databases. Consul ------ From f2fc68acde7130c7d55f398ad13459ce14a2e6e7 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 8 Jun 2016 10:31:59 +0200 Subject: [PATCH 36/49] Fix pep8 formatting --- patroni/config.py | 1 + 1 file changed, 1 insertion(+) diff --git a/patroni/config.py b/patroni/config.py index f8ec2686..31e849c6 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -166,6 +166,7 @@ class Config(object): @staticmethod def _build_environment_configuration(): ret = defaultdict(dict) + def _popenv(name): return os.environ.pop('PATRONI_' + name.upper(), None) From 23c5040ce561dc8a5103fe9d58dfc3ed7004f3a1 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 8 Jun 2016 12:35:53 +0200 Subject: [PATCH 37/49] Update documentation --- docs/ENVIRONMENT.rst | 29 ++++++++++++++--------------- docs/SETTINGS.rst | 22 +++++++++++++--------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/docs/ENVIRONMENT.rst b/docs/ENVIRONMENT.rst index 9fc3e398..ef29f8c6 100644 --- a/docs/ENVIRONMENT.rst +++ b/docs/ENVIRONMENT.rst @@ -2,24 +2,23 @@ Environment Configuration Settings ================================== -Some of configuration parameters defined in the configuration file is possible to override via Environment variables -This document list all possible environment variables handled by Patroni. -Environment variable always takes precedence on configuration file. +It is possible to override some of the configuration parameters defined in the Patroni configuration file using the system environment variables. This document lists all environment variables handled by Patroni. The values set via those variables always take precedence over the ones set in the Patroni configuration file. Global/Universal ---------------- +- **PATRONI\_CONFIGURATION**: it is possible to set the entire configuration for the Patroni via ``PATRONI_CONFIGURATION`` environment variable. In this case any other environment variables will not be considered! - **PATRONI\_NAME**: name of the node where the current instance of Patroni is running. Must be unique for the cluster. -- **PATRONI\_NAMESPACE**: path within configuration store where Patroni will keep information about cluster. Default value: "/service" +- **PATRONI\_NAMESPACE**: path within the configuration store where Patroni will keep information about the cluster. Default value: "/service" - **PATRONI\_SCOPE**: cluster name Bootstrap configuration ----------------------- -It is possible to define users which will be created right after initializing of a new cluster by defining following environment variables: +It is possible to create new database users right after the successful initialization of a new cluster. This process is defined by the following variables: - **PATRONI\_\_PASSWORD=''** - **PATRONI\_\_OPTIONS='list,of,options'** -Example: defining of ``PATRONI_admin_PASSWORD=strongpasswd`` and ``PATRONI_admin_OPTIONS='createrole,createdb'`` will cause creation of user **admin** with the password **strongpasswd**, which is allowed to create other users and databases. +Example: defining ``PATRONI_admin_PASSWORD=strongpasswd`` and ``PATRONI_admin_OPTIONS='createrole,createdb'`` will cause creation of the user **admin** with the password **strongpasswd** that is allowed to create other users and databases. Consul ------ @@ -33,21 +32,21 @@ PostgreSQL ---------- - **PATRONI\_POSTGRESQL\_LISTEN**: IP address + port that Postgres listens to. Multiple comma-separated addresses are permitted, as long as the port component is appended after to the last one with a colon, i.e. ``listen: 127.0.0.1,127.0.0.2:5432``. Patroni will use the first address from this list to establish local connections to the PostgreSQL node. - **PATRONI\_POSTGRESQL\_CONNECT\_ADDRESS**: IP address + port through which Postgres is accessible from other nodes and applications. -- **PATRONI\_POSTGRESQL\_DATA\_DIR**: file path to initialize and store Postgres data files. -- **PATRONI\_POSTGRESQL\_PGPASS**: path to `pgpass` file which would be created by Patroni when it is necessary (for example, before executing pg\_basebackup). This locations must be accessible for writing by Patroni. -- **PATRONI\_REPLICATION\_USERNAME**: replication username; user will be created during initialization. Replicas will use this user to access master via streaming replication -- **PATRONI\_REPLICATION\_PASSWORD**: replication password; user will be created during initialization. +- **PATRONI\_POSTGRESQL\_DATA\_DIR**: The location of the Postgres data directory, either existing or to be initialized by Patroni. +- **PATRONI\_POSTGRESQL\_PGPASS**: path to the [.pgpass password file](https://www.postgresql.org/docs/current/static/libpq-pgpass.html). Patroni creates this file before executing pg\_basebackup and under some other circumstances. The location must be writable by Patroni. +- **PATRONI\_REPLICATION\_USERNAME**: replication username; the user will be created during initialization. Replicas will use this user to access master via streaming replication +- **PATRONI\_REPLICATION\_PASSWORD**: replication password; the user will be created during initialization. - **PATRONI\_SUPERUSER\_USERNAME**: name for the superuser, set during initialization (initdb) and later used by Patroni to connect to the postgres. Also this user is used by pg_rewind. - **PATRONI\_SUPERUSER\_PASSWORD**: password for the superuser, set during initialization (initdb). REST API -------- -- **PATRONI\_RESTAPI\_CONNECT\_ADDRESS**: IP address and port through which restapi is accessible. +- **PATRONI\_RESTAPI\_CONNECT\_ADDRESS**: IP address and port to access the REST API. - **PATRONI\_RESTAPI\_LISTEN**: IP address and port that Patroni will listen to, to provide health-check information for HAProxy. -- **PATRONI\_RESTAPI\_USERNAME**: Basic-auth username to protect dangerous REST API endpoints. -- **PATRONI\_RESTAPI\_PASSWORD**: Basic-auth password to protect dangerous REST API endpoints. -- **PATRONI\_RESTAPI\_CERTFILE**: Specifies a file with the certificate in the PEM format. If the certfile is not specified or is left empty, the API server will work without SSL. -- **PATRONI\_RESTAPI\_KEYFILE**: Specifies a file with the secret key in the PEM format. +- **PATRONI\_RESTAPI\_USERNAME**: Basic-auth username to protect unsafe REST API endpoints. +- **PATRONI\_RESTAPI\_PASSWORD**: Basic-auth password to protect unsafe REST API endpoints. +- **PATRONI\_RESTAPI\_CERTFILE**: Specifies the file with the certificate in the PEM format. If the certfile is not specified or is left empty, the API server will work without SSL. +- **PATRONI\_RESTAPI\_KEYFILE**: Specifies the file with the secret key in the PEM format. ZooKeeper --------- diff --git a/docs/SETTINGS.rst b/docs/SETTINGS.rst index 6b0d4153..8524fbe8 100644 --- a/docs/SETTINGS.rst +++ b/docs/SETTINGS.rst @@ -5,7 +5,7 @@ YAML Configuration Settings Global/Universal ---------------- - **name**: the name of the host. Must be unique for the cluster. -- **namespace**: path within configuration store where Patroni will keep information about cluster. Default value: "/service" +- **namespace**: path within the configuration store where Patroni will keep information about the cluster. Default value: "/service" - **scope**: cluster name Bootstrap configuration @@ -48,8 +48,8 @@ PostgreSQL - **username**: name for the superuser, set during initialization (initdb) and later used by Patroni to connect to the postgres. - **password**: password for the superuser, set during initialization (initdb). - **replication**: - - **username**: replication username; user will be created during initialization. - - **password**: replication password; user will be created during initialization. + - **username**: replication username; the user will be created during initialization. Replicas will use this user to access master via streaming replication + - **password**: replication password; the user will be created during initialization. - **callbacks**: callback scripts to run on certain actions. Patroni will pass the action, role and cluster name. (See scripts/aws.py as an example of how to write them.) - **on\_reload**: run this script when configuration reload is triggered. - **on\_restart**: run this script when the cluster restarts. @@ -58,20 +58,24 @@ PostgreSQL - **on\_stop**: run this script when the cluster stops. - **connect\_address**: IP address + port through which Postgres is accessible from other nodes and applications. - **create\_replica\_methods**: an ordered list of the create methods for turning a Patroni node into a new replica. "basebackup" is the default method; other methods are assumed to refer to scripts, each of which is configured as its own config item. -- **data\_dir**: file path to initialize and store Postgres data files. +- **data\_dir**: The location of the Postgres data directory, either existing or to be initialized by Patroni. - **listen**: IP address + port that Postgres listens to; must be accessible from other nodes in the cluster, if you're using streaming replication. Multiple comma-separated addresses are permitted, as long as the port component is appended after to the last one with a colon, i.e. ``listen: 127.0.0.1,127.0.0.2:5432``. Patroni will use the first address from this list to establish local connections to the PostgreSQL node. +- **pgpass**: path to the [.pgpass password file](https://www.postgresql.org/docs/current/static/libpq-pgpass.html). Patroni creates this file before executing pg\_basebackup and under some other circumstances. The location must be writable by Patroni. - **recovery\_conf**: additional configuration settings written to recovery.conf when configuring follower. - **parameters**: list of configuration settings for Postgres. Many of these are required for replication to work. - **replica\_method** for each create_replica_method other than basebackup, you would add a configuration section of the same name. At a minimum, this should include "command" with a full path to the actual script to be executed. Other configuration parameters will be passed along to the script in the form "parameter=value". REST API -------- -- **connect\_address**: IP address and port through which restapi is accessible. +- **connect\_address**: IP address and port to access the REST API. - **listen**: IP address and port that Patroni will listen to, to provide health-check information for HAProxy. -- **Optional**: - - **auth**: 'username:password' to protect dangerous REST API endpoints. - - **certfile**: Specifies a file with the certificate in the PEM format. If the certfile is not specified or is left empty, the API server will work without SSL. - - **keyfile**: Specifies a file with the secret key in the PEM format. +- **Optional**: + - **authentication**: + - **username**: Basic-auth username to protect unsafe REST API endpoints. + - **password**: Basic-auth password to protect unsafe REST API endpoints. + + - **certfile**: Specifies the file with the certificate in the PEM format. If the certfile is not specified or is left empty, the API server will work without SSL. + - **keyfile**: Specifies the file with the secret key in the PEM format. ZooKeeper ---------- From 57c6641683663a066fbf383fc65e6cbd7395b45d Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 8 Jun 2016 19:02:22 +0200 Subject: [PATCH 38/49] Reimplement pg_ctl status in python subprocess.call was causing problems when server is running under high load. --- patroni/postgresql.py | 19 ++++++++++++++++--- tests/test_patroni.py | 1 + tests/test_postgresql.py | 41 +++++++++++++++++++++++++++++----------- 3 files changed, 47 insertions(+), 14 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 5672c616..f16c168a 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -74,6 +74,7 @@ class Postgresql(object): self._use_pg_rewind = config.get('use_pg_rewind', False) self._use_slots = config.get('use_slots', True) + self._version_file = os.path.join(self._data_dir, 'PG_VERSION') self._major_version = self.get_major_version() self._schedule_load_slots = self.use_slots @@ -112,10 +113,13 @@ class Postgresql(object): def use_slots(self): return self._use_slots and self._major_version >= 9.4 + def _version_file_exists(self): + return not self.data_directory_empty() and os.path.isfile(self._version_file) + def get_major_version(self): - if not self.data_directory_empty(): + if self._version_file_exists(): try: - with open(os.path.join(self._data_dir, 'PG_VERSION')) as f: + with open(self._version_file) as f: return float(f.read()) except Exception: logger.exception('Failed to read PG_VERSION from %s', self._data_dir) @@ -428,7 +432,16 @@ class Postgresql(object): return not self.query('SELECT pg_is_in_recovery()').fetchone()[0] def is_running(self): - return subprocess.call(' '.join(self._pg_ctl) + ' status > /dev/null 2>&1', shell=True) == 0 + if not (self._version_file_exists() and os.path.isfile(self._postmaster_pid)): + return False + try: + with open(self._postmaster_pid) as f: + pid = int(f.readline()) + if pid < 0: + pid = -pid + return pid > 0 and pid != os.getpid() and pid != os.getppid() and (os.kill(pid, 0) or True) + except Exception: + return False def call_nowait(self, cb_name): """ pick a callback command and call it without waiting for it to finish """ diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 1e53e27b..59adbaa4 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -20,6 +20,7 @@ from test_postgresql import Postgresql, psycopg2_connect @patch.object(Postgresql, 'write_pg_hba', Mock()) @patch.object(Postgresql, '_write_postgresql_conf', Mock()) @patch.object(Postgresql, 'write_recovery_conf', Mock()) +@patch.object(Postgresql, 'is_running', Mock(return_value=True)) @patch.object(BaseHTTPServer.HTTPServer, '__init__', Mock()) @patch.object(AsyncExecutor, 'run', Mock()) @patch.object(etcd.Client, 'write', etcd_write) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 92d2a3e8..badee349 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -160,6 +160,7 @@ class TestPostgresql(unittest.TestCase): @patch('psycopg2.connect', psycopg2_connect) @patch('os.rename', Mock()) @patch.object(Postgresql, 'get_major_version', Mock(return_value=9.4)) + @patch.object(Postgresql, 'is_running', Mock(return_value=True)) def setUp(self): self.data_dir = 'data/test0' if not os.path.exists(self.data_dir): @@ -197,9 +198,11 @@ class TestPostgresql(unittest.TestCase): def test_delete_trigger_file(self): self.p.delete_trigger_file() - def test_start(self): + @patch.object(Postgresql, 'is_running') + def test_start(self, mock_is_running): + mock_is_running.return_value = True self.assertTrue(self.p.start()) - self.p.is_running = false + mock_is_running.return_value = False open(os.path.join(self.data_dir, 'postmaster.pid'), 'w').close() pg_conf = os.path.join(self.data_dir, 'postgresql.conf') open(pg_conf, 'w').close() @@ -208,16 +211,16 @@ class TestPostgresql(unittest.TestCase): lines = f.readlines() self.assertTrue("f.oo = 'bar'\n" in lines) - def test_stop(self): + @patch.object(Postgresql, 'is_running') + def test_stop(self, mock_is_running): + mock_is_running.return_value = True self.assertTrue(self.p.stop()) with patch('subprocess.call', Mock(return_value=1)): + mock_is_running.return_value = False 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)') @@ -237,6 +240,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())) @patch('subprocess.check_output', Mock(return_value=0, side_effect=pg_controldata_string)) + @patch.object(Postgresql, 'is_running', Mock(return_value=True)) def test_follow(self, mock_pg_rewind): self.p.follow(None, None) self.p.follow(self.leader, self.leader) @@ -284,6 +288,7 @@ class TestPostgresql(unittest.TestCase): with patch('subprocess.call', Mock(side_effect=Exception("foo"))): self.assertEquals(self.p.create_replica(self.leader), 1) + @patch.object(Postgresql, 'is_running', Mock(return_value=True)) def test_sync_replication_slots(self): self.p.start() cluster = Cluster(True, None, self.leader, 0, [self.me, self.other, self.leadermem], None) @@ -312,9 +317,11 @@ class TestPostgresql(unittest.TestCase): def test_reload(self): self.assertTrue(self.p.reload()) - def test_is_healthy(self): + @patch.object(Postgresql, 'is_running') + def test_is_healthy(self, mock_is_running): + mock_is_running.return_value = True self.assertTrue(self.p.is_healthy()) - self.p.is_running = false + mock_is_running.return_value = False self.assertFalse(self.p.is_healthy()) def test_promote(self): @@ -325,6 +332,13 @@ class TestPostgresql(unittest.TestCase): def test_last_operation(self): self.assertEquals(self.p.last_operation(), '0') + @patch('os.path.isfile', Mock(return_value=True)) + @patch('os.kill', Mock(side_effect=Exception)) + @patch.object(builtins, 'open', mock_open(read_data='-999999999999999')) + @patch.object(Postgresql, '_version_file_exists', Mock(return_value=True)) + def test_is_running(self): + self.assertFalse(self.p.is_running()) + @patch('subprocess.Popen', Mock(side_effect=OSError)) def test_call_nowait(self): self.assertFalse(self.p.call_nowait('on_start')) @@ -332,6 +346,7 @@ class TestPostgresql(unittest.TestCase): def test_non_existing_callback(self): self.assertFalse(self.p.call_nowait('foobar')) + @patch.object(Postgresql, 'is_running', Mock(return_value=True)) def test_is_leader_exception(self): self.p.start() self.p.query = Mock(side_effect=psycopg2.OperationalError("not supported")) @@ -343,11 +358,11 @@ class TestPostgresql(unittest.TestCase): @patch('os.rename', Mock()) @patch('os.path.isdir', Mock(return_value=True)) def test_move_data_directory(self): - self.p.is_running = false self.p.move_data_directory() with patch('os.rename', Mock(side_effect=OSError)): self.p.move_data_directory() + @patch.object(Postgresql, 'is_running', Mock(return_value=True)) def test_bootstrap(self): with patch('subprocess.call', Mock(return_value=1)): self.assertRaises(PostgresException, self.p.bootstrap, {}) @@ -478,6 +493,7 @@ class TestPostgresql(unittest.TestCase): self.p.config['foo'] = {'command': 'bar'} self.assertFalse(self.p.replica_method_can_work_without_replication_connection('foo')) + @patch.object(Postgresql, 'is_running', Mock(return_value=True)) def test_reload_config(self): parameters = self._PARAMETERS.copy() parameters.pop('f.oo') @@ -490,6 +506,9 @@ class TestPostgresql(unittest.TestCase): parameters.pop('search_path') self.p.reload_config({'retry_timeout': 10, 'listen': '*:5433', 'parameters': parameters}) - @patch.object(builtins, 'open', mock_open(read_data='9.4')) + @patch.object(Postgresql, '_version_file_exists', Mock(return_value=True)) def test_get_major_version(self): - self.assertEquals(self.p.get_major_version(), 9.4) + with patch.object(builtins, 'open', mock_open(read_data='9.4')): + self.assertEquals(self.p.get_major_version(), 9.4) + with patch.object(builtins, 'open', Mock(side_effect=Exception)): + self.assertEquals(self.p.get_major_version(), 0.0) From f57631153267c7747ff326e63f04eff8292a88bc Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 9 Jun 2016 11:19:31 +0200 Subject: [PATCH 39/49] Add special treatment for zookeeper.exhibitor section --- patroni/config.py | 13 ++++++++++--- tests/test_ha.py | 4 ++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/patroni/config.py b/patroni/config.py index 541905a0..947eee41 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -167,16 +167,23 @@ class Config(object): elif name not in config: config[name] = deepcopy(value) if value else {} - pg_config = config['postgresql'] - # special treatment for old config + + # 'exhibitor' inside 'zookeeper': + if 'zookeeper' in config and 'exhibitor' in config['zookeeper']: + config['exhibitor'] = config['zookeeper'].pop('exhibitor') + config.pop('zookeeper') + + pg_config = config['postgresql'] + # no 'authentication' in 'postgresql', but 'replication' and 'superuser' if 'authentication' not in pg_config: pg_config['use_pg_rewind'] = 'pg_rewind' in pg_config pg_config['authentication'] = {u: pg_config[u] for u in ('replication', 'superuser') if u in pg_config} - + # no 'superuser' in 'postgresql'.'authentication' if 'superuser' not in pg_config['authentication'] and 'pg_rewind' in pg_config: pg_config['authentication']['superuser'] = pg_config['pg_rewind'] + # no 'name' in config if 'name' not in config and 'name' in pg_config: config['name'] = pg_config['name'] diff --git a/tests/test_ha.py b/tests/test_ha.py index ee90eae0..6b5c6deb 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -62,6 +62,10 @@ postgresql: pg_rewind: username: postgres password: postgres +zookeeper: + exhibitor: + hosts: [localhost] + port: 8181 """) self.postgresql = p self.dcs = d From e9be5e846290a7bdaaebdda4e5b3f97c9f15252f Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 9 Jun 2016 11:40:10 +0200 Subject: [PATCH 40/49] Configure exhibitor port via ENV --- docs/ENVIRONMENT.rst | 1 + patroni/config.py | 12 +++++++----- tests/test_config.py | 2 ++ 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/docs/ENVIRONMENT.rst b/docs/ENVIRONMENT.rst index 70aae806..d5ea6c1e 100644 --- a/docs/ENVIRONMENT.rst +++ b/docs/ENVIRONMENT.rst @@ -31,6 +31,7 @@ Etcd Exhibitor --------- - **PATRONI\_EXHIBITOR\_HOSTS**: initial list of Exhibitor (ZooKeeper) nodes in format: ['host1', 'host2', 'etc...' ]. This list updates automatically whenever the Exhibitor (ZooKeeper) cluster topology changes. +- **PATRONI\_EXHIBITOR\_PORT**: Exhibitor port. PostgreSQL ---------- diff --git a/patroni/config.py b/patroni/config.py index e4a4b390..07fd508f 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -8,7 +8,7 @@ from collections import defaultdict from copy import deepcopy from patroni.dcs import ClusterConfig from patroni.postgresql import Postgresql -from patroni.utils import deep_compare, patch_config +from patroni.utils import deep_compare, parse_int, patch_config logger = logging.getLogger(__name__) @@ -219,10 +219,13 @@ class Config(object): if param.startswith('PATRONI_'): name, suffix = (param[8:].rsplit('_', 1) + [''])[:2] if name and suffix: - # PATRONI_(ETCD|CONSUL|ZOOKEEPER|...)_HOSTS? - if suffix in ('HOST', 'HOSTS') and '_' not in name: + # PATRONI_(ETCD|CONSUL|ZOOKEEPER|EXHIBITOR|...)_(HOSTS?|PORT) + if suffix in ('HOST', 'HOSTS', 'PORT') and '_' not in name: value = os.environ.pop(param) - value = value if suffix == 'HOST' else value and _parse_list(value) + if suffix == 'PORT': + value = value and parse_int(value) + elif suffix == 'HOSTS': + value = value and _parse_list(value) if value: ret[name.lower()][suffix.lower()] = value # PATRONI__PASSWORD=, PATRONI__OPTIONS= @@ -252,7 +255,6 @@ class Config(object): elif name not in config: config[name] = deepcopy(value) if value else {} - # restapi server expects to get restapi.auth = 'username:password' if 'authentication' in config['restapi']: restapi = config['restapi'] diff --git a/tests/test_config.py b/tests/test_config.py index d85d4f3a..e6d212d6 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -36,6 +36,8 @@ class TestConfig(unittest.TestCase): 'PATRONI_ETCD_HOST': '127.0.0.1:2379', 'PATRONI_CONSUL_HOST': '127.0.0.1:8500', 'PATRONI_ZOOKEEPER_HOSTS': 'host1,host2', + 'PATRONI_EXHIBITOR_HOSTS': 'host1,host2', + 'PATRONI_EXHIBITOR_PORT': '8181', 'PATRONI_foo_HOSTS': '[host1,host2', # Exception in parse_list 'PATRONI_SUPERUSER_USERNAME': 'postgres', 'PATRONI_SUPERUSER_PASSWORD': 'zalando', From 7244739e2637f5d889dc3ca4bbdeb356f41fb1d2 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 9 Jun 2016 12:10:37 +0200 Subject: [PATCH 41/49] Fix link to the libpq-pgpass.html --- docs/ENVIRONMENT.rst | 2 +- docs/SETTINGS.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/ENVIRONMENT.rst b/docs/ENVIRONMENT.rst index d5ea6c1e..1db8dadf 100644 --- a/docs/ENVIRONMENT.rst +++ b/docs/ENVIRONMENT.rst @@ -38,7 +38,7 @@ PostgreSQL - **PATRONI\_POSTGRESQL\_LISTEN**: IP address + port that Postgres listens to. Multiple comma-separated addresses are permitted, as long as the port component is appended after to the last one with a colon, i.e. ``listen: 127.0.0.1,127.0.0.2:5432``. Patroni will use the first address from this list to establish local connections to the PostgreSQL node. - **PATRONI\_POSTGRESQL\_CONNECT\_ADDRESS**: IP address + port through which Postgres is accessible from other nodes and applications. - **PATRONI\_POSTGRESQL\_DATA\_DIR**: The location of the Postgres data directory, either existing or to be initialized by Patroni. -- **PATRONI\_POSTGRESQL\_PGPASS**: path to the [.pgpass password file](https://www.postgresql.org/docs/current/static/libpq-pgpass.html). Patroni creates this file before executing pg\_basebackup and under some other circumstances. The location must be writable by Patroni. +- **PATRONI\_POSTGRESQL\_PGPASS**: path to the `.pgpass `__ password file. Patroni creates this file before executing pg\_basebackup and under some other circumstances. The location must be writable by Patroni. - **PATRONI\_REPLICATION\_USERNAME**: replication username; the user will be created during initialization. Replicas will use this user to access master via streaming replication - **PATRONI\_REPLICATION\_PASSWORD**: replication password; the user will be created during initialization. - **PATRONI\_SUPERUSER\_USERNAME**: name for the superuser, set during initialization (initdb) and later used by Patroni to connect to the postgres. Also this user is used by pg_rewind. diff --git a/docs/SETTINGS.rst b/docs/SETTINGS.rst index 8e46e952..86f21d5c 100644 --- a/docs/SETTINGS.rst +++ b/docs/SETTINGS.rst @@ -66,7 +66,7 @@ PostgreSQL - **create\_replica\_methods**: an ordered list of the create methods for turning a Patroni node into a new replica. "basebackup" is the default method; other methods are assumed to refer to scripts, each of which is configured as its own config item. - **data\_dir**: The location of the Postgres data directory, either existing or to be initialized by Patroni. - **listen**: IP address + port that Postgres listens to; must be accessible from other nodes in the cluster, if you're using streaming replication. Multiple comma-separated addresses are permitted, as long as the port component is appended after to the last one with a colon, i.e. ``listen: 127.0.0.1,127.0.0.2:5432``. Patroni will use the first address from this list to establish local connections to the PostgreSQL node. -- **pgpass**: path to the [.pgpass password file](https://www.postgresql.org/docs/current/static/libpq-pgpass.html). Patroni creates this file before executing pg\_basebackup and under some other circumstances. The location must be writable by Patroni. +- **pgpass**: path to the `.pgpass `__ password file. Patroni creates this file before executing pg\_basebackup and under some other circumstances. The location must be writable by Patroni. - **recovery\_conf**: additional configuration settings written to recovery.conf when configuring follower. - **parameters**: list of configuration settings for Postgres. Many of these are required for replication to work. - **replica\_method** for each create_replica_method other than basebackup, you would add a configuration section of the same name. At a minimum, this should include "command" with a full path to the actual script to be executed. Other configuration parameters will be passed along to the script in the form "parameter=value". From 49efb371f9b46e0552d32c999dd03b80de27f678 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 9 Jun 2016 14:44:29 +0200 Subject: [PATCH 42/49] Make it possible to work without config.yml Most of the basic configuration could be done via ENV --- patroni/__init__.py | 20 +++----------------- patroni/config.py | 30 ++++++++++++++++++++++-------- tests/test_config.py | 11 +++++++++-- tests/test_ha.py | 10 ++++++---- tests/test_patroni.py | 12 ++---------- 5 files changed, 42 insertions(+), 41 deletions(-) diff --git a/patroni/__init__.py b/patroni/__init__.py index 84a1018d..c48eab40 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -1,7 +1,5 @@ import logging -import os import signal -import sys import time from patroni.api import RestApiServer @@ -17,11 +15,10 @@ logger = logging.getLogger(__name__) class Patroni(object): - PATRONI_CONFIG_VARIABLE = 'PATRONI_CONFIGURATION' - def __init__(self, config_file=None, config_env=None): + def __init__(self): self.version = __version__ - self.config = Config(config_file=config_file, config_env=config_env) + self.config = Config() self.dcs = get_dcs(self.config) self.load_dynamic_configuration() @@ -116,18 +113,7 @@ def main(): logging.getLogger('requests').setLevel(logging.WARNING) setup_signal_handlers() - # Patroni reads the configuration from the command-line argument if it exists, and from the environment otherwise. - config_env = False - config_file = len(sys.argv) >= 2 and os.path.isfile(sys.argv[1]) and sys.argv[1] - if not config_file: - config_env = os.environ.pop(Patroni.PATRONI_CONFIG_VARIABLE, None) - if config_env is None: - print('Usage: {0} config.yml'.format(sys.argv[0])) - print('\tPatroni may also read the configuration from the {} environment variable'. - format(Patroni.PATRONI_CONFIG_VARIABLE)) - return - - patroni = Patroni(config_file, config_env) + patroni = Patroni() try: patroni.run() except KeyboardInterrupt: diff --git a/patroni/config.py b/patroni/config.py index 07fd508f..d982842a 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -1,6 +1,7 @@ import json import logging import os +import sys import tempfile import yaml @@ -33,6 +34,9 @@ class Config(object): to work with it as with the old `config` object. """ + PATRONI_ENV_PREFIX = 'PATRONI_' + PATRONI_CONFIG_VARIABLE = PATRONI_ENV_PREFIX + 'CONFIGURATION' + __CACHE_FILENAME = 'patroni.dynamic.json' __DEFAULT_CONFIG = { 'ttl': 30, 'loop_wait': 10, 'retry_timeout': 10, @@ -42,15 +46,25 @@ class Config(object): } } - def __init__(self, config_file=None, config_env=None): - self._config_file = None if config_env else config_file + def __init__(self): self._modify_index = -1 self._dynamic_configuration = {} - if config_env: - self._local_configuration = yaml.safe_load(config_env) - else: - self.__environment_configuration = self._build_environment_configuration() + + self.__environment_configuration = self._build_environment_configuration() + + # Patroni reads the configuration from the command-line argument if it exists, otherwise from the environment + self._config_file = len(sys.argv) >= 2 and os.path.isfile(sys.argv[1]) and sys.argv[1] + if self._config_file: self._local_configuration = self._load_config_file() + else: + config_env = os.environ.pop(self.PATRONI_CONFIG_VARIABLE, None) + self._local_configuration = config_env and yaml.safe_load(config_env) or self.__environment_configuration + if not self._local_configuration: + print('Usage: {0} config.yml'.format(sys.argv[0])) + print('\tPatroni may also read the configuration from the {0} environment variable'. + format(self.PATRONI_CONFIG_VARIABLE)) + exit(1) + self.__effective_configuration = self._build_effective_configuration(self._dynamic_configuration, self._local_configuration) self._data_dir = self.__effective_configuration['postgresql']['data_dir'] @@ -168,7 +182,7 @@ class Config(object): ret = defaultdict(dict) def _popenv(name): - return os.environ.pop('PATRONI_' + name.upper(), None) + return os.environ.pop(Config.PATRONI_ENV_PREFIX + name.upper(), None) for param in ('name', 'namespace', 'scope'): value = _popenv(param) @@ -216,7 +230,7 @@ class Config(object): return None for param in list(os.environ.keys()): - if param.startswith('PATRONI_'): + if param.startswith(Config.PATRONI_ENV_PREFIX): name, suffix = (param[8:].rsplit('_', 1) + [''])[:2] if name and suffix: # PATRONI_(ETCD|CONSUL|ZOOKEEPER|EXHIBITOR|...)_(HOSTS?|PORT) diff --git a/tests/test_config.py b/tests/test_config.py index e6d212d6..a8a5a6c4 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,5 +1,6 @@ import os import unittest +import sys from mock import MagicMock, Mock, patch from patroni.config import Config @@ -12,7 +13,12 @@ class TestConfig(unittest.TestCase): @patch('json.load', Mock(side_effect=Exception)) @patch.object(builtins, 'open', MagicMock()) def setUp(self): - self.config = Config(config_env='restapi: {}\npostgresql: {data_dir: foo}') + sys.argv = ['patroni.py'] + os.environ[Config.PATRONI_CONFIG_VARIABLE] = 'restapi: {}\npostgresql: {data_dir: foo}' + self.config = Config() + + def test_no_config(self): + self.assertRaises(SystemExit, Config) @patch.object(Config, '_build_effective_configuration', Mock(side_effect=Exception)) def test_set_dynamic_configuration(self): @@ -46,7 +52,8 @@ class TestConfig(unittest.TestCase): 'PATRONI_admin_PASSWORD': 'admin', 'PATRONI_admin_OPTIONS': 'createrole,createdb' }) - config = Config(config_file='postgres0.yml') + sys.argv = ['patroni.py', 'postgres0.yml'] + config = Config() with patch.object(Config, '_load_config_file', Mock(return_value={'restapi': {}})): with patch.object(Config, '_build_effective_configuration', Mock(side_effect=Exception)): self.assertRaises(Exception, config.reload_local_configuration, True) diff --git a/tests/test_ha.py b/tests/test_ha.py index 59be45cd..4f691212 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -1,7 +1,8 @@ -import etcd -import unittest import datetime +import etcd +import os import pytz +import unittest from mock import Mock, MagicMock, patch from patroni.config import Config @@ -49,7 +50,7 @@ def get_cluster_initialized_with_only_leader(failover=None): class MockPatroni(object): def __init__(self, p, d): - self.config = Config(config_env=""" + os.environ[Config.PATRONI_CONFIG_VARIABLE] = """ restapi: listen: 0.0.0.0:8008 bootstrap: @@ -68,7 +69,8 @@ zookeeper: exhibitor: hosts: [localhost] port: 8181 -""") +""" + self.config = Config() self.postgresql = p self.dcs = d self.api = Mock() diff --git a/tests/test_patroni.py b/tests/test_patroni.py index b685f2ae..f9246cac 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -1,5 +1,4 @@ import etcd -import os import sys import time import unittest @@ -34,7 +33,8 @@ class TestPatroni(unittest.TestCase): RestApiServer.socket = 0 with patch.object(etcd.Client, 'machines') as mock_machines: mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) - self.p = Patroni('postgres0.yml') + sys.argv = ['patroni.py', 'postgres0.yml'] + self.p = Patroni() @patch('patroni.dcs.AbstractDCS.get_cluster', Mock(side_effect=[None, DCSError('foo'), None])) def test_load_dynamic_configuration(self): @@ -47,7 +47,6 @@ class TestPatroni(unittest.TestCase): @patch.object(etcd.Client, 'machines') def test_patroni_main(self, mock_machines): with patch('subprocess.call', Mock(return_value=1)): - _main() sys.argv = ['patroni.py', 'postgres0.yml'] mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) @@ -55,13 +54,6 @@ class TestPatroni(unittest.TestCase): self.assertRaises(SleepException, _main) with patch.object(Patroni, 'run', Mock(side_effect=KeyboardInterrupt())): _main() - sys.argv = ['patroni.py'] - # read the content of the yaml configuration file into the environment variable - # in order to test how does patroni handle the configuration passed from the environment. - with open('postgres0.yml', 'r') as f: - os.environ[Patroni.PATRONI_CONFIG_VARIABLE] = f.read() - with patch.object(Patroni, 'run', Mock(side_effect=SleepException())): - self.assertRaises(SleepException, _main) @patch('patroni.config.Config.save_cache', Mock()) @patch('patroni.config.Config.reload_local_configuration', Mock(return_value=True)) From 95db7259d411212f9702aa44e29357e5a796a3b6 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 10 Jun 2016 08:43:08 +0200 Subject: [PATCH 43/49] Implement strtol as close as possible to stdlib.strtol --- patroni/postgresql.py | 3 +-- patroni/utils.py | 60 +++++++++++++++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 61b8f53b..f7e2e79c 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -503,9 +503,8 @@ class Postgresql(object): self._pending_restart = False self.set_state('running' if ret else 'start failed') - if ret: - self._schedule_load_slots = self.use_slots + 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 diff --git a/patroni/utils.py b/patroni/utils.py index f85ec56f..0a18f25c 100644 --- a/patroni/utils.py +++ b/patroni/utils.py @@ -75,17 +75,51 @@ def parse_bool(value): return False -def split_int_unit(value, strict=True): - value = str(value) - l = len(value) - 1 - while l >= 0 and not value[l].isdigit(): - l -= 1 - unit = value[l + 1:].strip() - try: - value = int(value[:l + 1], 0) if six.PY3 else long(value[:l + 1], 0) - except ValueError: - value = None if strict else 1 - return (value, unit) +def strtol(value, strict=True): + """As most as possible close equivalent of strtol(3) function (with base=0), + used by postgres to parse parameter values. + >>> strtol(1) == (1, '') + True + >>> strtol(' +0x400MB') == (1024, 'MB') + True + >>> strtol(' -070d') == (-56, 'd') + True + >>> strtol(' d ') == (None, 'd') + True + >>> strtol(' s ', False) == (1, 's') + True + """ + value = str(value).strip() + l = len(value) + i = 0 + # skip sign: + if i < l and value[i] in ('-', '+'): + i += 1 + + # we always expect to get digit in the beginning + if i < l and value[i].isdigit(): + if value[i] == '0': + i += 1 + if i < l and value[i] == 'x': # '0' followed by 'x': HEX + base = 16 + i += 1 + else: # just starts with '0': OCT + base = 8 + else: # any other digit: DEC + base = 10 + + ret = None + while i < l: + try: # try to find maximally long number + i += 1 # by giving to `int` longer and longer strings + ret = int(value[:i], base) if six.PY3 else long(value[:i], base) + except ValueError: # until we will not get an exception or end of the string + i -= 1 + break + if ret is not None: # yay! there is a number in the beginning of the string + return ret, value[i:].strip() # return the number and the "rest" + + return (None if strict else 1), value.strip() def parse_int(value, base_unit=None): @@ -109,13 +143,13 @@ def parse_int(value, base_unit=None): 'min': {'ms': -1000 * 60, 's': -60, 'min': 1, 'h': 60, 'd': 60 * 24} } - value, unit = split_int_unit(value) + value, unit = strtol(value) if value is not None: if not unit: return value if base_unit and base_unit not in convert: - base_value, base_unit = split_int_unit(base_unit, False) + base_value, base_unit = strtol(base_unit, False) else: base_value = 1 if base_unit in convert and unit in convert[base_unit]: From 9ecff0f64d25ddbb9426423b692f6382c49867a5 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 10 Jun 2016 12:35:04 +0200 Subject: [PATCH 44/49] Bugfixes * GET /config was returning latesy "correct" version of dynamic configuration. * PATCH /config was breaking when trying to patch not dict with dict --- patroni/api.py | 6 +++++- patroni/utils.py | 6 +++++- tests/test_api.py | 10 ++++++---- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/patroni/api.py b/patroni/api.py index 420067ca..7000fc2f 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -106,7 +106,11 @@ class RestApiHandler(BaseHTTPRequestHandler): self._write_status_response(200, response) def do_GET_config(self): - self._write_json_response(200, self.server.patroni.config.dynamic_configuration) + cluster = self.server.patroni.ha.dcs.cluster or self.server.patroni.ha.dcs.get_cluster() + if cluster.config: + self._write_json_response(200, cluster.config.data) + else: + self.send_error(502) def _read_json_content(self): if 'content-length' not in self.headers: diff --git a/patroni/utils.py b/patroni/utils.py index 9c63c768..13869098 100644 --- a/patroni/utils.py +++ b/patroni/utils.py @@ -70,7 +70,11 @@ def patch_config(config, data): is_changed = True elif name in config: if isinstance(value, dict): - if patch_config(config[name], value): + if isinstance(config[name], dict): + if patch_config(config[name], value): + is_changed = True + else: + config[name] = value is_changed = True elif str(config[name]) != str(value): config[name] = value diff --git a/tests/test_api.py b/tests/test_api.py index 079fa7f7..8483299b 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -118,9 +118,11 @@ class TestRestApiHandler(unittest.TestCase): self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'POST /restart HTTP/1.0')) MockRestApiServer(RestApiHandler, 'POST /restart HTTP/1.0\nAuthorization:') - @patch.object(MockPatroni, 'config') - def test_do_GET_config(self, mock_config): - mock_config.dynamic_configuration = {} + @patch.object(MockHa, 'dcs') + def test_do_GET_config(self, mock_dcs): + mock_dcs.cluster.config.data = {} + self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'GET /config')) + mock_dcs.cluster.config = None self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'GET /config')) @patch.object(MockHa, 'dcs') @@ -132,7 +134,7 @@ class TestRestApiHandler(unittest.TestCase): request += '\nContent-Length: ' self.assertIsNotNone(MockRestApiServer(RestApiHandler, request + '34\n\n{"postgresql":{"use_slots":false}}')) config['ttl'] = 5 - config['postgresql'].update({'use_slots': True, "parameters": None}) + config['postgresql'].update({'use_slots': {'foo': True}, "parameters": None}) config = json.dumps(config) request += str(len(config)) + '\n\n' + config MockRestApiServer(RestApiHandler, request) From e373a1e0bb2b94dea8c9f71b78b758bae645dad2 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 13 Jun 2016 10:32:54 +0200 Subject: [PATCH 45/49] Hexadecimal can be written as 0X --- patroni/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/utils.py b/patroni/utils.py index 0a18f25c..fd9aefe1 100644 --- a/patroni/utils.py +++ b/patroni/utils.py @@ -100,7 +100,7 @@ def strtol(value, strict=True): if i < l and value[i].isdigit(): if value[i] == '0': i += 1 - if i < l and value[i] == 'x': # '0' followed by 'x': HEX + if i < l and value[i] in ('x', 'X'): # '0' followed by 'x': HEX base = 16 i += 1 else: # just starts with '0': OCT From c64170ef335025f4fd74efe6d7d6d6c7edb1cc69 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 13 Jun 2016 10:33:14 +0200 Subject: [PATCH 46/49] Extend list of postgres parameters controlled by Patroni These parameters usually must be the same across all cluster nodes and therefore must be set only via global configuration and always passed as a list of postgres arguments (via pg_ctl) to make it not possible accidentally change them by 'ALTER SYSTEM' --- patroni/config.py | 8 ++------ patroni/postgresql.py | 40 ++++++++++++++++++++++++++-------------- tests/test_postgresql.py | 6 ++++-- 3 files changed, 32 insertions(+), 22 deletions(-) diff --git a/patroni/config.py b/patroni/config.py index 947eee41..377337c9 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -37,7 +37,7 @@ class Config(object): 'ttl': 30, 'loop_wait': 10, 'retry_timeout': 10, 'maximum_lag_on_failover': 1048576, 'postgresql': { - 'parameters': Postgresql.CMDLINE_OPTIONS + 'parameters': {p: v[0] for p, v in Postgresql.CMDLINE_OPTIONS.items()} } } @@ -133,11 +133,7 @@ class Config(object): def _process_postgresql_parameters(self, parameters, is_local=False): ret = {} for name, value in (parameters or {}).items(): - if (is_local and name not in self.__DEFAULT_CONFIG['postgresql']['parameters']) \ - or not ((name == 'wal_level' and value not in ('hot_standby', 'logical')) or - (name in ('max_replication_slots', 'max_wal_senders', 'wal_keep_segments') and - int(value) < self.__DEFAULT_CONFIG['postgresql']['parameters'][name]) or - name in ('hot_standby', 'wal_log_hints')): + if name not in Postgresql.CMDLINE_OPTIONS or not is_local and Postgresql.CMDLINE_OPTIONS[name][1](value): ret[name] = value return ret diff --git a/patroni/postgresql.py b/patroni/postgresql.py index f7e2e79c..50d57c3a 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -8,7 +8,7 @@ import tempfile import time from patroni.exceptions import PostgresConnectionException, PostgresException -from patroni.utils import compare_values, parse_int, Retry, RetryFailedError +from patroni.utils import compare_values, parse_bool, parse_int, Retry, RetryFailedError from six import string_types from six.moves.urllib_parse import urlparse from threading import Lock @@ -48,15 +48,28 @@ class Postgresql(object): # These parameters could be changed only globally, i.e. via DCS. # P.S. 'listen_addresses' and 'port' are added here just for convenience, to mark them # as a parameters which should always be passed through command line. + # + # Format: + # key - parameter name + # value - tuple(default_value, check_function, min_version) + # default_value -- some sane default value + # check_function -- if the new value is not correct must return `!False` + # min_version -- major version of PostgreSQL when parameter was introduced CMDLINE_OPTIONS = { - 'listen_addresses': None, - 'port': None, - 'wal_level': 'hot_standby', - 'hot_standby': 'on', - 'max_wal_senders': 5, - 'wal_keep_segments': 8, - 'max_replication_slots': 5, - 'wal_log_hints': 'on' + 'listen_addresses': (None, lambda _: False, 9.1), + 'port': (None, lambda _: False, 9.1), + 'cluster_name': (None, lambda _: False, 9.5), + 'wal_level': ('hot_standby', lambda v: v.lower() in ('hot_standby', 'logical'), 9.1), + 'hot_standby': ('on', lambda _: False, 9.1), + 'max_connections': (100, lambda v: int(v) >= 100, 9.1), + 'max_wal_senders': (5, lambda v: int(v) >= 5, 9.1), + 'wal_keep_segments': (8, lambda v: int(v) >= 8, 9.1), + 'max_prepared_transactions': (0, lambda v: int(v) >= 0, 9.1), + 'max_locks_per_transaction': (64, lambda v: int(v) >= 64, 9.1), + 'track_commit_timestamp': ('off', lambda v: parse_bool(v) is not None, 9.5), + 'max_replication_slots': (5, lambda v: int(v) >= 5, 9.4), + 'max_worker_processes': (8, lambda v: int(v) >= 8, 9.4), + 'wal_log_hints': ('on', lambda _: False, 9.4) } def __init__(self, config): @@ -127,11 +140,10 @@ class Postgresql(object): logger.exception('Failed to read PG_VERSION from %s', self._data_dir) return 0.0 - @staticmethod - def get_server_parameters(config): + def get_server_parameters(self, config): parameters = config['parameters'].copy() listen_addresses, port = (config['listen'] + ':5432').split(':')[:2] - parameters.update({'listen_addresses': listen_addresses, 'port': port}) + parameters.update({'cluster_name': self.scope, 'listen_addresses': listen_addresses, 'port': port}) return parameters def resolve_connection_addresses(self): @@ -496,8 +508,8 @@ class Postgresql(object): self._write_postgresql_conf() self.resolve_connection_addresses() - options = ' '.join("--{0}='{1}'".format(p, self._server_parameters[p]) for p in self.CMDLINE_OPTIONS - if not (self._major_version < 9.4 and p in ('max_replication_slots', 'wal_log_hints'))) + options = ' '.join("--{0}='{1}'".format(p, self._server_parameters[p]) for p, v in self.CMDLINE_OPTIONS.items() + if self._major_version >= v[2]) ret = subprocess.call(self._pg_ctl + ['start', '-o', options], env=env, preexec_fn=os.setsid) == 0 self._pending_restart = False diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index badee349..7fa7279a 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -153,8 +153,10 @@ def fake_listdir(path): @patch('subprocess.call', Mock(return_value=0)) @patch('psycopg2.connect', psycopg2_connect) class TestPostgresql(unittest.TestCase): - _PARAMETERS = {'wal_level': 'hot_standby', 'max_replication_slots': 5, 'f.oo': 'bar', 'search_path': 'public', - 'hot_standby': 'on', 'max_wal_senders': 5, 'wal_keep_segments': 8, 'wal_log_hints': 'on'} + _PARAMETERS = {'wal_level': 'hot_standby', 'max_replication_slots': 5, 'f.oo': 'bar', + 'search_path': 'public', 'hot_standby': 'on', 'max_wal_senders': 5, + 'wal_keep_segments': 8, 'wal_log_hints': 'on', 'max_locks_per_transaction': 64, + 'max_worker_processes': 8, 'max_connections': 100, 'max_prepared_transactions': 0} @patch('subprocess.call', Mock(return_value=0)) @patch('psycopg2.connect', psycopg2_connect) From 8829ef6babe903c35c0ea5c6618aec93fbab5825 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 13 Jun 2016 10:55:15 +0200 Subject: [PATCH 47/49] Make QuantifiedCode happy --- patroni/config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/patroni/config.py b/patroni/config.py index 377337c9..788d1b11 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -130,7 +130,8 @@ class Config(object): if dry_run: raise - def _process_postgresql_parameters(self, parameters, is_local=False): + @staticmethod + def _process_postgresql_parameters(parameters, is_local=False): ret = {} for name, value in (parameters or {}).items(): if name not in Postgresql.CMDLINE_OPTIONS or not is_local and Postgresql.CMDLINE_OPTIONS[name][1](value): From a24b29deec75937b4fed1e262de9a094ca2ef437 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 13 Jun 2016 12:54:33 +0200 Subject: [PATCH 48/49] use_slots can be changed only globally --- patroni/config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/patroni/config.py b/patroni/config.py index 788d1b11..a9c52375 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -37,6 +37,7 @@ class Config(object): 'ttl': 30, 'loop_wait': 10, 'retry_timeout': 10, 'maximum_lag_on_failover': 1048576, 'postgresql': { + 'use_slots': True, 'parameters': {p: v[0] for p, v in Postgresql.CMDLINE_OPTIONS.items()} } } @@ -159,7 +160,7 @@ class Config(object): for name, value in (value or {}).items(): if name == 'parameters': config['postgresql'][name].update(self._process_postgresql_parameters(value, True)) - else: + elif name != 'use_slots': # replication slots must be enabled/disabled globally config['postgresql'][name] = deepcopy(value) elif name not in config: config[name] = deepcopy(value) if value else {} From 3ff11065263f7679556df54377b50d928fbe2c9f Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 13 Jun 2016 14:11:35 +0200 Subject: [PATCH 49/49] Reset restart_pending flag when parameter was set to the old value but restart didn't happened. And small bugfix: node can't rewind from themself. --- patroni/postgresql.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 50d57c3a..aa3adf0c 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -154,7 +154,7 @@ class Postgresql(object): def reload_config(self, config): server_parameters = self.get_server_parameters(config) - listen_address_changed = pending_reload = False + listen_address_changed = pending_reload = pending_restart = False if self.is_healthy(): changes = {p: v for p, v in server_parameters.items() if '.' not in p} changes.update({p: None for p, v in self._server_parameters.items() if not ('.' in p or p in changes)}) @@ -177,7 +177,7 @@ class Postgresql(object): new_value = changes.pop(r[0]) if new_value is None or not compare_values(r[3], unit, r[1], new_value): if r[4] == 'postmaster': - self._pending_restart = True + pending_restart = True if r[0] in ('listen_addresses', 'port'): listen_address_changed = True else: @@ -200,6 +200,7 @@ class Postgresql(object): break self.config = config + self._pending_restart = pending_restart self._server_parameters = server_parameters self._connect_address = config.get('connect_address') @@ -718,7 +719,7 @@ class Postgresql(object): need_rewind = change_role and self.can_rewind if need_rewind: logger.info("set the rewind flag after demote") - if leader and need_rewind: # we have a leader and need to rewind + if leader and leader.name != self.name and need_rewind: # 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