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 a filter_by_type param for purge_old_bank_snapshots #31191

Merged

Conversation

xiangzhu70
Copy link
Contributor

@xiangzhu70 xiangzhu70 commented Apr 13, 2023

Problem

In #30978, we need more refined control of the snapshot dir purging behavior.
Before booting, we want to purge all PRE snapshot dirs.
During run time, we should purge only the POST snapshot dirs, letting AHV to process PRE snapshot dirs into POST.

Summary of Changes

Add the filter_by_type param. Call purging by type.

Fixes #

@xiangzhu70
Copy link
Contributor Author

Debating if I should add test function for purge_old_bank_snapshots or it would be an overkill.

@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #31191 (7b43617) into master (667fad6) will increase coverage by 0.0%.
The diff coverage is 95.4%.

@@           Coverage Diff           @@
##           master   #31191   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         729      729           
  Lines      206560   206580   +20     
=======================================
+ Hits       168402   168441   +39     
+ Misses      38158    38139   -19     

@xiangzhu70 xiangzhu70 marked this pull request as ready for review April 13, 2023 22:25
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Also, with new public functions/functionality, can you ensure there is test coverage?

@xiangzhu70 xiangzhu70 changed the title Add a type_select param for purge_old_bank_snapshots Add a filter_by_type param for purge_old_bank_snapshots Apr 14, 2023
@xiangzhu70 xiangzhu70 force-pushed the purge_pre_state_snapshot_dirs branch from 27dce4d to e47b196 Compare April 14, 2023 23:11
@xiangzhu70
Copy link
Contributor Author

Adding test_purge_old_bank_snapshots()
Did a rebasing to pull in test_bank_from_latest_snapshot_dir() so that I can factor the common test snapshot generation code.

runtime/src/snapshot_utils.rs Outdated Show resolved Hide resolved
runtime/src/snapshot_utils.rs Outdated Show resolved Hide resolved
runtime/src/snapshot_utils.rs Outdated Show resolved Hide resolved
@xiangzhu70
Copy link
Contributor Author

Created a function create_snapshot_dirs_for_tests to refactor the common code for creating test snapshot dirs in any numbers of PRE or POST states.
#31223

@xiangzhu70 xiangzhu70 force-pushed the purge_pre_state_snapshot_dirs branch from 914ce95 to 877553a Compare April 17, 2023 21:23
@xiangzhu70
Copy link
Contributor Author

xiangzhu70 commented Apr 17, 2023

Rebased to take in changes from #31216 and #31223

I will wait for the CI tests to finish before committing it. But I think this PR is ready for a final review now.

@xiangzhu70 xiangzhu70 requested a review from brooksprumo April 17, 2023 21:35
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

lgtm 😻

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 - nice separating out the testing helper fn

@xiangzhu70 xiangzhu70 merged commit e74bc4e into solana-labs:master Apr 17, 2023
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.

3 participants