fix dynamic vat replay #1481
fix dynamic vat replay #1481
Conversation
Dynamic vats aren't getting reloaded properly: we aren't saving the source
bundle used to create them, nor are we loading them at startup time.
This adds test to exercise the problem, which fail ("unknown vatID") when we
start a new swingset from the old state vector. We'll fix the problem in the
next commit.
refs #1480
This saves the dynamic vat data (source bundle or bundleName, dynamicOptions)
to the kernel state DB when the vat is first created. Then, during replay, we
reload the dynamic vat from that data.
This refactors dynamic vat creation slightly, to merge create-by-bundle and
create-by-bundle-name into a single function that takes a `source` argument
which contains either `{ bundle }` or `{ bundleName }`. By passing `source`
to `createVatDynamically`, rather than first converting it into a full
bundle, we have enough information to store the much smaller bundleName in
the DB. Userspace code still sees the same API (`vatAdminSvc~.createVat(bundle)`
or `vatAdminSvc~.createVatByName(bundleName)`), but the kernel internals
and the vat-admin device endowments have changed.
We add a new `vat.dynamicIDs` entry to the kernelKeeper, which contains a
single JSON-encoded Array of dynamic vatIDs. This isn't great, but it's
expedient, and our DB doesn't really support enumerating anything more clever
anyways. We also add a pair of keys to the vatKeeper for each dynamic vat:
`v$NN.source` and `v$NN.options`, to record the bundle/bundleName and
dynamicOptions that were used the first time around, so we can use them on
subsequent calls too.
`createVatDynamically` normally sends a message (to the vatAdminVat) when the
new vat is ready, so it can inform the original caller and give them the new
root object. We suppress this message when reloading the kernel, because it
was already delivered the first time around. One side-effect is that failures
to reload the dynamic vat must panic the kernel, because there is nobody
ready to report the error to. This could happen if the new host application
failed to configure the same source bundles as the previous one did. At some
point we'll want to store all these bundles in the DB, which should remove
this failure mode.
closes #1480
warner
force-pushed
the
1480-dynamic-vat-replay
branch
from
2db3aa8
to
7d631bc
Compare
There was a problem hiding this comment.
Looks good aside from my issues below w.r.t the complexification of the API. I don't think those concerns are major enough to stop the train (hence I'm approving) but I'm a wee bit cranky about them. Also one or two of other totally non-blocking minor nits to address at your leisure.
| * that can occur during any given crank. Stack frames are limited as well. | ||
| * The meter is refilled between cranks, but if the meter ever underflows, | ||
| * the vat is terminated. If 'false', the vat is unmetered. | ||
| * @param source an object which either has a `bundle` (JSON-serializable |
There was a problem hiding this comment.
Really not a fan of switching from two methods with simple APIs to one method with a complicated API.
There was a problem hiding this comment.
I hear you, but we can't simplify it to the point that dynamicVat.js only hears about full bundle objects, as it did before. If we did that, the create-from-name case would throw away the name before dynamicVat.js heard about it, which means it would have to store the full (unnamed) bundle into the DB for both cases, not just create-from-bundle. By retaining the name (when one is available), we can store just the name in the DB. And that feels like it's going to be important for storage-performance pretty quickly.
The fully-denormalized(?) alternative would probably be for vatKeeper to have two separate methods (one that returns a source bundle or undefined, a second which returns a source bundleName or undefined). The kernel would call both in succession to see which one has a value. Then dynamicVat.js must expose four methods (first-time-vs-replay times name-vs-bundle). That felt like a net complexity increase over the polymorphic source argument. We could have it expose two methods (name-vs-bundle) and make both take an optional vatID to select between first-time-vs-replay, but I was more uncomfortable using polymorphism over vatID than I was over source.
| // vat.name.$NAME = $vatID = v$NN | ||
| // vat.nextID = $NN | ||
| // device.names = JSON([names..]) | ||
| // device.name.$NAME = $deviceID = d$NN | ||
| // device.nextID = $NN | ||
|
|
||
| // dynamic vats have these too: | ||
| // v$NN.source = JSON({ bundle }) or JSON({ bundleName }) |
There was a problem hiding this comment.
I suspect overloading this is the tail that wagged to dog w.r.t the API complexity I complained about above.
There was a problem hiding this comment.
It did make it make the other pieces fall into that shape, yeah. My first pass had separate v$NN.sourceBundle and v$NN.sourceBundleName, with a comment that there should only ever be one, which is a sort of fussy invariant that I always want to avoid (better to have the rule be present-or-not than one-of-two).
| @@ -498,6 +504,18 @@ export default function makeKernelKeeper(storage) { | |||
| return storage.get(k); | |||
| } | |||
|
|
|||
| function addDynamicVatID(vatID) { | |||
There was a problem hiding this comment.
I'm presuming that the plan is (eventually) for dead vats to be removed from vat.dynamicIDs? Also, might it make sense to put them in a separate identifier namespace (e.g., vd$NN) and allocate them here, instead of passing the ID in, as that might reduce the chances of a coordination failure between allocation and use?
There was a problem hiding this comment.
Yeah, we'll remove them from that list when they're really dead. For our intermediate step, the dead vats will be remembered by a vat.dynamicIDs entry and a v$NN.dead flag (and no other v$NN.* keys), which will tell our startup code to create a dead-vat no-manager no-vatKeeper ephemeral.vats entry for it. The step after that, when we remove them completely (no object-table/etc references), will delete the two remaining DB keys, and the only trace that they ever existed will be the suspiciously-elevated nextVatID counter. And of course the vat will live on in the memories (messages/transcripts) of all those (surviving vats) who loved it.
| @@ -31,12 +31,17 @@ const enableKernelPromiseGC = true; | |||
| // The schema is: | |||
| // | |||
| // vat.names = JSON([names..]) | |||
| // vat.dynamicIDs = JSON([vatIDs..]) | |||
There was a problem hiding this comment.
Let me know when you think the schema changes have stabilized, as I (or somebody, but probably me) will need to update the kerneldump utility to match.
There was a problem hiding this comment.
It's probably stable enough now. Unless it breaks something (which, if there were unit tests for kernelDump, I would have noticed 😉), I think we can ignore the new keys until your dead-vat changes land, and update kernelDump for both at the same time.
There was a problem hiding this comment.
Adding to the database schema doesn't break kernelDump but if it knows about the new items it can display them more usefully.
There was a problem hiding this comment.
Would it make sense to add the genesis vats to the kernelDB as well, and treat their sources the same as the dynamic vats? I would like this, so that it's a little harder to accidentally modify the genesis vat source code without resetting state from scratch, and also so that exporting the kernel state is just a matter of doing a direct dump of the kernelDB and not having to rebundle sources at export time (which is sensitive to the bundler we use, and any changes to the sources). It would also mean we don't need to copy the vat directory to every created basedir.
If not now, but you think this is a good idea, I can create an issue for it.
Yes, absolutely. I think we'll wind up going farther than that: the "payload"/deployment level of the stack will be defined by a set of additions to the "blob pool" (where bundles get stored), and an initial series of messages to the singular initial vat, which creates dynamic vats for zoe and whatever else the system should have from T=2 (where T=1 has the initial vat but nothing else). Genesis vats should be replaced by messages. I'm not sure how long the medium-term (genesis vat source code goes into the DB) will last, so I'm kinda deferring it a bit in the hopes we can jump directly to the messages form, but if we haven't made any progress on the latter within, say, a month, let's go ahead and do the genesis-code-in-DB thing. |
Also address some review notes
warner
force-pushed
the
1480-dynamic-vat-replay
branch
from
0c9f9c8
to
35ec3a5
Compare
warner
changed the title
Dynamic vats aren't getting reloaded properly: we aren't saving the source
bundle used to create them, nor are we loading them at startup time.
This adds test to exercise the problem, which fail ("unknown vatID") when we
start a new swingset from the old state vector. We'll fix the problem in the
next commit.
refs #1480