Skip to content

Commit

Permalink
use some newtypes, validate some issues
Browse files Browse the repository at this point in the history
  • Loading branch information
@davepacheco
davepacheco committed Oct 16, 2023
1 parent 0615a1b commit c06c1be
Show file tree
Hide file tree
Showing 4 changed files with 154 additions and 33 deletions.
84 changes: 75 additions & 9 deletions nexus/db-model/src/inventory.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,19 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use crate::impl_enum_type;
use crate::schema::{
hw_baseboard_id, inv_caboose, inv_collection, inv_collection_error,
inv_root_of_trust, inv_service_processor, sw_caboose,
};
use crate::{impl_enum_type, SqlU16, SqlU32};
use chrono::DateTime;
use chrono::Utc;
use diesel::backend::Backend;
use diesel::deserialize::{self, FromSql};
use diesel::expression::AsExpression;
use diesel::pg::Pg;
use diesel::serialize::ToSql;
use diesel::{serialize, sql_types};
use nexus_types::inventory::{
BaseboardId, Caboose, Collection, PowerState, RotSlot,
};
Expand Down Expand Up @@ -209,13 +214,17 @@ impl<'a> From<&'a Caboose> for SwCaboose {
#[diesel(table_name = inv_collection_error)]
pub struct InvCollectionError {
pub inv_collection_id: Uuid,
pub idx: i32,
pub idx: SqlU16,
pub message: String,
}

impl InvCollectionError {
pub fn new(inv_collection_id: Uuid, idx: i32, message: String) -> Self {
InvCollectionError { inv_collection_id, idx, message }
pub fn new(inv_collection_id: Uuid, idx: u16, message: String) -> Self {
InvCollectionError {
inv_collection_id,
idx: SqlU16::from(idx),
message,
}
}
}

Expand All @@ -228,16 +237,73 @@ pub struct InvServiceProcessor {
pub source: String,

pub sp_type: SpType,
// XXX-dap newtype all around
pub sp_slot: i32,
pub sp_slot: SpMgsSlot,

// XXX-dap newtype all around
// XXX-dap numeric types?
pub baseboard_revision: i64,
pub baseboard_revision: BaseboardRevision,
pub hubris_archive_id: String,
pub power_state: HwPowerState,
}

#[derive(Copy, Clone, Debug, AsExpression, FromSqlRow)]
#[diesel(sql_type = sql_types::Int4)]
pub struct SpMgsSlot(SqlU16);

NewtypeFrom! { () pub struct SpMgsSlot(SqlU16); }
NewtypeDeref! { () pub struct SpMgsSlot(SqlU16); }
NewtypeDisplay! { () pub struct SpMgsSlot(SqlU16); }

impl ToSql<sql_types::Int4, Pg> for SpMgsSlot {
fn to_sql<'a>(
&'a self,
out: &mut serialize::Output<'a, '_, Pg>,
) -> serialize::Result {
<SqlU16 as ToSql<sql_types::Int4, Pg>>::to_sql(
&self.0,
&mut out.reborrow(),
)
}
}

impl<DB> FromSql<sql_types::Int4, DB> for SpMgsSlot
where
DB: Backend,
SqlU16: FromSql<sql_types::Int4, DB>,
{
fn from_sql(bytes: DB::RawValue<'_>) -> deserialize::Result<Self> {
Ok(SpMgsSlot(SqlU16::from_sql(bytes)?))
}
}

#[derive(Copy, Clone, Debug, AsExpression, FromSqlRow)]
#[diesel(sql_type = sql_types::Int8)]
pub struct BaseboardRevision(SqlU32);

NewtypeFrom! { () pub struct BaseboardRevision(SqlU32); }
NewtypeDeref! { () pub struct BaseboardRevision(SqlU32); }
NewtypeDisplay! { () pub struct BaseboardRevision(SqlU32); }

impl ToSql<sql_types::Int8, Pg> for BaseboardRevision {
fn to_sql<'a>(
&'a self,
out: &mut serialize::Output<'a, '_, Pg>,
) -> serialize::Result {
<SqlU32 as ToSql<sql_types::Int8, Pg>>::to_sql(
&self.0,
&mut out.reborrow(),
)
}
}

impl<DB> FromSql<sql_types::Int8, DB> for BaseboardRevision
where
DB: Backend,
SqlU32: FromSql<sql_types::Int8, DB>,
{
fn from_sql(bytes: DB::RawValue<'_>) -> deserialize::Result<Self> {
Ok(BaseboardRevision(SqlU32::from_sql(bytes)?))
}
}

#[derive(Queryable, Clone, Debug, Selectable)]
#[diesel(table_name = inv_root_of_trust)]
pub struct InvRootOfTrust {
Expand Down
2 changes: 2 additions & 0 deletions nexus/db-model/src/unsigned.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ pub struct SqlU16(pub u16);

NewtypeFrom! { () pub struct SqlU16(u16); }
NewtypeDeref! { () pub struct SqlU16(u16); }
NewtypeDisplay! { () pub struct SqlU16(u16); }

impl SqlU16 {
pub fn new(value: u16) -> Self {
Expand Down Expand Up @@ -134,6 +135,7 @@ pub struct SqlU32(pub u32);

NewtypeFrom! { () pub struct SqlU32(u32); }
NewtypeDeref! { () pub struct SqlU32(u32); }
NewtypeDisplay! { () pub struct SqlU32(u32); }

impl SqlU32 {
pub fn new(value: u32) -> Self {
Expand Down
93 changes: 74 additions & 19 deletions nexus/db-queries/src/db/datastore/inventory.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use diesel::IntoSql;
use diesel::NullableExpressionMethods;
use diesel::QueryDsl;
use diesel::QuerySource;
use diesel::Table;
use nexus_db_model::CabooseWhich;
use nexus_db_model::CabooseWhichEnum;
use nexus_db_model::HwBaseboardId;
Expand Down Expand Up @@ -208,6 +209,32 @@ impl DataStore {
.internal_context("inserting service processor"),
)
})?;

// This statement is just here to force a compilation error
// if the set of columns in `inv_service_processor` changes.
// The code above attempts to insert a row into
// `inv_service_processor` using an explicit list of columns
// and values. Without the following statement, If a new
// required column were added, this would fail at runtime.
//
// If you're here because of a compile error, you might be
// changing the `inv_service_processor` table. Update the
// statement below and be sure to update the code above,
// too!
//
// See also similar comments in blocks below, near other
// uses of `all_columns().
let (
_inv_collection_id,
_hw_baseboard_id,
_time_collected,
_source,
_sp_type,
_sp_slot,
_baseboard_revision,
_hubris_archive_id,
_power_state,
) = sp_dsl::inv_service_processor::all_columns();
}
}

Expand All @@ -217,9 +244,6 @@ impl DataStore {
use db::schema::hw_baseboard_id::dsl as baseboard_dsl;
use db::schema::inv_root_of_trust::dsl as rot_dsl;

// XXX-dap is there some way to make this fail to column if,
// say, we add another column to one of these tables? Maybe a
// match on a struct and make sure we get all the fields?
for (baseboard_id, rot) in &collection.rots {
let selection = db::schema::hw_baseboard_id::table
.select((
Expand Down Expand Up @@ -285,6 +309,22 @@ impl DataStore {
.internal_context("inserting service processor"),
)
})?;

// See the comment in the previous block (where we use
// `inv_service_processor::all_columns()`). The same
// applies here.
let (
_inv_collection_id,
_hw_baseboard_id,
_time_collected,
_source,
_rot_slot_active,
_rot_slot_boot_pref_persistent,
_rot_slot_boot_pref_persistent_pending,
_rot_slot_boot_pref_transient,
_rot_slot_a_sha3_256,
_rot_slot_b_sha3_256,
) = rot_dsl::inv_root_of_trust::all_columns();
}
}

Expand Down Expand Up @@ -323,12 +363,20 @@ impl DataStore {
.iter()
.enumerate()
.map(|(i, error)| {
// XXX-dap unwrap
let index = i32::try_from(i).unwrap();
let index =
u16::try_from(i).map_err(|e| {
Error::internal_error(&format!(
"failed to convert error index to u16 (too \
many errors in inventory collection?): {}", e))
})?;
let message = format!("{:#}", error);
InvCollectionError::new(collection_id, index, message)
Ok(InvCollectionError::new(
collection_id,
index,
message,
))
})
.collect::<Vec<_>>();
.collect::<Result<Vec<_>, Error>>()?;
let _ = diesel::insert_into(errors_dsl::inv_collection_error)
.values(error_values)
.execute_async(&conn)
Expand Down Expand Up @@ -370,8 +418,6 @@ impl DataStore {
&self,
opctx: &OpContext,
nkeep: u32,
// XXX-dap can we not just update the log inside the opctx?
log: &slog::Logger,
) -> Result<(), Error> {
// There could be any number of collections in the database: 0, 1, ...,
// nkeep, nkeep + 1, ..., up to a very large number. Of these, some of
Expand Down Expand Up @@ -407,10 +453,10 @@ impl DataStore {
opctx.authorize(authz::Action::Modify, &authz::INVENTORY).await?;

loop {
match self.inventory_find_pruneable(opctx, nkeep, log).await? {
match self.inventory_find_pruneable(opctx, nkeep).await? {
None => break,
Some(collection_id) => {
self.inventory_delete_collection(opctx, log, collection_id)
self.inventory_delete_collection(opctx, collection_id)
.await?
}
}
Expand All @@ -423,7 +469,6 @@ impl DataStore {
&self,
opctx: &OpContext,
nkeep: u32,
log: &slog::Logger,
) -> Result<Option<Uuid>, Error> {
// The caller of this (non-pub) function is responsible for authz.

Expand Down Expand Up @@ -505,7 +550,7 @@ impl DataStore {

if u32::try_from(candidates.len()).unwrap() <= nkeep {
debug!(
log,
&opctx.log,
"inventory_prune_one: nothing eligible for removal (too few)";
"candidates" => ?candidates,
);
Expand All @@ -529,14 +574,14 @@ impl DataStore {
.map(|(collection_id, _nerrors)| *collection_id);
if let Some(c) = candidate {
debug!(
log,
&opctx.log,
"inventory_prune_one: eligible for removal";
"collection_id" => c.to_string(),
"candidates" => ?candidates,
);
} else {
debug!(
log,
&opctx.log,
"inventory_prune_one: nothing eligible for removal";
"candidates" => ?candidates,
);
Expand All @@ -547,7 +592,6 @@ impl DataStore {
async fn inventory_delete_collection(
&self,
opctx: &OpContext,
log: &slog::Logger,
collection_id: Uuid,
) -> Result<(), Error> {
// The caller of this (non-pub) function is responsible for authz.
Expand Down Expand Up @@ -677,7 +721,7 @@ impl DataStore {
}
})?;

info!(log, "removed inventory collection";
info!(&opctx.log, "removed inventory collection";
"collection_id" => collection_id.to_string(),
"ncollections" => ncollections,
"nsps" => nsps,
Expand Down Expand Up @@ -898,8 +942,6 @@ impl diesel::query_builder::QueryFragment<diesel::pg::Pg> for InvCabooseInsert {
self.into_inv_caboose.walk_ast(pass.reborrow())?;

pass.push_sql("(");
// XXX-dap want a way for this to fail at compile time if columns or
// types are wrong?
pass.push_identifier(dsl_inv_caboose::hw_baseboard_id::NAME)?;
pass.push_sql(", ");
pass.push_identifier(dsl_inv_caboose::sw_caboose_id::NAME)?;
Expand All @@ -914,6 +956,19 @@ impl diesel::query_builder::QueryFragment<diesel::pg::Pg> for InvCabooseInsert {
pass.push_sql(")\n");
pass.push_sql("SELECT * FROM my_new_row");

// See the comment in inventory_insert_collection() where we use
// `inv_service_processor::all_columns()`. The same applies here.
// If you update the statement below because the schema for
// `inv_caboose` has changed, be sure to update the code above, too!
let (
_hw_baseboard_id,
_sw_caboose_id,
_inv_collection_id,
_time_collected,
_source,
_which,
) = dsl_inv_caboose::inv_caboose::all_columns();

Ok(())
}
}
Expand Down
8 changes: 3 additions & 5 deletions nexus/src/app/background/inventory_collection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ impl BackgroundTask for InventoryCollector {
&self.resolver,
&self.creator,
self.nkeep,
&opctx.log,
)
.await
.context("failed to collect inventory")
Expand Down Expand Up @@ -89,10 +88,9 @@ async fn inventory_activate(
resolver: &internal_dns::resolver::Resolver,
creator: &str,
nkeep: u32,
log: &slog::Logger,
) -> Result<Collection, anyhow::Error> {
datastore
.inventory_prune_collections(opctx, nkeep, log)
.inventory_prune_collections(opctx, nkeep)
.await
.context("pruning old collections")?;

Expand All @@ -104,15 +102,15 @@ async fn inventory_activate(
.into_iter()
.map(|sockaddr| {
let url = format!("http://{}", sockaddr);
let log = log.new(o!("gateway_url" => url.clone()));
let log = opctx.log.new(o!("gateway_url" => url.clone()));
Arc::new(gateway_client::Client::new(&url, log))
})
.collect::<Vec<_>>();

// Run a collection.
let inventory = nexus_inventory::Collector::new(
creator,
"activation", // TODO-dap useless
"activation", // XXX-dap useless
&mgs_clients,
);
let collection =
Expand Down

0 comments on commit c06c1be

Please sign in to comment.