Skip to content
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

Add remove_incomplete_bank_snapshot_dir #31131

Merged
merged 6 commits into from
Apr 12, 2023

Conversation

xiangzhu70
Copy link
Contributor

@xiangzhu70 xiangzhu70 commented Apr 10, 2023

Problem

Creating a bank snapshot dir takes multiple steps. If the process is killed in the middle of the process, the directory is incomplete, not good for constructing a bank.
There is already a flag file "state_complete" to guard against it, so that a snapshot dir missing that flag does to get selected to construct a BankSnapshotInfo. But then the new design will purge the bank snaphosts based on the snapshot state, not by the directory. That could lead to incomplete dir being left on disk forever.

Summary of Changes

Remove incomplete snapshot dir when found.

Fixes #

@xiangzhu70
Copy link
Contributor Author

Thinking again, removing the incomplete dir in-place is much simpler, and achieving the same result. Hence 4d814bc.

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #31131 (b963775) into master (9a7b6ab) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #31131   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         728      728           
  Lines      205767   205771    +4     
=======================================
+ Hits       167800   167827   +27     
+ Misses      37967    37944   -23     

@xiangzhu70 xiangzhu70 marked this pull request as ready for review April 11, 2023 19:16
@@ -204,6 +204,7 @@ impl BankSnapshotInfo {
// filled. Check the completion to avoid using a highest found slot directory with missing content.
let completion_flag_file = bank_snapshot_dir.join(SNAPSHOT_STATE_COMPLETE_FILENAME);
if !fs::metadata(&completion_flag_file)?.is_file() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I probably missed this in a previous review, but is this line correct?
If the completion_flag_file does not exist, then fs::metadata returns an Err which is caught in the ?.

We only enter this if, if that path exists but is not a file. I don't think it mattered much before the change in this PR, but I think we ought to fix it here.

Might do something like:

    match fs::metadata(&completion_flag_file) {
        Ok(metadata) if metadata.is_file() => {}, // Directory correctly marked as complete
        _ => {
            ... // either path doesn't exist, or is not a file
        }
    }

Copy link
Contributor Author

@xiangzhu70 xiangzhu70 Apr 11, 2023

Choose a reason for hiding this comment

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

Good catch! I should have been more careful using metadata. Let me change it to the simple "if !completion_flag_file.is_file()".

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! I should have been more careful using metadata. Let me change it to the simple "if !completion_flag_file.is_file()".

Ah, yeah that's much simpler than my match suggestion!

@xiangzhu70 xiangzhu70 requested a review from apfitzge April 12, 2023 00:32
Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm

@xiangzhu70 xiangzhu70 merged commit 31784b2 into solana-labs:master Apr 12, 2023
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Apr 14, 2023
* Add remove_incomplete_bank_snapshot_dir

* Remove incomplete snapshot dir if incomplete

* Revert "Add remove_incomplete_bank_snapshot_dir"

This reverts commit aaafef7.

* Replace metadata's is_file with simple PathBuf's is_file

* In test verify the deletion of the incomplete snapshot dir

* Add comments to explain the snapshot dir cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants