Merge lp:~jimbaker/pyjuju/unit-rel-lifecycle-start-invoker into lp:pyjuju

[Jim Baker]

Ensure Invoker.start is called from UnitRelationLifecycle usage.

UnitRelationLifecycle did not properly call Invoker.start (a one-line fix), which prevented relation hook contexts from being cached for the corresponding hook call. Because caching of the hook contexts is logged, this can be verified to ensure this method is properly called for both UnitRelationLifecycle and UnitLifecycle usages.

https://codereview.appspot.com/6131061/

[Jim Baker (jimbaker)]

Reviewers: mp+104195_code.launchpad.net,

Message:
Please take a look.

Description:
Ensure Invoker.start is called from UnitRelationLifecycle usage.

UnitRelationLifecycle did not properly call Invoker.start (a one-line
fix), which prevented relation hook contexts from being cached for the
corresponding hook call. Because caching of the hook contexts is logged,
this can be verified to ensure this method is properly called for both
UnitRelationLifecycle and UnitLifecycle usages.

https://code.launchpad.net/~jimbaker/juju/unit-rel-lifecycle-start-invoker/+merge/104195

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/hooks/invoker.py
   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/hooks/invoker.py
=== modified file 'juju/hooks/invoker.py'
--- juju/hooks/invoker.py 2012-04-05 00:49:40 +0000
+++ juju/hooks/invoker.py 2012-05-01 00:26:25 +0000
@@ -175,11 +175,16 @@
          if isinstance(self._context, RelationHookContext):
              # Exclude the parent context for being looked up as a child
              relation_idents.discard(self._context.relation_ident)
+ display_parent_relation_ident = " on %r" % \
+ self._context.relation_ident
+ else:
+ display_parent_relation_ident = ""
          for relation_ident in relation_idents:
              child = yield self._context.get_relation_hook_context(
                  relation_ident)
              self._relation_contexts[relation_ident] = child
