Repository navigation
Conversation
|
Caution Review failedThe pull request is closed. No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
 |
|
Thanks for opening this pull request! A SPACE maintainer will take a look. CONTRIBUTING.md explains how we review outside contributions. |
e645258 to
5a5a8cd
Compare
A sandbox whose pod was force-deleted, or whose VM exited on its own,
ended up stopped with nothing telling it apart from a requested stop.
Only the node knows the difference, so it now says: the shim's legacy
pod teardown sends its Stop with a POD_TERMINATED cause, and
sbx-manager's runtime-exit recovery admits its stop with VM_EXITED. A
stop that newly owns the row records its attribution in the admission's
transaction; one that re-admits a row another stop owns keeps that
stop's. The node publishes the cause with the stopped status, the
stopped convergence stores it in the columns the previous change added,
and on both databases it lasts until the sandbox reaches started or
paused again. The admin sandbox response and list expose it as
stop_cause {reason, stopped_at} while they render the sandbox stopped;
a requested stop leaves it null.
Writing the cause during the stopped convergence lost it when an API stamp moved the row to stopped first, and could not clear an old cause when a requested stop followed a start that never reached started. The node now attributes every stop it newly owns, a requested one included, with its own time of that stop, and publishes the pair with a stopped status. The ingest applies it, after the convergence, to a row on that node that isn't running, when it is newer than the stored one; a node that publishes none leaves the stored attribution alone. The convergence no longer writes the cause, and the cached admin response decides on the cause after it picks the lifecycle it renders.
5a5a8cd to
12e342c
Compare
The ingest orders attributions by the publishing nodes' wall clocks. Say that this assumes NTP-synced nodes, and which sequence a skewed or stepped-back clock can get wrong: a crash, an evacuation and a start interrupted before started, all within the skew. Say too, on the node side, that the control plane applies an attribution only to a row on the publishing node that isn't started or paused.
Assert the ids the attribution statement writes, so a republished, older or attribution-less report is shown to write nothing, including on a row with no attribution stored. Send an older node's report as a real stopped publication without attribution fields. After the evacuation, publish from the destination node, and check that its newer attribution applies there while the old node's report, even a newer one, no longer does.
|
Closing: we decided not to add schema columns to record a stop's cause. The value doesn't justify a new column, a trigger on |
shehan-pplx
left a comment
shehan-pplx
left a comment
There was a problem hiding this comment.
Reviewed by Shehan's Review Bot
| /// Why a stop no caller requested happened. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum StopCause { | ||
| /// The shim tore the sandbox's Kubernetes pod down while it ran. | ||
| PodTerminated, | ||
| /// The sandbox's VM process exited on its own. | ||
| VmExited, | ||
| } | ||
|
|
||
| impl StopCause { | ||
| pub const fn as_str(self) -> &'static str { | ||
| match self { | ||
| Self::PodTerminated => "pod_terminated", | ||
| Self::VmExited => "vm_exited", | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for StopCause { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.write_str(self.as_str()) | ||
| } | ||
| } | ||
|
|
||
| impl FromStr for StopCause { | ||
| type Err = StopCauseParseError; | ||
|
|
||
| fn from_str(value: &str) -> std::result::Result<Self, Self::Err> { | ||
| match value { | ||
| "pod_terminated" => Ok(Self::PodTerminated), | ||
| "vm_exited" => Ok(Self::VmExited), | ||
| _ => Err(StopCauseParseError), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, Eq, PartialEq, thiserror::Error)] | ||
| #[error("stop cause must be pod_terminated or vm_exited")] | ||
| pub struct StopCauseParseError; |
There was a problem hiding this comment.
[Should fix] StopCause here and SandboxStopCause in sandbox-store/src/lib.rs are the same enum: same variants, same labels, same as_str/Display/FromStr, same parse error text. Adding a cause means editing both, plus the proto enum and three hand-written match maps (stop_cause_from_proto in grpc_core.rs, stop_attribution_to_proto in grpc_state_snapshot.rs, and published_stop_attribution in internal-api). Both stores already depend on pplx-space-common-core, where SandboxLifecycle lives (types/sandbox.rs).
Fix: define SandboxStopCause (with as_str, FromStr and snake_case serde) once in pplx_space_common_core::types::sandbox, and re-export or use it from both stores and the API response. That leaves one label table, and only the proto conversions at the edges.
| /// Whether a stop admitted over `superseded` starts a new stop rather than | ||
| /// re-admitting one: `stopping` and `stopped` rows, and the cleanup states a | ||
| /// stop's failed or interrupted cleanup leaves, already belong to a stop. | ||
| fn newly_owns_stop(superseded: Option<SandboxState>) -> bool { | ||
| !matches!( | ||
| superseded, | ||
| Some( | ||
| SandboxState::Stopping | ||
| | SandboxState::Stopped | ||
| | SandboxState::CleanupRequired | ||
| | SandboxState::CleanupFailed | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
[Consider] This is a second rule for "a stop already owns this row", and it disagrees with the shim's. releaseLegacyTaskTeardown treats CLEANUP_REQUIRED/CLEANUP_FAILED as stop-owned only when the recorded completion is MARK_STOPPED. Here every cleanup row counts as stop-owned, so a Stop over a failed suspend (mark_suspended), create rollback (mark_created) or quarantine (mark_failed) records no attribution. The node then publishes none, and the control plane keeps whatever it last stored. If it never saw the intervening started, that is the previous crash's cause.
Fix: read the superseded row's cleanup_completion next to its state, and count CleanupRequired | CleanupFailed as stop-owned only with Some(CleanupCompletion::MarkStopped). Then add the non-MarkStopped cleanup cases to a_readmitted_stop_keeps_the_first_stops_attribution.
| AND input.published_lifecycle = 'stopped' | ||
| AND input.stop_cause_at IS NOT NULL | ||
| AND LOWER(TRIM(s.lifecycle)) NOT IN ('started', 'paused') | ||
| AND (s.stop_cause_at IS NULL OR input.stop_cause_at > s.stop_cause_at) |
There was a problem hiding this comment.
[Consider] The doc comment above lists the clock-skew limitation, but the only cross-clock comparison is between attributions from different nodes. One node's republications are already handled by >. Because the active_node_id = $1 gate means only the current node writes, an attribution from a node other than the one that wrote the stored value is always the later stop.
Fix: while #1545 is unshipped, add a stop_cause_node_id column there, and accept the write when s.stop_cause_node_id IS DISTINCT FROM $1 OR input.stop_cause_at > s.stop_cause_at. That removes the evacuation and skew case, and its paragraph in the doc.
| stop_attribution: row | ||
| .get::<_, Option<i64>>("stop_cause_at")? | ||
| .map(|stopped_at_ms| super::StopAttribution { | ||
| cause: row | ||
| .get::<_, Option<String>>("stop_cause") | ||
| .ok() | ||
| .flatten() | ||
| .and_then(|value| value.parse().ok()), | ||
| stopped_at_ms, | ||
| }), |
There was a problem hiding this comment.
[Consider] .ok().flatten() swallows any column error on stop_cause, such as a projection that forgot the column or a type mismatch, and reports it as a requested stop. stop_cause_at beside it uses ?. The rollback case only needs the parse().ok().
| stop_attribution: row | |
| .get::<_, Option<i64>>("stop_cause_at")? | |
| .map(|stopped_at_ms| super::StopAttribution { | |
| cause: row | |
| .get::<_, Option<String>>("stop_cause") | |
| .ok() | |
| .flatten() | |
| .and_then(|value| value.parse().ok()), | |
| stopped_at_ms, | |
| }), | |
| stop_attribution: { | |
| let stop_cause: Option<String> = row.get("stop_cause")?; | |
| row.get::<_, Option<i64>>("stop_cause_at")? | |
| .map(|stopped_at_ms| super::StopAttribution { | |
| cause: stop_cause.as_deref().and_then(|value| value.parse().ok()), | |
| stopped_at_ms, | |
| }) | |
| }, |
| #[expect( | ||
| clippy::too_many_arguments, | ||
| reason = "each argument is an independent field of the Stop or Delete request being admitted" | ||
| )] |
There was a problem hiding this comment.
[Consider] A seventh positional argument, which adds a too_many_arguments suppression. Call sites now read (id, MarkStopped, false, true, None, false, None), and two adjacent Options with three bools are easy to swap without any error.
Fix: pass a small SelectedCleanup { completion, immediate_kill, persist_intent, template_filter, require_template_owner, stop_cause } struct, as CleanupAdmissionSpec already does one level down, and drop the #[expect].
| // Set only by a caller stopping the sandbox because its infrastructure went | ||
| // away rather than because anyone asked for a stop: the shim's legacy pod | ||
| // teardown (a first-generation pod without the durable-suspend teardown | ||
| // annotation) sends POD_TERMINATED; a later-generation pod takes the | ||
| // teardown-suspend path, which records no cause. sbx-manager records it only when this Stop newly admits the stop: | ||
| // a Stop that re-admits a row another stop owns keeps that stop's | ||
| // attribution. Absent for every requested stop, and from older callers. |
There was a problem hiding this comment.
[Consider] This wire contract describes one caller's internals: shim pod generations, the durable-suspend annotation and the teardown-suspend path. Those details will go stale while the field's meaning stays the same. Line 524 also runs past the wrap.
| // Set only by a caller stopping the sandbox because its infrastructure went | |
| // away rather than because anyone asked for a stop: the shim's legacy pod | |
| // teardown (a first-generation pod without the durable-suspend teardown | |
| // annotation) sends POD_TERMINATED; a later-generation pod takes the | |
| // teardown-suspend path, which records no cause. sbx-manager records it only when this Stop newly admits the stop: | |
| // a Stop that re-admits a row another stop owns keeps that stop's | |
| // attribution. Absent for every requested stop, and from older callers. | |
| // Why the sandbox is stopping when nobody asked for a stop (for example, | |
| // its pod was torn down). Absent for a requested stop and from older | |
| // callers. sbx-manager records it only when this Stop newly owns the row; | |
| // a Stop that re-admits a row another stop owns keeps that stop's | |
| // attribution. |
Summary
A sandbox whose Kubernetes pod is force-deleted, or whose VM exits on a Direct
node, ends up
stoppedexactly like an operator stop. The admin sandboxresponse and list now carry
stop_cause: {reason, stopped_at}:reasonispod_terminatedorvm_exited, andstopped_atis the node's time of thestop. It is
nullfor a requested stop and for any rendered lifecycle otherthan
stopped.The rule: the node attributes every stop it newly owns (a cause, or none for a
requested stop) with its own time of that stop, and publishes the attribution
with a
stoppedstatus. The publish ingest applies it in a statement of itsown, after the lifecycle convergence, to a row on that node that isn't
running, when it is newer than the stored one. Reaching
startedorpausedclears it.
pod without the durable-suspend annotation, sends its
StopwithPOD_TERMINATED. A-g2or later pod takes the teardown-suspend path,which records no cause.
recovery admits with
VM_EXITED.stop's attribution.
failure_reason, the public/v1sandbox, and the nodeassignment.
Limitation. After the shim's
Stop, the pod'sDeletedeletes the node'srow and its copy of the sandbox. If it lands before node-ctl publishes
stopped, the stop is never seen, as can happen when the stop's cleanup fails.It is the same teardown that leaves a force-deleted sandbox unable to start
again, which is tracked separately.
Validation
Store tests, each failing before its fix: an API-stamped
stoppedbefore the node publishes; a requested stop interrupting a start after a
crash; ordering, republication and older-node reports (asserting that
nothing is written); a running row; another node; an evacuation; the
cached response.
Checks. fmt, clippy
-D warnings, nextest on the touched packages,the shim checks, and
cargo arch(no required findings).Kind. A force-deleted pod gives
pod_terminated, a VM kill givesvm_exited, and an operator stop givesnull. Race 1 was staged by pausingnode-ctl; race 2 couldn't be staged, and its store test covers it.
Details and red runs:
validation.md.Severity and risk
Severity: moderate: a crash looked like a requested stop.
Risk: one more
UPDATEper publication that carries attributions. It writes arow only when the attribution is newer, so a republished one writes nothing.
The cause is written in the stop admission's transaction, and a failed write
is only logged. Unknown labels read as no cause. Attributions are ordered by
node wall clocks, assumed NTP-synced. A crash, an evacuation to a node whose
clock is behind, and a start interrupted there before
started, all withinthe skew, can keep the old crash shown, as can a backward clock step. A start
that reaches
startedclears it.Rollout
Stacked on #1545, the columns, which must apply first.
All wire additions are optional:
StopRequest.stop_cause,SandboxInfo.stop_attribution,PublishedSandboxState.stop_attribution,SandboxStopAttributionandSandboxStopCause. None of them has shipped, soreusing the field numbers is safe. The SQLite migration applies when
sbx-manager starts.
What an old peer does:
startedrow as a requestedstop, so VM-exit recovery never runs: a force-deleted pod reads as a plain
stop, as on
main.Rollback:
db/sandbox-manager/20261105162353_sandbox_stop_cause.up.sql: a binarywithout it refuses a node database that recorded it.
this internal-api is deployed: internal-api writes them inside the
node-publish transaction.
Overlap
The second to land adapts. No field numbers collide.
manager.pb.gois regenerated by [api] trigger a stopped sandbox's runtime upgrade from a test hook, and smoke it on Kind #1322, [sbx-mgr] boot a Kubernetes runtime upgrade in its pod's shim through a runtime-upgrade checkpoint #1380, spaced: advertise process output gap recovery #889, Add live gateway policy observations #1294 and [proto] publish whether a node-local cleanup owns a sandbox row #1554.StopRequestliteral; [api] record the activating client and label sandbox pods with it #1068 and api: search the admin sandbox list by ID, and name owners and pools #1523 addSandboxfields.StopCause.legacy_state: keep thestop-cause columns.
newly_owns_stopand the publication gate read, is unchanged.Stack
#1545 → this → #1521.
AI assistance: Claude Code drafted the code, tests and this description; a
human reviews them before this pull request opens.