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 diff --git a/src/hypervisor/classifier.rs b/src/hypervisor/classifier.rs index b76d26e..83e5da2 100644 --- a/src/hypervisor/classifier.rs +++ b/src/hypervisor/classifier.rs @@ -166,6 +166,27 @@ 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. +/// +/// 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) +} + +/// 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, @@ -179,7 +200,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 +279,76 @@ 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" + ); + } + + /// 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_and_reconciled_statuses_are_complements() { + use crate::models::deployments::RECONCILED_STATUSES; + + for status in DeploymentStatus::all() { + 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] fn insufficient_resources_is_terminal() { assert_eq!( @@ -381,12 +484,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 +511,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 +536,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/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 10f3fa2..7e0ed66 100644 --- a/src/runtime/containerd/lifecycle.rs +++ b/src/runtime/containerd/lifecycle.rs @@ -854,7 +854,21 @@ 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 = + 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 { deployment.restart_count = MAX_RESTART_COUNT; } else { 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, 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')