Repository: juju/python-libjuju

Issue:
`unit.is_leader_from_status` reports False for all units of a subordinate charm
**Context**
python-libjuju v2.6.3
juju --version => 2.6.10-bionic-amd64 (snap)

This affects the migration of the functional test of openstack charms to the zaza testing framework, as some of them are deployed as subordinates (neutron-openvswitch and others in https://bugs.launchpad.net/charm-aodh/+bug/1828424)

**How to reproduce:**

Start with a deployed bundle including subordinate charms (e.g. neutron-openvswitch below). `juju status` reports a leader: neutron-openvswitch/0*

![image](https://user-images.githubusercontent.com/1916322/70766853-45770180-1db3-11ea-9fc3-6023bd0c38c2.png)

However `unit.is_leader_from_status` (https://github.com/juju/python-libjuju/blob/master/juju/unit.py#L246) reports False for all units. There is a comment in the code about returning False for subordinates on purpose. However as shown above, `juju status` does report a leader. `juju run --unit neutron-openvswitch/leader <cmd>` does work as expected as well.

Code to reproduce the issue:

```python
from juju import loop
from juju.model import Model

async def subordinate_has_no_leader():
    model = Model()
    await model.connect()

    app_name = 'neutron-openvswitch'
    app_ovs = model.applications[app_name]
    print("app: {}, units: {}".format(app_name, app_ovs.units))

    for unit in app_ovs.units:
        print("unit: {}, leader?: {}".format(unit.entity_id,
            await unit.is_leader_from_status()))

    await model.disconnect()

if __name__ == "__main__":
    loop.run(subordinate_has_no_leader())
```

which outputs the following for the model above (works fine for a principle charm):

```
$ python reproduce.py
app: neutron-openvswitch, units: [<Unit entity_id="neutron-openvswitch/1">, <Unit entity_id="neutron-openvswitch/0">]
unit: neutron-openvswitch/1, leader?: False
unit: neutron-openvswitch/0, leader?: False
```

Discussion:
tlm: ![](https://github.trello.services/images/mini-trello-icon.png) [python lib bug with is unit leader](https://trello.com/c/XGtikBMe/1008-python-lib-bug-with-is-unit-leader)
tlm: @hpidcock take a look. Haven't handled the case of being a subordinate more than once. Is this something we need to cover ?
hpidcock: $$merge$$
hpidcock: @SimonRichardson did you figure out why the merge didn't work last time?

Patch:
diff --git a/juju/unit.py b/juju/unit.py
index 6cbde8d..9d8e27f 100644
--- a/juju/unit.py
+++ b/juju/unit.py
@@ -256,12 +256,13 @@ class Unit(model.ModelEntity):
         method.
 
         """
-        app = self.name.split("/")[0]
+        unit_parts = self.name.split("/")
+        app = unit_parts[0]
+        unit_index = unit_parts[1]
 
-        c = client.ClientFacade.from_connection(self.connection)
-
-        status = await c.FullStatus(patterns=None)
+        client_facade = client.ClientFacade.from_connection(self.connection)
 
+        status = await client_facade.FullStatus(patterns=None)
         # FullStatus may be more up to date than our model, and the
         # unit may have gone away, or we may be doing something silly,
         # like trying to fetch leadership for a subordinate, which
@@ -271,14 +272,30 @@ class Unit(model.ModelEntity):
         if not status.applications.get(app):
             return False
 
-        if not status.applications[app].get('units'):
-            return False
+        # We will attempt to look in two places for a leader property based on
+        # if the unit is subordinate or not. These variables allow for more
+        # generic non discriminate checks
+        app_data = status.applications.get(app)
+        target_unit = self.name
+
+        # Is the application a subordinate? If so change our data variables to
+        # the parent
+        if status.applications[app].get('subordinate-to'):
+            parent = status.applications[app]['subordinate-to'][0]
+            app_data = status.applications[parent]
+            target_unit = parent + "/" + unit_index
 
-        if not status.applications[app]['units'].get(self.name):
+        if not app_data.get('units') or not app_data['units'].get(target_unit):
             return False
 
-        return status.applications[app]['units'][self.name].get('leader',
-                                                                False)
+        unit = app_data['units'][target_unit]
+        if target_unit == self.name:
+            return unit.get('leader', False)
+
+        if unit.get('subordinates') and unit['subordinates'].get(self.name):
+            return unit['subordinates'][self.name].get('leader', False)
+
+        return False
 
     async def get_metrics(self):
         """Get metrics for the unit.


Test patch:
diff --git a/tests/unit/test_unit.py b/tests/unit/test_unit.py
new file mode 100644
index 0000000..9029a01
--- /dev/null
+++ b/tests/unit/test_unit.py
@@ -0,0 +1,83 @@
+import asynctest
+import mock
+import pytest
+
+from juju.model import Model
+from juju.unit import Unit
+
+from .. import base
+
+
+@asynctest.patch('juju.client.client.ClientFacade')
+@pytest.mark.asyncio
+async def test_unit_is_leader(mock_cf):
+    tests = [
+        {
+            'applications': {
+                'test': {
+                    'units': {
+                        'test/0': {
+                            'subordinates': {
+                                'test-sub/0': {
+                                    'leader': True
+                                }
+                            }
+                        }
+                    }
+                },
+                'test-sub': {
+                    'subordinate-to': [
+                        'test'
+                    ]
+                }
+            },
+            'description': "Tests that subordinate units report is leader correctly",
+            'unit': 'test-sub/0',
+            'rval': True
+        },
+        {
+            'applications': {
+                'test': {
+                    'units': {
+                        'test/0': {
+                            'leader': True
+                        }
+                    }
+                }
+            },
+            'description': "Tests that unit reports is leader correctly",
+            'unit': 'test/0',
+            'rval': True
+        },
+        {
+            'applications': {},
+            'description': "Tests that non existent apps return False as leader",
+            'unit': 'test/0',
+            'rval': False
+        },
+        {
+            'applications': {
+                'test': {
+                    'units': {}
+                }
+            },
+            'description': "Tests that apps with no units report False as leader",
+            'unit': 'test/0',
+            'rval': False
+        },
+    ]
+
+    model = Model()
+    model._connector = mock.MagicMock()
+
+    for test in tests:
+        status = mock.Mock()
+        status.applications = test['applications']
+        client_facade = mock_cf.from_connection()
+        client_facade.FullStatus = base.AsyncMock(return_value=status)
+
+        unit = Unit("test", model)
+        unit.name = test['unit']
+
+        rval = await unit.is_leader_from_status()
+        assert rval == test['rval']