Code review comment for lp:~hazmat/pyjuju/peers-from-hurd

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

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()

      @inlineCallbacks

Index: juju/unit/tests/test_lifecycle.py
=== 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 18:26:56 +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):
@@ -894,6 +894,44 @@
          yield new_lifecycle.stop()

      @inlineCallbacks
+ def test_multiple_rel_order(self):
+ """Given multiple new rels, rels processed in creation order."""
+ blog_states = yield self.add_opposite_service_unit(self.states)
+ yield blog_states['service_relations'][-1].add_unit_state(
+ self.states['unit'])
+
+ 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"))))
+ yield blog_states['service_relations'][-1].add_unit_state(
+ self.states['unit'])
+
+ results = []
+ mock_lifecycle = self.mocker.patch(self.lifecycle)
+ done = Deferred()
+
+ def collect(*args):
+ results.append(args[-1])
+ if len(results) == 3:
+ done.callback(True)
+ return True
+
+ mock_lifecycle._add_relation(ANY)
+ self.mocker.call(collect, with_object=True)
+ self.mocker.count(3)
+ self.mocker.replay()
+
+ yield self.lifecycle.start()
+ yield done
+ rel_ids = [int(rel.internal_relation_id.split("-")[1])
+ for rel in results]
+ self.assertEqual([0, 1, 2], rel_ids)
+
+ @inlineCallbacks
      def test_start_invoker(self):
          """Verify the invoker is started by the unit relation lifecycle"""
          # Setup three different wordpress services, wordpress,
@@ -901,7 +939,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 +947,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'])

« Back to merge proposal