diff --git a/crates/l10_foundation/common/grpc/transport/src/status.rs b/crates/l10_foundation/common/grpc/transport/src/status.rs index 8e1ab4c6d5..9a4bd3b11c 100644 --- a/crates/l10_foundation/common/grpc/transport/src/status.rs +++ b/crates/l10_foundation/common/grpc/transport/src/status.rs @@ -8,8 +8,8 @@ pub const RETRY_AFTER_MS_METADATA: &str = "x-space-retry-after-ms"; /// precondition: the node cannot make the desired state converge by retrying /// while cluster state is unchanged — the guest is unreachable because the /// sandbox is in a non-thawing state, or the auto-lifecycle target state is -/// stuck (`Failed`/`CleanupRequired`/`CleanupFailed`/`Quarantined`). The reconcile-queue ref -/// should complete until a lifecycle or cluster repair changes the state. +/// stuck (`Failed` or `Quarantined`, or owned by a cleanup). The reconcile-queue +/// ref should complete until a lifecycle or cluster repair changes the state. /// Absence means the failure is transient — a still-initializing guest (`guest_not_ready`), /// a mid-transition lifecycle state (`Starting`/`Resuming`/`Suspending`) — and /// the queue should re-drive it. The tonic `Code` alone can't carry this diff --git a/crates/l20_domain/sbx/mgr/protocol/src/lifecycle.rs b/crates/l20_domain/sbx/mgr/protocol/src/lifecycle.rs index b85026cc89..eafac5d5ca 100644 --- a/crates/l20_domain/sbx/mgr/protocol/src/lifecycle.rs +++ b/crates/l20_domain/sbx/mgr/protocol/src/lifecycle.rs @@ -15,6 +15,8 @@ pub enum LifecycleObservationError { UnknownLabel(String), #[error("unknown sandbox cleanup state value {0}")] UnknownCleanupState(i32), + #[error("sandbox cleanup over lifecycle {0:?}, which no cleanup can be over")] + CleanupOutsideTeardown(SandboxLifecycleState), } impl SandboxLifecycleState { @@ -35,8 +37,6 @@ impl SandboxLifecycleState { Self::Suspending => "suspending", Self::Suspended => "suspended", Self::Restoring => "restoring", - Self::CleanupRequired => "cleanup_required", - Self::CleanupFailed => "cleanup_failed", Self::Quarantined => "quarantined", Self::Failed => "failed", } @@ -58,8 +58,6 @@ impl SandboxLifecycleState { "suspending" => Ok(Self::Suspending), "suspended" => Ok(Self::Suspended), "restoring" => Ok(Self::Restoring), - "cleanup_required" => Ok(Self::CleanupRequired), - "cleanup_failed" => Ok(Self::CleanupFailed), "quarantined" => Ok(Self::Quarantined), "failed" => Ok(Self::Failed), other => Err(LifecycleObservationError::UnknownLabel(other.to_string())), @@ -79,41 +77,144 @@ pub fn decode_lifecycle( } } -/// The status a reader decides from. A node publishes a cleanup that owns the -/// row in `cleanup.state`, apart from its lifecycle, and may report the row -/// under the teardown transition it is in rather than as `CleanupRequired` or -/// `CleanupFailed`; either form decodes to the same status here, so a reader -/// answers both alike. A cleanup state this build doesn't know fails closed. +/// A cleanup that owns a sandbox row. A node publishes it in `cleanup.state`, +/// apart from the lifecycle the cleanup is over. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum ObservedCleanup { + /// A forced teardown owns the row and is owed. + Required, + /// A teardown owns the row and its last attempt failed; it is retried. + Failed, +} + +/// A teardown transition a cleanup can own a row over. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum Teardown { + Stopping, + Deleting, + Suspending, +} + +impl Teardown { + pub const fn lifecycle(self) -> SandboxLifecycleState { + match self { + Self::Stopping => SandboxLifecycleState::Stopping, + Self::Deleting => SandboxLifecycleState::Deleting, + Self::Suspending => SandboxLifecycleState::Suspending, + } + } + + /// The teardown transition `lifecycle` is, if it is one. + pub const fn of(lifecycle: SandboxLifecycleState) -> Option { + match lifecycle { + SandboxLifecycleState::Stopping => Some(Self::Stopping), + SandboxLifecycleState::Deleting => Some(Self::Deleting), + SandboxLifecycleState::Suspending => Some(Self::Suspending), + _ => None, + } + } +} + +/// What a reader decides from: the lifecycle a node publishes for a row no +/// cleanup owns, or the cleanup that owns it and the teardown transition it is +/// over. A status is not a lifecycle and doesn't compare equal to one: ask +/// [`Self::is_unowned`] or [`Self::unowned`] whether a row is in a lifecycle. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum ObservedStatus { + /// No cleanup owns the row. + Unowned(SandboxLifecycleState), + /// `cleanup` owns the row over `teardown`. + Owned { + teardown: Teardown, + cleanup: ObservedCleanup, + }, +} + +impl ObservedStatus { + /// The published lifecycle: a cleanup's teardown transition while one + /// owns the row. To decide whether a row is in a lifecycle, ask + /// [`Self::is_unowned`]. + pub const fn lifecycle(self) -> SandboxLifecycleState { + match self { + Self::Unowned(lifecycle) => lifecycle, + Self::Owned { teardown, .. } => teardown.lifecycle(), + } + } + + /// The cleanup that owns the row, if one does. + pub const fn cleanup(self) -> Option { + match self { + Self::Unowned(_) => None, + Self::Owned { cleanup, .. } => Some(cleanup), + } + } + + /// The lifecycle, unless a cleanup owns the row. + pub const fn unowned(self) -> Option { + match self { + Self::Unowned(lifecycle) => Some(lifecycle), + Self::Owned { .. } => None, + } + } + + /// Whether the row is in `lifecycle` and no cleanup owns it. + pub fn is_unowned(self, lifecycle: SandboxLifecycleState) -> bool { + self.unowned() == Some(lifecycle) + } + + /// The status label node-ctl publishes to the control plane: + /// `cleanup_required` or `cleanup_failed` for a cleanup-owned row, + /// otherwise the lifecycle's label. + pub fn label(self) -> &'static str { + match self { + Self::Unowned(lifecycle) => lifecycle.label(), + Self::Owned { + cleanup: ObservedCleanup::Required, + .. + } => "cleanup_required", + Self::Owned { + cleanup: ObservedCleanup::Failed, + .. + } => "cleanup_failed", + } + } +} + +/// A cleanup state this build doesn't know, or a cleanup over a lifecycle no +/// cleanup can be over, fails closed. fn decode_status( lifecycle: Option, cleanup: Option<&SandboxCleanup>, -) -> Result { +) -> Result { let lifecycle = decode_lifecycle(lifecycle)?; - let Some(raw) = cleanup.map(|cleanup| cleanup.state) else { - return Ok(lifecycle); + let raw = cleanup.map_or(SandboxCleanupState::Unspecified as i32, |cleanup| { + cleanup.state + }); + let cleanup = match SandboxCleanupState::try_from(raw) { + Ok(SandboxCleanupState::Unspecified) => return Ok(ObservedStatus::Unowned(lifecycle)), + Ok(SandboxCleanupState::Required) => ObservedCleanup::Required, + Ok(SandboxCleanupState::Failed) => ObservedCleanup::Failed, + Err(_) => return Err(LifecycleObservationError::UnknownCleanupState(raw)), }; - match SandboxCleanupState::try_from(raw) { - Ok(SandboxCleanupState::Unspecified) => Ok(lifecycle), - Ok(SandboxCleanupState::Required) => Ok(SandboxLifecycleState::CleanupRequired), - Ok(SandboxCleanupState::Failed) => Ok(SandboxLifecycleState::CleanupFailed), - Err(_) => Err(LifecycleObservationError::UnknownCleanupState(raw)), - } + let teardown = Teardown::of(lifecycle) + .ok_or(LifecycleObservationError::CleanupOutsideTeardown(lifecycle))?; + Ok(ObservedStatus::Owned { teardown, cleanup }) } impl SandboxInfo { - pub fn observed_lifecycle(&self) -> Result { + pub fn observed_status(&self) -> Result { decode_status(self.lifecycle, self.cleanup.as_ref()) } } impl SandboxListElement { - pub fn observed_lifecycle(&self) -> Result { + pub fn observed_status(&self) -> Result { decode_status(self.lifecycle, self.cleanup.as_ref()) } } impl WaitResponse { - pub fn observed_lifecycle(&self) -> Result { + pub fn observed_status(&self) -> Result { decode_status(self.lifecycle, self.cleanup.as_ref()) } } diff --git a/crates/l20_domain/sbx/mgr/protocol/src/lifecycle/tests.rs b/crates/l20_domain/sbx/mgr/protocol/src/lifecycle/tests.rs index e5d5f58834..d6c67318aa 100644 --- a/crates/l20_domain/sbx/mgr/protocol/src/lifecycle/tests.rs +++ b/crates/l20_domain/sbx/mgr/protocol/src/lifecycle/tests.rs @@ -2,8 +2,18 @@ use super::*; #[test] fn lifecycle_observation_requires_a_known_present_enum() { + let mut known = 0; for raw in 1..=18 { - let state = SandboxLifecycleState::try_from(raw).unwrap(); + let Ok(state) = SandboxLifecycleState::try_from(raw) else { + // The retired cleanup lifecycles are unknown values now. + assert!(matches!(raw, 15 | 16), "{raw}"); + assert_eq!( + decode_lifecycle(Some(raw)), + Err(LifecycleObservationError::UnknownValue(raw)) + ); + continue; + }; + known += 1; assert_eq!(decode_lifecycle(Some(raw)).unwrap(), state); assert_eq!( SandboxLifecycleState::from_label(state.label()).unwrap(), @@ -22,10 +32,14 @@ fn lifecycle_observation_requires_a_known_present_enum() { lifecycle: Some(raw), cleanup: None, }; - assert_eq!(info.observed_lifecycle().unwrap(), state); - assert_eq!(list.observed_lifecycle().unwrap(), state); - assert_eq!(wait.observed_lifecycle().unwrap(), state); + let unowned = ObservedStatus::Unowned(state); + assert_eq!(info.observed_status().unwrap(), unowned); + assert_eq!(list.observed_status().unwrap(), unowned); + assert_eq!(wait.observed_status().unwrap(), unowned); + assert!(unowned.is_unowned(state)); + assert_eq!(unowned.label(), state.label()); } + assert_eq!(known, 16); for raw in [0, -1, 19, i32::MAX] { assert_eq!( decode_lifecycle(Some(raw)), @@ -36,30 +50,34 @@ fn lifecycle_observation_requires_a_known_present_enum() { decode_lifecycle(None), Err(LifecycleObservationError::Missing) ); + for label in ["cleanup_required", "cleanup_failed"] { + assert_eq!( + SandboxLifecycleState::from_label(label), + Err(LifecycleObservationError::UnknownLabel(label.to_owned())) + ); + } } #[test] -fn an_owning_cleanup_decodes_to_its_combined_status_whatever_lifecycle_is_reported() { +fn an_owning_cleanup_is_observed_beside_its_lifecycle_and_passes_no_lifecycle_check() { let cleanup = |state: SandboxCleanupState| SandboxCleanup { state: state as i32, ..SandboxCleanup::default() }; - for (owner, status) in [ + for (owner, observed, label) in [ ( SandboxCleanupState::Required, - SandboxLifecycleState::CleanupRequired, + ObservedCleanup::Required, + "cleanup_required", ), ( SandboxCleanupState::Failed, - SandboxLifecycleState::CleanupFailed, + ObservedCleanup::Failed, + "cleanup_failed", ), ] { - for lifecycle in [ - status, - SandboxLifecycleState::Stopping, - SandboxLifecycleState::Deleting, - SandboxLifecycleState::Suspending, - ] { + for teardown in [Teardown::Stopping, Teardown::Deleting, Teardown::Suspending] { + let lifecycle = teardown.lifecycle(); let info = SandboxInfo { lifecycle: Some(lifecycle as i32), cleanup: Some(cleanup(owner)), @@ -75,9 +93,17 @@ fn an_owning_cleanup_decodes_to_its_combined_status_whatever_lifecycle_is_report lifecycle: Some(lifecycle as i32), cleanup: Some(cleanup(owner)), }; - assert_eq!(info.observed_lifecycle().unwrap(), status, "{lifecycle:?}"); - assert_eq!(list.observed_lifecycle().unwrap(), status, "{lifecycle:?}"); - assert_eq!(wait.observed_lifecycle().unwrap(), status, "{lifecycle:?}"); + let status = ObservedStatus::Owned { + teardown, + cleanup: observed, + }; + assert_eq!(info.observed_status().unwrap(), status, "{lifecycle:?}"); + assert_eq!(list.observed_status().unwrap(), status, "{lifecycle:?}"); + assert_eq!(wait.observed_status().unwrap(), status, "{lifecycle:?}"); + assert!(!status.is_unowned(lifecycle)); + assert_eq!(status.lifecycle(), lifecycle); + assert_eq!(status.unowned(), None); + assert_eq!(status.label(), label); } } // An armed activation's rollback target leaves the lifecycle as reported. @@ -87,8 +113,8 @@ fn an_owning_cleanup_decodes_to_its_combined_status_whatever_lifecycle_is_report ..Default::default() }; assert_eq!( - armed.observed_lifecycle().unwrap(), - SandboxLifecycleState::Starting + armed.observed_status().unwrap(), + ObservedStatus::Unowned(SandboxLifecycleState::Starting) ); let unknown = SandboxInfo { lifecycle: Some(SandboxLifecycleState::Stopping as i32), @@ -99,7 +125,19 @@ fn an_owning_cleanup_decodes_to_its_combined_status_whatever_lifecycle_is_report ..Default::default() }; assert_eq!( - unknown.observed_lifecycle(), + unknown.observed_status(), Err(LifecycleObservationError::UnknownCleanupState(i32::MAX)) ); + // A cleanup over a lifecycle no cleanup can be over names no status. + let misplaced = SandboxInfo { + lifecycle: Some(SandboxLifecycleState::Started as i32), + cleanup: Some(cleanup(SandboxCleanupState::Failed)), + ..Default::default() + }; + assert_eq!( + misplaced.observed_status(), + Err(LifecycleObservationError::CleanupOutsideTeardown( + SandboxLifecycleState::Started + )) + ); } diff --git a/crates/l30_storage/sbx/mgr/store/AGENTS.md b/crates/l30_storage/sbx/mgr/store/AGENTS.md index dc318c251d..3f72c69e79 100644 --- a/crates/l30_storage/sbx/mgr/store/AGENTS.md +++ b/crates/l30_storage/sbx/mgr/store/AGENTS.md @@ -45,8 +45,7 @@ there, but not a build or dev dependency, nor `libsqlite3-sys`. result unknown, as under `NOT`. A write of a lifecycle state clears `cleanup_state`, and a cleanup write sets both in one statement; the triggers in `db/sandbox-manager/20261105142930_sandbox_cleanup_state.up.sql` - reject any other shape. The generated `legacy_state` stays only until the - next release drops it, so a rollback still finds it; don't read it. + reject any other shape. - Only some child tables declare `ON DELETE CASCADE`: register a new sandbox child table in `delete_sandbox_child_rows` (`src/db.rs`). - Fixtures that sbx-mgr's other test suites share live in `db::fixture` and diff --git a/crates/l30_storage/sbx/mgr/store/src/db/read/counts.rs b/crates/l30_storage/sbx/mgr/store/src/db/read/counts.rs index e782d7ed1c..514fcd1b89 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/read/counts.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/read/counts.rs @@ -4,12 +4,11 @@ use crate::auto_lifecycle_delete_receipts::{ AutoLifecycleDeleteBlockedStats, AutoLifecycleDeleteOutboxStats, }; -use crate::db::{Connection, Result, SandboxLifecycleCount, SandboxStatusCount}; +use crate::db::{Connection, Result, SandboxStatusCount}; /// Only the store defines these; callers pick one, they do not add their own. mod sealed { pub trait Sealed {} - impl Sealed for super::ByLifecycle {} impl Sealed for super::ByStatus {} impl Sealed for super::Resident {} impl Sealed for super::MalformedSuspendedUpdatedAt {} @@ -29,21 +28,9 @@ pub fn counts(conn: &Connection, count: C) -> Result { count.count(conn) } -/// Rows per combined status, the legacy lifecycle that folds a cleanup into -/// it. A state label this binary does not know fails the count rather than -/// being dropped from it. -#[derive(Clone, Copy, Debug)] -pub struct ByLifecycle; - -impl Count for ByLifecycle { - type Output = Vec; - - fn count(self, conn: &Connection) -> Result { - crate::db::sandbox_lifecycle_counts(conn) - } -} - -/// Rows per stored lifecycle and cleanup state, each pair counted apart. +/// Rows per stored lifecycle, cleanup state and runtime, each lifecycle and +/// cleanup pair counted apart. A label this binary does not know fails the +/// count rather than being dropped from it. #[derive(Clone, Copy, Debug)] pub struct ByStatus; diff --git a/crates/l30_storage/sbx/mgr/store/src/db/sandboxes.rs b/crates/l30_storage/sbx/mgr/store/src/db/sandboxes.rs index c1018e3442..0955366839 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/sandboxes.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/sandboxes.rs @@ -41,21 +41,15 @@ db_private_outside_tests! { } } -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct SandboxLifecycleCount { - pub state: SandboxStatus, - pub runtime: Option, - pub runtime_version: Option, - pub count: u64, -} - -/// Rows with one stored lifecycle and cleanup state. `cleanup_state` is the -/// label of the cleanup that owns the rows (`required` or `failed`), absent -/// when none does. +/// Rows with one stored lifecycle, cleanup state and runtime. `cleanup_state` +/// is the label of the cleanup that owns the rows (`required` or `failed`), +/// absent when none does. #[derive(Clone, Debug, Eq, PartialEq)] pub struct SandboxStatusCount { pub state: SandboxState, pub cleanup_state: Option<&'static str>, + pub runtime: Option, + pub runtime_version: Option, pub count: u64, } @@ -159,9 +153,11 @@ pub enum InsertRecoveredSuspendedSandboxError { } db_private_outside_tests! { - /// Aggregate durable rows in SQLite, retaining only lifecycle/runtime buckets - /// rather than collecting each sandbox's complete execution metadata. - fn sandbox_lifecycle_counts(conn: &Connection) -> Result> { + /// Count every durable sandbox-manager row by its stored lifecycle, + /// cleanup state and runtime, retaining only those buckets rather than + /// collecting each sandbox's complete execution metadata. A label this + /// binary does not know fails the count rather than being dropped from it. + fn sandbox_status_counts(conn: &Connection) -> Result> { // Use the row reader's full serde decoder, not a second description of // its wire format in SQL. Only the validated runtime enters GROUP BY; // complete metadata is decoded one row at a time and then discarded. @@ -182,32 +178,35 @@ db_private_outside_tests! { }, )?; let mut statement = conn.prepare(sql!( - "SELECT state, runtime, + "SELECT state, cleanup_state, runtime, sandbox_execution_runtime(execution_metadata_json) AS execution_runtime, - COUNT(*), cleanup_state + COUNT(*) FROM sandboxes GROUP BY state, cleanup_state, runtime, execution_runtime" ))?; let rows = statement.query_map([], |row| { - let state_label: String = row.get(0)?; - let cleanup_label: Option = row.get(4)?; - let state = SandboxStatus::from_columns(&state_label, cleanup_label.as_deref()) - .ok_or_else(|| { - rusqlite::Error::FromSqlConversionFailure( - 0, - Type::Text, - format!("unknown sandbox state '{state_label}', cleanup {cleanup_label:?}") - .into(), - ) - })?; - // This legacy metric folds cleanup ownership across lifecycles. - // Normalize only its bucket key; status counts retain both columns. - let state = match state.cleanup() { - Some(cleanup) => SandboxStatus::owned(Teardown::Stopping, cleanup), - None => state, + let unknown = |column, label: &str| { + let kind = if column == 0 { "state" } else { "cleanup state" }; + rusqlite::Error::FromSqlConversionFailure( + column, + Type::Text, + format!("unknown sandbox {kind} '{label}'").into(), + ) }; - let runtime_label: String = row.get(1)?; - let execution_runtime: Option = row.get(2)?; + let state_label: String = row.get(0)?; + let state = state_label + .parse::() + .map_err(|_| unknown(0, &state_label))?; + let cleanup_state = row + .get::<_, Option>(1)? + .map(|label| { + CleanupState::from_column(&label) + .map(CleanupState::as_str) + .ok_or_else(|| unknown(1, &label)) + }) + .transpose()?; + let runtime_label: String = row.get(2)?; + let execution_runtime: Option = row.get(3)?; let identity = execution_runtime .as_deref() .and_then(|value| serde_json::from_str::(value).ok()); @@ -216,8 +215,8 @@ db_private_outside_tests! { .map(|identity| identity.kind) .or_else(|| runtime_label.parse().ok()); let runtime_version = identity.map(|identity| identity.version); - let count = row.get::<_, SqliteU64>(3)?.0; - Ok(((state, runtime, runtime_version), count)) + let count = row.get::<_, SqliteU64>(4)?.0; + Ok(((state, cleanup_state, runtime, runtime_version), count)) })?; // Runtime aliases must share a bucket; absent or malformed metadata must // not drop durable rows or invent a legacy runtime version. @@ -228,67 +227,34 @@ db_private_outside_tests! { } let mut counts: Vec<_> = grouped .into_iter() - .map(|((state, runtime, runtime_version), count)| SandboxLifecycleCount { - state, - runtime, - runtime_version, - count, - }) + .map( + |((state, cleanup_state, runtime, runtime_version), count)| SandboxStatusCount { + state, + cleanup_state, + runtime, + runtime_version, + count, + }, + ) .collect(); counts.sort_by(|left, right| { - (left.state.legacy_label(), left.runtime, &left.runtime_version).cmp(&( - right.state.legacy_label(), - right.runtime, - &right.runtime_version, - )) + ( + left.state.as_str(), + left.cleanup_state, + left.runtime, + &left.runtime_version, + ) + .cmp(&( + right.state.as_str(), + right.cleanup_state, + right.runtime, + &right.runtime_version, + )) }); Ok(counts) } } -db_private_outside_tests! { - /// Count every durable sandbox-manager row by its stored lifecycle and - /// cleanup state, unsummed. A label this binary does not know fails the - /// count rather than being dropped from it. - fn sandbox_status_counts(conn: &Connection) -> Result> { - let mut statement = conn.prepare( - "SELECT state, cleanup_state, COUNT(*) - FROM sandboxes - GROUP BY state, cleanup_state - ORDER BY state, cleanup_state", - )?; - let rows = statement.query_map([], |row| { - let unknown = |column, label: &str| { - let kind = if column == 0 { "state" } else { "cleanup state" }; - rusqlite::Error::FromSqlConversionFailure( - column, - Type::Text, - format!("unknown sandbox {kind} '{label}'").into(), - ) - }; - let state_label: String = row.get(0)?; - let state = state_label - .parse::() - .map_err(|_| unknown(0, &state_label))?; - let cleanup_state = row - .get::<_, Option>(1)? - .map(|label| { - CleanupState::from_column(&label) - .map(CleanupState::as_str) - .ok_or_else(|| unknown(1, &label)) - }) - .transpose()?; - let count = row.get::<_, SqliteU64>(2)?.0; - Ok(SandboxStatusCount { - state, - cleanup_state, - count, - }) - })?; - Ok(rows.collect::>()?) - } -} - /// A row's `state` and `cleanup_state` as stored, when they name no status /// this binary knows. #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/crates/l30_storage/sbx/mgr/store/src/db/test_fixtures/state_write_fixtures.rs b/crates/l30_storage/sbx/mgr/store/src/db/test_fixtures/state_write_fixtures.rs index 9af31e1886..d3e1c7b285 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/test_fixtures/state_write_fixtures.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/test_fixtures/state_write_fixtures.rs @@ -445,14 +445,13 @@ pub fn write_status(conn: &Connection, id: &str, state: impl Into /// A snapshot shows a `sandboxes` row's combined status as `state`, as rows /// stored it before cleanup ownership moved to its own column, so a write's /// expected diff names one status. The split columns are pinned by the -/// migration and writer tests. `unowned_state` and `legacy_state` are -/// generated from the two, so a snapshot leaves them out. +/// migration and writer tests. `unowned_state` is generated from the two, so +/// a snapshot leaves it out. fn fold_sandbox_status(table: &str, values: &mut Row) { if table != "sandboxes" { return; } values.remove("unowned_state"); - values.remove("legacy_state"); if let Some(Value::Text(cleanup)) = values.remove("cleanup_state") { values.insert( "state".to_owned(), diff --git a/crates/l30_storage/sbx/mgr/store/src/db/tests.rs b/crates/l30_storage/sbx/mgr/store/src/db/tests.rs index 5c46f8051e..3d3084f875 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/tests.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/tests.rs @@ -1053,56 +1053,7 @@ fn access_idle_policy() -> AutoLifecyclePolicy { } #[test] -fn sandbox_lifecycle_counts_groups_durable_rows() { - let conn = test_conn(); - for (id, uuid8, state) in [ - ( - "started-1", - "abc00001", - SandboxStatus::from(SandboxState::Started), - ), - ( - "started-2", - "abc00002", - SandboxStatus::from(SandboxState::Started), - ), - ("cleanup-suspend", "abc00003", SandboxStatus::CLEANUP_FAILED), - ("cleanup-delete", "abc00004", SandboxStatus::CLEANUP_FAILED), - ] { - insert_test_sandbox(&conn, TestSandboxInsert::new(id, uuid8, state)); - } - set_cleanup_intent( - &conn, - "cleanup-suspend", - true, - CleanupCompletion::MarkSuspended, - ) - .unwrap(); - set_cleanup_intent(&conn, "cleanup-delete", false, CleanupCompletion::DeleteRow).unwrap(); - - let counts = sandbox_lifecycle_counts(&conn).unwrap(); - - assert_eq!( - counts, - vec![ - SandboxLifecycleCount { - state: SandboxStatus::CLEANUP_FAILED, - runtime: Some(RuntimeKind::Firecracker), - runtime_version: None, - count: 2, - }, - SandboxLifecycleCount { - state: SandboxStatus::from(SandboxState::Started), - runtime: Some(RuntimeKind::Firecracker), - runtime_version: None, - count: 2, - }, - ] - ); -} - -#[test] -fn sandbox_lifecycle_counts_preserve_rows_across_runtime_versions_and_legacy_metadata() { +fn sandbox_status_counts_preserve_rows_across_runtime_versions_and_legacy_metadata() { let conn = test_conn(); let versions = [ ("old-a", "abc00101", SandboxState::Started, "fc", "1.15.1"), @@ -1203,13 +1154,14 @@ fn sandbox_lifecycle_counts_preserve_rows_across_runtime_versions_and_legacy_met ); } - let counts = sandbox_lifecycle_counts(&conn).unwrap(); + let counts = sandbox_status_counts(&conn).unwrap(); assert_eq!(counts.iter().map(|count| count.count).sum::(), 11); let actual: Vec<_> = counts .iter() .map(|count| { ( count.state, + count.cleanup_state, count.runtime, count .runtime_version @@ -1222,33 +1174,38 @@ fn sandbox_lifecycle_counts_preserve_rows_across_runtime_versions_and_legacy_met assert_eq!( actual, vec![ - (SandboxStatus::from(SandboxState::Started), None, None, 1), + (SandboxState::Started, None, None, None, 1), ( - SandboxStatus::from(SandboxState::Started), + SandboxState::Started, + None, Some(RuntimeKind::Firecracker), None, 5 ), ( - SandboxStatus::from(SandboxState::Started), + SandboxState::Started, + None, Some(RuntimeKind::Firecracker), Some("1.15.1"), 2 ), ( - SandboxStatus::from(SandboxState::Started), + SandboxState::Started, + None, Some(RuntimeKind::Firecracker), Some("1.17.0"), 1 ), ( - SandboxStatus::from(SandboxState::Started), + SandboxState::Started, + None, Some(RuntimeKind::CloudHypervisor), Some("49.0"), 1 ), ( - SandboxStatus::from(SandboxState::Suspended), + SandboxState::Suspended, + None, Some(RuntimeKind::Firecracker), Some("1.17.0"), 1 @@ -1258,10 +1215,10 @@ fn sandbox_lifecycle_counts_preserve_rows_across_runtime_versions_and_legacy_met conn.execute(sql!("DELETE FROM sandboxes WHERE id = ?1"), ["new"]) .unwrap(); - let counts = sandbox_lifecycle_counts(&conn).unwrap(); + let counts = sandbox_status_counts(&conn).unwrap(); assert_eq!(counts.iter().map(|count| count.count).sum::(), 10); assert!(!counts.iter().any(|count| { - count.state.is_unowned(SandboxState::Started) + count.state == SandboxState::Started && count .runtime_version .as_ref() @@ -1302,6 +1259,8 @@ fn sandbox_status_counts_keep_each_lifecycle_and_cleanup_pair_apart() { let status = |state, cleanup_state| SandboxStatusCount { state, cleanup_state, + runtime: Some(RuntimeKind::Firecracker), + runtime_version: None, count: 1, }; assert_eq!( @@ -1315,23 +1274,6 @@ fn sandbox_status_counts_keep_each_lifecycle_and_cleanup_pair_apart() { status(SandboxState::Stopping, Some("required")), ] ); - - // The legacy count folds each cleanup into its combined status. - let legacy: Vec<_> = sandbox_lifecycle_counts(&conn) - .unwrap() - .into_iter() - .map(|count| (count.state, count.count)) - .collect(); - assert_eq!( - legacy, - vec![ - (SandboxStatus::CLEANUP_FAILED, 2), - (SandboxStatus::CLEANUP_REQUIRED, 1), - (SandboxStatus::from(SandboxState::Deleting), 1), - (SandboxStatus::from(SandboxState::Started), 1), - (SandboxStatus::from(SandboxState::Stopping), 1), - ] - ); } #[test] diff --git a/crates/l30_storage/sbx/mgr/store/src/db/tests/cleanup_state_split.rs b/crates/l30_storage/sbx/mgr/store/src/db/tests/cleanup_state_split.rs index 9f4a1b64d5..aea077f962 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/tests/cleanup_state_split.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/tests/cleanup_state_split.rs @@ -194,6 +194,14 @@ const UNOWNED_UP: &str = include_str!( const UNOWNED_DOWN: &str = include_str!( "../../../../../../../../db/sandbox-manager/20261108130000_sandbox_unowned_state.down.sql" ); +/// The release after drops `legacy_state`, which the split's down migration +/// reads. +const LEGACY_DROP_UP: &str = include_str!( + "../../../../../../../../db/sandbox-manager/20261109120000_drop_sandbox_legacy_state.up.sql" +); +const LEGACY_DROP_DOWN: &str = include_str!( + "../../../../../../../../db/sandbox-manager/20261109120000_drop_sandbox_legacy_state.down.sql" +); /// Insert a row in the split form, keeping the filesystem for every /// completion that keeps the row. @@ -345,6 +353,7 @@ fn the_migration_round_trips_every_cleanup_shape_and_keeps_historical_labels() { .collect() }; + conn.execute_batch(LEGACY_DROP_DOWN).unwrap(); conn.execute_batch(UNOWNED_DOWN).unwrap(); conn.execute_batch(MIGRATION_DOWN).unwrap(); assert_eq!( @@ -362,6 +371,7 @@ fn the_migration_round_trips_every_cleanup_shape_and_keeps_historical_labels() { conn.execute_batch(MIGRATION_UP).unwrap(); conn.execute_batch(UNOWNED_UP).unwrap(); + conn.execute_batch(LEGACY_DROP_UP).unwrap(); // A migration can't tell a handed-off suspend from a restore rollback, so // `c` comes back `stopping`; its combined status is unchanged. for (id, expected) in [ diff --git a/crates/l30_storage/sbx/mgr/store/src/state.rs b/crates/l30_storage/sbx/mgr/store/src/state.rs index cd2eb89928..fee2b4266d 100644 --- a/crates/l30_storage/sbx/mgr/store/src/state.rs +++ b/crates/l30_storage/sbx/mgr/store/src/state.rs @@ -731,9 +731,10 @@ impl SandboxStatus { self.cleanup().map_or("none", CleanupState::as_str) } - /// The label `sandbox.worker.lifecycle.count` groups by, which predates - /// the split: `cleanup_required`/`cleanup_failed` for a cleanup-owned - /// row, otherwise the lifecycle. + /// The status label that predates the split: `cleanup_required` or + /// `cleanup_failed` for a cleanup-owned row, otherwise the lifecycle. + /// Episodes store it as `entered_from_state`, and the not-running marker + /// and the net-gateway inventory tag still carry it. pub fn legacy_label(self) -> &'static str { match self { Self::Unowned(lifecycle) => lifecycle.as_str(), diff --git a/crates/l40_api/api/src/controllers/sandbox/list_page/admin.rs b/crates/l40_api/api/src/controllers/sandbox/list_page/admin.rs index c4f6c0d13f..a35e08297a 100644 --- a/crates/l40_api/api/src/controllers/sandbox/list_page/admin.rs +++ b/crates/l40_api/api/src/controllers/sandbox/list_page/admin.rs @@ -49,11 +49,6 @@ const DELETED_LIFECYCLE: &str = "deleted"; /// The `sandboxes.lifecycle` values outside [`SandboxLifecycle`]. const TOMBSTONE_LIFECYCLES: [&str; 1] = [DELETED_LIFECYCLE]; -/// Values no writer stores any more that the filter still takes, for one -/// release, so an older console or a saved link gets an empty page rather -/// than a 400. They match no row. Remove them when the cleanup states retire. -const RETIRED_LIFECYCLES: [&str; 1] = ["cleanup_required"]; - /// A stored lifecycle (`sandboxes.lifecycle`) the admin list filters on: a /// [`SandboxLifecycle`] state or a tombstone, in the column's form. /// @@ -77,7 +72,6 @@ impl FromStr for AdminStoredLifecycle { let normalized = value.trim().to_ascii_lowercase(); if let Some(tombstone) = TOMBSTONE_LIFECYCLES .into_iter() - .chain(RETIRED_LIFECYCLES) .find(|tombstone| *tombstone == normalized) { return Ok(Self(tombstone)); diff --git a/crates/l40_api/api/src/controllers/sandbox/list_page/admin/tests.rs b/crates/l40_api/api/src/controllers/sandbox/list_page/admin/tests.rs index 2c0ec92d5a..2a4107e9df 100644 --- a/crates/l40_api/api/src/controllers/sandbox/list_page/admin/tests.rs +++ b/crates/l40_api/api/src/controllers/sandbox/list_page/admin/tests.rs @@ -863,7 +863,7 @@ async fn the_node_filters_match_the_node_the_response_reports() -> TestDbResult< } /// The filter takes every value `sandboxes.lifecycle` holds, as the stored -/// vocabulary normalizes it, and otherwise only the retired values. +/// vocabulary normalizes it, and nothing else. #[test] fn the_lifecycle_filter_takes_exactly_the_stored_lifecycles() { for stored in pplx_space_sandbox_store::StoredSandboxLifecycle::all() { @@ -886,17 +886,7 @@ fn the_lifecycle_filter_takes_exactly_the_stored_lifecycles() { ); } } - // A retired value is still taken, though no row stores it. - assert_eq!( - " Cleanup_Required " - .parse::() - .map(AdminStoredLifecycle::as_str), - Ok("cleanup_required") - ); - "cleanup_required" - .parse::() - .unwrap_err(); - for unknown in ["", "delete", "cleanup-required", "live"] { + for unknown in ["", "delete", "cleanup_required", "cleanup-required", "live"] { assert!( unknown.parse::().is_err(), "{unknown:?}" diff --git a/crates/l40_api/api/src/controllers/sandbox/responses.rs b/crates/l40_api/api/src/controllers/sandbox/responses.rs index 72829834f1..53e5d47aeb 100644 --- a/crates/l40_api/api/src/controllers/sandbox/responses.rs +++ b/crates/l40_api/api/src/controllers/sandbox/responses.rs @@ -745,7 +745,7 @@ pub fn sandbox_to_response(sandbox: &Sandbox, live: Option<&SandboxInfo>) -> San .and_then(|info| info.auto_lifecycle_status.as_ref()) .and_then(|status| status_from_proto(status).ok()); let live_lifecycle = live.map(|info| { - info.observed_lifecycle() + info.observed_status() .map(|state| state.label().to_string()) .unwrap_or_else(|_| "unknown".into()) }); diff --git a/crates/l40_api/api/src/controllers/sandbox/updates.rs b/crates/l40_api/api/src/controllers/sandbox/updates.rs index ea9e5f1cd7..62030d7dae 100644 --- a/crates/l40_api/api/src/controllers/sandbox/updates.rs +++ b/crates/l40_api/api/src/controllers/sandbox/updates.rs @@ -165,8 +165,9 @@ pub(super) fn observed_access_idle_generation( if observed.sandbox_id != sandbox_id.to_string() || !response.missing_ids.is_empty() || observed.state_degraded - || observed.observed_lifecycle().ok() - != Some(pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Started) + || !observed.observed_status().is_ok_and(|status| { + status.is_unowned(pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Started) + }) { return Err(unavailable()); } @@ -211,7 +212,11 @@ fn classify_deadline_execution( { return Err(tonic::Status::failed_precondition("access deadline execution changed").into()); } - match observed.observed_lifecycle().ok() { + match observed + .observed_status() + .ok() + .and_then(|status| status.unowned()) + { Some(SandboxLifecycleState::Started) => Ok(DeadlineExecutionReadiness::Ready), Some( SandboxLifecycleState::Creating diff --git a/crates/l40_api/api/src/controllers/substrate/direct.rs b/crates/l40_api/api/src/controllers/substrate/direct.rs index 72db37b006..596e533451 100644 --- a/crates/l40_api/api/src/controllers/substrate/direct.rs +++ b/crates/l40_api/api/src/controllers/substrate/direct.rs @@ -3201,9 +3201,13 @@ impl DirectSubstrate { ) -> Result { use pplx_space_sbx_mgr_client::proto::SandboxLifecycleState; - let (Ok(SandboxLifecycleState::Started), Some(record_generation)) = - (record.observed_lifecycle(), record.execution_generation) - else { + let (Some(SandboxLifecycleState::Started), Some(record_generation)) = ( + record + .observed_status() + .ok() + .and_then(|status| status.unowned()), + record.execution_generation, + ) else { return Ok(RouteCatchUp::NotNeeded); }; let trails = sqlx::query_scalar::<_, bool>(sql!( @@ -3341,18 +3345,17 @@ impl DirectSubstrate { use pplx_space_sbx_mgr_client::proto::SandboxLifecycleState; if !record - .observed_lifecycle() - .is_ok_and(lifecycle_holds_live_runtime) + .observed_status() + .is_ok_and(|status| status.unowned().is_some_and(lifecycle_holds_live_runtime)) { return Ok(None); } let Some(execution_generation) = record.execution_generation else { return Ok(None); }; - let started = matches!( - record.observed_lifecycle(), - Ok(SandboxLifecycleState::Started) - ); + let started = record + .observed_status() + .is_ok_and(|status| status.is_unowned(SandboxLifecycleState::Started)); self.adopt_live_execution( sandbox_id, read_lifecycle, @@ -3478,14 +3481,14 @@ impl DirectSubstrate { .await { Ok(Ok(None)) => return Ok(()), - Ok(Ok(Some(record))) => match record.observed_lifecycle() { - Ok(lifecycle) if !lifecycle_holds_live_runtime(lifecycle) => {} - Ok(lifecycle) => { + Ok(Ok(Some(record))) => match record.observed_status() { + Ok(status) if !status.unowned().is_some_and(lifecycle_holds_live_runtime) => {} + Ok(status) => { tracing::warn!( sandbox_id = %sandbox_id, source_node_id = %source_node_id, restored_node_id = %restored_node_id, - lifecycle = lifecycle.label(), + lifecycle = status.label(), "leaving the source-node copy after direct cross-node restore because it reports a live record" ); return Ok(()); diff --git a/crates/l40_api/api/src/controllers/substrate/direct/tests.rs b/crates/l40_api/api/src/controllers/substrate/direct/tests.rs index f6da5bcf89..0be4cbf61c 100644 --- a/crates/l40_api/api/src/controllers/substrate/direct/tests.rs +++ b/crates/l40_api/api/src/controllers/substrate/direct/tests.rs @@ -528,8 +528,6 @@ fn only_a_resident_that_restores_or_starts_reserves_an_identity_before_waking() | State::Checkpointing | State::Suspending | State::Restoring - | State::CleanupRequired - | State::CleanupFailed | State::Quarantined | State::Failed => false, }; @@ -539,7 +537,7 @@ fn only_a_resident_that_restores_or_starts_reserves_an_identity_before_waking() "{state:?}" ); } - assert_eq!(seen, 19, "every lifecycle state is covered"); + assert_eq!(seen, 17, "every lifecycle state is covered"); assert!(!resident_is_dormant(&resident(None))); } diff --git a/crates/l40_api/api/src/controllers/substrate/mod.rs b/crates/l40_api/api/src/controllers/substrate/mod.rs index e2748014f9..dd18fef3e5 100644 --- a/crates/l40_api/api/src/controllers/substrate/mod.rs +++ b/crates/l40_api/api/src/controllers/substrate/mod.rs @@ -2402,10 +2402,9 @@ fn node_reported_artifact_from_info( pin_unreadable, // An undecodable lifecycle reads as not stopped, which the caller // treats as the node disagreeing: a deferral, never a teardown. - reports_stopped: matches!( - info.observed_lifecycle(), - Ok(pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Stopped) - ), + reports_stopped: info.observed_status().is_ok_and(|status| { + status.is_unowned(pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Stopped) + }), // `namespace`, not `pid`: the node sets `namespace` from its live // process probe alone, while `pid` also needs the row's persisted pid, // which every write into `stopped` clears. diff --git a/crates/l40_api/api/src/controllers/substrate/sandbox_state_probe.rs b/crates/l40_api/api/src/controllers/substrate/sandbox_state_probe.rs index 32334957fe..60e2d32cb4 100644 --- a/crates/l40_api/api/src/controllers/substrate/sandbox_state_probe.rs +++ b/crates/l40_api/api/src/controllers/substrate/sandbox_state_probe.rs @@ -93,7 +93,7 @@ pub(super) fn observed_sandbox_states( // map so the caller classifies the requested id as inconclusive. continue; }; - let node_status = match info.observed_lifecycle() { + let node_status = match info.observed_status() { Ok(status) => status.label().parse::().ok(), Err(error) => { tracing::warn!( diff --git a/crates/l40_api/api/src/routes/admin/docs.rs b/crates/l40_api/api/src/routes/admin/docs.rs index 2b8ffb6ac8..c748d0682d 100644 --- a/crates/l40_api/api/src/routes/admin/docs.rs +++ b/crates/l40_api/api/src/routes/admin/docs.rs @@ -845,7 +845,7 @@ fn insert_admin_sandbox_create_paths(paths: &mut Map) { { "name": "limit", "in": "query", "required": false, "description": "1 to 5000, default 1000", "schema": { "type": "integer" } }, { "name": "cursor", "in": "query", "required": false, "description": "Opaque; empty for the first page, then the previous page's next_cursor", "schema": { "type": "string" } }, { "name": "org_id", "in": "query", "required": false, "schema": { "type": "string" } }, - { "name": "lifecycle", "in": "query", "required": false, "description": "Comma-separated values of the stored lifecycle, including deleted. The retired cleanup_required is still accepted and matches no sandbox. No response field shows the stored lifecycle: lifecycle and status report the node's observed status when there is one, so a row can show another value.", "schema": { "type": "string" } }, + { "name": "lifecycle", "in": "query", "required": false, "description": "Comma-separated values of the stored lifecycle, including deleted. No response field shows the stored lifecycle: lifecycle and status report the node's observed status when there is one, so a row can show another value.", "schema": { "type": "string" } }, { "name": "template", "in": "query", "required": false, "description": "The sandbox row's own template. A row without one does not match, although the response's template, like the spaced-version count, falls back to the template its node reports.", "schema": { "type": "string" } }, { "name": "node_id", "in": "query", "required": false, "description": "The node the response reports as active_node_id: a sandbox's current Direct route node when it has a route, whatever its row records, else the node its row records", "schema": { "type": "string", "format": "uuid" } }, { "name": "assigned", "in": "query", "required": false, "description": "true for sandboxes the response reports with an active_node_id, false for those without; false cannot be combined with node_id. Like every filter, it lists sandbox rows only, so false leaves out the node-reported pool slots and observed-local sandboxes that the unfiltered list appends, although they report no active_node_id", "schema": { "type": "boolean" } }, diff --git a/crates/l40_api/node/ctl/server/src/facets/cluster_deleted_eviction.rs b/crates/l40_api/node/ctl/server/src/facets/cluster_deleted_eviction.rs index f5bd65a9bc..cb4bd05a00 100644 --- a/crates/l40_api/node/ctl/server/src/facets/cluster_deleted_eviction.rs +++ b/crates/l40_api/node/ctl/server/src/facets/cluster_deleted_eviction.rs @@ -160,7 +160,11 @@ pub(super) fn select_cluster_deleted_evict_candidates<'a>( /// budget and back-off slots on ids sbx-manager is bound to refuse. pub(super) fn local_row_is_cluster_deleted_reclaimable(state: &SandboxInfo) -> bool { use pplx_space_sbx_mgr_client::proto::SandboxLifecycleState; - match state.observed_lifecycle().ok() { + match state + .observed_status() + .ok() + .and_then(|status| status.unowned()) + { Some( SandboxLifecycleState::Stopped | SandboxLifecycleState::Suspended diff --git a/crates/l40_api/node/ctl/server/src/facets/local_state.rs b/crates/l40_api/node/ctl/server/src/facets/local_state.rs index 059f660632..e3b4cc547c 100644 --- a/crates/l40_api/node/ctl/server/src/facets/local_state.rs +++ b/crates/l40_api/node/ctl/server/src/facets/local_state.rs @@ -383,18 +383,18 @@ pub(super) fn published_sandbox_state( policy: Option<&NetworkPolicyDetail>, snapshots: Vec, ) -> PublishedSandboxState { - let lifecycle = sandbox.observed_lifecycle(); - if let Err(error) = &lifecycle { + let observed = sandbox.observed_status(); + if let Err(error) = &observed { tracing::warn!(sandbox_id = %sandbox.sandbox_id, %error, "preserving cluster lifecycle after unknown node observation"); } PublishedSandboxState { sandbox_id: sandbox.sandbox_id.clone(), present: true, template: sandbox.template.clone(), - status: lifecycle + status: observed .as_ref() .ok() - .map(|state| state.label().to_string()), + .map(|status| status.label().to_string()), content_hash: sandbox.image_hash.clone(), runtime: sandbox.runtime.clone(), created_at: sandbox.created_at.clone(), @@ -412,11 +412,11 @@ pub(super) fn published_sandbox_state( failure_reason: sandbox.failure_reason.clone(), quarantine_reason_code: sandbox.quarantine_reason_code.clone(), transition_receipt: sandbox.transition_receipt.clone(), - state_degraded: sandbox.state_degraded || lifecycle.is_err(), + state_degraded: sandbox.state_degraded || observed.is_err(), current_local_artifact_id: sandbox.current_local_artifact_id.clone(), current_local_artifact_kind: sandbox.current_local_artifact_kind, current_local_artifact_origin: sandbox.current_local_artifact_origin, - state_changed_at: lifecycle.as_ref().ok().and(sandbox.state_changed_at), + state_changed_at: observed.as_ref().ok().and(sandbox.state_changed_at), quarantine_recovery: sandbox.quarantine_recovery.clone(), execution_generation: sandbox.execution_generation, pod_uid: sandbox.pod_uid.clone(), diff --git a/crates/l40_api/node/ctl/server/src/facets/port_exposure.rs b/crates/l40_api/node/ctl/server/src/facets/port_exposure.rs index dc16578573..0b9ab15616 100644 --- a/crates/l40_api/node/ctl/server/src/facets/port_exposure.rs +++ b/crates/l40_api/node/ctl/server/src/facets/port_exposure.rs @@ -38,8 +38,8 @@ impl std::fmt::Display for PortExposureRpc { /// /// Terminal **only** when sbx-manager marks the status. sbx-manager answers /// `FailedPrecondition` for both terminal cases (guest unreachable in a -/// settled non-reachable state; auto-lifecycle target stuck in -/// `Failed`/`CleanupRequired`/`CleanupFailed`/`Quarantined`) and transient ones (a guest still +/// settled non-reachable state; auto-lifecycle target stuck in `Failed` or +/// `Quarantined`, or owned by a cleanup) and transient ones (a guest still /// initializing — `guest_not_ready`; a mid-transition lifecycle state that /// settles), and `NotFound` for both a gone row and the startup race where the /// local row has not appeared yet (`ApplyPortExposuresAndRetry`). Neither bare diff --git a/crates/l40_api/node/ctl/server/src/facets/tests.rs b/crates/l40_api/node/ctl/server/src/facets/tests.rs index 9a8a7bdc59..a1b550548a 100644 --- a/crates/l40_api/node/ctl/server/src/facets/tests.rs +++ b/crates/l40_api/node/ctl/server/src/facets/tests.rs @@ -1320,6 +1320,43 @@ fn missing_or_unknown_lifecycle_preserves_cluster_state() { } } +/// The control plane still reads a cleanup-owned row's status as +/// `cleanup_required` or `cleanup_failed`, whichever teardown transition +/// sandbox-manager publishes it under. +#[test] +fn a_cleanup_owned_row_publishes_its_cleanup_status() { + use pplx_space_sbx_mgr_client::proto::{ + SandboxCleanup, SandboxCleanupState, SandboxLifecycleState, + }; + for (cleanup, expected) in [ + (SandboxCleanupState::Required, "cleanup_required"), + (SandboxCleanupState::Failed, "cleanup_failed"), + ] { + for lifecycle in [ + SandboxLifecycleState::Stopping, + SandboxLifecycleState::Deleting, + SandboxLifecycleState::Suspending, + ] { + let sandbox = SandboxInfo { + sandbox_id: TEST_SANDBOX_ID.to_string(), + lifecycle: Some(lifecycle as i32), + cleanup: Some(SandboxCleanup { + state: cleanup as i32, + ..SandboxCleanup::default() + }), + ..Default::default() + }; + let publication = published_sandbox_state(&sandbox, None, None, Vec::new()); + assert_eq!( + publication.status.as_deref(), + Some(expected), + "{lifecycle:?}" + ); + assert!(!publication.state_degraded); + } + } +} + #[test] fn recovered_suspended_published_sandbox_state_wire_shape() { let sandbox = pplx_space_sbx_mgr_client::test_support::with_suspend_artifact( diff --git a/crates/l40_api/sbx/mgr/server/src/lifecycle_metrics.rs b/crates/l40_api/sbx/mgr/server/src/lifecycle_metrics.rs index ae1d5f7bf9..54aba0553b 100644 --- a/crates/l40_api/sbx/mgr/server/src/lifecycle_metrics.rs +++ b/crates/l40_api/sbx/mgr/server/src/lifecycle_metrics.rs @@ -4,90 +4,49 @@ use std::time::Duration; use pplx_space_common_core::types::{RuntimeKind, RuntimeVersion}; use tokio_util::sync::CancellationToken; -use crate::db::{SandboxLifecycleCount, SandboxStatusCount}; +use crate::db::SandboxStatusCount; const SAMPLE_INTERVAL: Duration = Duration::from_secs(60); -#[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] -struct LifecycleBucket { - state: String, - runtime: Option, - runtime_version: Option, -} - -/// A `sandbox.worker.lifecycle.status_count` series: a stored lifecycle and -/// the cleanup state over it, `none` when no cleanup owns the rows. +/// A `sandbox.worker.lifecycle.status_count` series: a stored lifecycle, the +/// cleanup state over it (`none` when no cleanup owns the rows) and the +/// recorded runtime. #[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] struct StatusBucket { state: String, cleanup_state: String, + runtime: Option, + runtime_version: Option, } -/// A sampled count and the gauge series it is emitted under. -trait SampledCount { - type Bucket: Clone + Ord; - - fn bucket(&self) -> Self::Bucket; - fn count(&self) -> u64; -} - -impl SampledCount for SandboxLifecycleCount { - type Bucket = LifecycleBucket; - - fn bucket(&self) -> LifecycleBucket { - LifecycleBucket { - // `sandbox.worker.lifecycle.count` keeps the label that predates - // the cleanup-state split. - state: self.state.legacy_label().to_owned(), - runtime: self.runtime, - runtime_version: self.runtime_version.clone(), - } - } - - fn count(&self) -> u64 { - self.count - } -} - -impl SampledCount for SandboxStatusCount { - type Bucket = StatusBucket; - - fn bucket(&self) -> StatusBucket { - StatusBucket { - state: self.state.as_str().to_owned(), - cleanup_state: self.cleanup_state.unwrap_or("none").to_owned(), +impl StatusBucket { + fn of(count: &SandboxStatusCount) -> Self { + Self { + state: count.state.as_str().to_owned(), + cleanup_state: count.cleanup_state.unwrap_or("none").to_owned(), + runtime: count.runtime, + runtime_version: count.runtime_version.clone(), } } - - fn count(&self) -> u64 { - self.count - } } -/// Emits one metric's series and zeroes, once, each series that disappeared -/// since the previous sample. Each metric has its own sampler. -struct LifecycleGaugeSampler { - previous_buckets: BTreeSet, +/// Emits the series and zeroes, once, each series that disappeared since the +/// previous sample. +#[derive(Default)] +struct StatusGaugeSampler { + previous_buckets: BTreeSet, } -impl Default for LifecycleGaugeSampler { - fn default() -> Self { - Self { - previous_buckets: BTreeSet::new(), - } - } -} - -impl LifecycleGaugeSampler { - fn emit_sample>( +impl StatusGaugeSampler { + fn emit_sample( &mut self, - counts: &[C], - mut emit: impl FnMut(&B, u64), + counts: &[SandboxStatusCount], + mut emit: impl FnMut(&StatusBucket, u64), ) { - let current = counts.iter().map(C::bucket).collect::>(); + let current = counts.iter().map(StatusBucket::of).collect::>(); for count in counts { - emit(&count.bucket(), count.count()); + emit(&StatusBucket::of(count), count.count); } for bucket in self.previous_buckets.difference(¤t) { emit(bucket, 0); @@ -98,8 +57,7 @@ impl LifecycleGaugeSampler { } pub(crate) async fn run(db: crate::db::SharedConnection, shutdown_token: CancellationToken) { - let mut lifecycle_sampler = LifecycleGaugeSampler::default(); - let mut status_sampler = LifecycleGaugeSampler::default(); + let mut sampler = StatusGaugeSampler::default(); let mut interval = tokio::time::interval(SAMPLE_INTERVAL); interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); @@ -109,7 +67,7 @@ pub(crate) async fn run(db: crate::db::SharedConnection, shutdown_token: Cancell _ = interval.tick() => {} } - let (lifecycle_counts, status_counts) = { + let counts = { let db = match db.lock() { Ok(db) => db, Err(error) => { @@ -117,32 +75,16 @@ pub(crate) async fn run(db: crate::db::SharedConnection, shutdown_token: Cancell continue; } }; - ( - crate::db::read::counts(&db, crate::db::read::ByLifecycle), - crate::db::read::counts(&db, crate::db::read::ByStatus), - ) + crate::db::read::counts(&db, crate::db::read::ByStatus) }; - match lifecycle_counts { - Ok(counts) => { - lifecycle_sampler.emit_sample(&counts, |bucket: &LifecycleBucket, count| { - crate::metrics::sandbox::worker::lifecycle_count( - count, - &bucket.state, - bucket.runtime, - bucket.runtime_version.as_ref(), - ); - }) - } - Err(error) => { - tracing::warn!(%error, "failed to sample durable sandbox lifecycle counts"); - } - } - match status_counts { - Ok(counts) => status_sampler.emit_sample(&counts, |bucket: &StatusBucket, count| { + match counts { + Ok(counts) => sampler.emit_sample(&counts, |bucket, count| { crate::metrics::sandbox::worker::lifecycle_status_count( count, &bucket.state, &bucket.cleanup_state, + bucket.runtime, + bucket.runtime_version.as_ref(), ); }), Err(error) => { diff --git a/crates/l40_api/sbx/mgr/server/src/lifecycle_metrics/tests.rs b/crates/l40_api/sbx/mgr/server/src/lifecycle_metrics/tests.rs index 01194ee98c..7c7edc04a7 100644 --- a/crates/l40_api/sbx/mgr/server/src/lifecycle_metrics/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/lifecycle_metrics/tests.rs @@ -1,107 +1,37 @@ use super::*; -use crate::state::{SandboxState, SandboxStatus}; +use crate::state::SandboxState; #[test] -fn disappeared_lifecycle_buckets_are_zeroed_once() { - let mut sampler = LifecycleGaugeSampler::default(); - let initial = [ - SandboxLifecycleCount { - state: SandboxStatus::CLEANUP_FAILED, - runtime: None, - runtime_version: None, - count: 2, - }, - SandboxLifecycleCount { - state: SandboxState::Started.into(), - runtime: None, - runtime_version: None, - count: 4, - }, - ]; - let mut emitted = Vec::new(); - sampler.emit_sample(&initial, |bucket, count| { - emitted.push((bucket.clone(), count)); - }); - sampler.emit_sample(&initial[1..], |bucket, count| { - emitted.push((bucket.clone(), count)); - }); - sampler.emit_sample(&initial[1..], |bucket, count| { - emitted.push((bucket.clone(), count)); - }); - - assert_eq!( - emitted, - vec![ - ( - LifecycleBucket { - state: "cleanup_failed".to_owned(), - runtime: None, - runtime_version: None, - }, - 2, - ), - ( - LifecycleBucket { - state: "started".to_owned(), - runtime: None, - runtime_version: None, - }, - 4, - ), - ( - LifecycleBucket { - state: "started".to_owned(), - runtime: None, - runtime_version: None, - }, - 4, - ), - ( - LifecycleBucket { - state: "cleanup_failed".to_owned(), - runtime: None, - runtime_version: None, - }, - 0, - ), - ( - LifecycleBucket { - state: "started".to_owned(), - runtime: None, - runtime_version: None, - }, - 4, - ), - ] - ); -} - -#[test] -fn status_buckets_name_the_cleanup_state_and_zero_a_disappeared_series_once() { - let mut sampler = LifecycleGaugeSampler::default(); +fn status_buckets_name_the_cleanup_state_and_runtime_and_zero_a_disappeared_series_once() { + let mut sampler = StatusGaugeSampler::default(); let initial = [ SandboxStatusCount { state: SandboxState::Stopping, cleanup_state: Some("failed"), + runtime: Some(RuntimeKind::Firecracker), + runtime_version: None, count: 2, }, SandboxStatusCount { state: SandboxState::Stopping, cleanup_state: None, + runtime: Some(RuntimeKind::Firecracker), + runtime_version: None, count: 1, }, ]; let bucket = |cleanup_state: &str| StatusBucket { state: "stopping".to_owned(), cleanup_state: cleanup_state.to_owned(), + runtime: Some(RuntimeKind::Firecracker), + runtime_version: None, }; let mut emitted = Vec::new(); - sampler.emit_sample(&initial, |bucket, count| { - emitted.push((bucket.clone(), count)); - }); - sampler.emit_sample(&initial[1..], |bucket, count| { - emitted.push((bucket.clone(), count)); - }); + for sample in [&initial[..], &initial[1..], &initial[1..]] { + sampler.emit_sample(sample, |bucket, count| { + emitted.push((bucket.clone(), count)); + }); + } assert_eq!( emitted, @@ -110,6 +40,7 @@ fn status_buckets_name_the_cleanup_state_and_zero_a_disappeared_series_once() { (bucket("none"), 1), (bucket("none"), 1), (bucket("failed"), 0), + (bucket("none"), 1), ] ); } diff --git a/crates/l40_api/sbx/mgr/server/src/metrics/sandbox/worker/inventory.rs b/crates/l40_api/sbx/mgr/server/src/metrics/sandbox/worker/inventory.rs index 2d9e055803..5c3ecee086 100644 --- a/crates/l40_api/sbx/mgr/server/src/metrics/sandbox/worker/inventory.rs +++ b/crates/l40_api/sbx/mgr/server/src/metrics/sandbox/worker/inventory.rs @@ -1,21 +1,26 @@ -//! Inventory: list budgets, lifecycle and status counts, orphan rows and inventory reconciliation. +//! Inventory: list budgets, status counts, orphan rows and inventory reconciliation. use super::*; -/// Current node-local durable sandbox rows in one lifecycle/runtime bucket. The caller -/// supplies a value decoded through the sandbox-manager's bounded state enum; -/// do not attach sandbox, pod, node, or execution identifiers. -pub fn lifecycle_count( +/// Current node-local durable sandbox rows with one stored lifecycle, cleanup +/// state and runtime. A cleanup is not folded into the lifecycle: +/// `cleanup_state` is `required`, `failed` or `none`, and `state` is always a +/// lifecycle state. The caller supplies values decoded through the +/// sandbox-manager's bounded enums; do not attach sandbox, pod, node, or +/// execution identifiers. +pub fn lifecycle_status_count( value: u64, state: &str, + cleanup_state: &str, runtime: Option, runtime_version: Option<&pplx_space_common_core::types::RuntimeVersion>, ) { gauge( - "sandbox.worker.lifecycle.count", + "sandbox.worker.lifecycle.status_count", value as f64, &[ tag("state", state), + tag("cleanup_state", cleanup_state), tag( "runtime", runtime.map_or("unknown", |runtime| runtime.as_str()), @@ -28,20 +33,6 @@ pub fn lifecycle_count( ); } -/// Current node-local durable sandbox rows with one stored lifecycle and -/// cleanup state. Unlike `sandbox.worker.lifecycle.count`, a cleanup is not -/// folded into the lifecycle: `cleanup_state` is `required`, `failed` or -/// `none`, and `state` is always a lifecycle state. The caller supplies values -/// decoded through the sandbox-manager's bounded state enums; do not attach -/// sandbox, pod, node, or execution identifiers. -pub fn lifecycle_status_count(value: u64, state: &str, cleanup_state: &str) { - gauge( - "sandbox.worker.lifecycle.status_count", - value as f64, - &[tag("state", state), tag("cleanup_state", cleanup_state)], - ); -} - /// Disposition of an inventory-reconcile orphan row materialization — which /// arm fired when the reconcile found sandbox evidence (disk or runtime) /// with no sbx-manager metadata row. Shared by the diff --git a/crates/l40_api/sbx/mgr/server/src/service/tests.rs b/crates/l40_api/sbx/mgr/server/src/service/tests.rs index ffd60be887..b48b53206f 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/tests.rs @@ -7221,6 +7221,7 @@ pub(crate) mod service_tests { #[tokio::test(start_paused = true)] async fn wait_completes_only_for_terminal_statuses_without_a_caller_deadline() { + use pplx_space_sbx_mgr_client::lifecycle::{ObservedCleanup, ObservedStatus, Teardown}; for state in [ SandboxStatus::from(SandboxState::Quarantined), SandboxStatus::from(SandboxState::Stopped), @@ -7243,10 +7244,13 @@ pub(crate) mod service_tests { .await; let expected = match state.cleanup() { Some(CleanupState::Required) => None, - Some(CleanupState::Failed) => { - Some(crate::proto::SandboxLifecycleState::CleanupFailed) - } - None => Some(crate::proto::SandboxLifecycleState::from(state.lifecycle())), + Some(CleanupState::Failed) => Some(ObservedStatus::Owned { + teardown: Teardown::Stopping, + cleanup: ObservedCleanup::Failed, + }), + None => Some(ObservedStatus::Unowned( + crate::proto::SandboxLifecycleState::from(state.lifecycle()), + )), }; if let Some(expected) = expected { assert_eq!( @@ -7254,7 +7258,7 @@ pub(crate) mod service_tests { .expect("Wait must complete without requiring an API timeout") .unwrap() .into_inner() - .observed_lifecycle() + .observed_status() .unwrap(), expected, ); @@ -13616,8 +13620,10 @@ pub(crate) mod service_tests { .pop() .expect("quarantined sandbox should be returned"); assert_eq!( - published.observed_lifecycle().unwrap(), - crate::proto::SandboxLifecycleState::Quarantined + published.observed_status().unwrap(), + pplx_space_sbx_mgr_client::lifecycle::ObservedStatus::Unowned( + crate::proto::SandboxLifecycleState::Quarantined + ) ); assert_eq!( published.quarantine_reason_code.as_deref(), @@ -13683,8 +13689,10 @@ pub(crate) mod service_tests { .find(|sandbox| sandbox.sandbox_id == *sandbox_id) .unwrap(); assert_eq!( - published.observed_lifecycle().unwrap(), - crate::proto::SandboxLifecycleState::Quarantined + published.observed_status().unwrap(), + pplx_space_sbx_mgr_client::lifecycle::ObservedStatus::Unowned( + crate::proto::SandboxLifecycleState::Quarantined + ) ); assert_eq!( published.quarantine_reason_code.as_deref(), @@ -13806,7 +13814,7 @@ pub(crate) mod service_tests { .iter() .find(|sandbox| sandbox.sandbox_id == malformed_core_id) .expect("a malformed local row must still report its node lifecycle"); - malformed_core.observed_lifecycle().unwrap_err(); + malformed_core.observed_status().unwrap_err(); assert!(malformed_core.state_degraded); assert_eq!( malformed_core.cleanup.map(|cleanup| cleanup.completion), @@ -13828,8 +13836,10 @@ pub(crate) mod service_tests { .find(|sandbox| sandbox.sandbox_id == quarantined_id) .unwrap(); assert_eq!( - quarantined.observed_lifecycle().unwrap(), - crate::proto::SandboxLifecycleState::Quarantined + quarantined.observed_status().unwrap(), + pplx_space_sbx_mgr_client::lifecycle::ObservedStatus::Unowned( + crate::proto::SandboxLifecycleState::Quarantined + ) ); assert_eq!( quarantined.quarantine_reason_code.as_deref(), @@ -13868,8 +13878,10 @@ pub(crate) mod service_tests { .find(|sandbox| sandbox.sandbox_id == healthy_id) .unwrap(); assert_eq!( - healthy.observed_lifecycle().unwrap(), - crate::proto::SandboxLifecycleState::Created + healthy.observed_status().unwrap(), + pplx_space_sbx_mgr_client::lifecycle::ObservedStatus::Unowned( + crate::proto::SandboxLifecycleState::Created + ) ); assert!(!healthy.state_degraded); } @@ -16286,8 +16298,31 @@ pub(crate) mod service_tests { ); } + // Every store teardown is in `SandboxStatus::all()`: the match stops + // compiling when one is added, so it can't be published unchecked. + for teardown in [ + crate::state::Teardown::Stopping, + crate::state::Teardown::Deleting, + crate::state::Teardown::Suspending, + ] { + match teardown { + crate::state::Teardown::Stopping + | crate::state::Teardown::Deleting + | crate::state::Teardown::Suspending => {} + } + for cleanup in [ + crate::state::CleanupState::Required, + crate::state::CleanupState::Failed, + ] { + assert!( + SandboxStatus::all().contains(&SandboxStatus::owned(teardown, cleanup)), + "{teardown:?} {cleanup:?}" + ); + } + } // Without a recorded completion, only the two cleanup statuses - // publish a cleanup, and each names its own owner. + // publish a cleanup, and each names its own owner. Readers decode + // every status sandbox manager publishes, to the same label. for state in SandboxStatus::all() { let mut sandbox = service.lookup_sandbox(&sandbox_id).unwrap(); sandbox.state = state; @@ -16318,6 +16353,15 @@ pub(crate) mod service_tests { owner.map(|owner| (Completion::Unspecified as i32, owner as i32)), "{state}" ); + let observed = info + .observed_status() + .unwrap_or_else(|error| panic!("{state}: {error}")); + assert_eq!(observed.label(), state.legacy_label(), "{state}"); + assert_eq!( + observed.cleanup().is_some(), + state.is_cleanup_owned(), + "{state}" + ); } } diff --git a/crates/l40_api/template/mgr/server/src/pool_manager.rs b/crates/l40_api/template/mgr/server/src/pool_manager.rs index 8c771f40a8..bc769d93b8 100644 --- a/crates/l40_api/template/mgr/server/src/pool_manager.rs +++ b/crates/l40_api/template/mgr/server/src/pool_manager.rs @@ -235,15 +235,15 @@ async fn pool_tick(db: &Arc>) -> PoolResult>) -> PoolResult>) -> PoolResult>) -> PoolResult String { - if !sandbox.observed_lifecycle().is_ok_and(|state| { + if !sandbox.observed_status().is_ok_and(|status| { matches!( - state, - pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Started - | pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Paused + status.unowned(), + Some( + pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Started + | pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Paused + ) ) }) { return "-".into(); @@ -277,7 +279,7 @@ async fn handle_stats(client: &mut SandboxClient, sandbox_id: SandboxSelector) { println!( "Sandbox: {short_id} ({})", sandbox - .observed_lifecycle() + .observed_status() .map(|state| state.label()) .unwrap_or("unknown") ); diff --git a/crates/l50_app/sbx/mgr/cli/src/dispatch/core/inventory/tests.rs b/crates/l50_app/sbx/mgr/cli/src/dispatch/core/inventory/tests.rs index bb61732f30..e398593efe 100644 --- a/crates/l50_app/sbx/mgr/cli/src/dispatch/core/inventory/tests.rs +++ b/crates/l50_app/sbx/mgr/cli/src/dispatch/core/inventory/tests.rs @@ -69,13 +69,10 @@ fn a_listed_sandbox_state_no_longer_finds_keeps_its_cleanup() { }; let info = listed_sandbox_info(&SandboxListElement { sandbox_id: "sbx".to_owned(), - lifecycle: Some(SandboxLifecycleState::CleanupFailed as i32), + lifecycle: Some(SandboxLifecycleState::Stopping as i32), cleanup: Some(cleanup), ..Default::default() }); - assert_eq!( - info.lifecycle, - Some(SandboxLifecycleState::CleanupFailed as i32) - ); + assert_eq!(info.lifecycle, Some(SandboxLifecycleState::Stopping as i32)); assert_eq!(info.cleanup, Some(cleanup)); } diff --git a/crates/l50_app/sbx/mgr/cli/src/dispatch/diagnostics.rs b/crates/l50_app/sbx/mgr/cli/src/dispatch/diagnostics.rs index b2143e6b38..eaf2ac5943 100644 --- a/crates/l50_app/sbx/mgr/cli/src/dispatch/diagnostics.rs +++ b/crates/l50_app/sbx/mgr/cli/src/dispatch/diagnostics.rs @@ -9,16 +9,18 @@ pub(super) async fn handle( let sandbox = resolve_sandbox_state(client, &sandbox_id).await; let full_id = &sandbox.sandbox_id; let short_id = sandbox_label(full_id); - let lifecycle = sandbox - .observed_lifecycle() + let status = sandbox + .observed_status() .unwrap_or_else(|error| exit_error(error)); - println!("Doctor: {short_id} ({})", lifecycle.label()); + println!("Doctor: {short_id} ({})", status.label()); println!(); if !matches!( - lifecycle, - pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Started - | pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Paused + status.unowned(), + Some( + pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Started + | pplx_space_sbx_mgr_client::proto::SandboxLifecycleState::Paused + ) ) { println!("Sandbox is not running — nothing to diagnose."); return; @@ -171,7 +173,7 @@ pub(super) async fn handle( println!( "Sandbox {} reached state: {}", sandbox_label(&resp.sandbox_id), - resp.observed_lifecycle() + resp.observed_status() .unwrap_or_else(|error| exit_error(error)) .label() ); diff --git a/crates/l50_app/ssh/cli/src/main.rs b/crates/l50_app/ssh/cli/src/main.rs index 49125ce655..e3fcc337b0 100644 --- a/crates/l50_app/ssh/cli/src/main.rs +++ b/crates/l50_app/ssh/cli/src/main.rs @@ -101,7 +101,7 @@ async fn main() { }); let lifecycle = sandbox - .observed_lifecycle() + .observed_status() .unwrap_or_else(|error| pplx_space_common_cli::exit_error(error)); if !sandbox.pid.is_some_and(is_alive) { eprintln!( diff --git a/crates/test_support/node/api/sim/src/worker/tests.rs b/crates/test_support/node/api/sim/src/worker/tests.rs index 45a32cf6ed..acfeef3567 100644 --- a/crates/test_support/node/api/sim/src/worker/tests.rs +++ b/crates/test_support/node/api/sim/src/worker/tests.rs @@ -108,8 +108,10 @@ async fn create_then_state_then_start() { .into_inner(); assert_eq!(state_resp.sandboxes.len(), 1); assert_eq!( - state_resp.sandboxes[0].observed_lifecycle().unwrap(), - sbx::SandboxLifecycleState::Creating + state_resp.sandboxes[0].observed_status().unwrap(), + pplx_space_sbx_mgr_client::lifecycle::ObservedStatus::Unowned( + sbx::SandboxLifecycleState::Creating + ) ); assert_eq!(state_resp.missing_ids, vec!["missing".to_string()]); @@ -176,8 +178,10 @@ async fn stop_and_kill_reach_stopped() { assert_eq!(state_resp.sandboxes.len(), 2); for sandbox in state_resp.sandboxes { assert_eq!( - sandbox.observed_lifecycle().unwrap(), - sbx::SandboxLifecycleState::Stopped + sandbox.observed_status().unwrap(), + pplx_space_sbx_mgr_client::lifecycle::ObservedStatus::Unowned( + sbx::SandboxLifecycleState::Stopped + ) ); } } diff --git a/db/sandbox-manager/20261109120000_drop_sandbox_legacy_state.down.sql b/db/sandbox-manager/20261109120000_drop_sandbox_legacy_state.down.sql new file mode 100644 index 0000000000..c03f228da4 --- /dev/null +++ b/db/sandbox-manager/20261109120000_drop_sandbox_legacy_state.down.sql @@ -0,0 +1,5 @@ +-- Restore the combined status a release that reads it needs. +ALTER TABLE sandboxes ADD COLUMN legacy_state TEXT + GENERATED ALWAYS AS ( + CASE WHEN cleanup_state IS NOT NULL THEN 'cleanup_' || cleanup_state ELSE state END + ) VIRTUAL; diff --git a/db/sandbox-manager/20261109120000_drop_sandbox_legacy_state.up.sql b/db/sandbox-manager/20261109120000_drop_sandbox_legacy_state.up.sql new file mode 100644 index 0000000000..b8c88988e1 --- /dev/null +++ b/db/sandbox-manager/20261109120000_drop_sandbox_legacy_state.up.sql @@ -0,0 +1,4 @@ +-- Nothing has read the combined status since the release before this one, +-- which every node now runs, so the column the cleanup-state split kept for +-- older readers goes. +ALTER TABLE sandboxes DROP COLUMN legacy_state; diff --git a/docs/configuration.md b/docs/configuration.md index 4f070f45f0..f9c561174f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -141,17 +141,12 @@ manager publishes it as `cleanup.state` (`REQUIRED` or `FAILED`) on `State`, `Wait`, `List` and `ListBuildReferencingSandboxes`, beside the lifecycle the cleanup is over (`STOPPING`, `DELETING` or `SUSPENDING`). `Wait` answers only once the wait completes, which a cleanup does only when it has failed, so it -reports `FAILED` and never `REQUIRED`. - -Earlier sandbox managers fold the cleanup into the lifecycle as -`CLEANUP_REQUIRED` or `CLEANUP_FAILED`, and a reader that predates -`cleanup.state` takes a `STOPPING` row with a failed cleanup for a live stop. -Upgrade every reader of these RPCs, including the shim, to a release that reads -`cleanup.state`, and let shim processes started before it exit, before -upgrading sandbox manager past the fold. Those readers accept both shapes, so -rolling sandbox manager back is safe for them whichever one the restored -release publishes: the folded lifecycle by default, or the native one where -its `publish_native_lifecycle` was on. +reports `FAILED` and never `REQUIRED`. The retired `CLEANUP_REQUIRED` and +`CLEANUP_FAILED` lifecycle values are reserved, and readers, the shim included, +treat them as unknown, so don't roll sandbox manager back to a release that can +publish them. Rolling sandbox manager back past this release also needs this +image's down migrations run first: a release refuses a database whose applied +migrations it doesn't bundle. ## API, admission and limits diff --git a/docs/observability.md b/docs/observability.md index c53f7b5ecb..436afebc2c 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -1239,17 +1239,36 @@ recorded. ## Worker-local lifecycle populations -Sandbox-manager emits `sandbox.worker.lifecycle.count` every 60 seconds from -its durable SQLite rows, grouped by `state`, `runtime`, and `runtime_version`. -Runtime identity comes from the sandbox's recorded execution metadata, not the -node's default, installed-binary inventory, or a mutable template tag. Runtime -aliases are normalized to full names such as `firecracker` and -`cloud-hypervisor`. Legacy rows without a valid recorded identity keep their -known runtime kind and use `runtime_version:unknown`; an unrecognized kind is -also `unknown`, never an assumed legacy version. +Sandbox-manager emits `sandbox.worker.lifecycle.status_count` every 60 seconds +from its durable SQLite rows, grouped by `state`, `cleanup_state`, `runtime`, +and `runtime_version`. `state` is always a lifecycle state. `cleanup_state` is +`required` or `failed` for rows a node-local cleanup owns, whose `state` is +`stopping`, `deleting` or `suspending`, and `none` otherwise. It replaces +`sandbox.worker.lifecycle.count`, which folded those rows into +`state:cleanup_required` and `state:cleanup_failed`. Renaming the metric alone +doesn't translate a query: `status_count{state:stopping}` also counts stops a +cleanup owns. `lifecycle.count{state:stopping}` becomes +`status_count{state:stopping,cleanup_state:none}`, and +`lifecycle.count{state:cleanup_failed}` becomes +`status_count{cleanup_state:failed}`. Runtime identity comes from the sandbox's +recorded execution metadata, not the node's default, installed-binary inventory, +or a mutable template tag. Runtime aliases are normalized to full names such as +`firecracker` and `cloud-hypervisor`. Legacy rows without a valid recorded +identity keep their known runtime kind and use `runtime_version:unknown`; an +unrecognized kind is also `unknown`, never an assumed legacy version. With the default metric namespace, the running-state population by version is: +```text +sum:space.sandbox.worker.lifecycle.status_count{state:started} by {runtime,runtime_version} +``` + +A worker on a release that still emits `sandbox.worker.lifecycle.count` emits +`status_count` with only `state` and `cleanup_state`, so its `runtime` and +`runtime_version` breakdown is only on the old metric. During a rollout to the +release that retired it, keep the old per-version query beside the new one +until every worker has upgraded, then remove it: + ```text sum:space.sandbox.worker.lifecycle.count{state:started} by {runtime,runtime_version} ``` @@ -1257,16 +1276,17 @@ sum:space.sandbox.worker.lifecycle.count{state:started} by {runtime,runtime_vers Placement identity comes from telemetry context. No sandbox, template, or execution identifier is a metric dimension. Summing the version buckets preserves the existing per-state total. The sampler emits zero once when a -previously reported state/runtime/version bucket disappears, including when -only the version changes. Database read failures skip the sample rather than +previously reported state/cleanup/runtime/version bucket disappears, including +when only the version changes. Database read failures skip the sample rather than reporting a false zero. These are locally recorded lifecycle populations, not an independent process-liveness census. `started`, `paused`, and `suspended` are distinct states; a suspended row describes retained state, not a resident VM. A missing or stale sample does not establish that a node has zero sandboxes. -Metric backends with explicit tag-retention allowlists must retain `runtime` -and `runtime_version` alongside `state` to support the version breakdown. +Metric backends with explicit tag-retention allowlists must retain +`cleanup_state`, `runtime` and `runtime_version` alongside `state` to support +the cleanup and version breakdowns. ## Sandboxes in a transient lifecycle diff --git a/proto/sandbox/manager/v1/manager.proto b/proto/sandbox/manager/v1/manager.proto index 5d053fb004..9f50aa15f6 100644 --- a/proto/sandbox/manager/v1/manager.proto +++ b/proto/sandbox/manager/v1/manager.proto @@ -254,7 +254,7 @@ service SandboxManager { // Internal: find a paused pool sandbox, reassign to user, resume it. rpc AdoptPoolSandbox(AdoptPoolSandboxRequest) returns (AdoptPoolSandboxResponse); - // Wait for stopped, failed, cleanup_failed, suspended, or quarantined, or time out. + // Wait for stopped, failed, suspended, quarantined, or a failed cleanup, or time out. // Quarantine completes the wait without implying that runtime or files were removed. rpc Wait(WaitRequest) returns (WaitResponse); @@ -620,11 +620,10 @@ enum SandboxLifecycleState { SANDBOX_LIFECYCLE_STATE_SUSPENDING = 12; SANDBOX_LIFECYCLE_STATE_SUSPENDED = 13; SANDBOX_LIFECYCLE_STATE_RESTORING = 14; - // Published only by an older node, for a row a cleanup owns, with or without - // `SandboxCleanup.state`. A current node publishes the lifecycle the cleanup - // is over and names the cleanup in `SandboxCleanup.state`. - SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED = 15; - SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED = 16; + // Retired CLEANUP_REQUIRED and CLEANUP_FAILED. A row a cleanup owns is + // published with the lifecycle the cleanup is over and `SandboxCleanup.state`. + reserved 15, 16; + reserved "SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED", "SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED"; SANDBOX_LIFECYCLE_STATE_QUARANTINED = 17; SANDBOX_LIFECYCLE_STATE_FAILED = 18; } @@ -732,8 +731,7 @@ message SandboxInfo { // as does a stop, suspend or delete admitted with a persisted intent. A row a // cleanup owns keeps the completion it was admitted with, but can also have // none, for example one that sbx-manager's cleanup recovery will quarantine. -// A node that sets `state` publishes such a row with `state` set and -// `completion` UNSPECIFIED; an older node publishes no message for it. +// Such a row is published with `state` set and `completion` UNSPECIFIED. message SandboxCleanup { // What the cleanup does to the row when it finishes. Every writer of // MARK_STOPPED keeps the rootfs: a stop, whether a user's or sbx-manager's @@ -749,9 +747,7 @@ message SandboxCleanup { // field, not the presence of this message, to tell a cleanup-owned row. A // row can be REQUIRED or FAILED with no completion recorded. A node publishes // such a row with the teardown transition the cleanup is over (STOPPING, - // DELETING or SUSPENDING). An older node may instead publish the - // CLEANUP_REQUIRED or CLEANUP_FAILED lifecycle, with or without this field - // set: honor those too. + // DELETING or SUSPENDING). SandboxCleanupState state = 2; } @@ -780,11 +776,9 @@ enum SandboxCleanupCompletion { enum SandboxCleanupState { // No cleanup owns the row. SANDBOX_CLEANUP_STATE_UNSPECIFIED = 0; - // A forced teardown owns the row and is owed. An older node may also publish - // it as the CLEANUP_REQUIRED lifecycle. + // A forced teardown owns the row and is owed. SANDBOX_CLEANUP_STATE_REQUIRED = 1; - // A teardown owns the row and its last attempt failed; it is retried. An - // older node may also publish it as the CLEANUP_FAILED lifecycle. + // A teardown owns the row and its last attempt failed; it is retried. SANDBOX_CLEANUP_STATE_FAILED = 2; } diff --git a/shim/lifecycle_test.go b/shim/lifecycle_test.go index ae0e46ecc1..92b05ac369 100644 --- a/shim/lifecycle_test.go +++ b/shim/lifecycle_test.go @@ -8,13 +8,17 @@ import ( func TestObservedSandboxLifecycleRequiresKnownPresentEnum(t *testing.T) { for raw := int32(1); raw <= 18; raw++ { + if raw == 15 || raw == 16 { + // Retired CLEANUP_REQUIRED and CLEANUP_FAILED. + continue + } expected := pb.SandboxLifecycleState(raw) got, err := observedSandboxLifecycle(&pb.SandboxInfo{Lifecycle: &expected}) if err != nil || got != expected { t.Fatalf("%s: got %v, %v", expected, got, err) } } - for _, raw := range []int32{0, -1, 19, 2147483647} { + for _, raw := range []int32{0, -1, 15, 16, 19, 2147483647} { value := pb.SandboxLifecycleState(raw) if _, err := observedSandboxLifecycle(&pb.SandboxInfo{Lifecycle: &value}); err == nil { t.Fatalf("accepted unknown value %d", raw) diff --git a/shim/proto/sandbox/manager/v1/manager.pb.go b/shim/proto/sandbox/manager/v1/manager.pb.go index cf0b534b13..7405e8420d 100644 --- a/shim/proto/sandbox/manager/v1/manager.pb.go +++ b/shim/proto/sandbox/manager/v1/manager.pb.go @@ -529,13 +529,8 @@ const ( SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_SUSPENDING SandboxLifecycleState = 12 SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_SUSPENDED SandboxLifecycleState = 13 SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_RESTORING SandboxLifecycleState = 14 - // Published only by an older node, for a row a cleanup owns, with or without - // `SandboxCleanup.state`. A current node publishes the lifecycle the cleanup - // is over and names the cleanup in `SandboxCleanup.state`. - SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED SandboxLifecycleState = 15 - SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED SandboxLifecycleState = 16 - SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_QUARANTINED SandboxLifecycleState = 17 - SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_FAILED SandboxLifecycleState = 18 + SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_QUARANTINED SandboxLifecycleState = 17 + SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_FAILED SandboxLifecycleState = 18 ) // Enum value maps for SandboxLifecycleState. @@ -556,31 +551,27 @@ var ( 12: "SANDBOX_LIFECYCLE_STATE_SUSPENDING", 13: "SANDBOX_LIFECYCLE_STATE_SUSPENDED", 14: "SANDBOX_LIFECYCLE_STATE_RESTORING", - 15: "SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED", - 16: "SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED", 17: "SANDBOX_LIFECYCLE_STATE_QUARANTINED", 18: "SANDBOX_LIFECYCLE_STATE_FAILED", } SandboxLifecycleState_value = map[string]int32{ - "SANDBOX_LIFECYCLE_STATE_UNSPECIFIED": 0, - "SANDBOX_LIFECYCLE_STATE_CREATING": 1, - "SANDBOX_LIFECYCLE_STATE_CREATED": 2, - "SANDBOX_LIFECYCLE_STATE_STARTING": 3, - "SANDBOX_LIFECYCLE_STATE_STARTED": 4, - "SANDBOX_LIFECYCLE_STATE_STOPPING": 5, - "SANDBOX_LIFECYCLE_STATE_STOPPED": 6, - "SANDBOX_LIFECYCLE_STATE_DELETING": 7, - "SANDBOX_LIFECYCLE_STATE_PAUSING": 8, - "SANDBOX_LIFECYCLE_STATE_PAUSED": 9, - "SANDBOX_LIFECYCLE_STATE_RESUMING": 10, - "SANDBOX_LIFECYCLE_STATE_CHECKPOINTING": 11, - "SANDBOX_LIFECYCLE_STATE_SUSPENDING": 12, - "SANDBOX_LIFECYCLE_STATE_SUSPENDED": 13, - "SANDBOX_LIFECYCLE_STATE_RESTORING": 14, - "SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED": 15, - "SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED": 16, - "SANDBOX_LIFECYCLE_STATE_QUARANTINED": 17, - "SANDBOX_LIFECYCLE_STATE_FAILED": 18, + "SANDBOX_LIFECYCLE_STATE_UNSPECIFIED": 0, + "SANDBOX_LIFECYCLE_STATE_CREATING": 1, + "SANDBOX_LIFECYCLE_STATE_CREATED": 2, + "SANDBOX_LIFECYCLE_STATE_STARTING": 3, + "SANDBOX_LIFECYCLE_STATE_STARTED": 4, + "SANDBOX_LIFECYCLE_STATE_STOPPING": 5, + "SANDBOX_LIFECYCLE_STATE_STOPPED": 6, + "SANDBOX_LIFECYCLE_STATE_DELETING": 7, + "SANDBOX_LIFECYCLE_STATE_PAUSING": 8, + "SANDBOX_LIFECYCLE_STATE_PAUSED": 9, + "SANDBOX_LIFECYCLE_STATE_RESUMING": 10, + "SANDBOX_LIFECYCLE_STATE_CHECKPOINTING": 11, + "SANDBOX_LIFECYCLE_STATE_SUSPENDING": 12, + "SANDBOX_LIFECYCLE_STATE_SUSPENDED": 13, + "SANDBOX_LIFECYCLE_STATE_RESTORING": 14, + "SANDBOX_LIFECYCLE_STATE_QUARANTINED": 17, + "SANDBOX_LIFECYCLE_STATE_FAILED": 18, } ) @@ -689,11 +680,9 @@ type SandboxCleanupState int32 const ( // No cleanup owns the row. SandboxCleanupState_SANDBOX_CLEANUP_STATE_UNSPECIFIED SandboxCleanupState = 0 - // A forced teardown owns the row and is owed. An older node may also publish - // it as the CLEANUP_REQUIRED lifecycle. + // A forced teardown owns the row and is owed. SandboxCleanupState_SANDBOX_CLEANUP_STATE_REQUIRED SandboxCleanupState = 1 - // A teardown owns the row and its last attempt failed; it is retried. An - // older node may also publish it as the CLEANUP_FAILED lifecycle. + // A teardown owns the row and its last attempt failed; it is retried. SandboxCleanupState_SANDBOX_CLEANUP_STATE_FAILED SandboxCleanupState = 2 ) @@ -3920,8 +3909,7 @@ func (x *SandboxInfo) GetCleanup() *SandboxCleanup { // as does a stop, suspend or delete admitted with a persisted intent. A row a // cleanup owns keeps the completion it was admitted with, but can also have // none, for example one that sbx-manager's cleanup recovery will quarantine. -// A node that sets `state` publishes such a row with `state` set and -// `completion` UNSPECIFIED; an older node publishes no message for it. +// Such a row is published with `state` set and `completion` UNSPECIFIED. type SandboxCleanup struct { state protoimpl.MessageState `protogen:"open.v1"` // What the cleanup does to the row when it finishes. Every writer of @@ -3938,9 +3926,7 @@ type SandboxCleanup struct { // field, not the presence of this message, to tell a cleanup-owned row. A // row can be REQUIRED or FAILED with no completion recorded. A node publishes // such a row with the teardown transition the cleanup is over (STOPPING, - // DELETING or SUSPENDING). An older node may instead publish the - // CLEANUP_REQUIRED or CLEANUP_FAILED lifecycle, with or without this field - // set: honor those too. + // DELETING or SUSPENDING). State SandboxCleanupState `protobuf:"varint,2,opt,name=state,proto3,enum=space.sandbox.manager.v1.SandboxCleanupState" json:"state,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -14943,7 +14929,7 @@ const file_sandbox_manager_v1_manager_proto_rawDesc = "" + "\x16WAKE_TRANSITION_RESUME\x10\x02\x12\x1b\n" + "\x17WAKE_TRANSITION_RESTORE\x10\x03\x12\x19\n" + "\x15WAKE_TRANSITION_START\x10\x04\x12\x18\n" + - "\x14WAKE_TRANSITION_WAIT\x10\x05*\xfe\x05\n" + + "\x14WAKE_TRANSITION_WAIT\x10\x05*\x82\x06\n" + "\x15SandboxLifecycleState\x12'\n" + "#SANDBOX_LIFECYCLE_STATE_UNSPECIFIED\x10\x00\x12$\n" + " SANDBOX_LIFECYCLE_STATE_CREATING\x10\x01\x12#\n" + @@ -14960,11 +14946,9 @@ const file_sandbox_manager_v1_manager_proto_rawDesc = "" + "%SANDBOX_LIFECYCLE_STATE_CHECKPOINTING\x10\v\x12&\n" + "\"SANDBOX_LIFECYCLE_STATE_SUSPENDING\x10\f\x12%\n" + "!SANDBOX_LIFECYCLE_STATE_SUSPENDED\x10\r\x12%\n" + - "!SANDBOX_LIFECYCLE_STATE_RESTORING\x10\x0e\x12,\n" + - "(SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED\x10\x0f\x12*\n" + - "&SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED\x10\x10\x12'\n" + + "!SANDBOX_LIFECYCLE_STATE_RESTORING\x10\x0e\x12'\n" + "#SANDBOX_LIFECYCLE_STATE_QUARANTINED\x10\x11\x12\"\n" + - "\x1eSANDBOX_LIFECYCLE_STATE_FAILED\x10\x12*\xd7\x02\n" + + "\x1eSANDBOX_LIFECYCLE_STATE_FAILED\x10\x12\"\x04\b\x0f\x10\x0f\"\x04\b\x10\x10\x10*(SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED*&SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED*\xd7\x02\n" + "\x18SandboxCleanupCompletion\x12*\n" + "&SANDBOX_CLEANUP_COMPLETION_UNSPECIFIED\x10\x00\x12+\n" + "'SANDBOX_CLEANUP_COMPLETION_MARK_CREATED\x10\x01\x12+\n" + diff --git a/shim/proto/sandbox/manager/v1/manager_grpc.pb.go b/shim/proto/sandbox/manager/v1/manager_grpc.pb.go index 2226e0df97..574b37c0b6 100644 --- a/shim/proto/sandbox/manager/v1/manager_grpc.pb.go +++ b/shim/proto/sandbox/manager/v1/manager_grpc.pb.go @@ -303,7 +303,7 @@ type SandboxManagerClient interface { ServicePortIngress(ctx context.Context, opts ...grpc.CallOption) (grpc.BidiStreamingClient[v11.ServicePortIngressFrame, v11.ServicePortIngressFrame], error) // Internal: find a paused pool sandbox, reassign to user, resume it. AdoptPoolSandbox(ctx context.Context, in *AdoptPoolSandboxRequest, opts ...grpc.CallOption) (*AdoptPoolSandboxResponse, error) - // Wait for stopped, failed, cleanup_failed, suspended, or quarantined, or time out. + // Wait for stopped, failed, suspended, quarantined, or a failed cleanup, or time out. // Quarantine completes the wait without implying that runtime or files were removed. Wait(ctx context.Context, in *WaitRequest, opts ...grpc.CallOption) (*WaitResponse, error) // Clone a paused donor sandbox to create a new sandbox (for pool cloning). @@ -1421,7 +1421,7 @@ type SandboxManagerServer interface { ServicePortIngress(grpc.BidiStreamingServer[v11.ServicePortIngressFrame, v11.ServicePortIngressFrame]) error // Internal: find a paused pool sandbox, reassign to user, resume it. AdoptPoolSandbox(context.Context, *AdoptPoolSandboxRequest) (*AdoptPoolSandboxResponse, error) - // Wait for stopped, failed, cleanup_failed, suspended, or quarantined, or time out. + // Wait for stopped, failed, suspended, quarantined, or a failed cleanup, or time out. // Quarantine completes the wait without implying that runtime or files were removed. Wait(context.Context, *WaitRequest) (*WaitResponse, error) // Clone a paused donor sandbox to create a new sandbox (for pool cloning). diff --git a/shim/space.go b/shim/space.go index f0aa6c6a27..040f23dab8 100644 --- a/shim/space.go +++ b/shim/space.go @@ -1081,8 +1081,8 @@ func releaseLegacyTaskTeardown(ctx context.Context, client pb.SandboxManagerClie case owned: // The recorded completion says what the cleanup returns the row to. // MARK_SUSPENDED releases the task, and MARK_STOPPED does unless this - // task's own Kill issued the stop. Every other completion, and a node - // too old to report one, still falls through to the destructive call. + // task's own Kill issued the stop. Any other completion, or none, + // still falls through to the destructive call. // Only cleanup-owned rows read it: on other rows a completion can be // an armed activation's rollback target, published while the start // runs. @@ -1137,22 +1137,26 @@ func releaseLegacyTaskTeardown(ctx context.Context, client pb.SandboxManagerClie return false, nil } -// cleanupOwned reports whether a node-local cleanup owns the row: the -// lifecycle says so, as older nodes publish it, or the cleanup state does, -// whatever teardown transition the lifecycle names. A cleanup state this -// build doesn't know can't authorize teardown, as an unknown lifecycle can't -// (observedSandboxLifecycle), so it is an error. +// cleanupOwned reports whether a node-local cleanup owns the row, whatever +// teardown transition its lifecycle names. A cleanup state this build doesn't +// know, or a cleanup over a lifecycle no cleanup can be over, can't authorize +// teardown, as an unknown lifecycle can't (observedSandboxLifecycle), so each +// is an error. func cleanupOwned(sandbox *pb.SandboxInfo, lifecycle pb.SandboxLifecycleState) (bool, error) { state := sandbox.GetCleanup().GetState() if _, known := pb.SandboxCleanupState_name[int32(state)]; !known { return false, status.Errorf(codes.FailedPrecondition, "unknown sandbox cleanup state value %d", state) } + if state == pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_UNSPECIFIED { + return false, nil + } switch lifecycle { - case pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED, - pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED: + case pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPING, + pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_DELETING, + pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_SUSPENDING: return true, nil } - return state != pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_UNSPECIFIED, nil + return false, status.Errorf(codes.FailedPrecondition, "sandbox cleanup %s over lifecycle %s, which no cleanup can be over", state, lifecycle) } func firstSandboxState(resp *pb.StateResponse) *pb.SandboxInfo { @@ -1193,9 +1197,8 @@ func stoppedRestartCheckpoint(ctx context.Context, client pb.SandboxManagerClien return nil, err } if owned { - // A cleanup owns the row whatever transition it reports, so it - // is refused as CLEANUP_REQUIRED and CLEANUP_FAILED always were. - return nil, status.Errorf(codes.FailedPrecondition, "stopped restart cannot use local lifecycle %s", lifecycle) + // A cleanup owns the row whatever transition it reports. + return nil, status.Errorf(codes.FailedPrecondition, "stopped restart cannot use local lifecycle %s under cleanup %s", lifecycle, sandbox.GetCleanup().GetState()) } switch lifecycle { case pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPED: diff --git a/shim/space_test.go b/shim/space_test.go index c8e4d781a7..921c63cf82 100644 --- a/shim/space_test.go +++ b/shim/space_test.go @@ -2324,9 +2324,9 @@ func TestLegacyPodTeardownPreservesUserStoppedRootfs(t *testing.T) { // sbx-manager finishes the user's stop. {"stopping", sandboxStateResult{resp: &pb.StateResponse{Sandboxes: []*pb.SandboxInfo{{Lifecycle: pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPING.Enum()}}}}}, // A stop or suspend whose cleanup failed keeps its completion. - {"stop cleanup required", cleanupState(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED, pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_STOPPED.Enum())}, - {"stop cleanup failed", cleanupState(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED, pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_STOPPED.Enum())}, - {"suspend cleanup failed", cleanupState(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED, pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_SUSPENDED.Enum())}, + {"stop cleanup required", ownedCleanup(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPING, pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_REQUIRED, pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_STOPPED)}, + {"stop cleanup failed", ownedCleanup(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPING, pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_FAILED, pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_STOPPED)}, + {"suspend cleanup failed", ownedCleanup(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_SUSPENDING, pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_FAILED, pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_SUSPENDED)}, {"missing", sandboxStateResult{err: status.Error(codes.NotFound, "sandbox absent")}}, {"empty", sandboxStateResult{resp: &pb.StateResponse{}}}, } { @@ -2361,7 +2361,14 @@ func cleanupState(lifecycle pb.SandboxLifecycleState, completion *pb.SandboxClea return sandboxStateResult{resp: &pb.StateResponse{Sandboxes: []*pb.SandboxInfo{sandbox}}} } -// Outside CLEANUP_*, a published completion can be an armed activation's +// ownedCleanup is a row the cleanup in state owns, published under the +// teardown transition it is over. +func ownedCleanup(lifecycle pb.SandboxLifecycleState, state pb.SandboxCleanupState, completion pb.SandboxCleanupCompletion) sandboxStateResult { + sandbox := &pb.SandboxInfo{Lifecycle: lifecycle.Enum(), Cleanup: &pb.SandboxCleanup{Completion: completion, State: state}} + return sandboxStateResult{resp: &pb.StateResponse{Sandboxes: []*pb.SandboxInfo{sandbox}}} +} + +// Without a cleanup state, a published completion can be an armed activation's // rollback target while the start runs. These are the shapes sbx-manager // publishes today, each with its lifecycle's answer spelled out. func TestLegacyTeardownKeepsTheLifecycleAnswerForArmedActivations(t *testing.T) { @@ -2406,8 +2413,9 @@ func TestLegacyTeardownKeepsTheLifecycleAnswerForArmedActivations(t *testing.T) } } -// Every lifecycle outside CLEANUP_* gets the same answer with any completion -// as with none, so a lifecycle or completion added later is covered too. +// Without a cleanup state, every lifecycle gets the same answer with any +// completion as with none, so a lifecycle or completion added later is covered +// too. func TestLegacyTeardownIgnoresACompletionOutsideFailedCleanup(t *testing.T) { type outcome struct { detached bool @@ -2422,10 +2430,6 @@ func TestLegacyTeardownIgnoresACompletionOutsideFailedCleanup(t *testing.T) { } for lifecycleValue := range pb.SandboxLifecycleState_name { lifecycle := pb.SandboxLifecycleState(lifecycleValue) - if lifecycle == pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED || - lifecycle == pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED { - continue - } for completionValue := range pb.SandboxCleanupCompletion_name { completion := pb.SandboxCleanupCompletion(completionValue) for _, operation := range []string{"kill", "delete"} { @@ -2442,9 +2446,8 @@ func TestLegacyTeardownIgnoresACompletionOutsideFailedCleanup(t *testing.T) { } } -// A node may report a cleanup-owned row under the teardown transition it is -// in, with its cleanup state, instead of under CLEANUP_REQUIRED or -// CLEANUP_FAILED. Whichever it reports, the teardown answer is the same. +// A cleanup-owned row gets the same teardown answer whichever teardown +// transition it is published under. func TestLegacyTeardownDecidesAnOwnedCleanupByItsCleanupStateNotItsLifecycle(t *testing.T) { type outcome struct { detached bool @@ -2460,24 +2463,19 @@ func TestLegacyTeardownDecidesAnOwnedCleanupByItsCleanupStateNotItsLifecycle(t * detached, err := releaseLegacyTaskTeardown(context.Background(), client, operation, identity, nil) return outcome{detached, status.Code(err), client.stopCalls, client.deleteCalls} } - owners := map[pb.SandboxCleanupState]pb.SandboxLifecycleState{ - pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_REQUIRED: pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED, - pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_FAILED: pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED, - } - for owner, combined := range owners { + for _, owner := range []pb.SandboxCleanupState{pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_REQUIRED, pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_FAILED} { for completionValue := range pb.SandboxCleanupCompletion_name { cleanup := &pb.SandboxCleanup{Completion: pb.SandboxCleanupCompletion(completionValue), State: owner} for _, operation := range []string{"kill", "delete"} { for _, stopIssued := range []bool{false, true} { - want := run(combined, cleanup, operation, stopIssued) + want := run(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPING, cleanup, operation, stopIssued) for _, lifecycle := range []pb.SandboxLifecycleState{ - pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPING, pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_DELETING, pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_SUSPENDING, } { if got := run(lifecycle, cleanup, operation, stopIssued); got != want { - t.Errorf("%s with %s %s, %s, stop_issued=%t: got %+v, want the %s answer %+v", - lifecycle, owner, cleanup.Completion, operation, stopIssued, got, combined, want) + t.Errorf("%s with %s %s, %s, stop_issued=%t: got %+v, want the STOPPING answer %+v", + lifecycle, owner, cleanup.Completion, operation, stopIssued, got, want) } } } @@ -2495,11 +2493,28 @@ func TestLegacyTeardownDecidesAnOwnedCleanupByItsCleanupStateNotItsLifecycle(t * t.Errorf("handed-off suspend after own stop: got %+v, want a release", got) } + // A cleanup over a lifecycle no cleanup can be over authorizes nothing. + for _, lifecycle := range []pb.SandboxLifecycleState{ + pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STARTED, + pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPED, + pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_SUSPENDED, + } { + misplaced := &pb.SandboxCleanup{ + Completion: pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_SUSPENDED, + State: pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_FAILED, + } + for _, operation := range []string{"kill", "delete"} { + if got := run(lifecycle, misplaced, operation, false); got != (outcome{code: codes.FailedPrecondition}) { + t.Errorf("%s with a cleanup, %s: got %+v, want FailedPrecondition", lifecycle, operation, got) + } + } + } + // A cleanup state this shim doesn't know authorizes nothing. unknown := &pb.SandboxCleanup{State: pb.SandboxCleanupState(99)} for _, lifecycle := range []pb.SandboxLifecycleState{ pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPING, - pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED, + pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_DELETING, } { for _, operation := range []string{"kill", "delete"} { if got := run(lifecycle, unknown, operation, false); got != (outcome{code: codes.FailedPrecondition}) { @@ -2539,22 +2554,18 @@ func TestStoppedRestartRefusesACleanupOwnedRowWhateverItsLifecycle(t *testing.T) // whose completion this shim doesn't know, keeps the destructive teardown it // had before completions were reported. func TestLegacyTeardownStillTearsDownOtherFailedCleanups(t *testing.T) { - for _, completion := range []*pb.SandboxCleanupCompletion{ - pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_DELETE_ROW.Enum(), - pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_DELETE_POOL_CLONE.Enum(), - pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_CREATED.Enum(), - pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_QUARANTINE.Enum(), - nil, - pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_UNSPECIFIED.Enum(), - pb.SandboxCleanupCompletion(99).Enum(), + for _, completion := range []pb.SandboxCleanupCompletion{ + pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_DELETE_ROW, + pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_DELETE_POOL_CLONE, + pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_CREATED, + pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_QUARANTINE, + pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_UNSPECIFIED, + pb.SandboxCleanupCompletion(99), } { - for _, lifecycle := range []pb.SandboxLifecycleState{ - pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_REQUIRED, - pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED, - } { + for _, owner := range []pb.SandboxCleanupState{pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_REQUIRED, pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_FAILED} { for _, operation := range []string{"kill", "delete"} { - t.Run(fmt.Sprintf("%v/%s/%s", completion, lifecycle, operation), func(t *testing.T) { - client := &fakeSandboxManagerClient{results: []sandboxStateResult{cleanupState(lifecycle, completion)}} + t.Run(fmt.Sprintf("%v/%s/%s", completion, owner, operation), func(t *testing.T) { + client := &fakeSandboxManagerClient{results: []sandboxStateResult{ownedCleanup(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPING, owner, completion)}} identity := taskTeardownIdentity{sandboxID: "sbx", executionGeneration: 1} detached, err := releaseLegacyTaskTeardown(context.Background(), client, operation, identity, nil) if err != nil || detached { @@ -2635,7 +2646,7 @@ func TestLegacyKillThenDeleteStillCallsManagerDelete(t *testing.T) { }{ {"stopped", cleanupState(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPED, nil)}, {"stopping", cleanupState(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPING, nil)}, - {"stop cleanup failed", cleanupState(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_CLEANUP_FAILED, pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_STOPPED.Enum())}, + {"stop cleanup failed", ownedCleanup(pb.SandboxLifecycleState_SANDBOX_LIFECYCLE_STATE_STOPPING, pb.SandboxCleanupState_SANDBOX_CLEANUP_STATE_FAILED, pb.SandboxCleanupCompletion_SANDBOX_CLEANUP_COMPLETION_MARK_STOPPED)}, } { t.Run(tc.name, func(t *testing.T) { podTask := &task{