feat: run deferred events with fresh charm instances

ops/_main.py

@@ -49,27 +49,27 @@ def _exe_path(path: Path) -> Optional[Path]:
 
 
 def _create_event_link(
-    charm: '_charm.CharmBase',
+    charm_dir: Path,
     bound_event: '_framework.BoundEvent',
     link_to: Union[str, Path],
 ):
     """Create a symlink for a particular event.
 
     Args:
-        charm: A charm object.
+        charm_dir: The root charm directory.
         bound_event: An event for which to create a symlink.
         link_to: What the event link should point to
     """
     # type guard
     assert bound_event.event_kind, f'unbound BoundEvent {bound_event}'
 
     if issubclass(bound_event.event_type, _charm.HookEvent):
-        event_dir = charm.framework.charm_dir / 'hooks'
+        event_dir = charm_dir / 'hooks'
         event_path = event_dir / bound_event.event_kind.replace('_', '-')
     elif issubclass(bound_event.event_type, _charm.ActionEvent):
         if not bound_event.event_kind.endswith('_action'):
             raise RuntimeError(f'action event name {bound_event.event_kind} needs _action suffix')
-        event_dir = charm.framework.charm_dir / 'actions'
+        event_dir = charm_dir / 'actions'
         # The event_kind is suffixed with "_action" while the executable is not.
         event_path = event_dir / bound_event.event_kind[: -len('_action')].replace('_', '-')
     else:
@@ -109,7 +109,7 @@ def _setup_event_links(charm_dir: Path, charm: '_charm.CharmBase', juju_context:
     for bound_event in charm.on.events().values():
         # Only events that originate from Juju need symlinks.
         if issubclass(bound_event.event_type, (_charm.HookEvent, _charm.ActionEvent)):
-            _create_event_link(charm, bound_event, link_to)
+            _create_event_link(charm_dir, bound_event, link_to)
 
 
 def _get_event_args(
@@ -364,31 +364,36 @@ class _Manager:
       - the event that Juju is emitting on us
       - the charm instance (user-facing)
     - emit: core user-facing lifecycle step. Consists of:
-      - re-emit any deferred events found in the storage
       - emit the Juju event to the charm
         - emit any custom events emitted by the charm during this phase
       - emit  the ``collect-status`` events
     - commit: responsible for:
       - store any events deferred throughout this execution
       - graceful teardown of the storage
+
+    The above steps are first run for any deferred notices found in the storage
+    (all three steps for each notice, except for emitting status collection
+    events), and then run a final time for the Juju event that triggered this
+    execution.
     """
 
     def __init__(
         self,
         charm_class: Type['_charm.CharmBase'],
-        model_backend: Optional[_model._ModelBackend] = None,
+        juju_context: _JujuContext,
         use_juju_for_storage: Optional[bool] = None,
         charm_state_path: str = CHARM_STATE_FILE,
-        juju_context: Optional[_JujuContext] = None,
     ):
-        if juju_context is None:
-            juju_context = _JujuContext.from_dict(os.environ)
+        # The context is shared across deferred events and the Juju event. Any
+        # data from the context that is event-specific must be included in the
+        # event object snapshot/restore rather than re-read from the context.
+        # Data not connected to the event (debug settings, the Juju version, the
+        # app and unit name, and so forth) will be the *current* data, not the
+        # data at the time the event was deferred -- this aligns with the data
+        # from hook tools.
         self._juju_context = juju_context
         self._charm_state_path = charm_state_path
         self._charm_class = charm_class
-        if model_backend is None:
-            model_backend = _model._ModelBackend(juju_context=self._juju_context)
-        self._model_backend = model_backend
 
         # Do this as early as possible to be sure to catch the most logs.
         self._setup_root_logging()
@@ -397,33 +402,62 @@ def __init__(
         self._charm_meta = self._load_charm_meta()
         self._use_juju_for_storage = use_juju_for_storage
 
-        # Set up dispatcher, framework and charm objects.
-        self.dispatcher = _Dispatcher(self._charm_root, self._juju_context)
-        self.dispatcher.run_any_legacy_hook()
+        # Handle legacy hooks - this is only done once, not with each deferred
+        # event.
+        self._dispatcher = _Dispatcher(self._charm_root, self._juju_context)
+        self._dispatcher.run_any_legacy_hook()
+
+        # Storage is shared across all events, so we create it once here.
+        self._storage = self._make_storage()
+
+        self.run_deferred()
+
+        # This is the charm for the Juju event. We create it here so that it's
+        # available for pre-emit adjustments when being used in testing.
+        self.charm = self._make_charm(self._dispatcher.event_name)
 
-        self.framework = self._make_framework(self.dispatcher)
-        self.charm = self._make_charm(self.framework, self.dispatcher)
+        # This is with the charm used for the Juju event, but it's being removed
+        # later this cycle anyway, so we want minimum tweaking.
+        self._dispatcher.ensure_event_links(self.charm)
 
     def _load_charm_meta(self):
         return _charm.CharmMeta.from_charm_root(self._charm_root)
 
-    def _make_charm(self, framework: '_framework.Framework', dispatcher: _Dispatcher):
-        charm = self._charm_class(framework)
-        dispatcher.ensure_event_links(charm)
-        return charm
+    def _make_model_backend(self):
+        # model._ModelBackend is stateless and can be reused across events.
+        # However, in testing (both Harness and Scenario) the backend stores all
+        # the state that is normally in Juju. To be consistent, we create a new
+        # backend object even in production code.
+        return _model._ModelBackend(juju_context=self._juju_context)
+
+    def _make_charm(self, event_name: str):
+        framework = self._make_framework(event_name)
+        return self._charm_class(framework)
 
     def _setup_root_logging(self):
         # For actions, there is a communication channel with the user running the
         # action, so we want to send exception details through stderr, rather than
         # only to juju-log as normal.
         handling_action = self._juju_context.action_name is not None
+        # We don't really want to have a different backend here than when
+        # running the event. However, we need to create a new backend for each
+        # event and want the logging set up before we are ready to emit an
+        # event. In practice, this isn't a problem:
+        # * for model._ModelBackend, `juju_log` calls out directly to the hook
+        #   tool; it's effectively a staticmethod.
+        # * for _private.harness._TestingModelBackend, `juju_log` is not
+        #   implemented, and the logging is never configured.
+        # * for scenario.mocking._MockModelBackend, `juju_log` sends the logging
+        #   through to the `Context` object, which will be the same for all
+        #   events.
+        # TODO: write tests to make sure that everything remains ok here.
         setup_root_logging(
-            self._model_backend, debug=self._juju_context.debug, exc_stderr=handling_action
+            self._make_model_backend(), debug=self._juju_context.debug, exc_stderr=handling_action
         )
 
         logger.debug('ops %s up and running.', _version.version)
 
-    def _make_storage(self, dispatcher: _Dispatcher):
+    def _make_storage(self):
         charm_state_path = self._charm_root / self._charm_state_path
 
         use_juju_for_storage = self._use_juju_for_storage
@@ -443,15 +477,12 @@ def _make_storage(self, dispatcher: _Dispatcher):
                 category=DeprecationWarning,
             )
 
-        if use_juju_for_storage and dispatcher.is_restricted_context():
-            # TODO: jam 2020-06-30 This unconditionally avoids running a collect metrics event
-            #  Though we eventually expect that Juju will run collect-metrics in a
-            #  non-restricted context. Once we can determine that we are running
-            #  collect-metrics in a non-restricted context, we should fire the event as normal.
+        if use_juju_for_storage and self._dispatcher.is_restricted_context():
+            # collect-metrics is going away in Juju 4.0, and restricted context
+            # with it, so we don't need this to be particularly generic.
             logger.debug(
-                '"%s" is not supported when using Juju for storage\n'
+                '"collect_metrics" is not supported when using Juju for storage\n'
                 'see: https://github.com/canonical/operator/issues/348',
-                dispatcher.event_name,
             )
             # Note that we don't exit nonzero, because that would cause Juju to rerun the hook
             raise _Abort(0)
@@ -462,7 +493,7 @@ def _make_storage(self, dispatcher: _Dispatcher):
             store = _storage.SQLiteStorage(charm_state_path)
         return store
 
-    def _make_framework(self, dispatcher: _Dispatcher):
+    def _make_framework(self, event_name: str):
         # If we are in a RelationBroken event, we want to know which relation is
         # broken within the model, not only in the event's `.relation` attribute.
 
@@ -471,80 +502,99 @@ def _make_framework(self, dispatcher: _Dispatcher):
         else:
             broken_relation_id = None
 
+        model_backend = self._make_model_backend()
         model = _model.Model(
-            self._charm_meta, self._model_backend, broken_relation_id=broken_relation_id
+            self._charm_meta, model_backend, broken_relation_id=broken_relation_id
         )
-        store = self._make_storage(dispatcher)
         framework = _framework.Framework(
-            store,
+            self._storage,
             self._charm_root,
             self._charm_meta,
             model,
-            event_name=dispatcher.event_name,
+            event_name=event_name,
             juju_debug_at=self._juju_context.debug_at,
         )
         framework.set_breakpointhook()
         return framework
 
-    def _emit(self):
+    def _emit(self, charm: _charm.CharmBase, event_name: str):
         """Emit the event on the charm."""
-        # TODO: Remove the collect_metrics check below as soon as the relevant
-        #       Juju changes are made. Also adjust the docstring on
-        #       EventBase.defer().
-        #
-        # Skip reemission of deferred events for collect-metrics events because
-        # they do not have the full access to all hook tools.
-        if not self.dispatcher.is_restricted_context():
-            # Re-emit any deferred events from the previous run.
-            self.framework.reemit()
-
         # Emit the Juju event.
-        self._emit_charm_event(self.dispatcher.event_name)
+        self._emit_charm_event(charm, event_name)
         # Emit collect-status events.
-        _charm._evaluate_status(self.charm)
+        _charm._evaluate_status(charm)
 
-    def _get_event_to_emit(self, event_name: str) -> Optional[_framework.BoundEvent]:
+    def _get_event_to_emit(
+        self, charm: _charm.CharmBase, event_name: str
+    ) -> Optional[_framework.BoundEvent]:
         try:
-            return getattr(self.charm.on, event_name)
+            return getattr(charm.on, event_name)
         except AttributeError:
-            logger.debug('Event %s not defined for %s.', event_name, self.charm)
+            logger.debug('Event %s not defined for %s.', event_name, charm)
         return None
 
-    def _emit_charm_event(self, event_name: str):
+    def _emit_charm_event(self, charm: _charm.CharmBase, event_name: str):
         """Emits a charm event based on a Juju event name.
 
         Args:
             charm: A charm instance to emit an event from.
             event_name: A Juju event name to emit on a charm.
             juju_context: An instance of the _JujuContext class.
         """
-        event_to_emit = self._get_event_to_emit(event_name)
+        event_to_emit = self._get_event_to_emit(charm, event_name)
 
         # If the event is not supported by the charm implementation, do
         # not error out or try to emit it. This is to support rollbacks.
         if event_to_emit is None:
             return
 
-        args, kwargs = _get_event_args(self.charm, event_to_emit, self._juju_context)
+        args, kwargs = _get_event_args(charm, event_to_emit, self._juju_context)
         logger.debug('Emitting Juju event %s.', event_name)
         event_to_emit.emit(*args, **kwargs)
 
-    def _commit(self):
+    def _commit(self, framework: _framework.Framework):
         """Commit the framework and gracefully teardown."""
-        self.framework.commit()
+        framework.commit()
 
     def _close(self):
         """Perform any necessary cleanup before the framework is closed."""
         # Provided for child classes - nothing needs to be done in the base.
 
+    def run_deferred(self):
+        """Emit and then commit the framework.
+
+        A framework and charm object are created for each notice in the storage
+        (an event and observer pair), the relevant deferred event is emitted,
+        and the framework is committed. Note that collect-status events are not
+        emitted.
+        """
+        # TODO: Remove the restricted context check below once we no longer need
+        #       to support Juju < 4 (collect-metrics and restricted context are
+        #       being removed in Juju 4.0).
+        #
+        # Skip re-emission of deferred events for collect-metrics events because
+        # they do not have the full access to all hook tools.
+        if self._dispatcher.is_restricted_context():
+            logger.debug("Skipping re-emission of deferred events in restricted context.")
+            return
+        # Re-emit previously deferred events to the observers that deferred them.
+        for event_path, _, _ in self._storage.notices():
+            event_handle = _framework.Handle.from_path(event_path)
+            logger.debug('Re-emitting deferred event: %s', event_handle)
+            charm = self._make_charm(event_handle.kind)
+            charm.framework.reemit(event_path)
+            self._commit(charm.framework)
+            self._close()
+            charm._destroy_charm()
+
     def run(self):
         """Emit and then commit the framework."""
         try:
-            self._emit()
-            self._commit()
+            self._emit(self.charm, self._dispatcher.event_name)
+            self._commit(self.charm.framework)
             self._close()
         finally:
-            self.framework.close()
+            self.charm.framework.close()
 
 
 def main(charm_class: Type[_charm.CharmBase], use_juju_for_storage: Optional[bool] = None):
@@ -553,7 +603,10 @@ def main(charm_class: Type[_charm.CharmBase], use_juju_for_storage: Optional[boo
     See `ops.main() <#ops-main-entry-point>`_ for details.
     """
     try:
-        manager = _Manager(charm_class, use_juju_for_storage=use_juju_for_storage)
+        juju_context = _JujuContext.from_dict(os.environ)
+        manager = _Manager(
+            charm_class, use_juju_for_storage=use_juju_for_storage, juju_context=juju_context
+        )
 
         manager.run()
     except _Abort as e:

ops/charm.py

@@ -1359,6 +1359,31 @@ def __init__(self, framework: Framework):
                 f'{container_name}_pebble_check_recovered', PebbleCheckRecoveredEvent
             )
 
+    def _destroy_charm(self):
+        for relation_name in self.framework.meta.relations:
+            relation_name = relation_name.replace('-', '_')
+            self.on.undefine_event(f'{relation_name}_relation_created')
+            self.on.undefine_event(f'{relation_name}_relation_joined')
+            self.on.undefine_event(f'{relation_name}_relation_changed')
+            self.on.undefine_event(f'{relation_name}_relation_departed')
+            self.on.undefine_event(f'{relation_name}_relation_broken')
+
+        for storage_name in self.framework.meta.storages:
+            storage_name = storage_name.replace('-', '_')
+            self.on.undefine_event(f'{storage_name}_storage_attached')
+            self.on.undefine_event(f'{storage_name}_storage_detaching')
+
+        for action_name in self.framework.meta.actions:
+            action_name = action_name.replace('-', '_')
+            self.on.undefine_event(f'{action_name}_action')
+
+        for container_name in self.framework.meta.containers:
+            container_name = container_name.replace('-', '_')
+            self.on.undefine_event(f'{container_name}_pebble_ready')
+            self.on.undefine_event(f'{container_name}_pebble_custom_notice')
+            self.on.undefine_event(f'{container_name}_pebble_check_failed')
+            self.on.undefine_event(f'{container_name}_pebble_check_recovered')
+
     @property
     def app(self) -> model.Application:
         """Application that this unit is part of."""