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

remove stopVat(): allow vat upgrade without participation of old vat version #6650

Closed
warner opened this issue Dec 8, 2022 · 1 comment · Fixed by #7244
Closed

remove stopVat(): allow vat upgrade without participation of old vat version #6650

warner opened this issue Dec 8, 2022 · 1 comment · Fixed by #7244
Assignees
Labels
enhancement New feature or request liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet vaults_triage DO NOT USE
Milestone

Comments

@warner
Copy link
Member

warner commented Dec 8, 2022

What is the Problem Being Solved?

Our current mechanism for upgrading vats requires two steps:

  • send a stopVat() to the old version, giving it a chance to clean up and shut down
  • create a new worker (with the new version), send it a startVat() to launch/resume everything under the new code

The reasons we built upgrade with a stopVat include:

  • currently, the vat knows more about what data can be deleted than the kernel, so it seemed easier to let the vat drive the GC-like reference chasing (e.g. when a durable object is only referenced by a merely-virtual object, so the durable one should be deleted across the upgrade)
    • although we decided to defer most of the data-deletion for a future version, to reduce the risk of things going wrong during stopVat, at the cost of leaking some DB space
      • OT3H we don't expect vats to use a lot of merely-virtual (non-durable) objects, so the leak that represents may be insignificant
        • OT4H MarkM doesn't necessarily agree with me on that expectation
  • the vat's virtual object manager (VOM) has an LRU cache that needs to be flushed: it has unwritten changes to durable objects which must reach the DB before the old version is retired

But we've talked about getting rid of the stopVat step, because:

  • it's awkward that we depend upon the old version to assist in its own cleanup
  • if the old version is sufficiently broken (thus in need of upgrade), is it functional enough to complete the stopVat without error?
  • we don't know how long the vat will take to do stopVat; no user-space code is run, but it still raises the question of whether we should be doing some sort of metering
  • if something goes wrong during stopVat, the vat is now un-upgradeable, and we're kinda stuck

While we haven't really committed to removing it, we'd like to.

To do that, we need to have the kernel take over responsibility for everything we currently delegate to stopVat().

The stopVat() call currently does:

Description of the Design

First, we need to decide what to do about the LRU cache. Either we make it a write-through cache (so it's never stale, and doesn't need flushing), remove it entirely, or require a BOYD delivery in place of the stopVat (to allow the cache to be flushed, and incidentally providing one last chance to decrement references, which may reduce the amount of durable data retained across the upgrade boundary). Requiring a BOYD has many of the same issues as requiring stopVat: if the vat is really confused, it might not work, and thus break the upgrade. OTOH, BOYD avoids userspace interaction just as much as stopVat does, so the confusion would have to be in liveslots or lower for it to cause a problem.

Doing a BOYD before upgrade does have the potential to reduce the amount of data being retained across the upgrade, which might make things faster (and/or leak less data, until we manage to implement a full mark/sweep of the vatstore). I'm undecided about how the simplicity of not doing it compares to the consequences.

Then, we need to reject all the vat's old Promises. We already have code to locate these, in cleanupAfterTerminatedVat(), in

