[nexus] Populate IP pool, nexus service information, during rack setup #2358

smklein · 2023-02-13T20:37:22Z

Summary

My long-term goal is to have Nexus be in charge of provisioning all services.

For that to be possible, Nexus must be able to internalize all input during the handoff from RSS. This PR extends the RSS -> Nexus handoff to include:

What "Nexus Services" are being launched?
What are the ranges of IP addresses that may be used for internal services?
What external IP addresses, from that pool, are currently in-use for Nexus services?

Nexus Changes

Database Records

Adds a nexus_service record, which just includes the information about the in-use external IP address.

IP Address Allocation

Adds an explicit_ip option, which lets callers perform an allocation with an explicit request for a single IP address. You might ask the question: "Why not just directly create a record with the IP address in question, if you want to create it?" We could! But we'd need to recreate all the logic which validates that the IP address exists within the known-to-the-DB IP ranges within the pool.
- The ability for an operator to "request Nexus execute with a specific IP address" is a feature we want anyway, so this isn't wasted work.
- The implementation and tests for this behavior are mostly within nexus/src/db/queries/external_ip.rs

Rack Initialization

Populates IP pools and Service records as a part of the RSS handoff.
Implementation and tests exist within nexus/src/db/datastore/rack.rs.

Populate

Move the body of some of the "populate" functions into their correct spot in the datastore, which makes it easier to...
... call all the populate functions -- rather than just a chunk of them -- from omicron_nexus::db::datastore::datastore_test.
As a consequence, update some tests which assumed the rack would be "half-populated" -- it's either fully populated, or not populated at all.

Sled Agent changes

Explicitly pass the "IP pool ranges for internal services" up to Nexus.
In the future, it'll be possible to pass a larger range of addresses than just those used by running Nexus services.

Fixes: #1958
Unblocks: #732

smklein · 2023-02-14T04:51:42Z

nexus/src/db/queries/external_ip.rs

-    //     <pool_restriction> AND
-    //     time_deleted IS NULL
+    // SELECT * FROM (
+    //   SELECT


If you're curious about how this works, there's an example fiddle which shows the logic:

https://www.db-fiddle.com/f/4jyoMCicNSZpjMt4jFYoz5/7205

Checking my understanding -- we're adding another nested subquery, so that we can filter the entire list of candidates by equality on the IP address. Is that accurate?

If so, I really wonder if we should restructure this. The main issue I have is that we're calling generate_series(0, last_address - first_address), and then filtering that. That range can be very big, and we're going to by definition select zero or one of them if the explicit IP is provided. As a example, if you change the IPs in your linked query to 10.0.0.1 and 10.0.255.255, it runs in 25ms. If you change the last address to 10.10.255.255, then it's 250ms. This is part of a larger issue with the performance of this query, but I think it'd make sense to improve it where we can.

This gets annoyingly more complicated, as any query with some conditional evaluation. But we could feasibly do something like this. That uses a case expression to select exactly the IP address we're asking for, if it's within the IP range, or NULL if it's not. I've not looked at how to embed that in the rest of the query, but there are similar queries in this file. I think the case where the user requested an IP that's not in the range would be handled in the same way as the error when the range is completely used. That could be good or bad, and we may need to have a different ELSE clause in that conditional evaluation to distinguish between them. I'm not sure we care.

Anyway, that's a lot of work, so I'll let you decide if you want to deal with it now. If not, let's at least add a note to this issue that this part of the query could be improved.

So I admittedly struggled a bit with this query, because:

On the one hand, I really want to have a "unified allocation flow" for IP allocation. I don't want to, for example, forget to update the collection rcgen for service IPs vs instance IPs...

... but on the other hand, yeah, scanning through addresses when we know we want to allocate one is kinda non-optimal.

I went ahead and used this optimized form in 1637370 , which more-or-less matches the fiddle you provided.

bnaecker

Nice work, lots of effort here! I have some questions about the details of the updated query, and a few nits, but overall looks excellent.

common/src/sql/dbinit.sql

nexus/src/db/datastore/rack.rs

bnaecker · 2023-02-14T19:28:50Z

nexus/src/db/queries/external_ip.rs

