diff --git a/.github/workflows/db-charm-tests.yaml b/.github/workflows/db-charm-tests.yaml index b5ac5abd7..153c8c880 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/CHANGES.md b/CHANGES.md index 5be00362b..a26f33c5a 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 7eeeab267..04c756c72 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 f04b95b4e..fe59c7bab 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 @@ 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,8 +802,12 @@ 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(): @@ -806,6 +815,7 @@ def __init__(self, relations_meta: Dict[str, 'ops.RelationMeta'], our_unit: 'Uni 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". diff --git a/ops/testing.py b/ops/testing.py index e399747f3..d3b883b99 100644 --- a/ops/testing.py +++ b/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) diff --git a/test/charms/test_main/src/charm.py b/test/charms/test_main/src/charm.py index bb6312dae..fb48d5ecb 100755 --- a/test/charms/test_main/src/charm.py +++ b/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,6 +156,8 @@ 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() @@ -161,6 +165,8 @@ def _on_mon_relation_changed(self, event: ops.RelationChangedEvent): 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() diff --git a/test/test_model.py b/test/test_model.py index 00c5fc5c1..588b3e273 100644 --- a/test/test_model.py +++ b/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''') diff --git a/test/test_testing.py b/test/test_testing.py index f515f618b..44dd15ad6 100644 --- a/test/test_testing.py +++ b/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'],