2013/11/12 Hrvoje Ribicic <[email protected]>
>
>>
>> @@ -992,7 +997,12 @@ def VerifyNode(what, cluster_name, all_hvparams):
>> # Try to contact all nodes
>> val = {}
>> for node in nodes:
>> - success, message =
>> _GetSshRunner(cluster_name).VerifyNodeHostname(node)
>> + params = groups_cfg.get(node_groups.get(node))
>> + ssh_port = params["ndparams"].get(constants.ND_SSH_PORT)
>> + logging.debug("Ssh port %s (None = default), derived from %s for
>> %s",
>> + str(ssh_port), str(params), node)
>>
>
> While debugging messages are just fine, printing all the group params
> could clutter up logs, and provides little benefit here.
> Perhaps remove this part of the output, or print just the group name?
>
OK, sounds reasonable.
>
>
>> diff --git a/lib/cmdlib/node.py b/lib/cmdlib/node.py
>> index 89155ac..62e3795 100644
>> --- a/lib/cmdlib/node.py
>> +++ b/lib/cmdlib/node.py
>> @@ -313,7 +313,10 @@ class LUNodeAdd(LogicalUnit):
>> cname = self.cfg.GetClusterName()
>> result = rpcrunner.call_node_verify_light(
>> [node_name], vparams, cname,
>> - self.cfg.GetClusterInfo().hvparams)[node_name]
>> + self.cfg.GetClusterInfo().hvparams,
>> + {},
>> + self.cfg.GetAllNodeGroupsInfoDict()
>> + )[node_name]
>> (errmsgs, _) = CheckNodePVs(result.payload, excl_stor)
>> if errmsgs:
>> raise errors.OpPrereqError("Checks on node PVs failed: %s" %
>>
>
> Why is an empty dictionary being passed to the node_verify_light call here?
> It just invokes node_verify in the current code, and all node_verify calls
> provide a dictionary even for one node.
> If there is a reason, it would be nice to add a comment in the code to
> explain why.
>
The reason is that SSH check is being performed. The [node_name] says on
which nodes the operation should run, but the "what" argument, set to
"vparams" here, only contains {NV_PVLIST:[vg_name]), so only a PV check is
performed.
But I suppose I could add the group UUID for this node as well, in the case
someone in the future wants to use it for some purpose.
>
>
>> diff --git a/lib/rpc_defs.py b/lib/rpc_defs.py
>> index e56341d..712fb74 100644
>> --- a/lib/rpc_defs.py
>> +++ b/lib/rpc_defs.py
>> @@ -489,6 +489,9 @@ _NODE_CALLS = [
>> ("checkdict", None, "What to verify"),
>> ("cluster_name", None, "Cluster name"),
>> ("all_hvparams", None, "Dictionary mapping hypervisor names to
>> hvparams"),
>> + ("node_groups", None, "node names mapped to their group uuids"),
>> + ("groups_cfg", None,
>> + "a dictionary mapping group uuids to their configuration"),
>> ], None, None, "Request verification of given parameters"),
>> ("node_volumes", MULTI, None, constants.RPC_TMO_FAST, [], None, None,
>> "Gets all volumes on node(s)"),
>> @@ -611,6 +614,9 @@ CALLS = {
>> ("checkdict", None, "What to verify"),
>> ("cluster_name", None, "Cluster name"),
>> ("hvparams", None, "Dictionary mapping hypervisor names to
>> hvparams"),
>> + ("node_groups", None, "node names mapped to their group uuids"),
>> + ("groups_cfg", None,
>> + "a dictionary mapping group uuids to their configuration"),
>> ], None, None, "Request verification of given parameters"),
>> ]),
>> "RpcClientConfig": _Prepare([
>
>
> Nitpicking time: all other calls align the continued entries with the
> first entry - you have an extra space.
>
>
>> @classmethod
>> def perspective_node_verify_light(cls, params):
>> """Run a light verify sequence on this node.
>>
>> + This means verifying a node just before it's being added to a
>> cluster.
>> +
>> """
>> # So far it's the same as the normal node_verify
>> return cls.perspective_node_verify(params)
>>
>
> Nitpicking: It is currently used only to verify a node just before it is
> added to a cluster.
>
I'll fix it. (I wonder why plint/pep8 didn't shout at me.)
>
> And it could be a good idea to move the comment about this call being
> equivalent to node_verify into the docstring.
>
Which docstring do you have in mind? (The call has already been there, I
suppose there was some intention they could eventually be different.)
>
>
>> diff --git a/lib/ssh.py b/lib/ssh.py
>> index c694341..32a35e5 100644
>> --- a/lib/ssh.py
>> +++ b/lib/ssh.py
>> @@ -123,7 +123,8 @@ class SshRunner:
>> self.ipv6 = ipv6
>>
>> def _BuildSshOptions(self, batch, ask_key, use_cluster_key,
>> - strict_host_check, private_key=None, quiet=True):
>> + strict_host_check, private_key=None, quiet=True,
>> + port=None):
>> """Builds a list with needed SSH options.
>>
>> @param batch: same as ssh's batch option
>> @@ -134,6 +135,7 @@ class SshRunner:
>> @param strict_host_check: this makes the host key checking strict
>> @param private_key: use this private key instead of the default
>> @param quiet: whether to enable -q to ssh
>> + @param port: the SSH port to use, or None to use teh default
>>
>
> s/teh/the/
>
>
> Thanks,
>
> Hrvoje Ribicic
> Ganeti Engineering
> Google Germany GmbH
> Dienerstr. 12, 80331, München
>
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
> Steuernummer: 48/725/00206
> Umsatzsteueridentifikationsnummer: DE813741370
>
