AHV processes the snapshot dirs in place #30978

xiangzhu70 · 2023-03-30T03:37:18Z

Problem

For booting a bank from a snapshot dir, we need a post snapshot with hash calculated. Previously, the account package is generated with a tmp dir, and the hash was calculated for the tmp dir and packaged into the archive. The snapshot dir under snapshot/ stays at the PRE stage without the account hash.

Summary of Changes

Let account package use the snapshot dir in-place, so AHV computes the accounts hash and turns the pre snapshot dir into a post snapshot dir.

Change SnapshotPackage::snapshot_links from TempDir to PathBuf, let it point to the snapshot dir. Remove the tempdir setup.
Build archive staging directory using the snashot dir info from the updated SnapshotPackage.

Fixes #

xiangzhu70 · 2023-04-03T18:46:20Z

core/tests/snapshots.rs

@@ -471,17 +468,6 @@ fn test_concurrent_snapshot_packaging(
        }
    }

-    // Purge all the outdated snapshots, including the ones needed to generate the package


The purge function no longer purges the PRE-stage bank snapshot dirs. So this test is no longer valid. Removed it. Purging has been tested in other tests. No need to test it here.

Not in this PR any more.

codecov · 2023-04-03T20:13:23Z

Codecov Report

Merging #30978 (d750aeb) into master (466c3c3) will decrease coverage by 0.1%.
The diff coverage is 63.3%.

@@            Coverage Diff            @@
##           master   #30978     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         734      734             
  Lines      207153   207157      +4     
=========================================
- Hits       168954   168939     -15     
- Misses      38199    38218     +19

apfitzge

Some initial comments on my first pass. I will have to go over this change again, and will do so soon

apfitzge · 2023-04-04T16:51:10Z

core/src/snapshot_packager_service.rs

+        let storage_entries: Vec<_> = (0..=slot)
+            .map(|i| Arc::new(AccountStorageEntry::new(&accounts_dir, 0, i as u32, 10)))


i is an AppendVecId, the slot is 0 for all these entries.

Got it. Reverted the change here.

Not in this PR any more.

apfitzge · 2023-04-04T16:52:34Z

core/src/snapshot_packager_service.rs

+        // Create one fake snapshot.
+        let snapshots_paths: Vec<_> = (slot..=slot)


setting up the dir, then just having let snapshot_paths = vec![snapshot_path]; seems much more clear than this single-iteration loop

Updated it.

Not in this PR any more.

apfitzge · 2023-04-04T16:56:59Z

runtime/src/snapshot_utils.rs

-    do_purge(get_bank_snapshots_pre(&bank_snapshots_dir));
+    // Only purge the POST snapshot dirs.
+    // Now that the account packages use the snapshot dir in place, The AHV thread will process the PRE snapshot dirs
+    // and turn them into POST snapshot dirs, so they will be purged eventually, and there will be no leaking.


during development if there is an issue in the PRE -> POST transition time, then do the PRE files ever get cleaned up?

What can this issue be? This does have the assumption that AHV keeps running so it sooner or later advances states of the snapshot dirs from the PRE state into the POST state. Are you saying this assumption could be broken?

AHV won't keep running if the validator crashes/shutdown.

If these PRE snapshots are never cleaned up except by the PRE -> POST transition, then my directory could just keep growing, right?

Maybe I'm missing something where these are initially removed at start up or something?

Good point! There could be left-over uncleaned PRE snapshot dirs.

The highest POST snapshot dir will be used to construct a bank, so the validator starts from that bank. From then on, it loops to create new PRE snapshot dirs, which are sent to AHV to be converted to POST snapshot dirs.

The problem is with the old PRE snapshot dirs which are higher than the highest POST dirs. add_bank_snapshot has a check to remove any existing directory before setting up a new PRE dir. But the new PRE slot may not be the same as the old ones. In that case, the old PRE snapshot dirs are left in the system, never got cleaned up.

So, after we boot from a POST snapshot dir, we should clean up all the existing old PRE dirs.

after we boot from a POST snapshot dir, we should clean up all the existing old PRE dirs.

