What do you think about this interdiff?
diff --git a/lib/mcpu.py b/lib/mcpu.py
index 28033d8..d807861 100644
--- a/lib/mcpu.py
+++ b/lib/mcpu.py
@@ -721,7 +721,7 @@ class Processor(object):
# The post hooks below are always executed with a SUCCESS status
because
# all the possible errors during pre hooks and LU execution cause
- # exception and therefore the statement below will be skipped.
+ # exceptions and therefore the statement below will be skipped.
if self._hm is not None:
self._hm.RunPhase(constants.HOOKS_PHASE_POST, is_global=True,
post_status=constants.POST_HOOKS_STATUS_SUCCESS)
diff --git a/test/py/cmdlib/testsupport/rpc_runner_mock.py
b/test/py/cmdlib/testsupport/rpc_runner_mock.py
index 10e40b4..d183f41 100644
--- a/test/py/cmdlib/testsupport/rpc_runner_mock.py
+++ b/test/py/cmdlib/testsupport/rpc_runner_mock.py
@@ -38,16 +38,14 @@ from ganeti.rpc import node as rpc
from cmdlib.testsupport.util import patchModule
-
-# Disable 'Unused argument' because it's a mock function
-# pylint: disable=W0613
-def MockHooksExecutionFn(nodes, hpath, phase, env):
+# We don't need arguments other than nodes in this mock.
+def MockHooksExecutionFn(nodes, *unused):
"""Helper function that generate rpc results for call_hooks_runner mock
"""
results = RpcResultsBuilder()
for node in nodes:
- results.AddSuccessfulNode(node, data=None, node_id_passed=True)
+ results.AddSuccessfulNode(node, data=None, get_node_id=lambda nid: nid)
return results.Build()
@@ -119,7 +117,7 @@ class RpcResultsBuilder(object):
else:
return node.uuid
- def CreateSuccessfulNodeResult(self, node, data=None,
node_id_passed=False):
+ def CreateSuccessfulNodeResult(self, node, data=None, get_node_id=None):
"""@see L{RpcResultsBuilder}
@param node: @see L{RpcResultsBuilder}.
@@ -129,8 +127,8 @@ class RpcResultsBuilder(object):
"""
if data is None:
data = {}
- res_node = node if node_id_passed else self._GetNodeId(node)
- return rpc.RpcResult(data=(True, data), node=res_node)
+ node_id = get_node_id(node) if get_node_id else self._GetNodeId(node)
+ return rpc.RpcResult(data=(True, data), node=node_id)
def CreateFailedNodeResult(self, node):
"""@see L{RpcResultsBuilder}
@@ -158,7 +156,7 @@ class RpcResultsBuilder(object):
"""
return rpc.RpcResult(data=(False, error_msg),
node=self._GetNodeId(node))
- def AddSuccessfulNode(self, node, data=None, node_id_passed=False):
+ def AddSuccessfulNode(self, node, data=None, get_node_id=None):
"""@see L{CreateSuccessfulNode}
@rtype: L{RpcResultsBuilder}
@@ -166,7 +164,7 @@ class RpcResultsBuilder(object):
"""
self._results.append(self.CreateSuccessfulNodeResult(node, data,
- node_id_passed))
+ get_node_id))
return self
def AddFailedNode(self, node):
diff --git a/test/py/ganeti.hooks_unittest.py
b/test/py/ganeti.hooks_unittest.py
index 07b7aba..6bf2371 100755
--- a/test/py/ganeti.hooks_unittest.py
+++ b/test/py/ganeti.hooks_unittest.py
@@ -224,7 +224,7 @@ class TestHooksMaster(unittest.TestCase):
"""Testing case for HooksMaster"""
def _call_fail(*args):
- """Fake call_hooks_runner function which returns False."""
+ """Fake call_hooks_runner which returns an empty result dictionary."""
return {}
@staticmethod
On 11/24/2015 07:51 PM, Hrvoje Ribicic wrote:
s/successfull/successful/ in the title of the patch.
On Fri, Nov 20, 2015 at 5:11 PM, 'Oleg Ponomarev' via ganeti-devel
<[email protected] <mailto:[email protected]>>
wrote:
Run PRE and POST global hooks in case of succesful job execution on
successful
the master node and on the node_list provided by the logical unit.
POST hooks are always executed with status equal to SUCCESS.
Also fix the python tests:
- as currently at least global hooks are always executed and
hooksmaster account the rpc results, call_hooks_runner mock
modified
s/account/checks/ , if that's what you meant?
in order to always return successful results;
- the tests ensuring that hooks are executed only once are now
useless
the helper function ensuring ... is now ...
because of global hooks. They are replaced by the separate tests per
each hooks execution.
a separate test per each hook execution.
Signed-off-by: Oleg Ponomarev <[email protected]
<mailto:[email protected]>>
---
lib/cmdlib/common.py | 3 +++
lib/hooksmaster.py | 5 ++---
lib/mcpu.py | 22
+++++++++++++++++-----
src/Ganeti/Constants.hs | 9 +++++++++
test/py/cmdlib/cluster_unittest.py | 18 ++++++++++++------
test/py/cmdlib/testsupport/cmdlib_testcase.py | 19
++-----------------
test/py/cmdlib/testsupport/rpc_runner_mock.py | 23
+++++++++++++++++++----
test/py/ganeti.hooks_unittest.py
<http://ganeti.hooks_unittest.py> | 6 +++---
8 files changed, 67 insertions(+), 38 deletions(-)
diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py
index 696a331..04ba7a3 100644
--- a/lib/cmdlib/common.py
+++ b/lib/cmdlib/common.py
@@ -185,7 +185,10 @@ def RunPostHook(lu, node_name):
"""
hm = lu.proc.BuildHooksManager(lu)
try:
+ # Execute usual post hooks, then global post hooks.
hm.RunPhase(constants.HOOKS_PHASE_POST, node_names=[node_name])
+ hm.RunPhase(constants.HOOKS_PHASE_POST, [node_name],
is_global=True,
+ post_status=constants.POST_HOOKS_STATUS_SUCCESS)
Nice improvement, but isn't node_names (node_uuids) an optional
parameter as well?
If nothing else, we should change it for consistency with the
statement above :)
except Exception, err: # pylint: disable=W0703
lu.LogWarning("Errors occurred running hooks on %s: %s",
node_name, err)
diff --git a/lib/hooksmaster.py b/lib/hooksmaster.py
index 1472ea7..1326dc0 100644
--- a/lib/hooksmaster.py
+++ b/lib/hooksmaster.py
@@ -202,12 +202,11 @@ class HooksMaster(object):
ret = dict()
if is_global:
env["GANETI_IS_MASTER"] = constants.GLOBAL_HOOKS_MASTER
- master_set = set([self.master_name])
- ret.update(self.hooks_execution_fn(master_set, hpath,
phase, env))
+ ret.update(self.hooks_execution_fn([self.master_name], hpath,
phase, env))
if node_list:
# Master node might be either listed by the uuid or by
the name
- master_set.add(self.master_uuid)
+ master_set = frozenset([self.master_uuid, self.master_name])
node_list = frozenset(set(node_list) - master_set)
if not node_list:
return ret
diff --git a/lib/mcpu.py b/lib/mcpu.py
index 209e859..8bb7969 100644
--- a/lib/mcpu.py
+++ b/lib/mcpu.py
@@ -305,6 +305,7 @@ class Processor(object):
self.cfg = context.GetConfig(ec_id)
self.rpc = context.GetRpc(self.cfg)
self.hmclass = hooksmaster.HooksMaster
+ self._hm = None
self._enable_locks = enable_locks
self.wconfd = wconfd # Indirection to allow testing
self._wconfdcontext = context.GetWConfdContext(ec_id)
@@ -483,8 +484,11 @@ class Processor(object):
lu.cfg.OutDate()
lu.CheckPrereq()
- hm = self.BuildHooksManager(lu)
- h_results = hm.RunPhase(constants.HOOKS_PHASE_PRE)
+ self._hm = self.BuildHooksManager(lu)
+ # Run hooks twice: first for the global hooks, then for the
usual hooks.
+ self._hm.RunPhase(constants.HOOKS_PHASE_PRE, node_names=None,
+ is_global=True)
+ h_results = self._hm.RunPhase(constants.HOOKS_PHASE_PRE)
lu.HooksCallBack(constants.HOOKS_PHASE_PRE, h_results,
self.Log, None)
@@ -504,19 +508,20 @@ class Processor(object):
lusExecuting[0] += 1
try:
result = _ProcessResult(submit_mj_fn, lu.op, lu.Exec(self.Log))
- h_results = hm.RunPhase(constants.HOOKS_PHASE_POST)
+ h_results = self._hm.RunPhase(constants.HOOKS_PHASE_POST)
result = lu.HooksCallBack(constants.HOOKS_PHASE_POST,
h_results,
self.Log, result)
finally:
# FIXME: This needs locks if not lu_class.REQ_BGL
lusExecuting[0] -= 1
if write_count != self.cfg.write_count:
- hm.RunConfigUpdate()
+ self._hm.RunConfigUpdate()
return result
def BuildHooksManager(self, lu):
- return self.hmclass.BuildFromLu(lu.rpc.call_hooks_runner, lu)
+ return self.hmclass.BuildFromLu(lu.rpc.call_hooks_runner, lu,
+ self.GetECId())
def _LockAndExecLU(self, lu, level, calc_timeout, pending=None):
"""Execute a Logical Unit, with the needed locks.
@@ -715,6 +720,13 @@ class Processor(object):
self._CheckLUResult(op, result)
+ # The post hooks below are always executed with a SUCCESS
status because
+ # all the possible errors during pre hooks and LU execution cause
+ # exception and therefore the statement below will be skipped.
Nit: exceptions
+ if self._hm is not None:
+ self._hm.RunPhase(constants.HOOKS_PHASE_POST, node_names=None,
+ is_global=True,
+ post_status=constants.POST_HOOKS_STATUS_SUCCESS)
return result
def Log(self, *args):
diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs
index 1f96c0d..1dddefe 100644
--- a/src/Ganeti/Constants.hs
+++ b/src/Ganeti/Constants.hs
@@ -735,6 +735,15 @@ globalHooksMaster = "master"
globalHooksNotMaster :: String
globalHooksNotMaster = "not_master"
+postHooksStatusSuccess :: String
+postHooksStatusSuccess = "success"
+
+postHooksStatusError :: String
+postHooksStatusError = "error"
+
+postHooksStatusDisappear :: String
+postHooksStatusDisappear = "disappear"
+
-- * Hooks subject type (what object type does the LU deal with)
htypeCluster :: String
diff --git a/test/py/cmdlib/cluster_unittest.py
b/test/py/cmdlib/cluster_unittest.py
index 24c1ca9..8124555 100644
--- a/test/py/cmdlib/cluster_unittest.py
+++ b/test/py/cmdlib/cluster_unittest.py
@@ -232,9 +232,12 @@ class TestLUClusterDestroy(CmdlibTestCase):
self.ExecOpCode(op)
- self.assertSingleHooksCall([self.master.name
<http://self.master.name>],
- "cluster-destroy",
- constants.HOOKS_PHASE_POST)
+ self.assertHooksCall([self.master.name
<http://self.master.name>], constants.GLOBAL_HOOKS_DIR,
+ constants.HOOKS_PHASE_PRE, index=0)
+ self.assertHooksCall([self.master.name
<http://self.master.name>], "cluster-destroy",
+ constants.HOOKS_PHASE_POST, index=1)
+ self.assertHooksCall([self.master.name
<http://self.master.name>], constants.GLOBAL_HOOKS_DIR,
+ constants.HOOKS_PHASE_POST, index=2)
class TestLUClusterPostInit(CmdlibTestCase):
@@ -244,9 +247,12 @@ class TestLUClusterPostInit(CmdlibTestCase):
self.ExecOpCode(op)
- self.assertSingleHooksCall([self.master.uuid],
- "cluster-init",
- constants.HOOKS_PHASE_POST)
+ self.assertHooksCall([self.master.name
<http://self.master.name>], constants.GLOBAL_HOOKS_DIR,
+ constants.HOOKS_PHASE_PRE, index=0)
+ self.assertHooksCall([self.master.uuid], "cluster-init",
+ constants.HOOKS_PHASE_POST, index=1)
+ self.assertHooksCall([self.master.name
<http://self.master.name>], constants.GLOBAL_HOOKS_DIR,
+ constants.HOOKS_PHASE_POST, index=2)
class TestLUClusterQuery(CmdlibTestCase):
diff --git a/test/py/cmdlib/testsupport/cmdlib_testcase.py
b/test/py/cmdlib/testsupport/cmdlib_testcase.py
index 4e459f3..f98b751 100644
--- a/test/py/cmdlib/testsupport/cmdlib_testcase.py
+++ b/test/py/cmdlib/testsupport/cmdlib_testcase.py
@@ -326,7 +326,7 @@ class CmdlibTestCase(testutils.GanetiTestCase):
self.mcpu.assertLogContainsRegex(expected_regex)
def assertHooksCall(self, nodes, hook_path, phase,
- environment=None, count=None, index=0):
+ environment=None, index=0):
"""Asserts a call to C{rpc.call_hooks_runner}
@type nodes: list of string
@@ -338,16 +338,11 @@ class CmdlibTestCase(testutils.GanetiTestCase):
@type environment: dict
@param environment: the environment passed to the hooks.
C{None} to skip
asserting it
- @type count: int
- @param count: the number of hook invocations. C{None} to skip
asserting it
@type index: int
@param index: the index of the hook invocation to assert
"""
- if count is not None:
- self.assertEqual(count, self.rpc.call_hooks_runner.call_count)
-
- args = self.rpc.call_hooks_runner.call_args[index]
+ args = self.rpc.call_hooks_runner.mock_calls[index][1]
self.assertEqual(set(nodes), set(args[0]))
self.assertEqual(hook_path, args[1])
@@ -355,16 +350,6 @@ class CmdlibTestCase(testutils.GanetiTestCase):
if environment is not None:
self.assertEqual(environment, args[3])
- def assertSingleHooksCall(self, nodes, hook_path, phase,
- environment=None):
- """Asserts a single call to C{rpc.call_hooks_runner}
-
- @see L{assertHooksCall} for parameter description.
-
- """
- self.assertHooksCall(nodes, hook_path, phase,
- environment=environment, count=1)
-
def CopyOpCode(self, opcode, **kwargs):
"""Creates a copy of the given opcode and applies
modifications to it
diff --git a/test/py/cmdlib/testsupport/rpc_runner_mock.py
b/test/py/cmdlib/testsupport/rpc_runner_mock.py
index 9658963..10e40b4 100644
--- a/test/py/cmdlib/testsupport/rpc_runner_mock.py
+++ b/test/py/cmdlib/testsupport/rpc_runner_mock.py
@@ -39,11 +39,24 @@ from ganeti.rpc import node as rpc
from cmdlib.testsupport.util import patchModule
+# Disable 'Unused argument' because it's a mock function
+# pylint: disable=W0613
Rather than disabling the warning, prepend the unused argument with _,
that should fix it as well.
+def MockHooksExecutionFn(nodes, hpath, phase, env):
+ """Helper function that generate rpc results for
call_hooks_runner mock
+
+ """
+ results = RpcResultsBuilder()
+ for node in nodes:
+ results.AddSuccessfulNode(node, data=None, node_id_passed=True)
+ return results.Build()
+
+
def CreateRpcRunnerMock():
"""Creates a new L{mock.MagicMock} tailored for L{rpc.RpcRunner}
"""
ret = mock.MagicMock(spec=rpc.RpcRunner)
+ ret.call_hooks_runner.side_effect = MockHooksExecutionFn
return ret
@@ -106,7 +119,7 @@ class RpcResultsBuilder(object):
else:
return node.uuid
- def CreateSuccessfulNodeResult(self, node, data=None):
+ def CreateSuccessfulNodeResult(self, node, data=None,
node_id_passed=False):
Rather than passing this boolean parameter in, consider making the
_GetNodeId function mockable, and passing in a lambda function
returning the node.
"""@see L{RpcResultsBuilder}
@param node: @see L{RpcResultsBuilder}.
@@ -116,7 +129,8 @@ class RpcResultsBuilder(object):
"""
if data is None:
data = {}
- return rpc.RpcResult(data=(True, data),
node=self._GetNodeId(node))
+ res_node = node if node_id_passed else self._GetNodeId(node)
+ return rpc.RpcResult(data=(True, data), node=res_node)
def CreateFailedNodeResult(self, node):
"""@see L{RpcResultsBuilder}
@@ -144,14 +158,15 @@ class RpcResultsBuilder(object):
"""
return rpc.RpcResult(data=(False, error_msg),
node=self._GetNodeId(node))
- def AddSuccessfulNode(self, node, data=None):
+ def AddSuccessfulNode(self, node, data=None, node_id_passed=False):
"""@see L{CreateSuccessfulNode}
@rtype: L{RpcResultsBuilder}
@return: self for chaining
"""
- self._results.append(self.CreateSuccessfulNodeResult(node, data))
+ self._results.append(self.CreateSuccessfulNodeResult(node, data,
+ node_id_passed))
return self
def AddFailedNode(self, node):
diff --git a/test/py/ganeti.hooks_unittest.py
<http://ganeti.hooks_unittest.py>
b/test/py/ganeti.hooks_unittest.py <http://ganeti.hooks_unittest.py>
index ab7ddda..3de3bfa 100755
--- a/test/py/ganeti.hooks_unittest.py
<http://ganeti.hooks_unittest.py>
+++ b/test/py/ganeti.hooks_unittest.py
<http://ganeti.hooks_unittest.py>
@@ -222,9 +222,9 @@ def FakeHooksRpcSuccess(node_list, hpath,
phase, env):
class TestHooksMaster(unittest.TestCase):
"""Testing case for HooksMaster"""
- def _call_false(*args):
+ def _call_fail(*args):
"""Fake call_hooks_runner function which returns False."""
It does not return False anymore ;
- return False
+ return {}
@staticmethod
def _call_nodes_false(node_list, hpath, phase, env):
@@ -259,7 +259,7 @@ class TestHooksMaster(unittest.TestCase):
def testTotalFalse(self):
"""Test complete rpc failure"""
- hm = hooksmaster.HooksMaster.BuildFromLu(self._call_false,
self.lu <http://self.lu>)
+ hm = hooksmaster.HooksMaster.BuildFromLu(self._call_fail,
self.lu <http://self.lu>)
I have to wonder how that ever worked...
self.failUnlessRaises(errors.HooksFailure,
hm.RunPhase, constants.HOOKS_PHASE_PRE)
hm.RunPhase(constants.HOOKS_PHASE_POST)
--
2.6.0.rc2.230.g3dd15c0
Hrvoje Ribicic
Ganeti Engineering
Google Germany GmbH
Dienerstr. 12, 80331, München
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
sind, leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.
