[IPU] _handle_relation is incorrectly relying on relation-broken

[sed-i]

Bug Description

On relation-broken, we set current_urls to {}:

traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress_per_unit.py

Lines 737 to 742 in 3f0cc21

	current_urls = (
	{} if isinstance(event, RelationBrokenEvent) else self._urls_from_relation_data
	)
	self._stored.current_urls = current_urls # type: ignore
	removed = previous_urls.keys() - current_urls.keys() # type: ignore

And then we emit a custom event with self.realtion - which is None on relation-broken:

traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress_per_unit.py

Lines 761 to 762 in 3f0cc21

	for unit_name in removed:
	self.on.revoked.emit(self.relation, unit_name) # type: ignore

Now, with ops v2.10.0 (specifically ops/1091), this fails:

  File "/home/ubuntu/code/o11y/traefik-k8s-operator/.tox/unit/lib/python3.10/site-packages/ops/charm.py", line 434, in snapshot
    'relation_name': self.relation.name,
AttributeError: 'NoneType' object has no attribute 'name'

To Reproduce

Run the traefik-k8s-operator/tests/unit/test_lib_per_unit_requires.py utest with ops pinned to 2.10.0.

Environment

ops 2.10.0.

Relevant log output

File "/home/ubuntu/code/o11y/traefik-k8s-operator/tests/unit/test_lib_per_unit_requires.py", line 193, in test_ipu_events
    self.harness.remove_relation(self.rel_id)
  File "/home/ubuntu/code/o11y/traefik-k8s-operator/.tox/unit/lib/python3.10/site-packages/ops/testing.py", line 892, in remove_relation
    self._emit_relation_broken(relation_name, relation_id, remote_app)
  File "/home/ubuntu/code/o11y/traefik-k8s-operator/.tox/unit/lib/python3.10/site-packages/ops/testing.py", line 926, in _emit_relation_broken
    self._charm.on[relation_name].relation_broken.emit(relation, app)
  File "/home/ubuntu/code/o11y/traefik-k8s-operator/.tox/unit/lib/python3.10/site-packages/ops/framework.py", line 351, in emit
    framework._emit(event)
  File "/home/ubuntu/code/o11y/traefik-k8s-operator/.tox/unit/lib/python3.10/site-packages/ops/framework.py", line 853, in _emit
    self._reemit(event_path)
  File "/home/ubuntu/code/o11y/traefik-k8s-operator/.tox/unit/lib/python3.10/site-packages/ops/framework.py", line 943, in _reemit
    custom_handler(event)
  File "/home/ubuntu/code/o11y/traefik-k8s-operator/lib/charms/traefik_k8s/v1/ingress_per_unit.py", line 753, in _handle_relation
    self.on.revoked_for_unit.emit(self.relation)  # type: ignore
  File "/home/ubuntu/code/o11y/traefik-k8s-operator/.tox/unit/lib/python3.10/site-packages/ops/framework.py", line 351, in emit
    framework._emit(event)
  File "/home/ubuntu/code/o11y/traefik-k8s-operator/.tox/unit/lib/python3.10/site-packages/ops/framework.py", line 848, in _emit
    self.save_snapshot(event)
  File "/home/ubuntu/code/o11y/traefik-k8s-operator/.tox/unit/lib/python3.10/site-packages/ops/framework.py", line 717, in save_snapshot
    data = value.snapshot()
  File "/home/ubuntu/code/o11y/traefik-k8s-operator/.tox/unit/lib/python3.10/site-packages/ops/charm.py", line 434, in snapshot
    'relation_name': self.relation.name,
AttributeError: 'NoneType' object has no attribute 'name'

Additional context

No response

[tonyandrewmeyer]

Hi @sed-i ! Sorry about this - we tried to identify where charms might have issues with this change, but missed both this and also a similar case in mysql-router-k8s (hopefully that's it, but I'm trying to dig more to see if there are others - both of these were in a lib and the .relations usage was in a property, which contributed to me missing it) 😞.

For what it's worth, I think you can fix this fairly simply like:

diff --git a/lib/charms/traefik_k8s/v1/ingress_per_unit.py b/lib/charms/traefik_k8s/v1/ingress_per_unit.py
index 987bf46..2a93e46 100644
--- a/lib/charms/traefik_k8s/v1/ingress_per_unit.py
+++ b/lib/charms/traefik_k8s/v1/ingress_per_unit.py
@@ -63,7 +63,7 @@ import typing
 from typing import Any, Dict, Optional, Tuple, Union
 
 import yaml
-from ops.charm import CharmBase, RelationBrokenEvent, RelationEvent
+from ops.charm import CharmBase, RelationEvent
 from ops.framework import (
     EventSource,
     Object,
@@ -734,9 +734,7 @@ class IngressPerUnitRequirer(_IngressPerUnitBase):
         # we calculate the diff between the urls we were aware of
         # before and those we know now
         previous_urls = self._stored.current_urls or {}  # type: ignore
-        current_urls = (
-            {} if isinstance(event, RelationBrokenEvent) else self._urls_from_relation_data
-        )
+        current_urls = self._urls_from_relation_data
         self._stored.current_urls = current_urls  # type: ignore
 
         removed = previous_urls.keys() - current_urls.keys()  # type: ignore
@@ -750,7 +748,7 @@ class IngressPerUnitRequirer(_IngressPerUnitBase):
                 )
 
             if this_unit_name in removed:
-                self.on.revoked_for_unit.emit(self.relation)  # type: ignore
+                self.on.revoked_for_unit.emit(event.relation)  # type: ignore
 
         if self.listen_to in {"all-units", "both"}:
             for unit_name in changed:
@@ -759,7 +757,7 @@ class IngressPerUnitRequirer(_IngressPerUnitBase):
                 )
 
             for unit_name in removed:
-                self.on.revoked.emit(self.relation, unit_name)  # type: ignore
+                self.on.revoked.emit(event.relation, unit_name)  # type: ignore
 
         self._publish_auto_data()

This does get the tests passing again, and I think is what was intended. If you want to work with older ops then you could keep the isinstance check in place. This should actually be more reliable, since the relation-broken event will always have the appropriate relation attached but getting the relations from Juju would lose that (e.g. if the event was deferred) since relation-broken is the very end.