// now find all orphaned promises, which must be rejected
for (const k of kvStore.getKeys(`${vatID}.c.p`, `${vatID}.c.p.`)) {
, which walks the vat's c-list for kpids and looks for the ones that are unresolved and decided by the terminated vat. We can factor out this code and use it to perform the same search for vat upgrade, not just vat termination. There are two additional considerations:

  • We implemented "durable promise watchers", which allows a vat to subscribe a durable callback to an arbitrary promise (any promise with a vpid, so imported/exported/stored-in-virtual-data). If the vat uses this to subscribe to one of its own promises, then the upgrade will reject the promise (as usual), and the new version must receive the callback. We have code in stopVat to record the list of these rejected vpids (in the vatstore, inside a durable collection named watchedPromises), and code in startVat to invoke the registered callbacks. If the kernel does the rejection, we'll need some way to inform the startVat about the vpids and the rejection data.
    • one option is to have the kernel record the vpids, in the same place that stopVat used to
    • perhaps a better one would be for startVat to acquire an additional argument, with the list of vat-decided non-durable vpids that we rejected during upgrade, so it can walk the saved registration table and issue the callbacks
    • a third might be to deliver dispatch.notify() for all the rejected promises after the startVat
    • in any case, efficiency is a consideration: the vat might have a lot of outstanding promises, but only register a watcher on a tiny number of them
      • we might use syscall.subscribe() to inform the kernel about the own-promises too, and the kernel could limit the notifies/etc to the ones that were subscribed
  • In the future, we may introduce "durable promises" ("virtual promises" #3787, also see build durable notifier #4567). These should not be rejected during upgrade, because the new version of the vat will retain some way to resolve/reject them later. The kernel must be able to distinguish between the c-list vpids that are non-durable (and should be rejected by the kernel), and the durable ones (that should be left untouched)

Next, we need to abandon all the non-durable exported objects. Again, we have similar code already present in the vat-termination pathway,

// first, scan for exported objects, which must be orphaned
for (const k of kvStore.getKeys(`${vatID}.c.o+`, `${vatID}.c.o,`)) {
assert(k.startsWith(exportPrefix), k);
// The void for an object exported by a vat will always be of the form
// `o+NN`. The '+' means that the vat exported the object (rather than
// importing it) and therefore the object is owned by (i.e., within) the
// vat. The corresponding void->koid c-list entry will thus always
// begin with `vMM.c.o+`. In addition to deleting the c-list entry, we
// must also delete the corresponding kernel owner entry for the object,
// since the object will no longer be accessible.
const kref = kvStore.get(k);
orphanKernelObject(kref, vatID);
}
. The key difference is that vat-termination abandons all objects, whereas vat upgrade must not abandon the durable exports. So the kernel needs a way to categorize the exports (the object vrefs, which are technically called voids, just as promise vrefs are vpids, but we don't use the term for obvious reasons) into "durable" and "non-durable".

We want to limit the coupling between liveslots and the kernel, to give them each freedom to manage their own data structures without flag days or challenging upgrade steps, so we don't really want the kernel to know too much about how vrefs are structured. But I'm thinking that this durable/non-durable distinction might be worth bringing into the API.

Vat-exported vrefs start with o+ or p+ or d+ (for device nodes). Originally, the object vrefs were strictly o+NN, but with the introduction of virtual/durable data, that expanded into things like o+${kindID}/${instanceID}:${facetID} (e.g. o+11/2:0 is the first facet of the second instance of Kind o+11). The current void format is described in vatstore-usage.md . However the kernel doesn't know anything about the suffix: it will allocate krefs for any o+* that it sees, and the only special value is o+0 (which is automatically allocated for the root object during vat creation).

I'm thinking that we could expand that slightly, and introduce o+d* to mean "durable exported object". The kernel could then tell which c-list entries are for durable objects, and which are not. The kernel could then abandon all non-durables (except the o+0 root object) during upgrade, without needing to ask the vat about which ones are which.

(it might be cleaner to also introduce o+e* to mean "ephemeral exported object", and require all voids to start with either o+e or o+d, but I kind of like the pedagogical simplicity of talking about o+12 and o-34 without always needing to include additional characters)

This would allow the kernel to take responsibility for abandoning everything (promises and objects) that definitely goes away during the upgrade. It wouldn't immediately help with the GC-like release of durable objects which were only retained by now-deleted ephemeral/merely-virtual ones, but it would reveal more information to the kernel, that could help with some future process (e.g. if we also change vatstore to include kernel-visible slots, making the reference graph visible to a kernel-side mark/sweep operation).

If we did the same for Promise vrefs (vpids), i.e. p+dNN, then we'd be prepared for durable promises, which the kernel would not reject during upgrade (because the vat retains some durable mechanism to resolve/reject them in the new version).

Task List

Security Considerations

Doing less work during upgrade should reduce the risk of an upgrade going wrong, which improves security (or at least improves our ability to react to a security problem).

Removing stopVat reduces our reliance on vat code (liveslots, not userspace) slightly: a compromised liveslots could not longer avoid having non-durable promises be rejected during upgrade, nor could it prevent non-durable exports from being abandoned.

Compatibility Considerations

Changes to the vref format (o+d*) require coordination between vat (liveslots) and kernel, as does changing or removing the stopVat portion of the upgrade protocol, and changing which side is responsible for promise rejection and object abandonment. If we choose to implement this, but aren't able to deploy it before the bulldozer upgrade, then we'd need some sort of per-vat flag to tell the kernel what to do during an upgrade ("v1 vats should get a stopVat, >=v2 vats should have their c-lists walked and things rejected/abandoned").

Test Plan

Lots of unit tests.

cc @FUDCo @gibson042

@warner warner added enhancement New feature or request SwingSet package: SwingSet labels Dec 8, 2022
@warner
Copy link
Member Author

warner commented Dec 13, 2022

@FUDCo and I chatted about this a bit. Using o+dNN.. is awkward but not horrible, especially if we implicitly retain o+[a-z]\d+.* (i.e. o+eNN. and o+fNN.) for other/weirder purposes. And continuing to use o+NN does feel more pleasant than e.g. o+eNN (e for "ephemeral", effectively retiring the unadorned o+NN space).

@FUDCo mentioned that he's not sure we're getting a lot of value out of the LRU cache, compared to the confusion it creates when debugging, and that moving to a write-through cache seemed like a decent approach. I need to reread the code to remind myself of when exactly we benefit from the read-caching behavior (I know we have to do some operation on every userspace access, for determinism that is not sensitive to GC behavior), but Chip was pretty sure that we merely repeat the unserialize(), and that we do indeed get to cache the vatstoreRead.

@warner warner self-assigned this Dec 19, 2022
@otoole-brendan otoole-brendan added the vaults_triage DO NOT USE label Jan 12, 2023
@warner warner added the liveslots requires vat-upgrade to deploy changes label Jan 24, 2023
gibson042 pushed a commit that referenced this issue Mar 4, 2023
This makes room inside processUpgradeVat() for the new cleanup steps
that we want to add.

refs #6650
refs #7001
refs #6694
refs #6696
gibson042 pushed a commit that referenced this issue Mar 16, 2023
This makes room inside processUpgradeVat() for the new cleanup steps
that we want to add.

refs #6650
refs #7001
refs #6694
refs #6696
@mergify mergify bot closed this as completed in #7244 Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet vaults_triage DO NOT USE
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants