fix: Post review (new round)

.codespellignore

@@ -1,2 +1,5 @@
+# Copyright 2024 Canonical Ltd.
+# See LICENSE file for licensing details.
+
 juju
 charm

.gitignore

@@ -270,3 +270,5 @@ modules.xml
 # Azure Toolkit for IntelliJ plugin
 # https://plugins.jetbrains.com/plugin/8053-azure-toolkit-for-intellij
 .idea/**/azureSettings.xml
+
+# TODO: Add TF files to ignore

README.md

@@ -49,7 +49,7 @@ pre-commit run --all-files
 
 ## Project and community
 
-Charmed Mongos K8s is an open source project that warmly welcomes community contributions, suggestions, fixes, and constructive feedback.
+Mongo Charms Single Kernel library is an open source project that warmly welcomes community contributions, suggestions, fixes, and constructive feedback.
 
 * Check our [Code of Conduct](https://ubuntu.com/community/ethos/code-of-conduct)
 * Raise software issues or feature requests on [GitHub](https://github.com/canonical/mongo-single-kernel-library/issues)

single_kernel_mongo/abstract_charm.py

@@ -1,6 +1,10 @@
 # Copyright 2024 Canonical Ltd.
 # See LICENSE file for licensing details.
-"""Skeleton for the abstract charm."""
+"""Skeleton for the abstract charm.
+
+This abstract class is inherited by all actual charms that need to define the different ClassVar.
+An example can be found in ../tests/unit/mongodb_test_charm/src/charm.py.
+"""
 
 import logging
 from typing import ClassVar, Generic, TypeVar

single_kernel_mongo/config/literals.py

@@ -9,8 +9,6 @@
 
 LOCALHOST = "127.0.0.1"
 
-CONTAINER = "mongod"
-
 
 class Substrates(str, Enum):
     """Possible substrates."""

single_kernel_mongo/config/models.py

@@ -33,7 +33,7 @@ class AuditLogConfig:
     destination: str = "file"
 
 
-@dataclass
+@dataclass(frozen=True)
 class Role:
     """Defines a role for the charm."""
 

single_kernel_mongo/config/relations.py

@@ -9,14 +9,14 @@
 class PeerRelationNames(str, Enum):
     """The peer relation names."""
 
-    PEERS = "database-peers"  # check.
+    PEERS = "database-peers"
     ROUTER_PEERS = "router-peers"
 
 
 class RelationNames(str, Enum):
     """The different relations."""
 
-    DATABASE = "database"  # In progress
+    DATABASE = "database"
     SHARDING = "sharding"
     CONFIG_SERVER = "config-server"
     CLUSTER = "cluster"

single_kernel_mongo/core/k8s_worload.py

@@ -94,7 +94,7 @@ def write(self, path: Path, content: str, mode: str = "w") -> None:
         )
 
     @override
-    def delete(self, path: Path):
+    def delete(self, path: Path) -> None:
         self.container.remove_path(path)
 
     @override
@@ -113,7 +113,7 @@ def get_env(self) -> dict[str, str]:
         )
 
     @override
-    def update_env(self, parameters: chain[str]):
+    def update_env(self, parameters: chain[str]) -> None:
         self._env = " ".join(parameters)
 
     @override

single_kernel_mongo/core/operator.py

@@ -14,6 +14,7 @@
 from ops.model import Unit
 
 from single_kernel_mongo.config.literals import RoleEnum, Substrates
+from single_kernel_mongo.config.models import Role
 from single_kernel_mongo.managers.config import CommonConfigManager
 from single_kernel_mongo.managers.mongo import MongoManager
 from single_kernel_mongo.state.charm_state import CharmState
@@ -42,6 +43,7 @@ class OperatorProtocol(ABC, Object):
     charm: AbstractMongoCharm
     name: ClassVar[RoleEnum]
     substrate: Substrates
+    role: Role
     config_manager: CommonConfigManager
     tls_manager: TLSManager
     state: CharmState

single_kernel_mongo/core/structured_config.py

@@ -2,7 +2,10 @@
 # Copyright 2024 Canonical Ltd.
 # See LICENSE file for licensing details.
 
-"""Structure configuration for the Mongo charms."""
+"""Structured classes for the available configurations for Mongo charms.
+
+Modifiable configurations should be defined in `config.yaml` in each charm.
+"""
 
 from enum import Enum
 from typing import Annotated, TypeVar

single_kernel_mongo/core/vm_workload.py

@@ -38,42 +38,42 @@ class VMWorkload(WorkloadBase):
     def __init__(self, role: Role, container: Container | None) -> None:
         super().__init__(role, container)
         self.snap = SNAP
-        self.mongod = snap.SnapCache()[self.snap.name]
+        self.mongod_snap = snap.SnapCache()[self.snap.name]
 
     @property
     @override
     def workload_present(self) -> bool:
-        return self.mongod.present
+        return self.mongod_snap.present
 
     @override
     def start(self) -> None:
         try:
