From f9bb0a99b060c012041b9980f74d3e484183d56e Mon Sep 17 00:00:00 2001 From: Mlanawo MBECHEZI Date: Mon, 3 Aug 2026 16:03:51 +0300 Subject: [PATCH 1/4] fix(observability): stop reporting restarts that never happened --- src/hypervisor/classifier.rs | 145 ++++++++++++++++-- src/runtime/containerd/lifecycle.rs | 10 +- .../firecracker/t16_insufficient_memory.sh | 15 +- 3 files changed, 145 insertions(+), 25 deletions(-) diff --git a/src/hypervisor/classifier.rs b/src/hypervisor/classifier.rs index b76d26e..5b95b23 100644 --- a/src/hypervisor/classifier.rs +++ b/src/hypervisor/classifier.rs @@ -166,6 +166,24 @@ pub(crate) fn classify_vm_start_error( /// 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. +/// True when the scheduler already refuses to reconcile a deployment in this +/// status, so no restart-budget marker is needed to stop the retries. +/// +/// Must stay in sync with the status filter in `scheduler::schedule` — a status +/// dropped from that filter has to be added here, or its terminal deployments +/// would keep being picked up forever. The pairing is asserted in the tests +/// below rather than enforced by the compiler, since the filter is built from +/// strings for the DB query. +fn scheduler_skips_by_status(status: &DeploymentStatus) -> bool { + matches!( + status, + DeploymentStatus::InsufficientResources + | DeploymentStatus::CrashLoopBackOff + | DeploymentStatus::Failed + | DeploymentStatus::Completed + ) +} + pub(crate) fn apply_vm_start_failure( deployment: &mut crate::models::deployments::Deployment, err: &RuntimeError, @@ -179,7 +197,19 @@ pub(crate) fn apply_vm_start_failure( match status { Some(terminal) => { - deployment.restart_count = MAX_RESTART_COUNT; + // Exhausting the restart budget is how a terminal error stops the + // scheduler from retrying — but only for statuses the scheduler + // still reconciles (`ConfigError`, `ImagePullBackOff`, + // `CreateContainerError`, ...). A few terminal statuses are already + // excluded from its filter by status alone; for those the marker + // buys nothing and makes the field lie, so it is skipped. + // + // Concretely: a deployment refused by host-memory admission used to + // report 5 restarts having never started a single process, sending + // whoever read it looking for an instability that never existed. + if !scheduler_skips_by_status(&terminal) { + deployment.restart_count = MAX_RESTART_COUNT; + } deployment.status = terminal; } None => { @@ -246,6 +276,81 @@ mod tests { ); } + /// A deployment refused before anything ran must not claim restarts it + /// never made. `restart_count` is displayed by the CLI, the API and the + /// dashboard as a count of actual restarts; reporting 5 for a workload that + /// never started a process sends an operator hunting a phantom instability. + #[test] + fn admission_refusal_does_not_invent_restarts() { + let mut d = vm_deployment(); + assert_eq!(d.restart_count, 0); + + apply_vm_start_failure( + &mut d, + &RuntimeError::InsufficientResources("needs 4096 MiB but only 1800 MiB".into()), + "firecracker", + DeploymentStatus::CrashLoopBackOff, + ); + + assert_eq!(d.status, DeploymentStatus::InsufficientResources); + assert_eq!( + d.restart_count, 0, + "no VM was ever spawned, so no restart may be reported" + ); + } + + /// The counterpart: statuses the scheduler DOES keep reconciling still need + /// the exhausted budget, otherwise they would be retried forever. + #[test] + fn a_reconciled_terminal_status_still_exhausts_the_budget() { + let mut d = vm_deployment(); + + apply_vm_start_failure( + &mut d, + &RuntimeError::ConfigNotFound("missing-config".into()), + "firecracker", + DeploymentStatus::CrashLoopBackOff, + ); + + assert_eq!(d.status, DeploymentStatus::ConfigError); + assert_eq!( + d.restart_count, + crate::models::deployments::MAX_RESTART_COUNT, + "ConfigError is still reconciled, so the budget is what stops the retries" + ); + } + + /// Guard against drift: every status this module claims the scheduler skips + /// must be absent from the scheduler's reconcile filter. If someone adds a + /// status back to that filter without updating `scheduler_skips_by_status`, + /// its deployments would be retried forever with no budget to stop them. + #[test] + fn skipped_statuses_are_absent_from_the_scheduler_filter() { + // Mirrors the filter built in `scheduler::schedule`. + let reconciled = [ + DeploymentStatus::Pending, + DeploymentStatus::Creating, + DeploymentStatus::Running, + DeploymentStatus::Deleted, + DeploymentStatus::CreateContainerError, + DeploymentStatus::ImagePullBackOff, + DeploymentStatus::NetworkError, + DeploymentStatus::ConfigError, + DeploymentStatus::FileSystemError, + DeploymentStatus::Error, + ]; + + for status in DeploymentStatus::all() { + if scheduler_skips_by_status(&status) { + assert!( + !reconciled.contains(&status), + "{status:?} is claimed to be skipped but the scheduler reconciles it: \ + without a budget marker it would retry forever" + ); + } + } + } + #[test] fn insufficient_resources_is_terminal() { assert_eq!( @@ -381,12 +486,16 @@ mod tests { } } - /// 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. + /// The invariant that stops the infinite reboot loop: a failure must either + /// move `restart_count` or land on a status the scheduler refuses to + /// reconcile. Leaving BOTH untouched would retry a permanent failure on + /// every tick, forever. + /// + /// The counter is not required on its own: several terminal statuses are + /// already excluded from the reconcile filter, and marking those would only + /// report restarts that never happened. #[test] - fn every_start_failure_moves_the_restart_counter() { + fn every_start_failure_either_counts_or_lands_outside_the_reconcile_filter() { let errors = [ RuntimeError::FirmwareNotFound("f".into()), RuntimeError::ImageNotFound("i".into()), @@ -404,17 +513,23 @@ mod tests { DeploymentStatus::CrashLoopBackOff, ); assert!( - deployment.restart_count > 0, - "counter stayed at zero for {:?} — the deployment would retry forever", - err + deployment.restart_count > 0 || scheduler_skips_by_status(&deployment.status), + "{:?} left the counter at zero AND landed on {:?}, which the scheduler \ + still reconciles — the deployment would retry forever", + err, + deployment.status ); } } - /// A permanent failure lands on its status and exhausts the budget at once, - /// so the very next tick is terminal instead of the fifth. + /// A permanent failure lands on its terminal status immediately, so the very + /// next tick is terminal instead of the fifth. + /// + /// `Failed` is outside the scheduler's reconcile filter, so the status alone + /// stops the retries and the restart budget is left untouched — the + /// deployment never restarted, and must not claim it did. #[test] - fn terminal_start_failure_jumps_to_the_bound() { + fn terminal_start_failure_lands_on_its_status_at_once() { let mut deployment = vm_deployment(); apply_vm_start_failure( &mut deployment, @@ -423,11 +538,11 @@ mod tests { DeploymentStatus::CrashLoopBackOff, ); + assert_eq!(deployment.status, DeploymentStatus::Failed); assert_eq!( - deployment.restart_count, - crate::models::deployments::MAX_RESTART_COUNT + deployment.restart_count, 0, + "the VM never started, so no restart may be reported" ); - assert_eq!(deployment.status, DeploymentStatus::Failed); } /// A transient failure keeps its one-per-tick budget and converges to the diff --git a/src/runtime/containerd/lifecycle.rs b/src/runtime/containerd/lifecycle.rs index 10f3fa2..4f2d54a 100644 --- a/src/runtime/containerd/lifecycle.rs +++ b/src/runtime/containerd/lifecycle.rs @@ -854,7 +854,15 @@ fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment // 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 { + // `InsufficientResources` is excluded from the scheduler's reconcile filter + // by status alone, so it needs no counter marker — and writing one would + // report restarts that never happened (no container was ever created). The + // other terminal statuses (`ImagePullBackOff`, `CreateContainerError`) ARE + // still reconciled, so for those the exhausted budget remains what stops + // the retries. + let status_alone_stops_reconciliation = matches!(err, RuntimeError::InsufficientResources(_)); + + if increment_restart && !status_alone_stops_reconciliation { if terminal { deployment.restart_count = MAX_RESTART_COUNT; } else { diff --git a/tests/e2e/firecracker/t16_insufficient_memory.sh b/tests/e2e/firecracker/t16_insufficient_memory.sh index 149b61e..21f00ef 100755 --- a/tests/e2e/firecracker/t16_insufficient_memory.sh +++ b/tests/e2e/firecracker/t16_insufficient_memory.sh @@ -80,16 +80,13 @@ log "no VM was spawned and no rootfs was copied" # Terminal, not transient: the RAM is not coming back, so retrying would only # spam events. # -# `restart_count` reads 5 here, which does NOT mean five boot attempts were -# made: `apply_vm_start_failure` assigns MAX_RESTART_COUNT outright for any -# terminal VM-start error (src/hypervisor/classifier.rs), so the counter is -# used as a "do not reconcile again" marker rather than a tally. Cloud -# Hypervisor goes through the same function, so both runtimes behave alike — -# t23's "restart_count must stay 0" comment is simply stale. +# `restart_count` stays 0: the admission check refused the boot before any +# process existed, so there is nothing to report. It used to read 5 — the +# terminal path assigned MAX_RESTART_COUNT as a "stop reconciling" marker, +# which was redundant (the scheduler already skips this status) and made the +# field claim restarts that never happened. # -# What matters for this test is that the count is FROZEN, i.e. nothing is -# retrying underneath. Asserting the exact value would pin an implementation -# detail of that marker. +# What matters here is that the count is FROZEN: nothing is retrying underneath. COUNT_ONE=$("$RING_BIN" deployment inspect "$DEPLOYMENT_ID" --output json 2>/dev/null | jq -r '.restart_count // 0') sleep 12 COUNT_TWO=$("$RING_BIN" deployment inspect "$DEPLOYMENT_ID" --output json 2>/dev/null | jq -r '.restart_count // 0') From da746a56a6a96c6c089ff523e26e5008ac1dff26 Mon Sep 17 00:00:00 2001 From: Mlanawo MBECHEZI Date: Mon, 3 Aug 2026 16:05:08 +0300 Subject: [PATCH 2/4] chore: ignore packer serial console logs --- .gitignore | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 5c2a802..bda59c3 100644 --- a/.gitignore +++ b/.gitignore @@ -2,4 +2,7 @@ target ring.db ring.db-shm ring.db-wal -.config \ No newline at end of file +.config +# Packer build artifacts: serial console dumps from the CH image build. +# Regenerated on every build, hundreds of KB, no value in history. +tests/e2e/cloud-hypervisor/packer/*.log From e21c665a40966643000cabf0d8082d402ab6c5c4 Mon Sep 17 00:00:00 2001 From: Mlanawo MBECHEZI Date: Mon, 3 Aug 2026 16:12:17 +0300 Subject: [PATCH 3/4] fix(observability): apply the same rule to docker and derive both sides from one list --- src/hypervisor/classifier.rs | 65 ++++++++++++----------------- src/models/deployments.rs | 20 +++++++++ src/runtime/containerd/lifecycle.rs | 8 +++- src/runtime/docker/lifecycle.rs | 17 +++++++- src/scheduler/scheduler.rs | 16 ++----- 5 files changed, 73 insertions(+), 53 deletions(-) diff --git a/src/hypervisor/classifier.rs b/src/hypervisor/classifier.rs index 5b95b23..4c96d39 100644 --- a/src/hypervisor/classifier.rs +++ b/src/hypervisor/classifier.rs @@ -169,19 +169,13 @@ pub(crate) fn classify_vm_start_error( /// True when the scheduler already refuses to reconcile a deployment in this /// status, so no restart-budget marker is needed to stop the retries. /// -/// Must stay in sync with the status filter in `scheduler::schedule` — a status -/// dropped from that filter has to be added here, or its terminal deployments -/// would keep being picked up forever. The pairing is asserted in the tests -/// below rather than enforced by the compiler, since the filter is built from -/// strings for the DB query. -fn scheduler_skips_by_status(status: &DeploymentStatus) -> bool { - matches!( - status, - DeploymentStatus::InsufficientResources - | DeploymentStatus::CrashLoopBackOff - | DeploymentStatus::Failed - | DeploymentStatus::Completed - ) +/// Derived from [`RECONCILED_STATUSES`] rather than listed by hand: the two +/// must stay exact complements. A status ADDED to the reconcile filter while +/// still reported as skipped here would be retried forever, with no budget to +/// stop it — which is why this reads the same array the scheduler queries with +/// instead of duplicating it. +pub(crate) fn scheduler_skips_by_status(status: &DeploymentStatus) -> bool { + !crate::models::deployments::RECONCILED_STATUSES.contains(status) } pub(crate) fn apply_vm_start_failure( @@ -320,35 +314,30 @@ mod tests { ); } - /// Guard against drift: every status this module claims the scheduler skips - /// must be absent from the scheduler's reconcile filter. If someone adds a - /// status back to that filter without updating `scheduler_skips_by_status`, - /// its deployments would be retried forever with no budget to stop them. + /// The two sides must be exact complements. This no longer duplicates the + /// scheduler's list — both derive from `RECONCILED_STATUSES` — so the test + /// checks the property rather than a copy that could go stale. #[test] - fn skipped_statuses_are_absent_from_the_scheduler_filter() { - // Mirrors the filter built in `scheduler::schedule`. - let reconciled = [ - DeploymentStatus::Pending, - DeploymentStatus::Creating, - DeploymentStatus::Running, - DeploymentStatus::Deleted, - DeploymentStatus::CreateContainerError, - DeploymentStatus::ImagePullBackOff, - DeploymentStatus::NetworkError, - DeploymentStatus::ConfigError, - DeploymentStatus::FileSystemError, - DeploymentStatus::Error, - ]; + fn skipped_and_reconciled_statuses_are_complements() { + use crate::models::deployments::RECONCILED_STATUSES; for status in DeploymentStatus::all() { - if scheduler_skips_by_status(&status) { - assert!( - !reconciled.contains(&status), - "{status:?} is claimed to be skipped but the scheduler reconciles it: \ - without a budget marker it would retry forever" - ); - } + assert_eq!( + scheduler_skips_by_status(&status), + !RECONCILED_STATUSES.contains(&status), + "{status:?} is inconsistent between the reconcile filter and the skip check" + ); } + + // Sanity: neither side is empty, which would make the assertion above + // vacuously true. + assert!(!RECONCILED_STATUSES.is_empty()); + assert!( + DeploymentStatus::all() + .iter() + .any(scheduler_skips_by_status), + "no status is skipped — the marker would always be written" + ); } #[test] diff --git a/src/models/deployments.rs b/src/models/deployments.rs index 4195ba0..bfe7739 100644 --- a/src/models/deployments.rs +++ b/src/models/deployments.rs @@ -7,6 +7,26 @@ use std::str::FromStr; pub(crate) const MAX_RESTART_COUNT: u32 = 5; +/// Statuses the scheduler keeps reconciling on every tick. +/// +/// Single source of truth: `scheduler::schedule` builds its DB filter from this +/// list, and `classifier::scheduler_skips_by_status` is its complement. Keeping +/// them derived from one array is what stops the two drifting apart — a status +/// added here while still reported as "skipped" would be retried forever with +/// no restart budget to stop it. +pub(crate) const RECONCILED_STATUSES: &[DeploymentStatus] = &[ + DeploymentStatus::Pending, + DeploymentStatus::Creating, + DeploymentStatus::Running, + DeploymentStatus::Deleted, + DeploymentStatus::CreateContainerError, + DeploymentStatus::ImagePullBackOff, + DeploymentStatus::NetworkError, + DeploymentStatus::ConfigError, + DeploymentStatus::FileSystemError, + DeploymentStatus::Error, +]; + /// All variants serialize to snake_case, both on the wire (serde JSON) and /// in the SQLite `deployment.status` column (Display). Before this change, /// lifecycle states were lowercase (`running`, …) while error states were diff --git a/src/runtime/containerd/lifecycle.rs b/src/runtime/containerd/lifecycle.rs index 4f2d54a..7e0ed66 100644 --- a/src/runtime/containerd/lifecycle.rs +++ b/src/runtime/containerd/lifecycle.rs @@ -860,7 +860,13 @@ fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment // other terminal statuses (`ImagePullBackOff`, `CreateContainerError`) ARE // still reconciled, so for those the exhausted budget remains what stops // the retries. - let status_alone_stops_reconciliation = matches!(err, RuntimeError::InsufficientResources(_)); + let status_alone_stops_reconciliation = + match crate::hypervisor::classifier::classify_create_error(&err) { + crate::hypervisor::classifier::Disposition::Terminal(ref s) => { + crate::hypervisor::classifier::scheduler_skips_by_status(s) + } + crate::hypervisor::classifier::Disposition::Retry => false, + }; if increment_restart && !status_alone_stops_reconciliation { if terminal { diff --git a/src/runtime/docker/lifecycle.rs b/src/runtime/docker/lifecycle.rs index 3af0242..a76af23 100644 --- a/src/runtime/docker/lifecycle.rs +++ b/src/runtime/docker/lifecycle.rs @@ -179,8 +179,21 @@ fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment // the restart bound: the deployment converges to its terminal state on the // next tick instead of five ticks from now. Transient errors still bump by // one and retry within the budget, exactly as before. - let terminal = crate::hypervisor::classifier::classify_create_error(&err).is_terminal(); - if increment_restart { + // + // One exception: a few terminal statuses are already excluded from the + // scheduler's reconcile filter, so the budget marker buys nothing there and + // would report restarts that never happened — a host-memory refusal is + // checked before the image pull, so nothing was ever created. + let disposition = crate::hypervisor::classifier::classify_create_error(&err); + let terminal = disposition.is_terminal(); + let marker_needed = match &disposition { + crate::hypervisor::classifier::Disposition::Terminal(status) => { + !crate::hypervisor::classifier::scheduler_skips_by_status(status) + } + crate::hypervisor::classifier::Disposition::Retry => true, + }; + + if increment_restart && marker_needed { if terminal { deployment.restart_count = MAX_RESTART_COUNT; } else { diff --git a/src/scheduler/scheduler.rs b/src/scheduler/scheduler.rs index d50567f..d1a99d2 100644 --- a/src/scheduler/scheduler.rs +++ b/src/scheduler/scheduler.rs @@ -1439,18 +1439,10 @@ pub(crate) async fn schedule( let mut filters = HashMap::new(); filters.insert( String::from("status"), - vec![ - DeploymentStatus::Pending.to_string(), - DeploymentStatus::Creating.to_string(), - DeploymentStatus::Running.to_string(), - DeploymentStatus::Deleted.to_string(), - DeploymentStatus::CreateContainerError.to_string(), - DeploymentStatus::ImagePullBackOff.to_string(), - DeploymentStatus::NetworkError.to_string(), - DeploymentStatus::ConfigError.to_string(), - DeploymentStatus::FileSystemError.to_string(), - DeploymentStatus::Error.to_string(), - ], + crate::models::deployments::RECONCILED_STATUSES + .iter() + .map(|s| s.to_string()) + .collect(), ); let mut list_deployments = match deployments::find_all(&pool, filters).await { Ok(list) => list, From befe5e7105c33d227e382e248e4e1dbdc3e3cafc Mon Sep 17 00:00:00 2001 From: Mlanawo MBECHEZI Date: Mon, 3 Aug 2026 16:15:46 +0300 Subject: [PATCH 4/4] docs: restate the restart budget rule on apply_vm_start_failure --- src/hypervisor/classifier.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/hypervisor/classifier.rs b/src/hypervisor/classifier.rs index 4c96d39..83e5da2 100644 --- a/src/hypervisor/classifier.rs +++ b/src/hypervisor/classifier.rs @@ -178,6 +178,15 @@ pub(crate) fn scheduler_skips_by_status(status: &DeploymentStatus) -> bool { !crate::models::deployments::RECONCILED_STATUSES.contains(status) } +/// Apply a VM start failure to a deployment: record the event, set the status, +/// and decide what happens to the restart budget. +/// +/// A transient failure bumps `restart_count` by one and retries within the +/// budget. A terminal failure lands on its status immediately — and exhausts +/// the budget only when the scheduler would otherwise keep reconciling that +/// status. For the statuses it already skips, the counter is left alone: the +/// deployment never restarted, and reporting that it did sends whoever reads +/// the field looking for an instability that never happened. pub(crate) fn apply_vm_start_failure( deployment: &mut crate::models::deployments::Deployment, err: &RuntimeError,