From 793007efbf56a55019df9ec24d0ad663632d1bc3 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 5 Sep 2024 13:05:25 +1200 Subject: [PATCH 1/4] Always include the event's remote unit data in event.relation.data. --- ops/_main.py | 2 ++ ops/model.py | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/ops/_main.py b/ops/_main.py index a49284652..7d62f03a6 100644 --- a/ops/_main.py +++ b/ops/_main.py @@ -205,6 +205,8 @@ def _get_event_args( kwargs['app'] = model.get_app(remote_app_name) if remote_unit_name: kwargs['unit'] = model.get_unit(remote_unit_name) + if relation: + relation._event_remote_unit = kwargs['unit'] if departing_unit_name: kwargs['departing_unit_name'] = departing_unit_name diff --git a/ops/model.py b/ops/model.py index 9102f30d0..28e9330f1 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1601,6 +1601,8 @@ class Relation: ``relation-broken`` event associated with this relation. """ + _event_remote_unit: Optional[Unit] = None + def __init__( self, relation_name: str, @@ -1630,6 +1632,11 @@ def __init__( # If the relation is dead, just treat it as if it has no remote units. self.active = False + # In relation-departed, the relation ID is not included in relation-list, + # but the data should still be accessible, so we explicitly include it here. + if self._event_remote_unit: + self.units.add(self._event_remote_unit) + # 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". if app is None: From 3a69d28257ab61df08f35871d89a71327e2774e5 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 5 Sep 2024 13:13:29 +1200 Subject: [PATCH 2/4] Directly add to .units in main. --- ops/_main.py | 5 ++++- ops/model.py | 7 ------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/ops/_main.py b/ops/_main.py index 7d62f03a6..9012ee527 100644 --- a/ops/_main.py +++ b/ops/_main.py @@ -206,7 +206,10 @@ def _get_event_args( if remote_unit_name: kwargs['unit'] = model.get_unit(remote_unit_name) if relation: - relation._event_remote_unit = kwargs['unit'] + # In relation-departed, the relation ID is not included in + # relation-list, but the data should still be accessible, so we + # explicitly include it here. + relation.units.add(kwargs['unit']) if departing_unit_name: kwargs['departing_unit_name'] = departing_unit_name diff --git a/ops/model.py b/ops/model.py index 28e9330f1..9102f30d0 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1601,8 +1601,6 @@ class Relation: ``relation-broken`` event associated with this relation. """ - _event_remote_unit: Optional[Unit] = None - def __init__( self, relation_name: str, @@ -1632,11 +1630,6 @@ def __init__( # If the relation is dead, just treat it as if it has no remote units. self.active = False - # In relation-departed, the relation ID is not included in relation-list, - # but the data should still be accessible, so we explicitly include it here. - if self._event_remote_unit: - self.units.add(self._event_remote_unit) - # 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". if app is None: From 53c8c9c5101612a4cc0ea3ca8e8885c4fa8156dc Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 5 Sep 2024 13:35:33 +1200 Subject: [PATCH 3/4] Directly add the data, not the unit. --- ops/_main.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ops/_main.py b/ops/_main.py index 9012ee527..6080f9d22 100644 --- a/ops/_main.py +++ b/ops/_main.py @@ -209,7 +209,9 @@ def _get_event_args( # In relation-departed, the relation ID is not included in # relation-list, but the data should still be accessible, so we # explicitly include it here. - relation.units.add(kwargs['unit']) + relation.data._data[kwargs['unit']] = ops.model.RelationDataContent( + relation, kwargs['unit'], model._backend + ) if departing_unit_name: kwargs['departing_unit_name'] = departing_unit_name From 8a5d4d06a9cd9ab26e939ed6de1875972d442733 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 6 Sep 2024 12:42:18 +1200 Subject: [PATCH 4/4] Use a much more invasive approach, likely more correct, and more testable. --- ops/_main.py | 17 +++++++---------- ops/model.py | 27 +++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/ops/_main.py b/ops/_main.py index 6080f9d22..dca521af8 100644 --- a/ops/_main.py +++ b/ops/_main.py @@ -205,13 +205,6 @@ def _get_event_args( kwargs['app'] = model.get_app(remote_app_name) if remote_unit_name: kwargs['unit'] = model.get_unit(remote_unit_name) - if relation: - # In relation-departed, the relation ID is not included in - # relation-list, but the data should still be accessible, so we - # explicitly include it here. - relation.data._data[kwargs['unit']] = ops.model.RelationDataContent( - relation, kwargs['unit'], model._backend - ) if departing_unit_name: kwargs['departing_unit_name'] = departing_unit_name @@ -488,14 +481,18 @@ def _make_storage(self, dispatcher: _Dispatcher): def _make_framework(self, dispatcher: _Dispatcher): # 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 self._juju_context.dispatch_path.endswith('-relation-broken'): broken_relation_id = self._juju_context.relation_id else: broken_relation_id = None - + # Similarly, if we are in a RelationBroken or RelationDeparted event, we + # need to provide access to the remote relation data, even though the + # relation will not be returned from the `relation-list` hook tool. model = ops.model.Model( - self._charm_meta, self._model_backend, broken_relation_id=broken_relation_id + self._charm_meta, + self._model_backend, + broken_relation_id=broken_relation_id, + remote_unit_name=self._juju_context.remote_unit_name, ) store = self._make_storage(dispatcher) framework = ops.framework.Framework( diff --git a/ops/model.py b/ops/model.py index 9102f30d0..763ebd9a6 100644 --- a/ops/model.py +++ b/ops/model.py @@ -118,13 +118,19 @@ def __init__( meta: 'ops.charm.CharmMeta', backend: '_ModelBackend', broken_relation_id: Optional[int] = None, + remote_unit_name: Optional[str] = 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, broken_relation_id=broken_relation_id + relations, + self.unit, + self._backend, + self._cache, + broken_relation_id=broken_relation_id, + _remote_unit=self._cache.get(Unit, remote_unit_name) if remote_unit_name else None, ) self._config = ConfigData(self._backend) resources: Iterable[str] = meta.resources @@ -872,12 +878,14 @@ def __init__( backend: '_ModelBackend', cache: '_ModelCache', broken_relation_id: Optional[int], + _remote_unit: Optional['Unit'] = None, ): 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._remote_unit = _remote_unit self._backend = backend self._cache = cache self._broken_relation_id = broken_relation_id @@ -901,7 +909,13 @@ def __getitem__(self, relation_name: str) -> List['Relation']: if rid == self._broken_relation_id: continue relation = Relation( - relation_name, rid, is_peer, self._our_unit, self._backend, self._cache + relation_name, + rid, + is_peer, + self._our_unit, + self._backend, + self._cache, + _remote_unit=self._remote_unit, ) relation_list.append(relation) return relation_list @@ -936,6 +950,7 @@ def _get_unique(self, relation_name: str, relation_id: Optional[int] = None): self._backend, self._cache, active=False, + _remote_unit=self._remote_unit, ) relations = self[relation_name] num_related = len(relations) @@ -1601,6 +1616,8 @@ class Relation: ``relation-broken`` event associated with this relation. """ + _remote_unit: Optional[Unit] + def __init__( self, relation_name: str, @@ -1610,6 +1627,7 @@ def __init__( backend: '_ModelBackend', cache: '_ModelCache', active: bool = True, + _remote_unit: Optional[Unit] = None, ): self.name = relation_name self.id = relation_id @@ -1630,6 +1648,11 @@ def __init__( # If the relation is dead, just treat it as if it has no remote units. self.active = False + # In relation-departing and relation-broken, `relation-list` doesn't + # include the remote unit, but the data should still be available. + if _remote_unit is not None: + self.units.add(_remote_unit) + # 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". if app is None: