Repository: canonical/operator

Issue:
Remote app is not added to RelationMapping for relations in some cases
- Two apps, one primary, one subordinate;
- Both have two units;
- A leader subordinate writes app relation data to a container-scoped relation for all primaries to read;
- The first primary observes the leader subordinate (relation-joined) which then writes some data to the app relation data bag;
- The second primary comes up but gets a relation-changed event for the app relation data before it observes (relation-joined) its own subordinate - `relation-list` returns no units.

The reason why it fails is that we currently retrieve a remote app from a remote unit and special-case the peer relation but do not do anything for subordinates we do not yet observe.
https://github.com/canonical/operator/blob/8ef8bd7276a5bacf33d6559db7a381b24a7c8544/ops/model.py#L335-L342

It might be that Juju needs to make a sure subordinate `-joined` event fires first for a primary and only then `-changed` events are fired (created https://bugs.launchpad.net/juju/+bug/1866828).

Additional info:
```
juju status
Model    Controller           Cloud/Region         Version  SLA          Timestamp
default  localhost-localhost  localhost/localhost  2.7.4.1  unsupported  14:09:00+03:00

App           Version  Status   Scale  Charm         Store  Rev  OS      Notes
apache-httpd           error        2  apache-httpd  local    0  ubuntu  
dummy-vhost            waiting    1/2  dummy-vhost   local    0  ubuntu  

Unit              Workload  Agent       Machine  Public address  Ports  Message
apache-httpd/0*   active    idle        0        10.209.240.137         
  dummy-vhost/0*  active    idle                 10.209.240.137         
apache-httpd/1    error     idle        1        10.209.240.253         hook failed: "vhost-config-relation-changed"
  dummy-vhost/1   waiting   allocating           10.209.240.253         agent initializing
```

The unit for which everything is OK:

```
juju show-status-log apache-httpd/0
Time                        Type       Status       Message
10 Mar 2020 14:07:41+03:00  juju-unit  allocating   
10 Mar 2020 14:07:41+03:00  workload   waiting      waiting for machine
10 Mar 2020 14:08:19+03:00  workload   waiting      installing agent
10 Mar 2020 14:08:21+03:00  workload   waiting      agent initializing
10 Mar 2020 14:08:22+03:00  workload   maintenance  installing charm software
10 Mar 2020 14:08:22+03:00  juju-unit  executing    running install hook
10 Mar 2020 14:08:32+03:00  juju-unit  executing    running leader-elected hook
10 Mar 2020 14:08:33+03:00  juju-unit  executing    running config-changed hook
10 Mar 2020 14:08:33+03:00  workload   active       
10 Mar 2020 14:08:33+03:00  juju-unit  executing    running start hook
10 Mar 2020 14:08:34+03:00  juju-unit  idle         
10 Mar 2020 14:08:40+03:00  juju-unit  executing    running vhost-config-relation-joined hook
10 Mar 2020 14:08:41+03:00  juju-unit  executing    running vhost-config-relation-changed hook
10 Mar 2020 14:08:42+03:00  juju-unit  idle         
10 Mar 2020 14:08:57+03:00  juju-unit  executing    running httpd-peer-relation-joined hook
10 Mar 2020 14:08:58+03:00  juju-unit  executing    running httpd-peer-relation-changed hook
10 Mar 2020 14:08:58+03:00  juju-unit  idle         
```

The failing unit:

```juju show-status-log apache-httpd/1
Time                        Type       Status       Message
10 Mar 2020 14:08:44+03:00  workload   waiting      agent initializing
10 Mar 2020 14:08:44+03:00  workload   maintenance  installing charm software
10 Mar 2020 14:08:44+03:00  juju-unit  executing    running install hook
10 Mar 2020 14:08:56+03:00  juju-unit  executing    running leader-settings-changed hook
10 Mar 2020 14:08:56+03:00  juju-unit  executing    running config-changed hook
10 Mar 2020 14:08:56+03:00  workload   active       
10 Mar 2020 14:08:56+03:00  juju-unit  executing    running start hook
10 Mar 2020 14:08:57+03:00  juju-unit  executing    running httpd-peer-relation-joined hook
10 Mar 2020 14:08:58+03:00  juju-unit  executing    running httpd-peer-relation-changed hook
10 Mar 2020 14:08:59+03:00  juju-unit  executing    running vhost-config-relation-changed hook
10 Mar 2020 14:08:59+03:00  juju-unit  error        hook failed: "vhost-config-relation-changed"
```

```
./hooks/vhost-config-relation-changed 
> /var/lib/juju/agents/unit-apache-httpd-1/charm/hooks/vhost-config-relation-changed(212)on_vhost_config_relation_changed()
-> vhosts_serialized = event.relation.data[event.app].get('vhosts')
(Pdb) c
Traceback (most recent call last):
  File "./hooks/vhost-config-relation-changed", line 272, in <module>
    main(Charm)
  File "lib/ops/main.py", line 195, in main
    _emit_charm_event(charm, juju_event_name)
  File "lib/ops/main.py", line 120, in _emit_charm_event
    event_to_emit.emit(*args, **kwargs)
  File "lib/ops/framework.py", line 199, in emit
    framework._emit(event)
  File "lib/ops/framework.py", line 633, in _emit
    self._reemit(event_path)
  File "lib/ops/framework.py", line 668, in _reemit
    custom_handler(event)
  File "./hooks/vhost-config-relation-changed", line 212, in on_vhost_config_relation_changed
    vhosts_serialized = event.relation.data[event.app].get('vhosts')
  File "lib/ops/model.py", line 372, in __getitem__
    return self._data[key]
KeyError: <ops.model.Application
```


```
(Pdb) dict(event.relation.data)
{<ops.model.Unit apache-httpd/1>: <ops.model.RelationDataContent object at 0x7f5d38f4ab00>, <ops.model.Application apache-httpd>: <ops.model.RelationDataContent object at 0x7f5d38f4aa58>}

```

```
[1]+  Stopped                 ./hooks/vhost-config-relation-changed
root@juju-df5eba-1:/var/lib/juju/agents/unit-apache-httpd-1/charm# relation-ids
vhost-config:1
root@juju-df5eba-1:/var/lib/juju/agents/unit-apache-httpd-1/charm# relation-list
root@juju-df5eba-1:/var/lib/juju/agents/unit-apache-httpd-1/charm# relation-list ; echo $?
0
root@juju-df5eba-1:/var/lib/juju/agents/unit-apache-httpd-1/charm# env | grep JUJU_REMOTE
JUJU_REMOTE_UNIT=
JUJU_REMOTE_APP=dummy-vhost
root@juju-df5eba-1:/var/lib/juju/agents/unit-apache-httpd-1/charm# relation-get --app
vhosts: '- {port: "80", template: PFZpcnR1YWxIb3N0ICo6ODA+CglTZXJ2ZXJBZG1pbiB3ZWJtYXN0ZXJAbG9jYWxob3N0CglEb2N1bWVudFJvb3QgL3Zhci93d3cvZHVtbXktdmhvc3QKCUVycm9yTG9nICR7QVBBQ0hFX0xPR19ESVJ9L2Vycm9yLmxvZwoJQ3VzdG9tTG9nICR7QVBBQ0hFX0xPR19ESVJ9L2FjY2Vzcy5sb2cgY29tYmluZWQKPC9WaXJ0dWFsSG9zdD4K}'
```

```
cat metadata.yaml 
name: apache-httpd
# ... 
requires:
    vhost-config:
        interface: apache-vhost-config
        scope: container
```

Discussion:
benhoyt: @jameinel Ian pointed out that `relation-list --app` might exist already, and sure enough, it does! I've updated the code/tests to use that.

Patch:
diff --git a/ops/model.py b/ops/model.py
index 9efec7d..d03a387 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -652,18 +652,28 @@ class Relation:
         self.app = None
         self.units = set()
 
-        # For peer relations, both the remote and the local app are the same.
         if is_peer:
+            # For peer relations, both the remote and the local app are the same.
             self.app = our_unit.app
+
         try:
             for unit_name in backend.relation_list(self.id):
                 unit = cache.get(Unit, unit_name)
                 self.units.add(unit)
                 if self.app is None:
+                    # Use the app of one of the units if available.
                     self.app = unit.app
         except RelationNotFoundError:
             # If the relation is dead, just treat it as if it has no remote units.
             pass
+
+        # 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 self.app is None:
+            app_name = backend.relation_remote_app_name(relation_id)
+            if app_name is not None:
+                self.app = cache.get(Application, app_name)
+
         self.data = RelationData(self, our_unit, backend)
 
     def __repr__(self):
@@ -1151,6 +1161,10 @@ class _ModelBackend:
                 else:
                     return text
 
+    @staticmethod
+    def _is_relation_not_found(model_error):
+        return 'relation not found' in str(model_error)
+
     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]
