Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix extract transcript #7434

Merged
merged 5 commits into from
Apr 18, 2023
Merged

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Apr 17, 2023

refs: #7432

Description

Update the transcript extract tools (and swingstore) to support the latest changes to transcripts.

  • Add a bundle export tools
  • When exporting from kernelDB, reference bundle IDs for both the lockdown and supervisor bundles, as well as the vat source bundle
  • re-order the fields of create-vat in the slog extract tool to be more similar to the kernel DB extract tool (minus the bundle ID changes)

The swingstore is extended as needed to support the extract tooling.

Some of these commits are cherry-picks from #6723 that were omitted from #7432

Security Considerations

None, tooling only

Scaling Considerations

None

Documentation Considerations

Internal tooling

Testing Considerations

Manually verified with from the state and slog created by a modified test-vaults-integration.js bootstrap test. Verified in a merge with #7432 that the transcripts could be replayed successfully.

@mhofman
Copy link
Member Author

mhofman commented Apr 17, 2023

FYI @warner

I plan on figuring something out for handling bundleIDs in the workerOptions.

@mhofman mhofman marked this pull request as ready for review April 18, 2023 00:51
@mhofman mhofman requested a review from warner April 18, 2023 00:52
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

looks good

@@ -131,6 +140,14 @@ export function makeTranscriptStore(
return transcripts;
}

function* readVatTranscript(vatID) {
Copy link
Member

Choose a reason for hiding this comment

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

We might call this readFullVatTranscript to make it clear that it's fetching multiple spans, which is not something we'd normally do for operational purposes. readVatTranscript isn't wrong.. it fits with our hierarchy of names:

  • "transcript": everything from vat creation to vat termination (or latest delivery)
  • "incarnation": everything from one upgrade to the next upgrade (or latest delivery, or termination)
  • "span": everything from heap snapshot save/load to next upgrade, snapshot write, termination, or latest delivery
  • "item" or "entry": one delivery

but "transcript" to mean "everything" is so rarely used, that it doesn't hurt to give it a longer name.

function* readVatTranscript(vatID) {
for (const { startPos, endPos } of sqlDumpVatSpansQuery.iterate(vatID)) {
for (const row of sqlDumpItemsQuery.iterate(vatID, startPos, endPos)) {
yield row;
Copy link
Member

Choose a reason for hiding this comment

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

be thinking about what happens if we've pruned the span and/or the items, we probably want this to throw an error rather than yield broken items.. maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! This method will simply be missing the historical span entries that were pruned. While this is a "full transcript" I think it's acceptable for it to yield a partial history given what we yield is both the transcript entry and its position.

I've updated the consumer (extract-transcript) to not write anything until it sees a startVat delivery. There was already logic to throw if the entries were not continuous (gap in positions). Before the change it would have thrown on pruned historical transcript spans. Now it will only return a full transcript of the last incarnation, but handle partially pruned transcripts of previous incarnations.

transcriptNum += 1;
const entry = { transcriptNum, d: delivery, syscalls };
Copy link
Member

Choose a reason for hiding this comment

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

BTW I'd love to find a clean way to keep the transcriptNum in the DB, to avoid potential drift or fencepost errors with this external incrementing counter.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is in the DB already. Here it's from the slog, which doesn't include the transcript pos, so it has to be regenerated. It would indeed be nice to include this info in the slogfile.

return kvStore.get(key);
const value = kvStore.get(key);
if (value === undefined) {
throw Error(`Inexistent kvStore entry for ${key}`);
Copy link
Member

Choose a reason for hiding this comment

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

I'd say "non-existent" or "missing", but "inexistent" sounds like a cool word too :)

Copy link
Member Author

Choose a reason for hiding this comment

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

You caught my Latin roots :)

}
} else if (/(supervisor|lockdown)(B|-b)undle/.test(vatName)) {
Copy link
Member

Choose a reason for hiding this comment

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

is this abusing the CLI argument so that when you say "please replay the vat named supervisor-bundle", it will actually extract the given bundle and write it to disk?

weird, but I'm ok with it

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I had removed this, since it's been included in the extract-bundle tool instead...

for (const { position, item } of transcript) {
const entry = JSON.parse(item);
if (entry.d[0] === 'startVat') {
fs.ftruncateSync(fd);
Copy link
Member

Choose a reason for hiding this comment

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

This truncate is new, right? Were you having problems with overwriting pre-existing files that needed it? If so, it seems like it'd be better to open the file in the mode that overwrites/truncates the old one, or at least do the truncation immediately after opening, instead of inside this loop.

Or.. oh, are you writing every entry, but then you discover a startVat and retroactively decide that the transcript ought to begin here?

Ah, ok, so what you really want is a transcriptStore.readIncarnationItems(vatID, incarnationID), or a readCurrentIncarnation(vatID), so you don't have to deduce where the current incarnation begins.

As we improve the transcript to track upgrades and snapshot reads/writes, we should consider exposing incarnation numbers to the DB (as I think we discussed already, you and me and @FUDCo, and I think we agreed in principle on doing it). Probably sooner rather than later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup that was the only way I found to only extract the latest incarnation given our DB model, and the motivation for my request to include the incarnation number.

@mhofman mhofman force-pushed the mhofman/fix-extract-transcript-from-kernel-db branch from c0c0997 to 4cbfc93 Compare April 18, 2023 15:28
@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Apr 18, 2023
@mhofman mhofman force-pushed the mhofman/fix-extract-transcript-from-kernel-db branch from d3a7e00 to d67bec4 Compare April 18, 2023 19:29
@mergify mergify bot merged commit c966474 into master Apr 18, 2023
@mergify mergify bot deleted the mhofman/fix-extract-transcript-from-kernel-db branch April 18, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants