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

Create-snapshot check if slot is available on startup #25329

Conversation

apfitzge
Copy link
Contributor

Problem

create-snapshot will load bank_forks from a snapshot and try to replay up to snapshot_slot. If snapshot_slot does not exist, it will replay everything available and then fail. This can be slow.

Summary of Changes

Add a check to see if snapshot_slot is available in the blockstore BEFORE trying to load the bank_forks. If it's not we exit since we won't be able to create a snapshot.

Preliminary PR for minimized snapshot mode.

Fixes #

@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #25329 (bd00d86) into master (69a0ff9) will increase coverage by 0.0%.
The diff coverage is 66.9%.

@@            Coverage Diff            @@
##           master   #25329     +/-   ##
=========================================
  Coverage    82.0%    82.1%             
=========================================
  Files         598      614     +16     
  Lines      165882   169323   +3441     
=========================================
+ Hits       136125   139029   +2904     
- Misses      29757    30294    +537     

@apfitzge apfitzge marked this pull request as ready for review May 18, 2022 17:51
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

I like the thinking behind adding this check; however, I think we should go even further and check to make sure we have full slots all the way up to (and including) snapshot_slot. It may not be super common to have "gaps" on a validator (although there are instances where we purge slots), a common case is copying a ledger contents (shreds + snapshot) off of validator onto some other machine to debug. Checking the full range would be good to prevent human error in this scenario.

This would definitely add some overhead, but my gut says that in relation to the amount of time it takes to verify and create the snapshot, this extra overhead should be relatively small

@@ -2332,6 +2332,11 @@ fn main() {
value_t_or_exit!(arg_matches, "snapshot_slot", Slot)
};

if blockstore.meta(snapshot_slot).unwrap().is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to confirming we have a meta, we should make sure that the slot is also full with SlotMeta::is_full()

@apfitzge
Copy link
Contributor Author

apfitzge commented May 18, 2022

I think we should go even further and check to make sure we have full slots all the way up to (and including) snapshot_slot.

I think that's reasonable. We'd need to check the slots in the range old_snapshot_slot..=new_snapshot_slot, where old_snapshot_slot is the slot of the snapshot we loaded from. Is that right?

And if we are using the --no-snapshot option, it'd be from 0..=new_snapshot_slot.

@steviez
Copy link
Contributor

steviez commented May 18, 2022

I think that's reasonable. We'd need to check the slots in the range old_snapshot_slot..=new_snapshot_slot, where old_snapshot_slot is the slot of the snapshot we loaded from. Is that right?

Correct; I think there is some logic in load_bank_forks that you could either replicate or break into a sub-function to get highest available snapshot slot.

And if we are using the --no-snapshot option, it'd be from 0..=new_snapshot_slot.

Correct

@apfitzge
Copy link
Contributor Author

While testing this change, I found a bug w/ the --no-snapshot option in ledger-tool due to the recent addition of --snapshot-archive-format. Opened a PR here: #25368

@apfitzge apfitzge requested a review from steviez May 19, 2022 17:32
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
steviez
steviez previously approved these changes May 19, 2022
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

LGTM assuming you've done some testing (including cases where slots was not full and/or missing)

ledger-tool/src/main.rs Outdated Show resolved Hide resolved
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed steviez’s stale review May 19, 2022 19:57

Pull request has been modified.

@apfitzge apfitzge merged commit 1e7d877 into solana-labs:master May 20, 2022
@apfitzge apfitzge deleted the feature/ledger-tool-create-snapshot-check-slot branch May 20, 2022 14:11
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
)

* Add a check during ledger-tool create-snapshot startup to see if the snapshot slot is available

* check all slots from the start to snapshot_slot during load_bank_forks

* unwrap_or_default incremental snapshot slot before comparison

* Improve error messages on missing or not full slots
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
)

* Add a check during ledger-tool create-snapshot startup to see if the snapshot slot is available

* check all slots from the start to snapshot_slot during load_bank_forks

* unwrap_or_default incremental snapshot slot before comparison

* Improve error messages on missing or not full slots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants