From a89a902f4a8754e38d5c02c6f538d285dc3e0386 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 10 Nov 2017 11:48:50 +0100 Subject: [PATCH] Bump version and write release notes (#560) and implement missing unit-tests --- docs/releases.rst | 41 ++++++++++++++++++++++++++++++++++++++++ patroni/postgresql.py | 39 +++++++++++++++++++------------------- patroni/version.py | 2 +- tests/test_postgresql.py | 41 ++++++++++++++++++++++++++++++++++------ 4 files changed, 97 insertions(+), 26 deletions(-) diff --git a/docs/releases.rst b/docs/releases.rst index fbf35572..8a1a3ff7 100644 --- a/docs/releases.rst +++ b/docs/releases.rst @@ -3,6 +3,47 @@ Release notes ============= +Version 1.3.6 +------------- + +**Stability improvements** + +- Verify process start time when checking if postgres is running. (Ants Aasma) + + After a crash that doesn't clean up postmaster.pid there could be a new process with the same pid, resulting in a false positive for is_running(), which will lead to all kinds of bad behavior. + +- Shutdown postgresql before bootstrap when we lost data directory (ainlolcat) + + When data directory on the master is forcefully removed, postgres process can still stay alive for some time and prevent the replica created in place of that former master from starting or replicating. + The fix makes Patroni cache the postmaster pid and its start time and let it terminate the old postmaster in case it is still running after the corresponding data directory has been removed. + +- Perform crash recovery in a single user mode if postgres master dies (Alexander Kukushkin) + + It is unsafe to start immediately as a standby and not possible to run ``pg_rewind`` if postgres hasn't been shut down cleanly. + The single user crash recovery only kicks in if ``pg_rewind`` is enabled or there is no master at the moment. + +**Consul improvements** + +- Make it possible to provide datacenter configuration for Consul (DeathBorn, Alexander) + + Before that Patroni was always communicating with datacenter of the host it runs on. + +- Always send a token in X-Consul-Token http header (Alexander) + + If ``consul.token`` is defined in Patroni configuration, we will always send it in the 'X-Consul-Token' http header. + python-consul module tries to be "consistent" with Consul REST API, which doesn't accept token as a query parameter for `session API `__, but it still works with 'X-Consul-Token' header. + +- Adjust session TTL if supplied value is smaller than the minimum possible (Stas Fomin, Alexander) + + It could happen that the TTL provided in the Patroni configuration is smaller than the minimum one supported by Consul. In that case, Consul agent fails to create a new session. + Without a session Patroni cannot create member and leader keys in the Consul KV store, resulting in an unhealthy cluster. + +**Other improvements** + +- Define custom log format via environment variable ``PATRONI_LOGFORMAT`` (Stas) + + Allow disabling timestamps and other similar fields in Patroni logs if they are already added by the system logger (usually when Patroni runs as a service). + Version 1.3.5 ------------- diff --git a/patroni/postgresql.py b/patroni/postgresql.py index 43ca1c97..fee17f63 100644 --- a/patroni/postgresql.py +++ b/patroni/postgresql.py @@ -71,18 +71,18 @@ def null_context(): yield -def _update_postmaster_info(func): +def _update_postmaster_cached_info(func): def wrapper(self): ret = func(self) if ret and 'pid' in ret and 'start_time' in ret: - old_pid = self._postmaster_info.get('pid', 0) - old_start_time = self._postmaster_info.get('start_time', 0) + old_pid = self._postmaster_cached_info.get('pid', 0) + old_start_time = self._postmaster_cached_info.get('start_time', 0) try: pmpid = int(ret['pid']) pmstart = int(ret['start_time']) if pmpid != old_pid or pmstart != old_start_time: # this check removes repeating messages from logs - self._postmaster_info = {'pid': pmpid, 'start_time': pmstart} - logger.info("Updated postmaster info: %s .", self._postmaster_info) + self._postmaster_cached_info = {'pid': pmpid, 'start_time': pmstart} + logger.info("Updated postmaster info: %s .", self._postmaster_cached_info) except ValueError: logger.warning('Cannot update postmaster info with data due garbage in pid file: %s', ret) return ret @@ -161,7 +161,7 @@ class Postgresql(object): self._pg_hba_conf = os.path.join(self._config_dir, 'pg_hba.conf') self._recovery_conf = os.path.join(self._data_dir, 'recovery.conf') self._postmaster_pid = os.path.join(self._data_dir, 'postmaster.pid') - self._postmaster_info = {'pid': 0, 'start_time': 0} + self._postmaster_cached_info = {'pid': 0, 'start_time': 0} self._trigger_file = config.get('recovery_conf', {}).get('trigger_file') or 'promote' self._trigger_file = os.path.abspath(os.path.join(self._data_dir, self._trigger_file)) @@ -732,7 +732,7 @@ class Postgresql(object): return self._is_postmaster_pid_running(int_or_none(pidfile.get('pid')), start_time=int_or_none(pidfile.get('start_time'))) - @_update_postmaster_info + @_update_postmaster_cached_info def read_pid_file(self): """Reads and parses postmaster.pid from the data directory @@ -759,21 +759,21 @@ class Postgresql(object): def get_pid_with_lost_data_dir(self): logger.info("Trying to check if process running without directory " - "with cached postmaster info: %s .", self._postmaster_info) + "with cached postmaster info: %s .", self._postmaster_cached_info) try: - process = psutil.Process(self._postmaster_info['pid']) + process = psutil.Process(self._postmaster_cached_info['pid']) # check difference instead of values because of rounding issues - if abs(self._postmaster_info["start_time"] - process.create_time()) < 2: + if abs(self._postmaster_cached_info["start_time"] - process.create_time()) < 2: return process.pid else: logger.info("Process with pid %s was started at different time %s .", process.pid, process.create_time()) except psutil.NoSuchProcess: - logger.info("Cannot find process %s .", self._postmaster_info['pid']) + logger.info("Cannot find process %s .", self._postmaster_cached_info['pid']) return 0 - def clean_postmaster_info(self): - self._postmaster_info = {'pid': 0, 'start_time': 0} + def clean_postmaster_cached_info(self): + self._postmaster_cached_info = {'pid': 0, 'start_time': 0} logger.info("postmaster info was cleaned.") @staticmethod @@ -991,11 +991,12 @@ class Postgresql(object): def _do_stop(self, mode, block_callbacks, checkpoint, on_safepoint): if not self.is_running(): - pid = self.get_pid_with_lost_data_dir() - if pid > 0: - self.terminate_starting_postmaster(pid) - self.clean_postmaster_info() - return True, True + if self.data_directory_empty() and self._postmaster_cached_info['pid']: + pid = self.get_pid_with_lost_data_dir() + if pid > 0: + self.terminate_starting_postmaster(pid) + self.clean_postmaster_cached_info() + return True, True if on_safepoint: on_safepoint() return True, False @@ -1021,7 +1022,7 @@ class Postgresql(object): on_safepoint() self._wait_for_postmaster_stop(pid) - self.clean_postmaster_info() + self.clean_postmaster_cached_info() return True, True diff --git a/patroni/version.py b/patroni/version.py index 5b8f37aa..594fa812 100644 --- a/patroni/version.py +++ b/patroni/version.py @@ -1 +1 @@ -__version__ = '1.3.5' +__version__ = '1.3.6' diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 460a21e6..4c6a997b 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -277,6 +277,21 @@ class TestPostgresql(unittest.TestCase): mock_is_running.return_value = False self.assertTrue(self.p.stop(on_safepoint=mock_callback)) mock_callback.assert_called() + + with patch.object(Postgresql, '_is_postmaster_pid_running', Mock(return_value=False)), \ + patch.object(Postgresql, 'data_directory_empty', Mock(return_value=True)): + with patch('psutil.Process') as mock_psutil: + self.p._postmaster_cached_info = {'pid': 1, 'start_time': 1} + mock_psutil.return_value.pid = 1 + mock_psutil.return_value.create_time.return_value = 1 + self.assertTrue(self.p.stop()) + self.p._postmaster_cached_info = {'pid': 1, 'start_time': 1} + mock_psutil.return_value.create_time.return_value = 100 + self.assertTrue(self.p.stop()) + self.p._postmaster_cached_info = {'pid': 1, 'start_time': 1} + mock_psutil.side_effect = psutil.NoSuchProcess('') + self.assertTrue(self.p.stop()) + mock_is_running.return_value = True mock_get_pid.return_value = 0 mock_callback.reset_mock() @@ -773,13 +788,19 @@ class TestPostgresql(unittest.TestCase): os.remove(pidfile) self.assertEquals(self.p.read_pid_file(), {}) + @patch.object(Postgresql, '_version_file_exists', Mock(return_value=True)) + @patch('os.path.isfile', Mock(return_value=True)) + @patch.object(Postgresql, 'read_pid_file') @patch('psutil.Process') - def test_is_postmaster_pid_running(self, mock_psutil): - mock_proc = Mock() - mock_psutil.return_value = mock_proc - self.assertTrue(self.p._is_postmaster_pid_running(-100)) - self.assertFalse(self.p._is_postmaster_pid_running(0)) - self.assertFalse(self.p._is_postmaster_pid_running(None)) + def test_is_postmaster_pid_running(self, mock_psutil, mock_read_pid_file): + mock_psutil.return_value.create_time.return_value = 1 + mock_read_pid_file.return_value = {'pid': -100, 'start_time': 1} + self.assertTrue(self.p.is_running()) + with patch('os.getpid', Mock(return_value=100)): + mock_read_pid_file.return_value = {'pid': 100, 'start_time': 1} + self.assertFalse(self.p.is_running()) + mock_read_pid_file.return_value = {'pid': 100, 'start_time': 100} + self.assertFalse(self.p.is_running()) def test_pick_sync_standby(self): cluster = Cluster(True, None, self.leader, 0, [self.me, self.other, self.leadermem], None, @@ -924,3 +945,11 @@ class TestPostgresql(unittest.TestCase): @patch.object(Postgresql, 'single_user_mode', Mock(return_value=0)) def test_fix_cluster_state(self): self.assertTrue(self.p.fix_cluster_state()) + + def test__update_postmaster_cached_info(self): + with open(os.path.join(self.data_dir, 'postmaster.pid'), 'w') as f: + f.write('1\n\n1\n') + self.p.read_pid_file() + with open(os.path.join(self.data_dir, 'postmaster.pid'), 'w') as f: + f.write('a\n\n1\n') + self.p.read_pid_file()