[DPE-6965] Storage pools (#852)

* Separate storage pools

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Fix charm test

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Fix async replication

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Fix replica bootstrap

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Fix data directory removal on restore

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Fix backup do microceph test

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Fix upgrade integration tests

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Add test to check new multiple storages

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Add storages'descriptions

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Fix test_charm_garbage_ignorance

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Fix restore cluster test

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

* Fix storage re-use test

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

---------

Signed-off-by: Marcelo Henrique Neppel <[email protected]>

Author: marceloneppel

lib/charms/postgresql_k8s/v0/postgresql.py

@@ -35,7 +35,7 @@
 
 # Increment this PATCH version before using `charmcraft publish-lib` or reset
 # to 0 if you are raising the major API version
-LIBPATCH = 51
+LIBPATCH = 52
 
 # Groups to distinguish HBA access
 ACCESS_GROUP_IDENTITY = "identity_access"
@@ -698,15 +698,22 @@ def list_valid_privileges_and_roles(self) -> Tuple[Set[str], Set[str]]:
                 "superuser",
             }, {role[0] for role in cursor.fetchall() if role[0]}
 
-    def set_up_database(self) -> None:
+    def set_up_database(self, temp_location: Optional[str] = None) -> None:
         """Set up postgres database with the right permissions."""
         connection = None
+        cursor = None
         try:
-            with self._connect_to_database() as connection, connection.cursor() as cursor:
-                cursor.execute("SELECT TRUE FROM pg_roles WHERE rolname='admin';")
-                if cursor.fetchone() is not None:
-                    return
+            connection = self._connect_to_database()
+            cursor = connection.cursor()
 
+            if temp_location is not None:
+                cursor.execute("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';")
+                if cursor.fetchone() is None:
+                    cursor.execute(f"CREATE TABLESPACE temp LOCATION '{temp_location}';")
+                    cursor.execute("GRANT CREATE ON TABLESPACE temp TO public;")
+
+            cursor.execute("SELECT TRUE FROM pg_roles WHERE rolname='admin';")
+            if cursor.fetchone() is None:
                 # Allow access to the postgres database only to the system users.
                 cursor.execute("REVOKE ALL PRIVILEGES ON DATABASE postgres FROM PUBLIC;")
                 cursor.execute("REVOKE CREATE ON SCHEMA public FROM PUBLIC;")
@@ -725,6 +732,8 @@ def set_up_database(self) -> None:
             logger.error(f"Failed to set up databases: {e}")
             raise PostgreSQLDatabasesSetupError() from e
         finally:
+            if cursor is not None:
+                cursor.close()
             if connection is not None:
                 connection.close()
 

metadata.yaml

@@ -62,9 +62,22 @@ requires:
     optional: true
 
 storage:
-  pgdata:
+  archive:
     type: filesystem
-    location: /var/snap/charmed-postgresql/common
+    description: Storage mount used for holding local backups (before typically sending them to remote object storage) when relevant/needed.
+    location: /var/snap/charmed-postgresql/common/data/archive
+  data:
+    type: filesystem
+    description: Storage mount used for storing all tables, indexes, and so on (except those from temporary tablespaces).
+    location: /var/snap/charmed-postgresql/common/var/lib/postgresql
+  logs:
+    type: filesystem
+    description: Storage mount used for storing all the logs that are part of the transactional commit path (WAL files).
+    location: /var/snap/charmed-postgresql/common/data/logs
+  temp:
+    type: filesystem
+    description: Storage mount used for storing temporary tablespaces (where typically sort operations happen).
+    location: /var/snap/charmed-postgresql/common/data/temp
 
 assumes:
   - juju

src/backups.py

@@ -97,7 +97,7 @@ def _tls_ca_chain_filename(self) -> str:
         """Returns the path to the TLS CA chain file."""
         s3_parameters, _ = self._retrieve_s3_parameters()
         if s3_parameters.get("tls-ca-chain") is not None:
