Merge lp:~hazmat/pyjuju/peers-from-hurd into lp:pyjuju

[Kapil Thangavelu]

Unit process multiple relations in defined order.

Previously when unit agents were being notified of multiple relations, typicaly
at startup, they would process them in essentially random order. This would
complicate things for applications that would have peer relations as well, for
ha or replication, as they couldn't tell if they were being run standalone or
in a cluster/quorum. Instead when a unit has multiple relations, we process
peer relations first followed by client/server relations.

https://codereview.appspot.com/7341044/

[Kapil Thangavelu (hazmat)]

Reviewers: mp+148528_code.launchpad.net,

Message:
Please take a look.

Description:
Unit process multiple relations in defined order.

Previously when unit agents were being notified of multiple relations,
typicaly
at startup, they would process them in essentially random order. This
would
complicate things for applications that would have peer relations as
well, for
ha or replication, as they couldn't tell if they were being run
standalone or
in a cluster/quorum. Instead when a unit has multiple relations, we
process
the relations in creation order, given that a service's peer relations
are
always created upon service deploy, this gives deterministic behavior
that
solves the use case instead of previous random order.

https://code.launchpad.net/~hazmat/juju/peers-from-hurd/+merge/148528

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/unit/lifecycle.py
   M juju/unit/tests/test_lifecycle.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: juju/unit/lifecycle.py
=== modified file 'juju/unit/lifecycle.py'
--- juju/unit/lifecycle.py 2012-09-10 03:20:20 +0000
+++ juju/unit/lifecycle.py 2013-02-14 18:29:42 +0000
@@ -374,8 +374,15 @@
                  (service_relation.internal_relation_id, service_relation)
                  for service_relation in old_relations)

- added = set(new_relations.keys()) - set(self._relations.keys())
- removed = set(self._relations.keys()) - set(new_relations.keys())
+ # We process new relations in creation order, and remove rels
+ # most recently created to oldest. As a consequence, this
+ # ensures that new peer relations are processed (created at
+ # service deployment) are processed before client/server
+ # relations when multiple rels are available at unit startup.
+ added = sorted(set(new_relations.keys()) -
set(self._relations.keys()))
+ removed = list(reversed(sorted(
+ set(self._relations.keys()) - set(new_relations.keys()))))
+
          # Could this service be a principal container?
          is_principal = not (yield self._service.is_subordinate())

@@ -404,12 +411,13 @@
                  yield workflow.transition_state("departed")
              self._store_relations()

- # Process new relations.
+ # Process new relations
          for relation_id in added:
              service_relation = new_relations[relation_id]
              yield self._add_relation(service_relation)
- if (is_principal and service_relation.relation_scope
== "container"):
- self._add_subordinate_unit(service_relation)
+ if (is_principal
+ and service_relation.relation_scope == "container"):
+ yield self._add_subordinate_unit(service_relation)
              yield self._store_relations()

      @i...

[Benjamin Saller (bcsaller)]

LGTM so long as contract is documented as we talked about on the call.
Thanks.

https://codereview.appspot.com/7341044/

[lp:~hazmat/pyjuju/peers-from-hurd]

618. By Kapil Thangavelu on 2013-02-14

clean up comments

619. By Kapil Thangavelu on 2013-02-14

redo rel sorting, as peers rels can be recreated

[Kapil Thangavelu (hazmat)]

Please take a look.

https://codereview.appspot.com/7341044/

[Kapil Thangavelu (hazmat)]

On 2013/02/14 23:01:19, hazmat wrote:
> Please take a look.

this time more explicit about peer behavior with a custom sort function
and explicit test of peers instead of pure creation order.

https://codereview.appspot.com/7341044/

[Benjamin Saller (bcsaller)]

That is better, thanks for following up on that. +1

https://codereview.appspot.com/7341044/

[Kapil Thangavelu (hazmat)]

*** Submitted:

Unit process multiple relations in defined order.

Previously when unit agents were being notified of multiple relations,
typicaly
at startup, they would process them in essentially random order. This
would
complicate things for applications that would have peer relations as
well, for
ha or replication, as they couldn't tell if they were being run
standalone or
in a cluster/quorum. Instead when a unit has multiple relations, we
process
peer relations first followed by client/server relations.

R=bcsaller
CC=
https://codereview.appspot.com/7341044

https://codereview.appspot.com/7341044/

=== modified file 'juju/control/remove_relation.py'
--- juju/control/remove_relation.py	2012-04-12 09:23:50 +0000
+++ juju/control/remove_relation.py	2013-02-14 23:02:23 +0000
@@ -89,4 +89,4 @@
     yield client.close()
 
     log.info("Removed %s relation from all service units.",
-        endpoints[0].relation_type)
+             endpoints[0].relation_type)
 
