Repository: canonical/operator

Issue:
`ops.harness` prohibits setting application data both in peer relation created and leader elected events
## Issue

Attached to this report is a simple charm (zip file) demonstrating a problem with `ops.harness`. This (workload-less) charm only has handlers for `RelationCreated` and `LeaderElected` events. The charm sets different peer application data fields in response each of these two events. The charm can be built and deployed without any error. Also the charm successfully sets both data items in peer relation created and leader elected events (as can be seen using juju show-unit). 

Also provided with the charm are two unit tests both of which fail. The only difference between these two unit tests is the order in which the peer relation created and leader elected events are executed. The tests fail because accessing/setting the peer application relation data fields fails. Note this failure is not a result of a missing leader guard which is implemented. 

PS: If there is an ordering between these two events it was not obvious from the [reference docs](https://ops.readthedocs.io/en/latest/). If there is an ordering it will be great to have this made explicit in the doc strings. Note peer relation created and relation created are qualitatively different events in that the former will always happen when a charm is deployed. This issue pertains to the peer relation created event. 


[harness-test.zip](https://github.com/canonical/operator/files/8098103/harness-test.zip)



Discussion:
rwcarlsen: This breaks more tests than I had hoped.  Maybe @simskij @sed-i @balbirthomas could chime in on whether you think this would be worth pursuing anyway.
sed-i: Seems to me like an important change. I am happy to fix the tests. @simskij?
rwcarlsen: @sed-i - I wouldn't change your tests yet.  I was hoping to get some discussion here before we settle on whether or not this is the right approach.
simskij: > @sed-i - I wouldn't change your tests yet. I was hoping to get some discussion here before we settle on whether or not this is the right approach.

Good call! Regardless, we're happy to do whatever modifications are needed to our test (if any) if it helps improve the usefulness of the harness. 👍🏼  
simskij: Also, would it make more sense to reverse the check? I.e. if a peer relation is added **after** `set_leader` has been called, throw an error, rather than the opposite. It would have the benefit of accomodating the particular scenario @jameinel is describing. 
rwcarlsen: @jameinel That is a good point.  I'm trying to catch the first scenario you stated.  Technically, this PR generates an error on an invalid configuration of the test harness - meaning that juju wouldn't ever (hopefully ;-) put a charm in such a state.  But that doesn't necessarily mean that charm devs should be forced to care if they are not testing anything affected by that misconfiguration.  I poked around the code and couldn't come up with a good way to provide a better error for *only* the case where the test writer accesses app/peer relation data through the set_leader hook.  I'll ponder this a bit more.
jameinel: You should be able to look at CharmMeta to see if a peer relation is
defined and then check if there is an associated relation id.

On Mon, Mar 28, 2022 at 5:19 PM Robert Carlsen ***@***.***>
wrote:

> @jameinel <https://github.com/jameinel> That is a good point. I'm trying
> to catch the first scenario you stated. Technically, this PR generates an
> error on an invalid configuration of the test harness - meaning that juju
> wouldn't ever (hopefully ;-) put a charm in such a state. But that doesn't
> necessarily mean that charm devs should be forced to care if they are not
> testing anything affected by that misconfiguration. I poked around the code
> and couldn't come up with a good way to provide a better error for *only*
> the case where the test writer accesses app/peer relation data through the
> set_leader hook. I'll ponder this a bit more.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/canonical/operator/pull/733#issuecomment-1081160460>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABRQ7OPCWJE4GZB7XCDC6LVCIO55ANCNFSM5R4FPJCA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>

rwcarlsen: Okay - I have made some mods that solve the issue @jameinel brought up, but the code required to make it happen is a bit awkward.  I had to add a validation method to the backend that the RelationMapping class triggers when it's about to return `None` for a relation dict-access op.  For production, this method does nothing.  But in the harness, it checks to see if a special backend state/flag has been set (indicating we expect the peer relation to exist) and raises the error accordingly.  Do you guys think this is too much fanfare to deal with this error case?
rwcarlsen: This should be ready.  Let's get a final verdict on this and either get it in, or scrap it.

Patch:
diff --git a/ops/framework.py b/ops/framework.py
index f455bbd..f08845c 100755
--- a/ops/framework.py
+++ b/ops/framework.py
@@ -753,6 +753,31 @@ class Framework(Object):
         self._reemit()
 
     def _reemit(self, single_event_path=None):
+
+        class EventContext:
+            """Handles toggling the hook-is-running state in backends.
+
+            This allows e.g. harness logic to know if it is executing within a running hook context
+            or not.  It sets backend._hook_is_running equal to the name of the currently running
+            hook (e.g. "set-leader") and reverts back to the empty string when the hook execution
+            is completed.
+            """
+
+            def __init__(self, framework, event_name):
+                self._event = event_name
+                self._backend = None
+                if framework.model is not None:
+                    self._backend = framework.model._backend
+
+            def __enter__(self):
+                if self._backend:
+                    self._backend._hook_is_running = self._event
+                return self
+
+            def __exit__(self, exception_type, exception, traceback):
+                if self._backend:
+                    self._backend._hook_is_running = ''
+
         last_event_path = None
         deferred = True
         for event_path, observer_path, method_name in self._storage.notices(single_event_path):
@@ -779,15 +804,16 @@ class Framework(Object):
                 if custom_handler:
                     event_is_from_juju = isinstance(event, charm.HookEvent)
                     event_is_action = isinstance(event, charm.ActionEvent)
-                    if (
-                        event_is_from_juju or event_is_action
-                    ) and self._juju_debug_at.intersection({'all', 'hook'}):
-                        # Present the welcome message and run under PDB.
-                        self._show_debug_code_message()
-                        pdb.runcall(custom_handler, event)
-                    else:
-                        # Regular call to the registered method.
-                        custom_handler(event)
+                    with EventContext(self, event_handle.kind):
+                        if (
+                            event_is_from_juju or event_is_action
+                        ) and self._juju_debug_at.intersection({'all', 'hook'}):
+                            # Present the welcome message and run under PDB.
+                            self._show_debug_code_message()
+                            pdb.runcall(custom_handler, event)
+                        else:
+                            # Regular call to the registered method.
+                            custom_handler(event)
 
             if event.deferred:
                 deferred = True
diff --git a/ops/model.py b/ops/model.py
index 583c55b..388b18a 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -492,7 +492,9 @@ class RelationMapping(Mapping):
                 is_peer = relation_name in self._peers
                 return Relation(relation_name, relation_id, is_peer,
                                 self._our_unit, self._backend, self._cache)
-        num_related = len(self[relation_name])
+        relations = self[relation_name]
+        num_related = len(relations)
+        self._backend._validate_relation_access(relation_name, relations)
         if num_related == 0:
             return None
         elif num_related == 1:
@@ -1585,6 +1587,7 @@ class _ModelBackend:
 
         self._is_leader = None
         self._leader_check_time = None
+        self._hook_is_running = ''
 
     def _run(self, *args, return_output=False, use_json=False):
         kwargs = dict(stdout=PIPE, stderr=PIPE, check=True)
@@ -1609,6 +1612,14 @@ class _ModelBackend:
     def _is_relation_not_found(model_error):
         return 'relation not found' in str(model_error)
 
+    def _validate_relation_access(self, relation_name, relations):
+        """Checks for relation usage inconsistent with the framework/backend state.
+
+        This is used for catching Harness configuration errors and the production implementation
+        here should remain empty.
+        """
+        pass
+
     def relation_ids(self, relation_name):
         relation_ids = self._run('relation-ids', relation_name, return_output=True, use_json=True)
         return [int(relation_id.split(':')[-1]) for relation_id in relation_ids]


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index 335ae52..48ae58a 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -952,16 +952,19 @@ class Harness(typing.Generic[CharmType]):
     def set_leader(self, is_leader: bool = True) -> None:
         """Set whether this unit is the leader or not.
 
-        If this charm becomes a leader then `leader_elected` will be triggered.
+        If this charm becomes a leader then `leader_elected` will be triggered.  If Harness.begin()
+        has already been called, then the charm's peer relation should usually be added  *prior* to
+        calling this method (i.e. with Harness.add_relation) to properly initialize and make
+        available relation data that leader elected hooks may want to access.
 
         Args:
             is_leader: True/False as to whether this unit is the leader.
         """
-        was_leader = self._backend._is_leader
         self._backend._is_leader = is_leader
+
         # Note: jam 2020-03-01 currently is_leader is cached at the ModelBackend level, not in
         # the Model objects, so this automatically gets noticed.
-        if is_leader and not was_leader and self._charm is not None and self._hooks_enabled:
+        if is_leader and self._charm is not None and self._hooks_enabled:
             self._charm.on.leader_elected.emit()
 
     def set_planned_units(self, num_units: int) -> None:
@@ -1103,6 +1106,24 @@ class _TestingModelBackend:
         self._pebble_clients = {}  # type: {str: _TestingPebbleClient}
         self._pebble_clients_can_connect = {}  # type: {_TestingPebbleClient: bool}
         self._planned_units = None
+        self._hook_is_running = ''
+
+    def _validate_relation_access(self, relation_name, relations):
+        """Ensures that the named relation exists/has been added.
+
+        This is called whenever relation data is accessed via model.get_relation(...).
+        """
+        if len(relations) > 0:
+            return
+
+        relations = list(self._meta.peers.keys())
+        relations.extend(self._meta.requires.keys())
+        relations.extend(self._meta.provides.keys())
+        if self._hook_is_running == 'leader_elected' and relation_name in relations:
+            raise RuntimeError(
+                'cannot access relation data without first adding the relation: '
+                'use Harness.add_relation({!r}, <app>) before calling set_leader'
+                .format(relation_name))
 
     def _can_connect(self, pebble_client) -> bool:
         """Returns whether the mock client is active and can support API calls with no errors."""
diff --git a/test/test_testing.py b/test/test_testing.py
index 83c9333..795ea28 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -58,6 +58,20 @@ from ops.testing import (
 is_linux = platform.system() == 'Linux'
 
 
+class SetLeaderErrorTester(CharmBase):
+    """Sets peer relation data inside leader-elected."""
+
+    def __init__(self, framework):
+        super().__init__(framework)
+        self._peer_name = 'peer'
+        self.framework.observe(self.on.leader_elected,
+                               self._on_leader_elected)
+
+    def _on_leader_elected(self, event):
+        peers = self.model.get_relation(self._peer_name)
+        peers.data[self.app]["foo"] = "bar"
+
+
 class StorageTester(CharmBase):
     """Record the relation-changed events."""
 
@@ -791,6 +805,21 @@ class TestHarness(unittest.TestCase):
         self.assertEqual(rel.data[harness.charm.model.unit]['key'], 'v4')
         self.assertEqual([], helper.changes)
 
+    def test_harness_leader_misconfig(self):
+        # language=YAML
+        harness = Harness(SetLeaderErrorTester, meta='''
+            name: postgresql
+            peers:
+              peer:
+                interface: foo
+            ''')
+        self.addCleanup(harness.cleanup)
+        harness.begin()
+
+        with self.assertRaises(RuntimeError) as cm:
+            harness.set_leader(is_leader=True)
+        self.assertTrue(cm.exception.args[0].find('use Harness.add_relation') != -1)
+
     def test_update_peer_relation_app_data(self):
         # language=YAML
         harness = Harness(CharmBase, meta='''