From af2e30e383b48302441f063929d4e69959db9830 Mon Sep 17 00:00:00 2001 From: Nazar Mokrynskyi Date: Wed, 10 Jan 2024 13:56:44 +0200 Subject: [PATCH] Improve storage monitor API (#2899) This removes the need to unnecessarily provide a very specific data structure `DatabaseSource` and removes huge `sc-client-db` dependency from storage monitor. It is now possible to use storage monitor with any path. P.S. I still strongly dislike that it pulls `clap` dependency for such a small feature, but many other crates do as well, so nothing special here. --- Cargo.lock | 1 - polkadot/cli/src/command.rs | 12 ++-- prdoc/pr_2899.prdoc | 10 ++++ substrate/bin/node/cli/src/service.rs | 18 +++--- substrate/client/storage-monitor/Cargo.toml | 3 +- substrate/client/storage-monitor/src/lib.rs | 61 +++++++++------------ 6 files changed, 55 insertions(+), 50 deletions(-) create mode 100644 prdoc/pr_2899.prdoc diff --git a/Cargo.lock b/Cargo.lock index 3ad73feb5d59..e00c173228f3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -16587,7 +16587,6 @@ dependencies = [ "clap 4.4.14", "fs4", "log", - "sc-client-db", "sp-core", "thiserror", "tokio", diff --git a/polkadot/cli/src/command.rs b/polkadot/cli/src/command.rs index 9290ec584e47..1e25f6533f04 100644 --- a/polkadot/cli/src/command.rs +++ b/polkadot/cli/src/command.rs @@ -256,11 +256,13 @@ where ) .map(|full| full.task_manager)?; - sc_storage_monitor::StorageMonitorService::try_spawn( - cli.storage_monitor, - database_source, - &task_manager.spawn_essential_handle(), - )?; + if let Some(path) = database_source.path() { + sc_storage_monitor::StorageMonitorService::try_spawn( + cli.storage_monitor, + path.to_path_buf(), + &task_manager.spawn_essential_handle(), + )?; + } Ok(task_manager) }) diff --git a/prdoc/pr_2899.prdoc b/prdoc/pr_2899.prdoc new file mode 100644 index 000000000000..0c7afc0ad088 --- /dev/null +++ b/prdoc/pr_2899.prdoc @@ -0,0 +1,10 @@ +title: Improve storage monitor API + +doc: + - audience: Node Dev + description: | + This removes the need to unnecessarily provide a very specific data structure DatabaseSource and removes huge + sc-client-db dependency from storage monitor. It is now possible to use storage monitor with any path. + +crates: + - name: sc-storage-monitor diff --git a/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs index 5e6e794f7328..67d21ee69ed1 100644 --- a/substrate/bin/node/cli/src/service.rs +++ b/substrate/bin/node/cli/src/service.rs @@ -38,7 +38,7 @@ use sc_transaction_pool_api::OffchainTransactionPoolFactory; use sp_api::ProvideRuntimeApi; use sp_core::crypto::Pair; use sp_runtime::{generic, traits::Block as BlockT, SaturatedConversion}; -use std::sync::Arc; +use std::{path::Path, sync::Arc}; /// Host functions required for kitchensink runtime and Substrate node. #[cfg(not(feature = "runtime-benchmarks"))] @@ -769,16 +769,18 @@ pub fn new_full_base( /// Builds a new service for a full client. pub fn new_full(config: Configuration, cli: Cli) -> Result { let mixnet_config = cli.mixnet_params.config(config.role.is_authority()); - let database_source = config.database.clone(); + let database_path = config.database.path().map(Path::to_path_buf); let task_manager = new_full_base(config, mixnet_config, cli.no_hardware_benchmarks, |_, _| ()) .map(|NewFullBase { task_manager, .. }| task_manager)?; - sc_storage_monitor::StorageMonitorService::try_spawn( - cli.storage_monitor, - database_source, - &task_manager.spawn_essential_handle(), - ) - .map_err(|e| ServiceError::Application(e.into()))?; + if let Some(database_path) = database_path { + sc_storage_monitor::StorageMonitorService::try_spawn( + cli.storage_monitor, + database_path, + &task_manager.spawn_essential_handle(), + ) + .map_err(|e| ServiceError::Application(e.into()))?; + } Ok(task_manager) } diff --git a/substrate/client/storage-monitor/Cargo.toml b/substrate/client/storage-monitor/Cargo.toml index 2b46ccbcdc15..7185c62e276e 100644 --- a/substrate/client/storage-monitor/Cargo.toml +++ b/substrate/client/storage-monitor/Cargo.toml @@ -15,7 +15,6 @@ workspace = true clap = { version = "4.4.14", features = ["derive", "string"] } log = "0.4.17" fs4 = "0.7.0" -sc-client-db = { path = "../db", default-features = false } sp-core = { path = "../../primitives/core" } -tokio = "1.22.0" +tokio = { version = "1.22.0", features = ["time"] } thiserror = "1.0.48" diff --git a/substrate/client/storage-monitor/src/lib.rs b/substrate/client/storage-monitor/src/lib.rs index b88b66d2d60d..28d9063e5e4b 100644 --- a/substrate/client/storage-monitor/src/lib.rs +++ b/substrate/client/storage-monitor/src/lib.rs @@ -17,7 +17,6 @@ // along with this program. If not, see . use clap::Args; -use sc_client_db::DatabaseSource; use sp_core::traits::SpawnEssentialNamed; use std::{ io, @@ -70,43 +69,37 @@ impl StorageMonitorService { /// Creates new StorageMonitorService for given client config pub fn try_spawn( parameters: StorageMonitorParams, - database: DatabaseSource, + path: PathBuf, spawner: &impl SpawnEssentialNamed, ) -> Result<()> { - Ok(match (parameters.threshold, database.path()) { - (0, _) => { - log::info!( - target: LOG_TARGET, - "StorageMonitorService: threshold `0` given, storage monitoring disabled", - ); - }, - (_, None) => { - log::warn!( - target: LOG_TARGET, - "StorageMonitorService: no database path to observe", - ); - }, - (threshold, Some(path)) => { - log::debug!( - target: LOG_TARGET, - "Initializing StorageMonitorService for db path: {path:?}", - ); - - Self::check_free_space(&path, threshold)?; + if parameters.threshold == 0 { + log::info!( + target: LOG_TARGET, + "StorageMonitorService: threshold `0` given, storage monitoring disabled", + ); + } else { + log::debug!( + target: LOG_TARGET, + "Initializing StorageMonitorService for db path: {}", + path.display() + ); + + Self::check_free_space(&path, parameters.threshold)?; + + let storage_monitor_service = StorageMonitorService { + path, + threshold: parameters.threshold, + polling_period: Duration::from_secs(parameters.polling_period.into()), + }; - let storage_monitor_service = StorageMonitorService { - path: path.to_path_buf(), - threshold, - polling_period: Duration::from_secs(parameters.polling_period.into()), - }; + spawner.spawn_essential( + "storage-monitor", + None, + Box::pin(storage_monitor_service.run()), + ); + } - spawner.spawn_essential( - "storage-monitor", - None, - Box::pin(storage_monitor_service.run()), - ); - }, - }) + Ok(()) } /// Main monitoring loop, intended to be spawned as essential task. Quits if free space drop