Repository: zalando/patroni
Pull request #2350 (2022-06-22 13:03:07)

Fix bug with GET /read-only-sync endpoint
effectively it never worked


Patch:
diff --git a/docs/SETTINGS.rst b/docs/SETTINGS.rst
index 9712f445..205f80dd 100644
--- a/docs/SETTINGS.rst
+++ b/docs/SETTINGS.rst
@@ -131,7 +131,7 @@ Most of the parameters are optional, but you have to specify one of the **host**
 -  **checks**: (optional) list of Consul health checks used for the session. By default an empty list is used.
 -  **register\_service**: (optional) whether or not to register a service with the name defined by the scope parameter and the tag master, replica or standby-leader depending on the node's role. Defaults to **false**.
 -  **service\_tags**: (optional) additional static tags to add to the Consul service apart from the role (``master``/``replica``/``standby-leader``).  By default an empty list is used.
--  **service\_check\_interval**: (optional) how often to perform health check against registered url.
+-  **service\_check\_interval**: (optional) how often to perform health check against registered url. Defaults to '5s'.
 -  **service\_check\_tls\_server\_name**: (optional) overide SNI host when connecting via TLS, see also `consul agent check API reference <https://www.consul.io/api-docs/agent/check#tlsservername>`__.
 
 The ``token`` needs to have the following ACL permissions:
diff --git a/docs/replica_bootstrap.rst b/docs/replica_bootstrap.rst
index a9ed7848..1524f7f2 100644
--- a/docs/replica_bootstrap.rst
+++ b/docs/replica_bootstrap.rst
@@ -212,4 +212,9 @@ in a patroni configuration:
 Note, that these options will be applied only once during cluster bootstrap,
 and the only way to change them afterwards is through DCS.
 
+Patroni expects to find `postgresql.conf` or `postgresql.conf.backup` in PGDATA
+of the remote master and will not start if it does not find it after a
+basebackup. If the remote master keeps its `postgresql.conf` elsewhere, it is
+your responsibility to copy it to PGDATA.
+
 If you use replication slots on the standby cluster, you must also create the corresponding replication slot on the primary cluster.  It will not be done automatically by the standby cluster implementation.  You can use Patroni's permanent replication slots feature on the primary cluster to maintain a replication slot with the same name as ``primary_slot_name``, or its default value if ``primary_slot_name`` is not provided.
diff --git a/features/environment.py b/features/environment.py
index f07e4729..3cb1c533 100644
--- a/features/environment.py
+++ b/features/environment.py
@@ -903,3 +903,11 @@ def after_feature(context, feature):
     context.dcs_ctl.cleanup_service_tree()
     if feature.status == 'failed':
         shutil.copytree(context.pctl.output_dir, context.pctl.output_dir + '_failed')
+
+
+def before_scenario(context, scenario):
+    if 'slot-advance' in scenario.effective_tags:
+        for p in context.pctl._processes.values():
+            if p._conn and p._conn.server_version < 110000:
+                scenario.skip('pg_replication_slot_advance() is not supported on {0}'.format(p._conn.server_version))
+                break
diff --git a/features/standby_cluster.feature b/features/standby_cluster.feature
index bad8c51a..82a800b0 100644
--- a/features/standby_cluster.feature
+++ b/features/standby_cluster.feature
@@ -14,7 +14,7 @@ Feature: standby cluster
     Then "members/postgres0" key in DCS has state=running after 10 seconds
     And replication works from postgres1 to postgres0 after 15 seconds
 
-  @skip
+  @slot-advance
   Scenario: check permanent logical slots are synced to the replica
     Given I run patronictl.py restart batman postgres1 --force
     Then Logical slot test_logical is in sync between postgres0 and postgres1 after 10 seconds
diff --git a/patroni/api.py b/patroni/api.py
index d2fd36b2..3b7b4192 100644
--- a/patroni/api.py
+++ b/patroni/api.py
@@ -141,7 +141,7 @@ class RestApiHandler(BaseHTTPRequestHandler):
             ignore_tags = True
         elif 'replica' in path:
             status_code = replica_status_code
-        elif 'read-only' in path:
+        elif 'read-only' in path and 'sync' not in path:
             status_code = 200 if 200 in (primary_status_code, standby_leader_status_code) else replica_status_code
         elif 'health' in path:
             status_code = 200 if response.get('state') == 'running' else 503
diff --git a/patroni/config.py b/patroni/config.py
index 5996f68e..0ae9726a 100644
--- a/patroni/config.py
+++ b/patroni/config.py
@@ -102,9 +102,9 @@ class Config(object):
             config_env = os.environ.pop(self.PATRONI_CONFIG_VARIABLE, None)
             self._local_configuration = config_env and yaml.safe_load(config_env) or self.__environment_configuration
         if validator:
