Listen on multiple addresses in the DNS server(s) by bnaecker · Pull Request #11207 · oxidecomputer/omicron

bnaecker · 2026-09-01T03:10:52Z

Expand the DNS configuration with a list of socket addresses instead of a single one, to support binding multiple UDP sockets in the same server.
Also add some limits on the concurrency of the DNS server, by spawning a set of tasks to handling UDP packets and processs DNS queries separately, with a limited queue between them. Overload on the queue is shed and relies on DNS clients retrying.
Fixes External DNS needs to support multiple external addresses #11008

- Expand the DNS configuration with a list of socket addresses instead of a single one, to support binding multiple UDP sockets in the same server. - Also add some limits on the concurrency of the DNS server, by spawning a set of tasks to handling UDP packets and processs DNS queries separately, with a limited queue between them. Overload on the queue is shed and relies on DNS clients retrying. - Fixes #11008

jgallagher

Generally LGTM; most of my comments are more about the new task/queue structure than binding multiple addresses. Would prefer @davepacheco take a look at this too.

jgallagher · 2026-09-02T18:37:02Z

+            Err(flume::TrySendError::Full(_)) => {
+                debug!(
+                    &log,
+                    "dropping DNS request: worker queue full";


A client can't tell the difference between this branch and us being totally dead, right? Could/should we respond with whatever the DNS equivalent of a 429 is instead?

I had the same question, but AFAICT DNS doesn't really have a dedicated response type for "overloaded". We can send a generic SERVFAIL, but I believe that's more like a 500 or 503 than a 429. It might still be better, though, I'm not sure. I'll read some more.

Ok, I'm tempted to say we should keep this behavior, of dropping the packet if we're overloaded. It's true that SERVFAIL can be used to indicate that the server is overloaded, but it's very generic. I'm not sure what DNS clients tend to do with that. OTOH, if we drop the packet, they basically have to wait for some kind of timeout before trying again, which helps us shed the load we're trying to get rid of here.

The other reason is that, in order to generate a valid SERVFAIL packet, we have to decode the UDP message to get out things like the transaction ID and echo it back in the response. So it's more work for us, and work we already do on the other side of the queue. It wouldn't be the worst thing to do that inline with the code that fails to send on the queue, but it's not a great idea either.

I wonder if there's any kind of backpressure we're defeating here? If this were TCP for example, this would be analogous to reading requests from the TCP socket, putting them into a queue and dropping them if the queue is full. Better in that case is to not read from the socket until you know you're able to handle another request. That communicates (through the TCP window) that the server isn't able to handle more stuff and the client shouldn't send it.

I'm not sure if there's an analog for UDP. There's no TCP window of course. But for example if we avoided reading from the socket altogether because we were too busy, the kernel might buffer some amount, and then it would presumably have to drop packets. Might that be more visible (in a useful way), either to clients or administrators?

Basically what I'm saying is that we're not actually dropping the packet here -- we're dropping the request. I wonder if dropping the packet at the networking layer (because we're not reading from the socket) is better in some way.

The code on main doesn't do any better in this regard, but it's not trying to. Here it feels like we're trying to better handle overload. (One could also ask whether we would want more sophisticated policies like CoDel or something like that, but that does seem like overkill right now.)

There's another risk reflected here that I'm not sure how to weigh. Today we're spawning an unlimited number of tasks, which means our max concurrency is bounded by memory (right?). With this, we're bounded by 128. (That assumes no pre-existing bugs that leak tasks 😬 -- if those exist, we wouldn't notice today, but with this change, the server would become unavailable after hitting that 128 times.) 128 is obviously arbitrary, but whatever number we pick here, if it's too low, then we'd introduce request failures in the field. On the other hand, if it's too high, given our FIFO policy, we could wind up spending all of our time serving requests that clients have already timed out. (This is something CoDel would address.)

jgallagher · 2026-09-02T18:41:45Z

+        let (n, client_addr) = socket
+            .recv_from(&mut buf)
+            .await
+            .context("receiving packet from UDP listen socket")?;


I'm not sure about bailing out here - if we have a recv_from() failure, we'll drop one of our reader tasks, but leave the rest running. If they all fail, do we keep running with no notification out?

All that said, I think that was all true on main too, so this is presumably okay in practice? I do wonder if main() should be checking on "do we actually still have tasks running" though.

Yes, I think that we'll drop the reader task, which is what we did on main too. main only waits for the dropshot server to exit, so a failure here is equally bad in both branches.

I guess we should probably not bail here, but I do worry about distinguishing transient / permanent errors and retries etc.

Ok, I think your suggestion of main() checking on the tasks is the best approach. We can expose a way for main to wait for either the dropshot server or any of the tasks, and have it exit in that case. SMF will restart it in the product, and we'll have a mechanism for seeing these failures.

It avoids any checking what kind of error we see, worrying about rebinding or backoff, etc. The errors should be unlikely in practice, because we've never seen the DNS sever go out to lunch AFAIK.

I implemented this in 5ca8c3e. It puts all the tasks into a JoinSet and we wait for the next task in the DNS server binary's main(). So now, if either the Dropshot server or any of the DNS tasks (the UDP readers, for example) fail, we'll exit the whole thing and have some visibility.

I'm a little leery of this change having gotten too big now, though.

- Checked config builder over default - Remove unused IntoIterator impl - Remove unnecessary empty check in CLI parsing - Construct server handle early to ensure we drop tasks when we fail to bind

Store the DNS server tasks in a `JoinSet` and wait on the set in the DNS server `main()`. If any of the tasks exit, e.g., a UDP recv fails, the whole thing will exit rather than failing silently.

davepacheco · 2026-09-03T00:01:18Z

        let resolver = internal_dns_resolver::Resolver::new_from_addrs(
            log.clone(),
-            &[cptestctx.internal_dns.dns_server.local_address()],
+            &[cptestctx.internal_dns.dns_server.first_local_address()],


I'd like to suggest that for all of these (specifically: whenever we're talking about the internal DNS server, which should only ever have one address that's on the underlay network), what about a different helper like sole_local_address() that fails if there's not exactly one. Unfortunately, that's probably hard, since this isn't fallible today. If this is all from tests, we could panic? But we wouldn't want to do that in production code.

It looks like every callsite today is either a literal test, under #[cfg(test)] or in integration tests, or is in development-only code like the simulated sled-agent or omicron-dev. Panicking seems like a viable option. It's also not the worst thing to just make the method fallible. There are ~40 callsites, but it's pretty mechanical to add except() or ? to each.

I did this in 4f36377

bnaecker · 2026-09-03T03:37:50Z

It seems like all three of the people on this PR so far (me, @davepacheco, and @jgallagher) all agree that it's sort of risky to change the internals of the DNS server like this. It's not a large change in pure LoC, but it is pretty complicated and subtle, and the DNS server is crucial.

The thing we need support is multiple UDP sockets. That does require some way of either multiplexing packets from the different sockets, or running independent server tasks. The current implementation takes the former path, but I prototyped the latter too. Specifically, I tried spawning one DNS server entirely as it is on main for each address we want to bind. Basically, I took dns_server::Server::start(), and wrapped it into one independent call for each address.

That is still not a trivial change, because we need some way of waiting for all those servers. The code on main waits on exactly one. But it is much less risky, in that nothing inside that method changes at all, only how many times we call it.

That's all to say that we can definitely change course here and take that approach instead. It seemed awkward and gross to me, but that's obviously not that important when weighed against the risks of this more complex change.

Rename `first_local_address()` to `sole_local_address()`, which asserts there's exactly one address. All internal DNS callers switch to that, since they're in development tools or tests.

davepacheco

It seems like all three of the people on this PR so far (me, @davepacheco, and @jgallagher) all agree that it's sort of risky to change the internals of the DNS server like this. It's not a large change in pure LoC, but it is pretty complicated and subtle, and the DNS server is crucial.

The thing we need support is multiple UDP sockets. That does require some way of either multiplexing packets from the different sockets, or running independent server tasks. The current implementation takes the former path, but I prototyped the latter too. Specifically, I tried spawning one DNS server entirely as it is on main for each address we want to bind. Basically, I took dns_server::Server::start(), and wrapped it into one independent call for each address.

Okay, having looked more carefully at the code, I'm less worried than I was about the data path changes because basically nothing has changed about how the packet is processed, just which task it's happening on.

I don't feel strongly about this but I still think there's enough risk in reworking the tasks that it doesn't seem worthwhile right now. So just doing the first part (spawning a Server for each address) seems pretty appealing.

davepacheco · 2026-09-03T19:58:20Z

        {
            let resolver =
-                QorbResolver::new(vec![dns.dns_server.local_address()]);
+                QorbResolver::new(vec![dns.dns_server.sole_local_address()]);


nit, and feel free to ignore: a bunch of these probably could actually use the whole list (because they're wrapping it in a Vec anyway)

davepacheco · 2026-09-03T20:02:06Z

+        // Preserve the pre-existing `--dns-address` flag, which is how SMF
+        // populates it.
+        visible_alias = "dns-address",


I'm on the fence about the alias. I'd update existing consumers to use the new name. Looks like that's the two SMF manifests and the how-to-run-simulated doc. The SMF manifest itself is in the same deployment unit so I don't think you have to worry about compatibility there.

However, the SMF property that's used to fill the manifest is configured by sled agent, so there's a compatibility issue there. How are you planning to update that?

davepacheco · 2026-09-03T20:06:55Z

 use std::net::{SocketAddr, SocketAddrV6};
 use std::path::PathBuf;

+#[derive(Clone, Debug)]


This could use a doc comment. Looks like it's used as a clap argument type so that we can hook into its parsing?

davepacheco · 2026-09-03T20:10:14Z

+        dns_result = dns_server.wait_for_exit() => {
+            match dns_result {
+                Some(Ok(ExitDetails::DnsWorker { index } )) => {
+                    anyhow::bail!("DNS worker task {index} exited unexpectedly");


nit: this expression uses a mix of "return early with an error" (bail!) and "evaluate to an error value" (the code paths that just produce a Result). It'd be nice to do one or the other. I don't think the early returns are needed.

davepacheco · 2026-09-03T20:32:37Z

+    ///
+    /// This panics if there is not exactly one address. If you want to handle
+    /// any number of addresses, call [`ServerHandle::local_addresses`] instead.
+    pub fn sole_local_address(&self) -> SocketAddr {


I know I suggested this and feel free to ignore: this seems a tad easy to accidentally use in a prod context and I wonder about returning a Result instead. What do you think?

davepacheco · 2026-09-03T20:35:03Z

+        self.local_addresses.iter().copied()
+    }
+
+    /// Wait for any of the internal tasks to exit.


The caller appears to use this correctly and exits if this ever returns, but it's a little weird to me that the interface exposed allows the ServerHandle to exist in a state where some of its tasks aren't running.

davepacheco · 2026-09-03T21:45:44Z

+            Err(flume::TrySendError::Full(_)) => {
+                debug!(
+                    &log,
+                    "dropping DNS request: worker queue full";


I wonder if there's any kind of backpressure we're defeating here? If this were TCP for example, this would be analogous to reading requests from the TCP socket, putting them into a queue and dropping them if the queue is full. Better in that case is to not read from the socket until you know you're able to handle another request. That communicates (through the TCP window) that the server isn't able to handle more stuff and the client shouldn't send it.

I'm not sure if there's an analog for UDP. There's no TCP window of course. But for example if we avoided reading from the socket altogether because we were too busy, the kernel might buffer some amount, and then it would presumably have to drop packets. Might that be more visible (in a useful way), either to clients or administrators?

Basically what I'm saying is that we're not actually dropping the packet here -- we're dropping the request. I wonder if dropping the packet at the networking layer (because we're not reading from the socket) is better in some way.

The code on main doesn't do any better in this regard, but it's not trying to. Here it feels like we're trying to better handle overload. (One could also ask whether we would want more sophisticated policies like CoDel or something like that, but that does seem like overkill right now.)

There's another risk reflected here that I'm not sure how to weigh. Today we're spawning an unlimited number of tasks, which means our max concurrency is bounded by memory (right?). With this, we're bounded by 128. (That assumes no pre-existing bugs that leak tasks 😬 -- if those exist, we wouldn't notice today, but with this change, the server would become unavailable after hitting that 128 times.) 128 is obviously arbitrary, but whatever number we pick here, if it's too low, then we'd introduce request failures in the field. On the other hand, if it's too high, given our FIFO policy, we could wind up spending all of our time serving requests that clients have already timed out. (This is something CoDel would address.)

bnaecker force-pushed the ben/multiple-dns-addresses branch from d58d9d8 to c6f8c00 Compare September 1, 2026 03:29

bnaecker requested a review from jgallagher September 1, 2026 03:29

jgallagher reviewed Sep 2, 2026

View reviewed changes

jgallagher requested a review from davepacheco September 2, 2026 18:51

bnaecker added 3 commits September 2, 2026 13:27

Review feedback

5bb3d63

- Checked config builder over default - Remove unused IntoIterator impl - Remove unnecessary empty check in CLI parsing - Construct server handle early to ensure we drop tasks when we fail to bind

typo

712424d

Review feedback

5ca8c3e

Store the DNS server tasks in a `JoinSet` and wait on the set in the DNS server `main()`. If any of the tasks exit, e.g., a UDP recv fails, the whole thing will exit rather than failing silently.

davepacheco reviewed Sep 3, 2026

View reviewed changes

bnaecker and others added 6 commits September 3, 2026 09:14

Review feedback

4f36377

Rename `first_local_address()` to `sole_local_address()`, which asserts there's exactly one address. All internal DNS callers switch to that, since they're in development tools or tests.

Fixup illumos callsite

29f6db4

Fixup

f6966e7

Doc fixup

89efa0b

Fixup test callsite

849256f

Clippy

da7056e

davepacheco reviewed Sep 3, 2026

View reviewed changes

Uh oh!

Conversation

bnaecker commented Sep 1, 2026

Uh oh!

jgallagher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnaecker commented Sep 3, 2026

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants