Optimizations to the Nexus schema tests by smklein · Pull Request #8271 · oxidecomputer/omicron

smklein · 2025-06-04T21:47:30Z

This PR optimizes several of the Nexus schema tests:

It uses a "baseline" version of dbinit.sql to avoid scaling linearly with the number of schema changes. It adds the cargo xtask schema generate-base command to help pull down this baseline.
This PR also adds cargo xtask schema old-migrations to assist with cleaning up old migrations that are before the "baseline", and are no longer supported.
It changes a few of the schema tests to compare "baseline" -> "latest" versions, rather than applying all schema changes that have ever been created.

 $ cargo nt -p omicron-nexus -- integration_tests::schema
 ...
        PASS [   2.152s] omicron-nexus::test_all integration_tests::schema::dbinit_version_matches_version_known_to_nexus
        PASS [   8.946s] omicron-nexus::test_all integration_tests::schema::nexus_applies_update_on_boot
        PASS [  10.831s] omicron-nexus::test_all integration_tests::schema::compare_table_differing_constraint
        PASS [  10.861s] omicron-nexus::test_all integration_tests::schema::compare_sequence_differing_increment
        PASS [  10.997s] omicron-nexus::test_all integration_tests::schema::compare_index_creation_differing_columns
        PASS [  11.043s] omicron-nexus::test_all integration_tests::schema::compare_view_differing_where_clause
        PASS [  11.075s] omicron-nexus::test_all integration_tests::schema::compare_table_differing_not_null_order
        PASS [  11.111s] omicron-nexus::test_all integration_tests::schema::compare_index_creation_differing_where_clause
        PASS [   8.621s] omicron-nexus::test_all integration_tests::schema::update_since_base_has_idempotent_up
        PASS [   8.008s] omicron-nexus::test_all integration_tests::schema::validate_data_migrations
        PASS [  30.662s] omicron-nexus::test_all integration_tests::schema::nexus_cannot_apply_update_from_unknown_version
        PASS [  23.131s] omicron-nexus::test_all integration_tests::schema::validate_migration_from_base_version
────────────
     Summary [  34.713s] 12 tests run: 12 passed, 604 skipped

Fixes #10005

dev-tools/xtask/src/schema.rs

nexus/tests/integration_tests/schema.rs

smklein · 2025-06-04T22:07:15Z

nexus/tests/integration_tests/schema.rs

-        // Used as a regression test against
-        // https://github.com/oxidecomputer/omicron/issues/5561


Full admission, I'm dropping this regression test. It's sorta clinging onto the side of this data migration test, but is actually not validating data migration.

It was originally added in #5565, and moved in #7471 , but it's painful to maintain, because it relies on "finding an enum that has been created, dropped, and re-created with the same name", and then applying all updates to the most recent schema so we can issue diesel commands against it.

This cache has been gone for over a year - I think we can re-visit this if/when we ever add back an OID cache.

dev-tools/xtask/Cargo.toml

david-crespo · 2025-06-26T15:56:56Z

Noting that the kv.raft_log.disable_synchronization_unsafe change mentioned in the description was pulled out to #8275.

smklein · 2025-06-26T17:05:06Z

Noting that the kv.raft_log.disable_synchronization_unsafe change mentioned in the description was pulled out to #8275.

Good call, updated the description

david-crespo · 2025-06-26T17:38:05Z

schema/crdb/README.adoc

   `NEW_VERSION` and still appears at the top of the list (logically after the
   new version that came in from "main").
 * Update the version in `dbinit.sql` to match the new `NEW_VERSION`.
+* Re-run `cargo xtask schema generate-previous`.


Curious what is supposed to happen (like in CI) if you fail to do this

davepacheco · 2025-06-26T18:05:41Z

Should we consider just removing old schema versions altogether?

I'm imagining something where:

at runtime, instead of starting with schema/crdb/1.0.0/up.sql, we start with a new file called schema/crdb/dbinit-base.sql (initial contents would be copied from schema/crdb/1.0.0/up.sql)
immediately after cutting release N, we:
- copy the dbinit.sql file from release N - 1 to dbinit-base.sql
- delete all the schema versions (directories and entries from schema_versions.rs) that were present in release N - 1 (because those are incorporated into its dbinit-base.sql)

Concretely, say that today:

release 10: contains schema versions 1-12 + dbinit.sql representing version 12
release 11: contains schema versions 1-15 + dbinit.sql representing version 15
release 12: contains schema versions 1-20 + dbinit.sql representing version 20
main: contains schema versions 1-23 + dbinit.sql representing version 23

In this world:

release 11:
- contains dbinit-base.sql that's equivalent to dbinit.sql from version 12 (the one from release 10)
- contains schema migrations for versions 13 - 15
- contains dbinit.sql representing version 15
release 12:
- contains dbinit-base.sql that's equivalent to dbinit.sql from version 15 (the one from release 11)
- contains schema migrations for versions 16-20
- contains dbinit.sql representing version 20
main:
- contains dbinit-base.sql that's equivalent to dbinit.sql from version 20 (the one from release 12)
- contains schema migrations for versions 21-23
- contains dbinit.sql representing version 23

Basically:

dbinit.sql is exactly as it is today: it's a human-readable version of the latest schema
the migrations are just as they are today and the process for adding one is the same as today
the only difference is that we keep updating a "base" file with each release and have the tests use that

I think it's also pretty easy to do mechanically, though probably annoying to automate.

smklein · 2025-06-30T21:14:28Z

Should we consider just removing old schema versions altogether?

I'm imagining something where:

at runtime, instead of starting with schema/crdb/1.0.0/up.sql, we start with a new file called schema/crdb/dbinit-base.sql (initial contents would be copied from schema/crdb/1.0.0/up.sql)

immediately after cutting release N, we:

copy the dbinit.sql file from release N - 1 to dbinit-base.sql

delete all the schema versions (directories and entries from schema_versions.rs) that were present in release N - 1 (because those are incorporated into its dbinit-base.sql)

Concretely, say that today:

release 10: contains schema versions 1-12 + dbinit.sql representing version 12

release 11: contains schema versions 1-15 + dbinit.sql representing version 15

release 12: contains schema versions 1-20 + dbinit.sql representing version 20

main: contains schema versions 1-23 + dbinit.sql representing version 23

In this world:

release 11:

contains dbinit-base.sql that's equivalent to dbinit.sql from version 12 (the one from release 10)

contains schema migrations for versions 13 - 15

contains dbinit.sql representing version 15

release 12:

contains dbinit-base.sql that's equivalent to dbinit.sql from version 15 (the one from release 11)

contains schema migrations for versions 16-20

contains dbinit.sql representing version 20

main:

contains dbinit-base.sql that's equivalent to dbinit.sql from version 20 (the one from release 12)

contains schema migrations for versions 21-23

contains dbinit.sql representing version 23

Basically:

dbinit.sql is exactly as it is today: it's a human-readable version of the latest schema

the migrations are just as they are today and the process for adding one is the same as today

the only difference is that we keep updating a "base" file with each release and have the tests use that

I think it's also pretty easy to do mechanically, though probably annoying to automate.

Sounds totally reasonable to me. I might tweak some of the names and things, but if we're fine dropping old support beyond the past release, this seems like it would clean up a lot, and let us release a lot of the data migration tests too.

I'll give this a swing. I think I can modify the existing xtask to handle a lot of the functionality of "check out the base dbinit we want".

smklein · 2025-07-01T01:16:49Z

Okay, went ahead and implemented this, using rel/v15/rc1 (aka 189adbb0b39f3c1c4ac2371aaf4cac18c571f9bb) as our "baseline" dbinit.sql -- but it would be very straightforward to select something else as the "oldest-supported version", if we'd like.

…hanges

smklein · 2026-03-26T20:02:23Z

Okay! This PR should be updated. Of note:

I'm still moving the baseline to rel/v15/rc1. We could move it further forward. Happy to do this in a follow-up PR -- the meat of this PR is "building the mechanism", which should make subsequent "base-moving" easier. (edit: tested throwing claude at this for an upgrade to v16, and it aced it first try: Update dbinit-base to rel/v16.1/rc0 #10165)
I've updated the docs to include instructions of "how to move forward the baseline". IMO the most manual part of this, for now, is figuring out how to clean up the data migration tests. I'm going to try to make this easier in a follow-up PR (now done in Improve ergonomics of data migration test #10164).

All that said, this should be ready for review!

mergeconflict

Nice!

mergeconflict · 2026-03-27T03:10:22Z

nexus/tests/integration_tests/schema.rs

+    // Parse the base schema version from the header
+    for line in content.lines() {
+        if line.starts_with("-- Schema version:") {
+            let version_str = line
+                .strip_prefix("-- Schema version:")
+                .ok_or_else(|| {
+                    anyhow::anyhow!("Invalid schema version line format")
+                })?
+                .trim();
+            return semver::Version::parse(version_str).with_context(|| {
+                format!("Failed to parse base schema version: {}", version_str)
+            });
+        }
+    }


Nit: this duplicates parse_base_schema_version in dev-tools/schema/src/main.rs. I wonder if there's a reasonable way to have these share code? Possibly not.

Pulled it into nexus/db-model, since that's a common dependency, and it's a cheap function to compile

Nice, ok. Wasn't sure offhand if there were any sensible deps in common.

mergeconflict · 2026-03-27T03:12:19Z

dev-tools/schema/src/main.rs

+fn find_workspace_root() -> Result<Utf8PathBuf> {
+    let current_dir =
+        std::env::current_dir().context("Failed to get current directory")?;
+    let current_dir = Utf8PathBuf::try_from(current_dir)
+        .context("Current directory path is not valid UTF-8")?;
+
+    current_dir
+        .ancestors()
+        .find(|p| p.join("Cargo.toml").exists() && p.join(".git").exists())
+        .map(|p| p.to_path_buf())
+        .ok_or_else(|| anyhow::anyhow!("Could not find workspace root"))
+}


I think you can use cargo locate-project --workspace here.

Neat; updated to use this in 777a605

mergeconflict · 2026-03-27T03:19:55Z

dev-tools/schema/src/main.rs

+        println!(
+            "  - Update EARLIEST_SUPPORTED_VERSION in schema_versions.rs if needed"
+        );


I wonder if we should actually bail here if EARLIEST_SUPPORTED_VERSION != base_version?

Added this check in 777a605

mergeconflict · 2026-03-27T03:21:07Z

nexus/tests/integration_tests/schema.rs

+    #[allow(dead_code)]
+    log: &'a Logger,


Did you intend to keep this around? I think it's unused.

mergeconflict · 2026-03-27T03:22:27Z

schema/crdb/README.adoc

+==== Updating the baseline after a release
+
+After each Omicron release, update the baseline to match the previous release.
+The "baseline" will become the new "earlist supported CockroachDB schema version",


Typo: earlist

mergeconflict · 2026-03-27T03:25:58Z

dev-tools/schema/src/main.rs

+
+    let file_content = String::from_utf8(show_output.stdout)?;
+
+    // Parse the SCHEMA_VERSION from the file


This feels a bit fragile, although I can't really imagine a better way to do it. I wonder, if it fails for whatever reason, should we bail rather than returning Ok(None)?

I can just make get_schema_version_from_commit return a Result<Version>, and have this case be an error.

Updated in 777a605

smklein added 11 commits June 3, 2025 12:33

[schema] Clean M.2s out of managed control plane disks/zpools

930dfa7

compile pls

0836780

table scans, tests

3009f8f

comment

0fa1c73

idempotency

2943dd3

Test migration from 'last schema version'

fe8734c

read schema list

0a204d3

Passing data migration tests

3ba9e24

More schema checks, optimizations

0be4bbd

patch insert disks tests - u.2s only

97a308a

Merge branch 'phys-disk-table-for-u.2s-only' into schema-optimization

d2f6431

smklein commented Jun 4, 2025

View reviewed changes

smklein added 9 commits June 4, 2025 15:10

pq-sys

b336a81

Merge branch 'main' into phys-disk-table-for-u.2s-only

c0d9f87

Merge branch 'phys-disk-table-for-u.2s-only' into schema-optimization

073f5e6

schema update

2562efe

docs

5d0bbeb

fix migration tests

6685e9d

Merge branch 'phys-disk-table-for-u.2s-only' into schema-optimization

d628b06

comment

4ed2555

Simplify test output, order it

4c30498

smklein marked this pull request as ready for review June 5, 2025 17:24

Base automatically changed from phys-disk-table-for-u.2s-only to main June 5, 2025 18:09

smklein commented Jun 6, 2025

View reviewed changes

dev-tools/xtask/Cargo.toml Outdated Show resolved Hide resolved

smklein added 3 commits June 6, 2025 12:24

Merge branch 'main' into phys-disk-table-for-u.2s-only

1e50b29

Merge branch 'phys-disk-table-for-u.2s-only' into schema-optimization

1dacdcd

Move xtask schema to new binary

7637e78

smklein requested review from davepacheco and sunshowers June 6, 2025 19:40

Merge branch 'main' into schema-optimization

36e5c6c

Update previous-schema

6d96403

david-crespo reviewed Jun 26, 2025

View reviewed changes

Merge branch 'main' into schema-optimization

ca86be2

smklein added 3 commits June 30, 2025 18:03

Remove old schemas

7f523d3

Remove tests relying on old migrations

a620437

simplify parsing

b5d49e4

smklein mentioned this pull request Aug 1, 2025

[nexus] Add schema changes for quiescing #8533

Closed

6 tasks

smklein mentioned this pull request Mar 26, 2026

nexus: reuse single DB connection during schema migration #10159

Merged

smklein added 6 commits March 26, 2026 09:56

merge

065b4d7

fix test_migration_verification_files

5f86c59

cleaning up some dead code

8298d25

Add old-migration check to CI, run it to remove 'merged' old schema c…

ac02bc0

…hanges

merge

0d46edc

Improve docs, links

63ae9a7

smklein mentioned this pull request Mar 26, 2026

Improve ergonomics of data migration test #10164

Open

smklein requested a review from mergeconflict March 26, 2026 20:54

mergeconflict approved these changes Mar 27, 2026

View reviewed changes

review feedback

777a605

smklein merged commit 74b7742 into main Mar 27, 2026
18 checks passed

smklein deleted the schema-optimization branch March 27, 2026 22:10

		// Used as a regression test against
		// https://github.com/oxidecomputer/omicron/issues/5561


		let file_content = String::from_utf8(show_output.stdout)?;

		// Parse the SCHEMA_VERSION from the file

Conversation

smklein commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

david-crespo commented Jun 26, 2025

Uh oh!

smklein commented Jun 26, 2025

Uh oh!

david-crespo Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco commented Jun 26, 2025

Uh oh!

smklein commented Jun 30, 2025

Uh oh!

smklein commented Jul 1, 2025

Uh oh!

smklein commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergeconflict left a comment

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smklein commented Jun 4, 2025 •

edited

Loading

david-crespo Jun 26, 2025 •

edited

Loading

smklein commented Mar 26, 2026 •

edited

Loading