-
Notifications
You must be signed in to change notification settings - Fork 979
fix: clean old txhashset snapshot zips on every zip_read #3888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,7 @@ use crate::util::secp::pedersen::{Commitment, RangeProof}; | |
| use crate::util::{file, secp_static, zip, StopState}; | ||
| use crate::SyncState; | ||
| use croaring::Bitmap; | ||
| use grin_store::pmmr::{clean_files_by_prefix, PMMRBackend}; | ||
| use grin_store::pmmr::PMMRBackend; | ||
| use std::cmp::Ordering; | ||
| use std::fs::{self, File}; | ||
| use std::path::{Path, PathBuf}; | ||
|
|
@@ -2056,6 +2056,61 @@ impl<'a> Extension<'a> { | |
| } | ||
| } | ||
|
|
||
| /// Remove other `txhashset_snapshot_*.zip` files in `data_dir`, keeping only | ||
| /// the snapshot for the header currently being served (`keep`). | ||
| /// | ||
| /// Snapshot zips are multi-GB; retaining more than one wastes disk and can | ||
| /// exhaust low-end nodes (see #3647). Cleanup runs on every `zip_read`, | ||
| /// including when reusing an existing zip, so stale files do not linger when | ||
| /// no new archive is created. | ||
| pub fn cleanup_old_txhashset_zips(data_dir: &Path, keep: &Path) -> u32 { | ||
| let prefix = format!("{}_", TXHASHSET_ZIP); | ||
| let keep_name = keep.file_name(); | ||
|
|
||
| let entries = match fs::read_dir(data_dir) { | ||
| Ok(entries) => entries, | ||
| Err(e) => { | ||
| debug!( | ||
| "cleanup_old_txhashset_zips: cannot read {:?}: {}", | ||
| data_dir, e | ||
| ); | ||
| return 0; | ||
| } | ||
| }; | ||
|
|
||
| let mut removed = 0u32; | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); | ||
| if !path.is_file() { | ||
| continue; | ||
| } | ||
| if keep_name.is_some() && path.file_name() == keep_name { | ||
| continue; | ||
| } | ||
| let name = match entry.file_name().into_string() { | ||
| Ok(n) => n, | ||
| Err(_) => continue, | ||
| }; | ||
| // Match `txhashset_snapshot_<hash>.zip` only. | ||
| if !name.starts_with(&prefix) || !name.ends_with(".zip") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment says .zip, but this matches any text between the prefix and .zip. Generated snapshot names use a 12-character hex hash. Could we restrict cleanup to that exact format? |
||
| continue; | ||
| } | ||
| match fs::remove_file(&path) { | ||
| Ok(()) => { | ||
| debug!("cleanup_old_txhashset_zips: removed {:?}", path); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. zip_read() already logs the total number removed. Could this per file message be trace! or be dropped to avoid duplicate debug output during a rollover? |
||
| removed += 1; | ||
| } | ||
| Err(e) => { | ||
| warn!( | ||
| "cleanup_old_txhashset_zips: failed to remove {:?}: {}", | ||
| path, e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| removed | ||
| } | ||
|
|
||
| /// Packages the txhashset data files into a zip and returns a Read to the | ||
| /// resulting file | ||
| pub fn zip_read(root_dir: String, header: &BlockHeader) -> Result<File, Error> { | ||
|
|
@@ -2064,28 +2119,25 @@ pub fn zip_read(root_dir: String, header: &BlockHeader) -> Result<File, Error> { | |
| let txhashset_path = Path::new(&root_dir).join(TXHASHSET_SUBDIR); | ||
| let zip_path = Path::new(&root_dir).join(txhashset_zip); | ||
|
|
||
| // Always free disk: keep at most the zip for this header. | ||
| let n = cleanup_old_txhashset_zips(Path::new(&root_dir), &zip_path); | ||
| if n > 0 { | ||
| debug!( | ||
| "zip_read: cleaned up {} old txhashset zip file(s) in {:?}", | ||
| n, | ||
| Path::new(&root_dir) | ||
| ); | ||
| } | ||
|
|
||
| // if file exist, just re-use it | ||
| let zip_file = File::open(zip_path.clone()); | ||
| if let Ok(zip) = zip_file { | ||
| if let Ok(zip) = File::open(zip_path.clone()) { | ||
| debug!( | ||
| "zip_read: {} at {}: reusing existing zip file: {:?}", | ||
| header.hash(), | ||
| header.height, | ||
| zip_path | ||
| ); | ||
| return Ok(zip); | ||
| } else { | ||
| // clean up old zips. | ||
| // Theoretically, we only need clean-up those zip files older than STATE_SYNC_THRESHOLD. | ||
| // But practically, these zip files are not small ones, we just keep the zips in last 24 hours | ||
| let data_dir = Path::new(&root_dir); | ||
| let pattern = format!("{}_", TXHASHSET_ZIP); | ||
| if let Ok(n) = clean_files_by_prefix(data_dir, &pattern, 24 * 60 * 60) { | ||
| debug!( | ||
| "{} zip files have been clean up in folder: {:?}", | ||
| n, data_dir | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // otherwise, create the zip archive | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,3 +115,37 @@ fn test_unexpected_zip() { | |
| // Cleanup chain directory | ||
| clean_output_dir(&db_root); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_cleanup_old_txhashset_zips() { | ||
| let db_root = Path::new(".grin_txhashset_zip_cleanup"); | ||
| let _ = fs::remove_dir_all(db_root); | ||
| fs::create_dir_all(db_root).unwrap(); | ||
|
|
||
| let keep = db_root.join("txhashset_snapshot_keepme.zip"); | ||
| let old_a = db_root.join("txhashset_snapshot_aaaa.zip"); | ||
| let old_b = db_root.join("txhashset_snapshot_bbbb.zip"); | ||
| let unrelated = db_root.join("other_file.bin"); | ||
| let not_zip = db_root.join("txhashset_snapshot_cccc.txt"); | ||
|
|
||
| File::create(&keep).unwrap(); | ||
| File::create(&old_a).unwrap(); | ||
| File::create(&old_b).unwrap(); | ||
| File::create(&unrelated).unwrap(); | ||
| File::create(¬_zip).unwrap(); | ||
|
|
||
| let removed = txhashset::cleanup_old_txhashset_zips(db_root, &keep); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this also exercise zip_read() with the current archive already present and one stale archive beside it? The original regression was specifically that the reuse path skipped cleanup, while this test only covers the helper directly. |
||
| assert_eq!(removed, 2); | ||
| assert!(keep.exists()); | ||
| assert!(!old_a.exists()); | ||
| assert!(!old_b.exists()); | ||
| assert!(unrelated.exists()); | ||
| assert!(not_zip.exists()); | ||
|
|
||
| // Second pass: only the keep file remains matching the pattern. | ||
| let removed_again = txhashset::cleanup_old_txhashset_zips(db_root, &keep); | ||
| assert_eq!(removed_again, 0); | ||
| assert!(keep.exists()); | ||
|
|
||
| let _ = fs::remove_dir_all(db_root); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we shorten this? The key detail here is that cleanup runs before both reuse and creation