-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DoH/DoT/TCP-based lookups and connection re-use #439
Closed
phillip-stephens
wants to merge
25
commits into
phillip/336-dns-over-https
from
phillip/336-doh-and-dot-worker-pools
Closed
DoH/DoT/TCP-based lookups and connection re-use #439
phillip-stephens
wants to merge
25
commits into
phillip/336-dns-over-https
from
phillip/336-doh-and-dot-worker-pools
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 2cb7877.
After talking with Zakir, we can make this quite a bit simpler if we just re-use the existing connection in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds "name server stickiness" to lookups so Resolvers will prioritize processing queries from the same nameserver to avoid having to re-handshake TCP/TLS/HTTPS.
Changes
wireLookup
intowireLookupTCP
andwireLookupUDP
so the TCP variant could have connection infoAXFR
-AXFR
is unique in that if the user doesn't provide a name server, it first does an NS lookups for that domain. This means we should not suggest a NameServer for these lookups. It's a little gross, but I added a check inside the worker thread to discard the name server "suggestion" if we're doing AXFR. If a user specifies a name server (as opposed to our suggestion), this shouldn't be thrown awayWorkerPools
) than workers, I capped the number of pools at the--threads
count. Using nameservers > thread count will result in a performance penalty since we can't send the same name server lookups to the same workers anymore.Overview
In #431 (which this branch is based on, wanted to have this logic reviewed before merging into that to break up this larger feature), I added functionality with DoH and DoT connections that a given resolver would only re-handshake if the nameserver was different than the remote address on it's existing connection.
The issue is that in
ExternalLookup
here, if the user didn't provide a nameserver then a random one would be selected. Let's look at an example to see how this causes us to unnecessarily tear-down connections:./zdns google.com yahoo.com eBay.com --threads=2 --name-servers=1.1.1.1,8.8.8.8
--tlsAnd let's say that after the random NS selection, this gives us
In this toy example, thread #0 would tear down it's TLS connection and re-establish one to
8.8.8.8
even though thread #1 already has a connection to8.8.8.8
.Load Imbalance
As another design consideration, all external resolvers do not behave equally.
I measured the resolution time for 7k queries and to what resolver they were sent to.
This led to the threads responsible for Google queries sitting idle while the Cloudflare ones were busy working.
Design
To address both re-using TCP connections and dealing with load imbalance, this PR implements a Priority and Global work queue.
A new
inputDeMultiplexor
chooses an external NS for each input line and passes it to the assigned priority queue. Each priority queue is for queries to a specific name-server, ex: @1.1.1.1. If the priority queue is full, then the demultiplexer will block on both the Priority/Global queue. In this way, we prioritize sending work to the threads which have a pre-existing connection to the name server, but also avoid large work imbalance issues.Similarly, threads will prefer to read from their Priority queue, before blocking on both Priority and Global queues.
Tradeoff: Work Imbalance vs. Connection Re-use
Every time a worker chooses a work item from the
Global
queue, it will have to re-handshake. However, without this Global/Priority queue split, workers with Priority on a very fast nameserver will sit idle when they could be doing work too. To showcase this, see the below experiment.I ran an experiment to check different points on this spectrum:
main
- no TCP connection re-usethis branch
- first try to pull from Priority, then either Global/PriorityAs the blocking time increases, the odds that a non-Priority thread will need to take an item from the Global queue to load-balance decreases. This decreases new TCP handshakes but increases the runtime.
3x runs per condition
7,000 domains run with
"A", "--verbosity=3", "--threads=100", "--tcp-only", "--name-servers=1.1.1.1,1.0.0.1,8.8.8.8,8.8.4.4",
Unaffected
--iterative
lookups will ignore this suggestion and chose a random root server. Since we don't support--tls
or--https
with--iterative
anyway, this isn't a concern.Performance
Using the benchmark (7k domains), edited the command to use
./zdns A --name-servers=1.0.0.1,1.1.1.1,8.8.8.8,8.8.4.4 --threads=100
(external lookups)main
branch8.31 s.
/ 14,006 packets on lab VM (varies between 8-10s)9.82 s.
/ 70,012 packets6.60 s.
/ 14004 packets10.29 s.
/ 44,226 packetsTesting
--no-recycle-sockets --tcp-only
to be sure that we're not creating persistent TCP connections if the user doesn't want that