update internal DNS during blueprint execution #4989

davepacheco · 2024-02-05T22:32:46Z

davepacheco · 2024-02-06T01:34:12Z

internal-dns/src/config.rs

-        let scrimlet_srv_records =
-            self.scrimlets.clone().into_iter().map(|(location, addr)| {
-                let srv = DnsRecord::Srv(dns_service_client::types::Srv {
-                    prio: 0,
-                    weight: 0,
-                    port: addr.port(),
-                    target: format!("{location}.scrimlet.{}", DNS_ZONE),
-                });
-                (ServiceName::Scrimlet(location).dns_name(), vec![srv])
-            });
-
-        let scrimlet_aaaa_records =
-            self.scrimlets.into_iter().map(|(location, addr)| {
-                let aaaa = DnsRecord::Aaaa(*addr.ip());
-                (format!("{location}.scrimlet"), vec![aaaa])
-            });
-


tl;dr: I brought this up in oxide-control-plane chat and sync'd with @rcgoodfellow and @internet-diglett and I believe these DNS names can be removed.

More details:

I could not find any references to these DNS names anywhere in Omicron. I looked for "switch0.scrimlet" and "switch1.scrimlet" and even ".scrimlet" (in case it was being filled into a template). If they were used programmatically, they'd show up as ServiceName::Scrimlet. The only place that's used is a comment explaining some context.

What I got from chat is what that comment says: these DNS names were added at some point after dogfood was set up. It was then discovered that they couldn't be used for whatever they were going to be used for because these DNS names only get set up at RSS-time, so systems like dogfood were just never going to have them. Instead, a different mechanism was created to find these zones when needed.

From what I can tell, the RSS code that assigned these DNS names chose the switch location (switch0 vs. switch1) arbitrarily, based on which one's sled id came lexicographically first. That's potentially inconsistent with what MGS reported as switch0 and switch1. So these names were potentially wrong on any systems where they were set up.

It should be possible to augment the code in this PR to reliably establish correct DNS names for these, provided we have an inventory collection with the information that we need from MGS. This way, in the future, we can use DNS for this. I didn't add this to this PR.

davepacheco · 2024-02-06T01:39:49Z

schema/crdb/dbinit.sql

@@ -3082,6 +3082,11 @@ CREATE TABLE IF NOT EXISTS omicron.public.blueprint (
    -- now gone.
    parent_blueprint_id UUID,

+    -- identifies the latest internal DNS version when blueprint planning began
+    -- XXX-dap when it comes time for the migration, make sure to update


When this PR has approval, I'll implement the db migration part. That part needs to make sure we provide a value for the internal_dns_version field for existing rows. I'm planning to set it to 1. That should be the current DNS version deployed everywhere. It should also be safe even if it weren't -- having one that's too old is always safe. It just means you won't be able to execute that blueprint and you need to re-plan to make a new one.

davepacheco · 2024-02-06T01:40:26Z

sled-agent/src/rack_setup/plan/service.rs

-            let sled_address = get_sled_address(sled.subnet);
-            let switch_location = if i == 0 {
-                SwitchLocation::Switch0
-            } else {
-                SwitchLocation::Switch1
-            };
-            dns_builder.host_scrimlet(switch_location, sled_address).unwrap();


This is the bit I mentioned in my other comment. These DNS names were not used, and they were also not being assigned correctly.

…in service

jgallagher

This looks great; only small nits and clarification questions 👍

jgallagher · 2024-02-20T20:39:58Z

clients/dns-service-client/src/diff.rs

+
+        let example = example();
+        let error = DnsDiff::new(&example_empty, &example)
+            .expect_err("unexpectedly succeeded comparing two empty configs");


Tiny nit - wrong string on this expect I think? (Same on line 148 below)

jgallagher · 2024-02-20T21:07:29Z

nexus/types/src/inventory.rs

+        self.sled_agents
+            .iter()
+            .filter(|(_, inventory)| inventory.sled_role == SledRole::Scrimlet)
+            .map(|(sled_id, _)| *sled_id)


Tiny nit - if you want to squish this down to filter_map, you could use https://doc.rust-lang.org/std/primitive.bool.html#method.then_some (which I only learned about recently):

.filter_map(|(sled_id, inventory)| (inventory.sled_role == SledRole::Scrimlet).then_some(*sled_id))

This is neat and I didn't know it. But I think I like what's here better in this case.

jgallagher · 2024-02-20T21:20:38Z

nexus/blueprint-execution/src/dns.rs

+            .unwrap();
+    }
+
+    dns_builder.generation(blueprint.internal_dns_version.next());


This threw me a bit, because it looks like the returned DNS config has always has a generation bump, which means it will be marching up every time this RPW runs. But making sure I understand: that doesn't happen because the config we return here is compared against the existing config using DnsDiff, which does not look at the generation number, only the contents, right? That seems a little implicit, but I don't have a great suggestion off the top of my head for making it clearer. We could wait to bump the generation until after the comparison, maybe? That might be a little more obvious?

I see what you mean. I think it would be an improvement if instead of DnsConfigParams, we passed around the equivalent of dns_config_params.zones[0].records -- i.e., just the records that are in the internal DNS zone. Then we'd assemble that into a final DnsConfigParams at the last minute, before writing it to the database. Then it would be clearer what's being generated and diff'd, etc. But right now there's no type for that, just HashMap<String, Vec<DnsRecord>>. I think at minimum we'd want a newtype for this, and probably a richer one, too. DnsConfigBuilder maybe should produce one of those instead of DnsConfigParams. Anyway I think this would be better but I'd like to not do it right now.

I have at least added a few comments to explain this though.

(I inferred your suggestion to "wait to bump the generation until after the comparison" to mean "return something with the same generation as what's there, then do the comparison, then mutate it if we find we need to bump it". I don't love that. It seems misleading in a different way.)

jgallagher · 2024-02-20T21:36:08Z

nexus/blueprint-execution/src/dns.rs

+                    continue;
+                }
+
+                if let Some(scrimlet_id) = switch_sleds_by_ip.remove(addr) {


Just for my own understanding: if we remove the scrimlet ID the first time we get here for a particular address, why don't we trip up later due to switch zones having multiple DNS records (dendrite / mgs / mgd)?

Good question! I believe there are a few different records here:

one AAAA record for something like dendrite-$sled_id.control-plane.oxide.internal

three SRV records for stuff like _mgs._tcp.control-plane.oxide.internal, _dendrite._tcp.control-plane.oxide.internal, and _mgd._tcp.control-plane.oxide.internal. These have a target field which points at dendrite-$sled_id.control-plane.oxide.internal, not the IP address directly.

This loop iterates over just the names having AAAA records. There should be only one of those for each Scrimlet. It's the next loop that iterates over the SRV records and checks those.

jgallagher · 2024-02-20T21:43:37Z

nexus/blueprint-execution/src/dns.rs

+use std::collections::BTreeMap;
+use uuid::Uuid;
+
+pub async fn deploy_dns<S>(


Two tiny nits:

I'd make this pub(crate)

Given the former, I think you could drop <S> and take creator: String, and let the caller do the conversion. That keeps the ergonomics of the public API without needing the generic on this inner function.

davepacheco self-assigned this Feb 5, 2024

first cut

c7bdde5

davepacheco force-pushed the dap/blueprint-dns branch from 7ce194e to c7bdde5 Compare February 6, 2024 01:23

davepacheco commented Feb 6, 2024

View reviewed changes

davepacheco added 2 commits February 5, 2024 17:42

remove unused ServiceName for DNS names I ripped out; skip zones not …

40e1393

…in service

update internal API spec

61f7f05

hawkw self-requested a review February 7, 2024 19:59

davepacheco added 3 commits February 14, 2024 09:50

Merge branch 'main' into dap/blueprint-dns

fcea625

test serialization of different DNS version

d79162c

add tests for DnsDiff

78a46fa

davepacheco mentioned this pull request Feb 14, 2024

blueprints: Nexus needs to update external DNS #5068

Closed

davepacheco added 5 commits February 14, 2024 13:09

add test for sled_list_all_batched()

7fc7428

add test: blueprint_dns_config

ff4d4e3

add test for dns_compute_update()

3701ea5

add test for dns_update_from_version

3c0a665

add explanatory comment

d78c56f

davepacheco requested a review from jgallagher February 15, 2024 19:58

davepacheco marked this pull request as ready for review February 15, 2024 19:58

davepacheco removed the request for review from hawkw February 15, 2024 19:58

hawkw self-requested a review February 15, 2024 20:04

davepacheco added 2 commits February 15, 2024 13:02

the test found a bug

11fa30c

Merge branch 'main' into dap/blueprint-dns

e87a431

jgallagher approved these changes Feb 20, 2024

View reviewed changes

davepacheco added 6 commits February 20, 2024 13:53

Merge branch 'main' into dap/blueprint-dns

8c3c472

review feedback

b8b38ea

implement migration

c6830c9

missing directory

2c8b110

fixes

2003bc5

more fixes

294a7d9

davepacheco merged commit 1e76aca into main Feb 21, 2024
22 checks passed

davepacheco deleted the dap/blueprint-dns branch February 21, 2024 02:44

jgallagher mentioned this pull request Feb 21, 2024

Failed to fully add a new sled on madrid #5111

Closed

davepacheco mentioned this pull request Feb 22, 2024

"expensive operation" check was too aggressive #5113

Merged

jgallagher mentioned this pull request Feb 27, 2024

Blueprint execution: Add dataset records for Crucible zones #5143

Merged

davepacheco mentioned this pull request Apr 17, 2024

what's involved with removing a sled? #4787

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update internal DNS during blueprint execution #4989

update internal DNS during blueprint execution #4989

davepacheco commented Feb 5, 2024 •

edited

Loading

davepacheco Feb 6, 2024

davepacheco Feb 6, 2024

davepacheco Feb 6, 2024

jgallagher left a comment

jgallagher Feb 20, 2024

jgallagher Feb 20, 2024

davepacheco Feb 20, 2024

jgallagher Feb 20, 2024

davepacheco Feb 20, 2024

jgallagher Feb 20, 2024

davepacheco Feb 20, 2024

jgallagher Feb 20, 2024

update internal DNS during blueprint execution #4989

update internal DNS during blueprint execution #4989

Conversation

davepacheco commented Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgallagher 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

davepacheco commented Feb 5, 2024 •

edited

Loading