Generalize HooksMaster

- remove any dependence on Logical Units from the HooksMaster;
- add a new function parameter to the constructor, a function that is
  expected to convert the results of the hooks execution in a format
  understood by the HooksMaster;
- add a factory method that builds a HooksMaster from a LU, keeping the
  interface of the old constructor of HooksMaster;
- remove usage of Processor.hmclass from external classes, introducing
  the Processor.BuildHooksMaster method;
- update unit tests.

Signed-off-by: Andrea Spadaccini <spadaccio@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 9a44862..6442937 100644 (file)
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -758,7 +758,7 @@ def _RunPostHook(lu, node_name):
   """Runs the post-hook for an opcode on a single node.
 
   """
-  hm = lu.proc.hmclass(lu.rpc.call_hooks_runner, lu)
+  hm = lu.proc.BuildHooksManager(lu)
   try:
     hm.RunPhase(constants.HOOKS_PHASE_POST, nodes=[node_name])
   except:

diff --git a/lib/mcpu.py b/lib/mcpu.py
index 929ab1f..bc62482 100644 (file)
--- a/lib/mcpu.py
+++ b/lib/mcpu.py
@@ -171,6 +171,20 @@ def _ComputeDispatchTable():
               if op.WITH_LU)
 
 
+def _RpcResultsToHooksResults(rpc_results):
+  """Function to convert RPC results to the format expected by HooksMaster.
+
+  @type rpc_results: dict(node: L{rpc.RpcResult})
+  @param rpc_results: RPC results
+  @rtype: dict(node: (fail_msg, offline, hooks_results))
+  @return: RPC results unpacked according to the format expected by
+    L({mcpu.HooksMaster}
+
+  """
+  return dict((node, (rpc_res.fail_msg, rpc_res.offline, rpc_res.payload))
+              for (node, rpc_res) in rpc_results.items())
+
+
 class Processor(object):
   """Object which runs OpCodes"""
   DISPATCH_TABLE = _ComputeDispatchTable()
@@ -242,7 +256,8 @@ class Processor(object):
     """
     write_count = self.context.cfg.write_count
     lu.CheckPrereq()
-    hm = HooksMaster(self.rpc.call_hooks_runner, lu)
+
+    hm = self.BuildHooksManager(lu)
     h_results = hm.RunPhase(constants.HOOKS_PHASE_PRE)
     lu.HooksCallBack(constants.HOOKS_PHASE_PRE, h_results,
                      self.Log, None)
@@ -267,6 +282,9 @@ class Processor(object):
 
     return result
 
+  def BuildHooksManager(self, lu):
+    return self.hmclass.BuildFromLu(lu.rpc.call_hooks_runner, lu)
+
   def _LockAndExecLU(self, lu, level, calc_timeout, priority):
     """Execute a Logical Unit, with the needed locks.
 
@@ -444,28 +462,53 @@ class Processor(object):
 
 
 class HooksMaster(object):
-  """Hooks master.
-
-  This class distributes the run commands to the nodes based on the
-  specific LU class.
+  def __init__(self, opcode, hooks_path, nodes, hooks_execution_fn,
+    hooks_results_adapt_fn, build_env_fn, log_fn, htype=None, cluster_name=None,
+    master_name=None):
+    """Base class for hooks masters.
+
+    This class invokes the execution of hooks according to the behaviour
+    specified by its parameters.
+
+    @type opcode: string
+    @param opcode: opcode of the operation to which the hooks are tied
+    @type hooks_path: string
+    @param hooks_path: prefix of the hooks directories
+    @type nodes: 2-tuple of lists
+    @param nodes: 2-tuple of lists containing nodes on which pre-hooks must be
+      run and nodes on which post-hooks must be run
+    @type hooks_execution_fn: function that accepts the following parameters:
+      (node_list, hooks_path, phase, environment)
+    @param hooks_execution_fn: function that will execute the hooks; can be
+      None, indicating that no conversion is necessary.
+    @type hooks_results_adapt_fn: function
+    @param hooks_results_adapt_fn: function that will adapt the return value of
+      hooks_execution_fn to the format expected by RunPhase
+    @type build_env_fn: function that returns a dictionary having strings as
+      keys
+    @param build_env_fn: function that builds the environment for the hooks
+    @type log_fn: function that accepts a string
+    @param log_fn: logging function
+    @type htype: string or None
+    @param htype: None or one of L{constants.HTYPE_CLUSTER},
+     L{constants.HTYPE_NODE}, L{constants.HTYPE_INSTANCE}
+    @type cluster_name: string
+    @param cluster_name: name of the cluster
+    @type master_name: string
+    @param master_name: name of the master
 
-  In order to remove the direct dependency on the rpc module, the
-  constructor needs a function which actually does the remote
-  call. This will usually be rpc.call_hooks_runner, but any function
-  which behaves the same works.
+    """
+    self.opcode = opcode
+    self.hooks_path = hooks_path
+    self.hooks_execution_fn = hooks_execution_fn
+    self.hooks_results_adapt_fn = hooks_results_adapt_fn
+    self.build_env_fn = build_env_fn
+    self.log_fn = log_fn
+    self.htype = htype
+    self.cluster_name = cluster_name
+    self.master_name = master_name
 
-  """
-  def __init__(self, callfn, lu):
-    self.callfn = callfn
-    self.lu = lu
-    self.op = lu.op
     self.pre_env = self._BuildEnv(constants.HOOKS_PHASE_PRE)
-
-    if self.lu.HPATH is None:
-      nodes = (None, None)
-    else:
-      nodes = map(frozenset, self.lu.BuildHooksNodes())
-
     (self.pre_nodes, self.post_nodes) = nodes
 
   def _BuildEnv(self, phase):
@@ -484,12 +527,13 @@ class HooksMaster(object):
 
     env = {}
 
-    if self.lu.HPATH is not None:
-      lu_env = self.lu.BuildHooksEnv()
-      if lu_env:
-        assert not compat.any(key.upper().startswith(prefix) for key in lu_env)
+    if self.hooks_path is not None:
+      phase_env = self.build_env_fn()
+      if phase_env:
+        assert not compat.any(key.upper().startswith(prefix)
+                              for key in phase_env)
         env.update(("%s%s" % (prefix, key), value)
-                   for (key, value) in lu_env.items())
+                   for (key, value) in phase_env.items())
 
     if phase == constants.HOOKS_PHASE_PRE:
       assert compat.all((key.startswith("GANETI_") and
@@ -512,26 +556,26 @@ class HooksMaster(object):
   def _RunWrapper(self, node_list, hpath, phase, phase_env):
     """Simple wrapper over self.callfn.
 
-    This method fixes the environment before doing the rpc call.
+    This method fixes the environment before executing the hooks.
 
     """
-    cfg = self.lu.cfg
-
     env = {
       "PATH": "/sbin:/bin:/usr/sbin:/usr/bin",
       "GANETI_HOOKS_VERSION": constants.HOOKS_VERSION,
-      "GANETI_OP_CODE": self.op.OP_ID,
+      "GANETI_OP_CODE": self.opcode,
       "GANETI_DATA_DIR": constants.DATA_DIR,
       "GANETI_HOOKS_PHASE": phase,
       "GANETI_HOOKS_PATH": hpath,
       }
 
-    if self.lu.HTYPE:
-      env["GANETI_OBJECT_TYPE"] = self.lu.HTYPE
+    if self.htype:
+      env["GANETI_OBJECT_TYPE"] = self.htype
+
+    if self.cluster_name is not None:
+      env["GANETI_CLUSTER"] = self.cluster_name
 
-    if cfg is not None:
-      env["GANETI_CLUSTER"] = cfg.GetClusterName()
-      env["GANETI_MASTER"] = cfg.GetMasterNode()
+    if self.master_name is not None:
+      env["GANETI_MASTER"] = self.master_name
 
     if phase_env:
       env = utils.algo.JoinDisjointDicts(env, phase_env)
@@ -542,12 +586,15 @@ class HooksMaster(object):
     assert compat.all(key == "PATH" or key.startswith("GANETI_")
                       for key in env)
 
-    return self.callfn(node_list, hpath, phase, env)
+    return self.hooks_execution_fn(node_list, hpath, phase, env)
 
   def RunPhase(self, phase, nodes=None):
     """Run all the scripts for a phase.
 
     This is the main function of the HookMaster.
+    It executes self.hooks_execution_fn, and after running
+    self.hooks_results_adapt_fn on its results it expects them to be in the form
+    {node_name: (fail_msg, [(script, result, output), ...]}).
 
     @param phase: one of L{constants.HOOKS_PHASE_POST} or
         L{constants.HOOKS_PHASE_PRE}; it denotes the hooks phase
@@ -574,36 +621,37 @@ class HooksMaster(object):
       # even attempt to run, or this LU doesn't do hooks at all
       return
 
-    results = self._RunWrapper(nodes, self.lu.HPATH, phase, env)
+    results = self._RunWrapper(nodes, self.hooks_path, phase, env)
     if not results:
       msg = "Communication Failure"
       if phase == constants.HOOKS_PHASE_PRE:
         raise errors.HooksFailure(msg)
       else:
-        self.lu.LogWarning(msg)
+        self.log_fn(msg)
         return results
 
+    converted_res = results
+    if self.hooks_results_adapt_fn:
+      converted_res = self.hooks_results_adapt_fn(results)
+
     errs = []
-    for node_name in results:
-      res = results[node_name]
-      if res.offline:
+    for node_name, (fail_msg, offline, hooks_results) in converted_res.items():
+      if offline:
         continue
 
-      msg = res.fail_msg
-      if msg:
-        self.lu.LogWarning("Communication failure to node %s: %s",
-                           node_name, msg)
+      if fail_msg:
+        self.log_fn("Communication failure to node %s: %s", node_name, fail_msg)
         continue
 
-      for script, hkr, output in res.payload:
+      for script, hkr, output in hooks_results:
         if hkr == constants.HKR_FAIL:
           if phase == constants.HOOKS_PHASE_PRE:
             errs.append((node_name, script, output))
           else:
             if not output:
               output = "(no output)"
-            self.lu.LogWarning("On %s script %s failed, output: %s" %
-                               (node_name, script, output))
+            self.log_fn("On %s script %s failed, output: %s" %
+                        (node_name, script, output))
 
     if errs and phase == constants.HOOKS_PHASE_PRE:
       raise errors.HooksAbort(errs)
@@ -619,5 +667,21 @@ class HooksMaster(object):
     """
     phase = constants.HOOKS_PHASE_POST
     hpath = constants.HOOKS_NAME_CFGUPDATE
-    nodes = [self.lu.cfg.GetMasterNode()]
+    nodes = [self.master_name]
     self._RunWrapper(nodes, hpath, phase, self.pre_env)
+
+  @staticmethod
+  def BuildFromLu(hooks_execution_fn, lu):
+    if lu.HPATH is None:
+      nodes = (None, None)
+    else:
+      nodes = map(frozenset, lu.BuildHooksNodes())
+
+    master_name = cluster_name = None
+    if lu.cfg:
+      master_name = lu.cfg.GetMasterNode()
+      cluster_name = lu.cfg.GetClusterName()
+
+    return HooksMaster(lu.op.OP_ID, lu.HPATH, nodes, hooks_execution_fn,
+                       _RpcResultsToHooksResults, lu.BuildHooksEnv,
+                       lu.LogWarning, lu.HTYPE, cluster_name, master_name)

diff --git a/test/ganeti.hooks_unittest.py b/test/ganeti.hooks_unittest.py
index 6a6a6b3..ddeb105 100755 (executable)
--- a/test/ganeti.hooks_unittest.py
+++ b/test/ganeti.hooks_unittest.py
@@ -249,14 +249,14 @@ class TestHooksMaster(unittest.TestCase):
 
   def testTotalFalse(self):
     """Test complete rpc failure"""
-    hm = mcpu.HooksMaster(self._call_false, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(self._call_false, self.lu)
     self.failUnlessRaises(errors.HooksFailure,
                           hm.RunPhase, constants.HOOKS_PHASE_PRE)
     hm.RunPhase(constants.HOOKS_PHASE_POST)
 
   def testIndividualFalse(self):
     """Test individual node failure"""
-    hm = mcpu.HooksMaster(self._call_nodes_false, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(self._call_nodes_false, self.lu)
     hm.RunPhase(constants.HOOKS_PHASE_PRE)
     #self.failUnlessRaises(errors.HooksFailure,
     #                      hm.RunPhase, constants.HOOKS_PHASE_PRE)
@@ -264,14 +264,14 @@ class TestHooksMaster(unittest.TestCase):
 
   def testScriptFalse(self):
     """Test individual rpc failure"""
-    hm = mcpu.HooksMaster(self._call_script_fail, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(self._call_script_fail, self.lu)
     self.failUnlessRaises(errors.HooksAbort,
                           hm.RunPhase, constants.HOOKS_PHASE_PRE)
     hm.RunPhase(constants.HOOKS_PHASE_POST)
 
   def testScriptSucceed(self):
     """Test individual rpc failure"""
-    hm = mcpu.HooksMaster(FakeHooksRpcSuccess, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(FakeHooksRpcSuccess, self.lu)
     for phase in (constants.HOOKS_PHASE_PRE, constants.HOOKS_PHASE_POST):
       hm.RunPhase(phase)
 
@@ -322,7 +322,7 @@ class TestHooksRunnerEnv(unittest.TestCase):
   def testEmptyEnv(self):
     # Check pre-phase hook
     self.lu.hook_env = {}
-    hm = mcpu.HooksMaster(self._HooksRpc, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(self._HooksRpc, self.lu)
     hm.RunPhase(constants.HOOKS_PHASE_PRE)
 
     (node_list, hpath, phase, env) = self._rpcs.pop(0)
@@ -348,7 +348,7 @@ class TestHooksRunnerEnv(unittest.TestCase):
     self.lu.hook_env = {
       "FOO": "pre-foo-value",
       }
-    hm = mcpu.HooksMaster(self._HooksRpc, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(self._HooksRpc, self.lu)
     hm.RunPhase(constants.HOOKS_PHASE_PRE)
 
     (node_list, hpath, phase, env) = self._rpcs.pop(0)
@@ -395,7 +395,7 @@ class TestHooksRunnerEnv(unittest.TestCase):
       self.lu.hook_env = { name: "value" }
 
       # Test using a clean HooksMaster instance
-      hm = mcpu.HooksMaster(self._HooksRpc, self.lu)
+      hm = mcpu.HooksMaster.BuildFromLu(self._HooksRpc, self.lu)
 
       for phase in [constants.HOOKS_PHASE_PRE, constants.HOOKS_PHASE_POST]:
         self.assertRaises(AssertionError, hm.RunPhase, phase)
@@ -403,7 +403,7 @@ class TestHooksRunnerEnv(unittest.TestCase):
 
   def testNoNodes(self):
     self.lu.hook_env = {}
-    hm = mcpu.HooksMaster(self._HooksRpc, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(self._HooksRpc, self.lu)
     hm.RunPhase(constants.HOOKS_PHASE_PRE, nodes=[])
     self.assertRaises(IndexError, self._rpcs.pop)
 
@@ -415,7 +415,7 @@ class TestHooksRunnerEnv(unittest.TestCase):
       "node93782.example.net",
       ]
 
-    hm = mcpu.HooksMaster(self._HooksRpc, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(self._HooksRpc, self.lu)
 
     for phase in [constants.HOOKS_PHASE_PRE, constants.HOOKS_PHASE_POST]:
       hm.RunPhase(phase, nodes=nodes)
@@ -433,7 +433,7 @@ class TestHooksRunnerEnv(unittest.TestCase):
       "FOO": "value",
       }
 
-    hm = mcpu.HooksMaster(self._HooksRpc, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(self._HooksRpc, self.lu)
     hm.RunConfigUpdate()
 
     (node_list, hpath, phase, env) = self._rpcs.pop(0)
@@ -452,7 +452,7 @@ class TestHooksRunnerEnv(unittest.TestCase):
       "FOO": "value",
       }
 
-    hm = mcpu.HooksMaster(self._HooksRpc, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(self._HooksRpc, self.lu)
     hm.RunPhase(constants.HOOKS_PHASE_POST)
 
     (node_list, hpath, phase, env) = self._rpcs.pop(0)
@@ -470,7 +470,7 @@ class TestHooksRunnerEnv(unittest.TestCase):
     self.assertRaises(AssertionError, self.lu.BuildHooksEnv)
     self.assertRaises(AssertionError, self.lu.BuildHooksNodes)
 
-    hm = mcpu.HooksMaster(self._HooksRpc, self.lu)
+    hm = mcpu.HooksMaster.BuildFromLu(self._HooksRpc, self.lu)
     self.assertEqual(hm.pre_env, {})
     self.assertRaises(IndexError, self._rpcs.pop)