From 0ead20f6a41437edb61a30272bae71207d21efbb Mon Sep 17 00:00:00 2001 From: Israel Date: Tue, 23 May 2023 12:05:53 -0300 Subject: [PATCH 01/12] Remove `__str__` method from `PatroniException`. (#2688) The `__str__` method was calling `repr` over the exception message, which was causing `print` calls to render not so nicely, e.g.: ``` $ patronictl show-config Error: 'Can not find suitable configuration of distributed configuration store\nAvailable implementations: consul, etcd, etcd3, exhibitor, kubernetes, raft, zookeeper' ``` By removing the `__str__` method we get a better rendering, e.g.: ``` $ patronictl show-config Error: Can not find suitable configuration of distributed configuration store Available implementations: consul, etcd, etcd3, exhibitor, kubernetes, raft, zookeeper ``` References: PAT-107. Signed-off-by: Israel Barth Rubio --- patroni/exceptions.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/patroni/exceptions.py b/patroni/exceptions.py index 88edfe07..f8bf6df1 100644 --- a/patroni/exceptions.py +++ b/patroni/exceptions.py @@ -8,13 +8,6 @@ class PatroniException(Exception): def __init__(self, value: Any) -> None: self.value = value - def __str__(self) -> str: - """ - >>> str(PatroniException('foo')) - "'foo'" - """ - return repr(self.value) - class PatroniFatalException(PatroniException): pass From 6c8a3b0d252634a68af569f4558e20888e8d3986 Mon Sep 17 00:00:00 2001 From: Polina Bungina <27892524+hughcapet@users.noreply.github.com> Date: Wed, 24 May 2023 09:01:56 +0200 Subject: [PATCH 02/12] Remove bootstrap.pg_hba (#2684) * Remove bootstrap.pg_hba * Extend docs for postgresql.pg_hba/pg_ident * Add postgresql.pg_hba/pg_ident to dynamic config docs --------- Co-authored-by: Alexander Kukushkin --- Dockerfile | 7 ++++--- Dockerfile.citus | 7 ++++--- docs/dynamic_configuration.rst | 10 ++++++++++ docs/yaml_configuration.rst | 16 ++++++---------- kubernetes/Dockerfile.citus | 4 +++- kubernetes/entrypoint.sh | 6 +++--- patroni/validator.py | 1 - postgres0.yml | 15 +++++++-------- postgres1.yml | 15 +++++++-------- postgres2.yml | 15 +++++++-------- tests/test_validator.py | 1 - 11 files changed, 51 insertions(+), 46 deletions(-) diff --git a/Dockerfile b/Dockerfile index 5892e1ef..4aa0ce5c 100644 --- a/Dockerfile +++ b/Dockerfile @@ -155,14 +155,15 @@ WORKDIR $PGHOME RUN sed -i 's/env python/&3/' /patroni*.py \ # "fix" patroni configs - && sed -i 's/^\( connect_address:\| - host\)/#&/' postgres?.yml \ && sed -i 's/^ listen: 127.0.0.1/ listen: 0.0.0.0/' postgres?.yml \ && sed -i "s|^\( data_dir: \).*|\1$PGDATA|" postgres?.yml \ && sed -i "s|^#\( bin_dir: \).*|\1$PGBIN|" postgres?.yml \ && sed -i 's/^ - encoding: UTF8/ - locale: en_US.UTF-8\n&/' postgres?.yml \ - && sed -i 's/^\(scope\|name\|etcd\| host\| authentication\| pg_hba\| parameters\):/#&/' postgres?.yml \ + && sed -i 's/^\(scope\|name\|etcd\| host\| authentication\| connect_address\| parameters\):/#&/' postgres?.yml \ && sed -i 's/^ \(replication\|superuser\|rewind\|unix_socket_directories\|\(\( \)\{0,1\}\(username\|password\)\)\):/#&/' postgres?.yml \ - && sed -i 's/^ parameters:/ pg_hba:\n - local all all trust\n - host replication all all md5\n - host all all all md5\n&\n max_connections: 100/' postgres?.yml \ + && sed -i 's/^ parameters:/&\n max_connections: 100/' postgres?.yml \ + && sed -i 's/^ pg_hba:/&\n - local all all trust/' postgres?.yml \ + && sed -i 's/^\(.*\) \(.*\) md5/\1 all md5/' postgres?.yml \ && if [ "$COMPRESS" = "true" ]; then chmod u+s /usr/bin/sudo; fi \ && chmod +s /bin/ping \ && chown -R postgres:postgres "$PGHOME" /run /etc/haproxy diff --git a/Dockerfile.citus b/Dockerfile.citus index b0920557..2a10745e 100644 --- a/Dockerfile.citus +++ b/Dockerfile.citus @@ -176,16 +176,17 @@ WORKDIR $PGHOME RUN sed -i 's/env python/&3/' /patroni*.py \ # "fix" patroni configs - && sed -i 's/^\( connect_address:\| - host\)/#&/' postgres?.yml \ && sed -i 's/^ listen: 127.0.0.1/ listen: 0.0.0.0/' postgres?.yml \ && sed -i "s|^\( data_dir: \).*|\1$PGDATA|" postgres?.yml \ && sed -i "s|^#\( bin_dir: \).*|\1$PGBIN|" postgres?.yml \ && sed -i 's/^ - encoding: UTF8/ - locale: en_US.UTF-8\n&/' postgres?.yml \ && sed -i 's/^scope:/log:\n loggers:\n patroni.postgresql.citus: DEBUG\n#&/' postgres?.yml \ - && sed -i 's/^\(name\|etcd\| host\| authentication\| pg_hba\| parameters\):/#&/' postgres?.yml \ + && sed -i 's/^\(name\|etcd\| host\| authentication\| connect_address\| parameters\):/#&/' postgres?.yml \ && sed -i 's/^ \(replication\|superuser\|rewind\|unix_socket_directories\|\(\( \)\{0,1\}\(username\|password\)\)\):/#&/' postgres?.yml \ && sed -i 's/^postgresql:/&\n basebackup:\n checkpoint: fast/' postgres?.yml \ - && sed -i 's|^ parameters:| pg_hba:\n - local all all trust\n - hostssl replication all all md5 clientcert=verify-ca\n - hostssl all all all md5 clientcert=verify-ca\n&\n max_connections: 100\n shared_buffers: 16MB\n ssl: "on"\n ssl_ca_file: /etc/ssl/certs/ssl-cert-snakeoil.pem\n ssl_cert_file: /etc/ssl/certs/ssl-cert-snakeoil.pem\n ssl_key_file: /etc/ssl/private/ssl-cert-snakeoil.key\n citus.node_conninfo: "sslrootcert=/etc/ssl/certs/ssl-cert-snakeoil.pem sslkey=/etc/ssl/private/ssl-cert-snakeoil.key sslcert=/etc/ssl/certs/ssl-cert-snakeoil.pem sslmode=verify-ca"|' postgres?.yml \ + && sed -i 's|^ parameters:|&\n max_connections: 100\n shared_buffers: 16MB\n ssl: "on"\n ssl_ca_file: /etc/ssl/certs/ssl-cert-snakeoil.pem\n ssl_cert_file: /etc/ssl/certs/ssl-cert-snakeoil.pem\n ssl_key_file: /etc/ssl/private/ssl-cert-snakeoil.key\n citus.node_conninfo: "sslrootcert=/etc/ssl/certs/ssl-cert-snakeoil.pem sslkey=/etc/ssl/private/ssl-cert-snakeoil.key sslcert=/etc/ssl/certs/ssl-cert-snakeoil.pem sslmode=verify-ca"|' postgres?.yml \ + && sed -i 's/^ pg_hba:/&\n - local all all trust/' postgres?.yml \ + && sed -i 's/^\(.*\) \(.*\) \(.*\) \(.*\) \(.*\) md5.*$/\1 hostssl \3 \4 all md5 clientcert=verify-ca/' postgres?.yml \ && sed -i 's/^#\(ctl\| certfile\| keyfile\)/\1/' postgres?.yml \ && sed -i 's|^# cafile: .*$| verify_client: required\n cafile: /etc/ssl/certs/ssl-cert-snakeoil.pem|' postgres?.yml \ && sed -i 's|^# cacert: .*$| cacert: /etc/ssl/certs/ssl-cert-snakeoil.pem|' postgres?.yml \ diff --git a/docs/dynamic_configuration.rst b/docs/dynamic_configuration.rst index 85121d7c..5486bf6e 100644 --- a/docs/dynamic_configuration.rst +++ b/docs/dynamic_configuration.rst @@ -26,6 +26,16 @@ In order to change the dynamic configuration you can use either ``patronictl edi - **recovery\_conf**: additional configuration settings written to recovery.conf when configuring follower. There is no recovery.conf anymore in PostgreSQL 12, but you may continue using this section, because Patroni handles it transparently. - **parameters**: list of configuration settings for Postgres. + - **pg\_hba**: list of lines that Patroni will use to generate ``pg_hba.conf``. Patroni ignores this parameter if ``hba_file`` PostgreSQL parameter is set to a non-default value. + + - **- host all all 0.0.0.0/0 md5** + - **- host replication replicator 127.0.0.1/32 md5**: A line like this is required for replication. + + - **pg\_ident**: list of lines that Patroni will use to generate ``pg_ident.conf``. Patroni ignores this parameter if ``ident_file`` PostgreSQL parameter is set to a non-default value. + + - **- mapname1 systemname1 pguser1** + - **- mapname1 systemname2 pguser2** + - **standby\_cluster**: if this section is defined, we want to bootstrap a standby cluster. - **host**: an address of remote node diff --git a/docs/yaml_configuration.rst b/docs/yaml_configuration.rst index 51f769ca..dd84f7cb 100644 --- a/docs/yaml_configuration.rst +++ b/docs/yaml_configuration.rst @@ -43,10 +43,6 @@ Bootstrap configuration - **- data-checksums**: Must be enabled when pg_rewind is needed on 9.3. - **- encoding: UTF8**: default encoding for new databases. - **- locale: UTF8**: default locale for new databases. - - **pg\_hba**: list of lines that you should add to pg\_hba.conf. - - - **- host all all 0.0.0.0/0 md5**. - - **- host replication replicator 127.0.0.1/32 md5**: A line like this is required for replication. - **users**: Some additional users which need to be created after initializing new cluster - **admin**: the name of user @@ -270,18 +266,18 @@ PostgreSQL - **recovery\_conf**: additional configuration settings written to recovery.conf when configuring follower. - **custom\_conf** : path to an optional custom ``postgresql.conf`` file, that will be used in place of ``postgresql.base.conf``. The file must exist on all cluster nodes, be readable by PostgreSQL and will be included from its location on the real ``postgresql.conf``. Note that Patroni will not monitor this file for changes, nor backup it. However, its settings can still be overridden by Patroni's own configuration facilities - see :ref:`dynamic configuration ` for details. - **parameters**: list of configuration settings for Postgres. Many of these are required for replication to work. - - **pg\_hba**: list of lines that Patroni will use to generate ``pg_hba.conf``. This parameter has higher priority than ``bootstrap.pg_hba``. Together with :ref:`dynamic configuration ` it simplifies management of ``pg_hba.conf``. + - **pg\_hba**: list of lines that Patroni will use to generate ``pg_hba.conf``. Patroni ignores this parameter if ``hba_file`` PostgreSQL parameter is set to a non-default value. Together with :ref:`dynamic configuration ` this parameter simplifies management of ``pg_hba.conf``. - - **- host all all 0.0.0.0/0 md5**. + - **- host all all 0.0.0.0/0 md5** - **- host replication replicator 127.0.0.1/32 md5**: A line like this is required for replication. - - **pg\_ident**: list of lines that Patroni will use to generate ``pg_ident.conf``. Together with :ref:`dynamic configuration ` it simplifies management of ``pg_ident.conf``. + - **pg\_ident**: list of lines that Patroni will use to generate ``pg_ident.conf``. Patroni ignores this parameter if ``ident_file`` PostgreSQL parameter is set to a non-default value. Together with :ref:`dynamic configuration ` this parameter simplifies management of ``pg_ident.conf``. - - **- mapname1 systemname1 pguser1**. - - **- mapname1 systemname2 pguser2**. + - **- mapname1 systemname1 pguser1** + - **- mapname1 systemname2 pguser2** - **pg\_ctl\_timeout**: How long should pg_ctl wait when doing ``start``, ``stop`` or ``restart``. Default value is 60 seconds. - **use\_pg\_rewind**: try to use pg\_rewind on the former leader when it joins cluster as a replica. - **remove\_data\_directory\_on\_rewind\_failure**: If this option is enabled, Patroni will remove the PostgreSQL data directory and recreate the replica. Otherwise it will try to follow the new leader. Default value is **false**. - - **remove\_data\_directory\_on\_diverged\_timelines**: Patroni will remove the PostgreSQL data directory and recreate the replica if it notices that timelines are diverging and the former primary can not start streaming from the new primary. This option is useful when ``pg_rewind`` can not be used. While performing timelines divergence check on PostgreSQL v10 and older Patroni will try to connect with replication credential to the "postgres" database. Hence, such access should be allowed in the pg_hba.conf. Default value is **false**. + - **remove\_data\_directory\_on\_diverged\_timelines**: Patroni will remove the PostgreSQL data directory and recreate the replica if it notices that timelines are diverging and the former primary can not start streaming from the new primary. This option is useful when ``pg_rewind`` can not be used. While performing timelines divergence check on PostgreSQL v10 and older Patroni will try to connect with replication credential to the "postgres" database. Hence, such access should be allowed in the pg_hba.conf. Default value is **false**. - **replica\_method**: for each create_replica_methods other than basebackup, you would add a configuration section of the same name. At a minimum, this should include "command" with a full path to the actual script to be executed. Other configuration parameters will be passed along to the script in the form "parameter=value". - **pre\_promote**: a fencing script that executes during a failover after acquiring the leader lock but before promoting the replica. If the script exits with a non-zero code, Patroni does not promote the replica and removes the leader key from DCS. - **before\_stop**: a script that executes immediately prior to stopping postgres. As opposed to a callback, this script runs synchronously, blocking shutdown until it has completed. The return code of this script does not impact whether shutdown proceeds afterwards. diff --git a/kubernetes/Dockerfile.citus b/kubernetes/Dockerfile.citus index 195bb8e9..e61850c7 100644 --- a/kubernetes/Dockerfile.citus +++ b/kubernetes/Dockerfile.citus @@ -34,7 +34,9 @@ ADD entrypoint.sh / ENV PGSSLMODE=verify-ca PGSSLKEY=/etc/ssl/private/ssl-cert-snakeoil.key PGSSLCERT=/etc/ssl/certs/ssl-cert-snakeoil.pem PGSSLROOTCERT=/etc/ssl/certs/ssl-cert-snakeoil.pem RUN sed -i 's/^postgresql:/&\n basebackup:\n checkpoint: fast/' /entrypoint.sh \ - && sed -i "s|^ postgresql:|&\n pg_hba:\n - local all all trust\n - hostssl replication all all md5 clientcert=$PGSSLMODE\n - hostssl all all all md5 clientcert=$PGSSLMODE\n parameters:\n max_connections: 100\n shared_buffers: 16MB\n ssl: 'on'\n ssl_ca_file: $PGSSLROOTCERT\n ssl_cert_file: $PGSSLCERT\n ssl_key_file: $PGSSLKEY\n citus.node_conninfo: 'sslrootcert=$PGSSLROOTCERT sslkey=$PGSSLKEY sslcert=$PGSSLCERT sslmode=$PGSSLMODE'|" /entrypoint.sh \ + && sed -i "s|^ postgresql:|&\n parameters:\n max_connections: 100\n shared_buffers: 16MB\n ssl: 'on'\n ssl_ca_file: $PGSSLROOTCERT\n ssl_cert_file: $PGSSLCERT\n ssl_key_file: $PGSSLKEY\n citus.node_conninfo: 'sslrootcert=$PGSSLROOTCERT sslkey=$PGSSLKEY sslcert=$PGSSLCERT sslmode=$PGSSLMODE'|" /entrypoint.sh \ + && sed -i 's/^ pg_hba:/&\n - local all all trust/' /entrypoint.sh \ + && sed -i "s/^\(.*\) \(.*\) \(.*\) \(.*\) \(.*\) md5.*$/\1 hostssl \3 \4 all md5 clientcert=$PGSSLMODE/" /entrypoint.sh \ && sed -i "s#^ \(superuser\|replication\):#&\n sslmode: $PGSSLMODE\n sslkey: $PGSSLKEY\n sslcert: $PGSSLCERT\n sslrootcert: $PGSSLROOTCERT#" /entrypoint.sh EXPOSE 5432 8008 diff --git a/kubernetes/entrypoint.sh b/kubernetes/entrypoint.sh index b4fa58be..ad7f6263 100755 --- a/kubernetes/entrypoint.sh +++ b/kubernetes/entrypoint.sh @@ -12,15 +12,15 @@ bootstrap: dcs: postgresql: use_pg_rewind: true + pg_hba: + - host all all 0.0.0.0/0 md5 + - host replication ${PATRONI_REPLICATION_USERNAME} ${PATRONI_KUBERNETES_POD_IP}/16 md5 initdb: - auth-host: md5 - auth-local: trust - encoding: UTF8 - locale: en_US.UTF-8 - data-checksums - pg_hba: - - host all all 0.0.0.0/0 md5 - - host replication ${PATRONI_REPLICATION_USERNAME} ${PATRONI_KUBERNETES_POD_IP}/16 md5 restapi: connect_address: '${PATRONI_KUBERNETES_POD_IP}:8008' postgresql: diff --git a/patroni/validator.py b/patroni/validator.py index 3d30ca2c..ccf41fd0 100644 --- a/patroni/validator.py +++ b/patroni/validator.py @@ -773,7 +773,6 @@ schema = Schema({ Optional("retry_timeout"): int, Optional("maximum_lag_on_failover"): int }, - "pg_hba": [str], "initdb": [Or(str, dict)] }, Or(*available_dcs): Case({ diff --git a/postgres0.yml b/postgres0.yml index 7aed19b9..2e83c7e1 100644 --- a/postgres0.yml +++ b/postgres0.yml @@ -59,6 +59,13 @@ bootstrap: #primary_slot_name: patroni postgresql: use_pg_rewind: true + pg_hba: + # For kerberos gss based connectivity (discard @.*$) + #- host replication replicator 127.0.0.1/32 gss include_realm=0 + #- host all all 0.0.0.0/0 gss include_realm=0 + - host replication replicator 127.0.0.1/32 md5 + - host all all 0.0.0.0/0 md5 + # - hostssl all all 0.0.0.0/0 md5 # use_slots: true parameters: # wal_level: hot_standby @@ -83,14 +90,6 @@ bootstrap: - encoding: UTF8 - data-checksums - pg_hba: # Add following lines to pg_hba.conf after running 'initdb' - # For kerberos gss based connectivity (discard @.*$) - #- host replication replicator 127.0.0.1/32 gss include_realm=0 - #- host all all 0.0.0.0/0 gss include_realm=0 - - host replication replicator 127.0.0.1/32 md5 - - host all all 0.0.0.0/0 md5 -# - hostssl all all 0.0.0.0/0 md5 - # Additional script to be launched after initial cluster creation (will be passed the connection URL as parameter) # post_init: /usr/local/bin/setup_cluster.sh diff --git a/postgres1.yml b/postgres1.yml index ce7d1ee2..e8b2806d 100644 --- a/postgres1.yml +++ b/postgres1.yml @@ -53,6 +53,13 @@ bootstrap: maximum_lag_on_failover: 1048576 postgresql: use_pg_rewind: true + pg_hba: + # For kerberos gss based connectivity (discard @.*$) + #- host replication replicator 127.0.0.1/32 gss include_realm=0 + #- host all all 0.0.0.0/0 gss include_realm=0 + - host replication replicator 127.0.0.1/32 md5 + - host all all 0.0.0.0/0 md5 + # - hostssl all all 0.0.0.0/0 md5 # use_slots: true parameters: # wal_level: hot_standby @@ -77,14 +84,6 @@ bootstrap: - encoding: UTF8 - data-checksums - pg_hba: # Add following lines to pg_hba.conf after running 'initdb' - # For kerberos gss based connectivity (discard @.*$) - #- host replication replicator 127.0.0.1/32 gss include_realm=0 - #- host all all 0.0.0.0/0 gss include_realm=0 - - host replication replicator 127.0.0.1/32 md5 - - host all all 0.0.0.0/0 md5 -# - hostssl all all 0.0.0.0/0 md5 - # Additional script to be launched after initial cluster creation (will be passed the connection URL as parameter) # post_init: /usr/local/bin/setup_cluster.sh diff --git a/postgres2.yml b/postgres2.yml index 5ca44cd0..8272e3ba 100644 --- a/postgres2.yml +++ b/postgres2.yml @@ -53,6 +53,13 @@ bootstrap: maximum_lag_on_failover: 1048576 postgresql: use_pg_rewind: true + pg_hba: + # For kerberos gss based connectivity (discard @.*$) + #- host replication replicator 127.0.0.1/32 gss include_realm=0 + #- host all all 0.0.0.0/0 gss include_realm=0 + - host replication replicator 127.0.0.1/32 md5 + - host all all 0.0.0.0/0 md5 + # - hostssl all all 0.0.0.0/0 md5 # use_slots: true parameters: # wal_level: hot_standby @@ -77,14 +84,6 @@ bootstrap: - encoding: UTF8 - data-checksums - pg_hba: # Add following lines to pg_hba.conf after running 'initdb' - # For kerberos gss based connectivity (discard @.*$) - #- host replication replicator 127.0.0.1/32 gss include_realm=0 - #- host all all 0.0.0.0/0 gss include_realm=0 - - host replication replicator 127.0.0.1/32 md5 - - host all all 0.0.0.0/0 md5 -# - hostssl all all 0.0.0.0/0 md5 - # Some additional users users which needs to be created after initializing new cluster users: admin: diff --git a/tests/test_validator.py b/tests/test_validator.py index 87ea78b7..cec1f140 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -24,7 +24,6 @@ config = { "retry_timeout": 1000, "maximum_lag_on_failover": 1000 }, - "pg_hba": ["string"], "initdb": ["string", {"key": "value"}] }, "consul": { From d1fdb45179807f2c3b9b113031e90862205987ed Mon Sep 17 00:00:00 2001 From: Polina Bungina <27892524+hughcapet@users.noreply.github.com> Date: Wed, 24 May 2023 09:28:57 +0200 Subject: [PATCH 03/12] Make bootstrap.initdb optional (#2685) --- docs/yaml_configuration.rst | 2 +- patroni/validator.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/yaml_configuration.rst b/docs/yaml_configuration.rst index dd84f7cb..598143a7 100644 --- a/docs/yaml_configuration.rst +++ b/docs/yaml_configuration.rst @@ -38,7 +38,7 @@ Bootstrap configuration See :ref:`custom bootstrap methods documentation ` for details. When ``initdb`` is specified revert to the default ``initdb`` command. ``initdb`` is also triggered when no ``method`` parameter is present in the configuration file. - - **initdb**: List options to be passed on to initdb. + - **initdb**: (optional) list options to be passed on to initdb. - **- data-checksums**: Must be enabled when pg_rewind is needed on 9.3. - **- encoding: UTF8**: default encoding for new databases. diff --git a/patroni/validator.py b/patroni/validator.py index ccf41fd0..9a4b117c 100644 --- a/patroni/validator.py +++ b/patroni/validator.py @@ -773,7 +773,7 @@ schema = Schema({ Optional("retry_timeout"): int, Optional("maximum_lag_on_failover"): int }, - "initdb": [Or(str, dict)] + Optional("initdb"): [Or(str, dict)] }, Or(*available_dcs): Case({ "consul": { From b4afc6830b5c5584c7354d6f966375881e5993c0 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 24 May 2023 10:54:26 +0200 Subject: [PATCH 04/12] Little fixes in etcd3 and kubernetes (#2689) - Always pass etcd3 key revision as a string - Make sure the leader key isn't unconditionally overwritten. It may happen that the leader heart-beat loop didn't run properly and the session has expired. In this case the leader may create a new session and a new leader key. But, there are chances that the other node already created a leader key and we don't want to overwrite it. - Try to sync HA loops between nodes by adding 0.5 seconds to timeout on non-leader nodes --- patroni/dcs/etcd3.py | 12 ++++++++---- patroni/dcs/kubernetes.py | 6 +++++- tests/test_etcd3.py | 2 +- tests/test_kubernetes.py | 2 +- 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/patroni/dcs/etcd3.py b/patroni/dcs/etcd3.py index 308bb2c3..55923df3 100644 --- a/patroni/dcs/etcd3.py +++ b/patroni/dcs/etcd3.py @@ -813,7 +813,7 @@ class Etcd3(AbstractEtcd): return retry(*args, **kwargs) try: - return _retry(self._client.put, self.leader_path, self._name, self._lease, 0) + return _retry(self._client.put, self.leader_path, self._name, self._lease, '0') except LeaseNotFound: logger.error('Our lease disappeared from Etcd. Will try to get a new one and retry attempt') self._lease = None @@ -825,7 +825,7 @@ class Etcd3(AbstractEtcd): if retry.deadline < 1: raise Etcd3Error('_do_attempt_to_acquire_leader timeout') - return _retry(self._client.put, self.leader_path, self._name, self._lease, 0) + return _retry(self._client.put, self.leader_path, self._name, self._lease, '0') @catch_return_false_exception def attempt_to_acquire_leader(self) -> bool: @@ -886,14 +886,14 @@ class Etcd3(AbstractEtcd): try: self._run_and_handle_exceptions(self._client.put, self.leader_path, - self._name, self._lease, retry=_retry) + self._name, self._lease, '0', retry=_retry) except ReturnFalseException: pass return bool(self._lease) @catch_etcd_errors def initialize(self, create_new: bool = True, sysid: str = ""): - return self.retry(self._client.put, self.initialize_path, sysid, None, 0 if create_new else None) + return self.retry(self._client.put, self.initialize_path, sysid, None, '0' if create_new else None) @catch_etcd_errors def _delete_leader(self) -> bool: @@ -928,6 +928,10 @@ class Etcd3(AbstractEtcd): self.__do_not_watch = False return True + # We want to give a bit more time to non-leader nodes to synchronize HA loops + if leader_version: + timeout += 0.5 + try: return super(Etcd3, self).watch(None, timeout) finally: diff --git a/patroni/dcs/kubernetes.py b/patroni/dcs/kubernetes.py index 3e334e9d..3630ae1e 100644 --- a/patroni/dcs/kubernetes.py +++ b/patroni/dcs/kubernetes.py @@ -1350,7 +1350,11 @@ class Kubernetes(AbstractDCS): self.__do_not_watch = False return True + # We want to give a bit more time to non-leader nodes to synchronize HA loops + if leader_version: + timeout += 0.5 + try: - return super(Kubernetes, self).watch(None, timeout + 0.5) + return super(Kubernetes, self).watch(None, timeout) finally: self.event.clear() diff --git a/tests/test_etcd3.py b/tests/test_etcd3.py index 85c2173a..6f56d0e6 100644 --- a/tests/test_etcd3.py +++ b/tests/test_etcd3.py @@ -306,7 +306,7 @@ class TestEtcd3(BaseTestEtcd3): def test_watch(self): self.etcd3.set_ttl(10) self.etcd3.watch(None, 0) - self.etcd3.watch(None, 0) + self.etcd3.watch('5', 0) def test_set_socket_options(self): with patch('socket.SIO_KEEPALIVE_VALS', 1, create=True): diff --git a/tests/test_kubernetes.py b/tests/test_kubernetes.py index bbe55157..fbefb448 100644 --- a/tests/test_kubernetes.py +++ b/tests/test_kubernetes.py @@ -315,7 +315,7 @@ class TestKubernetesConfigMaps(BaseTestKubernetes): def test_watch(self): self.k.set_ttl(10) self.k.watch(None, 0) - self.k.watch(None, 0) + self.k.watch('5', 0) def test_set_history_value(self): self.k.set_history_value('{}') From 73797e85728cc2ba431d6a309515b7e95a4966f5 Mon Sep 17 00:00:00 2001 From: Matt Baker <93600443+matthbakeredb@users.noreply.github.com> Date: Wed, 24 May 2023 09:58:04 +0100 Subject: [PATCH 05/12] Add tox configuration for running multiple test envs (#2603) --- docs/CONTRIBUTING.rst | 122 +++++++++++++++++++++++++- features/Dockerfile | 91 ++++++++++++++++++++ pyrightconfig.json | 2 +- tox.ini | 195 +++++++++++++++++++++++++++++++++++++++++- 4 files changed, 406 insertions(+), 4 deletions(-) create mode 100644 features/Dockerfile diff --git a/docs/CONTRIBUTING.rst b/docs/CONTRIBUTING.rst index 58e5f7bc..44733a55 100644 --- a/docs/CONTRIBUTING.rst +++ b/docs/CONTRIBUTING.rst @@ -19,7 +19,7 @@ Requirements for running behave tests: 2. PostgreSQL binaries must be available in your `PATH`. You may need to add them to the path with something like `PATH=/usr/lib/postgresql/11/bin:$PATH python -m behave`. 3. If you'd like to test with external DCSs (e.g., Etcd, Consul, and Zookeeper) you'll need the packages installed and respective services running and accepting unencrypted/unprotected connections on localhost and default port. In the case of Etcd or Consul, the behave test suite could start them up if binaries are available in the `PATH`. - Install dependencies: +Install dependencies: .. code-block:: bash @@ -43,6 +43,126 @@ After you have all dependencies installed, you can run the various test suites: # modify DCS as desired (raft has no dependencies so is the easiest to start with): DCS=raft python -m behave +Testing with tox +---------------- + +To run tox tests you only need to install one dependency (other than Python) + +.. code-block:: bash + + pip install tox>=4 + +If you wish to run `behave` tests then you also need docker installed. + +Tox configuration in `tox.ini` has "environments" to run the following tasks: + +* lint: Python code lint with `flake8` +* test: unit tests for all available python interpreters with `pytest`, + generates XML reports or HTML reports if a TTY is detected +* dep: detect package dependency conflicts using `pipdeptree` +* type: static type checking with `pyright` +* black: code formatting with `black` +* docker-build: build docker image used for the `behave` env +* docker-cmd: run arbitrary command with the above image +* docker-behave-etcd: run tox for behave tests with above image +* py*behave: run behave with available python interpreters (without docker, although + this is what is called inside docker containers) +* docs: build docs with `sphinx` + +Running tox +^^^^^^^^^^^ + +To run the default env list; dep, lint, test, and docs, just run: + +.. code-block:: bash + + tox + +The `test` envs can be run with the label `test`: + +.. code-block:: bash + + tox -m test + +The `behave` docker tests can be run with the label `behave`: + +.. code-block:: bash + + tox -m behave + +Similarly, docs has the label `docs`. + +All other envs can be run with their respective env names: + +.. code-block:: bash + + tox -e lint + tox -e py39-test-lin + +It is also possible to select partial env lists using `factors`. For example, if you want to run +all envs for python 3.10: + +.. code-block:: bash + + tox -f py310 + +This is equivalent to running all the envs listed below: + +.. code-block:: bash + + $ tox -l -f py310 + py310-test-lin + py310-test-mac + py310-test-win + py310-type-lin + py310-type-mac + py310-type-win + py310-behave-etcd-lin + py310-behave-etcd-win + py310-behave-etcd-mac + + +You can list all configured combinations of environments with tox (>=v4) like so + +.. code-block:: bash + + tox l + +The envs `test` and `docs` will attempt to open the HTML output files +when the job completes, if tox is run with an active terminal. This +is intended to be for benefit of the developer running this env locally. +It will attempt to run `open` on a mac and `xdg-open` on Linux. +To use a different command set the env var `OPEN_CMD` to the name or path of +the command. If this step fails it will not fail the run overall. +If you want to disable this facility set the env var `OPEN_CMD` to the `:` no-op command. + +.. code-block:: bash + + OPEN_CMD=: tox -m docs + +Behave tests +^^^^^^^^^^^^ + +Behave tests with `-m behave` will build docker images based on PG_MAJOR version 11 through 15 and then run all +behave tests. This can take quite a long time to run so you might want to limit the scope to a select version of +Postgres or to a specific feature set or steps. + +To specify the version of postgres include the full name of the dependent image build env that you want and then the +behave env name. For instance if you want Postgres 15 use: + +.. code-block:: bash + + tox -e pg14-docker-build,pg14-docker-behave-etcd-lin + +If on the other hand you want to test a specific feature you can pass positional arguments to behave. This will run +the watchdog behave feature test scenario with all versions of Postgres. + +.. code-block:: bash + + tox -m behave -- features/watchdog.feature + +Of course you can combine the two. + Reporting issues ---------------- diff --git a/features/Dockerfile b/features/Dockerfile new file mode 100644 index 00000000..e2f5dbb9 --- /dev/null +++ b/features/Dockerfile @@ -0,0 +1,91 @@ +# syntax = docker/dockerfile:1.5 +# Used only for running tests using tox, see ../tox.ini +ARG PG_MAJOR +ARG PGHOME=/home/postgres +ARG LC_ALL=C.UTF-8 +ARG LANG=C.UTF-8 + +FROM postgres:${PG_MAJOR} + +ARG PGHOME +ARG LC_ALL +ARG LANG + +ENV PGHOME="$PGHOME" +ENV LC_ALL="$LC_ALL" +ENV LANG="$LANG" + +ARG ETCDVERSION=3.3.13 +ENV ETCDVERSION="$ETCDVERSION" +ARG ETCDURL="https://github.com/coreos/etcd/releases/download/v$ETCDVERSION" + +USER root +RUN set -ex \ + && apt-get update \ + && apt-get reinstall init-system-helpers \ + && apt-get install -y \ + python3-pip \ + python3-dev \ + rsync \ + curl \ + gcc \ + golang \ + jq \ + locales \ + sudo \ + busybox \ + net-tools \ + iputils-ping \ + && rm -rf /var/cache/apt \ + && python3 -m pip install --no-cache-dir tox \ + \ + && mkdir -p "$PGHOME" \ + && sed -i "s|/var/lib/postgresql.*|$PGHOME:/bin/bash|" /etc/passwd \ + && chown -R postgres:postgres /var/log /home/postgres \ + \ + # Download etcd \ + && curl -sL "$ETCDURL/etcd-v$ETCDVERSION-linux-$(dpkg --print-architecture).tar.gz" \ + | tar xz -C /usr/local/bin --strip=1 --wildcards --no-anchored etcd etcdctl + + +# This Dockerfile syntax only works with docker buildx and the syntax +# line at the top of this file. +COPY </dev/null \\ + | sed 's|^./||' >/tmp/copy_exclude.lst \\ + || true +runuser -u postgres -- \\ + rsync -a \\ + --exclude=.tox \\ + --exclude="features/output*" \\ + --exclude-from="/tmp/copy_exclude.lst" \\ + . "\$PGHOME/src/" +cd "\$PGHOME/src" +runuser -u postgres -w ETCD_UNSUPPORTED_ARCH -- "\$@" & +wait $! +# SIGINT whilst child proc is running is not seen by trap so we run a copy here instead of using +# trap copy_output SIGINT EXIT +copy_output +EOF +RUN chmod +x /tox-wrapper.sh + +VOLUME /src + +ENTRYPOINT ["/tox-wrapper.sh"] diff --git a/pyrightconfig.json b/pyrightconfig.json index 2394c6de..91bed713 100644 --- a/pyrightconfig.json +++ b/pyrightconfig.json @@ -2,7 +2,7 @@ "include": [ "patroni" ], - + "exclude": [ "**/__pycache__" ], diff --git a/tox.ini b/tox.ini index 67616ade..72353d4f 100644 --- a/tox.ini +++ b/tox.ini @@ -1,3 +1,194 @@ +[common] +python_matrix = {36,37,38,39,310,311} +postgres_matrix = + pg11: PG_MAJOR = 11 + pg12: PG_MAJOR = 12 + pg13: PG_MAJOR = 13 + pg14: PG_MAJOR = 14 + pg15: PG_MAJOR = 15 +psycopg_deps = + py{37,38,39,310,311}-{lin,win}: psycopg[binary] + mac: psycopg2-binary + py36: psycopg2-binary +platforms = + lin: linux + mac: darwin + win: win32 + +[tox] +min_version = 4.0 +requires = + tox>4 +env_list = + dep + lint + py{[common]python_matrix}-test-{lin,mac,win} + docs +skipsdist = True +toxworkdir = {env:TOX_WORK_DIR:.tox} +skip_missing_interpreters = True + +[testenv] +setenv = + PYTHONDONTWRITEBYTECODE = 1 + mac: OPEN_CMD = {env:OPEN_CMD:open} + lin: OPEN_CMD = {env:OPEN_CMD:xdg-open} +passenv = + BROWSER + DISPLAY + +[testenv:lint] +description = Lint code with flake8 +commands = flake8 {posargs:patroni tests setup.py} +deps = + flake8 + +[testenv:py{36,37,38,39,310,311}-test-{lin,win,mac}] +description = Run unit tests with pytest +labels = + test +commands_pre = + - {tty:rm -f "{toxworkdir}{/}cov_report_{env_name}_html{/}index.html":true} + - {tty:rm -f "{toxworkdir}{/}pytest_report_{env_name}.html":true} +commands = + pytest \ + -p no:cacheprovider \ + --verbose \ + --doctest-modules \ + --capture=fd \ + --cov=patroni \ + --cov-report=term-missing \ + --cov-append \ + {tty::--cov-report="xml\:{toxworkdir}{/}cov_report.{env_name}.xml"} \ + {tty:--cov-report="html\:{toxworkdir}{/}cov_report_{env_name}_html":} \ + {tty:--html="{toxworkdir}{/}pytest_report_{env_name}.html":} \ + {posargs:tests patroni} +commands_post = + - {tty:{env:OPEN_CMD} "{toxworkdir}{/}cov_report_{env_name}_html{/}index.html":true} + - {tty:{env:OPEN_CMD} "{toxworkdir}{/}pytest_report_{env_name}.html":true} +deps = + -r requirements.txt + mock>=2.0.0 + pytest + pytest-cov + pytest-html + {[common]psycopg_deps} +platform = + {[common]platforms} +allowlist_externals = + rm + {env:OPEN_CMD} + +[testenv:dep] +description = Check package dependency problems +commands = pipdeptree -w fail +deps = + -r requirements.txt + pipdeptree + {[common]psycopg_deps} + +[testenv:py{37,38,39,310,311}-type-{lin,mac,win}] +description = Run static type checking with pyright +labels = + type +deps = + -r requirements.txt + pyright + psycopg2-binary + psycopg[binary] +commands = pyright --venv-path {toxworkdir}{/}{envname} {posargs:patroni} +platform = + {[common]platforms} + +[testenv:black] +description = Reformat code with black +deps = black +commands = black {posargs:patroni tests} + +[testenv:pg{12,13,14,15}-docker-build] +description = Build docker containers needed for testing +labels = + behave + docker-build +setenv = + {[common]postgres_matrix} + DOCKER_BUILDKIT = 1 +commands = + docker build . \ + --tag patroni-dev:{env:PG_MAJOR} \ + --build-arg PG_MAJOR \ + --file features/Dockerfile +allowlist_externals = docker + +[testenv:pg{12,13,14,15}-docker-behave-{etcd}-{lin,mac}] +description = Run behaviour tests in patroni-dev docker container +setenv = + etcd: DCS=etcd + {[common]postgres_matrix} + CONTAINER_NAME = tox-{env_name}-{env:PYTHONHASHSEED} +labels = + behave +depends = + pg{11,12,13,14,15}-docker-build + +# There's a bug which affects calling multiple envs on the command line +# This should be a valid command: tox -e 'py{36,37,38,39,310,311}-behave-{env:DCS}-lin' +# Replaced with workaround, see https://github.com/tox-dev/tox/issues/2850 +commands = + docker run \ + --volume {tox_root}:/src \ + --env DCS={env:DCS} \ + --hostname {env:CONTAINER_NAME} \ + --name {env:CONTAINER_NAME} \ + --rm \ + --tty \ + {env:PATRONI_DEV_IMAGE:patroni-dev:{env:PG_MAJOR}} \ + tox run -x 'tox.env_list=py{[common]python_matrix}-behave-{env:DCS}-lin' \ + -- --format plain {posargs} + +allowlist_externals = + docker + find +platform = + lin: linux +; win: win32 + mac: darwin + +[testenv:py{36,38,39,310,311}-behave-{etcd}-{lin,win,mac}] +description = Run behaviour tests (locally with tox) +deps = + -r requirements.txt + behave + coverage + {[common]psycopg_deps} +setenv = + DCS = {env:DCS:etcd} +passenv = + ETCD_UNSUPPORTED_ARCH +commands = + python3 -m behave {posargs} +platform = + {[common]platforms} + +[testenv:docs-{lin,mac,win}] +description = Build Sphinx documentation +labels: + docs +deps = + sphinx>=4 + sphinx_rtd_theme +commands = + sphinx-build \ + -d "{envtmpdir}{/}doctree" docs "{toxworkdir}{/}docs_out" \ + --color -b html \ + {posargs} +commands_post = + - {tty:{env:OPEN_CMD} "{toxworkdir}{/}docs_out{/}index.html":true:} +allowlist_externals = + {env:OPEN_CMD} +platform = + {[common]platforms} + [flake8] -max-line-length=120 -ignore=D401,W503 +max-line-length = 120 +ignore = D401,W503 From 822b6ec7114799fbdb59ec17281e38a5676137cb Mon Sep 17 00:00:00 2001 From: Polina Bungina <27892524+hughcapet@users.noreply.github.com> Date: Wed, 24 May 2023 11:22:41 +0200 Subject: [PATCH 06/12] Subtle README fix (#2691) Remove misleading words --- docs/README.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/README.rst b/docs/README.rst index cd338155..8e38c908 100644 --- a/docs/README.rst +++ b/docs/README.rst @@ -179,7 +179,7 @@ That said, here are some pieces of your infrastructure you should be sure to tes * Network (the network in front of your system as well as the NICs [physical or virtual] themselves) * Disk IO * file limits (nofile in Linux) -* RAM. Even if you have oomkiller turned off as suggested, the unavailability of RAM could cause issues. +* RAM. Even if you have oomkiller turned off, the unavailability of RAM could cause issues. * CPU * Virtualization Contention (overcommitting the hypervisor) * Any cgroup limitation (likely to be related to the above) From 1c7bf2f59e755caccd1c8ad00849ba37dc992855 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 25 May 2023 10:01:43 +0200 Subject: [PATCH 07/12] Fix a problem with etcd3.update_leader() (#2693) It didn't took into account the fact that we can get a new lease after changing a TTL. In this case we have to update the exiting leader key with the new lease. To solve it we introduce the on transaction 'failure' callback. The whole workflow looks like (schematically): ```python txn( compare=(old_value == self._name)), success=put(self.leader_path, self._name, self._lease), failure=txn( compare=(create_revision == '0'), success=put(self.leader_path, self._name, self._lease) ) ) ``` The problem was introduced in d98d6d0b02c9cc67464a7b1b31b1c5570c26e12d --- patroni/dcs/etcd3.py | 47 ++++++++++++++++++++++++++++++-------------- tests/test_etcd3.py | 5 ++++- 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/patroni/dcs/etcd3.py b/patroni/dcs/etcd3.py index 55923df3..6c4312e5 100644 --- a/patroni/dcs/etcd3.py +++ b/patroni/dcs/etcd3.py @@ -16,7 +16,7 @@ from threading import Condition, Lock, Thread from typing import Any, Callable, Collection, Dict, Iterator, List, Optional, Tuple, Type, TYPE_CHECKING, Union from . import ClusterConfig, Cluster, Failover, Leader, Member, SyncState,\ - TimelineHistory, ReturnFalseException, catch_return_false_exception, citus_group_re + TimelineHistory, catch_return_false_exception, citus_group_re from .etcd import AbstractEtcdClientWithFailover, AbstractEtcd, catch_etcd_errors, DnsCachingResolver, Retry from ..exceptions import DCSError, PatroniException from ..utils import deep_compare, enable_keepalive, iter_response_objects, RetryFailedError, USER_AGENT @@ -343,9 +343,13 @@ class Etcd3Client(AbstractEtcdClientWithFailover): def lease_keepalive(self, ID: str, retry: Optional[Retry] = None) -> Optional[str]: return self.call_rpc('/lease/keepalive', {'ID': ID}, retry).get('result', {}).get('TTL') - def txn(self, compare: Dict[str, Any], success: Dict[str, Any], retry: Optional[Retry] = None) -> Dict[str, Any]: - ret = self.call_rpc('/kv/txn', {'compare': [compare], 'success': [success]}, retry) - return ret if ret.get('succeeded') else {} + def txn(self, compare: Dict[str, Any], success: Dict[str, Any], + failure: Optional[Dict[str, Any]] = None, retry: Optional[Retry] = None) -> Dict[str, Any]: + fields = {'compare': [compare], 'success': [success]} + if failure: + fields['failure'] = [failure] + ret = self.call_rpc('/kv/txn', fields, retry) + return ret if failure or ret.get('succeeded') else {} @_handle_auth_errors def put(self, key: str, value: str, lease: Optional[str] = None, create_revision: Optional[str] = None, @@ -360,7 +364,7 @@ class Etcd3Client(AbstractEtcdClientWithFailover): else: return self.call_rpc('/kv/put', fields, retry) compare['key'] = fields['key'] - return self.txn(compare, {'request_put': fields}, retry) + return self.txn(compare, {'request_put': fields}, retry=retry) @_handle_auth_errors def deleterange(self, key: str, range_end: Union[bytes, str, None] = None, @@ -369,7 +373,7 @@ class Etcd3Client(AbstractEtcdClientWithFailover): if mod_revision is None: return self.call_rpc('/kv/deleterange', fields, retry) compare = {'target': 'MOD', 'mod_revision': mod_revision, 'key': fields['key']} - return self.txn(compare, {'request_delete_range': fields}, retry) + return self.txn(compare, {'request_delete_range': fields}, retry=retry) def deleteprefix(self, key: str, retry: Optional[Retry] = None) -> Dict[str, Any]: return self.deleterange(key, prefix_range_end(key), retry=retry) @@ -603,7 +607,13 @@ class PatroniEtcd3Client(Etcd3Client): if self._kv_cache: value = delete = None - if method == '/kv/txn' and ret.get('succeeded'): + # For the 'failure' case we only support a second (nested) transaction that attempts to + # update/delete the same keys. Anything more complex than that we don't need and therefore it doesn't + # make sense to write a universal response analyzer and we can just check expected JSON path. + if method == '/kv/txn'\ + and (ret.get('succeeded') or 'failure' in fields and 'request_txn' in fields['failure'][0] + and ret.get('responses', [{'response_txn': {'succeeded': False}}])[0] + .get('response_txn', {}).get('succeeded')): on_success = fields['success'][0] value = on_success.get('request_put') delete = on_success.get('request_delete_range') @@ -813,7 +823,7 @@ class Etcd3(AbstractEtcd): return retry(*args, **kwargs) try: - return _retry(self._client.put, self.leader_path, self._name, self._lease, '0') + return _retry(self._client.put, self.leader_path, self._name, self._lease, create_revision='0') except LeaseNotFound: logger.error('Our lease disappeared from Etcd. Will try to get a new one and retry attempt') self._lease = None @@ -825,7 +835,7 @@ class Etcd3(AbstractEtcd): if retry.deadline < 1: raise Etcd3Error('_do_attempt_to_acquire_leader timeout') - return _retry(self._client.put, self.leader_path, self._name, self._lease, '0') + return _retry(self._client.put, self.leader_path, self._name, self._lease, create_revision='0') @catch_return_false_exception def attempt_to_acquire_leader(self) -> bool: @@ -884,16 +894,23 @@ class Etcd3(AbstractEtcd): if retry.deadline < 1: raise Etcd3Error('update_leader timeout') - try: - self._run_and_handle_exceptions(self._client.put, self.leader_path, - self._name, self._lease, '0', retry=_retry) - except ReturnFalseException: - pass + fields = {'key': base64_encode(self.leader_path), + 'value': base64_encode(self._name), 'lease': self._lease} + # First we try to update lease on existing leader key "hoping" that we still owning it + compare1 = {'key': fields['key'], 'target': 'VALUE', 'value': fields['value']} + request_put = {'request_put': fields} + # If the first comparison failed we will try to create the new leader key in a transaction + compare2 = {'key': fields['key'], 'target': 'CREATE', 'create_revision': '0'} + request_txn = {'request_txn': {'compare': [compare2], 'success': [request_put]}} + ret = self._run_and_handle_exceptions(self._client.txn, compare1, + request_put, request_txn, retry=_retry) + return ret.get('succeeded', False)\ + or ret.get('responses', [{}])[0].get('response_txn', {}).get('succeeded', False) return bool(self._lease) @catch_etcd_errors def initialize(self, create_new: bool = True, sysid: str = ""): - return self.retry(self._client.put, self.initialize_path, sysid, None, '0' if create_new else None) + return self.retry(self._client.put, self.initialize_path, sysid, create_revision='0' if create_new else None) @catch_etcd_errors def _delete_leader(self) -> bool: diff --git a/tests/test_etcd3.py b/tests/test_etcd3.py index 6f56d0e6..4b3a78ab 100644 --- a/tests/test_etcd3.py +++ b/tests/test_etcd3.py @@ -236,12 +236,15 @@ class TestEtcd3(BaseTestEtcd3): def test__update_leader(self): self.etcd3._lease = None - self.etcd3.update_leader('123', failsafe={'foo': 'bar'}) + with patch.object(Etcd3Client, 'txn', Mock(return_value={'succeeded': True})): + self.etcd3.update_leader('123', failsafe={'foo': 'bar'}) self.etcd3._last_lease_refresh = 0 self.etcd3.update_leader('124') with patch.object(PatroniEtcd3Client, 'lease_keepalive', Mock(return_value=True)),\ patch('time.time', Mock(side_effect=[0, 100, 200, 300])): self.assertRaises(Etcd3Error, self.etcd3.update_leader, '126') + self.etcd3._lease = self.etcd3.cluster.leader.session + self.etcd3.update_leader('124') self.etcd3._last_lease_refresh = 0 with patch.object(PatroniEtcd3Client, 'lease_keepalive', Mock(side_effect=Unknown)): self.assertFalse(self.etcd3.update_leader('125')) From af8e5f0d0fe94c85c7313764c72e195b3e6ba721 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 25 May 2023 14:21:05 +0200 Subject: [PATCH 08/12] Refactor update_leader interface (#2690) pass reference to a last known leader object in order to avoid obtaining it from the `AbstractDCS.cluster` cache. This change is useful for Consul, Etcd3 and Zookeeper implementations. --- patroni/dcs/__init__.py | 11 ++++++----- patroni/dcs/consul.py | 30 +++++++++++++----------------- patroni/dcs/etcd.py | 2 +- patroni/dcs/etcd3.py | 35 +++++++++++++++-------------------- patroni/dcs/kubernetes.py | 6 +++--- patroni/dcs/raft.py | 2 +- patroni/dcs/zookeeper.py | 6 ++---- patroni/ha.py | 4 +++- tests/test_consul.py | 17 +++++++++-------- tests/test_etcd.py | 11 ++++++----- tests/test_etcd3.py | 13 +++++++------ tests/test_kubernetes.py | 22 ++++++++++++---------- tests/test_raft.py | 4 ++-- tests/test_zookeeper.py | 11 ++++++----- 14 files changed, 86 insertions(+), 88 deletions(-) diff --git a/patroni/dcs/__init__.py b/patroni/dcs/__init__.py index 1ff4a60a..a61c6b59 100644 --- a/patroni/dcs/__init__.py +++ b/patroni/dcs/__init__.py @@ -965,25 +965,26 @@ class AbstractDCS(abc.ABC): return self._last_failsafe @abc.abstractmethod - def _update_leader(self) -> bool: + def _update_leader(self, leader: Leader) -> bool: """Update leader key (or session) ttl - :returns: `!True` if leader key (or session) has been updated successfully. + :param leader: a reference to a current leader key object + :returns: `!True` if leader key (or session) has been updated successfully You have to use CAS (Compare And Swap) operation in order to update leader key, for example for etcd `prevValue` parameter must be used. If update fails due to DCS not being accessible or because it is not able to process requests (hopefuly temporary), the ~DCSError exception should be raised.""" - def update_leader(self, last_lsn: Optional[int], slots: Optional[Dict[str, int]] = None, - failsafe: Optional[Dict[str, str]] = None) -> bool: + def update_leader(self, leader: Leader, last_lsn: Optional[int], + slots: Optional[Dict[str, int]] = None, failsafe: Optional[Dict[str, str]] = None) -> bool: """Update leader key (or session) ttl and optime/leader :param last_lsn: absolute WAL LSN in bytes :param slots: dict with permanent slots confirmed_flush_lsn :returns: `!True` if leader key (or session) has been updated successfully.""" - ret = self._update_leader() + ret = self._update_leader(leader) if ret and last_lsn: status: Dict[str, Any] = {self._OPTIME: last_lsn} if slots: diff --git a/patroni/dcs/consul.py b/patroni/dcs/consul.py index 2383a964..87ae4f6a 100644 --- a/patroni/dcs/consul.py +++ b/patroni/dcs/consul.py @@ -608,28 +608,24 @@ class Consul(AbstractDCS): raise ReturnFalseException @catch_return_false_exception - def _update_leader(self) -> bool: + def _update_leader(self, leader: Leader) -> bool: retry = self._retry.copy() self._run_and_handle_exceptions(self._do_refresh_session, True, retry=retry) - if self._session: - cluster = self.cluster - leader_session = cluster and isinstance(cluster.leader, Leader) and cluster.leader.session - if leader_session != self._session: - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 1: - raise ConsulError('update_leader timeout') - logger.warning('Recreating the leader key due to session mismatch') - if cluster and cluster.leader: - self._run_and_handle_exceptions(self._client.kv.delete, self.leader_path, - cas=cluster.leader.version) + if self._session and leader.session != self._session: + retry.deadline = retry.stoptime - time.time() + if retry.deadline < 1: + raise ConsulError('update_leader timeout') - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 0.5: - raise ConsulError('update_leader timeout') - self._run_and_handle_exceptions(self._client.kv.put, self.leader_path, - self._name, acquire=self._session) + logger.warning('Recreating the leader key due to session mismatch') + self._run_and_handle_exceptions(self._client.kv.delete, self.leader_path, cas=leader.version) + + retry.deadline = retry.stoptime - time.time() + if retry.deadline < 0.5: + raise ConsulError('update_leader timeout') + + self._run_and_handle_exceptions(self._client.kv.put, self.leader_path, self._name, acquire=self._session) return bool(self._session) diff --git a/patroni/dcs/etcd.py b/patroni/dcs/etcd.py index 190b79fc..3e7f0681 100644 --- a/patroni/dcs/etcd.py +++ b/patroni/dcs/etcd.py @@ -798,7 +798,7 @@ class Etcd(AbstractEtcd): return bool(self._client.set(self.failsafe_path, value)) @catch_return_false_exception - def _update_leader(self) -> bool: + def _update_leader(self, leader: Leader) -> bool: return bool(self._run_and_handle_exceptions(self._do_update_leader, retry=None)) @catch_etcd_errors diff --git a/patroni/dcs/etcd3.py b/patroni/dcs/etcd3.py index 6c4312e5..4d0ff75d 100644 --- a/patroni/dcs/etcd3.py +++ b/patroni/dcs/etcd3.py @@ -877,7 +877,7 @@ class Etcd3(AbstractEtcd): return bool(self._client.put(self.failsafe_path, value)) @catch_return_false_exception - def _update_leader(self) -> bool: + def _update_leader(self, leader: Leader) -> bool: retry = self._retry.copy() def _retry(*args: Any, **kwargs: Any) -> Any: @@ -886,26 +886,21 @@ class Etcd3(AbstractEtcd): self._run_and_handle_exceptions(self._do_refresh_lease, True, retry=_retry) - if self._lease: - cluster = self.cluster - leader_lease = cluster and isinstance(cluster.leader, Leader) and cluster.leader.session - if leader_lease != self._lease: - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 1: - raise Etcd3Error('update_leader timeout') + if self._lease and leader.session != self._lease: + retry.deadline = retry.stoptime - time.time() + if retry.deadline < 1: + raise Etcd3Error('update_leader timeout') - fields = {'key': base64_encode(self.leader_path), - 'value': base64_encode(self._name), 'lease': self._lease} - # First we try to update lease on existing leader key "hoping" that we still owning it - compare1 = {'key': fields['key'], 'target': 'VALUE', 'value': fields['value']} - request_put = {'request_put': fields} - # If the first comparison failed we will try to create the new leader key in a transaction - compare2 = {'key': fields['key'], 'target': 'CREATE', 'create_revision': '0'} - request_txn = {'request_txn': {'compare': [compare2], 'success': [request_put]}} - ret = self._run_and_handle_exceptions(self._client.txn, compare1, - request_put, request_txn, retry=_retry) - return ret.get('succeeded', False)\ - or ret.get('responses', [{}])[0].get('response_txn', {}).get('succeeded', False) + fields = {'key': base64_encode(self.leader_path), 'value': base64_encode(self._name), 'lease': self._lease} + # First we try to update lease on existing leader key "hoping" that we still owning it + compare1 = {'key': fields['key'], 'target': 'VALUE', 'value': fields['value']} + request_put = {'request_put': fields} + # If the first comparison failed we will try to create the new leader key in a transaction + compare2 = {'key': fields['key'], 'target': 'CREATE', 'create_revision': '0'} + request_txn = {'request_txn': {'compare': [compare2], 'success': [request_put]}} + ret = self._run_and_handle_exceptions(self._client.txn, compare1, request_put, request_txn, retry=_retry) + return ret.get('succeeded', False)\ + or ret.get('responses', [{}])[0].get('response_txn', {}).get('succeeded', False) return bool(self._lease) @catch_etcd_errors diff --git a/patroni/dcs/kubernetes.py b/patroni/dcs/kubernetes.py index 3630ae1e..ac65652d 100644 --- a/patroni/dcs/kubernetes.py +++ b/patroni/dcs/kubernetes.py @@ -1129,7 +1129,7 @@ class Kubernetes(AbstractDCS): """Unused""" raise NotImplementedError # pragma: no cover - def _update_leader(self) -> bool: + def _update_leader(self, leader: Leader) -> bool: """Unused""" raise NotImplementedError # pragma: no cover @@ -1182,8 +1182,8 @@ class Kubernetes(AbstractDCS): return bool(_run_and_handle_exceptions(self._patch_or_create, self.leader_path, annotations, kind_resource_version, ips=ips, retry=_retry)) - def update_leader(self, last_lsn: Optional[int], slots: Optional[Dict[str, int]] = None, - failsafe: Optional[Dict[str, str]] = None) -> bool: + def update_leader(self, leader: Leader, last_lsn: Optional[int], + slots: Optional[Dict[str, int]] = None, failsafe: Optional[Dict[str, str]] = None) -> bool: kind = self._kinds.get(self.leader_path) kind_annotations = kind and kind.metadata.annotations or {} diff --git a/patroni/dcs/raft.py b/patroni/dcs/raft.py index c068f7f0..c4286f1e 100644 --- a/patroni/dcs/raft.py +++ b/patroni/dcs/raft.py @@ -419,7 +419,7 @@ class Raft(AbstractDCS): def _write_failsafe(self, value: str) -> bool: return self._sync_obj.set(self.failsafe_path, value, timeout=1) is not False - def _update_leader(self) -> bool: + def _update_leader(self, leader: Leader) -> bool: ret = self._sync_obj.set(self.leader_path, self._name, ttl=self._ttl, handle_raft_error=False, prevValue=self._name) is not False if not ret and self._sync_obj.get(self.leader_path) is None: diff --git a/patroni/dcs/zookeeper.py b/patroni/dcs/zookeeper.py index 5ec9cd04..ed2b71ed 100644 --- a/patroni/dcs/zookeeper.py +++ b/patroni/dcs/zookeeper.py @@ -443,10 +443,8 @@ class ZooKeeper(AbstractDCS): def _write_failsafe(self, value: str) -> bool: return self._set_or_create(self.failsafe_path, value) is not False - def _update_leader(self) -> bool: - cluster = self.cluster - session = cluster and isinstance(cluster.leader, Leader) and cluster.leader.session - if self._client.client_id and self._client.client_id[0] != session: + def _update_leader(self, leader: Leader) -> bool: + if self._client.client_id and self._client.client_id[0] != leader.session: logger.warning('Recreating the leader ZNode due to ownership mismatch') try: self._client.retry(self._client.delete, self.leader_path) diff --git a/patroni/ha.py b/patroni/ha.py index 1190ad1c..3108f787 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -239,8 +239,10 @@ class Ha(object): slots = self.state_handler.slots() except Exception: logger.exception('Exception when called state_handler.last_operation()') + if TYPE_CHECKING: # pragma: no cover + assert self.cluster.leader is not None try: - ret = self.dcs.update_leader(last_lsn, slots, self._failsafe_config()) + ret = self.dcs.update_leader(self.cluster.leader, last_lsn, slots, self._failsafe_config()) except DCSError: raise except Exception: diff --git a/tests/test_consul.py b/tests/test_consul.py index 165d2382..ebbbb90e 100644 --- a/tests/test_consul.py +++ b/tests/test_consul.py @@ -176,23 +176,24 @@ class TestConsul(unittest.TestCase): @patch.object(consul.Consul.Session, 'renew') @patch.object(consul.Consul.KV, 'put', Mock(side_effect=ConsulException)) def test_update_leader(self, mock_renew): + leader = self.c.get_cluster().leader self.c._session = 'fd4f44fe-2cac-bba5-a60b-304b51ff39b8' with patch.object(consul.Consul.KV, 'delete', Mock(return_value=True)): with patch.object(consul.Consul.KV, 'put', Mock(return_value=True)): - self.assertTrue(self.c.update_leader(12345, failsafe={'foo': 'bar'})) + self.assertTrue(self.c.update_leader(leader, 12345, failsafe={'foo': 'bar'})) with patch.object(consul.Consul.KV, 'put', Mock(side_effect=ConsulException)): - self.assertFalse(self.c.update_leader(12345)) - with patch('time.time', Mock(side_effect=[0, 0, 0, 0, 0, 100, 200, 300])): - self.assertRaises(ConsulError, self.c.update_leader, 12345) + self.assertFalse(self.c.update_leader(leader, 12345)) + with patch('time.time', Mock(side_effect=[0, 0, 0, 0, 100, 200, 300])): + self.assertRaises(ConsulError, self.c.update_leader, leader, 12345) with patch('time.time', Mock(side_effect=[0, 100, 200, 300])): - self.assertRaises(ConsulError, self.c.update_leader, 12345) + self.assertRaises(ConsulError, self.c.update_leader, leader, 12345) with patch.object(consul.Consul.KV, 'delete', Mock(side_effect=ConsulException)): - self.assertFalse(self.c.update_leader(12347)) + self.assertFalse(self.c.update_leader(leader, 12347)) mock_renew.side_effect = RetryFailedError('') self.c._last_session_refresh = 0 - self.assertRaises(ConsulError, self.c.update_leader, 12346) + self.assertRaises(ConsulError, self.c.update_leader, leader, 12346) mock_renew.side_effect = ConsulException - self.assertFalse(self.c.update_leader(12347)) + self.assertFalse(self.c.update_leader(leader, 12347)) @patch.object(consul.Consul.KV, 'delete', Mock(return_value=True)) def test_delete_leader(self): diff --git a/tests/test_etcd.py b/tests/test_etcd.py index 6699127a..b5add201 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -296,14 +296,15 @@ class TestEtcd(unittest.TestCase): self.etcd.write_leader_optime('0') def test_update_leader(self): - self.assertTrue(self.etcd.update_leader(None, failsafe={'foo': 'bar'})) + leader = self.etcd.get_cluster().leader + self.assertTrue(self.etcd.update_leader(leader, None, failsafe={'foo': 'bar'})) with patch.object(etcd.Client, 'write', Mock(side_effect=[etcd.EtcdConnectionFailed, etcd.EtcdClusterIdChanged, Exception])): - self.assertRaises(EtcdError, self.etcd.update_leader, None) - self.assertFalse(self.etcd.update_leader(None)) - self.assertRaises(EtcdError, self.etcd.update_leader, None) + self.assertRaises(EtcdError, self.etcd.update_leader, leader, None) + self.assertFalse(self.etcd.update_leader(leader, None)) + self.assertRaises(EtcdError, self.etcd.update_leader, leader, None) with patch.object(etcd.Client, 'write', Mock(side_effect=etcd.EtcdKeyNotFound)): - self.assertFalse(self.etcd.update_leader(None)) + self.assertFalse(self.etcd.update_leader(leader, None)) def test_initialize(self): self.assertFalse(self.etcd.initialize()) diff --git a/tests/test_etcd3.py b/tests/test_etcd3.py index 4b3a78ab..a737f199 100644 --- a/tests/test_etcd3.py +++ b/tests/test_etcd3.py @@ -235,19 +235,20 @@ class TestEtcd3(BaseTestEtcd3): self.etcd3.touch_member({}) def test__update_leader(self): + leader = self.etcd3.get_cluster().leader self.etcd3._lease = None with patch.object(Etcd3Client, 'txn', Mock(return_value={'succeeded': True})): - self.etcd3.update_leader('123', failsafe={'foo': 'bar'}) + self.etcd3.update_leader(leader, '123', failsafe={'foo': 'bar'}) self.etcd3._last_lease_refresh = 0 - self.etcd3.update_leader('124') + self.etcd3.update_leader(leader, '124') with patch.object(PatroniEtcd3Client, 'lease_keepalive', Mock(return_value=True)),\ patch('time.time', Mock(side_effect=[0, 100, 200, 300])): - self.assertRaises(Etcd3Error, self.etcd3.update_leader, '126') - self.etcd3._lease = self.etcd3.cluster.leader.session - self.etcd3.update_leader('124') + self.assertRaises(Etcd3Error, self.etcd3.update_leader, leader, '126') + self.etcd3._lease = leader.session + self.etcd3.update_leader(leader, '124') self.etcd3._last_lease_refresh = 0 with patch.object(PatroniEtcd3Client, 'lease_keepalive', Mock(side_effect=Unknown)): - self.assertFalse(self.etcd3.update_leader('125')) + self.assertFalse(self.etcd3.update_leader(leader, '125')) def test_take_leader(self): self.assertFalse(self.etcd3.take_leader()) diff --git a/tests/test_kubernetes.py b/tests/test_kubernetes.py index fbefb448..cfed1559 100644 --- a/tests/test_kubernetes.py +++ b/tests/test_kubernetes.py @@ -340,35 +340,37 @@ class TestKubernetesEndpoints(BaseTestKubernetes): @patch.object(k8s_client.CoreV1Api, 'patch_namespaced_endpoints', create=True) def test_update_leader(self, mock_patch_namespaced_endpoints): - self.assertIsNotNone(self.k.update_leader('123', failsafe={'foo': 'bar'})) + leader = self.k.get_cluster().leader + self.assertIsNotNone(self.k.update_leader(leader, '123', failsafe={'foo': 'bar'})) args = mock_patch_namespaced_endpoints.call_args[0] self.assertEqual(args[2].subsets[0].addresses[0].target_ref.resource_version, '10') self.k._kinds._object_cache['test'].subsets[:] = [] - self.assertIsNotNone(self.k.update_leader('123')) + self.assertIsNotNone(self.k.update_leader(leader, '123')) self.k._kinds._object_cache['test'].metadata.annotations['leader'] = 'p-1' - self.assertFalse(self.k.update_leader('123')) + self.assertFalse(self.k.update_leader(leader, '123')) @patch.object(k8s_client.CoreV1Api, 'read_namespaced_endpoints', create=True) @patch.object(k8s_client.CoreV1Api, 'patch_namespaced_endpoints', create=True) def test__update_leader_with_retry(self, mock_patch, mock_read): + leader = self.k.get_cluster().leader mock_read.return_value = mock_read_namespaced_endpoints() mock_patch.side_effect = k8s_client.rest.ApiException(502, '') - self.assertFalse(self.k.update_leader('123')) + self.assertFalse(self.k.update_leader(leader, '123')) mock_patch.side_effect = RetryFailedError('') - self.assertRaises(KubernetesError, self.k.update_leader, '123') + self.assertRaises(KubernetesError, self.k.update_leader, leader, '123') mock_patch.side_effect = k8s_client.rest.ApiException(409, '') with patch('time.time', Mock(side_effect=[0, 100, 200, 0, 0, 0, 0, 100, 200])): - self.assertFalse(self.k.update_leader('123')) - self.assertFalse(self.k.update_leader('123')) - self.assertFalse(self.k.update_leader('123')) + self.assertFalse(self.k.update_leader(leader, '123')) + self.assertFalse(self.k.update_leader(leader, '123')) + self.assertFalse(self.k.update_leader(leader, '123')) mock_patch.side_effect = [k8s_client.rest.ApiException(409, ''), mock_namespaced_kind()] mock_read.return_value.metadata.resource_version = '2' self.assertIsNotNone(self.k._update_leader_with_retry({}, '1', [])) mock_patch.side_effect = k8s_client.rest.ApiException(409, '') mock_read.side_effect = RetryFailedError('') - self.assertRaises(KubernetesError, self.k.update_leader, '123') + self.assertRaises(KubernetesError, self.k.update_leader, leader, '123') mock_read.side_effect = Exception - self.assertFalse(self.k.update_leader('123')) + self.assertFalse(self.k.update_leader(leader, '123')) @patch.object(k8s_client.CoreV1Api, 'patch_namespaced_endpoints', Mock(side_effect=[k8s_client.rest.ApiException(500, ''), diff --git a/tests/test_raft.py b/tests/test_raft.py index 2029a61e..ab68c2e6 100644 --- a/tests/test_raft.py +++ b/tests/test_raft.py @@ -146,8 +146,8 @@ class TestRaft(unittest.TestCase): self.assertIsInstance(cluster, Cluster) self.assertIsInstance(cluster.workers[1], Cluster) self.assertTrue(raft._sync_obj.set(raft.status_path, '{"optime":1234567,"slots":{"ls":12345}}')) - raft.get_cluster() - self.assertTrue(raft.update_leader('1', failsafe={'foo': 'bat'})) + leader = raft.get_cluster().leader + self.assertTrue(raft.update_leader(leader, '1', failsafe={'foo': 'bat'})) self.assertTrue(raft._sync_obj.set(raft.failsafe_path, '{"foo"}')) self.assertTrue(raft._sync_obj.set(raft.status_path, '{')) raft.get_citus_coordinator() diff --git a/tests/test_zookeeper.py b/tests/test_zookeeper.py index c2f3f583..baf9ad10 100644 --- a/tests/test_zookeeper.py +++ b/tests/test_zookeeper.py @@ -250,14 +250,15 @@ class TestZooKeeper(unittest.TestCase): self.zk.take_leader() def test_update_leader(self): - self.assertFalse(self.zk.update_leader(12345)) + leader = self.zk.get_cluster().leader + self.assertFalse(self.zk.update_leader(leader, 12345)) with patch.object(MockKazooClient, 'delete', Mock(side_effect=RetryFailedError)): - self.assertRaises(ZooKeeperError, self.zk.update_leader, 12345) + self.assertRaises(ZooKeeperError, self.zk.update_leader, leader, 12345) with patch.object(MockKazooClient, 'delete', Mock(side_effect=NoNodeError)): - self.assertTrue(self.zk.update_leader(12345, failsafe={'foo': 'bar'})) + self.assertTrue(self.zk.update_leader(leader, 12345, failsafe={'foo': 'bar'})) with patch.object(MockKazooClient, 'create', Mock(side_effect=[RetryFailedError, Exception])): - self.assertRaises(ZooKeeperError, self.zk.update_leader, 12345) - self.assertFalse(self.zk.update_leader(12345)) + self.assertRaises(ZooKeeperError, self.zk.update_leader, leader, 12345) + self.assertFalse(self.zk.update_leader(leader, 12345)) @patch.object(Cluster, 'min_version', PropertyMock(return_value=(2, 0))) def test_write_leader_optime(self): From 2158f4a87b15861886cf26fe298937de86c6fc58 Mon Sep 17 00:00:00 2001 From: Matt Baker <93600443+matthbakeredb@users.noreply.github.com> Date: Fri, 26 May 2023 08:35:12 +0100 Subject: [PATCH 09/12] Add base image build arg for alt postgres (#2695) Allows for running behave tests with an alternative base image than the official postgres image. Also provides a PG_USER/PG_GROUP should that be different to the default `postgres`. --- features/Dockerfile | 13 ++++++++----- tox.ini | 3 +++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/features/Dockerfile b/features/Dockerfile index e2f5dbb9..86c6041d 100644 --- a/features/Dockerfile +++ b/features/Dockerfile @@ -4,14 +4,17 @@ ARG PG_MAJOR ARG PGHOME=/home/postgres ARG LC_ALL=C.UTF-8 ARG LANG=C.UTF-8 +ARG BASE_IMAGE=postgres -FROM postgres:${PG_MAJOR} +FROM ${BASE_IMAGE}:${PG_MAJOR} ARG PGHOME ARG LC_ALL ARG LANG ENV PGHOME="$PGHOME" +ENV PG_USER="${PG_USER:-postgres}" +ENV PG_GROUP="${PG_GROUP:-$PG_USER}" ENV LC_ALL="$LC_ALL" ENV LANG="$LANG" @@ -41,7 +44,7 @@ RUN set -ex \ \ && mkdir -p "$PGHOME" \ && sed -i "s|/var/lib/postgresql.*|$PGHOME:/bin/bash|" /etc/passwd \ - && chown -R postgres:postgres /var/log /home/postgres \ + && chown -R "$PG_USER:$PG_GROUP" /var/log /home/postgres \ \ # Download etcd \ && curl -sL "$ETCDURL/etcd-v$ETCDVERSION-linux-$(dpkg --print-architecture).tar.gz" \ @@ -67,18 +70,18 @@ trap 'copy_output' SIGTERM # so we can tell etcd we're ok with running an unsupported architecture. export ETCD_UNSUPPORTED_ARCH=$(go env GOARCH) cd /src -runuser -u postgres -- \\ +runuser -u "\$PG_USER" -- \\ find . ! -readable 2>/dev/null \\ | sed 's|^./||' >/tmp/copy_exclude.lst \\ || true -runuser -u postgres -- \\ +runuser -u "\$PG_USER" -- \\ rsync -a \\ --exclude=.tox \\ --exclude="features/output*" \\ --exclude-from="/tmp/copy_exclude.lst" \\ . "\$PGHOME/src/" cd "\$PGHOME/src" -runuser -u postgres -w ETCD_UNSUPPORTED_ARCH -- "\$@" & +runuser -u "\$PG_USER" -w ETCD_UNSUPPORTED_ARCH -- "\$@" & wait $! # SIGINT whilst child proc is running is not seen by trap so we run a copy here instead of using # trap copy_output SIGINT EXIT diff --git a/tox.ini b/tox.ini index 72353d4f..71104357 100644 --- a/tox.ini +++ b/tox.ini @@ -113,10 +113,13 @@ labels = setenv = {[common]postgres_matrix} DOCKER_BUILDKIT = 1 +passenv = + BASE_IMAGE commands = docker build . \ --tag patroni-dev:{env:PG_MAJOR} \ --build-arg PG_MAJOR \ + --build-arg BASE_IMAGE={env:BASE_IMAGE:postgres} \ --file features/Dockerfile allowlist_externals = docker From 101ea10e98c6f5a901be15cd184487383081c7f1 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 26 May 2023 11:04:58 +0200 Subject: [PATCH 10/12] Introduce Retry.ensure_deadline() method (#2694) it helps to get rid of recurring patterns: ```python retry.deadline = retry.stoptime - time.time() if retry.deadline < XXX: raise Exception(...) or return False ``` --- patroni/dcs/consul.py | 21 ++++++--------------- patroni/dcs/etcd3.py | 14 ++++---------- patroni/dcs/kubernetes.py | 6 ++---- patroni/utils.py | 15 +++++++++++++++ 4 files changed, 27 insertions(+), 29 deletions(-) diff --git a/patroni/dcs/consul.py b/patroni/dcs/consul.py index 87ae4f6a..5327be0f 100644 --- a/patroni/dcs/consul.py +++ b/patroni/dcs/consul.py @@ -549,13 +549,11 @@ class Consul(AbstractDCS): except InvalidSession: logger.error('Our session disappeared from Consul. Will try to get a new one and retry attempt') self._session = None - retry.deadline = retry.stoptime - time.time() + retry.ensure_deadline(0) retry(self._do_refresh_session) - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 1: - raise ConsulError('_do_attempt_to_acquire_leader timeout') + retry.ensure_deadline(1, ConsulError('_do_attempt_to_acquire_leader timeout')) return retry(self._client.kv.put, self.leader_path, self._name, acquire=self._session) @@ -564,9 +562,7 @@ class Consul(AbstractDCS): retry = self._retry.copy() self._run_and_handle_exceptions(self._do_refresh_session, retry=retry) - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 1: - raise ConsulError('attempt_to_acquire_leader timeout') + retry.ensure_deadline(1, ConsulError('attempt_to_acquire_leader timeout')) ret = self._run_and_handle_exceptions(self._do_attempt_to_acquire_leader, retry, retry=None) if not ret: @@ -614,16 +610,12 @@ class Consul(AbstractDCS): self._run_and_handle_exceptions(self._do_refresh_session, True, retry=retry) if self._session and leader.session != self._session: - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 1: - raise ConsulError('update_leader timeout') + retry.ensure_deadline(1, ConsulError('update_leader timeout')) logger.warning('Recreating the leader key due to session mismatch') self._run_and_handle_exceptions(self._client.kv.delete, self.leader_path, cas=leader.version) - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 0.5: - raise ConsulError('update_leader timeout') + retry.ensure_deadline(0.5, ConsulError('update_leader timeout')) self._run_and_handle_exceptions(self._client.kv.put, self.leader_path, self._name, acquire=self._session) @@ -659,8 +651,7 @@ class Consul(AbstractDCS): retry = self._retry.copy() ret = retry(self._client.kv.put, self.sync_path, value, cas=version) if ret: # We have no other choise, only read after write :( - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 0.5: + if not retry.ensure_deadline(0.5): return False _, ret = self.retry(self._client.kv.get, self.sync_path) if ret and (ret.get('Value') or b'').decode('utf-8') == value: diff --git a/patroni/dcs/etcd3.py b/patroni/dcs/etcd3.py index 4d0ff75d..b89e36a5 100644 --- a/patroni/dcs/etcd3.py +++ b/patroni/dcs/etcd3.py @@ -827,13 +827,11 @@ class Etcd3(AbstractEtcd): except LeaseNotFound: logger.error('Our lease disappeared from Etcd. Will try to get a new one and retry attempt') self._lease = None - retry.deadline = retry.stoptime - time.time() + retry.ensure_deadline(0) _retry(self._do_refresh_lease) - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 1: - raise Etcd3Error('_do_attempt_to_acquire_leader timeout') + retry.ensure_deadline(1, Etcd3Error('_do_attempt_to_acquire_leader timeout')) return _retry(self._client.put, self.leader_path, self._name, self._lease, create_revision='0') @@ -847,9 +845,7 @@ class Etcd3(AbstractEtcd): self._run_and_handle_exceptions(self._do_refresh_lease, retry=_retry) - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 1: - raise Etcd3Error('attempt_to_acquire_leader timeout') + retry.ensure_deadline(1, Etcd3Error('attempt_to_acquire_leader timeout')) ret = self._run_and_handle_exceptions(self._do_attempt_to_acquire_leader, retry, retry=None) if not ret: @@ -887,9 +883,7 @@ class Etcd3(AbstractEtcd): self._run_and_handle_exceptions(self._do_refresh_lease, True, retry=_retry) if self._lease and leader.session != self._lease: - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 1: - raise Etcd3Error('update_leader timeout') + retry.ensure_deadline(1, Etcd3Error('update_leader timeout')) fields = {'key': base64_encode(self.leader_path), 'value': base64_encode(self._name), 'lease': self._lease} # First we try to update lease on existing leader key "hoping" that we still owning it diff --git a/patroni/dcs/kubernetes.py b/patroni/dcs/kubernetes.py index ac65652d..0d457ba4 100644 --- a/patroni/dcs/kubernetes.py +++ b/patroni/dcs/kubernetes.py @@ -1153,8 +1153,7 @@ class Kubernetes(AbstractDCS): raise KubernetesError(e) # if we are here, that means update failed with 409 - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 1: + if not retry.ensure_deadline(1): return False # No time for retry. Tell ha.py that we have to demote due to failed update. # Try to get the latest version directly from K8s API instead of relying on async cache @@ -1168,8 +1167,7 @@ class Kubernetes(AbstractDCS): self._kinds.set(self.leader_path, kind) - retry.deadline = retry.stoptime - time.time() - if retry.deadline < 0.5: + if not retry.ensure_deadline(0.5): return False kind_annotations = kind and kind.metadata.annotations or {} diff --git a/patroni/utils.py b/patroni/utils.py index f6cfa919..eb02c561 100644 --- a/patroni/utils.py +++ b/patroni/utils.py @@ -531,6 +531,21 @@ class Retry(object): """Get the current stop time.""" return self._cur_stoptime or 0 + def ensure_deadline(self, timeout: float, raise_ex: Optional[Exception] = None) -> bool: + """Calculates, sets, and checks the remaining deadline time. + + :param timeout: if the *deadline* is smaller than the provided *timeout* value raise *raise_ex* exception + :param raise_ex: the exception object that will be raised if the *deadline* is smaller than provided *timeout* + :returns: `False` if *deadline* is smaller than a provided *timeout* and *raise_ex* isn't set. Otherwise `True` + :raises Exception: if calculated deadline is smaller than provided *timeout* + """ + self.deadline = self.stoptime - time.time() + if self.deadline < timeout: + if raise_ex: + raise raise_ex + return False + return True + def __call__(self, func: Callable[..., Any], *args: Any, **kwargs: Any) -> Any: """Call a function *func* with arguments ``*args`` and ``*kwargs`` in a loop. From 37fffa618f9ec2fdc09abde6faa6b40b28151e0e Mon Sep 17 00:00:00 2001 From: Polina Bungina <27892524+hughcapet@users.noreply.github.com> Date: Fri, 26 May 2023 15:13:04 +0200 Subject: [PATCH 11/12] Refactor daemon entrypoints (#2697) - abstract_main only creates Config object using the passed configfile and instantiates the passed daemon class - common args parser is extracted into a separate func that is called from daemons' main funcs (specific args can be added afterwards) Co-authored-by: Alexander Kukushkin --- patroni/__main__.py | 33 ++++++++++++++++++++----- patroni/daemon.py | 50 ++++++++++++++++---------------------- patroni/raft_controller.py | 7 ++++-- tests/test_patroni.py | 12 ++++----- 4 files changed, 59 insertions(+), 43 deletions(-) diff --git a/patroni/__main__.py b/patroni/__main__.py index 9c7f5cea..2a669d3f 100644 --- a/patroni/__main__.py +++ b/patroni/__main__.py @@ -1,11 +1,13 @@ import logging import os import signal +import sys import time +from argparse import Namespace from typing import Any, Dict, Optional, TYPE_CHECKING -from patroni.daemon import AbstractPatroniDaemon, abstract_main +from patroni.daemon import AbstractPatroniDaemon, abstract_main, get_base_arg_parser if TYPE_CHECKING: # pragma: no cover from .config import Config @@ -133,21 +135,40 @@ class Patroni(AbstractPatroniDaemon): logger.exception('Exception during Ha.shutdown') -def patroni_main() -> None: +def patroni_main(configfile: str) -> None: from multiprocessing import freeze_support - from patroni.validator import schema freeze_support() - abstract_main(Patroni, schema) + abstract_main(Patroni, configfile) + + +def process_arguments() -> Namespace: + parser = get_base_arg_parser() + parser.add_argument('--validate-config', action='store_true', help='Run config validator and exit') + args = parser.parse_args() + + if args.validate_config: + from patroni.validator import schema + from patroni.config import Config, ConfigParseError + + try: + Config(args.configfile, validator=schema) + sys.exit() + except ConfigParseError as e: + sys.exit(e.value) + + return args def main() -> None: from patroni import check_psycopg + args = process_arguments() + check_psycopg() if os.getpid() != 1: - return patroni_main() + return patroni_main(args.configfile) # Patroni started with PID=1, it looks like we are in the container from types import FrameType @@ -180,7 +201,7 @@ def main() -> None: signal.signal(signal.SIGTERM, passtochild) import multiprocessing - patroni = multiprocessing.Process(target=patroni_main) + patroni = multiprocessing.Process(target=patroni_main, args=(args.configfile,)) patroni.start() pid = patroni.pid patroni.join() diff --git a/patroni/daemon.py b/patroni/daemon.py index 88f6b133..e160ce46 100644 --- a/patroni/daemon.py +++ b/patroni/daemon.py @@ -6,6 +6,7 @@ Currently it is only used for the main "Thread" of ``patroni`` and ``patroni_raf from __future__ import print_function import abc +import argparse import os import signal import sys @@ -15,7 +16,22 @@ from typing import Any, Optional, Type, TYPE_CHECKING if TYPE_CHECKING: # pragma: no cover from .config import Config - from .validator import Schema + + +def get_base_arg_parser() -> argparse.ArgumentParser: + """Create a basic argument parser with the arguments used for both patroni and raft controller daemon. + + :returns: 'argparse.ArgumentParser' object + """ + from .config import Config + from .version import __version__ + + parser = argparse.ArgumentParser() + parser.add_argument('--version', action='version', version='%(prog)s {0}'.format(__version__)) + parser.add_argument('configfile', nargs='?', default='', + help='Patroni may also read the configuration from the {0} environment variable' + .format(Config.PATRONI_CONFIG_VARIABLE)) + return parser class AbstractPatroniDaemon(abc.ABC): @@ -141,41 +157,17 @@ class AbstractPatroniDaemon(abc.ABC): self.logger.shutdown() -def abstract_main(cls: Type[AbstractPatroniDaemon], validator: Optional['Schema'] = None) -> None: +def abstract_main(cls: Type[AbstractPatroniDaemon], configfile: str) -> None: """Create the main entry point of a given daemon process. - Expose a basic argument parser, parse the command-line arguments, and run the given daemon process. - :param cls: a class that should inherit from :class:`AbstractPatroniDaemon`. - :param validator: used to validate the daemon configuration schema, if requested by the user through - ``--validate-config`` CLI option. + :param configfile: """ - import argparse - from .config import Config, ConfigParseError - from .version import __version__ - - parser = argparse.ArgumentParser() - parser.add_argument('--version', action='version', version='%(prog)s {0}'.format(__version__)) - if validator: - parser.add_argument('--validate-config', action='store_true', help='Run config validator and exit') - parser.add_argument('configfile', nargs='?', default='', - 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 validate_config: - Config(args.configfile, validator=validator) - sys.exit() - - config = Config(args.configfile) + config = Config(configfile) except ConfigParseError as e: - if e.value: - print(e.value, file=sys.stderr) - if not validate_config: - parser.print_help() - sys.exit(1) + sys.exit(e.value) controller = cls(config) try: diff --git a/patroni/raft_controller.py b/patroni/raft_controller.py index 2f9f7858..a9d7424c 100644 --- a/patroni/raft_controller.py +++ b/patroni/raft_controller.py @@ -1,7 +1,7 @@ import logging from .config import Config -from .daemon import AbstractPatroniDaemon, abstract_main +from .daemon import AbstractPatroniDaemon, abstract_main, get_base_arg_parser from .dcs.raft import KVStoreTTL logger = logging.getLogger(__name__) @@ -27,4 +27,7 @@ class RaftController(AbstractPatroniDaemon): def main() -> None: - abstract_main(RaftController) + parser = get_base_arg_parser() + args = parser.parse_args() + + abstract_main(RaftController, args.configfile) diff --git a/tests/test_patroni.py b/tests/test_patroni.py index ad8e775c..cb40b232 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -15,7 +15,7 @@ from patroni.exceptions import DCSError from patroni.postgresql import Postgresql from patroni.postgresql.config import ConfigHandler from patroni import check_psycopg -from patroni.__main__ import Patroni, main as _main, patroni_main +from patroni.__main__ import Patroni, main as _main from threading import Thread from . import psycopg_connect, SleepException @@ -52,14 +52,14 @@ class TestPatroni(unittest.TestCase): @patch('sys.argv', ['patroni.py']) def test_no_config(self): - self.assertRaises(SystemExit, patroni_main) + self.assertRaises(SystemExit, _main) @patch('sys.argv', ['patroni.py', '--validate-config', 'postgres0.yml']) @patch('socket.socket.connect_ex', Mock(return_value=1)) def test_validate_config(self): - self.assertRaises(SystemExit, patroni_main) + self.assertRaises(SystemExit, _main) with patch.object(config.Config, '__init__', Mock(return_value=None)): - self.assertRaises(SystemExit, patroni_main) + self.assertRaises(SystemExit, _main) @patch('pkgutil.iter_importers', Mock(return_value=[MockFrozenImporter()])) @patch('sys.frozen', Mock(return_value=True), create=True) @@ -94,11 +94,11 @@ class TestPatroni(unittest.TestCase): with patch('subprocess.call', Mock(return_value=1)): with patch.object(Patroni, 'run', Mock(side_effect=SleepException)): os.environ['PATRONI_POSTGRESQL_DATA_DIR'] = 'data/test0' - self.assertRaises(SleepException, patroni_main) + self.assertRaises(SleepException, _main) with patch.object(Patroni, 'run', Mock(side_effect=KeyboardInterrupt())): with patch('patroni.ha.Ha.is_paused', Mock(return_value=True)): os.environ['PATRONI_POSTGRESQL_DATA_DIR'] = 'data/test0' - patroni_main() + _main() @patch('os.getpid') @patch('multiprocessing.Process') From d11328020de740523b42865c629e35323af43ae5 Mon Sep 17 00:00:00 2001 From: Israel Date: Tue, 30 May 2023 08:57:57 -0300 Subject: [PATCH 12/12] Add support for custom Postgres binary names (#2692) When using a custom Postgres distribution it may be the case that the Postgres binaries are compiled with different names other than the ones used by the community Postgres distribution. With that in mind we implemented a new set of settings for Patroni, so the user is able to override the default binary names with custom binary names through the new section postgresql.bin_name in the local configuration. References: PAT-17. --- docs/ENVIRONMENT.rst | 2 +- docs/yaml_configuration.rst | 11 ++++- patroni/postgresql/__init__.py | 13 ++++- patroni/validator.py | 89 ++++++++++++++++++++++++++++++++-- tests/test_validator.py | 35 +++++++++++++ 5 files changed, 142 insertions(+), 8 deletions(-) diff --git a/docs/ENVIRONMENT.rst b/docs/ENVIRONMENT.rst index d7e9670b..d2f8a49b 100644 --- a/docs/ENVIRONMENT.rst +++ b/docs/ENVIRONMENT.rst @@ -135,7 +135,7 @@ PostgreSQL - **PATRONI\_POSTGRESQL\_PROXY\_ADDRESS**: IP address + port through which a connection pool (e.g. pgbouncer) running next to Postgres is accessible. The value is written to the member key in DCS as ``proxy_url`` and could be used/useful for service discovery. - **PATRONI\_POSTGRESQL\_DATA\_DIR**: The location of the Postgres data directory, either existing or to be initialized by Patroni. - **PATRONI\_POSTGRESQL\_CONFIG\_DIR**: The location of the Postgres configuration directory, defaults to the data directory. Must be writable by Patroni. -- **PATRONI\_POSTGRESQL\_BIN_DIR**: Path to PostgreSQL binaries. (pg_ctl, pg_rewind, pg_basebackup, postgres) The default value is an empty string meaning that PATH environment variable will be used to find the executables. +- **PATRONI\_POSTGRESQL\_BIN_DIR**: Path to PostgreSQL binaries. (pg_ctl, initdb, pg_controldata, pg_basebackup, postgres, pg_isready, pg_rewind) The default value is an empty string meaning that PATH environment variable will be used to find the executables. - **PATRONI\_POSTGRESQL\_PGPASS**: path to the `.pgpass `__ password file. Patroni creates this file before executing pg\_basebackup and under some other circumstances. The location must be writable by Patroni. - **PATRONI\_REPLICATION\_USERNAME**: replication username; the user will be created during initialization. Replicas will use this user to access the replication source via streaming replication - **PATRONI\_REPLICATION\_PASSWORD**: replication password; the user will be created during initialization. diff --git a/docs/yaml_configuration.rst b/docs/yaml_configuration.rst index 598143a7..f58c9b0a 100644 --- a/docs/yaml_configuration.rst +++ b/docs/yaml_configuration.rst @@ -258,7 +258,16 @@ PostgreSQL own config item. See :ref:`custom replica creation methods documentation ` for further explanation. - **data\_dir**: The location of the Postgres data directory, either :ref:`existing ` or to be initialized by Patroni. - **config\_dir**: The location of the Postgres configuration directory, defaults to the data directory. Must be writable by Patroni. - - **bin\_dir**: (optional) Path to PostgreSQL binaries (pg_ctl, pg_rewind, pg_basebackup, postgres). If not provided or is an empty string, PATH environment variable will be used to find the executables. + - **bin\_dir**: (optional) Path to PostgreSQL binaries (pg_ctl, initdb, pg_controldata, pg_basebackup, postgres, pg_isready, pg_rewind). If not provided or is an empty string, PATH environment variable will be used to find the executables. + - **bin\_name**: (optional) Make it possible to override Postgres binary names, if you are using a custom Postgres distribution: + + - **pg\_ctl**: (optional) Custom name for ``pg_ctl`` binary. + - **initdb**: (optional) Custom name for ``initdb`` binary. + - **pg\controldata**: (optional) Custom name for ``pg_controldata`` binary. + - **pg\_basebackup**: (optional) Custom name for ``pg_basebackup`` binary. + - **postgres**: (optional) Custom name for ``postgres`` binary. + - **pg\_isready**: (optional) Custom name for ``pg_isready`` binary. + - **pg\_rewind**: (optional) Custom name for ``pg_rewind`` binary. - **listen**: IP address + port that Postgres listens to; must be accessible from other nodes in the cluster, if you're using streaming replication. Multiple comma-separated addresses are permitted, as long as the port component is appended after to the last one with a colon, i.e. ``listen: 127.0.0.1,127.0.0.2:5432``. Patroni will use the first address from this list to establish local connections to the PostgreSQL node. - **use\_unix\_socket**: specifies that Patroni should prefer to use unix sockets to connect to the cluster. Default value is ``false``. If ``unix_socket_directories`` is defined, Patroni will use the first suitable value from it to connect to the cluster and fallback to tcp if nothing is suitable. If ``unix_socket_directories`` is not specified in ``postgresql.parameters``, Patroni will assume that the default value should be used and omit ``host`` from the connection parameters. - **use\_unix\_socket\_repl**: specifies that Patroni should prefer to use unix sockets for replication user cluster connection. Default value is ``false``. If ``unix_socket_directories`` is defined, Patroni will use the first suitable value from it to connect to the cluster and fallback to tcp if nothing is suitable. If ``unix_socket_directories`` is not specified in ``postgresql.parameters``, Patroni will assume that the default value should be used and omit ``host`` from the connection parameters. diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py index 305aa4ce..6d2d18af 100644 --- a/patroni/postgresql/__init__.py +++ b/patroni/postgresql/__init__.py @@ -227,8 +227,17 @@ class Postgresql(object): return 0 def pgcommand(self, cmd: str) -> str: - """Returns path to the specified PostgreSQL command""" - return os.path.join(self._bin_dir, cmd) + """Return path to the specified PostgreSQL command. + + .. note:: + If ``postgresql.bin_name.*cmd*`` was configured by the user then that binary name is used, otherwise the + default binary name *cmd* is used. + + :param cmd: the Postgres binary name to get path to. + + :returns: path to Postgres binary named *cmd*. + """ + return os.path.join(self._bin_dir, (self.config.get('bin_name', {}) or {}).get(cmd, cmd)) def pg_ctl(self, cmd: str, *args: str, **kwargs: Any) -> bool: """Builds and executes pg_ctl command diff --git a/patroni/validator.py b/patroni/validator.py index 9a4b117c..f780f9e0 100644 --- a/patroni/validator.py +++ b/patroni/validator.py @@ -176,6 +176,16 @@ def is_ipv6_address(ip: str) -> bool: return True +def get_bin_name(bin_name: str) -> str: + """Get the value of ``postgresql.bin_name[*bin_name*]`` configuration option. + + :param bin_name: a key to be retrieved from ``postgresql.bin_name`` configuration. + + :returns: value of ``postgresql.bin_name[*bin_name*]``, if present, otherwise *bin_name*. + """ + return (schema.data.get('postgresql', {}).get('bin_name', {}) or {}).get(bin_name, bin_name) + + def get_major_version(bin_dir: OptionalType[str] = None) -> str: """Get the major version of PostgreSQL. @@ -191,9 +201,9 @@ def get_major_version(bin_dir: OptionalType[str] = None) -> str: * Returns `15` for PostgreSQL 15.2 """ if not bin_dir: - binary = 'postgres' + binary = get_bin_name('postgres') else: - binary = os.path.join(bin_dir, 'postgres') + binary = os.path.join(bin_dir, get_bin_name('postgres')) version = subprocess.check_output([binary, '--version']).decode() version = re.match(r'^[^\s]+ [^\s]+ (\d+)(\.(\d+))?', version) if TYPE_CHECKING: # pragma: no cover @@ -242,6 +252,38 @@ def validate_data_dir(data_dir: str) -> bool: return True +def validate_binary_name(bin_name: str) -> bool: + """Validate the value of ``postgresql.binary_name[*bin_name*]`` configuration option. + + If ``postgresql.bin_dir`` is set and the value of the *bin_name* meets these conditions: + + * The path join of ``postgresql.bin_dir`` plus the *bin_name* value exists; and + * The path join as above is executable + + If ``postgresql.bin_dir`` is not set, then validate that the value of *bin_name* meets this + condition: + + * Is found in the system PATH using ``which`` + + :param bin_name: the value of the ``postgresql.bin_name[*bin_name*]`` + + :returns: ``True`` if the conditions are true + + :raises :class:`patroni.exceptions.ConfigParserError`: if: + * *bin_name* is not set; or + * the path join of the ``postgresql.bin_dir`` plus *bin_name* does not exist; or + * the path join as above is not executable; or + * the *bin_name* cannot be found in the system PATH + + """ + if not bin_name: + raise ConfigParseError("is an empty string") + bin_dir = schema.data.get('postgresql', {}).get('bin_dir', None) + if not shutil.which(bin_name, path=bin_dir): + raise ConfigParseError(f"does not contain '{bin_name}' in '{bin_dir or '$PATH'}'") + return True + + class Result(object): """Represent the result of a given validation that was performed. @@ -406,6 +448,31 @@ class Directory(object): yield from self._check_executables(path=name) +class BinDirectory(Directory): + """Check if a Postgres binary directory contains the expected files. + + It is a subclass of :class:`Directory` with an extended capability: translating ``BINARIES`` according to configured + ``postgresql.bin_name``, if any. + + :cvar BINARIES: list of executable files that should exist directly under a given Postgres binary directory. + """ + + # ``pg_rewind`` is not in the list because its usage by Patroni is optional. Also, it is not available by default on + # Postgres 9.3 and 9.4, versions which Patroni supports. + BINARIES = ["pg_ctl", "initdb", "pg_controldata", "pg_basebackup", "postgres", "pg_isready"] + + def validate(self, name: str) -> Iterator[Result]: + """Check if the expected executables can be found under *name* binary directory. + + :param name: path to the base directory against which executables will be validated. Check against PATH if + *name* is not provided. + + :yields: objects with the error message related to the failure, if any check fails. + """ + self.contains_executable: List[str] = [get_bin_name(binary) for binary in self.BINARIES] + yield from super().validate(name) + + class Schema(object): """Define a configuration schema. @@ -700,6 +767,7 @@ class IntValidator(object): :ivar base_unit: the base unit to convert the value to before checking if it's within `min` and `max` range. :ivar raise_assert: if an ``assert`` call should be performed regarding expected type and valid range. """ + expected_type = int def __init__(self, min: OptionalType[int] = None, max: OptionalType[int] = None, @@ -736,6 +804,10 @@ class IntValidator(object): def validate_watchdog_mode(value: Any) -> None: + """Validate ``watchdog.mode`` configuration option. + + :param value: value of ``watchdog.mode`` to be validated. + """ assert_(isinstance(value, (str, bool)), "expected type is not a string") assert_(value in (False, "off", "automatic", "required")) @@ -748,6 +820,7 @@ setattr(validate_connect_address, 'expected_type', str) setattr(validate_host_port_listen, 'expected_type', str) setattr(validate_host_port_listen_multiple_hosts, 'expected_type', str) setattr(validate_data_dir, 'expected_type', str) +setattr(validate_binary_name, 'expected_type', str) validate_etcd = { Or("host", "hosts", "srv", "srv_suffix", "url", "proxy"): Case({ "host": validate_host_port, @@ -823,8 +896,16 @@ schema = Schema({ Optional("rewind"): userattributes }, "data_dir": validate_data_dir, - Optional("bin_dir", ""): Directory(contains_executable=["pg_ctl", "initdb", "pg_controldata", "pg_basebackup", - "postgres", "pg_isready"]), + Optional("bin_name"): { + Optional("pg_ctl"): validate_binary_name, + Optional("initdb"): validate_binary_name, + Optional("pg_controldata"): validate_binary_name, + Optional("pg_basebackup"): validate_binary_name, + Optional("postgres"): validate_binary_name, + Optional("pg_isready"): validate_binary_name, + Optional("pg_rewind"): validate_binary_name, + }, + Optional("bin_dir", ""): BinDirectory(), Optional("parameters"): { Optional("unix_socket_directories"): str }, diff --git a/tests/test_validator.py b/tests/test_validator.py index cec1f140..4c195a0e 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -271,3 +271,38 @@ class TestValidator(unittest.TestCase): errors = schema2(config_2) output = "\n".join(errors) self.assertEqual(['some_dir'], parse_output(output)) + + def test_validate_binary_name(self, mock_out, mock_err): + r = copy.copy(required_binaries) + r.remove('postgres') + r.append('fake-postgres') + binaries.extend(r) + c = copy.deepcopy(config) + c["postgresql"]["bin_name"] = {"postgres": "fake-postgres"} + del c["postgresql"]["bin_dir"] + errors = schema(c) + output = "\n".join(errors) + self.assertEqual(['raft.bind_addr', 'raft.self_addr'], parse_output(output)) + + def test_validate_binary_name_missing(self, mock_out, mock_err): + r = copy.copy(required_binaries) + r.remove('postgres') + binaries.extend(r) + c = copy.deepcopy(config) + c["postgresql"]["bin_name"] = {"postgres": "fake-postgres"} + del c["postgresql"]["bin_dir"] + errors = schema(c) + output = "\n".join(errors) + self.assertEqual(['postgresql.bin_dir', 'postgresql.bin_name.postgres', 'raft.bind_addr', 'raft.self_addr'], + parse_output(output)) + + def test_validate_binary_name_empty_string(self, mock_out, mock_err): + r = copy.copy(required_binaries) + binaries.extend(r) + c = copy.deepcopy(config) + c["postgresql"]["bin_name"] = {"postgres": ""} + del c["postgresql"]["bin_dir"] + errors = schema(c) + output = "\n".join(errors) + self.assertEqual(['postgresql.bin_dir', 'postgresql.bin_name.postgres', 'raft.bind_addr', 'raft.self_addr'], + parse_output(output))