Checks if bank snapshot is loadable before fastbooting #343

brooksprumo · 2024-03-20T19:11:13Z

Problem

Summary of Changes

Before loading a bank snapshot for fastboot, ensure it can be used! This means that if we do fastboot the bank snapshot, it has the correct other stuff on disk (aka the right full snapshot archive) so that subsequent snapshots will succeed, and we won't hit the error described in the problem.

Fixes solana-labs#35367

codecov-commenter · 2024-03-20T21:10:57Z

Codecov Report

Attention: Patch coverage is 68.13187% with 58 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (0e932c7) to head (796f5d1).
Report is 38 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master     #343    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         837      837            
  Lines      226830   226979   +149     
========================================
+ Hits       185856   186010   +154     
+ Misses      40974    40969     -5

jeffwashington · 2024-03-20T23:50:34Z

core/src/accounts_hash_verifier.rs

+                _ => accounts_package.slot,
+            };
+            snapshot_utils::write_fastboot_file(&snapshot_info.bank_snapshot_dir, fastboot_slot)
+                .unwrap();


Oops, missed this one. Let me fix that.

Done. Fixed in 3c2e77a.

jeffwashington · 2024-03-20T23:51:54Z

ledger/src/bank_forks_utils.rs

+        let Some(bank_snapshot) = latest_bank_snapshot else {
+            // If we do *not* have a local snapshot to fastboot, then it must be because there was
+            // *not* a loadable local snapshot AND we shall *not* startup from a snapshot archive.
+            assert_eq!(


what's going on here?

Yep, a little hairy.

If we get inside this else of the let-else, then we are in a condition where (1) we're supposed to fastboot, and (2) there is no local state (i.e. a bank snapshot) to fastboot from. This should only happen if we've specified to never use a snapshot archive. So we assert that here.

If instead we used either when-newest or always for when to load from snapshot archives, then we should not be in this else of the outer if-else. Instead we should've set will_startup_from_snapshot_archives to true, since latest_bank_snapshot would've been None.

I agree with Jeff here, it seems odd to me. We are basically assigning a bool, then having an assert we're assigning it correctly?

Line 250:

let will_startup_from_snapshot_archives = match process_options.use_snapshot_archives_at_startup { UseSnapshotArchivesAtStartup::Always => true, UseSnapshotArchivesAtStartup::Never => false, UseSnapshotArchivesAtStartup::WhenNewest => latest_bank_snapshot .as_ref() .map(|bank_snapshot| latest_snapshot_archive_slot > bank_snapshot.slot) .unwrap_or(true), };

let bank = if will_startup_from_snapshot_archives { // ...stuff } else { // fastboot from local state let Some(bank_snapshot) = latest_bank_snapshot else { // If we do *not* have a local snapshot to fastboot, then it must be because there was // *not* a loadable local snapshot AND we shall *not* startup from a snapshot archive. assert_eq!( process_options.use_snapshot_archives_at_startup, UseSnapshotArchivesAtStartup::Never, ); return Err(BankForksUtilsError::NoBankSnapshotDirectory { flag: use_snapshot_archives_at_startup::cli::LONG_ARG.to_string(), value: UseSnapshotArchivesAtStartup::Never.to_string(), }); }; }

Effectively yes 😅 The assert is there to ensure we don't refactor something and cause the returned error message to be wrong/misleading.

(I do like your suggestion here #343 (review) too, so likely will go in that direction.)

This has been refactored. Please refer to #343 (comment).

apfitzge

I think it makes sense but some of the naming could be more explicit.

I think we might do a small refactor to make some of this more clear. We have this odd pattern where we are getting highest loadable snapshot, doing match on how we start up, then an if based on the result of the previous match.

Can we do something like this?

    let fastboot_snapshot = match process_options.use_snapshot_archives_at_startup {
        UseSnapshotArchivesAtStartup::Always => None,
        UseSnapshotArchivesAtStartup::Never => {
            let fastboot_snapshot =
                snapshot_utils::get_highest_loadable_bank_snapshot(snapshot_config);
            if fastboot_snapshot.is_none() {
                return Err(BankForksUtilsError::NoBankSnapshotDirectory {
                    flag: use_snapshot_archives_at_startup::cli::LONG_ARG.to_string(),
                    value: UseSnapshotArchivesAtStartup::Never.to_string(),
                });
            }
            fastboot_snapshot
        }
        UseSnapshotArchivesAtStartup::WhenNewest => {
            snapshot_utils::get_highest_loadable_bank_snapshot(snapshot_config)
                .filter(|bank_snapshot| bank_snapshot.slot >= latest_snapshot_archive_slot)
        }
    };

    if let Some(fastboot_snapshot) = fastboot_snapshot {
        // fast boot code
    } else {
        // slow boot code
    }

The get_highest_loadable_bank_snapshot call is in multiple places, but we treat it differently because the behavior is different for those cases.
The error handling is more upfront if we cannot find it for Never case, and don't have this (match, if, match) thing going on.
At least to me something like this is more clear, wdyt?

apfitzge · 2024-03-21T21:53:09Z

core/src/accounts_hash_verifier.rs

@@ -303,6 +311,20 @@ impl AccountsHashVerifier {
                &accounts_hash_for_reserialize,
                bank_incremental_snapshot_persistence.as_ref(),
            );
+
+            // now write the fastboot file after reserializing so this bank snapshot is loadable
+            let fastboot_slot = match accounts_package.package_kind {


Could we rename this? It's more like "fastboot_most_recent_fullsnapshot_slot" or "fastboot_base_slot".

Same with the "fastboot_file". It seems like it should be "fastboot_base_slot_file".
Current naming made me think it contained the slot we were fastbooting from.

Good suggestion; will do.

For the fastboot file, do you want the function renamed, the comment reworded, or the file itself renamed?

Renaming fastboot_slot has been fixed in 2e351f3.

Sorry for not being more clear. I was hoping to rename the file and whole "concept" of this file.
We're using it for fastboot, sure, but the file itself contains the slot of the full snapshot this snapshot is based on. Seems like that's potentially useful info even without fastboot, so giving it a more clear name everywhere (file name, function names, variable names) would make it more obvious what information is available there.

Got it. Yeah, that makes sense. The file could be "full_snapshot_archive_slot" instead. I was purposely going the opposite way; being ambiguous about what's inside the file, but indicating that it is used for fastboot. Renaming the file would be clear about the contents of the file, but ambiguous about usage.

I think for the sake of any forward compatibility/breaking changes, I'll go with the "full_snapshot_archive_slot" idea. This is unlikely to change. If we need more/different information later, we'll create another file.

Does that sound ok?

Sounds good, thanks @brooksprumo!

Done. Renamed in 796f5d1.

apfitzge · 2024-03-21T22:02:23Z

ledger/src/bank_forks_utils.rs

+        let Some(bank_snapshot) = latest_bank_snapshot else {
+            // If we do *not* have a local snapshot to fastboot, then it must be because there was
+            // *not* a loadable local snapshot AND we shall *not* startup from a snapshot archive.
+            assert_eq!(


I agree with Jeff here, it seems odd to me. We are basically assigning a bool, then having an assert we're assigning it correctly?

Line 250:

let will_startup_from_snapshot_archives = match process_options.use_snapshot_archives_at_startup { UseSnapshotArchivesAtStartup::Always => true, UseSnapshotArchivesAtStartup::Never => false, UseSnapshotArchivesAtStartup::WhenNewest => latest_bank_snapshot .as_ref() .map(|bank_snapshot| latest_snapshot_archive_slot > bank_snapshot.slot) .unwrap_or(true), };

let bank = if will_startup_from_snapshot_archives { // ...stuff } else { // fastboot from local state let Some(bank_snapshot) = latest_bank_snapshot else { // If we do *not* have a local snapshot to fastboot, then it must be because there was // *not* a loadable local snapshot AND we shall *not* startup from a snapshot archive. assert_eq!( process_options.use_snapshot_archives_at_startup, UseSnapshotArchivesAtStartup::Never, ); return Err(BankForksUtilsError::NoBankSnapshotDirectory { flag: use_snapshot_archives_at_startup::cli::LONG_ARG.to_string(), value: UseSnapshotArchivesAtStartup::Never.to_string(), }); }; }

runtime/src/snapshot_utils.rs

ledger/src/bank_forks_utils.rs

brooksprumo · 2024-03-22T15:11:09Z

I think we might do a small refactor to make some of this more clear. [..]
Can we do something like this? [..]

Done! First ff1867c swaps the if-else block (to make the next diff easier). And second, 8b80597 does the refactor.

apfitzge

Think the refactor made it a lot clearer, thanks! A couple more comments/questions

core/src/accounts_hash_verifier.rs

apfitzge · 2024-03-22T17:36:52Z

core/src/accounts_hash_verifier.rs

@@ -303,6 +311,20 @@ impl AccountsHashVerifier {
                &accounts_hash_for_reserialize,
                bank_incremental_snapshot_persistence.as_ref(),
            );
+
+            // now write the fastboot file after reserializing so this bank snapshot is loadable
+            let fastboot_slot = match accounts_package.package_kind {


Sorry for not being more clear. I was hoping to rename the file and whole "concept" of this file.
We're using it for fastboot, sure, but the file itself contains the slot of the full snapshot this snapshot is based on. Seems like that's potentially useful info even without fastboot, so giving it a more clear name everywhere (file name, function names, variable names) would make it more obvious what information is available there.

apfitzge · 2024-03-26T19:05:36Z

runtime/src/snapshot_utils.rs

+/// To be loadable, the bank snapshot must be a BankSnapshotKind::Post.
+/// And if we're generating snapshots (e.g. running a normal validator), then
+/// the full snapshot file's slot must match the highest full snapshot archive's.
+pub fn get_highest_loadable_bank_snapshot(


Had a separate discussion with @brooksprumo about this function.
There is an edge-case that is not handled as efficiently as we'd like, but we decided to leave it as a follow-up since this PR covers the majority of cases and already has a lot going on.

The edge case is when we have a higher full snapshot archive than our highest snapshot. This can happen if ledger-tool is used to create a full snapshot archive - this does not create a snapshot.

This function could likely be improved so that we only check the existence of the full snapshot archive matching the full_snapshot_file_slot, even if it not the latest full snapshot archive. However, it requires changes elsewhere in ABS (others?) to gate cleaning properly.

Good find! I'll handle this in a follow-up PR.

apfitzge

Happy with changes as they are, as mentioned in my comment, it covers most of the cases and we are leaving an edge-case as follow-up work.

Checks if bank snapshot is loadable before fastbooting

c397759

brooksprumo self-assigned this Mar 20, 2024

brooksprumo marked this pull request as ready for review March 20, 2024 21:13

brooksprumo requested review from apfitzge and jeffwashington March 20, 2024 21:13

jeffwashington reviewed Mar 20, 2024

View reviewed changes

pr: do not use unwrap

3c2e77a

brooksprumo requested a review from jeffwashington March 21, 2024 02:18

apfitzge requested changes Mar 21, 2024

View reviewed changes

brooksprumo added 2 commits March 22, 2024 10:32

pr: swap the if-else for will_startup_from_snapshot_archives

ff1867c

pr: refactor how to get fastboot snapshot vs snapshot archive

8b80597

pr: rename fastboot_slot

2e351f3

brooksprumo requested a review from apfitzge March 22, 2024 15:51

apfitzge reviewed Mar 22, 2024

View reviewed changes

brooksprumo requested a review from apfitzge March 22, 2024 17:46

pr: rename the fastboot file to full snapshot slot file

796f5d1

apfitzge reviewed Mar 26, 2024

View reviewed changes

apfitzge approved these changes Mar 26, 2024

View reviewed changes

brooksprumo merged commit 182d27f into anza-xyz:master Mar 28, 2024
37 checks passed

brooksprumo deleted the fastboot/missing-full-snapshot-hash branch March 28, 2024 15:14

brooksprumo mentioned this pull request Mar 28, 2024

Do not purge all bank snapshots after fastboot #345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checks if bank snapshot is loadable before fastbooting #343

Checks if bank snapshot is loadable before fastbooting #343

brooksprumo commented Mar 20, 2024

codecov-commenter commented Mar 20, 2024 •

edited

Loading

jeffwashington Mar 20, 2024

brooksprumo Mar 21, 2024

brooksprumo Mar 21, 2024

jeffwashington Mar 20, 2024

brooksprumo Mar 21, 2024

apfitzge Mar 21, 2024

brooksprumo Mar 22, 2024

brooksprumo Mar 23, 2024

apfitzge left a comment

apfitzge Mar 21, 2024

brooksprumo Mar 22, 2024

brooksprumo Mar 22, 2024

brooksprumo Mar 22, 2024

apfitzge Mar 22, 2024

brooksprumo Mar 22, 2024

apfitzge Mar 22, 2024

brooksprumo Mar 23, 2024

apfitzge Mar 21, 2024

brooksprumo commented Mar 22, 2024

apfitzge left a comment

apfitzge Mar 22, 2024

apfitzge Mar 26, 2024

brooksprumo Mar 26, 2024

apfitzge left a comment

Checks if bank snapshot is loadable before fastbooting #343

Checks if bank snapshot is loadable before fastbooting #343

Conversation

brooksprumo commented Mar 20, 2024

Problem

Summary of Changes

codecov-commenter commented Mar 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brooksprumo commented Mar 22, 2024

apfitzge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 20, 2024 •

edited

Loading