-            self.mongod.start(services=[self.service])
+            self.mongod_snap.start(services=[self.service])
         except snap.SnapError as e:
             logger.exception(str(e))
             raise WorkloadServiceError(str(e)) from e
 
     @override
     def get_env(self) -> dict[str, str]:
-        return {self.env_var: self.mongod.get(self.snap_param)}
+        return {self.env_var: self.mongod_snap.get(self.snap_param)}
 
     @override
-    def update_env(self, parameters: chain[str]):
+    def update_env(self, parameters: chain[str]) -> None:
         content = " ".join(parameters)
-        self.mongod.set({self.snap_param: content})
+        self.mongod_snap.set({self.snap_param: content})
 
     @override
     def stop(self) -> None:
         try:
-            self.mongod.stop(services=[self.service])
+            self.mongod_snap.stop(services=[self.service])
         except snap.SnapError as e:
             logger.exception(str(e))
             raise WorkloadServiceError(str(e)) from e
 
     @override
     def restart(self) -> None:
         try:
-            self.mongod.restart(services=[self.service])
+            self.mongod_snap.restart(services=[self.service])
         except snap.SnapError as e:
             logger.exception(str(e))
             raise WorkloadServiceError(str(e)) from e
@@ -165,7 +165,7 @@ def run_bin_command(
     )
     def active(self) -> bool:
         try:
-            return self.mongod.services[self.service]["active"]
+            return self.mongod_snap.services[self.service]["active"]
         except KeyError:
             return False
 
