[DPE-8747] Handle persistent storage in set_up_database to prevent ObjectInUse errors (#54)

marceloneppel · web-flow · commit f6cb835c2947 · 2025-12-16T17:59:34.000-03:00
* fix: handle persistent storage in set_up_database to prevent ObjectInUse errors Fix a bug where set_up_database() fails with "directory already in use as a tablespace" error when running on persistent storage after a reboot or failover. The issue occurred because the method assumed tmpfs storage and would rename the existing 'temp' tablespace and try to create a new one at the same location. With persistent storage, the directory remains in use by the renamed tablespace, causing PostgreSQL to reject the CREATE TABLESPACE command. Changes: - Add is_tmpfs() utility function to detect filesystem type via /proc/mounts - Modify set_up_database() to differentiate between tmpfs and persistent storage: * tmpfs: Preserve existing behavior (rename + recreate for empty directory) * persistent: Only fix permissions, keep existing tablespace - Extract tablespace handling into _handle_temp_tablespace_on_reboot() helper method to reduce cyclomatic complexity - Add comprehensive test coverage (7 new tests for is_tmpfs, 1 for persistent storage handling) - Update existing tests to mock filesystem detection This ensures the library works correctly with both tmpfs and persistent storage configurations for the temp tablespace. Fixes cluster failures after reboot with persistent storage. Bumps version to 16.1.5. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * fix: include temp_location path in persistent storage log message Add temp_location parameter to the log message in _handle_temp_tablespace_on_reboot() when handling persistent storage scenarios. This provides better observability and makes debugging easier by showing operators the exact directory path where permissions were fixed. Addresses feedback from PR review comment: #54 (comment) Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * docs: explain timestamp collision prevention in tablespace rename Add comment documenting why timestamp collision cannot occur during temp tablespace rename operations on tmpfs storage. The comment clarifies that consuming charms ensure leader-only execution and PostgreSQL serializes operations through exclusive locks, making second-precision timestamps sufficient for uniqueness. Addresses feedback from PR review comment: #54 (comment) Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> --------- Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
diff --git 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.1.4"
+version = "16.1.5"
 readme = "README.md"
 license = {file = "LICENSE"}
 authors = [
diff --git a/single_kernel_postgresql/utils/filesystem.py b/single_kernel_postgresql/utils/filesystem.py
@@ -18,3 +18,45 @@ def change_owner(path: str) -> None:
     user_database = pwd.getpwnam(SNAP_USER)
     # Set the correct ownership for the file or directory.
     os.chown(path, uid=user_database.pw_uid, gid=user_database.pw_gid)
+
+
+def is_tmpfs(path: str) -> bool:
+    """Check if a path is on a tmpfs filesystem.
+
+    This function reads /proc/mounts to determine the filesystem type of the
+    mount point containing the given path.
+
+    Args:
+        path: Path to check.
+
+    Returns:
+        True if the path is on a tmpfs filesystem, False otherwise.
+        Returns False if /proc/mounts cannot be read (e.g., not on Linux).
+    """
+    try:
+        with open("/proc/mounts") as f:
+            mounts = f.readlines()
+    except (FileNotFoundError, PermissionError):
+        # Not on Linux or /proc not available, assume persistent storage
+        return False
+
+    # Get absolute path to handle relative paths and symlinks
+    abs_path = os.path.abspath(path)
+
+    # Find the longest matching mount point (most specific)
+    best_match_fs = None
+    best_match_len = 0
+
+    for line in mounts:
+        parts = line.split()
+        if len(parts) < 3:
+            continue
+        mount_point = parts[1]
+        fs_type = parts[2]
+
+        # Check if path is under this mount point and is more specific
+        if abs_path.startswith(mount_point) and len(mount_point) > best_match_len:
+            best_match_fs = fs_type
+            best_match_len = len(mount_point)
+
+    return best_match_fs == "tmpfs"
diff --git a/single_kernel_postgresql/utils/postgresql.py b/single_kernel_postgresql/utils/postgresql.py
@@ -37,7 +37,7 @@
     SYSTEM_USERS,
     Substrates,
 )
-from .filesystem import change_owner
+from .filesystem import change_owner, is_tmpfs
 
 # Groups to distinguish HBA access
 ACCESS_GROUP_IDENTITY = "identity_access"
@@ -1107,39 +1107,88 @@ def _get_existing_databases(self) -> List[str]:
                 connection.close()
         return databases
 
+    def _handle_temp_tablespace_on_reboot(
+        self, cursor, temp_location: str, temp_tablespace_exists: bool
+    ) -> None:
+        """Handle temp tablespace when permissions need fixing after reboot.
+
+        Args:
+            cursor: Database cursor.
+            temp_location: Path to the temp tablespace location.
+            temp_tablespace_exists: Whether the temp tablespace already exists.
+        """
+        if not temp_tablespace_exists:
+            return
+
+        # Different handling based on storage type
+        if is_tmpfs(temp_location):
+            # tmpfs: Directory is empty after reboot, safe to rename and recreate
+            # Rename existing temp tablespace instead of dropping it.
+            # Timestamp collision is not possible: the charm ensures this code runs leader-only,
+            # and it executes within a single database transaction holding exclusive locks.
+            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
+            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),
+            )
+        else:
+            # Persistent storage: Tablespace is still valid, permissions already fixed
+            # Log that we fixed permissions but didn't recreate
+            logger.info(
+                "Fixed permissions on temp tablespace directory at %s (persistent storage), "
+                "existing tablespace remains valid",
+                temp_location,
+            )
+
     def set_up_database(self, temp_location: Optional[str] = None) -> None:
