From fe3c538064253b8afd41d6b48520bfd534e99c8b Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Thu, 19 Jan 2023 15:55:05 -0800 Subject: [PATCH 01/11] Add run parent directory for accounts files --- core/src/snapshot_packager_service.rs | 5 ++- core/tests/epoch_accounts_hash.rs | 5 +-- core/tests/snapshots.rs | 21 +++++------ ledger-tool/src/main.rs | 20 +++++++++-- local-cluster/src/local_cluster.rs | 4 ++- local-cluster/tests/common.rs | 6 ++-- local-cluster/tests/local_cluster.rs | 6 ++-- runtime/src/accounts_db.rs | 6 +++- runtime/src/hardened_unpack.rs | 15 +++++++- runtime/src/serde_snapshot/tests.rs | 8 +++-- runtime/src/snapshot_utils.rs | 51 +++++++++++++++++++-------- test-validator/src/lib.rs | 7 ++-- validator/src/main.rs | 22 +++++++++--- 13 files changed, 130 insertions(+), 46 deletions(-) diff --git a/core/src/snapshot_packager_service.rs b/core/src/snapshot_packager_service.rs index 2c86efa35a6d08..d225a1e26870cc 100644 --- a/core/src/snapshot_packager_service.rs +++ b/core/src/snapshot_packager_service.rs @@ -228,7 +228,8 @@ mod tests { snapshot_hash::SnapshotHash, snapshot_package::{SnapshotPackage, SnapshotType}, snapshot_utils::{ - self, ArchiveFormat, SnapshotVersion, SNAPSHOT_STATUS_CACHE_FILENAME, + self, setup_accounts_run_and_snapshot_paths, ArchiveFormat, SnapshotVersion, + SNAPSHOT_STATUS_CACHE_FILENAME, }, }, solana_sdk::hash::Hash, @@ -267,6 +268,8 @@ mod tests { fn create_and_verify_snapshot(temp_dir: &Path) { let accounts_dir = temp_dir.join("accounts"); + let accounts_dir = setup_accounts_run_and_snapshot_paths(accounts_dir.as_path()).unwrap(); + let snapshots_dir = temp_dir.join("snapshots"); let full_snapshot_archives_dir = temp_dir.join("full_snapshot_archives"); let incremental_snapshot_archives_dir = temp_dir.join("incremental_snapshot_archives"); diff --git a/core/tests/epoch_accounts_hash.rs b/core/tests/epoch_accounts_hash.rs index b53dd404b5dec6..e54168e4486bf6 100755 --- a/core/tests/epoch_accounts_hash.rs +++ b/core/tests/epoch_accounts_hash.rs @@ -1,5 +1,6 @@ #![allow(clippy::integer_arithmetic)] use { + crate::snapshot_utils::generate_test_tmp_account_path, log::*, solana_core::{ accounts_hash_verifier::AccountsHashVerifier, @@ -442,9 +443,9 @@ fn test_snapshots_have_expected_epoch_accounts_hash() { std::thread::sleep(Duration::from_secs(1)); }; - let accounts_dir = TempDir::new().unwrap(); + let accounts_dir = generate_test_tmp_account_path(); let deserialized_bank = snapshot_utils::bank_from_snapshot_archives( - &[accounts_dir.path().to_path_buf()], + &[accounts_dir.as_path().to_path_buf()], &snapshot_config.bank_snapshots_dir, &full_snapshot_archive_info, None, diff --git a/core/tests/snapshots.rs b/core/tests/snapshots.rs index d5cf8cf123e909..e3d7ff80ae3d80 100644 --- a/core/tests/snapshots.rs +++ b/core/tests/snapshots.rs @@ -1,6 +1,7 @@ #![allow(clippy::integer_arithmetic)] use { + crate::snapshot_utils::generate_test_tmp_account_path, bincode::serialize_into, crossbeam_channel::unbounded, fs_extra::dir::CopyOptions, @@ -74,7 +75,7 @@ struct SnapshotTestConfig { incremental_snapshot_archives_dir: TempDir, full_snapshot_archives_dir: TempDir, bank_snapshots_dir: TempDir, - accounts_dir: TempDir, + accounts_dir: PathBuf, } impl SnapshotTestConfig { @@ -85,7 +86,7 @@ impl SnapshotTestConfig { full_snapshot_archive_interval_slots: Slot, incremental_snapshot_archive_interval_slots: Slot, ) -> SnapshotTestConfig { - let accounts_dir = TempDir::new().unwrap(); + let accounts_dir = generate_test_tmp_account_path(); let bank_snapshots_dir = TempDir::new().unwrap(); let full_snapshot_archives_dir = TempDir::new().unwrap(); let incremental_snapshot_archives_dir = TempDir::new().unwrap(); @@ -102,7 +103,7 @@ impl SnapshotTestConfig { let bank0 = Bank::new_with_paths_for_tests( &genesis_config_info.genesis_config, Arc::::default(), - vec![accounts_dir.path().to_path_buf()], + vec![accounts_dir.clone()], AccountSecondaryIndexes::default(), accounts_db::AccountShrinkThreshold::default(), ); @@ -296,8 +297,8 @@ fn run_bank_forks_snapshot_n( .unwrap(); // Restore bank from snapshot - let temporary_accounts_dir = TempDir::new().unwrap(); - let account_paths = &[temporary_accounts_dir.path().to_path_buf()]; + let temporary_accounts_dir = generate_test_tmp_account_path(); + let account_paths = &[temporary_accounts_dir]; let genesis_config = &snapshot_test_config.genesis_config_info.genesis_config; restore_from_snapshot(bank_forks, last_slot, genesis_config, account_paths); @@ -726,7 +727,7 @@ fn test_bank_forks_incremental_snapshot( INCREMENTAL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS, ); trace!("SnapshotTestConfig:\naccounts_dir: {}\nbank_snapshots_dir: {}\nfull_snapshot_archives_dir: {}\nincremental_snapshot_archives_dir: {}", - snapshot_test_config.accounts_dir.path().display(), snapshot_test_config.bank_snapshots_dir.path().display(), snapshot_test_config.full_snapshot_archives_dir.path().display(), snapshot_test_config.incremental_snapshot_archives_dir.path().display()); + snapshot_test_config.accounts_dir.display(), snapshot_test_config.bank_snapshots_dir.path().display(), snapshot_test_config.full_snapshot_archives_dir.path().display(), snapshot_test_config.incremental_snapshot_archives_dir.path().display()); let bank_forks = &mut snapshot_test_config.bank_forks; let mint_keypair = &snapshot_test_config.genesis_config_info.mint_keypair; @@ -821,11 +822,11 @@ fn test_bank_forks_incremental_snapshot( // Accounts directory needs to be separate from the active accounts directory // so that dropping append vecs in the active accounts directory doesn't // delete the unpacked appendvecs in the snapshot - let temporary_accounts_dir = TempDir::new().unwrap(); + let temporary_accounts_dir = generate_test_tmp_account_path(); restore_from_snapshots_and_check_banks_are_equal( &bank, &snapshot_test_config.snapshot_config, - temporary_accounts_dir.path().to_path_buf(), + temporary_accounts_dir.as_path().to_path_buf(), &snapshot_test_config.genesis_config_info.genesis_config, ) .unwrap(); @@ -1120,7 +1121,7 @@ fn test_snapshots_with_background_services( } // Load the snapshot and ensure it matches what's in BankForks - let temporary_accounts_dir = TempDir::new().unwrap(); + let temporary_accounts_dir = generate_test_tmp_account_path(); let (deserialized_bank, ..) = snapshot_utils::bank_from_latest_snapshot_archives( &snapshot_test_config.snapshot_config.bank_snapshots_dir, &snapshot_test_config @@ -1129,7 +1130,7 @@ fn test_snapshots_with_background_services( &snapshot_test_config .snapshot_config .incremental_snapshot_archives_dir, - &[temporary_accounts_dir.as_ref().to_path_buf()], + &[temporary_accounts_dir], &snapshot_test_config.genesis_config_info.genesis_config, &RuntimeConfig::default(), None, diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 84e469f8437f2b..b23afce13af66b 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -62,8 +62,8 @@ use { snapshot_hash::StartingSnapshotHashes, snapshot_minimizer::SnapshotMinimizer, snapshot_utils::{ - self, move_and_async_delete_path, ArchiveFormat, SnapshotVersion, - DEFAULT_ARCHIVE_COMPRESSION, SUPPORTED_ARCHIVE_COMPRESSION, + self, move_and_async_delete_path, setup_accounts_run_and_snapshot_paths, ArchiveFormat, + SnapshotVersion, DEFAULT_ARCHIVE_COMPRESSION, SUPPORTED_ARCHIVE_COMPRESSION, }, }, solana_sdk::{ @@ -1092,6 +1092,22 @@ fn load_bank_forks( ); vec![non_primary_accounts_path] }; + + // For all account_paths, set up the run/ and snapshot/ sub directories. + let account_run_paths: Vec = account_paths.into_iter().map( + |account_path| { + match setup_accounts_run_and_snapshot_paths(&account_path) { + Ok(account_run_path) => account_run_path, + Err(err) => { + eprintln!("Unable to set up account run and snapshot sub directories: {account_path:?}, err: {err:?}"); + exit(1); + } + } + }).collect(); + + // From now on, use run/ paths in the same way as the previous account_paths. + let account_paths = account_run_paths; + info!("Cleaning contents of account paths: {:?}", account_paths); let mut measure = Measure::start("clean_accounts_paths"); account_paths.iter().for_each(|path| { diff --git a/local-cluster/src/local_cluster.rs b/local-cluster/src/local_cluster.rs index cedf81f48c0d4f..c26d3342be5bcc 100644 --- a/local-cluster/src/local_cluster.rs +++ b/local-cluster/src/local_cluster.rs @@ -22,6 +22,7 @@ use { ValidatorVoteKeypairs, }, snapshot_config::SnapshotConfig, + snapshot_utils::setup_accounts_run_and_snapshot_paths, }, solana_sdk::{ account::{Account, AccountSharedData}, @@ -147,7 +148,8 @@ impl LocalCluster { config: &mut ValidatorConfig, ledger_path: &Path, ) { - config.account_paths = vec![ledger_path.join("accounts")]; + config.account_paths = + vec![setup_accounts_run_and_snapshot_paths(ledger_path.join("accounts")).unwrap()]; config.tower_storage = Arc::new(FileTowerStorage::new(ledger_path.to_path_buf())); let snapshot_config = &mut config.snapshot_config; diff --git a/local-cluster/tests/common.rs b/local-cluster/tests/common.rs index ba594af9f5fde5..7917fc1879a93c 100644 --- a/local-cluster/tests/common.rs +++ b/local-cluster/tests/common.rs @@ -21,7 +21,9 @@ use { validator_configs::*, }, solana_rpc_client::rpc_client::RpcClient, - solana_runtime::snapshot_config::SnapshotConfig, + solana_runtime::{ + snapshot_config::SnapshotConfig, snapshot_utils::setup_accounts_run_and_snapshot_paths, + }, solana_sdk::{ account::AccountSharedData, clock::{self, Slot, DEFAULT_MS_PER_SLOT, DEFAULT_TICKS_PER_SLOT}, @@ -436,7 +438,7 @@ pub fn generate_account_paths(num_account_paths: usize) -> (Vec, Vec = account_storage_dirs .iter() - .map(|a| a.path().to_path_buf()) + .map(|a| setup_accounts_run_and_snapshot_paths(a.path()).unwrap()) .collect(); (account_storage_dirs, account_storage_paths) } diff --git a/local-cluster/tests/local_cluster.rs b/local-cluster/tests/local_cluster.rs index c085a9566255bc..666bffa3e00fff 100644 --- a/local-cluster/tests/local_cluster.rs +++ b/local-cluster/tests/local_cluster.rs @@ -38,7 +38,9 @@ use { snapshot_archive_info::SnapshotArchiveInfoGetter, snapshot_config::SnapshotConfig, snapshot_package::SnapshotType, - snapshot_utils::{self, ArchiveFormat, SnapshotVersion}, + snapshot_utils::{ + self, setup_accounts_run_and_snapshot_paths, ArchiveFormat, SnapshotVersion, + }, }, solana_sdk::{ account::AccountSharedData, @@ -2152,7 +2154,7 @@ fn create_snapshot_to_hard_fork( let (bank_forks, ..) = bank_forks_utils::load( &genesis_config, blockstore, - vec![ledger_path.join("accounts")], + vec![setup_accounts_run_and_snapshot_paths(ledger_path.join("accounts")).unwrap()], None, snapshot_config.as_ref(), process_options, diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 637511370b291f..e0ccb0641c03b3 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -53,6 +53,7 @@ use { read_only_accounts_cache::ReadOnlyAccountsCache, rent_collector::RentCollector, rent_paying_accounts_by_partition::RentPayingAccountsByPartition, + snapshot_utils::setup_accounts_run_and_snapshot_paths, sorted_storages::SortedStorages, storable_accounts::StorableAccounts, verify_accounts_hash_in_background::VerifyAccountsHashInBackground, @@ -1113,7 +1114,10 @@ impl AccountStorageEntry { pub fn get_temp_accounts_paths(count: u32) -> IoResult<(Vec, Vec)> { let temp_dirs: IoResult> = (0..count).map(|_| TempDir::new()).collect(); let temp_dirs = temp_dirs?; - let paths: Vec = temp_dirs.iter().map(|t| t.path().to_path_buf()).collect(); + let paths: Vec = temp_dirs + .iter() + .map(|t: &TempDir| -> PathBuf { setup_accounts_run_and_snapshot_paths(t.path()).unwrap() }) + .collect(); Ok((temp_dirs, paths)) } diff --git a/runtime/src/hardened_unpack.rs b/runtime/src/hardened_unpack.rs index 9c8e85ec1cedae..fa1be008d144fb 100644 --- a/runtime/src/hardened_unpack.rs +++ b/runtime/src/hardened_unpack.rs @@ -132,6 +132,11 @@ where } let parts: Vec<_> = parts.map(|p| p.unwrap()).collect(); + let account_filename: Option = if parts.len() == 2 && parts[0] == "accounts" { + Some(PathBuf::from(parts[1])) + } else { + None + }; let unpack_dir = match entry_checker(parts.as_slice(), kind) { UnpackPath::Invalid => { return Err(UnpackError::Archive(format!( @@ -175,8 +180,16 @@ where let entry_path_buf = unpack_dir.join(entry.path()?); set_perms(&entry_path_buf, mode)?; + let entry_path = if let Some(account_filename) = account_filename { + let stripped_path = unpack_dir.join(account_filename); // strip away "accounts" + fs::rename(&entry_path_buf, &stripped_path)?; + stripped_path + } else { + entry_path_buf + }; + // Process entry after setting permissions - entry_processor(entry_path_buf); + entry_processor(entry_path); total_entries += 1; let now = Instant::now(); diff --git a/runtime/src/serde_snapshot/tests.rs b/runtime/src/serde_snapshot/tests.rs index a7980ff857ada1..cf666c7a86a130 100644 --- a/runtime/src/serde_snapshot/tests.rs +++ b/runtime/src/serde_snapshot/tests.rs @@ -13,7 +13,9 @@ use { bank::{Bank, BankTestConfig}, epoch_accounts_hash, genesis_utils::{self, activate_all_features, activate_feature}, - snapshot_utils::{get_storages_to_serialize, ArchiveFormat}, + snapshot_utils::{ + generate_test_tmp_account_path, get_storages_to_serialize, ArchiveFormat, + }, status_cache::StatusCache, }, bincode::serialize_into, @@ -575,7 +577,7 @@ fn test_extra_fields_full_snapshot_archive() { // Set extra field bank.fee_rate_governor.lamports_per_signature = 7000; - let accounts_dir = TempDir::new().unwrap(); + let accounts_dir = generate_test_tmp_account_path(); let bank_snapshots_dir = TempDir::new().unwrap(); let full_snapshot_archives_dir = TempDir::new().unwrap(); let incremental_snapshot_archives_dir = TempDir::new().unwrap(); @@ -595,7 +597,7 @@ fn test_extra_fields_full_snapshot_archive() { // Deserialize let (dbank, _) = snapshot_utils::bank_from_snapshot_archives( - &[PathBuf::from(accounts_dir.path())], + &[accounts_dir], bank_snapshots_dir.path(), &snapshot_archive_info, None, diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index 4455536dbd3643..d69841bccd5391 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -833,6 +833,19 @@ where Ok(()) } +/// To allow generating a bank snapshot directory with full state information, we need to +/// hardlink account appendvec files from the runtime operation directory to a snapshot +/// hardlink directory. This is to create the run/ and snapshot sub directories for an +/// account_path provided by the user. These two sub directories are on the same file +/// system partition to allow hard-linking. +pub fn setup_accounts_run_and_snapshot_paths>(path: P) -> Result { + let run_path = path.as_ref().join("run"); + let snapshot_path = path.as_ref().join("snapshot"); + fs::create_dir_all(&run_path)?; + fs::create_dir_all(snapshot_path)?; + Ok(run_path) +} + /// Serialize a bank to a snapshot /// /// **DEVELOPER NOTE** Any error that is returned from this function may bring down the node! This @@ -2123,10 +2136,11 @@ pub fn verify_snapshot_archive( { let temp_dir = tempfile::TempDir::new().unwrap(); let unpack_dir = temp_dir.path(); + let account_dir = setup_accounts_run_and_snapshot_paths(unpack_dir).unwrap(); untar_snapshot_in( snapshot_archive, unpack_dir, - &[unpack_dir.to_path_buf()], + &[account_dir.clone()], archive_format, 1, ) @@ -2147,9 +2161,11 @@ pub fn verify_snapshot_archive( assert!(!dir_diff::is_different(&snapshots_to_verify, unpacked_snapshots).unwrap()); + // The account files in the archive accounts/ have been expanded to [account_paths]. + // Remove the empty "accounts" directory for the directory comparison below. + std::fs::remove_dir(account_dir.join("accounts")).unwrap(); // Check the account entries are the same - let unpacked_accounts = unpack_dir.join("accounts"); - assert!(!dir_diff::is_different(&storages_to_verify, unpacked_accounts).unwrap()); + assert!(!dir_diff::is_different(&storages_to_verify, account_dir).unwrap()); } /// Remove outdated bank snapshots @@ -2406,6 +2422,11 @@ pub fn should_take_incremental_snapshot( && last_full_snapshot_slot.is_some() } +pub fn generate_test_tmp_account_path() -> PathBuf { + let accounts_dir = tempfile::TempDir::new().unwrap(); + setup_accounts_run_and_snapshot_paths(accounts_dir.path()).unwrap() +} + #[cfg(test)] mod tests { use { @@ -3324,7 +3345,7 @@ mod tests { original_bank.register_tick(&Hash::new_unique()); } - let accounts_dir = tempfile::TempDir::new().unwrap(); + let accounts_dir = generate_test_tmp_account_path(); let bank_snapshots_dir = tempfile::TempDir::new().unwrap(); let full_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); let incremental_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); @@ -3343,7 +3364,7 @@ mod tests { .unwrap(); let (roundtrip_bank, _) = bank_from_snapshot_archives( - &[PathBuf::from(accounts_dir.path())], + &[accounts_dir], bank_snapshots_dir.path(), &snapshot_archive_info, None, @@ -3436,7 +3457,7 @@ mod tests { bank4.register_tick(&Hash::new_unique()); } - let accounts_dir = tempfile::TempDir::new().unwrap(); + let accounts_dir = generate_test_tmp_account_path(); let bank_snapshots_dir = tempfile::TempDir::new().unwrap(); let full_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); let incremental_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); @@ -3455,7 +3476,7 @@ mod tests { .unwrap(); let (roundtrip_bank, _) = bank_from_snapshot_archives( - &[PathBuf::from(accounts_dir.path())], + &[accounts_dir], bank_snapshots_dir.path(), &full_snapshot_archive_info, None, @@ -3527,7 +3548,7 @@ mod tests { bank1.register_tick(&Hash::new_unique()); } - let accounts_dir = tempfile::TempDir::new().unwrap(); + let accounts_dir = generate_test_tmp_account_path(); let bank_snapshots_dir = tempfile::TempDir::new().unwrap(); let full_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); let incremental_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); @@ -3587,7 +3608,7 @@ mod tests { .unwrap(); let (roundtrip_bank, _) = bank_from_snapshot_archives( - &[PathBuf::from(accounts_dir.path())], + &[accounts_dir], bank_snapshots_dir.path(), &full_snapshot_archive_info, Some(&incremental_snapshot_archive_info), @@ -3649,7 +3670,7 @@ mod tests { bank1.register_tick(&Hash::new_unique()); } - let accounts_dir = tempfile::TempDir::new().unwrap(); + let accounts_dir = generate_test_tmp_account_path(); let bank_snapshots_dir = tempfile::TempDir::new().unwrap(); let full_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); let incremental_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); @@ -3712,7 +3733,7 @@ mod tests { &bank_snapshots_dir, &full_snapshot_archives_dir, &incremental_snapshot_archives_dir, - &[accounts_dir.as_ref().to_path_buf()], + &[accounts_dir], &genesis_config, &RuntimeConfig::default(), None, @@ -3762,7 +3783,7 @@ mod tests { let key1 = Keypair::new(); let key2 = Keypair::new(); - let accounts_dir = tempfile::TempDir::new().unwrap(); + let accounts_dir = generate_test_tmp_account_path(); let bank_snapshots_dir = tempfile::TempDir::new().unwrap(); let full_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); let incremental_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); @@ -3774,7 +3795,7 @@ mod tests { let bank0 = Arc::new(Bank::new_with_paths_for_tests( &genesis_config, Arc::::default(), - vec![accounts_dir.path().to_path_buf()], + vec![accounts_dir.clone()], AccountSecondaryIndexes::default(), AccountShrinkThreshold::default(), )); @@ -3848,7 +3869,7 @@ mod tests { ) .unwrap(); let (deserialized_bank, _) = bank_from_snapshot_archives( - &[accounts_dir.path().to_path_buf()], + &[accounts_dir.as_path().to_path_buf()], bank_snapshots_dir.path(), &full_snapshot_archive_info, Some(&incremental_snapshot_archive_info), @@ -3912,7 +3933,7 @@ mod tests { .unwrap(); let (deserialized_bank, _) = bank_from_snapshot_archives( - &[accounts_dir.path().to_path_buf()], + &[accounts_dir.as_path().to_path_buf()], bank_snapshots_dir.path(), &full_snapshot_archive_info, Some(&incremental_snapshot_archive_info), diff --git a/test-validator/src/lib.rs b/test-validator/src/lib.rs index d7016cff0fe9cd..aed8dc69530806 100644 --- a/test-validator/src/lib.rs +++ b/test-validator/src/lib.rs @@ -25,7 +25,7 @@ use { accounts_db::AccountsDbConfig, accounts_index::AccountsIndexConfig, bank_forks::BankForks, genesis_utils::create_genesis_config_with_leader_ex, hardened_unpack::MAX_GENESIS_ARCHIVE_UNPACKED_SIZE, runtime_config::RuntimeConfig, - snapshot_config::SnapshotConfig, + snapshot_config::SnapshotConfig, snapshot_utils::setup_accounts_run_and_snapshot_paths, }, solana_sdk::{ account::{Account, AccountSharedData}, @@ -802,7 +802,10 @@ impl TestValidator { rpc_config: config.rpc_config.clone(), pubsub_config: config.pubsub_config.clone(), accounts_hash_interval_slots: 100, - account_paths: vec![ledger_path.join("accounts")], + account_paths: vec![setup_accounts_run_and_snapshot_paths( + ledger_path.join("accounts"), + ) + .unwrap()], poh_verify: false, // Skip PoH verification of ledger on startup for speed snapshot_config: SnapshotConfig { full_snapshot_archive_interval_slots: 100, diff --git a/validator/src/main.rs b/validator/src/main.rs index 0035bfc4d14240..e716ab520fae4c 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -34,7 +34,9 @@ use { }, runtime_config::RuntimeConfig, snapshot_config::{SnapshotConfig, SnapshotUsage}, - snapshot_utils::{self, ArchiveFormat, SnapshotVersion}, + snapshot_utils::{ + self, setup_accounts_run_and_snapshot_paths, ArchiveFormat, SnapshotVersion, + }, }, solana_sdk::{ clock::{Slot, DEFAULT_S_PER_SLOT}, @@ -1251,7 +1253,7 @@ pub fn main() { .ok(); // Create and canonicalize account paths to avoid issues with symlink creation - validator_config.account_paths = account_paths + let account_run_paths: Vec = account_paths .into_iter() .map(|account_path| { match fs::create_dir_all(&account_path).and_then(|_| fs::canonicalize(&account_path)) { @@ -1261,8 +1263,20 @@ pub fn main() { exit(1); } } - }) - .collect(); + }).map( + |account_path| { + // For all account_paths, set up the run/ and snapshot/ sub directories. + match setup_accounts_run_and_snapshot_paths(&account_path) { + Ok(account_run_path) => account_run_path, + Err(err) => { + eprintln!("Unable to set up account run and snapshot sub directories: {account_path:?}, err: {err:?}"); + exit(1); + } + } + }).collect(); + + // From now on, use run/ paths in the same way as the previous account_paths. + validator_config.account_paths = account_run_paths; validator_config.account_shrink_paths = account_shrink_paths.map(|paths| { paths From aac0c1c0ed61b1ee06733873a8ce02fd8714a741 Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Thu, 19 Jan 2023 18:05:46 -0800 Subject: [PATCH 02/11] fix test test_concurrent_snapshot_packaging --- runtime/src/snapshot_utils.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index d69841bccd5391..8079522d510a9a 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -2161,9 +2161,12 @@ pub fn verify_snapshot_archive( assert!(!dir_diff::is_different(&snapshots_to_verify, unpacked_snapshots).unwrap()); - // The account files in the archive accounts/ have been expanded to [account_paths]. + // In the unarchiving case, there is an extra empty "accounts" directory. The account + // files in the archive accounts/ have been expanded to [account_paths]. // Remove the empty "accounts" directory for the directory comparison below. - std::fs::remove_dir(account_dir.join("accounts")).unwrap(); + // In some test cases the directory to compare do not come from unchiving. Use + // unwarp_or_default to ignore the error if this directory does not exist. + std::fs::remove_dir(account_dir.join("accounts")).unwrap_or_default(); // Check the account entries are the same assert!(!dir_diff::is_different(&storages_to_verify, account_dir).unwrap()); } From a4361c7a42078f5327b87023d900a4c278f95b9f Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Fri, 20 Jan 2023 12:58:16 -0800 Subject: [PATCH 03/11] review comments. renamed the path setup function --- core/src/snapshot_packager_service.rs | 6 ++++-- ledger-tool/src/main.rs | 6 +++--- local-cluster/src/local_cluster.rs | 9 ++++++--- local-cluster/tests/common.rs | 4 ++-- local-cluster/tests/local_cluster.rs | 8 ++++++-- runtime/src/accounts_db.rs | 6 ++++-- runtime/src/snapshot_utils.rs | 18 +++++++++++------- test-validator/src/lib.rs | 11 ++++++----- validator/src/main.rs | 6 +++--- 9 files changed, 45 insertions(+), 29 deletions(-) diff --git a/core/src/snapshot_packager_service.rs b/core/src/snapshot_packager_service.rs index d225a1e26870cc..c9b48716edc17b 100644 --- a/core/src/snapshot_packager_service.rs +++ b/core/src/snapshot_packager_service.rs @@ -228,7 +228,7 @@ mod tests { snapshot_hash::SnapshotHash, snapshot_package::{SnapshotPackage, SnapshotType}, snapshot_utils::{ - self, setup_accounts_run_and_snapshot_paths, ArchiveFormat, SnapshotVersion, + self, create_accounts_run_and_snapshot_dirs, ArchiveFormat, SnapshotVersion, SNAPSHOT_STATUS_CACHE_FILENAME, }, }, @@ -268,7 +268,9 @@ mod tests { fn create_and_verify_snapshot(temp_dir: &Path) { let accounts_dir = temp_dir.join("accounts"); - let accounts_dir = setup_accounts_run_and_snapshot_paths(accounts_dir.as_path()).unwrap(); + let accounts_dir = create_accounts_run_and_snapshot_dirs(accounts_dir.as_path()) + .unwrap() + .0; let snapshots_dir = temp_dir.join("snapshots"); let full_snapshot_archives_dir = temp_dir.join("full_snapshot_archives"); diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index b23afce13af66b..522b880a79a135 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -62,7 +62,7 @@ use { snapshot_hash::StartingSnapshotHashes, snapshot_minimizer::SnapshotMinimizer, snapshot_utils::{ - self, move_and_async_delete_path, setup_accounts_run_and_snapshot_paths, ArchiveFormat, + self, create_accounts_run_and_snapshot_dirs, move_and_async_delete_path, ArchiveFormat, SnapshotVersion, DEFAULT_ARCHIVE_COMPRESSION, SUPPORTED_ARCHIVE_COMPRESSION, }, }, @@ -1096,8 +1096,8 @@ fn load_bank_forks( // For all account_paths, set up the run/ and snapshot/ sub directories. let account_run_paths: Vec = account_paths.into_iter().map( |account_path| { - match setup_accounts_run_and_snapshot_paths(&account_path) { - Ok(account_run_path) => account_run_path, + match create_accounts_run_and_snapshot_dirs(&account_path) { + Ok((account_run_path, _account_snapshot_path)) => account_run_path, Err(err) => { eprintln!("Unable to set up account run and snapshot sub directories: {account_path:?}, err: {err:?}"); exit(1); diff --git a/local-cluster/src/local_cluster.rs b/local-cluster/src/local_cluster.rs index c26d3342be5bcc..1b759044c98139 100644 --- a/local-cluster/src/local_cluster.rs +++ b/local-cluster/src/local_cluster.rs @@ -22,7 +22,7 @@ use { ValidatorVoteKeypairs, }, snapshot_config::SnapshotConfig, - snapshot_utils::setup_accounts_run_and_snapshot_paths, + snapshot_utils::create_accounts_run_and_snapshot_dirs, }, solana_sdk::{ account::{Account, AccountSharedData}, @@ -148,8 +148,11 @@ impl LocalCluster { config: &mut ValidatorConfig, ledger_path: &Path, ) { - config.account_paths = - vec![setup_accounts_run_and_snapshot_paths(ledger_path.join("accounts")).unwrap()]; + config.account_paths = vec![ + create_accounts_run_and_snapshot_dirs(ledger_path.join("accounts")) + .unwrap() + .0, + ]; config.tower_storage = Arc::new(FileTowerStorage::new(ledger_path.to_path_buf())); let snapshot_config = &mut config.snapshot_config; diff --git a/local-cluster/tests/common.rs b/local-cluster/tests/common.rs index 7917fc1879a93c..c6618283161352 100644 --- a/local-cluster/tests/common.rs +++ b/local-cluster/tests/common.rs @@ -22,7 +22,7 @@ use { }, solana_rpc_client::rpc_client::RpcClient, solana_runtime::{ - snapshot_config::SnapshotConfig, snapshot_utils::setup_accounts_run_and_snapshot_paths, + snapshot_config::SnapshotConfig, snapshot_utils::create_accounts_run_and_snapshot_dirs, }, solana_sdk::{ account::AccountSharedData, @@ -438,7 +438,7 @@ pub fn generate_account_paths(num_account_paths: usize) -> (Vec, Vec = account_storage_dirs .iter() - .map(|a| setup_accounts_run_and_snapshot_paths(a.path()).unwrap()) + .map(|a| create_accounts_run_and_snapshot_dirs(a.path()).unwrap().0) .collect(); (account_storage_dirs, account_storage_paths) } diff --git a/local-cluster/tests/local_cluster.rs b/local-cluster/tests/local_cluster.rs index 666bffa3e00fff..96dec5c8899aba 100644 --- a/local-cluster/tests/local_cluster.rs +++ b/local-cluster/tests/local_cluster.rs @@ -39,7 +39,7 @@ use { snapshot_config::SnapshotConfig, snapshot_package::SnapshotType, snapshot_utils::{ - self, setup_accounts_run_and_snapshot_paths, ArchiveFormat, SnapshotVersion, + self, create_accounts_run_and_snapshot_dirs, ArchiveFormat, SnapshotVersion, }, }, solana_sdk::{ @@ -2154,7 +2154,11 @@ fn create_snapshot_to_hard_fork( let (bank_forks, ..) = bank_forks_utils::load( &genesis_config, blockstore, - vec![setup_accounts_run_and_snapshot_paths(ledger_path.join("accounts")).unwrap()], + vec![ + create_accounts_run_and_snapshot_dirs(ledger_path.join("accounts")) + .unwrap() + .0, + ], None, snapshot_config.as_ref(), process_options, diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index e0ccb0641c03b3..410de1a3bd2110 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -53,7 +53,7 @@ use { read_only_accounts_cache::ReadOnlyAccountsCache, rent_collector::RentCollector, rent_paying_accounts_by_partition::RentPayingAccountsByPartition, - snapshot_utils::setup_accounts_run_and_snapshot_paths, + snapshot_utils::create_accounts_run_and_snapshot_dirs, sorted_storages::SortedStorages, storable_accounts::StorableAccounts, verify_accounts_hash_in_background::VerifyAccountsHashInBackground, @@ -1116,7 +1116,9 @@ pub fn get_temp_accounts_paths(count: u32) -> IoResult<(Vec, Vec = temp_dirs .iter() - .map(|t: &TempDir| -> PathBuf { setup_accounts_run_and_snapshot_paths(t.path()).unwrap() }) + .map(|t: &TempDir| -> PathBuf { + create_accounts_run_and_snapshot_dirs(t.path()).unwrap().0 + }) .collect(); Ok((temp_dirs, paths)) } diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index 8079522d510a9a..63aa10a248a1d6 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -838,12 +838,14 @@ where /// hardlink directory. This is to create the run/ and snapshot sub directories for an /// account_path provided by the user. These two sub directories are on the same file /// system partition to allow hard-linking. -pub fn setup_accounts_run_and_snapshot_paths>(path: P) -> Result { - let run_path = path.as_ref().join("run"); - let snapshot_path = path.as_ref().join("snapshot"); +pub fn create_accounts_run_and_snapshot_dirs( + account_dir: impl AsRef, +) -> Result<(PathBuf, PathBuf)> { + let run_path = account_dir.as_ref().join("run"); + let snapshot_path = account_dir.as_ref().join("snapshot"); fs::create_dir_all(&run_path)?; - fs::create_dir_all(snapshot_path)?; - Ok(run_path) + fs::create_dir_all(&snapshot_path)?; + Ok((run_path, snapshot_path)) } /// Serialize a bank to a snapshot @@ -2136,7 +2138,7 @@ pub fn verify_snapshot_archive( { let temp_dir = tempfile::TempDir::new().unwrap(); let unpack_dir = temp_dir.path(); - let account_dir = setup_accounts_run_and_snapshot_paths(unpack_dir).unwrap(); + let account_dir = create_accounts_run_and_snapshot_dirs(unpack_dir).unwrap().0; untar_snapshot_in( snapshot_archive, unpack_dir, @@ -2427,7 +2429,9 @@ pub fn should_take_incremental_snapshot( pub fn generate_test_tmp_account_path() -> PathBuf { let accounts_dir = tempfile::TempDir::new().unwrap(); - setup_accounts_run_and_snapshot_paths(accounts_dir.path()).unwrap() + create_accounts_run_and_snapshot_dirs(accounts_dir.path()) + .unwrap() + .0 } #[cfg(test)] diff --git a/test-validator/src/lib.rs b/test-validator/src/lib.rs index aed8dc69530806..7d28fce8cd0c00 100644 --- a/test-validator/src/lib.rs +++ b/test-validator/src/lib.rs @@ -25,7 +25,7 @@ use { accounts_db::AccountsDbConfig, accounts_index::AccountsIndexConfig, bank_forks::BankForks, genesis_utils::create_genesis_config_with_leader_ex, hardened_unpack::MAX_GENESIS_ARCHIVE_UNPACKED_SIZE, runtime_config::RuntimeConfig, - snapshot_config::SnapshotConfig, snapshot_utils::setup_accounts_run_and_snapshot_paths, + snapshot_config::SnapshotConfig, snapshot_utils::create_accounts_run_and_snapshot_dirs, }, solana_sdk::{ account::{Account, AccountSharedData}, @@ -802,10 +802,11 @@ impl TestValidator { rpc_config: config.rpc_config.clone(), pubsub_config: config.pubsub_config.clone(), accounts_hash_interval_slots: 100, - account_paths: vec![setup_accounts_run_and_snapshot_paths( - ledger_path.join("accounts"), - ) - .unwrap()], + account_paths: vec![ + create_accounts_run_and_snapshot_dirs(ledger_path.join("accounts")) + .unwrap() + .0, + ], poh_verify: false, // Skip PoH verification of ledger on startup for speed snapshot_config: SnapshotConfig { full_snapshot_archive_interval_slots: 100, diff --git a/validator/src/main.rs b/validator/src/main.rs index e716ab520fae4c..4482a956e647ec 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -35,7 +35,7 @@ use { runtime_config::RuntimeConfig, snapshot_config::{SnapshotConfig, SnapshotUsage}, snapshot_utils::{ - self, setup_accounts_run_and_snapshot_paths, ArchiveFormat, SnapshotVersion, + self, create_accounts_run_and_snapshot_dirs, ArchiveFormat, SnapshotVersion, }, }, solana_sdk::{ @@ -1266,8 +1266,8 @@ pub fn main() { }).map( |account_path| { // For all account_paths, set up the run/ and snapshot/ sub directories. - match setup_accounts_run_and_snapshot_paths(&account_path) { - Ok(account_run_path) => account_run_path, + match create_accounts_run_and_snapshot_dirs(&account_path) { + Ok((account_run_path, _account_snapshot_path)) => account_run_path, Err(err) => { eprintln!("Unable to set up account run and snapshot sub directories: {account_path:?}, err: {err:?}"); exit(1); From 09975728bb56e507bf8a699795ff80a77679d535 Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Mon, 23 Jan 2023 11:58:06 -0800 Subject: [PATCH 04/11] Addressed most of the review comments --- core/tests/epoch_accounts_hash.rs | 4 ++-- core/tests/snapshots.rs | 12 +++++++----- ledger-tool/src/main.rs | 2 +- runtime/src/serde_snapshot/tests.rs | 4 ++-- runtime/src/snapshot_utils.rs | 23 ++++++++++++----------- validator/src/main.rs | 2 +- 6 files changed, 25 insertions(+), 22 deletions(-) diff --git a/core/tests/epoch_accounts_hash.rs b/core/tests/epoch_accounts_hash.rs index e54168e4486bf6..5c7fc4be7e1c5f 100755 --- a/core/tests/epoch_accounts_hash.rs +++ b/core/tests/epoch_accounts_hash.rs @@ -1,6 +1,6 @@ #![allow(clippy::integer_arithmetic)] use { - crate::snapshot_utils::generate_test_tmp_account_path, + crate::snapshot_utils::create_tmp_accounts_dir_for_tests, log::*, solana_core::{ accounts_hash_verifier::AccountsHashVerifier, @@ -443,7 +443,7 @@ fn test_snapshots_have_expected_epoch_accounts_hash() { std::thread::sleep(Duration::from_secs(1)); }; - let accounts_dir = generate_test_tmp_account_path(); + let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests(); let deserialized_bank = snapshot_utils::bank_from_snapshot_archives( &[accounts_dir.as_path().to_path_buf()], &snapshot_config.bank_snapshots_dir, diff --git a/core/tests/snapshots.rs b/core/tests/snapshots.rs index e3d7ff80ae3d80..05668bb30105fa 100644 --- a/core/tests/snapshots.rs +++ b/core/tests/snapshots.rs @@ -1,7 +1,7 @@ #![allow(clippy::integer_arithmetic)] use { - crate::snapshot_utils::generate_test_tmp_account_path, + crate::snapshot_utils::create_tmp_accounts_dir_for_tests, bincode::serialize_into, crossbeam_channel::unbounded, fs_extra::dir::CopyOptions, @@ -75,6 +75,7 @@ struct SnapshotTestConfig { incremental_snapshot_archives_dir: TempDir, full_snapshot_archives_dir: TempDir, bank_snapshots_dir: TempDir, + _accounts_tmp_dir: TempDir, accounts_dir: PathBuf, } @@ -86,7 +87,7 @@ impl SnapshotTestConfig { full_snapshot_archive_interval_slots: Slot, incremental_snapshot_archive_interval_slots: Slot, ) -> SnapshotTestConfig { - let accounts_dir = generate_test_tmp_account_path(); + let (_accounts_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests(); let bank_snapshots_dir = TempDir::new().unwrap(); let full_snapshot_archives_dir = TempDir::new().unwrap(); let incremental_snapshot_archives_dir = TempDir::new().unwrap(); @@ -131,6 +132,7 @@ impl SnapshotTestConfig { incremental_snapshot_archives_dir, full_snapshot_archives_dir, bank_snapshots_dir, + _accounts_tmp_dir, accounts_dir, } } @@ -297,7 +299,7 @@ fn run_bank_forks_snapshot_n( .unwrap(); // Restore bank from snapshot - let temporary_accounts_dir = generate_test_tmp_account_path(); + let (_tmp_dir, temporary_accounts_dir) = create_tmp_accounts_dir_for_tests(); let account_paths = &[temporary_accounts_dir]; let genesis_config = &snapshot_test_config.genesis_config_info.genesis_config; restore_from_snapshot(bank_forks, last_slot, genesis_config, account_paths); @@ -822,7 +824,7 @@ fn test_bank_forks_incremental_snapshot( // Accounts directory needs to be separate from the active accounts directory // so that dropping append vecs in the active accounts directory doesn't // delete the unpacked appendvecs in the snapshot - let temporary_accounts_dir = generate_test_tmp_account_path(); + let (_tmp_dir, temporary_accounts_dir) = create_tmp_accounts_dir_for_tests(); restore_from_snapshots_and_check_banks_are_equal( &bank, &snapshot_test_config.snapshot_config, @@ -1121,7 +1123,7 @@ fn test_snapshots_with_background_services( } // Load the snapshot and ensure it matches what's in BankForks - let temporary_accounts_dir = generate_test_tmp_account_path(); + let (_tmp_dir, temporary_accounts_dir) = create_tmp_accounts_dir_for_tests(); let (deserialized_bank, ..) = snapshot_utils::bank_from_latest_snapshot_archives( &snapshot_test_config.snapshot_config.bank_snapshots_dir, &snapshot_test_config diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 522b880a79a135..78ecf386c9524e 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -1099,7 +1099,7 @@ fn load_bank_forks( match create_accounts_run_and_snapshot_dirs(&account_path) { Ok((account_run_path, _account_snapshot_path)) => account_run_path, Err(err) => { - eprintln!("Unable to set up account run and snapshot sub directories: {account_path:?}, err: {err:?}"); + eprintln!("Unable to create account run and snapshot sub directories: {}, err: {err:?}", account_path.display()); exit(1); } } diff --git a/runtime/src/serde_snapshot/tests.rs b/runtime/src/serde_snapshot/tests.rs index cf666c7a86a130..b87f8bcca60ef3 100644 --- a/runtime/src/serde_snapshot/tests.rs +++ b/runtime/src/serde_snapshot/tests.rs @@ -14,7 +14,7 @@ use { epoch_accounts_hash, genesis_utils::{self, activate_all_features, activate_feature}, snapshot_utils::{ - generate_test_tmp_account_path, get_storages_to_serialize, ArchiveFormat, + create_tmp_accounts_dir_for_tests, get_storages_to_serialize, ArchiveFormat, }, status_cache::StatusCache, }, @@ -577,7 +577,7 @@ fn test_extra_fields_full_snapshot_archive() { // Set extra field bank.fee_rate_governor.lamports_per_signature = 7000; - let accounts_dir = generate_test_tmp_account_path(); + let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests(); let bank_snapshots_dir = TempDir::new().unwrap(); let full_snapshot_archives_dir = TempDir::new().unwrap(); let incremental_snapshot_archives_dir = TempDir::new().unwrap(); diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index 63aa10a248a1d6..ef97baf6246899 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -2166,9 +2166,9 @@ pub fn verify_snapshot_archive( // In the unarchiving case, there is an extra empty "accounts" directory. The account // files in the archive accounts/ have been expanded to [account_paths]. // Remove the empty "accounts" directory for the directory comparison below. - // In some test cases the directory to compare do not come from unchiving. Use + // In some test cases the directory to compare do not come from unarchiving. Use // unwarp_or_default to ignore the error if this directory does not exist. - std::fs::remove_dir(account_dir.join("accounts")).unwrap_or_default(); + _ = std::fs::remove_dir(account_dir.join("accounts")); // Check the account entries are the same assert!(!dir_diff::is_different(&storages_to_verify, account_dir).unwrap()); } @@ -2427,11 +2427,12 @@ pub fn should_take_incremental_snapshot( && last_full_snapshot_slot.is_some() } -pub fn generate_test_tmp_account_path() -> PathBuf { - let accounts_dir = tempfile::TempDir::new().unwrap(); - create_accounts_run_and_snapshot_dirs(accounts_dir.path()) +pub fn create_tmp_accounts_dir_for_tests() -> (TempDir, PathBuf) { + let tmp_dir = tempfile::TempDir::new().unwrap(); + let account_dir = create_accounts_run_and_snapshot_dirs(tmp_dir.path()) .unwrap() - .0 + .0; + (tmp_dir, account_dir) } #[cfg(test)] @@ -3352,7 +3353,7 @@ mod tests { original_bank.register_tick(&Hash::new_unique()); } - let accounts_dir = generate_test_tmp_account_path(); + let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests(); let bank_snapshots_dir = tempfile::TempDir::new().unwrap(); let full_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); let incremental_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); @@ -3464,7 +3465,7 @@ mod tests { bank4.register_tick(&Hash::new_unique()); } - let accounts_dir = generate_test_tmp_account_path(); + let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests(); let bank_snapshots_dir = tempfile::TempDir::new().unwrap(); let full_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); let incremental_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); @@ -3555,7 +3556,7 @@ mod tests { bank1.register_tick(&Hash::new_unique()); } - let accounts_dir = generate_test_tmp_account_path(); + let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests(); let bank_snapshots_dir = tempfile::TempDir::new().unwrap(); let full_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); let incremental_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); @@ -3677,7 +3678,7 @@ mod tests { bank1.register_tick(&Hash::new_unique()); } - let accounts_dir = generate_test_tmp_account_path(); + let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests(); let bank_snapshots_dir = tempfile::TempDir::new().unwrap(); let full_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); let incremental_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); @@ -3790,7 +3791,7 @@ mod tests { let key1 = Keypair::new(); let key2 = Keypair::new(); - let accounts_dir = generate_test_tmp_account_path(); + let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests(); let bank_snapshots_dir = tempfile::TempDir::new().unwrap(); let full_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); let incremental_snapshot_archives_dir = tempfile::TempDir::new().unwrap(); diff --git a/validator/src/main.rs b/validator/src/main.rs index 4482a956e647ec..67f39206a871e8 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -1269,7 +1269,7 @@ pub fn main() { match create_accounts_run_and_snapshot_dirs(&account_path) { Ok((account_run_path, _account_snapshot_path)) => account_run_path, Err(err) => { - eprintln!("Unable to set up account run and snapshot sub directories: {account_path:?}, err: {err:?}"); + eprintln!("Unable to create account run and snapshot sub directories: {}, err: {err:?}", account_path.display()); exit(1); } } From a7ad7171926147012026b0b8fc8edc6454bef2a8 Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Mon, 23 Jan 2023 12:25:19 -0800 Subject: [PATCH 05/11] remove explict type def for map result --- runtime/src/accounts_db.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 410de1a3bd2110..eafb9bbfdd3a67 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -1114,7 +1114,7 @@ impl AccountStorageEntry { pub fn get_temp_accounts_paths(count: u32) -> IoResult<(Vec, Vec)> { let temp_dirs: IoResult> = (0..count).map(|_| TempDir::new()).collect(); let temp_dirs = temp_dirs?; - let paths: Vec = temp_dirs + let paths = temp_dirs .iter() .map(|t: &TempDir| -> PathBuf { create_accounts_run_and_snapshot_dirs(t.path()).unwrap().0 From b562861b964922fe807b91a269f304b5158d2e91 Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Mon, 23 Jan 2023 16:11:53 -0800 Subject: [PATCH 06/11] handle create_accounts_run_and_snapshot_dirs error with expect --- runtime/src/accounts_db.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index eafb9bbfdd3a67..4581f99144649d 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -1117,7 +1117,9 @@ pub fn get_temp_accounts_paths(count: u32) -> IoResult<(Vec, Vec PathBuf { - create_accounts_run_and_snapshot_dirs(t.path()).unwrap().0 + let (run_dir, _snapshot_dir) = create_accounts_run_and_snapshot_dirs(t.path()) + .expect("failed to create the run and snapshot sub-directories for an ccount path"); + run_dir }) .collect(); Ok((temp_dirs, paths)) From 449a9ee08b34956bdbb7a74fe07b5526cf9b80a3 Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Mon, 23 Jan 2023 17:20:53 -0800 Subject: [PATCH 07/11] update with more review comments --- core/src/snapshot_packager_service.rs | 2 +- core/tests/snapshots.rs | 4 ++-- runtime/src/snapshot_utils.rs | 4 +--- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/core/src/snapshot_packager_service.rs b/core/src/snapshot_packager_service.rs index c9b48716edc17b..ca8edd609cdee7 100644 --- a/core/src/snapshot_packager_service.rs +++ b/core/src/snapshot_packager_service.rs @@ -268,7 +268,7 @@ mod tests { fn create_and_verify_snapshot(temp_dir: &Path) { let accounts_dir = temp_dir.join("accounts"); - let accounts_dir = create_accounts_run_and_snapshot_dirs(accounts_dir.as_path()) + let accounts_dir = create_accounts_run_and_snapshot_dirs(accounts_dir) .unwrap() .0; diff --git a/core/tests/snapshots.rs b/core/tests/snapshots.rs index 05668bb30105fa..56e8e4c6793492 100644 --- a/core/tests/snapshots.rs +++ b/core/tests/snapshots.rs @@ -75,8 +75,8 @@ struct SnapshotTestConfig { incremental_snapshot_archives_dir: TempDir, full_snapshot_archives_dir: TempDir, bank_snapshots_dir: TempDir, - _accounts_tmp_dir: TempDir, accounts_dir: PathBuf, + _accounts_tmp_dir: TempDir, } impl SnapshotTestConfig { @@ -132,8 +132,8 @@ impl SnapshotTestConfig { incremental_snapshot_archives_dir, full_snapshot_archives_dir, bank_snapshots_dir, - _accounts_tmp_dir, accounts_dir, + _accounts_tmp_dir, } } } diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index ef97baf6246899..b55ae7d8be4511 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -2429,9 +2429,7 @@ pub fn should_take_incremental_snapshot( pub fn create_tmp_accounts_dir_for_tests() -> (TempDir, PathBuf) { let tmp_dir = tempfile::TempDir::new().unwrap(); - let account_dir = create_accounts_run_and_snapshot_dirs(tmp_dir.path()) - .unwrap() - .0; + let account_dir = create_accounts_run_and_snapshot_dirs(&tmp_dir).unwrap().0; (tmp_dir, account_dir) } From 49f6fd3d35257ca02d013723727276fb8f0dd8d9 Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Mon, 23 Jan 2023 21:22:54 -0800 Subject: [PATCH 08/11] minor fixes from review comments --- core/tests/snapshots.rs | 2 +- runtime/src/accounts_db.rs | 2 +- runtime/src/snapshot_utils.rs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/tests/snapshots.rs b/core/tests/snapshots.rs index 56e8e4c6793492..c8bb603fb44d4c 100644 --- a/core/tests/snapshots.rs +++ b/core/tests/snapshots.rs @@ -828,7 +828,7 @@ fn test_bank_forks_incremental_snapshot( restore_from_snapshots_and_check_banks_are_equal( &bank, &snapshot_test_config.snapshot_config, - temporary_accounts_dir.as_path().to_path_buf(), + temporary_accounts_dir, &snapshot_test_config.genesis_config_info.genesis_config, ) .unwrap(); diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 4581f99144649d..8bbf1a1c3d3d95 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -1116,7 +1116,7 @@ pub fn get_temp_accounts_paths(count: u32) -> IoResult<(Vec, Vec PathBuf { + .map(|t| -> PathBuf { let (run_dir, _snapshot_dir) = create_accounts_run_and_snapshot_dirs(t.path()) .expect("failed to create the run and snapshot sub-directories for an ccount path"); run_dir diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index b55ae7d8be4511..c6f68dd13f73fe 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -2166,8 +2166,8 @@ pub fn verify_snapshot_archive( // In the unarchiving case, there is an extra empty "accounts" directory. The account // files in the archive accounts/ have been expanded to [account_paths]. // Remove the empty "accounts" directory for the directory comparison below. - // In some test cases the directory to compare do not come from unarchiving. Use - // unwarp_or_default to ignore the error if this directory does not exist. + // In some test cases the directory to compare do not come from unarchiving. + // Ignore the error when this directory does not exist. _ = std::fs::remove_dir(account_dir.join("accounts")); // Check the account entries are the same assert!(!dir_diff::is_different(&storages_to_verify, account_dir).unwrap()); From 1259ffedf47c2718d11103e887e3caa468c9173e Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Mon, 23 Jan 2023 21:54:30 -0800 Subject: [PATCH 09/11] simplify account_filename option assignment --- runtime/src/hardened_unpack.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/runtime/src/hardened_unpack.rs b/runtime/src/hardened_unpack.rs index fa1be008d144fb..cd43d5a092d14d 100644 --- a/runtime/src/hardened_unpack.rs +++ b/runtime/src/hardened_unpack.rs @@ -132,11 +132,8 @@ where } let parts: Vec<_> = parts.map(|p| p.unwrap()).collect(); - let account_filename: Option = if parts.len() == 2 && parts[0] == "accounts" { - Some(PathBuf::from(parts[1])) - } else { - None - }; + let account_filename = + (parts.len() == 2 && parts[0] == "accounts").then_some(PathBuf::from(parts[1])); let unpack_dir = match entry_checker(parts.as_slice(), kind) { UnpackPath::Invalid => { return Err(UnpackError::Archive(format!( From 0d7602bf91358e66ae9cb798702518e70d7d8930 Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Mon, 23 Jan 2023 22:11:44 -0800 Subject: [PATCH 10/11] handle error from create_accounts_run_and_snapshot_dirs --- runtime/src/accounts_db.rs | 10 +++++----- runtime/src/snapshot_utils.rs | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 8bbf1a1c3d3d95..04972ff2f62c1d 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -1114,14 +1114,14 @@ impl AccountStorageEntry { pub fn get_temp_accounts_paths(count: u32) -> IoResult<(Vec, Vec)> { let temp_dirs: IoResult> = (0..count).map(|_| TempDir::new()).collect(); let temp_dirs = temp_dirs?; - let paths = temp_dirs + + let paths: IoResult> = temp_dirs .iter() - .map(|t| -> PathBuf { - let (run_dir, _snapshot_dir) = create_accounts_run_and_snapshot_dirs(t.path()) - .expect("failed to create the run and snapshot sub-directories for an ccount path"); - run_dir + .map(|temp_dir| { + create_accounts_run_and_snapshot_dirs(temp_dir).map(|(run_dir, _snapshot_dir)| run_dir) }) .collect(); + let paths = paths?; Ok((temp_dirs, paths)) } diff --git a/runtime/src/snapshot_utils.rs b/runtime/src/snapshot_utils.rs index c6f68dd13f73fe..7515c7a6bca284 100644 --- a/runtime/src/snapshot_utils.rs +++ b/runtime/src/snapshot_utils.rs @@ -840,7 +840,7 @@ where /// system partition to allow hard-linking. pub fn create_accounts_run_and_snapshot_dirs( account_dir: impl AsRef, -) -> Result<(PathBuf, PathBuf)> { +) -> std::io::Result<(PathBuf, PathBuf)> { let run_path = account_dir.as_ref().join("run"); let snapshot_path = account_dir.as_ref().join("snapshot"); fs::create_dir_all(&run_path)?; From d69b9b679d17afbea86280865e055f6d6a01fde5 Mon Sep 17 00:00:00 2001 From: Xiang Zhu Date: Tue, 24 Jan 2023 09:54:36 -0800 Subject: [PATCH 11/11] use then instead of then_some for lazy evaluation --- runtime/src/hardened_unpack.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/hardened_unpack.rs b/runtime/src/hardened_unpack.rs index cd43d5a092d14d..e016fa1cae957f 100644 --- a/runtime/src/hardened_unpack.rs +++ b/runtime/src/hardened_unpack.rs @@ -133,7 +133,7 @@ where let parts: Vec<_> = parts.map(|p| p.unwrap()).collect(); let account_filename = - (parts.len() == 2 && parts[0] == "accounts").then_some(PathBuf::from(parts[1])); + (parts.len() == 2 && parts[0] == "accounts").then(|| PathBuf::from(parts[1])); let unpack_dir = match entry_checker(parts.as_slice(), kind) { UnpackPath::Invalid => { return Err(UnpackError::Archive(format!(