On Wed, Nov 25, 2015 at 11:16 AM, Oleg Ponomarev <[email protected]>
wrote:
> 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):
>
What we usually do in the codebase is something like this:
def MockHooksExecutionFn(nodes, _hpath, _phase, _env)
It's better because we want the tests to break and have to be updated if
the signature of the function changes.
>
> """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):
>
We usually have a suffix for function variables: _fn. So get_node_id_fn
would be better.
Furthermore, why assign it a value of None instead of self._GetNodeId by
default?
This way you do not have to check for the None value later on, but just
invoke the function.
>
> """@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):
>
Same here as above.
>
> """@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]>[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]>
>> [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.
>
>
>
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.
