Skip to content

Archives storages directly#503

Merged
mergify[bot] merged 1 commit intoanza-xyz:masterfrom
brooksprumo:snapshot/archive
Apr 2, 2024
Merged

Archives storages directly#503
mergify[bot] merged 1 commit intoanza-xyz:masterfrom
brooksprumo:snapshot/archive

Conversation

@brooksprumo
Copy link
Copy Markdown

Problem

When archiving a snapshot, we need to add all the storages to the archive. Currently, we flush each file and then have the archiver load the file from disk.

Since the storages are all resident/in-sync with the validator process, we know they are all up to date. Also, we already flushed all the storages to disk in AccountsBackgroundService when calling add_bank_snapshot(), so flushing again is unnecessary and negatively impacts the rest of the system1.

Summary of Changes

Instead of flushing to disk first, get the storages data directly from the mmaps and add it to the archive.

Additional Testing

Running ledger-tool create-snapshot on mnb showed that this PR does not impact how long it takes to archive a snapshot.

master:

solana_runtime::snapshot_utils] Successfully created /home/sol/ledger/snapshot-257173898-5hquoxg3NyjYVnX86x79VeS5KHBAwW5viLBiVbjsYZXe.tar.zst. slot: 257173898, elapsed ms: 1181220, size: 67725864176

this pr:

solana_runtime::snapshot_utils] Successfully created /home/sol/ledger/snapshot-257173898-5hquoxg3NyjYVnX86x79VeS5KHBAwW5viLBiVbjsYZXe.tar.zst. slot: 257173898, elapsed ms: 1181327, size: 67722885806

I also tested loading from this new snapshot created with this PR to ensure it actually works properly. Happy to report that was successful too.

Footnotes

  1. Quantifying the negative impacts is something that I believe @alessandrod has done/shared.

@brooksprumo brooksprumo self-assigned this Mar 29, 2024
@brooksprumo brooksprumo marked this pull request as ready for review March 29, 2024 19:24
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

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

Project coverage is 81.8%. Comparing base (b1e1799) to head (10bea1b).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #503   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         842      842           
  Lines      228462   228460    -2     
=======================================
  Hits       187084   187084           
+ Misses      41378    41376    -2     

@jeffwashington
Copy link
Copy Markdown

I don't understand this at first glance. we need to zoom to explain ;-)

jeffwashington
jeffwashington previously approved these changes Apr 2, 2024
Copy link
Copy Markdown

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm
Walked through on zoom with brooks and haoran.

HaoranYi
HaoranYi previously approved these changes Apr 2, 2024
Copy link
Copy Markdown

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo brooksprumo dismissed stale reviews from HaoranYi and jeffwashington via f3b90ae April 2, 2024 17:01
@jeffwashington jeffwashington self-requested a review April 2, 2024 17:01
Copy link
Copy Markdown

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo
Copy link
Copy Markdown
Author

Rebased and force-pushed to resolve merge conflicts. Nothing about the PR itself was changed.

@brooksprumo brooksprumo added the automerge automerge Merge this Pull Request automatically once CI passes label Apr 2, 2024
@mergify mergify Bot merged commit 4247a8a into anza-xyz:master Apr 2, 2024
@brooksprumo brooksprumo deleted the snapshot/archive branch April 2, 2024 19:42
@steviez steviez added the v1.18 label May 20, 2024
@mergify
Copy link
Copy Markdown

mergify Bot commented May 20, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify Bot pushed a commit that referenced this pull request May 20, 2024
(cherry picked from commit 4247a8a)

# Conflicts:
#	accounts-db/src/accounts_file.rs
#	accounts-db/src/tiered_storage/hot.rs
#	accounts-db/src/tiered_storage/readable.rs
#	runtime/src/snapshot_utils.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge automerge Merge this Pull Request automatically once CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants