Clean up old account snapshot directories to avoid the file existing hardlink error #30426
Conversation
runtime/src/snapshot_utils.rs
Outdated
| for account_path in account_paths { | ||
| let account_snapshot_path = account_path | ||
| .parent() | ||
| .ok_or(SnapshotError::InvalidAppendVecPath(account_path.clone()))? |
There was a problem hiding this comment.
How about SnapshotError::IoWithSourceAndFile instead? If we can't get parent() here, it doesn't feel like an AppendVec path issue. It'd be because the account_path here isn't within run/, right (basically)?
There was a problem hiding this comment.
Agree that it is probably better to use SnapshotError::IoWithSourceAndFile here. I doubt we'll actually see this though - parent() would only return None if the account path was root or an empty string here.
There was a problem hiding this comment.
IoWithSourceAndFile need an IO error.
#[error("source({1}) - I/O error: {0}, file: {2}")]
IoWithSourceAndFile(#[source] std::io::Error, &'static str, PathBuf),
There was a problem hiding this comment.
It is not an IO error. There is no std::io::Error supplied.
There was a problem hiding this comment.
As discussed offline, added a new error InvalidAccountPath
There was a problem hiding this comment.
lgtm with minor suggestion + second of @brooksprumo comment
runtime/src/snapshot_utils.rs
Outdated
| for account_path in account_paths { | ||
| let account_snapshot_path = account_path | ||
| .parent() | ||
| .ok_or(SnapshotError::InvalidAppendVecPath(account_path.clone()))? |
There was a problem hiding this comment.
Agree that it is probably better to use SnapshotError::IoWithSourceAndFile here. I doubt we'll actually see this though - parent() would only return None if the account path was root or an empty string here.
…hardlink error (solana-labs#30426) * Fix the hardlink failure * minor comment cleanup * use ? and slot_str * &slot_str * Add InvalidAccountPath
Problem
hard-link appendvec file failed. Error: File exists (os error 17)
When generating account states for a snapshot, the accouts files are hardlinked to snapshot directories. This requires the hardlink destinations are clean. When generating a new bank snapshot, we first clean up these directories. The existing code uses the existence of the bank snapshot directory to judge if all related directories are cleaned up. This turned out not be solid.
The bug happens here is in the situation that the bank snapshot directory does not exist (so remove_bank_snapshot is not called to clean up all related directories), but the related account snapshot hardlink directories still exist, causing the hardlink failure later on with the error "file exists".
It looks like there is code somewhere in the unarchiving process which removes the bank snapshot directory directly (not calling remove_bank_snapshot()), so the corresponding snapshot hardlink directories under <account_path> are not removed.
So, it is not solid to skip cleaning up account snapshot directories when the bank snapshot directory does not exist.
Summary of Changes
Check and clean up the account snapshot hardlink directories even when the bank snapshot directory does not exist.
This is in the group of the PRs split from #28745, and is fixing the bug generated by #29496.
Fixes #
hard-link appendvec file failure. Error: File exists (os error 17)