fix(microvm): honour the status filter in list_instances - #202
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RuntimeLifecycle::list_instancestakes astatusfilter that both microVM runtimes ignored: Cloud Hypervisor always scanned forRunning, Firecracker returned every instance with a socket on disk — including the stale sockets a crashed VM leaves behind, which then looked like running instances.Both now honour it, using the vocabulary the trait's callers already speak (Docker's):
allaccepts everything,activemeans running-or-coming-up, anything else is an exact state match.activeresolves to the same trio the runtime already treats as "alive or about to be" when scaling and when running a job. This is deliberately wider than Docker'sactive, which excludescreated: a CH VM sits inCreatedbetween spawn and boot, and dropping it there would make a freshly-started replica briefly invisible.allskips the per-instance API round trip entirely.activeandrunningcollapse onto that,exitedis its negation. Liveness is resolved with a single/procpass per listing rather than one per instance, and keyed on the full socket path: two servers with differentsocket_dirs can mint the same instance id, and matching on the basename alone would let one mark the other's stale socket as live.An unrecognised filter matches nothing on both runtimes. Falling back to "everything" would misreport a deployment as fully up.
One caller had to change with it. The rolling update refreshed the parent's instance list with
active, which now correctly excludes dead instances — and that would have been a regression: the drain is what releases each instance's host resources (tap, rootfs copy, console logs, temp volumes), and a dead-but-not-reaped instance still holds all of them. An exhausted parent would have looked empty and been marked deleted without ever callingremove_instance. It now asks forall. This also fixes the symmetric leak on Docker, where anexitedcontainer was never reaped by the rolling update either.Tests
779 unit tests pass (8 new);
cargo fmtandcargo clippy --all-targetsare clean. The new tests cover both filter mappings, the unknown-filter case, and the exact-path matching the liveness index relies on. The listing paths themselves need running VMs and are only exercised by the e2e suites, which do not run in CI.