feat: implement swingStore data export/import in support of state sync #7026
+2,228
−463
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,8 +18,7 @@ import { enumeratePrefixedKeys } from './storageHelper.js'; | |
| * @typedef { import('../../types-external.js').ManagerOptions } ManagerOptions | ||
| * @typedef { import('../../types-external.js').SnapStore } SnapStore | ||
| * @typedef { import('../../types-external.js').SourceOfBundle } SourceOfBundle | ||
| * @typedef { import('../../types-external.js').TranscriptStore } TranscriptStore | ||
| * @typedef { import('../../types-external.js').VatManager } VatManager | ||
| * @typedef { import('../../types-internal.js').RecordedVatOptions } RecordedVatOptions | ||
| * @typedef { import('../../types-external.js').TranscriptEntry } TranscriptEntry | ||
|
|
@@ -36,25 +35,24 @@ const FIRST_DEVICE_ID = 70n; | |
| * Establish a vat's state. | ||
| * | ||
| * @param {*} kvStore The key-value store in which the persistent state will be kept | ||
| * @param {*} transcriptStore Accompanying transcript store | ||
| * @param {string} vatID The vat ID string of the vat in question | ||
| * TODO: consider making this part of makeVatKeeper | ||
| */ | ||
| export function initializeVatState(kvStore, transcriptStore, vatID) { | ||
| kvStore.set(`${vatID}.o.nextID`, `${FIRST_OBJECT_ID}`); | ||
| kvStore.set(`${vatID}.p.nextID`, `${FIRST_PROMISE_ID}`); | ||
| kvStore.set(`${vatID}.d.nextID`, `${FIRST_DEVICE_ID}`); | ||
| kvStore.set(`${vatID}.nextDeliveryNum`, `0`); | ||
| kvStore.set(`${vatID}.incarnationNumber`, `1`); | ||
| transcriptStore.initTranscript(vatID); | ||
| } | ||
|
|
||
| /** | ||
| * Produce a vat keeper for a vat. | ||
| * | ||
| * @param {KVStore} kvStore The keyValue store in which the persistent state will be kept | ||
| * @param {TranscriptStore} transcriptStore Accompanying transcript store, for the transcripts | ||
| * @param {*} kernelSlog | ||
| * @param {string} vatID The vat ID string of the vat in question | ||
| * @param {*} addKernelObject Kernel function to add a new object to the kernel's | ||
|
|
@@ -76,7 +74,7 @@ export function initializeVatState(kvStore, streamStore, vatID) { | |
| */ | ||
| export function makeVatKeeper( | ||
| kvStore, | ||
| transcriptStore, | ||
| kernelSlog, | ||
| vatID, | ||
| addKernelObject, | ||
|
|
@@ -94,7 +92,6 @@ export function makeVatKeeper( | |
| snapStore = undefined, | ||
| ) { | ||
| insistVatID(vatID); | ||
|
|
||
| function getRequired(key) { | ||
| const value = kvStore.get(key); | ||
|
|
@@ -475,20 +472,12 @@ export function makeVatKeeper( | |
| /** | ||
| * Generator function to return the vat's transcript, one entry at a time. | ||
| * | ||
| * @param {number} [startPos] Optional position to begin reading from | ||
| * | ||
| * @yields { TranscriptEntry } a stream of transcript entries | ||
| */ | ||
| function* getTranscript(startPos) { | ||
| for (const entry of transcriptStore.readSpan(vatID, startPos)) { | ||
| yield /** @type { TranscriptEntry } */ (JSON.parse(entry)); | ||
| } | ||
| } | ||
|
|
@@ -499,21 +488,13 @@ export function makeVatKeeper( | |
| * @param {object} entry The transcript entry to append. | ||
| */ | ||
| function addToTranscript(entry) { | ||
| transcriptStore.addItem(vatID, JSON.stringify(entry)); | ||
| } | ||
|
|
||
| /** @returns {number} */ | ||
| function getTranscriptEndPosition() { | ||
| const { endPos } = transcriptStore.getCurrentSpanBounds(vatID); | ||
| return endPos; | ||
| } | ||
|
|
||
| function getSnapshotInfo() { | ||
|
|
@@ -540,6 +521,7 @@ export function makeVatKeeper( | |
|
|
||
| const endPosition = getTranscriptEndPosition(); | ||
| const info = await manager.makeSnapshot(endPosition, snapStore); | ||
| transcriptStore.rolloverSpan(vatID); | ||
| const { | ||
| hash, | ||
| uncompressedSize, | ||
|
|
@@ -560,19 +542,18 @@ export function makeVatKeeper( | |
| return true; | ||
| } | ||
|
|
||
| function deleteSnapshotsAndTranscript() { | ||
| if (snapStore) { | ||
| snapStore.deleteVatSnapshots(vatID); | ||
| } | ||
| transcriptStore.deleteVatTranscripts(vatID); | ||
| } | ||
|
|
||
| function dropSnapshotAndResetTranscript() { | ||
| if (snapStore) { | ||
| snapStore.stopUsingLastSnapshot(vatID); | ||
| } | ||
| transcriptStore.rolloverSpan(vatID); | ||
|
There was a problem hiding this comment. This is the right thing to do, although we should eventually consider some cleanup and make the caller responsible for updating the transcript bounds. Or rename the function. As @mhofman pointed out, this is used during vat upgrades, when we want to switch to a new incarnation, which means a new span boundary, and to stop using the previous snapshot. We call Thinking about our new data retention behavior, I think once this PR lands, we'll:
That's workable, but not entirely consistent. I think vat termination should delete everything: snapshots and transcripts. I think we'll need to make it easy for validators to turn on pruning of old snapshots.. pruning snapshots at upgrade points is helpful, but feels like an odd side-effect. Until we get the pruning config implemented, maybe we should mark the latest snapshot as inactive during vat upgrade, instead of deleting everything. There was a problem hiding this comment. Given that right now we only keep the latest snapshot of a vat, I believe the behavior is at least consistent within store types. However I agree that in the future we should have a pruning policy that decides how many "spans" of a vat to keep, which would keep both the snapshot and transcript for that span consistently with each other. Most validators likely only care to have the latest span for restart, and there is no reason to keep historical transcript spans. |
||
| } | ||
|
|
||
| function vatStats() { | ||
|
|
@@ -584,9 +565,8 @@ export function makeVatKeeper( | |
| const objectCount = getCount(`${vatID}.o.nextID`, FIRST_OBJECT_ID); | ||
| const promiseCount = getCount(`${vatID}.p.nextID`, FIRST_PROMISE_ID); | ||
| const deviceCount = getCount(`${vatID}.d.nextID`, FIRST_DEVICE_ID); | ||
| const { startPos, endPos } = transcriptStore.getCurrentSpanBounds(vatID); | ||
| const transcriptCount = endPos - startPos; | ||
|
|
||
| // TODO: Fix the downstream JSON.stringify to allow the counts to be BigInts | ||
| return harden({ | ||
|
|
@@ -645,8 +625,8 @@ export function makeVatKeeper( | |
| vatStats, | ||
| dumpState, | ||
| saveSnapshot, | ||
| getSnapshotInfo, | ||
| deleteSnapshotsAndTranscript, | ||
| dropSnapshotAndResetTranscript, | ||
| }); | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tracked the call sites, and it seems this is only used during upgrades, but not during vat termination. I've always assumed the
extract-transcript-from-kerneldb.jstool wasn't exporting terminated vats because their transcripts were fully pruned from the DB, but it seems to be that they're instead removed from thedynamicIDslist?If my understanding above is correct, is there a need to explicitly "close" the last span after such vat termination? At the very least, the span should no longer be "active", right? And its last transcript and snapshot should not be exported unless historical artifacts are included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vat termination should be allowed to fully delete the vat data: all transcripts, all heap snapshots, plus of course all the current-moment things like c-lists. Their vatIDs should also be removed from the
dynamicIDslist.I think we're not fully clear on retaining/not-retaining historical spans for terminated vats (and we haven't yet added an
openSwingStore()option to retain/not-retain old data). One coherent stance would be for a "vat is terminated"transcriptStoreAPI to close off the current span, mark it as not-current in the same way that thisrolloverSpanAPI would have done (thus leaving all spans marked as not-current), and make sure that whatever DB record is used to decide that certain spans are required artifacts knows to not require one for the terminated vat.Another coherent stance would be for this "vat is terminated" API to just delete everything immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by @mhofman's comment.
rolloverSpanis used every time there's a snapshot.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no snapshot when terminating a vat. The last span should be marked inactive on vat termination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I'm a little nervous about deleting everything on vat termination because it seems like the transcript leading up to a failure would be critical to diagnosing the problem that caused the failure (or is that already adequately captured by the slog?). So I guess the question is: mark the span as inactive or remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say mark the span as inactive, which makes it eligible for pruning based on (future) endpoint configuration. An archive node would keep the spans and could be used for diagnosis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-opening this conversation as I don't believe my blocking concern has been addressed: on vat termination, aka when
cleanupAfterTerminatedVatis executed, we do not seem to close the last transcript span.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment thread was attached to a line number which may have drifted, but lemme see if @mhofman 's concerns are addressed now:
removeSnapshotAndTranscript(called during vat termination, via kernelKeeper.cleanupAfterTerminatedVat) will:removeSnapshotAndResetTranscript(called during vat upgrade, via vat-warehouse.jsresetWorker) will:keepTranscripts = true(the default)false@mhofman: I still want to act upon your opinon about whether upgrade should delete snapshots (in the adjacent comment thread), but for this comment thread, does the above behavior address your concerns about vat termination and "closing the last transcript span"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the transcript is now removed, the original issue can be considered addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I am still unconvinced that forcibly removing transcripts and snapshots on termination is the right approach at this time, but this is not a a blocker. I'd still prefer to drop snapshots and "close" the transcript span instead (aka rollover without setting a new current span).
I understand this is cruft we're leaving behind on vat termination, but that's something we can address later with a migration once we figure out how to deal with accumulated historical metadata in general. I don't believe we're in a position to recycle
vatIDanyway, which would be the only other benefit.