Address PR feedback

Author: shayancanonical

poetry.lock

@@ -10,15 +10,15 @@ authors = []
 
 [tool.poetry.dependencies]
 python = "^3.8.1"  # ^3.8.1 required by flake8
-ops = "^2.8.0"
+ops = "^2.12.0"
 tenacity = "^8.2.3"
 poetry-core = "^1.7.0"
 jinja2 = "^3.1.2"
 requests = "^2.31.0"
 
 [tool.poetry.group.charm-libs.dependencies]
 # data_platform_libs/v0/data_interfaces.py
-ops = ">=2.8.0"
+ops = ">=2.12.0"
 # tls_certificates_interface/v1/tls_certificates.py
 cryptography = ">=42.0.5"
 jsonschema = "*"
@@ -52,7 +52,7 @@ pytest = "^7.4.0"
 pytest-xdist = "^3.3.1"
 pytest-cov = "^4.1.0"
 ops-scenario = "^5.4.1"
-ops = ">=2.8.0"
+ops = ">=2.12.0"
 pytest-mock = "^3.11.1"
 
 [tool.poetry.group.integration.dependencies]
@@ -64,7 +64,7 @@ pytest-github-secrets = {git = "https://github.com/canonical/data-platform-workf
 juju = "3.2.0.1"
 mysql-connector-python = "~8.0.33"
 tenacity = "^8.2.2"
-ops = ">=2.8.0"
+ops = ">=2.12.0"
 pytest-mock = "^3.11.1"
 
 

pyproject.toml

@@ -5,11 +5,9 @@
 
 import abc
 import logging
-import socket
 import typing
 
 import ops
-import tenacity
 
 import container
 import lifecycle
@@ -31,6 +29,8 @@ class MySQLRouterCharm(ops.CharmBase, abc.ABC):
 
     _READ_WRITE_PORT = 6446
     _READ_ONLY_PORT = 6447
+    _READ_WRITE_X_PORT = 6448
+    _READ_ONLY_X_PORT = 6449
 
     def __init__(self, *args) -> None:
         super().__init__(*args)
@@ -111,32 +111,28 @@ def _exposed_read_only_endpoint(self) -> str:
 
     @property
     @abc.abstractmethod
+    def is_exposed(self) -> typing.Optional[bool]:
+        """Whether router is exposed externally"""
+
+    @property
     def _tls_certificate_saved(self) -> bool:
         """Whether a TLS certificate is available to use"""
+        return self.tls.certificate_saved
 
     @property
-    @abc.abstractmethod
     def _tls_key(self) -> typing.Optional[str]:
         """Custom TLS key"""
+        return self.tls.key
 
     @property
-    @abc.abstractmethod
     def _tls_certificate_authority(self) -> typing.Optional[str]:
         """Custom TLS certificate authority"""
+        return self.tls.certificate_authority
 
     @property
-    @abc.abstractmethod
     def _tls_certificate(self) -> typing.Optional[str]:
         """Custom TLS certificate"""
-
-    @property
-    @abc.abstractmethod
-    def _substrate(self) -> str:
-        """Returns the substrate of the charm: vm or k8s"""
-
-    @abc.abstractmethod
-    def is_exposed(self, relation=None) -> typing.Optional[bool]:
-        """Whether router is exposed externally"""
+        return self.tls.certificate
 
     def _cos_exporter_config(self, event) -> typing.Optional[relations.cos.ExporterConfig]:
         """Returns the exporter config for MySQLRouter exporter if cos relation exists"""
@@ -212,37 +208,12 @@ def set_status(self, *, event, app=True, unit=True) -> None:
             self.unit.status = self._determine_unit_status(event=event)
             logger.debug(f"Set unit status to {self.unit.status}")
 
+    @abc.abstractmethod
     def wait_until_mysql_router_ready(self) -> None:
         """Wait until a connection to MySQL Router is possible.
 
         Retry every 5 seconds for up to 30 seconds.
         """
-        logger.debug("Waiting until MySQL Router is ready")
-        self.unit.status = ops.MaintenanceStatus("MySQL Router starting")
-        try:
-            for attempt in tenacity.Retrying(
-                reraise=True,
-                stop=tenacity.stop_after_delay(30),
-                wait=tenacity.wait_fixed(5),
-            ):
-                with attempt:
-                    if self._substrate == "k8s" or self.is_exposed():
-                        for port in (6446, 6447):
-                            with socket.socket() as s:
-                                assert s.connect_ex(("localhost", port)) == 0
-                    else:
-                        for socket_file in (
-                            "/run/mysqlrouter/mysql.sock",
-                            "/run/mysqlrouter/mysqlro.sock",
-                        ):
-                            assert self._container.path(socket_file).exists()
-                            with socket.socket(socket.AF_UNIX) as s:
-                                assert s.connect_ex(str(self._container.path(socket_file))) == 0
-        except AssertionError:
-            logger.exception("Unable to connect to MySQL Router")
-            raise
-        else:
-            logger.debug("MySQL Router is ready")
 
     @abc.abstractmethod
     def _reconcile_node_port(self, event) -> None:

src/abstract_charm.py

@@ -7,9 +7,11 @@
 """MySQL Router machine charm"""
 
 import logging
+import socket
 import typing
 
 import ops
+import tenacity
 
 import abstract_charm
 import machine_logrotate
@@ -82,41 +84,52 @@ def _exposed_read_only_endpoint(self) -> str:
         return f"{self.host_address}:{self._READ_ONLY_PORT}"
 
     @property
-    def _tls_certificate_saved(self) -> bool:
-        """Whether a TLS certificate is available to use"""
-        return self.tls.certificate_saved
-
-    @property
-    def _tls_key(self) -> typing.Optional[str]:
-        """Custom TLS key"""
-        return self.tls.key
-
-    @property
-    def _tls_certificate(self) -> typing.Optional[str]:
-        """Custom TLS certificate"""
-        return self.tls.certificate
-
-    @property
-    def _tls_certificate_authority(self) -> typing.Optional[str]:
-        return self.tls.certificate_authority
-
-    def is_exposed(self, relation=None) -> typing.Optional[bool]:
-        return self._database_provides.is_exposed
+    def is_exposed(self) -> typing.Optional[bool]:
+        return self._database_provides.external_connectivity
 
     def _reconcile_node_port(self, event) -> None:
         """Only applies to Kubernetes charm, so no-op."""
         pass
 
     def _reconcile_ports(self) -> None:
-        if self.is_exposed():
+        if self.is_exposed:
             ports = [self._READ_WRITE_PORT, self._READ_ONLY_PORT]
         else:
             ports = []
         self.unit.set_ports(*ports)
 
-    @property
-    def _substrate(self) -> str:
-        return "vm"
+    def wait_until_mysql_router_ready(self) -> None:
+        logger.debug("Waiting until MySQL Router is ready")
+        self.unit.status = ops.MaintenanceStatus("MySQL Router starting")
+        try:
+            for attempt in tenacity.Retrying(
+                reraise=True,
+                stop=tenacity.stop_after_delay(30),
+                wait=tenacity.wait_fixed(5),
+            ):
+                with attempt:
+                    if self.is_exposed:
+                        for port in (
+                            self._READ_WRITE_PORT,
+                            self._READ_ONLY_PORT,
+                            self._READ_WRITE_X_PORT,
+                            self._READ_ONLY_X_PORT,
+                        ):
+                            with socket.socket() as s:
+                                assert s.connect_ex(("localhost", port)) == 0
+                    else:
+                        for socket_file in (
+                            "/run/mysqlrouter/mysql.sock",
+                            "/run/mysqlrouter/mysqlro.sock",
+                        ):
+                            assert self._container.path(socket_file).exists()
+                            with socket.socket(socket.AF_UNIX) as s:
+                                assert s.connect_ex(str(self._container.path(socket_file))) == 0
+        except AssertionError:
+            logger.exception("Unable to connect to MySQL Router")
+            raise
+        else:
+            logger.debug("MySQL Router is ready")
 
     # =======================
     #  Handlers

src/machine_charm.py

@@ -20,7 +20,7 @@ class AuthenticatedMachineWorkload(workload.AuthenticatedWorkload):
     # TODO python3.10 min version: Use `list` instead of `typing.List`
     def _get_bootstrap_command(self, password: str) -> typing.List[str]:
         command = super()._get_bootstrap_command(password)
-        if self._charm.is_exposed():
+        if self._charm.is_exposed:
             command.extend(
                 [
                     "--conf-bind-address",
@@ -68,5 +68,5 @@ def _update_configured_socket_file_locations_and_bind_address(self) -> None:
 
     def _bootstrap_router(self, *, tls: bool) -> None:
         super()._bootstrap_router(tls=tls)
-        if not self._charm.is_exposed():
+        if not self._charm.is_exposed:
             self._update_configured_socket_file_locations_and_bind_address()

src/machine_workload.py

@@ -39,9 +39,9 @@ def __init__(
         )
 
     @property
-    def is_exposed(self) -> bool:
+    def external_connectivity(self) -> bool:
         """Whether the relation is exposed"""
-        return self._database_provides.is_exposed
+        return self._database_provides.external_connectivity
 
     def reconcile_users(
         self,

src/relations/database_providers_wrapper.py

@@ -181,7 +181,6 @@ class RelationEndpoint:
 
     def __init__(self, charm_: "abstract_charm.MySQLRouterCharm") -> None:
         self._interface = data_interfaces.DatabaseProvides(charm_, relation_name=self._NAME)
-        self._charm = charm_
 
         charm_.framework.observe(charm_.on[self._NAME].relation_created, charm_.reconcile)
         charm_.framework.observe(self._interface.on.database_requested, charm_.reconcile)
@@ -201,7 +200,7 @@ def _shared_users(self) -> typing.List[_RelationWithSharedUser]:
         return shared_users
 
     @property
-    def is_exposed(self) -> bool:
+    def external_connectivity(self) -> bool:
         """Whether the relation is exposed."""
         relation_data = self._interface.fetch_relation_data(fields=["external-node-connectivity"])
         return any(

src/relations/database_provides.py

@@ -114,7 +114,7 @@ def save_certificate(self, event: tls_certificates.CertificateAvailableEvent) ->
     def _generate_csr(self, key: bytes) -> bytes:
         """Generate certificate signing request (CSR)."""
         sans_ip = ["127.0.0.1"]  # needed for the HTTP server when related with COS
-        if self._charm.is_exposed():
+        if self._charm.is_exposed:
             sans_ip.append(self._charm.host_address)
 
         return tls_certificates.generate_csr(

src/relations/tls.py

@@ -101,9 +101,8 @@ def _tls_config_file_data(self) -> str:
     @property
     def _custom_certificate(self) -> typing.Optional[str]:
         """Whether custom TLS certs are enabled for MySQL Router"""
-        if not self._tls_key_file.exists() or not self._tls_certificate_file.exists():
-            return None
-        return self._tls_certificate_file.read_text()
+        if self._tls_key_file.exists() and self._tls_certificate_file.exists():
+            return self._tls_certificate_file.read_text()
 
     def cleanup_monitoring_user(self) -> None:
         """Clean up router REST API user for mysqlrouter exporter."""
@@ -333,6 +332,7 @@ def reconcile(
                 "`key`, `certificate`, and `certificate_authority` arguments required when tls=True"
             )
 
+        # `self._custom_certificate` will change after we enable/disable TLS
         custom_certificate = self._custom_certificate
         if tls:
             self._enable_tls(
@@ -348,7 +348,10 @@ def reconcile(
         # If the host or port changes, MySQL Router will receive topology change
         # notifications from MySQL.
         # Therefore, if the host or port changes, we do not need to restart MySQL Router.
-        if not self._container.mysql_router_service_enabled:
+        is_charm_exposed = self._charm.is_exposed
+        socket_file_exists = self._container.path("/run/mysqlrouter/mysql.sock").exists()
+        require_rebootstrap = is_charm_exposed == socket_file_exists
+        if not self._container.mysql_router_service_enabled or require_rebootstrap:
             logger.debug("Enabling MySQL Router service")
             self._cleanup_after_upgrade_or_potential_container_restart()
             # create an empty credentials file, if the file does not exist

src/workload.py

@@ -33,7 +33,7 @@ def disable_tenacity_retry(monkeypatch):
 @pytest.fixture(autouse=True)
 def patch(monkeypatch):
     monkeypatch.setattr(
-        "abstract_charm.MySQLRouterCharm.wait_until_mysql_router_ready",
+        "machine_charm.MachineSubordinateRouterCharm.wait_until_mysql_router_ready",
         lambda *args, **kwargs: None,
     )
     monkeypatch.setattr("workload.AuthenticatedWorkload._router_username", "")