Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
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
21 changes: 21 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,21 @@
-- 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,

-- 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,
Comment thread
andrewjstone marked this conversation as resolved.
Resolved
)
30 changes: 30 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,30 @@
-- 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,
-- Foreign key into the `unadopted_rack` table (though, also, the rack ID
-- that gets used in the `rack` table should be the same if/when the rack
-- is adopted)
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,

sled_agent_ip INET NOT NULL,

-- This tracks whether this particular sled's sled-agent has been
-- successfully started by the multirack join service:
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),
)
54 changes: 53 additions & 1 deletion schema/crdb/dbinit.sql
Original file line number Diff line number Diff line change
Expand Up @@ -9730,6 +9730,58 @@ 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,

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

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.

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?

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.

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

);

-- 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,
-- Foreign key into the `unadopted_rack` table (though, also, the rack ID
-- that gets used in the `rack` table should be the same if/when the rack
-- is adopted)
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,

sled_agent_ip INET NOT NULL,

-- This tracks whether this particular sled's sled-agent has been
-- successfully started by the multirack join service:
sled_agent_started BOOL 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 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?)

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.

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

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

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.

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.

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

);

-- 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 +9792,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