refactor: add database crates #1407

ethowitz · 2022-09-30T19:21:36Z

Overview

This PR breaks up syncstorage's and tokenserver's database functionality into a number of new crates:

syncserver-db-common: Database-related code that is shared between syncstorage and tokenserver
- Does not depend on any other database crates
syncstorage-db-common: Database-related code that is shared between the MySQL and Spanner backends of syncstorage. Note that this crate is not used in any way by Tokenserver
- Depends on syncserver-db-common
syncstorage-db: Acts as an interface between the two database backends and the syncserver crate
- Depends on:
  - syncstorage-mysql
  - syncstorage-spanner
  - syncserver-db-common
  - syncstorage-db-common
syncstorage-mysql: The code that powers syncstorage's MySQL database backend
- Depends on:
  - syncstorage-db-common
  - syncserver-db-common
syncstoarge-spanner: The code the powers syncstorage's Spanner database backend
- Depends on:
  - syncstorage-db-common
  - syncserver-db-common
tokenserver-db: The code that powers tokenserver's MySQL database backend
- Depends on:
  - syncserver-db-common

I'd recommend generating the docs using cargo doc --no-deps --workspace to view the public interfaces for each crate in the workspace. I tried to keep the public interfaces for each crate as simple as possible.

Crate Relationships

It might be useful to visualize how the new crates relate to one another (I generated this using cargo depgraph):

The idea is that syncserver should not need to know any details about the underlying database backend. syncstorage-db acts as an interface to the database that reveals as little as possible about what's happening under the hood. syncstorage-db has two features (mysql and spanner) that enable the MySQL and Spanner backends, respectively.

`cargo` features

I added two cargo features to the top-level syncserver crate: mysql and spanner. These features represent the two syncstorage database backends. This was an important addition because the Spanner backend pulls in protobuf and grpcio, which both contribute very substantially to the time it takes to build syncstorage. Giving developers the ability to omit the code related to Spanner when developing locally substantially improves compile times. By my measurements, compiling with the spanner feature takes about 4.5 minutes, whereas compiling with the mysql feature takes only about 1.5 minutes.

Adding these cargo features also reduces the size of the compiled binary, since we are omitting substantial amounts of code we never use.

Error Handling

The toughest part about this work was figuring out how to handle errors. Originally, we had a single DbError which encapsulated knowledge of both the Spanner and MySQL backends. Decoupling the Spanner and MySQL backend meant that each needed to have its own error type that included only knowledge of its own backend. To resolve this, I defined a number of different error types in different places and nested them using enums.

syncserver-db-common:
- MysqlError: An error representing low-level, MySQL-specific errors (e.g. connection errors, migration errors, errors that arise during queries, etc.). Basically, this wraps a few diesel errors. This type is shared by the syncstorage MySQL backend and the Tokenserver MySQL backend
syncstorage-db-common:
- SyncstorageDbError: An error that represents syncstorage-specific errors that arise during database queries. It represents things like CollectionNotFound, BsoNotFound, quota errors, batch errors, etc. This type doesn't wrap any lower-level errors, and the errors are not specific to MySQL or Spanner.
syncstorage-mysql:
- DbError: This is a top-level database error for the MySQL backend. It encapsulates both MysqlError and SyncstorageDbError, representing any error that may occur when processing a MySQL query for syncstorage.
syncstorage-spanner:
- DbError: This is a top-level database error for the Spanner backend. It encapsulates SyncstorageDbError and lower-level Spanner errors (there was no need for a SpannerError type akin to the MysqlError type, since the Tokenserver backend doesn't use Spanner). It represents any error that may occur when processing a Spanner query for syncstorage.
tokenserver-mysql:
- DbError: This is a top-level database error for tokenserver. It encapsulates MysqlError and any other internal errors that may occur when processing a MySQL query for tokenserver.

I also made the *ErrorKind types private. This was a more opinionated decision on my part, but IMO, it simplifies the public APIs of the error types, making them easier to understand and use by clients. I include a number of helper methods on the base error types to allow clients to access information hidden in the *ErrorKind types.

Use of `debug_assertions`

Unfortunately, it's not possible to pass through items defined conditionally under the test feature to other crates (i.e. if something is defined with #[cfg(test)] in a crate, it won't be rendered visible to parent crates). There are numerous testing utilities that we define under the test feature, and I thought it would be wrong to include them in release builds. To remedy this, I define testing utilities with the debug_assertions feature instead, which only compiles the testing utilities in debug builds. This feels a little inelegant, but I thought it would be preferable to including testing utilities in release builds or defining our own new testing feature, which would further complicate compiling syncserver.

