Prevent unnecessary restarts (#74)

* update update-logic

* Update tests/test_coordinated_workers/test_worker.py

Co-authored-by: PietroPasotti <starfire.daemon@gmail.com>

---------

Co-authored-by: PietroPasotti <starfire.daemon@gmail.com>

pyproject.toml

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

src/cosl/coordinated_workers/worker.py

@@ -371,6 +371,7 @@ def _set_pebble_layer(self) -> bool:
 
         current_plan = self._container.get_plan()
         new_layer = self._pebble_layer()
+
         self._add_readiness_check(new_layer)
 
         def diff(layer: Layer, plan: Plan):
@@ -394,7 +395,13 @@ def _add_readiness_check(self, new_layer: Layer):
             return
 
         new_layer.checks["ready"] = Check(
-            "ready", {"override": "replace", "http": {"url": self._readiness_check_endpoint(self)}}
+            "ready",
+            {
+                "override": "replace",
+                # threshold gets added automatically by pebble
+                "threshold": 3,
+                "http": {"url": self._readiness_check_endpoint(self)},
+            },
         )
 
     def _reconcile(self):
@@ -466,6 +473,27 @@ def _update_tls_certificates(self) -> bool:
             ca_cert = tls_data["ca_cert"]
             server_cert = tls_data["server_cert"]
 
+            # Read the current content of the files (if they exist)
+            current_server_cert = (
+                self._container.pull(CERT_FILE).read() if self._container.exists(CERT_FILE) else ""
+            )
+            current_private_key = (
+                self._container.pull(KEY_FILE).read() if self._container.exists(KEY_FILE) else ""
+            )
+            current_ca_cert = (
+                self._container.pull(CLIENT_CA_FILE).read()
+                if self._container.exists(CLIENT_CA_FILE)
+                else ""
+            )
+
+            if (
+                current_server_cert == server_cert
+                and current_private_key == private_key
+                and current_ca_cert == ca_cert
+            ):
+                # No update needed
+                return False
+
             # Save the workload certificates
             self._container.push(CERT_FILE, server_cert or "", make_dirs=True)
             self._container.push(KEY_FILE, private_key or "", make_dirs=True)
@@ -475,6 +503,16 @@ def _update_tls_certificates(self) -> bool:
             # Save the cacert in the charm container for charm traces
             ROOT_CA_CERT.write_text(ca_cert)
         else:
+
+            if not (
+                self._container.exists(CERT_FILE)
+                or self._container.exists(KEY_FILE)
+                or self._container.exists(CLIENT_CA_FILE)
+                or self._container.exists(ROOT_CA_CERT)
+            ):
+                # No update needed
+                return False
+
             self._container.remove_path(CERT_FILE, recursive=True)
             self._container.remove_path(KEY_FILE, recursive=True)
             self._container.remove_path(CLIENT_CA_FILE, recursive=True)

tests/test_coordinated_workers/test_worker.py

@@ -9,10 +9,16 @@
 import yaml
 from ops import Framework
 from ops.pebble import Layer, ServiceStatus
-from scenario import Container, Context, ExecOutput, Mount, Relation, State
+from scenario import Container, Context, ExecOutput, Mount, Relation, Secret, State
 from scenario.runtime import UncaughtCharmError
 
-from cosl.coordinated_workers.worker import CONFIG_FILE, Worker
+from cosl.coordinated_workers.worker import (
+    CERT_FILE,
+    CLIENT_CA_FILE,
+    CONFIG_FILE,
+    KEY_FILE,
+    Worker,
+)
 
 
 @pytest.fixture(autouse=True)
@@ -26,7 +32,13 @@ class MyCharm(ops.CharmBase):
 
     def __init__(self, framework: Framework):
         super().__init__(framework)
-        self.worker = Worker(self, "foo", lambda _: self.layer, {"cluster": "cluster"})
+        self.worker = Worker(
+            self,
+            "foo",
+            lambda _: self.layer,
+            {"cluster": "cluster"},
+            readiness_check_endpoint="http://localhost:3200/ready",
+        )
 
 
 def test_no_roles_error():
@@ -408,3 +420,90 @@ def __init__(self, framework: Framework):
     # THEN the data gets preprocessed
     fs = Path(str(state_out.get_container("foo").get_filesystem(ctx)) + CONFIG_FILE)
     assert fs.read_text() == yaml.safe_dump(new_config)
+
+
+@patch.object(Worker, "_update_worker_config", MagicMock(return_value=False))
+@patch.object(Worker, "_set_pebble_layer", MagicMock(return_value=False))
+@patch.object(Worker, "restart")
+def test_worker_does_not_restart(restart_mock, tmp_path):
+
+    ctx = Context(
+        MyCharm,
+        meta={
+            "name": "foo",
+            "requires": {"cluster": {"interface": "cluster"}},
+            "containers": {"foo": {"type": "oci-image"}},
+        },
+        config={"options": {"role-all": {"type": "boolean", "default": True}}},
+    )
+    relation = Relation(
+        "cluster",
+        remote_app_data={
+            "worker_config": json.dumps("some: yaml"),
+        },
+    )
+    # WHEN the charm receives any event and there are no changes to the config or the layer,
+    #  but some of the services are down
+    container = Container(
+        "foo",
+        can_connect=True,
+    )
+    ctx.run("update_status", State(containers=[container], relations=[relation]))
+
+    assert not restart_mock.called
+
+
+@patch.object(Worker, "_update_worker_config", MagicMock(return_value=False))
+@patch.object(Worker, "_set_pebble_layer", MagicMock(return_value=False))
+@patch.object(Worker, "restart")
+def test_worker_does_not_restart_on_no_cert_changed(restart_mock, tmp_path):
+
+    ctx = Context(
+        MyCharm,
+        meta={
+            "name": "foo",
+            "requires": {"cluster": {"interface": "cluster"}},
+            "containers": {"foo": {"type": "oci-image"}},
+        },
+        config={"options": {"role-all": {"type": "boolean", "default": True}}},
+    )
+    relation = Relation(
+        "cluster",
+        remote_app_data={
+            "worker_config": json.dumps("some: yaml"),
+            "ca_cert": json.dumps("ca"),
+            "server_cert": json.dumps("cert"),
+            "privkey_secret_id": json.dumps("private_id"),
+        },
+    )
+
+    cert = tmp_path / "cert.cert"
+    key = tmp_path / "key.key"
+    client_ca = tmp_path / "client_ca.cert"
+
+    cert.write_text("cert")
+    key.write_text("private")
+    client_ca.write_text("ca")
+
+    container = Container(
+        "foo",
+        can_connect=True,
+        mounts={
+            "cert": Mount(CERT_FILE, cert),
+            "key": Mount(KEY_FILE, key),
+            "client_ca": Mount(CLIENT_CA_FILE, client_ca),
+        },
+    )
+
+    secret = Secret(
+        "secret:private_id",
+        label="private_id",
+        owner="app",
+        contents={0: {"private-key": "private"}},
+    )
+    ctx.run(
+        "update_status",
+        State(leader=True, containers=[container], relations=[relation], secrets=[secret]),
+    )
+
+    assert restart_mock.call_count == 0