Make background services aware of incremental snapshots #19401

brooksprumo · 2021-08-24T16:58:03Z

AccountsBackgroundService now knows about incremental snapshots. It is
now also in charge of deciding if an AccountsPackage is destined to be a
SnapshotPackage or not (or just used by AccountsHashVerifier).

!!! New behavior changes !!!

Full snapshots (both bank and archive) MUST succeed.

This is required because of how the last full snapshot slot is
calculated, which is used by AccountsBackgroundService when calling
clean_accounts().

File system calls are now unwrapped and will result in a crash. As Trent told me:

Well I think if a snapshot fails due to some IO error, it's very likely that the operator is going to have to intervene before it works. We should exit error in this case, otherwise the validator might happily spin for several more hours, never successfully writing a complete snapshot, before something else brings it down. This would leave the validator's last local snapshot many more slots behind than it would be had we exited outright and potentially force the operator to abandon ledger continuity in favor of a quick catchup

Fixes #19167
Fixes #19168

AccountsBackgroundService now knows about incremental snapshots. It is now also in charge of deciding if an AccountsPackage is destined to be a SnapshotPackage or not (or just used by AccountsHashVerifier). !!! New behavior changes !!! Full snapshots (both bank and archive) **MUST** succeed. If there are failures, the functions will be retried. If they still fail, an assert will be triggered. This is required because of how the last full snapshot slot is calculated, which is used by AccountsBackgroundService when calling `clean_accounts()`.

brooksprumo

There's a lot going on in this PR. I've tried to call out most important bits!

brooksprumo · 2021-08-24T18:36:43Z

runtime/src/accounts_background_service.rs

+                let block_height = snapshot_root_bank.block_height();
+                let snapshot_type = if block_height
+                    % self.snapshot_config.full_snapshot_archive_interval_slots
+                    == 0
+                {
+                    Some(SnapshotType::FullSnapshot)
+                } else if block_height
+                    % self
+                        .snapshot_config
+                        .incremental_snapshot_archive_interval_slots
+                    == 0
+                {
+                    Some(SnapshotType::IncrementalSnapshot(last_full_snapshot_slot))
+                } else {
+                    None
+                };


Now AccountsBackgroundService is the one responsible for setting the snapshot type (previously this was in AccountsHashVerifier).

Note that the None here is for AccountsPackages that will not be archived, which is possible if the incremental snapshot interval does not equal the accounts hash interval.

Yeah using the block_height is a lot simpler than trying to find the first slot that crossed the full snapshot boundary. Interesting bit here is the full snapshots will be correlated with skip rate, i.e. the higher the skip rate, the less often - in wallclock time - we will take full snapshots. I think this is fine for v1.

Ah! That is interesting. Good to note.

runtime/src/accounts_background_service.rs

brooksprumo · 2021-08-24T18:41:32Z

core/src/accounts_hash_verifier.rs

+
+        let snapshot_package = SnapshotPackage::from(accounts_package);
+        let pending_snapshot_package = pending_snapshot_package.unwrap();
+        let _snapshot_config = snapshot_config.unwrap();


I may be able to remove SnapshotConfig from AccountsHashVerifier entirely too. I'm thinking about taking some things out of AccountsPackage that live in SnapshotConfig, which I may then use here, so I'm going to leave the SnapshotConfig here for now.

core/src/snapshot_packager_service.rs

brooksprumo · 2021-08-24T18:48:36Z

core/tests/snapshots.rs

+        assert_eq!(deserialized_bank.slot(), LAST_SLOT,);
        assert_eq!(
-            deserialized_bank.slot(),
-            EXPECTED_SLOT_FOR_LAST_SNAPSHOT_ARCHIVE
+            deserialized_bank,
+            **bank_forks
+                .read()
+                .unwrap()
+                .get(deserialized_bank.slot())
+                .unwrap()


Yay! Now we check the banks are equal too!

brooksprumo · 2021-08-24T18:50:22Z

runtime/src/snapshot_package.rs

+/// Snapshots come in two flavors, Full and Incremental.  The IncrementalSnapshot has a Slot field,
+/// which is the incremental snapshot base slot.
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
 pub enum SnapshotType {
    FullSnapshot,
-    IncrementalSnapshot,
-}
-
-impl SnapshotType {
-    /// Get the string prefix of the snapshot type
-    pub fn to_prefix(&self) -> &'static str {
-        match self {
-            SnapshotType::FullSnapshot => TMP_FULL_SNAPSHOT_PREFIX,
-            SnapshotType::IncrementalSnapshot => TMP_INCREMENTAL_SNAPSHOT_PREFIX,
-        }
-    }
+    IncrementalSnapshot(Slot),


By putting the Slot in the IncrementalSnapshot, I can always guarantee there is a last full snapshot if the snapshot type is Incremental. Otherwise I'd have asserts all over the place to ensure that was the case.

brooksprumo · 2021-08-24T18:51:14Z

runtime/src/snapshot_package.rs

-            SnapshotType::IncrementalSnapshot => TMP_INCREMENTAL_SNAPSHOT_PREFIX,
-        }
-    }
+    IncrementalSnapshot(Slot),
 }


While looking through the actual archive and unarchive functions more, I realized that the prefix didn't need to be unique between full and incremental snapshots. So that just makes this a bit simpler.

brooksprumo · 2021-08-24T18:53:24Z

runtime/src/snapshot_utils.rs

+    let snapshot_storages = snapshot_type.map_or_else(SnapshotStorages::default, |snapshot_type| {
+        let incremental_snapshot_base_slot = match snapshot_type {
+            SnapshotType::IncrementalSnapshot(incremental_snapshot_base_slot) => {
+                Some(incremental_snapshot_base_slot)
+            }
+            _ => None,
+        };
+        root_bank.get_snapshot_storages(incremental_snapshot_base_slot)
+    });


Based on the snapshot type, get the snapshot storages. In the None case, that means the "snapshot" is not meant to be archived, and will only be used to send to AccountsHashVerifier to push the hash out to the cluster. In that case, we don't need to snapshot any of the storages even.

runtime/src/snapshot_utils.rs

codecov · 2021-08-24T20:15:12Z

Codecov Report

Merging #19401 (e8c1aad) into master (6d93981) will decrease coverage by 0.1%.
The diff coverage is 63.0%.

@@            Coverage Diff            @@
##           master   #19401     +/-   ##
=========================================
- Coverage    82.8%    82.7%   -0.2%     
=========================================
  Files         457      459      +2     
  Lines      130682   130630     -52     
=========================================
- Hits       108289   108115    -174     
- Misses      22393    22515    +122

runtime/src/accounts_background_service.rs

runtime/src/snapshot_package.rs

core/src/accounts_hash_verifier.rs

lijunwangs · 2021-08-30T18:49:56Z

runtime/src/snapshot_package.rs

    pub hash: Hash, // temporarily here while we still have to calculate hash before serializing bank
    pub archive_format: ArchiveFormat,
    pub snapshot_version: SnapshotVersion,
    pub snapshot_archives_dir: PathBuf,
    pub expected_capitalization: u64,
    pub hash_for_testing: Option<Hash>,
    pub cluster_type: ClusterType,
+    pub snapshot_type: Option<SnapshotType>,


Why is this optional? Shouldn't we always know the snapshot type?

This comment should hopefully give an answer:
#19401 (comment)

And some more context here too:
#19401 (comment)

hmm. We are overloading the snapshot type for some other purpose -- maybe a separate flag for the Hash only. And when it is None, what is really the type? Is it a full or incremental? Does not matter?

Yeah, I debated how far to take this... There's a lot of coupling and implied behavior between bank snapshots, snapshot archives, and the hashing... plus how those three interact with the background services... Ultimately I figured that to do it "right" would likely require redoing the whole background services from the ground up, and that would probably be out of scope for this task, so I tried to make it least bad :)

This snapshot_type is used to decide which snapshot storages to get, when to take a snapshot/what time, and if it should be archived by SnapshotPackagerService.

core/src/accounts_hash_verifier.rs

runtime/src/accounts_background_service.rs

lijunwangs · 2021-08-30T20:04:36Z

runtime/src/snapshot_package.rs

        // Hard link the snapshot into a tmpdir, to ensure its not removed prior to packaging.
+        let snapshot_links = tempfile::Builder::new()
+            .prefix(&format!("{}{}-", TMP_BANK_SNAPSHOT_PREFIX, bank.slot()))


Do full snapshot and incremental snapshot look any different now for the tmp dir entry?

Nope. None of the different temp directories need different prefixes actually, but now we're only differentiating between bank snapshots and snapshot archives.

lijunwangs · 2021-08-30T20:09:17Z

runtime/src/snapshot_package.rs

+/// Snapshots come in two flavors, Full and Incremental.  The IncrementalSnapshot has a Slot field,
+/// which is the incremental snapshot base slot.
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
 pub enum SnapshotType {
    FullSnapshot,
-    IncrementalSnapshot,
-}
-
-impl SnapshotType {
-    /// Get the string prefix of the snapshot type
-    pub fn to_prefix(&self) -> &'static str {
-        match self {
-            SnapshotType::FullSnapshot => TMP_FULL_SNAPSHOT_PREFIX,
-            SnapshotType::IncrementalSnapshot => TMP_INCREMENTAL_SNAPSHOT_PREFIX,
-        }
-    }
+    IncrementalSnapshot(Slot),


core/src/accounts_hash_verifier.rs

lijunwangs · 2021-08-30T20:20:50Z

runtime/src/snapshot_package.rs

    pub hash: Hash, // temporarily here while we still have to calculate hash before serializing bank
    pub archive_format: ArchiveFormat,
    pub snapshot_version: SnapshotVersion,
    pub snapshot_archives_dir: PathBuf,
    pub expected_capitalization: u64,
    pub hash_for_testing: Option<Hash>,
    pub cluster_type: ClusterType,
