SSH config handling can be slow #110

Closed
wants to merge 4 commits into
from

Projects

None yet

4 participants

@fairview
Contributor

The timing of the ProxyCommand support was impeccable, but I noticed that when I started using my SSH config, my Fabric runs were excruciatingly slow. I traced it to the use of socket.getfqdn() in config.SSHConfig._expand_variables. This is an admittedly convoluted fix, but it's much, much faster for me.

@lndbrg
Contributor
lndbrg commented Nov 19, 2012

You do not have a local caching resolver (e.g bind)?
Do you have ipv6 enabled but no such address configured?I think the slow speed of the lookup is because python doing a getaddrinfo, waiting for ipv6 then falling back to ipv4 when ipv6 is not found/timimg out. You can disable ipv6 all together or (depending on kernel anf libc version (if linux)) prefer ipv4 over ipv6.

gethostbyname() will only return ipv4 addresses so this patch won't be compatible with ipv6.

The correct approach would probably be to fix your ipv6 configuration/have a local dns caching mechanism. While this might seem as a regression/bug this slowness should actually be apparent for other applications doing dns resolution too.

@fairview
Contributor

Olle, you're exactly right, it's the IPv6 lookups. I should have included more background.

I'm on a Mac, running 10.8.2, with no IPv6 anywhere in the network (unfortunately). Even if I disable IPv6 in network preferences or via the networksetup command, getaddrinfo with AF_UNSPEC will still try IPv6 lookups, and that of course is exactly how it's being called in the socket module.

I'm not seeing the problem with other applications, so I think that usage is the problem. For example, if I run the same command from my Fabric task via ssh directly, the timing is almost the same whether I use "AddressFamily inet" in my ~/.ssh/config or not.

I've revised my change to eliminate the IPv4-only functions and use getaddrinfo, trying IPv4 first, then IPv6. I'll understand if you don't want to embed that "improvement" over the standard library's logic, to work around one platform's weaknesses with IPv6, but on the other hand, if its results are correct, it will speed things up for a lot of people.

@lndbrg
Contributor
lndbrg commented Nov 19, 2012

It's up to @bitprophet to merge the patches, I'm just a happy contributer. :)

The socket.has_ipv6 actually only returns true or false based on if the socket library has been compiled with ipv6 support, not if you actually have ipv6 addressing available/enabled.

It's unfortunate that getaddrinfo does not seem to honour the os configuration for python. While, to me, the code looks nice, it think it will break what's specified in http://www.ietf.org/rfc/rfc3484.txt and the ability to change the priority of lookups in /etc/gai.conf (for linux).

Perhaps a patch considering if AddressFamily has been set and using the appropriate lookup?

inet -> ipv4
inet6 -> ipv6
empty/any -> getfqdn()

Might not get the speed up you want by default, but at least paramiko will follow what the sysadmin have configured and it won't break the RFC.

John Hensley Base SSHConfig template FQDN on AddressFamily.
In SSHConfig._expand_variables, continue using socket.getfqdn() by
default, but if the SSH config contains AddressFamily, honor that when
discovering the local host's FQDN.
5ff48d9
@fairview
Contributor

Olle, that's a great idea: preserves the default, correct behavior while letting those of us with IPv6 implementation or config problems work around them using the established ssh config mechanism. Thanks again.

@lndbrg
Contributor
lndbrg commented Nov 19, 2012

Patch looks good to me, but perhaps it would be nice to add why it is being done this way as a comment in the code? :)

@parantapa
Contributor

What is the need of always computing FQDN? As it seems to me, it is only required when '%l' format specifier is present in SSHConfig. I have added a pull request which calls socket.getfqdn only on demand.

@lndbrg lndbrg referenced this pull request Jan 28, 2013
Merged

Openssh compatibility #93

@bitprophet
Member

I am merging @parantapa's lazily deferred FQDN class with the lookup logic implemented here. Thanks to all involved!

@bitprophet bitprophet added a commit that referenced this pull request Feb 28, 2013
@bitprophet bitprophet Changelog re #110 b9242c6
@bitprophet bitprophet added a commit that referenced this pull request Feb 28, 2013
@bitprophet bitprophet Refactor duplicative code re #110 3563fca
@bitprophet
Member

Should be all done, works for me in trivial testing, if any of y'all want to get latest master and double check, that'd be grand. Otherwise, releasing this weekend. (Also going to try and pull in @lndbrg's related overhaul in #93).

@bitprophet bitprophet closed this Feb 28, 2013
@fairview
Contributor

I gave it a quick try too and it worked fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment