Snapshot-First Sandbox Lifecycle

[simongdavies]

I've moved this content to hackmd . I've moved Dorus and Lucys feedback (thanks) over there.

[dblnz]

This is awesome! I appreciate the effort to write this.
I think the new proposed API looks a lot cleaner.

Below are some comments that came to mind while reading:

• R2.7 - This assumes that the same external file is always used when loading a Snapshot, is this the intended behaviour? R1.8 mentions the OCI manifests, containing the companion blobs, but what about other use cases?
  R3.4 makes this clearer. By having the host application specifically map the external file, we can fail at that point in case something doesn't match.
• R1.9 - I am not sure that at the moment the memory profiling works, with the new cow changes, we need to check this out also.
• R4.4 - We need to measure this, I don't remember seeing a measurement for KVM
• R5.5 - This means that the user will have to redo snapshots from scratch (with all the layers) if we change the format version. Can we possibly make this more user-friendly to maybe somehow convert?
• R6 - Do I understand this correctly?
  Let's say we have a base snapshot we want to expand the heap for, and take a snapshot to save that change.
  The base defines the heap as, X, and, when loading, the host expands it to Y and takes a snapshot.
  As mentioned, the hash of the base only covers the original contents and is not invalidated, but the new snapshot's hash covers the expanded heap pages also, right?
• R10.5 - I think that it would also be helpful for one of the CLI tools to print what registered host function and guest functions are available for a snapshot (something like what wasm-tools does with the WIT imports/exports)
• One more thing we need to think about is how to handle debugging a snapshot (I already linked an issue with this one).
  Now we rely on an entrypoint breakpoint that makes the guest stop for the user to attach with the debugger and start the debugging session. This will have to change once we start working towards this proposal
• Q1-Q2 - I'm for the shorter extensions (.hls and .hll)
• Q3 - I don't have any insight here
• Q5 - I think having a deprecation period is nicer for our users

[syntactically]

In the ongoing discussion about the role of HIPs, I'll say that some of this (more the API design/implementation plan than the requirements) might make sense as one (at least in the "current" process).

I have a few comments on the requirements:

• R1.5: I would not want to commit to making snapshots loadable across hypervisors. Migrating across a hypervisor is probably pretty uncommon (people are pretty set in their choice of kvm/mshv) and it limits our ability to optimise things for one hypervisor (e.g. imagine that in the future we are able to move the initial RO page tables to the snapshot region on some hypervisor configurations but not others due to new APIs).
• R1.7: I'm not sure about optional compression. Compression in the format would make it impossible to mmap via the page cache, which seems slightly questionable for the fast loading goals here.
• R2.1-2.3: I'm not sure how important having a single standard CLI tool is, since I think a lot of things like hyperlight-wasm will want to do this programmatically in their higher-level tools that also do other stuff. However, I'm not particularly opposed to the idea either.
• R2.6: I'm not sure if I'm missing something here, but I think it would be good to support letting the user explicitly choose whether they want to squash everything down into a new base layer or build a new delta layer (or perhaps even do this selectively for different memory regions? not sure if there are use cases for selectivity).
• R2.7: I don't see much benefit to this, and I think it makes the whole design a lot more complex. I would be against this.
• R3.1: The CoW semantics are not provided by the host kernel based on the mmap, but rather by the guest code :)
• R3.4: As with comment on R2.7 above, I think this is probably a misfeature.
• R3.5: I assume the "snapshot cache" here is just in-memory and referring to caching the region obtained from mmap()? I'm not sure if we always want automatic unloading like this; it might be nice to have ways to manage the lifecycle manually, although this is a sane default.
• R4.1: The madvise is on the scratch mapping, not the snapshot mapping, and not strictly related to the snapshot format at all. I agree more validation around the approaches we are doing here would be good, but I think it's out of the scope of this issue?
• R4.2: Curious if there is any specific reason to prefer this. Is it just that we (currently) only support snapshotting with no calls inflight either direction, so we are basically guaranteed to be in a register state that semantically ought to be clean when we take the snapshot?
• R5.3: I am not sure if I am understanding correctly the proposed mechanism for loading delta layers. I would say that when the delta layer is baked, it should include a new version of the guest page tables which reconstructs the correct state from a situation where both the original layer and the delta layer are mapped at (different) GPA ranges. Then restore is just mapping both the original and delta layers and pointing the root page table pointer at the delta layer's page tables instead of the original's (copying to scratch if necessary, as is presently the case on amd64).
• R6.1: I don't think that heap management this way makes a lot of sense? I think that having the scratch region be resizable makes a lot of sense, but that doesn't really correlate well with heap growth because it's not about the total amount of information in the heap, but rather the number of pages that will be written to---it's fine to have a snapshot with 10MiB of read only heap data but resize the scratch region down to 1MiB as long as only 1MiB worth of CoW-causing writes happen. The guest heap should probably just be an arbitrarily (fixed) large region of virtual memory to which pages are demand-allocated.
• R7.2: Again skeptical of the need for non-snapshot/layer blobs.
• R9.1: We probably want at the very least a configuration knob to turn that off, since it may be quite slow
• R11: As with the comments above, I'm not totally convinced that this exemption from baking makes sense. Could you comment a bit more on motivation/use cases?

I might agree that these implementation phases make overall sense, but I'm not sure if our current priorities around e.g. getting wasm performance on the right track make them make sense. It could make sense to pull some of them, like the support for mmap-based load and restore and scratch region resizing, even ad-hoc and partially, up before the big API changes in order to enable us to quickly work on that use case.