CI

This PR introduces new cargo features (one for the Spanner backend and one for the MySQL backend), which are mutually exclusive. This makes it impossible to build a single Docker image against which we can run the MySQL and Spanner e2e tests. Instead, we need to build two separate Docker images: one compiled with the MySQL database backend and one compiled with the Spanner database backend. Our Docker image builds are quite lengthy, so I introduced cargo chef to facilitate the caching of build dependencies between CI runs. This dramatically reduces build times, since only our own crates need to be recompiled with code changes -- the dependencies are redownloaded and recompiled only if our Cargo.tomls change.

Additionally, I configured the MySQL and Spanner images/e2e tests to run in parallel, further improving build times. We might want to consider using a larger CircleCI resource class to allow cargo to build more crates in parallel and to improve single-threaded CPU performance.

Testing

Given that no functionality should be changing, ensuring compilation and that the unit and integration tests pass should be sufficient. We should definitely run through the end-to-end QA tests on stage before we deploy this to production.

Issue(s)

Closes #1277

syncserver-db-common/src/lib.rs

syncserver/src/tokenserver/handlers.rs

syncserver/src/web/extractors.rs

syncstorage-db/src/lib.rs

ethowitz · 2022-09-30T20:12:38Z

syncstorage-db/src/lib.rs

+#[cfg(all(feature = "mysql", feature = "spanner"))]
+compile_error!("only one of the \"mysql\" and \"spanner\" features can be enabled at a time");
+
+#[cfg(not(any(feature = "mysql", feature = "spanner")))]
+compile_error!("exactly one of the \"mysql\" and \"spanner\" features must be enabled");


Some notes on mutually exclusive features: https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features

To me, this seemed like a good use case for them, given that we specifically want to omit dependencies we don't need when compiling with the MySQL backend. An alternative would be to have one database backend be the "default" and then have a single feature to override that default to use the other backend. I opted not to go with that route because I don't think one database backend is obviously the "default", since we use Spanner in prod but MySQL locally (for the most part)

ethowitz · 2022-09-30T20:14:32Z

syncstorage-db-common/src/lib.rs

