Prevent parallel snapshots #4404

BeniaminDrasovean · 2022-08-29T11:34:05Z

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

If a snapshot takes more than one epoch, another snapshot will start in parallel. In extreme cases, this will lead to an IO bottleneck

Proposed Changes

Add isSnapshotInProgress flag in order to prevent this kind of behaviour, and skip a snapshot if another one is running.
Remove duplicated code from AccountsDB and PeerAccountsDB

Testing procedure

Create another branch, and add a time.Sleep(1h) to simulate a snapshot that takes 1h. No other snapshot should start during this time.
Another normal testing procedure to check that nothing has changed in normal cases.

iulianpascalau · 2022-08-29T13:10:02Z

state/accountsDB.go

+	return adb, nil
+}
+
+func getAccountsDb(args ArgsAccountsDB) *AccountsDB {


I think createAccountsDb will be a better suitable name

You're right, changed

iulianpascalau · 2022-08-29T13:13:20Z

state/accountsDB.go

+		return false
+	}
+
+	if !trieStorageManager.ShouldTakeSnapshot() {


return directly here? like

return trieStorageManager.ShouldTakeSnapshot()

andreibancioiu · 2022-08-29T12:37:28Z

state/accountsDB.go

+	trieStorageManager := adb.mainTrie.GetStorageManager()
+	val, err := trieStorageManager.GetFromCurrentEpoch([]byte(common.ActiveDBKey))
+	if err != nil || !bytes.Equal(val, []byte(common.ActiveDBVal)) {
+		startSnapshotAfterRestart(adb, args)


A separate goroutine will be launched due to calling startSnapshotAfterRestart, which is correct ✅

Ideally, we wouldn't call such a function in a constructor (NewAccountsDB), but can be left as it is for the moment (it was the same before).

Also, optionally, extract bytes.Equal(val, []byte(common.ActiveDBVal) to a separate variable.

Extracted in a separate variable.

startSnapshotAfterRestart is moved from the constructor in #4367

andreibancioiu · 2022-08-29T12:43:52Z

state/accountsDB.go

@@ -1005,14 +1012,11 @@ func (adb *AccountsDB) RecreateAllTries(rootHash []byte) (map[string]common.Trie
 		return nil, err
 	}

-	recreatedTrie, err := adb.mainTrie.Recreate(rootHash)
+	allTries, err := adb.recreateMainTrie(rootHash)


recreateMainTrie returns allTries - is not obvious from the name. Is there a better name for recreateMainTrie?

Not having any great idea

andreibancioiu · 2022-08-29T12:46:35Z

state/accountsDB.go

-	if err != nil {
-		log.Error("snapshotState error", "err", err.Error())
+	trieStorageManager, epoch, shouldTakeSnapshot := adb.prepareSnapshot(rootHash)
+	if !shouldTakeSnapshot {


Perhaps log a message that we won't take the snapshot yet?

There is a log message inside prepareSnapshot:

log.Debug("skipping snapshot", "last snapshot rootHash", adb.lastSnapshot.rootHash, "rootHash", rootHash, "last snapshot epoch", adb.lastSnapshot.epoch, "epoch", epoch, "isSnapshotInProgress", adb.isSnapshotInProgress.IsSet(), )

andreibancioiu · 2022-08-29T12:48:44Z

state/accountsDB.go

+func (adb *AccountsDB) prepareSnapshot(rootHash []byte) (common.StorageManager, uint32, bool) {
+	trieStorageManager, epoch, err := adb.getTrieStorageManagerAndLatestEpoch()
+	if err != nil {
+		log.Error("snapshot user state error", "err", err.Error())


Shouldn't this be considered a fatal error (e.g. stop node)?

There might be a chance that the next snapshot will succeed, so the node can continue processing. What do you say @iulianpascalau ?

andreibancioiu · 2022-08-29T13:06:04Z

state/accountsDB.go

+
+	go adb.markActiveDBAfterSnapshot(stats, errChan, rootHash, "snapshotState user trie", epoch)
+
+	adb.waitForCompletionIfRunningInImportDB(stats)


Perhaps, in the future, we can also waitForCompletionIfHeavilySyncing() (when out of sync). From my understanding of the logs (if correct), that would greatly reduce the snapshot time. Does this make sense?

E.g. import db finishes snapshot in 2-3 minutes.

The snapshot process is started on a goroutine. During import-db, epochs are processed much faster than an actual epoch time, so there is a chance that an epoch is processed faster than it will take the snapshot to finish. adb.waitForCompletionIfRunningInImportDB(stats) is added to assure that the processing will wait for the snapshot to finish.

andreibancioiu · 2022-08-29T13:10:35Z

state/accountsDB.go

+
+func (adb *AccountsDB) prepareSnapshot(rootHash []byte) (common.StorageManager, uint32, bool) {
+	trieStorageManager, epoch, err := adb.getTrieStorageManagerAndLatestEpoch()
+	if err != nil {


Perhaps also log the results of this call (e.g. the returned epoch?) - for debugging purposes.

It is logged. If we skip the snapshot, we also log the epoch. If we take the snapshot, we log the epoch on "starting snapshot..." print

andreibancioiu · 2022-08-29T13:13:04Z

state/accountsDB.go

 }

 func (adb *AccountsDB) markActiveDBAfterSnapshot(stats *snapshotStatistics, errChan chan error, rootHash []byte, message string, epoch uint32) {
 	stats.PrintStats(message, rootHash)

+	defer adb.isSnapshotInProgress.Reset()


So the function markActiveDBAfterSnapshot() is now more like a doStuffOnSnapshotCompletion.

Right. Renamed to processSnapshotCompletion. I look forward to a better naming idea 😅

andreibancioiu · 2022-08-29T13:19:53Z

state/peerAccountsDB.go

-	if !trieStorageManager.ShouldTakeSnapshot() {
-		log.Debug("skipping snapshot for rootHash", "hash", rootHash)
+	trieStorageManager, epoch, shouldTakeSnapshot := adb.prepareSnapshot(rootHash)
+	if !shouldTakeSnapshot {


Perhaps log a message for not taking the snapshot?

It is logged inside prepareSnapshot

The base branch was changed.

codecov-commenter · 2022-09-09T10:38:46Z

Codecov Report

Merging #4404 (fd20734) into rc/v1.4.0 (ff936b2) will increase coverage by 0.00%.
The diff coverage is 97.01%.

@@            Coverage Diff             @@
##           rc/v1.4.0    #4404   +/-   ##
==========================================
  Coverage      73.84%   73.84%           
==========================================
  Files            689      689           
  Lines          88127    88120    -7     
==========================================
- Hits           65075    65073    -2     
+ Misses         18140    18137    -3     
+ Partials        4912     4910    -2

Impacted Files	Coverage Δ
state/accountsDB.go	`79.06% <96.55%> (+0.53%)`	⬆️
state/peerAccountsDB.go	`100.00% <100.00%> (+8.53%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

…-snapshot-in-progress # Conflicts: # state/accountsDB.go

gabi-vuls

System test passed.

add isSnapshotInProgress flag and remove duplicated code

296f7f9

BeniaminDrasovean added area:trie complexity:trivial impact:low interested:protocol size:small type:improvement type:refactoring labels Aug 29, 2022

BeniaminDrasovean self-assigned this Aug 29, 2022

Merge branch 'rc/2022-july' into skip-snapshot-if-snapshot-in-progress

08bb77b

andreibancioiu self-requested a review August 29, 2022 12:28

iulianpascalau self-requested a review August 29, 2022 12:40

iulianpascalau reviewed Aug 29, 2022

View reviewed changes

andreibancioiu previously approved these changes Aug 29, 2022

View reviewed changes

fix after review

b1c21ab

BeniaminDrasovean dismissed andreibancioiu’s stale review via b1c21ab August 29, 2022 14:07

iulianpascalau previously approved these changes Aug 29, 2022

View reviewed changes

andreibancioiu previously approved these changes Aug 29, 2022

View reviewed changes

BeniaminDrasovean added complexity:medium and removed complexity:trivial labels Aug 29, 2022

BeniaminDrasovean changed the base branch from rc/2022-july to rc/v1.4.0 September 9, 2022 06:12

BeniaminDrasovean added 2 commits September 9, 2022 09:13

Merge branch 'rc/v1.4.0' into skip-snapshot-if-snapshot-in-progress

5b0ac6f

Merge branch 'rc/v1.4.0' into skip-snapshot-if-snapshot-in-progress

09612ca

iulianpascalau previously approved these changes Sep 9, 2022

View reviewed changes

BeniaminDrasovean added 2 commits September 13, 2022 09:08

Merge remote-tracking branch 'origin/rc/v1.4.0' into skip-snapshot-if…

0ddd82b

…-snapshot-in-progress # Conflicts: # state/accountsDB.go

fix after merge

8e84a1d

BeniaminDrasovean dismissed iulianpascalau’s stale review via 8e84a1d September 13, 2022 07:41

BeniaminDrasovean added 2 commits September 13, 2022 10:42

Merge branch 'rc/v1.4.0' into skip-snapshot-if-snapshot-in-progress

fd20734

Merge branch 'rc/v1.4.0' into skip-snapshot-if-snapshot-in-progress

ed4d26e

gabi-vuls approved these changes Sep 14, 2022

View reviewed changes

andreibancioiu approved these changes Sep 14, 2022

View reviewed changes

iulianpascalau approved these changes Sep 14, 2022

View reviewed changes

Merge branch 'rc/v1.4.0' into skip-snapshot-if-snapshot-in-progress

731530c

BeniaminDrasovean merged commit a72b09a into rc/v1.4.0 Sep 14, 2022

BeniaminDrasovean deleted the skip-snapshot-if-snapshot-in-progress branch September 14, 2022 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent parallel snapshots #4404

Prevent parallel snapshots #4404

BeniaminDrasovean commented Aug 29, 2022

iulianpascalau Aug 29, 2022

BeniaminDrasovean Aug 29, 2022

iulianpascalau Aug 29, 2022

BeniaminDrasovean Aug 29, 2022

andreibancioiu Aug 29, 2022

BeniaminDrasovean Aug 29, 2022

andreibancioiu Aug 29, 2022

BeniaminDrasovean Aug 29, 2022

andreibancioiu Aug 29, 2022

BeniaminDrasovean Aug 29, 2022

andreibancioiu Aug 29, 2022

BeniaminDrasovean Aug 29, 2022

andreibancioiu Aug 29, 2022

andreibancioiu Aug 29, 2022

BeniaminDrasovean Aug 29, 2022

andreibancioiu Aug 29, 2022

BeniaminDrasovean Aug 29, 2022

andreibancioiu Aug 29, 2022

BeniaminDrasovean Aug 29, 2022

andreibancioiu Aug 29, 2022

BeniaminDrasovean Aug 29, 2022

codecov-commenter commented Sep 9, 2022 •

edited

gabi-vuls left a comment


		go adb.markActiveDBAfterSnapshot(stats, errChan, rootHash, "snapshotState user trie", epoch)

		adb.waitForCompletionIfRunningInImportDB(stats)

Prevent parallel snapshots #4404

Prevent parallel snapshots #4404

Conversation

BeniaminDrasovean commented Aug 29, 2022

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

Proposed Changes

Testing procedure

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

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

codecov-commenter commented Sep 9, 2022 • edited

Codecov Report

gabi-vuls left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 9, 2022 •

edited