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]> 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]> > --- > 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 | 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], > - "cluster-destroy", > - constants.HOOKS_PHASE_POST) > + self.assertHooksCall([self.master.name], constants.GLOBAL_HOOKS_DIR, > + constants.HOOKS_PHASE_PRE, index=0) > + self.assertHooksCall([self.master.name], "cluster-destroy", > + constants.HOOKS_PHASE_POST, index=1) > + self.assertHooksCall([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], 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], 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 b/test/py/ > ganeti.hooks_unittest.py > index ab7ddda..3de3bfa 100755 > --- a/test/py/ganeti.hooks_unittest.py > +++ b/test/py/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) > + hm = hooksmaster.HooksMaster.BuildFromLu(self._call_fail, 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.