+
+#[async_trait]
+pub trait DbPoolTrait: Sync + Send + Debug + GetPoolState {
+    type Error;


We need to introduce an associated type here because implementers of this trait have their own DbError types

ethowitz · 2022-09-30T20:16:25Z

syncstorage-mysql/src/error.rs

+#[derive(Debug, Error)]
+enum DbErrorKind {
+    #[error("{}", _0)]
+    Common(SyncstorageDbError),


I struggled a bit with the naming here. Common is supposed to be a Syncstorage database error that is not specific to the database backend

ethowitz · 2022-09-30T20:18:14Z

syncstorage-spanner/src/error.rs

+    #[error("Connection expired")]
+    Expired,
+
+    #[error("A database error occurred: {}", _0)]
+    Grpc(#[from] grpcio::Error),
+
+    #[error("Database integrity error: {}", _0)]
+    Integrity(String),
+
+    #[error("Spanner data load too large: {}", _0)]
+    TooLarge(String),


These could have been pulled into a SpannerDbError type akin to the MysqlDbError type, but the reason I made the MysqlDbError type was because it's also used by Tokenserver

ethowitz · 2022-09-30T20:29:39Z

This can't be merged until #1276 is merged

jrconlin

Looks good, aside from the Makefile pointing to your directory.

Dockerfile

Makefile

jrconlin · 2022-12-08T23:25:36Z

syncserver/Cargo.toml

 actix-cors = "0.5"
-actix-service = "1.0.6"
 async-trait = "0.1.40"


Similar to the package stuff, we could also boost a bunch of the common dependency crates to up to the parent crates.
https://doc.rust-lang.org/cargo/reference/workspaces.html#the-workspacedependencies-table

Good call! I created a separate ticket to do this: https://mozilla-hub.atlassian.net/browse/SYNC-3549

jrconlin · 2022-12-08T23:26:54Z

syncserver-common/Cargo.toml

@@ -4,5 +4,16 @@ version = "0.13.0"
 edition = "2021"


I wonder if it makes sense to use some of the more modern crate management stuff here:
https://doc.rust-lang.org/cargo/reference/workspaces.html#the-workspacepackage-table

That way we don't have to muck with updating a dozen versions whenever something changes.

Similar to above, I created a ticket to track this: https://mozilla-hub.atlassian.net/browse/SYNC-3550

jrconlin · 2022-12-09T00:08:46Z

syncstorage-spanner/src/batch.rs

@@ -98,7 +98,7 @@ pub async fn append_async(db: &SpannerDb, params: params::AppendToBatch) -> Resu
    if !exists {
        // NOTE: db tests expects this but it doesn't seem necessary w/ the
        // handler validating the batch before appends
-        Err(DbErrorKind::BatchNotFound)?
+        return Err(DbError::batch_not_found());


I know that adding the ? does the same thing, but I agree that this is more readable, particularly when fast reading the code.

pjenvey

Looks good. Wow on the cargo chef changes, thanks for adding those!

Of course on a huge PR like this I really only have one thing to bikeshed, and it's mostly just food for thought: the Trait suffix for Db/PoolTrait feels odd -- I don't think I've ever seen it used like that in Rust -- and a bit superfluous when there's a dyn beside it (which is most of the time when referencing traits). One alternative could be to keep the trait names as is and rename syncstorage_db::Db and DbPool to DbImpl/DbPoolImpl (I saw a complex trait setup in servo that does this) -- and maybe that Impl suffix kinda helps mark that the type's swapped via the feature flag and backed by a trait?

pjenvey · 2023-01-06T19:45:26Z

syncstorage-mysql/src/error.rs

+        match &kind {
+            DbErrorKind::Common(dbe) => Self {
+                status: dbe.status,
+                backtrace: Box::new(dbe.backtrace.clone()),


I'm a little worried about the amount of cloning we're doing of backtraces. At least in mysql, we'd clone it from the Kind and then ApiError eventually clones it again, all for propagating one error.

However the current 0.13.1 on prod isn't far from this, I think it's generating 2 different stack traces for a DbError (in DbError and then another one in ApiError) so maybe this does not matter much.

The other problem with it is the new std::backtrace::Backtrace doesn't implement Clone (which will hopefully replace all uses of the backtrace::Backtrace at some point in the future).

Maybe we can just log an issue to look at having backtrace methods that would return them from the inner types to avoid the cloning?

I think the backtrace method approach sounds good 👍🏻 I created a ticket here

ethowitz · 2023-01-09T15:37:35Z

@pjenvey I like your idea about removing the Trait suffix and adding an Impl suffix to the types that implement the traits. I'll make the change!

jrconlin · 2023-01-09T21:18:21Z

syncstorage-db/src/tests/db.rs

@@ -16,9 +19,9 @@ lazy_static! {
 }

 #[tokio::test]
-async fn bso_successfully_updates_single_values() -> Result<()> {
+async fn bso_successfully_updates_single_values() -> Result<(), DbError> {


Bit surprised you didn't create a type for this, just to save keystrokes, but it's fine.

There were a few different Result/DbResult/DbFuture aliases, and I thought it would make things easier to follow if we were more explicit with our return types but 🤷🏻 perhaps that's just me

ethowitz · 2023-01-10T15:25:38Z

CI was stuck so I did a git commit --amend --no-edit and force pushed, though I still see that GitHub appears to be waiting for the status of the e2e-tests job, which no longer exists in the CircleCI file...

jrconlin · 2023-01-10T16:14:25Z

I removed the requirement for the missing e2e-test from the master branch protection settings.

I am curious why they're missing.

ethowitz · 2023-01-10T17:01:56Z

@jrconlin The original e2e-tests job was removed and replaced with mysql-e2e-tests and spanner-e2e-tests -- should I update the branch protection to reflect that?

jrconlin · 2023-01-10T17:43:14Z

Ah, yeah. Sorry, forgot about that.

ethowitz · 2023-01-10T18:29:53Z

@jrconlin Ok I updated the branch protection settings, and that seems to have fixed the issue. I think this needs another approval before I can merge