From c1f126922509781c720c3ed98b3e9b4b08a84fb5 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 11 Sep 2025 17:11:49 -0300 Subject: [PATCH 1/6] Recreate temp tablespace on reboot Signed-off-by: Marcelo Henrique Neppel --- pyproject.toml | 2 +- single_kernel_postgresql/utils/postgresql.py | 13 ++++++++++--- uv.lock | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index c9bcbd9..bfe4124 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ [project] name = "postgresql-charms-single-kernel" description = "Shared and reusable code for PostgreSQL-related charms" -version = "16.0.0" +version = "16.0.1" readme = "README.md" license = "Apache-2.0" authors = [ diff --git a/single_kernel_postgresql/utils/postgresql.py b/single_kernel_postgresql/utils/postgresql.py index 2ad421c..9a2c38a 100644 --- a/single_kernel_postgresql/utils/postgresql.py +++ b/single_kernel_postgresql/utils/postgresql.py @@ -21,6 +21,7 @@ import logging import os +import pwd from collections import OrderedDict from typing import Dict, List, Optional, Set, Tuple @@ -28,7 +29,7 @@ from ops import ConfigData from psycopg2.sql import SQL, Identifier, Literal -from ..config.literals import BACKUP_USER, POSTGRESQL_STORAGE_PERMISSIONS, SYSTEM_USERS +from ..config.literals import BACKUP_USER, POSTGRESQL_STORAGE_PERMISSIONS, SNAP_USER, SYSTEM_USERS from .filesystem import change_owner # Groups to distinguish HBA access @@ -1074,8 +1075,14 @@ def set_up_database(self, temp_location: Optional[str] = None) -> None: if temp_location is not None: # Fix permissions on the temporary tablespace location when a reboot happens and tmpfs is being used. - change_owner(temp_location) - os.chmod(temp_location, POSTGRESQL_STORAGE_PERMISSIONS) + temp_location_stats = os.stat(temp_location) + if ( + pwd.getpwuid(temp_location_stats.st_uid).pw_name != SNAP_USER + or temp_location_stats.st_mode != POSTGRESQL_STORAGE_PERMISSIONS + ): + change_owner(temp_location) + os.chmod(temp_location, POSTGRESQL_STORAGE_PERMISSIONS) + cursor.execute("DROP TABLESPACE IF EXISTS temp;") cursor.execute("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';") if cursor.fetchone() is None: diff --git a/uv.lock b/uv.lock index 3f2527c..470ee73 100644 --- a/uv.lock +++ b/uv.lock @@ -268,7 +268,7 @@ wheels = [ [[package]] name = "postgresql-charms-single-kernel" -version = "16.0.0" +version = "16.0.1" source = { virtual = "." } [package.dev-dependencies] From 59d9092301cc57ed0e15d6152373176fba49536e Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 11 Sep 2025 17:23:29 -0300 Subject: [PATCH 2/6] Fix unit test Signed-off-by: Marcelo Henrique Neppel --- tests/unit/test_postgresql.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/unit/test_postgresql.py b/tests/unit/test_postgresql.py index 340c3e1..d41cb7b 100644 --- a/tests/unit/test_postgresql.py +++ b/tests/unit/test_postgresql.py @@ -329,7 +329,14 @@ def test_set_up_database_with_temp_tablespace_and_missing_owner_role(harness): patch("single_kernel_postgresql.utils.postgresql.PostgreSQL.create_user") as _create_user, patch("single_kernel_postgresql.utils.postgresql.change_owner") as _change_owner, patch("single_kernel_postgresql.utils.postgresql.os.chmod") as _chmod, + patch("single_kernel_postgresql.utils.postgresql.os.stat") as _stat, + patch("single_kernel_postgresql.utils.postgresql.pwd.getpwuid") as _getpwuid, ): + # Simulate a temp location owned by wrong user/permissions to trigger fixup + stat_result = type("stat_result", (), {"st_uid": 0, "st_mode": 0o755}) + _stat.return_value = stat_result + _getpwuid.return_value.pw_name = "root" + # First connection (non-context) for temp tablespace execute_direct = _connect_to_database.return_value.cursor.return_value.execute fetchone_direct = _connect_to_database.return_value.cursor.return_value.fetchone From 4d332fd4a1f1bc90be273c4588b1166b16bfdee1 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 11 Sep 2025 17:55:30 -0300 Subject: [PATCH 3/6] Improve unit tests Signed-off-by: Marcelo Henrique Neppel --- tests/unit/test_postgresql.py | 78 ++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 5 deletions(-) diff --git a/tests/unit/test_postgresql.py b/tests/unit/test_postgresql.py index d41cb7b..a23c202 100644 --- a/tests/unit/test_postgresql.py +++ b/tests/unit/test_postgresql.py @@ -1,17 +1,23 @@ # Copyright 2025 Canonical Ltd. # See LICENSE file for licensing details. +# ruff: noqa: I001 from unittest.mock import call, patch, sentinel import psycopg2 import pytest from ops.testing import Harness -from psycopg2.sql import SQL, Composed, Identifier, Literal +from psycopg2.sql import Composed, Identifier, Literal, SQL + from single_kernel_postgresql.abstract_charm import AbstractPostgreSQLCharm -from single_kernel_postgresql.config.literals import PEER, SYSTEM_USERS +from single_kernel_postgresql.config.literals import ( + PEER, + POSTGRESQL_STORAGE_PERMISSIONS, + SNAP_USER, + SYSTEM_USERS, +) from single_kernel_postgresql.utils.postgresql import ( - ACCESS_GROUP_INTERNAL, ACCESS_GROUPS, - ROLE_DATABASES_OWNER, + ACCESS_GROUP_INTERNAL, PostgreSQL, PostgreSQLCreateDatabaseError, PostgreSQLCreateUserError, @@ -19,6 +25,7 @@ PostgreSQLGetLastArchivedWALError, PostgreSQLUndefinedHostError, PostgreSQLUndefinedPasswordError, + ROLE_DATABASES_OWNER, ) @@ -353,7 +360,8 @@ def test_set_up_database_with_temp_tablespace_and_missing_owner_role(harness): _change_owner.assert_called_once_with("/var/lib/postgresql/tmp") _chmod.assert_called_once_with("/var/lib/postgresql/tmp", 0o700) - # Validate temp tablespace operations + # Validate temp tablespace operations, including DROP when permissions are fixed + execute_direct.assert_any_call("DROP TABLESPACE IF EXISTS temp;") execute_direct.assert_has_calls([ call("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';"), call("CREATE TABLESPACE temp LOCATION '/var/lib/postgresql/tmp';"), @@ -378,6 +386,66 @@ def test_set_up_database_with_temp_tablespace_and_missing_owner_role(harness): execute_cm.assert_has_calls(expected, any_order=False) +def test_set_up_database_owner_mismatch_triggers_drop_and_fix(harness): + with ( + patch( + "single_kernel_postgresql.utils.postgresql.PostgreSQL._connect_to_database" + ) as _connect_to_database, + patch("single_kernel_postgresql.utils.postgresql.PostgreSQL.set_up_login_hook_function"), + patch( + "single_kernel_postgresql.utils.postgresql.PostgreSQL.set_up_predefined_catalog_roles_function" + ), + patch("single_kernel_postgresql.utils.postgresql.change_owner") as _change_owner, + patch("single_kernel_postgresql.utils.postgresql.os.chmod") as _chmod, + patch("single_kernel_postgresql.utils.postgresql.os.stat") as _stat, + patch("single_kernel_postgresql.utils.postgresql.pwd.getpwuid") as _getpwuid, + ): + # Owner differs, permissions are correct + stat_result = type( + "stat_result", (), {"st_uid": 0, "st_mode": POSTGRESQL_STORAGE_PERMISSIONS} + ) + _stat.return_value = stat_result + _getpwuid.return_value.pw_name = "root" + + execute_direct = _connect_to_database.return_value.cursor.return_value.execute + _connect_to_database.return_value.cursor.return_value.fetchone.return_value = True + + harness.charm.postgresql.set_up_database(temp_location="/var/lib/postgresql/tmp") + + _change_owner.assert_called_once_with("/var/lib/postgresql/tmp") + _chmod.assert_called_once_with("/var/lib/postgresql/tmp", POSTGRESQL_STORAGE_PERMISSIONS) + execute_direct.assert_any_call("DROP TABLESPACE IF EXISTS temp;") + + +def test_set_up_database_permissions_mismatch_triggers_drop_and_fix(harness): + with ( + patch( + "single_kernel_postgresql.utils.postgresql.PostgreSQL._connect_to_database" + ) as _connect_to_database, + patch("single_kernel_postgresql.utils.postgresql.PostgreSQL.set_up_login_hook_function"), + patch( + "single_kernel_postgresql.utils.postgresql.PostgreSQL.set_up_predefined_catalog_roles_function" + ), + patch("single_kernel_postgresql.utils.postgresql.change_owner") as _change_owner, + patch("single_kernel_postgresql.utils.postgresql.os.chmod") as _chmod, + patch("single_kernel_postgresql.utils.postgresql.os.stat") as _stat, + patch("single_kernel_postgresql.utils.postgresql.pwd.getpwuid") as _getpwuid, + ): + # Owner matches SNAP_USER, permissions differ + stat_result = type("stat_result", (), {"st_uid": 0, "st_mode": 0o755}) + _stat.return_value = stat_result + _getpwuid.return_value.pw_name = SNAP_USER + + execute_direct = _connect_to_database.return_value.cursor.return_value.execute + _connect_to_database.return_value.cursor.return_value.fetchone.return_value = True + + harness.charm.postgresql.set_up_database(temp_location="/var/lib/postgresql/tmp") + + _change_owner.assert_called_once_with("/var/lib/postgresql/tmp") + _chmod.assert_called_once_with("/var/lib/postgresql/tmp", POSTGRESQL_STORAGE_PERMISSIONS) + execute_direct.assert_any_call("DROP TABLESPACE IF EXISTS temp;") + + def test_set_up_database_no_temp_and_existing_owner_role(harness): with ( patch( From ece989a04907388cca86947d72fa7f006e09b554 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Mon, 15 Sep 2025 17:41:29 -0300 Subject: [PATCH 4/6] Rename tablespace instead of dropping it Signed-off-by: Marcelo Henrique Neppel --- single_kernel_postgresql/utils/postgresql.py | 10 ++++++- tests/unit/test_postgresql.py | 31 +++++++++++++++----- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/single_kernel_postgresql/utils/postgresql.py b/single_kernel_postgresql/utils/postgresql.py index 9a2c38a..ad800c6 100644 --- a/single_kernel_postgresql/utils/postgresql.py +++ b/single_kernel_postgresql/utils/postgresql.py @@ -23,6 +23,7 @@ import os import pwd from collections import OrderedDict +from datetime import datetime, timezone from typing import Dict, List, Optional, Set, Tuple import psycopg2 @@ -1082,8 +1083,15 @@ def set_up_database(self, temp_location: Optional[str] = None) -> None: ): change_owner(temp_location) os.chmod(temp_location, POSTGRESQL_STORAGE_PERMISSIONS) - cursor.execute("DROP TABLESPACE IF EXISTS temp;") + # Rename existing temp tablespace if it exists, instead of dropping it. + cursor.execute("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';") + if cursor.fetchone() is not None: + new_name = ( + f"temp_{datetime.now(timezone.utc).strftime('%Y_%m_%d_%H_%M_%S')}" + ) + cursor.execute(f"ALTER TABLESPACE temp RENAME TO {new_name};") + # Ensure a fresh temp tablespace exists at the expected location. cursor.execute("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';") if cursor.fetchone() is None: cursor.execute(f"CREATE TABLESPACE temp LOCATION '{temp_location}';") diff --git a/tests/unit/test_postgresql.py b/tests/unit/test_postgresql.py index a23c202..f760745 100644 --- a/tests/unit/test_postgresql.py +++ b/tests/unit/test_postgresql.py @@ -2,6 +2,7 @@ # See LICENSE file for licensing details. # ruff: noqa: I001 from unittest.mock import call, patch, sentinel +from datetime import datetime, timezone, UTC import psycopg2 import pytest @@ -360,9 +361,9 @@ def test_set_up_database_with_temp_tablespace_and_missing_owner_role(harness): _change_owner.assert_called_once_with("/var/lib/postgresql/tmp") _chmod.assert_called_once_with("/var/lib/postgresql/tmp", 0o700) - # Validate temp tablespace operations, including DROP when permissions are fixed - execute_direct.assert_any_call("DROP TABLESPACE IF EXISTS temp;") + # Validate temp tablespace operations: check existence and create/grant when missing execute_direct.assert_has_calls([ + call("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';"), call("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';"), call("CREATE TABLESPACE temp LOCATION '/var/lib/postgresql/tmp';"), call("GRANT CREATE ON TABLESPACE temp TO public;"), @@ -386,7 +387,7 @@ def test_set_up_database_with_temp_tablespace_and_missing_owner_role(harness): execute_cm.assert_has_calls(expected, any_order=False) -def test_set_up_database_owner_mismatch_triggers_drop_and_fix(harness): +def test_set_up_database_owner_mismatch_triggers_rename_and_fix(harness): with ( patch( "single_kernel_postgresql.utils.postgresql.PostgreSQL._connect_to_database" @@ -399,6 +400,7 @@ def test_set_up_database_owner_mismatch_triggers_drop_and_fix(harness): patch("single_kernel_postgresql.utils.postgresql.os.chmod") as _chmod, patch("single_kernel_postgresql.utils.postgresql.os.stat") as _stat, patch("single_kernel_postgresql.utils.postgresql.pwd.getpwuid") as _getpwuid, + patch("single_kernel_postgresql.utils.postgresql.datetime") as _dt, ): # Owner differs, permissions are correct stat_result = type( @@ -407,17 +409,23 @@ def test_set_up_database_owner_mismatch_triggers_drop_and_fix(harness): _stat.return_value = stat_result _getpwuid.return_value.pw_name = "root" + # Mock datetime.now(timezone.utc) to a fixed timestamp + _dt.now.return_value = datetime(2025, 1, 1, 1, 2, 3, tzinfo=UTC) + _dt.timezone = timezone # ensure timezone.utc is available in the patch target + execute_direct = _connect_to_database.return_value.cursor.return_value.execute - _connect_to_database.return_value.cursor.return_value.fetchone.return_value = True + fetchone_direct = _connect_to_database.return_value.cursor.return_value.fetchone + fetchone_direct.side_effect = [True, None] harness.charm.postgresql.set_up_database(temp_location="/var/lib/postgresql/tmp") _change_owner.assert_called_once_with("/var/lib/postgresql/tmp") _chmod.assert_called_once_with("/var/lib/postgresql/tmp", POSTGRESQL_STORAGE_PERMISSIONS) - execute_direct.assert_any_call("DROP TABLESPACE IF EXISTS temp;") + execute_direct.assert_any_call("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';") + execute_direct.assert_any_call("ALTER TABLESPACE temp RENAME TO temp_2025_01_01_01_02_03;") -def test_set_up_database_permissions_mismatch_triggers_drop_and_fix(harness): +def test_set_up_database_permissions_mismatch_triggers_rename_and_fix(harness): with ( patch( "single_kernel_postgresql.utils.postgresql.PostgreSQL._connect_to_database" @@ -430,20 +438,27 @@ def test_set_up_database_permissions_mismatch_triggers_drop_and_fix(harness): patch("single_kernel_postgresql.utils.postgresql.os.chmod") as _chmod, patch("single_kernel_postgresql.utils.postgresql.os.stat") as _stat, patch("single_kernel_postgresql.utils.postgresql.pwd.getpwuid") as _getpwuid, + patch("single_kernel_postgresql.utils.postgresql.datetime") as _dt, ): # Owner matches SNAP_USER, permissions differ stat_result = type("stat_result", (), {"st_uid": 0, "st_mode": 0o755}) _stat.return_value = stat_result _getpwuid.return_value.pw_name = SNAP_USER + # Mock datetime.now(timezone.utc) to a fixed timestamp + _dt.now.return_value = datetime(2025, 1, 1, 1, 2, 3, tzinfo=UTC) + _dt.timezone = timezone + execute_direct = _connect_to_database.return_value.cursor.return_value.execute - _connect_to_database.return_value.cursor.return_value.fetchone.return_value = True + fetchone_direct = _connect_to_database.return_value.cursor.return_value.fetchone + fetchone_direct.side_effect = [True, None] harness.charm.postgresql.set_up_database(temp_location="/var/lib/postgresql/tmp") _change_owner.assert_called_once_with("/var/lib/postgresql/tmp") _chmod.assert_called_once_with("/var/lib/postgresql/tmp", POSTGRESQL_STORAGE_PERMISSIONS) - execute_direct.assert_any_call("DROP TABLESPACE IF EXISTS temp;") + execute_direct.assert_any_call("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';") + execute_direct.assert_any_call("ALTER TABLESPACE temp RENAME TO temp_2025_01_01_01_02_03;") def test_set_up_database_no_temp_and_existing_owner_role(harness): From 8a6a7b70ef0889b334a85c56b996683e7c6774de Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 16 Sep 2025 09:38:47 -0300 Subject: [PATCH 5/6] Change timestamp format Signed-off-by: Marcelo Henrique Neppel --- single_kernel_postgresql/utils/postgresql.py | 4 +--- tests/unit/test_postgresql.py | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/single_kernel_postgresql/utils/postgresql.py b/single_kernel_postgresql/utils/postgresql.py index ad800c6..7cd23e7 100644 --- a/single_kernel_postgresql/utils/postgresql.py +++ b/single_kernel_postgresql/utils/postgresql.py @@ -1086,9 +1086,7 @@ def set_up_database(self, temp_location: Optional[str] = None) -> None: # Rename existing temp tablespace if it exists, instead of dropping it. cursor.execute("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';") if cursor.fetchone() is not None: - new_name = ( - f"temp_{datetime.now(timezone.utc).strftime('%Y_%m_%d_%H_%M_%S')}" - ) + new_name = f"temp_{datetime.now(timezone.utc).strftime('%Y%m%d%H%M%S')}" cursor.execute(f"ALTER TABLESPACE temp RENAME TO {new_name};") # Ensure a fresh temp tablespace exists at the expected location. diff --git a/tests/unit/test_postgresql.py b/tests/unit/test_postgresql.py index f760745..09cf0e5 100644 --- a/tests/unit/test_postgresql.py +++ b/tests/unit/test_postgresql.py @@ -422,7 +422,7 @@ def test_set_up_database_owner_mismatch_triggers_rename_and_fix(harness): _change_owner.assert_called_once_with("/var/lib/postgresql/tmp") _chmod.assert_called_once_with("/var/lib/postgresql/tmp", POSTGRESQL_STORAGE_PERMISSIONS) execute_direct.assert_any_call("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';") - execute_direct.assert_any_call("ALTER TABLESPACE temp RENAME TO temp_2025_01_01_01_02_03;") + execute_direct.assert_any_call("ALTER TABLESPACE temp RENAME TO temp_20250101010203;") def test_set_up_database_permissions_mismatch_triggers_rename_and_fix(harness): @@ -458,7 +458,7 @@ def test_set_up_database_permissions_mismatch_triggers_rename_and_fix(harness): _change_owner.assert_called_once_with("/var/lib/postgresql/tmp") _chmod.assert_called_once_with("/var/lib/postgresql/tmp", POSTGRESQL_STORAGE_PERMISSIONS) execute_direct.assert_any_call("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';") - execute_direct.assert_any_call("ALTER TABLESPACE temp RENAME TO temp_2025_01_01_01_02_03;") + execute_direct.assert_any_call("ALTER TABLESPACE temp RENAME TO temp_20250101010203;") def test_set_up_database_no_temp_and_existing_owner_role(harness): From adc214e7ac040ac2f62f9273fd69570a4bdae3d7 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 16 Sep 2025 09:50:48 -0300 Subject: [PATCH 6/6] Log count and names of temp tablespaces Signed-off-by: Marcelo Henrique Neppel --- single_kernel_postgresql/utils/postgresql.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/single_kernel_postgresql/utils/postgresql.py b/single_kernel_postgresql/utils/postgresql.py index 7cd23e7..d0f88cd 100644 --- a/single_kernel_postgresql/utils/postgresql.py +++ b/single_kernel_postgresql/utils/postgresql.py @@ -1089,6 +1089,17 @@ def set_up_database(self, temp_location: Optional[str] = None) -> None: new_name = f"temp_{datetime.now(timezone.utc).strftime('%Y%m%d%H%M%S')}" cursor.execute(f"ALTER TABLESPACE temp RENAME TO {new_name};") + # List temp tablespaces with suffix for operator follow-up cleanup and log them. + cursor.execute( + "SELECT spcname FROM pg_tablespace WHERE spcname LIKE 'temp_%';" + ) + temp_tbls = sorted([row[0] for row in cursor.fetchall()]) + logger.info( + "There are %d temp tablespaces that should be checked and removed: %s", + len(temp_tbls), + ", ".join(temp_tbls), + ) + # Ensure a fresh temp tablespace exists at the expected location. cursor.execute("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';") if cursor.fetchone() is None: