diff --git a/dev-tools/reconfigurator-exec-unsafe/src/main.rs b/dev-tools/reconfigurator-exec-unsafe/src/main.rs index 0d3896eb039..1f02f938be7 100644 --- a/dev-tools/reconfigurator-exec-unsafe/src/main.rs +++ b/dev-tools/reconfigurator-exec-unsafe/src/main.rs @@ -108,7 +108,7 @@ impl ReconfiguratorExec { internal_dns_resolver::QorbResolver::new(vec![self.dns_server]); info!(&log, "setting up database pool"); - let pool = Arc::new(db::Pool::new(&log, &qorb_resolver)); + let pool = Arc::new(db::Pool::new(&log, &qorb_resolver, true)); let datastore = Arc::new( DataStore::new_failfast(&log, pool) .await diff --git a/live-tests/tests/common/mod.rs b/live-tests/tests/common/mod.rs index a74f81eb4f0..fc2d5b33d9b 100644 --- a/live-tests/tests/common/mod.rs +++ b/live-tests/tests/common/mod.rs @@ -134,8 +134,9 @@ async fn create_datastore( .context("failed to parse constructed postgres URL")?; let db_config = nexus_db_queries::db::Config { url }; - let pool = - Arc::new(nexus_db_queries::db::Pool::new_single_host(log, &db_config)); + let pool = Arc::new(nexus_db_queries::db::Pool::new_single_host( + log, &db_config, true, + )); DataStore::new_failfast(log, pool) .await .context("creating DataStore") diff --git a/nexus-config/src/nexus_config.rs b/nexus-config/src/nexus_config.rs index b18a621ab9e..0773ece48d4 100644 --- a/nexus-config/src/nexus_config.rs +++ b/nexus-config/src/nexus_config.rs @@ -188,12 +188,27 @@ pub struct DeploymentConfig { /// Configuration for HTTP clients to external services. #[serde(default)] pub external_http_clients: ExternalHttpClientConfig, + /// By default, we capture backtraces when claiming a connection from the DB pool, but setting + /// this flag to `false` will disable that behavior. + /// + /// This flag is intended as an escape hatch in case we ever encounter an unexpected + /// pathological case where capturing backtraces is slow enough to be an issue. + /// + /// Since we don't expect to encounter this in production, it can only currently be disabled for + /// debug and testing use cases. For this reason, we skip it when serializing and deseralizing + /// and default it to `true`. + #[serde(skip, default = "default_record_db_claim_backtraces")] + pub record_db_claim_backtraces: bool, } fn default_techport_external_server_port() -> u16 { NEXUS_TECHPORT_EXTERNAL_PORT } +fn default_record_db_claim_backtraces() -> bool { + true +} + impl DeploymentConfig { /// Load a `DeploymentConfig` from the given TOML file /// diff --git a/nexus/db-queries/src/db/pool.rs b/nexus/db-queries/src/db/pool.rs index b975b6509c7..6a334c25271 100644 --- a/nexus/db-queries/src/db/pool.rs +++ b/nexus/db-queries/src/db/pool.rs @@ -51,6 +51,7 @@ pub struct Pool { log: Logger, terminated: std::sync::atomic::AtomicBool, quiesce: watch::Sender, + record_db_claim_backtraces: bool, } // Provides an alternative to the DNS resolver for cases where we want to @@ -116,7 +117,11 @@ impl Pool { /// /// Creating this pool does not necessarily wait for connections to become /// available, as backends may shift over time. - pub fn new(log: &Logger, resolver: &QorbResolver) -> Self { + pub fn new( + log: &Logger, + resolver: &QorbResolver, + record_db_claim_backtraces: bool, + ) -> Self { let resolver = resolver.for_service(ServiceName::Cockroach); let connector = make_postgres_connector(log); let policy = Policy::default(); @@ -135,7 +140,7 @@ impl Pool { err.into_inner() } }; - Self::new_common(inner, log.clone()) + Self::new_common(inner, log.clone(), record_db_claim_backtraces) } /// Creates a new qorb-backed connection pool to a single instance of the @@ -145,7 +150,11 @@ impl Pool { /// on a single instance of the database. /// /// In production, [Self::new] should be preferred. - pub fn new_single_host(log: &Logger, db_config: &DbConfig) -> Self { + pub fn new_single_host( + log: &Logger, + db_config: &DbConfig, + record_db_claim_backtraces: bool, + ) -> Self { let resolver = make_single_host_resolver(db_config); let connector = make_postgres_connector(log); let policy = Policy::default(); @@ -164,7 +173,7 @@ impl Pool { err.into_inner() } }; - Self::new_common(inner, log.clone()) + Self::new_common(inner, log.clone(), record_db_claim_backtraces) } /// Creates a new qorb-backed connection pool to a fixed set of database @@ -194,7 +203,7 @@ impl Pool { err.into_inner() } }; - Self::new_common(inner, log.clone()) + Self::new_common(inner, log.clone(), true) } /// Creates a new qorb-backed connection pool which returns an error @@ -229,12 +238,13 @@ impl Pool { err.into_inner() } }; - Self::new_common(inner, log.clone()) + Self::new_common(inner, log.clone(), true) } fn new_common( inner: qorb::pool::Pool, log: Logger, + record_db_claim_backtraces: bool, ) -> Self { let (quiesce, _) = watch::channel(Quiesce { new_claims_allowed: ClaimsAllowed::Allowed, @@ -246,6 +256,7 @@ impl Pool { log, terminated: std::sync::atomic::AtomicBool::new(false), quiesce, + record_db_claim_backtraces, } } @@ -253,7 +264,13 @@ impl Pool { pub async fn claim(&self) -> Result { let id = self.next_id.fetch_add(1, Ordering::Relaxed); let held_since = Utc::now(); - let debug = Backtrace::force_capture().to_string(); + // This is an escape hatch in case we ever encounter an unexpected pathological case where + // capturing backtraces is slow enough to be an issue: + let debug = if self.record_db_claim_backtraces { + Backtrace::force_capture().to_string() + } else { + "(backtraces disabled)".to_string() + }; let allowed = self.quiesce.send_if_modified(|q| { if let ClaimsAllowed::Disallowed = q.new_claims_allowed { false @@ -430,7 +447,7 @@ mod test { let mut db = crdb::test_setup_database(log).await; let cfg = crate::db::Config { url: db.pg_config().clone() }; { - let pool = Pool::new_single_host(&log, &cfg); + let pool = Pool::new_single_host(&log, &cfg, true); pool.terminate().await; } db.cleanup().await.unwrap(); @@ -447,7 +464,7 @@ mod test { let mut db = crdb::test_setup_database(log).await; let cfg = crate::db::Config { url: db.pg_config().clone() }; { - let pool = Pool::new_single_host(&log, &cfg); + let pool = Pool::new_single_host(&log, &cfg, true); drop(pool); } db.cleanup().await.unwrap(); diff --git a/nexus/db-queries/src/db/pub_test_utils/mod.rs b/nexus/db-queries/src/db/pub_test_utils/mod.rs index b144cc0e5aa..733d13866e4 100644 --- a/nexus/db-queries/src/db/pub_test_utils/mod.rs +++ b/nexus/db-queries/src/db/pub_test_utils/mod.rs @@ -38,7 +38,7 @@ enum Interface { fn new_pool(log: &Logger, db: &CockroachInstance) -> Arc { let cfg = db::Config { url: db.pg_config().clone() }; - Arc::new(db::Pool::new_single_host(log, &cfg)) + Arc::new(db::Pool::new_single_host(log, &cfg, true)) } struct TestDatabaseBuilder { @@ -325,7 +325,7 @@ async fn datastore_test( use crate::authn; let cfg = db::Config { url: db.pg_config().clone() }; - let pool = Arc::new(db::Pool::new_single_host(&log, &cfg)); + let pool = Arc::new(db::Pool::new_single_host(&log, &cfg, true)); let datastore = Arc::new( DataStore::new(&log, pool, None, IdentityCheckPolicy::DontCare) .await diff --git a/nexus/src/bin/schema-updater.rs b/nexus/src/bin/schema-updater.rs index 322bb94beda..a8b3bced097 100644 --- a/nexus/src/bin/schema-updater.rs +++ b/nexus/src/bin/schema-updater.rs @@ -80,7 +80,7 @@ async fn main_impl() -> anyhow::Result<()> { let all_versions = AllSchemaVersions::load(&schema_config.schema_dir)?; let crdb_cfg = db::Config { url: args.url }; - let pool = Arc::new(db::Pool::new_single_host(&log, &crdb_cfg)); + let pool = Arc::new(db::Pool::new_single_host(&log, &crdb_cfg, true)); // We use the unchecked constructor of the datastore because we // don't want to block on someone else applying an upgrade. diff --git a/nexus/src/context.rs b/nexus/src/context.rs index 7f8a47bf5c4..07c939f2905 100644 --- a/nexus/src/context.rs +++ b/nexus/src/context.rs @@ -288,6 +288,7 @@ impl ServerContext { db::Pool::new_single_host( &log, &db::Config { url: url.clone() }, + config.deployment.record_db_claim_backtraces, ) } nexus_config::Database::FromDns => { @@ -295,7 +296,11 @@ impl ServerContext { log, "Setting up qorb database pool from DNS"; "dns_addrs" => ?qorb_resolver.bootstrap_dns_ips(), ); - db::Pool::new(&log, &qorb_resolver) + db::Pool::new( + &log, + &qorb_resolver, + config.deployment.record_db_claim_backtraces, + ) } }; diff --git a/nexus/src/populate.rs b/nexus/src/populate.rs index d60f943f66d..28c5ed7167a 100644 --- a/nexus/src/populate.rs +++ b/nexus/src/populate.rs @@ -367,7 +367,7 @@ mod test { let logctx = dev::test_setup_log("test_populator"); let db = TestDatabase::new_populate_schema_only(&logctx.log).await; let cfg = db::Config { url: db.crdb().pg_config().clone() }; - let pool = Arc::new(db::Pool::new_single_host(&logctx.log, &cfg)); + let pool = Arc::new(db::Pool::new_single_host(&logctx.log, &cfg, true)); let datastore = Arc::new( db::DataStore::new( &logctx.log, diff --git a/nexus/test-utils/src/starter.rs b/nexus/test-utils/src/starter.rs index a8d4f3d4d46..872233445d7 100644 --- a/nexus/test-utils/src/starter.rs +++ b/nexus/test-utils/src/starter.rs @@ -117,6 +117,7 @@ use slog::{Logger, debug, error, info, o}; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashMap; +use std::env; use std::fmt::Debug; use std::iter::{once, repeat, zip}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV6}; @@ -203,6 +204,12 @@ impl<'a, N: NexusServer> ControlPlaneStarter<'a, N> { let debug_dropbox_dir = TestTempDir::new(&logctx.log); + // Note that the record_db_claim_backtraces flag defaults to true; we only disable the flag + // specifically when this env var is set to "0": + if env::var_os("NEXUS_ENABLE_DB_CLAIM_BACKTRACES") == Some("0".into()) { + config.deployment.record_db_claim_backtraces = false; + } + Self { config, test_name, diff --git a/sled-agent/src/services.rs b/sled-agent/src/services.rs index 2900319b660..77dc08b67c3 100644 --- a/sled-agent/src/services.rs +++ b/sled-agent/src/services.rs @@ -2282,6 +2282,7 @@ impl ServiceManager { treat_loopback_as_external: nexus_config::TreatLoopbackAsExternal::No, }, + record_db_claim_backtraces: true, }; // Copy the partial config file to the expected