Propolis zone cleanup could happen outside of `InstanceRunner`
Nobody has claimed this yet.
Assessment
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Newbie friendliness
- 45/100
- Issue type
- Refactor
- Clarity
- Mostly clear
- Activity status
- Stale
- Tech stack
- rust
- Domain
- backend, infrastructure
Research direction
Start in sled-agent/src/instance.rs, especially the state handling around lines 374-389 and 600-615 and termination around 759-793. Trace how VMM shutdown currently collects the zone bundle, tears down the zone, deregisters the instance, and publishes state; done means the runner can return promptly while deferring cleanup without breaking instance state updates or resource teardown.
Written by the indexing model from the issue text.
Description
Every instance managed by a sled agent has a "runner" task and a "monitor" task. All requests to do anything with the instance (change its state, forcibly remove it from the sled agent, etc.) need to execute on the runner task, which handles these requests sequentially. This includes the processing of state change messages from Propolis, which are sent from the monitor task to the runner task and processed here: https://github.com/oxidecomputer/omicron/blob/8697f39aac3323ffda3f33f4b5aa44fd7ad857e3/sled-agent/src/instance.rs#L374-L389
If Propolis indicated that it shut down, InstanceRunner::observe_state will call InstanceRunner::terminate before returning and allowing new state to be published to Nexus: https://github.com/oxidecomputer/omicron/blob/8697f39aac3323ffda3f33f4b5aa44fd7ad857e3/sled-agent/src/instance.rs#L600-L615
Terminating an instance this way collects a zone bundle from the runner thread before removing the instance from the sled agent's InstanceManager and tearing down the zone: https://github.com/oxidecomputer/omicron/blob/8697f39aac3323ffda3f33f4b5aa44fd7ad857e3/sled-agent/src/instance.rs#L759-L793
Creating a zone bundle may be an expensive operation, since it (potentially) has to copy and compress many different log files and command outputs. This causes a couple of problems:
- Instance stop will appear to take much longer than it actually does, because the relevant state transition doesn't go to Nexus until the zone is destroyed.
- More importantly, the instance remains in the sled agent's instance table, but the
InstanceRunneris completely blocked while all this work is going on. This means that sled agent API calls targeting an instance in this state are highly likely to time out waiting for the runner to respond. If the caller is Nexus and the request is to change the instance's state, this can cause instances to be marked as Failed (due to the error conversion rules described in #3238) even though they would correctly go to Stopped if left alone.
The reason I made these operations happen in this order (zone bundle collection -> zone teardown -> deregister instance -> publish to Nexus) was to try to mitigate #3325. I suspect, though, that that issue is not as much of a problem now that every newly-started instance gets a fresh Propolis ID (not the case prior to #4194), such that every incarnation of an instance on a sled will get a distinct zone name. If that's so, then it should be possible to mitigate these problems by changing what happens on VMM shutdown: the runner can remove the instance from the table, publish the new VMM state to Nexus, and then hand the defunct zone off to some other task to be cleaned up.[^1]
[^1]: Cleaning up the defunct zone outside of the InstanceRunner task allows that task to return immediately and produce a "runner task closed" error message for anyone who happened to have requested something of the runner while it was in the "observe state change" arm of its request handler. If zone cleanup happens on the runner, Nexus's instance state will still update right away, but calls that land in this window are much more likely to time out than to get a "clean" instance gone error.
- Dominant language
- Rust
- Stars
- 572
- Forks
- 97
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 96
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from oxidecomputer/omicron
-
Difficulty 1/5 Under an hour Newbie friendliness 88/100
oxidecomputer/omicron#11269 ·
-
Difficulty 1/5 Under an hour Newbie friendliness 92/100
oxidecomputer/omicron#11266 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
oxidecomputer/omicron#11260 · 1 comment ·
-
wicket's errors should be better when trying to read sensitive data from ssh without a pseudo-tty Open
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
oxidecomputer/omicron#11148 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
oxidecomputer/omicron#10907 ·
All issues in oxidecomputer/omicron
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 86/100
kwakseongjae/auto-hwp#319 ·
-
area:cli bug filter-quality good first issue priority:medium
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
-
Difficulty 1/5 Under an hour Newbie friendliness 72/100
bevyengine/bevy#25861 ·
-
comp-datalake
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
ClickHouse/ClickHouse#121222 ·
-
enhancement remote
Difficulty 2/5 1-3 hours Newbie friendliness 68/100