From 718b108c9ed90d45f73a8649fbdce87fce8b9449 Mon Sep 17 00:00:00 2001 From: Mlanawo MBECHEZI Date: Sat, 25 Jul 2026 08:59:56 +0300 Subject: [PATCH] fix(runtime): share the failure classifier across containerd and the microVM runtimes --- .../concepts/deployment-status-lifecycle.md | 2 + documentation/concepts/runtimes.md | 5 +- src/hypervisor/classifier.rs | 287 +++++++++++++++++- src/runtime/cloud_hypervisor/lifecycle.rs | 64 +--- src/runtime/containerd/lifecycle.rs | 152 +++++++++- src/runtime/firecracker/lifecycle.rs | 89 ++++-- 6 files changed, 513 insertions(+), 86 deletions(-) diff --git a/documentation/concepts/deployment-status-lifecycle.md b/documentation/concepts/deployment-status-lifecycle.md index f0f3f11..7b9000c 100644 --- a/documentation/concepts/deployment-status-lifecycle.md +++ b/documentation/concepts/deployment-status-lifecycle.md @@ -91,6 +91,8 @@ Ring tracks a cumulative `restart_count` per deployment. It is bumped when: Once `restart_count` reaches `MAX_RESTART_COUNT` (5), the next tick flips a **worker** to `crash_loop_back_off` (terminal) and a **job** to `failed` (terminal): the reconciler stops retrying, protecting the host from a tight crash loop. The counter is **cumulative for the deployment's lifetime**, not a sliding window; `ring apply` with a fixed manifest resets it. +**Permanent failures skip the budget.** Some failures cannot fix themselves on a retry: the image genuinely doesn't exist, a referenced config or key is absent, the container/VM spec is rejected, the Firecracker kernel or Cloud Hypervisor firmware is missing at its configured path, or the host is out of memory. Rather than bumping the counter by one and burning five reconcile cycles to reach the same conclusion, Ring classifies these as terminal and lands on the matching status (`image_pull_back_off`, `config_error`, `create_container_error`, `failed`, `insufficient_resources`) on the next tick. Transient failures — a pull that died mid-flight, a busy port, a network setup race — still bump by one and retry within the budget. The classification is shared by every runtime, so Docker, Podman, containerd, Cloud Hypervisor and Firecracker converge identically. + Counters live in memory only, so restarting `ring server` clears them, so each `(deployment, instance, check)` triple starts back at zero after a server restart. ## Observing the status diff --git a/documentation/concepts/runtimes.md b/documentation/concepts/runtimes.md index 10bdcfc..41a505f 100644 --- a/documentation/concepts/runtimes.md +++ b/documentation/concepts/runtimes.md @@ -28,7 +28,7 @@ Enable just the runtimes a host actually runs: Docker-only, Podman-only, contain | Crash detection | ✓ event-driven (sub-second) | ✓ reconcile-based (per scheduler tick) | ✓ reconcile-based (per scheduler tick) | ✓ reconcile-based (per scheduler tick) | ✓ reconcile-based (per scheduler tick) | | `command` health checks | `docker exec` | `podman exec` (same API) | `Tasks.Exec` (gRPC) | In-guest `ring-agent` over AF_VSOCK | In-guest `ring-agent` over vsock (host Unix socket) | | `kind: job` | Exit code visible | Exit code visible | Exit code visible | Clean shutdown = success (no exit code from host) | Clean shutdown (guest reboot) = success (no exit code from host) | -| Labels (`labels:`) | Forwarded to container | Forwarded to container | Forwarded to container | Silently ignored | Silently ignored | +| Labels (`labels:`) | Stored by Ring, forwarded to the container | Stored by Ring, forwarded to the container | Stored by Ring, forwarded to the container | Stored by Ring, not applied to the VM | Stored by Ring, not applied to the VM | | Host networking | Supported | Not supported by Ring yet | Not supported by Ring yet | N/A | N/A | | Private registry creds | Supported | Supported | Supported (basic auth) | N/A (no image pull) | N/A (no image pull) | @@ -154,7 +154,8 @@ checks via the in-guest `ring-agent` over vsock, `kind: job` run-to-completion, `volumes:` mounted as virtio-block ext4 images. **Current limitations (experimental):** -- Crash detection is tick-bound (no event stream), and `labels` are silently ignored +- Crash detection is tick-bound (no event stream), and `labels`, while stored and + filterable as Ring metadata, are not applied to the VM itself A `ring-server` restart is transparent: running microVMs (and their persistent host taps) survive it, and the reconciler re-adopts them (re-deriving each instance's network from its id and re-spawning the host port-forwarders the old process took down) so a deployment keeps its guest state and its published ports across a restart. diff --git a/src/hypervisor/classifier.rs b/src/hypervisor/classifier.rs index b40ae6a..b9259a5 100644 --- a/src/hypervisor/classifier.rs +++ b/src/hypervisor/classifier.rs @@ -17,8 +17,10 @@ //! //! It generalises the Cloud Hypervisor runtime's `classify_vm_start_error`. It is //! deliberately runtime-agnostic (it only reads the shared [`RuntimeError`] enum -//! and a raw exit code) so containerd and the VM runtimes can adopt it next; for -//! now only the Docker runtime is wired to it. +//! and a raw exit code): Docker and Podman go through [`classify_create_error`] +//! and [`classify_exit_code`] directly, while the VM runtimes (Cloud Hypervisor, +//! Firecracker) share [`classify_vm_start_error`], which layers an event reason +//! on top of the same verdict. use crate::hypervisor::error::RuntimeError; use crate::models::deployments::DeploymentStatus; @@ -59,6 +61,12 @@ pub(crate) fn classify_create_error(err: &RuntimeError) -> Disposition { // Permanent: Docker rejecting `create`/`start` almost always means a bad // container spec (entrypoint, mount, options) — retrying re-submits the // same rejected spec. Fail fast onto CreateContainerError. + // + // Caveat for adopters: this verdict assumes the variant carries a + // *rejected spec*, as it does on Docker. The containerd runtime reuses it + // for any failed gRPC call in the create path (including a transient shim + // outage), so it deliberately overrides this one case back to Retry — see + // `containerd::lifecycle::handle_create_error`. RuntimeError::InstanceCreationFailed(_) => { Disposition::Terminal(DeploymentStatus::CreateContainerError) } @@ -115,6 +123,74 @@ pub(crate) fn classify_exit_code(exit_code: Option) -> Disposition { } } +/// Classify a VM start failure for the microVM runtimes (Cloud Hypervisor, +/// Firecracker), returning the terminal status to land on — or `None` when the +/// failure is transient and should bump `restart_count` — alongside the event +/// reason to record. +/// +/// The verdict comes from [`classify_create_error`], so the VM runtimes converge +/// exactly like Docker: a missing config, a rejected instance spec or absent +/// firmware fails fast instead of burning the whole restart budget. Only the +/// event reason is runtime-flavoured, which is why this wrapper exists at all. +pub(crate) fn classify_vm_start_error( + e: &RuntimeError, +) -> (Option, &'static str) { + let reason = match e { + RuntimeError::FirmwareNotFound(_) => "FirmwareNotFound", + RuntimeError::ImageNotFound(_) => "ImageNotFound", + RuntimeError::ImagePullFailed(_) => "ImagePullFailed", + RuntimeError::InstanceCreationFailed(_) => "InstanceCreationFailed", + RuntimeError::ConfigNotFound(_) | RuntimeError::ConfigKeyNotFound(_) => "ConfigError", + RuntimeError::InsufficientResources(_) => "insufficient_resources", + RuntimeError::PortAlreadyInUse(_) => "PortAllocationFailed", + RuntimeError::NetworkCreationFailed(_) => "NetworkCreationFailed", + _ => "VmStartFailed", + }; + + let status = match classify_create_error(e) { + Disposition::Terminal(status) => Some(status), + Disposition::Retry => None, + }; + + (status, reason) +} + +/// Apply a VM start failure to `deployment`: record the event, then either land +/// on the terminal status or consume one unit of the restart budget. +/// +/// `bound_status` is where a *transient* failure converges once the budget runs +/// out — `CrashLoopBackOff` for a worker, `Failed` for a job. +/// +/// Both branches must move `restart_count`. Several terminal statuses +/// (`image_pull_back_off`, `config_error`, `create_container_error`) are still +/// polled by the reconcile loop, so setting the status without pushing the +/// counter to the bound would retry a permanent failure on every tick forever — +/// exactly the loop this classifier exists to stop. +pub(crate) fn apply_vm_start_failure( + deployment: &mut crate::models::deployments::Deployment, + err: &RuntimeError, + runtime: &str, + bound_status: DeploymentStatus, +) { + use crate::models::deployments::MAX_RESTART_COUNT; + + let (status, reason) = classify_vm_start_error(err); + deployment.emit_event("error", format!("{}", err), runtime, Some(reason)); + + match status { + Some(terminal) => { + deployment.restart_count = MAX_RESTART_COUNT; + deployment.status = terminal; + } + None => { + deployment.restart_count += 1; + if deployment.restart_count >= MAX_RESTART_COUNT { + deployment.status = bound_status; + } + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -200,6 +276,213 @@ mod tests { ); } + #[test] + fn vm_start_missing_firmware_is_terminal_failed() { + // Firecracker used to fall through to the catch-all here and retry a + // missing kernel/rootfs five times over. + assert_eq!( + classify_vm_start_error(&RuntimeError::FirmwareNotFound("/no/kernel".into())), + (Some(DeploymentStatus::Failed), "FirmwareNotFound") + ); + } + + #[test] + fn vm_start_config_errors_are_terminal() { + assert_eq!( + classify_vm_start_error(&RuntimeError::ConfigNotFound("c".into())), + (Some(DeploymentStatus::ConfigError), "ConfigError") + ); + assert_eq!( + classify_vm_start_error(&RuntimeError::ConfigKeyNotFound("k".into())), + (Some(DeploymentStatus::ConfigError), "ConfigError") + ); + } + + #[test] + fn vm_start_instance_creation_failed_is_terminal() { + assert_eq!( + classify_vm_start_error(&RuntimeError::InstanceCreationFailed("bad spec".into())), + ( + Some(DeploymentStatus::CreateContainerError), + "InstanceCreationFailed" + ) + ); + } + + #[test] + fn vm_start_transient_errors_have_no_terminal_status() { + assert_eq!( + classify_vm_start_error(&RuntimeError::PortAlreadyInUse(8080)), + (None, "PortAllocationFailed") + ); + assert_eq!( + classify_vm_start_error(&RuntimeError::VmStartFailed("boot".into())), + (None, "VmStartFailed") + ); + assert_eq!( + classify_vm_start_error(&RuntimeError::ImagePullFailed("net".into())), + (None, "ImagePullFailed") + ); + } + + #[test] + fn vm_start_matches_create_error_verdict() { + // The two entry points must never drift: same error, same terminal-ness. + let errors = [ + RuntimeError::FirmwareNotFound("f".into()), + RuntimeError::ImageNotFound("i".into()), + RuntimeError::ImagePullFailed("p".into()), + RuntimeError::InstanceCreationFailed("c".into()), + RuntimeError::ConfigNotFound("c".into()), + RuntimeError::InsufficientResources("m".into()), + RuntimeError::PortAlreadyInUse(1), + RuntimeError::NetworkCreationFailed("n".into()), + RuntimeError::Other("x".into()), + ]; + for err in &errors { + let (status, _) = classify_vm_start_error(err); + assert_eq!( + status.is_some(), + classify_create_error(err).is_terminal(), + "verdict drifted for {:?}", + err + ); + } + } + + fn vm_deployment() -> crate::models::deployments::Deployment { + crate::models::deployments::Deployment { + id: "vm1".to_string(), + created_at: chrono::Utc::now().to_string(), + updated_at: None, + status: DeploymentStatus::Creating, + restart_count: 0, + namespace: "test".to_string(), + name: "vm".to_string(), + image: "/var/lib/ring/rootfs.ext4".to_string(), + config: None, + runtime: "firecracker".to_string(), + kind: "worker".to_string(), + replicas: 1, + command: vec![], + instances: vec![], + labels: std::collections::HashMap::new(), + environment: std::collections::HashMap::new(), + volumes: "[]".to_string(), + health_checks: vec![], + resources: None, + image_digest: None, + ports: vec![], + pending_events: vec![], + parent_id: None, + network: None, + } + } + + /// The invariant that stops the infinite reboot loop: whichever branch is + /// taken, `restart_count` must move. Several terminal statuses are still + /// polled by the reconcile loop, so a terminal verdict that left the counter + /// at zero would retry a permanent failure on every tick, forever. + #[test] + fn every_start_failure_moves_the_restart_counter() { + let errors = [ + RuntimeError::FirmwareNotFound("f".into()), + RuntimeError::ImageNotFound("i".into()), + RuntimeError::ConfigNotFound("c".into()), + RuntimeError::InsufficientResources("m".into()), + RuntimeError::PortAlreadyInUse(1), + RuntimeError::VmStartFailed("boot".into()), + ]; + for err in &errors { + let mut deployment = vm_deployment(); + apply_vm_start_failure( + &mut deployment, + err, + "firecracker", + DeploymentStatus::CrashLoopBackOff, + ); + assert!( + deployment.restart_count > 0, + "counter stayed at zero for {:?} — the deployment would retry forever", + err + ); + } + } + + /// A permanent failure lands on its status and exhausts the budget at once, + /// so the very next tick is terminal instead of the fifth. + #[test] + fn terminal_start_failure_jumps_to_the_bound() { + let mut deployment = vm_deployment(); + apply_vm_start_failure( + &mut deployment, + &RuntimeError::FirmwareNotFound("/no/vmlinux".into()), + "firecracker", + DeploymentStatus::CrashLoopBackOff, + ); + + assert_eq!( + deployment.restart_count, + crate::models::deployments::MAX_RESTART_COUNT + ); + assert_eq!(deployment.status, DeploymentStatus::Failed); + } + + /// A transient failure keeps its one-per-tick budget and converges to the + /// caller's bound status (CrashLoopBackOff for a worker, Failed for a job). + #[test] + fn transient_start_failure_converges_to_the_bound_status() { + let mut deployment = vm_deployment(); + let max = crate::models::deployments::MAX_RESTART_COUNT; + + for tick in 1..max { + apply_vm_start_failure( + &mut deployment, + &RuntimeError::VmStartFailed("boot timeout".into()), + "firecracker", + DeploymentStatus::CrashLoopBackOff, + ); + assert_eq!(deployment.restart_count, tick); + assert_ne!(deployment.status, DeploymentStatus::CrashLoopBackOff); + } + + apply_vm_start_failure( + &mut deployment, + &RuntimeError::VmStartFailed("boot timeout".into()), + "firecracker", + DeploymentStatus::CrashLoopBackOff, + ); + assert_eq!(deployment.restart_count, max); + assert_eq!(deployment.status, DeploymentStatus::CrashLoopBackOff); + } + + /// A job converges to Failed rather than CrashLoopBackOff. + #[test] + fn job_bound_status_is_honoured() { + let mut deployment = vm_deployment(); + deployment.restart_count = crate::models::deployments::MAX_RESTART_COUNT - 1; + + apply_vm_start_failure( + &mut deployment, + &RuntimeError::VmStartFailed("boot timeout".into()), + "cloud-hypervisor", + DeploymentStatus::Failed, + ); + + assert_eq!(deployment.status, DeploymentStatus::Failed); + } + + #[test] + fn vm_start_insufficient_resources_is_terminal() { + assert_eq!( + classify_vm_start_error(&RuntimeError::InsufficientResources("need".into())), + ( + Some(DeploymentStatus::InsufficientResources), + "insufficient_resources" + ) + ); + } + #[test] fn other_exit_codes_retry() { // Generic failures and signal kills stay retryable (could be transient). diff --git a/src/runtime/cloud_hypervisor/lifecycle.rs b/src/runtime/cloud_hypervisor/lifecycle.rs index dd16646..808ce76 100644 --- a/src/runtime/cloud_hypervisor/lifecycle.rs +++ b/src/runtime/cloud_hypervisor/lifecycle.rs @@ -4,6 +4,7 @@ use super::client::{ }; use crate::config::config::get_config_dir; use crate::config::server::CloudHypervisorConfig; +use crate::hypervisor::classifier::apply_vm_start_failure; use crate::hypervisor::error::RuntimeError; use crate::hypervisor::host_net::{InstanceNet, cid_for_instance}; use crate::hypervisor::lifecycle_trait::{Log, RuntimeLifecycle, classify_log, extract_date}; @@ -958,23 +959,12 @@ impl CloudHypervisorLifecycle { deployment.id, e ); - let (status, reason) = classify_vm_start_error(&e); - - deployment.emit_event( - "error", - format!("{}", e), + apply_vm_start_failure( + &mut deployment, + &e, "cloud-hypervisor", - Some(reason), + DeploymentStatus::CrashLoopBackOff, ); - - if let Some(terminal_status) = status { - deployment.status = terminal_status; - } else { - deployment.restart_count += 1; - if deployment.restart_count >= MAX_RESTART_COUNT { - deployment.status = DeploymentStatus::CrashLoopBackOff; - } - } } } } else if current_count > target_count @@ -1128,23 +1118,12 @@ impl CloudHypervisorLifecycle { deployment.id, e ); - let (status, reason) = classify_vm_start_error(&e); - - deployment.emit_event( - "error", - format!("{}", e), + apply_vm_start_failure( + &mut deployment, + &e, "cloud-hypervisor", - Some(reason), + DeploymentStatus::Failed, ); - - if let Some(terminal_status) = status { - deployment.status = terminal_status; - } else { - deployment.restart_count += 1; - if deployment.restart_count >= MAX_RESTART_COUNT { - deployment.status = DeploymentStatus::Failed; - } - } } } } @@ -1153,27 +1132,6 @@ impl CloudHypervisorLifecycle { } } -/// Classify a VM start failure into either a terminal deployment status -/// (permanent: missing firmware/image) or `None` for transient errors that -/// should bump `restart_count` and let the scheduler retry. -fn classify_vm_start_error(e: &RuntimeError) -> (Option, &'static str) { - match e { - RuntimeError::FirmwareNotFound(_) => (Some(DeploymentStatus::Failed), "FirmwareNotFound"), - RuntimeError::ImageNotFound(_) => { - (Some(DeploymentStatus::ImagePullBackOff), "ImageNotFound") - } - RuntimeError::PortAlreadyInUse(_) => (None, "PortAllocationFailed"), - // Terminal, not transient: the host is short on memory now and a retry - // on the next tick won't conjure more. Crash-looping would only spam - // events without changing the outcome — surface it and stop. - RuntimeError::InsufficientResources(_) => ( - Some(DeploymentStatus::InsufficientResources), - "insufficient_resources", - ), - _ => (None, "VmStartFailed"), - } -} - fn parse_resources(deployment: &Deployment) -> (u32, u32) { let mut vcpus = 1u32; let mut memory_mb = 256u32; @@ -1456,6 +1414,10 @@ impl CloudHypervisorLifecycle { #[cfg(test)] mod tests { use super::*; + // The classification itself now lives in the shared classifier; this module + // still asserts the verdicts CH depends on, so the move can't silently + // change them. + use crate::hypervisor::classifier::classify_vm_start_error; fn cfg_with_binary(binary_path: &str) -> CloudHypervisorRuntimeConfig { CloudHypervisorRuntimeConfig { diff --git a/src/runtime/containerd/lifecycle.rs b/src/runtime/containerd/lifecycle.rs index 1794af7..9782cf6 100644 --- a/src/runtime/containerd/lifecycle.rs +++ b/src/runtime/containerd/lifecycle.rs @@ -840,8 +840,26 @@ async fn write_config_files( /// Translate a runtime error into the deployment's status + event, mirroring the /// Docker runtime's `handle_create_error`. fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment_restart: bool) { + // Decide retry-vs-give-up before mapping the message, like Docker: a terminal + // error (image absent, config missing) can't fix itself on a retry, so jump + // straight to the restart bound instead of burning MAX_RESTART_COUNT + // reconcile cycles to reach the same outcome. + // + // `InstanceCreationFailed` is the one exception, and it is a containerd + // quirk rather than a classifier one. Docker only raises that variant when + // the daemon rejects a container spec — permanent by construction. Here it + // wraps every gRPC call in the create path (`PrepareSnapshot`, + // `CreateContainer`, `CreateTask`, `StartTask`), so it also covers a shim + // that is momentarily unavailable or a snapshotter under contention. Those + // do recover, so this runtime keeps them inside the retry budget. + let terminal = crate::hypervisor::classifier::classify_create_error(&err).is_terminal() + && !matches!(err, RuntimeError::InstanceCreationFailed(_)); if increment_restart { - deployment.restart_count += 1; + if terminal { + deployment.restart_count = MAX_RESTART_COUNT; + } else { + deployment.restart_count += 1; + } } let (status, reason, message) = match &err { RuntimeError::ImageNotFound(detail) => ( @@ -864,6 +882,14 @@ fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment "insufficient_resources", detail.clone(), ), + // Terminal per the classifier: the referenced config (or key) is absent + // and only the operator can create it. Give it the dedicated status + // rather than the generic `Error` catch-all. + RuntimeError::ConfigNotFound(detail) | RuntimeError::ConfigKeyNotFound(detail) => ( + DeploymentStatus::ConfigError, + "config_error", + detail.clone(), + ), other => ( DeploymentStatus::Error, "runtime_error", @@ -874,3 +900,127 @@ fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment deployment.status = status; deployment.emit_event("error", message, "containerd", Some(reason)); } + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashMap; + + fn worker() -> Deployment { + Deployment { + id: "d1".to_string(), + created_at: chrono::Utc::now().to_string(), + updated_at: None, + status: DeploymentStatus::Creating, + restart_count: 0, + namespace: "test".to_string(), + name: "app".to_string(), + image: "nginx:alpine".to_string(), + config: None, + runtime: "containerd".to_string(), + kind: "worker".to_string(), + replicas: 1, + command: vec![], + instances: vec![], + labels: HashMap::new(), + environment: HashMap::new(), + volumes: "[]".to_string(), + health_checks: vec![], + resources: None, + image_digest: None, + ports: vec![], + pending_events: vec![], + parent_id: None, + network: None, + } + } + + /// A permanent failure must not burn the whole restart budget: one create + /// attempt is enough to jump to the bound, so the deployment converges on + /// the next tick instead of five ticks from now. + #[test] + fn terminal_create_error_jumps_to_the_restart_bound() { + let mut deployment = worker(); + handle_create_error( + &mut deployment, + RuntimeError::ImageNotFound("nope".into()), + true, + ); + + assert_eq!(deployment.restart_count, MAX_RESTART_COUNT); + assert_eq!(deployment.status, DeploymentStatus::ImagePullBackOff); + } + + /// A missing config is terminal too, and gets its own status rather than the + /// generic `Error` catch-all. + #[test] + fn missing_config_is_terminal_with_config_error_status() { + let mut deployment = worker(); + handle_create_error( + &mut deployment, + RuntimeError::ConfigNotFound("app-config".into()), + true, + ); + + assert_eq!(deployment.restart_count, MAX_RESTART_COUNT); + assert_eq!(deployment.status, DeploymentStatus::ConfigError); + } + + /// Unlike Docker, containerd wraps every gRPC call in the create path in + /// `InstanceCreationFailed`, so it also covers a shim that is momentarily + /// unavailable. That must stay retryable — failing fast here would abandon a + /// deployment that a retry recovers. + /// + /// Asserting `restart_count == 1` alone would not prove much, so drive the + /// full budget: the deployment must take `MAX_RESTART_COUNT` failed ticks to + /// reach the bound, one increment at a time, and never jump early. + #[test] + fn instance_creation_failed_stays_retryable_on_containerd() { + let mut deployment = worker(); + + for tick in 1..=MAX_RESTART_COUNT { + handle_create_error( + &mut deployment, + RuntimeError::InstanceCreationFailed("CreateTask: transport error".into()), + true, + ); + assert_eq!( + deployment.restart_count, tick, + "a transient shim error must consume exactly one restart per tick" + ); + assert_eq!(deployment.status, DeploymentStatus::CreateContainerError); + } + + // Only now is the budget exhausted; the guard in `handle_worker` turns + // this into CrashLoopBackOff on the next tick. + assert_eq!(deployment.restart_count, MAX_RESTART_COUNT); + } + + /// A transient failure keeps its one-by-one budget. + #[test] + fn transient_create_error_increments_by_one() { + let mut deployment = worker(); + handle_create_error( + &mut deployment, + RuntimeError::ImagePullFailed("registry timeout".into()), + true, + ); + + assert_eq!(deployment.restart_count, 1); + } + + /// With `increment_restart == false` the counter is left alone entirely, + /// terminal verdict or not. + #[test] + fn no_increment_leaves_the_counter_untouched() { + let mut deployment = worker(); + handle_create_error( + &mut deployment, + RuntimeError::ImageNotFound("nope".into()), + false, + ); + + assert_eq!(deployment.restart_count, 0); + assert_eq!(deployment.status, DeploymentStatus::ImagePullBackOff); + } +} diff --git a/src/runtime/firecracker/lifecycle.rs b/src/runtime/firecracker/lifecycle.rs index 7b6c624..b343274 100644 --- a/src/runtime/firecracker/lifecycle.rs +++ b/src/runtime/firecracker/lifecycle.rs @@ -14,6 +14,7 @@ //! presence on disk (its `.sock`) is the source of truth for "is it running". use crate::config::server::FirecrackerConfig; +use crate::hypervisor::classifier::{apply_vm_start_failure, classify_vm_start_error}; use crate::hypervisor::cloud_init::{GuestMount, GuestNet}; use crate::hypervisor::error::RuntimeError; use crate::hypervisor::host_net::{InstanceNet, cid_for_instance}; @@ -956,7 +957,24 @@ impl FirecrackerLifecycle { Ok(_) => {} Err(e) => { error!("Firecracker: failed to start instance: {}", e); - deployment.status = DeploymentStatus::CreateContainerError; + // This path used to set the status without ever touching + // `restart_count`, so the scheduler — which keeps polling + // `create_container_error` — retried the boot forever. + let terminal = classify_vm_start_error(&e).0.is_some(); + apply_vm_start_failure( + &mut deployment, + &e, + "firecracker", + DeploymentStatus::CrashLoopBackOff, + ); + // A transient failure that still has budget left keeps + // reporting a create error, so the retry stays visible + // instead of silently sitting in Creating. A terminal + // verdict already owns the status and must not be + // overwritten here. + if !terminal && deployment.restart_count < MAX_RESTART_COUNT { + deployment.status = DeploymentStatus::CreateContainerError; + } break; } } @@ -1088,16 +1106,12 @@ impl FirecrackerLifecycle { "Firecracker: failed to start job VM for deployment {}: {}", deployment.id, e ); - let (status, reason) = classify_vm_start_error(&e); - deployment.emit_event("error", format!("{}", e), "firecracker", Some(reason)); - if let Some(terminal) = status { - deployment.status = terminal; - } else { - deployment.restart_count += 1; - if deployment.restart_count >= MAX_RESTART_COUNT { - deployment.status = DeploymentStatus::Failed; - } - } + apply_vm_start_failure( + &mut deployment, + &e, + "firecracker", + DeploymentStatus::Failed, + ); } } } @@ -1106,24 +1120,6 @@ impl FirecrackerLifecycle { } } -/// Classify a VM start failure into either a terminal deployment status -/// (permanent: missing kernel/rootfs) or `None` for transient errors that -/// should bump `restart_count` and let the scheduler retry. Mirrors the Cloud -/// Hypervisor classifier so the two runtimes converge identically. -fn classify_vm_start_error(e: &RuntimeError) -> (Option, &'static str) { - match e { - RuntimeError::ImageNotFound(_) => { - (Some(DeploymentStatus::ImagePullBackOff), "ImageNotFound") - } - RuntimeError::PortAlreadyInUse(_) => (None, "PortAllocationFailed"), - RuntimeError::InsufficientResources(_) => ( - Some(DeploymentStatus::InsufficientResources), - "insufficient_resources", - ), - _ => (None, "VmStartFailed"), - } -} - /// Decide the post-scale worker status from the current status and whether any /// instance is genuinely alive. Returns `Some(Running)` only when liveness is /// confirmed; `None` leaves the status untouched (so a VM that booted then died @@ -1136,7 +1132,19 @@ fn liveness_confirmed_status( current: &DeploymentStatus, any_alive: bool, ) -> Option { - if *current == DeploymentStatus::CreateContainerError { + // A status the scale loop just set from a failed boot must survive this + // gate. With replicas > 1 one instance can be alive while another failed to + // start, and reporting Running would erase the failure the operator needs to + // see — including the terminal verdicts the classifier now produces. + if matches!( + current, + DeploymentStatus::CreateContainerError + | DeploymentStatus::CrashLoopBackOff + | DeploymentStatus::ImagePullBackOff + | DeploymentStatus::ConfigError + | DeploymentStatus::InsufficientResources + | DeploymentStatus::Failed + ) { return None; } any_alive.then_some(DeploymentStatus::Running) @@ -1574,6 +1582,27 @@ mod tests { ); } + /// Same reasoning for every terminal verdict the classifier can produce on a + /// failed boot: with replicas > 1 a sibling instance can be alive, and + /// promoting to Running there would erase the failure entirely. + #[test] + fn terminal_boot_failures_are_preserved() { + for status in [ + DeploymentStatus::CrashLoopBackOff, + DeploymentStatus::ImagePullBackOff, + DeploymentStatus::ConfigError, + DeploymentStatus::InsufficientResources, + DeploymentStatus::Failed, + ] { + assert_eq!( + liveness_confirmed_status(&status, true), + None, + "{:?} must survive the liveness gate", + status + ); + } + } + #[test] fn scan_instances_reads_disk_not_memory() { // Post-restart simulation: sockets exist on disk, `pids` is empty.