-        """Set up postgres database with the right permissions."""
+        """Set up postgres database with the right permissions.
+
+        This method configures the postgres database with appropriate permissions and
+        optionally creates a temporary tablespace.
+
+        Args:
+            temp_location: Optional path for the temp tablespace. If provided, the method
+                will ensure proper permissions and create the tablespace if it doesn't exist.
+
+        Behavior on reboot:
+            - For tmpfs storage: If permissions are incorrect after reboot, renames the old
+              tablespace and creates a new one (tmpfs directory is empty after reboot).
+            - For persistent storage: If permissions are incorrect after reboot, fixes
+              permissions but keeps the existing tablespace (directory contents persist).
+
+        Raises:
+            PostgreSQLDatabasesSetupError: If database setup fails.
+        """
         connection = None
         cursor = None
         try:
             connection = self._connect_to_database()
             cursor = connection.cursor()
 
             if temp_location is not None:
-                # Fix permissions on the temporary tablespace location when a reboot happens and tmpfs is being used.
+                # Fix permissions on the temporary tablespace location when a reboot happens.
                 temp_location_stats = os.stat(temp_location)
-                if self.substrate == Substrates.VM and (
+                permissions_need_fix = self.substrate == Substrates.VM and (
                     pwd.getpwuid(temp_location_stats.st_uid).pw_name != SNAP_USER
                     or int(temp_location_stats.st_mode & 0o777) != POSTGRESQL_STORAGE_PERMISSIONS
-                ):
+                )
+
+                if permissions_need_fix:
                     change_owner(temp_location)
                     os.chmod(temp_location, POSTGRESQL_STORAGE_PERMISSIONS)
-                    # 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};")
 
-                        # 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),
-                        )
+                    # Check if temp tablespace exists and handle it appropriately
+                    cursor.execute("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';")
+                    temp_tablespace_exists = cursor.fetchone() is not None
+                    self._handle_temp_tablespace_on_reboot(
+                        cursor, temp_location, temp_tablespace_exists
+                    )
 
                 # Ensure a fresh temp tablespace exists at the expected location.
                 cursor.execute("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';")
diff --git a/tests/unit/test_filesystem.py b/tests/unit/test_filesystem.py
@@ -1,11 +1,11 @@
 # Copyright 2025 Canonical Ltd.
 # See LICENSE file for licensing details.
 from tempfile import NamedTemporaryFile
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock, mock_open, patch
 
 import pytest
 from single_kernel_postgresql.config.literals import SNAP_USER
