Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Consistently create temp dirs under ledger/farf#10848

Merged
ryoqun merged 1 commit intosolana-labs:masterfrom
ryoqun:consistent-temp-dir
Jul 1, 2020
Merged

Consistently create temp dirs under ledger/farf#10848
ryoqun merged 1 commit intosolana-labs:masterfrom
ryoqun:consistent-temp-dir

Conversation

@ryoqun
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun commented Jun 30, 2020

Problem

Sometimes temp dirs are created under /tmp (the tempfile crate's default) or not. This is confusing:

$ git grep tempdir_in origin/master 
origin/master:core/src/snapshot_packager_service.rs:        let link_snapshots_dir = tempfile::tempdir_in(&temp_dir).unwrap();
origin/master:runtime/src/snapshot_utils.rs:    let snapshot_hard_links_dir = tempfile::tempdir_in(snapshot_path)?;
origin/master:runtime/src/snapshot_utils.rs:    let unpack_dir = tempfile::tempdir_in(snapshot_path)?;
$ git grep TempDir::new origin/master 
origin/master:core/src/accounts_hash_verifier.rs:            let snapshot_links = TempDir::new().unwrap();
origin/master:core/src/snapshot_packager_service.rs:        create_and_verify_snapshot(TempDir::new().unwrap().path())
origin/master:core/tests/bank_forks.rs:            let accounts_dir = TempDir::new().unwrap();
origin/master:core/tests/bank_forks.rs:            let snapshot_dir = TempDir::new().unwrap();
origin/master:core/tests/bank_forks.rs:            let snapshot_output_path = TempDir::new().unwrap();
origin/master:core/tests/bank_forks.rs:        let saved_snapshots_dir = TempDir::new().unwrap();
origin/master:core/tests/bank_forks.rs:        let saved_accounts_dir = TempDir::new().unwrap();
origin/master:install/src/command.rs:    let temp_dir = TempDir::new(clap::crate_name!())?;
origin/master:ledger-tool/src/main.rs:                    let temp_dir = tempfile::TempDir::new().unwrap_or_else(|err| {
origin/master:ledger/src/blockstore.rs:        let temp_dir = tempfile::TempDir::new().unwrap();
origin/master:local-cluster/tests/local_cluster.rs:        .map(|_| TempDir::new().unwrap())
origin/master:local-cluster/tests/local_cluster.rs:    let snapshot_dir = TempDir::new().unwrap();
origin/master:local-cluster/tests/local_cluster.rs:    let snapshot_output_path = TempDir::new().unwrap();
origin/master:runtime/src/accounts_db.rs:    let temp_dirs: IOResult<Vec<TempDir>> = (0..count).map(|_| TempDir::new()).collect();
origin/master:runtime/src/hardened_unpack.rs:        let temp_dir = tempfile::TempDir::new().unwrap();
origin/master:runtime/src/serde_snapshot/tests.rs:    let copied_accounts = TempDir::new().unwrap();
origin/master:runtime/src/serde_snapshot/tests.rs:    let copied_accounts = TempDir::new().unwrap();
origin/master:runtime/src/serde_snapshot/tests.rs:    let copied_accounts = TempDir::new().unwrap();
origin/master:runtime/src/snapshot_utils.rs:    let staging_dir = TempDir::new()?;
origin/master:runtime/src/snapshot_utils.rs:    let temp_dir = tempfile::TempDir::new().unwrap();
origin/master:runtime/src/snapshot_utils.rs:        let temp_dir = tempfile::TempDir::new().unwrap();
origin/master:runtime/src/snapshot_utils.rs:        let temp_dir = tempfile::TempDir::new().unwrap();
origin/master:runtime/src/snapshot_utils.rs:        let temp_dir = tempfile::TempDir::new().unwrap();
origin/master:runtime/src/snapshot_utils.rs:        let temp_dir = tempfile::TempDir::new().unwrap();
origin/master:runtime/src/snapshot_utils.rs:        let temp_dir = tempfile::TempDir::new().unwrap();
$ git rev-parse origin/master
4b93a7c1f6859d3771d5e6a5ec1a03f763eb12b1

It took some time to notice mount-ing local-cluster/farf isn't enough while debugging: #10718 (comment)

Also, when I was testing on cloud instances in the distant past, I was crippled by the use of /tmp for snapshot-handling, because /tmp was on the system partition file system with little free space albeit plenty of capacity for the ledger dir.

Also, when creating snapshots, using same temp dir with the accounts dir may realize more cheaper file system move/copies.

Also, using the system-wide /tmp for testing and running validator as a background hampers the general system performance (i.e. my local computer) and CTRL-C-ing it pollutes /tmp for unneeded reason too often even if I provision a special dir farf on a different block device, ignoring system administrator's intention.....

Summary of Changes

Consistently, create them under the farf dir or the ledger dir. This PR doesn't fix all of uses in our codebase but still it should cover almost all relevant codepath.

Part of #10718

@ryoqun ryoqun requested a review from carllin June 30, 2020 08:10
leader_node.info.rpc.port(),
leader_node.info.rpc_pubsub.port(),
));
leader_config.account_paths = vec![leader_ledger_path.join("accounts")];
Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I wanted to fix this sneaky behavior as well... I don't think we should accept empty account_paths first of all... But I punted it...:

} else {
// Create a temporary set of accounts directories, used primarily
// for testing
let (temp_dirs, paths) = get_temp_accounts_paths(DEFAULT_NUM_DIRS).unwrap();
Self {
paths,
temp_paths: Some(temp_dirs),
..Self::default()
}
};

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 30, 2020

Codecov Report

Merging #10848 into master will increase coverage by 0.0%.
The diff coverage is 50.0%.

@@           Coverage Diff           @@
##           master   #10848   +/-   ##
=======================================
  Coverage    81.8%    81.9%           
=======================================
  Files         303      303           
  Lines       71079    71084    +5     
=======================================
+ Hits        58208    58228   +20     
+ Misses      12871    12856   -15     

@ryoqun ryoqun merged commit b89e506 into solana-labs:master Jul 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants