Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only wait if an object is awaitable. #650

Merged
merged 3 commits into from
Mar 4, 2024
Merged

Only wait if an object is awaitable. #650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a68ae2
Only wait if an object is awaitable.
Mar 1, 2024
ae61814
Update status of action results when unknown
Mar 3, 2024
a3e18a9
Update unit tests to fix coverage
Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions unit_tests/test_zaza_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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')
Expand Down Expand Up @@ -900,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)
Expand Down Expand Up @@ -1700,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',
Expand All @@ -1725,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',
Expand Down Expand Up @@ -1805,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',
Expand Down Expand Up @@ -2776,6 +2790,51 @@ 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()

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,
Expand Down
54 changes: 50 additions & 4 deletions zaza/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -1104,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.
Expand All @@ -1130,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how this condition arise of status unknown given the L#1171 will wait for status completed or failed [1]. The code in [1] is not changed for 8 years.

If you are seeing this during your test runs, there should be something wrong in python-libjuju action object updates.
+1 for the changes since you are seeing the error in your test runs.

[1] https://github.com/juju/python-libjuju/blob/master/juju/model.py#L1347

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine it's something to do with the async nature of juju updates from the model to libjuju (any client), as I've seen other transient examples like this. The .get_action_status() does a round-trip to the controller, and so forces the update.


if raise_on_failure and action_obj.data['status'] != 'completed':
try:
output = await model.get_action_output(action_obj.id)
Expand Down Expand Up @@ -1171,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)
Expand Down Expand Up @@ -1225,6 +1268,7 @@ async def _check_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)
Expand Down Expand Up @@ -2116,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
Expand Down Expand Up @@ -2257,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', {}))
Expand Down
Loading