Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion nexus/db-model/src/schema_versions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock};
///
/// This must be updated when you change the database schema. Refer to
/// schema/crdb/README.adoc in the root of this repository for details.
pub const SCHEMA_VERSION: Version = Version::new(309, 0, 0);
pub const SCHEMA_VERSION: Version = Version::new(310, 0, 0);

/// List of all past database schema versions, in *reverse* order
///
Expand All @@ -28,6 +28,7 @@ pub static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = LazyLock::new(|| {
// | leaving the first copy as an example for the next person.
// v
// KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
KnownVersion::new(310, "add-unadopted-sled-and-rack-tables"),
KnownVersion::new(309, "fm-certificate-de"),
KnownVersion::new(308, "inv-power-shelf-psu"),
KnownVersion::new(307, "add-rack-id-to-inv-sled-agent"),
Expand Down
24 changes: 24 additions & 0 deletions schema/crdb/add-unadopted-sled-and-rack-tables/up01.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
-- Tracks other racks that have joined the underlay network but haven't been
-- adopted into the multirack cluster.
--
-- If/when a rack is adopted, its row in this table should be marked deleted,
-- and a corresponding row should be inserted into the `rack` table.
CREATE TABLE IF NOT EXISTS omicron.public.unadopted_rack (
-- This should be the same id that gets used for the `rack` table, as well
-- as the `rack_id` column in `unadopted_sled`:
id UUID PRIMARY KEY,

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

-- The IPv6 underlay /56 prefix for the rack:
rack_subnet INET NOT NULL,

-- Socket address of the multirack join service running on a sled-agent in
-- the unadopted rack:
join_service_ip INET NOT NULL,
Comment thread
andrewjstone marked this conversation as resolved.
Resolved
join_service_port INT4
CHECK (join_service_port BETWEEN 0 AND 65535)
NOT NULL
)
20 changes: 20 additions & 0 deletions schema/crdb/add-unadopted-sled-and-rack-tables/up02.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
-- Tracks sleds from other racks that have joined the underlay network but
-- haven't been adopted into the multirack cluster.
--
-- 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.
CREATE TABLE IF NOT EXISTS omicron.public.unadopted_sled (
id UUID PRIMARY KEY,
rack_id UUID NOT NULL,
Comment thread
andrewjstone marked this conversation as resolved.
Resolved

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

-- Baseboard information about the sled
serial_number STRING(63) NOT NULL,
part_number STRING(63) NOT NULL,

ip INET NOT NULL

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call this sled_agent_ip

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)
47 changes: 46 additions & 1 deletion schema/crdb/dbinit.sql
Original file line number Diff line number Diff line change
Expand Up @@ -9730,6 +9730,51 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_config (
)
);

-- Tracks other racks that have joined the underlay network but haven't been
-- adopted into the multirack cluster.
--
-- If/when a rack is adopted, its row in this table should be marked deleted,
-- and a corresponding row should be inserted into the `rack` table.
CREATE TABLE IF NOT EXISTS omicron.public.unadopted_rack (
-- This should be the same id that gets used for the `rack` table, as well
-- as the `rack_id` column in `unadopted_sled`:
id UUID PRIMARY KEY,

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


-- The IPv6 underlay /56 prefix for the rack:
rack_subnet INET NOT NULL,

-- Socket address of the multirack join service running on a sled-agent in
-- the unadopted rack:
join_service_ip INET NOT NULL,
join_service_port INT4
CHECK (join_service_port BETWEEN 0 AND 65535)
NOT NULL
);

-- Tracks sleds from other racks that have joined the underlay network but
-- haven't been adopted into the multirack cluster.
--
-- 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😎 .

CREATE TABLE IF NOT EXISTS omicron.public.unadopted_sled (
id UUID PRIMARY KEY,
rack_id UUID NOT NULL,

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

-- Baseboard information about the sled
serial_number STRING(63) NOT NULL,
part_number STRING(63) NOT NULL,

ip INET NOT NULL
);

-- Keep this at the end of file so that the database does not contain a version
-- until it is fully populated.
Expand All @@ -9740,7 +9785,7 @@ INSERT INTO omicron.public.db_metadata (
version,
target_version
) VALUES
(TRUE, NOW(), NOW(), '309.0.0', NULL)
(TRUE, NOW(), NOW(), '310.0.0', NULL)
ON CONFLICT DO NOTHING;

COMMIT;
Loading