Add `Relation.active` attribute to indicate if a relation is active or currently breaking #958

ca-scribner · 2023-06-30T15:50:32Z

(Still WIP - opened as draft to start discussion on implementation)

This PR adds the Relation.active attribute to indicate whether the relation is active (most cases) or inactive (as implemented here, if the relation is part of a relation-broken event, but maybe there are other cases that should be inactive as well). See #940 for more context.

Alternate approaches/ideas:

This could have been Relation.broken or Relation.breaking, which is more directly descriptive, but the expectation is that people probably want to know what is active rather than not, and there may be other future reasons that a Relation is inactive
as discussed here, if iterating through model.relations[my_relation_name], should you even see inactive relations? Hiding inactive relations from that iterable would be a breaking change, but probably one most people would expect in the first place. We could add something like model.relations.get_active() or similar so at least people don't need to do their own filtering.

Checklist

Have any types changed? If so, have the type annotations been updated?

If this code exposes model data, does it do so thoughtfully, in a way that aids understanding?

We want to avoid simple passthrough of model data, without contextualization, where possible.

NA Do error messages, if any, present charm authors or operators with enough information to solve the problem?

QA steps

TODO

Documentation changes

TODO: May need to elaborate in the docs

Bug reference

Closes #940

Changelog

adds Relation.active attribute, indicating whether a relation is active vs breaking

Adds an attribute to Relation that indicates whether the relation is active (most cases) or inactive (when this relation is currently in a relation-broken event).

sed-i · 2023-06-30T16:19:18Z

ops/model.py

        self.name = relation_name
        self.id = relation_id
        self.app: Optional[Application] = None
        self.units: Set[Unit] = set()
+        self.active = active


Could/should we evaluate it here instead of propagating?

Sorry I'm not sure what you mean - can you restate this?

on the one hand it would greatly reduce the amount of code changes, on the other it is nice that code this 'deep' in ops is profoundly unaware of juju-specific envvars. It'd be nice if the only pieces of code doing envvar unpacking were in ops.main.

How we propagate that info tastefully and in a future-proof manner, that is the question

Yeah, it would be "simpler", but I definitely prefer doing as much env var handling as possible in main.py, so it's contained there. It's a bit awkward to pass it down, but I think important for testability and explicitness.

PietroPasotti · 2023-07-04T06:39:03Z

ops/model.py

        self.name = relation_name
        self.id = relation_id
        self.app: Optional[Application] = None
        self.units: Set[Unit] = set()
+        self.active = active


on the one hand it would greatly reduce the amount of code changes, on the other it is nice that code this 'deep' in ops is profoundly unaware of juju-specific envvars. It'd be nice if the only pieces of code doing envvar unpacking were in ops.main.

How we propagate that info tastefully and in a future-proof manner, that is the question

PietroPasotti · 2023-07-04T06:43:14Z

ops/model.py

@@ -107,12 +107,14 @@ class Model:
    from any class that derives from Object.
    """

-    def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend'):
+    def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend',
+                 breaking_relation_id: Optional[int] = None):


I think it would be more elegant if we attached this information to the event instead of the model.

breaking-relation-id is deceptive as it suggests any relation ID could be that of a relation that is breaking, while in fact the only relation we could possibly know to be breaking is 'the current one', aka 'the one the current event is about'.
So it makes more sense to me to attach that information to the event.

That also spares us to add model.relation.get_active because there's only ever going to be one relation in breaking state: the one we get from the event we're observing.

I propose the following api:

def _on_foo_relation_event(self, e:RelationEvent): if e.is_dying: ...

this feels a bit more opsy.

that said, this would make it more complicated to access the same information from code that doesn't directly have access to the event object. Maybe this was the whole point of this exercise?

yeah the motivation for me with this issue is a case where, if possible, not being tied to the event would be nice.

I had tried to implement a nicely encapsulated getter on a relation charm lib, but found since it might be called during relation-broken I needed to spot and omit the breaking application. To do that existing ops and without spying on the JUJU_* variables, it meant passing the event through several layers of code down to the getter. Not impossible, but it felt awkward at best

You're right though that this is event-specific behaviour. But then since model.relations exposes the relation information and the event influences what that data means, I think ops sort of commits itself to giving access to the full context via model.relations

Yeah, I agree with Andrew here: we specifically want this info to be available on the Relation object, rather then the event.

benhoyt

Thanks for this! I've left some comments requesting a few minor changes, but I think the logic is sound and how you're passing the environment variable down explicitly is good.

This will of course need tests, but let's wait on that for a bit -- I'm planning to discuss this with John M tomorrow, about whether we go with this approach or move toward relations not even including breaking/inactive relations at all. So you're welcome to hold off a bit until we decide whether to proceed with this approach.

benhoyt · 2023-07-05T03:09:14Z

ops/main.py

@@ -361,6 +361,15 @@ def _should_use_controller_storage(db_path: Path, meta: CharmMeta) -> bool:
        return False


+def _get_breaking_relation_id() -> Optional[int]:
+    """Returns the relation id of the currently breaking relation, if applicable."""
+    if not os.environ.get("JUJU_HOOK_NAME", "").endswith("-relation-broken"):


We don't actually use JUJU_HOOK_NAME in ops anymore, we use the newer JUJU_DISPATCH_PATH. However, that's already parsed by the _Dispatcher class here, and the event name (with - already replaced with _) is available as dispatcher.event_name. So I suggest we do this in main():

breaking_relation_id = None if dispatcher.event_name.endswith('_relation_broken'): breaking_relation_id = _parse_relation_id()

benhoyt · 2023-07-05T03:10:37Z

ops/main.py

+        # Not the relation-broken event
+        return None
+
+    return int(os.environ.get("JUJU_RELATION_ID"))


This is not actually correct, as JUJU_RELATION_ID has the format relation-name:123. Elsewhere in main.py this is parsed using int(os.environ['JUJU_RELATION_ID'].split(':')[-1]). So let's pull that one-liner out into _parse_relation_id() -> int and use that in both places.

benhoyt · 2023-07-05T03:12:36Z

ops/main.py

    meta = CharmMeta.from_yaml(metadata, actions_metadata)
-    model = ops.model.Model(meta, model_backend)
+    model = ops.model.Model(meta, model_backend, breaking_relation_id)


I know it's not required, but let's pass in optional arguments using their keywords, to avoid accidental breakage if we add more and change the order. So Model(meta, model_backend, breaking_relation_id=breaking_relation_id).

benhoyt · 2023-07-05T03:13:34Z

ops/model.py

@@ -107,12 +107,14 @@ class Model:
    from any class that derives from Object.
    """

-    def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend'):
+    def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend',
+                 breaking_relation_id: Optional[int] = None):


Yeah, I agree with Andrew here: we specifically want this info to be available on the Relation object, rather then the event.

benhoyt · 2023-07-05T03:13:56Z

ops/model.py

        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,
+                                          breaking_relation_id)


Similar here: let's use breaking_relation_id=breaking_relation_id for clarity and to avoid mistakes.

benhoyt · 2023-07-05T03:19:59Z

ops/model.py

@@ -1244,15 +1250,18 @@ class Relation:
            For subordinate relations, this set will include only one unit: the principal unit.
        data: A :class:`RelationData` holding the data buckets for each entity
            of a relation. Accessed via eg Relation.data[unit]['foo']
+        active: Indicates whether this relation is active (True) or inactive (False, because
+            this relation is currently part of a relation-broken event)


These attributes have now been type-hinted and their docstrings moved to under the type hint so Sphinx picks it up for each attribute, so I believe this will conflict with the main branch now. Let's do something like this:

class Relation: ... active: bool """Indicates whether this relation is active. This will be ``False`` if the current event is a ``relation-broken`` event associated with this relation. """

benhoyt · 2023-07-05T03:25:06Z

ops/model.py

    """

    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):


For this one, I'd probably use a required (non-keyword) argument -- that is, don't default to True, but require active to be provided. My rationale is that we should not forget to specify active correctly on all call sites.

That would have caught the fact that we've forgotten here to specify active=False in the other call to Relation() in _get_unique. See the comment "The relation may be dead, but it is not forgotten." I believe that case should always set active to False.

benhoyt · 2023-07-05T03:26:09Z

ops/model.py

        self.name = relation_name
        self.id = relation_id
        self.app: Optional[Application] = None
        self.units: Set[Unit] = set()
+        self.active = active


Yeah, it would be "simpler", but I definitely prefer doing as much env var handling as possible in main.py, so it's contained there. It's a bit awkward to pass it down, but I think important for testability and explicitness.

ca-scribner · 2023-07-05T15:11:34Z

@benhoyt ty for review! everything makes sense, but I'll hold off till you talk with John in case something changes.

ca-scribner · 2023-08-04T13:46:55Z

Closing as I believe this issue will be addressed elsewhere in juju

@ca-scribner

…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

feat: add Relation.active attribute

67f5f5f

Adds an attribute to Relation that indicates whether the relation is active (most cases) or inactive (when this relation is currently in a relation-broken event).

sed-i reviewed Jun 30, 2023

View reviewed changes

PietroPasotti reviewed Jul 4, 2023

View reviewed changes

benhoyt requested changes Jul 5, 2023

View reviewed changes

ca-scribner closed this Aug 4, 2023

tonyandrewmeyer mentioned this pull request Dec 14, 2023

feat: add Relation.active, exclude inactive relations from Model.relations #1091

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Relation.active` attribute to indicate if a relation is active or currently breaking #958

Add `Relation.active` attribute to indicate if a relation is active or currently breaking #958

ca-scribner commented Jun 30, 2023

sed-i Jun 30, 2023

ca-scribner Jun 30, 2023

PietroPasotti Jul 4, 2023

benhoyt Jul 5, 2023

PietroPasotti Jul 4, 2023

PietroPasotti Jul 4, 2023

PietroPasotti Jul 4, 2023

ca-scribner Jul 4, 2023 •

edited

Loading

ca-scribner Jul 4, 2023

benhoyt Jul 5, 2023

benhoyt left a comment

benhoyt Jul 5, 2023

benhoyt Jul 5, 2023

benhoyt Jul 5, 2023

benhoyt Jul 5, 2023

benhoyt Jul 5, 2023

benhoyt Jul 5, 2023

benhoyt Jul 5, 2023

benhoyt Jul 5, 2023

ca-scribner commented Jul 5, 2023

ca-scribner commented Aug 4, 2023

Add Relation.active attribute to indicate if a relation is active or currently breaking #958

Add Relation.active attribute to indicate if a relation is active or currently breaking #958

Conversation

ca-scribner commented Jun 30, 2023

Checklist

QA steps

Documentation changes

Bug reference

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ca-scribner Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ca-scribner commented Jul 5, 2023

ca-scribner commented Aug 4, 2023

Add `Relation.active` attribute to indicate if a relation is active or currently breaking #958

Add `Relation.active` attribute to indicate if a relation is active or currently breaking #958

ca-scribner Jul 4, 2023 •

edited

Loading