Repository: canonical/operator
Pull request #1123 (2024-02-07 04:08:27)

docs: tweak the wording around can_connect() usage
`can_connect()` is a point-in-time check, and, if used as a guard for interacting with Pebble, introduces a race condition where Pebble was responding at the `can_connect()` time, but is not when the rest of the work is done.

We already encourage people to handle errors communicating with Pebble when doing the later work (catching `ConnectionError`), which means that most of the time the `can_connect` call is not providing any value (and incurs a small cost). However, the documentation, particularly the example, encourages this pattern, and this has been copied to many other places (charm sources, docs).

This PR adjusts the documentation to be less encouraging of `can_connect()` use as a guard, and changes the example to one where a point-in-time check is wanted (for collecting unit status).


Patch:
diff --git a/CHANGES.md b/CHANGES.md
index c0049d3..8a52d81 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -1,3 +1,8 @@
+# 2.11.0
+
+* `StopEvent`, `RemoveEvent`, and all `LifeCycleEvent`s are no longer deferrable, and will raise a `RuntimeError` if `defer()` is called on the event object.
+* Added `ActionEvent.id`, exposing the JUJU_ACTION_UUID environment variable.
+
 # 2.10.0
 
 * Added support for Pebble Notices (`PebbleCustomNoticeEvent`, `get_notices`, and so on)
diff --git a/ops/charm.py b/ops/charm.py
index 4496383..8b2586d 100644
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -125,9 +125,16 @@ class ActionEvent(EventBase):
     :meth:`log`.
     """
 
+    id: str = ""
+    """The Juju ID of the action invocation."""
+
     params: Dict[str, Any]
     """The parameters passed to the action."""
 
+    def __init__(self, handle: 'Handle', id: Optional[str] = None):
+        super().__init__(handle)
+        self.id = id  # type: ignore (for backwards compatibility)
+
     def defer(self) -> NoReturn:
         """Action events are not deferrable like other events.
 
@@ -144,10 +151,18 @@ class ActionEvent(EventBase):
 
         Not meant to be called directly by charm code.
         """
+        self.id = cast(str, snapshot['id'])
         # Params are loaded at restore rather than __init__ because
         # the model is not available in __init__.
         self.params = self.framework.model._backend.action_get()
 
+    def snapshot(self) -> Dict[str, Any]:
+        """Used by the framework to serialize the event to disk.
+
+        Not meant to be called by charm code.
+        """
+        return {'id': self.id}
+
     def set_results(self, results: Dict[str, Any]):
         """Report the result of the action.
 
@@ -243,6 +258,17 @@ class StopEvent(HookEvent):
     and that it will not start again on reboot.
     """
 
+    def defer(self) -> NoReturn:
+        """Stop events are not deferrable like other events.
+
+        This is because the unit is in the process of tearing down, and there
+        will not be an opportunity for the deferred event to run.
+
+        Raises:
+            RuntimeError: always.
+        """
+        raise RuntimeError('cannot defer stop events')
+
 
 class RemoveEvent(HookEvent):
     """Event triggered when a unit is about to be terminated.
@@ -250,6 +276,17 @@ class RemoveEvent(HookEvent):
     This event fires prior to Juju removing the charm and terminating its unit.
     """
 
+    def defer(self) -> NoReturn:
+        """Remove events are not deferrable like other events.
+
+        This is because the unit is about to be torn down, and there
+        will not be an opportunity for the deferred event to run.
+
+        Raises:
+            RuntimeError: always.
+        """
+        raise RuntimeError('cannot defer remove events')
+
 
 class ConfigChangedEvent(HookEvent):
     """Event triggered when a configuration change occurs.