- self._log.debug("Cached relation hook contexts: %s" % (
+ self._log.debug("Cached relation hook contexts%s: %r" % (
+ display_parent_relation_ident,
                  sorted(relation_idents)))

      @property

Index: juju/unit/lifecycle.py
=== modified file 'juju/unit/lifecycle.py'
--- juju/unit/lifecycle.py 2012-04-10 22:58:44 +0000
+++ juju/unit/lifecycle.py 2012-05-01 00:26:25 +0000
@@ -731,6 +731,7 @@

      @inlineCallbacks
      def _execute_hook(self, invoker, hook_name, change):
+ yield invoker.start()
          hook_path = os.path.join(
              self._unit_dir, "charm", "hooks", hook_name)
          yield self._run_lock.acquire()

Index: juju/unit/tests/test_lifecycle.py
=== modified file 'juju/unit/tests/test_lifecycle.py'
--- juju/unit/tests/test_lifecycle.py 2012-04-10 07:10:47 +0000
+++ juju/unit/tests/test_lifecycle.py 2012-05-01 00:26:25 +0000
@@ -34,7 +34,7 @@
  from juju.unit.tests.test_charm import CharmPublisherTestBase

  from juju.lib.testing import TestCase
-from juju.lib.mocker import (ANY, MATCH, Mocker)
+from juju.lib.mocker import MATCH

  class UnwriteablePath(object):
@@ -358,7 +358,6 @@
          yield self.states["unit"].set_relation_resolved(
              {self.states["unit_rela...

[Jim Baker (jimbaker)]

Please take a look.

https://codereview.appspot.com/6131061/

[lp:~jimbaker/pyjuju/unit-rel-lifecycle-start-invoker]

534. By Jim Baker on 2012-05-01

Fixed tests that saw augmented log changes

[Kapil Thangavelu (hazmat)]

lgtm

https://codereview.appspot.com/6131061/diff/3001/juju/unit/tests/test_lifecycle.py
File juju/unit/tests/test_lifecycle.py (right):

https://codereview.appspot.com/6131061/diff/3001/juju/unit/tests/test_lifecycle.py#newcode901
juju/unit/tests/test_lifecycle.py:901: for i in xrange(1, 3):
don't use xrange, its of little value and dropped in py3.

https://codereview.appspot.com/6131061/diff/3001/juju/unit/tests/test_lifecycle.py#newcode1331
juju/unit/tests/test_lifecycle.py:1331: for i in xrange(1, 5):
ditto re xrange

https://codereview.appspot.com/6131061/

[lp:~jimbaker/pyjuju/unit-rel-lifecycle-start-invoker]

535. By Jim Baker on 2012-05-04

Merged trunk

536. By Jim Baker on 2012-05-04

Addressed review points

[Jim Baker (jimbaker)]

Thanks, changed the minors and pushed accordingly

https://codereview.appspot.com/6131061/diff/3001/juju/unit/tests/test_lifecycle.py
File juju/unit/tests/test_lifecycle.py (right):

https://codereview.appspot.com/6131061/diff/3001/juju/unit/tests/test_lifecycle.py#newcode901
juju/unit/tests/test_lifecycle.py:901: for i in xrange(1, 3):
Pure habit here, fixed by using range instead

https://codereview.appspot.com/6131061/diff/3001/juju/unit/tests/test_lifecycle.py#newcode1331
juju/unit/tests/test_lifecycle.py:1331: for i in xrange(1, 5):
Ditto fix

https://codereview.appspot.com/6131061/

[Kapil Thangavelu (hazmat)]

looks good, pls go ahead w the merge.

On 2012/05/04 03:48:08, jimbaker wrote:
> Thanks, changed the minors and pushed accordingly

https://codereview.appspot.com/6131061/diff/3001/juju/unit/tests/test_lifecycle.py
> File juju/unit/tests/test_lifecycle.py (right):

https://codereview.appspot.com/6131061/diff/3001/juju/unit/tests/test_lifecycle.py#newcode901
> juju/unit/tests/test_lifecycle.py:901: for i in xrange(1, 3):
> Pure habit here, fixed by using range instead

https://codereview.appspot.com/6131061/diff/3001/juju/unit/tests/test_lifecycle.py#newcode1331
> juju/unit/tests/test_lifecycle.py:1331: for i in xrange(1, 5):
> Ditto fix

https://codereview.appspot.com/6131061/

=== modified file 'juju/hooks/invoker.py'
--- juju/hooks/invoker.py	2012-04-05 00:49:40 +0000
+++ juju/hooks/invoker.py	2012-05-04 03:42:20 +0000
@@ -175,11 +175,16 @@
         if isinstance(self._context, RelationHookContext):
             # Exclude the parent context for being looked up as a child
             relation_idents.discard(self._context.relation_ident)
+            display_parent_relation_ident = " on %r" % \
+                self._context.relation_ident
+        else:
+            display_parent_relation_ident = ""
         for relation_ident in relation_idents:
             child = yield self._context.get_relation_hook_context(
                 relation_ident)
             self._relation_contexts[relation_ident] = child
-        self._log.debug("Cached relation hook contexts: %s" % (
+        self._log.debug("Cached relation hook contexts%s: %r" % (
+                display_parent_relation_ident,
                 sorted(relation_idents)))
 
     @property

=== modified file 'juju/hooks/tests/test_invoker.py'
--- juju/hooks/tests/test_invoker.py	2012-05-02 00:49:10 +0000
+++ juju/hooks/tests/test_invoker.py	2012-05-04 03:42:20 +0000
@@ -1035,7 +1035,7 @@
         # then by children
         self.assertLogLines(
             self.log.getvalue(),
-            ["Cached relation hook contexts: ['db:1']",
+            ["Cached relation hook contexts on 'db:0': ['db:1']",
              "Flushed values for hook 'success-hook' on 'db:0'",
              "    Setting changed: u'a'=u'42' (was unset)",
              "    Setting changed: u'b'=u'xyz' (was unset)",
@@ -1087,7 +1087,7 @@
                 "private-address": "mysql-0.example.com"})
         self.assertLogLines(
             self.log.getvalue(),
-            ["Cached relation hook contexts: ['db:1', 'db:2']",
+            ["Cached relation hook contexts on 'db:0': ['db:1', 'db:2']",
              "Flushed values for hook 'success-hook' on 'db:0'",
              "    Setting changed: u'a'=u'42' (was unset)",
              "    Setting changed: u'b'=u'xyz' (was unset)",
@@ -1221,7 +1221,7 @@
                 "private-address": "mysql-0.example.com"})
         self.assertLogLines(
             self.log.getvalue(),
-            ["Cached relation hook contexts: ['db:1']",
+            ["Cached relation hook contexts on 'db:0': ['db:1']",
              "Flushed values for hook %r on 'db:0'" % os.path.basename(hook),
              "    Setting changed: u'a'=u'42' (was unset) on 'db:1'",
              "    Setting changed: u'b'=u'xyz' (was unset) on 'db:1'"])

=== modified file 'juju/unit/lifecycle.py'
--- juju/unit/lifecycle.py	2012-04-10 22:58:44 +0000
+++ juju/unit/lifecycle.py	2012-05-04 03:42:20 +0000
@@ -731,6 +731,7 @@
 
     @inlineCallbacks
     def _execute_hook(self, invoker, hook_name, change):
+        yield invoker.start()
         hook_path = os.path.join(
             self._unit_dir, "charm", "hooks", hook_name)
         yield self._run_lock.acquire()

=== modified file 'juju/unit/tests/test_lifecycle.py'
--- juju/unit/tests/test_lifecycle.py	2012-04-10 07:10:47 +0000
+++ juju/unit/tests/test_lifecycle.py	2012-05-04 03:42:20 +0000
@@ -34,7 +34,7 @@
 from juju.unit.tests.test_charm import CharmPublisherTestBase
 
 from juju.lib.testing import TestCase
-from juju.lib.mocker import (ANY, MATCH, Mocker)
+from juju.lib.mocker import MATCH
 
 
 class UnwriteablePath(object):
@@ -358,7 +358,6 @@
         yield self.states["unit"].set_relation_resolved(
             {self.states["unit_relation"].internal_relation_id: NO_HOOKS})
 
-
         # Wait for the relation to come back up
         value = yield self.states["unit"].get_relation_resolved()
 
@@ -889,6 +888,38 @@
         self.assertEqual("up", (yield staying_workflow.get_state()))
         yield new_lifecycle.stop()
 
+    @inlineCallbacks
+    def test_start_invoker(self):
+        """Verify the invoker is started by the unit relation lifecycle"""
+        # Setup three different wordpress services, wordpress,
+        # wordpress-1, wordpress-2 with one service unit each. Each of
+        # 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')
+        yield self.add_opposite_service_unit(self.states)
+        for i in range(1, 3):
+            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"))))
+
+        finished = self.wait_on_hook("app-relation-changed", 3)
+        yield self.lifecycle.start()
+        yield finished
+
+        # This will verify that config-changed and start both are
+        # cached properly. The remaining hook caching will be for the
+        # relation hook contexts, along some permutation based on how
+        # the hooks are actually scheduled. However, this is tested in
+        # UnitRelationLifecycleTest.test_start_invoker so need not be
+        # covered here.
+        self.assertLogLines(
+            self.hook_log.getvalue(),
+            ["Cached relation hook contexts: ['app:0', 'app:1', 'app:2']"] * 2)
+
 
 class UnitLifecycleUpgradeTest(
         LifecycleTestBase, CharmPublisherTestBase, CharmUpgradeTestBase):
@@ -1290,6 +1321,32 @@
         # The scheduler should run the hooks it queued up earlier
         yield hooks_complete
 
+    @inlineCallbacks
+    def test_start_invoker(self):
+        """Verify the invoker is started by the unit relation lifecycle"""
+        # Setup 5 different wordpress services, wordpress, wordpress-1
+        # through wordpress-4 with one service unit each. Each of these
+        # will be on the relation app:0 ... app:4
+        yield self.add_opposite_service_unit(self.states)
+        for i in range(1, 5):
+            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 self.lifecycle.start()
+        yield self.wait_on_hook(
+            sequence=["app-relation-joined", "app-relation-changed"])
+
+        # Currently the only action taken by the invoker.start is to
+        # cache the relation hook context; app:0 is excluded because
+        # it is the parent
+        self.assertIn(
+            ("Cached relation hook contexts on 'app:0': "
+             "['app:1', 'app:2', 'app:3', 'app:4']"),
+            self.hook_log.getvalue())
+
 
 class SubordinateUnitLifecycleTest(LifecycleTestBase, CharmPublisherTestBase):