+    pub snapshot_type: Option<SnapshotType>,


hmm. We are overloading the snapshot type for some other purpose -- maybe a separate flag for the Hash only. And when it is None, what is really the type? Is it a full or incremental? Does not matter?

brooksprumo · 2021-08-30T22:16:04Z

@carllin @lijunwangs Thanks for the reviews so far! I've fixed or commented on everything, so this PR should be ready for another round. Thanks in advance!

runtime/src/accounts_background_service.rs

runtime/src/snapshot_utils.rs

…na-labs#19401)" This reverts commit 78341d7.

brooksprumo force-pushed the iss-accounts-background-service-4 branch from 0c5eb27 to 9b5d5f9 Compare August 24, 2021 18:20

brooksprumo force-pushed the iss-accounts-background-service-4 branch from 9b5d5f9 to 6d24603 Compare August 24, 2021 18:20

brooksprumo commented Aug 24, 2021

View reviewed changes

brooksprumo marked this pull request as ready for review August 24, 2021 20:18

brooksprumo requested review from carllin, lijunwangs and ryoqun August 24, 2021 20:19

carllin reviewed Aug 29, 2021

View reviewed changes

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

carllin reviewed Aug 29, 2021

View reviewed changes

runtime/src/snapshot_package.rs Show resolved Hide resolved

brooksprumo added 2 commits August 30, 2021 09:50

pr: add comment to assert

1b3a734

pr: fix value passed in as last full snapshot slot, not block height

5985f4f

lijunwangs reviewed Aug 30, 2021

View reviewed changes

brooksprumo added 5 commits August 30, 2021 16:20

pr: fs should not fail, so call .expect()

e38ff0c

fixup!

88fc378

pr: slot -> u64

78c39fd

pr: add is_incremental_snapshot()

b6d0370

fixup!

a443e7d

brooksprumo requested review from carllin and lijunwangs August 30, 2021 21:44

remove retry from snapshotpackagerservice

218faf9

brooksprumo added 2 commits August 30, 2021 17:34

fixup

2605731

fixup

289568e

carllin reviewed Aug 31, 2021

View reviewed changes

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

carllin reviewed Aug 31, 2021

View reviewed changes

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

pr: use bank snapshot info from add_bank_snapshot()

c581c36

carllin reviewed Aug 31, 2021

View reviewed changes

runtime/src/snapshot_utils.rs Show resolved Hide resolved

carllin and others added 6 commits August 31, 2021 10:06

Fixup result propagation

92afafc

fixup: remove old assert

7201106

pr: add comment on snapshot_bank

fa2cada

pr: check if error from snapshot_utils is fatal

8a61ca6

remove calculate_last_full_snapshot_slot()

2bda2ae

move fatal check into snapshot request handler

e8c1aad

carllin approved these changes Aug 31, 2021

View reviewed changes

This was referenced Aug 31, 2021

Revert "Add snapshot_runtime_info module (#18199)" #19530

Merged

Revert "Add LastFullSnapshotSlot to SnapshotConfig (#19341)" #19529

Merged

brooksprumo merged commit fe9ee91 into solana-labs:master Aug 31, 2021

brooksprumo deleted the iss-accounts-background-service-4 branch August 31, 2021 23:33

frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021

Revert "Make background services aware of incremental snapshots (sola…

6651b39

…na-labs#19401)" This reverts commit 78341d7.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make background services aware of incremental snapshots #19401

Make background services aware of incremental snapshots #19401

brooksprumo commented Aug 24, 2021 •

edited

Loading

brooksprumo left a comment

brooksprumo Aug 24, 2021

carllin Aug 29, 2021

brooksprumo Aug 29, 2021

brooksprumo Aug 24, 2021 •

edited

Loading

brooksprumo Aug 24, 2021

brooksprumo Aug 24, 2021

lijunwangs Aug 30, 2021

brooksprumo Aug 24, 2021

brooksprumo Aug 24, 2021

codecov bot commented Aug 24, 2021 •

edited

Loading

lijunwangs Aug 30, 2021

brooksprumo Aug 30, 2021

lijunwangs Aug 30, 2021

brooksprumo Aug 30, 2021

lijunwangs Aug 30, 2021

brooksprumo Aug 30, 2021

lijunwangs Aug 30, 2021

lijunwangs Aug 30, 2021

brooksprumo commented Aug 30, 2021

Make background services aware of incremental snapshots #19401

Make background services aware of incremental snapshots #19401

Conversation

brooksprumo commented Aug 24, 2021 • edited Loading

brooksprumo 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

brooksprumo Aug 24, 2021 • 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

codecov bot commented Aug 24, 2021 • 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

brooksprumo commented Aug 30, 2021

brooksprumo commented Aug 24, 2021 •

edited

Loading

brooksprumo Aug 24, 2021 •

edited

Loading

codecov bot commented Aug 24, 2021 •

edited

Loading