fix: properly remove old zNodes during relation-broken (#108)

src/events/provider.py

@@ -49,8 +49,11 @@ def _on_client_relation_updated(self, event: RelationEvent) -> None:
             return
 
         # ACLs created before passwords set to avoid restarting before successful adding
+        # passing event here allows knowledge of broken app, removing it's chroot from ACL list
         try:
-            self.charm.quorum_manager.update_acls()
+            self.charm.quorum_manager.update_acls(
+                event=event if isinstance(event, RelationBrokenEvent) else None
+            )
         except (
             MembersSyncingError,
             MemberNotReadyError,

src/managers/config.py

@@ -390,10 +390,10 @@ def config_changed(self) -> bool:
         config_properties = self.static_properties
 
         properties_changed = set(server_properties) ^ set(config_properties)
-        logger.debug(f"{properties_changed=}")
 
-        jaas_config = self.current_jaas
-        jaas_changed = set(jaas_config) ^ set(self.jaas_config.splitlines())
+        clean_server_jaas = [conf.strip() for conf in self.current_jaas]
+        clean_config_jaas = [conf.strip() for conf in self.jaas_config.splitlines()]
+        jaas_changed = set(clean_server_jaas) ^ set(clean_config_jaas)
 
         log_level_changed = self.log_level not in "".join(self.current_env)
 
@@ -411,8 +411,6 @@ def config_changed(self) -> bool:
             self.set_zookeeper_properties()
 
         if jaas_changed:
-            clean_server_jaas = [conf.strip() for conf in jaas_config]
-            clean_config_jaas = [conf.strip() for conf in self.jaas_config.splitlines()]
             logger.info(
                 (
                     f"Server.{self.state.unit_server.unit_id} updating JAAS config - "

src/managers/quorum.py

@@ -17,6 +17,7 @@
 from kazoo.exceptions import BadArgumentsError
 from kazoo.handlers.threading import KazooTimeoutError
 from kazoo.security import make_acl
+from ops.charm import RelationEvent
 
 from core.cluster import ClusterState
 from literals import CLIENT_PORT
@@ -138,7 +139,7 @@ def _is_child_of(path: str, chroots: Set[str]) -> bool:
 
         return False
 
-    def update_acls(self) -> None:
+    def update_acls(self, event: RelationEvent | None = None) -> None:
         """Compares leader auth config to incoming relation config, applies add/remove actions.
 
         Args:
@@ -176,19 +177,22 @@ def update_acls(self) -> None:
 
             # FIXME: data-platform-libs should handle this when it's implemented
             if client.chroot:
-                requested_chroots.add(client.chroot)
+                if event and client.relation and client.relation.id == event.relation.id:
+                    continue  # skip broken chroots, so they're removed
+                else:
+                    requested_chroots.add(client.chroot)
 
             # Looks for newly related applications not in config yet
             if client.chroot not in leader_chroots:
                 logger.debug(f"CREATE CHROOT - {client.chroot}")
-                zk.create_znode_leader(client.chroot, [generated_acl])
+                zk.create_znode_leader(path=client.chroot, acls=[generated_acl])
 
             # Looks for existing related applications
             logger.debug(f"UPDATE CHROOT - {client.chroot}")
-            zk.set_acls_znode_leader(client.chroot, [generated_acl])
+            zk.set_acls_znode_leader(path=client.chroot, acls=[generated_acl])
 
         # Looks for applications no longer in the relation but still in config
-        for chroot in leader_chroots - requested_chroots:
+        for chroot in sorted(leader_chroots - requested_chroots, reverse=True):
             if not self._is_child_of(chroot, requested_chroots):
                 logger.debug(f"DROP CHROOT - {chroot}")
-                zk.delete_znode_leader(chroot)
+                zk.delete_znode_leader(path=chroot)

tests/integration/test_provider.py

@@ -88,7 +88,9 @@ async def test_deploy_multiple_charms_relate_active(ops_test: OpsTest):
 async def test_scale_up_gets_new_jaas_users(ops_test: OpsTest):
     await ops_test.model.applications[APP_NAME].add_units(count=1)
     await ops_test.model.block_until(lambda: len(ops_test.model.applications[APP_NAME].units) == 4)
-    await ops_test.model.wait_for_idle(apps=[APP_NAME], status="active")
+
+    async with ops_test.fast_forward():
+        await ops_test.model.wait_for_idle(apps=[APP_NAME], status="active", idle_period=30)
 
     assert ping_servers(ops_test)
     for unit in ops_test.model.applications[APP_NAME].units:

tests/unit/test_provider.py

@@ -7,6 +7,7 @@
 
 import pytest
 import yaml
+from ops import RelationBrokenEvent
 from ops.testing import Harness
 
 from charm import ZooKeeperCharm
@@ -80,6 +81,29 @@ def test_client_relation_updated_creates_passwords_with_chroot(harness):
         assert harness.charm.state.cluster.client_passwords
 
 
+def test_client_relation_broken_sets_acls_with_broken_events(harness):
+    with harness.hooks_disabled():
+        app_id = harness.add_relation(REL_NAME, "application")
+        harness.set_leader(True)
+
+    with (
+        patch("core.cluster.ClusterState.stable", new_callable=PropertyMock, return_value=True),
+        patch("managers.quorum.QuorumManager.update_acls") as patched_update_acls,
+        patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_acquire_lock"),
+    ):
+        harness.update_relation_data(app_id, "application", {"chroot": "balrog"})
+        patched_update_acls.assert_called_with(event=None)
+
+    with (
+        patch("core.cluster.ClusterState.stable", new_callable=PropertyMock, return_value=True),
+        patch("managers.quorum.QuorumManager.update_acls") as patched_update_acls,
+        patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_acquire_lock"),
+    ):
+        harness.remove_relation(app_id)
+
+        isinstance(patched_update_acls.call_args_list[0], RelationBrokenEvent)
+
+
 def test_client_relation_broken_removes_passwords(harness):
     with harness.hooks_disabled():
         harness.set_leader(True)

tests/unit/test_quorum.py

@@ -4,13 +4,14 @@
 
 import logging
 from pathlib import Path
+from unittest.mock import DEFAULT, MagicMock, patch
 
 import pytest
 import yaml
 from ops.testing import Harness
 
 from charm import ZooKeeperCharm
-from literals import CHARM_KEY
+from literals import CHARM_KEY, PEER, REL_NAME
 
 logger = logging.getLogger(__name__)
 
@@ -22,9 +23,10 @@
 @pytest.fixture
 def harness():
     harness = Harness(ZooKeeperCharm, meta=METADATA, config=CONFIG, actions=ACTIONS)
-    harness.add_relation("restart", CHARM_KEY)
     upgrade_rel_id = harness.add_relation("upgrade", CHARM_KEY)
+    harness.add_relation("restart", CHARM_KEY)
     harness.update_relation_data(upgrade_rel_id, f"{CHARM_KEY}/0", {"state": "idle"})
+    harness.add_relation(PEER, CHARM_KEY)
     harness._update_config({"init-limit": 5, "sync-limit": 2, "tick-time": 2000})
     harness.begin()
     return harness
@@ -57,3 +59,52 @@ def test_is_child_of_not(harness):
     chroots = {"/gandalf", "/saruman"}
 
     assert not harness.charm.quorum_manager._is_child_of(path=chroot, chroots=chroots)
+
+
+def test_update_acls_correctly_handles_relation_chroots(harness):
+    dummy_leader_znodes = {
+        "/fellowship",
+        "/fellowship/men",
+        "/fellowship/dwarves",
+        "/fellowship/men/aragorn",
+        "/fellowship/men/boromir",
+        "/fellowship/elves/legolas",
+        "/fellowship/dwarves/gimli",
+        "/fellowship/men/aragorn/anduril",
+    }
+
+    with harness.hooks_disabled():
+        app_id = harness.add_relation(REL_NAME, "application")
+        harness.update_relation_data(app_id, "application", {"chroot": "/rohan"})
+        harness.set_leader(True)
+
+    with patch.multiple(
+        "charms.zookeeper.v0.client.ZooKeeperManager",
+        get_leader=DEFAULT,
+        leader_znodes=MagicMock(return_value=dummy_leader_znodes),
+        create_znode_leader=DEFAULT,
+        set_acls_znode_leader=DEFAULT,
+        delete_znode_leader=DEFAULT,
+    ) as patched_manager:
+        harness.charm.quorum_manager.update_acls()
+
+        for _, kwargs in patched_manager["create_znode_leader"].call_args_list:
+            assert "/rohan" in kwargs["path"]
+
+        for _, kwargs in patched_manager["set_acls_znode_leader"].call_args_list:
+            assert "/rohan" in kwargs["path"]
+
+        removed_men = False
+        for counter, call in enumerate(patched_manager["delete_znode_leader"].call_args_list):
+            _, kwargs = call
+
+            if "/fellowship/men" in kwargs["path"]:
+                assert not removed_men, "Parent zNode removed before all it's children"
+
+            if kwargs["path"] == "/fellowship/men":
+                removed_men = True
+
+        # ensure last node to go is the parent
+        assert (
+            patched_manager["delete_znode_leader"].call_args_list[-1][1]["path"] == "/fellowship"
+        )