From d2d5146ed3057ef89219b1cc27f9e8e3f25b98b7 Mon Sep 17 00:00:00 2001 From: Nazar Mokrynskyi Date: Wed, 10 Jan 2024 04:02:03 +0200 Subject: [PATCH 1/4] Improve storage monitor API --- Cargo.lock | 1 - polkadot/cli/src/command.rs | 2 +- substrate/bin/node/cli/src/service.rs | 2 +- substrate/client/storage-monitor/Cargo.toml | 1 - substrate/client/storage-monitor/src/lib.rs | 10 +++++----- 5 files changed, 7 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c3e5e2ff41bc..68ff028fa4ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -16585,7 +16585,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..2e472b26aa80 100644 --- a/polkadot/cli/src/command.rs +++ b/polkadot/cli/src/command.rs @@ -258,7 +258,7 @@ where sc_storage_monitor::StorageMonitorService::try_spawn( cli.storage_monitor, - database_source, + database_source.path(), &task_manager.spawn_essential_handle(), )?; diff --git a/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs index 5e6e794f7328..9c4f19024115 100644 --- a/substrate/bin/node/cli/src/service.rs +++ b/substrate/bin/node/cli/src/service.rs @@ -775,7 +775,7 @@ pub fn new_full(config: Configuration, cli: Cli) -> Result. use clap::Args; -use sc_client_db::DatabaseSource; use sp_core::traits::SpawnEssentialNamed; use std::{ io, @@ -70,10 +69,10 @@ impl StorageMonitorService { /// Creates new StorageMonitorService for given client config pub fn try_spawn( parameters: StorageMonitorParams, - database: DatabaseSource, + path: Option<&Path>, spawner: &impl SpawnEssentialNamed, ) -> Result<()> { - Ok(match (parameters.threshold, database.path()) { + Ok(match (parameters.threshold, path) { (0, _) => { log::info!( target: LOG_TARGET, @@ -89,10 +88,11 @@ impl StorageMonitorService { (threshold, Some(path)) => { log::debug!( target: LOG_TARGET, - "Initializing StorageMonitorService for db path: {path:?}", + "Initializing StorageMonitorService for db path: {}", + path.display() ); - Self::check_free_space(&path, threshold)?; + Self::check_free_space(path, threshold)?; let storage_monitor_service = StorageMonitorService { path: path.to_path_buf(), From eeb67438583a17fba2dfafa4114593a528539778 Mon Sep 17 00:00:00 2001 From: Nazar Mokrynskyi Date: Wed, 10 Jan 2024 04:35:36 +0200 Subject: [PATCH 2/4] Add prdoc --- prdoc/pr_2899.prdoc | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 prdoc/pr_2899.prdoc 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 From f415b4d7d35883c4aadd481de3241570c9f3f779 Mon Sep 17 00:00:00 2001 From: Nazar Mokrynskyi Date: Wed, 10 Jan 2024 06:04:57 +0200 Subject: [PATCH 3/4] Fix missing tokio feature that was previously enabled implicitly --- substrate/client/storage-monitor/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/storage-monitor/Cargo.toml b/substrate/client/storage-monitor/Cargo.toml index c20cdd0067b2..7185c62e276e 100644 --- a/substrate/client/storage-monitor/Cargo.toml +++ b/substrate/client/storage-monitor/Cargo.toml @@ -16,5 +16,5 @@ clap = { version = "4.4.14", features = ["derive", "string"] } log = "0.4.17" fs4 = "0.7.0" sp-core = { path = "../../primitives/core" } -tokio = "1.22.0" +tokio = { version = "1.22.0", features = ["time"] } thiserror = "1.0.48" From efb0de29e424869f52f3ac34ed541dbf259eefd2 Mon Sep 17 00:00:00 2001 From: Nazar Mokrynskyi Date: Wed, 10 Jan 2024 13:09:38 +0200 Subject: [PATCH 4/4] Simplify storage monitor API to have non-optional path argument --- polkadot/cli/src/command.rs | 12 ++-- substrate/bin/node/cli/src/service.rs | 18 +++--- substrate/client/storage-monitor/src/lib.rs | 61 +++++++++------------ 3 files changed, 44 insertions(+), 47 deletions(-) diff --git a/polkadot/cli/src/command.rs b/polkadot/cli/src/command.rs index 2e472b26aa80..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.path(), - &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/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs index 9c4f19024115..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.path(), - &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/src/lib.rs b/substrate/client/storage-monitor/src/lib.rs index 1021090262ad..28d9063e5e4b 100644 --- a/substrate/client/storage-monitor/src/lib.rs +++ b/substrate/client/storage-monitor/src/lib.rs @@ -69,44 +69,37 @@ impl StorageMonitorService { /// Creates new StorageMonitorService for given client config pub fn try_spawn( parameters: StorageMonitorParams, - path: Option<&Path>, + path: PathBuf, spawner: &impl SpawnEssentialNamed, ) -> Result<()> { - Ok(match (parameters.threshold, 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.display() - ); - - 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