@@ -1160,10 +1174,32 @@ class _ModelBackend:
             return self._run('relation-list', '-r', str(relation_id),
                              return_output=True, use_json=True)
         except ModelError as e:
-            if 'relation not found' in str(e):
+            if self._is_relation_not_found(e):
                 raise RelationNotFoundError() from e
             raise
 
+    def relation_remote_app_name(self, relation_id: int) -> typing.Optional[str]:
+        """Return remote app name for given relation ID, or None if not known."""
+        if 'JUJU_RELATION_ID' in os.environ and 'JUJU_REMOTE_APP' in os.environ:
+            event_relation_id = int(os.environ['JUJU_RELATION_ID'].split(':')[-1])
+            if relation_id == event_relation_id:
+                # JUJU_RELATION_ID is this relation, use JUJU_REMOTE_APP.
+                return os.environ['JUJU_REMOTE_APP']
+
+        # If caller is asking for information about another relation, use
+        # "relation-list --app" to get it.
+        try:
+            return self._run('relation-list', '-r', str(relation_id), '--app',
+                             return_output=True, use_json=True)
+        except ModelError as e:
+            if self._is_relation_not_found(e):
+                return None
+            if 'option provided but not defined: --app' in str(e):
+                # "--app" was introduced to relation-list in Juju 2.8.1, so
+                # handle previous verions of Juju gracefully
+                return None
+            raise
+
     def relation_get(self, relation_id, member_name, is_app):
         if not isinstance(is_app, bool):
             raise TypeError('is_app parameter to relation_get must be a boolean')
