Skip to content

Commit

Permalink
review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@smklein
smklein committed Oct 17, 2024
1 parent 144d5ea commit 95c6825
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 45 deletions.
5 changes: 2 additions & 3 deletions live-tests/tests/common/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,8 @@ impl LiveTestContext {

/// Clean up this `LiveTestContext`
///
/// This mainly removes log files created by the test. We do this in this
/// explicit cleanup function rather than on `Drop` because we want the log
/// files preserved on test failure.
/// This removes log files and cleans up the [`DataStore`], which
/// but be terminated asynchronously.
pub async fn cleanup_successful(self) {
self.datastore.terminate().await;
self.logctx.cleanup_successful();
Expand Down
28 changes: 14 additions & 14 deletions nexus/db-queries/src/db/datastore/external_ip.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl DataStore {
probe_id: Uuid,
pool: Option<authz::IpPool>,
) -> CreateResult<ExternalIp> {
let authz_pool = self.resolve_pool_for_allocation(&opctx, pool).await?;
let authz_pool = self.resolve_pool_for_allocation(opctx, pool).await?;
let data = IncompleteExternalIp::for_ephemeral_probe(
ip_id,
probe_id,
Expand Down Expand Up @@ -123,7 +123,7 @@ impl DataStore {
// Naturally, we now *need* to destroy the ephemeral IP if the newly alloc'd
// IP was not attached, including on idempotent success.

let authz_pool = self.resolve_pool_for_allocation(&opctx, pool).await?;
let authz_pool = self.resolve_pool_for_allocation(opctx, pool).await?;
let data = IncompleteExternalIp::for_ephemeral(ip_id, authz_pool.id());

// We might not be able to acquire a new IP, but in the event of an
Expand Down Expand Up @@ -205,7 +205,7 @@ impl DataStore {
// If no pool specified, use the default logic
None => {
let (authz_pool, ..) =
self.ip_pools_fetch_default(&opctx).await?;
self.ip_pools_fetch_default(opctx).await?;
authz_pool
}
};
Expand All @@ -224,7 +224,7 @@ impl DataStore {
) -> CreateResult<ExternalIp> {
let ip_id = Uuid::new_v4();

let authz_pool = self.resolve_pool_for_allocation(&opctx, pool).await?;
let authz_pool = self.resolve_pool_for_allocation(opctx, pool).await?;

let data = if let Some(ip) = ip {
IncompleteExternalIp::for_floating_explicit(
Expand Down Expand Up @@ -695,7 +695,7 @@ impl DataStore {
ip_id: Uuid,
instance_id: InstanceUuid,
) -> Result<Option<ExternalIp>, Error> {
let _ = LookupPath::new(&opctx, self)
let _ = LookupPath::new(opctx, self)
.instance_id(instance_id.into_untyped_uuid())
.lookup_for(authz::Action::Modify)
.await?;
Expand Down Expand Up @@ -951,7 +951,7 @@ impl DataStore {
instance_id: InstanceUuid,
creating_instance: bool,
) -> UpdateResult<(ExternalIp, bool)> {
let (.., authz_instance) = LookupPath::new(&opctx, self)
let (.., authz_instance) = LookupPath::new(opctx, self)
.instance_id(instance_id.into_untyped_uuid())
.lookup_for(authz::Action::Modify)
.await?;
Expand Down Expand Up @@ -993,7 +993,7 @@ impl DataStore {
instance_id: InstanceUuid,
creating_instance: bool,
) -> UpdateResult<(ExternalIp, bool)> {
let (.., authz_instance) = LookupPath::new(&opctx, self)
let (.., authz_instance) = LookupPath::new(opctx, self)
.instance_id(instance_id.into_untyped_uuid())
.lookup_for(authz::Action::Modify)
.await?;
Expand Down Expand Up @@ -1167,7 +1167,7 @@ mod tests {
let (opctx, datastore) = (db.opctx(), db.datastore());

// No IPs, to start
let ips = read_all_service_ips(&datastore, &opctx).await;
let ips = read_all_service_ips(&datastore, opctx).await;
assert_eq!(ips, vec![]);

// Set up service IP pool range
Expand All @@ -1177,11 +1177,11 @@ mod tests {
))
.unwrap();
let (service_ip_pool, _) = datastore
.ip_pools_service_lookup(&opctx)
.ip_pools_service_lookup(opctx)
.await
.expect("lookup service ip pool");
datastore
.ip_pool_add_range(&opctx, &service_ip_pool, &ip_range)
.ip_pool_add_range(opctx, &service_ip_pool, &ip_range)
.await
.expect("add range to service ip pool");

Expand All @@ -1207,7 +1207,7 @@ mod tests {
};
let external_ip = datastore
.external_ip_allocate_omicron_zone(
&opctx,
opctx,
OmicronZoneUuid::new_v4(),
ZoneKind::Nexus,
external_ip,
Expand All @@ -1220,7 +1220,7 @@ mod tests {
external_ips.sort_by_key(|ip| ip.id);

// Ensure we see them all.
let ips = read_all_service_ips(&datastore, &opctx).await;
let ips = read_all_service_ips(&datastore, opctx).await;
assert_eq!(ips, external_ips);

// Deallocate a few, and ensure we don't see them anymore.
Expand All @@ -1229,7 +1229,7 @@ mod tests {
if i % 3 == 0 {
let id = external_ip.id;
datastore
.deallocate_external_ip(&opctx, id)
.deallocate_external_ip(opctx, id)
.await
.expect("failed to deallocate IP");
removed_ip_ids.insert(id);
Expand All @@ -1242,7 +1242,7 @@ mod tests {
external_ips.retain(|ip| !removed_ip_ids.contains(&ip.id));

// Ensure we see them all remaining IPs.
let ips = read_all_service_ips(&datastore, &opctx).await;
let ips = read_all_service_ips(&datastore, opctx).await;
assert_eq!(ips, external_ips);

db.terminate().await;
Expand Down
9 changes: 6 additions & 3 deletions nexus/db-queries/src/db/datastore/pub_test_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ mod test {
impl TestDatabase {
/// Creates a new database for test usage, with a pool.
///
/// [Self::terminate] should be called before the test finishes.
/// [`Self::terminate`] should be called before the test finishes,
/// or dropping the [`TestDatabase`] will panic.
pub async fn new_with_pool(log: &Logger) -> Self {
let db = test_setup_database(log).await;
let cfg = db::Config { url: db.pg_config().clone() };
Expand All @@ -48,7 +49,8 @@ mod test {

/// Creates a new database for test usage, with a pre-loaded datastore.
///
/// [Self::terminate] should be called before the test finishes.
/// [`Self::terminate`] should be called before the test finishes,
/// or dropping the [`TestDatabase`] will panic.
pub async fn new_with_datastore(log: &Logger) -> Self {
let db = test_setup_database(log).await;
let (opctx, datastore) =
Expand All @@ -60,7 +62,8 @@ mod test {

/// Creates a new database for test usage, with a raw datastore.
///
/// [Self::terminate] should be called before the test finishes.
/// [`Self::terminate`] should be called before the test finishes,
/// or dropping the [`TestDatabase`] will panic.
pub async fn new_with_raw_datastore(log: &Logger) -> Self {
let db = test_setup_database(log).await;
let cfg = db::Config { url: db.pg_config().clone() };
Expand Down
48 changes: 24 additions & 24 deletions nexus/db-queries/src/db/queries/external_ip.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1059,7 +1059,7 @@ mod tests {
.db
.datastore()
.allocate_instance_snat_ip(
&context.db.opctx(),
context.db.opctx(),
id,
instance_id,
context.default_pool_id().await,
Expand All @@ -1077,7 +1077,7 @@ mod tests {
.db
.datastore()
.allocate_instance_snat_ip(
&context.db.opctx(),
context.db.opctx(),
Uuid::new_v4(),
instance_id,
context.default_pool_id().await,
Expand Down Expand Up @@ -1115,7 +1115,7 @@ mod tests {
.db
.datastore()
.allocate_instance_ephemeral_ip(
&context.db.opctx(),
context.db.opctx(),
Uuid::new_v4(),
instance_id,
/* pool_name = */ None,
Expand All @@ -1135,7 +1135,7 @@ mod tests {
.db
.datastore()
.allocate_instance_snat_ip(
&context.db.opctx(),
context.db.opctx(),
Uuid::new_v4(),
instance_id,
context.default_pool_id().await,
Expand All @@ -1159,7 +1159,7 @@ mod tests {
.db
.datastore()
.allocate_instance_ephemeral_ip(
&context.db.opctx(),
context.db.opctx(),
Uuid::new_v4(),
instance_id,
/* pool_name = */ None,
Expand Down Expand Up @@ -1214,7 +1214,7 @@ mod tests {
.db
.datastore()
.allocate_instance_snat_ip(
&context.db.opctx(),
context.db.opctx(),
Uuid::new_v4(),
instance_id,
context.default_pool_id().await,
Expand All @@ -1233,7 +1233,7 @@ mod tests {
context
.db
.datastore()
.deallocate_external_ip(&context.db.opctx(), ips[0].id)
.deallocate_external_ip(context.db.opctx(), ips[0].id)
.await
.expect("Failed to release the first external IP address");

Expand All @@ -1244,7 +1244,7 @@ mod tests {
.db
.datastore()
.allocate_instance_snat_ip(
&context.db.opctx(),
context.db.opctx(),
Uuid::new_v4(),
instance_id,
context.default_pool_id().await,
Expand Down Expand Up @@ -1272,7 +1272,7 @@ mod tests {
.db
.datastore()
.allocate_instance_snat_ip(
&context.db.opctx(),
context.db.opctx(),
Uuid::new_v4(),
instance_id,
context.default_pool_id().await,
Expand Down Expand Up @@ -1310,7 +1310,7 @@ mod tests {
.db
.datastore()
.allocate_instance_ephemeral_ip(
&context.db.opctx(),
context.db.opctx(),
id,
instance_id,
pool_name,
Expand Down Expand Up @@ -1359,7 +1359,7 @@ mod tests {
.db
.datastore()
.external_ip_allocate_omicron_zone(
&context.db.opctx(),
context.db.opctx(),
service_id,
ZoneKind::Nexus,
ip_10_0_0_3,
Expand All @@ -1377,7 +1377,7 @@ mod tests {
.db
.datastore()
.external_ip_allocate_omicron_zone(
&context.db.opctx(),
context.db.opctx(),
service_id,
ZoneKind::Nexus,
ip_10_0_0_3,
Expand All @@ -1393,7 +1393,7 @@ mod tests {
let err = context
.db.datastore()
.external_ip_allocate_omicron_zone(
&context.db.opctx(),
context.db.opctx(),
service_id,
ZoneKind::Nexus,
OmicronZoneExternalIp::Floating(OmicronZoneExternalFloatingIp {
Expand All @@ -1413,7 +1413,7 @@ mod tests {
let err = context
.db.datastore()
.external_ip_allocate_omicron_zone(
&context.db.opctx(),
context.db.opctx(),
service_id,
ZoneKind::Nexus,
OmicronZoneExternalIp::Floating(OmicronZoneExternalFloatingIp {
Expand All @@ -1439,7 +1439,7 @@ mod tests {
let err = context
.db.datastore()
.external_ip_allocate_omicron_zone(
&context.db.opctx(),
context.db.opctx(),
service_id,
ZoneKind::BoundaryNtp,
ip_10_0_0_3_snat_0,
Expand Down Expand Up @@ -1467,7 +1467,7 @@ mod tests {
.db
.datastore()
.external_ip_allocate_omicron_zone(
&context.db.opctx(),
context.db.opctx(),
snat_service_id,
ZoneKind::BoundaryNtp,
ip_10_0_0_1_snat_32768,
Expand All @@ -1489,7 +1489,7 @@ mod tests {
.db
.datastore()
.external_ip_allocate_omicron_zone(
&context.db.opctx(),
context.db.opctx(),
snat_service_id,
ZoneKind::BoundaryNtp,
ip_10_0_0_1_snat_32768,
Expand Down Expand Up @@ -1517,7 +1517,7 @@ mod tests {
let err = context
.db.datastore()
.external_ip_allocate_omicron_zone(
&context.db.opctx(),
context.db.opctx(),
snat_service_id,
ZoneKind::BoundaryNtp,
ip_10_0_0_1_snat_49152,
Expand Down Expand Up @@ -1557,7 +1557,7 @@ mod tests {
.db
.datastore()
.external_ip_allocate_omicron_zone(
&context.db.opctx(),
context.db.opctx(),
service_id,
ZoneKind::Nexus,
ip_10_0_0_5,
Expand Down Expand Up @@ -1592,7 +1592,7 @@ mod tests {
.db
.datastore()
.allocate_instance_snat_ip(
&context.db.opctx(),
context.db.opctx(),
id,
instance_id,
context.default_pool_id().await,
Expand All @@ -1610,7 +1610,7 @@ mod tests {
.db
.datastore()
.allocate_instance_snat_ip(
&context.db.opctx(),
context.db.opctx(),
id,
instance_id,
context.default_pool_id().await,
Expand Down Expand Up @@ -1664,7 +1664,7 @@ mod tests {
.db
.datastore()
.allocate_instance_ephemeral_ip(
&context.db.opctx(),
context.db.opctx(),
id,
instance_id,
Some(p1),
Expand Down Expand Up @@ -1710,7 +1710,7 @@ mod tests {
.db
.datastore()
.allocate_instance_ephemeral_ip(
&context.db.opctx(),
context.db.opctx(),
Uuid::new_v4(),
instance_id,
Some(p1.clone()),
Expand All @@ -1733,7 +1733,7 @@ mod tests {
.db
.datastore()
.allocate_instance_ephemeral_ip(
&context.db.opctx(),
context.db.opctx(),
Uuid::new_v4(),
instance_id,
Some(p1),
Expand Down
2 changes: 1 addition & 1 deletion nexus/src/app/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,7 @@ impl Nexus {
/// Awaits termination without triggering it.
///
/// To trigger termination, see:
/// - [Self::close_servers] or [Self::terminate]
/// - [`Self::close_servers`] or [`Self::terminate`]
pub(crate) async fn wait_for_shutdown(&self) -> Result<(), String> {
// The internal server is the last server to be closed.
//
Expand Down

0 comments on commit 95c6825

Please sign in to comment.