After the PR to add the function bank_from_latest_snapshot_dir is committed, I will add the PRE dir cleanup logic in that function.

After #31191 is committed, will use that to do type selective purging.

After #31191 is committed, will use that to do type selective purging.

Will do purging in #30978.

core/src/accounts_hash_verifier.rs

core/src/snapshot_packager_service.rs

brooksprumo · 2023-04-07T14:23:34Z

core/src/snapshot_packager_service.rs

@@ -394,42 +392,56 @@ mod tests {
        fs::create_dir_all(&incremental_snapshot_archives_dir).unwrap();

        fs::create_dir_all(&accounts_dir).unwrap();
+
+        let slot: Slot = 4;


It's idiomatic in Rust to elide type declarations that can be inferred by the compiler.

Please tie this value to the storage entries below to ensure they stay in sync. So the line below can be:

let storage_entries: Vec<_> = (0..=slot)

Not in this PR any more.

core/src/snapshot_packager_service.rs

apfitzge · 2023-04-07T18:51:23Z

core/src/snapshot_packager_service.rs

+        let mut fake_snapshot_file = OpenOptions::new()
+            .read(true)
+            .write(true)
+            .create(true)
+            .open(&fake_snapshot_path)
+            .unwrap();
+        fake_snapshot_file.write_all(b"Hello, world!").unwrap();
+        let fake_status_cache = snapshot_dir.join(SNAPSHOT_STATUS_CACHE_FILENAME);
+        let mut fake_status_cache_file = OpenOptions::new()
+            .read(true)
+            .write(true)
+            .create(true)
+            .open(fake_status_cache)
+            .unwrap();
+        fake_status_cache_file
+            .write_all(b"Hello, status cache!")
+            .unwrap();


I realize it was using OpenOptions before, but this seems equivalent and a lot less cluttered.

let mut fake_snapshot_file = File::create(&fake_snapshot_path).unwrap(); fake_snapshot_file.write_all(b"Hello, world!").unwrap(); let fake_status_cache = snapshot_dir.join(SNAPSHOT_STATUS_CACHE_FILENAME); let mut fake_status_cache_file = File::create(&fake_status_cache).unwrap();

Unless I am missing something that makes this different.

Not in this PR any more.

apfitzge · 2023-04-07T19:02:55Z

hey @xiangzhu70, could you have a description of the "life of a snapshot". Brooks is probably more familiar than I am, and I'm struggling to understand the reason the change is necessary.

Just like snapshot is created -> pre, when does this get transitioned to post, when do they get deleted.

xiangzhu70 · 2023-04-07T19:21:15Z

hey @xiangzhu70, could you have a description of the "life of a snapshot". Brooks is probably more familiar than I am, and I'm struggling to understand the reason the change is necessary.

Just like snapshot is created -> pre, when does this get transitioned to post, when do they get deleted.

Sure.

Here is the existing code:

ABS does the following in order:
Call add_bank_snapshot() which creates a snapshot dir (with a pre type snapshot file in it)
It creates a tmp dir with the snapshot pre file, creates an AccountsPacakge pointing to the tmp dir, sends the account package to AHV
It calls purge_old_bank_snapshots, which delete the old snapshot_dirs.

AHV:
process the received AccountsPackage, calculate the snapshot hash, and update the pre-type snapshot file in the tmp dir, to post type.
sends a snapshot package to SPS

SPS:
process the snapshot package and creates an archive.

NOTE that the snapshot type being POST only happens in the tmp dir, used for archiving. The original snapshot dir always has the PRE type, never updated, never has the hash calculated.

== Below is the change in this PR:
When ABS creates an account package, instead of creating a tmp dir, use the original snapshot dir. Consider the snapshot/<slot> dirs as having states. They are created in PRE state. AHV processes them and turn them into POST state, which have the correct hashes, and are good for constructing the banks.

apfitzge · 2023-04-07T19:59:56Z

runtime/src/snapshot_package.rs

@@ -55,7 +50,6 @@ impl AccountsPackage {
        package_type: AccountsPackageType,
        bank: &Bank,
        bank_snapshot_info: &BankSnapshotInfo,
-        bank_snapshots_dir: impl AsRef<Path>,


It looks like BankSnapshotInfo has had snapshot_dir since #29409.
Seems like removing this parameter could be split out from the other parts of this PR.

To split this parameter out, then I need to derive bank_snapshots_dir from bank_snapshot_info.snapshot_dir.parent(), and handle the parent being None error case by defining a new error. But this PR is removing the need of bank_snapshots_dir. So I am not sure if it is worth to get bank_snapshots_dir error handling to work first and then remove it.

Another way of splitting is to commit this PR first with bank_snapshots_dir kept as _bank_snapshots_dir, and then after this PR is committed, create another PR just to remove _bank_snapshots_dir and all the calling parts.

Please let me know your preference. @apfitzge

OK, I see that parent() has to be done for the snapshot_links anyway. will split it

Created PR #31249 for this.

Not in this PR any more.

apfitzge · 2023-04-07T20:09:41Z

runtime/src/snapshot_utils.rs

-    do_purge(get_bank_snapshots_pre(&bank_snapshots_dir));
+    // Only purge the POST snapshot dirs.
+    // Now that the account packages use the snapshot dir in place, The AHV thread will process the PRE snapshot dirs
+    // and turn them into POST snapshot dirs, so they will be purged eventually, and there will be no leaking.


AHV won't keep running if the validator crashes/shutdown.

If these PRE snapshots are never cleaned up except by the PRE -> POST transition, then my directory could just keep growing, right?

Maybe I'm missing something where these are initially removed at start up or something?

apfitzge · 2023-04-07T20:47:59Z

runtime/src/snapshot_package.rs

@@ -197,9 +175,12 @@ impl AccountsPackage {
    /// NOTE 2: This fn will panic if the AccountsPackage is of type EpochAccountsHash.


Cannot comment outside of line range....

NOTE 1 is now wrong. It's not within a TempDir

Removed NOTE 1

runtime/src/snapshot_package.rs

Let account pacakge use the snapshot dir, so AHV computes the accounts hash and turns the pre snapshot dir into a post snapshot dir

… snapshot dir archiving

…ank_snapshots_dir

brooksprumo · 2023-04-26T13:31:10Z

runtime/src/snapshot_package.rs


        let snapshot_info = SupplementalSnapshotInfo {
-            snapshot_links,
+            snapshot_links: bank_snapshots_dir,


In a follow-up PR (not this PR), let's rename this field to bank_snapshot_dir (no s), since we're not making any new hard/sym links for AccountsPackage.

Sure. WIll do it.

runtime/src/snapshot_utils.rs

brooksprumo

lgtm

apfitzge

I did not catch any problems while reading through, but one of the tests is now failing CI.

Fix that one and I think this is ready to ship.

xiangzhu70 · 2023-04-26T17:16:28Z

one of the tests is now failing CI.

Yes, after switching from hardlink to symlink, archive_snapshot_package() is failing. Debugging it.

xiangzhu70 mentioned this pull request Mar 30, 2023

Add arg boot-from-local-state, enable it in validator #30859

Closed

xiangzhu70 commented Apr 3, 2023

View reviewed changes

xiangzhu70 requested review from brooksprumo and apfitzge April 3, 2023 20:21

xiangzhu70 marked this pull request as ready for review April 3, 2023 20:21

apfitzge reviewed Apr 4, 2023

View reviewed changes

brooksprumo reviewed Apr 7, 2023

View reviewed changes

apfitzge self-requested a review April 7, 2023 18:38

apfitzge reviewed Apr 7, 2023

View reviewed changes

xiangzhu70 mentioned this pull request Apr 13, 2023

Add a filter_by_type param for purge_old_bank_snapshots #31191

Merged

xiangzhu70 force-pushed the account_package_use_snapshot_dir_in_place branch from 673a094 to 5883b30 Compare April 17, 2023 23:56

This was referenced Apr 18, 2023

Remove bank_snapshots_dir param #31249

Merged

Update fn create_and_verify_snapshot #31245

Merged

Purge snapshot dirs before loading from an archive #31283

Merged

xiangzhu70 force-pushed the account_package_use_snapshot_dir_in_place branch from c4204dd to 5fe622b Compare April 25, 2023 16:41

brooksprumo reviewed Apr 25, 2023

View reviewed changes

runtime/src/snapshot_package.rs Outdated Show resolved Hide resolved

xiangzhu70 added 10 commits April 25, 2023 16:03

AHV processes the snapshot dirs in place

597a91a

Let account pacakge use the snapshot dir, so AHV computes the accounts hash and turns the pre snapshot dir into a post snapshot dir

fix status cache path to maintain the archive layout for the in-place…

99552f5

… snapshot dir archiving

fix test_package_snapshots

a3d17bd

Fix test_concurrent_snapshot_packaging

558a0bb

Remove debug change.

bf2ff60

Fix snapshot_links path

81d0637

change to borrow for bank_snapshots_dir

d2fda3c

Reverted changes in create_and_verify_snapshot

fff384d

Fix param errors

4fa8359

Fix rebase errors

9c1c0a5

xiangzhu70 added 3 commits April 25, 2023 16:03

Remove NOTE 1

ec89de8

Remove unwrap

448c662

Remove the variables to make it apparent taht snapshot_links is the b…

50ea692

…ank_snapshots_dir

xiangzhu70 force-pushed the account_package_use_snapshot_dir_in_place branch from fdf4caf to 50ea692 Compare April 25, 2023 23:03

xiangzhu70 requested review from brooksprumo and apfitzge April 26, 2023 00:34

brooksprumo reviewed Apr 26, 2023

View reviewed changes

Use soft link instead of hard link for snapshot and status cache

ce879aa

brooksprumo previously approved these changes Apr 26, 2023

View reviewed changes

apfitzge reviewed Apr 26, 2023

View reviewed changes

After switching to soft symlinking, the src path should be absolute

d750aeb

xiangzhu70 dismissed brooksprumo’s stale review via d750aeb April 26, 2023 17:34

xiangzhu70 requested review from apfitzge and brooksprumo April 26, 2023 18:39

brooksprumo approved these changes Apr 26, 2023

View reviewed changes

xiangzhu70 merged commit f3e94ca into solana-labs:master Apr 26, 2023

xiangzhu70 mentioned this pull request Apr 26, 2023

Clean up the outdated SnapshotPackage snapshot_links field #31360

Merged

		let storage_entries: Vec<_> = (0..=slot)
		.map(\|i\| Arc::new(AccountStorageEntry::new(&accounts_dir, 0, i as u32, 10)))

		// Create one fake snapshot.
		let snapshots_paths: Vec<_> = (slot..=slot)

		@@ -197,9 +175,12 @@ impl AccountsPackage {
		/// NOTE 2: This fn will panic if the AccountsPackage is of type EpochAccountsHash.

AHV processes the snapshot dirs in place #30978

AHV processes the snapshot dirs in place #30978

Conversation

xiangzhu70 commented Mar 30, 2023 • edited Loading

Problem

Summary of Changes

xiangzhu70 Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 3, 2023 • edited Loading

Codecov Report

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

xiangzhu70 Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge Apr 4, 2023 • edited by xiangzhu70 Loading

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

xiangzhu70 Apr 12, 2023 • edited Loading

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 commented Apr 7, 2023

xiangzhu70 commented Apr 7, 2023 • edited Loading

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

Choose a reason for hiding this comment

brooksprumo left a comment

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

xiangzhu70 commented Apr 26, 2023

xiangzhu70 commented Mar 30, 2023 •

edited

Loading

xiangzhu70 Apr 3, 2023 •

edited

Loading

codecov bot commented Apr 3, 2023 •

edited

Loading

xiangzhu70 Apr 4, 2023 •

edited

Loading

apfitzge Apr 4, 2023 •

edited by xiangzhu70

Loading

xiangzhu70 Apr 12, 2023 •

edited

Loading

xiangzhu70 commented Apr 7, 2023 •

edited

Loading