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 >
