From c2f8d9b00eb9ebf3fd3c7db460189758c74a3327 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Fri, 24 Apr 2026 14:24:26 -0300 Subject: [PATCH 01/19] feat(storage): migrate all data mounts to versioned 16/main subdirectories Introduce versioned path constants (POSTGRESQL_DATA_DIR, ARCHIVE_DATA_DIR, LOGS_DATA_DIR, TEMP_DATA_DIR = /16/main) alongside the existing storage-root constants. All internal path references are updated to use the versioned paths. On snap refresh, _ensure_storage_layout() performs a one-time, idempotent migration of existing files from each storage root into the versioned subdirectory, recording completion via a `storage_layout_migrated` flag in the peer-relation databag. _repair_pg_wal_symlink() updates the pg_wal symlink to point at the new WAL path after migration. _reconcile_storage_permissions() runs unconditionally on every refresh to heal storage root permissions (0755) and pg_wal symlink ownership (_daemon_:_daemon_), fixing units migrated by earlier builds. On the primary, _ensure_temp_tablespace_location() migrates the temp tablespace location in the PostgreSQL catalog. The connection requires autocommit=True since DROP/CREATE TABLESPACE cannot run inside a transaction block. _clear_pg_version_dirs() purges PG_* version directories from the target path before CREATE TABLESPACE to prevent ObjectInUse errors. patroni.yml.j2 now uses a {{ wal_dir }} template variable instead of the hardcoded storage-root WAL path. Signed-off-by: Marcelo Henrique Neppel --- src/backups.py | 17 +- src/charm.py | 240 ++++++++++++- src/cluster.py | 11 +- src/constants.py | 9 + src/relations/async_replication.py | 18 +- templates/patroni.yml.j2 | 4 +- tests/integration/ha_tests/helpers.py | 2 +- tests/integration/jubilant_helpers.py | 2 +- tests/integration/test_tls.py | 2 +- tests/unit/test_backups.py | 16 +- tests/unit/test_charm.py | 497 ++++++++++++++++++++++---- tests/unit/test_cluster.py | 14 +- 12 files changed, 714 insertions(+), 118 deletions(-) diff --git a/src/backups.py b/src/backups.py index 457b21c23d..78353bd3cd 100644 --- a/src/backups.py +++ b/src/backups.py @@ -30,9 +30,11 @@ from tenacity import RetryError, Retrying, stop_after_attempt, wait_fixed from constants import ( + ARCHIVE_DATA_DIR, BACKUP_ID_FORMAT, BACKUP_TYPE_OVERRIDES, BACKUP_USER, + LOGS_DATA_DIR, PATRONI_CONF_PATH, PGBACKREST_ARCHIVE_TIMEOUT_ERROR_CODE, PGBACKREST_BACKUP_ID_FORMAT, @@ -42,7 +44,8 @@ PGBACKREST_LOG_LEVEL_STDERR, PGBACKREST_LOGROTATE_FILE, PGBACKREST_LOGS_PATH, - POSTGRESQL_DATA_PATH, + POSTGRESQL_DATA_DIR, + TEMP_DATA_DIR, UNIT_SCOPE, ) from relations.async_replication import REPLICATION_CONSUMER_RELATION, REPLICATION_OFFER_RELATION @@ -245,7 +248,7 @@ def can_use_s3_repository(self) -> tuple[bool, str]: return_code, system_identifier_from_instance, error = self._execute_command([ f"/snap/charmed-postgresql/current/usr/lib/postgresql/{self.charm._patroni.get_postgresql_version().split('.')[0]}/bin/pg_controldata", - POSTGRESQL_DATA_PATH, + POSTGRESQL_DATA_DIR, ]) if return_code != 0: raise Exception(error) @@ -353,10 +356,10 @@ def _create_bucket_if_not_exists(self) -> None: def _empty_data_files(self) -> bool: """Empty the PostgreSQL data directory in preparation of backup restore.""" paths = [ - "/var/snap/charmed-postgresql/common/data/archive", - POSTGRESQL_DATA_PATH, - "/var/snap/charmed-postgresql/common/data/logs", - "/var/snap/charmed-postgresql/common/data/temp", + ARCHIVE_DATA_DIR, + POSTGRESQL_DATA_DIR, + LOGS_DATA_DIR, + TEMP_DATA_DIR, ] path = None try: @@ -1379,7 +1382,7 @@ def _render_pgbackrest_conf_file(self) -> bool: enable_tls=len(self.charm._peer_members_ips) > 0, peer_endpoints=self.charm._peer_members_ips, path=s3_parameters["path"], - data_path=f"{POSTGRESQL_DATA_PATH}", + data_path=POSTGRESQL_DATA_DIR, log_path=f"{PGBACKREST_LOGS_PATH}", region=s3_parameters.get("region"), endpoint=s3_parameters["endpoint"], diff --git a/src/charm.py b/src/charm.py index fd40d6ed2c..9d8db797fc 100755 --- a/src/charm.py +++ b/src/charm.py @@ -10,7 +10,9 @@ import os import pathlib import platform +import pwd import re +import shutil import subprocess import sys import time @@ -60,6 +62,7 @@ BACKUP_USER, MONITORING_USER, PEER, + POSTGRESQL_STORAGE_PERMISSIONS, REPLICATION_USER, REWIND_USER, SYSTEM_USERS, @@ -100,9 +103,14 @@ from config import CharmConfig from constants import ( APP_SCOPE, + ARCHIVE_DATA_DIR, + ARCHIVE_STORAGE_PATH, + DATA_DIR_SUBFOLDER, DATABASE, DATABASE_DEFAULT_NAME, DATABASE_PORT, + LOGS_DATA_DIR, + LOGS_STORAGE_PATH, METRICS_PORT, MONITORING_PASSWORD_KEY, MONITORING_SNAP_SERVICE, @@ -111,6 +119,7 @@ PGBACKREST_METRICS_PORT, PGBACKREST_MONITORING_SNAP_SERVICE, PLUGIN_OVERRIDES, + POSTGRESQL_DATA_DIR, POSTGRESQL_DATA_PATH, RAFT_PASSWORD_KEY, REPLICATION_CONSUMER_RELATION, @@ -121,6 +130,9 @@ SECRET_INTERNAL_LABEL, SECRET_KEY_OVERRIDES, SPI_MODULE, + STORAGE_LAYOUT_MIGRATED_KEY, + TEMP_DATA_DIR, + TEMP_STORAGE_PATH, TLS_CA_BUNDLE_FILE, TLS_CA_FILE, TLS_CERT_FILE, @@ -136,13 +148,14 @@ from relations.postgresql_provider import PostgreSQLProvider from relations.tls import TLS from rotate_logs import RotateLogs -from utils import label2name, new_password, render_file +from utils import _change_owner, label2name, new_password, render_file logger = logging.getLogger(__name__) logging.getLogger("httpx").setLevel(logging.WARNING) logging.getLogger("httpcore").setLevel(logging.WARNING) logging.getLogger("asyncio").setLevel(logging.WARNING) logging.getLogger("boto3").setLevel(logging.WARNING) +STORAGE_LAYOUT_IGNORED_ENTRIES = {DATA_DIR_SUBFOLDER.split("/", maxsplit=1)[0], "lost+found"} logging.getLogger("botocore").setLevel(logging.WARNING) PRIMARY_NOT_REACHABLE_MESSAGE = "waiting for primary to be reachable from this unit" @@ -405,11 +418,8 @@ def __init__(self, *args): self.tracing = Tracing(self, tracing_relation_name=TRACING_RELATION_NAME) charm_tracing_config(self._grafana_agent) - def _post_snap_refresh(self, refresh: charm_refresh.Machines): - """Start PostgreSQL, check if this app and unit are healthy, and allow next unit to refresh. - - Called after snap refresh - """ + def _check_and_update_internal_cert(self) -> None: + """Check if the internal cert CN matches the unit IP and regenerate if needed.""" try: if raw_cert := self.get_secret(UNIT_SCOPE, "internal-cert"): cert = load_pem_x509_certificate(raw_cert.encode()) @@ -421,6 +431,22 @@ def _post_snap_refresh(self, refresh: charm_refresh.Machines): except Exception: logger.exception("Unable to check or update internal cert") + def _post_snap_refresh(self, refresh: charm_refresh.Machines): + """Start PostgreSQL, check if this app and unit are healthy, and allow next unit to refresh. + + Called after snap refresh + """ + self._check_and_update_internal_cert() + + try: + self._ensure_storage_layout() + except (OSError, RuntimeError): + logger.exception("Unable to migrate PostgreSQL storage layout") + self.set_unit_status( + ops.BlockedStatus("Failed to migrate PostgreSQL storage layout"), refresh=refresh + ) + return + if not self._patroni.start_patroni(): self.set_unit_status(ops.BlockedStatus("Failed to start PostgreSQL"), refresh=refresh) return @@ -450,6 +476,7 @@ def _post_snap_refresh(self, refresh: charm_refresh.Machines): "Did not allow next unit to refresh: member not ready or not joined the cluster yet" ) else: + self._ensure_temp_tablespace_location_if_primary() try: self._patroni.set_max_timelines_history() except Exception: @@ -654,6 +681,179 @@ def is_unit_stopped(self) -> bool: """Returns whether the unit is stopped.""" return "stopped" in self.unit_peer_data + @property + def is_storage_layout_migrated(self) -> bool: + """Returns whether this unit already migrated to the versioned storage layout.""" + return self.unit_peer_data.get(STORAGE_LAYOUT_MIGRATED_KEY) == "True" + + def _migrate_storage_mount(self, storage_path: str, target_path: str) -> None: + """Move a storage mount root into the versioned PostgreSQL data layout.""" + Path(storage_path).mkdir(parents=True, exist_ok=True) + Path(target_path).mkdir(parents=True, exist_ok=True) + _change_owner(target_path) + os.chmod(target_path, POSTGRESQL_STORAGE_PERMISSIONS) + + for item in os.listdir(storage_path): + if item in STORAGE_LAYOUT_IGNORED_ENTRIES: + continue + + source = os.path.join(storage_path, item) + destination = os.path.join(target_path, item) + logger.info("Moving %s to %s", source, destination) + os.rename(source, destination) + + def _repair_pg_wal_symlink(self) -> None: + """Ensure pg_wal points to the versioned WAL directory for migrated deployments.""" + pg_wal_path = Path(POSTGRESQL_DATA_DIR) / "pg_wal" + if not pg_wal_path.is_symlink(): + return + + current_target = Path(os.path.realpath(pg_wal_path)) + new_target = Path(LOGS_DATA_DIR) + if current_target == new_target: + return + + old_target = Path(LOGS_STORAGE_PATH) + if current_target != old_target: + logger.warning( + "Skipping pg_wal repair: unexpected target %s (expected %s or %s)", + current_target, + old_target, + new_target, + ) + return + + self._migrate_storage_mount(LOGS_STORAGE_PATH, LOGS_DATA_DIR) + pg_wal_path.unlink() + pg_wal_path.symlink_to(LOGS_DATA_DIR) + logger.info("Updated pg_wal symlink to %s", LOGS_DATA_DIR) + + def _ensure_storage_layout(self) -> None: + """Ensure the PostgreSQL storage layout is prepared or migrated exactly once.""" + if self._peers is None: + raise RuntimeError("Peer relation not ready") + if not self.is_storage_layout_migrated: + for storage_path, target_path in ( + (POSTGRESQL_DATA_PATH, POSTGRESQL_DATA_DIR), + (ARCHIVE_STORAGE_PATH, ARCHIVE_DATA_DIR), + (LOGS_STORAGE_PATH, LOGS_DATA_DIR), + (TEMP_STORAGE_PATH, TEMP_DATA_DIR), + ): + self._migrate_storage_mount(storage_path, target_path) + + self.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] = "True" + + self._repair_pg_wal_symlink() + self._reconcile_storage_permissions() + + def _reconcile_storage_permissions(self) -> None: + """Reconcile storage root permissions and pg_wal symlink ownership. + + Storage roots (e.g. common/var/lib/postgresql/) are container directories + after migration and must be 0755, not the 0700 of a PostgreSQL data dir. + The pg_wal symlink must be owned by _daemon_ — it is created by Patroni on + fresh deploys, but recreated as root when the charm repairs it during upgrade. + This method is idempotent and always runs so it heals units migrated before + these fixes were applied. + """ + for storage_path in ( + POSTGRESQL_DATA_PATH, + ARCHIVE_STORAGE_PATH, + LOGS_STORAGE_PATH, + TEMP_STORAGE_PATH, + ): + if os.path.exists(storage_path): + os.chmod(storage_path, 0o755) # noqa: S103 + + pg_wal_path = Path(POSTGRESQL_DATA_DIR) / "pg_wal" + if pg_wal_path.is_symlink(): + user_database = pwd.getpwnam("_daemon_") + os.lchown(str(pg_wal_path), user_database.pw_uid, user_database.pw_gid) + + @staticmethod + def _clear_pg_version_dirs(path: Path) -> None: + """Remove PostgreSQL version subdirectories (PG__) from a directory. + + These directories are created by PostgreSQL when a tablespace is created and must not + exist at a target path before CREATE TABLESPACE is called. Temp tablespace data is + ephemeral, so removal is safe. + """ + if path.exists(): + for entry in path.iterdir(): + if entry.name.startswith("PG_"): + shutil.rmtree(entry) + + def _ensure_temp_tablespace_location(self) -> bool: + """Ensure the temp tablespace points to the versioned temp directory. + + DROP TABLESPACE and CREATE TABLESPACE cannot run inside a transaction block, so this + method avoids using the connection as a context manager (which would create one in + psycopg2). Instead it uses plain assignments and explicit close(), mirroring the + pattern in the single_kernel_postgresql set_up_database helper. + """ + connection = None + cursor = None + try: + connection = self.postgresql._connect_to_database() + connection.autocommit = True # DROP/CREATE TABLESPACE cannot run inside a transaction + cursor = connection.cursor() + + cursor.execute( + "SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';" + ) + row = cursor.fetchone() + if row is None: + # The tablespace may have been dropped by a previous partially-failed + # migration (DROP succeeded, CREATE failed). If the versioned directory + # already exists, recreate the tablespace there. + pg_data_dir = Path(TEMP_DATA_DIR) + if pg_data_dir.exists(): + self._clear_pg_version_dirs(pg_data_dir) + cursor.execute(f"CREATE TABLESPACE temp LOCATION '{TEMP_DATA_DIR}';") + cursor.execute("GRANT CREATE ON TABLESPACE temp TO public;") + return True + + current_location = row[0] + if current_location == TEMP_DATA_DIR: + return True + + if current_location != TEMP_STORAGE_PATH: + logger.warning( + "Skipping temp tablespace migration: unexpected location %s " + "(expected %s or %s)", + current_location, + TEMP_STORAGE_PATH, + TEMP_DATA_DIR, + ) + return True + + logger.info( + "Migrating temp tablespace location from %s to %s", + TEMP_STORAGE_PATH, + TEMP_DATA_DIR, + ) + cursor.execute("DROP TABLESPACE temp;") + self._clear_pg_version_dirs(Path(TEMP_DATA_DIR)) + cursor.execute(f"CREATE TABLESPACE temp LOCATION '{TEMP_DATA_DIR}';") + cursor.execute("GRANT CREATE ON TABLESPACE temp TO public;") + except psycopg2.Error: + logger.exception("Failed to migrate temp tablespace location") + return False + finally: + if cursor is not None: + cursor.close() + if connection is not None: + connection.close() + + return True + + def _ensure_temp_tablespace_location_if_primary(self) -> bool: + """Ensure the temp tablespace is migrated when this unit is the primary.""" + if not self.is_primary: + return True + + return self._ensure_temp_tablespace_location() + @cached_property def postgresql(self) -> PostgreSQL: """Returns an instance of the object used to interact with the database.""" @@ -1000,6 +1200,10 @@ def _on_peer_relation_changed(self, event: HookEvent): self._start_stop_pgbackrest_service(event) + if not self._ensure_temp_tablespace_location_if_primary(): + event.defer() + return + # This is intended to be executed only when leader is reinitializing S3 connection due to the leader change. if ( "s3-initialization-start" in self.app_peer_data @@ -1660,6 +1864,12 @@ def _on_start(self, event: StartEvent) -> None: self.tls.generate_internal_peer_cert() self.unit_peer_data.update({"ip": self._unit_ip}) + try: + self._ensure_storage_layout() + except (OSError, RuntimeError): + logger.exception("Unable to migrate PostgreSQL storage layout") + self.set_unit_status(BlockedStatus("Failed to migrate PostgreSQL storage layout")) + return # Open port try: @@ -1803,9 +2013,7 @@ def _setup_users(self) -> None: extra_user_roles=[ROLE_STATS], ) - self.postgresql.set_up_database( - temp_location="/var/snap/charmed-postgresql/common/data/temp" - ) + self.postgresql.set_up_database(temp_location=TEMP_DATA_DIR) access_groups = self.postgresql.list_access_groups() if access_groups != set(ACCESS_GROUPS): @@ -1990,7 +2198,7 @@ def promote_primary_unit(self, event: ActionEvent) -> None: except SwitchoverFailedError: event.fail("Switchover failed or timed out, check the logs for details") - def _on_update_status(self, _) -> None: + def _on_update_status(self, event) -> None: """Update the unit status message and users list in the database.""" if not self._can_run_on_update_status(): return @@ -2003,6 +2211,12 @@ def _on_update_status(self, _) -> None: if self._handle_processes_failures(): return + if self.unit.is_leader() and not self._reconfigure_cluster(event): + return + + if not self._ensure_temp_tablespace_location_if_primary(): + return + self.postgresql_client_relation.oversee_users() if self.primary_endpoint: self._update_relation_endpoints() @@ -2129,11 +2343,11 @@ def _handle_processes_failures(self) -> bool: # Restart the PostgreSQL process if it was frozen (in that case, the Patroni # process is running by the PostgreSQL process not). if self._unit_ip in self.members_ips and self._patroni.member_inactive: - data_directory_contents = os.listdir(POSTGRESQL_DATA_PATH) + data_directory_contents = os.listdir(POSTGRESQL_DATA_DIR) if len(data_directory_contents) == 1 and data_directory_contents[0] == "pg_wal": os.rename( - os.path.join(POSTGRESQL_DATA_PATH, "pg_wal"), - os.path.join(POSTGRESQL_DATA_PATH, f"pg_wal-{datetime.now(UTC).isoformat()}"), + os.path.join(POSTGRESQL_DATA_DIR, "pg_wal"), + os.path.join(POSTGRESQL_DATA_DIR, f"pg_wal-{datetime.now(UTC).isoformat()}"), ) logger.info("PostgreSQL data directory was not empty. Moved pg_wal") return True diff --git a/src/cluster.py b/src/cluster.py index f03f238294..8176956a47 100644 --- a/src/cluster.py +++ b/src/cluster.py @@ -48,13 +48,14 @@ from constants import ( API_REQUEST_TIMEOUT, + LOGS_DATA_DIR, PATRONI_CLUSTER_STATUS_ENDPOINT, PATRONI_CONF_PATH, PATRONI_LOGS_PATH, PATRONI_SERVICE_DEFAULT_PATH, PGBACKREST_CONFIGURATION_FILE, POSTGRESQL_CONF_PATH, - POSTGRESQL_DATA_PATH, + POSTGRESQL_DATA_DIR, POSTGRESQL_LOGS_PATH, TLS_CA_BUNDLE_FILE, ) @@ -222,14 +223,15 @@ def bootstrap_cluster(self) -> bool: def configure_patroni_on_unit(self): """Configure Patroni (configuration files and service) on the unit.""" - _change_owner(POSTGRESQL_DATA_PATH) + os.makedirs(POSTGRESQL_DATA_DIR, exist_ok=True) + _change_owner(POSTGRESQL_DATA_DIR) # Create empty base config open(PG_BASE_CONF_PATH, "a").close() # Expected permission # Replicas refuse to start with the default permissions - os.chmod(POSTGRESQL_DATA_PATH, POSTGRESQL_STORAGE_PERMISSIONS) + os.chmod(POSTGRESQL_DATA_DIR, POSTGRESQL_STORAGE_PERMISSIONS) @cached_property def cluster_members(self) -> set: @@ -675,7 +677,8 @@ def render_patroni_yml_file( is_creating_backup=is_creating_backup, log_path=PATRONI_LOGS_PATH, postgresql_log_path=POSTGRESQL_LOGS_PATH, - data_path=POSTGRESQL_DATA_PATH, + data_path=POSTGRESQL_DATA_DIR, + wal_dir=LOGS_DATA_DIR, enable_ldap=enable_ldap, enable_tls=enable_tls, member_name=self.member_name, diff --git a/src/constants.py b/src/constants.py index e3f6c84fa6..def315fc71 100644 --- a/src/constants.py +++ b/src/constants.py @@ -39,10 +39,14 @@ SNAP_COMMON_PATH = "/var/snap/charmed-postgresql/common" SNAP_CURRENT_PATH = "/var/snap/charmed-postgresql/current" +DATA_DIR_SUBFOLDER = "16/main" SNAP_CONF_PATH = f"{SNAP_CURRENT_PATH}/etc" SNAP_DATA_PATH = f"{SNAP_COMMON_PATH}/var/lib" SNAP_LOGS_PATH = f"{SNAP_COMMON_PATH}/var/log" +ARCHIVE_STORAGE_PATH = f"{SNAP_COMMON_PATH}/data/archive" +LOGS_STORAGE_PATH = f"{SNAP_COMMON_PATH}/data/logs" +TEMP_STORAGE_PATH = f"{SNAP_COMMON_PATH}/data/temp" PATRONI_CONF_PATH = f"{SNAP_CONF_PATH}/patroni" PATRONI_LOGS_PATH = f"{SNAP_LOGS_PATH}/patroni" @@ -52,6 +56,10 @@ POSTGRESQL_CONF_PATH = f"{SNAP_CONF_PATH}/postgresql" POSTGRESQL_DATA_PATH = f"{SNAP_DATA_PATH}/postgresql" +POSTGRESQL_DATA_DIR = f"{POSTGRESQL_DATA_PATH}/{DATA_DIR_SUBFOLDER}" +ARCHIVE_DATA_DIR = f"{ARCHIVE_STORAGE_PATH}/{DATA_DIR_SUBFOLDER}" +LOGS_DATA_DIR = f"{LOGS_STORAGE_PATH}/{DATA_DIR_SUBFOLDER}" +TEMP_DATA_DIR = f"{TEMP_STORAGE_PATH}/{DATA_DIR_SUBFOLDER}" POSTGRESQL_LOGS_PATH = f"{SNAP_LOGS_PATH}/postgresql" UPDATE_CERTS_BIN_PATH = "/usr/sbin/update-ca-certificates" @@ -90,3 +98,4 @@ TRACING_RELATION_NAME = "tracing" PGBACKREST_LOGROTATE_FILE = "/etc/logrotate.d/pgbackrest.logrotate" +STORAGE_LAYOUT_MIGRATED_KEY = "storage_layout_migrated" diff --git a/src/relations/async_replication.py b/src/relations/async_replication.py index b590f5442e..fe71b3b189 100644 --- a/src/relations/async_replication.py +++ b/src/relations/async_replication.py @@ -46,11 +46,15 @@ from cluster import ClusterNotPromotedError, NotReadyError, StandbyClusterAlreadyPromotedError from constants import ( APP_SCOPE, + ARCHIVE_DATA_DIR, + LOGS_DATA_DIR, PATRONI_CONF_PATH, PEER, + POSTGRESQL_DATA_DIR, POSTGRESQL_DATA_PATH, REPLICATION_CONSUMER_RELATION, REPLICATION_OFFER_RELATION, + TEMP_DATA_DIR, ) logger = logging.getLogger(__name__) @@ -212,7 +216,7 @@ def _configure_standby_cluster(self, event: RelationChangedEvent) -> bool: logger.info("Creating backup of data folder") filename = f"{POSTGRESQL_DATA_PATH}-{str(datetime.now()).replace(' ', '-').replace(':', '-')}.tar.gz" # Input is hardcoded - subprocess.check_call(f"tar -zcf {filename} {POSTGRESQL_DATA_PATH}".split()) # noqa: S603 + subprocess.check_call(f"tar -zcf {filename} {POSTGRESQL_DATA_DIR}".split()) # noqa: S603 logger.warning("Please review the backup file %s and handle its removal", filename) self.charm.app_peer_data["suppress-oversee-users"] = "true" return True @@ -370,7 +374,7 @@ def result(): process = run( # noqa: S603 [ f"/snap/charmed-postgresql/current/usr/lib/postgresql/{self.charm._patroni.get_postgresql_version().split('.')[0]}/bin/pg_controldata", - POSTGRESQL_DATA_PATH, + POSTGRESQL_DATA_DIR, ], capture_output=True, preexec_fn=demote(), @@ -701,10 +705,10 @@ def _re_emit_async_relation_changed_event(self) -> None: def _reinitialise_pgdata(self) -> None: """Reinitialise the data folder.""" paths = [ - "/var/snap/charmed-postgresql/common/data/archive", - POSTGRESQL_DATA_PATH, - "/var/snap/charmed-postgresql/common/data/logs", - "/var/snap/charmed-postgresql/common/data/temp", + ARCHIVE_DATA_DIR, + POSTGRESQL_DATA_DIR, + LOGS_DATA_DIR, + TEMP_DATA_DIR, ] path = None try: @@ -754,7 +758,7 @@ def set_app_status(self) -> None: def _stop_database(self, event: RelationChangedEvent) -> bool: """Stop the database.""" if not self.charm.is_unit_stopped and not self._is_following_promoted_cluster(): - if not self.charm.unit.is_leader() and not os.path.exists(POSTGRESQL_DATA_PATH): + if not self.charm.unit.is_leader() and not os.path.exists(POSTGRESQL_DATA_DIR): logger.debug("Early exit on_async_relation_changed: following promoted cluster.") return False diff --git a/templates/patroni.yml.j2 b/templates/patroni.yml.j2 index d17fad8fe5..e7486d061b 100644 --- a/templates/patroni.yml.j2 +++ b/templates/patroni.yml.j2 @@ -133,13 +133,13 @@ bootstrap: initdb: - encoding: UTF8 - data-checksums - - waldir: /var/snap/charmed-postgresql/common/data/logs + - waldir: {{ wal_dir }} {%- endif %} postgresql: listen: {% for ip in listen_ips %}{{ ip }}{%- if not loop.last %},{% endif %}{% endfor %}:5432 basebackup: - - waldir: /var/snap/charmed-postgresql/common/data/logs + - waldir: {{ wal_dir }} connect_address: '{{ self_ip }}:5432' # Path to PostgreSQL binaries used in the database bootstrap process. bin_dir: /snap/charmed-postgresql/current/usr/lib/postgresql/{{ version }}/bin diff --git a/tests/integration/ha_tests/helpers.py b/tests/integration/ha_tests/helpers.py index 3a0f396de2..380502d5c2 100644 --- a/tests/integration/ha_tests/helpers.py +++ b/tests/integration/ha_tests/helpers.py @@ -681,7 +681,7 @@ async def get_primary(ops_test: OpsTest, app, down_unit: str | None = None) -> s 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/snap/charmed-postgresql/common/var/lib/postgresql/pg_wal/" + command = "ls -1 /var/snap/charmed-postgresql/common/var/lib/postgresql/16/main/pg_wal/" files = {} for unit in units: stdout = await run_command_on_unit(ops_test, unit, command) diff --git a/tests/integration/jubilant_helpers.py b/tests/integration/jubilant_helpers.py index 284c68bcd7..309619bf19 100644 --- a/tests/integration/jubilant_helpers.py +++ b/tests/integration/jubilant_helpers.py @@ -417,7 +417,7 @@ def check_for_fix_log_message(juju: jubilant.Juju, unit_name: str) -> bool: ) expected_message = ( - "Fixed permissions on temp tablespace directory at /var/snap/charmed-postgresql/common/data/temp " + "Fixed permissions on temp tablespace directory at /var/snap/charmed-postgresql/common/data/temp/16/main " "(persistent storage), existing tablespace remains valid" ) diff --git a/tests/integration/test_tls.py b/tests/integration/test_tls.py index dc48f81c35..ab8d6d5246 100644 --- a/tests/integration/test_tls.py +++ b/tests/integration/test_tls.py @@ -110,7 +110,7 @@ async def test_tls_enabled(ops_test: OpsTest) -> None: await run_command_on_unit( ops_test, replica, - "sudo charmed-postgresql.pg-ctl -D /var/snap/charmed-postgresql/common/var/lib/postgresql/ promote", + "sudo charmed-postgresql.pg-ctl -D /var/snap/charmed-postgresql/common/var/lib/postgresql/16/main promote", ) # Check that the replica was promoted. diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index f1e69f2bc0..e361bea337 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -21,7 +21,7 @@ PostgreSQLBackups, ) from charm import PostgresqlOperatorCharm -from constants import PEER +from constants import ARCHIVE_DATA_DIR, LOGS_DATA_DIR, PEER, POSTGRESQL_DATA_DIR, TEMP_DATA_DIR ANOTHER_CLUSTER_REPOSITORY_ERROR_MESSAGE = "the S3 repository has backups from another cluster" FAILED_TO_ACCESS_CREATE_BUCKET_ERROR_MESSAGE = ( @@ -555,19 +555,17 @@ def test_empty_data_files(harness): _is_dir.return_value = True _rmtree.side_effect = OSError assert not harness.charm.backup._empty_data_files() - _rmtree.assert_called_once_with( - "/var/snap/charmed-postgresql/common/data/archive/test_file.txt" - ) + _rmtree.assert_called_once_with(f"{ARCHIVE_DATA_DIR}/test_file.txt") # Test when data files are successfully removed. _rmtree.reset_mock() _rmtree.side_effect = None assert harness.charm.backup._empty_data_files() _rmtree.assert_has_calls([ - call("/var/snap/charmed-postgresql/common/data/archive/test_file.txt"), - call("/var/snap/charmed-postgresql/common/var/lib/postgresql/test_file.txt"), - call("/var/snap/charmed-postgresql/common/data/logs/test_file.txt"), - call("/var/snap/charmed-postgresql/common/data/temp/test_file.txt"), + call(f"{ARCHIVE_DATA_DIR}/test_file.txt"), + call(f"{POSTGRESQL_DATA_DIR}/test_file.txt"), + call(f"{LOGS_DATA_DIR}/test_file.txt"), + call(f"{TEMP_DATA_DIR}/test_file.txt"), ]) @@ -1855,7 +1853,7 @@ def test_render_pgbackrest_conf_file(harness, tls_ca_chain_filename): enable_tls=len(harness.charm._peer_members_ips) > 0, peer_endpoints=harness.charm._peer_members_ips, path="test-path/", - data_path="/var/snap/charmed-postgresql/common/var/lib/postgresql", + data_path=POSTGRESQL_DATA_DIR, log_path="/var/snap/charmed-postgresql/common/var/log/pgbackrest", region="us-east-1", endpoint="https://storage.googleapis.com", diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 9e042fef30..a73aa6a297 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -48,7 +48,15 @@ SwitchoverFailedError, SwitchoverNotSyncError, ) -from constants import PEER, POSTGRESQL_DATA_PATH, SECRET_INTERNAL_LABEL, UPDATE_CERTS_BIN_PATH +from constants import ( + PEER, + POSTGRESQL_DATA_DIR, + SECRET_INTERNAL_LABEL, + STORAGE_LAYOUT_MIGRATED_KEY, + TEMP_DATA_DIR, + TEMP_STORAGE_PATH, + UPDATE_CERTS_BIN_PATH, +) CREATE_CLUSTER_CONF_PATH = "/etc/postgresql-common/createcluster.d/pgcharm.conf" @@ -371,38 +379,72 @@ def test_enable_disable_extensions(harness, caplog): assert isinstance(harness.charm.unit.status, ActiveStatus) -def test_on_start(harness): +def test_on_start_no_password(harness): + """Test start is deferred when passwords are not yet generated.""" + with ( + patch("charm.PostgresqlOperatorCharm._check_detached_storage"), + patch("charm.PostgresqlOperatorCharm._get_password") as _get_password, + ): + _get_password.return_value = None + harness.charm.on.start.emit() + assert isinstance(harness.model.unit.status, WaitingStatus) + + # ModelError when fetching the password has the same outcome. + _get_password.side_effect = ModelError + harness.charm.on.start.emit() + assert isinstance(harness.model.unit.status, WaitingStatus) + + +def test_on_start_bootstrap_failure(harness): + """Test start is blocked when cluster bootstrap fails.""" with ( patch( "charm.PostgresqlOperatorCharm._restart_services_after_reboot" ) as _restart_services_after_reboot, patch( - "charm.PostgresqlOperatorCharm._set_primary_status_message" - ) as _set_primary_status_message, - patch( - "charm.PostgresqlOperatorCharm.enable_disable_extensions" - ) as _enable_disable_extensions, - patch("charm.snap.SnapCache") as _snap_cache, - patch("charm.Patroni.get_postgresql_version") as _get_postgresql_version, - patch("charm.PostgreSQLProvider.oversee_users") as _oversee_users, - patch( - "charm.PostgresqlOperatorCharm._update_relation_endpoints", new_callable=PropertyMock - ) as _update_relation_endpoints, - patch("charm.PostgresqlOperatorCharm.postgresql") as _postgresql, - patch("charm.PostgreSQLProvider.update_endpoints"), + "charm.PostgresqlOperatorCharm._replication_password", + new_callable=PropertyMock, + ) as _replication_password, + patch("charm.PostgresqlOperatorCharm._get_password") as _get_password, + patch("charm.PostgresqlOperatorCharm._ensure_storage_layout"), + patch("charm.PostgresqlOperatorCharm._check_detached_storage"), + patch("charm.PostgresqlOperatorCharm.get_secret"), + patch("charm.TLS.generate_internal_peer_cert"), + patch("charm.Patroni.bootstrap_cluster") as _bootstrap_cluster, patch("charm.PostgresqlOperatorCharm.update_config"), + ): + _get_password.return_value = "fake-operator-password" + _replication_password.return_value = "fake-replication-password" + _bootstrap_cluster.return_value = False + + # TODO: test replicas start (DPE-494). + harness.set_leader() + harness.charm.on.start.emit() + _bootstrap_cluster.assert_called_once() + _restart_services_after_reboot.assert_called_once() + assert isinstance(harness.model.unit.status, BlockedStatus) + + +def test_on_start_create_user_error(harness): + """Test start is blocked when creating the default postgres user fails.""" + with ( patch( - "charm.Patroni.member_started", + "charm.PostgresqlOperatorCharm._restart_services_after_reboot" + ) as _restart_services_after_reboot, + patch( + "charm.PostgresqlOperatorCharm._replication_password", new_callable=PropertyMock, - ) as _member_started, - patch("charm.Patroni.bootstrap_cluster") as _bootstrap_cluster, - patch("charm.PostgresqlOperatorCharm._replication_password") as _replication_password, + ) as _replication_password, patch("charm.PostgresqlOperatorCharm._get_password") as _get_password, + patch("charm.PostgresqlOperatorCharm._ensure_storage_layout"), patch("charm.PostgresqlOperatorCharm._check_detached_storage"), + patch("charm.PostgresqlOperatorCharm.get_secret"), + patch("charm.TLS.generate_internal_peer_cert"), + patch("charm.Patroni.bootstrap_cluster") as _bootstrap_cluster, patch( - "charm.PostgresqlOperatorCharm._is_storage_attached", - side_effect=[False, True, True, True, True, True], - ), + "charm.Patroni.member_started", + new_callable=PropertyMock, + ) as _member_started, patch( "charm.PostgresqlOperatorCharm._can_connect_to_postgresql", new_callable=PropertyMock, @@ -413,61 +455,73 @@ def test_on_start(harness): new_callable=PropertyMock, return_value=True, ), - patch("charm.PostgresqlOperatorCharm.get_secret"), - patch("charm.TLS.generate_internal_peer_cert"), + patch("charm.PostgresqlOperatorCharm.postgresql") as _postgresql, + patch("charm.PostgresqlOperatorCharm.update_config"), ): - _get_postgresql_version.return_value = "16.6" - - # Test before the passwords are generated. - _member_started.return_value = False - _get_password.return_value = None - harness.charm.on.start.emit() - _bootstrap_cluster.assert_not_called() - assert isinstance(harness.model.unit.status, WaitingStatus) - - # ModelError in get password - _get_password.side_effect = ModelError - harness.charm.on.start.emit() - _bootstrap_cluster.assert_not_called() - assert isinstance(harness.model.unit.status, WaitingStatus) - - # Mock the passwords. - _get_password.side_effect = None _get_password.return_value = "fake-operator-password" _replication_password.return_value = "fake-replication-password" + _bootstrap_cluster.return_value = True + _member_started.return_value = True + _postgresql.list_users.return_value = [] + _postgresql.create_user.side_effect = PostgreSQLCreateUserError - # Mock cluster start and postgres user creation success values. - _bootstrap_cluster.side_effect = [False, True, True] - _postgresql.list_users.side_effect = [[], [], []] - _postgresql.create_user.side_effect = [PostgreSQLCreateUserError, None, None, None] - - # Test for a failed cluster bootstrapping. - # TODO: test replicas start (DPE-494). harness.set_leader() harness.charm.on.start.emit() - _bootstrap_cluster.assert_called_once() - _oversee_users.assert_not_called() - _restart_services_after_reboot.assert_called_once() - assert isinstance(harness.model.unit.status, BlockedStatus) - # Set an initial waiting status (like after the install hook was triggered). - harness.model.unit.status = WaitingStatus("fake message") - - # Test the event of an error happening when trying to create the default postgres user. - _restart_services_after_reboot.reset_mock() - _member_started.return_value = True - harness.charm.on.start.emit() _postgresql.create_user.assert_called_once() - _oversee_users.assert_not_called() _restart_services_after_reboot.assert_called_once() assert isinstance(harness.model.unit.status, BlockedStatus) - # Set an initial waiting status again (like after the install hook was triggered). - harness.model.unit.status = WaitingStatus("fake message") - # Then test the event of a correct cluster bootstrapping. - _restart_services_after_reboot.reset_mock() +def test_on_start_success(harness): + """Test successful cluster bootstrapping on the primary unit.""" + with ( + patch( + "charm.PostgresqlOperatorCharm._restart_services_after_reboot" + ) as _restart_services_after_reboot, + patch( + "charm.PostgresqlOperatorCharm._set_primary_status_message" + ) as _set_primary_status_message, + patch( + "charm.PostgresqlOperatorCharm.enable_disable_extensions" + ) as _enable_disable_extensions, + patch("charm.PostgresqlOperatorCharm._update_relation_endpoints"), + patch("charm.PostgreSQLProvider.oversee_users") as _oversee_users, + patch( + "charm.PostgresqlOperatorCharm._replication_password", + new_callable=PropertyMock, + ) as _replication_password, + patch("charm.PostgresqlOperatorCharm._get_password") as _get_password, + patch("charm.PostgresqlOperatorCharm._ensure_storage_layout"), + patch("charm.PostgresqlOperatorCharm._check_detached_storage"), + patch("charm.PostgresqlOperatorCharm.get_secret"), + patch("charm.TLS.generate_internal_peer_cert"), + patch("charm.Patroni.bootstrap_cluster") as _bootstrap_cluster, + patch( + "charm.Patroni.member_started", + new_callable=PropertyMock, + ) as _member_started, + patch( + "charm.PostgresqlOperatorCharm._can_connect_to_postgresql", + new_callable=PropertyMock, + return_value=True, + ), + patch( + "charm.PostgresqlOperatorCharm.primary_endpoint", + new_callable=PropertyMock, + return_value=True, + ), + patch("charm.PostgresqlOperatorCharm.postgresql") as _postgresql, + patch("charm.PostgresqlOperatorCharm.update_config"), + ): + _get_password.return_value = "fake-operator-password" + _replication_password.return_value = "fake-replication-password" + _bootstrap_cluster.return_value = True + _member_started.return_value = True + _postgresql.list_users.return_value = [] + + harness.set_leader() harness.charm.on.start.emit() - assert _postgresql.create_user.call_count == 3 # Considering the previous failed call. + assert _postgresql.create_user.call_count == 2 # backup user + monitoring user _oversee_users.assert_called_once() _enable_disable_extensions.assert_called_once() _set_primary_status_message.assert_called_once() @@ -492,6 +546,7 @@ def test_on_start_replica(harness): patch.object(EventBase, "defer") as _defer, patch("charm.PostgresqlOperatorCharm._replication_password") as _replication_password, patch("charm.PostgresqlOperatorCharm._get_password") as _get_password, + patch("charm.PostgresqlOperatorCharm._ensure_storage_layout"), patch( "charm.PostgresqlOperatorCharm._is_storage_attached", return_value=True, @@ -546,6 +601,7 @@ def test_on_start_no_patroni_member(harness): patch("charm.PostgresqlOperatorCharm.postgresql") as _postgresql, patch("charm.Patroni") as patroni, patch("charm.PostgresqlOperatorCharm._get_password") as _get_password, + patch("charm.PostgresqlOperatorCharm._ensure_storage_layout"), patch( "charm.PostgresqlOperatorCharm._is_storage_attached", return_value=True ) as _is_storage_attached, @@ -593,12 +649,254 @@ def test_on_start_after_blocked_state(harness): assert harness.model.unit.status == initial_status +def test_ensure_storage_layout(harness, tmp_path): + data_root = tmp_path / "postgresql" + archive_root = tmp_path / "archive" + logs_root = tmp_path / "logs" + temp_root = tmp_path / "temp" + + data_root.mkdir() + archive_root.mkdir() + logs_root.mkdir() + temp_root.mkdir() + + (data_root / "data.txt").write_text("data") + (archive_root / "archive.txt").write_text("archive") + (logs_root / "logs.txt").write_text("logs") + (temp_root / "temp.txt").write_text("temp") + (logs_root / "lost+found").mkdir() + + with ( + patch("charm.POSTGRESQL_DATA_PATH", str(data_root)), + patch("charm.POSTGRESQL_DATA_DIR", str(data_root / "16" / "main")), + patch("charm.ARCHIVE_STORAGE_PATH", str(archive_root)), + patch("charm.ARCHIVE_DATA_DIR", str(archive_root / "16" / "main")), + patch("charm.LOGS_STORAGE_PATH", str(logs_root)), + patch("charm.LOGS_DATA_DIR", str(logs_root / "16" / "main")), + patch("charm.TEMP_STORAGE_PATH", str(temp_root)), + patch("charm.TEMP_DATA_DIR", str(temp_root / "16" / "main")), + patch("charm._change_owner") as _change_owner, + patch("charm.os.chmod") as _chmod, + patch("charm.os.lchown") as _lchown, + patch("charm.pwd.getpwnam"), + ): + harness.charm._ensure_storage_layout() + + assert (data_root / "16" / "main" / "data.txt").read_text() == "data" + assert (archive_root / "16" / "main" / "archive.txt").read_text() == "archive" + assert (logs_root / "16" / "main" / "logs.txt").read_text() == "logs" + assert (temp_root / "16" / "main" / "temp.txt").read_text() == "temp" + assert not (data_root / "data.txt").exists() + assert (logs_root / "lost+found").exists() + assert harness.charm.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] == "True" + # 4 calls from _migrate_storage_mount (target dirs) + 4 from _reconcile_storage_permissions (roots) + assert _change_owner.call_count == 4 + assert _chmod.call_count == 8 + _chmod.assert_any_call(str(data_root), 0o755) + _chmod.assert_any_call(str(archive_root), 0o755) + _chmod.assert_any_call(str(logs_root), 0o755) + _chmod.assert_any_call(str(temp_root), 0o755) + # No pg_wal symlink in this test + _lchown.assert_not_called() + + (data_root / "stray.txt").write_text("stray") + _change_owner.reset_mock() + _chmod.reset_mock() + _lchown.reset_mock() + + harness.charm._ensure_storage_layout() + + assert (data_root / "stray.txt").read_text() == "stray" + # _migrate_storage_mount skipped (already migrated), but reconciliation always runs + assert not _change_owner.called + assert _chmod.call_count == 4 + _chmod.assert_any_call(str(data_root), 0o755) + _lchown.assert_not_called() + + +def test_ensure_storage_layout_repairs_stale_pg_wal_symlink(harness, tmp_path): + data_root = tmp_path / "postgresql" + archive_root = tmp_path / "archive" + logs_root = tmp_path / "logs" + temp_root = tmp_path / "temp" + + for path in ( + data_root / "16" / "main", + archive_root / "16" / "main", + logs_root / "16" / "main", + temp_root / "16" / "main", + ): + path.mkdir(parents=True) + + stale_pg_wal = data_root / "16" / "main" / "pg_wal" + stale_pg_wal.symlink_to(logs_root) + (logs_root / "000000010000000000000008").write_text("wal") + (logs_root / "archive_status").mkdir() + harness.charm.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] = "True" + + with ( + patch("charm.POSTGRESQL_DATA_PATH", str(data_root)), + patch("charm.POSTGRESQL_DATA_DIR", str(data_root / "16" / "main")), + patch("charm.ARCHIVE_STORAGE_PATH", str(archive_root)), + patch("charm.ARCHIVE_DATA_DIR", str(archive_root / "16" / "main")), + patch("charm.LOGS_STORAGE_PATH", str(logs_root)), + patch("charm.LOGS_DATA_DIR", str(logs_root / "16" / "main")), + patch("charm.TEMP_STORAGE_PATH", str(temp_root)), + patch("charm.TEMP_DATA_DIR", str(temp_root / "16" / "main")), + patch("charm._change_owner") as _change_owner, + patch("charm.os.chmod") as _chmod, + patch("charm.os.lchown") as _lchown, + patch("charm.pwd.getpwnam") as _getpwnam, + ): + harness.charm._ensure_storage_layout() + + assert os.readlink(stale_pg_wal) == str(logs_root / "16" / "main") + assert (logs_root / "16" / "main" / "000000010000000000000008").read_text() == "wal" + assert not (logs_root / "000000010000000000000008").exists() + assert (logs_root / "16" / "main" / "archive_status").exists() + # _migrate_storage_mount for logs (called by _repair_pg_wal_symlink) + _change_owner.assert_called_once_with(str(logs_root / "16" / "main")) + _chmod.assert_any_call(str(logs_root / "16" / "main"), 0o700) + # _reconcile_storage_permissions: chmod on all 4 storage roots + _chmod.assert_any_call(str(data_root), 0o755) + _chmod.assert_any_call(str(logs_root), 0o755) + assert _chmod.call_count == 5 # 1 target (logs/16/main) + 4 storage roots + # pg_wal symlink ownership reconciled + _lchown.assert_called_once_with( + str(stale_pg_wal), _getpwnam.return_value.pw_uid, _getpwnam.return_value.pw_gid + ) + _getpwnam.assert_called_with("_daemon_") + + +def test_reconcile_storage_permissions_heals_already_migrated_units(harness, tmp_path): + """Reconciliation runs even when already migrated, fixing ownership/perms from before the fix.""" + data_root = tmp_path / "postgresql" + archive_root = tmp_path / "archive" + logs_root = tmp_path / "logs" + temp_root = tmp_path / "temp" + + for path in ( + data_root / "16" / "main", + archive_root / "16" / "main", + logs_root / "16" / "main", + temp_root / "16" / "main", + ): + path.mkdir(parents=True) + + # Simulate pg_wal symlink with wrong root:root ownership (the bug) + pg_wal = data_root / "16" / "main" / "pg_wal" + pg_wal.symlink_to(logs_root / "16" / "main") + + # Unit was already migrated before this fix was applied + harness.charm.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] = "True" + + with ( + patch("charm.POSTGRESQL_DATA_PATH", str(data_root)), + patch("charm.POSTGRESQL_DATA_DIR", str(data_root / "16" / "main")), + patch("charm.ARCHIVE_STORAGE_PATH", str(archive_root)), + patch("charm.ARCHIVE_DATA_DIR", str(archive_root / "16" / "main")), + patch("charm.LOGS_STORAGE_PATH", str(logs_root)), + patch("charm.LOGS_DATA_DIR", str(logs_root / "16" / "main")), + patch("charm.TEMP_STORAGE_PATH", str(temp_root)), + patch("charm.TEMP_DATA_DIR", str(temp_root / "16" / "main")), + patch("charm._change_owner"), + patch("charm.os.chmod") as _chmod, + patch("charm.os.lchown") as _lchown, + patch("charm.pwd.getpwnam") as _getpwnam, + ): + harness.charm._ensure_storage_layout() + + # Storage root permissions reconciled (0755) even though migration was already done + _chmod.assert_any_call(str(data_root), 0o755) + _chmod.assert_any_call(str(archive_root), 0o755) + _chmod.assert_any_call(str(logs_root), 0o755) + _chmod.assert_any_call(str(temp_root), 0o755) + assert _chmod.call_count == 4 + + # pg_wal symlink ownership reconciled (_daemon_) + _lchown.assert_called_once_with( + str(pg_wal), _getpwnam.return_value.pw_uid, _getpwnam.return_value.pw_gid + ) + _getpwnam.assert_called_with("_daemon_") + + connection = MagicMock() + cursor = MagicMock() + cursor.fetchone.return_value = (TEMP_STORAGE_PATH,) + connection.cursor.return_value = cursor + postgresql = MagicMock() + postgresql._connect_to_database.return_value = connection + + with patch( + "charm.PostgresqlOperatorCharm.postgresql", + new_callable=PropertyMock, + return_value=postgresql, + ): + assert harness.charm._ensure_temp_tablespace_location() + + cursor.execute.assert_has_calls([ + call("SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';"), + call("DROP TABLESPACE temp;"), + call(f"CREATE TABLESPACE temp LOCATION '{TEMP_DATA_DIR}';"), + call("GRANT CREATE ON TABLESPACE temp TO public;"), + ]) + + +def test_ensure_temp_tablespace_location_recovers_dropped_tablespace(harness, tmp_path): + """If the tablespace was previously dropped but TEMP_DATA_DIR exists, recreate it.""" + temp_data_dir = tmp_path / "temp" / "16" / "main" + temp_data_dir.mkdir(parents=True) + stale_pg_dir = temp_data_dir / "PG_16_202307071" + stale_pg_dir.mkdir() + + connection = MagicMock() + cursor = MagicMock() + cursor.fetchone.return_value = None # tablespace was previously dropped + connection.cursor.return_value = cursor + postgresql = MagicMock() + postgresql._connect_to_database.return_value = connection + + with ( + patch( + "charm.PostgresqlOperatorCharm.postgresql", + new_callable=PropertyMock, + return_value=postgresql, + ), + patch("charm.TEMP_DATA_DIR", str(temp_data_dir)), + ): + assert harness.charm._ensure_temp_tablespace_location() + + # Stale PG version directory should have been removed + assert not stale_pg_dir.exists() + cursor.execute.assert_has_calls([ + call("SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';"), + call(f"CREATE TABLESPACE temp LOCATION '{temp_data_dir}';"), + call("GRANT CREATE ON TABLESPACE temp TO public;"), + ]) + + +def test_post_snap_refresh_storage_layout_failure(harness): + refresh = MagicMock(unit_status_higher_priority=None) + with ( + patch("charm.PostgresqlOperatorCharm._ensure_storage_layout", side_effect=OSError), + patch("charm.PostgresqlOperatorCharm.get_secret", return_value=None), + patch("charm.Patroni.start_patroni") as _start_patroni, + ): + harness.charm._post_snap_refresh(refresh) + + _start_patroni.assert_not_called() + assert isinstance(harness.charm.unit.status, BlockedStatus) + assert harness.charm.unit.status.message == "Failed to migrate PostgreSQL storage layout" + + def test_on_update_status(harness): with ( patch("charm.ClusterTopologyObserver.start_observer") as _start_observer, patch( "charm.PostgresqlOperatorCharm._set_primary_status_message" ) as _set_primary_status_message, + patch( + "charm.PostgresqlOperatorCharm._reconfigure_cluster", return_value=True + ) as _reconfigure_cluster, patch("charm.Patroni.restart_patroni") as _restart_patroni, patch("charm.Patroni.is_member_isolated") as _is_member_isolated, patch("charm.Patroni.member_started", new_callable=PropertyMock) as _member_started, @@ -625,11 +923,15 @@ def test_on_update_status(harness): patch("charm.PostgresqlOperatorCharm.update_config") as _update_config, patch("charm.PostgresqlOperatorCharm.log_pitr_last_transaction_time"), patch("charm.PostgreSQL.drop_hba_triggers") as _drop_hba_triggers, + patch( + "charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location", return_value=True + ) as _ensure_temp_tablespace_location, ): rel_id = harness.model.get_relation(PEER).id # Test before the cluster is initialised. harness.charm.on.update_status.emit() _set_primary_status_message.assert_not_called() + _reconfigure_cluster.assert_not_called() # Test after the cluster was initialised, but with the unit in a blocked state. with harness.hooks_disabled(): @@ -640,6 +942,7 @@ def test_on_update_status(harness): harness.charm.unit.status = BlockedStatus("fake blocked status") harness.charm.on.update_status.emit() _set_primary_status_message.assert_not_called() + _reconfigure_cluster.assert_not_called() # Test the point-in-time-recovery fail. with harness.hooks_disabled(): @@ -656,6 +959,7 @@ def test_on_update_status(harness): _patroni_logs.return_value = "2022-02-24 02:00:00 UTC patroni.exceptions.PatroniFatalException: Failed to bootstrap cluster" harness.charm.on.update_status.emit() _set_primary_status_message.assert_not_called() + _reconfigure_cluster.assert_not_called() assert harness.charm.unit.status.message == CANNOT_RESTORE_PITR # Test with the unit in a status different that blocked. @@ -666,12 +970,16 @@ def test_on_update_status(harness): {"cluster_initialised": "True", "restoring-backup": "", "restore-to-time": ""}, ) harness.charm.unit.status = ActiveStatus() + _ensure_temp_tablespace_location.reset_mock() harness.charm.on.update_status.emit() _set_primary_status_message.assert_called_once() + _reconfigure_cluster.assert_called_once() + _ensure_temp_tablespace_location.assert_called_once() # Test call to restart when the member is isolated from the cluster. _set_primary_status_message.reset_mock() _start_observer.reset_mock() + _reconfigure_cluster.reset_mock() _member_started.return_value = False _is_member_isolated.return_value = True with harness.hooks_disabled(): @@ -679,17 +987,63 @@ def test_on_update_status(harness): rel_id, harness.charm.unit.name, {"postgresql_restarted": ""} ) harness.charm.on.update_status.emit() + _reconfigure_cluster.assert_called_once() _restart_patroni.assert_called_once() _start_observer.assert_called_once_with() _drop_hba_triggers.assert_called_once_with() +def test_on_update_status_skips_remainder_when_reconfigure_cluster_fails(harness): + with ( + patch("charm.PostgresqlOperatorCharm._can_run_on_update_status", return_value=True), + patch("charm.PostgresqlOperatorCharm._handle_processes_failures", return_value=False), + patch( + "charm.PostgresqlOperatorCharm._reconfigure_cluster", return_value=False + ) as _reconfigure_cluster, + patch("charm.PostgreSQLProvider.oversee_users") as _oversee_users, + ): + with harness.hooks_disabled(): + harness.set_leader() + + harness.charm.on.update_status.emit() + + _reconfigure_cluster.assert_called_once() + _oversee_users.assert_not_called() + + +def test_on_update_status_skips_remainder_when_temp_tablespace_migration_fails(harness): + with ( + patch("charm.PostgresqlOperatorCharm._can_run_on_update_status", return_value=True), + patch("charm.PostgresqlOperatorCharm._handle_processes_failures", return_value=False), + patch("charm.PostgresqlOperatorCharm._reconfigure_cluster", return_value=True), + patch( + "charm.PostgresqlOperatorCharm.is_primary", + new_callable=PropertyMock, + return_value=True, + ), + patch( + "charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location", return_value=False + ) as _ensure_temp_tablespace_location, + patch("charm.PostgreSQLProvider.oversee_users") as _oversee_users, + ): + with harness.hooks_disabled(): + harness.set_leader() + + harness.charm.on.update_status.emit() + + _ensure_temp_tablespace_location.assert_called_once() + _oversee_users.assert_not_called() + + def test_on_update_status_after_restore_operation(harness): with ( patch("charm.ClusterTopologyObserver.start_observer"), patch( "charm.PostgresqlOperatorCharm._set_primary_status_message" ) as _set_primary_status_message, + patch( + "charm.PostgresqlOperatorCharm._reconfigure_cluster", return_value=True + ) as _reconfigure_cluster, patch( "charm.PostgresqlOperatorCharm._update_relation_endpoints" ) as _update_relation_endpoints, @@ -713,6 +1067,7 @@ def test_on_update_status_after_restore_operation(harness): "charm.PostgresqlOperatorCharm.enable_disable_extensions" ) as _enable_disable_extensions, patch("charm.PostgreSQL.drop_hba_triggers") as _drop_hba_triggers, + patch("charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location", return_value=True), ): _get_current_timeline.return_value = "2" rel_id = harness.model.get_relation(PEER).id @@ -736,6 +1091,7 @@ def test_on_update_status_after_restore_operation(harness): _update_relation_endpoints.assert_not_called() _set_primary_status_message.assert_not_called() _enable_disable_extensions.assert_not_called() + _reconfigure_cluster.assert_not_called() assert isinstance(harness.charm.unit.status, BlockedStatus) # Test when the restore operation hasn't finished yet. @@ -749,6 +1105,7 @@ def test_on_update_status_after_restore_operation(harness): _update_relation_endpoints.assert_not_called() _set_primary_status_message.assert_not_called() _enable_disable_extensions.assert_not_called() + _reconfigure_cluster.assert_not_called() assert isinstance(harness.charm.unit.status, ActiveStatus) # Assert that the backup id is still in the application relation databag. @@ -765,6 +1122,7 @@ def test_on_update_status_after_restore_operation(harness): harness.charm.on.update_status.emit() _update_config.assert_called_once() _handle_processes_failures.assert_called_once() + _reconfigure_cluster.assert_called_once() _oversee_users.assert_called_once() _update_relation_endpoints.assert_called_once() _set_primary_status_message.assert_called_once() @@ -1779,6 +2137,10 @@ def test_on_peer_relation_changed(harness): patch("charm.PostgresqlOperatorCharm.is_primary") as _is_primary, patch("charm.Patroni.member_started", new_callable=PropertyMock) as _member_started, patch("charm.Patroni.start_patroni") as _start_patroni, + patch( + "charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location_if_primary", + return_value=True, + ) as _ensure_temp_tablespace_location_if_primary, patch("charm.PostgresqlOperatorCharm.update_config") as _update_config, patch("charm.PostgresqlOperatorCharm._update_member_ip") as _update_member_ip, patch("charm.PostgresqlOperatorCharm._reconfigure_cluster") as _reconfigure_cluster, @@ -1823,6 +2185,7 @@ def test_on_peer_relation_changed(harness): _reconfigure_cluster.assert_called_once_with(mock_event) _update_config.assert_called_once() _start_patroni.assert_called_once() + _ensure_temp_tablespace_location_if_primary.assert_called_once() _update_new_unit_status.assert_called_once() # Test when the unit fails to update the Patroni configuration. @@ -3055,8 +3418,8 @@ def test_handle_processes_failures(harness): assert harness.charm._handle_processes_failures() assert not _restart_patroni.called _rename.assert_called_once_with( - os.path.join(POSTGRESQL_DATA_PATH, "pg_wal"), - os.path.join(POSTGRESQL_DATA_PATH, f"pg_wal-{_now.isoformat()}"), + os.path.join(POSTGRESQL_DATA_DIR, "pg_wal"), + os.path.join(POSTGRESQL_DATA_DIR, f"pg_wal-{_now.isoformat()}"), ) _rename.reset_mock() diff --git a/tests/unit/test_cluster.py b/tests/unit/test_cluster.py index 970213713e..8570e4ad47 100644 --- a/tests/unit/test_cluster.py +++ b/tests/unit/test_cluster.py @@ -26,9 +26,10 @@ SwitchoverNotSyncError, ) from constants import ( + LOGS_DATA_DIR, PATRONI_CONF_PATH, PATRONI_LOGS_PATH, - POSTGRESQL_DATA_PATH, + POSTGRESQL_DATA_DIR, POSTGRESQL_LOGS_PATH, ) @@ -290,9 +291,10 @@ def test_render_patroni_yml_file(peers_ips, patroni): expected_content = template.render( conf_path=PATRONI_CONF_PATH, - data_path=POSTGRESQL_DATA_PATH, + data_path=POSTGRESQL_DATA_DIR, log_path=PATRONI_LOGS_PATH, postgresql_log_path=POSTGRESQL_LOGS_PATH, + wal_dir=LOGS_DATA_DIR, member_name=member_name, partner_addrs=["2.2.2.2", "3.3.3.3"], peers_ips=sorted(peers_ips), @@ -463,6 +465,7 @@ def test_set_max_timelines_history(peers_ips, patroni): def test_configure_patroni_on_unit(peers_ips, patroni): with ( + patch("os.makedirs") as _makedirs, patch("os.chmod") as _chmod, patch("builtins.open") as _open, patch("os.chown") as _chown, @@ -474,17 +477,16 @@ def test_configure_patroni_on_unit(peers_ips, patroni): patroni.configure_patroni_on_unit() _getpwnam.assert_called_once_with("_daemon_") + _makedirs.assert_called_once_with(POSTGRESQL_DATA_DIR, exist_ok=True) _chown.assert_any_call( - "/var/snap/charmed-postgresql/common/var/lib/postgresql", + POSTGRESQL_DATA_DIR, uid=sentinel.uid, gid=sentinel.gid, ) _open.assert_called_once_with(CREATE_CLUSTER_CONF_PATH, "a") - _chmod.assert_called_once_with( - "/var/snap/charmed-postgresql/common/var/lib/postgresql", 448 - ) + _chmod.assert_called_once_with(POSTGRESQL_DATA_DIR, 448) def test_member_started_true(peers_ips, patroni): From 73bbade498562aa04ec762d03ae7a299a3604cce Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Fri, 24 Apr 2026 19:17:03 -0300 Subject: [PATCH 02/19] fix: reinitialise temp tablespace after tmpfs wipe on reboot After the PR introduced versioned storage paths (16/main), three bugs prevented temp tablespaces from recovering after a reboot: 1. TEMP_DATA_DIR was not recreated on tmpfs after the migration flag was already set. Add an else-branch in _ensure_storage_layout that calls mkdir(parents=True, exist_ok=True) without setting owner/mode. The resulting root-owned directory triggers set_up_database's permission check, which calls _handle_temp_tablespace_on_reboot to reinitialise the tablespace directory structure for the tmpfs case. 2. As a belt-and-suspenders fallback, _ensure_temp_tablespace_location now detects an empty TEMP_DATA_DIR (no PG__/ subdir) and drops then recreates the tablespace so PostgreSQL rebuilds the internal directory structure. 3. _ensure_temp_tablespace_location_if_primary raised PostgreSQLUndefinedHostError when primary_endpoint was None during early startup. Return True early when the endpoint is not yet available. The persistent storage integration test assertion for the library's "Fixed permissions" log message is softened to a warning: that message only appears when permissions needed fixing, but units initialised by _migrate_storage_mount already have correct owner/mode so the fix path is not triggered. Signed-off-by: Marcelo Henrique Neppel --- src/charm.py | 26 ++++ tests/integration/conftest.py | 3 +- .../test_persistent_temp_storage_restart.py | 14 +- tests/unit/test_charm.py | 136 +++++++++++++++++- 4 files changed, 167 insertions(+), 12 deletions(-) diff --git a/src/charm.py b/src/charm.py index 9d8db797fc..7b31131db9 100755 --- a/src/charm.py +++ b/src/charm.py @@ -742,6 +742,14 @@ def _ensure_storage_layout(self) -> None: self._migrate_storage_mount(storage_path, target_path) self.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] = "True" + else: + # The temp data directory may live on a tmpfs mount that is wiped on + # reboot, even after the one-time migration flag has been recorded. + # Create it WITHOUT setting owner/mode: set_up_database detects the + # root-owned directory as needing a permissions fix, which triggers its + # _handle_temp_tablespace_on_reboot path for the tmpfs case and ensures + # the tablespace directory structure is reinitialised by PostgreSQL. + Path(TEMP_DATA_DIR).mkdir(parents=True, exist_ok=True) self._repair_pg_wal_symlink() self._reconcile_storage_permissions() @@ -815,6 +823,20 @@ def _ensure_temp_tablespace_location(self) -> bool: current_location = row[0] if current_location == TEMP_DATA_DIR: + # After a tmpfs wipe TEMP_DATA_DIR is recreated empty. PostgreSQL will + # not be able to use the tablespace until its internal PG__/ + # directory structure has been recreated. Detect this and reinitialise + # by dropping and recreating the tablespace. + pg_data_dir = Path(TEMP_DATA_DIR) + if pg_data_dir.exists() and not any( + d.is_dir() and d.name.startswith("PG_") for d in pg_data_dir.iterdir() + ): + logger.info( + "Temp tablespace directory is empty after tmpfs wipe; reinitialising" + ) + cursor.execute("DROP TABLESPACE temp;") + cursor.execute(f"CREATE TABLESPACE temp LOCATION '{TEMP_DATA_DIR}';") + cursor.execute("GRANT CREATE ON TABLESPACE temp TO public;") return True if current_location != TEMP_STORAGE_PATH: @@ -852,6 +874,10 @@ def _ensure_temp_tablespace_location_if_primary(self) -> bool: if not self.is_primary: return True + if not self.primary_endpoint: + logger.debug("Primary endpoint not yet available; skipping temp tablespace check") + return True + return self._ensure_temp_tablespace_location() @cached_property diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index e0b532943e..dae48b7653 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -121,12 +121,13 @@ def juju(request: pytest.FixtureRequest): """ model = request.config.getoption("--model") keep_models = bool(request.config.getoption("--keep-models")) + controller = request.config.getoption("--controller") if model: juju = jubilant.Juju(model=model) yield juju else: - with jubilant.temp_model(keep=keep_models) as juju: + with jubilant.temp_model(keep=keep_models, controller=controller) as juju: yield juju diff --git a/tests/integration/test_persistent_temp_storage_restart.py b/tests/integration/test_persistent_temp_storage_restart.py index c0f43caf16..78d7eb3350 100644 --- a/tests/integration/test_persistent_temp_storage_restart.py +++ b/tests/integration/test_persistent_temp_storage_restart.py @@ -124,10 +124,16 @@ def test_leader_change_and_restart(juju: jubilant.Juju) -> None: verify_leader_active(status, new_leader) logger.info(f"New leader {new_leader} is active after restart") - # Check for the log message that confirms the fix is working - assert check_for_fix_log_message(juju, new_leader), ( - "Expected library fix log message not found in unit logs" - ) + # Check for the fix log message - this only appears when permissions needed fixing + # (i.e. wrong owner/mode on TEMP_DATA_DIR). For units correctly initialised by + # _migrate_storage_mount, permissions are already right so no fix is needed and + # the message won't appear. Log a warning rather than failing the test. + if not check_for_fix_log_message(juju, new_leader): + logger.warning( + "Library fix log message not found for %s - permissions were already correct " + "(expected for units initialised with the current charm)", + new_leader, + ) # Test temporary table creation logger.info("Testing temporary table creation on database") diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index a73aa6a297..d559205132 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -707,9 +707,10 @@ def test_ensure_storage_layout(harness, tmp_path): harness.charm._ensure_storage_layout() assert (data_root / "stray.txt").read_text() == "stray" - # _migrate_storage_mount skipped (already migrated), but reconciliation always runs - assert not _change_owner.called - assert _chmod.call_count == 4 + # Already migrated: only TEMP_DATA_DIR is re-created (tmpfs reboot guard) + reconciliation + # mkdir only — no _change_owner or chmod on TEMP_DATA_DIR (library detects root-owned dir) + _change_owner.assert_not_called() + assert _chmod.call_count == 4 # only the 4 storage root chmod calls _chmod.assert_any_call(str(data_root), 0o755) _lchown.assert_not_called() @@ -754,13 +755,14 @@ def test_ensure_storage_layout_repairs_stale_pg_wal_symlink(harness, tmp_path): assert (logs_root / "16" / "main" / "000000010000000000000008").read_text() == "wal" assert not (logs_root / "000000010000000000000008").exists() assert (logs_root / "16" / "main" / "archive_status").exists() - # _migrate_storage_mount for logs (called by _repair_pg_wal_symlink) + # TEMP_DATA_DIR re-created (mkdir only) + _migrate_storage_mount for logs + assert _change_owner.call_count == 1 _change_owner.assert_called_once_with(str(logs_root / "16" / "main")) _chmod.assert_any_call(str(logs_root / "16" / "main"), 0o700) # _reconcile_storage_permissions: chmod on all 4 storage roots _chmod.assert_any_call(str(data_root), 0o755) _chmod.assert_any_call(str(logs_root), 0o755) - assert _chmod.call_count == 5 # 1 target (logs/16/main) + 4 storage roots + assert _chmod.call_count == 5 # 1 target dir (logs) + 4 storage roots # pg_wal symlink ownership reconciled _lchown.assert_called_once_with( str(stale_pg_wal), _getpwnam.return_value.pw_uid, _getpwnam.return_value.pw_gid @@ -811,7 +813,7 @@ def test_reconcile_storage_permissions_heals_already_migrated_units(harness, tmp _chmod.assert_any_call(str(archive_root), 0o755) _chmod.assert_any_call(str(logs_root), 0o755) _chmod.assert_any_call(str(temp_root), 0o755) - assert _chmod.call_count == 4 + assert _chmod.call_count == 4 # only the 4 storage root chmod calls (no TEMP_DATA_DIR) # pg_wal symlink ownership reconciled (_daemon_) _lchown.assert_called_once_with( @@ -874,7 +876,122 @@ def test_ensure_temp_tablespace_location_recovers_dropped_tablespace(harness, tm ]) -def test_post_snap_refresh_storage_layout_failure(harness): +def test_ensure_temp_tablespace_location_reinitialises_after_tmpfs_wipe(harness, tmp_path): + """If tablespace dir is empty (tmpfs wipe), DROP+CREATE to reinitialise it.""" + temp_data_dir = tmp_path / "temp" / "16" / "main" + temp_data_dir.mkdir(parents=True) + # No PG_version/ directory — simulates an empty dir after tmpfs wipe + + connection = MagicMock() + cursor = MagicMock() + cursor.fetchone.return_value = (str(temp_data_dir),) # tablespace exists at correct location + connection.cursor.return_value = cursor + postgresql = MagicMock() + postgresql._connect_to_database.return_value = connection + + with ( + patch( + "charm.PostgresqlOperatorCharm.postgresql", + new_callable=PropertyMock, + return_value=postgresql, + ), + patch("charm.TEMP_DATA_DIR", str(temp_data_dir)), + ): + assert harness.charm._ensure_temp_tablespace_location() + + cursor.execute.assert_has_calls([ + call("SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';"), + call("DROP TABLESPACE temp;"), + call(f"CREATE TABLESPACE temp LOCATION '{temp_data_dir}';"), + call("GRANT CREATE ON TABLESPACE temp TO public;"), + ]) + + +def test_ensure_temp_tablespace_location_skips_reinit_when_pg_dir_present(harness, tmp_path): + """If PG_version/ already exists in tablespace dir, no DROP+CREATE is performed.""" + temp_data_dir = tmp_path / "temp" / "16" / "main" + temp_data_dir.mkdir(parents=True) + (temp_data_dir / "PG_16_202307071").mkdir() # directory already initialised + + connection = MagicMock() + cursor = MagicMock() + cursor.fetchone.return_value = (str(temp_data_dir),) + connection.cursor.return_value = cursor + postgresql = MagicMock() + postgresql._connect_to_database.return_value = connection + + with ( + patch( + "charm.PostgresqlOperatorCharm.postgresql", + new_callable=PropertyMock, + return_value=postgresql, + ), + patch("charm.TEMP_DATA_DIR", str(temp_data_dir)), + ): + assert harness.charm._ensure_temp_tablespace_location() + + # Only the SELECT should have been executed — no DROP/CREATE + cursor.execute.assert_called_once_with( + "SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';" + ) + + +def test_ensure_storage_layout_recreates_temp_dir_on_reboot(harness, tmp_path): + """TEMP_DATA_DIR is recreated even when already migrated (e.g. after a tmpfs wipe on reboot).""" + temp_root = tmp_path / "temp" + temp_root.mkdir() + # Other storage roots are pre-created with versioned subdirs (persistent storage) + for path in ( + tmp_path / "postgresql" / "16" / "main", + tmp_path / "archive" / "16" / "main", + tmp_path / "logs" / "16" / "main", + ): + path.mkdir(parents=True) + + # Mark as already migrated — temp_root/16/main does NOT exist (simulating tmpfs wipe) + harness.charm.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] = "True" + + with ( + patch("charm.POSTGRESQL_DATA_PATH", str(tmp_path / "postgresql")), + patch("charm.POSTGRESQL_DATA_DIR", str(tmp_path / "postgresql" / "16" / "main")), + patch("charm.ARCHIVE_STORAGE_PATH", str(tmp_path / "archive")), + patch("charm.ARCHIVE_DATA_DIR", str(tmp_path / "archive" / "16" / "main")), + patch("charm.LOGS_STORAGE_PATH", str(tmp_path / "logs")), + patch("charm.LOGS_DATA_DIR", str(tmp_path / "logs" / "16" / "main")), + patch("charm.TEMP_STORAGE_PATH", str(temp_root)), + patch("charm.TEMP_DATA_DIR", str(temp_root / "16" / "main")), + patch("charm._change_owner"), + patch("charm.os.chmod"), + patch("charm.os.lchown"), + patch("charm.pwd.getpwnam"), + ): + harness.charm._ensure_storage_layout() + + assert (temp_root / "16" / "main").is_dir() + + +def test_ensure_temp_tablespace_location_if_primary_skips_when_no_endpoint(harness): + """If primary_endpoint is not yet set, the tablespace check is skipped (not deferred).""" + with ( + patch( + "charm.PostgresqlOperatorCharm.is_primary", + new_callable=PropertyMock, + return_value=True, + ), + patch( + "charm.PostgresqlOperatorCharm.primary_endpoint", + new_callable=PropertyMock, + return_value=None, + ), + patch( + "charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location" + ) as _ensure_temp_tablespace_location, + ): + result = harness.charm._ensure_temp_tablespace_location_if_primary() + + assert result is True + _ensure_temp_tablespace_location.assert_not_called() + refresh = MagicMock(unit_status_higher_priority=None) with ( patch("charm.PostgresqlOperatorCharm._ensure_storage_layout", side_effect=OSError), @@ -1021,6 +1138,11 @@ def test_on_update_status_skips_remainder_when_temp_tablespace_migration_fails(h new_callable=PropertyMock, return_value=True, ), + patch( + "charm.PostgresqlOperatorCharm.primary_endpoint", + new_callable=PropertyMock, + return_value="10.0.0.1", + ), patch( "charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location", return_value=False ) as _ensure_temp_tablespace_location, From c4f496db6b71f5fd56cb5bb8ada369db8c12d66a Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Mon, 27 Apr 2026 15:33:21 -0300 Subject: [PATCH 03/19] fix(tests): assert versioned data_directory path in test_settings_are_correct After the versioned storage layout migration (PR #1649), PostgreSQL's data_directory is POSTGRESQL_DATA_DIR (.../postgresql/16/main) rather than the Juju storage mount point STORAGE_PATH (.../postgresql). Update the assertion accordingly. Signed-off-by: Marcelo Henrique Neppel --- tests/integration/test_charm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 0675131cc9..89607af9b6 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -10,6 +10,7 @@ import psycopg2 import pytest import requests +from constants import POSTGRESQL_DATA_DIR from locales import SNAP_LOCALES from psycopg2 import sql from pytest_operator.plugin import OpsTest @@ -19,7 +20,6 @@ from .helpers import ( CHARM_BASE, DATABASE_APP_NAME, - STORAGE_PATH, check_cluster_members, convert_records_to_dict, db_connect, @@ -133,7 +133,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"] == DATABASE_APP_NAME - assert settings["data_directory"] == STORAGE_PATH + assert settings["data_directory"] == POSTGRESQL_DATA_DIR assert settings["data_checksums"] == "on" assert settings["fsync"] == "on" assert settings["full_page_writes"] == "on" From a0b9c61f851b8bb506f97d2976feb2d3138531d9 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 28 Apr 2026 19:12:24 -0300 Subject: [PATCH 04/19] fix(storage): skip temp tablespace migration while async replication is active MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DROP/CREATE TABLESPACE during the temp tablespace location migration generates WAL that is streamed to the standby cluster. If the standby has not yet been upgraded to the versioned storage layout, it lacks the TEMP_DATA_DIR directory, causing PostgreSQL to crash with "FATAL: directory does not exist" during WAL replay — leaving all standby members stuck in "start failed" state. Defer the migration by returning early from _ensure_temp_tablespace_location_if_primary() when cross-cluster async replication is active. The _on_update_status and _on_peer_relation_changed hooks will retry it after the standby has been upgraded too. Signed-off-by: Marcelo Henrique Neppel --- src/charm.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/charm.py b/src/charm.py index 7b31131db9..3cd3672c89 100755 --- a/src/charm.py +++ b/src/charm.py @@ -878,6 +878,15 @@ def _ensure_temp_tablespace_location_if_primary(self) -> bool: logger.debug("Primary endpoint not yet available; skipping temp tablespace check") return True + # Do not migrate the temp tablespace while cross-cluster async replication is + # active. The DROP/CREATE TABLESPACE generates WAL that is streamed to the + # standby cluster. If the standby has not been upgraded to the versioned + # storage layout yet, it will not have the TEMP_DATA_DIR directory, causing + # PostgreSQL to crash with "FATAL: directory does not exist" during WAL replay. + if self.async_replication._relation is not None: + logger.debug("Skipping temp tablespace migration while async replication is active") + return True + return self._ensure_temp_tablespace_location() @cached_property From 8823c6ce0beaeeac912502622622dd3541d77f99 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 6 May 2026 15:11:20 -0300 Subject: [PATCH 05/19] feat(storage): perform forward migration in charm and add bidirectional snap daemon for safe rollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: The initial versioned storage layout (c2f8d9b0) was one-way only — migration ran in the snap post-refresh hook and had no rollback path. These changes re-architect it: the charm (unconfined) handles forward migration before the snap refresh, while a new migrate-data daemon in the snap handles both forward (belt-and-suspenders) and reverse migration by reading the Patroni YAML for direction detection. Enables clean rollback from versioned paths to root layout when reverting to 16/stable. Includes: simplified _ensure_storage_layout() in charm, _migrate_storage_roots_to_versioned(), removed obsoleted migration code/constants, bumped snap revisions to 310/311, fixed 16/ parent dir chown, updated unit tests, added IPv6 URL handling and increased timeouts in integration tests. Signed-off-by: Marcelo Henrique Neppel --- refresh_versions.toml | 8 +- src/charm.py | 169 ++++-------- src/constants.py | 1 - .../high_availability_helpers_new.py | 15 +- .../test_upgrade_skip_pre_upgrade_check.py | 25 +- tests/unit/test_charm.py | 260 ++---------------- 6 files changed, 114 insertions(+), 364 deletions(-) diff --git a/refresh_versions.toml b/refresh_versions.toml index c5b2393b53..220d41926f 100644 --- a/refresh_versions.toml +++ b/refresh_versions.toml @@ -6,6 +6,8 @@ name = "charmed-postgresql" [snap.revisions] # amd64 -x86_64 = "283" -# arm64 -aarch64 = "282" +x86_64 = "310" +# arm64 (Linux) +aarch64 = "311" +# arm64 (macOS / Apple Silicon - same snap revision) +arm64 = "311" diff --git a/src/charm.py b/src/charm.py index 3cd3672c89..b7d488f2c0 100755 --- a/src/charm.py +++ b/src/charm.py @@ -10,7 +10,6 @@ import os import pathlib import platform -import pwd import re import shutil import subprocess @@ -62,7 +61,6 @@ BACKUP_USER, MONITORING_USER, PEER, - POSTGRESQL_STORAGE_PERMISSIONS, REPLICATION_USER, REWIND_USER, SYSTEM_USERS, @@ -105,7 +103,6 @@ APP_SCOPE, ARCHIVE_DATA_DIR, ARCHIVE_STORAGE_PATH, - DATA_DIR_SUBFOLDER, DATABASE, DATABASE_DEFAULT_NAME, DATABASE_PORT, @@ -130,7 +127,6 @@ SECRET_INTERNAL_LABEL, SECRET_KEY_OVERRIDES, SPI_MODULE, - STORAGE_LAYOUT_MIGRATED_KEY, TEMP_DATA_DIR, TEMP_STORAGE_PATH, TLS_CA_BUNDLE_FILE, @@ -148,14 +144,13 @@ from relations.postgresql_provider import PostgreSQLProvider from relations.tls import TLS from rotate_logs import RotateLogs -from utils import _change_owner, label2name, new_password, render_file +from utils import label2name, new_password, render_file logger = logging.getLogger(__name__) logging.getLogger("httpx").setLevel(logging.WARNING) logging.getLogger("httpcore").setLevel(logging.WARNING) logging.getLogger("asyncio").setLevel(logging.WARNING) logging.getLogger("boto3").setLevel(logging.WARNING) -STORAGE_LAYOUT_IGNORED_ENTRIES = {DATA_DIR_SUBFOLDER.split("/", maxsplit=1)[0], "lost+found"} logging.getLogger("botocore").setLevel(logging.WARNING) PRIMARY_NOT_REACHABLE_MESSAGE = "waiting for primary to be reachable from this unit" @@ -256,6 +251,11 @@ def refresh_snap( self._charm.set_unit_status(MaintenanceStatus("updating configuration"), refresh=refresh) self._charm.update_config(refresh=refresh) + # Create versioned storage directories before the snap refresh. + # The snap's post-refresh hook runs confined and may be unable to + # create subdirectories inside daemon-owned storage roots. + self._charm._ensure_storage_layout() + # TODO add graceful shutdown before refreshing snap? # TODO future improvement: if snap refresh fails (i.e. same snap revision installed) after # graceful shutdown, restart workload @@ -437,15 +437,7 @@ def _post_snap_refresh(self, refresh: charm_refresh.Machines): Called after snap refresh """ self._check_and_update_internal_cert() - - try: - self._ensure_storage_layout() - except (OSError, RuntimeError): - logger.exception("Unable to migrate PostgreSQL storage layout") - self.set_unit_status( - ops.BlockedStatus("Failed to migrate PostgreSQL storage layout"), refresh=refresh - ) - return + self._ensure_storage_layout() if not self._patroni.start_patroni(): self.set_unit_status(ops.BlockedStatus("Failed to start PostgreSQL"), refresh=refresh) @@ -458,7 +450,7 @@ def _post_snap_refresh(self, refresh: charm_refresh.Machines): # Wait until the database initialise. self.set_unit_status(WaitingStatus("waiting for database initialisation"), refresh=refresh) try: - for attempt in Retrying(stop=stop_after_attempt(6), wait=wait_fixed(10)): + for attempt in Retrying(stop=stop_after_attempt(30), wait=wait_fixed(10)): with attempt: # Check if the member hasn't started or hasn't joined the cluster yet. if ( @@ -681,102 +673,60 @@ def is_unit_stopped(self) -> bool: """Returns whether the unit is stopped.""" return "stopped" in self.unit_peer_data - @property - def is_storage_layout_migrated(self) -> bool: - """Returns whether this unit already migrated to the versioned storage layout.""" - return self.unit_peer_data.get(STORAGE_LAYOUT_MIGRATED_KEY) == "True" - - def _migrate_storage_mount(self, storage_path: str, target_path: str) -> None: - """Move a storage mount root into the versioned PostgreSQL data layout.""" - Path(storage_path).mkdir(parents=True, exist_ok=True) - Path(target_path).mkdir(parents=True, exist_ok=True) - _change_owner(target_path) - os.chmod(target_path, POSTGRESQL_STORAGE_PERMISSIONS) - - for item in os.listdir(storage_path): - if item in STORAGE_LAYOUT_IGNORED_ENTRIES: - continue - - source = os.path.join(storage_path, item) - destination = os.path.join(target_path, item) - logger.info("Moving %s to %s", source, destination) - os.rename(source, destination) - - def _repair_pg_wal_symlink(self) -> None: - """Ensure pg_wal points to the versioned WAL directory for migrated deployments.""" - pg_wal_path = Path(POSTGRESQL_DATA_DIR) / "pg_wal" - if not pg_wal_path.is_symlink(): - return - - current_target = Path(os.path.realpath(pg_wal_path)) - new_target = Path(LOGS_DATA_DIR) - if current_target == new_target: - return - - old_target = Path(LOGS_STORAGE_PATH) - if current_target != old_target: - logger.warning( - "Skipping pg_wal repair: unexpected target %s (expected %s or %s)", - current_target, - old_target, - new_target, - ) - return - - self._migrate_storage_mount(LOGS_STORAGE_PATH, LOGS_DATA_DIR) - pg_wal_path.unlink() - pg_wal_path.symlink_to(LOGS_DATA_DIR) - logger.info("Updated pg_wal symlink to %s", LOGS_DATA_DIR) + _daemon_uid = 584792 def _ensure_storage_layout(self) -> None: - """Ensure the PostgreSQL storage layout is prepared or migrated exactly once.""" - if self._peers is None: - raise RuntimeError("Peer relation not ready") - if not self.is_storage_layout_migrated: - for storage_path, target_path in ( - (POSTGRESQL_DATA_PATH, POSTGRESQL_DATA_DIR), - (ARCHIVE_STORAGE_PATH, ARCHIVE_DATA_DIR), - (LOGS_STORAGE_PATH, LOGS_DATA_DIR), - (TEMP_STORAGE_PATH, TEMP_DATA_DIR), - ): - self._migrate_storage_mount(storage_path, target_path) + """Ensure versioned storage directories exist and migrate data. - self.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] = "True" - else: - # The temp data directory may live on a tmpfs mount that is wiped on - # reboot, even after the one-time migration flag has been recorded. - # Create it WITHOUT setting owner/mode: set_up_database detects the - # root-owned directory as needing a permissions fix, which triggers its - # _handle_temp_tablespace_on_reboot path for the tmpfs case and ensures - # the tablespace directory structure is reinitialised by PostgreSQL. - Path(TEMP_DATA_DIR).mkdir(parents=True, exist_ok=True) - - self._repair_pg_wal_symlink() - self._reconcile_storage_permissions() - - def _reconcile_storage_permissions(self) -> None: - """Reconcile storage root permissions and pg_wal symlink ownership. - - Storage roots (e.g. common/var/lib/postgresql/) are container directories - after migration and must be 0755, not the 0700 of a PostgreSQL data dir. - The pg_wal symlink must be owned by _daemon_ — it is created by Patroni on - fresh deploys, but recreated as root when the charm repairs it during upgrade. - This method is idempotent and always runs so it heals units migrated before - these fixes were applied. + Snap hooks run confined and cannot access daemon-owned storage + roots. The charm runs unconfined — directory creation and forward + data migration are done here, before the snap refresh. """ - for storage_path in ( - POSTGRESQL_DATA_PATH, - ARCHIVE_STORAGE_PATH, - LOGS_STORAGE_PATH, - TEMP_STORAGE_PATH, + for path in (POSTGRESQL_DATA_DIR, ARCHIVE_DATA_DIR, LOGS_DATA_DIR, TEMP_DATA_DIR): + p = Path(path) + p.mkdir(parents=True, exist_ok=True) + shutil.chown(p, user=self._daemon_uid, group=0) + # Also chown the parent 16/ directory created by mkdir -p. + if p.parent.name == "16": + shutil.chown(p.parent, user=self._daemon_uid, group=0) + os.chmod(POSTGRESQL_DATA_DIR, 0o700) + + if ( + Path(POSTGRESQL_DATA_PATH, "PG_VERSION").exists() + and not Path(POSTGRESQL_DATA_DIR, "PG_VERSION").exists() ): - if os.path.exists(storage_path): - os.chmod(storage_path, 0o755) # noqa: S103 + self._migrate_storage_roots_to_versioned() + # Ensure PostgreSQL can access the migrated data. + shutil.chown(POSTGRESQL_DATA_DIR, user=self._daemon_uid, group=0) + os.chmod(POSTGRESQL_DATA_DIR, 0o700) - pg_wal_path = Path(POSTGRESQL_DATA_DIR) / "pg_wal" - if pg_wal_path.is_symlink(): - user_database = pwd.getpwnam("_daemon_") - os.lchown(str(pg_wal_path), user_database.pw_uid, user_database.pw_gid) + @staticmethod + def _migrate_storage_roots_to_versioned() -> None: + """Move data from storage roots into versioned 16/main subdirectories.""" + storage_pairs = [ + (POSTGRESQL_DATA_PATH, POSTGRESQL_DATA_DIR), + (ARCHIVE_STORAGE_PATH, ARCHIVE_DATA_DIR), + (LOGS_STORAGE_PATH, LOGS_DATA_DIR), + (TEMP_STORAGE_PATH, TEMP_DATA_DIR), + ] + for storage_root, versioned_path in storage_pairs: + for item in os.listdir(storage_root): + if item in ("16", "lost+found"): + continue + src = os.path.join(storage_root, item) + dst = os.path.join(versioned_path, item) + if os.path.exists(dst): + continue + os.rename(src, dst) + + pg_wal = Path(POSTGRESQL_DATA_DIR) / "pg_wal" + if pg_wal.is_symlink(): + current_target = Path(os.path.realpath(pg_wal)) + if current_target != Path(LOGS_DATA_DIR): + pg_wal.unlink() + pg_wal.symlink_to(LOGS_DATA_DIR) + elif not pg_wal.exists(): + pg_wal.symlink_to(LOGS_DATA_DIR) @staticmethod def _clear_pg_version_dirs(path: Path) -> None: @@ -1899,12 +1849,7 @@ def _on_start(self, event: StartEvent) -> None: self.tls.generate_internal_peer_cert() self.unit_peer_data.update({"ip": self._unit_ip}) - try: - self._ensure_storage_layout() - except (OSError, RuntimeError): - logger.exception("Unable to migrate PostgreSQL storage layout") - self.set_unit_status(BlockedStatus("Failed to migrate PostgreSQL storage layout")) - return + self._ensure_storage_layout() # Open port try: diff --git a/src/constants.py b/src/constants.py index def315fc71..27f631a5fd 100644 --- a/src/constants.py +++ b/src/constants.py @@ -98,4 +98,3 @@ TRACING_RELATION_NAME = "tracing" PGBACKREST_LOGROTATE_FILE = "/etc/logrotate.d/pgbackrest.logrotate" -STORAGE_LAYOUT_MIGRATED_KEY = "storage_layout_migrated" diff --git a/tests/integration/high_availability/high_availability_helpers_new.py b/tests/integration/high_availability/high_availability_helpers_new.py index 4ed731eb7c..8144be295b 100644 --- a/tests/integration/high_availability/high_availability_helpers_new.py +++ b/tests/integration/high_availability/high_availability_helpers_new.py @@ -22,6 +22,11 @@ JujuAppsStatusFn = Callable[[Status, str], bool] +def bracket_ipv6(addr: str) -> str: + """Wrap an IPv6 address in brackets for URL use.""" + return f"[{addr}]" if ":" in addr else addr + + def check_db_units_writes_increment( juju: Juju, app_name: str, @@ -178,9 +183,9 @@ def get_db_standby_leader_unit(juju: Juju, app_name: str) -> str: """Get the current standby node of the cluster.""" unit_address = get_unit_ip(juju, app_name, get_app_leader(juju, app_name)) - for member in requests.get(f"https://{unit_address}:8008/cluster", verify=False).json()[ - "members" - ]: + for member in requests.get( + f"https://{bracket_ipv6(unit_address)}:8008/cluster", verify=False + ).json()["members"]: if member["role"] == "standby_leader": return member["name"][::-1].replace("-", "/")[::-1] @@ -255,5 +260,7 @@ def count_switchovers(juju: Juju, app_name: str) -> int: """Return the number of performed switchovers.""" app_primary = get_db_primary_unit(juju, app_name) unit_address = get_unit_ip(juju, app_name, app_primary) - switchover_history_info = requests.get(f"https://{unit_address}:8008/history", verify=False) + switchover_history_info = requests.get( + f"https://{bracket_ipv6(unit_address)}:8008/history", verify=False + ) return len(switchover_history_info.json()) diff --git a/tests/integration/high_availability/test_upgrade_skip_pre_upgrade_check.py b/tests/integration/high_availability/test_upgrade_skip_pre_upgrade_check.py index 8590227c08..42f0089c9f 100644 --- a/tests/integration/high_availability/test_upgrade_skip_pre_upgrade_check.py +++ b/tests/integration/high_availability/test_upgrade_skip_pre_upgrade_check.py @@ -2,6 +2,7 @@ # See LICENSE file for licensing details. import logging +import platform import jubilant from jubilant import Juju @@ -30,6 +31,7 @@ def test_deploy_stable(juju: Juju) -> None: base="ubuntu@24.04", channel="16/stable", config={"profile": "testing"}, + constraints={"arch": "arm64"} if platform.machine() == "aarch64" else None, num_units=3, ) juju.deploy( @@ -37,6 +39,7 @@ def test_deploy_stable(juju: Juju) -> None: app=DB_TEST_APP_NAME, base="ubuntu@24.04", channel="latest/edge", + constraints={"arch": "arm64"} if platform.machine() == "aarch64" else None, num_units=1, ) @@ -48,7 +51,7 @@ def test_deploy_stable(juju: Juju) -> None: logging.info("Wait for applications to become active") juju.wait( ready=wait_for_apps_status(jubilant.all_active, DB_APP_NAME, DB_TEST_APP_NAME), - timeout=20 * MINUTE_SECS, + timeout=40 * MINUTE_SECS, ) @@ -61,7 +64,7 @@ def test_refresh_without_pre_refresh_check(juju: Juju, charm: str, continuous_wr logging.info("Wait for refresh to block as paused or incompatible") try: - juju.wait(lambda status: status.apps[DB_APP_NAME].is_blocked, timeout=5 * MINUTE_SECS) + juju.wait(lambda status: status.apps[DB_APP_NAME].is_blocked, timeout=10 * MINUTE_SECS) units = get_app_units(juju, DB_APP_NAME) unit_names = sorted(units.keys()) @@ -72,13 +75,13 @@ def test_refresh_without_pre_refresh_check(juju: Juju, charm: str, continuous_wr unit=unit_names[-1], action="force-refresh-start", params={"check-compatibility": False}, - wait=5 * MINUTE_SECS, + wait=40 * MINUTE_SECS, ) - juju.wait(jubilant.all_agents_idle, timeout=5 * MINUTE_SECS) + juju.wait(jubilant.all_agents_idle, timeout=10 * MINUTE_SECS) logging.info("Run resume-refresh action") - juju.run(unit=unit_names[1], action="resume-refresh", wait=5 * MINUTE_SECS) + juju.run(unit=unit_names[1], action="resume-refresh", wait=40 * MINUTE_SECS) except TimeoutError: logging.info("Upgrade completed without snap refresh (charm.py upgrade only)") assert juju.status().apps[DB_APP_NAME].is_active @@ -86,7 +89,7 @@ def test_refresh_without_pre_refresh_check(juju: Juju, charm: str, continuous_wr logging.info("Wait for upgrade to complete") juju.wait( ready=wait_for_apps_status(jubilant.all_active, DB_APP_NAME), - timeout=20 * MINUTE_SECS, + timeout=40 * MINUTE_SECS, ) logging.info("Ensure continuous writes are incrementing") @@ -109,7 +112,7 @@ async def test_rollback_without_pre_refresh_check( logging.info("Wait for refresh to block as paused or incompatible") try: - juju.wait(lambda status: status.apps[DB_APP_NAME].is_blocked, timeout=5 * MINUTE_SECS) + juju.wait(lambda status: status.apps[DB_APP_NAME].is_blocked, timeout=10 * MINUTE_SECS) units = get_app_units(juju, DB_APP_NAME) unit_names = sorted(units.keys()) @@ -120,13 +123,13 @@ async def test_rollback_without_pre_refresh_check( unit=unit_names[-1], action="force-refresh-start", params={"check-compatibility": False}, - wait=5 * MINUTE_SECS, + wait=40 * MINUTE_SECS, ) - juju.wait(jubilant.all_agents_idle, timeout=5 * MINUTE_SECS) + juju.wait(jubilant.all_agents_idle, timeout=10 * MINUTE_SECS) logging.info("Run resume-refresh action") - juju.run(unit=unit_names[1], action="resume-refresh", wait=5 * MINUTE_SECS) + juju.run(unit=unit_names[1], action="resume-refresh", wait=40 * MINUTE_SECS) except TimeoutError: logging.info("Upgrade completed without snap refresh (charm.py upgrade only)") assert juju.status().apps[DB_APP_NAME].is_active @@ -134,7 +137,7 @@ async def test_rollback_without_pre_refresh_check( logging.info("Wait for upgrade to complete") juju.wait( ready=wait_for_apps_status(jubilant.all_active, DB_APP_NAME), - timeout=20 * MINUTE_SECS, + timeout=40 * MINUTE_SECS, ) check_db_units_writes_increment(juju, DB_APP_NAME) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index d559205132..5927e68ed0 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -52,9 +52,6 @@ PEER, POSTGRESQL_DATA_DIR, SECRET_INTERNAL_LABEL, - STORAGE_LAYOUT_MIGRATED_KEY, - TEMP_DATA_DIR, - TEMP_STORAGE_PATH, UPDATE_CERTS_BIN_PATH, ) @@ -650,197 +647,23 @@ def test_on_start_after_blocked_state(harness): def test_ensure_storage_layout(harness, tmp_path): - data_root = tmp_path / "postgresql" - archive_root = tmp_path / "archive" - logs_root = tmp_path / "logs" - temp_root = tmp_path / "temp" - - data_root.mkdir() - archive_root.mkdir() - logs_root.mkdir() - temp_root.mkdir() - - (data_root / "data.txt").write_text("data") - (archive_root / "archive.txt").write_text("archive") - (logs_root / "logs.txt").write_text("logs") - (temp_root / "temp.txt").write_text("temp") - (logs_root / "lost+found").mkdir() - - with ( - patch("charm.POSTGRESQL_DATA_PATH", str(data_root)), - patch("charm.POSTGRESQL_DATA_DIR", str(data_root / "16" / "main")), - patch("charm.ARCHIVE_STORAGE_PATH", str(archive_root)), - patch("charm.ARCHIVE_DATA_DIR", str(archive_root / "16" / "main")), - patch("charm.LOGS_STORAGE_PATH", str(logs_root)), - patch("charm.LOGS_DATA_DIR", str(logs_root / "16" / "main")), - patch("charm.TEMP_STORAGE_PATH", str(temp_root)), - patch("charm.TEMP_DATA_DIR", str(temp_root / "16" / "main")), - patch("charm._change_owner") as _change_owner, - patch("charm.os.chmod") as _chmod, - patch("charm.os.lchown") as _lchown, - patch("charm.pwd.getpwnam"), + """_ensure_storage_layout creates all four versioned storage dirs.""" + data_root = tmp_path / "data" / "16" / "main" + archive_root = tmp_path / "archive" / "16" / "main" + logs_root = tmp_path / "logs" / "16" / "main" + temp_root = tmp_path / "temp" / "16" / "main" + with ( + patch("charm.POSTGRESQL_DATA_DIR", str(data_root)), + patch("charm.ARCHIVE_DATA_DIR", str(archive_root)), + patch("charm.LOGS_DATA_DIR", str(logs_root)), + patch("charm.TEMP_DATA_DIR", str(temp_root)), + patch("charm.shutil.chown"), ): harness.charm._ensure_storage_layout() - - assert (data_root / "16" / "main" / "data.txt").read_text() == "data" - assert (archive_root / "16" / "main" / "archive.txt").read_text() == "archive" - assert (logs_root / "16" / "main" / "logs.txt").read_text() == "logs" - assert (temp_root / "16" / "main" / "temp.txt").read_text() == "temp" - assert not (data_root / "data.txt").exists() - assert (logs_root / "lost+found").exists() - assert harness.charm.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] == "True" - # 4 calls from _migrate_storage_mount (target dirs) + 4 from _reconcile_storage_permissions (roots) - assert _change_owner.call_count == 4 - assert _chmod.call_count == 8 - _chmod.assert_any_call(str(data_root), 0o755) - _chmod.assert_any_call(str(archive_root), 0o755) - _chmod.assert_any_call(str(logs_root), 0o755) - _chmod.assert_any_call(str(temp_root), 0o755) - # No pg_wal symlink in this test - _lchown.assert_not_called() - - (data_root / "stray.txt").write_text("stray") - _change_owner.reset_mock() - _chmod.reset_mock() - _lchown.reset_mock() - - harness.charm._ensure_storage_layout() - - assert (data_root / "stray.txt").read_text() == "stray" - # Already migrated: only TEMP_DATA_DIR is re-created (tmpfs reboot guard) + reconciliation - # mkdir only — no _change_owner or chmod on TEMP_DATA_DIR (library detects root-owned dir) - _change_owner.assert_not_called() - assert _chmod.call_count == 4 # only the 4 storage root chmod calls - _chmod.assert_any_call(str(data_root), 0o755) - _lchown.assert_not_called() - - -def test_ensure_storage_layout_repairs_stale_pg_wal_symlink(harness, tmp_path): - data_root = tmp_path / "postgresql" - archive_root = tmp_path / "archive" - logs_root = tmp_path / "logs" - temp_root = tmp_path / "temp" - - for path in ( - data_root / "16" / "main", - archive_root / "16" / "main", - logs_root / "16" / "main", - temp_root / "16" / "main", - ): - path.mkdir(parents=True) - - stale_pg_wal = data_root / "16" / "main" / "pg_wal" - stale_pg_wal.symlink_to(logs_root) - (logs_root / "000000010000000000000008").write_text("wal") - (logs_root / "archive_status").mkdir() - harness.charm.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] = "True" - - with ( - patch("charm.POSTGRESQL_DATA_PATH", str(data_root)), - patch("charm.POSTGRESQL_DATA_DIR", str(data_root / "16" / "main")), - patch("charm.ARCHIVE_STORAGE_PATH", str(archive_root)), - patch("charm.ARCHIVE_DATA_DIR", str(archive_root / "16" / "main")), - patch("charm.LOGS_STORAGE_PATH", str(logs_root)), - patch("charm.LOGS_DATA_DIR", str(logs_root / "16" / "main")), - patch("charm.TEMP_STORAGE_PATH", str(temp_root)), - patch("charm.TEMP_DATA_DIR", str(temp_root / "16" / "main")), - patch("charm._change_owner") as _change_owner, - patch("charm.os.chmod") as _chmod, - patch("charm.os.lchown") as _lchown, - patch("charm.pwd.getpwnam") as _getpwnam, - ): - harness.charm._ensure_storage_layout() - - assert os.readlink(stale_pg_wal) == str(logs_root / "16" / "main") - assert (logs_root / "16" / "main" / "000000010000000000000008").read_text() == "wal" - assert not (logs_root / "000000010000000000000008").exists() - assert (logs_root / "16" / "main" / "archive_status").exists() - # TEMP_DATA_DIR re-created (mkdir only) + _migrate_storage_mount for logs - assert _change_owner.call_count == 1 - _change_owner.assert_called_once_with(str(logs_root / "16" / "main")) - _chmod.assert_any_call(str(logs_root / "16" / "main"), 0o700) - # _reconcile_storage_permissions: chmod on all 4 storage roots - _chmod.assert_any_call(str(data_root), 0o755) - _chmod.assert_any_call(str(logs_root), 0o755) - assert _chmod.call_count == 5 # 1 target dir (logs) + 4 storage roots - # pg_wal symlink ownership reconciled - _lchown.assert_called_once_with( - str(stale_pg_wal), _getpwnam.return_value.pw_uid, _getpwnam.return_value.pw_gid - ) - _getpwnam.assert_called_with("_daemon_") - - -def test_reconcile_storage_permissions_heals_already_migrated_units(harness, tmp_path): - """Reconciliation runs even when already migrated, fixing ownership/perms from before the fix.""" - data_root = tmp_path / "postgresql" - archive_root = tmp_path / "archive" - logs_root = tmp_path / "logs" - temp_root = tmp_path / "temp" - - for path in ( - data_root / "16" / "main", - archive_root / "16" / "main", - logs_root / "16" / "main", - temp_root / "16" / "main", - ): - path.mkdir(parents=True) - - # Simulate pg_wal symlink with wrong root:root ownership (the bug) - pg_wal = data_root / "16" / "main" / "pg_wal" - pg_wal.symlink_to(logs_root / "16" / "main") - - # Unit was already migrated before this fix was applied - harness.charm.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] = "True" - - with ( - patch("charm.POSTGRESQL_DATA_PATH", str(data_root)), - patch("charm.POSTGRESQL_DATA_DIR", str(data_root / "16" / "main")), - patch("charm.ARCHIVE_STORAGE_PATH", str(archive_root)), - patch("charm.ARCHIVE_DATA_DIR", str(archive_root / "16" / "main")), - patch("charm.LOGS_STORAGE_PATH", str(logs_root)), - patch("charm.LOGS_DATA_DIR", str(logs_root / "16" / "main")), - patch("charm.TEMP_STORAGE_PATH", str(temp_root)), - patch("charm.TEMP_DATA_DIR", str(temp_root / "16" / "main")), - patch("charm._change_owner"), - patch("charm.os.chmod") as _chmod, - patch("charm.os.lchown") as _lchown, - patch("charm.pwd.getpwnam") as _getpwnam, - ): - harness.charm._ensure_storage_layout() - - # Storage root permissions reconciled (0755) even though migration was already done - _chmod.assert_any_call(str(data_root), 0o755) - _chmod.assert_any_call(str(archive_root), 0o755) - _chmod.assert_any_call(str(logs_root), 0o755) - _chmod.assert_any_call(str(temp_root), 0o755) - assert _chmod.call_count == 4 # only the 4 storage root chmod calls (no TEMP_DATA_DIR) - - # pg_wal symlink ownership reconciled (_daemon_) - _lchown.assert_called_once_with( - str(pg_wal), _getpwnam.return_value.pw_uid, _getpwnam.return_value.pw_gid - ) - _getpwnam.assert_called_with("_daemon_") - - connection = MagicMock() - cursor = MagicMock() - cursor.fetchone.return_value = (TEMP_STORAGE_PATH,) - connection.cursor.return_value = cursor - postgresql = MagicMock() - postgresql._connect_to_database.return_value = connection - - with patch( - "charm.PostgresqlOperatorCharm.postgresql", - new_callable=PropertyMock, - return_value=postgresql, - ): - assert harness.charm._ensure_temp_tablespace_location() - - cursor.execute.assert_has_calls([ - call("SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';"), - call("DROP TABLESPACE temp;"), - call(f"CREATE TABLESPACE temp LOCATION '{TEMP_DATA_DIR}';"), - call("GRANT CREATE ON TABLESPACE temp TO public;"), - ]) + assert data_root.is_dir() + assert archive_root.is_dir() + assert logs_root.is_dir() + assert temp_root.is_dir() def test_ensure_temp_tablespace_location_recovers_dropped_tablespace(harness, tmp_path): @@ -937,37 +760,20 @@ def test_ensure_temp_tablespace_location_skips_reinit_when_pg_dir_present(harnes def test_ensure_storage_layout_recreates_temp_dir_on_reboot(harness, tmp_path): - """TEMP_DATA_DIR is recreated even when already migrated (e.g. after a tmpfs wipe on reboot).""" - temp_root = tmp_path / "temp" - temp_root.mkdir() - # Other storage roots are pre-created with versioned subdirs (persistent storage) - for path in ( - tmp_path / "postgresql" / "16" / "main", - tmp_path / "archive" / "16" / "main", - tmp_path / "logs" / "16" / "main", - ): - path.mkdir(parents=True) - - # Mark as already migrated — temp_root/16/main does NOT exist (simulating tmpfs wipe) - harness.charm.unit_peer_data[STORAGE_LAYOUT_MIGRATED_KEY] = "True" - - with ( - patch("charm.POSTGRESQL_DATA_PATH", str(tmp_path / "postgresql")), - patch("charm.POSTGRESQL_DATA_DIR", str(tmp_path / "postgresql" / "16" / "main")), - patch("charm.ARCHIVE_STORAGE_PATH", str(tmp_path / "archive")), - patch("charm.ARCHIVE_DATA_DIR", str(tmp_path / "archive" / "16" / "main")), - patch("charm.LOGS_STORAGE_PATH", str(tmp_path / "logs")), - patch("charm.LOGS_DATA_DIR", str(tmp_path / "logs" / "16" / "main")), - patch("charm.TEMP_STORAGE_PATH", str(temp_root)), - patch("charm.TEMP_DATA_DIR", str(temp_root / "16" / "main")), - patch("charm._change_owner"), - patch("charm.os.chmod"), - patch("charm.os.lchown"), - patch("charm.pwd.getpwnam"), + """All versioned dirs, including TEMP_DATA_DIR, are recreated after a tmpfs wipe on reboot.""" + data_root = tmp_path / "data" / "16" / "main" + archive_root = tmp_path / "archive" / "16" / "main" + logs_root = tmp_path / "logs" / "16" / "main" + temp_root = tmp_path / "temp" / "16" / "main" + with ( + patch("charm.POSTGRESQL_DATA_DIR", str(data_root)), + patch("charm.ARCHIVE_DATA_DIR", str(archive_root)), + patch("charm.LOGS_DATA_DIR", str(logs_root)), + patch("charm.TEMP_DATA_DIR", str(temp_root)), + patch("charm.shutil.chown"), ): harness.charm._ensure_storage_layout() - - assert (temp_root / "16" / "main").is_dir() + assert temp_root.is_dir() def test_ensure_temp_tablespace_location_if_primary_skips_when_no_endpoint(harness): @@ -992,18 +798,6 @@ def test_ensure_temp_tablespace_location_if_primary_skips_when_no_endpoint(harne assert result is True _ensure_temp_tablespace_location.assert_not_called() - refresh = MagicMock(unit_status_higher_priority=None) - with ( - patch("charm.PostgresqlOperatorCharm._ensure_storage_layout", side_effect=OSError), - patch("charm.PostgresqlOperatorCharm.get_secret", return_value=None), - patch("charm.Patroni.start_patroni") as _start_patroni, - ): - harness.charm._post_snap_refresh(refresh) - - _start_patroni.assert_not_called() - assert isinstance(harness.charm.unit.status, BlockedStatus) - assert harness.charm.unit.status.message == "Failed to migrate PostgreSQL storage layout" - def test_on_update_status(harness): with ( From f29d5c1ecbe8ce075c593088aaed1db38d5e71bd Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 7 May 2026 14:05:49 -0300 Subject: [PATCH 06/19] refactor(storage): delegate data migration to snap hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move forward and reverse storage layout migration from the charm into the snap's post-refresh and pre-refresh hooks. The charm now only ensures the ephemeral temp tablespace directory exists, which may live on a tmpfs mount wiped on reboot. Data migration direction is determined by the snap hooks reading the already-rendered Patroni YAML as ground truth — versioned paths (16/main) trigger forward migration, root paths trigger reverse. This makes rollbacks from a versioned-storage charm to an older root-storage charm transparent. Pin snap revisions: amd64 313, aarch64/arm64 314. Signed-off-by: Marcelo Henrique Neppel --- refresh_versions.toml | 6 ++-- src/charm.py | 76 +++++++++------------------------------- src/constants.py | 1 + tests/unit/test_charm.py | 32 +++++++---------- 4 files changed, 34 insertions(+), 81 deletions(-) diff --git a/refresh_versions.toml b/refresh_versions.toml index 5b4a66fa27..f27f3c3b02 100644 --- a/refresh_versions.toml +++ b/refresh_versions.toml @@ -6,6 +6,8 @@ name = "charmed-postgresql" [snap.revisions] # amd64 -x86_64 = "310" +x86_64 = "313" # arm64 (Linux) -aarch64 = "311" +aarch64 = "314" +# arm64 (macOS / Apple Silicon - same snap revision) +arm64 = "314" diff --git a/src/charm.py b/src/charm.py index f83b34f67c..96dd9ecfc8 100755 --- a/src/charm.py +++ b/src/charm.py @@ -101,13 +101,9 @@ from config import CharmConfig from constants import ( APP_SCOPE, - ARCHIVE_DATA_DIR, - ARCHIVE_STORAGE_PATH, DATABASE, DATABASE_DEFAULT_NAME, DATABASE_PORT, - LOGS_DATA_DIR, - LOGS_STORAGE_PATH, METRICS_PORT, MONITORING_PASSWORD_KEY, MONITORING_SNAP_SERVICE, @@ -117,7 +113,6 @@ PGBACKREST_MONITORING_SNAP_SERVICE, PLUGIN_OVERRIDES, POSTGRESQL_DATA_DIR, - POSTGRESQL_DATA_PATH, RAFT_PASSWORD_KEY, REPLICATION_CONSUMER_RELATION, REPLICATION_OFFER_RELATION, @@ -126,6 +121,7 @@ SECRET_DELETED_LABEL, SECRET_INTERNAL_LABEL, SECRET_KEY_OVERRIDES, + SNAP_DAEMON_USER, SPI_MODULE, TEMP_DATA_DIR, TEMP_STORAGE_PATH, @@ -251,9 +247,10 @@ def refresh_snap( self._charm.set_unit_status(MaintenanceStatus("updating configuration"), refresh=refresh) self._charm.update_config(refresh=refresh) - # Create versioned storage directories before the snap refresh. - # The snap's post-refresh hook runs confined and may be unable to - # create subdirectories inside daemon-owned storage roots. + # Ensure the temp tablespace directory exists before the snap refresh. + # Data migration between storage roots and versioned paths is handled + # by the snap's post-refresh hook (forward) and pre-refresh hook + # (reverse), which start a one-shot daemon running as _daemon_. self._charm._ensure_storage_layout() # TODO add graceful shutdown before refreshing snap? @@ -675,60 +672,19 @@ def is_unit_stopped(self) -> bool: """Returns whether the unit is stopped.""" return "stopped" in self.unit_peer_data - _daemon_uid = 584792 - def _ensure_storage_layout(self) -> None: - """Ensure versioned storage directories exist and migrate data. - - Snap hooks run confined and cannot access daemon-owned storage - roots. The charm runs unconfined — directory creation and forward - data migration are done here, before the snap refresh. + """Ensure the temp tablespace directory exists. + + Data migration between storage roots and versioned 16/main + subdirectories is handled by the snap hooks (pre-refresh for + reverse, post-refresh for forward). TEMP_DATA_DIR may live on + a tmpfs mount that is wiped on reboot, so we recreate it + unconditionally. CREATE TABLESPACE requires the directory to + be writable by the PostgreSQL _daemon_ user, so we chown it. """ - for path in (POSTGRESQL_DATA_DIR, ARCHIVE_DATA_DIR, LOGS_DATA_DIR, TEMP_DATA_DIR): - p = Path(path) - p.mkdir(parents=True, exist_ok=True) - shutil.chown(p, user=self._daemon_uid, group=0) - # Also chown the parent 16/ directory created by mkdir -p. - if p.parent.name == "16": - shutil.chown(p.parent, user=self._daemon_uid, group=0) - os.chmod(POSTGRESQL_DATA_DIR, 0o700) - - if ( - Path(POSTGRESQL_DATA_PATH, "PG_VERSION").exists() - and not Path(POSTGRESQL_DATA_DIR, "PG_VERSION").exists() - ): - self._migrate_storage_roots_to_versioned() - # Ensure PostgreSQL can access the migrated data. - shutil.chown(POSTGRESQL_DATA_DIR, user=self._daemon_uid, group=0) - os.chmod(POSTGRESQL_DATA_DIR, 0o700) - - @staticmethod - def _migrate_storage_roots_to_versioned() -> None: - """Move data from storage roots into versioned 16/main subdirectories.""" - storage_pairs = [ - (POSTGRESQL_DATA_PATH, POSTGRESQL_DATA_DIR), - (ARCHIVE_STORAGE_PATH, ARCHIVE_DATA_DIR), - (LOGS_STORAGE_PATH, LOGS_DATA_DIR), - (TEMP_STORAGE_PATH, TEMP_DATA_DIR), - ] - for storage_root, versioned_path in storage_pairs: - for item in os.listdir(storage_root): - if item in ("16", "lost+found"): - continue - src = os.path.join(storage_root, item) - dst = os.path.join(versioned_path, item) - if os.path.exists(dst): - continue - os.rename(src, dst) - - pg_wal = Path(POSTGRESQL_DATA_DIR) / "pg_wal" - if pg_wal.is_symlink(): - current_target = Path(os.path.realpath(pg_wal)) - if current_target != Path(LOGS_DATA_DIR): - pg_wal.unlink() - pg_wal.symlink_to(LOGS_DATA_DIR) - elif not pg_wal.exists(): - pg_wal.symlink_to(LOGS_DATA_DIR) + temp_dir = Path(TEMP_DATA_DIR) + temp_dir.mkdir(parents=True, exist_ok=True) + shutil.chown(temp_dir, user=SNAP_DAEMON_USER, group=SNAP_DAEMON_USER) @staticmethod def _clear_pg_version_dirs(path: Path) -> None: diff --git a/src/constants.py b/src/constants.py index 95a8d90084..f8d686a0e9 100644 --- a/src/constants.py +++ b/src/constants.py @@ -25,6 +25,7 @@ PATRONI_SERVICE_DEFAULT_PATH = f"/etc/systemd/system/{PATRONI_SERVICE_NAME}" # Snap constants. +SNAP_DAEMON_USER = "_daemon_" PGBACKREST_EXECUTABLE = "charmed-postgresql.pgbackrest" # pgBackRest logging configuration # We use stderr for all error/warning output to have a consistent, predictable error extraction diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 5927e68ed0..a31011e2d0 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -647,23 +647,23 @@ def test_on_start_after_blocked_state(harness): def test_ensure_storage_layout(harness, tmp_path): - """_ensure_storage_layout creates all four versioned storage dirs.""" - data_root = tmp_path / "data" / "16" / "main" - archive_root = tmp_path / "archive" / "16" / "main" - logs_root = tmp_path / "logs" / "16" / "main" + """_ensure_storage_layout creates TEMP_DATA_DIR only. + + Data migration between storage roots and versioned paths is handled + by the snap hooks — the charm only ensures TEMP_DATA_DIR exists + (it may live on a tmpfs mount that is wiped on reboot). + """ temp_root = tmp_path / "temp" / "16" / "main" with ( - patch("charm.POSTGRESQL_DATA_DIR", str(data_root)), - patch("charm.ARCHIVE_DATA_DIR", str(archive_root)), - patch("charm.LOGS_DATA_DIR", str(logs_root)), patch("charm.TEMP_DATA_DIR", str(temp_root)), - patch("charm.shutil.chown"), + patch("charm.shutil"), ): harness.charm._ensure_storage_layout() - assert data_root.is_dir() - assert archive_root.is_dir() - assert logs_root.is_dir() assert temp_root.is_dir() + # No other dirs should be created by the charm. + assert not (tmp_path / "data").exists() + assert not (tmp_path / "archive").exists() + assert not (tmp_path / "logs").exists() def test_ensure_temp_tablespace_location_recovers_dropped_tablespace(harness, tmp_path): @@ -760,17 +760,11 @@ def test_ensure_temp_tablespace_location_skips_reinit_when_pg_dir_present(harnes def test_ensure_storage_layout_recreates_temp_dir_on_reboot(harness, tmp_path): - """All versioned dirs, including TEMP_DATA_DIR, are recreated after a tmpfs wipe on reboot.""" - data_root = tmp_path / "data" / "16" / "main" - archive_root = tmp_path / "archive" / "16" / "main" - logs_root = tmp_path / "logs" / "16" / "main" + """TEMP_DATA_DIR is recreated after a tmpfs wipe on reboot.""" temp_root = tmp_path / "temp" / "16" / "main" with ( - patch("charm.POSTGRESQL_DATA_DIR", str(data_root)), - patch("charm.ARCHIVE_DATA_DIR", str(archive_root)), - patch("charm.LOGS_DATA_DIR", str(logs_root)), patch("charm.TEMP_DATA_DIR", str(temp_root)), - patch("charm.shutil.chown"), + patch("charm.shutil"), ): harness.charm._ensure_storage_layout() assert temp_root.is_dir() From 734c218e119255e2ac5ef6dba627c1354d121510 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Fri, 8 May 2026 09:21:14 -0300 Subject: [PATCH 07/19] test(upgrade): increase refresh timeouts to prevent spurious CI failures Measurements from 16/stable snap refreshes showed that 5-minute timeouts are too tight for the charm_refresh flow. Snap download and install takes ~2.5 min per unit, and database initialization can stall briefly during rolling restarts. Bump the blocked-wait, force-refresh- start, and agents-idle timeouts to 10 minutes, and resume-refresh to 15 minutes, matching the empirically observed timing. Signed-off-by: Marcelo Henrique Neppel --- .../high_availability/test_async_replication_upgrade.py | 8 ++++---- tests/integration/high_availability/test_upgrade.py | 8 ++++---- .../high_availability/test_upgrade_from_stable.py | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/integration/high_availability/test_async_replication_upgrade.py b/tests/integration/high_availability/test_async_replication_upgrade.py index e491184395..351a702dbb 100644 --- a/tests/integration/high_availability/test_async_replication_upgrade.py +++ b/tests/integration/high_availability/test_async_replication_upgrade.py @@ -241,7 +241,7 @@ def run_upgrade_from_edge(juju: Juju, app_name: str, charm: str) -> None: juju.refresh(app=app_name, path=charm) logging.info("Wait for refresh to block as paused or incompatible") try: - juju.wait(lambda status: status.apps[app_name].is_blocked, timeout=5 * MINUTE_SECS) + juju.wait(lambda status: status.apps[app_name].is_blocked, timeout=10 * MINUTE_SECS) units = get_app_units(juju, app_name) unit_names = sorted(units.keys()) @@ -252,13 +252,13 @@ def run_upgrade_from_edge(juju: Juju, app_name: str, charm: str) -> None: unit=unit_names[-1], action="force-refresh-start", params={"check-compatibility": False}, - wait=5 * MINUTE_SECS, + wait=10 * MINUTE_SECS, ) - juju.wait(jubilant.all_agents_idle, timeout=5 * MINUTE_SECS) + juju.wait(jubilant.all_agents_idle, timeout=10 * MINUTE_SECS) logging.info("Run resume-refresh action") - juju.run(unit=unit_names[1], action="resume-refresh", wait=5 * MINUTE_SECS) + juju.run(unit=unit_names[1], action="resume-refresh", wait=15 * MINUTE_SECS) except TimeoutError: logging.info("Upgrade completed without snap refresh (charm.py upgrade only)") assert juju.status().apps[app_name].is_active diff --git a/tests/integration/high_availability/test_upgrade.py b/tests/integration/high_availability/test_upgrade.py index 168ffd4fd8..114aefc9b9 100644 --- a/tests/integration/high_availability/test_upgrade.py +++ b/tests/integration/high_availability/test_upgrade.py @@ -80,7 +80,7 @@ def test_upgrade_from_edge(juju: Juju, charm: str, continuous_writes) -> None: juju.refresh(app=DB_APP_NAME, path=charm) logging.info("Wait for refresh to block as paused or incompatible") try: - juju.wait(lambda status: status.apps[DB_APP_NAME].is_blocked, timeout=5 * MINUTE_SECS) + juju.wait(lambda status: status.apps[DB_APP_NAME].is_blocked, timeout=10 * MINUTE_SECS) units = get_app_units(juju, DB_APP_NAME) unit_names = sorted(units.keys()) @@ -91,13 +91,13 @@ def test_upgrade_from_edge(juju: Juju, charm: str, continuous_writes) -> None: unit=unit_names[-1], action="force-refresh-start", params={"check-compatibility": False}, - wait=5 * MINUTE_SECS, + wait=10 * MINUTE_SECS, ) - juju.wait(jubilant.all_agents_idle, timeout=5 * MINUTE_SECS) + juju.wait(jubilant.all_agents_idle, timeout=10 * MINUTE_SECS) logging.info("Run resume-refresh action") - juju.run(unit=unit_names[1], action="resume-refresh", wait=5 * MINUTE_SECS) + juju.run(unit=unit_names[1], action="resume-refresh", wait=15 * MINUTE_SECS) except TimeoutError: logging.info("Upgrade completed without snap refresh (charm.py upgrade only)") assert juju.status().apps[DB_APP_NAME].is_active diff --git a/tests/integration/high_availability/test_upgrade_from_stable.py b/tests/integration/high_availability/test_upgrade_from_stable.py index a50019a326..84c5a8377e 100644 --- a/tests/integration/high_availability/test_upgrade_from_stable.py +++ b/tests/integration/high_availability/test_upgrade_from_stable.py @@ -77,7 +77,7 @@ def test_upgrade_from_stable(juju: Juju, charm: str, continuous_writes) -> None: logging.info("Wait for refresh to block as paused or incompatible") try: - juju.wait(lambda status: status.apps[DB_APP_NAME].is_blocked, timeout=5 * MINUTE_SECS) + juju.wait(lambda status: status.apps[DB_APP_NAME].is_blocked, timeout=10 * MINUTE_SECS) units = get_app_units(juju, DB_APP_NAME) unit_names = sorted(units.keys()) @@ -88,13 +88,13 @@ def test_upgrade_from_stable(juju: Juju, charm: str, continuous_writes) -> None: unit=unit_names[-1], action="force-refresh-start", params={"check-compatibility": False}, - wait=5 * MINUTE_SECS, + wait=10 * MINUTE_SECS, ) - juju.wait(jubilant.all_agents_idle, timeout=5 * MINUTE_SECS) + juju.wait(jubilant.all_agents_idle, timeout=10 * MINUTE_SECS) logging.info("Run resume-refresh action") - juju.run(unit=unit_names[1], action="resume-refresh", wait=5 * MINUTE_SECS) + juju.run(unit=unit_names[1], action="resume-refresh", wait=15 * MINUTE_SECS) except TimeoutError: logging.info("Upgrade completed without snap refresh (charm.py upgrade only)") assert juju.status().apps[DB_APP_NAME].is_active From f20eae0714389ad8a6b3e5399411c85b719b9a7b Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Fri, 8 May 2026 15:16:58 -0300 Subject: [PATCH 08/19] refactor(storage): simplify temp tablespace migration to one-shot handler Replace _ensure_temp_tablespace_location and _ensure_temp_tablespace_location_if_primary with _migrate_temp_tablespace_location, a one-shot handler that only handles the old-to-versioned-path migration (Scenario C). Other scenarios (missing tablespace, tmpfs wipe) are already covered by set_up_database. Drop calls from _on_peer_relation_changed and _on_update_status since migration is a one-time upgrade event. Signed-off-by: Marcelo Henrique Neppel --- src/charm.py | 93 +++++++---------- tests/unit/test_charm.py | 217 ++++++++++++++++++++++++--------------- 2 files changed, 173 insertions(+), 137 deletions(-) diff --git a/src/charm.py b/src/charm.py index 951fe2d7f8..c0cef70962 100755 --- a/src/charm.py +++ b/src/charm.py @@ -470,7 +470,7 @@ def _post_snap_refresh(self, refresh: charm_refresh.Machines): "Did not allow next unit to refresh: member not ready or not joined the cluster yet" ) else: - self._ensure_temp_tablespace_location_if_primary() + self._migrate_temp_tablespace_location() try: self._patroni.set_max_timelines_history() except Exception: @@ -702,14 +702,41 @@ def _clear_pg_version_dirs(path: Path) -> None: if entry.name.startswith("PG_"): shutil.rmtree(entry) - def _ensure_temp_tablespace_location(self) -> bool: - """Ensure the temp tablespace points to the versioned temp directory. + def _migrate_temp_tablespace_location(self) -> bool: + """One-shot migration of the temp tablespace to the versioned directory. - DROP TABLESPACE and CREATE TABLESPACE cannot run inside a transaction block, so this - method avoids using the connection as a context manager (which would create one in - psycopg2). Instead it uses plain assignments and explicit close(), mirroring the - pattern in the single_kernel_postgresql set_up_database helper. + During a snap upgrade, the snap hooks migrate temp data from the old + non-versioned storage root (TEMP_STORAGE_PATH) to the versioned + subdirectory (TEMP_DATA_DIR). This method updates the PostgreSQL catalog + entry to match. + + Other temp tablespace recovery scenarios (missing catalog entry after a + partially-failed migration, empty directory after a tmpfs wipe) are + handled by the single-kernel library's set_up_database during the + leader-elected event. + + DROP TABLESPACE and CREATE TABLESPACE cannot run inside a transaction + block, so this method avoids using the connection as a context manager + (which would create one in psycopg2). Instead it uses plain assignments + and explicit close(), mirroring the pattern in the single_kernel_postgresql + set_up_database helper. """ + if not self.is_primary: + return True + + if not self.primary_endpoint: + logger.debug("Primary endpoint not yet available; skipping temp tablespace check") + return True + + # Do not migrate the temp tablespace while cross-cluster async replication is + # active. The DROP/CREATE TABLESPACE generates WAL that is streamed to the + # standby cluster. If the standby has not been upgraded to the versioned + # storage layout yet, it will not have the TEMP_DATA_DIR directory, causing + # PostgreSQL to crash with "FATAL: directory does not exist" during WAL replay. + if self.async_replication._relation is not None: + logger.debug("Skipping temp tablespace migration while async replication is active") + return True + connection = None cursor = None try: @@ -722,32 +749,13 @@ def _ensure_temp_tablespace_location(self) -> bool: ) row = cursor.fetchone() if row is None: - # The tablespace may have been dropped by a previous partially-failed - # migration (DROP succeeded, CREATE failed). If the versioned directory - # already exists, recreate the tablespace there. - pg_data_dir = Path(TEMP_DATA_DIR) - if pg_data_dir.exists(): - self._clear_pg_version_dirs(pg_data_dir) - cursor.execute(f"CREATE TABLESPACE temp LOCATION '{TEMP_DATA_DIR}';") - cursor.execute("GRANT CREATE ON TABLESPACE temp TO public;") + # Tablespace was already dropped by a previous migration or was + # never created (e.g. fresh deploy). Nothing to migrate. return True current_location = row[0] if current_location == TEMP_DATA_DIR: - # After a tmpfs wipe TEMP_DATA_DIR is recreated empty. PostgreSQL will - # not be able to use the tablespace until its internal PG__/ - # directory structure has been recreated. Detect this and reinitialise - # by dropping and recreating the tablespace. - pg_data_dir = Path(TEMP_DATA_DIR) - if pg_data_dir.exists() and not any( - d.is_dir() and d.name.startswith("PG_") for d in pg_data_dir.iterdir() - ): - logger.info( - "Temp tablespace directory is empty after tmpfs wipe; reinitialising" - ) - cursor.execute("DROP TABLESPACE temp;") - cursor.execute(f"CREATE TABLESPACE temp LOCATION '{TEMP_DATA_DIR}';") - cursor.execute("GRANT CREATE ON TABLESPACE temp TO public;") + # Already at the versioned path. Nothing to migrate. return True if current_location != TEMP_STORAGE_PATH: @@ -780,26 +788,6 @@ def _ensure_temp_tablespace_location(self) -> bool: return True - def _ensure_temp_tablespace_location_if_primary(self) -> bool: - """Ensure the temp tablespace is migrated when this unit is the primary.""" - if not self.is_primary: - return True - - if not self.primary_endpoint: - logger.debug("Primary endpoint not yet available; skipping temp tablespace check") - return True - - # Do not migrate the temp tablespace while cross-cluster async replication is - # active. The DROP/CREATE TABLESPACE generates WAL that is streamed to the - # standby cluster. If the standby has not been upgraded to the versioned - # storage layout yet, it will not have the TEMP_DATA_DIR directory, causing - # PostgreSQL to crash with "FATAL: directory does not exist" during WAL replay. - if self.async_replication._relation is not None: - logger.debug("Skipping temp tablespace migration while async replication is active") - return True - - return self._ensure_temp_tablespace_location() - @cached_property def postgresql(self) -> PostgreSQL: """Returns an instance of the object used to interact with the database.""" @@ -1153,10 +1141,6 @@ def _on_peer_relation_changed(self, event: HookEvent): self._start_stop_pgbackrest_service(event) - if not self._ensure_temp_tablespace_location_if_primary(): - event.defer() - return - if not self._handle_s3_initialization(event): return @@ -2198,9 +2182,6 @@ def _on_update_status(self, event) -> None: if self.unit.is_leader() and not self._reconfigure_cluster(event): return - if not self._ensure_temp_tablespace_location_if_primary(): - return - self.postgresql_client_relation.oversee_users() if self.primary_endpoint: self._update_relation_endpoints() diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 2f35b83fb2..94da35c6d7 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -667,53 +667,108 @@ def test_ensure_storage_layout(harness, tmp_path): assert not (tmp_path / "logs").exists() -def test_ensure_temp_tablespace_location_recovers_dropped_tablespace(harness, tmp_path): - """If the tablespace was previously dropped but TEMP_DATA_DIR exists, recreate it.""" +def test_migrate_temp_tablespace_location_skips_when_not_primary(harness): + """If the unit is not primary, the migration is skipped.""" + with ( + patch( + "charm.PostgresqlOperatorCharm.is_primary", + new_callable=PropertyMock, + return_value=False, + ), + ): + result = harness.charm._migrate_temp_tablespace_location() + + assert result is True + + +def test_migrate_temp_tablespace_location_skips_when_no_endpoint(harness): + """If primary_endpoint is not yet set, the migration is skipped.""" + with ( + patch( + "charm.PostgresqlOperatorCharm.is_primary", + new_callable=PropertyMock, + return_value=True, + ), + patch( + "charm.PostgresqlOperatorCharm.primary_endpoint", + new_callable=PropertyMock, + return_value=None, + ), + ): + result = harness.charm._migrate_temp_tablespace_location() + + assert result is True + + +def test_migrate_temp_tablespace_location_migrates_from_old_path(harness, tmp_path): + """When temp tablespace is at old TEMP_STORAGE_PATH, it is migrated to TEMP_DATA_DIR.""" temp_data_dir = tmp_path / "temp" / "16" / "main" + temp_storage_path = str(tmp_path / "temp") temp_data_dir.mkdir(parents=True) stale_pg_dir = temp_data_dir / "PG_16_202307071" stale_pg_dir.mkdir() connection = MagicMock() cursor = MagicMock() - cursor.fetchone.return_value = None # tablespace was previously dropped + cursor.fetchone.return_value = (temp_storage_path,) # still at old path connection.cursor.return_value = cursor postgresql = MagicMock() postgresql._connect_to_database.return_value = connection with ( + patch( + "charm.PostgresqlOperatorCharm.is_primary", + new_callable=PropertyMock, + return_value=True, + ), + patch( + "charm.PostgresqlOperatorCharm.primary_endpoint", + new_callable=PropertyMock, + return_value="10.0.0.1", + ), patch( "charm.PostgresqlOperatorCharm.postgresql", new_callable=PropertyMock, return_value=postgresql, ), patch("charm.TEMP_DATA_DIR", str(temp_data_dir)), + patch("charm.TEMP_STORAGE_PATH", temp_storage_path), ): - assert harness.charm._ensure_temp_tablespace_location() + assert harness.charm._migrate_temp_tablespace_location() - # Stale PG version directory should have been removed + # Stale PG version directory should have been removed before CREATE assert not stale_pg_dir.exists() cursor.execute.assert_has_calls([ call("SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';"), + call("DROP TABLESPACE temp;"), call(f"CREATE TABLESPACE temp LOCATION '{temp_data_dir}';"), call("GRANT CREATE ON TABLESPACE temp TO public;"), ]) -def test_ensure_temp_tablespace_location_reinitialises_after_tmpfs_wipe(harness, tmp_path): - """If tablespace dir is empty (tmpfs wipe), DROP+CREATE to reinitialise it.""" +def test_migrate_temp_tablespace_location_skips_when_already_at_versioned_path(harness, tmp_path): + """When temp tablespace is already at TEMP_DATA_DIR, no migration is performed.""" temp_data_dir = tmp_path / "temp" / "16" / "main" temp_data_dir.mkdir(parents=True) - # No PG_version/ directory — simulates an empty dir after tmpfs wipe connection = MagicMock() cursor = MagicMock() - cursor.fetchone.return_value = (str(temp_data_dir),) # tablespace exists at correct location + cursor.fetchone.return_value = (str(temp_data_dir),) # already at versioned path connection.cursor.return_value = cursor postgresql = MagicMock() postgresql._connect_to_database.return_value = connection with ( + patch( + "charm.PostgresqlOperatorCharm.is_primary", + new_callable=PropertyMock, + return_value=True, + ), + patch( + "charm.PostgresqlOperatorCharm.primary_endpoint", + new_callable=PropertyMock, + return_value="10.0.0.1", + ), patch( "charm.PostgresqlOperatorCharm.postgresql", new_callable=PropertyMock, @@ -721,58 +776,88 @@ def test_ensure_temp_tablespace_location_reinitialises_after_tmpfs_wipe(harness, ), patch("charm.TEMP_DATA_DIR", str(temp_data_dir)), ): - assert harness.charm._ensure_temp_tablespace_location() - - cursor.execute.assert_has_calls([ - call("SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';"), - call("DROP TABLESPACE temp;"), - call(f"CREATE TABLESPACE temp LOCATION '{temp_data_dir}';"), - call("GRANT CREATE ON TABLESPACE temp TO public;"), - ]) + assert harness.charm._migrate_temp_tablespace_location() + # Only the SELECT should have been executed — no DROP/CREATE + cursor.execute.assert_called_once_with( + "SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';" + ) -def test_ensure_temp_tablespace_location_skips_reinit_when_pg_dir_present(harness, tmp_path): - """If PG_version/ already exists in tablespace dir, no DROP+CREATE is performed.""" - temp_data_dir = tmp_path / "temp" / "16" / "main" - temp_data_dir.mkdir(parents=True) - (temp_data_dir / "PG_16_202307071").mkdir() # directory already initialised +def test_migrate_temp_tablespace_location_skips_when_tablespace_missing(harness, tmp_path): + """When the tablespace doesn't exist in pg_catalog, no migration is needed.""" connection = MagicMock() cursor = MagicMock() - cursor.fetchone.return_value = (str(temp_data_dir),) + cursor.fetchone.return_value = None # tablespace was never created or already dropped connection.cursor.return_value = cursor postgresql = MagicMock() postgresql._connect_to_database.return_value = connection with ( + patch( + "charm.PostgresqlOperatorCharm.is_primary", + new_callable=PropertyMock, + return_value=True, + ), + patch( + "charm.PostgresqlOperatorCharm.primary_endpoint", + new_callable=PropertyMock, + return_value="10.0.0.1", + ), patch( "charm.PostgresqlOperatorCharm.postgresql", new_callable=PropertyMock, return_value=postgresql, ), - patch("charm.TEMP_DATA_DIR", str(temp_data_dir)), ): - assert harness.charm._ensure_temp_tablespace_location() + assert harness.charm._migrate_temp_tablespace_location() - # Only the SELECT should have been executed — no DROP/CREATE + # Only the SELECT should have been executed cursor.execute.assert_called_once_with( "SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';" ) -def test_ensure_storage_layout_recreates_temp_dir_on_reboot(harness, tmp_path): - """TEMP_DATA_DIR is recreated after a tmpfs wipe on reboot.""" - temp_root = tmp_path / "temp" / "16" / "main" +def test_migrate_temp_tablespace_location_skips_when_unexpected_location(harness, tmp_path): + """When the tablespace is at an unexpected location, migration is skipped with a warning.""" + connection = MagicMock() + cursor = MagicMock() + cursor.fetchone.return_value = ("/some/unexpected/path",) + connection.cursor.return_value = cursor + postgresql = MagicMock() + postgresql._connect_to_database.return_value = connection + with ( - patch("charm.TEMP_DATA_DIR", str(temp_root)), - patch("charm.shutil"), + patch( + "charm.PostgresqlOperatorCharm.is_primary", + new_callable=PropertyMock, + return_value=True, + ), + patch( + "charm.PostgresqlOperatorCharm.primary_endpoint", + new_callable=PropertyMock, + return_value="10.0.0.1", + ), + patch( + "charm.PostgresqlOperatorCharm.postgresql", + new_callable=PropertyMock, + return_value=postgresql, + ), + patch("charm.logger") as logger, ): - harness.charm._ensure_storage_layout() - assert temp_root.is_dir() + assert harness.charm._migrate_temp_tablespace_location() + + cursor.execute.assert_called_once_with( + "SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';" + ) + logger.warning.assert_called_once() -def test_ensure_temp_tablespace_location_if_primary_skips_when_no_endpoint(harness): - """If primary_endpoint is not yet set, the tablespace check is skipped (not deferred).""" +def test_migrate_temp_tablespace_location_returns_false_on_db_error(harness): + """When a psycopg2 error occurs, the method returns False.""" + postgresql = MagicMock() + postgresql._connect_to_database.side_effect = psycopg2.Error("connection failed") + with ( patch( "charm.PostgresqlOperatorCharm.is_primary", @@ -782,16 +867,26 @@ def test_ensure_temp_tablespace_location_if_primary_skips_when_no_endpoint(harne patch( "charm.PostgresqlOperatorCharm.primary_endpoint", new_callable=PropertyMock, - return_value=None, + return_value="10.0.0.1", ), patch( - "charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location" - ) as _ensure_temp_tablespace_location, + "charm.PostgresqlOperatorCharm.postgresql", + new_callable=PropertyMock, + return_value=postgresql, + ), ): - result = harness.charm._ensure_temp_tablespace_location_if_primary() + assert not harness.charm._migrate_temp_tablespace_location() - assert result is True - _ensure_temp_tablespace_location.assert_not_called() + +def test_ensure_storage_layout_recreates_temp_dir_on_reboot(harness, tmp_path): + """TEMP_DATA_DIR is recreated after a tmpfs wipe on reboot.""" + temp_root = tmp_path / "temp" / "16" / "main" + with ( + patch("charm.TEMP_DATA_DIR", str(temp_root)), + patch("charm.shutil"), + ): + harness.charm._ensure_storage_layout() + assert temp_root.is_dir() def test_on_update_status(harness): @@ -829,9 +924,6 @@ def test_on_update_status(harness): patch("charm.PostgresqlOperatorCharm.update_config") as _update_config, patch("charm.PostgresqlOperatorCharm.log_pitr_last_transaction_time"), patch("charm.PostgreSQL.drop_hba_triggers") as _drop_hba_triggers, - patch( - "charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location", return_value=True - ) as _ensure_temp_tablespace_location, ): rel_id = harness.model.get_relation(PEER).id # Test before the cluster is initialised. @@ -876,11 +968,9 @@ def test_on_update_status(harness): {"cluster_initialised": "True", "restoring-backup": "", "restore-to-time": ""}, ) harness.charm.unit.status = ActiveStatus() - _ensure_temp_tablespace_location.reset_mock() harness.charm.on.update_status.emit() _set_primary_status_message.assert_called_once() _reconfigure_cluster.assert_called_once() - _ensure_temp_tablespace_location.assert_called_once() # Test call to restart when the member is isolated from the cluster. _set_primary_status_message.reset_mock() @@ -917,35 +1007,6 @@ def test_on_update_status_skips_remainder_when_reconfigure_cluster_fails(harness _oversee_users.assert_not_called() -def test_on_update_status_skips_remainder_when_temp_tablespace_migration_fails(harness): - with ( - patch("charm.PostgresqlOperatorCharm._can_run_on_update_status", return_value=True), - patch("charm.PostgresqlOperatorCharm._handle_processes_failures", return_value=False), - patch("charm.PostgresqlOperatorCharm._reconfigure_cluster", return_value=True), - patch( - "charm.PostgresqlOperatorCharm.is_primary", - new_callable=PropertyMock, - return_value=True, - ), - patch( - "charm.PostgresqlOperatorCharm.primary_endpoint", - new_callable=PropertyMock, - return_value="10.0.0.1", - ), - patch( - "charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location", return_value=False - ) as _ensure_temp_tablespace_location, - patch("charm.PostgreSQLProvider.oversee_users") as _oversee_users, - ): - with harness.hooks_disabled(): - harness.set_leader() - - harness.charm.on.update_status.emit() - - _ensure_temp_tablespace_location.assert_called_once() - _oversee_users.assert_not_called() - - def test_on_update_status_after_restore_operation(harness): with ( patch("charm.ClusterTopologyObserver.start_observer"), @@ -978,7 +1039,6 @@ def test_on_update_status_after_restore_operation(harness): "charm.PostgresqlOperatorCharm.enable_disable_extensions" ) as _enable_disable_extensions, patch("charm.PostgreSQL.drop_hba_triggers") as _drop_hba_triggers, - patch("charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location", return_value=True), ): _get_current_timeline.return_value = "2" rel_id = harness.model.get_relation(PEER).id @@ -2048,10 +2108,6 @@ def test_on_peer_relation_changed(harness): patch("charm.PostgresqlOperatorCharm.is_primary") as _is_primary, patch("charm.Patroni.member_started", new_callable=PropertyMock) as _member_started, patch("charm.Patroni.start_patroni") as _start_patroni, - patch( - "charm.PostgresqlOperatorCharm._ensure_temp_tablespace_location_if_primary", - return_value=True, - ) as _ensure_temp_tablespace_location_if_primary, patch("charm.PostgresqlOperatorCharm.update_config") as _update_config, patch("charm.PostgresqlOperatorCharm._update_member_ip") as _update_member_ip, patch("charm.PostgresqlOperatorCharm._reconfigure_cluster") as _reconfigure_cluster, @@ -2096,7 +2152,6 @@ def test_on_peer_relation_changed(harness): _reconfigure_cluster.assert_called_once_with(mock_event) _update_config.assert_called_once() _start_patroni.assert_called_once() - _ensure_temp_tablespace_location_if_primary.assert_called_once() _update_new_unit_status.assert_called_once() # Test when the unit fails to update the Patroni configuration. From 48a4a6139c6a557b8cd7b1626265d60def50e2bc Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 19 May 2026 16:29:47 -0300 Subject: [PATCH 09/19] docs: document pre-refresh hook handling of temp tablespace rollback Update docstrings in _migrate_temp_tablespace_location and _ensure_storage_layout to note that the reverse catalog migration for the temp tablespace is handled by the snap's pre-refresh hook during rollback, not by the charm itself. The charm's existing forward-only migration remains unchanged. Signed-off-by: Marcelo Henrique Neppel --- refresh_versions.toml | 7 ++++--- src/charm.py | 15 +++++++++++++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/refresh_versions.toml b/refresh_versions.toml index f27f3c3b02..da2fef2e46 100644 --- a/refresh_versions.toml +++ b/refresh_versions.toml @@ -1,13 +1,14 @@ charm_major = 1 workload = "16.13" +charm = "16/1.999.0" # Unique version for upgrade test [snap] name = "charmed-postgresql" [snap.revisions] # amd64 -x86_64 = "313" +x86_64 = "323" # arm64 (Linux) -aarch64 = "314" +aarch64 = "322" # arm64 (macOS / Apple Silicon - same snap revision) -arm64 = "314" +arm64 = "322" diff --git a/src/charm.py b/src/charm.py index c0cef70962..859e78c7d5 100755 --- a/src/charm.py +++ b/src/charm.py @@ -684,10 +684,17 @@ def _ensure_storage_layout(self) -> None: a tmpfs mount that is wiped on reboot, so we recreate it unconditionally. CREATE TABLESPACE requires the directory to be writable by the PostgreSQL _daemon_ user, so we chown it. + + The 16/ parent dir must also be _daemon_-owned: the snap daemon + runs as _daemon_ and needs write permission to clean up the + versioned subdirectory and run DROP/CREATE TABLESPACE during + rollback (handled by the snap's pre-refresh hook). """ temp_dir = Path(TEMP_DATA_DIR) temp_dir.mkdir(parents=True, exist_ok=True) shutil.chown(temp_dir, user=SNAP_DAEMON_USER, group=SNAP_DAEMON_USER) + if temp_dir.parent.exists(): + shutil.chown(temp_dir.parent, user=SNAP_DAEMON_USER, group=SNAP_DAEMON_USER) @staticmethod def _clear_pg_version_dirs(path: Path) -> None: @@ -705,11 +712,15 @@ def _clear_pg_version_dirs(path: Path) -> None: def _migrate_temp_tablespace_location(self) -> bool: """One-shot migration of the temp tablespace to the versioned directory. - During a snap upgrade, the snap hooks migrate temp data from the old - non-versioned storage root (TEMP_STORAGE_PATH) to the versioned + During a snap upgrade, the post-refresh hook migrates temp data from the + old non-versioned storage root (TEMP_STORAGE_PATH) to the versioned subdirectory (TEMP_DATA_DIR). This method updates the PostgreSQL catalog entry to match. + During a snap downgrade (rollback), the pre-refresh hook handles both + file migration and catalog migration (DROP/CREATE TABLESPACE) back to + the non-versioned root. This method only handles the forward case. + Other temp tablespace recovery scenarios (missing catalog entry after a partially-failed migration, empty directory after a tmpfs wipe) are handled by the single-kernel library's set_up_database during the From cea5cc3e2d107f848aa35bcd3782e18dd42626c8 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 20 May 2026 17:02:49 -0300 Subject: [PATCH 10/19] fix(snap): update amd64 snap revision to 329 (fixes SNAP_CURRENT bug) Snap rev 323 had an unbound SNAP_CURRENT variable in migrate-data.sh that silently crashed the data migration daemon. Rev 329 is built from the corrected source that uses SNAP_DATA. Signed-off-by: Marcelo Henrique Neppel --- refresh_versions.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refresh_versions.toml b/refresh_versions.toml index da2fef2e46..0cd56c3b4d 100644 --- a/refresh_versions.toml +++ b/refresh_versions.toml @@ -7,7 +7,7 @@ name = "charmed-postgresql" [snap.revisions] # amd64 -x86_64 = "323" +x86_64 = "329" # arm64 (Linux) aarch64 = "322" # arm64 (macOS / Apple Silicon - same snap revision) From e2ae844364fa4643192bb676fdd82ac9dcd77366 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 20 May 2026 17:09:40 -0300 Subject: [PATCH 11/19] fix: remove unused imports Signed-off-by: Marcelo Henrique Neppel --- tests/unit/test_charm.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 31a3a97491..f6a1d5eb5d 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -53,8 +53,6 @@ from constants import ( PEER, POSTGRESQL_DATA_DIR, - POSTGRESQL_DATA_PATH, - RAFT_PORT, SECRET_INTERNAL_LABEL, UPDATE_CERTS_BIN_PATH, ) From 93e86a40bd3d986edcd9028a11d2104d982c4aa8 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 20 May 2026 23:04:00 -0300 Subject: [PATCH 12/19] fix: update arm64 snap revision to 330 for versioned storage layout fix The previous arm64 snap revision (322) had the SNAP_CURRENT unbound variable bug in migrate-data.sh, causing upgrade failures on arm64. Signed-off-by: Marcelo Henrique Neppel --- refresh_versions.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/refresh_versions.toml b/refresh_versions.toml index 0cd56c3b4d..f625e5e140 100644 --- a/refresh_versions.toml +++ b/refresh_versions.toml @@ -9,6 +9,6 @@ name = "charmed-postgresql" # amd64 x86_64 = "329" # arm64 (Linux) -aarch64 = "322" +aarch64 = "330" # arm64 (macOS / Apple Silicon - same snap revision) -arm64 = "322" +arm64 = "330" From b5558f1683f73e6a547dc332e244b2315f7696dc Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 21 May 2026 09:56:41 -0300 Subject: [PATCH 13/19] fix(tests): increase sync_standby retry timeout in stereo mode primary shutdown test The 180s timeout was insufficient on CI where a new replica joining after a force-destroyed primary can take longer to bootstrap via basebackup and register in the Raft cluster. Increased to 600s. Signed-off-by: Marcelo Henrique Neppel --- tests/integration/ha_tests/test_stereo_mode.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/ha_tests/test_stereo_mode.py b/tests/integration/ha_tests/test_stereo_mode.py index ec91592b6e..01646710ad 100644 --- a/tests/integration/ha_tests/test_stereo_mode.py +++ b/tests/integration/ha_tests/test_stereo_mode.py @@ -385,7 +385,7 @@ async def test_primary_shutdown_with_watcher(ops_test: OpsTest, continuous_write # Wait for the new replica to become a sync_standby # This can take a while as the new unit needs to fully sync and be recognized - for attempt in Retrying(stop=stop_after_delay(180), wait=wait_fixed(10), reraise=True): + for attempt in Retrying(stop=stop_after_delay(600), wait=wait_fixed(10), reraise=True): with attempt: final_roles = await get_cluster_roles( ops_test, ops_test.model.applications[DATABASE_APP_NAME].units[0].name From 7e2d815505104f2193397ea4d8fcdf4abb2bc1aa Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 21 May 2026 16:44:09 -0300 Subject: [PATCH 14/19] fix(tests): increase verify_raft_cluster_health retry timeout for watcher network recovery Signed-off-by: Marcelo Henrique Neppel --- tests/integration/ha_tests/test_stereo_mode.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/ha_tests/test_stereo_mode.py b/tests/integration/ha_tests/test_stereo_mode.py index 01646710ad..33f9c7f9da 100644 --- a/tests/integration/ha_tests/test_stereo_mode.py +++ b/tests/integration/ha_tests/test_stereo_mode.py @@ -92,7 +92,7 @@ async def verify_raft_cluster_health( assert return_code == 0, f"Failed to get watcher address from {watcher_unit.name}" watcher_ip = watcher_ip.strip() - for attempt in Retrying(stop=stop_after_delay(180), wait=wait_fixed(5), reraise=True): + for attempt in Retrying(stop=stop_after_delay(600), wait=wait_fixed(5), reraise=True): with attempt: for unit in ops_test.model.applications[db_app_name].units: # Get the Raft password from Patroni config using juju exec directly From ba2f865e51262bc4ecee52dae2a5c8e169de3f20 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Mon, 25 May 2026 07:41:49 -0300 Subject: [PATCH 15/19] feat: add versioned storage layout migration for temp tablespace Migrate the temp tablespace catalog entry to the versioned 16/main subdirectory during rolling upgrades, with a Patroni settling retry to handle transient primary endpoint resolution after snap refresh. Restrict migration to the lowest unit to avoid duplicate DDL in multi-unit clusters. Add pre-refresh checks for active temp tablespace objects and update snap revisions (amd64: 345, arm64: 344). Signed-off-by: Marcelo Henrique Neppel --- refresh_versions.toml | 9 +- src/charm.py | 137 ++++++++++-------- .../high_availability_helpers_new.py | 15 +- tests/unit/test_charm.py | 37 ----- 4 files changed, 82 insertions(+), 116 deletions(-) diff --git a/refresh_versions.toml b/refresh_versions.toml index f625e5e140..b2c127d22b 100644 --- a/refresh_versions.toml +++ b/refresh_versions.toml @@ -1,14 +1,11 @@ charm_major = 1 workload = "16.13" -charm = "16/1.999.0" # Unique version for upgrade test [snap] name = "charmed-postgresql" [snap.revisions] # amd64 -x86_64 = "329" -# arm64 (Linux) -aarch64 = "330" -# arm64 (macOS / Apple Silicon - same snap revision) -arm64 = "330" +x86_64 = "345" +# arm64 +aarch64 = "344" diff --git a/src/charm.py b/src/charm.py index 543e397d15..a67a559ec3 100755 --- a/src/charm.py +++ b/src/charm.py @@ -173,9 +173,30 @@ class StorageUnavailableError(Exception): class _PostgreSQLRefresh(charm_refresh.CharmSpecificMachines): _charm: "PostgresqlOperatorCharm" - @staticmethod - def run_pre_refresh_checks_after_1_unit_refreshed() -> None: - pass + def _check_temp_tablespace_objects(self) -> None: + try: + connection = self._charm.postgresql._connect_to_database() + connection.autocommit = True + cursor = connection.cursor() + cursor.execute( + "SELECT count(*) FROM pg_class WHERE reltablespace = " + "(SELECT oid FROM pg_tablespace WHERE spcname = 'temp');" + ) + count = cursor.fetchone()[0] + cursor.close() + connection.close() + if count > 0: + raise charm_refresh.PrecheckFailed( + f"Temp tablespace has {count} active object(s). " + "Please ensure no sessions are using temp tables before refreshing." + ) + except charm_refresh.PrecheckFailed: + raise + except Exception: + logger.debug("Unable to check temp tablespace objects", exc_info=True) + + def run_pre_refresh_checks_after_1_unit_refreshed(self) -> None: + self._check_temp_tablespace_objects() def run_pre_refresh_checks_before_any_units_refreshed(self) -> None: for attempt in Retrying(stop=stop_after_attempt(2), wait=wait_fixed(1), reraise=True): @@ -184,6 +205,7 @@ def run_pre_refresh_checks_before_any_units_refreshed(self) -> None: raise charm_refresh.PrecheckFailed("PostgreSQL is not running on 1+ units") if self._charm._patroni.is_creating_backup: raise charm_refresh.PrecheckFailed("Backup in progress") + self._check_temp_tablespace_objects() # Switch primary to last unit to refresh @@ -252,12 +274,6 @@ def refresh_snap( self._charm.set_unit_status(MaintenanceStatus("updating configuration"), refresh=refresh) self._charm.update_config(refresh=refresh) - # Ensure the temp tablespace directory exists before the snap refresh. - # Data migration between storage roots and versioned paths is handled - # by the snap's post-refresh hook (forward) and pre-refresh hook - # (reverse), which start a one-shot daemon running as _daemon_. - self._charm._ensure_storage_layout() - # TODO add graceful shutdown before refreshing snap? # TODO future improvement: if snap refresh fails (i.e. same snap revision installed) after # graceful shutdown, restart workload @@ -399,6 +415,7 @@ def __init__(self, *args): if self.refresh.in_progress: self._post_snap_refresh(self.refresh) else: + self._migrate_temp_tablespace_location() self.refresh.next_unit_allowed_to_refresh = True self._observer.start_observer() @@ -442,7 +459,6 @@ def _post_snap_refresh(self, refresh: charm_refresh.Machines): Called after snap refresh """ self._check_and_update_internal_cert() - self._ensure_storage_layout() if not self._patroni.start_patroni(): self.set_unit_status(BlockedStatus("Failed to start PostgreSQL"), refresh=refresh) @@ -473,12 +489,19 @@ def _post_snap_refresh(self, refresh: charm_refresh.Machines): "Did not allow next unit to refresh: member not ready or not joined the cluster yet" ) else: - self._migrate_temp_tablespace_location() try: self._patroni.set_max_timelines_history() except Exception: logger.warning("Unable to patch in max_timelines_history") + peer_relation = self.model.get_relation("database-peers") + all_units = sorted( + [self.unit, *(peer_relation.units if peer_relation else [])], + key=lambda u: int(u.name.split("/")[1]), + ) + if self.unit == all_units[0]: + self._migrate_temp_tablespace_location() refresh.next_unit_allowed_to_refresh = True + self.set_unit_status(ActiveStatus(), refresh=refresh) def set_unit_status( self, status: ops.StatusBase, /, *, refresh: charm_refresh.Machines | None = None @@ -689,18 +712,33 @@ def _ensure_storage_layout(self) -> None: if temp_dir.parent.exists(): shutil.chown(temp_dir.parent, user=SNAP_DAEMON_USER, group=SNAP_DAEMON_USER) - @staticmethod - def _clear_pg_version_dirs(path: Path) -> None: - """Remove PostgreSQL version subdirectories (PG__) from a directory. + def _resolve_primary_host(self) -> str | None: + """Wait for Patroni to settle and return the primary host. - These directories are created by PostgreSQL when a tablespace is created and must not - exist at a target path before CREATE TABLESPACE is called. Temp tablespace data is - ephemeral, so removal is safe. + After a snap refresh, Patroni may briefly report this unit as the + primary before discovering the real cluster topology. Retry until + primary_endpoint points to a different host or this unit truly is + the primary. """ - if path.exists(): - for entry in path.iterdir(): - if entry.name.startswith("PG_"): - shutil.rmtree(entry) + try: + for attempt in Retrying(stop=stop_after_delay(60), wait=wait_fixed(3)): + with attempt: + target_host = self.primary_endpoint + if not target_host: + raise Exception("primary_endpoint not available yet") + if target_host != self._unit_ip or self.is_primary: + return target_host + logger.info( + "primary_endpoint=%s matches unit_ip but unit is not primary; " + "Patroni may not have settled yet (attempt %d)", + target_host, + attempt.retry_state.attempt_number, + ) + raise Exception("Patroni not settled yet") + except RetryError: + logger.warning("Patroni did not settle within 60s") + return None + return None def _migrate_temp_tablespace_location(self) -> bool: """One-shot migration of the temp tablespace to the versioned directory. @@ -714,38 +752,29 @@ def _migrate_temp_tablespace_location(self) -> bool: file migration and catalog migration (DROP/CREATE TABLESPACE) back to the non-versioned root. This method only handles the forward case. - Other temp tablespace recovery scenarios (missing catalog entry after a - partially-failed migration, empty directory after a tmpfs wipe) are - handled by the single-kernel library's set_up_database during the - leader-elected event. - DROP TABLESPACE and CREATE TABLESPACE cannot run inside a transaction block, so this method avoids using the connection as a context manager (which would create one in psycopg2). Instead it uses plain assignments and explicit close(), mirroring the pattern in the single_kernel_postgresql set_up_database helper. """ - if not self.is_primary: - return True - if not self.primary_endpoint: logger.debug("Primary endpoint not yet available; skipping temp tablespace check") return True - # Do not migrate the temp tablespace while cross-cluster async replication is - # active. The DROP/CREATE TABLESPACE generates WAL that is streamed to the - # standby cluster. If the standby has not been upgraded to the versioned - # storage layout yet, it will not have the TEMP_DATA_DIR directory, causing - # PostgreSQL to crash with "FATAL: directory does not exist" during WAL replay. if self.async_replication._relation is not None: logger.debug("Skipping temp tablespace migration while async replication is active") return True + target_host = self._resolve_primary_host() + if target_host is None: + return False + connection = None cursor = None try: - connection = self.postgresql._connect_to_database() - connection.autocommit = True # DROP/CREATE TABLESPACE cannot run inside a transaction + connection = self.postgresql._connect_to_database(database_host=target_host) + connection.autocommit = True cursor = connection.cursor() cursor.execute( @@ -753,13 +782,10 @@ def _migrate_temp_tablespace_location(self) -> bool: ) row = cursor.fetchone() if row is None: - # Tablespace was already dropped by a previous migration or was - # never created (e.g. fresh deploy). Nothing to migrate. return True current_location = row[0] if current_location == TEMP_DATA_DIR: - # Already at the versioned path. Nothing to migrate. return True if current_location != TEMP_STORAGE_PATH: @@ -778,7 +804,6 @@ def _migrate_temp_tablespace_location(self) -> bool: TEMP_DATA_DIR, ) cursor.execute("DROP TABLESPACE temp;") - self._clear_pg_version_dirs(Path(TEMP_DATA_DIR)) cursor.execute(f"CREATE TABLESPACE temp LOCATION '{TEMP_DATA_DIR}';") cursor.execute("GRANT CREATE ON TABLESPACE temp TO public;") except psycopg2.Error: @@ -1140,7 +1165,15 @@ def _on_peer_relation_changed(self, event: HookEvent): event.defer() return - if not self._check_member_registration(event): + # In Raft mode with a watcher, ensure this member is properly registered in the DCS. + # A new member may be running but not registered if it was added to Raft after starting. + if ( + self.watcher_offer.is_watcher_connected + and not self._patroni.is_member_registered_in_cluster() + ): + logger.info("Member running but not registered in Raft cluster - restarting Patroni") + self._patroni.restart_patroni() + event.defer() return self._start_stop_pgbackrest_service(event) @@ -1156,23 +1189,6 @@ def _on_peer_relation_changed(self, event: HookEvent): self._update_new_unit_status() - # Split off into separate function, because of complexity _on_peer_relation_changed - def _check_member_registration(self, event: HookEvent) -> bool: - """Check and ensure the member is registered in the Raft/replication cluster. - - Returns: - True if processing should continue, False if we should return early. - """ - if ( - self.watcher_offer.is_watcher_connected - and not self._patroni.is_member_registered_in_cluster() - ): - logger.info("Member running but not registered in Raft cluster - restarting Patroni") - self._patroni.restart_patroni() - event.defer() - return False - return True - # Split off into separate function, because of complexity _on_peer_relation_changed def _handle_s3_initialization(self, event: HookEvent) -> bool: """Handle S3 initialization during peer relation changes. @@ -2149,7 +2165,7 @@ def promote_primary_unit(self, event: ActionEvent) -> None: except SwitchoverFailedError: event.fail("Switchover failed or timed out, check the logs for details") - def _on_update_status(self, event) -> None: + def _on_update_status(self, _) -> None: """Update the unit status message and users list in the database.""" if not self._can_run_on_update_status(): return @@ -2162,9 +2178,6 @@ def _on_update_status(self, event) -> None: if self._handle_processes_failures(): return - if self.unit.is_leader() and not self._reconfigure_cluster(event): - return - self.postgresql_client_relation.oversee_users() if self.primary_endpoint: self._update_relation_endpoints() diff --git a/tests/integration/high_availability/high_availability_helpers_new.py b/tests/integration/high_availability/high_availability_helpers_new.py index 8144be295b..4ed731eb7c 100644 --- a/tests/integration/high_availability/high_availability_helpers_new.py +++ b/tests/integration/high_availability/high_availability_helpers_new.py @@ -22,11 +22,6 @@ JujuAppsStatusFn = Callable[[Status, str], bool] -def bracket_ipv6(addr: str) -> str: - """Wrap an IPv6 address in brackets for URL use.""" - return f"[{addr}]" if ":" in addr else addr - - def check_db_units_writes_increment( juju: Juju, app_name: str, @@ -183,9 +178,9 @@ def get_db_standby_leader_unit(juju: Juju, app_name: str) -> str: """Get the current standby node of the cluster.""" unit_address = get_unit_ip(juju, app_name, get_app_leader(juju, app_name)) - for member in requests.get( - f"https://{bracket_ipv6(unit_address)}:8008/cluster", verify=False - ).json()["members"]: + for member in requests.get(f"https://{unit_address}:8008/cluster", verify=False).json()[ + "members" + ]: if member["role"] == "standby_leader": return member["name"][::-1].replace("-", "/")[::-1] @@ -260,7 +255,5 @@ def count_switchovers(juju: Juju, app_name: str) -> int: """Return the number of performed switchovers.""" app_primary = get_db_primary_unit(juju, app_name) unit_address = get_unit_ip(juju, app_name, app_primary) - switchover_history_info = requests.get( - f"https://{bracket_ipv6(unit_address)}:8008/history", verify=False - ) + switchover_history_info = requests.get(f"https://{unit_address}:8008/history", verify=False) return len(switchover_history_info.json()) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index f6a1d5eb5d..8f290cd082 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -707,8 +707,6 @@ def test_migrate_temp_tablespace_location_migrates_from_old_path(harness, tmp_pa temp_data_dir = tmp_path / "temp" / "16" / "main" temp_storage_path = str(tmp_path / "temp") temp_data_dir.mkdir(parents=True) - stale_pg_dir = temp_data_dir / "PG_16_202307071" - stale_pg_dir.mkdir() connection = MagicMock() cursor = MagicMock() @@ -738,8 +736,6 @@ def test_migrate_temp_tablespace_location_migrates_from_old_path(harness, tmp_pa ): assert harness.charm._migrate_temp_tablespace_location() - # Stale PG version directory should have been removed before CREATE - assert not stale_pg_dir.exists() cursor.execute.assert_has_calls([ call("SELECT pg_tablespace_location(oid) FROM pg_tablespace WHERE spcname='temp';"), call("DROP TABLESPACE temp;"), @@ -897,9 +893,6 @@ def test_on_update_status(harness): patch( "charm.PostgresqlOperatorCharm._set_primary_status_message" ) as _set_primary_status_message, - patch( - "charm.PostgresqlOperatorCharm._reconfigure_cluster", return_value=True - ) as _reconfigure_cluster, patch("charm.Patroni.restart_patroni") as _restart_patroni, patch("charm.Patroni.is_member_isolated") as _is_member_isolated, patch("charm.Patroni.member_started", new_callable=PropertyMock) as _member_started, @@ -931,7 +924,6 @@ def test_on_update_status(harness): # Test before the cluster is initialised. harness.charm.on.update_status.emit() _set_primary_status_message.assert_not_called() - _reconfigure_cluster.assert_not_called() # Test after the cluster was initialised, but with the unit in a blocked state. with harness.hooks_disabled(): @@ -942,7 +934,6 @@ def test_on_update_status(harness): harness.charm.unit.status = BlockedStatus("fake blocked status") harness.charm.on.update_status.emit() _set_primary_status_message.assert_not_called() - _reconfigure_cluster.assert_not_called() # Test the point-in-time-recovery fail. with harness.hooks_disabled(): @@ -959,7 +950,6 @@ def test_on_update_status(harness): _patroni_logs.return_value = "2022-02-24 02:00:00 UTC patroni.exceptions.PatroniFatalException: Failed to bootstrap cluster" harness.charm.on.update_status.emit() _set_primary_status_message.assert_not_called() - _reconfigure_cluster.assert_not_called() assert harness.charm.unit.status.message == CANNOT_RESTORE_PITR # Test with the unit in a status different that blocked. @@ -972,12 +962,10 @@ def test_on_update_status(harness): harness.charm.unit.status = ActiveStatus() harness.charm.on.update_status.emit() _set_primary_status_message.assert_called_once() - _reconfigure_cluster.assert_called_once() # Test call to restart when the member is isolated from the cluster. _set_primary_status_message.reset_mock() _start_observer.reset_mock() - _reconfigure_cluster.reset_mock() _member_started.return_value = False _is_member_isolated.return_value = True with harness.hooks_disabled(): @@ -985,39 +973,17 @@ def test_on_update_status(harness): rel_id, harness.charm.unit.name, {"postgresql_restarted": ""} ) harness.charm.on.update_status.emit() - _reconfigure_cluster.assert_called_once() _restart_patroni.assert_called_once() _start_observer.assert_called_once_with() _drop_hba_triggers.assert_called_once_with() -def test_on_update_status_skips_remainder_when_reconfigure_cluster_fails(harness): - with ( - patch("charm.PostgresqlOperatorCharm._can_run_on_update_status", return_value=True), - patch("charm.PostgresqlOperatorCharm._handle_processes_failures", return_value=False), - patch( - "charm.PostgresqlOperatorCharm._reconfigure_cluster", return_value=False - ) as _reconfigure_cluster, - patch("charm.PostgreSQLProvider.oversee_users") as _oversee_users, - ): - with harness.hooks_disabled(): - harness.set_leader() - - harness.charm.on.update_status.emit() - - _reconfigure_cluster.assert_called_once() - _oversee_users.assert_not_called() - - def test_on_update_status_after_restore_operation(harness): with ( patch("charm.ClusterTopologyObserver.start_observer"), patch( "charm.PostgresqlOperatorCharm._set_primary_status_message" ) as _set_primary_status_message, - patch( - "charm.PostgresqlOperatorCharm._reconfigure_cluster", return_value=True - ) as _reconfigure_cluster, patch( "charm.PostgresqlOperatorCharm._update_relation_endpoints" ) as _update_relation_endpoints, @@ -1064,7 +1030,6 @@ def test_on_update_status_after_restore_operation(harness): _update_relation_endpoints.assert_not_called() _set_primary_status_message.assert_not_called() _enable_disable_extensions.assert_not_called() - _reconfigure_cluster.assert_not_called() assert isinstance(harness.charm.unit.status, BlockedStatus) # Test when the restore operation hasn't finished yet. @@ -1078,7 +1043,6 @@ def test_on_update_status_after_restore_operation(harness): _update_relation_endpoints.assert_not_called() _set_primary_status_message.assert_not_called() _enable_disable_extensions.assert_not_called() - _reconfigure_cluster.assert_not_called() assert isinstance(harness.charm.unit.status, ActiveStatus) # Assert that the backup id is still in the application relation databag. @@ -1095,7 +1059,6 @@ def test_on_update_status_after_restore_operation(harness): harness.charm.on.update_status.emit() _update_config.assert_called_once() _handle_processes_failures.assert_called_once() - _reconfigure_cluster.assert_called_once() _oversee_users.assert_called_once() _update_relation_endpoints.assert_called_once() _set_primary_status_message.assert_called_once() From 370dedfc0c4b6f1e3d54ba5035cada63a39c30d5 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Mon, 25 May 2026 08:18:03 -0300 Subject: [PATCH 16/19] fix: update snap revisions to 346/347 for versioned storage layout with PostgreSQL 16.14 Signed-off-by: Marcelo Henrique Neppel --- refresh_versions.toml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/refresh_versions.toml b/refresh_versions.toml index b2c127d22b..2f525efd6b 100644 --- a/refresh_versions.toml +++ b/refresh_versions.toml @@ -1,11 +1,11 @@ charm_major = 1 -workload = "16.13" +workload = "16.14" [snap] name = "charmed-postgresql" [snap.revisions] # amd64 -x86_64 = "345" +x86_64 = "346" # arm64 -aarch64 = "344" +aarch64 = "347" From 728e8841f2e4bb2ae3091b7492824828d1624ebe Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Mon, 25 May 2026 08:42:00 -0300 Subject: [PATCH 17/19] test: fix unit test Signed-off-by: Marcelo Henrique Neppel --- tests/unit/test_cluster.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_cluster.py b/tests/unit/test_cluster.py index cd409f7915..2ecafaea0b 100644 --- a/tests/unit/test_cluster.py +++ b/tests/unit/test_cluster.py @@ -141,7 +141,7 @@ def test_get_patroni_health(peers_ips, patroni): def test_get_postgresql_version(peers_ips, patroni): - assert patroni.get_postgresql_version() == "16.13" + assert patroni.get_postgresql_version() == "16.14" def test_dict_to_hba_string(harness, patroni): From 69c96fbc75543015833af6100ec1796c339ff3e2 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Mon, 25 May 2026 11:32:15 -0300 Subject: [PATCH 18/19] fix: resolve stale primary detection and WAL replay crash in temp tablespace migration Query the Patroni REST API directly in _resolve_primary_host instead of reading self.primary_endpoint, which returns stale data from the peer databag after failover during rolling upgrades. Add a retry loop with the `required` parameter at the caller in _post_snap_refresh to ensure the migration completes. Issue a CHECKPOINT after the CREATE TABLESPACE DDL to flush WAL past the record, preventing PostgreSQL crashes on replicas during rollback when the snap's pre-refresh hook deletes the versioned temp directory. Signed-off-by: Marcelo Henrique Neppel --- src/charm.py | 47 ++++++++++++++++++++++++++-------------- tests/unit/test_charm.py | 30 +++++-------------------- 2 files changed, 36 insertions(+), 41 deletions(-) diff --git a/src/charm.py b/src/charm.py index e1e493f5d1..db53194593 100755 --- a/src/charm.py +++ b/src/charm.py @@ -500,7 +500,12 @@ def _post_snap_refresh(self, refresh: charm_refresh.Machines): key=lambda u: int(u.name.split("/")[1]), ) if self.unit == all_units[0]: - self._migrate_temp_tablespace_location() + for attempt in Retrying( + stop=stop_after_delay(180), wait=wait_fixed(5), reraise=True + ): + with attempt: + if not self._migrate_temp_tablespace_location(required=True): + raise Exception("Temp tablespace migration not yet complete") refresh.next_unit_allowed_to_refresh = True self.set_unit_status(ActiveStatus(), refresh=refresh) @@ -717,31 +722,29 @@ def _resolve_primary_host(self) -> str | None: """Wait for Patroni to settle and return the primary host. After a snap refresh, Patroni may briefly report this unit as the - primary before discovering the real cluster topology. Retry until - primary_endpoint points to a different host or this unit truly is - the primary. + primary before discovering the real cluster topology. Query the + Patroni API directly (bypassing primary_endpoint, which can return + stale data from the peer databag) and retry until the primary + points to a different host or this unit truly is the primary. """ try: for attempt in Retrying(stop=stop_after_delay(60), wait=wait_fixed(3)): with attempt: - target_host = self.primary_endpoint + primary = self._patroni.get_primary() + if not primary: + raise Exception("No primary found yet") + target_host = self._patroni.get_member_ip(primary) if not target_host: - raise Exception("primary_endpoint not available yet") + raise Exception("Primary IP not available yet") if target_host != self._unit_ip or self.is_primary: return target_host - logger.info( - "primary_endpoint=%s matches unit_ip but unit is not primary; " - "Patroni may not have settled yet (attempt %d)", - target_host, - attempt.retry_state.attempt_number, - ) raise Exception("Patroni not settled yet") except RetryError: logger.warning("Patroni did not settle within 60s") return None return None - def _migrate_temp_tablespace_location(self) -> bool: + def _migrate_temp_tablespace_location(self, *, required: bool = False) -> bool: """One-shot migration of the temp tablespace to the versioned directory. During a snap upgrade, the post-refresh hook migrates temp data from the @@ -758,19 +761,27 @@ def _migrate_temp_tablespace_location(self) -> bool: (which would create one in psycopg2). Instead it uses plain assignments and explicit close(), mirroring the pattern in the single_kernel_postgresql set_up_database helper. + + Args: + required: If True (used during upgrade), return False when the + primary is unavailable so the caller can retry. If False + (default, used during install), return True to skip gracefully + when no cluster exists yet. """ if not self.primary_endpoint: - logger.debug("Primary endpoint not yet available; skipping temp tablespace check") - return True + return not required if self.async_replication._relation is not None: - logger.debug("Skipping temp tablespace migration while async replication is active") return True target_host = self._resolve_primary_host() if target_host is None: return False + return self._execute_temp_tablespace_migration(target_host) + + def _execute_temp_tablespace_migration(self, target_host: str) -> bool: + """Execute the temp tablespace DDL migration on the given host.""" connection = None cursor = None try: @@ -807,6 +818,10 @@ def _migrate_temp_tablespace_location(self) -> bool: cursor.execute("DROP TABLESPACE temp;") cursor.execute(f"CREATE TABLESPACE temp LOCATION '{TEMP_DATA_DIR}';") cursor.execute("GRANT CREATE ON TABLESPACE temp TO public;") + # Flush WAL past the CREATE TABLESPACE record so replicas won't + # need to replay it during a future rollback (the versioned + # directory may not exist after the snap's pre-refresh hook). + cursor.execute("CHECKPOINT;") except psycopg2.Error: logger.exception("Failed to migrate temp tablespace location") return False diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index f3709f9311..11c4148965 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -716,16 +716,12 @@ def test_migrate_temp_tablespace_location_migrates_from_old_path(harness, tmp_pa postgresql._connect_to_database.return_value = connection with ( - patch( - "charm.PostgresqlOperatorCharm.is_primary", - new_callable=PropertyMock, - return_value=True, - ), patch( "charm.PostgresqlOperatorCharm.primary_endpoint", new_callable=PropertyMock, return_value="10.0.0.1", ), + patch.object(harness.charm, "_resolve_primary_host", return_value="10.0.0.1"), patch( "charm.PostgresqlOperatorCharm.postgresql", new_callable=PropertyMock, @@ -757,16 +753,12 @@ def test_migrate_temp_tablespace_location_skips_when_already_at_versioned_path(h postgresql._connect_to_database.return_value = connection with ( - patch( - "charm.PostgresqlOperatorCharm.is_primary", - new_callable=PropertyMock, - return_value=True, - ), patch( "charm.PostgresqlOperatorCharm.primary_endpoint", new_callable=PropertyMock, return_value="10.0.0.1", ), + patch.object(harness.charm, "_resolve_primary_host", return_value="10.0.0.1"), patch( "charm.PostgresqlOperatorCharm.postgresql", new_callable=PropertyMock, @@ -792,16 +784,12 @@ def test_migrate_temp_tablespace_location_skips_when_tablespace_missing(harness, postgresql._connect_to_database.return_value = connection with ( - patch( - "charm.PostgresqlOperatorCharm.is_primary", - new_callable=PropertyMock, - return_value=True, - ), patch( "charm.PostgresqlOperatorCharm.primary_endpoint", new_callable=PropertyMock, return_value="10.0.0.1", ), + patch.object(harness.charm, "_resolve_primary_host", return_value="10.0.0.1"), patch( "charm.PostgresqlOperatorCharm.postgresql", new_callable=PropertyMock, @@ -826,16 +814,12 @@ def test_migrate_temp_tablespace_location_skips_when_unexpected_location(harness postgresql._connect_to_database.return_value = connection with ( - patch( - "charm.PostgresqlOperatorCharm.is_primary", - new_callable=PropertyMock, - return_value=True, - ), patch( "charm.PostgresqlOperatorCharm.primary_endpoint", new_callable=PropertyMock, return_value="10.0.0.1", ), + patch.object(harness.charm, "_resolve_primary_host", return_value="10.0.0.1"), patch( "charm.PostgresqlOperatorCharm.postgresql", new_callable=PropertyMock, @@ -857,16 +841,12 @@ def test_migrate_temp_tablespace_location_returns_false_on_db_error(harness): postgresql._connect_to_database.side_effect = psycopg2.Error("connection failed") with ( - patch( - "charm.PostgresqlOperatorCharm.is_primary", - new_callable=PropertyMock, - return_value=True, - ), patch( "charm.PostgresqlOperatorCharm.primary_endpoint", new_callable=PropertyMock, return_value="10.0.0.1", ), + patch.object(harness.charm, "_resolve_primary_host", return_value="10.0.0.1"), patch( "charm.PostgresqlOperatorCharm.postgresql", new_callable=PropertyMock, From 8c6815df666b891d45eb503eb1b61dd3bd0e4137 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Mon, 25 May 2026 15:04:11 -0300 Subject: [PATCH 19/19] fix: log actionable error when temp tablespace migration fails due to existing objects When the temp tablespace migration (DROP TABLESPACE) fails during upgrade, query pg_class to detect blocking objects and log an error with clear instructions for the operator to resolve the issue. Signed-off-by: Marcelo Henrique Neppel --- src/charm.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/charm.py b/src/charm.py index db53194593..43aa92423f 100755 --- a/src/charm.py +++ b/src/charm.py @@ -824,6 +824,26 @@ def _execute_temp_tablespace_migration(self, target_host: str) -> bool: cursor.execute("CHECKPOINT;") except psycopg2.Error: logger.exception("Failed to migrate temp tablespace location") + try: + check_conn = self.postgresql._connect_to_database(database_host=target_host) + check_conn.autocommit = True + check_cur = check_conn.cursor() + check_cur.execute( + "SELECT count(*) FROM pg_class WHERE reltablespace = " + "(SELECT oid FROM pg_tablespace WHERE spcname = 'temp')" + ) + obj_count = check_cur.fetchone()[0] + check_cur.close() + check_conn.close() + if obj_count > 0: + logger.error( + "Temp tablespace has %d object(s). " + "Please move or drop all objects from the temp tablespace, " + "then run 'juju resolved postgresql/' to retry.", + obj_count, + ) + except Exception: + logger.debug("Could not query temp tablespace for blocking objects") return False finally: if cursor is not None: