Skip to content

Commit 8e3d2e1

Browse files
smkleinsmklein
authored
[db-errors] Make conversion from TransactionError to PublicError explicit (#9505)
- Removes `impl From<TransactionError<PublicError>> for PublicError` - Makes the conversion explicit, with a caller-acknowledged function called `into_public_ignore_retries` - Updates call-sites. Generally, this was "just a rename", but `nexus/db-queries/src/db/datastore/deployment/external_networking.rs` had structural changes which now allow retry errors to be propagated up to the transaction retry wrapper.
1 parent f07d7b0 commit 8e3d2e1

File tree

14 files changed

+178
-116
lines changed

14 files changed

+178
-116
lines changed

‎nexus/db-errors/src/transaction_error.rs‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,24 @@ impl<T> TransactionError<T> {
6868
_ => NotRetryable(self),
6969
}
7070
}
71+
72+
/// Maps a `TransactionError<T>` to a `TransactionError<T2>`
73+
///
74+
/// Applyies an operation transforming `T` to `T2`.
75+
/// The API here resembles [Result::map_err] intentionally.
76+
pub fn map<T2, O>(self, op: O) -> TransactionError<T2>
77+
where
78+
O: FnOnce(T) -> T2,
79+
{
80+
match self {
81+
TransactionError::CustomError(err) => {
82+
TransactionError::CustomError(op(err))
83+
}
84+
TransactionError::Database(db_err) => {
85+
TransactionError::Database(db_err)
86+
}
87+
}
88+
}
7189
}
7290

7391
impl<T: std::fmt::Debug> TransactionError<T> {
@@ -100,9 +118,22 @@ impl From<PublicError> for TransactionError<PublicError> {
100118
}
101119
}
102120

103-
impl From<TransactionError<PublicError>> for PublicError {
104-
fn from(err: TransactionError<PublicError>) -> Self {
105-
match err {
121+
impl TransactionError<PublicError> {
122+
/// Converts a `TransactionError<PublicError>` into a `PublicError`.
123+
///
124+
/// This is basically an `impl From<Self> -> PublicError`, but with
125+
/// a different name to force the caller to label the contract
126+
/// happening here:
127+
///
128+
/// - When Diesel APIs are invoked within a transaction, they
129+
/// may return errors indicating "please retry the transaction".
130+
/// - If those are silently converted into the `PublicError` type, retry
131+
/// information is not propagated up to the transaction retry wrapper, which
132+
/// uses those errors to direct automated retry.
133+
///
134+
/// However, outside of transactions, this conversion is harmless.
135+
pub fn into_public_ignore_retries(self) -> PublicError {
136+
match self {
106137
TransactionError::CustomError(err) => err,
107138
TransactionError::Database(err) => {
108139
public_error_from_diesel(err, ErrorHandler::Server)

‎nexus/db-queries/src/db/datastore/deployment.rs‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ impl DataStore {
210210
})
211211
.await
212212
.map_err(|e| match err.take() {
213-
Some(txn_error) => txn_error.into(),
213+
Some(txn_error) => txn_error.into_public_ignore_retries(),
214214
None => public_error_from_diesel(e, ErrorHandler::Server),
215215
})?;
216216
Ok(r)
@@ -632,7 +632,7 @@ impl DataStore {
632632
})
633633
.await
634634
.map_err(|e| match err.take() {
635-
Some(err) => err.into(),
635+
Some(err) => err.into_public_ignore_retries(),
636636
None => public_error_from_diesel(e, ErrorHandler::Server),
637637
})?;
638638

@@ -1828,7 +1828,7 @@ impl DataStore {
18281828
})
18291829
.await
18301830
.map_err(|e| match err.take() {
1831-
Some(err) => err.into(),
1831+
Some(err) => err.into_public_ignore_retries(),
18321832
None => public_error_from_diesel(e, ErrorHandler::Server),
18331833
})?;
18341834

@@ -1962,7 +1962,7 @@ impl DataStore {
19621962
.map(|(_sled_id, zone)| zone),
19631963
)
19641964
.await
1965-
.map_err(|e| err.bail(e.into()))?;
1965+
.map_err(|e| err.bail(e))?;
19661966
self.ensure_zone_external_networking_allocated_on_connection(
19671967
&conn,
19681968
opctx,
@@ -1973,15 +1973,15 @@ impl DataStore {
19731973
.map(|(_sled_id, zone)| zone),
19741974
)
19751975
.await
1976-
.map_err(|e| err.bail(e.into()))?;
1976+
.map_err(|e| err.bail(e))?;
19771977

19781978
Ok(())
19791979
}
19801980
})
19811981
.await
19821982
.map_err(|e| {
19831983
if let Some(err) = err.take() {
1984-
err.into()
1984+
err.into_public_ignore_retries()
19851985
} else {
19861986
public_error_from_diesel(e, ErrorHandler::Server)
19871987
}
@@ -2132,7 +2132,9 @@ impl DataStore {
21322132
opctx.authorize(authz::Action::Read, &authz::BLUEPRINT_CONFIG).await?;
21332133

21342134
let conn = self.pool_connection_authorized(opctx).await?;
2135-
let target = Self::blueprint_current_target_only(&conn).await?;
2135+
let target = Self::blueprint_current_target_only(&conn)
2136+
.await
2137+
.map_err(|err| err.into_public_ignore_retries())?;
21362138

21372139
// The blueprint for the current target cannot be deleted while it is
21382140
// the current target, but it's possible someone else (a) made a new
@@ -2162,7 +2164,9 @@ impl DataStore {
21622164
) -> Result<BlueprintTarget, Error> {
21632165
opctx.authorize(authz::Action::Read, &authz::BLUEPRINT_CONFIG).await?;
21642166
let conn = self.pool_connection_authorized(opctx).await?;
2165-
Self::blueprint_current_target_only(&conn).await.map_err(|e| e.into())
2167+
Self::blueprint_current_target_only(&conn)
2168+
.await
2169+
.map_err(|e| e.into_public_ignore_retries())
21662170
}
21672171

21682172
// Helper to fetch the current blueprint target (without fetching the entire

‎nexus/db-queries/src/db/datastore/deployment/external_networking.rs‎

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use crate::db::DataStore;
1010
use crate::db::fixed_data::vpc_subnet::DNS_VPC_SUBNET;
1111
use crate::db::fixed_data::vpc_subnet::NEXUS_VPC_SUBNET;
1212
use crate::db::fixed_data::vpc_subnet::NTP_VPC_SUBNET;
13+
use nexus_db_errors::TransactionError;
1314
use nexus_db_lookup::DbConnection;
1415
use nexus_db_model::IncompleteNetworkInterface;
1516
use nexus_db_model::IpConfig;
@@ -89,7 +90,7 @@ impl DataStore {
8990
conn: &async_bb8_diesel::Connection<DbConnection>,
9091
opctx: &OpContext,
9192
zones_to_allocate: impl Iterator<Item = &BlueprintZoneConfig>,
92-
) -> Result<(), Error> {
93+
) -> Result<(), TransactionError<Error>> {
9394
// Looking up the service pool IDs requires an opctx; we'll do this at
9495
// most once inside the loop below, when we first encounter an address
9596
// of the same IP version.
@@ -150,7 +151,7 @@ impl DataStore {
150151
conn: &async_bb8_diesel::Connection<DbConnection>,
151152
log: &Logger,
152153
zones_to_deallocate: impl Iterator<Item = &BlueprintZoneConfig>,
153-
) -> Result<(), Error> {
154+
) -> Result<(), TransactionError<Error>> {
154155
for z in zones_to_deallocate {
155156
let Some((external_ip, nic)) = z.zone_type.external_networking()
156157
else {
@@ -185,7 +186,7 @@ impl DataStore {
185186
nic.id,
186187
)
187188
.await
188-
.map_err(|err| err.into_external())?;
189+
.map_err(|txn_err| txn_err.map(|err| err.into_external()))?;
189190
if deleted_nic {
190191
info!(log, "successfully deleted Omicron zone vNIC");
191192
} else {
@@ -204,7 +205,7 @@ impl DataStore {
204205
zone_id: OmicronZoneUuid,
205206
external_ip: OmicronZoneExternalIp,
206207
log: &Logger,
207-
) -> Result<bool, Error> {
208+
) -> Result<bool, TransactionError<Error>> {
208209
// localhost is used by many components in the test suite. We can't use
209210
// the normal path because normally a given external IP must only be
210211
// used once. Just treat localhost in the test suite as though it's
@@ -240,7 +241,8 @@ impl DataStore {
240241
return Err(Error::invalid_request(format!(
241242
"zone {zone_id} already has {} IPs allocated (expected 1)",
242243
allocated_ips.len()
243-
)));
244+
))
245+
.into());
244246
}
245247
};
246248

@@ -254,7 +256,8 @@ impl DataStore {
254256
return Err(Error::invalid_request(format!(
255257
"zone {zone_id} has invalid IP database record: {}",
256258
InlineErrorChain::new(&err)
257-
)));
259+
))
260+
.into());
258261
}
259262
};
260263

@@ -268,7 +271,8 @@ impl DataStore {
268271
);
269272
return Err(Error::invalid_request(format!(
270273
"zone {zone_id} has a different IP allocated ({existing_ip:?})",
271-
)));
274+
))
275+
.into());
272276
}
273277
}
274278

@@ -280,7 +284,7 @@ impl DataStore {
280284
zone_id: OmicronZoneUuid,
281285
nic: &NetworkInterface,
282286
log: &Logger,
283-
) -> Result<bool, Error> {
287+
) -> Result<bool, TransactionError<Error>> {
284288
// See the comment in is_external_ip_already_allocated().
285289
//
286290
// TODO-completeness: Ensure this works for dual-stack Omicron service
@@ -338,7 +342,8 @@ impl DataStore {
338342
return Err(Error::invalid_request(format!(
339343
"zone {zone_id} already has {} non-matching NIC(s) allocated",
340344
allocated_nics.len()
341-
)));
345+
))
346+
.into());
342347
}
343348

344349
info!(log, "NIC allocation required for zone");
@@ -354,7 +359,7 @@ impl DataStore {
354359
zone_id: OmicronZoneUuid,
355360
external_ip: OmicronZoneExternalIp,
356361
log: &Logger,
357-
) -> Result<(), Error> {
362+
) -> Result<(), TransactionError<Error>> {
358363
// Only attempt to allocate `external_ip` if it isn't already assigned
359364
// to this zone.
360365
//
@@ -395,20 +400,22 @@ impl DataStore {
395400
service_id: OmicronZoneUuid,
396401
nic: &NetworkInterface,
397402
log: &Logger,
398-
) -> Result<(), Error> {
403+
) -> Result<(), TransactionError<Error>> {
399404
// We don't pass `nic.kind` into the database below, but instead
400405
// explicitly call `service_create_network_interface`. Ensure this is
401406
// indeed a service NIC.
402407
match &nic.kind {
403408
NetworkInterfaceKind::Instance { .. } => {
404409
return Err(Error::invalid_request(
405410
"invalid NIC kind (expected service, got instance)",
406-
));
411+
)
412+
.into());
407413
}
408414
NetworkInterfaceKind::Probe { .. } => {
409415
return Err(Error::invalid_request(
410416
"invalid NIC kind (expected service, got probe)",
411-
));
417+
)
418+
.into());
412419
}
413420
NetworkInterfaceKind::Service { .. } => (),
414421
}
@@ -429,7 +436,8 @@ impl DataStore {
429436
return Err(Error::invalid_request(format!(
430437
"no VPC subnet available for {} zone",
431438
zone_kind.report_str()
432-
)));
439+
))
440+
.into());
433441
}
434442
};
435443

@@ -465,7 +473,7 @@ impl DataStore {
465473
let created_nic = self
466474
.create_network_interface_raw_conn(conn, nic_arg)
467475
.await
468-
.map_err(|err| err.into_external())?;
476+
.map_err(|txn_err| txn_err.map(|err| err.into_external()))?;
469477

470478
// We don't pass all the properties of `nic` into the create request
471479
// above. Double-check that the properties the DB assigned match
@@ -496,7 +504,8 @@ impl DataStore {
496504
"database cleanup required: unexpected NIC ({created_nic:?}) \
497505
allocated for {} {service_id}",
498506
zone_kind.report_str(),
499-
)));
507+
))
508+
.into());
500509
}
501510

502511
info!(log, "successfully allocated service vNIC");

‎nexus/db-queries/src/db/datastore/dns.rs‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,9 @@ impl DataStore {
7070
dns_group: DnsGroup,
7171
) -> ListResultVec<DnsZone> {
7272
let conn = self.pool_connection_authorized(opctx).await?;
73-
Ok(self
74-
.dns_zones_list_all_on_connection(opctx, &conn, dns_group)
75-
.await?)
73+
self.dns_zones_list_all_on_connection(opctx, &conn, dns_group)
74+
.await
75+
.map_err(|err| err.into_public_ignore_retries())
7676
}
7777

7878
/// Variant of [`Self::dns_zones_list_all`] which may be called from a
@@ -116,7 +116,8 @@ impl DataStore {
116116
&*self.pool_connection_authorized(opctx).await?,
117117
dns_group,
118118
)
119-
.await?;
119+
.await
120+
.map_err(|err| err.into_public_ignore_retries())?;
120121
Ok(version)
121122
}
122123

@@ -411,7 +412,7 @@ impl DataStore {
411412
})
412413
.await
413414
.map_err(|e| match err.take() {
414-
Some(err) => err.into(),
415+
Some(err) => err.into_public_ignore_retries(),
415416
None => public_error_from_diesel(e, ErrorHandler::Server),
416417
})
417418
}
@@ -1865,12 +1866,11 @@ mod test {
18651866
update.add_name(String::from("n2"), records1.clone()).unwrap();
18661867

18671868
let conn = datastore.pool_connection_for_tests().await.unwrap();
1868-
let error = Error::from(
1869-
datastore
1870-
.dns_update_incremental(&opctx, &conn, update)
1871-
.await
1872-
.unwrap_err(),
1873-
);
1869+
let error = datastore
1870+
.dns_update_incremental(&opctx, &conn, update)
1871+
.await
1872+
.unwrap_err()
1873+
.into_public_ignore_retries();
18741874
let msg = error.to_string();
18751875
assert!(msg.starts_with("Internal Error: "), "Message: {msg:}");
18761876
assert!(

‎nexus/db-queries/src/db/datastore/external_ip.rs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ impl DataStore {
178178
&self,
179179
conn: &async_bb8_diesel::Connection<DbConnection>,
180180
service_id: Uuid,
181-
) -> LookupResult<Vec<ExternalIp>> {
181+
) -> Result<Vec<ExternalIp>, TransactionError<Error>> {
182182
use nexus_db_schema::schema::external_ip::dsl;
183183
dsl::external_ip
184184
.filter(dsl::is_service.eq(true))
@@ -187,7 +187,7 @@ impl DataStore {
187187
.select(ExternalIp::as_select())
188188
.get_results_async(conn)
189189
.await
190-
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
190+
.map_err(|err| err.into())
191191
}
192192

193193
/// Allocates a floating IP address for instance usage.
@@ -233,7 +233,9 @@ impl DataStore {
233233
data: IncompleteExternalIp,
234234
) -> CreateResult<ExternalIp> {
235235
let conn = self.pool_connection_authorized(opctx).await?;
236-
let ip = Self::allocate_external_ip_on_connection(&conn, data).await?;
236+
let ip = Self::allocate_external_ip_on_connection(&conn, data)
237+
.await
238+
.map_err(|err| err.into_public_ignore_retries())?;
237239
Ok(ip)
238240
}
239241

@@ -675,7 +677,9 @@ impl DataStore {
675677
ip_id: Uuid,
676678
) -> Result<bool, Error> {
677679
let conn = self.pool_connection_authorized(opctx).await?;
678-
self.deallocate_external_ip_on_connection(&conn, ip_id).await
680+
self.deallocate_external_ip_on_connection(&conn, ip_id)
681+
.await
682+
.map_err(|err| err.into_public_ignore_retries())
679683
}
680684

681685
/// Variant of [Self::deallocate_external_ip] which may be called from a
@@ -684,7 +688,7 @@ impl DataStore {
684688
&self,
685689
conn: &async_bb8_diesel::Connection<DbConnection>,
686690
ip_id: Uuid,
687-
) -> Result<bool, Error> {
691+
) -> Result<bool, TransactionError<Error>> {
688692
use nexus_db_schema::schema::external_ip::dsl;
689693
let now = Utc::now();
690694
diesel::update(dsl::external_ip)
@@ -698,7 +702,7 @@ impl DataStore {
698702
UpdateStatus::Updated => true,
699703
UpdateStatus::NotUpdatedButExists => false,
700704
})
701-
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
705+
.map_err(|e| e.into())
702706
}
703707

704708
/// Moves an instance's ephemeral IP from 'Attached' to 'Detaching'.

0 commit comments

Comments
 (0)