Add new multirack CRDB tables for unadopted racks and sleds by nickelization · Pull Request #11548 · oxidecomputer/omicron

nickelization · 2026-10-21T20:53:47Z

This doesn't do anything with the new tables yet, just adds them to the schema. We may not want to merge this until we're sure that this is precisely the schema we want, but putting this up for review gives us something to comment on and build off of in the short term.

For context, the plan is that Nexus will discover new sleds and racks via DDM, and will store records in these tables until we issue the command to complete the multirack join and adopt the new sleds and rack. Once the join is complete, we can delete these rows, and we'll ultimately end up with corresponding rows in the standard rack and sled tables.

We had previously hoped to handle this via the inventory system (hence #11496) but @andrewjstone realized more recently that this approach doesn't make sense since inventory collection only runs once every ten minutes, and we don't want to introduce that kind of delay into the multirack join process.

This doesn't do anything with the new tables yet, just adds them to the schema. We may not want to merge this until we're sure that this is the schema we want, but putting this up for review gives us something to comment on and build off of in the short term.

andrewjstone · 2026-10-21T22:48:30Z

We had previously hoped to handle this via the inventory system (hence #11496) but @andrewjstone realized more recently that this approach doesn't make sense since inventory collection only runs once every ten minutes, and we don't want to introduce that kind of delay into the multirack join process.

Actually @karencfv reminded me of that. Another issue however was how we are doing discovery changed. We are now going to know which sled is running the multirack join service via ddm, and we have a "completion" request that stops it when the user adopts the rack. So it's easier to just talk to it directly. However, we also need to record what we discover so we don't lose it later. Hence this data that we can use to feed API requests and background tasks.

andrewjstone

Looks pretty good. Just some minor changes requested. Happy to talk through them as needed.

andrewjstone · 2026-10-21T23:17:22Z

+    serial_number STRING(63) NOT NULL,
+    part_number STRING(63) NOT NULL,
+
+    ip INET NOT NULL


I'd call this sled_agent_ip

andrewjstone · 2026-10-21T23:24:27Z

+    serial_number STRING(63) NOT NULL,
+    part_number STRING(63) NOT NULL,
+
+    ip INET NOT NULL


We may want to provide a status about whether the sled-agent is running. It depends on how we're planning to use this. If we are planning to decouple the operator request for listing unadopted racks and their sleds from the request to get the status of the join service by just reading this table, we should have the latest polled status. I added support for the start status in #11535.

I think that's probably the right direction. We'd then have background task responsible for doing this polling after discovering a new rack subnet/sled-agent ip. Once all sled-agents start we'll allow the operator to go ahead and adopt the rack. If they don't all start eventually, we'll have a support issue. That bg task (or another one) can also attempt to send the completion event until success to the sled-agent hosting the join service when the operator selects adoption via the external API. It would then be responsible for publishing the bootstore generation and triggering the movement of these sleds into the sled table and the rack into the rack table. That should allow reconfigurator to take over.

Yeah, I like this plan. I would rather the unadopted sled/rack listing API be able to just query the table without having to chain in another separate API call to grab the sled-agent statuses. I'll go ahead and add the status to this table.

Would you say it's a good idea to add a new ENUM type in CRDB to represent this? (I would tend to assume that's a good way forward, but I haven't used CRDB a ton yet so I'm not sure if there might be any gotchas or anything around ENUMs that I'm not aware of.)

I'm not sure if you need an enum here, although I'm not against one. You could probably just use a bool started, with a nullable last_seen_error_while_starting or something like that.

@jgallagher What's your take?

Yeah having thought some more about this, I guess since the database doesn't have algebraic types for enums, a bool flag and a nullable error string probably do make more sense. I was also sort of thinking we might want to future-proof ourselves in case we want to represent more fine-grained startup states at some point in the future, but if we do decide to do that then it'll be easy enough to write a migration to turn the bool into an enum anyway, so probably better to keep it simple for now. (Though happy to change my mind if @jgallagher responds with a different take.)

if we do decide to do that then it'll be easy enough to write a migration to turn the bool into an enum anyway, so probably better to keep it simple for now

Yeah, this is the way I'd go, I think. A wrinkle here is that adding a new enum in a migration is really easy, but changing an existing enum can be a real pain.

Is last_start_error_seen still meaningful if sled_agent_started is true? (Wondering if there should be a check constraint preventing that combo.)

Is last_start_error_seen still meaningful if sled_agent_started is true? (Wondering if there should be a check constraint preventing that combo.)

It is not relevant at that point. A constraint would be good!

Yeah I'd thought about a constraint, but then figured we might want to keep the last-seen error around, in case somebody wants to be able to see what it was for some reason, even if the error went away and the sled-agent is now started. Admittedly that is sort of a potentially confusing/weird state for the table row to be in, though, and I assume the error would still be in the logs somewhere if somebody needed to see what it was for debugging reasons, so I'll go ahead and change this.

andrewjstone · 2026-10-21T23:29:24Z

+--
+-- If/when these sleds are adopted, these rows should be marked deleted, and
+-- there will subsequently be corresponding rows created in the `sled` and
+-- `inv_sled_agent` tables via the normal mechanisms.


I don't believe that sled-agents will be added to the sled-table via any normal process. We'll need to put them there in the background task after receiving the completion response that sets the bootstore generation. I opened an issue for this earlier today.

I take it back. The sled-agent always upserts itself once it can locate a nexus. That might be fine. What I'm worried about is the sled getting added to the sled-table before we are ready - mainly before the bootstore generation for this rack is updated and the rack is present in the rack table.

Ah right, yeah, I remember originally we were thinking about having a separate parallel insertion path but I figured if we're not inserting sled rows until after the sleds are adopted then yeah they'll naturally want to go ahead and upsert themselves once they can connect to Nexus, so that fits my mental model.

I'm less familiar with the bootstore stuff, so not sure if that throws a wrench in things, but if we just needed to wait for a record to be inserted in the rack table then I wonder if we'd just want to allow for returning a special status code to the sled-agent upsert request saying "we're not ready for this yet" (with a Retry-After header, maybe). I guess this could be a slightly tricky change to make from an update perspective, since we're talking about modifying the way an upcall works, but since multirack is still unreleased I suspect we could safely make these kinds of changes now since returning the new status code isn't a thing that would actually happen until a later release...?

This is also getting pretty tangential to this PR though so we can probably discuss this later, maybe in the sync meeting tonight.

Or, well, I don't know. I published that comment and then immediately started questioning this. Maybe the initial inserts into rack and sled are triggered by the user hitting the external API to finish rack adoption, and then only after that do we tell the new sleds about Nexus, after which the adopted sled-agents would continue upserting sled rows via the normal path as needed. TBD I guess.

I think it depends somewhat on how they find Nexus, which is DNS. Can they find DNS via the hardcoded addresses for the AZ? In any event you are right, we should discuss at huddle. I already added a note last night 😎 .

jgallagher · 2026-10-22T19:59:33Z

+
+    time_created TIMESTAMPTZ NOT NULL,
+    time_modified TIMESTAMPTZ NOT NULL,
+    time_deleted TIMESTAMPTZ,


It's fine to defer this for now, but just a heads-up that when you start writing queries against this and want to filter out deleted rows, you'll probably nee a unique partial index (lots of other examples in here though).

Ah cool, thanks for the pointer, I hadn't encountered/noticed that yet. Will add, either in this PR or the next one in the stack, but probably this one just to save the boilerplate of doing another migration later on.

jgallagher · 2026-10-22T20:00:09Z

+    sled_agent_started BOOL NOT NULL,
+    -- This nullable column should be set to the last error we've seen for this
+    -- particular sled's sled-agent startup process, if any:
+    last_start_error_seen STRING(512)


Could/should this be

Suggested change

last_start_error_seen STRING(512)

last_start_error_seen TEXT

? I'm not sure we know how long these errors could be, and I'm not sure there will be enough of them to worry about it if some are long.

Yeah, I figured 512 would be above the length of any error string we would reasonably expect to use here, but it seemed marginally safer to put some kind of cap on it since the CRDB docs alluded to long strings being bad for performance and stuff. I guess maybe you're right, though, this is probably a pretty remote possibility and arguably not worth limiting.

jgallagher · 2026-10-22T20:01:27Z

+
+    -- Underlay address of the particular sled-agent instance on the unadopted
+    -- rack that's running the multirack join service:
+    join_service_ip INET NOT NULL


Do we need a way to represent "the multirack join service failed"? I think in practice that means its rack will need to be clean-slated, which presumably means when it comes back it'll have a different ID and a new row in this table. But then how does the old one ever get deleted?

Yes, we need to be able to do this. We should discuss at multirack huddle.

jgallagher · 2026-10-22T20:03:20Z

+
+    -- This tracks whether this particular sled's sled-agent has been
+    -- successfully started by the multirack join service:
+    sled_agent_started BOOL NOT NULL,


I assume this is a "once it becomes true, we never set it to false again" column? (Wondering if there's any reason to guard against concurrent Nexuses operating on different views of a given sled-agent, but if we're going to "fuse" this column it's probably unnecessary. If we do end up wanting to record more varied states, we might need something?)

yeah, once it's true we're done.

No test for now, but that'll be coming later once we add the datastore code for this stuff.

nickelization self-assigned this Oct 21, 2026

nickelization added database Related to database access multi-rack labels Oct 21, 2026

nickelization requested a review from andrewjstone October 21, 2026 20:57

nickelization marked this pull request as ready for review October 21, 2026 21:02

andrewjstone reviewed Oct 21, 2026

View reviewed changes

nickelization added 6 commits October 22, 2026 12:38

Improve/fix comment on join_service_ip column per review comments

eb2ebaf

Remove unnecessary port column per review comments

bdbe9dd

Rename ip column per review

0b0ba42

Add sled-agent start status/error columns per review

b1bbed4

Add rack_id foreign key comments per review

d3844c6

Fix trailing commas in dbinit.sql

eede59c

jgallagher reviewed Oct 22, 2026

View reviewed changes

nickelization added 4 commits October 23, 2026 14:52

Fix trailing commas and missing semicolons

42cf02d

Add unique partial indexes for soft deletion columns

324d548

Remove error string length limit per review

8b69ba5

Add constraint around nulling start error once agent is started

3989c3e

No test for now, but that'll be coming later once we add the datastore code for this stuff.

andrewjstone approved these changes Oct 23, 2026

View reviewed changes

Fix missing paren in migration

2a6ffe6

nickelization mentioned this pull request Oct 23, 2026

Add db-model types and DataStore methods for new unadopted rack/sled tables #11585

Closed

Add generated migration verification SQL files

cefd279

nickelization merged commit f0b6107 into main Oct 26, 2026
19 checks passed

nickelization deleted the nick/unadopted-sled-and-rack-schemas branch October 26, 2026 13:57

nickelization mentioned this pull request Oct 28, 2026

Clean up unused tables for unadopted racks/sleds #11614

Open

Conversation

nickelization commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewjstone commented Oct 21, 2026

Uh oh!

andrewjstone left a comment

Choose a reason for hiding this comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nickelization commented Oct 21, 2026 •

edited

Loading