Skip to content

Optimizations to the Nexus schema tests #8271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Jun 4, 2025

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

Comment on lines -1332 to -1333
// Used as a regression test against
// https://github.com/oxidecomputer/omicron/issues/5561
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@smklein 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 smklein requested review from sunshowers and davepacheco June 6, 2025 19:40
@david-crespo
Copy link
Contributor

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

@smklein
Copy link
Collaborator Author

smklein commented Jun 26, 2025

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

Good call, updated the description

@@ -125,6 +127,7 @@ same `NEW_VERSION`:**, then your `OLD_VERSION` has changed and so _your_
`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`.
Copy link
Contributor

@david-crespo david-crespo Jun 26, 2025

Choose a reason for hiding this comment

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

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

@davepacheco
Copy link
Collaborator

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
Copy link
Collaborator Author

smklein commented Jun 30, 2025

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
Copy link
Collaborator Author

smklein commented Jul 1, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants