Merge lp:~hazmat/pyjuju/hooks-with-formula-dir into lp:pyjuju

[Kapil Thangavelu]

Hooks are now executed in the unit root directory, and have a FORMULA_DIR environment variable pointing to the formula directory.

[Gustavo Niemeyer (niemeyer)]

[1]

We talked about having the formula path as the cwd, and I've seen at least one person mentioning the expectation that the cwd is actually formula/hooks. I'd probably be worth to do a quick individual questioning about what each formula author we have expects cwd to be, and use that instead of the unit dir.

[Gustavo Niemeyer (niemeyer)]

Putting it to WIP for pondering about [1] and so that the second reviewer gets to see the result of it.

review: Needs Fixing

[Kapil Thangavelu (hazmat)]

Excerpts from Gustavo Niemeyer's message of Wed Jul 06 00:51:26 UTC 2011:
> [1]
>
> We talked about having the formula path as the cwd, and I've seen at least one person mentioning the expectation that the cwd is actually formula/hooks. I'd probably be worth to do a quick individual questioning about what each formula author we have expects cwd to be, and use that instead of the unit dir.

from irc

<hazmat> a question for formula authors in the room, which directory would you want your hooks to run in.. The formula hooks directory, the formula directory, or the root of the unit directory (the formula for the unit lives at '/formula' under the unit root)?
<fwereade> what else lives in the unit root at the moment?
<fwereade> the formula directory feels natural to me but I am basically 100% ignorant
<hazmat> fwereade, with lxc, the unit root will be the fs root "/" of the container
<fwereade> ah ok
<hazmat> at the moment, there's just the service unit log file and the formula directory in the root
<fwereade> ok, the hooks directory feels wrong because... well, it's the formula's interface, and if we run in that dir we encourage people to clutter it up with other stuff
<fwereade> I don't think I have a context to judge the distinction between running in the formula dir and the unit root
<hazmat> fwereade, agreed, authors can include arbitrary library or configuration stuff in the formula, its probably nicer to address that from the root, then including a '..' everywhere or cluttering up the hooks directory
<hazmat> er... s/root/ i mean the formula directory
<fwereade> yep
* hazmat makes it so

[lp:~hazmat/pyjuju/hooks-with-formula-dir]

262. By Kapil Thangavelu on 2011-07-06

merge the debug-with-formula-dir, forget this was setup as a bzr-pipeline

[Gustavo Niemeyer (niemeyer)]

This looks good, thanks!

Two details to ponder about before merging:

[2]

The variable name: should it be prefixed by ENSEMBLE_ like the rest of them? Is it more common to have _PATH rather than _DIR?

[3]

Would be good to document the directory next to the hooks description in the formulas spec.

review: Approve

[Kapil Thangavelu (hazmat)]

hmm.. i actually meant to yank the formula dir env variable, but i had shelved it while resolving some test failures. i don't really see the need for it anymore since its the starting current dir of the executing hook. some hooks may want the env variable, but they can easily save cwd if they do.

[Gustavo Niemeyer (niemeyer)]

Can we name it as ENSEMBLE_FORMULA_PATH then, since it's not so common anymore?

I think it's still useful to have it around, for the purpose of tools. It's convenient to be able to call anything in a tool and have the tool knowing where to find things relative to the formula, independently from the CWD (e.g. templates).

[William Reade (fwereade)]

Looks good to me(as do niemeyer's suggestions).

review: Approve

=== modified file 'docs/source/faq.rst'
--- docs/source/faq.rst	2011-06-20 19:04:35 +0000
+++ docs/source/faq.rst	2011-07-06 18:05:30 +0000
@@ -59,6 +59,13 @@
   Also integration work with the `Orchestra <https://launchpad.net/orchestra>`_
   project is underway to enable deployment to hardware machines
 
+What directory are hooks executed in?
+
+  Hooks are executed in the formula directory (the parent directory to the hook
+  directory). This is primarily to encourage putting additional resources that
+  a hook may use outside of the hooks directory which is the public interface
+  of the formula.
+
 How can I contact the Ensemble team?
 
   User and formula author oriented resources

=== modified file 'ensemble/hooks/invoker.py'
--- ensemble/hooks/invoker.py	2011-06-11 02:25:26 +0000
+++ ensemble/hooks/invoker.py	2011-07-06 18:05:30 +0000
@@ -57,18 +57,24 @@
     `logger` -- instance of a logging.Logger object used to capture
                 hook output.
     """
-    def __init__(self, context, change, client_id, socket_path, logger):
+    def __init__(self, context, change, client_id, socket_path,
+                 unit_path, logger):
         self.environment = {}
         self._context = context
         self._change = change
         self._client_id = client_id
         self._socket_path = socket_path
+        self._unit_path = unit_path
         self._log = logger
         # The twisted.internet.process.Process instance.
         self._process = None
         # The hook executable path
         self._process_executable = None
 
+    @property
+    def unit_path(self):
+        return self._unit_path
+
     def get_environment(self):
         """
         Returns the environment used to run the hook as a dict.
@@ -78,6 +84,7 @@
         """
         base = dict(ENSEMBLE_AGENT_SOCKET=self._socket_path,
                     ENSEMBLE_CLIENT_ID=self._client_id,
+                    FORMULA_DIR=os.path.join(self._unit_path, "formula"),
                     ENSEMBLE_UNIT_NAME=os.environ["ENSEMBLE_UNIT_NAME"],
                     PATH=os.environ["PATH"],
                     PYTHON=os.environ.get("PYTHON", sys.executable),
@@ -158,7 +165,9 @@
 
             return result
 
-        self._process = reactor.spawnProcess(hook_proto, hook, [hook], env)
+        self._process = reactor.spawnProcess(
+            hook_proto, hook, [hook], env,
+            os.path.join(self._unit_path, "formula"))
         deferred.addBoth(cleanup_process)
 
         return deferred

=== modified file 'ensemble/hooks/tests/test_executor.py'
--- ensemble/hooks/tests/test_executor.py	2011-06-03 14:17:31 +0000
+++ ensemble/hooks/tests/test_executor.py	2011-07-06 18:05:30 +0000
@@ -3,7 +3,6 @@
 import subprocess
 
 from twisted.internet.defer import inlineCallbacks, Deferred
-from twisted.internet.process import Process
 from twisted.internet.error import ProcessExitedAlready
 
 import ensemble.hooks.executor
@@ -192,23 +191,30 @@
             ensemble.hooks.executor, "DEBUG_HOOK_TEMPLATE",
             "\n".join(("#!/bin/bash",
              "exit_handler() {",
-             "  echo clean exit %s",
+             "  echo clean exit",
              "  exit 0",
              "}",
              'trap "exit_handler" HUP',
              "sleep 0.2",
              "exit 1")))
 
+        unit_dir = self.makeDir()
+
+        formula_dir = os.path.join(unit_dir, "formula")
+        self.makeDir(path=formula_dir)
+
         self._executor.set_debug(["*"])
         log = logging.getLogger("invoker")
         # Populate environment variables for default invoker.
         self.change_environment(
             ENSEMBLE_UNIT_NAME="dummy/1", PATH="/bin/:/usr/bin")
         output = self.capture_logging("invoker", level=logging.DEBUG)
-        invoker = Invoker(None, None, "constant", self.makeFile(), log)
+        invoker = Invoker(
+            None, None, "constant", self.makeFile(), unit_dir, log)
 
         self._executor.start()
         hook_done = self._executor(invoker, "abc")
+
         # Give a moment for execution to start.
         yield self.sleep(0.1)
         self._executor.set_debug(None)

=== modified file 'ensemble/hooks/tests/test_invoker.py'
--- ensemble/hooks/tests/test_invoker.py	2011-06-11 02:37:35 +0000
+++ ensemble/hooks/tests/test_invoker.py	2011-07-06 18:05:30 +0000
@@ -8,7 +8,6 @@
 import yaml
 
 from twisted.internet import defer
-from twisted.python.log import PythonLoggingObserver
 
 import ensemble
 from ensemble import errors
@@ -26,9 +25,10 @@
 class MockUnitAgent(object):
     """Pretends to implement the client state cache, and the UA hook socket.
     """
-    def __init__(self, client, socket_path):
+    def __init__(self, client, socket_path, formula_dir):
         self.client = client
         self.socket_path = socket_path
+        self.formula_dir = formula_dir
         self._clients = {}  # client_id -> HookContext
 
         self._agent_log = logging.getLogger("unit-agent")
@@ -74,6 +74,7 @@
         exe = invoker.Invoker(context, change,
                               self.get_client_id(),
                               self.socket_path,
+                              self.formula_dir,
                               logger)
         return exe
 
@@ -185,7 +186,12 @@
 
         self.update_invoker_env("mysql/0", "wordpress/0")
         self.socket_path = self.makeFile()
-        self.ua = MockUnitAgent(self.client, self.socket_path)
+        unit_dir = self.makeDir()
+        self.makeDir(path=os.path.join(unit_dir, "formula"))
+        self.ua = MockUnitAgent(
+            self.client,
+            self.socket_path,
+            unit_dir)
 
     @defer.inlineCallbacks
     def tearDown(self):
@@ -279,9 +285,38 @@
         self.assertEqual(data["forgotten"], "lyrics")
 
     @defer.inlineCallbacks
+    def test_hook_exec_in_formula_directory(self):
+        """Hooks are executed in the formula directory."""
+        yield self.build_default_relationships()
+        hook_log = self.capture_logging("hook")
+        exe = self.ua.get_invoker("db", "add", "mysql/0", self.mysql_relation,
+                                  client_id="client_id")
+        self.assertTrue(os.path.isdir(exe.unit_path))
+        exe.environment["ENSEMBLE_REMOTE_UNIT"] = "wordpress/0"
+
+        # verify the hook's execution directory
+        hook_path = self.makeFile("#!/bin/bash\necho $PWD")
+        os.chmod(hook_path, stat.S_IEXEC | stat.S_IREAD)
+        result = yield exe(hook_path)
+        self.assertEqual(hook_log.getvalue().strip(),
+                         os.path.join(exe.unit_path, "formula"))
+        self.assertEqual(result, 0)
+
+        # Reset the output capture
+        hook_log.seek(0)
+        hook_log.truncate()
+
+        # Verify the environment variable is set.
+        hook_path = self.makeFile("#!/bin/bash\necho $FORMULA_DIR")
+        os.chmod(hook_path, stat.S_IEXEC | stat.S_IREAD)
+        result = yield exe(hook_path)
+        self.assertEqual(hook_log.getvalue().strip(),
+                         os.path.join(exe.unit_path, "formula"))
+
+    @defer.inlineCallbacks
     def test_spawn_cli_config_get(self):
         """Validate that config-get returns expected values."""
-        state = yield self.build_default_relationships()
+        yield self.build_default_relationships()
 
         hook_log = self.capture_logging("hook")
 
@@ -294,13 +329,13 @@
         exe = self.ua.get_invoker("db", "add", "mysql/0", self.mysql_relation,
                                   client_id="client_id")
 
-        mysql = state["services"]["mysql"]
         context = yield self.ua.get_context("client_id")
         config = yield context.get_config()
         config.update(expected)
         yield config.write()
 
         # invoke relation-get and verify the result
+
         result = yield exe(self.create_hook("config-get", "--format=json"))
         self.assertEqual(result, 0)
 
@@ -318,7 +353,12 @@
         yield self._default_relations()
         self.update_invoker_env("mysql/0", "wordpress/0")
         self.socket_path = self.makeFile()
-        self.ua = MockUnitAgent(self.client, self.socket_path)
+        unit_dir = self.makeDir()
+        self.makeDir(path=os.path.join(unit_dir, "formula"))
+        self.ua = MockUnitAgent(
+            self.client,
+            self.socket_path,
+            unit_dir)
         self.log = self.capture_logging(
             formatter=logging.Formatter(
                 "%(name)s:%(levelname)s:: %(message)s"),

=== modified file 'ensemble/unit/lifecycle.py'
--- ensemble/unit/lifecycle.py	2011-05-25 20:52:05 +0000
+++ ensemble/unit/lifecycle.py	2011-07-06 18:05:30 +0000
@@ -309,7 +309,8 @@
         hook_path = os.path.join(unit_path, "formula", "hooks", hook_name)
         socket_path = os.path.join(unit_path, HOOK_SOCKET_FILE)
 
-        invoker = Invoker(None, None, "constant", socket_path, hook_log)
+        invoker = Invoker(
+            None, None, "constant", socket_path, self._unit_path, hook_log)
 
         if now:
             yield self._executor.run_priority_hook(invoker, hook_path)
@@ -387,8 +388,9 @@
                 hook_names = ["%s-relation-changed" % self._relation_name]
         else:
             hook_names = [hook_name]
-        invoker = RelationInvoker(
-            context, change, "constant", socket_path, hook_log)
+
+        invoker = RelationInvoker(context, change, "constant", socket_path,
+                                  self._unit_path, hook_log)
 
         for hook_name in hook_names:
             hook_path = os.path.join(

=== modified file 'ensemble/unit/tests/test_lifecycle.py'
--- ensemble/unit/tests/test_lifecycle.py	2011-05-26 11:20:43 +0000
+++ ensemble/unit/tests/test_lifecycle.py	2011-07-06 18:05:30 +0000
@@ -750,10 +750,13 @@
             PATH=os.environ["PATH"],
             ENSEMBLE_UNIT_NAME="service-unit/0")
         change = RelationChange("clients", "joined", "s/2")
-        invoker = RelationInvoker(None, change, "", "", None)
+        unit_hook_path = self.makeDir()
+        invoker = RelationInvoker(None, change, "", "", unit_hook_path, None)
         environ = invoker.get_environment()
         self.assertEqual(environ["ENSEMBLE_RELATION"], "clients")
         self.assertEqual(environ["ENSEMBLE_REMOTE_UNIT"], "s/2")
+        self.assertEqual(environ["FORMULA_DIR"],
+                         os.path.join(unit_hook_path, "formula"))
 
 
 class UnitRelationLifecycleTest(LifecycleTestBase):