[reconfigurator] Move resource allocation out of `BlueprintBuilder` #7235

jgallagher · 2024-12-11T21:55:36Z

Most of this diff is moving stuff around in preparation for a larger rework, but there are some nontrivial changes worth reviewing that I'll call out in comments below. This is staged on top of #7234.

Prior to this PR, BlueprintBuilder had 3 fields that allocated various kinds of resources:

sled_ip_allocators (for allocating underlay IPs)
external_networking (for creating OPTE NICs and assigning external IP addresses for Nexus/boundary NTP/external DNS)
internal_dns_subnets (for assigning internal DNS zones, which get IPs distinct from the normal sled subnet)

All three were lazily instantiated on first use. This is to allow the planner to first expunge zones then start adding; the first "add" that needs a resource from one of these allocators creates the allocator by snapshotting the state of available resources at that point (after expungement). Eventually I'd like to break the builder up to make these two phases more explicit, but for now this PR settles for shrinking the builder's surface area some:

sled_ip_allocators is gone; each SledEditor now maintains its own underlay IP allocator. These are no longer instantiated lazily, but for underlay IPs specifically this is fine: we never reuse underlay IPs from expunged zones, so we don't need to wait for expungement to reuse resources like the other two.
external_networking/internal_dns_subnets were replaced by resource_allocator, which internally holds both of those types and has pass-through methods for the builder to use. This is still lazily instantiated via OnceCell to keep the existing implicit ordering/behavior.

jgallagher · 2024-12-11T21:56:38Z

nexus/reconfigurator/planning/src/blueprint_builder/builder.rs

+    }
+}
+
+// Helper to validate that the system hasn't gone off the rails. There should


This function moved wholesale from external_networking.rs without any changes. I noted where we call it above, but I think ultimately this should move to blippy.

jgallagher · 2024-12-11T21:58:40Z

nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs

+        // TODO-cleanup This is the only reason we take `target_redundancy`; do
+        // we have another use for it in the future, or should someone else do
+        // this check?
+        if target_redundancy > INTERNAL_DNS_REDUNDANCY {


This check might be able to move to blippy too? Or maybe the planner? We might be able to drop it altogether - a caller can try to alloc() as many subnets as they want, regardless of what they pass here, but we'll fail at allocation time if they try to ask for more than INTERNAL_DNS_REDUNDANCY (as we should!).

I don't have a strong opinion here. Seems fine to leave it for now and figure it out later, but also doesn't seem to be adding much.

jgallagher · 2024-12-11T21:59:28Z

nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/underlay_ip_allocator.rs

@@ -0,0 +1,220 @@
+// This Source Code Form is subject to the terms of the Mozilla Public


This file was moved from blueprint_builder/ip_allocator.rs, but changed enough (and is small enough) that it's probably worth a review as though it were new.

jgallagher · 2024-12-11T22:00:07Z

nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/underlay_ip_allocator.rs

+        // We intentionally ignore any internal DNS underlay IPs; they live
+        // outside the sled subnet and are allocated separately.
+        if zone_kind == ZoneKind::InternalDns {
+            return Ok(());
+        }


This feels bad, but seemed less gross than all the callers of this type doing their own filtering ahead of time to not try to pass in internal DNS IPs. Suggestions welcome.

I see. My first reaction to this was: this is an allocator for IPs from the sled's subnet. Callers should know if that's what they have and they should simply avoid calling this for the DNS IP, which would be fairly obviously not from the sled's subnet.

Looking at the call stack though, that isn't the case. BlueprintZoneConfig::underlay_ip() hides this detail.

It kind of makes me wonder if we want a sort of smarter super-allocator where you can gave it an enum like SledUnderlayIp, InternalDnsUnderlayIp, maybe even fold NexusExternalIp and ExternalDnsExternalIp into it, and then you'd specify which thing you wanted when you mark things as used or allocated. But I haven't thought about this much -- it's pretty speculative -- and it seems way out of scope just to avoid this check! So I say go with this for now.

I think we can check against the reserved rack subnet. That amounts to the same thing as this check, but I think it has better semantics. It also lets us drop zone_kind, which is a nice simplification. I'm most of the way through a patch for this, if you think it's a good idea I can push to this branch.

It kind of makes me wonder if we want a sort of smarter super-allocator where you can gave it an enum like SledUnderlayIp, InternalDnsUnderlayIp, maybe even fold NexusExternalIp and ExternalDnsExternalIp into it, and then you'd specify which thing you wanted when you mark things as used or allocated.

This sounds nice to me at first glance, but I'm not sure about the mechanics. Sled underlay IP decisions can be made locally (with just the information about the one sled in question), but the other three need information about all sleds. I'll think about this one (but agreed that'd be a followup PR).

I think we can check against the reserved rack subnet. That amounts to the same thing as this check, but I think it has better semantics. It also lets us drop zone_kind, which is a nice simplification.

I'm less sure about this - if we didn't pass in zone_kind, would we allow a mismatch (e.g., a Nexus zone using an IP from the reserved rack subnet)? Maybe that kind of thing should be caught by a different level (e.g., blippy, which I'm hoping to have a draft out for soon), and then it would be reasonable to simplify this as you suggest?

jgallagher · 2024-12-11T22:03:29Z

nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_2_2a.txt

@@ -185,7 +185,7 @@ to:   blueprint 9f71f5d3-a272-4382-9154-6ea2e171a6c6

 REMOVED SLEDS:

-  sled 68d24ac5-f341-49ea-a92a-0381b52ab387 (was active):
+  sled 68d24ac5-f341-49ea-a92a-0381b52ab387 (state was unknown):


unknown here is a side effect of the builder removing the sled_state entry for fully-decommissioned sleds. We should stop doing that, but it's not directly relevant to this PR. (This changed because of the extra check for "look up the subnet of any active sled"; the test was claiming a sled was decommissioned in PlanningInput but not the parent blueprint, which is nonsense and caused the test to fail due to its data mucking rather than what it was actually trying to test.)

andrewjstone

Looks good!

andrewjstone · 2024-12-16T17:15:57Z

nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs

+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+//! Blueprint planner resource allocation


Are you planning to add other resource allocation besides networking stuff here? I'm asking because the name is pretty generic, but I assume that is on purpose.

Yeah, I thought we could (e.g.) move ClickhouseAllocator under this module at some point as we rework the builder. I'd also like zpool allocation to land here (it's currently just a method on BlueprintBuilder) eventually.

andrewjstone · 2024-12-16T17:17:43Z

nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs

+        self.external_networking.for_new_boundary_ntp()
+    }
+
+    /// Allow a test to manually add an external DNS address, which could


We could probably add a testing feature for this, but it's likely not worth it. I really wish rust let stuff marked with #[cfg(test)] be used in tests outside the crate.

Agreed on both counts.

andrewjstone · 2024-12-16T17:26:30Z

nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs

+        // TODO-cleanup This is the only reason we take `target_redundancy`; do
+        // we have another use for it in the future, or should someone else do
+        // this check?
+        if target_redundancy > INTERNAL_DNS_REDUNDANCY {


I don't have a strong opinion here. Seems fine to leave it for now and figure it out later, but also doesn't seem to be adding much.

andrewjstone · 2024-12-16T17:30:13Z

nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/underlay_ip_allocator.rs

+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+


Nit: Maybe add a doc comment?

Certainly; added in 1cfaddc

jgallagher requested review from andrewjstone and plotnick December 11, 2024 21:55

jgallagher commented Dec 11, 2024

View reviewed changes

Base automatically changed from john/sled-editor-decommissioned to main December 16, 2024 14:04

jgallagher added 12 commits December 16, 2024 11:22

port over sled IP allocator

406f4ab

port over external networking allocators

f71e3f6

port over internal dns subnet allocator

fc0cecb

introduce (unused for now) BlueprintResourceAllocator

cc01ee3

add BlueprintResourceAllocator to builder

e6844ff

move sled underlay IP allocator into SledEditor

5e90d62

builder: use SledEditor to allocate IPs

3920472

builder: use resource allocator

3b7bc76

remove now-unused modules

660ff2c

remove now-unused error variants

189654b

when adding a zone, check and mark its underlay IP as allocated

78d44c5

ignore localhost IPs for tests

64d2164

jgallagher force-pushed the john/sled-editor-allocators branch from 5fcc1bd to 64d2164 Compare December 16, 2024 16:52

andrewjstone approved these changes Dec 16, 2024

View reviewed changes

jgallagher added 2 commits December 16, 2024 14:43

fix broken db-queries test

8daab18

doc comment on underlay_ip module

1cfaddc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[reconfigurator] Move resource allocation out of `BlueprintBuilder` #7235

[reconfigurator] Move resource allocation out of `BlueprintBuilder` #7235

jgallagher commented Dec 11, 2024

jgallagher Dec 11, 2024

jgallagher Dec 11, 2024

andrewjstone Dec 16, 2024

jgallagher Dec 11, 2024

jgallagher Dec 11, 2024

davepacheco Dec 12, 2024

plotnick Dec 12, 2024 •

edited

Loading

jgallagher Dec 16, 2024

jgallagher Dec 11, 2024

andrewjstone left a comment

andrewjstone Dec 16, 2024

jgallagher Dec 16, 2024

andrewjstone Dec 16, 2024

jgallagher Dec 16, 2024

andrewjstone Dec 16, 2024

andrewjstone Dec 16, 2024

jgallagher Dec 16, 2024

		@@ -0,0 +1,220 @@
		// This Source Code Form is subject to the terms of the Mozilla Public

[reconfigurator] Move resource allocation out of BlueprintBuilder #7235

Are you sure you want to change the base?

[reconfigurator] Move resource allocation out of BlueprintBuilder #7235

Conversation

jgallagher commented Dec 11, 2024

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

plotnick Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewjstone 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

[reconfigurator] Move resource allocation out of `BlueprintBuilder` #7235

[reconfigurator] Move resource allocation out of `BlueprintBuilder` #7235

plotnick Dec 12, 2024 •

edited

Loading