Merge lp:~johnsca/charm-helpers/multi-unit into lp:~cf-charmers/charm-helpers/cloud-foundry

[Cory Johns]

Refactored RelationContext multi-unit support

Added a little magic to the RelationContext to make it better able to support
multiple units and still DWIM in both cases. This might be too magical,
though.

https://codereview.appspot.com/96680043/

[Cory Johns (johnsca)]

Reviewers: mp+221801_code.launchpad.net,

Message:
Please take a look.

Description:
Refactored RelationContext multi-unit support

Added a little magic to the RelationContext to make it better able to
support
multiple units and still DWIM in both cases. This might be too magical,
though.

https://code.launchpad.net/~johnsca/charm-helpers/multi-unit/+merge/221801

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/96680043/

Affected files (+105, -28 lines):
   A [revision details]
   M charmhelpers/contrib/cloudfoundry/contexts.py
   M charmhelpers/core/services.py
   M tests/contrib/cloudfoundry/test_render_context.py
   M tests/core/test_services.py

[lp:~johnsca/charm-helpers/multi-unit]

186. By Cory Johns on 2014-06-03

Fixed issue with default unit in services framework and improved docs

[Cory Johns (johnsca)]

Please take a look.

https://codereview.appspot.com/96680043/

[Benjamin Saller (bcsaller)]

Thanks for this. I include some thoughts below, I hope they are
agreeable.

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services.py
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services.py#newcode239
charmhelpers/core/services.py:239:
I make suggestions below on altering this process. Let me know what you
think.

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services.py#newcode298
charmhelpers/core/services.py:298: over the units, not the items of the
default unit.
There is a rule I try to use when generating/process data these days, no
data should live in the key side of the mapping. In the context of a
template for iteration is simpler to iterate

context[interface][{_unit: 'unit/0', 'foo': 'bar'},
                    {_unit: 'unit/1', 'foo': bar}]

This rule makes additional sense when thinking that knowing the unit
names to request them would imply a broken charm and thus shouldn't be
an index in this context. I'd suggest transitioning to that. We could
also make the rule (and document here) that only elements with complete
data appear in the list.

https://codereview.appspot.com/96680043/

[Cory Johns (johnsca)]

On 2014/06/04 20:57:39, benjamin.saller wrote:

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services.py#newcode298
> charmhelpers/core/services.py:298: over the units, not the items of
the default
> unit.
> There is a rule I try to use when generating/process data these days,
no data
> should live in the key side of the mapping. In the context of a
template for
> iteration is simpler to iterate

> context[interface][{_unit: 'unit/0', 'foo': 'bar'},
> {_unit: 'unit/1', 'foo': bar}]

> This rule makes additional sense when thinking that knowing the unit
names to
> request them would imply a broken charm and thus shouldn't be an index
in this
> context. I'd suggest transitioning to that. We could also make the
rule (and
> document here) that only elements with complete data appear in the
list.

Can you explain your reasoning behind making use of the key? I'm not
averse to changing it to being a list instead of a mapping, but using a
"special" field to hold the unit name ('_unit', in your example) seems
more dangerous, since it could potentially conflict with actual fields,
even if unlikely due to the underscore prefix.

With your comment that the charm shouldn't actually know / care about
the unit name, maybe we can just leave it out altogether, especially if
it only contains "complete" data sets.

https://codereview.appspot.com/96680043/

[Cory Johns (johnsca)]

That should be: "behind *not* making use of the key"

https://codereview.appspot.com/96680043/

[Benjamin Saller (bcsaller)]

On 2014/06/04 21:09:21, johnsca wrote:
> On 2014/06/04 20:57:39, benjamin.saller wrote:
> >

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services.py#newcode298
> > charmhelpers/core/services.py:298: over the units, not the items of
the
> default
> > unit.
> > There is a rule I try to use when generating/process data these
days, no data
> > should live in the key side of the mapping. In the context of a
template for
> > iteration is simpler to iterate
> >
> > context[interface][{_unit: 'unit/0', 'foo': 'bar'},
> > {_unit: 'unit/1', 'foo': bar}]
> >
> > This rule makes additional sense when thinking that knowing the unit
names to
> > request them would imply a broken charm and thus shouldn't be an
index in this
> > context. I'd suggest transitioning to that. We could also make the
rule (and
> > document here) that only elements with complete data appear in the
list.

> Can you explain your reasoning behind making use of the key? I'm not
averse to
> changing it to being a list instead of a mapping, but using a
"special" field to
> hold the unit name ('_unit', in your example) seems more dangerous,
since it
> could potentially conflict with actual fields, even if unlikely due to
the
> underscore prefix.

> With your comment that the charm shouldn't actually know / care about
the unit
> name, maybe we can just leave it out altogether, especially if it only
contains
> "complete" data sets.

I think for our use case we don't actually need the unitname at all and
could omit it, in the more general case I expect that there are cases
where people will want to delta the relation data vs previous hook calls
(cached data) before taking some actions.

The list vs dict thing is a pattern that I picked up working in the
machine learning space. It tends to be easier to consume and pivot flat
tuple style data. I was looking for a nice right up justifying this
pattern, its even known as "<someones> law" but I can't remember that
name :-/

However if the value in the key side of the mapping can't be known/used
its a good indicator to me that we can flatten the structure. Maybe not
very persuasive.

https://codereview.appspot.com/96680043/

[lp:~johnsca/charm-helpers/multi-unit]

187. By Cory Johns on 2014-06-05

Refactored RelationContext to be a bit more intuitive

[Cory Johns (johnsca)]

Please take a look.

https://codereview.appspot.com/96680043/

[lp:~johnsca/charm-helpers/multi-unit]

188. By Cory Johns on 2014-06-05

Added mapping iteration methods to RelationContext helper

[Cory Johns (johnsca)]

Please take a look.

https://codereview.appspot.com/96680043/

[lp:~johnsca/charm-helpers/multi-unit]

189. By Cory Johns on 2014-06-05

Added ports for log relations and cleaned up other relation keys

[Cory Johns (johnsca)]

Please take a look.

https://codereview.appspot.com/96680043/

[Benjamin Saller (bcsaller)]

Thanks for this, very close. Mostly this comes down to me wanting
another pass over the examples/docs around the multi-unit data access.

Hopefully I pointed to what I am after clearly, if not please let me
know.

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloudfoundry/contexts.py
File charmhelpers/contrib/cloudfoundry/contexts.py (right):

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloudfoundry/contexts.py#newcode36
charmhelpers/contrib/cloudfoundry/contexts.py:36: required_keys =
['address', 'port', 'user', 'password']
You have a follow on for the charms updating the templates as well?

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloudfoundry/contexts.py#newcode69
charmhelpers/contrib/cloudfoundry/contexts.py:69: required_keys =
['hostname', 'port']
I am a little cautious about this one as this isn't really a CF
component, it would be interesting if down the road we had something
like

EtcdInterface = charmhelpers.interfaces.Interface('etcd')

To produce this class from a interface registry. We'll want that sort of
thing for the interface testing plans anyway.

just rambling, nothing to do now.

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/core/services.py
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/core/services.py#newcode210
charmhelpers/core/services.py:210: """
This feels a bit magical. I'm fine with letting it in but the 'why'
you'd use it both ways should be explained a little more below. See my
following comment about the example being a bit unclear.

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/core/services.py#newcode308
charmhelpers/core/services.py:308: from unit 'wordpress/0'.
I worry that this example isn't clear. self.interface and the 'foo' key
are assumed.

I'd like two examples here in the style of

'if you need to iterate all the units of a relation, for example in a
template, you can ...'

'if you want the keys and values of any matching unit containing all the
expected keys of an interface, you can ...'

sorry to ask you to rephrase this, and thank you.

https://codereview.appspot.com/96680043/

[Cory Johns (johnsca)]

On 2014/06/06 16:43:12, benjamin.saller wrote:

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloudfoundry/contexts.py#newcode36
> charmhelpers/contrib/cloudfoundry/contexts.py:36: required_keys =
['address',
> 'port', 'user', 'password']
> You have a follow on for the charms updating the templates as well?

Yes, as part of the port conflict resolution. Just testing and hashing
out bugs currently.

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloudfoundry/contexts.py#newcode69
> charmhelpers/contrib/cloudfoundry/contexts.py:69: required_keys =
['hostname',
> 'port']
> I am a little cautious about this one as this isn't really a CF
component, it
> would be interesting if down the road we had something like

> EtcdInterface = charmhelpers.interfaces.Interface('etcd')

> To produce this class from a interface registry. We'll want that sort
of thing
> for the interface testing plans anyway.

I had considered having RelationContext.__init__ accept the interface
name and required keys as parameters so that you don't have to create
stub subclasses, and only need to subclass if you want to add extra
behavior (e.g., the MysqlRelation). I think this is the right way to
go, and will do so.

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/core/services.py#newcode210
> charmhelpers/core/services.py:210: """
> This feels a bit magical. I'm fine with letting it in but the 'why'
you'd use it
> both ways should be explained a little more below. See my following
comment
> about the example being a bit unclear.

I agree. I am very on the fence as to whether to stick with the
"magical" implementation vs creating separate classes, or even just
forcing it to always be a list to force charm authors to always consider
the possibility that there may be more units.

The reason I didn't go with separate classes is because I didn't want to
have to duplicate all the stub RelationContext subclasses, but if we
make it so the interface and keys can be passed in, we can avoid
creating 95% of the stub classes, so that'd be ok.

But now I'm thinking that forcing it to be a list might be better, since
charm authors really should keep in mind that the admin could throw more
units at the relation.

https://codereview.appspot.com/96680043/

[lp:~johnsca/charm-helpers/multi-unit]

190. By Cory Johns on 2014-06-06

Fixed Cloud Foundry MysqlRelation for new RelationContext data format

191. By Cory Johns on 2014-06-06

Removed magical default unit behavior from RelationContext

[Cory Johns (johnsca)]

Please take a look.

https://codereview.appspot.com/96680043/

[Benjamin Saller (bcsaller)]

Thanks, notes follow.

https://codereview.appspot.com/96680043/diff/100001/charmhelpers/core/services.py
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/96680043/diff/100001/charmhelpers/core/services.py#newcode259
charmhelpers/core/services.py:259: order: 'wordpress/0', 'wordpress/1',
'mediawiki/0'.
I think you do a fine job of explaining this now, but the use-case for
it is still unclear to me.

If there is more than one relation with the same interface I don't think
we can silently combine those lists (the data is coming from different
relation ids). They represent different services and should expect
different orchestration.

I'd go so far as to say this is a whole in our model.

In CF mysql has relations to UAA and CC but doesn't use the services
framework to manage this. If it did and iterated those units uniformly
havoc could follow, no?

The default case isn't this and we built for it, but unless my thinking
here is muddy, we'd need to do something different here. Currently the
relation id information would be lost. We'd at minimum have to put the
rid in the data, or break the units into collections by relation.

Breaking them up by relation is to my mind similar to always processing
this as a list (rather than the default unit notion of before), its the
more complex case, but its one that some services need to be aware of.

I apologize for not picking up on this sooner. If you'd like to talk
about this in the hangout let me know.

https://codereview.appspot.com/96680043/diff/100001/charmhelpers/core/services.py#newcode268
charmhelpers/core/services.py:268: {%- endfor %}
This is good, thanks, it might have to turn into

interface[0][0]['key']

for interface[first relation][first unit][key]

lets talk.

https://codereview.appspot.com/96680043/

[lp:~johnsca/charm-helpers/multi-unit]

192. By Cory Johns on 2014-06-06

Added doc note about not (currently) preserving relation-id and unit-id info in RelationContext

[Cory Johns (johnsca)]

Please take a look.

https://codereview.appspot.com/96680043/

[Benjamin Saller (bcsaller)]

+1 LTGM, thanks for talking through that issue.

https://codereview.appspot.com/96680043/diff/120001/charmhelpers/core/services.py
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/96680043/diff/120001/charmhelpers/core/services.py#newcode272
charmhelpers/core/services.py:272: set of data came from, you'll need to
extend this class to preserve
Great, thanks

https://codereview.appspot.com/96680043/

=== modified file 'charmhelpers/contrib/cloudfoundry/contexts.py'
--- charmhelpers/contrib/cloudfoundry/contexts.py	2014-05-26 18:49:24 +0000
+++ charmhelpers/contrib/cloudfoundry/contexts.py	2014-06-06 19:43:53 +0000
@@ -33,7 +33,7 @@
 
 class NatsRelation(RelationContext):
     interface = 'nats'
-    required_keys = ['nats_port', 'nats_address', 'nats_user', 'nats_password']
+    required_keys = ['address', 'port', 'user', 'password']
 
 
 class MysqlRelation(RelationContext):
@@ -44,9 +44,10 @@
     def get_data(self):
         RelationContext.get_data(self)
         if self.is_ready():
-            if 'port' not in self['db']:
-                self['db']['port'] = '3306'
-            self['db']['dsn'] = self.dsn_template.format(**self['db'])
+            for unit in self['db']:
+                if 'port' not in unit:
+                    unit['port'] = '3306'
+                unit['dsn'] = self.dsn_template.format(**unit)
 
 
 class RouterRelation(RelationContext):
@@ -56,9 +57,19 @@
 
 class LogRouterRelation(RelationContext):
     interface = 'logrouter'
-    required_keys = ['shared-secret', 'logrouter-address']
+    required_keys = ['shared_secret', 'address', 'incoming_port', 'outgoing_port']
 
 
 class LoggregatorRelation(RelationContext):
     interface = 'loggregator'
-    required_keys = ['shared_secret', 'loggregator_address']
+    required_keys = ['shared_secret', 'address', 'incoming_port', 'outgoing_port']
+
+
+class EtcdRelation(RelationContext):
+    interface = 'etcd'
+    required_keys = ['hostname', 'port']
+
+
+class CloudControllerRelation(RelationContext):
+    interface = 'cc'
+    required_keys = ['hostname', 'port', 'user', 'password']

=== modified file 'charmhelpers/core/services.py'
--- charmhelpers/core/services.py	2014-05-29 17:23:48 +0000
+++ charmhelpers/core/services.py	2014-06-06 19:43:53 +0000
@@ -11,6 +11,14 @@
         """
         Register a list of services, given their definitions.
 
+        Traditional charm authoring is focused on implementing hooks.  That is,
+        the charm author is thinking in terms of "What hook am I handling; what
+        does this hook need to do?"  However, in most cases, the real question
+        should be "Do I have the information I need to configure and start this
+        piece of software and, if so, what are the steps for doing so."  The
+        ServiceManager framework tries to bring the focus to the data and the
+        setup tasks, in the most declarative way possible.
+
         Service definitions are dicts in the following formats (all keys except
         'service' are optional):
 
@@ -67,28 +75,28 @@
         a mongodb relation and which runs a custom `db_migrate` function prior to
         restarting the service, and a Runit serivce called spadesd.
 
-            >>> manager = services.ServiceManager([
-            ...     {
-            ...         'service': 'bingod',
-            ...         'ports': [80, 443],
-            ...         'required_data': [MongoRelation(), config()],
-            ...         'data_ready': [
-            ...             services.template(source='bingod.conf'),
-            ...             services.template(source='bingod.ini',
-            ...                               target='/etc/bingod.ini',
-            ...                               owner='bingo', perms=0400),
-            ...         ],
-            ...     },
-            ...     {
-            ...         'service': 'spadesd',
-            ...         'data_ready': services.template(source='spadesd_run.j2',
-            ...                                         target='/etc/sv/spadesd/run',
-            ...                                         perms=0555),
-            ...         'start': runit_start,
-            ...         'stop': runit_stop,
-            ...     },
-            ... ])
-            ... manager.manage()
+            manager = services.ServiceManager([
+                {
+                    'service': 'bingod',
+                    'ports': [80, 443],
+                    'required_data': [MongoRelation(), config(), {'my': 'data'}],
+                    'data_ready': [
+                        services.template(source='bingod.conf'),
+                        services.template(source='bingod.ini',
+                                          target='/etc/bingod.ini',
+                                          owner='bingo', perms=0400),
+                    ],
+                },
+                {
+                    'service': 'spadesd',
+                    'data_ready': services.template(source='spadesd_run.j2',
+                                                    target='/etc/sv/spadesd/run',
+                                                    perms=0555),
+                    'start': runit_start,
+                    'stop': runit_stop,
+                },
+            ])
+            manager.manage()
         """
         self.services = {}
         for service in services or []:
@@ -222,46 +230,57 @@
 
     __nonzero__ = __bool__
 
+    def __repr__(self):
+        return super(RelationContext, self).__repr__()
+
     def is_ready(self):
         """
-        Returns True if all of the required_keys are available.
-        """
-        return set(self.get(self.interface, {}).keys()).issuperset(set(self.required_keys))
+        Returns True if all of the `required_keys` are available from any units.
+        """
+        return len(self.get(self.interface, [])) > 0
+
+    def _is_ready(self, unit_data):
+        """
+        Helper method that tests a set of relation data and returns True if
+        all of the `required_keys` are present.
+        """
+        return set(unit_data.keys()).issuperset(set(self.required_keys))
 
     def get_data(self):
         """
-        Retrieve the relation data and store it under `self[self.interface]`.
-
-        If there are more than one units related on the desired interface,
-        then each unit will have its data stored under `self[self.interface][unit_id]`
-        and one of the units with complete information will chosen at random
-        to fill the values at `self[self.interface]`.
-
-
-        For example:
-
-            {
-                'foo': 'bar',
-                'unit/0': {
-                    'foo': 'bar',
-                },
-                'unit/1': {
-                    'foo': 'baz',
-                },
-            }
+        Retrieve the relation data for each unit involved in a realtion and,
+        if complete, store it in a list under `self[self.interface]`.
+
+        The units are sorted lexographically first by the service ID, then by
+        the unit ID.  Thus, if an interface has two other services, 'db:1'
+        and 'db:2', with 'db:1' having two units, 'wordpress/0' and 'wordpress/1',
+        and 'db:2' having one unit, 'mediawiki/0', all of which have a complete
+        set of data, the relation data for the units will be stored in the
+        order: 'wordpress/0', 'wordpress/1', 'mediawiki/0'.
+
+        If you only care about a single unit on the relation, you can just
+        access it as `{{ interface[0]['key'] }}`.  However, if you can at all
+        support multiple units on a relation, you should iterate over the list,
+        like:
+
+            {% for unit in interface -%}
+                {{ unit['key'] }}{% if not loop.last %},{% endif %}
+            {%- endfor %}
+
+        Note that since all sets of relation data from all related services and
+        units are in a single list, if you need to know which service or unit a
+        set of data came from, you'll need to extend this class to preserve
+        that information.
         """
         if not hookenv.relation_ids(self.interface):
             return
 
-        ns = self.setdefault(self.interface, {})
-        required = set(self.required_keys)
-        for rid in hookenv.relation_ids(self.interface):
-            for unit in hookenv.related_units(rid):
+        ns = self.setdefault(self.interface, [])
+        for rid in sorted(hookenv.relation_ids(self.interface)):
+            for unit in sorted(hookenv.related_units(rid)):
                 reldata = hookenv.relation_get(rid=rid, unit=unit)
-                unit_ns = ns.setdefault(unit, {})
-                unit_ns.update(reldata)
-                if set(reldata.keys()).issuperset(required):
-                    ns.update(reldata)
+                if self._is_ready(reldata):
+                    ns.append(reldata)
 
 
 class ManagerCallback(object):
@@ -316,6 +335,6 @@
 
 
 # Convenience aliases
-template = TemplateCallback
+render_template = template = TemplateCallback
 open_ports = PortManagerCallback()
 close_ports = PortManagerCallback()

=== modified file 'tests/contrib/cloudfoundry/test_render_context.py'
--- tests/contrib/cloudfoundry/test_render_context.py	2014-05-26 18:49:24 +0000
+++ tests/contrib/cloudfoundry/test_render_context.py	2014-06-06 19:43:53 +0000
@@ -18,23 +18,22 @@
     @mock.patch('charmhelpers.core.hookenv.relation_get')
     def test_nats_relation_populated(self, mrel, mid, mrelated):
         mid.return_value = ['nats']
-        mrel.return_value = {'nats_port': 1234, 'nats_address': 'host',
-                             'nats_user': 'user', 'nats_password': 'password'}
+        mrel.return_value = {'port': 1234, 'address': 'host',
+                             'user': 'user', 'password': 'password'}
         mrelated.return_value = ['router/0']
         n = contexts.NatsRelation()
-        expected = {'nats': {'nats_port': 1234, 'nats_address': 'host',
-                             'nats_user': 'user', 'nats_password': 'password',
-                             'router/0': {'nats_port': 1234, 'nats_address': 'host',
-                                          'nats_user': 'user', 'nats_password': 'password'}}}
+        expected = {'nats': [{'port': 1234, 'address': 'host',
+                              'user': 'user', 'password': 'password'}]}
         self.assertTrue(bool(n))
         self.assertEqual(n, expected)
+        self.assertEqual(n['nats'][0]['port'], 1234)
 
     @mock.patch('charmhelpers.core.hookenv.related_units')
     @mock.patch('charmhelpers.core.hookenv.relation_ids')
     @mock.patch('charmhelpers.core.hookenv.relation_get')
     def test_nats_relation_partial(self, mrel, mid, mrelated):
         mid.return_value = ['nats']
-        mrel.return_value = {'nats_address': 'host'}
+        mrel.return_value = {'address': 'host'}
         mrelated.return_value = ['router/0']
         n = contexts.NatsRelation()
         self.assertEqual(n, {})
@@ -56,10 +55,10 @@
         mrel.return_value = {'domain': 'example.com'}
         mrelated.return_value = ['router/0']
         n = contexts.RouterRelation()
-        expected = {'router': {'domain': 'example.com',
-                               'router/0': {'domain': 'example.com'}}}
+        expected = {'router': [{'domain': 'example.com'}]}
         self.assertTrue(bool(n))
         self.assertEqual(n, expected)
+        self.assertEqual(n['router'][0]['domain'], 'example.com')
 
 
 class TestStoredContext(unittest.TestCase):

=== modified file 'tests/core/test_services.py'
--- tests/core/test_services.py	2014-05-29 17:23:48 +0000
+++ tests/core/test_services.py	2014-06-06 19:43:53 +0000
@@ -310,7 +310,7 @@
         mhookenv.related_units.return_value = []
         self.context.get_data()
         self.assertFalse(self.context.is_ready())
-        self.assertEqual(self.context, {'http': {}})
+        self.assertEqual(self.context, {'http': []})
 
     @mock.patch.object(services, 'hookenv')
     def test_incomplete(self, mhookenv):
@@ -319,8 +319,8 @@
         mhookenv.relation_get.side_effect = [{}, {'foo': '1'}]
         self.assertFalse(bool(self.context))
         self.assertEqual(mhookenv.relation_get.call_args_list, [
+            mock.call(rid='apache', unit='apache/0'),
             mock.call(rid='nginx', unit='nginx/0'),
-            mock.call(rid='apache', unit='apache/0'),
         ])
 
     @mock.patch.object(services, 'hookenv')
@@ -329,23 +329,17 @@
         mhookenv.related_units.side_effect = lambda i: [i+'/0']
         mhookenv.relation_get.side_effect = [{'foo': '1'}, {'foo': '2', 'bar': '3'}, {}]
         self.context.get_data()
-        self.assertEqual(self.context, {'http': {
-            'foo': '2',
-            'bar': '3',
-            'nginx/0': {
-                'foo': '1',
-            },
-            'apache/0': {
+        self.assertTrue(self.context.is_ready())
+        self.assertEqual(self.context, {'http': [
+            {
                 'foo': '2',
                 'bar': '3',
             },
-            'tomcat/0': {
-            },
-        }})
+        ]})
         mhookenv.relation_ids.assert_called_with('http')
         self.assertEqual(mhookenv.relation_get.call_args_list, [
+            mock.call(rid='apache', unit='apache/0'),
             mock.call(rid='nginx', unit='nginx/0'),
-            mock.call(rid='apache', unit='apache/0'),
             mock.call(rid='tomcat', unit='tomcat/0'),
         ])