Skip to content

Commit 5a75bc1

Browse files
authored
[1/n] [nexus-db-queries] move LookupPath to another crate (#7985)
Introduce `nexus-db-lookup`, a new crate that contains `LookupPath` and all of the corresponding resources. This change has two advantages: 1. While attempting to split some of omicron-nexus up into multiple crates that could potentially compile in parallel with nexus-db-queries, I ran into the issue that `LookupPath`, which is a fairly fundamental type, lived inside nexus-db-queries. 2. This is an okay build speed improvement in its own right. With this change, `touch nexus-db-queries/src/lib.rs && cargo check -p nexus-db-queries` goes from around 2.45 seconds to 2.15 seconds (around 1.15x faster). That's because the `lookup_resource` macros in the new nexus-db-lookup crate [expand](https://crates.io/crates/cargo-expand/) to quite a lot of code: around 15k lines or so compared to the rest of nexus-db-queries' 75k lines when expanded. In order to achieve this goal: * This PR provides a trait as an interface that `DataStore` implements. That isn't too bad, though. I'm not totally sure this will end up being the final shape of such an interface, but it works for now and can be changed later with minimal disruption. * This PR splits some error handling code in nexus-db-queries into its own crate as well. Relatively small but otherwise quite nice chunk of code to move out. This PR aims to minimize ugliness at callsites (i.e. `nexus.datastore()` rather than `&**nexus.datastore()`). However I did let some churn through, specifically places where we passed in immediately-dereferenced types like `&&DataStore`.
1 parent 267d3d8 commit 5a75bc1

File tree

185 files changed

+1156
-916
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

185 files changed

+1156
-916
lines changed

Cargo.lock

Lines changed: 46 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,9 @@ members = [
7777
"nexus-sled-agent-shared",
7878
"nexus/authz-macros",
7979
"nexus/auth",
80+
"nexus/db-errors",
8081
"nexus/db-fixed-data",
82+
"nexus/db-lookup",
8183
"nexus/db-macros",
8284
"nexus/db-model",
8385
"nexus/db-queries",
@@ -223,7 +225,9 @@ default-members = [
223225
"nexus-sled-agent-shared",
224226
"nexus/authz-macros",
225227
"nexus/auth",
228+
"nexus/db-errors",
226229
"nexus/db-fixed-data",
230+
"nexus/db-lookup",
227231
"nexus/db-macros",
228232
"nexus/db-model",
229233
"nexus/db-queries",
@@ -492,7 +496,9 @@ multimap = "0.10.0"
492496
nexus-auth = { path = "nexus/auth" }
493497
nexus-client = { path = "clients/nexus-client" }
494498
nexus-config = { path = "nexus-config" }
499+
nexus-db-errors = { path = "nexus/db-errors" }
495500
nexus-db-fixed-data = { path = "nexus/db-fixed-data" }
501+
nexus-db-lookup = { path = "nexus/db-lookup" }
496502
nexus-db-model = { path = "nexus/db-model" }
497503
nexus-db-queries = { path = "nexus/db-queries" }
498504
nexus-db-schema = { path = "nexus/db-schema" }

dev-tools/omdb/Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ internal-dns-types.workspace = true
3333
itertools.workspace = true
3434
nexus-client.workspace = true
3535
nexus-config.workspace = true
36+
nexus-db-errors.workspace = true
37+
nexus-db-lookup.workspace = true
3638
nexus-db-model.workspace = true
3739
nexus-db-queries.workspace = true
3840
nexus-db-schema.workspace = true

dev-tools/omdb/src/bin/omdb/db.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@ use ipnetwork::IpNetwork;
5656
use itertools::Itertools;
5757
use nexus_config::PostgresConfigWithUrl;
5858
use nexus_config::RegionAllocationStrategy;
59+
use nexus_db_errors::OptionalError;
60+
use nexus_db_lookup::DataStoreConnection;
61+
use nexus_db_lookup::LookupPath;
5962
use nexus_db_model::CrucibleDataset;
6063
use nexus_db_model::Disk;
6164
use nexus_db_model::DnsGroup;
@@ -106,20 +109,17 @@ use nexus_db_queries::context::OpContext;
106109
use nexus_db_queries::db;
107110
use nexus_db_queries::db::DataStore;
108111
use nexus_db_queries::db::datastore::CrucibleTargets;
109-
use nexus_db_queries::db::datastore::DataStoreConnection;
110112
use nexus_db_queries::db::datastore::InstanceAndActiveVmm;
111113
use nexus_db_queries::db::datastore::InstanceStateComputer;
112114
use nexus_db_queries::db::datastore::SQL_BATCH_SIZE;
113115
use nexus_db_queries::db::datastore::VolumeCookedResult;
114116
use nexus_db_queries::db::datastore::read_only_resources_associated_with_volume;
115117
use nexus_db_queries::db::identity::Asset;
116-
use nexus_db_queries::db::lookup::LookupPath;
117118
use nexus_db_queries::db::model::ServiceKind;
118119
use nexus_db_queries::db::pagination::Paginator;
119120
use nexus_db_queries::db::pagination::paginated;
120121
use nexus_db_queries::db::queries::ALLOW_FULL_TABLE_SCAN_SQL;
121122
use nexus_db_queries::db::queries::region_allocation;
122-
use nexus_db_queries::transaction_retry::OptionalError;
123123
use nexus_types::deployment::Blueprint;
124124
use nexus_types::deployment::BlueprintZoneDisposition;
125125
use nexus_types::deployment::BlueprintZoneType;

dev-tools/omdb/src/bin/omdb/db/saga.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,13 @@ use clap::Subcommand;
1818
use diesel::prelude::*;
1919
use internal_dns_resolver::ResolveError;
2020
use internal_dns_types::names::ServiceName;
21+
use nexus_db_lookup::DataStoreConnection;
2122
use nexus_db_model::Saga;
2223
use nexus_db_model::SagaNodeEvent;
2324
use nexus_db_model::SagaState;
2425
use nexus_db_model::SecId;
2526
use nexus_db_queries::context::OpContext;
2627
use nexus_db_queries::db::DataStore;
27-
use nexus_db_queries::db::datastore::DataStoreConnection;
2828
use nexus_db_queries::db::datastore::SQL_BATCH_SIZE;
2929
use nexus_db_queries::db::pagination::Paginator;
3030
use nexus_db_queries::db::pagination::paginated;

dev-tools/omdb/src/bin/omdb/nexus.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ use nexus_client::types::PhysicalDiskPath;
3535
use nexus_client::types::SagaState;
3636
use nexus_client::types::SledSelector;
3737
use nexus_client::types::UninitializedSledId;
38+
use nexus_db_lookup::LookupPath;
3839
use nexus_db_queries::db::DataStore;
39-
use nexus_db_queries::db::lookup::LookupPath;
4040
use nexus_inventory::now_db_precision;
4141
use nexus_saga_recovery::LastPass;
4242
use nexus_types::deployment::Blueprint;
@@ -3167,7 +3167,7 @@ async fn cmd_nexus_sled_expunge_with_datastore(
31673167
let opctx = &opctx;
31683168

31693169
// First, we need to look up the sled so we know its serial number.
3170-
let (_authz_sled, sled) = LookupPath::new(opctx, &datastore)
3170+
let (_authz_sled, sled) = LookupPath::new(opctx, datastore)
31713171
.sled_id(args.sled_id.into_untyped_uuid())
31723172
.fetch()
31733173
.await
@@ -3271,7 +3271,7 @@ async fn cmd_nexus_sled_expunge_disk_with_datastore(
32713271

32723272
// First, we need to look up the disk so we can lookup identity information.
32733273
let (_authz_physical_disk, physical_disk) =
3274-
LookupPath::new(opctx, &datastore)
3274+
LookupPath::new(opctx, datastore)
32753275
.physical_disk(args.physical_disk_id)
32763276
.fetch()
32773277
.await

nexus/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ uuid.workspace = true
106106

107107
nexus-auth.workspace = true
108108
nexus-defaults.workspace = true
109+
nexus-db-lookup.workspace = true
109110
nexus-db-model.workspace = true
110111
nexus-db-queries.workspace = true
111112
nexus-db-schema.workspace = true

nexus/auth/src/authz/api_resources.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
//! Authz types for resources in the API hierarchy
66
//!
77
//! The general pattern in Nexus for working with an object is to look it up
8-
//! (see `nexus_db_queries::db::lookup::LookupPath`) and get back a so-called
8+
//! (see `nexus_db_lookup::LookupPath`) and get back a so-called
99
//! `authz` type. This type uniquely identifies the resource regardless of
1010
//! any other changes (e.g., name change or moving it to a different parent
1111
//! collection). The various datastore functions that modify API resources

nexus/db-errors/Cargo.toml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
[package]
2+
name = "nexus-db-errors"
3+
version = "0.1.0"
4+
edition = "2021"
5+
license = "MPL-2.0"
6+
7+
[lints]
8+
workspace = true
9+
10+
[dependencies]
11+
diesel.workspace = true
12+
nexus-auth.workspace = true
13+
omicron-common.workspace = true
14+
omicron-workspace-hack.workspace = true
15+
thiserror.workspace = true

nexus/db-errors/src/lib.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// This Source Code Form is subject to the terms of the Mozilla Public
2+
// License, v. 2.0. If a copy of the MPL was not distributed with this
3+
// file, You can obtain one at https://mozilla.org/MPL/2.0/.
4+
5+
mod optional_error;
6+
mod transaction_error;
7+
8+
pub use optional_error::*;
9+
pub use transaction_error::*;

nexus/db-errors/src/optional_error.rs

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
// This Source Code Form is subject to the terms of the Mozilla Public
2+
// License, v. 2.0. If a copy of the MPL was not distributed with this
3+
// file, You can obtain one at https://mozilla.org/MPL/2.0/.
4+
5+
use diesel::result::Error as DieselError;
6+
use std::sync::{Arc, Mutex};
7+
8+
/// Helper utility for passing non-retryable errors out-of-band from
9+
/// transactions.
10+
///
11+
/// Transactions prefer to act on the `diesel::result::Error` type,
12+
/// but transaction users may want more meaningful error types.
13+
/// This utility helps callers safely propagate back Diesel errors while
14+
/// retaining auxiliary error info.
15+
pub struct OptionalError<E>(Arc<Mutex<Option<E>>>);
16+
17+
impl<E> Clone for OptionalError<E> {
18+
fn clone(&self) -> Self {
19+
Self(self.0.clone())
20+
}
21+
}
22+
23+
impl<E: std::fmt::Debug> OptionalError<E> {
24+
#[expect(clippy::new_without_default)] // ehh Default isn't really logical here?
25+
pub fn new() -> Self {
26+
Self(Arc::new(Mutex::new(None)))
27+
}
28+
29+
/// Sets "Self" to the value of `error` and returns `DieselError::RollbackTransaction`.
30+
pub fn bail(&self, err: E) -> DieselError {
31+
(*self.0.lock().unwrap()).replace(err);
32+
DieselError::RollbackTransaction
33+
}
34+
35+
/// If `diesel_error` is retryable, returns it without setting Self.
36+
///
37+
/// Otherwise, sets "Self" to the value of `err`, and returns
38+
/// `DieselError::RollbackTransaction`.
39+
pub fn bail_retryable_or(
40+
&self,
41+
diesel_error: DieselError,
42+
err: E,
43+
) -> DieselError {
44+
self.bail_retryable_or_else(diesel_error, |_diesel_error| err)
45+
}
46+
47+
/// If `diesel_error` is retryable, returns it without setting Self.
48+
///
49+
/// Otherwise, sets "Self" to the value of `f` applied to `diesel_err`, and
50+
/// returns `DieselError::RollbackTransaction`.
51+
pub fn bail_retryable_or_else<F>(
52+
&self,
53+
diesel_error: DieselError,
54+
f: F,
55+
) -> DieselError
56+
where
57+
F: FnOnce(DieselError) -> E,
58+
{
59+
if crate::retryable(&diesel_error) {
60+
diesel_error
61+
} else {
62+
self.bail(f(diesel_error))
63+
}
64+
}
65+
66+
/// If "Self" was previously set to a non-retryable error, return it.
67+
pub fn take(self) -> Option<E> {
68+
(*self.0.lock().unwrap()).take()
69+
}
70+
}

0 commit comments

Comments
 (0)