-from single_kernel_postgresql.utils.filesystem import change_owner
+from single_kernel_postgresql.utils.filesystem import change_owner, is_tmpfs
 
 
 def test_change_owner_calls_pwd_and_os_chown_with_daemon_user():
@@ -50,3 +50,80 @@ def test_change_owner_bubbles_up_os_error():
         getpwnam.return_value = entry
         with pytest.raises(OSError):
             change_owner(tmp.name)
+
+
+def test_is_tmpfs_detects_tmpfs_correctly():
+    """Test that is_tmpfs correctly identifies tmpfs filesystems."""
+    proc_mounts_content = (
+        "tmpfs /tmp tmpfs rw,nosuid,nodev 0 0\n"
+        "tmpfs /run tmpfs rw,nosuid,nodev,noexec,relatime 0 0\n"
+        "/dev/sda1 / ext4 rw,relatime 0 0\n"
+        "/dev/sda1 /var ext4 rw,relatime 0 0\n"
+    )
+    with patch("builtins.open", mock_open(read_data=proc_mounts_content)):
+        assert is_tmpfs("/tmp/test") is True
+        assert is_tmpfs("/run/postgresql") is True
+        assert is_tmpfs("/var/lib/postgresql") is False
+        assert is_tmpfs("/") is False
+
+
+def test_is_tmpfs_handles_nested_paths():
+    """Test that is_tmpfs correctly handles nested paths under mount points."""
+    proc_mounts_content = (
+        "tmpfs /dev/shm tmpfs rw,nosuid,nodev 0 0\n/dev/sda1 / ext4 rw,relatime 0 0\n"
+    )
+    with patch("builtins.open", mock_open(read_data=proc_mounts_content)):
+        assert is_tmpfs("/dev/shm") is True
+        assert is_tmpfs("/dev/shm/postgresql") is True
+        assert is_tmpfs("/dev/shm/postgresql/temp") is True
+        assert is_tmpfs("/dev/other") is False
+
+
+def test_is_tmpfs_uses_longest_mount_match():
+    """Test that is_tmpfs uses the most specific (longest) mount point."""
+    proc_mounts_content = (
+        "/dev/sda1 / ext4 rw,relatime 0 0\n"
+        "tmpfs /var/tmp tmpfs rw,nosuid,nodev 0 0\n"
+        "/dev/sdb1 /var ext4 rw,relatime 0 0\n"
+    )
+    with patch("builtins.open", mock_open(read_data=proc_mounts_content)):
+        # /var is ext4, but /var/tmp is tmpfs - should match the longer path
+        assert is_tmpfs("/var/tmp/test") is True
+        assert is_tmpfs("/var/lib/test") is False
+
+
+def test_is_tmpfs_handles_relative_paths():
+    """Test that is_tmpfs correctly resolves relative paths."""
+    proc_mounts_content = "tmpfs /run tmpfs rw,nosuid,nodev 0 0\n"
+    with (
+        patch("builtins.open", mock_open(read_data=proc_mounts_content)),
+        patch("single_kernel_postgresql.utils.filesystem.os.path.abspath") as mock_abspath,
+    ):
+        mock_abspath.return_value = "/run/test"
+        assert is_tmpfs("../run/test") is True
+        mock_abspath.assert_called_once_with("../run/test")
+
+
+def test_is_tmpfs_handles_missing_proc_mounts():
+    """Test that is_tmpfs returns False when /proc/mounts is unavailable."""
+    with patch("builtins.open", side_effect=FileNotFoundError):
+        assert is_tmpfs("/any/path") is False
+
+
+def test_is_tmpfs_handles_permission_error():
+    """Test that is_tmpfs returns False when /proc/mounts cannot be read."""
+    with patch("builtins.open", side_effect=PermissionError):
+        assert is_tmpfs("/any/path") is False
+
+
+def test_is_tmpfs_handles_malformed_mount_lines():
+    """Test that is_tmpfs gracefully handles malformed lines in /proc/mounts."""
+    proc_mounts_content = (
+        "invalid line\n"
+        "device /path\n"  # Only 2 parts
+        "tmpfs /tmp tmpfs rw,nosuid,nodev 0 0\n"  # Valid line
+        "\n"  # Empty line
+    )
+    with patch("builtins.open", mock_open(read_data=proc_mounts_content)):
+        # Should still work and find the valid tmpfs line
+        assert is_tmpfs("/tmp/test") is True
diff --git a/tests/unit/test_postgresql.py b/tests/unit/test_postgresql.py
@@ -406,7 +406,11 @@ def test_set_up_database_owner_mismatch_triggers_rename_and_fix(harness):
         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,
+        patch("single_kernel_postgresql.utils.postgresql.is_tmpfs") as _is_tmpfs,
     ):
+        # Simulate tmpfs storage for this test
+        _is_tmpfs.return_value = True
+
         # Owner differs, permissions are correct (16832 means 0o700)
         # Simulate directory owned by uid 1000 while expected owner has uid 0 to force mismatch
         stat_result = type("stat_result", (), {"st_uid": 1000, "st_gid": 1000, "st_mode": 16832})
@@ -446,7 +450,11 @@ def test_set_up_database_permissions_mismatch_triggers_rename_and_fix(harness):
         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,
+        patch("single_kernel_postgresql.utils.postgresql.is_tmpfs") as _is_tmpfs,
     ):
+        # Simulate tmpfs storage for this test
+        _is_tmpfs.return_value = True
+
         # Owner matches SNAP_USER, permissions differ (33188 means 0o644)
         stat_result = type("stat_result", (), {"st_uid": 0, "st_gid": 0, "st_mode": 33188})
         _stat.return_value = stat_result
@@ -470,6 +478,53 @@ def test_set_up_database_permissions_mismatch_triggers_rename_and_fix(harness):
         execute_direct.assert_any_call("ALTER TABLESPACE temp RENAME TO temp_20250101010203;")
 
 
+def test_set_up_database_persistent_storage_no_rename(harness):
+    """Test that persistent storage permissions fix doesn't rename tablespace."""
+    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,
+        patch("single_kernel_postgresql.utils.postgresql.is_tmpfs") as _is_tmpfs,
+    ):
+        # Simulate persistent storage (not tmpfs)
+        _is_tmpfs.return_value = False
+
+        # Permissions need fixing (wrong owner)
+        stat_result = type("stat_result", (), {"st_uid": 1000, "st_gid": 1000, "st_mode": 16832})
+        _stat.return_value = stat_result
+        _getpwuid.return_value.pw_name = "root"
+
+        execute = _connect_to_database.return_value.cursor.return_value.execute
+        fetchone = _connect_to_database.return_value.cursor.return_value.fetchone
+        # First check: tablespace exists, second check: still exists (not renamed)
+        fetchone.side_effect = [True, True]
+
+        harness.charm.postgresql.set_up_database(temp_location="/var/lib/postgresql/tmp")
+
+        # Permissions should be fixed
+        _change_owner.assert_called_once_with("/var/lib/postgresql/tmp")
+        _chmod.assert_called_once_with("/var/lib/postgresql/tmp", POSTGRESQL_STORAGE_PERMISSIONS)
+
+        # Tablespace should NOT be renamed
+        for call in execute.call_args_list:
+            call_str = str(call)
+            assert "ALTER TABLESPACE temp RENAME" not in call_str, f"Found rename in: {call_str}"
+
+        # Should NOT try to create new tablespace (because it still exists)
+        create_calls = [
+            call for call in execute.call_args_list if "CREATE TABLESPACE temp" in str(call)
+        ]
+        assert len(create_calls) == 0, f"Unexpected CREATE TABLESPACE calls: {create_calls}"
+
+
 def test_set_up_database_no_temp_and_existing_owner_role(harness):
     with (
         patch(
diff --git a/uv.lock b/uv.lock
