Serialize blueprints in the database #4899

jgallagher · 2024-01-25T20:11:19Z

This replaces the in-memory blueprint storage added as a placeholder in #4804 with cockroachdb-backed tables. Both the tables and related queries are heavily derived from the similar tables in the inventory system (particularly serializing omicron zones and their related properties). The tables are effectively identical as of this PR, but we opted to keep the separate because we expect them to diverge some over time (e.g., inventory might start collecting additional per-zone properties that don't exist for blueprints, such as uptime).

The big exception to "basically the same as inventory" is the bp_target table which tracks the current (and past) target blueprint. Inserting into this table has some subtleties, and we use a CTE to check and enforce the invariants. This is the first diesel/CTE I've written; it's based on other similar CTEs in Nexus, but I'd still appreciate a particularly careful look there.

Fixes #4793.

These were performed by nexus when blueprints were in memory, but are now performed by the datastore

davepacheco

Nice! I checked everything here except the function that writes out the SQL directly. I can come back to this but wanted to get this in now.

nexus/src/app/deployment.rs

davepacheco · 2024-01-25T22:33:20Z

nexus/src/app/deployment.rs

-            .get(&blueprint_id)
-            .cloned()
-            .ok_or_else(|| blueprint.not_found())
+        self.db_datastore.blueprint_read(opctx, &blueprint).await


We could consider making BlueprintMetadata a resource with its own authz type and lookup type so that people could use the usual LookupPath::new(..).blueprint_id(blueprint_id).fetch().await API instead of a function like this. I'm not sure if that's a good idea and it's definitely not important now.

nexus/src/app/deployment.rs

schema/crdb/dbinit.sql

davepacheco · 2024-01-25T22:47:46Z

nexus/db-model/src/deployment.rs

