Page MenuHomePhabricator

Spicerack's `Discovery.resolve_with_client_ip` should set a timeout on `udp_with_fallback`
Closed, ResolvedPublic

Description

In short, without setting the timeout parameter, calls to udp_with_fallback will wait forever if, e.g., our initial resolution request or response packet gets particularly unlucky and is lost.

More discussion on how this can become a practical concern in T393600#11207352.

As noted there, other resolution calls in Discovery that use the cached per-nameserver resolver.Resolver instances do not have this problem, as they get the poorly documented default timeout of 2 seconds (see BaseResolver.reset).

As noted by @Volans, it's probably reasonable to set a sensible default at the callsite. Perhaps 2 seconds, by analogy to what the cached Resolvers do.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

For completeness, another option might be to do pretty much exactly what https://gerrit.wikimedia.org/r/1072612 did, since we're iterating over self._resolvers anyway (i.e., set timeout=dns_resolver.timeout, so the timeout is at least consistent between resolution methods). From a readability perspective, I think that really only makes the code more understandable if we also make the Resolver timeout explicit at construction.

Change #1190770 had a related patch set uploaded (by Scott French; author: Scott French):

[operations/software/spicerack@master] dnsdisc: set a timeout on udp_with_fallback

https://gerrit.wikimedia.org/r/1190770

While we should not need this for Day 2 of the switchover tomorrow (see T393600#11207352), it would be ideal to get this merged and released in spicerack before the Day 8 (September 30) repool of eqiad.

@elukey - Do you think that would be feasible?

When spicerack is updated on whichever pilot host we choose (looks like this might be cumin2002 by convention), we should be able to at least confirm this doesn't break anything with a dry-run of sre.discovery.datacenter, noting that will need to differ from the current production state (e.g., simulate repooling eqiad), so that we make it past this early return and into a code path that will call resolve_with_client_ip.

Change #1190770 merged by jenkins-bot:

[operations/software/spicerack@master] dnsdisc: set a timeout on udp_with_fallback

https://gerrit.wikimedia.org/r/1190770

I've gone ahead and merged https://gerrit.wikimedia.org/r/1190770, in the interest of potentially getting it out in a spicerack release before the September 30 repool of eqiad.

Spicerack 11.9.0 deployed on all cumin nodes :)

Scott_French claimed this task.

Amazing - thank you very much, @elukey!

Although it would be extremely surprising if this were the case, I just manually tested the equivalent of sre.discovery.datacenter's DiscoveryRecord.check_records code path hitting resolve_with_client_ip in spicerack-shell to confirm I've not broken anything. Indeed, all seems well.