Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion ops/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Not the relation-broken event
return None

return int(os.environ.get("JUJU_RELATION_ID"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



def main(charm_class: Type[ops.charm.CharmBase],
use_juju_for_storage: Optional[bool] = None):
"""Setup the charm and dispatch the observed event.
Expand Down Expand Up @@ -391,8 +400,10 @@ def main(charm_class: Type[ops.charm.CharmBase],
else:
actions_metadata = None

breaking_relation_id = _get_breaking_relation_id()

meta = CharmMeta.from_yaml(metadata, actions_metadata)
model = ops.model.Model(meta, model_backend)
model = ops.model.Model(meta, model_backend, breaking_relation_id)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


charm_state_path = charm_dir / CHARM_STATE_FILE

Expand Down
19 changes: 14 additions & 5 deletions ops/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@ca-scribner ca-scribner Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self._config = ConfigData(self._backend)
resources: Iterable[str] = meta.resources
self._resources = Resources(list(resources), self._backend)
Expand Down Expand Up @@ -667,7 +669,8 @@ 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'):
backend: '_ModelBackend', cache: '_ModelCache',
breaking_relation_id: Optional[int] = None):
self._peers: Set[str] = set()
for name, relation_meta in relations_meta.items():
if relation_meta.role.is_peer():
Expand All @@ -676,6 +679,7 @@ def __init__(self, relations_meta: Dict[str, 'ops.RelationMeta'], our_unit: 'Uni
self._backend = backend
self._cache = cache
self._data: _RelationMapping_Raw = {r: None for r in relations_meta}
self._breaking_relation_id = breaking_relation_id

def __contains__(self, key: str):
return key in self._data
Expand All @@ -692,8 +696,10 @@ 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):
active = rid != self._breaking_relation_id
relation = Relation(relation_name, rid, is_peer,
self._our_unit, self._backend, self._cache)
self._our_unit, self._backend, self._cache,
active)
relation_list.append(relation)
return relation_list

Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
    """

"""

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):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could/should we evaluate it here instead of propagating?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if is_peer:
# For peer relations, both the remote and the local app are the same.
Expand Down