From 828585079f5b5a2312dfa0152bb64374e6e98996 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 25 Oct 2019 14:09:44 +0200 Subject: [PATCH] Improve workflow when PGDATA is not empty during bootstrap (#1217) Recently it has happened two times when people tried to deploy the new cluster but postgres data directory wasn't empty and also wasn't valid. In this case Patroni was still creating initialize key in DCS and trying to start the postgres up. Now it will complain about non-empty invalid postgres data directory and exit. Close https://github.com/zalando/patroni/issues/1216 --- patroni/ha.py | 27 ++++++++++++++------------- patroni/postgresql/__init__.py | 5 +++-- tests/__init__.py | 1 + tests/test_bootstrap.py | 4 ++++ tests/test_postgresql.py | 2 ++ 5 files changed, 24 insertions(+), 15 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 1002f440..6218c87f 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -1257,15 +1257,6 @@ class Ha(object): return 'released leader key voluntarily as data dir empty and currently leader' return self.bootstrap() # new node - # "bootstrap", but data directory is not empty - elif not self.sysid_valid(self.cluster.initialize) and self.cluster.is_unlocked() and not self.is_paused(): - if not self.state_handler.cb_called and self.state_handler.is_running() \ - and not self.state_handler.is_leader(): - self._join_aborted = True - logger.error('No initialize key in DCS and PostgreSQL is running as replica, aborting start') - logger.error('Please first start Patroni on the node running as master') - sys.exit(1) - self.dcs.initialize(create_new=(self.cluster.initialize is None), sysid=self.state_handler.sysid) else: # check if we are allowed to join data_sysid = self.state_handler.sysid @@ -1273,10 +1264,20 @@ class Ha(object): # data directory is not empty, but no valid sysid, cluster must be broken, suggest reinit return "data dir for the cluster is not empty, but system ID is invalid; consider doing reinitalize" - if self.sysid_valid(self.cluster.initialize) and self.cluster.initialize != self.state_handler.sysid: - logger.fatal("system ID mismatch, node %s belongs to a different cluster: %s != %s", - self.state_handler.name, self.cluster.initialize, self.state_handler.sysid) - sys.exit(1) + if self.sysid_valid(self.cluster.initialize): + if self.cluster.initialize != data_sysid: + logger.fatal("system ID mismatch, node %s belongs to a different cluster: %s != %s", + self.state_handler.name, self.cluster.initialize, data_sysid) + sys.exit(1) + elif self.cluster.is_unlocked() and not self.is_paused(): + # "bootstrap", but data directory is not empty + if not self.state_handler.cb_called and self.state_handler.is_running() \ + and not self.state_handler.is_leader(): + self._join_aborted = True + logger.error('No initialize key in DCS and PostgreSQL is running as replica, aborting start') + logger.error('Please first start Patroni on the node running as master') + sys.exit(1) + self.dcs.initialize(create_new=(self.cluster.initialize is None), sysid=data_sysid) if not self.state_handler.is_healthy(): if self.is_paused(): diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py index ffb5c7d2..932b6b6d 100644 --- a/patroni/postgresql/__init__.py +++ b/patroni/postgresql/__init__.py @@ -208,7 +208,7 @@ class Postgresql(object): return self._sysid def get_postgres_role_from_data_directory(self): - if self.data_directory_empty(): + if self.data_directory_empty() or not self.controldata(): return 'uninitialized' elif self.config.recovery_conf_exists(): return 'replica' @@ -257,7 +257,8 @@ class Postgresql(object): raise PostgresConnectionException(str(e)) def data_directory_empty(self): - return not os.path.exists(self._data_dir) or os.listdir(self._data_dir) == [] + return not os.path.exists(self._data_dir) or \ + all(os.name != 'nt' and (n.startswith('.') or n == 'lost+found') for n in os.listdir(self._data_dir)) def replica_method_options(self, method): return deepcopy(self.config.get(method, {})) diff --git a/tests/__init__.py b/tests/__init__.py index 2f2118ba..319bc0ee 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -181,6 +181,7 @@ class PostgresInit(unittest.TestCase): @patch.object(ConfigHandler, 'write_postgresql_conf', Mock()) @patch.object(ConfigHandler, 'replace_pg_hba', Mock()) @patch.object(ConfigHandler, 'replace_pg_ident', Mock()) + @patch.object(Postgresql, 'get_postgres_role_from_data_directory', Mock(return_value='master')) def setUp(self): data_dir = 'data/test0' self.p = Postgresql({'name': 'postgresql0', 'scope': 'batman', 'data_dir': data_dir, diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index f0b438ce..f00dbe58 100644 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -101,6 +101,9 @@ class TestBootstrap(BaseTestPostgresql): @patch.object(CancellableSubprocess, 'call', Mock()) @patch.object(Postgresql, 'is_running', Mock(return_value=True)) @patch.object(Postgresql, 'data_directory_empty', Mock(return_value=False)) + @patch.object(Postgresql, 'controldata', Mock(return_value={'max_connections setting': 100, + 'max_prepared_xacts setting': 0, + 'max_locks_per_xact setting': 64})) def test_bootstrap(self): with patch('subprocess.call', Mock(return_value=1)): self.assertFalse(self.b.bootstrap({})) @@ -126,6 +129,7 @@ class TestBootstrap(BaseTestPostgresql): @patch.object(CancellableSubprocess, 'call') @patch.object(Postgresql, 'get_major_version', Mock(return_value=90600)) + @patch.object(Postgresql, 'controldata', Mock(return_value={'Database cluster state': 'in production'})) def test_custom_bootstrap(self, mock_cancellable_subprocess_call): self.p.config._config.pop('pg_hba') config = {'method': 'foo', 'foo': {'command': 'bar'}} diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 534519d1..62976faf 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -101,6 +101,7 @@ class TestPostgresql(BaseTestPostgresql): @patch.object(Postgresql, 'wait_for_startup') @patch.object(Postgresql, 'wait_for_port_open') @patch.object(Postgresql, 'is_running') + @patch.object(Postgresql, 'controldata', Mock()) def test_start(self, mock_is_running, mock_wait_for_port_open, mock_wait_for_startup, mock_popen): mock_is_running.return_value = MockPostmaster() mock_wait_for_port_open.return_value = True @@ -362,6 +363,7 @@ class TestPostgresql(BaseTestPostgresql): @patch('os.listdir', Mock(return_value=['recovery.conf'])) @patch('os.path.exists', Mock(return_value=True)) + @patch.object(Postgresql, 'controldata', Mock()) def test_get_postgres_role_from_data_directory(self): self.assertEqual(self.p.get_postgres_role_from_data_directory(), 'replica')