Repository navigation
Conversation
|
@syntactically first draft at #1747 |
ludfjig
added
the
kind/enhancement
ludfjig
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a public MultiUseSandbox::recreate() API in hyperlight-host to rebuild an unrecoverable sandbox from scratch using its existing configuration and the snapshot that triggered the unrecoverable restore failure (addressing #1747’s “replace this sandbox with a new one with the same configuration” request).
Changes:
- Track additional lifecycle state needed for recreation (store the failed-restore snapshot and sandbox config when transitioning to
Unrecoverable). - Add
MultiUseSandbox::recreate(self) -> Result<Self>plus tests validating what state/config is preserved vs not preserved. - Update errors, changelog, and test recipes to cover the new recreation flow.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/src/sandbox/uninitialized_evolve.rs | Thread sandbox configuration into MultiUseSandbox::from_uninit so it can be reused for recreation. |
| src/hyperlight_host/src/sandbox/initialized_multi_use.rs | Introduce SandboxLifecycle, store recreation snapshot/config, and implement MultiUseSandbox::recreate() with accompanying tests. |
| src/hyperlight_host/src/sandbox/host_funcs.rs | Add an internal constructor to rebuild HostFunctions from a FunctionRegistry during recreation. |
| src/hyperlight_host/src/error.rs | Add recreation-specific error variants and adjust unrecoverable messaging. |
| Justfile | Ensure the new unrecoverable-mapping test is exercised in crashdump/coverage recipes. |
| CHANGELOG.md | Document the new MultiUseSandbox::recreate() API. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9e79d10 to
f806823
Compare
syntactically
left a comment
syntactically
left a comment
There was a problem hiding this comment.
I left a few comments inline.
I'm still trying to work out what the best API here is. It feels to me like it is a smaller/less breaking change to move this into restore() and add a new way to opt-out of the implicit recreate, but the possibility of recreation itself failing (which is probably pretty likely if we've recently failed to restore, since the relevant mapping failures seem likely to be resource exhaustion) is a little difficult to handle ergonomically. It seems inescapable to me that client code, if it is going to handle this locally, will need to do something like
while should_keep_trying() {
match sb.<restore/recreate/whatever>(snapshot) {
Ok(()) => break,
Err(HyperlightError::UnrecoverableSandbox) => {
// maybe try to free up resources somehow, try a different snapshot, etc
continue;
},
Err(e) => return e,
}which is just kind of nasty, whether you have one function for restore/recreate or multiple.
(The other option for clients I guess would be to just back out all the way to some top level retry when they get an unrecoverable error. That probably doesn't benefit at all from having recreate functionality at all, though...)
| self.0 | ||
| } | ||
|
|
||
| #[cfg(not(gdb))] |
There was a problem hiding this comment.
Why is so much stuff not(gdb) guarded?
| /// again on the rebuilt sandbox. | ||
| /// | ||
| /// Existing interrupt handles do not apply to the rebuilt sandbox. Call | ||
| /// [`interrupt_handle()`](Self::interrupt_handle) on the rebuilt sandbox to |
There was a problem hiding this comment.
This seems like a nasty footgun. Is there really no way to get around this? I guess we should just need to update the partition handle here on the WHP sandboxes? I would expect the linux/hvf ones might just work, since the tid update happens just before run? If the latter is true, maybe we can just check/update the partition id on the whp sandboxes before run so that the lifecycle is the same?
There was a problem hiding this comment.
Fixed! I didn't realize it would be possible
| if let Err(error) = self.restore_memory_and_mappings(&snapshot) { | ||
| self.status = SandboxStatus::Unrecoverable; | ||
| self.lifecycle = SandboxLifecycle::Unrecoverable { | ||
| recreation_snapshot: snapshot, |
There was a problem hiding this comment.
Why do we want to save the snapshot here rather than in self.snapshot?
There was a problem hiding this comment.
changed so recreate now takes snapshot as parameter
| Poisoned, | ||
| Unrecoverable { | ||
| #[cfg_attr(gdb, allow(dead_code))] | ||
| recreation_snapshot: Arc<Snapshot>, |
There was a problem hiding this comment.
I think with the current lifecycle of MultiUseSandbox::snapshot, this means we can only ever transition into the unrecoverable state from restore(), since we are not guaranteed to have a valid snapshot around otherwise? That doesn't seem right to me---surely we should be able to end up in unrecoverable from e.g. map_region as well, even if we don't have a current valid snapshot? Like with poisoned, I would expect that the new snapshot should be supplied on the recreate operation?
There was a problem hiding this comment.
fixed, recreate now takes snapshot as param, just like restore
recreate() replaces a sandbox's VM and memory with new ones built from a snapshot. It recovers unrecoverable sandboxes. A failed recreate leaves the sandbox unchanged, so it can be retried. The sandbox keeps its configuration, host functions, page table root finder, guest log level, and interrupt handles. Existing interrupt handles control the new VM. Sandbox stores its configuration, which replaces the separate guest log level field. Closes hyperlight-dev#1747 Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
f806823 to
d37d029
Compare
ludfjig
changed the title
This comment has been minimized.
This comment has been minimized.
borsh-derive still depended on syn 3.0.6 after the syn bump to 3.0.8 removed that version from the lock file. The two dependency bumps merged separately, which left Cargo.lock out of date and made `cargo fetch --locked` fail. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Benchmark ResultsMeasured commit: kvm / amd (Linux) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
kvm / intel (Linux) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
mshv3 / amd (Linux) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
mshv3 / intel (Linux) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
hyperv-ws2025 / amd (Windows) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
hyperv-ws2025 / intel (Windows) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
Reported by |
Adds
Sandbox::recreate(&mut self, snapshot), which replaces a sandbox's VM and memory with new ones built fromsnapshot. It recovers sandboxes leftUnrecoverableby a failed restore.restore(), recreation removes regions added withmap_region()ormap_file_cow().Sandboxstores its configuration. It replaces the separate guest log level field.On Windows the new VM needs a surrogate process, as when creating any sandbox. Recreation fails with
HYPERLIGHT_MAX_SURROGATES=0and blocks while all surrogates are in use. The rustdoc documents this.Closes #1747