From 5e28801a142a8ace2d03344e9412f10f813452dc Mon Sep 17 00:00:00 2001 From: Joseph Phillips Date: Wed, 24 Jan 2024 20:35:51 +0100 Subject: [PATCH] Ensures correct SIGHUP behaviour in unit tests. --- src/charm.py | 12 +++++------- tests/test_charm.py | 42 ++++++++++++++++++++++++++++-------------- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/charm.py b/src/charm.py index 2490902..89bc1a8 100755 --- a/src/charm.py +++ b/src/charm.py @@ -187,8 +187,6 @@ def _on_dbcluster_relation_changed(self, event): self._update_config_file(all_bind_addresses) - self._sighup_controller_process - 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. @@ -221,6 +219,7 @@ def _update_config_file(self, bind_addresses): with open(file_path, 'w') as conf_file: yaml.dump(conf, conf_file) + self._sighup_controller_process() self._stored.all_bind_addresses = bind_addresses def api_port(self) -> str: @@ -258,21 +257,20 @@ def _controller_config_path(self) -> str: raise AgentConfException('Unable to determine ID for running controller') controller_id = match.group(1) - return f'/var/lib/juju/agents/controller-{controller_id}/agent.conf' def _sighup_controller_process(self): - result = subprocess.check_output( + res = subprocess.check_output( ["systemctl", "show", "--property=MainPID", self._controller_service_name()]) - pid = result.decode('utf-8').strip().split('=')[-1] - os.kill(pid, signal.SIGHUP) + pid = res.decode('utf-8').strip().split('=')[-1] + os.kill(int(pid), signal.SIGHUP) def _controller_service_name(self) -> str: res = subprocess.check_output( ['systemctl', 'list-units', 'jujud_machine*.service', '--no-legend'], text=True) - services = [line.split()[0] for line in res.strip().split('\n') if line] + services = [line.split()[0] for line in res.decode('utf-8').strip().split('\n') if line] if len(services) != 1: raise AgentConfException('Unable to determine service for running controller') diff --git a/tests/test_charm.py b/tests/test_charm.py index e7eeca5..8e8417b 100644 --- a/tests/test_charm.py +++ b/tests/test_charm.py @@ -4,6 +4,7 @@ import ipaddress import json import os +import signal import unittest import yaml @@ -142,21 +143,23 @@ def test_apiaddresses_missing_status(self, *_): @patch("builtins.open", new_callable=mock_open, read_data=agent_conf_ipv4) def test_apiaddresses_ipv4(self, _): - harness = self.harness - - self.assertEqual(harness.charm.api_port(), 17070) + self.assertEqual(self.harness.charm.api_port(), 17070) @patch("builtins.open", new_callable=mock_open, read_data=agent_conf_ipv6) def test_apiaddresses_ipv6(self, _): - harness = self.harness - - self.assertEqual(harness.charm.api_port(), 17070) + self.assertEqual(self.harness.charm.api_port(), 17070) + @patch('os.kill') @patch("builtins.open", new_callable=mock_open, read_data=agent_conf) + @patch('subprocess.check_output') @patch("ops.model.Model.get_binding") - def test_dbcluster_relation_changed_single_addr(self, binding, _): + def test_dbcluster_relation_changed_single_addr(self, mock_get_binding, mock_check_out, *_): harness = self.harness - binding.return_value = mockBinding(['192.168.1.17']) + mock_get_binding.return_value = mockBinding(['192.168.1.17']) + + # First calls are to get the controller service name; last is for its PID. + mock_check_out.side_effect = [ + b'jujud-machine-0.service', b'jujud-machine-0.service', b'pid=12345'] harness.set_leader() @@ -192,11 +195,19 @@ def test_dbcluster_relation_changed_multi_addr_error(self, binding, _): harness.evaluate_status() self.assertIsInstance(harness.charm.unit.status, BlockedStatus) + @patch('os.kill') + @patch('subprocess.check_output') @patch("builtins.open", new_callable=mock_open) @patch("ops.model.Model.get_binding") - def test_dbcluster_relation_changed_write_file(self, binding, mock_open): + def test_dbcluster_relation_changed_write_file( + self, mock_get_binding, mock_open, mock_check_out, mock_kill): + harness = self.harness - binding.return_value = mockBinding(['192.168.1.17']) + mock_get_binding.return_value = mockBinding(['192.168.1.17']) + + # First calls are to get the controller service name; last is for its PID. + mock_check_out.side_effect = [ + b'jujud-machine-0.service', b'jujud-machine-0.service', b'pid=12345'] relation_id = harness.add_relation('dbcluster', harness.charm.app) harness.add_relation_unit(relation_id, 'juju-controller/1') @@ -208,12 +219,12 @@ def test_dbcluster_relation_changed_write_file(self, binding, mock_open): self.assertEqual(mock_open.call_count, 2) # First call to read out the YAML - first_call_args, _ = mock_open.call_args_list[0] - self.assertEqual(first_call_args, (file_path,)) + first_open_args, _ = mock_open.call_args_list[0] + self.assertEqual(first_open_args, (file_path,)) # Second call to write the updated YAML. - second_call_args, _ = mock_open.call_args_list[1] - self.assertEqual(second_call_args, (file_path, 'w')) + second_open_args, _ = mock_open.call_args_list[1] + self.assertEqual(second_open_args, (file_path, 'w')) # yaml.dump appears to write the the file incrementally, # so we need to hoover up the call args to reconstruct. @@ -223,6 +234,9 @@ def test_dbcluster_relation_changed_write_file(self, binding, mock_open): self.assertEqual(yaml.safe_load(written), {'db-bind-addresses': bound}) + # The last thing we should have done is sent SIGHUP to Juju to reload the config. + mock_kill.assert_called_once_with(12345, signal.SIGHUP) + class mockNetwork: def __init__(self, addresses):