Check DB schema version at startup #1272

tgeoghegan · 2023-04-18T15:55:31Z

Creating a Datastore now checks the version of the most recently applied migration script against an array of supported versions compiled into Janus. If the current schema is not supported, Datastore::new returns an error.

Resolves #1241

tgeoghegan · 2023-04-18T22:10:44Z

Question for reviewers: what do we think about baking the list of supported schema versions into the binary versus making it a config option? I think I favor the former (which is what I've implemented here) because I think whether a given version of Janus can work with a given schema version is a static property of the code, not a configuration decision. And doing it this way means we don't have to wrangle another configuration option in all our deploys. But maybe we'll be glad during some future incident to be able to force Janus to try running on an arbitrary schema version we didn't know about at the time of compiling Janus?

branlwyd · 2023-04-18T22:04:35Z

interop_binaries/config/aggregation_job_creator.yaml

@@ -3,6 +3,7 @@
 # tests, and is not intended for production use.
 database:
  url: postgres://postgres@127.0.0.1:5432/postgres
+  check_schema_version: false


Q: why do we need to set check_schema_version to false in various interop binaries? Is the schema populated in a different way there?

It's because the interop binaries use janus_aggregator::binary_utils::janus_main, just like the "real" Janus components, and that function is responsible for instantiating a Datastore before any of the specific application's code gets to run. The interop binaries must opt out of schema version checks so that they can get to their own code (the function pointer invoked by janus_main), which will apply the migration scripts in the container image.

branlwyd · 2023-04-18T22:09:18Z

aggregator_core/src/datastore.rs

@@ -61,6 +61,10 @@ pub mod test_util;

 // TODO(#196): retry network-related & other transient failures once we know what they look like

+/// List of schema versions that this version of Janus can safely run on. If any other schema
+/// version is seen, [`Datastore::new`] fails.
+const SUPPORTED_SCHEMA_VERSIONS: &[i64] = &[20230405185602, 20230417204528];


It's easy to see when we'd add a new version to SUPPORTED_SCHEMA_VERSIONS (presumably, when we're creating a new DB migration, and we'd be reminded to do so because tests would begin failing until we do so).

But what's the process for removing a version from this list: how do we evaluate when it's correct to remove an item? more importantly, what reminds us to remove versions?

(somewhat separately from this, I wonder if it would be useful to have an env var or other configuration knob that controls which DB schema version is used for ephemeral datastores. this could be a useful tool, but not a complete solution, in evaluating if a given schema version still works with a given revision of the code.)

It's easy to see when we'd add a new version to SUPPORTED_SCHEMA_VERSIONS (presumably, when we're creating a new DB migration, and we'd be reminded to do so because tests would begin failing until we do so).

But what's the process for removing a version from this list: how do we evaluate when it's correct to remove an item? more importantly, what reminds us to remove versions?

With things as they stand in this PR, it's up to the person making a change to remove schema versions from the supported list. That does mean that nothing will remind us or force us to drop unsupported schemas. What would be really nice -- and I think this also speaks to your idea about configuring what schema version tests uses -- is for tests that involve the datastore to automatically be run against all the schema versions that are listed in this array. I think what we'd need is a macro for tests that use storage. The macro would generate code to instantiate an ephemeral datastore with the appropriate schema version, and it could also stamp out multiple tests, each one using a different supported version.

Then we'd get real assurance from the tests that Janus functions with all the schema versions we claim are supported, and it'd be completely obvious when an older schema is not supported: you'd see a plain test failure, and then could decide whether to drop that schema version or not.

This macro should be a proc-macro in function attribute position, as declarative macros would get in the way of code formatting or IDE features. We could replace tokio::test macro calls with it, and add a Datastore function argument to test functions with the new macro. We should probably pass in the SUPPORTED_SCHEMA_VERSIONS identifier to the attribute macro, rather than try to fight macro hygiene.

In order to properly sequence deployment of new schema versions, I think we'll need a db-next folder, akin to Boulder. We'd run sqlx migrate add with db-next as the source directory. Tests would dynamically choose subsets of this folder, depending on different values in SUPPORTED_SCHEMA_VERSIONS. Then, we can land a new migration, while testing against it, and release and deploy the addition to SUPPORTED_SCHEMA_VERSIONS before we put the new migration in our production bucket with the following release.

I'm experimenting with rstest for this, and it looks pretty promising! edit: #1277

branlwyd · 2023-04-18T22:16:02Z

aggregator_core/src/datastore.rs

+    /// The version of the DB schema found in `_sqlx_migrations` is not supported by this version of
+    /// Janus.
+    #[error("unsupported schema version {version} / {description}")]
+    UnsupportedSchemaVersion { version: i64, description: String },


Does this need to be a separately-discriminated error? It's hard to imagine we'll ever do anything with this error other than report it to a human being.

If we don't think we'll make program-logic decisions based on this error discriminant, let's introduce an Other value that wraps an anyhow::Error.

In that case we may as well use Error::DbState and represent the error as a String.

This works, but my point is: I'm not sure we need a separate DbState error discriminant, either -- we generate a number of DbState errors, but nothing ever consumes a DbState error to make a program-logic decision, so DbState should ideally also be Other.

I don't think we need to clean up existing error discriminants, but there's no reason to introduce additional unnecessary discriminants, either.

[I'm not married to the idea of using anyhow::Error as the internal representation--String would work too. I guess I see the advantage of anyhow::Error as having a nice API & providing error-wrapping.]

branlwyd · 2023-04-18T22:18:55Z

Question for reviewers: what do we think about baking the list of supported schema versions into the binary versus making it a config option? I think I favor the former (which is what I've implemented here) because I think whether a given version of Janus can work with a given schema version is a static property of the code, not a configuration decision. And doing it this way means we don't have to wrangle another configuration option in all our deploys. But maybe we'll be glad during some future incident to be able to force Janus to try running on an arbitrary schema version we didn't know about at the time of compiling Janus?

I'm OK baking in a static list of versions, as well as a boolean flag to disable version-checking entirely, as is implemented in this PR.

Creating a `Datastore` now checks the version of the most recently applied migration script against an array of supported versions compiled into Janus. If the current schema is not supported, `Datastore::new` returns an error. Also adds a configuration option to disable this new feature. This is needed because the Janus interop binaries apply schema migrations at startup, after being handed control by `janus_main`. Resolves #1241

tgeoghegan requested a review from a team as a code owner April 18, 2023 15:55

tgeoghegan force-pushed the timg/startup-check-schema-version branch from b2e096a to 1e1a9fa Compare April 18, 2023 16:05

divergentdave approved these changes Apr 18, 2023

View reviewed changes

branlwyd reviewed Apr 18, 2023

View reviewed changes

tgeoghegan requested review from branlwyd and divergentdave April 19, 2023 21:58

tgeoghegan mentioned this pull request Apr 24, 2023

Run datastore tests against multiple schemas #1277

Merged

branlwyd approved these changes Apr 24, 2023

View reviewed changes

divergentdave approved these changes Apr 24, 2023

View reviewed changes

tgeoghegan force-pushed the timg/startup-check-schema-version branch from 47264b0 to 9c1457a Compare April 24, 2023 18:45

tgeoghegan merged commit efbc6ee into main Apr 24, 2023

tgeoghegan deleted the timg/startup-check-schema-version branch April 24, 2023 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check DB schema version at startup #1272

Check DB schema version at startup #1272

tgeoghegan commented Apr 18, 2023

tgeoghegan commented Apr 18, 2023

branlwyd Apr 18, 2023

tgeoghegan Apr 19, 2023

branlwyd Apr 18, 2023

tgeoghegan Apr 19, 2023

divergentdave Apr 19, 2023 •

edited

Loading

tgeoghegan Apr 19, 2023 •

edited

Loading

branlwyd Apr 18, 2023

tgeoghegan Apr 19, 2023

branlwyd Apr 24, 2023

branlwyd commented Apr 18, 2023

Check DB schema version at startup #1272

Check DB schema version at startup #1272

Conversation

tgeoghegan commented Apr 18, 2023

tgeoghegan commented Apr 18, 2023

branlwyd Apr 18, 2023

Choose a reason for hiding this comment

tgeoghegan Apr 19, 2023

Choose a reason for hiding this comment

branlwyd Apr 18, 2023

Choose a reason for hiding this comment

tgeoghegan Apr 19, 2023

Choose a reason for hiding this comment

divergentdave Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

tgeoghegan Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

branlwyd Apr 18, 2023

Choose a reason for hiding this comment

tgeoghegan Apr 19, 2023

Choose a reason for hiding this comment

branlwyd Apr 24, 2023

Choose a reason for hiding this comment

branlwyd commented Apr 18, 2023

divergentdave Apr 19, 2023 •

edited

Loading

tgeoghegan Apr 19, 2023 •

edited

Loading