Address PR feedback + fix broken upgrades

Author: shayancanonical

poetry.lock

@@ -10,6 +10,7 @@ authors = []
 
 [tool.poetry.dependencies]
 python = "^3.8.1"  # ^3.8.1 required by flake8
+# there is a breaking change in ops 2.10.0: https://github.com/canonical/operator/pull/1091#issuecomment-1888644075
 ops = "<2.10.0"
 tenacity = "^8.2.3"
 poetry-core = "^1.7.0"
@@ -18,6 +19,7 @@ requests = "^2.31.0"
 
 [tool.poetry.group.charm-libs.dependencies]
 # data_platform_libs/v0/data_interfaces.py
+# there is a breaking change in ops 2.10.0: https://github.com/canonical/operator/pull/1091#issuecomment-1888644075
 ops = "<2.10.0"
 # tls_certificates_interface/v2/tls_certificates.py
 # tls_certificates lib v2 uses a feature only available in cryptography >=42.0.5
@@ -53,7 +55,7 @@ pytest = "^7.4.0"
 pytest-xdist = "^3.3.1"
 pytest-cov = "^4.1.0"
 ops-scenario = "^5.4.1"
-ops = "<2.10.0"
+ops = ">=2.0.0"
 pytest-mock = "^3.11.1"
 
 [tool.poetry.group.integration.dependencies]
@@ -65,7 +67,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.10.0"
+ops = ">=2.0.0"
 pytest-mock = "^3.11.1"
 
 

pyproject.toml

@@ -109,10 +109,9 @@ def _exposed_read_write_endpoint(self) -> str:
     def _exposed_read_only_endpoint(self) -> str:
         """The exposed read-only endpoint"""
 
-    @property
     @abc.abstractmethod
-    def is_exposed(self) -> typing.Optional[bool]:
-        """Whether router is exposed externally"""
+    def is_externally_accessible(self, event=None) -> typing.Optional[bool]:
+        """Whether router is externally accessible"""
 
     @property
     def _tls_certificate_saved(self) -> bool:
@@ -265,7 +264,9 @@ def reconcile(self, event=None) -> None:  # noqa: C901
             if self._upgrade.unit_state == "outdated":
                 if self._upgrade.authorized:
                     self._upgrade.upgrade_unit(
-                        workload_=workload_, tls=self._tls_certificate_saved
+                        workload_=workload_,
+                        tls=self._tls_certificate_saved,
+                        exporter_config=self._cos_exporter_config(event),
                     )
                 else:
                     self.set_status(event=event)

src/abstract_charm.py

@@ -83,16 +83,15 @@ def _exposed_read_write_endpoint(self) -> str:
     def _exposed_read_only_endpoint(self) -> str:
         return f"{self.host_address}:{self._READ_ONLY_PORT}"
 
-    @property
-    def is_exposed(self) -> typing.Optional[bool]:
-        return self._database_provides.external_connectivity
+    def is_externally_accessible(self, event=None) -> typing.Optional[bool]:
+        return self._database_provides.external_connectivity(event)
 
     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_externally_accessible():
             ports = [self._READ_WRITE_PORT, self._READ_ONLY_PORT]
         else:
             ports = []
@@ -108,7 +107,7 @@ def wait_until_mysql_router_ready(self) -> None:
                 wait=tenacity.wait_fixed(5),
             ):
                 with attempt:
