Looks a lot better, just a few more minor things!
On Tue, Nov 24, 2015 at 2:45 PM, Oleg Ponomarev <[email protected]>
wrote:
> Interdiff for the hooksmaster:
>
> diff --git a/lib/hooksmaster.py b/lib/hooksmaster.py
> index d1b4adb..d2de4b8 100644
> --- a/lib/hooksmaster.py
> +++ b/lib/hooksmaster.py
> @@ -158,6 +158,17 @@ class HooksMaster(object):
>
> return env
>
> + def _CheckParamsAndExecHooks(self, node_list, hpath, phase, env):
>
While I'm ok with omitting docstring parameter descriptions for a helper
function invoked from a well-documented function, please add a one-line
description here (which will take three lines because of our style
restrictions).
> + if node_list is None or not node_list:
> + return
> +
> + # Convert everything to strings
> + env = dict([(str(key), str(val)) for key, val in env.iteritems()])
> + assert compat.all(key == "PATH" or key.startswith("GANETI_")
> + for key in env)
> +
> + return self.hooks_execution_fn(node_list, hpath, phase, env)
> +
> def _RunWrapper(self, node_list, hpath, phase, phase_env,
> is_global=False,
> post_status=None):
> """Simple wrapper over self.callfn.
> @@ -192,26 +203,18 @@ class HooksMaster(object):
> if phase_env:
> env = utils.algo.JoinDisjointDicts(env, phase_env)
>
> - # Convert everything to strings
> - env = dict([(str(key), str(val)) for key, val in env.iteritems()])
> -
> - assert compat.all(key == "PATH" or key.startswith("GANETI_")
> - for key in env)
> + if not is_global:
> + return self._CheckParamsAndExecHooks(node_list, hpath, phase, env)
>
>
One-line comment here, on the lines of:
"For global hooks, we need to send different env values to master and the
others"
> ret = dict()
> - if is_global:
> - env["GANETI_IS_MASTER"] = constants.GLOBAL_HOOKS_MASTER
> - master_set = frozenset([self.master_uuid])
> - ret.update(self.hooks_execution_fn(master_set, hpath, phase, env))
> -
> - if node_list:
> - node_list = frozenset(set(node_list) - master_set)
> - if not node_list:
> - return ret
> - env["GANETI_IS_MASTER"] = constants.GLOBAL_HOOKS_NOT_MASTER
> + env["GANETI_IS_MASTER"] = constants.GLOBAL_HOOKS_MASTER
> + master_set = frozenset([self.master_uuid])
> + ret.update(self._CheckParamsAndExecHooks(master_set, hpath, phase,
> env))
>
> if node_list:
> - ret.update(self.hooks_execution_fn(node_list, hpath, phase, env))
> + node_list = frozenset(set(node_list) - master_set)
> + env["GANETI_IS_MASTER"] = constants.GLOBAL_HOOKS_NOT_MASTER
> + ret.update(self._CheckParamsAndExecHooks(node_list, hpath, phase,
> env))
>
Empty line here to match formatting of paragraph above?
> return ret
>
> def RunPhase(self, phase, node_uuids=None, is_global=False,
>
> On 11/23/2015 06:21 PM, Hrvoje Ribicic wrote:
>
>
>
> On Fri, Nov 20, 2015 at 5:11 PM, 'Oleg Ponomarev' via ganeti-devel <
> <[email protected]>[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]>
>> [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.
>
>
>
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.
