From 1a68ae28851fcfb3d7b59c3b22b21120152077b4 Mon Sep 17 00:00:00 2001 From: Billy Olsen Date: Fri, 1 Mar 2024 12:24:58 -0700 Subject: [PATCH 1/3] Only wait if an object is awaitable. Between juju 2.x and 3.x, the resulting return objects from the run method has changed to be awaitable or not. This change checks to see if the resulting object can be awaited or not. Without this, zaza will simply hang waiting on the results of an object which is not actually awaitable. Signed-off-by: Billy Olsen --- unit_tests/test_zaza_model.py | 45 +++++++++++++++++++++++++++++++++++ zaza/model.py | 18 ++++++++++---- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/unit_tests/test_zaza_model.py b/unit_tests/test_zaza_model.py index 66fd9b1bd..a07ca814f 100644 --- a/unit_tests/test_zaza_model.py +++ b/unit_tests/test_zaza_model.py @@ -785,11 +785,14 @@ def test_run_on_unit(self): self.cmd = cmd = 'somecommand someargument' self.patch_object(model, 'Model') self.patch_object(model, 'get_unit_from_name') + self.patch("inspect.isawaitable", name="isawaitable", + return_value=True) self.get_unit_from_name.return_value = self.unit1 self.Model.return_value = self.Model_mock self.assertEqual(model.run_on_unit('app/2', cmd), expected) self.unit1.run.assert_called_once_with(cmd, timeout=None) + self.action.wait.assert_called_once() def test_run_on_unit_juju2_x(self): del self.action.results @@ -803,11 +806,14 @@ def test_run_on_unit_juju2_x(self): self.cmd = cmd = 'somecommand someargument' self.patch_object(model, 'Model') self.patch_object(model, 'get_unit_from_name') + self.patch("inspect.isawaitable", name="isawaitable", + return_value=False) self.get_unit_from_name.return_value = self.unit1 self.Model.return_value = self.Model_mock self.assertEqual(model.run_on_unit('app/2', cmd), expected) self.unit1.run.assert_called_once_with(cmd, timeout=None) + self.action.wait.assert_not_called() def test_run_on_unit_lc_keys(self): self.patch_object(model, 'get_juju_model', return_value='mname') @@ -1117,6 +1123,9 @@ def test_run_action_on_units(self): self.patch_object(model, 'Model') self.Model.return_value = self.Model_mock self.patch_object(model, 'async_get_unit_from_name') + self.patch("inspect.isawaitable", return_value=False, + name="isawaitable") + self.patch_object(model, 'async_block_until') units = { 'app/1': self.unit1, 'app/2': self.unit2} @@ -1138,11 +1147,15 @@ async def _async_get_unit_from_name(x, *args): 'backup', backup_dir='/dev/null') + self.async_block_until.assert_not_called() + def test_run_action_on_units_timeout(self): self.patch_object(model, 'get_juju_model', return_value='mname') self.patch_object(model, 'Model') self.Model.return_value = self.Model_mock self.patch_object(model, 'get_unit_from_name') + self.patch("inspect.isawaitable", return_value=True, + name="isawaitable") self.get_unit_from_name.return_value = self.unit1 self.run_action.data = {'status': 'running'} with self.assertRaises(AsyncTimeoutError): @@ -1170,6 +1183,38 @@ async def _fake_get_action_output(_): raise_on_failure=True, action_params={'backup_dir': '/dev/null'}) + def test_run_action_on_units_async(self): + """Tests that non-awaitable action results aren't awaited on.""" + self.patch_object(model, 'get_juju_model', return_value='mname') + self.patch_object(model, 'Model') + self.Model.return_value = self.Model_mock + self.patch_object(model, 'async_get_unit_from_name') + self.patch_object(model, 'async_block_until') + self.patch("inspect.isawaitable", return_value=True, + name="isawaitable") + units = { + 'app/1': self.unit1, + 'app/2': self.unit2} + + async def _async_get_unit_from_name(x, *args): + nonlocal units + return units[x] + + self.async_get_unit_from_name.side_effect = _async_get_unit_from_name + self.run_action.data = {'status': 'completed'} + model.run_action_on_units( + ['app/1', 'app/2'], + 'backup', + action_params={'backup_dir': '/dev/null'}) + self.unit1.run_action.assert_called_once_with( + 'backup', + backup_dir='/dev/null') + self.unit2.run_action.assert_called_once_with( + 'backup', + backup_dir='/dev/null') + + assert self.async_block_until.called + def _application_states_setup(self, setup, units_idle=True): self.system_ready = True self._block_until_calls = 0 diff --git a/zaza/model.py b/zaza/model.py index 04166394d..0856da5d6 100644 --- a/zaza/model.py +++ b/zaza/model.py @@ -570,7 +570,8 @@ async def async_run_on_unit(unit_name, command, model_name=None, timeout=None): model = await get_model(model_name) unit = await async_get_unit_from_name(unit_name, model) action = await unit.run(command, timeout=timeout) - await action.wait() + if inspect.isawaitable(action): + await action.wait() action = _normalise_action_object(action) results = action.data.get('results') return _normalise_action_results(results) @@ -598,7 +599,8 @@ async def async_run_on_leader(application_name, command, model_name=None, is_leader = await unit.is_leader_from_status() if is_leader: action = await unit.run(command, timeout=timeout) - await action.wait() + if inspect.isawaitable(action): + await action.wait() action = _normalise_action_object(action) results = action.data.get('results') return _normalise_action_results(results) @@ -1211,18 +1213,24 @@ async def async_run_action_on_units(units, action_name, action_params=None, model = await get_model(model_name) actions = [] + async_actions = [] for unit_name in units: unit = await async_get_unit_from_name(unit_name, model) action_obj = await unit.run_action(action_name, **action_params) - actions.append(action_obj) + if inspect.isawaitable(action_obj): + async_actions.append(action_obj) + else: + actions.append(action_obj) async def _check_actions(): - for action_obj in actions: + for action_obj in async_actions: if action_obj.data['status'] in ['running', 'pending']: return False return True - await async_block_until(_check_actions, timeout=timeout) + if async_actions: + await async_block_until(_check_actions, timeout=timeout) + actions.extend(async_actions) for action_obj in actions: if raise_on_failure and action_obj.data['status'] != 'completed': From ae61814c8cf29ff8075a12937f9e5db176892936 Mon Sep 17 00:00:00 2001 From: Billy Olsen Date: Sat, 2 Mar 2024 21:10:21 -0700 Subject: [PATCH 2/3] Update status of action results when unknown Actions are awaited on and then completed but the status may still be 'unknown' even with a completion timestamp. Update the action status by querying the model in this case. This change also includes the removal of the isawaitable check on the unit.run_action change in the previous commit. It is not necessary to wait on the unit.run_action as the contract did not change between Juju 2.x and 3.x. Additionally, the isawaitable check was added to all locations that invoke the unit.run(...) method. Signed-off-by: Billy Olsen --- unit_tests/test_zaza_model.py | 84 +++++++++++++++++++---------------- zaza/model.py | 60 ++++++++++++++++++++----- 2 files changed, 94 insertions(+), 50 deletions(-) diff --git a/unit_tests/test_zaza_model.py b/unit_tests/test_zaza_model.py index a07ca814f..fedba2d87 100644 --- a/unit_tests/test_zaza_model.py +++ b/unit_tests/test_zaza_model.py @@ -1027,6 +1027,31 @@ def test_get_relation_id_interface(self): remote_interface_name='interface'), 51) + def test_update_unknown_action_status_invalid_params(self): + """Test update unknown action status invalid params.""" + self.assertRaises(ValueError, model.update_unknown_action_status, + None, mock.MagicMock()) + self.assertRaises(ValueError, model.update_unknown_action_status, + mock.MagicMock(), None) + + def test_update_unknown_action_status_not_unknown(self): + """Test update unknown action status with status not unknown.""" + model = mock.MagicMock() + action_obj = mock.MagicMock() + action_obj.data.return_value = {"status": "completed"} + + model.update_unknown_action_status(model, action_obj) + model.get_action_status.assert_not_called() + + def test_update_unknown_action_status_no_completion_timestamp(self): + """Test update unknown action status without a completion timestamp.""" + model = mock.MagicMock() + action_obj = mock.MagicMock() + action_obj.data.return_value = {"status": "running", "completed": ""} + + model.update_unknown_action_status(model, action_obj) + model.get_action_status.assert_not_called() + def test_run_action(self): self.patch_object(model, 'get_juju_model', return_value='mname') self.patch_object(model, 'Model') @@ -1123,9 +1148,6 @@ def test_run_action_on_units(self): self.patch_object(model, 'Model') self.Model.return_value = self.Model_mock self.patch_object(model, 'async_get_unit_from_name') - self.patch("inspect.isawaitable", return_value=False, - name="isawaitable") - self.patch_object(model, 'async_block_until') units = { 'app/1': self.unit1, 'app/2': self.unit2} @@ -1147,15 +1169,11 @@ async def _async_get_unit_from_name(x, *args): 'backup', backup_dir='/dev/null') - self.async_block_until.assert_not_called() - def test_run_action_on_units_timeout(self): self.patch_object(model, 'get_juju_model', return_value='mname') self.patch_object(model, 'Model') self.Model.return_value = self.Model_mock self.patch_object(model, 'get_unit_from_name') - self.patch("inspect.isawaitable", return_value=True, - name="isawaitable") self.get_unit_from_name.return_value = self.unit1 self.run_action.data = {'status': 'running'} with self.assertRaises(AsyncTimeoutError): @@ -1183,38 +1201,6 @@ async def _fake_get_action_output(_): raise_on_failure=True, action_params={'backup_dir': '/dev/null'}) - def test_run_action_on_units_async(self): - """Tests that non-awaitable action results aren't awaited on.""" - self.patch_object(model, 'get_juju_model', return_value='mname') - self.patch_object(model, 'Model') - self.Model.return_value = self.Model_mock - self.patch_object(model, 'async_get_unit_from_name') - self.patch_object(model, 'async_block_until') - self.patch("inspect.isawaitable", return_value=True, - name="isawaitable") - units = { - 'app/1': self.unit1, - 'app/2': self.unit2} - - async def _async_get_unit_from_name(x, *args): - nonlocal units - return units[x] - - self.async_get_unit_from_name.side_effect = _async_get_unit_from_name - self.run_action.data = {'status': 'completed'} - model.run_action_on_units( - ['app/1', 'app/2'], - 'backup', - action_params={'backup_dir': '/dev/null'}) - self.unit1.run_action.assert_called_once_with( - 'backup', - backup_dir='/dev/null') - self.unit2.run_action.assert_called_once_with( - 'backup', - backup_dir='/dev/null') - - assert self.async_block_until.called - def _application_states_setup(self, setup, units_idle=True): self.system_ready = True self._block_until_calls = 0 @@ -2821,6 +2807,26 @@ async def _g(): await model.async_block_until(_f, _g, timeout=0.1) + async def test_update_unknown_action_status(self): + """Test update unknown action status updates status.""" + mock_model = mock.AsyncMock() + + class ActionObj: + id = "1234" + data = { + "status": "unknown", + "completed": "2024-03-01T12:45:14" + } + + async def get_action_status(_id): + return {"1234": "completed"} + + mock_model.get_action_status.side_effect = get_action_status + action_obj = ActionObj() + + await model.async_update_unknown_action_status(mock_model, action_obj) + self.assertEqual(action_obj.data["status"], "completed") + async def test_run_on_machine(self): with mock.patch.object( model.generic_utils, diff --git a/zaza/model.py b/zaza/model.py index 0856da5d6..7e7526b6d 100644 --- a/zaza/model.py +++ b/zaza/model.py @@ -1106,6 +1106,44 @@ def _normalise_action_object(action_obj): return action_obj +async def async_update_unknown_action_status(model, action_obj): + """Update the action status if the status is unknown. + + Updates the action status for an action object when its data has + a completion timestamp, indicating it did complete, but the status + is set to unknown. When the action object is in this state, this + function will query for the latest status. This function will only + query for the status update a single time. + + :param model: the model the action_obj belongs to + :type model: juju.model.ModelEntity + :param action_obj: an action that should be updated if the status + is unknown. + :type action_obj: juju.model.ActionEntity + :raises ValueError: If either the model or action_obj is invalid + :return: None + """ + if not model or not action_obj: + raise ValueError("Expected model and action_obj to be valid. " + f"Got model: {model}, action_obj: {action_obj}") + + # If the status is not unknown, don't update the status + if action_obj.data.get('status', '') != 'unknown': + return + + # If the completed timestamp is not set, don't update the status + if not action_obj.data.get('completed', ''): + return + + logging.debug("Action results were complete but status is unknown. " + "Refreshing status.") + updated_status = await model.get_action_status(action_obj.id) + action_obj.data['status'] = updated_status.get(action_obj.id) + + +update_unknown_action_status = sync_wrapper(async_update_unknown_action_status) + + async def async_run_action(unit_name, action_name, model_name=None, action_params=None, raise_on_failure=False): """Run action on given unit. @@ -1132,6 +1170,8 @@ async def async_run_action(unit_name, action_name, model_name=None, action_obj = await unit.run_action(action_name, **action_params) await action_obj.wait() action_obj = _normalise_action_object(action_obj) + await async_update_unknown_action_status(model, action_obj) + if raise_on_failure and action_obj.data['status'] != 'completed': try: output = await model.get_action_output(action_obj.id) @@ -1173,6 +1213,7 @@ async def async_run_action_on_leader(application_name, action_name, **action_params) await action_obj.wait() action_obj = _normalise_action_object(action_obj) + await async_update_unknown_action_status(model, action_obj) if raise_on_failure and action_obj.data['status'] != 'completed': try: output = await model.get_action_output(action_obj.id) @@ -1213,26 +1254,21 @@ async def async_run_action_on_units(units, action_name, action_params=None, model = await get_model(model_name) actions = [] - async_actions = [] for unit_name in units: unit = await async_get_unit_from_name(unit_name, model) action_obj = await unit.run_action(action_name, **action_params) - if inspect.isawaitable(action_obj): - async_actions.append(action_obj) - else: - actions.append(action_obj) + actions.append(action_obj) async def _check_actions(): - for action_obj in async_actions: + for action_obj in actions: if action_obj.data['status'] in ['running', 'pending']: return False return True - if async_actions: - await async_block_until(_check_actions, timeout=timeout) - actions.extend(async_actions) + await async_block_until(_check_actions, timeout=timeout) for action_obj in actions: + await async_update_unknown_action_status(model, action_obj) if raise_on_failure and action_obj.data['status'] != 'completed': try: output = await model.get_action_output(action_obj.id) @@ -2124,7 +2160,8 @@ async def _check_file(): for unit in units: try: output = await unit.run('cat {}'.format(remote_file)) - await output.wait() + if inspect.isawaitable(output): + await output.wait() results = {} try: results = output.results @@ -2265,7 +2302,8 @@ async def _check_for_file(model): for unit in units: try: output = await unit.run('test -e "{}"; echo $?'.format(path)) - await output.wait() + if inspect.isawaitable(output): + await output.wait() output = _normalise_action_object(output) output_result = _normalise_action_results( output.data.get('results', {})) From a3e18a99d5a0ef9be78761b2ac03eb819047d59b Mon Sep 17 00:00:00 2001 From: Billy Olsen Date: Sun, 3 Mar 2024 19:46:47 -0700 Subject: [PATCH 3/3] Update unit tests to fix coverage Increase the coverage of all code changed in this patch series to 100%. Move tests to async tests to ensure that the actual tests are working as expected. Signed-off-by: Billy Olsen --- unit_tests/test_zaza_model.py | 58 ++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/unit_tests/test_zaza_model.py b/unit_tests/test_zaza_model.py index fedba2d87..f46d5158f 100644 --- a/unit_tests/test_zaza_model.py +++ b/unit_tests/test_zaza_model.py @@ -906,6 +906,8 @@ def test_run_on_leader(self): self.cmd = cmd = 'somecommand someargument' self.patch_object(model, 'Model') self.Model.return_value = self.Model_mock + self.patch('inspect.isawaitable', return_value=True, + name='isawaitable') self.assertEqual(model.run_on_leader('app', cmd), expected) self.unit2.run.assert_called_once_with(cmd, timeout=None) @@ -1027,31 +1029,6 @@ def test_get_relation_id_interface(self): remote_interface_name='interface'), 51) - def test_update_unknown_action_status_invalid_params(self): - """Test update unknown action status invalid params.""" - self.assertRaises(ValueError, model.update_unknown_action_status, - None, mock.MagicMock()) - self.assertRaises(ValueError, model.update_unknown_action_status, - mock.MagicMock(), None) - - def test_update_unknown_action_status_not_unknown(self): - """Test update unknown action status with status not unknown.""" - model = mock.MagicMock() - action_obj = mock.MagicMock() - action_obj.data.return_value = {"status": "completed"} - - model.update_unknown_action_status(model, action_obj) - model.get_action_status.assert_not_called() - - def test_update_unknown_action_status_no_completion_timestamp(self): - """Test update unknown action status without a completion timestamp.""" - model = mock.MagicMock() - action_obj = mock.MagicMock() - action_obj.data.return_value = {"status": "running", "completed": ""} - - model.update_unknown_action_status(model, action_obj) - model.get_action_status.assert_not_called() - def test_run_action(self): self.patch_object(model, 'get_juju_model', return_value='mname') self.patch_object(model, 'Model') @@ -1731,6 +1708,8 @@ def test_block_until_file_has_contents(self): _fileobj = mock.MagicMock() _fileobj.__enter__().read.return_value = "somestring" self._open.return_value = _fileobj + self.patch('inspect.isawaitable', return_value=True, + name='isawaitable') model.block_until_file_has_contents( 'app', '/tmp/src/myfile.txt', @@ -1756,6 +1735,8 @@ def test_block_until_file_has_contents_juju2_x(self): _fileobj = mock.MagicMock() _fileobj.__enter__().read.return_value = "somestring" self._open.return_value = _fileobj + self.patch('inspect.isawaitable', return_value=False, + name='isawaitable') model.block_until_file_has_contents( 'app', '/tmp/src/myfile.txt', @@ -1836,6 +1817,8 @@ def test_block_until_file_missing(self): self.Model.return_value = self.Model_mock self.patch_object(model, 'get_juju_model', return_value='mname') self.action.results = {'stdout': "1"} + self.patch('inspect.isawaitable', return_value=True, + name='isawaitable') model.block_until_file_missing( 'app', '/tmp/src/myfile.txt', @@ -2807,6 +2790,31 @@ async def _g(): await model.async_block_until(_f, _g, timeout=0.1) + async def test_update_unknown_action_status_invalid_params(self): + """Test update unknown action status invalid params.""" + self.assertRaises(ValueError, model.update_unknown_action_status, + None, mock.MagicMock()) + self.assertRaises(ValueError, model.update_unknown_action_status, + mock.MagicMock(), None) + + async def test_update_unknown_action_status_not_unknown(self): + """Test update unknown action status with status not unknown.""" + mock_model = mock.AsyncMock() + action_obj = mock.AsyncMock() + action_obj.data = {"status": "running"} + + await model.async_update_unknown_action_status(model, action_obj) + mock_model.get_action_status.assert_not_called() + + async def test_update_unknown_action_status_no_completion_timestamp(self): + """Test update unknown action status without a completion timestamp.""" + model_mock = mock.AsyncMock() + action_obj = mock.MagicMock() + action_obj.data = {"status": "unknown", "completed": ""} + + await model.async_update_unknown_action_status(model_mock, action_obj) + model_mock.get_action_status.assert_not_called() + async def test_update_unknown_action_status(self): """Test update unknown action status updates status.""" mock_model = mock.AsyncMock()