@@ -392,23 +393,34 @@ impl NextExternalIp {
    // each IP Pool, along with the pool/range IDs.


This docstring isn't accurate anymore. We're selecting zero or one IP address (and other candidate information) or the entire sequence, depending on whether explicit_ip is provided.

Updated in bc87a47

bnaecker · 2023-02-14T19:48:33Z

nexus/src/db/queries/external_ip.rs

-    //     <pool_restriction> AND
-    //     time_deleted IS NULL
+    // SELECT * FROM (
+    //   SELECT


Checking my understanding -- we're adding another nested subquery, so that we can filter the entire list of candidates by equality on the IP address. Is that accurate?

If so, I really wonder if we should restructure this. The main issue I have is that we're calling generate_series(0, last_address - first_address), and then filtering that. That range can be very big, and we're going to by definition select zero or one of them if the explicit IP is provided. As a example, if you change the IPs in your linked query to 10.0.0.1 and 10.0.255.255, it runs in 25ms. If you change the last address to 10.10.255.255, then it's 250ms. This is part of a larger issue with the performance of this query, but I think it'd make sense to improve it where we can.

This gets annoyingly more complicated, as any query with some conditional evaluation. But we could feasibly do something like this. That uses a case expression to select exactly the IP address we're asking for, if it's within the IP range, or NULL if it's not. I've not looked at how to embed that in the rest of the query, but there are similar queries in this file. I think the case where the user requested an IP that's not in the range would be handled in the same way as the error when the range is completely used. That could be good or bad, and we may need to have a different ELSE clause in that conditional evaluation to distinguish between them. I'm not sure we care.

Anyway, that's a lot of work, so I'll let you decide if you want to deal with it now. If not, let's at least add a note to this issue that this part of the query could be improved.

nexus/src/db/queries/external_ip.rs

nexus/types/src/internal_api/params.rs

sled-agent/src/rack_setup/service.rs

andrewjstone

This generally looks good to me. @bnaecker already did a thorough review and I'll let him take the lead here.

bnaecker · 2023-02-21T18:58:23Z

Thanks for all the rework @smklein, this all looks good to me! Feel free to merge at your discretion.

smklein added 3 commits February 9, 2023 13:36

Handoff IP, nexus information during rack initialization

32c79c1

Merge branch 'main' into initialize-nexus-service

6338f5e

more tests

c92deeb

smklein marked this pull request as ready for review February 13, 2023 20:40

smklein added nexus Related to nexus bootstrap services For those occasions where you want the rack to turn on labels Feb 13, 2023

smklein marked this pull request as draft February 13, 2023 20:55

smklein added 2 commits February 13, 2023 23:26

Allow explicit IP allocation requests

b385b67

Comment patch

3cc942a

smklein marked this pull request as ready for review February 14, 2023 04:42

Simplify multi-nexus test

74480c8

smklein commented Feb 14, 2023

View reviewed changes

smklein requested review from andrewjstone and bnaecker February 14, 2023 04:53

bnaecker reviewed Feb 14, 2023

View reviewed changes

smklein added 5 commits February 15, 2023 13:23

Merge branch 'main' into initialize-nexus-service

0cf0426

Clarify IDs

e8f3836

less comment stutter

c7e71be

External IP is not optional for Nexus

c5f3d15

ranges

5aaaa72

andrewjstone reviewed Feb 15, 2023

View reviewed changes

smklein added 5 commits February 20, 2023 10:13

Safer idempotent re-allocation

99ffdce

Optimized explicit IP query

1637370

Fix flake in rack_set_initialized_with_many_nexus_services test

5b9263f

Update comment

bc87a47

Const service pool name

64b0969

smklein merged commit b5ad4a8 into main Feb 21, 2023

smklein deleted the initialize-nexus-service branch February 21, 2023 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nexus] Populate IP pool, nexus service information, during rack setup #2358

[nexus] Populate IP pool, nexus service information, during rack setup #2358

smklein commented Feb 13, 2023 •

edited

Loading

smklein Feb 14, 2023 •

edited

Loading

bnaecker Feb 14, 2023

smklein Feb 20, 2023

bnaecker left a comment

bnaecker Feb 14, 2023

smklein Feb 20, 2023

bnaecker Feb 14, 2023

andrewjstone left a comment

bnaecker commented Feb 21, 2023

		@@ -392,23 +393,34 @@ impl NextExternalIp {
		// each IP Pool, along with the pool/range IDs.

[nexus] Populate IP pool, nexus service information, during rack setup #2358

[nexus] Populate IP pool, nexus service information, during rack setup #2358

Conversation

smklein commented Feb 13, 2023 • edited Loading

Summary

Nexus Changes

Database Records

IP Address Allocation

Rack Initialization

Populate

Sled Agent changes

smklein Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

bnaecker Feb 14, 2023

Choose a reason for hiding this comment

smklein Feb 20, 2023

Choose a reason for hiding this comment

bnaecker left a comment

Choose a reason for hiding this comment

bnaecker Feb 14, 2023

Choose a reason for hiding this comment

smklein Feb 20, 2023

Choose a reason for hiding this comment

bnaecker Feb 14, 2023

Choose a reason for hiding this comment

andrewjstone left a comment

Choose a reason for hiding this comment

bnaecker commented Feb 21, 2023

smklein commented Feb 13, 2023 •

edited

Loading

smklein Feb 14, 2023 •

edited

Loading