Bound DNS lookups in grains collection to fix salt-call --local hangs (#65324)#69840
Bound DNS lookups in grains collection to fix salt-call --local hangs (#65324)#69840charzl wants to merge 1 commit into
Conversation
Fixes saltstack#65324. salt-call --local (and any grains refresh) could hang for tens of seconds when the minion's hostname had no usable DNS/hosts entry, because three call sites made synchronous, native-timeout-less DNS lookups: - salt.utils.network.get_fqhostname() (socket.getfqdn() + socket.getaddrinfo()), used by the hostname grain - ip_fqdn()'s socket.getaddrinfo() calls for fqdn_ip4/fqdn_ip6 - network.fqdns()'s socket.gethostbyaddr() reverse lookups, which were already parallelized via ThreadPoolExecutor but still blocked on shutdown()/context-exit joining a straggler thread All three now go through daemon-thread-based helpers bound by a new grains_dns_lookup_timeout minion option (default 5s). Daemon threads are used deliberately instead of ThreadPoolExecutor: the executor's implicit shutdown(wait=True) on context exit (and its atexit handler) would still block joining an abandoned lookup, silently reintroducing the same unbounded wait. Also removed the __opts__["__role"] == "master" gate on ip_fqdn()'s timeout warning, added in 2017 to silence per-minion log noise on masters aggregating many minions -- a reasonable call there, but one that left a single-host salt-call --local run with zero diagnostic output during the exact hang being reported. Verified against a live repro (multipass VM, unresolvable hostname + unreachable DNS): 40.7s non-deterministic hang -> 20.6s deterministic wait, confirmed via strace -f -tt to be exactly bounded by grains_dns_lookup_timeout x (number of lookups) rather than OS resolver retry policy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey. There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. |
What does this PR do?
Fixes #65324 —
salt-call --local(and any grains refresh) can hang for tens of seconds when the minion's hostname/FQDN does not resolve to an IPv4/IPv6 address via DNS, with no log output to explain why.Root cause
Grains collection makes three separate, synchronous DNS lookups with no timeout, all of which the OS resolver will retry against
/etc/resolv.conf's nameservers before giving up if the hostname has no usable DNS/hosts entry:salt.utils.network.get_fqhostname()(salt/utils/network.py) — callssocket.getfqdn()and thensocket.getaddrinfo()to determine the minion's own FQDN. Invoked once per process from thehostnamegrain (salt/grains/core.py), which loads early — this is the actual point where the hang the original reporter saw (near the alphabetically-adjacentzfsgrain) begins.ip_fqdn()(salt/grains/core.py) — callssocket.getaddrinfo(_fqdn, None, socket_type)once each forAF_INETandAF_INET6to populate thefqdn_ip4/fqdn_ip6grains.network.fqdns()(salt/modules/network.py, backing thefqdnsgrain) — reverse-resolves every local interface address viasocket.gethostbyaddr(). This one is parallelized across aThreadPoolExecutor, but the pool's implicitshutdown(wait=True)on context-manager exit still blocks until the slowest lookup finishes (or the pool is abandoned only at interpreter exit), so an unresolvable address is still effectively unbounded.None of
socket.getfqdn(),socket.getaddrinfo(), orsocket.gethostbyaddr()accept a timeout — how long they block is entirely up to/etc/resolv.conf/ glibc resolver retry settings, not anything Salt controls.A second, compounding problem: the only diagnostic in
ip_fqdn()was gated behind__opts__["__role"] == "master":This condition is always false for
salt-call --local(minion role, no master), so even a multi-second stall produced zero log output — the operation just appeared to hang with no explanation, which is exactly what the reporter observed. Git history shows this gate has an unusually contested past: added in7855cd6ce6("Only display IPvX warning if role is master"), reverted minutes later in88f49f9146, then re-added the same day in6e1ab69710("Partial revert of #40934") with the rationale that the warning itself was "spurious" — specifically in the context of a master aggregating logs from many minions. That reasoning doesn't apply to a single-hostsalt-call --localrun, where the warning is the only diagnostic available and there's no fan-out noise to worry about.Reproduction
Reproduced on a fresh Ubuntu 22.04 VM (multipass) running Salt 3006.23. Because systemd-resolved synthesizes a local A/AAAA record for the machine's own hostname (bypassing the network entirely), reproducing the reported hang required both removing the hostname's IPv4 entry from
/etc/hostsand pointing/etc/resolv.confat an unreachable nameserver directly (bypassing the systemd-resolved stub).salt-call --local state.testwall time/etc/hostsentry)grains_dns_lookup_timeout: 5)strace -f -ttbefore the fix shows repeated ~5s gaps, each following a DNS query for the hostname, consistent with glibc's default single-query resolver timeout being hit multiple times across the three call sites above:After the fix,
straceshows exactly 4 gaps of ~5.0s each (the twoget_fqhostname()calls plus the twoip_fqdn()calls, run sequentially since they occur in different grains), totaling 20.08s — matchinggrains_dns_lookup_timeout's default 5s bound times 4 calls, and no longer dependent on OS resolver retry behavior. Loweringgrains_dns_lookup_timeoutin the minion config proportionally lowers the worst-case wait.The debug log in both the original report and this reproduction shows the stall beginning right around the
zfsgrains module — that module is not implicated; grains load alphabetically andhostname/ip_fqdnsimply sit near it in load order.Fix
salt/utils/network.py: added_call_with_timeout(), a small helper that runs a blocking call (socket.getfqdn,socket.getaddrinfo, etc.) in a daemon thread and bounds how long the caller waits via aqueue.Queue.get(timeout=...). A daemon thread is used deliberately instead ofconcurrent.futures.ThreadPoolExecutor: the executor'sshutdown()(called implicitly on context-manager exit, and again by itsatexithandler at interpreter shutdown) joins outstanding workers, which would silently re-introduce an unbounded wait the moment the pool is torn down or the process exits. An abandoned daemon-thread lookup is simply left to finish (or never finish) on its own without blocking anything.get_fqhostname()now accepts atimeoutparameter (default 5s) and uses this helper for both of its underlying calls; on timeout it falls back tosocket.gethostname()(no network) rather than propagating the error, matching whatsocket.getfqdn()would eventually have returned anyway.salt/grains/core.py:ip_fqdn()now resolves each address family through the same bounded-wait pattern instead of callingsocket.getaddrinfo()directly.hostname()passes the configured timeout through toget_fqhostname().__role == "master"gate on the timeout warning is removed — the warning now fires any time a lookup takes a non-trivial fraction of the configured timeout (or hits it outright), regardless of role, sosalt-call --localruns get a diagnostic instead of silence.salt/modules/network.py:fqdns()'s reverse-lookup loop is rewritten to launch one daemon thread per address (still resolved in parallel) with a single shared deadline, rather than aThreadPoolExecutorwhose exit/shutdown can still block on a straggler. N unresolvable addresses now cost at mostgrains_dns_lookup_timeoutin total, notN × timeout.salt/config/__init__.py: new minion optiongrains_dns_lookup_timeout(default:5seconds), shared by all three call sites above.Test plan
_call_with_timeout()covering the fast path, the timeout path, and exception propagation (tests/pytests/unit/utils/test_network.py)get_fqhostname()falling back tosocket.gethostname()on timeout instead of hanging/raisingip_fqdn()covering the timeout path and confirming the warning now logs regardless of__role(tests/pytests/unit/grains/test_core.py)fqdns()covering a single hanging lookup and confirming N parallel hanging lookups still cost ~1 timeout, not N (tests/pytests/unit/modules/test_network.py)strace -f -ttthat post-fix wait time is exactly bounded bygrains_dns_lookup_timeout × (number of lookups), not OS resolver retry policytests/support/pytest/loader.py'sLoaderModuleMockto confirm__opts__defaults to{}for modules under test that don't explicitly provide it, so the new__opts__.get("grains_dns_lookup_timeout", 5)calls don't break existing tests that predate this optionpytest-salt-factoriesand friends) — new and existing tests are syntax-checked and pattern-matched against the existing suite, but should be run in CI before merge