From 2adf593fb873675e25f6b556904affc770a8b73b Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 9 May 2016 09:31:29 +0200 Subject: [PATCH 1/4] finish method does not have any arguments --- patroni/api.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/patroni/api.py b/patroni/api.py index 0bda86ed..526da8b4 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -46,9 +46,10 @@ class RestApiHandler(BaseHTTPRequestHandler): self.wfile.write(body.encode('utf-8')) def send_auth_request(self, body): - self._write_response(401, body, {'WWW-Authenticate': 'Basic realm=\"Patroni\"'}) + headers = {'WWW-Authenticate': 'Basic realm="' + self.server.patroni.__class__.__name__ + '"'} + self._write_response(401, body, headers) - def finish(self, *args, **kwargs): + def finish(self): try: if not self.wfile.closed: self.wfile.flush() From defc987328ed379dacdf212800e1cc694e5718c2 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 9 May 2016 09:33:54 +0200 Subject: [PATCH 2/4] Encode request body only once in a MockRequest to avoid using bytestrings all over the file --- tests/test_api.py | 73 +++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index 308ddf8e..5871d240 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -59,67 +59,67 @@ class MockPatroni(object): class MockRequest(object): - def __init__(self, path): - self.path = path + def __init__(self, request): + self.request = request.encode('utf-8') def makefile(self, *args, **kwargs): - return IO(self.path) + return IO(self.request) class MockRestApiServer(RestApiServer): - def __init__(self, Handler, path): + def __init__(self, Handler, request): self.socket = 0 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'} super(MockRestApiServer, self).__init__(MockPatroni(), config) - Handler(MockRequest(path), ('0.0.0.0', 8080), self) + Handler(MockRequest(request), ('0.0.0.0', 8080), self) @patch('ssl.wrap_socket', Mock(return_value=0)) class TestRestApiHandler(unittest.TestCase): def test_do_GET(self): - MockRestApiServer(RestApiHandler, b'GET /replica') + MockRestApiServer(RestApiHandler, 'GET /replica') with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={})): - MockRestApiServer(RestApiHandler, b'GET /replica') + MockRestApiServer(RestApiHandler, 'GET /replica') with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={'role': 'master'})): - MockRestApiServer(RestApiHandler, b'GET /replica') - MockRestApiServer(RestApiHandler, b'GET /master') + MockRestApiServer(RestApiHandler, 'GET /replica') + MockRestApiServer(RestApiHandler, 'GET /master') MockPatroni.dcs.cluster.leader.name = MockPostgresql.name - MockRestApiServer(RestApiHandler, b'GET /replica') + MockRestApiServer(RestApiHandler, 'GET /replica') MockPatroni.dcs.cluster = None with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={'role': 'master'})): - MockRestApiServer(RestApiHandler, b'GET /master') + MockRestApiServer(RestApiHandler, 'GET /master') with patch.object(MockHa, 'restart_scheduled', Mock(return_value=True)): - MockRestApiServer(RestApiHandler, b'GET /master') - self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'GET /master')) + MockRestApiServer(RestApiHandler, 'GET /master') + self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'GET /master')) def test_do_OPTIONS(self): - self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'OPTIONS / HTTP/1.0')) + 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, b'OPTIONS / HTTP/1.0') + 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, b'OPTIONS / HTTP/1.0') + MockRestApiServer(RestApiHandler, 'OPTIONS / HTTP/1.0') def test_do_GET_patroni(self): - self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'GET /patroni')) + self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'GET /patroni')) def test_basicauth(self): - self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'POST /restart HTTP/1.0')) - MockRestApiServer(RestApiHandler, b'POST /restart HTTP/1.0\nAuthorization:') + self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'POST /restart HTTP/1.0')) + MockRestApiServer(RestApiHandler, 'POST /restart HTTP/1.0\nAuthorization:') def test_do_POST_restart(self): - request = b'POST /restart HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' + request = 'POST /restart HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' self.assertIsNotNone(MockRestApiServer(RestApiHandler, request)) with patch.object(MockHa, 'restart', Mock(side_effect=Exception)): MockRestApiServer(RestApiHandler, request) @@ -127,7 +127,7 @@ class TestRestApiHandler(unittest.TestCase): @patch.object(MockHa, 'dcs') def test_do_POST_reinitialize(self, dcs): cluster = dcs.get_cluster.return_value - request = b'POST /reinitialize HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' + request = 'POST /reinitialize HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' MockRestApiServer(RestApiHandler, request) cluster.is_unlocked.return_value = False MockRestApiServer(RestApiHandler, request) @@ -139,29 +139,28 @@ class TestRestApiHandler(unittest.TestCase): @patch('time.sleep', Mock()) def test_RestApiServer_query(self): with patch.object(MockCursor, 'execute', Mock(side_effect=psycopg2.OperationalError)): - self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'GET /patroni')) + self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'GET /patroni')) with patch.object(MockPostgresql, 'connection', Mock(side_effect=psycopg2.OperationalError)): - self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'GET /patroni')) + self.assertIsNotNone(MockRestApiServer(RestApiHandler, '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: 0\n\n' + request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\nContent-Length: 0\n\n' MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'postgresql1' MockRestApiServer(RestApiHandler, request) - request = b'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\n' +\ - b'Content-Length: 25\n\n{"leader": "postgresql1"}' + request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\n' +\ + 'Content-Length: 25\n\n{"leader": "postgresql1"}' MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'postgresql2' - request = b'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\n' +\ - b'Content-Length: 53\n\n{"leader": "postgresql1", "candidate": "postgresql2"}' + request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\n' +\ + 'Content-Length: 53\n\n{"leader": "postgresql1", "candidate": "postgresql2"}' MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'postgresql1' @@ -187,24 +186,24 @@ class TestRestApiHandler(unittest.TestCase): MockRestApiServer(RestApiHandler, request) # Valid future date - request = b'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\nContent-Length: 103\n\n{"leader": ' +\ - b'"postgresql1", "member": "postgresql2", "scheduled_at": "6016-02-15T18:13:30.568224+01:00"}' + request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\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: d.manual_failover.return_value = False MockRestApiServer(RestApiHandler, request) # Exception: No timezone specified - request = b'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\nContent-Length: 97\n\n{"leader": ' +\ - b'"postgresql1", "member": "postgresql2", "scheduled_at": "6016-02-15T18:13:30.568224"}' + request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\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 = b'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\nContent-Length: 103\n\n{"leader": ' +\ - b'"postgresql1", "member": "postgresql2", "scheduled_at": "1016-02-15T18:13:30.568224+01:00"}' + request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\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 = b'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\nContent-Length: 103\n\n{"leader": ' +\ - b'"postgresql1", "member": "postgresql2", "scheduled_at": "2010-02-29T18:13:30.568224+01:00"}' + request = 'POST /failover HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0\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 73119f96aa54563efb34cd645e340d9dabcec219 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 9 May 2016 09:47:32 +0200 Subject: [PATCH 3/4] Set application_name to node name in primary_conninfo It will make it simplier to identify node the in pg_stat_replication view --- patroni/postgresql.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index e771bcfb..c8b7a8a9 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -450,10 +450,11 @@ class Postgresql(object): 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', [])))) - @staticmethod - def primary_conninfo(leader_url): + def primary_conninfo(self, leader_url): r = parseurl(leader_url) - return 'user={user} password={password} host={host} port={port} sslmode=prefer sslcompression=1'.format(**r) + r.update({'application_name': self.name, 'sslmode': 'prefer', 'sslcompression': '1'}) + keywords = 'user password host port sslmode sslcompression application_name'.split() + return ' '.join('{0}={{{0}}}'.format(kw) for kw in keywords).format(**r) def check_recovery_conf(self, leader): if not os.path.isfile(self.recovery_conf): From edf372e8b697e30dbf32346d133d23efd7bddbda Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 9 May 2016 09:41:06 +0200 Subject: [PATCH 4/4] Reset _sysid and don't call pg_controldata when restore of backup in progress Otherwise there were some errors in a log from rest-api healthcheck endpoint --- patroni/postgresql.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index e771bcfb..4178d572 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -261,6 +261,8 @@ class Postgresql(object): for replica_method in replica_methods) def create_replica(self, clone_member, env): + self.set_state('creating replica') + self._sysid = None # create the replica according to the replica_method # defined by the user. this is a list, so we need to # loop through all methods the user supplies @@ -308,6 +310,7 @@ class Postgresql(object): logger.exception('Error creating replica using method {0}: {1}'.format(replica_method, str(e))) ret = 1 + self.set_state('stopped') return ret def is_leader(self): @@ -501,13 +504,14 @@ class Postgresql(object): def controldata(self): """ return the contents of pg_controldata, or non-True value if pg_controldata call failed """ result = {} - try: - data = subprocess.check_output(['pg_controldata', self.data_dir]) - if data: - data = data.decode('utf-8').splitlines() - result = {l.split(':')[0].replace('Current ', '', 1): l.split(':')[1].strip() for l in data if l} - except subprocess.CalledProcessError: - logger.exception("Error when calling pg_controldata") + if self.state != 'creating replica': # Don't try to call pg_controldata during backup restore + try: + data = subprocess.check_output(['pg_controldata', self.data_dir]) + if data: + data = data.decode('utf-8').splitlines() + result = {l.split(':')[0].replace('Current ', '', 1): l.split(':')[1].strip() for l in data if l} + except subprocess.CalledProcessError: + logger.exception("Error when calling pg_controldata") return result def read_postmaster_opts(self):