@@ -177,12 +177,12 @@ def install(self) -> bool:
             True if successfully installed. False otherwise.
         """
         try:
-            self.mongod.ensure(
+            self.mongod_snap.ensure(
                 snap.SnapState.Latest,
                 channel=self.snap.channel,
                 revision=self.snap.revision,
             )
-            self.mongod.hold()
+            self.mongod_snap.hold()
 
             return True
         except snap.SnapError as err:

single_kernel_mongo/events/database.py

@@ -82,7 +82,6 @@ def _on_relation_event(self, event: RelationEvent):
 
         if isinstance(event, RelationBrokenEvent):
             relation_departing = True
-            # TODO: Checks
             if not self.dependent.state.has_departed_run(event.relation.id):
                 defer_event_with_info_log(
                     logger,

single_kernel_mongo/events/lifecycle.py

@@ -24,7 +24,7 @@
 )
 from ops.framework import Object
 
-from single_kernel_mongo.config.literals import Substrates
+from single_kernel_mongo.config.literals import RoleEnum, Substrates
 from single_kernel_mongo.config.relations import PeerRelationNames
 from single_kernel_mongo.core.operator import OperatorProtocol
 from single_kernel_mongo.exceptions import (
@@ -71,12 +71,13 @@ def __init__(self, dependent: OperatorProtocol, rel_name: PeerRelationNames):
             self.charm.on[rel_name.value].relation_departed, self.on_relation_departed
         )
 
-        self.framework.observe(
-            getattr(self.charm.on, "mongodb_storage_attached"), self.on_storage_attached
-        )
-        self.framework.observe(
-            getattr(self.charm.on, "mongodb_storage_detaching"), self.on_storage_detaching
-        )
+        if self.dependent.name == RoleEnum.MONGOD:
+            self.framework.observe(
+                getattr(self.charm.on, "mongodb_storage_attached"), self.on_storage_attached
+            )
+            self.framework.observe(
+                getattr(self.charm.on, "mongodb_storage_detaching"), self.on_storage_detaching
+            )
 
     def on_start(self, event: StartEvent):
         """Start event."""

single_kernel_mongo/managers/backups.py

@@ -17,8 +17,9 @@
 import logging
 import re
 import time
+from enum import Enum
 from functools import cached_property
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, NewType
 
 from ops import Container
 from ops.framework import Object
@@ -55,6 +56,8 @@
 if TYPE_CHECKING:
     from single_kernel_mongo.abstract_charm import AbstractMongoCharm  # pragma: nocover
 
+BackupListType = NewType("BackupListType", list[tuple[str, str, str]])
+
 BACKUP_RESTORE_MAX_ATTEMPTS = 10
 BACKUP_RESTORE_ATTEMPT_COOLDOWN = 15
 REMAPPING_PATTERN = r"\ABackup doesn't match current cluster topology - it has different replica set names. Extra shards in the backup will cause this, for a simple example. The extra/unknown replica set names found in the backup are: ([\w\d\-,\s]+)([.] Backup has no data for the config server or sole replicaset)?\Z"
@@ -68,9 +71,18 @@
     "endpoint": "storage.s3.endpointUrl",
     "storage-class": "storage.s3.storageClass",
 }
+
 logger = logging.getLogger(__name__)
 
 
+class StatusCodeError(str, Enum):
+    """Status codes returned in PBM."""
+
+    MOVED_PERMANENTLY = "status code: 301"
+    FORBIDDEN = "status code: 403"
+    NOTFOUND = "status code: 404"
+
+
 def _backup_restore_retry_before_sleep(retry_state) -> None:
     logger.error(
         f"Attempt {retry_state.attempt_number} failed. {BACKUP_RESTORE_MAX_ATTEMPTS - retry_state.attempt_number} attempts left."
@@ -105,7 +117,11 @@ def __init__(
 
     @cached_property
     def environment(self) -> dict[str, str]:
-        """The environment used to run PBM commands."""
+        """The environment used to run PBM commands.
+
+        For security reason, we never provide the URI via arguments to the PBM
+        CLI. So we provide it as an environment variable.
+        """
         return {self.workload.env_var: self.state.backup_config.uri}
 
     def is_valid_s3_integration(self) -> bool:
@@ -151,12 +167,25 @@ def create_backup_action(self) -> str:  # type: ignore[return]
 
     def list_backup_action(self) -> str:
         """List the backups entries."""
-        backup_list: list[tuple[str, str, str]] = []
+        backup_list: BackupListType = BackupListType([])
         try:
             pbm_status_output = self.pbm_status
         except WorkloadExecError as e:
             raise ListBackupError from e
         pbm_status = json.loads(pbm_status_output)
+
+        finished_backups = self.list_finished_backups(pbm_status=pbm_status)
+        backup_list = self.list_with_backups_in_progress(
+            pbm_status=pbm_status, backup_list=finished_backups
+        )
+
+        # process in progress backups
+
+        return self._format_backup_list(sorted(backup_list, key=lambda pair: pair[0]))
+
+    def list_finished_backups(self, pbm_status: dict) -> BackupListType:
+        """Lists the finished backups from the status."""
+        backup_list: BackupListType = BackupListType([])
         backups = pbm_status.get("backups", {}).get("snapshot", [])
         for backup in backups:
             backup_status = "finished"
@@ -171,8 +200,12 @@ def list_backup_action(self) -> str:
             if backup["status"] not in ["error", "done"]:
                 backup_status = "in progress"
             backup_list.append((backup["name"], backup["type"], backup_status))
+        return backup_list
 
-        # process in progress backups
+    def list_with_backups_in_progress(
+        self, pbm_status: dict, backup_list: BackupListType
+    ) -> BackupListType:
+        """Lists all the backups with the one in progress from the status and finished list."""
         running_backup = pbm_status["running"]
         if running_backup.get("type", None) == "backup":
             # backups are sorted in reverse order
@@ -187,8 +220,7 @@ def list_backup_action(self) -> str:
                 )
             else:
                 backup_list.append((running_backup["name"], "logical", "in progress"))
-
-        return self._format_backup_list(sorted(backup_list, key=lambda pair: pair[0]))
+        return backup_list
 
     @retry(
         stop=stop_after_attempt(BACKUP_RESTORE_MAX_ATTEMPTS),
@@ -352,11 +384,11 @@ def process_pbm_error(self, pbm_status: str) -> str:
         except json.JSONDecodeError:
             error_message = pbm_status
 
-        if "status code: 403" in error_message:
+        if StatusCodeError.FORBIDDEN in error_message:
             message = "s3 credentials are incorrect."
-        elif "status code: 404" in error_message:
+        elif StatusCodeError.NOTFOUND in error_message:
             message = "s3 configurations are incompatible."
-        elif "status code: 301" in error_message:
+        elif StatusCodeError.MOVED_PERMANENTLY in error_message:
             message = "s3 configurations are incompatible."
         return message
 

single_kernel_mongo/managers/config.py

@@ -319,6 +319,8 @@ def __init__(self, config: MongoConfigModel, workload: WorkloadBase, state: Char
     @property
     def config_server_db_parameter(self) -> list[str]:
         """The config server DB parameter."""
+        # In case we are integrated with a config-server, we need to provide
+        # it's URI to mongos so it can connect to it.
         if uri := self.state.cluster.config_server_uri:
             return [f"--configdb {uri}"]
         return [

single_kernel_mongo/managers/mongodb_operator.py

@@ -19,7 +19,6 @@
 from typing_extensions import override
 
 from single_kernel_mongo.config.literals import (
-    CONTAINER,
     MAX_PASSWORD_LENGTH,
     MongoPorts,
     RoleEnum,
@@ -95,7 +94,9 @@ def __init__(self, charm: AbstractMongoCharm):
         )
 
         container = (
-            self.charm.unit.get_container(CONTAINER) if self.substrate == Substrates.K8S else None
+            self.charm.unit.get_container(self.role.name)
+            if self.substrate == Substrates.K8S
+            else None
         )
 
         # Defined workloads and configs
@@ -501,7 +502,7 @@ def on_set_password_action(self, username: str, password: str | None = None) ->
         if user == MonitorUser:
             # Update and restart mongodb exporter.
             self.mongodb_exporter_config_manager.connect()
-        # Rotate password.
+        # TODO: Rotate password.
         if user in (OperatorUser, BackupUser):
             pass