On Tue, May 03, 2011 at 03:22:08PM +0100, Adeodato Simo wrote:
> This commit introduces no behavior changes, and is only a minor refactoring
> that aids with a cleaner division of future LUClusterVerify work. The
> change consists in:
>
> - substitute the {node,instance}{list,info} structures previously created
> in Exec() by member variables created in CheckPrereq; and
>
> - mechanically convert all references to the old variables to the new
> member variables.
>
> Creating both self.all_{node,inst}_info and self.my_{node,inst}_info, both
> with the same contents at the moment, is not capricious. We've now made
> Exec use the my_* variables pervasively; in future commits, we'll break the
> assumption that all nodes and instances are listed there, and it'll become
> clear when the all_* variables have to be substituted instead.
>
> Signed-off-by: Adeodato Simo <[email protected]>
> ---
> lib/cmdlib.py | 76 +++++++++++++++++++++++++++++++++-----------------------
> 1 files changed, 45 insertions(+), 31 deletions(-)
>
> diff --git a/lib/cmdlib.py b/lib/cmdlib.py
> index bf87cc5..028aa4d 100644
> --- a/lib/cmdlib.py
> +++ b/lib/cmdlib.py
> @@ -1390,6 +1390,22 @@ class LUClusterVerify(LogicalUnit):
> }
> self.share_locks = dict.fromkeys(locking.LEVELS, 1)
>
> + def CheckPrereq(self):
> + self.all_node_info = self.cfg.GetAllNodesInfo()
> + self.all_inst_info = self.cfg.GetAllInstancesInfo()
> +
> + self.my_node_names = utils.NiceSort([n.name
> + for n in
> self.all_node_info.values()])
> +
> + self.my_node_info = dict((name, self.all_node_info[name])
> + for name in self.my_node_names)
Uh, how is my_node_info different from all_node_info?
> +
> + self.my_inst_names = utils.NiceSort([i.name
> + for i in
> self.all_inst_info.values()])
> +
> + self.my_inst_info = dict((name, self.all_inst_info[name])
> + for name in self.my_inst_names)
Same here.
> def _Error(self, ecode, item, msg, *args, **kwargs):
> """Format an error message.
>
> @@ -2202,14 +2218,12 @@ class LUClusterVerify(LogicalUnit):
> the output be logged in the verify output and the verification to fail.
>
> """
> - cfg = self.cfg
> -
> env = {
> - "CLUSTER_TAGS": " ".join(cfg.GetClusterInfo().GetTags())
> + "CLUSTER_TAGS": " ".join(self.cfg.GetClusterInfo().GetTags())
> }
>
> env.update(("NODE_TAGS_%s" % node.name, " ".join(node.GetTags()))
> - for node in cfg.GetAllNodesInfo().values())
> + for node in self.my_node_info.values())
>
> return env
>
> @@ -2217,7 +2231,7 @@ class LUClusterVerify(LogicalUnit):
> """Build hooks nodes.
>
> """
> - return ([], self.cfg.GetNodeList())
> + return ([], self.my_node_names)
This requires that hooks are always computed after check prereq. Maybe
add an assert here?
> def Exec(self, feedback_fn):
> """Verify integrity of cluster, performing various test on nodes.
> @@ -2241,13 +2255,9 @@ class LUClusterVerify(LogicalUnit):
> drbd_helper = self.cfg.GetDRBDHelper()
> hypervisors = self.cfg.GetClusterInfo().enabled_hypervisors
> cluster = self.cfg.GetClusterInfo()
> - nodelist = utils.NiceSort(self.cfg.GetNodeList())
> - nodeinfo = [self.cfg.GetNodeInfo(nname) for nname in nodelist]
> - nodeinfo_byname = dict(zip(nodelist, nodeinfo))
> - instancelist = utils.NiceSort(self.cfg.GetInstanceList())
> - instanceinfo = dict((iname, self.cfg.GetInstanceInfo(iname))
> - for iname in instancelist)
In order to reduce (IMHO) unneeded code churn below, you could simply
have local variables that point to self.…
> groupinfo = self.cfg.GetAllNodeGroupsInfo()
> + node_data_list = [self.my_node_info[name] for name in self.my_node_names]
Why not keep the name nodeinfo?
If it's simply for a cleanup of the name, that should be done in a
separate patch "Variable name cleanup" pretty please.
iustin
