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.

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

Add collect-status events for multi-status handling #954

Merged
merged 21 commits into from
Jul 21, 2023

Conversation

benhoyt
Copy link
Collaborator

@benhoyt benhoyt commented Jun 15, 2023

This implements the collect_app_status and collect_unit_status events that can be used by charms to allow the framework to automatically collect status (from multiple components of the charm) and set status at the end of every hook.

API reference under CollectStatusEvent, though I'll also add usage examples to the new "charm status" SDK doc that Dora is creating.

Spec and examples described in OP037.

The implementation is very straight-forward: model.Application and model.Unit each have a list of _collected_statuses, and CollectStatusEvent.add_status adds to the correct one of those. Then after every hook in ops/main.py we call _evaluate_status, which triggers each event and sets the app or unit status to the highest-priority status collected (if any statuses were collected). Note that collect_app_status is only done on the leader unit.

To test when using ops.testing.Harness, use Harness.evaluate_status.

This has full unit tests, including in test_main.py, but I've also confirms this works on a real production deploying on Juju.

@benhoyt benhoyt changed the title Try approach to stateless multistatus that uses Framework events Add collect-status events for multi-status handling Jul 12, 2023
]
calls = fake_script_calls(self)
self.assertRegex(' '.join(calls.pop(-2)), 'Initializing SQLite local storage: ')
self.assertRegex(' '.join(calls.pop(-3)), 'Initializing SQLite local storage: ')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not like how we have these brittle indexes in the tests, but I've left that as is for future cleanup work!

@@ -620,6 +624,7 @@ def test_collect_metrics(self):
VERSION_LOGLINE,
['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event collect_metrics.'],
['add-metric', '--labels', 'bar=4.2', 'foo=42'],
['is-leader', '--format=json'],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The testing of _evaluate_status is done in test_charm.py; all this has to do is test whether it's called.

@benhoyt benhoyt marked this pull request as ready for review July 12, 2023 04:22
ops/charm.py Show resolved Hide resolved
ops/charm.py Show resolved Hide resolved
ops/charm.py Outdated Show resolved Hide resolved
Copy link
Member

@jnsgruk jnsgruk left a comment

Choose a reason for hiding this comment

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

Very nice work, Ben! :)

@benhoyt

This comment was marked as outdated.

Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

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

Overall it does what we were intending, which is to provide a way to do callbacks to set the status of the application, and be able to handle multiple pieces that want to have a say on the status, without having one of them resolving causing the others to get confused.

ops/charm.py Outdated Show resolved Hide resolved
ops/charm.py Show resolved Hide resolved
ops/testing.py Show resolved Hide resolved
ops/charm.py Outdated Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
@benhoyt
Copy link
Collaborator Author

benhoyt commented Jul 19, 2023

I decided to do the debug logging a different way. Log every call to add_status at debug level, but only if it's not ActiveStatus.

@PietroPasotti
Copy link
Contributor

I decided to do the debug logging a different way. Log every call to add_status at debug level, but only if it's not ActiveStatus.

Why would we not want to log active?
Also, I'd rather see a single machine-readable logline containing all collected statuses rather than them one by one, but I'm aware that might mean some extra caution in case the charm fails before having collected all of them

@benhoyt
Copy link
Collaborator Author

benhoyt commented Jul 21, 2023

Why would we not want to log active?

Because I expect that a lot of the time there'll be several ActiveStatus() from various components in an active/steady state, and we don't want to log each of those. (Though maybe also ensuring the message is empty string is better?) We could also consider only logging the first unique status (unique on name and message).

Also, I'd rather see a single machine-readable logline containing all collected statuses rather than them one by one, but I'm aware that might mean some extra caution in case the charm fails before having collected all of them

I guess you're probably thinking ahead to parsing these in jhack, but I'd really rather not commit to a particular format of log debug message as a kind of API. I don't mind tweaking what we have in future to make jhack's life easier, but I think I'm going to merge what I have for now.

jujubot added a commit to juju/juju that referenced this pull request Jul 21, 2023
#15962

Per @jameinel's [suggestion on Mattermost](https://chat.charmhub.io/charmhub/pl/59gsuisc6jdi9mxnhej7pethjr), we should prioritize Maintenance (which means "we're busy") above Waiting ("someone else is busy").

> I highly doubt that anyone has code that uses the fact that Maintenance is lower priority than Waiting to make a decision out in the wild.

See also: canonical/operator#954 (comment)
@benhoyt benhoyt merged commit a3b110e into canonical:main Jul 21, 2023
@benhoyt benhoyt deleted the multistatus-poc branch July 21, 2023 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants