From 7f42bc9f3f58647b1030fd0dab42451a0bd79200 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Mon, 20 Nov 2023 14:31:07 -0500 Subject: [PATCH] Add `omdb db validate` subcommands (#4376) In order to diagnose if customer-support#57 is caused by _not_ having the fixes for omicron#3866, add a few commands to omdb to validate the contents of the database: Usage: omdb db validate Commands: validate-volume-references Validate each `volume_references` column in the region snapshots table validate-region-snapshots Find either region snapshots Nexus knows about that the corresponding Crucible agent says were deleted, or region snapshots that Nexus doesn't know about help Print this message or the help of the given subcommand(s) This commit also adds an environment variable OMDB_FETCH_LIMIT, which overrides the default fetch limit. --- Cargo.lock | 1 + dev-tools/omdb/Cargo.toml | 3 +- dev-tools/omdb/src/bin/omdb/db.rs | 463 +++++++++++++++++++- dev-tools/omdb/tests/usage_errors.out | 8 +- nexus/db-queries/src/db/datastore/mod.rs | 1 + nexus/db-queries/src/db/datastore/volume.rs | 2 +- 6 files changed, 473 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 609e1699cf..487227e187 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4684,6 +4684,7 @@ dependencies = [ "async-bb8-diesel", "chrono", "clap 4.4.3", + "crucible-agent-client", "diesel", "dropshot", "expectorate", diff --git a/dev-tools/omdb/Cargo.toml b/dev-tools/omdb/Cargo.toml index ff3c650d6d..a8834a0b29 100644 --- a/dev-tools/omdb/Cargo.toml +++ b/dev-tools/omdb/Cargo.toml @@ -12,6 +12,7 @@ anyhow.workspace = true async-bb8-diesel.workspace = true chrono.workspace = true clap.workspace = true +crucible-agent-client.workspace = true diesel.workspace = true dropshot.workspace = true futures.workspace = true @@ -39,10 +40,10 @@ tokio = { workspace = true, features = [ "full" ] } uuid.workspace = true ipnetwork.workspace = true omicron-workspace-hack.workspace = true +nexus-test-utils.workspace = true [dev-dependencies] expectorate.workspace = true -nexus-test-utils.workspace = true nexus-test-utils-macros.workspace = true omicron-nexus.workspace = true omicron-test-utils.workspace = true diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index d009c05f86..5fa19a1a27 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/dev-tools/omdb/src/bin/omdb/db.rs @@ -19,7 +19,9 @@ use crate::Omdb; use anyhow::anyhow; use anyhow::bail; use anyhow::Context; +use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; +use async_bb8_diesel::AsyncSimpleConnection; use chrono::SecondsFormat; use clap::Args; use clap::Subcommand; @@ -30,6 +32,7 @@ use diesel::BoolExpressionMethods; use diesel::ExpressionMethods; use diesel::JoinOnDsl; use diesel::NullableExpressionMethods; +use diesel::TextExpressionMethods; use gateway_client::types::SpType; use nexus_db_model::Dataset; use nexus_db_model::Disk; @@ -49,15 +52,19 @@ use nexus_db_model::Snapshot; use nexus_db_model::SnapshotState; use nexus_db_model::SwCaboose; use nexus_db_model::Vmm; +use nexus_db_model::Volume; use nexus_db_model::Zpool; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; +use nexus_db_queries::db::datastore::read_only_resources_associated_with_volume; +use nexus_db_queries::db::datastore::CrucibleTargets; use nexus_db_queries::db::datastore::DataStoreConnection; use nexus_db_queries::db::datastore::InstanceAndActiveVmm; use nexus_db_queries::db::identity::Asset; use nexus_db_queries::db::lookup::LookupPath; use nexus_db_queries::db::model::ServiceKind; use nexus_db_queries::db::DataStore; +use nexus_test_utils::db::ALLOW_FULL_TABLE_SCAN_SQL; use nexus_types::identity::Resource; use nexus_types::internal_api::params::DnsRecord; use nexus_types::internal_api::params::Srv; @@ -66,6 +73,7 @@ use nexus_types::inventory::Collection; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Generation; use omicron_common::postgres_config::PostgresConfigWithUrl; +use sled_agent_client::types::VolumeConstructionRequest; use std::cmp::Ordering; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -125,7 +133,8 @@ pub struct DbArgs { /// limit to apply to queries that fetch rows #[clap( long = "fetch-limit", - default_value_t = NonZeroU32::new(500).unwrap() + default_value_t = NonZeroU32::new(500).unwrap(), + env("OMDB_FETCH_LIMIT"), )] fetch_limit: NonZeroU32, @@ -152,6 +161,8 @@ enum DbCommands { Network(NetworkArgs), /// Print information about snapshots Snapshots(SnapshotArgs), + /// Validate the contents of the database + Validate(ValidateArgs), } #[derive(Debug, Args)] @@ -308,6 +319,23 @@ struct SnapshotInfoArgs { uuid: Uuid, } +#[derive(Debug, Args)] +struct ValidateArgs { + #[command(subcommand)] + command: ValidateCommands, +} + +#[derive(Debug, Subcommand)] +enum ValidateCommands { + /// Validate each `volume_references` column in the region snapshots table + ValidateVolumeReferences, + + /// Find either region snapshots Nexus knows about that the corresponding + /// Crucible agent says were deleted, or region snapshots that Nexus doesn't + /// know about. + ValidateRegionSnapshots, +} + impl DbArgs { /// Run a `omdb db` subcommand. pub(crate) async fn run_cmd( @@ -429,6 +457,18 @@ impl DbArgs { DbCommands::Snapshots(SnapshotArgs { command: SnapshotCommands::List, }) => cmd_db_snapshot_list(&datastore, self.fetch_limit).await, + DbCommands::Validate(ValidateArgs { + command: ValidateCommands::ValidateVolumeReferences, + }) => { + cmd_db_validate_volume_references(&datastore, self.fetch_limit) + .await + } + DbCommands::Validate(ValidateArgs { + command: ValidateCommands::ValidateRegionSnapshots, + }) => { + cmd_db_validate_region_snapshots(&datastore, self.fetch_limit) + .await + } } } } @@ -1705,6 +1745,427 @@ async fn cmd_db_eips( Ok(()) } +/// Validate the `volume_references` column of the region snapshots table +async fn cmd_db_validate_volume_references( + datastore: &DataStore, + limit: NonZeroU32, +) -> Result<(), anyhow::Error> { + // First, get all region snapshot records + let region_snapshots: Vec = { + let region_snapshots: Vec = datastore + .pool_connection_for_tests() + .await? + .transaction_async(|conn| async move { + // Selecting all region snapshots requires a full table scan + conn.batch_execute_async(ALLOW_FULL_TABLE_SCAN_SQL).await?; + + use db::schema::region_snapshot::dsl; + dsl::region_snapshot + .select(RegionSnapshot::as_select()) + .get_results_async(&conn) + .await + }) + .await?; + + check_limit(®ion_snapshots, limit, || { + String::from("listing region snapshots") + }); + + region_snapshots + }; + + #[derive(Tabled)] + struct Row { + dataset_id: Uuid, + region_id: Uuid, + snapshot_id: Uuid, + error: String, + } + + let mut rows = Vec::new(); + + // Then, for each, make sure that the `volume_references` matches what is in + // the volume table + for region_snapshot in region_snapshots { + let matching_volumes: Vec = { + let snapshot_addr = region_snapshot.snapshot_addr.clone(); + + let matching_volumes = datastore + .pool_connection_for_tests() + .await? + .transaction_async(|conn| async move { + // Selecting all volumes based on the data column requires a + // full table scan + conn.batch_execute_async(ALLOW_FULL_TABLE_SCAN_SQL).await?; + + let pattern = format!("%{}%", &snapshot_addr); + + use db::schema::volume::dsl; + + // Find all volumes that have not been deleted that contain + // this snapshot_addr. If a Volume has been soft deleted, + // then the region snapshot record should have had its + // volume references column updated accordingly. + dsl::volume + .filter(dsl::time_deleted.is_null()) + .filter(dsl::data.like(pattern)) + .select(Volume::as_select()) + .get_results_async(&conn) + .await + }) + .await?; + + check_limit(&matching_volumes, limit, || { + String::from("finding matching volumes") + }); + + matching_volumes + }; + + // The Crucible Agent will reuse ports for regions and running snapshots + // when they're deleted. Check that the matching volume construction requests + // reference this snapshot addr as a read-only target. + let matching_volumes = matching_volumes + .into_iter() + .filter(|volume| { + let vcr: VolumeConstructionRequest = + serde_json::from_str(&volume.data()).unwrap(); + + let mut targets = CrucibleTargets::default(); + read_only_resources_associated_with_volume(&vcr, &mut targets); + + targets + .read_only_targets + .contains(®ion_snapshot.snapshot_addr) + }) + .count(); + + if matching_volumes != region_snapshot.volume_references as usize { + rows.push(Row { + dataset_id: region_snapshot.dataset_id, + region_id: region_snapshot.region_id, + snapshot_id: region_snapshot.snapshot_id, + error: format!( + "record has {} volume references when it should be {}!", + region_snapshot.volume_references, matching_volumes, + ), + }); + } else { + // The volume references are correct, but additionally check to see + // deleting is true when matching_volumes is 0. Be careful: in the + // snapshot create saga, the region snapshot record is created + // before the snapshot's volume is inserted into the DB. There's a + // time between these operations that this function would flag that + // this region snapshot should have `deleting` set to true. + + if matching_volumes == 0 && !region_snapshot.deleting { + rows.push(Row { + dataset_id: region_snapshot.dataset_id, + region_id: region_snapshot.region_id, + snapshot_id: region_snapshot.snapshot_id, + error: String::from( + "record has 0 volume references but deleting is false!", + ), + }); + } + } + } + + let table = tabled::Table::new(rows) + .with(tabled::settings::Style::empty()) + .to_string(); + + println!("{}", table); + + Ok(()) +} + +async fn cmd_db_validate_region_snapshots( + datastore: &DataStore, + limit: NonZeroU32, +) -> Result<(), anyhow::Error> { + let mut regions_to_snapshots_map: BTreeMap> = + BTreeMap::default(); + + // First, get all region snapshot records (with their corresponding dataset) + let datasets_and_region_snapshots: Vec<(Dataset, RegionSnapshot)> = { + let datasets_region_snapshots: Vec<(Dataset, RegionSnapshot)> = + datastore + .pool_connection_for_tests() + .await? + .transaction_async(|conn| async move { + // Selecting all datasets and region snapshots requires a full table scan + conn.batch_execute_async(ALLOW_FULL_TABLE_SCAN_SQL).await?; + + use db::schema::dataset::dsl as dataset_dsl; + use db::schema::region_snapshot::dsl; + + dsl::region_snapshot + .inner_join( + dataset_dsl::dataset + .on(dsl::dataset_id.eq(dataset_dsl::id)), + ) + .select(( + Dataset::as_select(), + RegionSnapshot::as_select(), + )) + .get_results_async(&conn) + .await + }) + .await?; + + check_limit(&datasets_region_snapshots, limit, || { + String::from("listing datasets and region snapshots") + }); + + datasets_region_snapshots + }; + + #[derive(Tabled)] + struct Row { + dataset_id: Uuid, + region_id: Uuid, + snapshot_id: Uuid, + dataset_addr: std::net::SocketAddrV6, + error: String, + } + + let mut rows = Vec::new(); + + // Then, for each one, reconcile with the corresponding Crucible Agent: do + // the region_snapshot records match reality? + for (dataset, region_snapshot) in datasets_and_region_snapshots { + regions_to_snapshots_map + .entry(region_snapshot.region_id) + .or_default() + .insert(region_snapshot.snapshot_id); + + use crucible_agent_client::types::RegionId; + use crucible_agent_client::types::State; + use crucible_agent_client::Client as CrucibleAgentClient; + + let url = format!("http://{}", dataset.address()); + let client = CrucibleAgentClient::new(&url); + + let actual_region_snapshots = client + .region_get_snapshots(&RegionId( + region_snapshot.region_id.to_string(), + )) + .await?; + + let snapshot_id = region_snapshot.snapshot_id.to_string(); + + if actual_region_snapshots + .snapshots + .iter() + .any(|x| x.name == snapshot_id) + { + // A snapshot currently exists, matching the database entry + } else { + // In this branch, there's a database entry for a snapshot that was + // deleted. Due to how the snapshot create saga is currently + // written, a database entry would not have been created unless a + // snapshot was successfully made: unless that saga changes, we can + // be reasonably sure that this snapshot existed at some point. + + match actual_region_snapshots.running_snapshots.get(&snapshot_id) { + Some(running_snapshot) => { + match running_snapshot.state { + State::Destroyed | State::Failed => { + // In this branch, we can be sure a snapshot previously + // existed and was deleted: a running snapshot was made + // from it, then deleted, and the snapshot does not + // currently exist in the list of snapshots for this + // region. This record should be deleted. + + // Before recommending anything, validate the higher + // level Snapshot object too: it should have been + // destroyed. + + let snapshot: Snapshot = { + use db::schema::snapshot::dsl; + + dsl::snapshot + .filter( + dsl::id.eq(region_snapshot.snapshot_id), + ) + .select(Snapshot::as_select()) + .first_async( + &*datastore + .pool_connection_for_tests() + .await?, + ) + .await? + }; + + if snapshot.time_deleted().is_some() { + // This is ok - Nexus currently soft-deletes its + // resource records. + rows.push(Row { + dataset_id: region_snapshot.dataset_id, + region_id: region_snapshot.region_id, + snapshot_id: region_snapshot.snapshot_id, + dataset_addr: dataset.address(), + error: String::from( + "region snapshot was deleted, please remove its record", + ), + }); + } else { + // If the higher level Snapshot was _not_ + // deleted, this is a Nexus bug: something told + // the Agent to delete the snapshot when the + // higher level Snapshot was not deleted! + + rows.push(Row { + dataset_id: region_snapshot.dataset_id, + region_id: region_snapshot.region_id, + snapshot_id: region_snapshot.snapshot_id, + dataset_addr: dataset.address(), + error: String::from( + "NEXUS BUG: region snapshot was deleted, but the higher level snapshot was not!", + ), + }); + } + } + + State::Requested + | State::Created + | State::Tombstoned => { + // The agent is in a bad state: we did not find the + // snapshot in the list of snapshots for this + // region, but either: + // + // - there's a requested or existing running + // snapshot for it, or + // + // - there's a running snapshot that should have + // been completely deleted before the snapshot + // itself was deleted. + // + // This should have never been allowed to happen by + // the Agent, so it's a bug. + + rows.push(Row { + dataset_id: region_snapshot.dataset_id, + region_id: region_snapshot.region_id, + snapshot_id: region_snapshot.snapshot_id, + dataset_addr: dataset.address(), + error: format!( + "AGENT BUG: region snapshot was deleted but has a running snapshot in state {:?}!", + running_snapshot.state, + ), + }); + } + } + } + + None => { + // A running snapshot never existed for this snapshot + } + } + } + } + + // Second, get all regions + let datasets_and_regions: Vec<(Dataset, Region)> = { + let datasets_and_regions: Vec<(Dataset, Region)> = datastore + .pool_connection_for_tests() + .await? + .transaction_async(|conn| async move { + // Selecting all datasets and regions requires a full table scan + conn.batch_execute_async(ALLOW_FULL_TABLE_SCAN_SQL).await?; + + use db::schema::dataset::dsl as dataset_dsl; + use db::schema::region::dsl; + + dsl::region + .inner_join( + dataset_dsl::dataset + .on(dsl::dataset_id.eq(dataset_dsl::id)), + ) + .select((Dataset::as_select(), Region::as_select())) + .get_results_async(&conn) + .await + }) + .await?; + + check_limit(&datasets_and_regions, limit, || { + String::from("listing datasets and regions") + }); + + datasets_and_regions + }; + + // Reconcile with the Crucible agents: are there snapshots that Nexus does + // not know about? + for (dataset, region) in datasets_and_regions { + use crucible_agent_client::types::RegionId; + use crucible_agent_client::types::State; + use crucible_agent_client::Client as CrucibleAgentClient; + + let url = format!("http://{}", dataset.address()); + let client = CrucibleAgentClient::new(&url); + + let actual_region_snapshots = client + .region_get_snapshots(&RegionId(region.id().to_string())) + .await?; + + let default = HashSet::default(); + let nexus_region_snapshots: &HashSet = + regions_to_snapshots_map.get(®ion.id()).unwrap_or(&default); + + for actual_region_snapshot in &actual_region_snapshots.snapshots { + let snapshot_id: Uuid = actual_region_snapshot.name.parse()?; + if !nexus_region_snapshots.contains(&snapshot_id) { + rows.push(Row { + dataset_id: dataset.id(), + region_id: region.id(), + snapshot_id, + dataset_addr: dataset.address(), + error: String::from( + "Nexus does not know about this snapshot!", + ), + }); + } + } + + for (_, actual_region_running_snapshot) in + &actual_region_snapshots.running_snapshots + { + let snapshot_id: Uuid = + actual_region_running_snapshot.name.parse()?; + + match actual_region_running_snapshot.state { + State::Destroyed | State::Failed | State::Tombstoned => { + // don't check, Nexus would consider this gone + } + + State::Requested | State::Created => { + if !nexus_region_snapshots.contains(&snapshot_id) { + rows.push(Row { + dataset_id: dataset.id(), + region_id: region.id(), + snapshot_id, + dataset_addr: dataset.address(), + error: String::from( + "Nexus does not know about this running snapshot!" + ), + }); + } + } + } + } + } + + let table = tabled::Table::new(rows) + .with(tabled::settings::Style::empty()) + .to_string(); + + println!("{}", table); + + Ok(()) +} + fn print_name( prefix: &str, name: &str, diff --git a/dev-tools/omdb/tests/usage_errors.out b/dev-tools/omdb/tests/usage_errors.out index e859c325a5..eaabf970a6 100644 --- a/dev-tools/omdb/tests/usage_errors.out +++ b/dev-tools/omdb/tests/usage_errors.out @@ -98,11 +98,13 @@ Commands: instances Print information about customer instances network Print information about the network snapshots Print information about snapshots + validate Validate the contents of the database help Print this message or the help of the given subcommand(s) Options: --db-url URL of the database SQL interface [env: OMDB_DB_URL=] - --fetch-limit limit to apply to queries that fetch rows [default: 500] + --fetch-limit limit to apply to queries that fetch rows [env: + OMDB_FETCH_LIMIT=] [default: 500] -h, --help Print help ============================================= EXECUTING COMMAND: omdb ["db", "--help"] @@ -122,11 +124,13 @@ Commands: instances Print information about customer instances network Print information about the network snapshots Print information about snapshots + validate Validate the contents of the database help Print this message or the help of the given subcommand(s) Options: --db-url URL of the database SQL interface [env: OMDB_DB_URL=] - --fetch-limit limit to apply to queries that fetch rows [default: 500] + --fetch-limit limit to apply to queries that fetch rows [env: + OMDB_FETCH_LIMIT=] [default: 500] -h, --help Print help --------------------------------------------- stderr: diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 7385970fb1..8be3386183 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -103,6 +103,7 @@ pub use rack::RackInit; pub use silo::Discoverability; pub use switch_port::SwitchPortSettingsCombinedResult; pub use virtual_provisioning_collection::StorageType; +pub use volume::read_only_resources_associated_with_volume; pub use volume::CrucibleResources; pub use volume::CrucibleTargets; diff --git a/nexus/db-queries/src/db/datastore/volume.rs b/nexus/db-queries/src/db/datastore/volume.rs index 5d753f0742..1e64d784f7 100644 --- a/nexus/db-queries/src/db/datastore/volume.rs +++ b/nexus/db-queries/src/db/datastore/volume.rs @@ -1024,7 +1024,7 @@ impl DataStore { /// Return the targets from a VolumeConstructionRequest. /// /// The targets of a volume construction request map to resources. -fn read_only_resources_associated_with_volume( +pub fn read_only_resources_associated_with_volume( vcr: &VolumeConstructionRequest, crucible_targets: &mut CrucibleTargets, ) {