-                    if self.is_exposed:
+                    if self.is_externally_accessible():
                         for port in (
                             self._READ_WRITE_PORT,
                             self._READ_ONLY_PORT,

src/machine_charm.py

@@ -51,6 +51,8 @@ def enable(self) -> None:
 
     def disable(self) -> None:
         logger.debug("Removing cron job for log rotation of mysqlrouter")
-        self._logrotate_config.unlink()
-        self._cron_file.unlink()
+        if self._logrotate_config.exists():
+            self._logrotate_config.unlink()
+        if self._cron_file.exists():
+            self._cron_file.unlink()
         logger.debug("Removed cron job for log rotation of mysqlrouter")

src/machine_logrotate.py

@@ -17,6 +17,9 @@
 import upgrade
 import workload
 
+if typing.TYPE_CHECKING:
+    import relations.cos
+
 logger = logging.getLogger(__name__)
 
 
@@ -135,9 +138,15 @@ def authorized(self) -> bool:
                 return False
         return False
 
-    def upgrade_unit(self, *, workload_: workload.Workload, tls: bool) -> None:
+    def upgrade_unit(
+        self,
+        *,
+        workload_: workload.Workload,
+        tls: bool,
+        exporter_config: "relations.cos.ExporterConfig",
+    ) -> None:
         logger.debug(f"Upgrading {self.authorized=}")
         self.unit_state = "upgrading"
-        workload_.upgrade(unit=self._unit, tls=tls)
+        workload_.upgrade(unit=self._unit, tls=tls, exporter_config=exporter_config)
         self._unit_databag["snap_revision"] = snap.REVISION
         logger.debug(f"Saved {snap.REVISION=} in unit databag after upgrade")

src/machine_upgrade.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_externally_accessible():
             command.extend(
                 [
                     "--conf-bind-address",
@@ -35,17 +35,18 @@ def _get_bootstrap_command(self, password: str) -> typing.List[str]:
                     # set. Workaround for https://bugs.mysql.com/bug.php?id=107291
                     "--conf-set-option",
                     "DEFAULT.server_ssl_mode=PREFERRED",
+                    "--conf-skip-tcp",
                 ]
             )
         return command
 
-    def _update_configured_socket_file_locations_and_bind_address(self) -> None:
+    def _update_configured_socket_file_locations(self) -> None:
         """Update configured socket file locations from `/tmp` to `/run/mysqlrouter`.
 
         Called after MySQL Router bootstrap & before MySQL Router service is enabled
 
         Change configured location of socket files before socket files are created by MySQL Router
-        service. Also remove bind_address and bind_port for all router services: rw, ro, x_rw, x_ro
+        service.
 
         Needed since `/tmp` inside a snap is not accessible to non-root users. The socket files
         must be accessible to applications related via database_provides endpoint.
@@ -59,14 +60,12 @@ def _update_configured_socket_file_locations_and_bind_address(self) -> None:
             section["socket"] = str(
                 self._container.path("/run/mysqlrouter") / pathlib.PurePath(section["socket"]).name
             )
-            del section["bind_address"]
-            del section["bind_port"]
         with io.StringIO() as output:
             config.write(output)
             self._container.router_config_file.write_text(output.getvalue())
         logger.debug("Updated configured socket file locations")
 
     def _bootstrap_router(self, *, tls: bool) -> None:
         super()._bootstrap_router(tls=tls)
-        if not self._charm.is_exposed:
-            self._update_configured_socket_file_locations_and_bind_address()
+        if not self._charm.is_externally_accessible():
+            self._update_configured_socket_file_locations()

src/machine_workload.py

@@ -38,10 +38,9 @@ def __init__(
             charm_
         )
 
-    @property
-    def external_connectivity(self) -> bool:
-        """Whether the relation is exposed"""
-        return self._database_provides.external_connectivity
+    def external_connectivity(self, event) -> bool:
+        """Whether any of the relations are marked as external."""
+        return self._database_provides.external_connectivity(event)
 
     def reconcile_users(
         self,

src/relations/database_providers_wrapper.py

@@ -74,7 +74,7 @@ def __init__(
         # (e.g. when related to `data-integrator` charm)
         # Implements DA073 - Add Expose Flag to the Database Interface
         # https://docs.google.com/document/d/1Y7OZWwMdvF8eEMuVKrqEfuFV3JOjpqLHL7_GPqJpRHU
-        self._external_connectivity = databag.get("external-node-connectivity") == "true"
+        self.external_connectivity = databag.get("external-node-connectivity") == "true"
         if databag.get("extra-user-roles"):
             raise _UnsupportedExtraUserRole(
                 app_name=relation.app.name, endpoint_name=relation.name
@@ -125,13 +125,11 @@ def create_database_and_user(
 
         rw_endpoint = (
             exposed_read_write_endpoint
-            if self._external_connectivity
+            if self.external_connectivity
             else router_read_write_endpoint
         )
         ro_endpoint = (
-            exposed_read_only_endpoint
-            if self._external_connectivity
-            else router_read_only_endpoint
+            exposed_read_only_endpoint if self.external_connectivity else router_read_only_endpoint
         )
 
         self._set_databag(
@@ -199,13 +197,23 @@ def _shared_users(self) -> typing.List[_RelationWithSharedUser]:
                 pass
         return shared_users
 
-    @property
-    def external_connectivity(self) -> bool:
-        """Whether the relation is exposed."""
-        relation_data = self._interface.fetch_relation_data(fields=["external-node-connectivity"])
-        return any(
-            [data.get("external-node-connectivity") == "true" for data in relation_data.values()]
-        )
+    def external_connectivity(self, event) -> bool:
+        """Whether any of the relations are marked as external."""
+        requested_users = []
+        for relation in self._interface.relations:
+            try:
+                requested_users.append(
+                    _RelationThatRequestedUser(
+                        relation=relation, interface=self._interface, event=event
+                    )
+                )
+            except (
+                _RelationBreaking,
+                remote_databag.IncompleteDatabag,
+                _UnsupportedExtraUserRole,
+            ):
+                pass
+        return any(relation.external_connectivity for relation in requested_users)
 
     def reconcile_users(
         self,

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_externally_accessible():
             sans_ip.append(self._charm.host_address)
 
         return tls_certificates.generate_csr(