@@ -1181,7 +1217,7 @@ class _ModelBackend:
         try:
             return self._run(*args, return_output=True, use_json=True)
         except ModelError as e:
-            if 'relation not found' in str(e):
+            if self._is_relation_not_found(e):
                 raise RelationNotFoundError() from e
             raise
 
@@ -1202,7 +1238,7 @@ class _ModelBackend:
         try:
             return self._run(*args)
         except ModelError as e:
-            if 'relation not found' in str(e):
+            if self._is_relation_not_found(e):
                 raise RelationNotFoundError() from e
             raise
 
@@ -1337,7 +1373,7 @@ class _ModelBackend:
         try:
             return self._run(*cmd, return_output=True, use_json=True)
         except ModelError as e:
-            if 'relation not found' in str(e):
+            if self._is_relation_not_found(e):
                 raise RelationNotFoundError() from e
             raise
 


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index 0bce767..d7e6c2f 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -736,6 +736,12 @@ class _TestingModelBackend:
         except KeyError as e:
             raise model.RelationNotFoundError from e
 
+    def relation_remote_app_name(self, relation_id: int) -> typing.Optional[str]:
+        if relation_id not in self._relation_app_and_units:
+            # Non-existent or dead relation
+            return None
+        return self._relation_app_and_units[relation_id]['app']
+
     def relation_get(self, relation_id, member_name, is_app):
         if is_app and '/' in member_name:
             member_name = member_name.split('/')[0]
diff --git a/test/test_model.py b/test/test_model.py
index 6c2bde5..afe18d6 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -120,6 +120,7 @@ class TestModel(unittest.TestCase):
         self.assertEqual(dead_rel.data[self.model.unit], {})
         self.assertBackendCalls([
             ('relation_list', 7),
+            ('relation_remote_app_name', 7),
             ('relation_get', 7, 'myapp/0', False),
         ])
 
@@ -134,7 +135,9 @@ class TestModel(unittest.TestCase):
         self.assertBackendCalls([
             ('relation_ids', 'db0'),
             ('relation_list', self.relation_id_db0),
+            ('relation_remote_app_name', 0),
             ('relation_list', relation_id_db0_b),
+            ('relation_remote_app_name', 2),
         ])
 
     def test_peer_relation_app(self):
@@ -403,6 +406,17 @@ class TestModel(unittest.TestCase):
             ('relation_get', relation_id, 'myapp/0', False),
         ])
 
