Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Add incremental snapshot utils #18504

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Jul 7, 2021

This commit adds high-level functions (bank_to_incremental_snapshot() and bank_from_incremental_snapshot()), and additionally
refactors snapshot_utils and serde_snapshot to use a common code path for
snapshots. New tests were added to snapshot_utils for incremental snapshots.

This PR grabs bits from PR #17875, and incorporated feedback from that PR review. This PR is another in the pursuit of Incremental Snapshots (#17088), and this PR finally/actually adds the ability to both load-from and store-to an incremental snapshot. Yay!

This PR is quite big. A lot of the deserialization code was touched to support incremental snapshots and avoid code duplication. The diff may be easier to read when ignoring whitespace.

The next PR in this series will add new integration-style tests to core/tests/, and will address new issues those tests uncover (likely clean_accounts()).

Additional Bits

Care has been taken to rename (almost) everything around snapshots.

  • Wherever specific, all variables and functions will be full_ or incremental_ unless they work for both.
  • The term "snapshot" was overloaded in the code; sometimes called a bank snapshot, sometimes a slot snapshot, and sometimes referring to the snapshot package/snapshot archive. Outside of Bank, it is now a "bank snapshot".

Outstanding Questions

  • Naming: Should it be "snapshot" and "incremental snapshot" , or "full snapshot" and "incremental snapshot" There's quite a bit of existing non-code that uses just "snapshot"; should that be updated too? More discussion here
    • resolved by renaming
  • Assert vs error when storages overlap. Thread 1 here, thread 2 here, and comment 3 here
    • resolved by error-ing instead of panic-ing
  • Should I remove deserialize_snapshot_data_file_capped()? Thread here
  • Should I create another struct for common parameters? Thread here

New Issues Created

@brooksprumo brooksprumo force-pushed the iss-v3-snapshot-utils branch from b82ec32 to 5ebbc4a Compare July 8, 2021 16:08
@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #18504 (a83e0b2) into master (dfde1de) will decrease coverage by 0.0%.
The diff coverage is 87.8%.

@@            Coverage Diff            @@
##           master   #18504     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         443      443             
  Lines      125343   125957    +614     
=========================================
+ Hits       103895   104389    +494     
- Misses      21448    21568    +120     

@brooksprumo brooksprumo marked this pull request as ready for review July 8, 2021 18:12
@brooksprumo brooksprumo requested review from carllin and lijunwangs July 8, 2021 18:12
@brooksprumo brooksprumo force-pushed the iss-v3-snapshot-utils branch from 5ebbc4a to 4f45db6 Compare July 12, 2021 15:25
@brooksprumo brooksprumo requested a review from ryoqun July 19, 2021 22:27
@brooksprumo
Copy link
Contributor Author

@lijunwangs @ryoqun @carllin Checks have passed, and all review comments have been addressed. I've replied with answers/questions to any outstanding review comments. PR is ready for another round of reviewing. Thanks in advance!

@ryoqun
Copy link
Contributor

ryoqun commented Jul 20, 2021

@brooksprumo as a context, we want to support untrusted snapshots eventually in the (distant) future. so, panic! shouldn't be reachable when consuming (= untaring) any downloaded snapshots. #7167

That context might help to decide panic!/error?

Also, well-groomed pr description. :)

@brooksprumo
Copy link
Contributor Author

@brooksprumo as a context, we want to support untrusted snapshots eventually in the (distant) future. so, panic! shouldn't be reachable when consuming (= untaring) any downloaded snapshots. #7167

That context might help to decide panic!/error?

@ryoqun Yes, thank you. Makes sense!

Also, well-groomed pr description. :)

Thanks :)

@brooksprumo brooksprumo changed the title Add bank_to_incremental_snapshot() and bank_from_incremental_snapshot() Add incremental snapshot utils Jul 20, 2021
lijunwangs
lijunwangs previously approved these changes Jul 21, 2021
Copy link
Contributor

@lijunwangs lijunwangs left a comment

Choose a reason for hiding this comment

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

Looks good to me.

carllin
carllin previously approved these changes Jul 21, 2021
@brooksprumo brooksprumo force-pushed the iss-v3-snapshot-utils branch from d048750 to 4cfdeac Compare July 21, 2021 21:38
@mergify mergify bot dismissed stale reviews from lijunwangs and carllin July 21, 2021 21:39

Pull request has been modified.

This commit adds high-level functions for creating and loading-from
incremental snapshots, plus all low-level functions required to perform
those tasks.  This commit **does not** add taking incremental snapshots
as part of a running validator, nor starting up a node with an
incremental snapshot; just laying ground work.

Additionally, `snapshot_utils` and `serde_snapshot` have been
refactored to use a common code paths for the different snapshots.

Also of note, some renaming has happened:
  1. Snapshots are now either `full_` or `incremental_` throughout the
     codebase.  If not specified, the code applies to both.
  2. Bank snapshots now are called "bank snapshots" everywhere too
     (before they were called "slot snapshots", "bank snapshots", or
      just "snapshots").  The one exception is within `Bank`, where they
     are still just "snapshots", because they are already "bank
     snapshots".
  3. Snapshot archives now have `_archive` everywhere in the code.  This
     should clear up an ambiguity between bank snapshots and snapshot
     archives.
@brooksprumo
Copy link
Contributor Author

@carllin @lijunwangs I had to fix a merge conflict, so your approvals were removed. Nothing else changed, but let me know if you'd like me to wait for you to re-review before I merge.

@ryoqun Would you like to re-review before I merge?

ryoqun
ryoqun previously approved these changes Jul 22, 2021
Copy link
Contributor

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

I'd say a masterpiece of plumbing pr ever. xD

I only left some minor comments. merge anytime without needing another review round.

@mergify mergify bot dismissed ryoqun’s stale review July 22, 2021 17:43

Pull request has been modified.

@brooksprumo brooksprumo merged commit d1debcd into solana-labs:master Jul 22, 2021
@brooksprumo brooksprumo deleted the iss-v3-snapshot-utils branch July 22, 2021 19:40
brooksprumo added a commit that referenced this pull request Jul 29, 2021
This commit builds on PR #18504 by adding a test to core/tests/snapshot.rs for Incremental Snapshots. The test adds banks to bank forks in a loop and takes both full snapshots and incremental snapshots at intervals, and validates they are rebuild-able.

For background info about Incremental Snapshots, see #17088.

Fixes #18829 and #18972
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Incremental Snapshots] Add basic functionality to create and load-from incremental snapshots
4 participants