Async DNS over TCP #25690

raboof · 2018-09-27T07:19:58Z

Basic functionality
Retry/reconnect logic on failures
Buffer/reassemble fragmented replies

The current implementation includes an integration test that relies on being online and receiving a truncated response when resolving the AAAA records for many.bzzt.net. We're considering creating a Docker image that acts as a DNS server that shows the behavior we want to test against.

chbatey · 2018-09-27T07:32:52Z

akka-actor-tests/src/test/scala/akka/io/dns/OnlineAsyncDnsResolverIntegrationSpec.scala

+    }
+
+    "resolve queries that are too big for UDP" in {
+      val name = "many.bzzt.net"


is this still up? Not working for me

Should! (serving AAAA records)

akka-ci · 2018-09-27T08:29:55Z

Test FAILed.

akka-ci · 2018-09-27T10:45:35Z

Test PASSed.

akka-ci · 2018-09-27T14:08:03Z

Test FAILed.

akka-ci · 2018-09-27T14:08:27Z

Test FAILed.

raboof · 2018-09-27T14:09:05Z

(cancelled 2 builds to make some room on Jenkins, we're mostly interested in the newest commit after all)

akka-ci · 2018-09-27T15:15:03Z

Test PASSed.

johanandren

Just a partial review...

johanandren · 2018-09-27T15:35:46Z

akka-actor/src/main/scala/akka/io/dns/internal/DnsClient.scala

+    minBackoff = 10.millis,
+    maxBackoff = 20.seconds,
+    randomFactor = 0.1
+  ), "tcpDnsClientSupervisor")


I'm guessing this only happens for the SRV use case, not normal DNS, so should we perhaps start it lazily instead of always?

In theory it can also happen for large normal DNS responses, but I agree that is a corner case and it'd make sense to start this lazily

akka-actor/src/main/scala/akka/io/dns/internal/DnsClient.scala

johanandren · 2018-09-27T15:41:45Z

akka-actor/src/main/scala/akka/io/dns/internal/DnsClient.scala

