From f35d1098102f484846f7eb10a15678e61646e822 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 15 Oct 2015 16:17:11 +0200 Subject: [PATCH 1/5] Bugfix: do not try to double encode data --- patroni/zookeeper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/zookeeper.py b/patroni/zookeeper.py index 6f8ab981..9a2ee8bb 100644 --- a/patroni/zookeeper.py +++ b/patroni/zookeeper.py @@ -198,7 +198,7 @@ class ZooKeeper(AbstractDCS): self.client.retry(self.client.set, self.failover_path, value.encode('utf-8'), version=index or -1) return True except NoNodeError: - return value == '' or (not index and self._create(self.failover_path, value.encode('utf-8'))) + return value == '' or (not index and self._create(self.failover_path, value)) except: logging.exception('set_failover_value') return False From 3ed82ae22c5f3ec7c22ad944338e4ea5c09ace3e Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 15 Oct 2015 16:18:28 +0200 Subject: [PATCH 2/5] Manual failover via rest api curl -XPOST --data '{"leader": "leader_name", "member": "member_name"}' http://127.0.0.1:8008/failover It will execute some preliminary checks and write failover key into DCS. Afterward it will wait until new leader key will appear in a DCS. It's better to execute this request on the master node. It will send a signal to the main HA loop which makes possible to release leader key immidiately even if you are working with etcd. --- patroni/api.py | 49 +++++++++++++++++++++++++++++++++++++++++++++++ tests/test_api.py | 26 +++++++++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/patroni/api.py b/patroni/api.py index dc83249f..f673d1c0 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -3,6 +3,7 @@ import fcntl import json import logging import psycopg2 +import time from patroni.exceptions import PostgresConnectionException from patroni.utils import Retry, RetryFailedError @@ -121,6 +122,54 @@ class RestApiHandler(BaseHTTPRequestHandler): self.end_headers() self.wfile.write(data) + def poll_failover_result(self, leader, member): + for a in range(0, 15): + time.sleep(1) + try: + cluster = self.server.patroni.dcs.get_cluster() + if cluster.leader and cluster.leader.name != leader: + return 200, ('Successfully failed over to ' + cluster.leader.name).encode('utf-8') + except: + pass + return 503, b'Failover failed' + + def is_failover_possible(self, cluster, leader, member): + if leader and not cluster.leader or cluster.leader.name != leader: + return b'leader name does not match' + if member: + members = [m for m in cluster.members if m.name == member] + if not members: + return b'member does not exists' + else: + members = [m for m in cluster.members if m.name != cluster.leader.name and m.api_url] + if not members: + return b'failover is not possible: cluster does not have members except leader' + for member, reachable, in_recovery, xlog_location in self.server.patroni.ha.fetch_nodes_statuses(members): + if reachable: + return None + return b'failover is not possible: no good candidates have been found' + + @check_auth + def do_POST_failover(self): + content_length = int(self.headers.get('content-length', 0)) + request = json.loads(self.rfile.read(content_length).decode('utf-8')) + leader = request.get('leader', None) + member = request.get('member', None) + cluster = self.server.patroni.ha.dcs.get_cluster() + status_code = 503 + data = self.is_failover_possible(cluster, leader, member) + if not data: + if not self.server.patroni.dcs.manual_failover(leader, member): + data = b'failed to write failover key into DCS' + else: + self.server.patroni.dcs.event.set() + status_code, data = self.poll_failover_result(cluster.leader and cluster.leader.name, member) + + self.send_response(status_code) + self.send_header('Content-Type', 'text/html') + self.end_headers() + self.wfile.write(data) + def parse_request(self): """Override parse_request method to enrich basic functionality of `BaseHTTPRequestHandler` class diff --git a/tests/test_api.py b/tests/test_api.py index e4b8f87e..aa4608c4 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -3,6 +3,7 @@ import unittest from mock import Mock, patch from patroni.api import RestApiHandler, RestApiServer +from patroni.dcs import Member from six import BytesIO as IO from six.moves import BaseHTTPServer from test_postgresql import psycopg2_connect, MockCursor @@ -38,6 +39,9 @@ class MockHa(Mock): def restart_scheduled(self): return False + def fetch_nodes_statuses(self, members): + return [[None, True, None, None]] + class MockPatroni: @@ -117,3 +121,25 @@ class TestRestApiHandler(unittest.TestCase): MockRestApiServer(RestApiHandler, b'GET /patroni') with patch.object(MockPostgresql, 'connection', Mock(side_effect=psycopg2.OperationalError)): MockRestApiServer(RestApiHandler, b'GET /patroni') + + @patch('time.sleep', Mock()) + @patch.object(MockHa, 'dcs') + def test_do_POST_failover(self, dcs): + cluster = dcs.get_cluster.return_value + request = b'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\n' +\ + b'Content-Length: 25\n\n{"leader": "postgresql1"}' + MockRestApiServer(RestApiHandler, request) + cluster.leader.name = 'postgresql1' + MockRestApiServer(RestApiHandler, request) + cluster.members = [Member(0, 'postgresql0', 30, {'api_url': 'http'})] + MockRestApiServer(RestApiHandler, request) + with patch.object(MockPatroni, 'dcs') as d: + d.get_cluster = Mock(side_effect=Exception()) + MockRestApiServer(RestApiHandler, request) + d.manual_failover.return_value = False + MockRestApiServer(RestApiHandler, request) + with patch.object(MockHa, 'fetch_nodes_statuses', Mock(return_value=[])): + MockRestApiServer(RestApiHandler, request) + request = b'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\n' +\ + b'Content-Length: 50\n\n{"leader": "postgresql1", "member": "postgresql2"}' + MockRestApiServer(RestApiHandler, request) From 18eebdadaa7ef40613d129981e3c9c532d3ef25c Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 19 Oct 2015 15:00:06 +0200 Subject: [PATCH 3/5] Watch for change of failover key. If the value is empty and leader didn't changed, this probably means that failover failed. After 15 seconds timeout we will consider failover status = unknown --- patroni/api.py | 4 +++- tests/test_api.py | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/patroni/api.py b/patroni/api.py index f673d1c0..1fa5ca62 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -129,9 +129,11 @@ class RestApiHandler(BaseHTTPRequestHandler): cluster = self.server.patroni.dcs.get_cluster() if cluster.leader and cluster.leader.name != leader: return 200, ('Successfully failed over to ' + cluster.leader.name).encode('utf-8') + if not cluster.failover: + return 503, b'Failover failed' except: pass - return 503, b'Failover failed' + return 503, b'Failover status unknown' def is_failover_possible(self, cluster, leader, member): if leader and not cluster.leader or cluster.leader.name != leader: diff --git a/tests/test_api.py b/tests/test_api.py index aa4608c4..ae25964d 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -134,6 +134,10 @@ class TestRestApiHandler(unittest.TestCase): cluster.members = [Member(0, 'postgresql0', 30, {'api_url': 'http'})] MockRestApiServer(RestApiHandler, request) with patch.object(MockPatroni, 'dcs') as d: + cluster = d.get_cluster.return_value + cluster.leader.name = 'postgresql1' + cluster.failover = None + MockRestApiServer(RestApiHandler, request) d.get_cluster = Mock(side_effect=Exception()) MockRestApiServer(RestApiHandler, request) d.manual_failover.return_value = False From 8f606e4ff9a85f6f6d76db744a48880ec940f827 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Mon, 19 Oct 2015 15:13:24 +0200 Subject: [PATCH 4/5] Add a missing call to restore_configuration_files. I accidentially removed the call when moving the backup functions to the external script. It is intended to save the configuration, so that at the restore phase one can just copy backup files. Its primary intention was to save configuration files in the WAL-E case (WAL-E just omits everything with .conf), but it is also useful in the pg_basebackup case, which omits all symlinks, leaving the cluster with .conf files symlinked in the broken state. --- patroni/postgresql.py | 17 +++++++++++------ tests/test_postgresql.py | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index f75324e7..b4e8ff94 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -485,19 +485,23 @@ recovery_target_timeline = 'latest' def save_configuration_files(self): """ - copy postgresql.conf to postgresql.conf.backup to preserve it in the WAL-e backup. - see http://comments.gmane.org/gmane.comp.db.postgresql.wal-e/239 + copy postgresql.conf to postgresql.conf.backup to be able to retrive configuration files + - originally stored as symlinks, those are normally skipped by pg_basebackup + - in case of WAL-E basebackup (see http://comments.gmane.org/gmane.comp.db.postgresql.wal-e/239) """ - for f in self.configuration_to_save: - shutil.copy(f, f + '.backup') + try: + for f in self.configuration_to_save: + os.path.isfile(f) and shutil.copy(f, f + '.backup') + except: + logger.exception('unable to create backup copies of configuration files') def restore_configuration_files(self): """ restore a previously saved postgresql.conf """ try: for f in self.configuration_to_save: - shutil.copy(f + '.backup', f) + not os.path.isfile(f) and os.path.isfile(f+'.backup') and shutil.copy(f + '.backup', f) except: - logger.exception('unable to restore configuration from WAL-E backup') + logger.exception('unable to restore configuration files from backup') def promote(self): if self.role == 'master': @@ -585,6 +589,7 @@ recovery_target_timeline = 'latest' raise PostgresException("Could not bootstrap master PostgreSQL") else: if self.sync_from_leader(current_leader): + self.restore_configuration_files() self.write_recovery_conf(current_leader) ret = self.start() return ret diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 9a02ece6..ca60760f 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -19,6 +19,11 @@ from test_ha import false import subprocess +def is_file_raise_on_backup(*args, **kwargs): + if args[0].endswith('.backup'): + raise Exception("foo") + + class MockCursor: def __init__(self, connection): @@ -429,3 +434,15 @@ class TestPostgresql(unittest.TestCase): self.p.cleanup_archive_status() mock_unlink.assert_not_called() mock_remove.assert_not_called() + + @patch('os.path.isfile', MagicMock(return_value=True)) + @patch('shutil.copy', side_effect=Exception) + def test_save_configuration_files(self, mock_copy): + shutil.copy = mock_copy + self.p.save_configuration_files() + + @patch('os.path.isfile', MagicMock(side_effect=is_file_raise_on_backup)) + @patch('shutil.copy', side_effect=Exception) + def test_restore_configuration_files(self, mock_copy): + shutil.copy = mock_copy + self.p.restore_configuration_files() From f53c968d8b17fc526c0883af70dfe99797dbb1ca Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 20 Oct 2015 14:36:49 +0200 Subject: [PATCH 5/5] Improve tests --- tests/test_api.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_api.py b/tests/test_api.py index ae25964d..72d5c2b0 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -135,6 +135,8 @@ class TestRestApiHandler(unittest.TestCase): MockRestApiServer(RestApiHandler, request) with patch.object(MockPatroni, 'dcs') as d: cluster = d.get_cluster.return_value + cluster.leader.name = 'postgresql0' + MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'postgresql1' cluster.failover = None MockRestApiServer(RestApiHandler, request)