From 028b0c5318b49a27f7b6578292b892812252ac67 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 13 Oct 2023 19:18:40 -0700 Subject: [PATCH] broken attempt to fix it --- dev-tools/omdb/src/bin/omdb/db.rs | 21 ++-- nexus/db-model/src/inventory.rs | 4 +- nexus/db-model/src/schema.rs | 3 +- .../db-queries/src/db/datastore/inventory.rs | 102 ++++++++---------- nexus/inventory/src/builder.rs | 2 - schema/crdb/dbinit.sql | 2 +- 6 files changed, 58 insertions(+), 76 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index 7bc56c692d..c163b4e70f 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/dev-tools/omdb/src/bin/omdb/db.rs @@ -1528,16 +1528,13 @@ async fn cmd_db_inventory_collections_list( .context("counting SPs")? }; - let took = collection - .time_done - .map(|t| { - format!( - "{} ms", - t.signed_duration_since(&collection.time_started) - .num_milliseconds() - ) - }) - .unwrap_or_else(|| format!("-")); + let took = format!( + "{} ms", + collection + .time_done + .signed_duration_since(&collection.time_started) + .num_milliseconds() + ); rows.push(CollectionRow { id: collection.id, started: humantime::format_rfc3339_seconds( @@ -1654,9 +1651,7 @@ async fn inv_collection_print( ); println!( "done: {}", - c.time_done - .map(|t| humantime::format_rfc3339_millis(t.into()).to_string()) - .unwrap_or_else(|| String::from("-")) + humantime::format_rfc3339_millis(c.time_done.into()).to_string() ); Ok(()) diff --git a/nexus/db-model/src/inventory.rs b/nexus/db-model/src/inventory.rs index 1dba924ee0..621f002300 100644 --- a/nexus/db-model/src/inventory.rs +++ b/nexus/db-model/src/inventory.rs @@ -138,7 +138,7 @@ impl From for SpType { pub struct InvCollection { pub id: Uuid, pub time_started: DateTime, - pub time_done: Option>, + pub time_done: DateTime, pub collector: String, pub comment: String, } @@ -148,7 +148,7 @@ impl<'a> From<&'a Collection> for InvCollection { InvCollection { id: c.id, time_started: c.time_started, - time_done: Some(c.time_done), + time_done: c.time_done, collector: c.collector.clone(), comment: c.comment.clone(), } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index 5c17ef575e..0c018c618a 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1140,7 +1140,7 @@ table! { inv_collection (id) { id -> Uuid, time_started -> Timestamptz, - time_done -> Nullable, + time_done -> Timestamptz, collector -> Text, comment -> Text, } @@ -1226,6 +1226,7 @@ allow_tables_to_appear_in_same_query!(ip_pool_range, ip_pool); joinable!(ip_pool_range -> ip_pool (ip_pool_id)); allow_tables_to_appear_in_same_query!(inv_collection, inv_collection_error); +joinable!(inv_collection_error -> inv_collection (inv_collection_id)); allow_tables_to_appear_in_same_query!(sw_caboose, inv_caboose); allow_tables_to_appear_in_same_query!( diff --git a/nexus/db-queries/src/db/datastore/inventory.rs b/nexus/db-queries/src/db/datastore/inventory.rs index 2bb83ac6a3..3c2abdf97e 100644 --- a/nexus/db-queries/src/db/datastore/inventory.rs +++ b/nexus/db-queries/src/db/datastore/inventory.rs @@ -11,7 +11,6 @@ use crate::db::error::ErrorHandler; use crate::db::TransactionError; use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; -use async_bb8_diesel::OptionalExtension; use chrono::DateTime; use chrono::Utc; use diesel::sql_types; @@ -427,70 +426,59 @@ impl DataStore { ) -> Result, Error> { // The caller of this (non-pub) function is responsible for authz. - use db::schema::inv_collection::dsl as cdsl; - use db::schema::inv_collection_error::dsl as edsl; - + // XXX-dap this is running afoul of a table scan. I don't think it + // should. But I don't think it's worth debugging, either. I could + // instead just select the most recent "nkeep+1" along with the error + // count for each one and figure this out in Rust. let conn = self.pool_connection_authorized(opctx).await?; - let errors_subquery = edsl::inv_collection_error - .select(edsl::inv_collection_id) - .filter(edsl::inv_collection_id.eq(cdsl::id)); - - let latest_complete: Option = cdsl::inv_collection - .select(cdsl::id) - .filter(cdsl::time_done.is_not_null()) - .filter(diesel::dsl::not(diesel::dsl::exists(errors_subquery))) - .order_by(cdsl::time_started.desc()) - .limit(1) - .first_async(&*conn) - .await - .optional() - .map_err(|e| { - public_error_from_diesel(e.into(), ErrorHandler::Server) - }) - .internal_context("finding latest successful collection")?; - - debug!( - log, - "inventory_prune_one: latest"; - "latest_complete" => ?latest_complete - ); - - // If we ever start truly supporting inserting collections that are not - // yet finished, we'll have to figure out how to tell if somebody's - // still trying to update them when we go to clean them. - // XXX-dap should we just make that illegal now? - let mut candidates_query = cdsl::inv_collection - .select(cdsl::id) - .filter(cdsl::time_done.is_not_null()) - .into_boxed(); - - if let Some(latest) = latest_complete { - candidates_query = candidates_query.filter(cdsl::id.ne(latest)); - } - - let candidates: Vec = candidates_query - .order_by(cdsl::time_started.asc()) + let candidates: Vec<(Uuid, i64)> = db::schema::inv_collection::table + .inner_join(db::schema::inv_collection_error::table) + .group_by(db::schema::inv_collection::id) + .select(( + db::schema::inv_collection::id, + diesel::dsl::count( + db::schema::inv_collection_error::inv_collection_id, + ), + )) + .order_by(db::schema::inv_collection::time_started.desc()) .limit(i64::from(nkeep) + 1) .load_async(&*conn) .await .map_err(|e| { public_error_from_diesel(e.into(), ErrorHandler::Server) }) - .internal_context("finding oldest collections")?; - - if u32::try_from(candidates.len()).unwrap() <= nkeep { - debug!(log, "inventory_prune_one: found nothing to prune"); - return Ok(None); + .internal_context("listing oldest collections")?; + + // We've now got the "nkeep + 1" oldest collections, starting with the + // very oldest. We can get rid of the oldest one unless it's the only + // complete one. Another way to think about it: find the _last_ + // complete one. Remove it from the list of candidates. Now mark the + // first item in the remaining list for deletion. + let last_completed_idx = candidates + .iter() + .enumerate() + .rev() + .find(|(_i, (_collection_id, nerrors))| *nerrors == 0); + let candidate = match last_completed_idx { + Some((i, _)) if i == 0 => candidates.iter().skip(1).next(), + _ => candidates.iter().next(), } - - let oldest = candidates[0]; - debug!( - log, - "inventory_prune_one: eligible for removal"; - "collection_id" => oldest.to_string() - ); - - Ok(Some(oldest)) + .map(|(collection_id, _nerrors)| *collection_id); + if let Some(c) = candidate { + debug!( + log, + "inventory_prune_one: eligible for removal"; + "collection_id" => c.to_string(), + "candidates" => ?candidates, + ); + } else { + debug!( + log, + "inventory_prune_one: nothing eligible for removal"; + "candidates" => ?candidates, + ); + } + Ok(candidate) } async fn inventory_delete_collection( diff --git a/nexus/inventory/src/builder.rs b/nexus/inventory/src/builder.rs index 5af75dfcba..47d29d3e87 100644 --- a/nexus/inventory/src/builder.rs +++ b/nexus/inventory/src/builder.rs @@ -22,8 +22,6 @@ use std::collections::BTreeSet; use std::sync::Arc; use uuid::Uuid; -// XXX-dap add rack id - #[derive(Debug)] pub struct CollectionBuilder { errors: Vec, diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 4cf849eed1..a65f7ab26a 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2588,7 +2588,7 @@ CREATE UNIQUE INDEX IF NOT EXISTS caboose_properties CREATE TABLE IF NOT EXISTS inv_collection ( id UUID PRIMARY KEY, time_started TIMESTAMPTZ NOT NULL, - time_done TIMESTAMPTZ, + time_done TIMESTAMPTZ NOT NULL, collector TEXT NOT NULL, comment TEXT NOT NULL );