On Fri, Nov 20, 2015 at 5:11 PM, 'Oleg Ponomarev' via ganeti-devel < [email protected]> wrote:
> Add the *glob* argument to the RunPhase function. With *glob* set to > True, HooksMaster runs global hooks instead of the opcode's hooks. The > global hooks should be placed in the "global" subdirectory of the hooks > directory. > Thanks for the rename of "glob" - please modify the commit message as well. > > Global hooks run separately for the master node and for the others because enviromental variable IS_MASTER should be set properly. > As more than one global hook can be triggered by the same Ganeti job in a very unpredictable way, we set the IS_MASTER environmental variable to allow hooks to make sure that exactly one action gets executed per hook activation. > > Signed-off-by: Oleg Ponomarev <[email protected]> > --- > lib/hooksmaster.py | 55 > +++++++++++++++++++++++++++++++++++++++++-------- > src/Ganeti/Constants.hs | 11 ++++++++++ > 2 files changed, 57 insertions(+), 9 deletions(-) > > diff --git a/lib/hooksmaster.py b/lib/hooksmaster.py > index c23d857..1472ea7 100644 > --- a/lib/hooksmaster.py > +++ b/lib/hooksmaster.py > @@ -56,7 +56,8 @@ def _RpcResultsToHooksResults(rpc_results): > class HooksMaster(object): > def __init__(self, opcode, hooks_path, nodes, hooks_execution_fn, > hooks_results_adapt_fn, build_env_fn, > prepare_post_nodes_fn, > - log_fn, htype=None, cluster_name=None, master_name=None): > + log_fn, htype=None, cluster_name=None, master_name=None, > + master_uuid=None, job_id=None): > """Base class for hooks masters. > > This class invokes the execution of hooks according to the behaviour > @@ -93,6 +94,11 @@ class HooksMaster(object): > @param cluster_name: name of the cluster > @type master_name: string > @param master_name: name of the master > + @type master_uuid: string > + @param master_uuid: uuid of the master (used in global hooks in order > + to distinguish between the master node and the others) > + @type job_id: int > + @param job_id: the id of the job process (used in global post hooks) > > """ > self.opcode = opcode > @@ -105,6 +111,8 @@ class HooksMaster(object): > self.htype = htype > self.cluster_name = cluster_name > self.master_name = master_name > + self.master_uuid = master_uuid > + self.job_id = job_id > > self.pre_env = self._BuildEnv(constants.HOOKS_PHASE_PRE) > (self.pre_nodes, self.post_nodes) = nodes > @@ -151,7 +159,8 @@ class HooksMaster(object): > > return env > > - def _RunWrapper(self, node_list, hpath, phase, phase_env): > + def _RunWrapper(self, node_list, hpath, phase, phase_env, > is_global=False, > + post_status=None): """Simple wrapper over self.callfn. > > This method fixes the environment before executing the hooks. > @@ -175,6 +184,12 @@ class HooksMaster(object): > if self.master_name is not None: > env["GANETI_MASTER"] = self.master_name > > + if self.job_id and is_global: > + env["GANETI_JOB_ID"] = self.job_id > + if phase == constants.HOOKS_PHASE_POST and is_global: > + assert post_status is not None > + env["GANETI_POST_STATUS"] = post_status > + > if phase_env: > env = utils.algo.JoinDisjointDicts(env, phase_env) > > @@ -184,9 +199,26 @@ class HooksMaster(object): > assert compat.all(key == "PATH" or key.startswith("GANETI_") > for key in env) > The assert being triggered before the env finalization is a bit messy. > > - return self.hooks_execution_fn(node_list, hpath, phase, env) > - > - def RunPhase(self, phase, node_names=None): > + 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)) > + > + if node_list: > + # Master node might be either listed by the uuid or by the name > How come? Where do the different value types come from? I would rather fix this at the source rather than here. > + master_set.add(self.master_uuid) > + node_list = frozenset(set(node_list) - master_set) > + if not node_list: > + return ret > + env["GANETI_IS_MASTER"] = constants.GLOBAL_HOOKS_NOT_MASTER Urgh, a sideeffect of this code spilling over to the next invocation is a bit yucky. > + > + if node_list: > + ret.update(self.hooks_execution_fn(node_list, hpath, phase, env)) + return ret > I have to admit I do not like the way this code is structured. I would suggest using a local helper function that will operate on a set of nodes, set the environmental value if is_global is used, update ret, and optionally not invoke the execution function if the set of nodes is empty - we can also leave that bit to the execution function. This would allow us to separate the set calculation from the invocation, resulting in a more readable execution flow. > + > + def RunPhase(self, phase, node_names=None, is_global=False, > + post_status=None): > """Run all the scripts for a phase. > > This is the main function of the HookMaster. > @@ -198,6 +230,8 @@ class HooksMaster(object): > L{constants.HOOKS_PHASE_PRE}; it denotes the hooks phase > @param node_names: overrides the predefined list of nodes for the > given > phase > + @param is_global: whether global or per-opcode hooks should be > executed > + @param post_status: the job execution status for the global post hooks > for global post hooks Please add a @type declaration as well, as the type is not apparent from the code. > @return: the processed results of the hooks multi-node rpc call > @raise errors.HooksFailure: on communication failure to the nodes > @raise errors.HooksAbort: on failure of one of the hooks > @@ -216,13 +250,15 @@ class HooksMaster(object): > else: > raise AssertionError("Unknown phase '%s'" % phase) > > - if not node_names: > + if not node_names and not is_global: > # 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 > # even attempt to run, or this LU doesn't do hooks at all > return > > - results = self._RunWrapper(node_names, self.hooks_path, phase, env) > + hooks_path = constants.GLOBAL_HOOKS_DIR if is_global else > self.hooks_path > + results = self._RunWrapper(node_names, hooks_path, phase, env, > is_global, > + post_status) > if not results: > msg = "Communication Failure" > if phase == constants.HOOKS_PHASE_PRE: > @@ -272,7 +308,7 @@ class HooksMaster(object): > self._RunWrapper(nodes, hpath, phase, self.pre_env) > > @staticmethod > - def BuildFromLu(hooks_execution_fn, lu): > + def BuildFromLu(hooks_execution_fn, lu, job_id=None): > if lu.HPATH is None: > nodes = (None, None) > else: > @@ -285,9 +321,10 @@ class HooksMaster(object): > master_name = cluster_name = None > if lu.cfg: > master_name = lu.cfg.GetMasterNodeName() > + master_uuid = lu.cfg.GetMasterNode() > cluster_name = lu.cfg.GetClusterName() > > return HooksMaster(lu.op.OP_ID, lu.HPATH, nodes, hooks_execution_fn, > _RpcResultsToHooksResults, lu.BuildHooksEnv, > lu.PreparePostHookNodes, lu.LogWarning, lu.HTYPE, > - cluster_name, master_name) > + cluster_name, master_name, master_uuid, job_id) > diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs > index 86c8c95..1f96c0d 100644 > --- a/src/Ganeti/Constants.hs > +++ b/src/Ganeti/Constants.hs > @@ -724,6 +724,17 @@ hooksPhasePre = "pre" > hooksVersion :: Int > hooksVersion = 2 > > +-- * Global hooks related constants > + > +globalHooksDir :: String > +globalHooksDir = "global" > + > +globalHooksMaster :: String > +globalHooksMaster = "master" > + > +globalHooksNotMaster :: String > +globalHooksNotMaster = "not_master" > + > -- * Hooks subject type (what object type does the LU deal with) > > htypeCluster :: String > -- > 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.
