Merge lp:~jimbaker/pyjuju/new-hook-semantics-1-departed-hook into lp:pyjuju

[Jim Baker]

Adds a <name>-relation-departed hook, while keeping the existing
<name<-relation-changed hook. After some deliberation, the simplest way to
add this new hook semantic (and the other aspects, to be introduced in
future branches) is in UnitRelationCycle._execute_change_hook (of
ensemble.unit.lifecycle). This method iterates over the desired change
hooks corresponding to a given change. So in the case of a departed
event:

           if change.change_type == "departed":
               hook_names = [
                   "%s-relation-changed" % self._relation_name,
                   "%s-relation-departed" % self._relation_name]

A subsequent branch will remove the above <name>-relation-changed.

[Gustavo Niemeyer (niemeyer)]

It looks good to me, but please wait for Kapil's review since he
knows more about this area.

Some minor comments as well:

[1]

+ self._log.debug("%s: executing change hook", self._relation_name)

This log message seems incorrect now. It should probably be something like

    "Executing hook %s" % hook_name

Please do something similar to the other log messages.

[2]

51 + if not self._error_handler:
52 + raise

It's not entirely clear to me that this is correct. Do we really want
to ignore the other hook when this happens?

[3]

         # Queue up our wait condition, of 3 hooks firing

s/3/4/

[4]

84 + "app-relation-changed",
85 + "app-relation-changed", "app-relation-departed"])
(...)
96 + "start",
97 + "app-relation-changed", "app-relation-departed",
98 + "app-relation-changed"])

Please organize the lists evenly.

[5]

+ ("#!/bin/bash\n" "echo departed >> %s\n" % (file_path)))

Please remove the parenthesis around file_path in both cases. They're
are not doing anything.

review: Approve

[Kapil Thangavelu (hazmat)]

Looks good to me, +1

[1] There are some pep8/style formatting issues, i've got a diff to fix here
https://pastebin.canonical.com/45766/

[2] Regarding gustavo's 2. Any error in a hook should exit processing as soon as possible, any error in the hook should be handled and as early possible. The hook executor will raise an errback on the execution deferred, and the workflow will transition to an error state. a recovery transition, will have to deal with partial state recoveries, but i think that's regardless of mechanism (although possible w/ btrfs snapshots pre transition would make a reasonable mechanism of partial minimization coupled with a multi hook execution flush of the associated hook api context buffers.

[Kapil Thangavelu (hazmat)]

forget the lp merge approve.

review: Approve

[Jim Baker (jimbaker)]

On Wed, Apr 6, 2011 at 9:40 AM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Approve
> It looks good to me, but please wait for Kapil's review since he
> knows more about this area.
>
> Some minor comments as well:
>
>
> [1]
>
> + self._log.debug("%s: executing change hook",
> self._relation_name)
>
> This log message seems incorrect now. It should probably be something like
>
> "Executing hook %s" % hook_name
>
> Please do something similar to the other log messages.
>
>
Cleaned up

>
> [2]
>
> 51 + if not self._error_handler:
> 52 + raise
>
> It's not entirely clear to me that this is correct. Do we really want
> to ignore the other hook when this happens?
>
>
Kapil comments on this in his review; additionally, there has been no change
in this logic except that it has now part of an enclosing loop to
accommodate the fact that one or two hooks will be invoked for a change
event

>
> [3]
>
> # Queue up our wait condition, of 3 hooks firing
>
> s/3/4/
>

Changed

>
> [4]
>
> 84 + "app-relation-changed",
> 85 + "app-relation-changed",
> "app-relation-departed"])
> (...)
> 96 + "start",
> 97 + "app-relation-changed", "app-relation-departed",
> 98 + "app-relation-changed"])
>
> Please organize the lists evenly.
>
>
I reformatted this and also added comments to make more clear why I was
doing this organization to begin with, namely to show that the two hooks on
one line were actually resulting from one event.

>
> [5]
>
>
> + ("#!/bin/bash\n" "echo departed >> %s\n" % (file_path)))
>
> Please remove the parenthesis around file_path in both cases. They're
> are not doing anything.
>

Done

