On Tue, Nov 10, 2015 at 3:56 PM, 'Oleg Ponomarev' via ganeti-devel < [email protected]> wrote:
> Run PRE and POST global hooks in case of succesfull job execution on > s/full/ful/, both here and in the commit title. > the master node and on the node_list provided by the logical unit. > POST hooks always always executed with status SUCCEEDED. On errors > better: POST hooks are always executed with status equal to > POST hooks will not be executed because all the errors cause related > exeptions. > s/exeptions/exceptions/ Also, this description mixes up implementation details with behavior - I'd prefer only to say that hooks are not executed, and put a comment about exceptions in the code where it will be more useful. > > Also fix test which expects that hooks run only once. Now rpc called > twice: for usual hooks and for global hooks. Other test fix is that > currently the hooks path for the last hook is always 'global'. > > Signed-off-by: Oleg Ponomarev <[email protected]> > --- > lib/cmdlib/common.py | 2 ++ > lib/hooksmaster.py | 8 ++++++++ > lib/mcpu.py | 17 ++++++++++++----- > src/Ganeti/Constants.hs | 9 +++++++++ > test/py/cmdlib/cluster_unittest.py | 6 ++++-- > test/py/cmdlib/testsupport/cmdlib_testcase.py | 3 ++- > 6 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py > index 696a331..5b70e16 100644 > --- a/lib/cmdlib/common.py > +++ b/lib/cmdlib/common.py > @@ -186,6 +186,8 @@ def RunPostHook(lu, node_name): > hm = lu.proc.BuildHooksManager(lu) > try: > hm.RunPhase(constants.HOOKS_PHASE_POST, node_names=[node_name]) > + hm.RunPhase(constants.HOOKS_PHASE_POST, [node_name], True, > + constants.POST_HOOKS_STATUS_SUCCEEDED) > We always explicitly list the argument name for keyword arguments, as noted in the style guide. > 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 221e589..7c0f037 100644 > --- a/lib/hooksmaster.py > +++ b/lib/hooksmaster.py > @@ -226,6 +226,14 @@ class HooksMaster(object): > else: > raise AssertionError("Unknown phase '%s'" % phase) > > + if glob: > + if node_names is None or not node_names: > + node_names = frozenset([self.master_name]) > + elif self.master_name not in list(node_names): > Shouldn't frozenset should support the in operator? > + new_nodes = list(node_names) > + new_nodes.append(self.master_name) > + node_names = frozenset(new_nodes) > + > if not node_names: > # empty node list, we should not attempt to run this as either > # we're in the cluster init phase and the rpc client part can't > diff --git a/lib/mcpu.py b/lib/mcpu.py > index 209e859..9be8d46 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,9 @@ 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) > + self._hm.RunPhase(constants.HOOKS_PHASE_PRE, None, True) > Keyword argument names here, plus a comment about why two RunPhase invocations are here, please. > + h_results = self._hm.RunPhase(constants.HOOKS_PHASE_PRE) > lu.HooksCallBack(constants.HOOKS_PHASE_PRE, h_results, > self.Log, None) > > @@ -504,19 +506,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 +718,10 @@ class Processor(object): > > self._CheckLUResult(op, result) > > + if self._hm is not None: > + self._hm.RunPhase(constants.HOOKS_PHASE_POST, None, True, > + constants.POST_HOOKS_STATUS_SUCCEEDED) > Keyword args. Also, this is a good place to point out that exceptions of failed jobs will skip this invocation, resulting in the correct hook status. > + > return result > > def Log(self, *args): > diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs > index 180ff4d..93b817a 100644 > --- a/src/Ganeti/Constants.hs > +++ b/src/Ganeti/Constants.hs > @@ -729,6 +729,15 @@ hooksVersion = 2 > globalHooksDir :: String > globalHooksDir = "global" > > +postHooksStatusSucceeded :: String > +postHooksStatusSucceeded = "succeeded" > + > +postHooksStatusFailed :: String > +postHooksStatusFailed = "failed" > + > +postHooksStatusDisappeared :: String > +postHooksStatusDisappeared = "disappeared" > Should we make the statuses here match the job ones? "disappeared" has no match, but opcodes use "success" and "error" for similar outcomes. > + > -- * 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..31772e0 100644 > --- a/test/py/cmdlib/cluster_unittest.py > +++ b/test/py/cmdlib/cluster_unittest.py > @@ -232,8 +232,9 @@ class TestLUClusterDestroy(CmdlibTestCase): > > self.ExecOpCode(op) > > + # The hooksdir name should be "global" because of global hooks > self.assertSingleHooksCall([self.master.name], > - "cluster-destroy", > + constants.GLOBAL_HOOKS_DIR, > constants.HOOKS_PHASE_POST) > > > @@ -244,8 +245,9 @@ class TestLUClusterPostInit(CmdlibTestCase): > > self.ExecOpCode(op) > > + # The hooksdir name should be "global" because of global hooks > self.assertSingleHooksCall([self.master.uuid], > - "cluster-init", > + constants.GLOBAL_HOOKS_DIR, > constants.HOOKS_PHASE_POST) > > > diff --git a/test/py/cmdlib/testsupport/cmdlib_testcase.py > b/test/py/cmdlib/testsupport/cmdlib_testcase.py > index 4e459f3..bd96418 100644 > --- a/test/py/cmdlib/testsupport/cmdlib_testcase.py > +++ b/test/py/cmdlib/testsupport/cmdlib_testcase.py > @@ -362,8 +362,9 @@ class CmdlibTestCase(testutils.GanetiTestCase): > @see L{assertHooksCall} for parameter description. > > """ > + # Count below is set to 2 because both hooks ang global hooks will > run. > s/ang/and/ > self.assertHooksCall(nodes, hook_path, phase, > - environment=environment, count=1) > + environment=environment, count=2) > Since assertHooksCall is used just for the invocation of this function, please modify it so that it accepts a list of hook_paths of length equal to count, and checks that the paths are correctly invoked. Then provide ["global", hook_path], and don't just replace the hook_path which should be invoked with "global". > > def CopyOpCode(self, opcode, **kwargs): > """Creates a copy of the given opcode and applies modifications to it > -- > 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.