-            error = validator(self._local_configuration)
-            if error:
-                raise ConfigParseError(error)
+            errors = validator(self._local_configuration)
+            if errors:
+                raise ConfigParseError("\n".join(errors))
 
         self.__effective_configuration = self._build_effective_configuration({}, self._local_configuration)
         self._data_dir = self.__effective_configuration.get('postgresql', {}).get('data_dir', "")
diff --git a/patroni/daemon.py b/patroni/daemon.py
index 5bb99990..ca118acf 100644
--- a/patroni/daemon.py
+++ b/patroni/daemon.py
@@ -1,3 +1,5 @@
+from __future__ import print_function
+
 import abc
 import os
 import signal
@@ -83,16 +85,18 @@ def abstract_main(cls, validator=None):
                         help='Patroni may also read the configuration from the {0} environment variable'
                         .format(Config.PATRONI_CONFIG_VARIABLE))
     args = parser.parse_args()
+    validate_config = validator and args.validate_config
     try:
-        if validator and args.validate_config:
+        if validate_config:
             Config(args.configfile, validator=validator)
             sys.exit()
 
         config = Config(args.configfile)
     except ConfigParseError as e:
         if e.value:
-            print(e.value)
-        parser.print_help()
+            print(e.value, file=sys.stderr)
+        if not validate_config:
+            parser.print_help()
         sys.exit(1)
 
     controller = cls(config)
diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py
index c52c397b..a5b3d592 100644
--- a/patroni/postgresql/__init__.py
+++ b/patroni/postgresql/__init__.py
@@ -428,11 +428,11 @@ class Postgresql(object):
                     # If the cluster is shutdown with archive_mode=on, WAL is switched before writing the checkpoint.
                     # In this case we want to take the LSN of previous record (switch) as the last known WAL location.
                     if parse_lsn(lsn) == prev and desc.strip() in ('xlog switch', 'SWITCH'):
-                        return str(prev)
+                        return prev
             except Exception as e:
                 logger.error('Exception when parsing WAL pg_%sdump output: %r', self.wal_name, e)
             if isinstance(checkpoint_lsn, six.integer_types):