-            return f"{self.charm._storage_path}/pgbackrest-tls-ca-chain.crt"
+            return f"{PGBACKREST_CONF_PATH}/pgbackrest-tls-ca-chain.crt"
         return ""
 
     def _get_s3_session_resource(self, s3_parameters: dict):
@@ -327,12 +327,25 @@ 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",
+        ]
+        path = None
         try:
-            path = Path(POSTGRESQL_DATA_PATH)
-            if path.exists() and path.is_dir():
-                shutil.rmtree(path)
+            for path in paths:
+                path_object = Path(path)
+                if path_object.exists() and path_object.is_dir():
+                    for item in os.listdir(path):
+                        item_path = os.path.join(path, item)
+                        if os.path.isfile(item_path) or os.path.islink(item_path):
+                            os.remove(item_path)
+                        elif os.path.isdir(item_path):
+                            shutil.rmtree(item_path)
         except OSError as e:
-            logger.warning(f"Failed to remove contents of the data directory with error: {e!s}")
+            logger.warning(f"Failed to remove contents from {path} with error: {e!s}")
             return False
 
         return True

src/charm.py

@@ -86,6 +86,7 @@
     PATRONI_PASSWORD_KEY,
     PEER,
     PLUGIN_OVERRIDES,
+    POSTGRESQL_DATA_PATH,
     POSTGRESQL_SNAP_NAME,
     RAFT_PASSWORD_KEY,
     REPLICATION_PASSWORD_KEY,
@@ -197,9 +198,8 @@ def __init__(self, *args):
         self.framework.observe(self.on.update_status, self._on_update_status)
         self.cluster_name = self.app.name
         self._member_name = self.unit.name.replace("/", "-")
-
         self._certs_path = "/usr/local/share/ca-certificates"
-        self._storage_path = self.meta.storages["pgdata"].location
+        self._storage_path = self.meta.storages["data"].location
 
         self.upgrade = PostgreSQLUpgrade(
             self,
@@ -1469,7 +1469,9 @@ def _start_primary(self, event: StartEvent) -> None:
             event.defer()
             return
 
-        self.postgresql.set_up_database()
+        self.postgresql.set_up_database(
+            temp_location="/var/snap/charmed-postgresql/common/data/temp"
+        )
 
         access_groups = self.postgresql.list_access_groups()
         if access_groups != set(ACCESS_GROUPS):
@@ -1722,6 +1724,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)
+            if len(data_directory_contents) == 1 and data_directory_contents[0] == "pg_wal":
+                os.remove(os.path.join(POSTGRESQL_DATA_PATH, "pg_wal"))
+                logger.info("PostgreSQL data directory was not empty. Removed pg_wal")
+                return True
             try:
                 self._patroni.restart_patroni()
                 logger.info("restarted PostgreSQL because it was not running")

src/relations/async_replication.py

@@ -186,7 +186,7 @@ def _configure_standby_cluster(self, event: RelationChangedEvent) -> bool:
             raise Exception(error)
         if system_identifier != relation.data[relation.app].get("system-id"):
             # Store current data in a tar.gz file.
-            logger.info("Creating backup of pgdata folder")
+            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
@@ -649,19 +649,26 @@ def _re_emit_async_relation_changed_event(self) -> None:
         )
 
     def _reinitialise_pgdata(self) -> None:
