From f7d60c61b6cd107c9e81fa9fd05fdb692067e64e Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 17 Feb 2016 14:09:00 +0100 Subject: [PATCH 1/9] remove unused code --- patroni/postgresql.py | 3 +-- tests/test_postgresql.py | 22 +++++++--------------- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index f5f3d37c..893503e2 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -526,8 +526,7 @@ recovery_target_timeline = 'latest' result[name] = val except IOError: logger.exception('Error when reading postmaster.opts') - finally: - return result + 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 """ diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 4c701c60..5558437b 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -2,21 +2,16 @@ import mock # for the mock.call method, importing it without a namespace breaks import os import psycopg2 import shutil +import subprocess import unittest -from six.moves import builtins 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 from patroni.utils import RetryFailedError +from six.moves import builtins 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(object): @@ -283,7 +278,7 @@ class TestPostgresql(unittest.TestCase): 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("foo")): + with mock.patch('subprocess.call', side_effect=OSError): self.assertFalse(self.p.can_rewind) tmp = self.p.controldata self.p.controldata = lambda: {'wal_log_hints setting': 'on'} @@ -292,7 +287,7 @@ class TestPostgresql(unittest.TestCase): @patch('time.sleep', Mock()) def test_create_replica(self): - self.p.delete_trigger_file = Mock(side_effect=OSError()) + self.p.delete_trigger_file = Mock(side_effect=OSError) with patch('subprocess.call', Mock(side_effect=[1, 0])): self.assertEquals(self.p.create_replica(self.leader, ''), 0) with patch('subprocess.call', Mock(side_effect=[Exception(), 0])): @@ -349,7 +344,7 @@ class TestPostgresql(unittest.TestCase): def test_last_operation(self): self.assertEquals(self.p.last_operation(), '0') - @patch('subprocess.Popen', Mock(side_effect=OSError())) + @patch('subprocess.Popen', Mock(side_effect=OSError)) def test_call_nowait(self): self.assertFalse(self.p.call_nowait('on_start')) @@ -369,7 +364,7 @@ class TestPostgresql(unittest.TestCase): def test_move_data_directory(self): self.p.is_running = false self.p.move_data_directory() - with patch('os.rename', Mock(side_effect=OSError())): + with patch('os.rename', Mock(side_effect=OSError)): self.p.move_data_directory() @patch('patroni.postgresql.Postgresql.write_pgpass', MagicMock(return_value=dict())) @@ -411,13 +406,10 @@ class TestPostgresql(unittest.TestCase): self.assertEquals(int(data['max_replication_slots']), 5) self.assertEqual(data.get('D'), None) - m.side_effect = IOError("foo") + m.side_effect = IOError data = self.p.read_postmaster_opts() self.assertEqual(data, dict()) - m.side_effect = Exception("foo") - self.assertRaises(Exception, self.p.read_postmaster_opts()) - @patch('subprocess.Popen') @patch.object(builtins, 'open', MagicMock(return_value=42)) def test_single_user_mode(self, subprocess_popen_mock): From a210cfd1abde26a56f6fae93c13c7c72d720a4f7 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 17 Feb 2016 14:51:59 +0100 Subject: [PATCH 2/9] Fix more codacy issues --- tests/test_api.py | 16 ++++++++-------- tests/test_ctl.py | 43 ++++++++++++++++++------------------------- tests/test_utils.py | 6 +++--- 3 files changed, 29 insertions(+), 36 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index 2faee523..265394e8 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -87,7 +87,7 @@ class MockRestApiServer(RestApiServer): @patch('ssl.wrap_socket', Mock(return_value=0)) class TestRestApiHandler(unittest.TestCase): - def test_do_GET(self): + def test_do_GET(*args): MockRestApiServer(RestApiHandler, b'GET /replica') with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={})): MockRestApiServer(RestApiHandler, b'GET /replica') @@ -103,7 +103,7 @@ class TestRestApiHandler(unittest.TestCase): MockRestApiServer(RestApiHandler, b'GET /master') MockRestApiServer(RestApiHandler, b'GET /master') - def test_do_OPTIONS(self): + def test_do_OPTIONS(*args): MockRestApiServer(RestApiHandler, b'OPTIONS / HTTP/1.0') with patch.object(BaseHTTPRequestHandler, 'handle_one_request') as mock_handle_request: @@ -117,14 +117,14 @@ class TestRestApiHandler(unittest.TestCase): makefile.return_value.flush = Mock(side_effect=socket.error("foo")) MockRestApiServer(RestApiHandler, b'OPTIONS / HTTP/1.0') - def test_do_GET_patroni(self): + def test_do_GET_patroni(*args): MockRestApiServer(RestApiHandler, b'GET /patroni') - def test_basicauth(self): + def test_basicauth(*args): MockRestApiServer(RestApiHandler, b'POST /restart HTTP/1.0') MockRestApiServer(RestApiHandler, b'POST /restart HTTP/1.0\nAuthorization:') - def test_do_POST_restart(self): + def test_do_POST_restart(*args): request = b'POST /restart HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' MockRestApiServer(RestApiHandler, request) with patch.object(MockHa, 'restart', Mock(side_effect=Exception)): @@ -140,10 +140,10 @@ class TestRestApiHandler(unittest.TestCase): with patch.object(MockHa, 'schedule_reinitialize', Mock(return_value=None)): MockRestApiServer(RestApiHandler, request) cluster.leader.name = 'test' - MockRestApiServer(RestApiHandler, request) + self.assertIsNotNone(MockRestApiServer(RestApiHandler, request)) @patch('time.sleep', Mock()) - def test_RestApiServer_query(self): + def test_RestApiServer_query(*args): with patch.object(MockCursor, 'execute', Mock(side_effect=psycopg2.OperationalError)): MockRestApiServer(RestApiHandler, b'GET /patroni') with patch.object(MockPostgresql, 'connection', Mock(side_effect=psycopg2.OperationalError)): @@ -175,4 +175,4 @@ class TestRestApiHandler(unittest.TestCase): 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) + self.assertIsNotNone(MockRestApiServer(RestApiHandler, request)) diff --git a/tests/test_ctl.py b/tests/test_ctl.py index fb63aebd..12ab4ad8 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -69,20 +69,16 @@ class TestCtl(unittest.TestCase): @patch('psycopg2.connect', psycopg2_connect) def test_get_cursor(self): - c = get_cursor(get_cluster_initialized_without_leader(), role='master') - assert c is None + self.assertIsNone(get_cursor(get_cluster_initialized_without_leader(), role='master')) - c = get_cursor(get_cluster_initialized_with_leader(), role='master') - assert c is not None + self.assertIsNotNone(get_cursor(get_cluster_initialized_with_leader(), role='master')) - c = get_cursor(get_cluster_initialized_with_leader(), role='replica') - # # MockCursor returns pg_is_in_recovery as false - assert c is None + # MockCursor returns pg_is_in_recovery as false + self.assertIsNone(get_cursor(get_cluster_initialized_with_leader(), role='replica')) - c = get_cursor(get_cluster_initialized_with_leader(), role='any') - assert c is not None + self.assertIsNotNone(get_cursor(get_cluster_initialized_with_leader(), role='any')) - def test_output_members(self): + def test_output_members(*args): cluster = get_cluster_initialized_with_leader() output_members(cluster, name='abc', fmt='pretty') output_members(cluster, name='abc', fmt='json') @@ -224,17 +220,17 @@ y''') @patch('patroni.ctl.get_cursor', Mock(return_value=MockConnect().cursor())) def test_query_member(self): rows = query_member(None, None, None, 'master', 'SELECT pg_is_in_recovery()') - assert 'False' in str(rows) + self.assertTrue('False' in str(rows)) rows = query_member(None, None, None, 'replica', 'SELECT pg_is_in_recovery()') - assert rows == (None, None) + self.assertEquals(rows, (None, None)) with patch('patroni.ctl.get_cursor', Mock(return_value=None)): rows = query_member(None, None, None, None, 'SELECT pg_is_in_recovery()') - assert 'No connection to' in str(rows) + self.assertTrue('No connection to' in str(rows)) rows = query_member(None, None, None, 'replica', 'SELECT pg_is_in_recovery()') - assert 'No connection to' in str(rows) + self.assertTrue('No connection to' in str(rows)) with patch('patroni.ctl.get_cursor', Mock(side_effect=psycopg2.OperationalError('bla'))): rows = query_member(None, None, None, 'replica', 'SELECT pg_is_in_recovery()') @@ -337,26 +333,23 @@ leader''') assert 'Usage:' in result.output def test_get_any_member(self): - m = get_any_member(get_cluster_initialized_without_leader(), role='master') - assert m is None + self.assertIsNone(get_any_member(get_cluster_initialized_without_leader(), role='master')) m = get_any_member(get_cluster_initialized_with_leader(), role='master') - assert m.name == 'leader' + self.assertEquals(m.name, 'leader') def test_get_all_members(self): - r = list(get_all_members(get_cluster_initialized_without_leader(), role='master')) - assert len(r) == 0 + self.assertEquals(list(get_all_members(get_cluster_initialized_without_leader(), role='master')), []) r = list(get_all_members(get_cluster_initialized_with_leader(), role='master')) - assert len(r) == 1 - assert r[0].name == 'leader' + self.assertEquals(len(r), 1) + self.assertEquals(r[0].name, 'leader') r = list(get_all_members(get_cluster_initialized_with_leader(), role='replica')) - assert len(r) == 1 - assert r[0].name == 'other' + self.assertEquals(len(r), 1) + self.assertEquals(r[0].name, 'other') - r = list(get_all_members(get_cluster_initialized_without_leader(), role='replica')) - assert len(r) == 2 + self.assertEquals(len(list(get_all_members(get_cluster_initialized_without_leader(), role='replica'))), 2) @patch('patroni.etcd.Etcd.get_cluster', Mock(return_value=get_cluster_initialized_with_leader())) @patch('patroni.etcd.Etcd.get_etcd_client', Mock(return_value=None)) diff --git a/tests/test_utils.py b/tests/test_utils.py index 6f66f4c3..740fef03 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -16,14 +16,14 @@ class TestUtils(unittest.TestCase): @patch('time.sleep', Mock()) def test_reap_children(self): - reap_children() + self.assertIsNone(reap_children()) with patch('os.waitpid', Mock(return_value=(0, 0))): sigchld_handler(None, None) - reap_children() + self.assertIsNone(reap_children()) @patch('time.sleep', time_sleep) def test_sleep(self): - sleep(0.01) + self.assertIsNone(sleep(0.01)) @patch('time.sleep', Mock()) From 4038d94c5ac61b345c5248d72a640e7bfd975073 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 17 Feb 2016 14:59:17 +0100 Subject: [PATCH 3/9] Fix more codacy issues --- tests/test_api.py | 26 +++++++++++++------------- tests/test_ctl.py | 8 ++++---- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index 265394e8..dd0f3dfa 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -87,7 +87,7 @@ class MockRestApiServer(RestApiServer): @patch('ssl.wrap_socket', Mock(return_value=0)) class TestRestApiHandler(unittest.TestCase): - def test_do_GET(*args): + def test_do_GET(self): MockRestApiServer(RestApiHandler, b'GET /replica') with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={})): MockRestApiServer(RestApiHandler, b'GET /replica') @@ -101,10 +101,10 @@ class TestRestApiHandler(unittest.TestCase): MockRestApiServer(RestApiHandler, b'GET /master') with patch.object(MockHa, 'restart_scheduled', Mock(return_value=True)): MockRestApiServer(RestApiHandler, b'GET /master') - MockRestApiServer(RestApiHandler, b'GET /master') + self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'GET /master')) - def test_do_OPTIONS(*args): - MockRestApiServer(RestApiHandler, b'OPTIONS / HTTP/1.0') + def test_do_OPTIONS(self): + self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'OPTIONS / HTTP/1.0')) with patch.object(BaseHTTPRequestHandler, 'handle_one_request') as mock_handle_request: mock_handle_request.side_effect = socket.error("foo") @@ -117,16 +117,16 @@ class TestRestApiHandler(unittest.TestCase): makefile.return_value.flush = Mock(side_effect=socket.error("foo")) MockRestApiServer(RestApiHandler, b'OPTIONS / HTTP/1.0') - def test_do_GET_patroni(*args): - MockRestApiServer(RestApiHandler, b'GET /patroni') + def test_do_GET_patroni(self): + self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'GET /patroni')) - def test_basicauth(*args): - MockRestApiServer(RestApiHandler, b'POST /restart HTTP/1.0') + def test_basicauth(self): + self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'POST /restart HTTP/1.0')) MockRestApiServer(RestApiHandler, b'POST /restart HTTP/1.0\nAuthorization:') - def test_do_POST_restart(*args): + def test_do_POST_restart(self): request = b'POST /restart HTTP/1.0\nAuthorization: Basic dGVzdDp0ZXN0' - MockRestApiServer(RestApiHandler, request) + self.assertIsNotNone(MockRestApiServer(RestApiHandler, request)) with patch.object(MockHa, 'restart', Mock(side_effect=Exception)): MockRestApiServer(RestApiHandler, request) @@ -143,11 +143,11 @@ class TestRestApiHandler(unittest.TestCase): self.assertIsNotNone(MockRestApiServer(RestApiHandler, request)) @patch('time.sleep', Mock()) - def test_RestApiServer_query(*args): + def test_RestApiServer_query(self): with patch.object(MockCursor, 'execute', Mock(side_effect=psycopg2.OperationalError)): - MockRestApiServer(RestApiHandler, b'GET /patroni') + self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'GET /patroni')) with patch.object(MockPostgresql, 'connection', Mock(side_effect=psycopg2.OperationalError)): - MockRestApiServer(RestApiHandler, b'GET /patroni') + self.assertIsNotNone(MockRestApiServer(RestApiHandler, b'GET /patroni')) @patch('time.sleep', Mock()) @patch.object(MockHa, 'dcs') diff --git a/tests/test_ctl.py b/tests/test_ctl.py index 12ab4ad8..8559e793 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -78,11 +78,11 @@ class TestCtl(unittest.TestCase): self.assertIsNotNone(get_cursor(get_cluster_initialized_with_leader(), role='any')) - def test_output_members(*args): + def test_output_members(self): cluster = get_cluster_initialized_with_leader() - output_members(cluster, name='abc', fmt='pretty') - output_members(cluster, name='abc', fmt='json') - output_members(cluster, name='abc', fmt='tsv') + self.assertIsNone(output_members(cluster, name='abc', fmt='pretty')) + self.assertIsNone(output_members(cluster, name='abc', fmt='json')) + self.assertIsNone(output_members(cluster, name='abc', fmt='tsv')) @patch('patroni.etcd.Etcd.get_cluster', Mock(return_value=get_cluster_initialized_with_leader())) @patch('patroni.etcd.Etcd.get_etcd_client', Mock(return_value=None)) From 641cc4013e76156dc27b22642499341a8cc703e9 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 23 Feb 2016 11:46:49 +0100 Subject: [PATCH 4/9] Mock a few of methods in Postgresql class instead of the whole class --- tests/test_ha.py | 100 +++++++++++++++++------------------------------ 1 file changed, 36 insertions(+), 64 deletions(-) diff --git a/tests/test_ha.py b/tests/test_ha.py index 3589e952..cdf1156f 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -1,13 +1,14 @@ -import etcd import unittest import datetime import pytz +from etcd import EtcdException from mock import Mock, MagicMock, patch from patroni.dcs import Cluster, Failover, Leader, Member from patroni.etcd import Client, Etcd from patroni.exceptions import DCSError, PostgresException from patroni.ha import Ha +from patroni.postgresql import Postgresql from test_etcd import socket_getaddrinfo, etcd_read, etcd_write, requests_get @@ -45,56 +46,6 @@ def get_cluster_initialized_with_only_leader(failover=None): return get_cluster(True, l, [l], failover) -class MockPostgresql(Mock): - - name = 'postgresql0' - role = 'replica' - state = 'running' - connection_string = 'postgres://foo@bar/postgres' - server_version = '999999' - scope = 'dummy' - - @staticmethod - def is_healthy(): - return True - - @staticmethod - def start(): - return True - - @staticmethod - def is_healthiest_node(members): - return True - - @staticmethod - def is_leader(): - return True - - @staticmethod - def xlog_position(): - return 0 - - @staticmethod - def last_operation(): - return 0 - - @staticmethod - def data_directory_empty(): - return False - - @staticmethod - def bootstrap(*args, **kwargs): - return True - - @staticmethod - def check_replication_lag(last_leader_operation): - return True - - @staticmethod - def check_recovery_conf(leader): - return False - - class MockPatroni(object): def __init__(self, p, d): @@ -111,18 +62,36 @@ def run_async(func, args=()): return func(*args) if args else func() +@patch.object(Postgresql, 'is_running', Mock(return_value=True)) +@patch.object(Postgresql, 'is_leader', Mock(return_value=True)) +@patch.object(Postgresql, 'xlog_position', Mock(return_value=0)) +@patch.object(Postgresql, 'call_nowait', Mock(return_value=True)) +@patch.object(Postgresql, 'data_directory_empty', Mock(return_value=False)) +@patch.object(Postgresql, 'controldata', Mock(return_value={})) +@patch.object(Postgresql, 'sync_replication_slots', Mock()) +@patch.object(Postgresql, 'write_pg_hba', Mock()) +@patch.object(Postgresql, 'write_pgpass', Mock()) +@patch.object(Postgresql, 'write_recovery_conf', Mock()) +@patch.object(Postgresql, 'query', Mock()) +@patch.object(Postgresql, 'checkpoint', Mock()) +@patch('subprocess.call', Mock(return_value=0)) class TestHa(unittest.TestCase): @patch('socket.getaddrinfo', socket_getaddrinfo) def setUp(self): with patch.object(Client, 'machines') as mock_machines: mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) - self.p = MockPostgresql() + self.p = Postgresql({'name': 'postgresql0', 'scope': 'dummy', 'listen': '127.0.0.1:5432', + 'data_dir': 'data/postgresql0', 'superuser': {}, 'admin': {}, + 'replication': {'username': '', 'password': '', 'network': ''}}) + self.p._state = 'running' + self.p._sysid = '1234567890' + self.p.check_replication_lag = true self.p.can_create_replica_without_leader = MagicMock(return_value=False) self.e = Etcd('foo', {'ttl': 30, 'host': 'ok:2379', 'scope': 'test'}) self.e.client.read = etcd_read self.e.client.write = etcd_write - self.e.client.delete = Mock(side_effect=etcd.EtcdException()) + self.e.client.delete = Mock(side_effect=EtcdException()) self.ha = Ha(MockPatroni(self.p, self.e)) self.ha._async_executor.run_async = run_async self.ha.old_cluster = self.e.get_cluster() @@ -154,7 +123,7 @@ class TestHa(unittest.TestCase): self.p.is_healthy = false self.p.is_running = false self.ha.has_lock = true - self.p.role = 'master' + self.p._role = 'master' self.p.controldata = lambda: {'Database cluster state': 'in production'} self.assertEquals(self.ha.run_cycle(), 'started as readonly because i had the session lock') self.assertEquals(self.ha.run_cycle(), 'removed leader key after trying and failing to start postgres') @@ -302,57 +271,60 @@ class TestHa(unittest.TestCase): self.ha.has_lock = true self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', '', None)) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') - self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, '', MockPostgresql.name, None)) + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, '', self.p.name, None)) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, '', 'blabla', None)) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') - f = Failover(0, MockPostgresql.name, '', None) + f = Failover(0, self.p.name, '', None) self.ha.cluster = get_cluster_initialized_with_leader(f) self.assertEquals(self.ha.run_cycle(), 'manual failover: demoting myself') self.ha.fetch_node_status = lambda e: (e, True, True, 0, {'nofailover': 'True'}) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') # manual failover from the previous leader to us won't happen if we hold the nofailover flag - self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', MockPostgresql.name, None)) + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', self.p.name, None)) self.assertEquals(self.ha.run_cycle(), 'no action. i am the leader with the lock') # Failover scheduled time must include timezone scheduled = datetime.datetime.now() - self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', MockPostgresql.name, scheduled)) + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', self.p.name, scheduled)) self.ha.run_cycle() scheduled = datetime.datetime.utcnow().replace(tzinfo=pytz.UTC) - self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', MockPostgresql.name, scheduled)) + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', self.p.name, scheduled)) self.assertEquals('no action. i am the leader with the lock', self.ha.run_cycle()) scheduled = scheduled + datetime.timedelta(seconds=30) - self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', MockPostgresql.name, scheduled)) + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', self.p.name, scheduled)) self.assertEquals('no action. i am the leader with the lock', self.ha.run_cycle()) scheduled = scheduled + datetime.timedelta(seconds=-600) - self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', MockPostgresql.name, scheduled)) + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', self.p.name, scheduled)) self.assertEquals('no action. i am the leader with the lock', self.ha.run_cycle()) scheduled = None - self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', MockPostgresql.name, scheduled)) + self.ha.cluster = get_cluster_initialized_with_leader(Failover(0, 'blabla', self.p.name, scheduled)) self.assertEquals('no action. i am the leader with the lock', self.ha.run_cycle()) @patch('requests.get', requests_get) def test_manual_failover_process_no_leader(self): self.p.is_leader = false - self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', MockPostgresql.name, None)) + self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', self.p.name, None)) self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', 'leader', None)) + self.p._role = 'replica' self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') self.ha.fetch_node_status = lambda e: (e, True, True, 0, {}) # accessible, in_recovery self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') - self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, MockPostgresql.name, '', None)) + self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, self.p.name, '', None)) self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') self.ha.fetch_node_status = lambda e: (e, False, True, 0, {}) # inaccessible, in_recovery + self.p._role = 'replica' self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') # set failover flag to True for all members of the cluster # this should elect the current member, as we are not going to call the API for it. self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', 'other', None)) self.ha.fetch_node_status = lambda e: (e, True, True, 0, {'nofailover': 'True'}) # accessible, in_recovery + self.p._role = 'replica' self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') # same as previous, but set the current member to nofailover. In no case it should be elected as a leader self.ha.patroni.nofailover = True From dd20fc7e71ad05423b65fa6d2c24e77a25888d44 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 23 Feb 2016 11:47:47 +0100 Subject: [PATCH 5/9] Refactor follow method --- patroni/postgresql.py | 76 +++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 893503e2..a1cedb58 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -559,47 +559,45 @@ recovery_target_timeline = 'latest' logger.exception("Unable to list %s", status_dir) def follow(self, leader, recovery=False): - if not self.check_recovery_conf(leader) or recovery: - change_role = (self.role == 'master') - - self._need_rewind = (self._need_rewind or change_role) and self.can_rewind - if self._need_rewind: - logger.info("set the rewind flag after demote") - self.write_recovery_conf(leader) - if not leader or not self._need_rewind: # do not rewind until the leader becomes available - ret = self.restart() - else: # 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 - # and not shutdown in recovery. We have to remove the recovery.conf if present - # and start/shutdown in a single user mode to emulate this. - # 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: - 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 - opts = self.read_postmaster_opts() - opts['archive_mode'] = 'on' - opts['archive_command'] = 'false' - self.single_user_mode(options=opts) - if self.rewind(leader): - ret = self.start() - else: - logger.error("unable to rewind the former master") - self.remove_data_directory() - ret = True - self._need_rewind = False - if change_role and ret: - self.call_nowait(ACTION_ON_ROLE_CHANGE) - return ret - else: + if self.check_recovery_conf(leader) and not recovery: return True + change_role = self.role == 'master' + self._need_rewind = (self._need_rewind or change_role) and self.can_rewind + if self._need_rewind: + logger.info("set the rewind flag after demote") + self.write_recovery_conf(leader) + if leader and self._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 + # and not shutdown in recovery. We have to remove the recovery.conf if present + # and start/shutdown in a single user mode to emulate this. + # 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: + 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 + opts = self.read_postmaster_opts() + opts.update({'archive_mode': 'on', 'archive_command': 'false'}) + self.single_user_mode(options=opts) + if self.rewind(leader): + ret = self.start() + else: + logger.error("unable to rewind the former master") + self.remove_data_directory() + ret = True + self._need_rewind = False + else: # do not rewind until the leader becomes available + ret = self.restart() + if change_role and ret: + self.call_nowait(ACTION_ON_ROLE_CHANGE) + return ret + def save_configuration_files(self): """ copy postgresql.conf to postgresql.conf.backup to be able to retrive configuration files From ce33090c0d4d0665ab80788e106b4059dec5bd99 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 23 Feb 2016 11:48:52 +0100 Subject: [PATCH 6/9] Mock dcs.watch directly instead of using wraper --- tests/test_patroni.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/test_patroni.py b/tests/test_patroni.py index aaefdf1b..2698de58 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -15,10 +15,6 @@ from test_postgresql import Postgresql, psycopg2_connect from test_zookeeper import MockKazooClient -def time_sleep(*args): - raise SleepException() - - @patch('time.sleep', Mock()) @patch('subprocess.call', Mock(return_value=0)) @patch('psycopg2.connect', psycopg2_connect) @@ -62,7 +58,7 @@ class TestPatroni(unittest.TestCase): @patch('time.sleep', Mock(side_effect=SleepException())) def test_run(self): - self.p.ha.dcs.watch = time_sleep + self.p.ha.dcs.watch = Mock(side_effect=SleepException()) self.assertRaises(SleepException, self.p.run) self.p.ha.state_handler.is_leader = Mock(return_value=False) From 6b3c4697fc36409280911e378d7c146854bbb08e Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 23 Feb 2016 11:49:22 +0100 Subject: [PATCH 7/9] Remove unused code --- tests/test_ctl.py | 40 ++++++++++++++-------------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/tests/test_ctl.py b/tests/test_ctl.py index a877ed4b..b2590da5 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -1,25 +1,19 @@ -#!/usr/bin/env python -# -*- coding: utf-8 -*- - import os import pytest import unittest -import psycopg2 -import requests -import patroni.exceptions -import etcd -from mock import patch, Mock, MagicMock - from click.testing import CliRunner +from etcd import EtcdException +from mock import patch, Mock, MagicMock from patroni.ctl import ctl, members, store_config, load_config, output_members, post_patroni, get_dcs, \ wait_for_leader, get_all_members, get_any_member, get_cursor, query_member, configure -from patroni.ha import Ha from patroni.etcd import Etcd, Client -from test_ha import get_cluster_initialized_without_leader, get_cluster_initialized_with_leader, \ - get_cluster_initialized_with_only_leader, MockPostgresql, MockPatroni, run_async, \ - get_cluster_not_initialized_without_leader +from patroni.exceptions import PatroniCtlException +from psycopg2 import OperationalError +from requests.exceptions import ConnectionError from test_etcd import etcd_read, etcd_write, requests_get, socket_getaddrinfo, MockResponse +from test_ha import get_cluster_initialized_without_leader, get_cluster_initialized_with_leader, \ + get_cluster_initialized_with_only_leader from test_postgresql import MockConnect, psycopg2_connect CONFIG_FILE_PATH = './test-ctl.yaml' @@ -56,16 +50,10 @@ class TestCtl(unittest.TestCase): self.runner = CliRunner() with patch.object(Client, 'machines') as mock_machines: mock_machines.__get__ = Mock(return_value=['http://remotehost:2379']) - self.p = MockPostgresql() self.e = Etcd('foo', {'ttl': 30, 'host': 'ok:2379', 'scope': 'test'}) self.e.client.read = etcd_read self.e.client.write = etcd_write - self.e.client.delete = Mock(side_effect=etcd.EtcdException()) - self.ha = Ha(MockPatroni(self.p, self.e)) - self.ha._async_executor.run_async = run_async - self.ha.old_cluster = self.e.get_cluster() - self.ha.cluster = get_cluster_not_initialized_without_leader() - self.ha.load_cluster_from_dcs = Mock() + self.e.client.delete = Mock(side_effect=EtcdException) @patch('psycopg2.connect', psycopg2_connect) def test_get_cursor(self): @@ -186,7 +174,7 @@ y''') assert 'Failover failed' in result.output def test_(self): - self.assertRaises(patroni.exceptions.PatroniCtlException, get_dcs, {'scheme': 'dummy'}, 'dummy') + self.assertRaises(PatroniCtlException, get_dcs, {'scheme': 'dummy'}, 'dummy') @patch('psycopg2.connect', psycopg2_connect) @patch('patroni.ctl.query_member', Mock(return_value=([['mock column']], None))) @@ -248,10 +236,10 @@ y''') rows = query_member(None, None, None, 'replica', 'SELECT pg_is_in_recovery()') self.assertTrue('No connection to' in str(rows)) - with patch('patroni.ctl.get_cursor', Mock(side_effect=psycopg2.OperationalError('bla'))): + with patch('patroni.ctl.get_cursor', Mock(side_effect=OperationalError('bla'))): rows = query_member(None, None, None, 'replica', 'SELECT pg_is_in_recovery()') - with patch('test_postgresql.MockCursor.execute', Mock(side_effect=psycopg2.OperationalError('bla'))): + with patch('test_postgresql.MockCursor.execute', Mock(side_effect=OperationalError('bla'))): rows = query_member(None, None, None, 'replica', 'SELECT pg_is_in_recovery()') @patch('patroni.dcs.AbstractDCS.get_cluster', Mock(return_value=get_cluster_initialized_with_leader())) @@ -341,15 +329,15 @@ leader''') @patch('patroni.etcd.Etcd.get_cluster', Mock(return_value=get_cluster_initialized_with_leader())) def test_wait_for_leader(self): dcs = self.e - self.assertRaises(patroni.exceptions.PatroniCtlException, wait_for_leader, dcs, 0) + self.assertRaises(PatroniCtlException, wait_for_leader, dcs, 0) cluster = wait_for_leader(dcs=dcs, timeout=2) assert cluster.leader.member.name == 'leader' def test_post_patroni(self): - with patch('requests.post', MagicMock(side_effect=requests.exceptions.ConnectionError('foo'))): + with patch('requests.post', MagicMock(side_effect=ConnectionError('foo'))): member = get_cluster_initialized_with_leader().leader.member - self.assertRaises(requests.exceptions.ConnectionError, post_patroni, member, 'dummy', {}) + self.assertRaises(ConnectionError, post_patroni, member, 'dummy', {}) def test_ctl(self): self.runner.invoke(ctl, ['list']) From 756158a735efd3cbb127e5749fc82b8b232a1d74 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 23 Feb 2016 11:59:02 +0100 Subject: [PATCH 8/9] make codacy and quantifiedcode happier --- tests/test_ctl.py | 6 +++--- tests/test_ha.py | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_ctl.py b/tests/test_ctl.py index b2590da5..3987619d 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -1,5 +1,6 @@ import os import pytest +import requests.exceptions import unittest from click.testing import CliRunner @@ -10,7 +11,6 @@ from patroni.ctl import ctl, members, store_config, load_config, output_members, from patroni.etcd import Etcd, Client from patroni.exceptions import PatroniCtlException from psycopg2 import OperationalError -from requests.exceptions import ConnectionError from test_etcd import etcd_read, etcd_write, requests_get, socket_getaddrinfo, MockResponse from test_ha import get_cluster_initialized_without_leader, get_cluster_initialized_with_leader, \ get_cluster_initialized_with_only_leader @@ -335,9 +335,9 @@ leader''') assert cluster.leader.member.name == 'leader' def test_post_patroni(self): - with patch('requests.post', MagicMock(side_effect=ConnectionError('foo'))): + with patch('requests.post', MagicMock(side_effect=requests.exceptions.ConnectionError('foo'))): member = get_cluster_initialized_with_leader().leader.member - self.assertRaises(ConnectionError, post_patroni, member, 'dummy', {}) + self.assertRaises(requests.exceptions.ConnectionError, post_patroni, member, 'dummy', {}) def test_ctl(self): self.runner.invoke(ctl, ['list']) diff --git a/tests/test_ha.py b/tests/test_ha.py index cdf1156f..57754aa6 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -84,7 +84,7 @@ class TestHa(unittest.TestCase): self.p = Postgresql({'name': 'postgresql0', 'scope': 'dummy', 'listen': '127.0.0.1:5432', 'data_dir': 'data/postgresql0', 'superuser': {}, 'admin': {}, 'replication': {'username': '', 'password': '', 'network': ''}}) - self.p._state = 'running' + self.p.set_state('running') self.p._sysid = '1234567890' self.p.check_replication_lag = true self.p.can_create_replica_without_leader = MagicMock(return_value=False) @@ -123,7 +123,7 @@ class TestHa(unittest.TestCase): self.p.is_healthy = false self.p.is_running = false self.ha.has_lock = true - self.p._role = 'master' + self.p.set_role('master') self.p.controldata = lambda: {'Database cluster state': 'in production'} self.assertEquals(self.ha.run_cycle(), 'started as readonly because i had the session lock') self.assertEquals(self.ha.run_cycle(), 'removed leader key after trying and failing to start postgres') @@ -311,20 +311,20 @@ class TestHa(unittest.TestCase): self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', self.p.name, None)) self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', 'leader', None)) - self.p._role = 'replica' + self.p.set_role('replica') self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') self.ha.fetch_node_status = lambda e: (e, True, True, 0, {}) # accessible, in_recovery self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, self.p.name, '', None)) self.assertEquals(self.ha.run_cycle(), 'following a different leader because i am not the healthiest node') self.ha.fetch_node_status = lambda e: (e, False, True, 0, {}) # inaccessible, in_recovery - self.p._role = 'replica' + self.p.set_role('replica') self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') # set failover flag to True for all members of the cluster # this should elect the current member, as we are not going to call the API for it. self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, '', 'other', None)) self.ha.fetch_node_status = lambda e: (e, True, True, 0, {'nofailover': 'True'}) # accessible, in_recovery - self.p._role = 'replica' + self.p.set_role('replica') self.assertEquals(self.ha.run_cycle(), 'promoted self to leader by acquiring session lock') # same as previous, but set the current member to nofailover. In no case it should be elected as a leader self.ha.patroni.nofailover = True From ec85e2eb4908a7fa1b50257e1de8cfd2d81f9a23 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 23 Feb 2016 12:05:02 +0100 Subject: [PATCH 9/9] make quantifiedcode happier --- tests/test_ha.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_ha.py b/tests/test_ha.py index 57754aa6..a65a444a 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -67,7 +67,7 @@ def run_async(func, args=()): @patch.object(Postgresql, 'xlog_position', Mock(return_value=0)) @patch.object(Postgresql, 'call_nowait', Mock(return_value=True)) @patch.object(Postgresql, 'data_directory_empty', Mock(return_value=False)) -@patch.object(Postgresql, 'controldata', Mock(return_value={})) +@patch.object(Postgresql, 'controldata', Mock(return_value={'Database system identifier': '1234567890'})) @patch.object(Postgresql, 'sync_replication_slots', Mock()) @patch.object(Postgresql, 'write_pg_hba', Mock()) @patch.object(Postgresql, 'write_pgpass', Mock()) @@ -85,7 +85,6 @@ class TestHa(unittest.TestCase): 'data_dir': 'data/postgresql0', 'superuser': {}, 'admin': {}, 'replication': {'username': '', 'password': '', 'network': ''}}) self.p.set_state('running') - self.p._sysid = '1234567890' self.p.check_replication_lag = true self.p.can_create_replica_without_leader = MagicMock(return_value=False) self.e = Etcd('foo', {'ttl': 30, 'host': 'ok:2379', 'scope': 'test'})