diff --git a/ops/framework.py b/ops/framework.py
index 717c74c..cc84436 100644
--- a/ops/framework.py
+++ b/ops/framework.py
@@ -38,6 +38,7 @@ from typing import (
     Iterable,
     List,
     Literal,
+    NoReturn,
     Optional,
     Protocol,
     Set,
@@ -523,6 +524,17 @@ class PrefixedEvents:
 class LifecycleEvent(EventBase):
     """Events tied to the lifecycle of the Framework object."""
 
+    def defer(self) -> NoReturn:
+        """Lifecycle events are not deferrable like other events.
+
+        This is because these events are run alongside each event invocation,
+        so deferring would always end up simply doubling the work.
+
+        Raises:
+            RuntimeError: always.
+        """
+        raise RuntimeError('cannot defer lifecycle events')
+
 
 class PreCommitEvent(LifecycleEvent):
     """Event that will be emitted first on commit."""
@@ -1057,10 +1069,10 @@ class BoundStoredState:
     if TYPE_CHECKING:
         # to help the type checker and IDEs:
         @property
-        def _data(self) -> StoredStateData: ...  # noqa, type: ignore
+        def _data(self) -> StoredStateData: ...  # type: ignore
 
         @property
-        def _attr_name(self) -> str: ...  # noqa, type: ignore
+        def _attr_name(self) -> str: ...  # type: ignore
 
     def __init__(self, parent: Object, attr_name: str):
         parent.framework.register_type(StoredStateData, parent)
diff --git a/ops/main.py b/ops/main.py
index 04c756c..6f7b893 100644
--- a/ops/main.py
+++ b/ops/main.py
@@ -191,6 +191,9 @@ def _get_event_args(charm: 'ops.charm.CharmBase',
         storage = cast(Union[ops.storage.JujuStorage, ops.storage.SQLiteStorage], storage)
         storage.location = storage_location  # type: ignore
         return [storage], {}
+    elif issubclass(event_type, ops.charm.ActionEvent):
+        args: List[Any] = [os.environ['JUJU_ACTION_UUID']]
+        return args, {}
     elif issubclass(event_type, ops.charm.RelationEvent):
         relation_name = os.environ['JUJU_RELATION']
         relation_id = _get_juju_relation_id()
diff --git a/ops/model.py b/ops/model.py
index 793d54e..ad9ed81 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -2065,13 +2065,15 @@ class Container:
     For methods that make changes to the container, if the change fails or times out, then a
     :class:`ops.pebble.ChangeError` or :class:`ops.pebble.TimeoutError` will be raised.
 
-    Interactions with the container use Pebble, so all methods may raise exceptions when there are
-    problems communicating with Pebble. Problems connecting to or transferring data with Pebble
-    will raise a :class:`ops.pebble.ConnectionError` - generally you can guard against these by
-    first checking :meth:`can_connect`, but it is possible for problems to occur after
-    :meth:`can_connect` has succeeded. When an error occurs executing the request, such as trying
-    to add an invalid layer or execute a command that does not exist, an
-    :class:`ops.pebble.APIError` is raised.
+    Interactions with the container use Pebble, so all methods may raise
+    exceptions when there are problems communicating with Pebble. Problems
+    connecting to or transferring data with Pebble will raise a
+    :class:`ops.pebble.ConnectionError` - you can guard against these by first
+    checking :meth:`can_connect`, but that generally introduces a race condition
+    where problems occur after :meth:`can_connect` has succeeded. When an error
+    occurs executing the request, such as trying to add an invalid layer or
+    execute a command that does not exist, an :class:`ops.pebble.APIError` is
+    raised.
     """
 
     name: str
@@ -2095,14 +2097,12 @@ class Container:
 
         For example::
 
+            # Add status based on any earlier errors communicating with Pebble.
+            ...
+            # Check that Pebble is still reachable now.
             container = self.unit.get_container("example")
-            if container.can_connect():
-                try:
-                    c.pull('/does/not/exist')
-                except ProtocolError, PathError:
-                    # handle it
-            else:
-                event.defer()
+            if not container.can_connect():
+                event.add_status(ops.WaitingStatus("Waiting for Pebble..."))
         """
         try:
             self._pebble.get_system_info()


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index 0d782a6..5d509cb 100644
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -246,6 +246,7 @@ class Harness(Generic[CharmType]):
         self._unit_name: str = f"{self._meta.name}/0"
         self._hooks_enabled: bool = True
         self._relation_id_counter: int = 0
+        self._action_id_counter: int = 0
         config_ = self._get_config(config)
         self._backend = _TestingModelBackend(self._unit_name, self._meta, config_)
         self._model = model.Model(self._meta, self._backend)
@@ -1883,7 +1884,8 @@ class Harness(Generic[CharmType]):
         action_under_test = _RunningAction(action_name, ActionOutput([], {}), params)
         handler = getattr(self.charm.on, f"{action_name.replace('-', '_')}_action")
         self._backend._running_action = action_under_test
-        handler.emit()
+        self._action_id_counter += 1
+        handler.emit(str(self._action_id_counter))
         self._backend._running_action = None
         if action_under_test.failure_message is not None:
             raise ActionFailed(
diff --git a/test/test_charm.py b/test/test_charm.py
index ac14784..627d2d5 100644
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -463,7 +463,7 @@ start:
             def _on_foo_bar_action(self, event: ops.ActionEvent):
                 self.seen_action_params = event.params
                 event.log('test-log')
-                event.set_results({'res': 'val with spaces'})
+                event.set_results({'res': 'val with spaces', 'id': event.id})
                 event.fail('test-fail')
 
             def _on_start_action(self, event: ops.ActionEvent):
@@ -477,12 +477,13 @@ start:
         self.assertIn('foo_bar_action', events)
         self.assertIn('start_action', events)
 
-        charm.on.foo_bar_action.emit()
+        action_id = "1234"
+        charm.on.foo_bar_action.emit(id=action_id)
         self.assertEqual(charm.seen_action_params, {"foo-name": "name", "silent": True})
         self.assertEqual(fake_script_calls(self), [
             ['action-get', '--format=json'],
             ['action-log', "test-log"],
-            ['action-set', "res=val with spaces"],
+            ['action-set', "res=val with spaces", f"id={action_id}"],
             ['action-fail', "test-fail"],
         ])
 
@@ -511,7 +512,7 @@ start:
             charm.res = bad_res
 
             with self.assertRaises(ValueError):
-                charm.on.foo_bar_action.emit()
+                charm.on.foo_bar_action.emit(id='1')
 
     def _test_action_event_defer_fails(self, cmd_type: str):
 
@@ -532,7 +533,7 @@ start:
         charm = MyCharm(framework)
 
         with self.assertRaises(RuntimeError):
-            charm.on.start_action.emit()
+            charm.on.start_action.emit(id='2')
 
     def test_action_event_defer_fails(self):
         self._test_action_event_defer_fails('action')
diff --git a/test/test_framework.py b/test/test_framework.py
index e7c7a20..7a2a778 100644
--- a/test/test_framework.py
+++ b/test/test_framework.py
@@ -1813,7 +1813,7 @@ class DebugHookTests(BaseTestCase):
 
         with patch('sys.stderr', new_callable=io.StringIO):
             with patch('pdb.runcall') as mock:
-                publisher.foobar_action.emit()
+                publisher.foobar_action.emit(id='1')
 
         self.assertEqual(mock.call_count, 1)
         self.assertFalse(observer.called)
@@ -1833,7 +1833,7 @@ class DebugHookTests(BaseTestCase):
 
         with patch('sys.stderr', new_callable=io.StringIO):
             with patch('pdb.runcall') as mock:
-                publisher.foobar_action.emit()
+                publisher.foobar_action.emit(id='2')
 
         self.assertEqual(mock.call_count, 1)
         self.assertFalse(observer.called)
diff --git a/test/test_main.py b/test/test_main.py
index 7c18495..3c9d5ec 100644
--- a/test/test_main.py
+++ b/test/test_main.py
@@ -586,11 +586,13 @@ class _TestMain(abc.ABC):
              'departing_unit': 'remote/42'},
         ), (
             EventSpec(ops.ActionEvent, 'start_action',
-                      env_var='JUJU_ACTION_NAME'),
+                      env_var='JUJU_ACTION_NAME',
+                      set_in_env={'JUJU_ACTION_UUID': '1'}),
             {},
         ), (
             EventSpec(ops.ActionEvent, 'foo_bar_action',
-                      env_var='JUJU_ACTION_NAME'),
+                      env_var='JUJU_ACTION_NAME',
+                      set_in_env={'JUJU_ACTION_UUID': '2'}),
             {},
         ), (
             EventSpec(ops.PebbleReadyEvent, 'test_pebble_ready',
@@ -726,19 +728,20 @@ class _TestMain(abc.ABC):
         fake_script(typing.cast(unittest.TestCase, self), 'action-get', "echo '{}'")
 
         test_cases = [(
-            EventSpec(ops.ActionEvent, 'log_critical_action', env_var='JUJU_ACTION_NAME'),
+            EventSpec(ops.ActionEvent, 'log_critical_action', env_var='JUJU_ACTION_NAME',
+                      set_in_env={'JUJU_ACTION_UUID': '1'}),
             ['juju-log', '--log-level', 'CRITICAL', '--', 'super critical'],
         ), (
             EventSpec(ops.ActionEvent, 'log_error_action',
-                      env_var='JUJU_ACTION_NAME'),
+                      env_var='JUJU_ACTION_NAME', set_in_env={'JUJU_ACTION_UUID': '2'}),
             ['juju-log', '--log-level', 'ERROR', '--', 'grave error'],
         ), (
             EventSpec(ops.ActionEvent, 'log_warning_action',
-                      env_var='JUJU_ACTION_NAME'),
+                      env_var='JUJU_ACTION_NAME', set_in_env={'JUJU_ACTION_UUID': '3'}),
             ['juju-log', '--log-level', 'WARNING', '--', 'wise warning'],
         ), (
             EventSpec(ops.ActionEvent, 'log_info_action',
-                      env_var='JUJU_ACTION_NAME'),
+                      env_var='JUJU_ACTION_NAME', set_in_env={'JUJU_ACTION_UUID': '4'}),
             ['juju-log', '--log-level', 'INFO', '--', 'useful info'],
         )]
 
