From 31be7a7d3bc2aa4aae0798360a92f2f0af2855a0 Mon Sep 17 00:00:00 2001 From: Falco Hirschenberger Date: Sat, 28 Aug 2021 20:29:24 +0200 Subject: [PATCH 1/5] Store the database in a role specific subdirectory This is a cleaned up version of #8658 fixing #6880 polkadot companion: paritytech/polkadot#2923 --- Cargo.lock | 1 + bin/node/cli/Cargo.toml | 1 + bin/node/cli/tests/check_block_works.rs | 2 +- bin/node/cli/tests/common.rs | 4 +- .../tests/database_role_subdir_migration.rs | 105 +++++++++++++++++ bin/node/cli/tests/export_import_flow.rs | 2 +- bin/node/cli/tests/inspect_works.rs | 2 +- bin/node/cli/tests/purge_chain_works.rs | 4 +- client/cli/src/config.rs | 11 +- client/db/src/lib.rs | 18 ++- client/db/src/upgrade.rs | 23 ++-- client/db/src/utils.rs | 109 +++++++++++++++++- 12 files changed, 258 insertions(+), 24 deletions(-) create mode 100644 bin/node/cli/tests/database_role_subdir_migration.rs diff --git a/Cargo.lock b/Cargo.lock index a613b7ba03686..a89ed4538ff9d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4336,6 +4336,7 @@ dependencies = [ "sc-chain-spec", "sc-cli", "sc-client-api", + "sc-client-db", "sc-consensus", "sc-consensus-babe", "sc-consensus-epochs", diff --git a/bin/node/cli/Cargo.toml b/bin/node/cli/Cargo.toml index 75ac03266cff9..acbf1b9888b86 100644 --- a/bin/node/cli/Cargo.toml +++ b/bin/node/cli/Cargo.toml @@ -109,6 +109,7 @@ sp-trie = { version = "4.0.0-dev", default-features = false, path = "../../../pr [dev-dependencies] sc-keystore = { version = "4.0.0-dev", path = "../../../client/keystore" } +sc-client-db = { version = "0.10.0-dev", path = "../../../client/db" } sc-consensus = { version = "0.10.0-dev", path = "../../../client/consensus/common" } sc-consensus-babe = { version = "0.10.0-dev", path = "../../../client/consensus/babe" } sc-consensus-epochs = { version = "0.10.0-dev", path = "../../../client/consensus/epochs" } diff --git a/bin/node/cli/tests/check_block_works.rs b/bin/node/cli/tests/check_block_works.rs index 39963fb002876..707fd217e33e8 100644 --- a/bin/node/cli/tests/check_block_works.rs +++ b/bin/node/cli/tests/check_block_works.rs @@ -28,7 +28,7 @@ pub mod common; fn check_block_works() { let base_path = tempdir().expect("could not create a temp dir"); - common::run_dev_node_for_a_while(base_path.path()); + common::run_node_for_a_while(base_path.path(), &["--dev"]); let status = Command::new(cargo_bin("substrate")) .args(&["check-block", "--dev", "--pruning", "archive", "-d"]) diff --git a/bin/node/cli/tests/common.rs b/bin/node/cli/tests/common.rs index 50776202d79eb..36822122b175e 100644 --- a/bin/node/cli/tests/common.rs +++ b/bin/node/cli/tests/common.rs @@ -54,10 +54,10 @@ pub fn wait_for(child: &mut Child, secs: usize) -> Option { } /// Run the node for a while (30 seconds) -pub fn run_dev_node_for_a_while(base_path: &Path) { +pub fn run_node_for_a_while(base_path: &Path, args: &[&str]) { let mut cmd = Command::new(cargo_bin("substrate")); - let mut cmd = cmd.args(&["--dev"]).arg("-d").arg(base_path).spawn().unwrap(); + let mut cmd = cmd.args(args).arg("-d").arg(base_path).spawn().unwrap(); // Let it produce some blocks. thread::sleep(Duration::from_secs(30)); diff --git a/bin/node/cli/tests/database_role_subdir_migration.rs b/bin/node/cli/tests/database_role_subdir_migration.rs new file mode 100644 index 0000000000000..881325784e1c9 --- /dev/null +++ b/bin/node/cli/tests/database_role_subdir_migration.rs @@ -0,0 +1,105 @@ +// This file is part of Substrate. + +// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +use sc_client_db::{ + light::LightStorage, DatabaseSettings, DatabaseSource, KeepBlocks, PruningMode, + TransactionStorageMode, +}; +use sp_runtime::testing::{Block as RawBlock, ExtrinsicWrapper}; +use std::fs; +use tempfile::tempdir; + +pub mod common; + +#[test] +#[cfg(unix)] +fn database_role_subdir_migration() { + type Block = RawBlock>; + + let base_path = tempdir().expect("could not create a temp dir"); + let dummy_path = base_path.path().join("dummy"); + // create a dummy database dir + { + let _old_db = LightStorage::::new(DatabaseSettings { + state_cache_size: 0, + state_cache_child_ratio: None, + state_pruning: PruningMode::ArchiveAll, + source: DatabaseSource::RocksDb { path: dummy_path.to_path_buf(), cache_size: 128 }, + keep_blocks: KeepBlocks::All, + transaction_storage: TransactionStorageMode::BlockBody, + }) + .unwrap(); + } + + // copy dummy to a directory resembling the old layout + let old_db_path = base_path.path().join("chains/dev/db"); + fs::create_dir_all(&old_db_path).unwrap(); + fs::rename(dummy_path.join("light"), &old_db_path).unwrap(); + + assert!(old_db_path.join("db_version").exists()); + assert!(!old_db_path.join("light").exists()); + + // start a light client + common::run_node_for_a_while( + base_path.path(), + &["--dev", "--light", "--rpc-port", "44444", "--ws-port", "44445"], + ); + + // check if the database dir had been migrated + assert!(old_db_path.join("light/db_version").exists()); +} + +#[test] +#[cfg(unix)] +fn database_role_subdir_migration_not_fail_on_different_role() { + type Block = RawBlock>; + + let base_path = tempdir().expect("could not create a temp dir"); + let dummy_path = base_path.path().join("dummy"); + + // create a database with the old layout + { + let _old_db = LightStorage::::new(DatabaseSettings { + state_cache_size: 0, + state_cache_child_ratio: None, + state_pruning: PruningMode::ArchiveAll, + source: DatabaseSource::RocksDb { path: dummy_path.to_path_buf(), cache_size: 128 }, + keep_blocks: KeepBlocks::All, + transaction_storage: TransactionStorageMode::BlockBody, + }) + .unwrap(); + } + + // copy dummy to a directory resembling the old layout + let old_db_path = base_path.path().join("chains/dev/db/light"); + fs::create_dir_all(&old_db_path).unwrap(); + fs::rename(dummy_path.join("light"), &old_db_path).unwrap(); + + assert!(old_db_path.join("db_version").exists()); + + // start a client with a different role (full), it should not fail but create a db with the + // full database next to the light database + common::run_node_for_a_while( + base_path.path(), + &["--dev", "--rpc-port", "44446", "--ws-port", "44447"], + ); + + // check if both database dirs coexist + assert!(base_path.path().join("chains/dev/db/light/db_version").exists()); + assert!(base_path.path().join("chains/dev/db/full/db_version").exists()); +} diff --git a/bin/node/cli/tests/export_import_flow.rs b/bin/node/cli/tests/export_import_flow.rs index 7bf64900b752a..7cbaa152699b4 100644 --- a/bin/node/cli/tests/export_import_flow.rs +++ b/bin/node/cli/tests/export_import_flow.rs @@ -188,7 +188,7 @@ fn export_import_revert() { let exported_blocks_file = base_path.path().join("exported_blocks"); let db_path = base_path.path().join("db"); - common::run_dev_node_for_a_while(base_path.path()); + common::run_node_for_a_while(base_path.path(), &["--dev"]); let mut executor = ExportImportRevertExecutor::new(&base_path, &exported_blocks_file, &db_path); diff --git a/bin/node/cli/tests/inspect_works.rs b/bin/node/cli/tests/inspect_works.rs index 67dbc97056cf6..2a89801547a4b 100644 --- a/bin/node/cli/tests/inspect_works.rs +++ b/bin/node/cli/tests/inspect_works.rs @@ -28,7 +28,7 @@ pub mod common; fn inspect_works() { let base_path = tempdir().expect("could not create a temp dir"); - common::run_dev_node_for_a_while(base_path.path()); + common::run_node_for_a_while(base_path.path(), &["--dev"]); let status = Command::new(cargo_bin("substrate")) .args(&["inspect", "--dev", "--pruning", "archive", "-d"]) diff --git a/bin/node/cli/tests/purge_chain_works.rs b/bin/node/cli/tests/purge_chain_works.rs index 4c0727d26cb17..0f16a51e5d0a4 100644 --- a/bin/node/cli/tests/purge_chain_works.rs +++ b/bin/node/cli/tests/purge_chain_works.rs @@ -27,7 +27,7 @@ pub mod common; fn purge_chain_works() { let base_path = tempdir().expect("could not create a temp dir"); - common::run_dev_node_for_a_while(base_path.path()); + common::run_node_for_a_while(base_path.path(), &["--dev"]); let status = Command::new(cargo_bin("substrate")) .args(&["purge-chain", "--dev", "-d"]) @@ -39,5 +39,5 @@ fn purge_chain_works() { // Make sure that the `dev` chain folder exists, but the `db` is deleted. assert!(base_path.path().join("chains/dev/").exists()); - assert!(!base_path.path().join("chains/dev/db").exists()); + assert!(!base_path.path().join("chains/dev/db/full").exists()); } diff --git a/client/cli/src/config.rs b/client/cli/src/config.rs index bfc7c6eb7bacc..36f267e4a3008 100644 --- a/client/cli/src/config.rs +++ b/client/cli/src/config.rs @@ -219,9 +219,14 @@ pub trait CliConfiguration: Sized { base_path: &PathBuf, cache_size: usize, database: Database, + role: &Role, ) -> Result { - let rocksdb_path = base_path.join("db"); - let paritydb_path = base_path.join("paritydb"); + let role_dir = match role { + Role::Light => "light", + Role::Full | Role::Authority => "full", + }; + let rocksdb_path = base_path.join("db").join(role_dir); + let paritydb_path = base_path.join("paritydb").join(role_dir); Ok(match database { Database::RocksDb => DatabaseSource::RocksDb { path: rocksdb_path, cache_size }, Database::ParityDb => DatabaseSource::ParityDb { path: rocksdb_path }, @@ -499,7 +504,7 @@ pub trait CliConfiguration: Sized { )?, keystore_remote, keystore, - database: self.database_config(&config_dir, database_cache_size, database)?, + database: self.database_config(&config_dir, database_cache_size, database, &role)?, state_cache_size: self.state_cache_size()?, state_cache_child_ratio: self.state_cache_child_ratio()?, state_pruning: self.state_pruning(unsafe_pruning, &role)?, diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index c7d6029c5356d..66adb64c0109e 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -355,7 +355,7 @@ pub enum DatabaseSource { } impl DatabaseSource { - /// Return dabase path for databases that are on the disk. + /// Return path for databases that are stored on disk. pub fn path(&self) -> Option<&Path> { match self { // as per https://github.com/paritytech/substrate/pull/9500#discussion_r684312550 @@ -367,6 +367,22 @@ impl DatabaseSource { DatabaseSource::Custom(..) => None, } } + + /// Set path for databases that are stored on disk. + pub fn set_path(&mut self, p: &Path) -> bool { + match self { + DatabaseSource::Auto { ref mut paritydb_path, .. } => { + *paritydb_path = p.into(); + true + }, + DatabaseSource::RocksDb { ref mut path, .. } | + DatabaseSource::ParityDb { ref mut path } => { + *path = p.into(); + true + }, + DatabaseSource::Custom(..) => false, + } + } } impl std::fmt::Display for DatabaseSource { diff --git a/client/db/src/upgrade.rs b/client/db/src/upgrade.rs index 5c9c2ccdc51d9..0f3578ad99a37 100644 --- a/client/db/src/upgrade.rs +++ b/client/db/src/upgrade.rs @@ -186,7 +186,7 @@ mod tests { } } - fn open_database(db_path: &Path) -> sp_blockchain::Result<()> { + fn open_database(db_path: &Path, db_type: DatabaseType) -> sp_blockchain::Result<()> { crate::utils::open_database::( &DatabaseSettings { state_cache_size: 0, @@ -196,7 +196,7 @@ mod tests { keep_blocks: KeepBlocks::All, transaction_storage: TransactionStorageMode::BlockBody, }, - DatabaseType::Full, + db_type, ) .map(|_| ()) } @@ -205,25 +205,28 @@ mod tests { fn downgrade_never_happens() { let db_dir = tempfile::TempDir::new().unwrap(); create_db(db_dir.path(), Some(CURRENT_VERSION + 1)); - assert!(open_database(db_dir.path()).is_err()); + assert!(open_database(db_dir.path(), DatabaseType::Full).is_err()); } #[test] fn open_empty_database_works() { + let db_type = DatabaseType::Full; let db_dir = tempfile::TempDir::new().unwrap(); - open_database(db_dir.path()).unwrap(); - open_database(db_dir.path()).unwrap(); - assert_eq!(current_version(db_dir.path()).unwrap(), CURRENT_VERSION); + let db_dir = db_dir.path().join(db_type.as_str()); + open_database(&db_dir, db_type).unwrap(); + open_database(&db_dir, db_type).unwrap(); + assert_eq!(current_version(&db_dir).unwrap(), CURRENT_VERSION); } #[test] fn upgrade_to_3_works() { + let db_type = DatabaseType::Full; for version_from_file in &[None, Some(1), Some(2)] { let db_dir = tempfile::TempDir::new().unwrap(); - let db_path = db_dir.path(); - create_db(db_path, *version_from_file); - open_database(db_path).unwrap(); - assert_eq!(current_version(db_path).unwrap(), CURRENT_VERSION); + let db_path = db_dir.path().join(db_type.as_str()); + create_db(&db_path, *version_from_file); + open_database(&db_path, db_type).unwrap(); + assert_eq!(current_version(&db_path).unwrap(), CURRENT_VERSION); } } } diff --git a/client/db/src/utils.rs b/client/db/src/utils.rs index 604a0e1328767..f63ef3601c000 100644 --- a/client/db/src/utils.rs +++ b/client/db/src/utils.rs @@ -19,9 +19,14 @@ //! Db-based backend utility structures and functions, used by both //! full and light storages. -use std::{convert::TryInto, fmt, io, path::Path, sync::Arc}; +use std::{ + convert::TryInto, + fmt, fs, io, + path::{Path, PathBuf}, + sync::Arc, +}; -use log::debug; +use log::{debug, info}; use crate::{Database, DatabaseSettings, DatabaseSource, DbHash}; use codec::Decode; @@ -213,7 +218,32 @@ pub fn open_database( config: &DatabaseSettings, db_type: DatabaseType, ) -> sp_blockchain::Result>> { - let db: Arc> = match &config.source { + // Try to open the database to check if the current `DatabaseType` matches the type of database + // stored in the target directory and close the database on success. + open_database_at::(&config.source, db_type).and_then(|db| { + drop(db); + let mut source = config.source.clone(); + if let Some(db_path) = config.source.path() { + // Maybe migrate (copy) the database to a type specific subdirectory to make it + // possible that light and full databases coexist + let new_db_path = maybe_migrate_to_type_subdir(&db_path, db_type).map_err(|e| { + sp_blockchain::Error::Backend(format!( + "Error in migration to role subdirectory {}", + e + )) + })?; + source.set_path(&new_db_path); + }; + + open_database_at::(&source, db_type) + }) +} + +fn open_database_at( + source: &DatabaseSource, + db_type: DatabaseType, +) -> sp_blockchain::Result>> { + let db: Arc> = match &source { DatabaseSource::ParityDb { path } => open_parity_db::(&path, db_type, true)?, DatabaseSource::RocksDb { path, cache_size } => open_kvdb_rocksdb::(&path, db_type, true, *cache_size)?, @@ -394,6 +424,33 @@ pub fn check_database_type( Ok(()) } +fn maybe_migrate_to_type_subdir(p: &Path, db_type: DatabaseType) -> io::Result { + let mut basedir = p.to_path_buf(); + basedir.pop(); + + // Do we have to migrate to a database-type-based subdirectory layout: + // See if the base dir exists (`db`) and there's no `light` or `full` directory inside + // of it, so we assume the database is not-migrated yet. + if basedir.exists() && !(basedir.join("light").exists() || basedir.join("full").exists()) { + info!( + "Migrating database to a database-type-based subdirectory: '{:?}' -> '{:?}'", + p, + p.join(db_type.as_str()) + ); + let mut p = p.to_path_buf(); + let mut tmp_dir = p.clone(); + tmp_dir.pop(); + tmp_dir.push("tmp"); + + fs::rename(&p, &tmp_dir)?; + p.push(db_type.as_str()); + fs::create_dir_all(&p)?; + fs::rename(tmp_dir, &p)?; + return Ok(p) + } + Ok(p.to_path_buf()) +} + /// Read database column entry for the given block. pub fn read_db( db: &dyn Database, @@ -572,6 +629,52 @@ mod tests { use sp_runtime::testing::{Block as RawBlock, ExtrinsicWrapper}; type Block = RawBlock>; + #[cfg(any(feature = "with-kvdb-rocksdb", test))] + #[test] + fn database_type_subdir_migration() { + type Block = RawBlock>; + + fn check_dir_for_db_type(dir: &str, db_type: DatabaseType) { + let base_path = tempfile::TempDir::new().unwrap(); + let old_db_path = base_path.path().join("chains/dev/db"); + + let source = DatabaseSource::RocksDb { path: old_db_path.clone(), cache_size: 128 }; + let settings = db_settings(source); + + let db_res = open_database::(&settings, db_type); + assert!(db_res.is_ok(), "New database should be created."); + + // check if the database dir had been migrated + assert!(!old_db_path.join("db_version").exists()); + assert!(old_db_path.join(dir).join("db_version").exists()); + } + + check_dir_for_db_type("light", DatabaseType::Light); + check_dir_for_db_type("full", DatabaseType::Full); + + // check failure on reopening with wrong role + { + let base_path = tempfile::TempDir::new().unwrap(); + let old_db_path = base_path.path().join("chains/dev/db"); + + let source = DatabaseSource::RocksDb { path: old_db_path.clone(), cache_size: 128 }; + let settings = db_settings(source); + { + let db_res = open_database::(&settings, DatabaseType::Light); + assert!(db_res.is_ok(), "New database should be created."); + + // check if the database dir had been migrated + assert!(!old_db_path.join("db_version").exists()); + assert!(old_db_path.join("light/db_version").exists()); + } + let source = + DatabaseSource::RocksDb { path: old_db_path.join("light"), cache_size: 128 }; + let settings = db_settings(source); + let db_res = open_database::(&settings, DatabaseType::Full); + assert!(db_res.is_err(), "Opening a light database in full role should fail"); + } + } + #[test] fn number_index_key_doesnt_panic() { let id = BlockId::::Number(72340207214430721); From 641d398d3efc926302a6566d50e65d895faf3f52 Mon Sep 17 00:00:00 2001 From: Falco Hirschenberger Date: Sat, 28 Aug 2021 21:05:41 +0200 Subject: [PATCH 2/5] Disable prometheus in tests --- bin/node/cli/tests/database_role_subdir_migration.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/node/cli/tests/database_role_subdir_migration.rs b/bin/node/cli/tests/database_role_subdir_migration.rs index 881325784e1c9..568e6b512e9a7 100644 --- a/bin/node/cli/tests/database_role_subdir_migration.rs +++ b/bin/node/cli/tests/database_role_subdir_migration.rs @@ -57,7 +57,7 @@ fn database_role_subdir_migration() { // start a light client common::run_node_for_a_while( base_path.path(), - &["--dev", "--light", "--rpc-port", "44444", "--ws-port", "44445"], + &["--dev", "--light", "--rpc-port", "44444", "--ws-port", "44445", "--no-prometheus"], ); // check if the database dir had been migrated @@ -96,7 +96,7 @@ fn database_role_subdir_migration_not_fail_on_different_role() { // full database next to the light database common::run_node_for_a_while( base_path.path(), - &["--dev", "--rpc-port", "44446", "--ws-port", "44447"], + &["--dev", "--rpc-port", "44446", "--ws-port", "44447", "--no-prometheus"], ); // check if both database dirs coexist From f4abbada4b46212a72bb4de33e6ac24d41b0467b Mon Sep 17 00:00:00 2001 From: Falco Hirschenberger Date: Sat, 28 Aug 2021 21:34:22 +0200 Subject: [PATCH 3/5] Also change p2p port --- .../tests/database_role_subdir_migration.rs | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/bin/node/cli/tests/database_role_subdir_migration.rs b/bin/node/cli/tests/database_role_subdir_migration.rs index 568e6b512e9a7..1bbdb4a223928 100644 --- a/bin/node/cli/tests/database_role_subdir_migration.rs +++ b/bin/node/cli/tests/database_role_subdir_migration.rs @@ -57,7 +57,17 @@ fn database_role_subdir_migration() { // start a light client common::run_node_for_a_while( base_path.path(), - &["--dev", "--light", "--rpc-port", "44444", "--ws-port", "44445", "--no-prometheus"], + &[ + "--dev", + "--light", + "--port", + "30335", + "--rpc-port", + "44444", + "--ws-port", + "44445", + "--no-prometheus", + ], ); // check if the database dir had been migrated @@ -96,7 +106,16 @@ fn database_role_subdir_migration_not_fail_on_different_role() { // full database next to the light database common::run_node_for_a_while( base_path.path(), - &["--dev", "--rpc-port", "44446", "--ws-port", "44447", "--no-prometheus"], + &[ + "--dev", + "--port", + "30334", + "--rpc-port", + "44446", + "--ws-port", + "44447", + "--no-prometheus", + ], ); // check if both database dirs coexist From e9f497140271493b20a7f006f40df320e8903955 Mon Sep 17 00:00:00 2001 From: Falco Hirschenberger Date: Sun, 5 Sep 2021 19:58:40 +0200 Subject: [PATCH 4/5] Fix migration logic --- bin/node/cli/tests/common.rs | 11 ++ .../tests/database_role_subdir_migration.rs | 45 +++--- client/db/src/utils.rs | 135 ++++++++++-------- 3 files changed, 104 insertions(+), 87 deletions(-) diff --git a/bin/node/cli/tests/common.rs b/bin/node/cli/tests/common.rs index 36822122b175e..54b9c749bf1de 100644 --- a/bin/node/cli/tests/common.rs +++ b/bin/node/cli/tests/common.rs @@ -67,3 +67,14 @@ pub fn run_node_for_a_while(base_path: &Path, args: &[&str]) { kill(Pid::from_raw(cmd.id().try_into().unwrap()), SIGINT).unwrap(); assert!(wait_for(&mut cmd, 40).map(|x| x.success()).unwrap_or_default()); } + +/// Run the node asserting that it fails with an error +pub fn run_node_assert_fail(base_path: &Path, args: &[&str]) { + let mut cmd = Command::new(cargo_bin("substrate")); + + let mut cmd = cmd.args(args).arg("-d").arg(base_path).spawn().unwrap(); + + // Let it produce some blocks. + thread::sleep(Duration::from_secs(10)); + assert!(cmd.try_wait().unwrap().is_some(), "the process should not be running anymore"); +} diff --git a/bin/node/cli/tests/database_role_subdir_migration.rs b/bin/node/cli/tests/database_role_subdir_migration.rs index 1bbdb4a223928..516908111ae72 100644 --- a/bin/node/cli/tests/database_role_subdir_migration.rs +++ b/bin/node/cli/tests/database_role_subdir_migration.rs @@ -21,7 +21,6 @@ use sc_client_db::{ TransactionStorageMode, }; use sp_runtime::testing::{Block as RawBlock, ExtrinsicWrapper}; -use std::fs; use tempfile::tempdir; pub mod common; @@ -32,27 +31,22 @@ fn database_role_subdir_migration() { type Block = RawBlock>; let base_path = tempdir().expect("could not create a temp dir"); - let dummy_path = base_path.path().join("dummy"); + let path = base_path.path().join("chains/dev/db"); // create a dummy database dir { let _old_db = LightStorage::::new(DatabaseSettings { state_cache_size: 0, state_cache_child_ratio: None, state_pruning: PruningMode::ArchiveAll, - source: DatabaseSource::RocksDb { path: dummy_path.to_path_buf(), cache_size: 128 }, + source: DatabaseSource::RocksDb { path: path.to_path_buf(), cache_size: 128 }, keep_blocks: KeepBlocks::All, transaction_storage: TransactionStorageMode::BlockBody, }) .unwrap(); } - // copy dummy to a directory resembling the old layout - let old_db_path = base_path.path().join("chains/dev/db"); - fs::create_dir_all(&old_db_path).unwrap(); - fs::rename(dummy_path.join("light"), &old_db_path).unwrap(); - - assert!(old_db_path.join("db_version").exists()); - assert!(!old_db_path.join("light").exists()); + assert!(path.join("db_version").exists()); + assert!(!path.join("light").exists()); // start a light client common::run_node_for_a_while( @@ -71,16 +65,17 @@ fn database_role_subdir_migration() { ); // check if the database dir had been migrated - assert!(old_db_path.join("light/db_version").exists()); + assert!(!path.join("db_version").exists()); + assert!(path.join("light/db_version").exists()); } #[test] #[cfg(unix)] -fn database_role_subdir_migration_not_fail_on_different_role() { +fn database_role_subdir_migration_fail_on_different_role() { type Block = RawBlock>; let base_path = tempdir().expect("could not create a temp dir"); - let dummy_path = base_path.path().join("dummy"); + let path = base_path.path().join("chains/dev/db"); // create a database with the old layout { @@ -88,24 +83,19 @@ fn database_role_subdir_migration_not_fail_on_different_role() { state_cache_size: 0, state_cache_child_ratio: None, state_pruning: PruningMode::ArchiveAll, - source: DatabaseSource::RocksDb { path: dummy_path.to_path_buf(), cache_size: 128 }, + source: DatabaseSource::RocksDb { path: path.to_path_buf(), cache_size: 128 }, keep_blocks: KeepBlocks::All, transaction_storage: TransactionStorageMode::BlockBody, }) .unwrap(); } - // copy dummy to a directory resembling the old layout - let old_db_path = base_path.path().join("chains/dev/db/light"); - fs::create_dir_all(&old_db_path).unwrap(); - fs::rename(dummy_path.join("light"), &old_db_path).unwrap(); - - assert!(old_db_path.join("db_version").exists()); + assert!(path.join("db_version").exists()); + assert!(!path.join("light/db_version").exists()); - // start a client with a different role (full), it should not fail but create a db with the - // full database next to the light database - common::run_node_for_a_while( - base_path.path(), + // start a client with a different role (full), it should fail and not change any files on disk + common::run_node_assert_fail( + &base_path.path(), &[ "--dev", "--port", @@ -118,7 +108,8 @@ fn database_role_subdir_migration_not_fail_on_different_role() { ], ); - // check if both database dirs coexist - assert!(base_path.path().join("chains/dev/db/light/db_version").exists()); - assert!(base_path.path().join("chains/dev/db/full/db_version").exists()); + // check if the files are unchanged + assert!(path.join("db_version").exists()); + assert!(!path.join("light/db_version").exists()); + assert!(!path.join("full/db_version").exists()); } diff --git a/client/db/src/utils.rs b/client/db/src/utils.rs index f63ef3601c000..ce77005c9e602 100644 --- a/client/db/src/utils.rs +++ b/client/db/src/utils.rs @@ -19,12 +19,7 @@ //! Db-based backend utility structures and functions, used by both //! full and light storages. -use std::{ - convert::TryInto, - fmt, fs, io, - path::{Path, PathBuf}, - sync::Arc, -}; +use std::{convert::TryInto, fmt, fs, io, path::Path, sync::Arc}; use log::{debug, info}; @@ -218,25 +213,14 @@ pub fn open_database( config: &DatabaseSettings, db_type: DatabaseType, ) -> sp_blockchain::Result>> { - // Try to open the database to check if the current `DatabaseType` matches the type of database - // stored in the target directory and close the database on success. - open_database_at::(&config.source, db_type).and_then(|db| { - drop(db); - let mut source = config.source.clone(); - if let Some(db_path) = config.source.path() { - // Maybe migrate (copy) the database to a type specific subdirectory to make it - // possible that light and full databases coexist - let new_db_path = maybe_migrate_to_type_subdir(&db_path, db_type).map_err(|e| { - sp_blockchain::Error::Backend(format!( - "Error in migration to role subdirectory {}", - e - )) - })?; - source.set_path(&new_db_path); - }; + // Maybe migrate (copy) the database to a type specific subdirectory to make it + // possible that light and full databases coexist + // NOTE: This function can be removed in a few releases + maybe_migrate_to_type_subdir::(&config.source, db_type).map_err(|e| { + sp_blockchain::Error::Backend(format!("Error in migration to role subdirectory: {}", e)) + })?; - open_database_at::(&source, db_type) - }) + open_database_at::(&config.source, db_type) } fn open_database_at( @@ -424,31 +408,44 @@ pub fn check_database_type( Ok(()) } -fn maybe_migrate_to_type_subdir(p: &Path, db_type: DatabaseType) -> io::Result { - let mut basedir = p.to_path_buf(); - basedir.pop(); - - // Do we have to migrate to a database-type-based subdirectory layout: - // See if the base dir exists (`db`) and there's no `light` or `full` directory inside - // of it, so we assume the database is not-migrated yet. - if basedir.exists() && !(basedir.join("light").exists() || basedir.join("full").exists()) { - info!( - "Migrating database to a database-type-based subdirectory: '{:?}' -> '{:?}'", - p, - p.join(db_type.as_str()) - ); - let mut p = p.to_path_buf(); - let mut tmp_dir = p.clone(); - tmp_dir.pop(); - tmp_dir.push("tmp"); - - fs::rename(&p, &tmp_dir)?; - p.push(db_type.as_str()); - fs::create_dir_all(&p)?; - fs::rename(tmp_dir, &p)?; - return Ok(p) +fn maybe_migrate_to_type_subdir( + source: &DatabaseSource, + db_type: DatabaseType, +) -> io::Result<()> { + if let Some(p) = source.path() { + let mut basedir = p.to_path_buf(); + basedir.pop(); + + // Do we have to migrate to a database-type-based subdirectory layout: + // See if there's a file `db_version` in the parent dir and the target path ends in a role + // specific directory + if basedir.join("db_version").exists() && + (p.ends_with(DatabaseType::Full.as_str()) || + p.ends_with(DatabaseType::Light.as_str())) + { + // Try to open the database to check if the current `DatabaseType` matches the type of + // database stored in the target directory and close the database on success. + let mut old_source = source.clone(); + old_source.set_path(&basedir); + open_database_at::(&old_source, db_type) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; + + info!( + "Migrating database to a database-type-based subdirectory: '{:?}' -> '{:?}'", + basedir, + basedir.join(db_type.as_str()) + ); + let mut tmp_dir = basedir.clone(); + tmp_dir.pop(); + tmp_dir.push("tmp"); + + fs::rename(&basedir, &tmp_dir)?; + fs::create_dir_all(&p)?; + fs::rename(tmp_dir, &p)?; + } } - Ok(p.to_path_buf()) + + Ok(()) } /// Read database column entry for the given block. @@ -634,23 +631,34 @@ mod tests { fn database_type_subdir_migration() { type Block = RawBlock>; - fn check_dir_for_db_type(dir: &str, db_type: DatabaseType) { + fn check_dir_for_db_type(db_type: DatabaseType) { let base_path = tempfile::TempDir::new().unwrap(); let old_db_path = base_path.path().join("chains/dev/db"); let source = DatabaseSource::RocksDb { path: old_db_path.clone(), cache_size: 128 }; let settings = db_settings(source); - let db_res = open_database::(&settings, db_type); - assert!(db_res.is_ok(), "New database should be created."); + { + let db_res = open_database::(&settings, db_type); + assert!(db_res.is_ok(), "New database should be created."); + assert!(old_db_path.join("db_version").exists()); + assert!(!old_db_path.join(db_type.as_str()).join("db_version").exists()); + } + let source = DatabaseSource::RocksDb { + path: old_db_path.join(db_type.as_str()), + cache_size: 128, + }; + let settings = db_settings(source); + let db_res = open_database::(&settings, db_type); + assert!(db_res.is_ok(), "Reopening the db with the same role should work"); // check if the database dir had been migrated assert!(!old_db_path.join("db_version").exists()); - assert!(old_db_path.join(dir).join("db_version").exists()); + assert!(old_db_path.join(db_type.as_str()).join("db_version").exists()); } - check_dir_for_db_type("light", DatabaseType::Light); - check_dir_for_db_type("full", DatabaseType::Full); + check_dir_for_db_type(DatabaseType::Light); + check_dir_for_db_type(DatabaseType::Full); // check failure on reopening with wrong role { @@ -660,18 +668,25 @@ mod tests { let source = DatabaseSource::RocksDb { path: old_db_path.clone(), cache_size: 128 }; let settings = db_settings(source); { - let db_res = open_database::(&settings, DatabaseType::Light); + let db_res = open_database::(&settings, DatabaseType::Full); assert!(db_res.is_ok(), "New database should be created."); // check if the database dir had been migrated - assert!(!old_db_path.join("db_version").exists()); - assert!(old_db_path.join("light/db_version").exists()); + assert!(old_db_path.join("db_version").exists()); + assert!(!old_db_path.join("light/db_version").exists()); + assert!(!old_db_path.join("full/db_version").exists()); } - let source = - DatabaseSource::RocksDb { path: old_db_path.join("light"), cache_size: 128 }; + let source = DatabaseSource::RocksDb { + path: old_db_path.join(DatabaseType::Light.as_str()), + cache_size: 128, + }; let settings = db_settings(source); - let db_res = open_database::(&settings, DatabaseType::Full); + let db_res = open_database::(&settings, DatabaseType::Light); assert!(db_res.is_err(), "Opening a light database in full role should fail"); + // assert nothing was changed + assert!(old_db_path.join("db_version").exists()); + assert!(!old_db_path.join("light/db_version").exists()); + assert!(!old_db_path.join("full/db_version").exists()); } } From 9166560a3abd24d13f6f43fdcaf9646d81c15c44 Mon Sep 17 00:00:00 2001 From: Falco Hirschenberger Date: Mon, 6 Sep 2021 17:29:10 +0200 Subject: [PATCH 5/5] Use different identification file for rocks and parity db Add tests for paritydb migration --- client/db/src/utils.rs | 53 ++++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/client/db/src/utils.rs b/client/db/src/utils.rs index ce77005c9e602..ea22c774f463e 100644 --- a/client/db/src/utils.rs +++ b/client/db/src/utils.rs @@ -417,9 +417,9 @@ fn maybe_migrate_to_type_subdir( basedir.pop(); // Do we have to migrate to a database-type-based subdirectory layout: - // See if there's a file `db_version` in the parent dir and the target path ends in a role - // specific directory - if basedir.join("db_version").exists() && + // See if there's a file identifying a rocksdb or paritydb folder in the parent dir and + // the target path ends in a role specific directory + if (basedir.join("db_version").exists() || basedir.join("metadata").exists()) && (p.ends_with(DatabaseType::Full.as_str()) || p.ends_with(DatabaseType::Light.as_str())) { @@ -624,6 +624,7 @@ mod tests { use codec::Input; use sc_state_db::PruningMode; use sp_runtime::testing::{Block as RawBlock, ExtrinsicWrapper}; + use std::path::PathBuf; type Block = RawBlock>; #[cfg(any(feature = "with-kvdb-rocksdb", test))] @@ -631,34 +632,56 @@ mod tests { fn database_type_subdir_migration() { type Block = RawBlock>; - fn check_dir_for_db_type(db_type: DatabaseType) { + fn check_dir_for_db_type( + db_type: DatabaseType, + mut source: DatabaseSource, + db_check_file: &str, + ) { let base_path = tempfile::TempDir::new().unwrap(); let old_db_path = base_path.path().join("chains/dev/db"); - let source = DatabaseSource::RocksDb { path: old_db_path.clone(), cache_size: 128 }; - let settings = db_settings(source); + source.set_path(&old_db_path); + let settings = db_settings(source.clone()); { let db_res = open_database::(&settings, db_type); assert!(db_res.is_ok(), "New database should be created."); - assert!(old_db_path.join("db_version").exists()); + assert!(old_db_path.join(db_check_file).exists()); assert!(!old_db_path.join(db_type.as_str()).join("db_version").exists()); } - let source = DatabaseSource::RocksDb { - path: old_db_path.join(db_type.as_str()), - cache_size: 128, - }; + source.set_path(&old_db_path.join(db_type.as_str())); let settings = db_settings(source); let db_res = open_database::(&settings, db_type); assert!(db_res.is_ok(), "Reopening the db with the same role should work"); // check if the database dir had been migrated - assert!(!old_db_path.join("db_version").exists()); - assert!(old_db_path.join(db_type.as_str()).join("db_version").exists()); + assert!(!old_db_path.join(db_check_file).exists()); + assert!(old_db_path.join(db_type.as_str()).join(db_check_file).exists()); } - check_dir_for_db_type(DatabaseType::Light); - check_dir_for_db_type(DatabaseType::Full); + check_dir_for_db_type( + DatabaseType::Light, + DatabaseSource::RocksDb { path: PathBuf::new(), cache_size: 128 }, + "db_version", + ); + check_dir_for_db_type( + DatabaseType::Full, + DatabaseSource::RocksDb { path: PathBuf::new(), cache_size: 128 }, + "db_version", + ); + + #[cfg(feature = "with-parity-db")] + check_dir_for_db_type( + DatabaseType::Light, + DatabaseSource::ParityDb { path: PathBuf::new() }, + "metadata", + ); + #[cfg(feature = "with-parity-db")] + check_dir_for_db_type( + DatabaseType::Full, + DatabaseSource::ParityDb { path: PathBuf::new() }, + "metadata", + ); // check failure on reopening with wrong role {