An interdiff that fixes the comment(s):
diff --git a/lib/cli.py b/lib/cli.py
index 8a17313..aebf301 100644
--- a/lib/cli.py
+++ b/lib/cli.py
@@ -3549,7 +3549,7 @@ def GetNodesSshPorts(nodes, cl):
@type nodes: a list of strings
@param cl: a client to use for the query
@type cl: L{Client}
- @return: the list SSH ports corresponding to the nodes
+ @return: the list of SSH ports corresponding to the nodes
@rtype: a list of tuples
"""
return map(lambda t: t[0],
diff --git a/lib/client/gnt_node.py b/lib/client/gnt_node.py
index d8a2f86..7a94980 100644
--- a/lib/client/gnt_node.py
+++ b/lib/client/gnt_node.py
@@ -250,6 +250,7 @@ def AddNode(opts, args):
# Retrieve relevant parameters of the node group.
ssh_port = None
try:
+ # Passing [] to QueryGroups means query the default group:
node_groups = [opts.nodegroup] if opts.nodegroup is not None else []
output = cl.QueryGroups(names=node_groups, fields=["ndp/ssh_port"],
use_locking=False)
2013/11/12 Petr Pudlak <[email protected]>
>
>
>
> 2013/11/12 Hrvoje Ribicic <[email protected]>
>
>>
>>>
>>> @@ -3535,6 +3542,22 @@ def GetOnlineNodes(nodes, cl=None, nowarn=False,
>>> secondary_ips=False,
>>> return map(fn, online)
>>>
>>>
>>> +def GetNodesSshPorts(nodes, cl):
>>> + """Retrieves SSH ports of given nodes.
>>> +
>>> + @param nodes: the names of nodes
>>> + @type nodes: a list of strings
>>> + @param cl: a client to use for the query
>>> + @type cl: L{Client}
>>> + @return: the list SSH ports corresponding to the nodes
>>>
>>
>> s/the list SSH ports/the list of SSH ports/
>>
>>
>
> Will fix.
>
>
>> diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py
>>> index 64594ce..6733f49 100644
>>> --- a/lib/client/gnt_cluster.py
>>> +++ b/lib/client/gnt_cluster.py
>>> @@ -570,17 +570,20 @@ def ClusterCopyFile(opts, args):
>>> errors.ECODE_INVAL)
>>>
>>> cl = GetClient()
>>> + try:
>>> + cluster_name = cl.QueryConfigValues(["cluster_name"])[0]
>>>
>>> - cluster_name = cl.QueryConfigValues(["cluster_name"])[0]
>>> -
>>> - results = GetOnlineNodes(nodes=opts.nodes, cl=cl, filter_master=True,
>>> - secondary_ips=opts.use_replication_network,
>>> - nodegroup=opts.nodegroup)
>>> + results = GetOnlineNodes(nodes=opts.nodes, cl=cl,
>>> filter_master=True,
>>> + secondary_ips=opts.use_replication_network,
>>> + nodegroup=opts.nodegroup)
>>> + ports = GetNodesSshPorts(opts.nodes, cl)
>>> + finally:
>>> + cl.Close()
>>>
>>>
>> Actually, this is the first use I have seen of the client's Close method
>> in our codebase. Could we perhaps remove it?
>>
>> Either the resources of the client are collected without the use of
>> close, or the whole codebase is doing everything horribly wrong.
>> If the first is true, and it is more likely to be true, then adding an
>> explicit close with no reason is just confusing.
>>
>> As a result of looking into this, you will probably find out when and why
>> Close should be invoked.
>> Could you adjust the function documentation as well, perhaps in a
>> separate patch?
>>
>
> I think there is one other place that calls "Close" as well. The resources
> are indeed collected, because it's only used in the command line tools, so
> the process ends, closing the socket. But It's really code smell. I already
> have a (low-priority) plan to refactor using GetClient in the code base to
> use the "with ...:" construct to close the socket properly.
>
>
>>
>>> # Retrieve relevant parameters of the node group.
>>> ssh_port = None
>>> - if opts.nodegroup:
>>> - try:
>>> - output = cl.QueryGroups(names=[opts.nodegroup],
>>> fields=["ndp/ssh_port"],
>>> - use_locking=False)
>>> - (ssh_port, ) = output[0]
>>> - except (errors.OpPrereqError, errors.OpExecError):
>>> - pass
>>> + try:
>>> + node_groups = [opts.nodegroup] if opts.nodegroup is not None else []
>>> + output = cl.QueryGroups(names=node_groups, fields=["ndp/ssh_port"],
>>> + use_locking=False)
>>> + (ssh_port, ) = output[0]
>>> + except (errors.OpPrereqError, errors.OpExecError):
>>> + pass
>>>
>>> try:
>>> output = cl.QueryNodes(names=[node],
>>>
>>
>> If the node group is not specified, you are querying for [], probably
>> returning no results in the query.
>> The output[0] would give you an IndexError here, which the except does
>> not catch.
>>
>> Does this work? I assume it does, but I am curious to know why.
>>
>
> This is a peculiar feature of QueryGroups. If you pass [] there, it
> queries the default group. I'm not saying I like this kind of features, but
> since it's used at other places, I used it here as well. Maybe I could add
> a comment explaining it.
>
>
>>
>>
>> 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
>>
>
>
