diff --git a/docs/explanation/juju.md b/docs/explanation/juju.md index 91d33bb3cd6..8504104fc42 100644 --- a/docs/explanation/juju.md +++ b/docs/explanation/juju.md @@ -26,7 +26,7 @@ In the context of this documentation, the pertinent changes are as follows: |`run`|`exec`| |`run-action --wait`|`run`| -See the [Juju 3.0 release notes](https://documentation.ubuntu.com/juju/3.6/reference/juju/juju-roadmap-and-releases/#juju-3-0-0-22-oct-2022) for the comprehensive list of changes. +See the [Juju 3.0 release notes](https://documentation.ubuntu.com/juju/3.6/releasenotes/unsupported/juju_3.x.x/#juju-3-0) for the comprehensive list of changes. Example substitutions: diff --git a/docs/explanation/logs.md b/docs/explanation/logs.md index 4e430381b39..378d85315d5 100644 --- a/docs/explanation/logs.md +++ b/docs/explanation/logs.md @@ -78,7 +78,7 @@ The naming convention of the Pgbackrest logs is `.patroni- --repo1-s3-key-secret= --repo1-s3-region=eu-central-1 --repo1-s3-uri-style=host --repo1-type=s3 --stanza=discourse.patroni-pg --start-fast --type=full +2023-10-11 13:06:29.857 P00 INFO: backup command begin 2.47: --no-backup-standby --exec-id=843-b0d896e1 --log-level-console=debug --pg1-path=/var/lib/pg/data/16/main --pg1-user=backup --repo1-path=/postgresql-test --repo1-retention-full=9999999 --repo1-s3-bucket=dragop-test-bucket --repo1-s3-endpoint=https://s3.eu-central-1.amazonaws.com --repo1-s3-key= --repo1-s3-key-secret= --repo1-s3-region=eu-central-1 --repo1-s3-uri-style=host --repo1-type=s3 --stanza=discourse.patroni-pg --start-fast --type=full 2023-10-11 13:06:30.869 P00 INFO: execute non-exclusive backup start: backup begins after the requested immediate checkpoint completes 2023-10-11 13:06:31.671 P00 INFO: backup start archive = 000000010000000000000004, lsn = 0/4000060 2023-10-11 13:06:31.671 P00 INFO: check archive for prior segment 000000010000000000000003 diff --git a/docs/how-to/data-migration/migrate-data-via-pg-dump.md b/docs/how-to/data-migration/migrate-data-via-pg-dump.md index 85aaaed97be..6a1287ff06b 100644 --- a/docs/how-to/data-migration/migrate-data-via-pg-dump.md +++ b/docs/how-to/data-migration/migrate-data-via-pg-dump.md @@ -21,7 +21,7 @@ Always test migration in a safe environment before performing it in production! - **[Your application is compatible](/explanation/legacy-charm) with Charmed PostgreSQL K8s** - A client machine with access to the deployed legacy charm -- `juju v.2.9` or later (check [Juju 3.0 Release Notes](https://documentation.ubuntu.com/juju/3.6/reference/juju/juju-roadmap-and-releases/#juju-3-0-0-22-oct-2022) for more information about key differences) +- `juju v.2.9` or later (check [Juju 3.0 Release Notes](https://documentation.ubuntu.com/juju/3.6/releasenotes/unsupported/juju_3.x.x/#juju-3-0) for more information about key differences) - Enough storage in the cluster to support backup/restore of the databases. ## Obtain existing database credentials diff --git a/docs/reference/troubleshooting.md b/docs/reference/troubleshooting.md index 4a4f2b99581..345c18547fe 100644 --- a/docs/reference/troubleshooting.md +++ b/docs/reference/troubleshooting.md @@ -119,9 +119,9 @@ postgresql enabled active today at 12:29 UTC root@postgresql-k8s-0:/# ps auxww USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND root 1 0.0 0.0 718264 10916 ? Ssl 12:29 0:00 /charm/bin/pebble run --create-dirs --hold --http :38813 --verbose -postgres 14 0.1 0.1 565020 39412 ? Sl 12:29 0:01 python3 /usr/bin/patroni /var/lib/postgresql/data/patroni.yml +postgres 14 0.1 0.1 565020 39412 ? Sl 12:29 0:01 python3 /usr/bin/patroni /var/lib/pg/data/patroni.yml postgres 30 0.0 0.0 1082704 9076 ? Sl 12:30 0:00 /usr/bin/prometheus-postgres-exporter -postgres 48 0.0 0.0 215488 28912 ? S 12:30 0:00 /usr/lib/postgresql/14/bin/postgres -D /var/lib/postgresql/data/pgdata --config-file=/var/lib/postgresql/data/pgdata/postgresql.conf --listen_addresses=0.0.0.0 --port=5432 --cluster_name=patroni-postgresql-k8s --wal_level=logical --hot_standby=on --max_connections=100 --max_wal_senders=10 --max_prepared_transactions=0 --max_locks_per_transaction=64 --track_commit_timestamp=off --max_replication_slots=10 --max_worker_processes=8 --wal_log_hints=on +postgres 48 0.0 0.0 215488 28912 ? S 12:30 0:00 /usr/lib/postgresql/16/bin/postgres -D /var/lib/pg/data/16/main --config-file=/var/lib/pg/data/16/main/postgresql.conf --listen_addresses=0.0.0.0 --port=5432 --cluster_name=patroni-postgresql-k8s --wal_level=logical --hot_standby=on --max_connections=100 --max_wal_senders=10 --max_prepared_transactions=0 --max_locks_per_transaction=64 --track_commit_timestamp=off --max_replication_slots=10 --max_worker_processes=8 --wal_log_hints=on postgres 50 0.0 0.0 70080 7488 ? Ss 12:30 0:00 postgres: patroni-postgresql-k8s: logger postgres 52 0.0 0.0 215592 9136 ? Ss 12:30 0:00 postgres: patroni-postgresql-k8s: checkpointer postgres 53 0.0 0.0 215604 9632 ? Ss 12:30 0:00 postgres: patroni-postgresql-k8s: background writer diff --git a/metadata.yaml b/metadata.yaml index 6c68f7f8e69..05813459741 100644 --- a/metadata.yaml +++ b/metadata.yaml @@ -29,19 +29,19 @@ containers: resource: postgresql-image mounts: - storage: archive - location: /var/lib/postgresql/archive + location: /var/lib/pg/archive - storage: data - location: /var/lib/postgresql/data + location: /var/lib/pg/data - storage: logs - location: /var/lib/postgresql/logs + location: /var/lib/pg/logs - storage: temp - location: /var/lib/postgresql/temp + location: /var/lib/pg/temp resources: postgresql-image: type: oci-image description: OCI image for PostgreSQL - upstream-source: ghcr.io/canonical/charmed-postgresql@sha256:fcb648db2c418403fbf5f58f04892c4677f113526097ca7314cbd9f527bb1baf # renovate: oci-image tag: 16.11-24.04_edge + upstream-source: ghcr.io/canonical/charmed-postgresql@sha256:7ee3f31ce88a48a94634d852a50e6cf3c50c6773eb885e4b896d3039150aa633 # renovate: oci-image tag: 16.11-24.04_edge peers: database-peers: @@ -109,19 +109,19 @@ storage: archive: type: filesystem description: Storage mount used for holding local backups (before typically sending them to remote object storage) when relevant/needed. - location: /var/lib/postgresql/archive + location: /var/lib/pg/archive data: type: filesystem description: Storage mount used for storing all tables, indexes, and so on (except those from temporary tablespaces). - location: /var/lib/postgresql/data + location: /var/lib/pg/data logs: type: filesystem description: Storage mount used for storing all the logs that are part of the transactional commit path (WAL files). - location: /var/lib/postgresql/logs + location: /var/lib/pg/logs temp: type: filesystem description: Storage mount used for storing temporary tablespaces (where typically sort operations happen). - location: /var/lib/postgresql/temp + location: /var/lib/pg/temp assumes: - k8s-api diff --git a/scripts/authorisation_rules_observer.py b/scripts/authorisation_rules_observer.py index 54ea62e52e7..205bb324971 100644 --- a/scripts/authorisation_rules_observer.py +++ b/scripts/authorisation_rules_observer.py @@ -19,7 +19,7 @@ PATRONI_CLUSTER_STATUS_ENDPOINT = "cluster" PATRONI_CONFIG_STATUS_ENDPOINT = "config" TLS_CA_BUNDLE_FILE = "peer_ca_bundle.pem" -PATRONI_CONF_FILE_PATH = "/var/lib/postgresql/data/patroni.yml" +PATRONI_CONF_FILE_PATH = "/var/lib/pg/data/patroni.yml" # File path for the spawned cluster topology observer process to write logs. LOG_FILE_PATH = "/var/log/authorisation_rules_observer.log" diff --git a/src/backups.py b/src/backups.py index bc29f64999b..cbbab09d24a 100644 --- a/src/backups.py +++ b/src/backups.py @@ -31,9 +31,13 @@ from tenacity import RetryError, Retrying, stop_after_attempt, wait_fixed from constants import ( + ARCHIVE_PATH, BACKUP_TYPE_OVERRIDES, BACKUP_USER, + LOGS_STORAGE_PATH, PGBACKREST_LOGROTATE_FILE, + POSTGRESQL_DATA_PATH, + TEMP_STORAGE_PATH, WORKLOAD_OS_GROUP, WORKLOAD_OS_USER, ) @@ -195,7 +199,7 @@ def can_use_s3_repository(self) -> tuple[bool, str]: system_identifier_from_instance, error = self._execute_command([ f"/usr/lib/postgresql/{self.charm._patroni.rock_postgresql_version.split('.')[0]}/bin/pg_controldata", - "/var/lib/postgresql/data/pgdata", + POSTGRESQL_DATA_PATH, ]) if error != "": raise Exception(error) @@ -272,15 +276,24 @@ def _create_bucket_if_not_exists(self) -> None: def _empty_data_files(self) -> None: """Empty the PostgreSQL data directory in preparation of backup restore.""" - try: - self.container.exec(["rm", "-r", "/var/lib/postgresql/data/pgdata"]).wait_output() - except ExecError as e: - # If previous PITR restore was unsuccessful, there is no such directory. - if "No such file or directory" not in str(e.stderr): - logger.exception( - "Failed to empty data directory in prep for backup restore", exc_info=e - ) - raise + # Clear all storage directories, not just data. The logs directory must be cleared + # so that when new replicas join after restore, pg_basebackup can use the --waldir + # option (which requires an empty directory). + for path in [ + ARCHIVE_PATH, + self.charm._actual_pgdata_path, + LOGS_STORAGE_PATH, + TEMP_STORAGE_PATH, + ]: + try: + self.container.exec(["find", path, "-mindepth", "1", "-delete"]).wait_output() + except ExecError as e: + # If previous PITR restore was unsuccessful, there may be no such directory. + if "No such file or directory" not in str(e.stderr): + logger.exception( + f"Failed to empty {path} in prep for backup restore", exc_info=e + ) + raise def _change_connectivity_to_database(self, connectivity: bool) -> None: """Enable or disable the connectivity to the database.""" @@ -1212,6 +1225,7 @@ def _render_pgbackrest_conf_file(self) -> bool: secret_key=s3_parameters["secret-key"], stanza=self.stanza_name, storage_path=self.charm._storage_path, + pgdata_path=POSTGRESQL_DATA_PATH, user=BACKUP_USER, retention_full=s3_parameters["delete-older-than-days"], process_max=max(cpu_count - 2, 1), @@ -1234,7 +1248,9 @@ def _render_pgbackrest_conf_file(self) -> bool: "/home/postgres/rotate_logs.py", f.read(), ) - self.container.start(self.charm.rotate_logs_service) + services = self.container.pebble.get_services(names=[self.charm.rotate_logs_service]) + if services: + self.container.start(self.charm.rotate_logs_service) return True diff --git a/src/charm.py b/src/charm.py index c77c638fe7a..c2b59f4f7ca 100755 --- a/src/charm.py +++ b/src/charm.py @@ -135,6 +135,7 @@ SECRET_KEY_OVERRIDES, SPI_MODULE, SYSTEM_USERS, + TEMP_STORAGE_PATH, TLS_CA_BUNDLE_FILE, TLS_CA_FILE, TLS_CERT_FILE, @@ -267,7 +268,8 @@ def __init__(self, *args): self._certs_path = "/usr/local/share/ca-certificates" self._storage_path = str(self.meta.storages["data"].location) - self.pgdata_path = f"{self._storage_path}/pgdata" + self._actual_pgdata_path = f"{self._storage_path}/16/main" + self.pgdata_path = "/var/lib/postgresql/16/main" self.framework.observe(self.on.upgrade_charm, self._on_upgrade_charm) self.postgresql_client_relation = PostgreSQLProvider(self) @@ -1135,17 +1137,108 @@ def fix_leader_annotation(self) -> bool: raise e return True + def _replica_can_start(self) -> bool: + """Check whether this replica is ready to start Patroni. + + Returns False if the cluster hasn't been bootstrapped yet, or if the + leader hasn't added this unit's endpoint to the members list (which + controls the pg_hba replication entries on the primary). + """ + if not self.is_cluster_initialised: + logger.debug("Replica not ready: cluster not initialized") + return False + if self._endpoint not in self._endpoints: + logger.debug("Replica not ready: endpoint not yet in members list") + return False + return True + def _create_pgdata(self, container: Container): - """Create the PostgreSQL data directory.""" - if not container.exists(self.pgdata_path): + """Create the PostgreSQL data directories.""" + logs_path = str(self.meta.storages["logs"].location) + waldir_path = f"{logs_path}/16/main/pg_wal" + temp_path = str(self.meta.storages["temp"].location) + temp_tablespace_path = f"{temp_path}/16/main/pgsql_tmp" + + # Clear stale storage directories when a replica joins an initialized cluster. + # This is needed because PersistentVolumes may retain data from previous pods, + # and pg_basebackup requires empty --waldir and --tablespace directories. + # Clear on every call until pgdata is populated (PG_VERSION exists), since + # pg_basebackup can fail and leave stale files that prevent retries. + pgdata_populated = container.exists(f"{self._actual_pgdata_path}/PG_VERSION") + if not self.unit.is_leader() and self.is_cluster_initialised and not pgdata_populated: + for path in [waldir_path, temp_tablespace_path]: + if container.exists(path): + try: + container.exec(["find", path, "-mindepth", "1", "-delete"]).wait_output() + logger.info( + f"Cleared stale content from {path} for replica initialization" + ) + except ExecError as e: + if "No such file or directory" not in str(e.stderr): + logger.warning(f"Failed to clear {path}: {e}") + + # Create the pgdata directory on the storage mount (e.g., /var/lib/pg/data/16/main) + if not container.exists(self._actual_pgdata_path): + container.make_dir( + self._actual_pgdata_path, + permissions=0o700, + user=WORKLOAD_OS_USER, + group=WORKLOAD_OS_GROUP, + make_parents=True, + ) + # Create the WAL directory (e.g., /var/lib/pg/logs/16/main/pg_wal) + if not container.exists(waldir_path): container.make_dir( - self.pgdata_path, permissions=0o700, user=WORKLOAD_OS_USER, group=WORKLOAD_OS_GROUP + waldir_path, + permissions=0o700, + user=WORKLOAD_OS_USER, + group=WORKLOAD_OS_GROUP, + make_parents=True, ) + # Create the temp tablespace directory (e.g., /var/lib/pg/temp/16/main/pgsql_tmp) + if not container.exists(temp_tablespace_path): + container.make_dir( + temp_tablespace_path, + permissions=0o700, + user=WORKLOAD_OS_USER, + group=WORKLOAD_OS_GROUP, + make_parents=True, + ) + # Create a symlink from the default PostgreSQL data directory to our data directory + # (e.g., /var/lib/postgresql/16/main -> /var/lib/pg/data/16/main) + # Patroni and other tools will use the symlink path (self.pgdata_path) + # Note: This symlink is on ephemeral storage and may not persist across container restarts. + # It gets recreated on each pebble-ready event. + # The OCI image ships /var/lib/postgresql/16/main as a real directory, so we must + # remove it first if it exists as a non-symlink (e.g., on replicas). + container.make_dir( + "/var/lib/postgresql/16", + user=WORKLOAD_OS_USER, + group=WORKLOAD_OS_GROUP, + make_parents=True, + ) + container.exec([ + "bash", + "-c", + f"[ -L {self.pgdata_path} ] || rm -rf {self.pgdata_path}", + ]).wait() + container.exec([ + "ln", + "-sfn", + self._actual_pgdata_path, + self.pgdata_path, + ]).wait() + container.exec([ + "chown", + "-h", + f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}", + self.pgdata_path, + ]).wait() # Also, fix the permissions from the parent directory. container.exec([ "chown", f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}", - "/var/lib/postgresql/archive", + str(self.meta.storages["archive"].location), ]).wait() container.exec([ "chown", @@ -1155,12 +1248,12 @@ def _create_pgdata(self, container: Container): container.exec([ "chown", f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}", - "/var/lib/postgresql/logs", + logs_path, ]).wait() container.exec([ "chown", f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}", - "/var/lib/postgresql/temp", + temp_path, ]).wait() def _on_start(self, _) -> None: @@ -1196,15 +1289,14 @@ def _on_postgresql_pebble_ready(self, event: WorkloadEvent) -> None: # where the volume is mounted with more restrictive permissions. self._create_pgdata(container) - # Defer the initialization of the workload in the replicas - # if the cluster hasn't been bootstrap on the primary yet. - # Otherwise, each unit will create a different cluster and - # any update in the members list on the units won't have effect - # on fixing that. - if not self.unit.is_leader() and not self.is_cluster_initialised: - logger.debug( - "Deferring on_postgresql_pebble_ready: Not leader and cluster not initialized" - ) + # Defer the initialization of the workload in the replicas if the cluster + # hasn't been bootstrapped on the primary yet, or the leader hasn't added + # this unit's endpoint to the cluster members list yet. The endpoint + # controls pg_hba replication entries on the primary — without it, + # pg_basebackup is rejected, triggering retries and remove_data_directory() + # calls that can race with _create_pgdata() and break the pg_wal symlink + # created by --waldir. + if not self.unit.is_leader() and not self._replica_can_start(): event.defer() return @@ -1363,7 +1455,7 @@ def _setup_users(self) -> None: extra_user_roles=["pg_monitor"], ) - self.postgresql.set_up_database(temp_location="/var/lib/postgresql/temp") + self.postgresql.set_up_database(temp_location=f"{TEMP_STORAGE_PATH}/16/main/pgsql_tmp") access_groups = self.postgresql.list_access_groups() if access_groups != set(ACCESS_GROUPS): @@ -1613,6 +1705,7 @@ def _fix_pod(self) -> None: # Recreate k8s resources and add labels required for replication # when the pod loses them (like when it's deleted). self.push_tls_files_to_workload() + if self.refresh is not None and not self.refresh.in_progress: try: self._create_services() @@ -1759,9 +1852,9 @@ def _on_update_status_early_exit_checks(self, container) -> bool: def _check_pgdata_storage_size(self) -> None: """Asserts that pgdata volume has at least 10% free space and blocks charm if not.""" try: - total_size, _, free_size = shutil.disk_usage(self.pgdata_path) + total_size, _, free_size = shutil.disk_usage(self._actual_pgdata_path) except FileNotFoundError: - logger.error("pgdata folder not found in %s", self.pgdata_path) + logger.error("pgdata folder not found in %s", self._actual_pgdata_path) return logger.debug( @@ -1896,6 +1989,7 @@ def _patroni(self): self.primary_endpoint, self._namespace, self._storage_path, + self.pgdata_path, self.get_secret(APP_SCOPE, USER_PASSWORD_KEY), self.get_secret(APP_SCOPE, REPLICATION_PASSWORD_KEY), self.get_secret(APP_SCOPE, REWIND_PASSWORD_KEY), @@ -2608,7 +2702,10 @@ def update_config(self, is_creating_backup: bool = False) -> bool: logger.warning("Early exit update_config: Unable to patch Patroni API") return False - self._patroni.ensure_slots_controller_by_patroni(replication_slots) + if not self._patroni.ensure_slots_controller_by_patroni(replication_slots): + logger.warning( + "Failed to sync replication slots with Patroni — will retry on next config update" + ) self._handle_postgresql_restart_need( self.unit_peer_data.get("config_hash") != self.generate_config_hash diff --git a/src/constants.py b/src/constants.py index 8bc47801339..a5334c505f6 100644 --- a/src/constants.py +++ b/src/constants.py @@ -19,8 +19,15 @@ WORKLOAD_OS_USER = "postgres" METRICS_PORT = "9187" PGBACKREST_METRICS_PORT = "9854" -POSTGRESQL_DATA_PATH = "/var/lib/postgresql/data/pgdata" +POSTGRESQL_DATA_PATH = "/var/lib/postgresql/16/main" POSTGRESQL_LOGS_PATH = "/var/log/postgresql" + +# Storage mount paths (must match metadata.yaml storage locations). +STORAGE_PATH = "/var/lib/pg" +ARCHIVE_PATH = f"{STORAGE_PATH}/archive" +DATA_STORAGE_PATH = f"{STORAGE_PATH}/data" +LOGS_STORAGE_PATH = f"{STORAGE_PATH}/logs" +TEMP_STORAGE_PATH = f"{STORAGE_PATH}/temp" POSTGRESQL_LOGS_PATTERN = "postgresql*.log" POSTGRES_LOG_FILES = [ "/var/log/pgbackrest/*", diff --git a/src/patroni.py b/src/patroni.py index 2a636141596..c9061cc5af0 100644 --- a/src/patroni.py +++ b/src/patroni.py @@ -33,6 +33,7 @@ from constants import ( API_REQUEST_TIMEOUT, + LOGS_STORAGE_PATH, PATRONI_CLUSTER_STATUS_ENDPOINT, POSTGRESQL_LOGS_PATH, POSTGRESQL_LOGS_PATTERN, @@ -99,6 +100,7 @@ def __init__( primary_endpoint: str, namespace: str, storage_path: str, + pgdata_path: str, superuser_password: str | None, replication_password: str | None, rewind_password: str | None, @@ -110,6 +112,7 @@ def __init__( self._primary_endpoint = primary_endpoint self._namespace = namespace self._storage_path = storage_path + self._pgdata_path = pgdata_path self._members_count = len(self._endpoints) self._superuser_password = superuser_password self._replication_password = replication_password @@ -540,29 +543,38 @@ def bulk_update_parameters_controller_by_patroni( ) r.raise_for_status() - def ensure_slots_controller_by_patroni(self, slots: dict[str, str]) -> None: + def ensure_slots_controller_by_patroni(self, slots: dict[str, str]) -> bool: """Synchronises slots controlled by Patroni with the provided state by removing unneeded slots and creating new ones. Args: slots: dictionary of slots in the {slot: database} format. + + Returns: + True if successful, False if Patroni API is not ready. """ - for attempt in Retrying(stop=stop_after_delay(60), wait=wait_fixed(3), reraise=True): - with attempt: - current_config = requests.get( - f"{self._patroni_url}/config", - verify=self._verify, - timeout=API_REQUEST_TIMEOUT, - auth=self._patroni_auth, - ) - logger.debug( - "API ensure_slots_controller_by_patroni: %s (%s)", - current_config, - current_config.elapsed.total_seconds(), - ) - if current_config.status_code != 200: - raise Exception( - f"Failed to get current Patroni config: {current_config.status_code} {current_config.text}" + try: + for attempt in Retrying(stop=stop_after_delay(60), wait=wait_fixed(3)): + with attempt: + current_config = requests.get( + f"{self._patroni_url}/config", + verify=self._verify, + timeout=API_REQUEST_TIMEOUT, + auth=self._patroni_auth, ) + logger.debug( + "API ensure_slots_controller_by_patroni: %s (%s)", + current_config, + current_config.elapsed.total_seconds(), + ) + if current_config.status_code in (502, 503): + raise Exception(f"Patroni API not ready: {current_config.status_code}") + if current_config.status_code != 200: + raise Exception( + f"Failed to get current Patroni config: {current_config.status_code} {current_config.text}" + ) + except RetryError: + logger.warning("Patroni config API not ready after retries, will retry later") + return False slots_patch: dict[str, dict[str, str] | None] = dict.fromkeys( current_config.json().get("slots", ()) or {} @@ -580,6 +592,7 @@ def ensure_slots_controller_by_patroni(self, slots: dict[str, str]) -> None: auth=self._patroni_auth, timeout=API_REQUEST_TIMEOUT, ) + return True def promote_standby_cluster(self) -> None: """Promote a standby cluster to be a regular cluster.""" @@ -690,6 +703,8 @@ def render_patroni_yml_file( is_no_sync_member=is_no_sync_member, namespace=self._namespace, storage_path=self._storage_path, + logs_storage_path=LOGS_STORAGE_PATH, + pgdata_path=self._pgdata_path, superuser_password=self._superuser_password, replication_password=self._replication_password, rewind_user=REWIND_USER, diff --git a/src/relations/async_replication.py b/src/relations/async_replication.py index 41155f37eed..a942c0ad9ae 100644 --- a/src/relations/async_replication.py +++ b/src/relations/async_replication.py @@ -41,8 +41,11 @@ from constants import ( APP_SCOPE, + ARCHIVE_PATH, + LOGS_STORAGE_PATH, PEER, POSTGRESQL_DATA_PATH, + TEMP_STORAGE_PATH, WORKLOAD_OS_GROUP, WORKLOAD_OS_USER, ) @@ -353,7 +356,10 @@ def _handle_database_start(self, event: RelationChangedEvent) -> None: if self.charm._patroni.member_started: # If the database is started, update the databag in a way the unit is marked as configured # for async replication. - self.charm._peers.data[self.charm.unit].update({"stopped": ""}) + self.charm._peers.data[self.charm.unit].update({ + "stopped": "", + "standby-pgdata-cleared": "", + }) self.charm._peers.data[self.charm.unit].update({ "unit-promoted-cluster-counter": self._get_highest_promoted_cluster_counter_value() }) @@ -480,6 +486,7 @@ def _on_async_relation_broken(self, _) -> None: self.charm._peers.data[self.charm.unit].update({ "stopped": "", + "standby-pgdata-cleared": "", "unit-promoted-cluster-counter": "", }) @@ -508,10 +515,18 @@ def _on_async_relation_changed(self, event: RelationChangedEvent) -> None: if self._configure_primary_cluster(primary_cluster, event): return - # Return if this is a new unit. + # Return if this is a new unit joining an existing standby cluster. + # If the database is already running (i.e., we're a late joiner that completed setup), + # just return early - the unit is already part of the standby cluster. if not self.charm.unit.is_leader() and self._is_following_promoted_cluster(): - logger.debug("Early exit on_async_relation_changed: following promoted cluster.") - return + if self.charm._patroni.member_started: + logger.debug("Early exit on_async_relation_changed: following promoted cluster.") + return + # Database not running - clear pgdata if needed so Patroni can run pg_basebackup. + # Only clear once, tracked by standby-pgdata-cleared flag. + if self.charm._peers.data[self.charm.unit].get("standby-pgdata-cleared") != "True": + self._clear_pgdata() + self.charm._peers.data[self.charm.unit].update({"standby-pgdata-cleared": "True"}) if not self._stop_database(event): return @@ -718,22 +733,29 @@ def _stop_database(self, event: RelationChangedEvent) -> bool: if not self._configure_standby_cluster(event): return False - # Remove and recreate the pgdata folder to enable replication of the data from the - # primary cluster. - for path in [ - "/var/lib/postgresql/archive", - POSTGRESQL_DATA_PATH, - "/var/lib/postgresql/logs", - "/var/lib/postgresql/temp", - ]: - logger.info(f"Removing contents from {path}") - self.container.exec(f"find {path} -mindepth 1 -delete".split()).wait_output() - self.charm._create_pgdata(self.container) + # Only the leader clears pgdata here. Non-leaders will clear pgdata + # after the standby leader has started (in _wait_for_standby_leader) + # to avoid system ID mismatch issues. + self._clear_pgdata() self.charm._peers.data[self.charm.unit].update({"stopped": "True"}) return True + def _clear_pgdata(self) -> None: + """Remove and recreate the pgdata folder to enable replication.""" + # Note: Use _actual_pgdata_path instead of POSTGRESQL_DATA_PATH because + # POSTGRESQL_DATA_PATH is a symlink, and find doesn't follow symlinks by default. + for path in [ + ARCHIVE_PATH, + self.charm._actual_pgdata_path, + LOGS_STORAGE_PATH, + TEMP_STORAGE_PATH, + ]: + logger.info(f"Removing contents from {path}") + self.container.exec(["find", path, "-mindepth", "1", "-delete"]).wait_output() + self.charm._create_pgdata(self.container) + def update_async_replication_data(self) -> None: """Updates the async-replication data, if the unit is the leader. @@ -796,7 +818,11 @@ def _update_primary_cluster_data( ) def _wait_for_standby_leader(self, event: RelationChangedEvent) -> bool: - """Wait for the standby leader to be up and running.""" + """Wait for the standby leader to be up and running. + + For non-leader units, this also clears pgdata once the standby leader is confirmed + running to avoid system ID mismatch issues. + """ try: standby_leader = self.charm._patroni.get_standby_leader(check_whether_is_running=True) except RetryError: @@ -808,4 +834,15 @@ def _wait_for_standby_leader(self, event: RelationChangedEvent) -> bool: logger.debug("Deferring on_async_relation_changed: standby leader hasn't started yet.") event.defer() return True + + # For non-leader units, clear pgdata now that the standby leader is running. + # This ensures replicas get the correct system ID from the standby leader. + # Only clear pgdata once - use a flag to track if we've already done it. + if ( + not self.charm.unit.is_leader() + and self.charm._peers.data[self.charm.unit].get("standby-pgdata-cleared") != "True" + ): + self._clear_pgdata() + self.charm._peers.data[self.charm.unit].update({"standby-pgdata-cleared": "True"}) + return False diff --git a/templates/patroni.yml.j2 b/templates/patroni.yml.j2 index 954ddfa1329..d2eb01aef3c 100644 --- a/templates/patroni.yml.j2 +++ b/templates/patroni.yml.j2 @@ -67,7 +67,7 @@ bootstrap: method: pgbackrest pgbackrest: command: > - pgbackrest --stanza={{ restore_stanza }} --pg1-path={{ storage_path }}/pgdata + pgbackrest --stanza={{ restore_stanza }} --pg1-path={{ pgdata_path }} {%- if backup_id %} --set={{ backup_id }} {%- endif %} {%- if restore_timeline %} --target-timeline="0x{{ restore_timeline }}" {% endif %} {%- if restore_to_latest %} --type=default {%- else %} @@ -87,7 +87,7 @@ bootstrap: - auth-local: trust - encoding: UTF8 - data-checksums - - waldir: /var/lib/postgresql/logs + - waldir: {{ logs_storage_path }}/16/main/pg_wal {%- endif %} pg_hba: - {{ 'hostssl' if enable_tls else 'host' }} all all 0.0.0.0/0 {{ instance_password_encryption }} @@ -118,9 +118,9 @@ ctl: pod_ip: '{{ endpoint }}' postgresql: basebackup: - - waldir: /var/lib/postgresql/logs + - waldir: {{ logs_storage_path }}/16/main/pg_wal connect_address: '{{ endpoint }}:5432' - data_dir: {{ storage_path }}/pgdata + data_dir: {{ pgdata_path }} bin_dir: /usr/lib/postgresql/{{ version }}/bin listen: 0.0.0.0:5432 parameters: diff --git a/templates/pgbackrest.conf.j2 b/templates/pgbackrest.conf.j2 index 12bd7374d63..2def70f137f 100644 --- a/templates/pgbackrest.conf.j2 +++ b/templates/pgbackrest.conf.j2 @@ -33,7 +33,7 @@ tls-server-key-file={{ storage_path }}/peer_key.pem {%- endif %} [{{ stanza }}] -pg1-path={{ storage_path }}/pgdata +pg1-path={{ pgdata_path }} pg1-user={{ user }} {%- if enable_tls %} {% set ns = namespace(count=2) %} @@ -43,7 +43,7 @@ pg{{ ns.count }}-host-ca-file={{ storage_path }}/peer_ca.pem pg{{ ns.count }}-host-cert-file={{ storage_path }}/peer_cert.pem pg{{ ns.count }}-host-key-file={{ storage_path }}/peer_key.pem pg{{ ns.count }}-host={{ peer_endpoint }} -pg{{ ns.count }}-path={{ storage_path }}/pgdata +pg{{ ns.count }}-path={{ pgdata_path }} pg{{ ns.count }}-user={{ user }} {% set ns.count = ns.count + 1 %} {%- endfor %} diff --git a/tests/helpers.py b/tests/helpers.py index a4adedaf15e..002f9648cd9 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -8,3 +8,9 @@ METADATA = yaml.safe_load(Path("./metadata.yaml").read_text()) STORAGE_PATH = METADATA["storage"]["data"]["location"] + +# PGDATA_PATH points to the symlink path that Patroni and PostgreSQL use. +PGDATA_PATH = "/var/lib/postgresql/16/main" + +# ACTUAL_PGDATA_PATH points to the actual storage location (versioned path under the storage mount). +ACTUAL_PGDATA_PATH = f"{STORAGE_PATH}/16/main" diff --git a/tests/integration/ha_tests/clean-data-dir.sh b/tests/integration/ha_tests/clean-data-dir.sh index 96a0a6ad558..b3c64c72e17 100755 --- a/tests/integration/ha_tests/clean-data-dir.sh +++ b/tests/integration/ha_tests/clean-data-dir.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash set -Eeuo pipefail -rm -rf /var/lib/postgresql/archive/* -rm -rf /var/lib/postgresql/data/pgdata/* -rm -rf /var/lib/postgresql/logs/* -rm -rf /var/lib/postgresql/temp/* +find /var/lib/pg/archive -mindepth 1 -delete +find /var/lib/pg/data/16/main -mindepth 1 -delete +find /var/lib/pg/logs -mindepth 1 -delete +find /var/lib/pg/temp -mindepth 1 -delete diff --git a/tests/integration/ha_tests/helpers.py b/tests/integration/ha_tests/helpers.py index 3e7b68541bf..4f7e18d5969 100644 --- a/tests/integration/ha_tests/helpers.py +++ b/tests/integration/ha_tests/helpers.py @@ -38,6 +38,7 @@ from ..helpers import ( APPLICATION_NAME, KUBECTL, + PGDATA_PATH, app_name, db_connect, execute_query_on_unit, @@ -621,7 +622,7 @@ async def is_replica(ops_test: OpsTest, unit_name: str) -> bool: async def list_wal_files(ops_test: OpsTest, app: str) -> set: """Returns the list of WAL segment files in each unit.""" units = [unit.name for unit in ops_test.model.applications[app].units] - command = "ls -1 /var/lib/postgresql/data/pgdata/pg_wal/" + command = f"ls -1 {PGDATA_PATH}/pg_wal/" files = {} for unit in units: complete_command = f"run --unit {unit} -- {command}" diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 7beea9882c1..b7b39938bb6 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -43,6 +43,12 @@ DATA_INTEGRATOR_APP_NAME = "data-integrator" STORAGE_PATH = METADATA["storage"]["data"]["location"] +# PGDATA_PATH points to the symlink path that Patroni and PostgreSQL use. +PGDATA_PATH = "/var/lib/postgresql/16/main" + +# ACTUAL_PGDATA_PATH points to the actual storage location (versioned path under the storage mount). +ACTUAL_PGDATA_PATH = f"{STORAGE_PATH}/16/main" + class SecretNotFoundError(Exception): """Raised when a secret is not found.""" diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 17d9ba0e4d2..6d9ec1a821b 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -20,7 +20,7 @@ from .helpers import ( CHARM_BASE, METADATA, - STORAGE_PATH, + PGDATA_PATH, build_and_deploy, convert_records_to_dict, db_connect, @@ -147,7 +147,7 @@ async def test_settings_are_correct(ops_test: OpsTest, unit_id: int): assert settings["archive_mode"] == "on" assert settings["autovacuum"] == "on" assert settings["cluster_name"] == f"patroni-{APP_NAME}" - assert settings["data_directory"] == f"{STORAGE_PATH}/pgdata" + assert settings["data_directory"] == f"{PGDATA_PATH}" assert settings["data_checksums"] == "on" assert settings["fsync"] == "on" assert settings["full_page_writes"] == "on" @@ -368,7 +368,7 @@ async def test_automatic_failover_after_leader_issue(ops_test: OpsTest) -> None: primary = await get_primary(ops_test) # Crash PostgreSQL by removing the data directory. - await ops_test.model.units.get(primary).run(f"rm -rf {STORAGE_PATH}/pgdata") + await ops_test.model.units.get(primary).run(f"rm -rf {PGDATA_PATH}") # Wait for charm to stabilise await ops_test.model.wait_for_idle( @@ -500,7 +500,7 @@ async def test_storage_with_more_restrictive_permissions(ops_test: OpsTest, char ) # Restrict the permissions of the storage. - command = "chmod 755 /var/lib/postgresql/data" + command = "chmod 755 /var/lib/pg/data" complete_command = f"ssh --container postgresql {app_name}/0 {command}" for attempt in Retrying(stop=stop_after_delay(60), wait=wait_fixed(3), reraise=True): with attempt: diff --git a/tests/integration/test_storage.py b/tests/integration/test_storage.py index 5d7211cc0dd..92d4ab95250 100644 --- a/tests/integration/test_storage.py +++ b/tests/integration/test_storage.py @@ -10,7 +10,7 @@ from . import markers from .helpers import ( DATABASE_APP_NAME, - STORAGE_PATH, + PGDATA_PATH, build_and_deploy, get_primary, run_command_on_unit, @@ -46,7 +46,7 @@ async def test_filling_and_emptying_pgdata_storage(ops_test: OpsTest, charm): await run_command_on_unit( ops_test, primary, - f"FREE_SPACE=$(df --output=avail {STORAGE_PATH}/pgdata | tail -1) && dd if=/dev/urandom of={STORAGE_PATH}/pgdata/tmp bs=1M count=$(( (FREE_SPACE * 91 / 100) / 1024 ))", + f"FREE_SPACE=$(df --output=avail {PGDATA_PATH} | tail -1) && dd if=/dev/urandom of={PGDATA_PATH}/tmp bs=1M count=$(( (FREE_SPACE * 91 / 100) / 1024 ))", ) # wait for charm to get blocked @@ -61,7 +61,7 @@ async def test_filling_and_emptying_pgdata_storage(ops_test: OpsTest, charm): ) # Delete big file to release storage space - await run_command_on_unit(ops_test, primary, f"rm {STORAGE_PATH}/pgdata/tmp") + await run_command_on_unit(ops_test, primary, f"rm {PGDATA_PATH}/tmp") # wait for charm to resolve await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active", timeout=1000) diff --git a/tests/integration/test_symlinks.py b/tests/integration/test_symlinks.py new file mode 100644 index 00000000000..2e4872e25cb --- /dev/null +++ b/tests/integration/test_symlinks.py @@ -0,0 +1,92 @@ +#!/usr/bin/env python3 +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Integration tests for PostgreSQL symlinks (pgdata).""" + +import logging + +import jubilant +import pytest + +from .helpers import ( + ACTUAL_PGDATA_PATH, + DATABASE_APP_NAME, + METADATA, +) + +logger = logging.getLogger(__name__) + +APP_NAME = DATABASE_APP_NAME +UNIT_IDS = [0, 1, 2] +PGDATA_SYMLINK_PATH = "/var/lib/postgresql/16/main" + + +@pytest.mark.abort_on_fail +def test_build_and_deploy(juju: jubilant.Juju, charm): + """Build the charm-under-test and deploy it. + + Assert on the unit status before any relations/configurations take place. + """ + if APP_NAME not in juju.status().apps: + resources = { + "postgresql-image": METADATA["resources"]["postgresql-image"]["upstream-source"] + } + juju.deploy( + charm, + config={"profile": "testing"}, + num_units=len(UNIT_IDS), + resources=resources, + trust=True, + ) + + def _all_active_idle(status): + if not jubilant.all_active(status, APP_NAME): + return False + for uid in UNIT_IDS: + unit = status.apps[APP_NAME].units[f"{APP_NAME}/{uid}"] + if unit.juju_status.current != "idle": + return False + return True + + juju.wait(_all_active_idle, timeout=1500) + status = juju.status() + for unit_id in UNIT_IDS: + unit_name = f"{APP_NAME}/{unit_id}" + assert status.apps[APP_NAME].units[unit_name].is_active + + +@pytest.mark.parametrize("unit_id", UNIT_IDS) +def test_pgdata_symlinks(juju: jubilant.Juju, unit_id: int): + """Test that symlink for pgdata is correctly created.""" + unit_name = f"{APP_NAME}/{unit_id}" + + # Check pgdata symlink exists and points to correct location + pgdata_symlink_check = juju.ssh( + unit_name, "readlink", "-f", PGDATA_SYMLINK_PATH, container="postgresql" + ) + assert pgdata_symlink_check.strip() == ACTUAL_PGDATA_PATH, ( + f"Expected pgdata symlink to point to {ACTUAL_PGDATA_PATH}, got {pgdata_symlink_check.strip()}" + ) + + # Verify symlink is owned by postgres:postgres + pgdata_owner = juju.ssh( + unit_name, "stat", "-c", "%U:%G", PGDATA_SYMLINK_PATH, container="postgresql" + ) + assert pgdata_owner.strip() == "postgres:postgres", ( + f"Expected pgdata symlink to be owned by postgres:postgres, got {pgdata_owner.strip()}" + ) + + +@pytest.mark.parametrize("unit_id", UNIT_IDS) +def test_pg_wal_symlink(juju: jubilant.Juju, unit_id: int): + """Test that pg_wal symlink inside pgdata points to WAL directory on logs storage.""" + unit_name = f"{APP_NAME}/{unit_id}" + + # Check pg_wal symlink exists and points to correct location + pg_wal_symlink_path = f"{PGDATA_SYMLINK_PATH}/pg_wal" + expected_target = "/var/lib/pg/logs/16/main/pg_wal" + result = juju.ssh(unit_name, "readlink", "-f", pg_wal_symlink_path, container="postgresql") + assert result.strip() == expected_target, ( + f"Expected pg_wal symlink to point to {expected_target}, got {result.strip()}" + ) diff --git a/tests/integration/test_tls.py b/tests/integration/test_tls.py index 997ed2cd12a..e3957ac0ba0 100644 --- a/tests/integration/test_tls.py +++ b/tests/integration/test_tls.py @@ -14,6 +14,7 @@ from .helpers import ( CHARM_BASE_NOBLE, DATABASE_APP_NAME, + PGDATA_PATH, build_and_deploy, check_tls, check_tls_patroni_api, @@ -119,7 +120,7 @@ async def test_tls(ops_test: OpsTest) -> None: await run_command_on_unit( ops_test, replica, - 'su postgres -c "/usr/lib/postgresql/16/bin/pg_ctl -D /var/lib/postgresql/data/pgdata promote"', + f'su postgres -c "/usr/lib/postgresql/16/bin/pg_ctl -D {PGDATA_PATH} promote"', ) # Check that the replica was promoted. diff --git a/tests/spread/test_async_replication_upgrade.py/task.yaml b/tests/spread/test_async_replication_upgrade.py/task.yaml index b5b034dcef7..b62aa052bc1 100644 --- a/tests/spread/test_async_replication_upgrade.py/task.yaml +++ b/tests/spread/test_async_replication_upgrade.py/task.yaml @@ -3,5 +3,8 @@ environment: TEST_MODULE: high_availability/test_async_replication_upgrade.py execute: | tox run -e integration -- "tests/integration/$TEST_MODULE" --model testing --alluredir="$SPREAD_TASK/allure-results" +systems: + - -ubuntu-24.04 + - -ubuntu-24.04-arm artifacts: - allure-results diff --git a/tests/spread/test_symlinks.py/task.yaml b/tests/spread/test_symlinks.py/task.yaml new file mode 100644 index 00000000000..b6a7b4e8079 --- /dev/null +++ b/tests/spread/test_symlinks.py/task.yaml @@ -0,0 +1,7 @@ +summary: test_symlinks.py +environment: + TEST_MODULE: test_symlinks.py +execute: | + tox run -e integration -- "tests/integration/$TEST_MODULE" --model testing --alluredir="$SPREAD_TASK/allure-results" +artifacts: + - allure-results diff --git a/tests/spread/test_upgrade.py/task.yaml b/tests/spread/test_upgrade.py/task.yaml index f99ac69384c..1e3c6b58f1e 100644 --- a/tests/spread/test_upgrade.py/task.yaml +++ b/tests/spread/test_upgrade.py/task.yaml @@ -3,5 +3,8 @@ environment: TEST_MODULE: high_availability/test_upgrade.py execute: | tox run -e integration -- "tests/integration/$TEST_MODULE" --model testing --alluredir="$SPREAD_TASK/allure-results" +systems: + - -ubuntu-24.04 + - -ubuntu-24.04-arm artifacts: - allure-results diff --git a/tests/spread/test_upgrade_from_stable.py/task.yaml b/tests/spread/test_upgrade_from_stable.py/task.yaml index ffdb002d254..427af453e0e 100644 --- a/tests/spread/test_upgrade_from_stable.py/task.yaml +++ b/tests/spread/test_upgrade_from_stable.py/task.yaml @@ -3,5 +3,8 @@ environment: TEST_MODULE: high_availability/test_upgrade_from_stable.py execute: | tox run -e integration -- "tests/integration/$TEST_MODULE" --model testing --alluredir="$SPREAD_TASK/allure-results" +systems: + - -ubuntu-24.04 + - -ubuntu-24.04-arm artifacts: - allure-results diff --git a/tests/spread/test_upgrade_skip_pre_upgrade_check.py/task.yaml b/tests/spread/test_upgrade_skip_pre_upgrade_check.py/task.yaml index a57bec914b0..b0c946cdafb 100644 --- a/tests/spread/test_upgrade_skip_pre_upgrade_check.py/task.yaml +++ b/tests/spread/test_upgrade_skip_pre_upgrade_check.py/task.yaml @@ -3,5 +3,8 @@ environment: TEST_MODULE: high_availability/test_upgrade_skip_pre_upgrade_check.py execute: | tox run -e integration -- "tests/integration/$TEST_MODULE" --model testing --alluredir="$SPREAD_TASK/allure-results" +systems: + - -ubuntu-24.04 + - -ubuntu-24.04-arm artifacts: - allure-results diff --git a/tests/unit/test_async_replication.py b/tests/unit/test_async_replication.py index 0ad84ab111f..64377dac43b 100644 --- a/tests/unit/test_async_replication.py +++ b/tests/unit/test_async_replication.py @@ -2,7 +2,7 @@ # See LICENSE file for licensing details. import json -from unittest.mock import PropertyMock, patch +from unittest.mock import MagicMock, PropertyMock, patch import pytest from ops.testing import Harness @@ -386,3 +386,144 @@ def test_on_secret_changed(harness, relation_name): harness.get_relation_data(rel_id, harness.charm.app.name).get("primary-cluster-data") ) assert primary_cluster_data.get("secret-id") != updated_cluster_data.get("secret-id") + + +def test_clear_pgdata(harness): + with ( + patch("ops.model.Container.exec") as _exec, + patch("charm.PostgresqlOperatorCharm._create_pgdata") as _create_pgdata, + ): + mock_process = MagicMock() + mock_process.wait_output.return_value = ("", "") + _exec.return_value = mock_process + + harness.set_can_connect("postgresql", True) + harness.charm.async_replication._clear_pgdata() + + # Should be called 4 times: archive, actual pgdata, logs, temp + assert _exec.call_count == 4 + _exec.assert_any_call(["find", "/var/lib/pg/archive", "-mindepth", "1", "-delete"]) + _exec.assert_any_call(["find", "/var/lib/pg/data/16/main", "-mindepth", "1", "-delete"]) + _exec.assert_any_call(["find", "/var/lib/pg/logs", "-mindepth", "1", "-delete"]) + _exec.assert_any_call(["find", "/var/lib/pg/temp", "-mindepth", "1", "-delete"]) + _create_pgdata.assert_called_once() + + +def test_stop_database_leader_clears_pgdata(harness): + with ( + patch( + "relations.async_replication.PostgreSQLAsyncReplication._get_unit_ip", + return_value="1.1.1.1", + ), + ): + peer_rel_id = harness.add_relation(PEER, harness.charm.app.name) + + with ( + patch("ops.model.Container.stop") as _stop, + patch( + "charm.PostgresqlOperatorCharm.is_unit_stopped", + new_callable=PropertyMock, + return_value=False, + ), + patch( + "relations.async_replication.PostgreSQLAsyncReplication._is_following_promoted_cluster", + return_value=False, + ), + patch("ops.model.Container.exists", return_value=True), + patch( + "relations.async_replication.PostgreSQLAsyncReplication._configure_standby_cluster", + return_value=True, + ), + patch( + "relations.async_replication.PostgreSQLAsyncReplication._clear_pgdata" + ) as _clear_pgdata, + ): + harness.set_can_connect("postgresql", True) + + event = MagicMock() + result = harness.charm.async_replication._stop_database(event) + + assert result is True + _stop.assert_called_once() + _clear_pgdata.assert_called_once() + assert ( + harness.get_relation_data(peer_rel_id, harness.charm.unit.name).get("stopped") + == "True" + ) + + +def test_wait_for_standby_leader_non_leader_clears_pgdata(harness): + with ( + patch( + "relations.async_replication.PostgreSQLAsyncReplication._get_unit_ip", + return_value="1.1.1.1", + ), + ): + peer_rel_id = harness.add_relation(PEER, harness.charm.app.name) + harness.add_relation( + REPLICATION_OFFER_RELATION, + harness.charm.app.name, + unit_data={"unit-address": "10.1.1.10"}, + ) + + with ( + patch( + "charm.Patroni.get_standby_leader", + return_value="postgresql-k8s-0", + ), + patch( + "relations.async_replication.PostgreSQLAsyncReplication._clear_pgdata" + ) as _clear_pgdata, + ): + harness.set_leader(False) + + event = MagicMock() + result = harness.charm.async_replication._wait_for_standby_leader(event) + + assert result is False + _clear_pgdata.assert_called_once() + assert ( + harness.get_relation_data(peer_rel_id, harness.charm.unit.name).get( + "standby-pgdata-cleared" + ) + == "True" + ) + + +def test_wait_for_standby_leader_non_leader_skips_if_already_cleared(harness): + with ( + patch( + "relations.async_replication.PostgreSQLAsyncReplication._get_unit_ip", + return_value="1.1.1.1", + ), + ): + peer_rel_id = harness.add_relation(PEER, harness.charm.app.name) + harness.add_relation( + REPLICATION_OFFER_RELATION, + harness.charm.app.name, + unit_data={"unit-address": "10.1.1.10"}, + ) + + with harness.hooks_disabled(): + harness.update_relation_data( + peer_rel_id, + harness.charm.unit.name, + {"standby-pgdata-cleared": "True"}, + ) + + with ( + patch( + "charm.Patroni.get_standby_leader", + return_value="postgresql-k8s-0", + ), + patch( + "relations.async_replication.PostgreSQLAsyncReplication._clear_pgdata" + ) as _clear_pgdata, + ): + harness.set_leader(False) + + event = MagicMock() + result = harness.charm.async_replication._wait_for_standby_leader(event) + + assert result is False + _clear_pgdata.assert_not_called() diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index dcde60bc266..cd42cf981a4 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -67,7 +67,7 @@ def test_tls_ca_chain_filename(harness): ) assert ( harness.charm.backup._tls_ca_chain_filename - == "/var/lib/postgresql/data/pgbackrest-tls-ca-chain.crt" + == "/var/lib/pg/data/pgbackrest-tls-ca-chain.crt" ) @@ -310,7 +310,7 @@ def test_construct_endpoint(harness): @pytest.mark.parametrize( - "tls_ca_chain_filename", ["", "/var/lib/postgresql/data/pgbackrest-tls-ca-chain.crt"] + "tls_ca_chain_filename", ["", "/var/lib/pg/data/pgbackrest-tls-ca-chain.crt"] ) def test_create_bucket_if_not_exists(harness, tls_ca_chain_filename): with ( @@ -399,21 +399,50 @@ def test_create_bucket_if_not_exists(harness, tls_ca_chain_filename): def test_empty_data_files(harness): with patch("ops.model.Container.exec") as _exec: - # Test when the removal of the data files fails. - command = ["rm", "-r", "/var/lib/postgresql/data/pgdata"] - _exec.side_effect = ExecError(command=command, exit_code=1, stdout="", stderr="fake error") + # Mock the wait_output method + mock_process = MagicMock() + mock_process.wait_output.return_value = ("", "") + _exec.return_value = mock_process + + # Test when the removal of data files fails on the first directory. + _exec.side_effect = ExecError( + command=["find", "/var/lib/pg/archive", "-mindepth", "1", "-delete"], + exit_code=1, + stdout="", + stderr="fake error", + ) try: harness.charm.backup._empty_data_files() assert False except ExecError: pass - _exec.assert_called_once_with(command) - # Test when data files are successfully removed. + # Test when data files are successfully removed from all directories. _exec.reset_mock() _exec.side_effect = None + _exec.return_value = mock_process + harness.charm.backup._empty_data_files() + # Should be called 4 times: archive, data, logs, temp + assert _exec.call_count == 4 + expected_calls = [ + (["find", "/var/lib/pg/archive", "-mindepth", "1", "-delete"],), + (["find", "/var/lib/pg/data/16/main", "-mindepth", "1", "-delete"],), + (["find", "/var/lib/pg/logs", "-mindepth", "1", "-delete"],), + (["find", "/var/lib/pg/temp", "-mindepth", "1", "-delete"],), + ] + for call, expected in zip(_exec.call_args_list, expected_calls, strict=True): + assert call[0] == expected + + # Test when exec raises ExecError with "No such file or directory" — should not raise. + _exec.reset_mock() + _exec.side_effect = ExecError( + command=["find", "/var/lib/pg/archive", "-mindepth", "1", "-delete"], + exit_code=1, + stdout="", + stderr="find: '/var/lib/pg/archive': No such file or directory", + ) + # Should complete without raising. harness.charm.backup._empty_data_files() - _exec.assert_called_once_with(command) def test_change_connectivity_to_database(harness): @@ -443,7 +472,7 @@ def test_change_connectivity_to_database(harness): def test_execute_command(harness): with patch("ops.model.Container.exec") as _exec: - command = ["rm", "-r", "/var/lib/postgresql/data/pgdata"] + command = ["rm", "-r", "/var/lib/postgresql/16/main"] _exec.return_value.wait_output.return_value = ("fake stdout", "") # Test when the command runs successfully. @@ -1720,9 +1749,11 @@ def test_pre_restore_checks(harness): @pytest.mark.parametrize( - "tls_ca_chain_filename", ["", "/var/lib/postgresql/data/pgbackrest-tls-ca-chain.crt"] + "tls_ca_chain_filename", ["", "/var/lib/pg/data/pgbackrest-tls-ca-chain.crt"] ) def test_render_pgbackrest_conf_file(harness, tls_ca_chain_filename): + mock_pebble = MagicMock() + mock_pebble.get_services.return_value = [MagicMock()] with ( patch("ops.model.Container.start") as _start, patch("ops.model.Container.push") as _push, @@ -1732,6 +1763,7 @@ def test_render_pgbackrest_conf_file(harness, tls_ca_chain_filename): ) as _tls_ca_chain_filename, patch("charm.PostgreSQLBackups._retrieve_s3_parameters") as _retrieve_s3_parameters, patch("charm.PostgresqlOperatorCharm.get_available_resources", return_value=(4, 1024)), + patch("ops.model.Container.pebble", new_callable=PropertyMock(return_value=mock_pebble)), ): # Set up a mock for the `open` method, set returned data to postgresql.conf template. with open("templates/pgbackrest.conf.j2") as f: @@ -1781,6 +1813,7 @@ def test_render_pgbackrest_conf_file(harness, tls_ca_chain_filename): secret_key="test-secret-key", stanza=harness.charm.backup.stanza_name, storage_path=harness.charm._storage_path, + pgdata_path=harness.charm.pgdata_path, user="backup", retention_full=30, process_max=2, @@ -2015,7 +2048,7 @@ def test_start_stop_pgbackrest_service(harness): @pytest.mark.parametrize( - "tls_ca_chain_filename", ["", "/var/lib/postgresql/data/pgbackrest-tls-ca-chain.crt"] + "tls_ca_chain_filename", ["", "/var/lib/pg/data/pgbackrest-tls-ca-chain.crt"] ) def test_upload_content_to_s3(harness, tls_ca_chain_filename): with ( diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index f2cc4de797b..ea1aa891df4 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -770,7 +770,7 @@ def test_postgresql_layer(harness): POSTGRESQL_SERVICE: { "override": "replace", "summary": "entrypoint of the postgresql + patroni image", - "command": "patroni /var/lib/postgresql/data/patroni.yml", + "command": "patroni /var/lib/pg/data/patroni.yml", "startup": "enabled", "on-failure": "restart", "user": "postgres", @@ -1527,17 +1527,54 @@ def test_create_pgdata(harness): container = MagicMock() container.exists.return_value = False harness.charm._create_pgdata(container) - container.make_dir.assert_called_once_with( - "/var/lib/postgresql/data/pgdata", permissions=448, user="postgres", group="postgres" - ) + # When directories don't exist, all three should be created + container.make_dir.assert_has_calls([ + call( + "/var/lib/pg/data/16/main", + permissions=448, + user="postgres", + group="postgres", + make_parents=True, + ), + call( + "/var/lib/pg/logs/16/main/pg_wal", + permissions=448, + user="postgres", + group="postgres", + make_parents=True, + ), + call( + "/var/lib/pg/temp/16/main/pgsql_tmp", + permissions=448, + user="postgres", + group="postgres", + make_parents=True, + ), + call( + "/var/lib/postgresql/16", + user="postgres", + group="postgres", + make_parents=True, + ), + ]) container.exec.assert_has_calls([ - call(["chown", "postgres:postgres", "/var/lib/postgresql/archive"]), + call([ + "bash", + "-c", + "[ -L /var/lib/postgresql/16/main ] || rm -rf /var/lib/postgresql/16/main", + ]), call().wait(), - call(["chown", "postgres:postgres", "/var/lib/postgresql/data"]), + call(["ln", "-sfn", "/var/lib/pg/data/16/main", "/var/lib/postgresql/16/main"]), call().wait(), - call(["chown", "postgres:postgres", "/var/lib/postgresql/logs"]), + call(["chown", "-h", "postgres:postgres", "/var/lib/postgresql/16/main"]), call().wait(), - call(["chown", "postgres:postgres", "/var/lib/postgresql/temp"]), + call(["chown", "postgres:postgres", "/var/lib/pg/archive"]), + call().wait(), + call(["chown", "postgres:postgres", "/var/lib/pg/data"]), + call().wait(), + call(["chown", "postgres:postgres", "/var/lib/pg/logs"]), + call().wait(), + call(["chown", "postgres:postgres", "/var/lib/pg/temp"]), call().wait(), ]) @@ -1545,15 +1582,33 @@ def test_create_pgdata(harness): container.exec.reset_mock() container.exists.return_value = True harness.charm._create_pgdata(container) - container.make_dir.assert_not_called() + # When directories exist, none should be created (except the symlink parent and symlink itself) + container.make_dir.assert_has_calls([ + call( + "/var/lib/postgresql/16", + user="postgres", + group="postgres", + make_parents=True, + ), + ]) container.exec.assert_has_calls([ - call(["chown", "postgres:postgres", "/var/lib/postgresql/archive"]), + call([ + "bash", + "-c", + "[ -L /var/lib/postgresql/16/main ] || rm -rf /var/lib/postgresql/16/main", + ]), + call().wait(), + call(["ln", "-sfn", "/var/lib/pg/data/16/main", "/var/lib/postgresql/16/main"]), call().wait(), - call(["chown", "postgres:postgres", "/var/lib/postgresql/data"]), + call(["chown", "-h", "postgres:postgres", "/var/lib/postgresql/16/main"]), call().wait(), - call(["chown", "postgres:postgres", "/var/lib/postgresql/logs"]), + call(["chown", "postgres:postgres", "/var/lib/pg/archive"]), call().wait(), - call(["chown", "postgres:postgres", "/var/lib/postgresql/temp"]), + call(["chown", "postgres:postgres", "/var/lib/pg/data"]), + call().wait(), + call(["chown", "postgres:postgres", "/var/lib/pg/logs"]), + call().wait(), + call(["chown", "postgres:postgres", "/var/lib/pg/temp"]), call().wait(), ]) @@ -1976,3 +2031,151 @@ def test_calculate_worker_process_config_all_workers_validation_blocking(harness result = harness.charm._calculate_worker_process_config(cpu_cores) assert result["max_parallel_workers"] == "18" # Should accept valid value + + +def test_replica_can_start(harness): + with ( + patch( + "charm.PostgresqlOperatorCharm.is_cluster_initialised", + new_callable=PropertyMock, + ) as _is_cluster_initialised, + patch( + "charm.PostgresqlOperatorCharm._endpoint", + new_callable=PropertyMock, + ) as _endpoint, + patch( + "charm.PostgresqlOperatorCharm._endpoints", + new_callable=PropertyMock, + ) as _endpoints, + ): + # Returns False when cluster is not initialised. + _is_cluster_initialised.return_value = False + _endpoint.return_value = "postgresql-k8s-0" + _endpoints.return_value = ["postgresql-k8s-0"] + assert harness.charm._replica_can_start() is False + + # Returns False when endpoint is not in members list. + _is_cluster_initialised.return_value = True + _endpoint.return_value = "postgresql-k8s-1" + _endpoints.return_value = ["postgresql-k8s-0"] + assert harness.charm._replica_can_start() is False + + # Returns True when both conditions are met. + _endpoint.return_value = "postgresql-k8s-0" + _endpoints.return_value = ["postgresql-k8s-0", "postgresql-k8s-1"] + assert harness.charm._replica_can_start() is True + + +def test_create_pgdata_clears_stale_dirs_on_replica(harness): + container = MagicMock() + + # Simulate: waldir and temp exist, but PG_VERSION does not (pgdata not populated). + def exists_side_effect(path): + if path.endswith("PG_VERSION"): + return False + return "pg_wal" in path or "pgsql_tmp" in path + + container.exists.side_effect = exists_side_effect + + with ( + patch( + "charm.PostgresqlOperatorCharm.is_cluster_initialised", + new_callable=PropertyMock, + return_value=True, + ), + ): + harness.set_leader(False) + harness.charm._create_pgdata(container) + + # Verify that find -delete was called for waldir and temp_tablespace paths. + find_calls = [c for c in container.exec.call_args_list if "find" in str(c)] + assert len(find_calls) >= 2 + container.exec.assert_any_call( + ["find", "/var/lib/pg/logs/16/main/pg_wal", "-mindepth", "1", "-delete"] + ) + container.exec.assert_any_call( + ["find", "/var/lib/pg/temp/16/main/pgsql_tmp", "-mindepth", "1", "-delete"] + ) + + +def test_create_pgdata_stale_dir_clearing_reraises_unexpected_exec_error(harness): + from ops.pebble import ExecError + + container = MagicMock() + + def exists_side_effect(path): + if path.endswith("PG_VERSION"): + return False + return "pg_wal" in path + + container.exists.side_effect = exists_side_effect + + # Make exec raise ExecError with a non "No such file or directory" message. + mock_process = MagicMock() + mock_process.wait_output.side_effect = ExecError( + command=["find"], exit_code=1, stdout="", stderr="Permission denied" + ) + container.exec.return_value = mock_process + + with ( + patch( + "charm.PostgresqlOperatorCharm.is_cluster_initialised", + new_callable=PropertyMock, + return_value=True, + ), + ): + harness.set_leader(False) + # Should NOT raise — the ExecError with non "No such file" stderr is logged as warning + # but not re-raised. + harness.charm._create_pgdata(container) + + +def test_create_pgdata_stale_dir_clearing_ignores_no_such_file(harness): + from ops.pebble import ExecError + + container = MagicMock() + + def exists_side_effect(path): + if path.endswith("PG_VERSION"): + return False + return "pg_wal" in path + + container.exists.side_effect = exists_side_effect + + # Make exec raise ExecError with "No such file or directory" message. + mock_process = MagicMock() + mock_process.wait_output.side_effect = ExecError( + command=["find"], exit_code=1, stdout="", stderr="No such file or directory" + ) + container.exec.return_value = mock_process + + with ( + patch( + "charm.PostgresqlOperatorCharm.is_cluster_initialised", + new_callable=PropertyMock, + return_value=True, + ), + ): + harness.set_leader(False) + # Should not raise — graceful handling of missing directory. + harness.charm._create_pgdata(container) + + +def test_on_postgresql_pebble_ready_defers_when_replica_not_ready(harness): + with ( + patch("charm.PostgresqlOperatorCharm._create_pgdata") as _create_pgdata, + patch("charm.PostgresqlOperatorCharm._replica_can_start", return_value=False), + patch( + "charm.PostgresqlOperatorCharm._update_pebble_layers" + ) as _update_pebble_layers, + ): + harness.set_leader(False) + harness.set_can_connect(POSTGRESQL_CONTAINER, True) + mock_event = MagicMock() + mock_event.workload = harness.model.unit.get_container(POSTGRESQL_CONTAINER) + + harness.charm._on_postgresql_pebble_ready(mock_event) + + _create_pgdata.assert_called_once() + mock_event.defer.assert_called_once() + _update_pebble_layers.assert_not_called() diff --git a/tests/unit/test_patroni.py b/tests/unit/test_patroni.py index 9c9d53ba905..702a05417f0 100644 --- a/tests/unit/test_patroni.py +++ b/tests/unit/test_patroni.py @@ -12,9 +12,9 @@ from tenacity import RetryError, stop_after_delay, wait_fixed from charm import PostgresqlOperatorCharm -from constants import API_REQUEST_TIMEOUT, REWIND_USER +from constants import API_REQUEST_TIMEOUT, LOGS_STORAGE_PATH, REWIND_USER from patroni import Patroni, SwitchoverFailedError, SwitchoverNotSyncError -from tests.helpers import STORAGE_PATH +from tests.helpers import PGDATA_PATH, STORAGE_PATH @pytest.fixture(autouse=True) @@ -35,6 +35,7 @@ def patroni(harness): "postgresql-k8s-primary.dev.svc.cluster.local", "test-model", STORAGE_PATH, + PGDATA_PATH, "superuser-password", "replication-password", "rewind-password", @@ -224,6 +225,8 @@ def test_render_patroni_yml_file(harness, patroni): endpoints=patroni._endpoints, namespace=patroni._namespace, storage_path=patroni._storage_path, + logs_storage_path=LOGS_STORAGE_PATH, + pgdata_path=patroni._pgdata_path, superuser_password=patroni._superuser_password, replication_password=patroni._replication_password, rewind_user=REWIND_USER, @@ -261,6 +264,8 @@ def test_render_patroni_yml_file(harness, patroni): endpoints=patroni._endpoints, namespace=patroni._namespace, storage_path=patroni._storage_path, + logs_storage_path=LOGS_STORAGE_PATH, + pgdata_path=patroni._pgdata_path, superuser_password=patroni._superuser_password, replication_password=patroni._replication_password, rewind_user=REWIND_USER, @@ -288,9 +293,9 @@ def test_render_patroni_yml_file(harness, patroni): # Also, ensure the right parameters are in the expected content # (as it was already validated with the above render file call). assert "ssl: on" in expected_content_with_tls - assert "ssl_ca_file: /var/lib/postgresql/data/ca.pem" in expected_content_with_tls - assert "ssl_cert_file: /var/lib/postgresql/data/cert.pem" in expected_content_with_tls - assert "ssl_key_file: /var/lib/postgresql/data/key.pem" in expected_content_with_tls + assert "ssl_ca_file: /var/lib/pg/data/ca.pem" in expected_content_with_tls + assert "ssl_cert_file: /var/lib/pg/data/cert.pem" in expected_content_with_tls + assert "ssl_key_file: /var/lib/pg/data/key.pem" in expected_content_with_tls def test_primary_endpoint_ready(harness, patroni): @@ -526,3 +531,76 @@ def test_update_synchronous_node_count(harness, patroni): with pytest.raises(RetryError): patroni.update_synchronous_node_count() assert False + + +def test_ensure_slots_controller_by_patroni(harness, patroni): + with ( + patch("patroni.stop_after_delay", return_value=stop_after_delay(0)), + patch("patroni.wait_fixed", return_value=wait_fixed(0)), + patch("requests.get") as _get, + patch("requests.patch") as _patch, + ): + # Success path: no existing slots, add one new slot. + get_response = MagicMock() + get_response.status_code = 200 + get_response.json.return_value = {"slots": {}} + get_response.elapsed.total_seconds.return_value = 0.1 + _get.return_value = get_response + + patch_response = MagicMock() + patch_response.status_code = 200 + _patch.return_value = patch_response + + result = patroni.ensure_slots_controller_by_patroni({"slot1": "db1"}) + assert result is True + _patch.assert_called_once_with( + f"{patroni._patroni_url}/config", + verify=patroni._verify, + json={"slots": {"slot1": {"database": "db1", "plugin": "pgoutput", "type": "logical"}}}, + auth=patroni._patroni_auth, + timeout=API_REQUEST_TIMEOUT, + ) + + # Existing slots are merged: old_slot should be set to None (removed). + _get.reset_mock() + _patch.reset_mock() + get_response.json.return_value = { + "slots": {"old_slot": {"database": "old_db", "plugin": "pgoutput", "type": "logical"}} + } + _get.return_value = get_response + _patch.return_value = patch_response + + result = patroni.ensure_slots_controller_by_patroni({"slot1": "db1"}) + assert result is True + expected_slots = { + "old_slot": None, + "slot1": {"database": "db1", "plugin": "pgoutput", "type": "logical"}, + } + _patch.assert_called_once_with( + f"{patroni._patroni_url}/config", + verify=patroni._verify, + json={"slots": expected_slots}, + auth=patroni._patroni_auth, + timeout=API_REQUEST_TIMEOUT, + ) + + # RetryError path: 502 status code. + _get.reset_mock() + _patch.reset_mock() + get_response_502 = MagicMock() + get_response_502.status_code = 502 + get_response_502.elapsed.total_seconds.return_value = 0.1 + _get.return_value = get_response_502 + + result = patroni.ensure_slots_controller_by_patroni({"slot1": "db1"}) + assert result is False + _patch.assert_not_called() + + # RetryError path: connection error. + _get.reset_mock() + _patch.reset_mock() + _get.side_effect = requests.exceptions.ConnectionError("connection refused") + + result = patroni.ensure_slots_controller_by_patroni({"slot1": "db1"}) + assert result is False + _patch.assert_not_called()