+    def test_relation_no_units(self):
+        self.harness.add_relation('db1', 'remoteapp1')
+        rel = self.model.get_relation('db1')
+        self.assertEqual(rel.units, set())
+        self.assertIs(rel.app, self.model.get_app('remoteapp1'))
+        self.assertBackendCalls([
+            ('relation_ids', 'db1'),
+            ('relation_list', 1),
+            ('relation_remote_app_name', 1),
+        ])
+
     def test_config(self):
         self.harness.update_config({'foo': 'foo', 'bar': 1, 'qux': True})
         self.assertEqual(self.model.config, {
@@ -1567,6 +1581,61 @@ class TestModelBackend(unittest.TestCase):
             with self.assertRaises(ops.model.ModelError):
                 self.backend.add_metrics(metrics, labels)
 
+    def test_relation_remote_app_name_env(self):
+        self.addCleanup(os.environ.pop, 'JUJU_RELATION_ID', None)
+        self.addCleanup(os.environ.pop, 'JUJU_REMOTE_APP', None)
+
+        os.environ['JUJU_RELATION_ID'] = 'x:5'
+        os.environ['JUJU_REMOTE_APP'] = 'remoteapp1'
+        self.assertEqual(self.backend.relation_remote_app_name(5), 'remoteapp1')
+        os.environ['JUJU_RELATION_ID'] = '5'
+        self.assertEqual(self.backend.relation_remote_app_name(5), 'remoteapp1')
+
+    def test_relation_remote_app_name_script_success(self):
+        self.addCleanup(os.environ.pop, 'JUJU_RELATION_ID', None)
+        self.addCleanup(os.environ.pop, 'JUJU_REMOTE_APP', None)
+
+        # JUJU_RELATION_ID and JUJU_REMOTE_APP both unset
+        fake_script(self, 'relation-list', r"""
+echo '"remoteapp2"'
+""")
+        self.assertEqual(self.backend.relation_remote_app_name(1), 'remoteapp2')
+        self.assertEqual(fake_script_calls(self, clear=True), [
+            ['relation-list', '-r', '1', '--app', '--format=json'],
+        ])
+
+        # JUJU_RELATION_ID set but JUJU_REMOTE_APP unset
+        os.environ['JUJU_RELATION_ID'] = 'x:5'
+        self.assertEqual(self.backend.relation_remote_app_name(5), 'remoteapp2')
+
+        # JUJU_RELATION_ID unset but JUJU_REMOTE_APP set
+        del os.environ['JUJU_RELATION_ID']
+        os.environ['JUJU_REMOTE_APP'] = 'remoteapp1'
+        self.assertEqual(self.backend.relation_remote_app_name(5), 'remoteapp2')
+
+        # Both set, but JUJU_RELATION_ID a different relation
+        os.environ['JUJU_RELATION_ID'] = 'x:6'
+        self.assertEqual(self.backend.relation_remote_app_name(5), 'remoteapp2')
+
+    def test_relation_remote_app_name_script_errors(self):
+        fake_script(self, 'relation-list', r"""
+echo "ERROR invalid value \"6\" for option -r: relation not found" >&2  # NOQA
+exit 2
+""")
+        self.assertIs(self.backend.relation_remote_app_name(6), None)
+        self.assertEqual(fake_script_calls(self, clear=True), [
+            ['relation-list', '-r', '6', '--app', '--format=json'],
+        ])
+
+        fake_script(self, 'relation-list', r"""
+echo "ERROR option provided but not defined: --app" >&2
+exit 2
+""")
+        self.assertIs(self.backend.relation_remote_app_name(6), None)
+        self.assertEqual(fake_script_calls(self, clear=True), [
+            ['relation-list', '-r', '6', '--app', '--format=json'],
+        ])
+
 
 class TestLazyMapping(unittest.TestCase):
 
diff --git a/test/test_testing.py b/test/test_testing.py
index 119cbed..be87ed7 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -763,6 +763,7 @@ class TestHarness(unittest.TestCase):
             harness._get_backend_calls(reset=True), [
                 ('relation_ids', 'db'),
                 ('relation_list', rel_id),
+                ('relation_remote_app_name', 0),
             ])
         # add_relation_unit resets the relation_list, but doesn't trigger backend calls
         harness.add_relation_unit(rel_id, 'postgresql/0')
@@ -1591,3 +1592,20 @@ class TestTestingModelBackend(unittest.TestCase):
         self.assertIn(
             "units/unit-test-app-0/resources/foo: resource#test-app/foo not found",
             str(cm.exception))
+
+    def test_relation_remote_app_name(self):
+        harness = Harness(CharmBase, meta='''
+            name: test-charm
+            ''')
+        self.addCleanup(harness.cleanup)
+        backend = harness._backend
+
+        self.assertIs(backend.relation_remote_app_name(1), None)
+
+        rel_id = harness.add_relation('db', 'postgresql')
+        self.assertEqual(backend.relation_remote_app_name(rel_id), 'postgresql')
+        harness.add_relation_unit(rel_id, 'postgresql/0')
+        harness.add_relation_unit(rel_id, 'postgresql/1')
+        self.assertEqual(backend.relation_remote_app_name(rel_id), 'postgresql')
+
+        self.assertIs(backend.relation_remote_app_name(7), None)