+        }
+      } else {
+        val (recs, additionalRecs) = if (msg.flags.responseCode == ResponseCode.SUCCESS) (msg.answerRecs, msg.additionalRecs) else (Nil, Nil)
+        self ! Answer(msg.id, recs, additionalRecs)


Why do we need to reschedule it a message, couldn't we just deal with the result right away?

in the truncated case, it's the TcpDnsClient that sends the Answer to the DnsClient

Could it not get the replyTo out of the inflightRequests then send it back right away?

@chbatey I'm not sure I follow. The reason we're sending Answer to self here because that makes it possible to share that logic with the code path where TcpDnsClient sends the Answer to us. We could also put that logic in a helper function and call that helper function here and when we receive an Answer from TcpDnsClient, do you think that would be clearer?

johanandren · 2018-09-27T15:43:15Z

akka-actor/src/main/scala/akka/io/dns/internal/TcpDnsClient.scala

+  val connecting: Receive = {
+    case CommandFailed(_: Connect) ⇒
+      log.warning("Failed to connect to [{}]", ns)
+      throw new AkkaException("Connecting failed")


Isn't just throwing going to lead to it getting logged? Put the ns in the exception instead?

Probably, but that might not be bad? Weird if connecting to the DNS server fails.

Mostly thinking that if we get two errors in the log that is confusing but maybe it doesn't matter.

Ah, good one, will remove the warning 👍

johanandren · 2018-09-27T15:51:07Z

akka-actor/src/main/scala/akka/io/dns/internal/TcpDnsClient.scala

+
+  private def parseResponse(data: ByteString) = {
+    val msg = Message.parse(data)
+    log.debug(s"Decoded: $msg")


debug("Decoded: {}", msg)

johanandren · 2018-09-27T15:52:16Z

akka-actor/src/main/scala/akka/io/dns/internal/TcpDnsClient.scala

+    case msg: Message ⇒
+      val bytes = msg.write()
+      connection ! Tcp.Write(encodeLength(bytes.length))
+      connection ! Tcp.Write(bytes)


Do them with one write message?

johanandren · 2018-09-27T15:53:22Z

akka-actor/src/main/scala/akka/io/dns/internal/TcpDnsClient.scala

+        if (data.drop(prefixSize).length < expectedPayloadLength)
+          context.become(ready(connection, data))
+        else {
+          val payload = data.drop(prefixSize).take(expectedPayloadLength)


Isn't there something with drop and take on ByteStrings being overly expensive?

Since we're doing I/O anyway and we need to seek in the message while parsing references to domain names, I think the drop is worth it. The take we can skip, though.

akka-ci · 2018-09-28T10:28:13Z

Test PASSed.

akka-ci · 2018-10-02T19:50:02Z

Test PASSed.

akka-ci · 2018-10-03T10:22:24Z

Test FAILed.

akka-ci · 2018-10-03T10:31:24Z

Test FAILed.

raboof · 2018-10-03T10:40:48Z

PLS BUILD

akka-ci · 2018-10-03T11:52:56Z

Test PASSed.

johanandren

LGTM

chbatey

Nice, looks good

chbatey · 2018-10-12T07:26:28Z

akka-actor/src/main/scala/akka/io/dns/internal/DnsClient.scala

+        }
+      } else {
+        val (recs, additionalRecs) = if (msg.flags.responseCode == ResponseCode.SUCCESS) (msg.answerRecs, msg.additionalRecs) else (Nil, Nil)
+        self ! Answer(msg.id, recs, additionalRecs)


Could it not get the replyTo out of the inflightRequests then send it back right away?

chbatey · 2018-10-12T07:30:25Z

akka-actor/src/main/scala/akka/io/dns/internal/TcpDnsClient.scala

+
+  val idle: Receive = {
+    case _: Message ⇒
+      stash()


I guess the stash will never get too big if the dns server doesn't support TCP as we'll restart back a backoff

patriknw

LGTM

patriknw · 2018-10-15T11:53:42Z

akka-actor/src/main/scala/akka/io/dns/internal/TcpDnsClient.scala

+      val bytes = msg.write()
+      connection ! Tcp.Write(encodeLength(bytes.length) ++ bytes)
+    case CommandFailed(_: Write) ⇒
+      throw new AkkaException("Write failed")


does it include any reason for the failure that we want to have logged?

Ha - I expected not, but CommandFailed actually has a cause: Option[Throwable]. I'll see if I can add a 'log' here.

akka-ci · 2018-10-15T14:35:22Z

Test PASSed.

akka-ci added the validating PR is currently being validated by Jenkins label Sep 27, 2018

chbatey reviewed Sep 27, 2018

View reviewed changes

akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Sep 27, 2018

raboof force-pushed the asyncDnsRetryOverTcpWhenTruncated branch from 47e7e4a to 42ed417 Compare September 27, 2018 09:31

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Sep 27, 2018

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Sep 27, 2018

raboof changed the title ~~[wip] Async DNS over TCP~~ Async DNS over TCP Sep 27, 2018

akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Sep 27, 2018

akka-ci added the needs-attention Indicates a PR validation failure (set by CI infrastructure) label Sep 27, 2018

akka-ci added tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Sep 27, 2018

johanandren reviewed Sep 27, 2018

View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Sep 28, 2018

akka-ci removed the validating PR is currently being validated by Jenkins label Oct 2, 2018

Skip AsyncDnsResolverIntegrationSpec when docker missing

60644e5

akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Oct 3, 2018

Scalariform

463206a

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Oct 3, 2018

raboof mentioned this pull request Oct 5, 2018

Run DnsDiscoverySpec against dockerized DNS server akka/akka-management#337

Closed

johanandren approved these changes Oct 10, 2018

View reviewed changes

chbatey approved these changes Oct 12, 2018

View reviewed changes

raboof mentioned this pull request Oct 12, 2018

Service discovery doc 404 link akka/akka-grpc#435

Closed

patriknw approved these changes Oct 15, 2018

View reviewed changes

Include cause in exception if possible

72a608f

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Oct 15, 2018

raboof merged commit 23b7f86 into master Oct 16, 2018

johanandren deleted the asyncDnsRetryOverTcpWhenTruncated branch January 24, 2019 15:17

pjfanning mentioned this pull request Nov 22, 2023

investigate license issue with /actor-tests/src/test/bind/etc/ files apache/pekko#809

Open

Async DNS over TCP #25690

Async DNS over TCP #25690

Conversation

raboof commented Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Sep 27, 2018

akka-ci commented Sep 27, 2018

akka-ci commented Sep 27, 2018

akka-ci commented Sep 27, 2018

raboof commented Sep 27, 2018

akka-ci commented Sep 27, 2018

johanandren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Sep 28, 2018

akka-ci commented Oct 2, 2018

akka-ci commented Oct 3, 2018

akka-ci commented Oct 3, 2018

raboof commented Oct 3, 2018

akka-ci commented Oct 3, 2018

johanandren left a comment

Choose a reason for hiding this comment

chbatey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Oct 15, 2018

raboof commented Sep 27, 2018 •

edited

Loading