Add common exit hook to coordinator (#85)

* add reconcile func

* static fix

* remove commented out lines

* fix tests

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "hatchling.build"
 
 [project]
 name = "cosl"
-version = "0.0.39"
+version = "0.0.40"
 authors = [
   { name="sed-i", email="82407168+sed-i@users.noreply.github.com" },
 ]

src/cosl/coordinated_workers/coordinator.py

@@ -236,7 +236,6 @@ def __init__(
         self.topology = cosl.JujuTopology.from_charm(self._charm)
         self._external_url = external_url
         self._worker_metrics_port = worker_metrics_port
-
         self._endpoints = endpoints
 
         _validate_container_name(container_name, resources_requests)
@@ -345,41 +344,7 @@ def __init__(
             )
             return
 
-        # lifecycle
-        self.framework.observe(self._charm.on.config_changed, self._on_config_changed)
-
-        # nginx
-        self.framework.observe(self._charm.on.nginx_pebble_ready, self._on_nginx_pebble_ready)
-        self.framework.observe(
-            self._charm.on.nginx_prometheus_exporter_pebble_ready,
-            self._on_nginx_prometheus_exporter_pebble_ready,
-        )
-
-        # s3
-        self.framework.observe(
-            self.s3_requirer.on.credentials_changed, self._on_s3_credentials_changed
-        )
-        self.framework.observe(self.s3_requirer.on.credentials_gone, self._on_s3_credentials_gone)
-
-        # tracing
-        # self.framework.observe(self._charm.on.peers_relation_created, self._on_peers_relation_created)
-        # self.framework.observe(self._charm.on.peers_relation_changed, self._on_peers_relation_changed)
-
-        # logging
-        self.framework.observe(
-            self._logging.on.loki_push_api_endpoint_joined,
-            self._on_loki_relation_changed,
-        )
-        self.framework.observe(
-            self._logging.on.loki_push_api_endpoint_departed,
-            self._on_loki_relation_changed,
-        )
-
-        # tls
-        self.framework.observe(self.cert_handler.on.cert_changed, self._on_cert_handler_changed)
-
-        # cluster
-        self.framework.observe(self.cluster.on.changed, self._on_cluster_changed)
+        self._reconcile()
 
     ######################
     # UTILITY PROPERTIES #
@@ -578,60 +543,11 @@ def _scrape_jobs(self) -> List[Dict[str, Any]]:
     ##################
     # EVENT HANDLERS #
     ##################
-    def _on_cert_handler_changed(self, _: ops.RelationChangedEvent):
-        if self.tls_available:
-            logger.debug("enabling TLS")
-            self.nginx.configure_tls(
-                server_cert=self.cert_handler.server_cert,  # type: ignore
-                ca_cert=self.cert_handler.ca_cert,  # type: ignore
-                private_key=self.cert_handler.private_key,  # type: ignore
-            )
-        else:
-            logger.debug("disabling TLS")
-            self.nginx.delete_certificates()
-
-        # notify the cluster
-        self.update_cluster()
-
-    def _on_cluster_changed(self, _: ops.RelationEvent):
-        self.update_cluster()
-
-    def _on_nginx_pebble_ready(self, _: ops.PebbleReadyEvent):
-        self.update_cluster()
-
-    def _on_nginx_prometheus_exporter_pebble_ready(self, _: ops.PebbleReadyEvent):
-        self.update_cluster()
-
-    def _on_loki_relation_changed(self, _: ops.EventBase):
-        self.update_cluster()
-
-    def _on_s3_credentials_changed(self, _: ops.RelationChangedEvent):
-        self._on_s3_changed()
-
-    def _on_s3_credentials_gone(self, _: ops.RelationChangedEvent):
-        self._on_s3_changed()
-
-    def _on_s3_changed(self):
-        self.update_cluster()
 
     def _on_peers_relation_created(self, event: ops.RelationCreatedEvent):
         if self._local_ip:
             event.relation.data[self._charm.unit]["local-ip"] = self._local_ip
 
-    def _on_peers_relation_changed(self, _: ops.RelationChangedEvent):
-        self.update_cluster()
-
-    def _on_config_changed(self, _: ops.ConfigChangedEvent):
-        if self.tls_available:
-            self.nginx.configure_tls(
-                server_cert=self.cert_handler.server_cert,  # type: ignore
-                ca_cert=self.cert_handler.ca_cert,  # type: ignore
-                private_key=self.cert_handler.private_key,  # type: ignore
-            )
-        else:
-            self.nginx.delete_certificates()
-        self.update_cluster()
-
     # keep this event handler at the bottom
     def _on_collect_unit_status(self, e: ops.CollectStatusEvent):
         # todo add [nginx.workload] statuses
@@ -662,6 +578,30 @@ def _on_collect_unit_status(self, e: ops.CollectStatusEvent):
     ###################
     # UTILITY METHODS #
     ###################
+    def _update_nginx_tls_certificates(self) -> None:
+        """Update the TLS certificates for nginx on disk according to their availability."""
+        if self.tls_available:
+            self.nginx.configure_tls(
+                server_cert=self.cert_handler.server_cert,  # type: ignore
+                ca_cert=self.cert_handler.ca_cert,  # type: ignore
+                private_key=self.cert_handler.private_key,  # type: ignore
+            )
+        else:
+            self.nginx.delete_certificates()
+
+    def _reconcile(self):
+        """Run all logic that is independent of what event we're processing."""
+        # There could be a race between the resource patch and pebble operations
+        # i.e., charm code proceeds beyond a can_connect guard, and then lightkube patches the statefulset
+        # and the workload is no longer available.
+        # `resources_patch` might be `None` when no resources requests or limits are requested by the charm.
+        if self.resources_patch and not self.resources_patch.is_ready():
+            logger.debug("Resource patch not ready yet. Skipping cluster update step.")
+            return
+
+        self._update_nginx_tls_certificates()
+        self.update_cluster()
+
     @property
     def _peers(self) -> Optional[Set[ops.model.Unit]]:
         relation = self.model.get_relation("peers")
@@ -698,14 +638,6 @@ def loki_endpoints_by_unit(self) -> Dict[str, str]:
 
     def update_cluster(self):
         """Build the workers config and distribute it to the relations."""
-        # There could be a race between the resource patch and pebble operations
-        # i.e., charm code proceeds beyond a can_connect guard, and then lightkube patches the statefulset
-        # and the workload is no longer available.
-        # `resources_patch` might be `None` when no resources requests or limits are requested by the charm.
-        if self.resources_patch and not self.resources_patch.is_ready():
-            logger.debug("Resource patch not ready yet. Skipping cluster update step.")
-            return
-
         self.nginx.configure_pebble_layer()
         self.nginx_exporter.configure_pebble_layer()
         if not self.is_coherent:

src/cosl/coordinated_workers/nginx.py

@@ -60,6 +60,26 @@ def are_certificates_on_disk(self) -> bool:
     def configure_tls(self, private_key: str, server_cert: str, ca_cert: str) -> None:
         """Save the certificates file to disk and run update-ca-certificates."""
         if self._container.can_connect():
+            # Read the current content of the files (if they exist)
+            current_server_cert = (
+                self._container.pull(CERT_PATH).read() if self._container.exists(CERT_PATH) else ""
+            )
+            current_private_key = (
+                self._container.pull(KEY_PATH).read() if self._container.exists(KEY_PATH) else ""
+            )
+            current_ca_cert = (
+                self._container.pull(CA_CERT_PATH).read()
+                if self._container.exists(CA_CERT_PATH)
+                else ""
+            )
+
+            if (
+                current_server_cert == server_cert
+                and current_private_key == private_key
+                and current_ca_cert == ca_cert
+            ):
+                # No update needed
+                return
             self._container.push(KEY_PATH, private_key, make_dirs=True)
             self._container.push(CERT_PATH, server_cert, make_dirs=True)
             self._container.push(CA_CERT_PATH, ca_cert, make_dirs=True)
@@ -69,9 +89,12 @@ def configure_tls(self, private_key: str, server_cert: str, ca_cert: str) -> Non
     def delete_certificates(self) -> None:
         """Delete the certificate files from disk and run update-ca-certificates."""
         if self._container.can_connect():
-            self._container.remove_path(CERT_PATH, recursive=True)
-            self._container.remove_path(KEY_PATH, recursive=True)
-            self._container.remove_path(CA_CERT_PATH, recursive=True)
+            if self._container.exists(CERT_PATH):
+                self._container.remove_path(CERT_PATH, recursive=True)
+            if self._container.exists(KEY_PATH):
+                self._container.remove_path(KEY_PATH, recursive=True)
+            if self._container.exists(CA_CERT_PATH):
+                self._container.remove_path(CA_CERT_PATH, recursive=True)
             # FIXME: uncomment as soon as the nginx image contains the ca-certificates package
             # self._container.exec(["update-ca-certificates", "--fresh"])
 
@@ -142,13 +165,24 @@ def __init__(self, charm: CharmBase, options: Optional[NginxMappingOverrides] =
 
     def configure_pebble_layer(self) -> None:
         """Configure pebble layer."""
-        self._container.add_layer("nginx-prometheus-exporter", self.layer, combine=True)
-        self._container.autostart()
+        if self._container.can_connect():
+            self._container.add_layer("nginx-prometheus-exporter", self.layer, combine=True)
+            self._container.autostart()
+
+    @property
+    def are_certificates_on_disk(self) -> bool:
+        """Return True if the certificates files are on disk."""
+        return (
+            self._container.can_connect()
+            and self._container.exists(CERT_PATH)
+            and self._container.exists(KEY_PATH)
+            and self._container.exists(CA_CERT_PATH)
+        )
 
     @property
     def layer(self) -> pebble.Layer:
         """Return the Pebble layer for Nginx Prometheus exporter."""
-        scheme = "https" if self._charm.coordinator.tls_available else "http"  # type: ignore
+        scheme = "https" if self.are_certificates_on_disk else "http"  # type: ignore
         return pebble.Layer(
             {
                 "summary": "nginx prometheus exporter layer",

tests/test_coordinated_workers/test_coordinator.py

@@ -119,8 +119,8 @@ def __init__(self, framework: Framework):
                     "tracing": "my-tracing",
                     "s3": "my-s3",
                 },
-                nginx_config=lambda coordinator: f"nginx configuration for {coordinator.name}",
-                workers_config=lambda coordinator: f"workers configuration for {coordinator.name}",
+                nginx_config=lambda coordinator: f"nginx configuration for {coordinator._charm.meta.name}",
+                workers_config=lambda coordinator: f"workers configuration for {coordinator._charm.meta.name}",
                 # nginx_options: Optional[NginxMappingOverrides] = None,
                 # is_coherent: Optional[Callable[[ClusterProvider, ClusterRolesConfig], bool]] = None,
                 # is_recommended: Optional[Callable[[ClusterProvider, ClusterRolesConfig], bool]] = None,