Update trust-dns-resolver to fix UDP socket leak#169
Merged
Conversation
This should pick up the change in bluejekyll/trust-dns#635, which fixes a UDP socket leak (bluejekyll/trust-dns#633). I suspect this is likely the cause of linkerd/linkerd2#2012. Signed-off-by: Eliza Weisman <[email protected]>
seanmonstar
reviewed
Jan 5, 2019
|
|
||
| [[package]] | ||
| name = "rand" | ||
| version = "0.6.3" |
Contributor
There was a problem hiding this comment.
Rand 0.4, 0.5, and 0.6... 😭
Contributor
Author
There was a problem hiding this comment.
Oh boy, I hadn't noticed. It would be nice if we could do something about that...
Contributor
Author
There was a problem hiding this comment.
Looks like the rand 0.4 dependency is pulled in by tempdir, which is a dependency of prost-build. The tempdir crate is deprecated in favour of tempfile, but we use it in our tests as well (to pull in fewer dependencies).
I'll look at a patch to prost-build replacing tempdir with tempfile, and hopefully we can get rid of rand 0.4
Contributor
Author
There was a problem hiding this comment.
@seanmonstar if we merge both this branch and #170, we will depend exclusively on the latest rand (0.6.3).
Sorry it took a bit, the dependency update was a surprisingly large amount of work.
This was referenced Jan 7, 2019
seanmonstar
approved these changes
Jan 9, 2019
hawkw
added a commit
to linkerd/linkerd2
that referenced
this pull request
Jan 10, 2019
proxy: bump version to fix memory leak - Update to trust-dns-resolver 0.10.1 (linkerd/linkerd2-proxy#169)
hawkw
added a commit
to linkerd/linkerd2
that referenced
this pull request
Jan 10, 2019
- Update to trust-dns-resolver 0.10.1 (linkerd/linkerd2-proxy#169) Signed-off-by: Eliza Weisman <[email protected]>
hawkw
added a commit
that referenced
this pull request
Feb 1, 2019
This branch updates the proxy's dependencies to eliminate multiple incompatible versions of the `rand` crate. Previously we depended on both `rand` 0.4.3 and 0.5.1. After this branch, we depend on `rand` 0.6.3 (the latest) and 0.5.1. However, PR #169 will also update `trust-dns-resolver` (the only crate depending on `rand` 0.5.1) to a version that depends on `rand` 0.6.3, so once both this branch and #169 are merged, we will depend only on `rand` 0.6.3. Note that this is a fairly large change to `Cargo.toml` --- it was necessary to update many of the proxy's dependencies in order to consolidate on one `rand` version. Additionally, I had to push branches of some of those dependencies in order to update their `rand` dependency, so it's currently necessary to patch some of the proxy's dependencies. When those branches merge upstream, the patches can be removed. Signed-off-by: Eliza Weisman <[email protected]>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
An upstream bug in the
trust-dns-protolibrary can causetrust-dns-resolverto leak UDP sockets when DNS queries time out. Thisissue appears to be the cause of the memory leak described in
linkerd/linkerd2#2012.
This branch updates the
trust-dnsdependency to pick up the change inbluejekyll/trust-dns#635, which fixes the UDP socket leak.
I confirmed that the socket leak was fixed by modifying the proxy to
hard-code a 0-second DNS timeout, sending requests to the proxy's
outbound listener, and using
lsof -p $(pgrep linkerd2-proxy)to count the number of open UDP sockets. On master, every request to a
different DNS name that times out leaves behind an additional open UDP
socket, which show up in
lsof, while on this branch, only TCP socketsremain open after the request ends.
In addition, I'm running a test in GCP to watch the memory and file
descriptor use of the proxy over a long period of time. This is still in
progress, but given the above, I strongly believe this branch fixes the
leak.
Fixes linkerd/linkerd2#2012.
Signed-off-by: Eliza Weisman [email protected]