Repository: canonical/operator

Issue:
Add Relation.broken to allow querying if a relation is broken (breaking)
It's sometimes good to be able to query for relations and then determine if the relation you're looking at is broken (due to the current event being a relation-broken event for the relation in question, or while handling deferred events before that event). So it would be good to add a `Relation.broken` boolean attribute (or perhaps its inverse, `Relation.active`?) that ops sets based on the relation data and incoming event.

This is to solve the issue I've been discussing with @carlcsaposs-canonical with the mysql-router-k8s charm using a ["stateless status setting" approach](https://github.com/canonical/mysql-router-k8s-operator/commit/608c094273afdb7adb8ad3e9ef975ca4f8af4865), but having to pass the current event down to all their get-status functions. It'd be conceptually simpler (and perhaps better in the case of deferred events?) to just be able to query `relation.broken`.

So this should probably be done as part of or shortly after the multi-status work (spec OP033).

Discussion:
tonyandrewmeyer: I found 25 Charms (in the Canonical charms list we have collated) that have relation-broken events. 4 of those 25 use `Model.relations` in a relation-broken handler, and all should continue to work with (and be better for) this change. 3 of the 25 charms have an existing form of "relation is active" so could have simplified code after this change.
tonyandrewmeyer: Among the use cases this should address:
* Collect status - to put the unit/app into a Blocked state when a required relation is broken ([example](https://github.com/canonical/sdcore-smf-k8s-operator/pull/34/files)) - this can just do `if not self.model.relations[name]` and it will cover both the relation being broken and the relation not existing.
* Avoiding setting relation data during relation broken ([example](https://github.com/canonical/operator/issues/793)) - this can just do `and event.active`
* Update configuration to exclude the broken relation without passing around the event ([example](https://github.com/canonical/sdcore-nms-k8s-operator/issues/27)) - this should also be covered by `relations` excluding the broken relation. The only open question is the idea mentioned in that issue that an event _after_ relation-broken could still list the broken relation.
* Know which properties can be accessed ([example](https://github.com/canonical/mysql-router-k8s-operator/commit/608c094273afdb7adb8ad3e9ef975ca4f8af4865)) - covered by `.active`.
benhoyt: >  3 of the 25 charms have an existing form of "relation is active" so could have simplified code after this change.

@tonyandrewmeyer Do you happen to have links to the code where these 3 charms do this? I'd be curious to see examples of that and how people are doing it now.
benhoyt: Oh wait, I see you've linked to a couple of them in your follow-up [comment](https://github.com/canonical/operator/pull/1091#issuecomment-1855552240) above -- that's fine, thanks.
tonyandrewmeyer: > Ideally people don't need to respond to `.active` because juju just doesn't include those ids in `relation-ids` but I think this follows what we are trying to do, and is better than where we have been.

Yes, the intention is that Juju changes to do this but to have ops lead the way.
benhoyt: > Yes, the intention is that Juju changes to do this but to have ops lead the way.

@tonyandrewmeyer Can you please search to see if we already have a [Juju bug](https://bugs.launchpad.net/juju/) open for this (I kinda doubt it), and if not, open one to track?

Patch:
diff --git a/CHANGES.md b/CHANGES.md
index 5be0036..a26f33c 100644
--- a/CHANGES.md
+++ b/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
 
diff --git a/ops/main.py b/ops/main.py
index 7eeeab2..04c756c 100644
--- a/ops/main.py
+++ b/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
 
diff --git a/ops/model.py b/ops/model.py
index f04b95b..fe59c7b 100644
--- a/ops/model.py
+++ b/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 @@ class Model:
 
         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,8 +802,12 @@ class LazyMapping(Mapping[str, str], ABC):
 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():
@@ -806,6 +815,7 @@ class RelationMapping(Mapping[str, List['Relation']]):
         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 @@ class RelationMapping(Mapping[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 @@ class RelationMapping(Mapping[str, List['Relation']]):
                 # 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 @@ class Relation:
                     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".


Test patch:
diff --git a/.github/workflows/db-charm-tests.yaml b/.github/workflows/db-charm-tests.yaml
index b5ac5ab..153c8c8 100644
--- a/.github/workflows/db-charm-tests.yaml
+++ b/.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"
diff --git a/ops/testing.py b/ops/testing.py
index e399747..d3b883b 100644
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -880,8 +880,18 @@ class Harness(Generic[CharmType]):
         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)
diff --git a/test/charms/test_main/src/charm.py b/test/charms/test_main/src/charm.py
index bb6312d..fb48d5e 100755
--- a/test/charms/test_main/src/charm.py
+++ b/test/charms/test_main/src/charm.py
@@ -143,6 +143,8 @@ class Charm(ops.CharmBase):
 
     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,6 +156,8 @@ class Charm(ops.CharmBase):
             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()
@@ -161,6 +165,8 @@ class Charm(ops.CharmBase):
     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 @@ class Charm(ops.CharmBase):
             '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()
diff --git a/test/test_model.py b/test/test_model.py
index 00c5fc5..588b3e2 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -2243,6 +2243,34 @@ class TestModelBindings(unittest.TestCase):
             ['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''')
diff --git a/test/test_testing.py b/test/test_testing.py
index f515f61..44dd15a 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -583,6 +583,33 @@ class TestHarness(unittest.TestCase):
         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'],