-        """Reinitialise the pgdata folder."""
+        """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",
+        ]
+        path = None
         try:
-            path = Path(POSTGRESQL_DATA_PATH)
-            if path.exists() and path.is_dir():
-                shutil.rmtree(path)
+            for path in paths:
+                path_object = Path(path)
+                if path_object.exists() and path_object.is_dir():
+                    for item in os.listdir(path):
+                        item_path = os.path.join(path, item)
+                        if os.path.isfile(item_path) or os.path.islink(item_path):
+                            os.remove(item_path)
+                        elif os.path.isdir(item_path):
+                            shutil.rmtree(item_path)
         except OSError as e:
-            raise Exception(
-                f"Failed to remove contents of the data directory with error: {e!s}"
-            ) from e
-        os.mkdir(POSTGRESQL_DATA_PATH)
-        # Expected permissions
-        os.chmod(POSTGRESQL_DATA_PATH, 0o750)  # noqa: S103
-        self.charm._patroni._change_owner(POSTGRESQL_DATA_PATH)
+            raise Exception(f"Failed to remove contents from {path} with error: {e!s}") from e
 
     @property
     def _relation(self) -> Relation:
@@ -712,9 +719,9 @@ def _stop_database(self, event: RelationChangedEvent) -> bool:
                 if not self._configure_standby_cluster(event):
                     return False
 
-            # Remove and recreate the pgdata folder to enable replication of the data from the
+            # Remove and recreate the data folder to enable replication of the data from the
             # primary cluster.
-            logger.info("Removing and recreating pgdata folder")
+            logger.info("Removing and recreating data folder")
             self._reinitialise_pgdata()
 
             # Remove previous cluster information to make it possible to initialise a new cluster.

src/upgrade.py

@@ -247,7 +247,9 @@ def _prepare_upgrade_from_legacy(self) -> None:
                 self.charm.get_secret(APP_SCOPE, MONITORING_PASSWORD_KEY),
                 extra_user_roles="pg_monitor",
             )
-        self.charm.postgresql.set_up_database()
+        self.charm.postgresql.set_up_database(
+            temp_location="/var/snap/charmed-postgresql/common/data/temp"
+        )
         self._set_up_new_access_roles_for_legacy()
 
     def _set_up_new_access_roles_for_legacy(self) -> None:

templates/patroni.yml.j2

@@ -126,9 +126,12 @@ bootstrap:
   initdb:
   - encoding: UTF8
   - data-checksums
+  - waldir: /var/snap/charmed-postgresql/common/data/logs
   {%- endif %}
 
 postgresql:
+  basebackup:
+    - waldir: /var/snap/charmed-postgresql/common/data/logs
   listen: '{{ self_ip }}:5432'
   connect_address: '{{ self_ip }}:5432'
   # Path to PostgreSQL binaries used in the database bootstrap process.
@@ -147,6 +150,7 @@ postgresql:
     ssl_cert_file: {{ conf_path }}/cert.pem
     ssl_key_file: {{ conf_path }}/key.pem
     {%- endif %}
+    temp_tablespaces: temp
     unix_socket_directories: /tmp
     {%- if pg_parameters %}
     {%- for key, value in pg_parameters.items() %}

tests/helpers.py

@@ -7,4 +7,4 @@
 import yaml
 
 METADATA = yaml.safe_load(Path("./metadata.yaml").read_text())
-STORAGE_PATH = METADATA["storage"]["pgdata"]["location"]
+STORAGE_PATH = METADATA["storage"]["data"]["location"]

tests/integration/ha_tests/helpers.py

@@ -914,12 +914,13 @@ def storage_type(ops_test, app):
             return line.split()[3]
 
 
-def storage_id(ops_test, unit_name):
-    """Retrieves  storage id associated with provided unit.
+def get_storage_ids(ops_test, unit_name):
+    """Retrieves  storages ids associated with provided unit.
 
     Note: this function exists as a temporary solution until this issue is ported to libjuju 2:
     https://github.com/juju/python-libjuju/issues/694
     """
+    storage_ids = []
     model_name = ops_test.model.info.name
     proc = subprocess.check_output(f"juju storage --model={model_name}".split())
     proc = proc.decode("utf-8")
@@ -934,18 +935,22 @@ def storage_id(ops_test, unit_name):
             continue
 
         if line.split()[0] == unit_name:
-            return line.split()[1]
+            storage_ids.append(line.split()[1])
+    return storage_ids
 
 
-async def add_unit_with_storage(ops_test, app, storage):
-    """Adds unit with storage.
+async def add_unit_with_storage(ops_test, app, storages):
+    """Adds unit with storages.
 
     Note: this function exists as a temporary solution until this issue is resolved:
     https://github.com/juju/python-libjuju/issues/695
     """
     original_units = {unit.name for unit in ops_test.model.applications[app].units}
     model_name = ops_test.model.info.name
-    add_unit_cmd = f"add-unit {app} --model={model_name} --attach-storage={storage}".split()
+    add_unit_cmd = f"add-unit {app} --model={model_name}"
+    for storage in storages:
+        add_unit_cmd = add_unit_cmd + f" --attach-storage={storage}"
+    add_unit_cmd = add_unit_cmd.split()
     return_code, _, _ = await ops_test.juju(*add_unit_cmd)
     assert return_code == 0, "Failed to add unit with storage"
     async with ops_test.fast_forward():
@@ -958,7 +963,9 @@ async def add_unit_with_storage(ops_test, app, storage):
 
     # verify storage attached
     new_unit = (current_units - original_units).pop()
-    assert storage_id(ops_test, new_unit) == storage, "unit added with incorrect storage"
+    assert sorted(get_storage_ids(ops_test, new_unit)) == sorted(storages), (
+        "unit added with incorrect storage"
+    )
 
     # return a reference to newly added unit
     for unit in ops_test.model.applications[app].units:
@@ -1050,18 +1057,22 @@ async def check_db(ops_test: OpsTest, app: str, db: str) -> bool:
     return db in query
 
 
-async def get_any_deatached_storage(ops_test: OpsTest) -> str:
-    """Returns any of the current available deatached storage."""
+async def get_detached_storages(ops_test: OpsTest) -> list[str]:
+    """Returns the current available detached storage."""
     return_code, storages_list, stderr = await ops_test.juju(
         "storage", "-m", f"{ops_test.controller_name}:{ops_test.model.info.name}", "--format=json"
     )
     if return_code != 0:
         raise Exception(f"failed to get storages info with error: {stderr}")
 
     parsed_storages_list = json.loads(storages_list)
+    detached_storages = []
     for storage_name, storage in parsed_storages_list["storage"].items():
         if (str(storage["status"]["current"]) == "detached") and (str(storage["life"] == "alive")):
-            return storage_name
+            detached_storages.append(storage_name)
+
+    if len(detached_storages) > 0:
+        return detached_storages
 
     raise Exception("failed to get deatached storage")
 

tests/integration/ha_tests/test_restore_cluster.py

@@ -17,8 +17,8 @@
 )
 from .helpers import (
     add_unit_with_storage,
+    get_storage_ids,
     reused_full_cluster_recovery_storage,
-    storage_id,
 )
 
 FIRST_APPLICATION = "first-cluster"
@@ -40,7 +40,12 @@ async def test_build_and_deploy(ops_test: OpsTest, charm) -> None:
             application_name=FIRST_APPLICATION,
             num_units=3,
             base=CHARM_BASE,
-            storage={"pgdata": {"pool": "lxd-btrfs", "size": 2048}},
+            storage={
+                "archive": {"pool": "lxd-btrfs", "size": 8046},
+                "data": {"pool": "lxd-btrfs", "size": 8046},
+                "logs": {"pool": "lxd-btrfs", "size": 8046},
+                "temp": {"pool": "lxd-btrfs", "size": 8046},
+            },
             config={"profile": "testing"},
         )
 
@@ -91,7 +96,7 @@ async def test_cluster_restore(ops_test):
     logger.info("Downscaling the existing cluster")
     storages = []
     for unit in ops_test.model.applications[FIRST_APPLICATION].units:
-        storages.append(storage_id(ops_test, unit.name))
+        storages.append(get_storage_ids(ops_test, unit.name))
         await ops_test.model.destroy_unit(unit.name)
 
     await ops_test.model.remove_application(FIRST_APPLICATION, block_until_done=True)