diff --git a/src/runtime/cloud_hypervisor/lifecycle.rs b/src/runtime/cloud_hypervisor/lifecycle.rs index 54e13f6..52236b4 100644 --- a/src/runtime/cloud_hypervisor/lifecycle.rs +++ b/src/runtime/cloud_hypervisor/lifecycle.rs @@ -1141,6 +1141,43 @@ impl CloudHypervisorLifecycle { } } +/// Translate the runtime-agnostic `status` filter of +/// [`RuntimeLifecycle::list_instances`] into the Cloud Hypervisor VM states that +/// satisfy it. `None` means "no filter" (the `all` case). +/// +/// The filter vocabulary is Docker's, since that is what the trait's callers +/// speak: `all` accepts everything, `active` means running-or-coming-up, and +/// anything else is an exact state match. +/// +/// `active` maps to the same trio this runtime already treats as "alive or +/// about to be" when scaling and when running a job — the counterpart of +/// Docker's running/restarting. It is deliberately wider than Docker's +/// `active`, which excludes `created`: a CH VM sits in `Created` for a moment +/// between spawn and boot, and dropping it there would make a freshly-started +/// replica briefly invisible. +/// +/// This used to be hardcoded to `["Running"]` regardless of the argument, so a +/// caller asking for `all` (or for a stopped instance) silently got only the +/// running ones. +fn ch_states_for_status(status: &str) -> Option> { + match status { + "all" => None, + "active" => Some(vec!["Running", "Created", "Booting"]), + "running" => Some(vec!["Running"]), + "exited" | "stopped" => Some(vec!["Shutdown"]), + // An explicit CH state name, passed through as-is. + "Created" => Some(vec!["Created"]), + "Booting" => Some(vec!["Booting"]), + "Running" => Some(vec!["Running"]), + "Shutdown" => Some(vec!["Shutdown"]), + "Paused" => Some(vec!["Paused"]), + "BreakPoint" => Some(vec!["BreakPoint"]), + // Unknown filter: match nothing rather than silently returning + // everything, which would misreport a deployment as fully up. + _ => Some(Vec::new()), + } +} + fn parse_resources(deployment: &Deployment) -> (u32, u32) { let mut vcpus = 1u32; let mut memory_mb = 256u32; @@ -1190,8 +1227,13 @@ impl RuntimeLifecycle for CloudHypervisorLifecycle { } } - async fn list_instances(&self, deployment_id: String, _status: &str) -> Vec { - self.scan_instances(&deployment_id, &["Running"]).await + async fn list_instances(&self, deployment_id: String, status: &str) -> Vec { + match ch_states_for_status(status) { + Some(states) => self.scan_instances(&deployment_id, &states).await, + // "all" — no state filter at all, which also skips one API round + // trip per instance. + None => self.scan_instances(&deployment_id, &[]).await, + } } async fn remove_instance(&self, instance_id: String) -> bool { @@ -1740,6 +1782,41 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } + /// The status argument used to be ignored — `list_instances` always scanned + /// for `Running` only, so a caller asking for `all` (or for a stopped + /// instance) silently got just the running ones. + #[test] + fn status_filter_maps_to_ch_states() { + // `all` means no state filter at all, which also skips one API round + // trip per instance. + assert_eq!(ch_states_for_status("all"), None); + // `active` is running-or-coming-up, mirroring Docker's + // running/restarting intent. + assert_eq!( + ch_states_for_status("active"), + Some(vec!["Running", "Created", "Booting"]) + ); + assert_eq!(ch_states_for_status("running"), Some(vec!["Running"])); + assert_eq!(ch_states_for_status("exited"), Some(vec!["Shutdown"])); + assert_eq!(ch_states_for_status("stopped"), Some(vec!["Shutdown"])); + } + + /// A raw CH state name passes through, so a caller can ask for exactly one. + #[test] + fn explicit_ch_state_names_pass_through() { + for state in ["Created", "Booting", "Running", "Shutdown"] { + assert_eq!(ch_states_for_status(state), Some(vec![state])); + } + } + + /// An unrecognised filter must match nothing rather than fall back to + /// everything, which would misreport a deployment as fully up. + #[test] + fn unknown_status_filter_matches_nothing() { + assert_eq!(ch_states_for_status("paused"), Some(Vec::new())); + assert_eq!(ch_states_for_status(""), Some(Vec::new())); + } + #[test] fn classify_vm_start_error_terminal_vs_transient() { let (s, r) = classify_vm_start_error(&RuntimeError::FirmwareNotFound("/x".into())); diff --git a/src/runtime/firecracker/lifecycle.rs b/src/runtime/firecracker/lifecycle.rs index 7c9a46a..e6982c2 100644 --- a/src/runtime/firecracker/lifecycle.rs +++ b/src/runtime/firecracker/lifecycle.rs @@ -1268,6 +1268,72 @@ fn find_pid_by_socket(socket_path: &str) -> Option { None } +/// Every API socket path currently served by a live `firecracker` process, read +/// in one pass over `/proc`. +/// +/// Filtering a listing by liveness would otherwise call `instance_alive` per +/// instance, and each of those walks all of `/proc` — N full scans for N +/// replicas. One scan answers the question for every instance at once. +/// +/// Keyed on the full socket path, not the instance id: two `ring-server` +/// instances with different `socket_dir`s can mint the same instance id, and +/// matching on the basename alone would let one of them mark the other's stale +/// socket as live. This mirrors the exact-path comparison `instance_alive` does. +fn live_socket_paths() -> std::collections::HashSet { + let mut live = std::collections::HashSet::new(); + let Ok(entries) = std::fs::read_dir("/proc") else { + return live; + }; + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(pid) = name.to_str().and_then(|s| s.parse::().ok()) else { + continue; + }; + let Ok(cmdline) = std::fs::read(format!("/proc/{}/cmdline", pid)) else { + continue; + }; + if let Some(path) = socket_path_from_cmdline(&cmdline) { + live.insert(path); + } + } + live +} + +/// Translate the runtime-agnostic `status` filter of +/// [`RuntimeLifecycle::list_instances`] into the liveness an instance must have +/// to satisfy it: `Some(true)` for alive, `Some(false)` for dead, `None` for no +/// filter at all. +/// +/// The filter vocabulary is Docker's, since that is what the trait's callers +/// speak. Firecracker exposes no VM-state API, so the only observable +/// distinction is whether a live process still backs the instance — `active` and +/// `running` collapse onto the same thing here, and `exited` is its negation. +/// +/// The argument used to be ignored entirely, so every caller got every instance +/// with a socket on disk, including the stale sockets crashed VMs leave behind. +fn fc_liveness_for_status(status: &str) -> FcFilter { + match status { + "all" => FcFilter::Any, + "active" | "running" => FcFilter::Alive(true), + "exited" | "stopped" => FcFilter::Alive(false), + // Unknown filter: match nothing. Falling back to "everything" would + // misreport a deployment as fully up, and falling back to "dead" would + // be just as wrong — neither is what the caller asked for. + _ => FcFilter::None, + } +} + +/// What [`fc_liveness_for_status`] resolved a status filter to. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum FcFilter { + /// No filtering — every instance with a socket on disk. + Any, + /// Keep instances whose liveness matches the flag. + Alive(bool), + /// Unrecognised filter: keep nothing. + None, +} + /// Is a live `firecracker` process currently serving the instance that owns /// `tap_name`? Scans `/proc` for firecracker processes and re-derives each /// one's tap name from the instance id embedded in its `--api-sock` path. @@ -1325,21 +1391,25 @@ fn live_vm_uses_tap(tap_name: &str, booting_instance_id: &str) -> bool { false } -/// The instance id of the firecracker process described by `cmdline`, taken from -/// the file stem of its `--api-sock` argument (`/run/fc/.sock`). +/// The API socket path of the firecracker process described by `cmdline`. /// `None` when this is not a firecracker process or carries no socket argument. -fn instance_id_from_cmdline(cmdline: &[u8]) -> Option { +fn socket_path_from_cmdline(cmdline: &[u8]) -> Option { let mut args = cmdline.split(|&b| b == 0); if !args.next()?.ends_with(b"firecracker") { return None; } - for arg in args { - let s = std::str::from_utf8(arg).ok()?; - if let Some(stem) = s.strip_suffix(".sock") { - return stem.rsplit('/').next().map(|id| id.to_string()); - } - } - None + args.filter_map(|arg| std::str::from_utf8(arg).ok()) + .find(|s| s.ends_with(".sock")) + .map(|s| s.to_string()) +} + +/// The instance id of the firecracker process described by `cmdline`, taken from +/// the file stem of its `--api-sock` argument (`/run/fc/.sock`). +/// `None` when this is not a firecracker process or carries no socket argument. +fn instance_id_from_cmdline(cmdline: &[u8]) -> Option { + let path = socket_path_from_cmdline(cmdline)?; + let stem = path.strip_suffix(".sock")?; + stem.rsplit('/').next().map(|id| id.to_string()) } /// Does this `/proc//cmdline` (NUL-separated argv) belong to a @@ -1473,8 +1543,32 @@ impl RuntimeLifecycle for FirecrackerLifecycle { } } - async fn list_instances(&self, deployment_id: String, _status: &str) -> Vec { - self.scan_instances(&deployment_id) + async fn list_instances(&self, deployment_id: String, status: &str) -> Vec { + let ids = self.scan_instances(&deployment_id); + match fc_liveness_for_status(status) { + // "all" — every instance with a socket on disk, alive or not. + FcFilter::Any => ids, + // Firecracker has no VM-state API: an instance is either backed by a + // live process or it is not. `instance_alive` is that distinction, + // and it also filters out the stale sockets a crashed VM leaves + // behind — which a bare socket scan would report as running. + FcFilter::Alive(want) => { + // Index /proc once for the whole listing. Calling + // `instance_alive` per id would rescan every process for each + // instance — N full /proc walks for N replicas. + let live = live_socket_paths(); + ids.into_iter() + .filter(|id| { + // Same test as `instance_alive`: the socket is on disk + // AND a live process is bound to that exact path. + let socket = self.socket_path(id); + let alive = Path::new(&socket).exists() && live.contains(&socket); + alive == want + }) + .collect() + } + FcFilter::None => Vec::new(), + } } async fn remove_instance(&self, instance_id: String) -> bool { @@ -1951,6 +2045,55 @@ mod tests { assert!(cmdline_matches_socket(cmd, sock)); } + /// The status argument used to be ignored, so every caller got every + /// instance with a socket on disk — including the stale sockets crashed VMs + /// leave behind, which then looked like running instances. + #[test] + fn status_filter_maps_to_liveness() { + assert_eq!(fc_liveness_for_status("all"), FcFilter::Any); + // Firecracker has no VM-state API, so active and running are the same + // observable thing: a live process backs the instance. + assert_eq!(fc_liveness_for_status("active"), FcFilter::Alive(true)); + assert_eq!(fc_liveness_for_status("running"), FcFilter::Alive(true)); + assert_eq!(fc_liveness_for_status("exited"), FcFilter::Alive(false)); + assert_eq!(fc_liveness_for_status("stopped"), FcFilter::Alive(false)); + } + + /// Liveness is keyed on the full socket path, not the instance id: two + /// `ring-server` instances with different `socket_dir`s can mint the same + /// id, and matching on the basename alone would let one mark the other's + /// stale socket as live. + #[test] + fn socket_path_is_captured_whole() { + let cmd = b"/usr/bin/firecracker\0--api-sock\0/run/fc/dep-1-aaa.sock\0"; + assert_eq!( + socket_path_from_cmdline(cmd).as_deref(), + Some("/run/fc/dep-1-aaa.sock"), + "the directory must be kept, not just the file name" + ); + // Same instance id under a different socket_dir is a different socket. + let other = b"/usr/bin/firecracker\0--api-sock\0/var/run/other/dep-1-aaa.sock\0"; + assert_ne!( + socket_path_from_cmdline(cmd), + socket_path_from_cmdline(other) + ); + } + + #[test] + fn socket_path_ignores_non_firecracker_processes() { + let cmd = b"/usr/bin/socat\0--api-sock\0/run/fc/dep-1-aaa.sock\0"; + assert_eq!(socket_path_from_cmdline(cmd), None); + } + + /// An unrecognised filter must match nothing. Returning everything would + /// report a deployment as fully up; returning the dead ones would be just as + /// wrong. Neither is what the caller asked for. + #[test] + fn unknown_status_filter_matches_nothing() { + assert_eq!(fc_liveness_for_status("paused"), FcFilter::None); + assert_eq!(fc_liveness_for_status(""), FcFilter::None); + } + /// The instance id is what lets a live VM be matched to the tap it owns, so /// a running guest's interface is never reclaimed. It comes from the socket /// argument's file stem. diff --git a/src/scheduler/scheduler.rs b/src/scheduler/scheduler.rs index cb71ded..a25e4b9 100644 --- a/src/scheduler/scheduler.rs +++ b/src/scheduler/scheduler.rs @@ -958,8 +958,14 @@ async fn handle_rolling_update( } }; - // Refresh parent's live instance list. - parent.instances = runtime.list_instances(parent.id.clone(), "active").await; + // Refresh the parent's instance list. Deliberately `all`, not `active`: the + // drain below 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. Listing only the live ones would make an + // exhausted parent look empty, and the finalization below would mark it + // deleted without ever calling `remove_instance` — leaking everything a + // crashed VM left behind. + parent.instances = runtime.list_instances(parent.id.clone(), "all").await; // Drain one instance per cycle if the parent still has some. If the // remove fails, bail — the next cycle will retry.