feat: add Relation.active, exclude inactive relations from Model.rela…

…tions (#1091)

This PR allows Charmers to easily check whether a relation is broken
during a `relation-broken` event (and also framework events that
share the same Juju context) without having to store state or pass
the `event` object around. In addition, the broken relation is
excluded from the `Model.relations` list, to make it easy for charms
to work on only the relations that will continue to exist after the
relation-broken events complete (without having to do `(rel for rel
in self.model.relations if rel.active)` themselves.

To do this:

* In `relation-broken` events, set a new attribute `active` on the
  event's `Relation` object to False (and have this be True in all
  other cases).
* In `relation-broken` events, `RelationMapping` `__getitem__`
  excludes the event's relation.

Builds on work from @ca-scribner in
#958.

[Additional background notes]
(https://docs.google.com/document/d/1yKLRtfffxxLeH4tkfkOL4ELfzyJK33WC5lQaCZXgjjk/edit?usp=sharing)
(limited to Canonical).

Fixes #940

.github/workflows/db-charm-tests.yaml

@@ -9,7 +9,8 @@ jobs:
       fail-fast: false
       matrix:
         charm-repo:
-          - "canonical/postgresql-operator"
+# TODO: uncomment once secrets issue is fixed in this charm (see https://github.com/canonical/postgresql-operator/pull/307)
+#          - "canonical/postgresql-operator"
           - "canonical/postgresql-k8s-operator"
 # TODO: uncomment once secrets issues are fixed in these charms (eg: https://github.com/canonical/mysql-operator/issues/367)
 #          - "canonical/mysql-operator"

CHANGES.md

@@ -1,6 +1,7 @@
 # 2.10.0
 
 * Added support for Pebble Notices (`PebbleCustomNoticeEvent`, `get_notices`, and so on)
+* Added `Relation.active`, and excluded inactive relations from `Model.relations`
 
 # 2.9.0
 

ops/main.py

@@ -144,6 +144,10 @@ def _emit_charm_event(charm: 'ops.charm.CharmBase', event_name: str):
         event_to_emit.emit(*args, **kwargs)
 
 
+def _get_juju_relation_id():
+    return int(os.environ['JUJU_RELATION_ID'].split(':')[-1])
+
+
 def _get_event_args(charm: 'ops.charm.CharmBase',
                     bound_event: 'ops.framework.BoundEvent') -> Tuple[List[Any], Dict[str, Any]]:
     event_type = bound_event.event_type
@@ -189,7 +193,7 @@ def _get_event_args(charm: 'ops.charm.CharmBase',
         return [storage], {}
     elif issubclass(event_type, ops.charm.RelationEvent):
         relation_name = os.environ['JUJU_RELATION']
-        relation_id = int(os.environ['JUJU_RELATION_ID'].split(':')[-1])
+        relation_id = _get_juju_relation_id()
         relation: Optional[ops.model.Relation] = model.get_relation(relation_name, relation_id)
 
     remote_app_name = os.environ.get('JUJU_REMOTE_APP', '')
@@ -396,8 +400,14 @@ def main(charm_class: Type[ops.charm.CharmBase],
     else:
         actions_metadata = None
 
+    # 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.
+    if os.environ.get('JUJU_DISPATCH_PATH', '').endswith('-relation-broken'):
+        broken_relation_id = _get_juju_relation_id()
+    else:
+        broken_relation_id = None
     meta = CharmMeta.from_yaml(metadata, actions_metadata)
-    model = ops.model.Model(meta, model_backend)
+    model = ops.model.Model(meta, model_backend, broken_relation_id=broken_relation_id)
 
     charm_state_path = charm_dir / CHARM_STATE_FILE
 

ops/model.py

@@ -111,12 +111,14 @@ class Model:
     as ``self.model`` from any class that derives from :class:`Object`.
     """
 
-    def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend'):
+    def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend',
+                 broken_relation_id: Optional[int] = None):
         self._cache = _ModelCache(meta, backend)
         self._backend = backend
         self._unit = self.get_unit(self._backend.unit_name)
         relations: Dict[str, 'ops.RelationMeta'] = meta.relations
-        self._relations = RelationMapping(relations, self.unit, self._backend, self._cache)
+        self._relations = RelationMapping(relations, self.unit, self._backend, self._cache,
+                                          broken_relation_id=broken_relation_id)
         self._config = ConfigData(self._backend)
         resources: Iterable[str] = meta.resources
         self._resources = Resources(list(resources), self._backend)
@@ -147,6 +149,9 @@ def relations(self) -> 'RelationMapping':
 
         Answers the question "what am I currently related to".
         See also :meth:`.get_relation`.
+
+        In a ``relation-broken`` event, the broken relation is excluded from
+        this list.
         """
         return self._relations
 
@@ -797,15 +802,20 @@ def __repr__(self):
 class RelationMapping(Mapping[str, List['Relation']]):
     """Map of relation names to lists of :class:`Relation` instances."""
 
-    def __init__(self, relations_meta: Dict[str, 'ops.RelationMeta'], our_unit: 'Unit',
-                 backend: '_ModelBackend', cache: '_ModelCache'):
+    def __init__(self,
+                 relations_meta: Dict[str, 'ops.RelationMeta'],
+                 our_unit: 'Unit',
+                 backend: '_ModelBackend',
+                 cache: '_ModelCache',
+                 broken_relation_id: Optional[int]):
         self._peers: Set[str] = set()
         for name, relation_meta in relations_meta.items():
             if relation_meta.role.is_peer():
                 self._peers.add(name)
         self._our_unit = our_unit
         self._backend = backend
         self._cache = cache
+        self._broken_relation_id = broken_relation_id
         self._data: _RelationMapping_Raw = {r: None for r in relations_meta}
 
     def __contains__(self, key: str):
@@ -823,6 +833,8 @@ def __getitem__(self, relation_name: str) -> List['Relation']:
         if not isinstance(relation_list, list):
             relation_list = self._data[relation_name] = []  # type: ignore
             for rid in self._backend.relation_ids(relation_name):
+                if rid == self._broken_relation_id:
+                    continue
                 relation = Relation(relation_name, rid, is_peer,
                                     self._our_unit, self._backend, self._cache)
                 relation_list.append(relation)
@@ -850,7 +862,7 @@ def _get_unique(self, relation_name: str, relation_id: Optional[int] = None):
                 # The relation may be dead, but it is not forgotten.
                 is_peer = relation_name in self._peers
                 return Relation(relation_name, relation_id, is_peer,
-                                self._our_unit, self._backend, self._cache)
+                                self._our_unit, self._backend, self._cache, active=False)
         relations = self[relation_name]
         num_related = len(relations)
         self._backend._validate_relation_access(
@@ -1454,13 +1466,21 @@ class Relation:
     This is accessed using, for example, ``Relation.data[unit]['foo']``.
     """
 
+    active: bool
+    """Indicates whether this relation is active.
+
+    This is normally ``True``; it will be ``False`` if the current event is a
+    ``relation-broken`` event associated with this relation.
+    """
+
     def __init__(
             self, relation_name: str, relation_id: int, is_peer: bool, our_unit: Unit,
-            backend: '_ModelBackend', cache: '_ModelCache'):
+            backend: '_ModelBackend', cache: '_ModelCache', active: bool = True):
         self.name = relation_name
         self.id = relation_id
         self.app: Optional[Application] = None
         self.units: Set[Unit] = set()
+        self.active = active
 
         if is_peer:
             # For peer relations, both the remote and the local app are the same.
@@ -1475,7 +1495,7 @@ def __init__(
                     self.app = unit.app
         except RelationNotFoundError:
             # If the relation is dead, just treat it as if it has no remote units.
-            pass
+            self.active = False
 
         # If we didn't get the remote app via our_unit.app or the units list,
         # look it up via JUJU_REMOTE_APP or "relation-list --app".

ops/testing.py

@@ -880,8 +880,18 @@ def remove_relation(self, relation_id: int) -> None:
         for unit_name in rel_list_map[relation_id].copy():
             self.remove_relation_unit(relation_id, unit_name)
 
+        prev_broken_id = None  # Silence linter warning.
+        if self._model is not None:
+            # Let the model's RelationMapping know that this relation is broken.
+            # Normally, this is handled in `main`, but while testing we create
+            # the `Model` object and keep it around for multiple events.
+            prev_broken_id = self._model._relations._broken_relation_id
+            self._model.relations._broken_relation_id = relation_id
+            # Ensure that we don't offer a cached relation.
+            self._model.relations._invalidate(relation_name)
         self._emit_relation_broken(relation_name, relation_id, remote_app)
         if self._model is not None:
+            self._model.relations._broken_relation_id = prev_broken_id
             self._model.relations._invalidate(relation_name)
 
         self._backend._relation_app_and_units.pop(relation_id)

test/charms/test_main/src/charm.py

@@ -143,6 +143,8 @@ def _on_leader_settings_changed(self, event: ops.LeaderSettingsChangedEvent):
 
     def _on_db_relation_joined(self, event: ops.RelationJoinedEvent):
         assert event.app is not None, 'application name cannot be None for a relation-joined event'
+        assert event.relation.active, 'a joined relation is always active'
+        assert self.model.relations['db']
         self._stored.on_db_relation_joined.append(type(event).__name__)
         self._stored.observed_event_types.append(type(event).__name__)
         self._stored.db_relation_joined_data = event.snapshot()
@@ -154,13 +156,17 @@ def _on_mon_relation_changed(self, event: ops.RelationChangedEvent):
             assert event.unit is not None, (
                 'a unit name cannot be None for a relation-changed event'
                 ' associated with a remote unit')
+        assert event.relation.active, 'a changed relation is always active'
+        assert self.model.relations['mon']
         self._stored.on_mon_relation_changed.append(type(event).__name__)
         self._stored.observed_event_types.append(type(event).__name__)
         self._stored.mon_relation_changed_data = event.snapshot()
 
     def _on_mon_relation_departed(self, event: ops.RelationDepartedEvent):
         assert event.app is not None, (
             'application name cannot be None for a relation-departed event')
+        assert event.relation.active, 'a departed relation is still active'
+        assert self.model.relations['mon']
         self._stored.on_mon_relation_departed.append(type(event).__name__)
         self._stored.observed_event_types.append(type(event).__name__)
         self._stored.mon_relation_departed_data = event.snapshot()
@@ -170,6 +176,8 @@ def _on_ha_relation_broken(self, event: ops.RelationBrokenEvent):
             'relation-broken events cannot have a reference to a remote application')
         assert event.unit is None, (
             'relation broken events cannot have a reference to a remote unit')
+        assert not event.relation.active, 'relation broken events always have a broken relation'
+        assert not self.model.relations['ha']
         self._stored.on_ha_relation_broken.append(type(event).__name__)
         self._stored.observed_event_types.append(type(event).__name__)
         self._stored.ha_relation_broken_data = event.snapshot()

test/test_model.py

@@ -2243,6 +2243,34 @@ def test_dead_relations(self):
             ['network-get', 'db0', '--format=json'],
         ])
 
+    def test_broken_relations(self):
+        meta = ops.CharmMeta()
+        meta.relations = {
+            'db0': ops.RelationMeta(
+                ops.RelationRole.provides, 'db0', {'interface': 'db0', 'scope': 'global'}),
+            'db1': ops.RelationMeta(
+                ops.RelationRole.requires, 'db1', {'interface': 'db1', 'scope': 'global'}),
+            'db2': ops.RelationMeta(
+                ops.RelationRole.peer, 'db2', {'interface': 'db2', 'scope': 'global'}),
+        }
+        backend = _ModelBackend('myapp/0')
+        model = ops.Model(meta, backend, broken_relation_id=8)
+        fake_script(self, 'relation-ids',
+                    """if [ "$1" = "db0" ]; then
+                         echo '["db0:4"]'
+                       elif [ "$1" = "db1" ]; then
+                         echo '["db1:8"]'
+                       elif [ "$1" = "db2" ]; then
+                         echo '["db2:16"]'
+                       else
+                         echo '[]'
+                       fi
+                    """)
+        fake_script(self, 'relation-list', """echo '""'""")
+        self.assertTrue(model.relations['db0'])
+        self.assertFalse(model.relations['db1'])
+        self.assertTrue(model.relations['db2'])
+
     def test_binding_by_relation_name(self):
         fake_script(self, 'network-get',
                     f'''[ "$1" = db0 ] && echo '{self.network_get_out}' || exit 1''')

test/test_testing.py

@@ -583,6 +583,33 @@ def test_removing_relation_refreshes_charm_model(self):
         harness.remove_relation(rel_id)
         self.assertIsNone(self._find_relation_in_model_by_id(harness, rel_id))
 
+    def test_remove_relation_marks_relation_as_inactive(self):
+        relations: typing.List[str] = []
+        is_broken = False
+
+        class MyCharm(ops.CharmBase):
+            def __init__(self, framework: ops.Framework):
+                super().__init__(framework)
+                framework.observe(self.on.db_relation_broken, self._db_relation_broken)
+
+            def _db_relation_broken(self, event: ops.RelationBrokenEvent):
+                nonlocal is_broken, relations
+                is_broken = not event.relation.active
+                relations = [rel.name for rel in self.model.relations["db"]]
+
+        harness = ops.testing.Harness(MyCharm, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            ''')
+        self.addCleanup(harness.cleanup)
+        harness.begin()
+        rel_id = harness.add_relation('db', 'postgresql')
+        harness.remove_relation(rel_id)
+        self.assertTrue(is_broken, 'event.relation.active not False in relation-broken event')
+        self.assertFalse(relations, 'Model.relations contained broken relation')
+
     def _find_relation_in_model_by_id(
             self,
             harness: ops.testing.Harness['RelationEventCharm'],