diff --git a/ops/__init__.py b/ops/__init__.py index 840ecca30..1d6e383dc 100644 --- a/ops/__init__.py +++ b/ops/__init__.py @@ -55,6 +55,7 @@ 'CharmEvents', 'CharmMeta', 'CollectMetricsEvent', + 'CollectStatusEvent', 'ConfigChangedEvent', 'ContainerMeta', 'ContainerStorageMeta', @@ -186,6 +187,7 @@ CharmEvents, CharmMeta, CollectMetricsEvent, + CollectStatusEvent, ConfigChangedEvent, ContainerMeta, ContainerStorageMeta, diff --git a/ops/charm.py b/ops/charm.py index 7d735e512..cb599863b 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -15,6 +15,7 @@ """Base objects for the Charm, events and metadata.""" import enum +import logging import os import pathlib from typing import ( @@ -74,6 +75,9 @@ total=False) +logger = logging.getLogger(__name__) + + class HookEvent(EventBase): """Events raised by Juju to progress a charm's lifecycle. @@ -826,6 +830,83 @@ def defer(self) -> None: 'this event until you create a new revision.') +class CollectStatusEvent(EventBase): + """Event triggered at the end of every hook to collect statuses for evaluation. + + If the charm wants to provide application or unit status in a consistent + way after the end of every hook, it should observe the + :attr:`collect_app_status ` or + :attr:`collect_unit_status ` event, + respectively. + + The framework will trigger these events after the hook code runs + successfully (``collect_app_status`` will only be triggered on the leader + unit). If any statuses were added by the event handlers using + :meth:`add_status`, the framework will choose the highest-priority status + and set that as the status (application status for ``collect_app_status``, + or unit status for ``collect_unit_status``). + + The order of priorities is as follows, from highest to lowest: + + * error + * blocked + * maintenance + * waiting + * active + * unknown + + If there are multiple statuses with the same priority, the first one added + wins (and if an event is observed multiple times, the handlers are called + in the order they were observed). + + A collect-status event can be observed multiple times, and + :meth:`add_status` can be called multiple times to add multiple statuses + for evaluation. This is useful when a charm has multiple components that + each have a status. Each code path in a collect-status handler should + call ``add_status`` at least once. + + Below is an example "web app" charm component that observes + ``collect_unit_status`` to provide the status of the component, which + requires a "port" config option set before it can proceed:: + + class MyCharm(ops.CharmBase): + def __init__(self, *args): + super().__init__(*args) + self.webapp = Webapp(self) + # initialize other components + + class WebApp(ops.Object): + def __init__(self, charm: ops.CharmBase): + super().__init__(charm, 'webapp') + self.framework.observe(charm.on.collect_unit_status, self._on_collect_status) + + def _on_collect_status(self, event: ops.CollectStatusEvent): + if 'port' not in self.model.config: + event.add_status(ops.BlockedStatus('please set "port" config')) + return + event.add_status(ops.ActiveStatus()) + + .. # noqa (pydocstyle barfs on the above for unknown reasons I've spent hours on) + """ + + def add_status(self, status: model.StatusBase): + """Add a status for evaluation. + + See :class:`CollectStatusEvent` for a description of how to use this. + """ + if not isinstance(status, model.StatusBase): + raise TypeError(f'status should be a StatusBase, not {type(status).__name__}') + model_ = self.framework.model + if self.handle.kind == 'collect_app_status': + if not isinstance(status, model.ActiveStatus): + logger.debug('Adding app status %s', status, stacklevel=2) + model_.app._collected_statuses.append(status) + else: + if not isinstance(status, model.ActiveStatus): + logger.debug('Adding unit status %s', status, stacklevel=2) + model_.unit._collected_statuses.append(status) + + class CharmEvents(ObjectEvents): """Events generated by Juju pertaining to application lifecycle. @@ -882,26 +963,41 @@ class CharmEvents(ObjectEvents): leader_settings_changed = EventSource(LeaderSettingsChangedEvent) """DEPRECATED. Triggered when leader changes any settings (see - :class:`LeaderSettingsChangedEvent`).""" + :class:`LeaderSettingsChangedEvent`). + """ collect_metrics = EventSource(CollectMetricsEvent) """Triggered by Juju to collect metrics (see :class:`CollectMetricsEvent`).""" secret_changed = EventSource(SecretChangedEvent) """Triggered by Juju on the observer when the secret owner changes its contents (see - :class:`SecretChangedEvent`).""" + :class:`SecretChangedEvent`). + """ secret_expired = EventSource(SecretExpiredEvent) """Triggered by Juju on the owner when a secret's expiration time elapses (see - :class:`SecretExpiredEvent`).""" + :class:`SecretExpiredEvent`). + """ secret_rotate = EventSource(SecretRotateEvent) """Triggered by Juju on the owner when the secret's rotation policy elapses (see - :class:`SecretRotateEvent`).""" + :class:`SecretRotateEvent`). + """ secret_remove = EventSource(SecretRemoveEvent) """Triggered by Juju on the owner when a secret revision can be removed (see - :class:`SecretRemoveEvent`).""" + :class:`SecretRemoveEvent`). + """ + + collect_app_status = EventSource(CollectStatusEvent) + """Triggered on the leader at the end of every hook to collect app statuses for evaluation + (see :class:`CollectStatusEvent`). + """ + + collect_unit_status = EventSource(CollectStatusEvent) + """Triggered at the end of every hook to collect unit statuses for evaluation + (see :class:`CollectStatusEvent`). + """ class CharmBase(Object): @@ -995,6 +1091,23 @@ def config(self) -> model.ConfigData: return self.model.config +def _evaluate_status(charm: CharmBase): # pyright: ignore[reportUnusedFunction] + """Trigger collect-status events and evaluate and set the highest-priority status. + + See :class:`CollectStatusEvent` for details. + """ + if charm.framework.model._backend.is_leader(): + charm.on.collect_app_status.emit() + app = charm.app + if app._collected_statuses: + app.status = model.StatusBase._get_highest_priority(app._collected_statuses) + + charm.on.collect_unit_status.emit() + unit = charm.unit + if unit._collected_statuses: + unit.status = model.StatusBase._get_highest_priority(unit._collected_statuses) + + class CharmMeta: """Object containing the metadata for the charm. diff --git a/ops/main.py b/ops/main.py index 6bbc2d3fe..d7a186f62 100755 --- a/ops/main.py +++ b/ops/main.py @@ -440,6 +440,8 @@ def main(charm_class: Type[ops.charm.CharmBase], _emit_charm_event(charm, dispatcher.event_name) + ops.charm._evaluate_status(charm) + framework.commit() finally: framework.close() diff --git a/ops/model.py b/ops/model.py index 24f8568d3..a96da2e3c 100644 --- a/ops/model.py +++ b/ops/model.py @@ -319,6 +319,7 @@ def __init__(self, name: str, meta: 'ops.charm.CharmMeta', self._cache = cache self._is_our_app = self.name == self._backend.app_name self._status = None + self._collected_statuses: 'List[StatusBase]' = [] def _invalidate(self): self._status = None @@ -331,6 +332,10 @@ def status(self) -> 'StatusBase': The status of remote units is always Unknown. + You can also use the :attr:`collect_app_status ` + event if you want to evaluate and set application status consistently + at the end of every hook. + Raises: RuntimeError: if you try to set the status of another application, or if you try to set the status of this application as a unit that is not the leader. @@ -465,6 +470,7 @@ def __init__(self, name: str, meta: 'ops.charm.CharmMeta', self._cache = cache self._is_our_unit = self.name == self._backend.unit_name self._status = None + self._collected_statuses: 'List[StatusBase]' = [] if self._is_our_unit and hasattr(meta, "containers"): containers: _ContainerMeta_Raw = meta.containers @@ -479,6 +485,10 @@ def status(self) -> 'StatusBase': The status of any unit other than yourself is always Unknown. + You can also use the :attr:`collect_unit_status ` + event if you want to evaluate and set unit status consistently at the + end of every hook. + Raises: RuntimeError: if you try to set the status of a unit other than yourself. InvalidStatusError: if you try to set the status to something other than @@ -1583,6 +1593,23 @@ def register(cls, child: Type['StatusBase']): cls._statuses[child.name] = child return child + _priorities = { + 'error': 5, + 'blocked': 4, + 'maintenance': 3, + 'waiting': 2, + 'active': 1, + # 'unknown' or any other status is handled below + } + + @classmethod + def _get_highest_priority(cls, statuses: 'List[StatusBase]') -> 'StatusBase': + """Return the highest-priority status from a list of statuses. + + If there are multiple highest-priority statuses, return the first one. + """ + return max(statuses, key=lambda status: cls._priorities.get(status.name, 0)) + @StatusBase.register class UnknownStatus(StatusBase): diff --git a/ops/testing.py b/ops/testing.py index a290be1ef..fd69a1968 100755 --- a/ops/testing.py +++ b/ops/testing.py @@ -1484,6 +1484,21 @@ def __init__(self, *args): container_name = container.name return self._backend._pebble_clients[container_name]._root + def evaluate_status(self) -> None: + """Trigger the collect-status events and set application and/or unit status. + + This will always trigger ``collect_unit_status``, and set the unit status if any + statuses were added. + + If running on the leader unit (:meth:`set_leader` has been called with ``True``), + this will trigger ``collect_app_status``, and set the application status if any + statuses were added. + + Tests should normally call this and then assert that ``self.model.app.status`` + or ``self.model.unit.status`` is the value expected. + """ + charm._evaluate_status(self.charm) + def _get_app_or_unit_name(app_or_unit: AppUnitOrName) -> str: """Return name of given application or unit (return strings directly).""" diff --git a/requirements-dev.txt b/requirements-dev.txt index 0b2ac7faa..0e2df6322 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -7,7 +7,7 @@ flake8-builtins==2.1.0 pyproject-flake8==4.0.1 pep8-naming==0.13.2 pytest==7.2.1 -pyright==1.1.316 +pyright==1.1.317 pytest-operator==0.23.0 coverage[toml]==7.0.5 typing_extensions==4.2.0 diff --git a/test/test_charm.py b/test/test_charm.py index 9b7ba83e1..d28b042db 100755 --- a/test/test_charm.py +++ b/test/test_charm.py @@ -19,6 +19,7 @@ from pathlib import Path import ops +import ops.charm from ops.model import _ModelBackend from ops.storage import SQLiteStorage @@ -631,3 +632,186 @@ def on_secret_expired(self, event): 'SecretRemoveEvent', 'SecretExpiredEvent', ]) + + def test_collect_app_status_leader(self): + class MyCharm(ops.CharmBase): + def __init__(self, *args): + super().__init__(*args) + self.framework.observe(self.on.collect_app_status, self._on_collect_status) + + def _on_collect_status(self, event): + event.add_status(ops.ActiveStatus()) + event.add_status(ops.BlockedStatus('first')) + event.add_status(ops.WaitingStatus('waiting')) + event.add_status(ops.BlockedStatus('second')) + + fake_script(self, 'is-leader', 'echo true') + fake_script(self, 'status-set', 'exit 0') + + charm = MyCharm(self.create_framework()) + ops.charm._evaluate_status(charm) + + self.assertEqual(fake_script_calls(self, True), [ + ['is-leader', '--format=json'], + ['status-set', '--application=True', 'blocked', 'first'], + ]) + + def test_collect_app_status_no_statuses(self): + class MyCharm(ops.CharmBase): + def __init__(self, *args): + super().__init__(*args) + self.framework.observe(self.on.collect_app_status, self._on_collect_status) + + def _on_collect_status(self, event): + pass + + fake_script(self, 'is-leader', 'echo true') + + charm = MyCharm(self.create_framework()) + ops.charm._evaluate_status(charm) + + self.assertEqual(fake_script_calls(self, True), [ + ['is-leader', '--format=json'], + ]) + + def test_collect_app_status_non_leader(self): + class MyCharm(ops.CharmBase): + def __init__(self, *args): + super().__init__(*args) + self.framework.observe(self.on.collect_app_status, self._on_collect_status) + + def _on_collect_status(self, event): + raise Exception # shouldn't be called + + fake_script(self, 'is-leader', 'echo false') + + charm = MyCharm(self.create_framework()) + ops.charm._evaluate_status(charm) + + self.assertEqual(fake_script_calls(self, True), [ + ['is-leader', '--format=json'], + ]) + + def test_collect_unit_status(self): + class MyCharm(ops.CharmBase): + def __init__(self, *args): + super().__init__(*args) + self.framework.observe(self.on.collect_unit_status, self._on_collect_status) + + def _on_collect_status(self, event): + event.add_status(ops.ActiveStatus()) + event.add_status(ops.BlockedStatus('first')) + event.add_status(ops.WaitingStatus('waiting')) + event.add_status(ops.BlockedStatus('second')) + + fake_script(self, 'is-leader', 'echo false') # called only for collecting app statuses + fake_script(self, 'status-set', 'exit 0') + + charm = MyCharm(self.create_framework()) + ops.charm._evaluate_status(charm) + + self.assertEqual(fake_script_calls(self, True), [ + ['is-leader', '--format=json'], + ['status-set', '--application=False', 'blocked', 'first'], + ]) + + def test_collect_unit_status_no_statuses(self): + class MyCharm(ops.CharmBase): + def __init__(self, *args): + super().__init__(*args) + self.framework.observe(self.on.collect_unit_status, self._on_collect_status) + + def _on_collect_status(self, event): + pass + + fake_script(self, 'is-leader', 'echo false') # called only for collecting app statuses + + charm = MyCharm(self.create_framework()) + ops.charm._evaluate_status(charm) + + self.assertEqual(fake_script_calls(self, True), [ + ['is-leader', '--format=json'], + ]) + + def test_collect_app_and_unit_status(self): + class MyCharm(ops.CharmBase): + def __init__(self, *args): + super().__init__(*args) + self.framework.observe(self.on.collect_app_status, self._on_collect_app_status) + self.framework.observe(self.on.collect_unit_status, self._on_collect_unit_status) + + def _on_collect_app_status(self, event): + event.add_status(ops.ActiveStatus()) + + def _on_collect_unit_status(self, event): + event.add_status(ops.WaitingStatus('blah')) + + fake_script(self, 'is-leader', 'echo true') + fake_script(self, 'status-set', 'exit 0') + + charm = MyCharm(self.create_framework()) + ops.charm._evaluate_status(charm) + + self.assertEqual(fake_script_calls(self, True), [ + ['is-leader', '--format=json'], + ['status-set', '--application=True', 'active', ''], + ['status-set', '--application=False', 'waiting', 'blah'], + ]) + + def test_add_status_type_error(self): + class MyCharm(ops.CharmBase): + def __init__(self, *args): + super().__init__(*args) + self.framework.observe(self.on.collect_app_status, self._on_collect_status) + + def _on_collect_status(self, event): + event.add_status('active') + + fake_script(self, 'is-leader', 'echo true') + + charm = MyCharm(self.create_framework()) + with self.assertRaises(TypeError): + ops.charm._evaluate_status(charm) + + def test_collect_status_priority(self): + class MyCharm(ops.CharmBase): + def __init__(self, *args, statuses=None): + super().__init__(*args) + self.framework.observe(self.on.collect_app_status, self._on_collect_status) + self.statuses = statuses + + def _on_collect_status(self, event): + for status in self.statuses: + event.add_status(ops.StatusBase.from_name(status, '')) + + fake_script(self, 'is-leader', 'echo true') + fake_script(self, 'status-set', 'exit 0') + + charm = MyCharm(self.create_framework(), statuses=['blocked', 'error']) + ops.charm._evaluate_status(charm) + + charm = MyCharm(self.create_framework(), statuses=['waiting', 'blocked']) + ops.charm._evaluate_status(charm) + + charm = MyCharm(self.create_framework(), statuses=['waiting', 'maintenance']) + ops.charm._evaluate_status(charm) + + charm = MyCharm(self.create_framework(), statuses=['active', 'waiting']) + ops.charm._evaluate_status(charm) + + charm = MyCharm(self.create_framework(), statuses=['active', 'unknown']) + ops.charm._evaluate_status(charm) + + charm = MyCharm(self.create_framework(), statuses=['unknown']) + ops.charm._evaluate_status(charm) + + status_set_calls = [call for call in fake_script_calls(self, True) + if call[0] == 'status-set'] + self.assertEqual(status_set_calls, [ + ['status-set', '--application=True', 'error', ''], + ['status-set', '--application=True', 'blocked', ''], + ['status-set', '--application=True', 'maintenance', ''], + ['status-set', '--application=True', 'waiting', ''], + ['status-set', '--application=True', 'active', ''], + ['status-set', '--application=True', 'unknown', ''], + ]) diff --git a/test/test_main.py b/test/test_main.py index 2e9d140fe..25c0ba286 100755 --- a/test/test_main.py +++ b/test/test_main.py @@ -17,6 +17,7 @@ import io import logging import os +import re import shutil import subprocess import sys @@ -73,6 +74,8 @@ def __init__(self, event_type, event_name, env_var=None, @patch('ops.main.setup_root_logging', new=lambda *a, **kw: None) +@patch('ops.main._emit_charm_event', new=lambda *a, **kw: None) +@patch('ops.charm._evaluate_status', new=lambda *a, **kw: None) class CharmInitTestCase(unittest.TestCase): @patch('sys.stderr', new_callable=io.StringIO) @@ -107,16 +110,15 @@ def _check(self, charm_class, *, extra_environ=None, **kwargs): if extra_environ is not None: fake_environ.update(extra_environ) with patch.dict(os.environ, fake_environ): - with patch('ops.main._emit_charm_event'): - with patch('ops.main._get_charm_dir') as mock_charmdir: - with tempfile.TemporaryDirectory() as tmpdirname: - tmpdirname = Path(tmpdirname) - fake_metadata = tmpdirname / 'metadata.yaml' - with fake_metadata.open('wb') as fh: - fh.write(b'name: test') - mock_charmdir.return_value = tmpdirname + with patch('ops.main._get_charm_dir') as mock_charmdir: + with tempfile.TemporaryDirectory() as tmpdirname: + tmpdirname = Path(tmpdirname) + fake_metadata = tmpdirname / 'metadata.yaml' + with fake_metadata.open('wb') as fh: + fh.write(b'name: test') + mock_charmdir.return_value = tmpdirname - ops.main(charm_class, **kwargs) + ops.main(charm_class, **kwargs) def test_init_signature_passthrough(self): class MyCharm(ops.CharmBase): @@ -174,6 +176,7 @@ def test_controller_storage_deprecated(self): @patch('sys.argv', new=("hooks/config-changed",)) @patch('ops.main.setup_root_logging', new=lambda *a, **kw: None) +@patch('ops.charm._evaluate_status', new=lambda *a, **kw: None) class TestDispatch(unittest.TestCase): def _check(self, *, with_dispatch=False, dispatch_path=''): """Helper for below tests.""" @@ -265,7 +268,8 @@ def cleanup(): ops.CharmBase.on = ops.CharmEvents() self.addCleanup(cleanup) - fake_script(self, 'juju-log', "exit 0") + fake_script(self, 'is-leader', 'echo true') + fake_script(self, 'juju-log', 'exit 0') # set to something other than None for tests that care self.stdout = None @@ -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'], ] calls = fake_script_calls(self) @@ -632,14 +637,19 @@ def test_custom_event(self): self._simulate_event(EventSpec(ops.UpdateStatusEvent, 'update-status', set_in_env={'EMIT_CUSTOM_EVENT': "1"})) + calls = fake_script_calls(self) + + custom_event_prefix = 'Emitting custom event .'], + ['juju-log', '--log-level', 'DEBUG', '--', custom_event_prefix], + ['is-leader', '--format=json'], ] - calls = fake_script_calls(self) - self.assertEqual(expected, calls) + # Remove the "[key]>" suffix from the end of the event string + self.assertRegex(calls[2][-1], re.escape(custom_event_prefix) + '.*') + calls[2][-1] = custom_event_prefix + self.assertEqual(calls, expected) def test_logger(self): fake_script(self, 'action-get', "echo '{}'") @@ -898,9 +908,10 @@ def test_hook_and_dispatch(self): 'Using local storage: not a Kubernetes podspec charm'], ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event install.'], + ['is-leader', '--format=json'], ] 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: ') self.assertEqual(calls, expected) def test_non_executable_hook_and_dispatch(self): @@ -917,9 +928,10 @@ def test_non_executable_hook_and_dispatch(self): 'Using local storage: not a Kubernetes podspec charm'], ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event install.'], + ['is-leader', '--format=json'], ] 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: ') self.assertEqual(calls, expected) def test_hook_and_dispatch_with_failing_hook(self): @@ -999,9 +1011,10 @@ def test_hook_and_dispatch_but_hook_is_dispatch_copy(self): 'Using local storage: not a Kubernetes podspec charm'], ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event install.'], + ['is-leader', '--format=json'], ] 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: ') self.assertEqual(calls, expected) diff --git a/test/test_testing.py b/test/test_testing.py index cdf76c2ce..f7c3b1d82 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -2691,6 +2691,27 @@ def test_get_filesystem_root(self): container = harness.charm.unit.get_container("foo") self.assertEqual(foo_root, harness.get_filesystem_root(container)) + def test_evaluate_status(self): + class TestCharm(ops.CharmBase): + def __init__(self, framework): + super().__init__(framework) + self.framework.observe(self.on.collect_app_status, self._on_collect_app_status) + self.framework.observe(self.on.collect_unit_status, self._on_collect_unit_status) + + def _on_collect_app_status(self, event): + event.add_status(ops.ActiveStatus()) + + def _on_collect_unit_status(self, event): + event.add_status(ops.BlockedStatus('bar')) + + harness = ops.testing.Harness(TestCharm) + harness.set_leader(True) + harness.begin() + # Tests for the behaviour of status evaluation are in test_charm.py + harness.evaluate_status() + self.assertEqual(harness.model.app.status, ops.ActiveStatus()) + self.assertEqual(harness.model.unit.status, ops.BlockedStatus('bar')) + class TestNetwork(unittest.TestCase): def setUp(self):