+#[derive(Queryable, Clone, Debug, Selectable, Insertable)]
+#[diesel(table_name = bp_target)]
+pub struct BpTarget {
+    pub version: i64, // i64 only for db serialization; should never be negative


Should this use one of the SqlU* types instead?

Sure, switched to SqlU32. I wasn't 100% sure that was big enough to never worry about exhausting it, but I think I've convinced myself it's fine. Even if runaway automation set a new target every second (which seems pretty unlikely, since that first requires generating and inserting a new blueprint in its entirety), u32 is fine for 100+ years.

nexus/db-model/src/deployment.rs

davepacheco · 2024-01-25T23:03:26Z

nexus/db-queries/src/db/datastore/deployment.rs

+// Error messages generated from the above sentinel values.
+const NO_SUCH_BLUEPRINT_ERROR_MESSAGE: &str =
+    "could not parse \"no-such-blueprint\" as type uuid: \
+     uuid: incorrect UUID length: no-such-blueprint";
+const PARENT_NOT_TARGET_ERROR_MESSAGE: &str =
+    "could not parse \"parent-not-target\" as type uuid: \
+     uuid: incorrect UUID length: parent-not-target";


I am not sure these are stable messages. But I guess as long as we test the behavior, we should be fine.

Agreed on both counts. FWIW this is not a new technique; I cribbed it from

omicron/nexus/db-queries/src/db/queries/network_interface.rs

Lines 89 to 94 in d5dace6

// Error message generated when we're attempting to operate on an instance,

// either inserting or deleting an interface, and that instance does not exist

// at all or has been destroyed. These are the same thing from the point of view

// of the client's API call.

const NO_INSTANCE_ERROR_MESSAGE: &'static str =

"could not parse \"no-instance\" as type uuid: uuid: incorrect UUID length: no-instance";

, which also has tests covering it.

andrewjstone

Epic work @jgallagher. I particularly like the tests!

andrewjstone · 2024-01-25T22:45:04Z

nexus/db-model/src/deployment.rs

+    pub bp_nic_id: Option<Uuid>,
+    pub dns_gz_address: Option<ipv6::Ipv6Addr>,
+    pub dns_gz_address_index: Option<SqlU32>,
+    pub ntp_ntp_servers: Option<Vec<String>>,


Is ntp_ntp intentional?

It is - these are NTP zone's NTP servers.

schema/crdb/dbinit.sql

andrewjstone · 2024-01-25T23:56:37Z

nexus/db-model/src/omicron_zone_config.rs

+use uuid::Uuid;
+
+#[derive(Debug)]
+pub(crate) struct OmicronZone {


Just a question here, nothing to do in response.

It looks like this is essentially flattening the OmicronZoneType enum into a struct full of options. Is this because it's not really possible to represent ZoneType in the DB without flattening?

It's not that it's not possible, it's just that it's a lot of work with dubious payoff. At one point I think Dave had 3 different impls at least partly done / sketched out: fully (or mostly) normalized, this one, and "just stuff JSON in there". This seemed like the most reasonable one: we get almost all the benefits of putting this into the DB schema, without all the diesel pain of working with a dozen tables.

Ah, got it. This is what you guys were discussing the other day. Thanks.

andrewjstone · 2024-01-26T15:40:21Z

nexus/db-model/src/omicron_zone_config.rs

+                ensure!(expected_id == nic_row.id, "caller provided wrong NIC");
+                Ok(nic_row.into_network_interface_for_zone(self.id)?)
+            }
+            (None, None) => Err(anyhow!(


Long rambling nit below:

I was confused by the (None, None) case, even though it's documented well in the block comment. I think this was partially my brain not functioning when reviewing yesterday and it became clear when I read the rest of the function.

However, while I like the pattern I still find it somewhat confusing to mix an early return of a result with an error that will be returned later. I think this could be made clearer if you inlined a comment for the (None, None) case. Maybe that's just moving the last paragraph of the block comment and slightly rewording it. While I typically dislike inlined comments like this for small match statements it may work better here. I'm interested in other's opinions.

Added an inline comment in 0c14431

nexus/db-queries/src/db/datastore/deployment.rs

andrewjstone · 2024-01-26T17:13:47Z

nexus/db-queries/src/db/datastore/deployment.rs

+    ) {
+        let conn = datastore.pool_connection_for_tests().await.unwrap();
+
+        macro_rules! query_count {


From embedded closures to embedded macros, the John Gallagher story.

…se-1

jgallagher added 11 commits January 25, 2024 14:21

add blueprints to database schema and models

6845680

add queries to read/write/delete blueprints from the db

07e2cef

Add queries for getting and setting the current blueprint target

8d3cc25

doc comment fixes

d5c3d24

sort zones in blueprint builder for testing niceness

bfe97bb

move schema to rebase onto main

eaafb23

replace nexus in-memory blueprints with database

2bf00ff

remove duplicate authz checks

db84a5d

These were performed by nexus when blueprints were in memory, but are now performed by the datastore

minor cleanup, mostly comments

c5e4718

remove unused method

0681a8d

fix nondeterministic test

e9d4bb1

jgallagher requested review from sunshowers, andrewjstone and davepacheco January 25, 2024 20:11

add missing migration file

cfe797a

davepacheco reviewed Jan 25, 2024

View reviewed changes

jgallagher added 5 commits January 26, 2024 11:29

remove "one we store blueprints in the db" comments

afc7243

restore placeholder comment for blueprint execution

dccd0d2

comment cleanup

ff88b37

rename time_set -> time_made_target

14e7d5a

use SqlU32 instead of not-really-signed i64

02d0540

andrewjstone reviewed Jan 26, 2024

View reviewed changes

davepacheco approved these changes Jan 26, 2024

View reviewed changes

jgallagher added 7 commits January 26, 2024 13:58

fix OpenAPI spec

f2f445c

add clarifying comment

0c14431

typo fix

fe8dcb0

address "open question" comments

45d0421

remove extra-cautious primary key check

bf84a0e

cargo fmt

17abc3f

Merge remote-tracking branch 'origin/main' into john/blueprint-databa…

b3799f2

…se-1

catch rename in omdb

df8a9e3

jgallagher merged commit 80cc001 into main Jan 26, 2024
22 checks passed

jgallagher deleted the john/blueprint-database-1 branch January 26, 2024 21:44

andrewjstone added a commit that referenced this pull request Jan 27, 2024

Use DB backed blueprints from #4899

24b34f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize blueprints in the database #4899

Serialize blueprints in the database #4899

jgallagher commented Jan 25, 2024

davepacheco left a comment

davepacheco Jan 25, 2024

davepacheco Jan 25, 2024

jgallagher Jan 26, 2024

davepacheco Jan 25, 2024

jgallagher Jan 26, 2024

andrewjstone left a comment

andrewjstone Jan 25, 2024

jgallagher Jan 26, 2024

andrewjstone Jan 25, 2024

jgallagher Jan 26, 2024

andrewjstone Jan 26, 2024

andrewjstone Jan 26, 2024

jgallagher Jan 26, 2024

andrewjstone Jan 26, 2024

	// Error message generated when we're attempting to operate on an instance,
	// either inserting or deleting an interface, and that instance does not exist
	// at all or has been destroyed. These are the same thing from the point of view
	// of the client's API call.
	const NO_INSTANCE_ERROR_MESSAGE: &'static str =
	"could not parse \"no-instance\" as type uuid: uuid: incorrect UUID length: no-instance";

Serialize blueprints in the database #4899

Serialize blueprints in the database #4899

Conversation

jgallagher commented Jan 25, 2024

davepacheco 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

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

Choose a reason for hiding this comment