On Tue, May 3, 2011 at 5:55 PM, Iustin Pop <[email protected]> wrote:
Hi,
>> + 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.
>
The commit message says these will be different in the next commits,
where "my" will be the ones acting upon, and all will still contain
all.
Is that ok or should we introduce those names later?
>> 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?
>
I added an assert.
>> 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.…
>
Can be done, but anyway the distiction between "all" and "my" lists
will be needed (if we follow this patch series proposal)
>> 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.
>
I believe the difference is one being a list, while the other "info"
being dicts.
I can fix it in a separate commit though if you prefer.
Thanks,
Guido