=== modified file 'juju/unit/lifecycle.py'
--- juju/unit/lifecycle.py	2012-09-10 03:20:20 +0000
+++ juju/unit/lifecycle.py	2013-02-14 23:02:23 +0000
@@ -360,6 +360,30 @@
         finally:
             self._run_lock.release()
 
+    def _sort_relations(self, rel_ids, rels, invert=False):
+        """ Sort a set of relations.
+
+        We process peer relations first when adding, and last when
+        removing, else deferring to creation order.
+        """
+        rel_ids = list(rel_ids)
+
+        def _sort(x, y):
+            xr, yr = rels[x].relation_role, rels[y].relation_role
+            if xr == yr:
+                return cmp(x, y)
+            elif xr == "peer":
+                return -1
+            elif yr == "peer":
+                return 1
+            return cmp(x, y)
+
+        rel_ids.sort(_sort)
+
+        if invert:
+            return list(reversed(rel_ids))
+        return rel_ids
+
     @inlineCallbacks
     def _process_service_changes(self, old_relations, new_relations):
         """Add and remove unit lifecycles per the service relations Determine.
@@ -374,8 +398,17 @@
                 (service_relation.internal_relation_id, service_relation)
                 for service_relation in old_relations)
 
-        added = set(new_relations.keys()) - set(self._relations.keys())
-        removed = set(self._relations.keys()) - set(new_relations.keys())
+        added = self._sort_relations(
+            set(new_relations.keys()) - set(self._relations.keys()),
+            new_relations)
+        print 'add', added, [new_relations[a] for a in added]
+
+        removed = self._sort_relations(
+            set(self._relations.keys()) - set(new_relations.keys()),
+            self._relations,
+            invert=True)
+        print 'removed', removed
+
         # Could this service be a principal container?
         is_principal = not (yield self._service.is_subordinate())
 
@@ -404,12 +437,13 @@
                 yield workflow.transition_state("departed")
             self._store_relations()
 
-        # Process new relations.
+        # Process new relations
         for relation_id in added:
             service_relation = new_relations[relation_id]
             yield self._add_relation(service_relation)
-            if (is_principal and service_relation.relation_scope == "container"):
-                self._add_subordinate_unit(service_relation)
+            if (is_principal
+                    and service_relation.relation_scope == "container"):
+                yield self._add_subordinate_unit(service_relation)
             yield self._store_relations()
 
     @inlineCallbacks
 
=== modified file 'juju/unit/tests/test_lifecycle.py'
--- juju/unit/tests/test_lifecycle.py	2013-02-01 16:24:08 +0000
+++ juju/unit/tests/test_lifecycle.py	2013-02-14 23:02:23 +0000
@@ -34,7 +34,7 @@
 from juju.unit.tests.test_charm import CharmPublisherTestBase
 
 from juju.lib.testing import TestCase
-from juju.lib.mocker import MATCH
+from juju.lib.mocker import MATCH, ANY
 
 
 class UnwriteablePath(object):
@@ -893,6 +893,20 @@
         self.assertEqual("up", (yield staying_workflow.get_state()))
         yield new_lifecycle.stop()
 
+    def test_multiple_rel_order(self):
+        """Given multiple new rels, verify peer processed first."""
+
+        class rel(object):
+            def __init__(self, r):
+                self.relation_role = r
+
+        rels = {'5': rel('peer'), '4': rel('client'),
+                '2': rel('server'), '1': rel('peer')}
+
+        self.assertEqual(
+            self.lifecycle._sort_relations(rels.keys(), rels),
+            ["1", "5", "2", "4"])
+
     @inlineCallbacks
     def test_start_invoker(self):
         """Verify the invoker is started by the unit relation lifecycle"""
@@ -901,7 +915,7 @@
         # these will be on the relation with ident app:0, app:1, app:2
         self.write_hook(
             "start",
-            '#!/bin/sh\n echo "hello world"\n')
+            '#!/bin/sh\n echo hello\n')
         blog_states = yield self.add_opposite_service_unit(self.states)
         yield blog_states['service_relations'][-1].add_unit_state(
             self.states['unit'])
@@ -909,10 +923,10 @@
         for i in range(1, 3):
             blog_states = yield self.add_opposite_service_unit(
                 (yield self.add_relation_service_unit_to_another_endpoint(
-                        self.states,
-                        RelationEndpoint(
-                            "wordpress-%d" % i,
-                            "client-server", "db", "client"))))
+                    self.states,
+                    RelationEndpoint(
+                        "wordpress-%d" % i,
+                        "client-server", "db", "client"))))
             yield blog_states['service_relations'][-1].add_unit_state(
                 self.states['unit'])