From 7d0f390a601f334846ff16cd35f623bdc1c609ee Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Wed, 27 Mar 2024 14:18:36 -0400 Subject: [PATCH 1/2] Always update relation settings on relation changed. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The controller charm attempted to minimise rewriting relation settings by maintaining all_db_bind_addresses and only when it detected differences. This is not necessary, because as long as the same data has the same representation (JSON in the application data bag for example), this doesn’t cause settings watchers to fire. Now we always just rewrite state. --- src/charm.py | 7 ------- tests/test_charm.py | 9 +++++++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/charm.py b/src/charm.py index 26ccdb5..8a86014 100755 --- a/src/charm.py +++ b/src/charm.py @@ -39,7 +39,6 @@ def __init__(self, *args): self._stored.set_default( db_bind_address='', last_bind_addresses=[], - all_bind_addresses=dict(), ) # TODO (manadart 2024-03-05): Get these at need. @@ -179,9 +178,6 @@ def _on_dbcluster_relation_changed(self, event): agent_id = unit_data[self.AGENT_ID_KEY] all_bind_addresses[agent_id] = unit_data[self.DB_BIND_ADDR_KEY] - if self._stored.all_bind_addresses == all_bind_addresses: - return - relation.data[self.app][self.ALL_BIND_ADDRS_KEY] = json.dumps(all_bind_addresses) self._update_config_file(all_bind_addresses) else: @@ -191,9 +187,6 @@ def _on_dbcluster_relation_changed(self, event): else: all_bind_addresses = dict() - if self._stored.all_bind_addresses == all_bind_addresses: - return - self._update_config_file(all_bind_addresses) def _ensure_db_bind_address(self, relation): diff --git a/tests/test_charm.py b/tests/test_charm.py index 8e6becd..c26b302 100644 --- a/tests/test_charm.py +++ b/tests/test_charm.py @@ -188,10 +188,14 @@ def test_dbcluster_relation_changed_single_addr( self.assertIsInstance(harness.charm.unit.status, ActiveStatus) @patch("builtins.open", new_callable=mock_open, read_data=agent_conf) + @patch("configchangesocket.ConfigChangeSocketClient.get_controller_agent_id") @patch("ops.model.Model.get_binding") - def test_dbcluster_relation_changed_multi_addr_error(self, binding, _): + @patch("configchangesocket.ConfigChangeSocketClient.reload_config") + def test_dbcluster_relation_changed_multi_addr_error( + self, mock_reload_config, mock_get_binding, mock_get_agent_id, *_): harness = self.harness - binding.return_value = mockBinding(["192.168.1.17", "192.168.1.18"]) + mock_get_binding.return_value = mockBinding(["192.168.1.17", "192.168.1.18"]) + mock_get_agent_id.return_value = '0' relation_id = harness.add_relation('dbcluster', harness.charm.app) harness.add_relation_unit(relation_id, 'juju-controller/1') @@ -201,6 +205,7 @@ def test_dbcluster_relation_changed_multi_addr_error(self, binding, _): harness.evaluate_status() self.assertIsInstance(harness.charm.unit.status, BlockedStatus) + mock_reload_config.assert_called_once() @patch("configchangesocket.ConfigChangeSocketClient.get_controller_agent_id") @patch("builtins.open", new_callable=mock_open) From b65d7808cf806fc3998772e85bbd0c48ef4af508 Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Thu, 28 Mar 2024 15:52:06 -0400 Subject: [PATCH 2/2] Remove early return in when getting db bind address --- src/charm.py | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/charm.py b/src/charm.py index 8a86014..553a26d 100755 --- a/src/charm.py +++ b/src/charm.py @@ -37,7 +37,6 @@ def __init__(self, *args): self._observe() self._stored.set_default( - db_bind_address='', last_bind_addresses=[], ) @@ -164,12 +163,15 @@ def _on_dbcluster_relation_changed(self, event): If the aggregate addresses have changed, rewrite the config file. """ relation = event.relation - self._ensure_db_bind_address(relation) + try: + ip = self._set_db_bind_address(relation) + except DBBindAddressException as e: + logger.error(e) + ip = None if self.unit.is_leader(): # The event only has *other* units so include this # unit's bind address if we have managed to set it. - ip = self._stored.db_bind_address all_bind_addresses = {self._controller_agent_id(): ip} if ip else dict() for unit in relation.units: @@ -178,7 +180,8 @@ def _on_dbcluster_relation_changed(self, event): agent_id = unit_data[self.AGENT_ID_KEY] all_bind_addresses[agent_id] = unit_data[self.DB_BIND_ADDR_KEY] - relation.data[self.app][self.ALL_BIND_ADDRS_KEY] = json.dumps(all_bind_addresses) + relation.data[self.app][self.ALL_BIND_ADDRS_KEY] = json.dumps( + all_bind_addresses, sort_keys=True) self._update_config_file(all_bind_addresses) else: app_data = relation.data[self.app] @@ -189,28 +192,26 @@ def _on_dbcluster_relation_changed(self, event): self._update_config_file(all_bind_addresses) - def _ensure_db_bind_address(self, relation): - """Ensure that a bind address for Dqlite is set in relation data, - if we can determine a unique one from the relation's bound space. + def _set_db_bind_address(self, relation): + """Set a db bind address for Dqlite in relation data, if we can + determine a unique one from the relation's bound space. + + Returns the db bind address. """ ips = [str(ip) for ip in self.model.get_binding(relation).network.ingress_addresses] self._stored.last_bind_addresses = ips + ip = ips[0] if len(ips) > 1: - logger.error( - 'multiple possible DB bind addresses; set a suitable cluster network binding') - return + raise DBBindAddressException( + 'multiple possible DB bind addresses;set a suitable cluster network binding') - ip = ips[0] - if self._stored.db_bind_address == ip: - return - - logger.info('setting new DB bind address: %s', ip) + logger.info('setting DB bind address: %s', ip) relation.data[self.unit].update({ self.DB_BIND_ADDR_KEY: ip, self.AGENT_ID_KEY: self._controller_agent_id() }) - self._stored.db_bind_address = ip + return ip def _update_config_file(self, bind_addresses): logger.info('writing new DB cluster to config file: %s', bind_addresses) @@ -291,5 +292,9 @@ class ControllerProcessException(Exception): """Raised when there are errors regarding detection of controller service or process.""" +class DBBindAddressException(Exception): + """Raised when there are errors regarding the database bind addresses""" + + if __name__ == "__main__": main(JujuControllerCharm)