=== modified file 'ensemble/unit/lifecycle.py'
--- ensemble/unit/lifecycle.py	2011-02-25 13:55:16 +0000
+++ ensemble/unit/lifecycle.py	2011-03-29 17:12:32 +0000
@@ -279,31 +279,37 @@
         """
         socket_path = os.path.join(self._unit_path, HOOK_SOCKET_FILE)
         if hook_name is None:
-            hook_name = "%s-relation-changed" % self._relation_name
-
-        hook_path = os.path.join(
-            self._unit_path, "formula", "hooks", hook_name)
-
+            if change.change_type == "departed":
+                hook_names = [
+                    "%s-relation-changed" % self._relation_name,
+                    "%s-relation-departed" % self._relation_name]
+            else:
+                hook_names = ["%s-relation-changed" % self._relation_name]
+        else:
+            hook_names = [hook_name]
         invoker = RelationInvoker(
             context, change, "constant", socket_path, hook_log)
 
-        yield self._run_lock.acquire()
-        self._log.debug("%s: executing change hook", self._relation_name)
-        try:
-            yield self._executor(invoker, hook_path)
-        except Exception, e:
-            yield self._run_lock.release()
-            self._log.warn("%s: change hook error: %s", self._relation_name, e)
+        for hook_name in hook_names:
+            hook_path = os.path.join(
+                self._unit_path, "formula", "hooks", hook_name)
+            yield self._run_lock.acquire()
+            self._log.debug("%s: executing change hook", self._relation_name)
+            try:
+                yield self._executor(invoker, hook_path)
+            except Exception, e:
+                yield self._run_lock.release()
+                self._log.warn("%s: change hook error: %s", self._relation_name, e)
 
-            if not self._error_handler:
-                raise
-            self._log.info(
-                "%s: invoke change hook error handler", self._relation_name)
-            # We can't hold the run lock, when we invoke the error handler, or
-            # we get a deadlock if the handler manipulates the lifecycle.
-            yield self._error_handler(change, e)
-        else:
-            yield self._run_lock.release()
+                if not self._error_handler:
+                    raise
+                self._log.info(
+                    "%s: invoke change hook error handler", self._relation_name)
+                # We can't hold the run lock, when we invoke the error handler, or
+                # we get a deadlock if the handler manipulates the lifecycle.
+                yield self._error_handler(change, e)
+            else:
+                yield self._run_lock.release()
 
     def set_hook_error_handler(self, handler):
         """Set an error handler to be invoked if a hook errors.

=== modified file 'ensemble/unit/tests/test_lifecycle.py'
--- ensemble/unit/tests/test_lifecycle.py	2011-02-27 01:12:50 +0000
+++ ensemble/unit/tests/test_lifecycle.py	2011-03-29 17:12:32 +0000
@@ -226,6 +226,9 @@
         self.write_hook(
             "%s-relation-changed" % relation_name,
             ("#!/bin/bash\n" "echo $ENSEMBLE_CHANGE >> %s\n" % (file_path)))
+        self.write_hook(
+            "%s-relation-departed" % relation_name,
+            ("#!/bin/bash\n" "echo departed >> %s\n" % (file_path)))
 
         self.assertFalse(os.path.exists(file_path))
         return file_path
@@ -251,7 +254,10 @@
                          ["joined"])
 
         # Queue up our wait condition, of 3 hooks firing
-        hooks_complete = self.wait_on_hook("app-relation-changed", 3)
+        hooks_complete = self.wait_on_hook(
+            sequence=["app-relation-changed",
+                      "app-relation-changed",
+                      "app-relation-changed", "app-relation-departed"])
 
         # add another.
         wordpress2_states = yield self.add_opposite_service_unit(
@@ -371,11 +377,15 @@
         # XXX - With scheduler local state recovery, we should get the modify.
 
         # Start and verify events.
-        hooks_executed = self.wait_on_hook("app-relation-changed", 2)
+        hooks_executed = self.wait_on_hook(
+            sequence=[
+                "start", 
+                "app-relation-changed", "app-relation-departed",
+                "app-relation-changed"])
         yield self.lifecycle.start()
         yield hooks_executed
         self.assertEqual([x.strip() for x in open(file_path)],
-                         ["joined", "joined", "departed", "joined"])
+                         ["joined", "joined", "departed", "departed", "joined"])
 
     @inlineCallbacks
     def test_lock_start_stop_watch(self):