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

@@ -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:
+                if self.sysid_valid(self.cluster.initialize):
-                    logger.fatal("system ID mismatch, node %s belongs to a different cluster: %s != %s",
+                    if self.cluster.initialize != data_sysid:
-                                 self.state_handler.name, self.cluster.initialize, self.state_handler.sysid)
+                        logger.fatal("system ID mismatch, node %s belongs to a different cluster: %s != %s",
-                    sys.exit(1)
+                                     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():

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, {}))

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,

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'}}

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')