@@ -779,7 +782,8 @@ class _TestMain(abc.ABC):
         state = self._simulate_event(EventSpec(
             ops.ActionEvent, 'get_model_name_action',
             env_var='JUJU_ACTION_NAME',
-            model_name='test-model-name'))
+            model_name='test-model-name',
+            set_in_env={'JUJU_ACTION_UUID': '1'}))
         assert isinstance(state, ops.BoundStoredState)
         self.assertEqual(state._on_get_model_name_action, ['test-model-name'])
 
@@ -791,7 +795,8 @@ class _TestMain(abc.ABC):
                     """echo '{"status": "unknown", "message": ""}'""")
         state = self._simulate_event(EventSpec(
             ops.ActionEvent, 'get_status_action',
-            env_var='JUJU_ACTION_NAME'))
+            env_var='JUJU_ACTION_NAME',
+            set_in_env={'JUJU_ACTION_UUID': '1'}))
         assert isinstance(state, ops.BoundStoredState)
         self.assertEqual(state.status_name, 'unknown')
         self.assertEqual(state.status_message, '')
@@ -801,7 +806,8 @@ class _TestMain(abc.ABC):
             """echo '{"status": "blocked", "message": "help meeee"}'""")
         state = self._simulate_event(EventSpec(
             ops.ActionEvent, 'get_status_action',
-            env_var='JUJU_ACTION_NAME'))
+            env_var='JUJU_ACTION_NAME',
+            set_in_env={'JUJU_ACTION_UUID': '1'}))
         assert isinstance(state, ops.BoundStoredState)
         self.assertEqual(state.status_name, 'blocked')
         self.assertEqual(state.status_message, 'help meeee')
@@ -1169,7 +1175,8 @@ class TestMainWithDispatch(_TestMainWithDispatch, unittest.TestCase):
         with self.assertRaises(subprocess.CalledProcessError):
             self._simulate_event(EventSpec(
                 ops.ActionEvent, 'keyerror_action',
-                env_var='JUJU_ACTION_NAME'))
+                env_var='JUJU_ACTION_NAME',
+                set_in_env={'JUJU_ACTION_UUID': '1'}))
         self.stderr.seek(0)
         stderr = self.stderr.read()
         self.assertIn('KeyError', stderr)
diff --git a/test/test_testing.py b/test/test_testing.py
index e937265..a2d0cfd 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -5445,6 +5445,7 @@ class TestActions(unittest.TestCase):
             def _on_simple_action(self, event: ops.ActionEvent):
                 """An action that doesn't generate logs, have any results, or fail."""
                 self.simple_was_called = True
+                assert isinstance(event.id, str)
 
             def _on_fail_action(self, event: ops.ActionEvent):
                 event.fail("this will be ignored")