Minor nits, but the patch otherwise looks good. One possible improvement: we can consider asserting that all node_uuids passed are truly uuids whenever RunPhase is invoked. Or do you think this is too paranoid?
Patch title change: uuids but not node names On Tue, Nov 24, 2015 at 2:48 PM, 'Oleg Ponomarev' via ganeti-devel < [email protected]> wrote: > Currently hooksmaster is able to deal with both uuids and names. > Besides, in most cases, uuids are passed. In spite of it, the node list > While uuids are passed in most cases, the node list > has legacy naming *node_names* inside the hooksmaster implementation > uses the legacy name ... > which leads to misunderstandings. This patch modifies the existing code > to make it always pass node uuids to hooksmaster. Also the discouraging > It also renames the misleading *node_names* parameter to *node_uuids*. > *node_names* parameter renamed to *node_uuids*. > > Signed-off-by: Oleg Ponomarev <[email protected]> > --- > lib/cmdlib/cluster/__init__.py | 2 +- > lib/cmdlib/common.py | 6 +++--- > lib/hooksmaster.py | 31 ++++++++++++++++++------------- > test/py/cmdlib/cluster_unittest.py | 2 +- > test/py/ganeti.hooks_unittest.py | 2 +- > 5 files changed, 24 insertions(+), 19 deletions(-) > > diff --git a/lib/cmdlib/cluster/__init__.py > b/lib/cmdlib/cluster/__init__.py > index 951ad54..15631cb 100644 > --- a/lib/cmdlib/cluster/__init__.py > +++ b/lib/cmdlib/cluster/__init__.py > @@ -298,7 +298,7 @@ class LUClusterDestroy(LogicalUnit): > master_params = self.cfg.GetMasterNetworkParameters() > > # Run post hooks on master node before it's removed > - RunPostHook(self, self.cfg.GetNodeName(master_params.uuid)) > + RunPostHook(self, master_params.uuid) > > ems = self.cfg.GetUseExternalMipScript() > result = self.rpc.call_node_deactivate_master_ip(master_params.uuid, > diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py > index 696a331..edd00af 100644 > --- a/lib/cmdlib/common.py > +++ b/lib/cmdlib/common.py > @@ -179,16 +179,16 @@ def GetWantedInstances(lu, short_inst_names): > return (inst_uuids, [lu.cfg.GetInstanceName(uuid) for uuid in > inst_uuids]) > > > -def RunPostHook(lu, node_name): > +def RunPostHook(lu, node_uuid): > """Runs the post-hook for an opcode on a single node. > > """ > hm = lu.proc.BuildHooksManager(lu) > try: > - hm.RunPhase(constants.HOOKS_PHASE_POST, node_names=[node_name]) > + hm.RunPhase(constants.HOOKS_PHASE_POST, node_uuids=[node_uuid]) > except Exception, err: # pylint: disable=W0703 > lu.LogWarning("Errors occurred running hooks on %s: %s", > - node_name, err) > + node_uuid, err) > > > def RedistributeAncillaryFiles(lu): > diff --git a/lib/hooksmaster.py b/lib/hooksmaster.py > index c23d857..1289dfa 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): > """Base class for hooks masters. > > This class invokes the execution of hooks according to the behaviour > @@ -93,6 +94,8 @@ class HooksMaster(object): > @param cluster_name: name of the cluster > @type master_name: string > @param master_name: name of the master > + @type master_name: string > + @param master_name: uuid of the master > master_uuid rather than master_name? > > """ > self.opcode = opcode > @@ -105,6 +108,7 @@ class HooksMaster(object): > self.htype = htype > self.cluster_name = cluster_name > self.master_name = master_name > + self.master_uuid = master_uuid > > self.pre_env = self._BuildEnv(constants.HOOKS_PHASE_PRE) > (self.pre_nodes, self.post_nodes) = nodes > @@ -186,7 +190,7 @@ class HooksMaster(object): > > return self.hooks_execution_fn(node_list, hpath, phase, env) > > - def RunPhase(self, phase, node_names=None): > + def RunPhase(self, phase, node_uuids=None): > """Run all the scripts for a phase. > > This is the main function of the HookMaster. > @@ -196,7 +200,7 @@ class HooksMaster(object): > > @param phase: one of L{constants.HOOKS_PHASE_POST} or > L{constants.HOOKS_PHASE_PRE}; it denotes the hooks phase > - @param node_names: overrides the predefined list of nodes for the > given > + @param node_uuids: overrides the predefined list of nodes for the > given > phase > @return: the processed results of the hooks multi-node rpc call > @raise errors.HooksFailure: on communication failure to the nodes > @@ -204,25 +208,25 @@ class HooksMaster(object): > > """ > if phase == constants.HOOKS_PHASE_PRE: > - if node_names is None: > - node_names = self.pre_nodes > + if node_uuids is None: > + node_uuids = self.pre_nodes > env = self.pre_env > elif phase == constants.HOOKS_PHASE_POST: > - if node_names is None: > - node_names = self.post_nodes > - if node_names is not None and self.prepare_post_nodes_fn is not > None: > - node_names = > frozenset(self.prepare_post_nodes_fn(list(node_names))) > + if node_uuids is None: > + node_uuids = self.post_nodes > + if node_uuids is not None and self.prepare_post_nodes_fn is not > None: > + node_uuids = > frozenset(self.prepare_post_nodes_fn(list(node_uuids))) > env = self._BuildEnv(phase) > else: > raise AssertionError("Unknown phase '%s'" % phase) > > - if not node_names: > + if not node_uuids: > # 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) > + results = self._RunWrapper(node_uuids, self.hooks_path, phase, env) > if not results: > msg = "Communication Failure" > if phase == constants.HOOKS_PHASE_PRE: > @@ -268,7 +272,7 @@ class HooksMaster(object): > """ > phase = constants.HOOKS_PHASE_POST > hpath = constants.HOOKS_NAME_CFGUPDATE > - nodes = [self.master_name] > + nodes = [self.master_uuid] > self._RunWrapper(nodes, hpath, phase, self.pre_env) > > @staticmethod > @@ -285,9 +289,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) > diff --git a/test/py/cmdlib/cluster_unittest.py > b/test/py/cmdlib/cluster_unittest.py > index 24c1ca9..b77c87a 100644 > --- a/test/py/cmdlib/cluster_unittest.py > +++ b/test/py/cmdlib/cluster_unittest.py > @@ -232,7 +232,7 @@ class TestLUClusterDestroy(CmdlibTestCase): > > self.ExecOpCode(op) > > - self.assertSingleHooksCall([self.master.name], > + self.assertSingleHooksCall([self.master.uuid], > "cluster-destroy", > constants.HOOKS_PHASE_POST) > > diff --git a/test/py/ganeti.hooks_unittest.py b/test/py/ > ganeti.hooks_unittest.py > index ab7ddda..899c411 100755 > --- a/test/py/ganeti.hooks_unittest.py > +++ b/test/py/ganeti.hooks_unittest.py > @@ -414,7 +414,7 @@ class TestHooksRunnerEnv(unittest.TestCase): > def testNoNodes(self): > self.lu.hook_env = {} > hm = hooksmaster.HooksMaster.BuildFromLu(self._HooksRpc, self.lu) > - hm.RunPhase(constants.HOOKS_PHASE_PRE, node_names=[]) > + hm.RunPhase(constants.HOOKS_PHASE_PRE, node_uuids=[]) > self.assertRaises(IndexError, self._rpcs.pop) > > def testSpecificNodes(self): > -- > 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.
