From 342eec5c2f8bfb59cbf7636f869657d30e670751 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 25 May 2016 20:50:28 +0200 Subject: [PATCH] Bugfix: pg_rewind can work only with master --- patroni/ha.py | 6 +++--- patroni/postgresql.py | 16 +++++++--------- tests/test_postgresql.py | 20 ++++++++++---------- 3 files changed, 20 insertions(+), 22 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 1e825d4f..1d51c93f 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -71,7 +71,7 @@ class Ha(object): logger.info('bootstrapped %s', msg) 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: logger.error('failed to bootstrap %s', msg) self.state_handler.remove_data_directory() @@ -134,7 +134,7 @@ class Ha(object): if not self.state_handler.check_recovery_conf(node_to_follow) or recovery: self._async_executor.schedule('changing primary_conninfo and restarting') - self._async_executor.run_async(self.state_handler.follow, (node_to_follow, recovery)) + self._async_executor.run_async(self.state_handler.follow, (node_to_follow, self.cluster.leader, recovery)) return ret def enforce_master_role(self, message, promote_message): @@ -278,7 +278,7 @@ class Ha(object): sleep(2) # Give a time to somebody to promote self.recover() else: - self.state_handler.follow(None) + self.state_handler.follow(None, None) def process_manual_failover_from_leader(self): failover = self.cluster.failover diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 060932c7..d314b1c5 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -511,12 +511,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 """ @@ -561,14 +558,13 @@ class Postgresql(object): except OSError: logger.exception("Unable to list %s", status_dir) - def follow(self, leader, recovery=False): - if self.check_recovery_conf(leader) and not recovery: + def follow(self, member, leader, recovery=False): + if self.check_recovery_conf(member) and not recovery: return True change_role = self.role == 'master' 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() @@ -578,7 +574,7 @@ 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. @@ -586,12 +582,14 @@ class Postgresql(object): # Start in a single user mode and stop to produce a clean shutdown self.single_user_mode(options={'archive_mode': 'on', 'archive_command': 'false'}) if self.rewind(leader): + self.write_recovery_conf(member) 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(member) ret = self.restart() if change_role and ret: self.call_nowait(ACTION_ON_ROLE_CHANGE) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index f4b34ec0..2c03faa9 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -248,22 +248,22 @@ class TestPostgresql(unittest.TestCase): @patch('patroni.postgresql.Postgresql.write_pgpass', MagicMock(return_value=dict())) @patch('subprocess.check_output', Mock(return_value=0, side_effect=pg_controldata_string)) def test_follow(self, mock_pg_rewind): - self.p.follow(None) - self.p.follow(self.leader) - self.p.follow(Leader(-1, 28, self.other)) + self.p.follow(None, None) + self.p.follow(self.leader, self.leader) + self.p.follow(Leader(-1, 28, self.other), self.leader) self.p.rewind = mock_pg_rewind - self.p.follow(self.leader) + self.p.follow(self.leader, self.leader) with mock.patch('os.path.islink', MagicMock(return_value=True)): with mock.patch('patroni.postgresql.Postgresql.can_rewind', new_callable=PropertyMock(return_value=True)): with mock.patch('os.unlink', MagicMock(return_value=True)): - self.p.follow(self.leader, recovery=True) + self.p.follow(self.leader, self.leader, recovery=True) with mock.patch('patroni.postgresql.Postgresql.can_rewind', new_callable=PropertyMock(return_value=True)): self.p.rewind.return_value = True - self.p.follow(self.leader, recovery=True) + self.p.follow(self.leader, self.leader, recovery=True) self.p.rewind.return_value = False - self.p.follow(self.leader, recovery=True) + self.p.follow(self.leader, self.leader, recovery=True) with mock.patch('patroni.postgresql.Postgresql.check_recovery_conf', MagicMock(return_value=True)): - self.assertTrue(self.p.follow(None)) + self.assertTrue(self.p.follow(None, None)) @patch('subprocess.check_output', Mock(return_value=0, side_effect=pg_controldata_string)) def test_can_rewind(self): @@ -423,7 +423,7 @@ class TestPostgresql(unittest.TestCase): def test_cleanup_archive_status(self, mock_file, mock_link, mock_remove, mock_unlink): ap = os.path.join(self.data_dir, 'pg_xlog', 'archive_status/') self.p.cleanup_archive_status() - mock_remove.assert_has_calls([mock.call(ap+'a'), mock.call(ap+'b'), mock.call(ap+'c')]) + mock_remove.assert_has_calls([mock.call(ap + 'a'), mock.call(ap + 'b'), mock.call(ap + 'c')]) mock_unlink.assert_not_called() mock_remove.reset_mock() @@ -431,7 +431,7 @@ class TestPostgresql(unittest.TestCase): mock_file.return_value = False mock_link.return_value = True self.p.cleanup_archive_status() - mock_unlink.assert_has_calls([mock.call(ap+'a'), mock.call(ap+'b'), mock.call(ap+'c')]) + mock_unlink.assert_has_calls([mock.call(ap + 'a'), mock.call(ap + 'b'), mock.call(ap + 'c')]) mock_remove.assert_not_called() mock_unlink.reset_mock()