-                return str(checkpoint_lsn)
+                return checkpoint_lsn
 
     def is_running(self):
         """Returns PostmasterProcess if one is running on the data directory or None. If most recently seen process
@@ -668,7 +668,7 @@ class Postgresql(object):
             while postmaster.is_running():
                 data = self.controldata()
                 if data.get('Database cluster state', '') == 'shut down':
-                    on_shutdown(int(self.latest_checkpoint_location()))
+                    on_shutdown(self.latest_checkpoint_location())
                     break
                 elif data.get('Database cluster state', '').startswith('shut down'):  # shut down in recovery
                     break
diff --git a/patroni/validator.py b/patroni/validator.py
index 6d2a060c..595d4355 100644
--- a/patroni/validator.py
+++ b/patroni/validator.py
@@ -178,9 +178,11 @@ class Schema(object):
         self.validator = validator
 
     def __call__(self, data):
+        errors = []
         for i in self.validate(data):
             if not i.status:
-                print(i)
+                errors.append(str(i))
+        return errors
 
     def validate(self, data):
         self.data = data


Test patch:
diff --git a/.github/workflows/run_tests.py b/.github/workflows/run_tests.py
index 04666996..3ca52b81 100644
--- a/.github/workflows/run_tests.py
+++ b/.github/workflows/run_tests.py
@@ -28,16 +28,15 @@ def main():
         version = versions.get(what)
         path = '/usr/lib/postgresql/{0}/bin:.'.format(version)
         unbuffer = ['timeout', '900', 'unbuffer']
-        args = ['--tags=-skip'] if what == 'etcd' else []
     else:
         path = os.path.abspath(os.path.join('pgsql', 'bin'))
         if sys.platform == 'darwin':
             path += ':.'
-        args = unbuffer = []
+        unbuffer = []
     env['PATH'] = path + os.pathsep + env['PATH']
     env['DCS'] = what
 
-    ret = subprocess.call(unbuffer + [sys.executable, '-m', 'behave'] + args, env=env)
+    ret = subprocess.call(unbuffer + [sys.executable, '-m', 'behave'], env=env)
 
     if ret != 0:
         if subprocess.call('grep . features/output/*_failed/*postgres?.*', shell=True) != 0:
diff --git a/tests/test_api.py b/tests/test_api.py
index 107bb638..01e5dc89 100644
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -169,7 +169,6 @@ class TestRestApiHandler(unittest.TestCase):
         MockRestApiServer(RestApiHandler, 'GET /replica?lag=10MB')
         MockRestApiServer(RestApiHandler, 'GET /replica?lag=10485760')
         MockRestApiServer(RestApiHandler, 'GET /read-only')
-        MockRestApiServer(RestApiHandler, 'GET /read-only-sync')
         with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={})):
             MockRestApiServer(RestApiHandler, 'GET /replica')
         with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={'role': 'master'})):
@@ -181,13 +180,13 @@ class TestRestApiHandler(unittest.TestCase):
         MockPatroni.dcs.cluster.is_synchronous_mode = Mock(return_value=True)
         with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={'role': 'replica'})):
             MockRestApiServer(RestApiHandler, 'GET /synchronous')
-        with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={'role': 'replica'})):
             MockRestApiServer(RestApiHandler, 'GET /read-only-sync')
         with patch.object(RestApiHandler, 'get_postgresql_status', Mock(return_value={'role': 'replica'})):
             MockPatroni.dcs.cluster.sync.members = []
             MockRestApiServer(RestApiHandler, 'GET /asynchronous')
         with patch.object(MockHa, 'is_leader', Mock(return_value=True)):
             MockRestApiServer(RestApiHandler, 'GET /replica')
+            MockRestApiServer(RestApiHandler, 'GET /read-only-sync')
             with patch.object(MockHa, 'is_standby_cluster', Mock(return_value=True)):
                 MockRestApiServer(RestApiHandler, 'GET /standby_leader')
         MockPatroni.dcs.cluster = None
diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py
index 3f9bd738..055ee2e0 100644
--- a/tests/test_postgresql.py
+++ b/tests/test_postgresql.py
@@ -347,7 +347,7 @@ class TestPostgresql(BaseTestPostgresql):
     @patch('subprocess.Popen')
     def test_latest_checkpoint_location(self, mock_popen):
         mock_popen.return_value.communicate.return_value = (None, None)
-        self.assertEqual(self.p.latest_checkpoint_location(), '28163096')
+        self.assertEqual(self.p.latest_checkpoint_location(), 28163096)
         # 9.3 and 9.4 format
         mock_popen.return_value.communicate.side_effect = [
             (b'rmgr: XLOG        len (rec/tot):     72/   104, tx:          0, lsn: 0/01ADBC18, prev 0/01ADBBB8, ' +
@@ -355,14 +355,14 @@ class TestPostgresql(BaseTestPostgresql):
              b' 1; offset 0; oldest xid 715 in DB 1; oldest multi 1 in DB 1; oldest running xid 0; shutdown', None),
             (b'rmgr: Transaction len (rec/tot):     64/    96, tx:        726, lsn: 0/01ADBBB8, prev 0/01ADBB70, ' +
              b'bkp: 0000, desc: commit: 2021-02-26 11:19:37.900918 CET; inval msgs: catcache 11 catcache 10', None)]
-        self.assertEqual(self.p.latest_checkpoint_location(), '28163096')
+        self.assertEqual(self.p.latest_checkpoint_location(), 28163096)
         mock_popen.return_value.communicate.side_effect = [
             (b'rmgr: XLOG        len (rec/tot):     72/   104, tx:          0, lsn: 0/01ADBC18, prev 0/01ADBBB8, ' +
              b'bkp: 0000, desc: checkpoint: redo 0/1ADBC18; tli 1; prev tli 1; fpw true; xid 0/727; oid 16386; multi' +
              b' 1; offset 0; oldest xid 715 in DB 1; oldest multi 1 in DB 1; oldest running xid 0; shutdown', None),
             (b'rmgr: XLOG        len (rec/tot):      0/    32, tx:          0, lsn: 0/01ADBBB8, prev 0/01ADBBA0, ' +
              b'bkp: 0000, desc: xlog switch ', None)]
-        self.assertEqual(self.p.latest_checkpoint_location(), '28163000')
+        self.assertEqual(self.p.latest_checkpoint_location(), 28163000)
         # 9.5+ format
         mock_popen.return_value.communicate.side_effect = [
             (b'rmgr: XLOG        len (rec/tot):    114/   114, tx:          0, lsn: 0/01ADBC18, prev 0/018260F8, ' +
@@ -371,7 +371,7 @@ class TestPostgresql(BaseTestPostgresql):
              b' oldest running xid 0; shutdown', None),
             (b'rmgr: XLOG        len (rec/tot):     24/    24, tx:          0, lsn: 0/018260F8, prev 0/01826080, ' +
              b'desc: SWITCH ', None)]
-        self.assertEqual(self.p.latest_checkpoint_location(), '25321720')
+        self.assertEqual(self.p.latest_checkpoint_location(), 25321720)
 
     def test_reload(self):
         self.assertTrue(self.p.reload())
diff --git a/tests/test_validator.py b/tests/test_validator.py
index a04403ce..830b415e 100644
--- a/tests/test_validator.py
+++ b/tests/test_validator.py
@@ -141,14 +141,14 @@ class TestValidator(unittest.TestCase):
         del directories[:]
 
     def test_empty_config(self, mock_out, mock_err):
-        schema({})
-        output = mock_out.getvalue()
+        errors = schema({})
+        output = "\n".join(errors)
         expected = list(sorted(['name', 'postgresql', 'restapi', 'scope'] + available_dcs))
         self.assertEqual(expected, parse_output(output))
 
     def test_complete_config(self, mock_out, mock_err):
-        schema(config)
-        output = mock_out.getvalue()
+        errors = schema(config)
+        output = "\n".join(errors)
         self.assertEqual(['postgresql.bin_dir', 'raft.bind_addr', 'raft.self_addr'], parse_output(output))
 
     def test_bin_dir_is_file(self, mock_out, mock_err):
@@ -158,8 +158,8 @@ class TestValidator(unittest.TestCase):
         c["restapi"]["connect_address"] = 'False:blabla'
         c["etcd"]["hosts"] = ["127.0.0.1:2379", "1244.0.0.1:2379", "127.0.0.1:invalidport"]
         c["kubernetes"]["pod_ip"] = "127.0.0.1111"
-        schema(c)
-        output = mock_out.getvalue()
+        errors = schema(c)
+        output = "\n".join(errors)
         self.assertEqual(['etcd.hosts.1', 'etcd.hosts.2', 'kubernetes.pod_ip', 'postgresql.bin_dir',
                           'postgresql.data_dir', 'raft.bind_addr', 'raft.self_addr',
                           'restapi.connect_address'], parse_output(output))
@@ -176,8 +176,8 @@ class TestValidator(unittest.TestCase):
         c["etcd"]["host"] = "127.0.0.1:237"
         c["postgresql"]["listen"] = "127.0.0.1:5432"
         with patch('patroni.validator.open', mock_open(read_data='9')):
-            schema(c)
-        output = mock_out.getvalue()
+            errors = schema(c)
+        output = "\n".join(errors)
         self.assertEqual(['consul.host', 'etcd.host', 'postgresql.bin_dir', 'postgresql.data_dir', 'postgresql.listen',
                           'raft.bind_addr', 'raft.self_addr', 'restapi.connect_address'], parse_output(output))
 
@@ -195,8 +195,8 @@ class TestValidator(unittest.TestCase):
         files.append(os.path.join(config["postgresql"]["bin_dir"], "postgres"))
         files.append(os.path.join(config["postgresql"]["bin_dir"], "pg_isready"))
         with patch('patroni.validator.open', mock_open(read_data='12')):
-            schema(config)
-        output = mock_out.getvalue()
+            errors = schema(config)
+        output = "\n".join(errors)
         self.assertEqual(['raft.bind_addr', 'raft.self_addr'], parse_output(output))
 
     @patch('subprocess.check_output', Mock(return_value=b"postgres (PostgreSQL) 12.1"))
@@ -210,8 +210,8 @@ class TestValidator(unittest.TestCase):
         c["etcd"]["hosts"] = []
         del c["postgresql"]["bin_dir"]
         with patch('patroni.validator.open', mock_open(read_data='11')):
-            schema(c)
-        output = mock_out.getvalue()
+            errors = schema(c)
+        output = "\n".join(errors)
         self.assertEqual(['etcd.hosts', 'postgresql.data_dir',
                           'raft.bind_addr', 'raft.self_addr'], parse_output(output))
 
@@ -224,8 +224,8 @@ class TestValidator(unittest.TestCase):
         c = copy.deepcopy(config)
         del c["postgresql"]["bin_dir"]
         with patch('patroni.validator.open', mock_open(read_data='11')):
-            schema(c)
-        output = mock_out.getvalue()
+            errors = schema(c)
+        output = "\n".join(errors)
         self.assertEqual(['postgresql.data_dir', 'raft.bind_addr', 'raft.self_addr'], parse_output(output))
 
     def test_data_dir_is_empty_string(self, mock_out, mock_err):
@@ -236,7 +236,7 @@ class TestValidator(unittest.TestCase):
         c["postgresql"]["pg_hba"] = ""
         c["postgresql"]["data_dir"] = ""
         c["postgresql"]["bin_dir"] = ""
-        schema(c)
-        output = mock_out.getvalue()
+        errors = schema(c)
+        output = "\n".join(errors)
         self.assertEqual(['kubernetes', 'postgresql.bin_dir', 'postgresql.data_dir',
                           'postgresql.pg_hba', 'raft.bind_addr', 'raft.self_addr'], parse_output(output))