feat(orch): persist an fs-quiesced flag on filesystem-only pause, with telemetry

[kalyazin]

feat(orch): persist an fs-quiesced flag on filesystem-only pause, with telemetry

Why

A filesystem-only snapshot (pause(memory:false)) is only safe to cold-boot — and to rewrite offline — if its rootfs was frozen (FIFREEZE) at pause, not merely sync'd. The pause path already knows which happened (guestPrepareFsForPause picks native fsfreeze for envd ≥ 0.6.6, fsfreeze-exec for older envd, or a sync fallback), but that fact is thrown away: it lives only in a transient span/histogram attribute and is never persisted with the snapshot. So nothing downstream can tell, for a given snapshot, whether its rootfs is crash-consistent, and there is no fleet-level signal for how many fs-only snapshots are frozen.

This is the building block for a later offline envd-upgrade feature (separate PR): to upgrade an envd-unaware, paused sandbox by rewriting its rootfs and cold-booting, that feature must restrict itself to snapshots it knows were frozen — otherwise it would have to repair a possibly-torn journal. This PR records and measures the fact; it does not consume it.

What

Persist the flag in the snapshot's own metadata. A new FsQuiesced bool field (json:"fs_quiesced,omitempty") on metadata.Template, with IsFsQuiesced() / MarkFsQuiesced() helpers, sitting right next to filesystem_only in the per-snapshot metadata.json. guestPrepareFsForPause now returns whether a real freeze ran (true for both the native and the exec-API paths; false only for the sync fallback), and Sandbox.Pause stamps m.MarkFsQuiesced(filesystemSnapshot && frozen). It is re-stamped on every pause, so it never goes stale, and is deliberately not carried by the copy-constructors (like filesystem_only).

Telemetry — two signals so the flag is observable both in aggregate and per-pause:

signal	where	what it shows
orchestrator.sandbox.pause.fs_quiesced{quiesced=true|false} counter	Mimir	count of fs-only pauses split by frozen vs sync — the eligible-snapshot population
fs_quiesced bool attribute on the sandbox-snapshot span	Tempo (sampled)	per-pause value, for inspecting individual snapshots

The counter increments only on fs-only pauses, so the headline query is the eligible fraction:

sum(rate(orchestrator_sandbox_pause_fs_quiesced_total{quiesced="true"}[$__rate_interval]))
  / sum(rate(orchestrator_sandbox_pause_fs_quiesced_total[$__rate_interval]))

The counter is registered in telemetry (meters.go) with description + unit map entries, guarded by a meter-map test.

Validation

• Unit (go test, orchestrator + shared): metadata round-trip test asserting fs_quiesced survives serialize→deserialize on a filesystem-only snapshot (no dedicated metadata version needed), that a sync-fallback fs-only snapshot stays not-quiesced, and that a legacy snapshot without the field deserializes as not-quiesced (the safe default); the #3420 fsfreeze-pause test adapted + strengthened to assert the new frozen return (true on a successful native freeze, false on an aborted one); a meter-map guard test. Build + go vet clean, gofmt clean.
• Dev-cluster e2e: deployed this build to a dev orchestrator node, funneled placement to it, created a sandbox (guest envd 0.6.13, native FIFREEZE), and did a real memory:false pause. The metric appeared in dev Mimir through the actual OTEL pipeline: sum by (quiesced)(orchestrator_sandbox_pause_fs_quiesced_total) → {quiesced="true"} = 1. Reverted afterwards.

Key decisions

• No new metadata version. The field is only meaningful on fs-only snapshots, which MarkFilesystemOnly already stamps at >= FilesystemOnlyVersion — a version deserialize() fully unmarshals — so the flag survives without its own version bump. Verified by the round-trip test.
• Backward-compatible by construction. omitempty + the false zero value mean a pre-existing/legacy snapshot (no field) reads as not quiesced. For the future consumer that is the safe "not eligible, wait for a fresh freezing pause" default; no migration.
• Both freeze paths count as quiesced. quiesced=true means "a real FIFREEZE ran," native (≥0.6.6) or exec-API (<0.6.6) alike — both yield an equally crash-consistent rootfs; only sync is false. The native-vs-exec split remains available on the existing guest_sync.duration method label, so it is not duplicated here.
• Signature change: guestPrepareFsForPause now returns (frozen bool, err error); its sole caller (Sandbox.Pause) and the existing fsfreeze test are updated in this PR.
• Deliberately scoped to record + measure. The consumer of the flag — the offline rootfs envd swap, the reboot-time gate, and the version-remap flag — is intentionally not in this PR; it lands separately on top of this building block.

🤖 Generated with Claude Code

[cursor]

PR Summary

Low Risk
Additive metadata and metrics on the existing pause path; no new gating of resume or rewrite behavior in this PR.

Overview
Filesystem-only pauses now record whether the rootfs was actually frozen (FIFREEZE via native envd or exec) versus a plain sync fallback, instead of discarding that after quiesce. guestPrepareFsForPause returns a frozen boolean; successful filesystem-only pauses write fs_quiesced into snapshot metadata (legacy snapshots default to not quiesced), set a fs_quiesced attribute on the pause span, and increment orchestrator.sandbox.pause.fs_quiesced only when a snapshot is successfully minted—intended as groundwork for later features that need crash-consistent fs-only snapshots.

^{Reviewed by Cursor Bugbot for commit 48776be. Bugbot is set up for automated code reviews on this repo. Configure here.}

[codecov]

Codecov Report

❌ Patch coverage is 36.36364% with 21 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
packages/orchestrator/pkg/sandbox/sandbox.go	0.00%	17 Missing ⚠️
packages/orchestrator/pkg/sandbox/reclaim.go	60.00%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2616e8500f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Move the fs-quiesced count after snapshot success

When guestPrepareFsForPause succeeds but a later step in Pause fails (for example s.process.Pause, CreateSnapshot, rootfs diff export, or metadata write), this increments the orchestrator.sandbox.pause.fs_quiesced counter even though no filesystem-only snapshot was actually minted or persisted. Since the new metric is documented as the eligible snapshot population, failures after this point will overstate the quiesced/total fraction; record it only once the snapshot has been created successfully, or add a success dimension.

Useful? React with 👍 / 👎.

[kalyazin]

Fixed. The counter now records on the success path only — moved it into a deferred closure gated on e == nil (and filesystemSnapshot) in Sandbox.Pause, so a pause that aborts after the freeze (process.Pause, snapshot creation, rootfs export, metadata write) no longer increments it. quiesced/total now reflects successfully-minted fs-only snapshots, matching the metric's documented meaning. The fs_quiesced span attribute stays inline since the span already carries the operation's outcome. Folded into feat(orch): persist an fs-quiesced flag on filesystem-only pause.

[claude]

LGTM — this is a well-scoped, additive-only change that persists a new fs_quiesced bool in snapshot metadata and adds matching telemetry, without any consumer of the flag yet.

What was reviewed: the guestPrepareFsForPause signature change and its two freeze paths (native + exec-API) plus the sync fallback; the Pause wiring that stamps fs_quiesced into metadata alongside filesystem_only; the metadata round-trip/version-compat logic (legacy snapshots default to not-quiesced); and the new counter's registration in meters.go. The counter/span recording frozen before process.Pause/CreateSnapshot complete was checked — the metadata write itself only happens after those succeed, so a failed pause doesn't persist a stale flag, only the telemetry counter reflects the freeze attempt rather than snapshot success.

Extended reasoning...

Overview

This PR adds a new FsQuiesced bool field to metadata.Template, changes guestPrepareFsForPause's signature to return whether a real FIFREEZE ran, threads that through Sandbox.Pause to stamp the new metadata field and emit a counter + span attribute, and registers the new counter in packages/shared/pkg/telemetry/meters.go. It touches the orchestrator's pause path (sandbox.go, reclaim.go) and template metadata (template_metadata.go), plus corresponding tests.

Security risks

None identified. No auth, crypto, or permission logic is touched. The change is purely additive metadata/telemetry on an existing internal pause flow; it does not change what data is exposed externally (the field lives in internal snapshot metadata.json, not any public API response).

Level of scrutiny

This warranted a moderate but not high level of scrutiny: it touches the orchestrator's sandbox pause path, which is production-critical, but the change itself is deliberately non-behavioral — nothing consumes the new flag yet, and the modified guestPrepareFsForPause return value is additive (the existing error-handling and control flow are unchanged, only a second return value was added). The metadata version/backward-compat handling reuses the existing FilesystemOnlyVersion upgrade path rather than introducing new versioning logic, which reduces the risk of a subtle migration bug.

Other factors

Test coverage is solid: a dedicated round-trip test covers frozen/sync-fallback/legacy-snapshot cases, the existing fsfreeze pause test was strengthened to assert the new frozen return value, and a meter-map guard test protects against a missing description/unit entry. The author also validated in a dev cluster via the real OTEL pipeline. The bug-hunting system found no bugs, and the one candidate issue it raised (counter incrementing before the pause is fully successful) was examined and is a reasonable telemetry design choice — the metadata write is correctly gated on pause success — not a functional bug.