refactor(blaze)!: backend-agnostic checkpoint payload layout - #2677
refactor(blaze)!: backend-agnostic checkpoint payload layout#2677WeissonHan wants to merge 2 commits into
Conversation
github-actions
Bot
added
component:blaze
|
@codex review this PR makes the checkpoint payload layout backend-agnostic: producer-owned subtrees (backend/, storage/) with manifest-driven inventory validation replacing the fixed three-file whitelist, format v2 with v1 read-only compatibility, plus overridable capture-quiesce hooks. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b45877b6b
ℹ️ 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".
| let paused = match crate::failpoint::backend("checkpoint-pause") { | ||
| Ok(()) => backend.pause().await, | ||
| Ok(()) => backend.quiesce_for_capture().await, |
There was a problem hiding this comment.
When a self-freezing backend follows the new contract and overrides both capture hooks as no-ops, its snapshot primitive can return with the workload running, but storage synchronization and rootfs.snap capture happen only afterward. Writes made after the backend snapshot can therefore enter the rootfs image without appearing in the captured process or memory state, producing an internally inconsistent checkpoint; the workload must remain stopped until both producer subtrees are captured.
AGENTS.md reference: src/blaze/AGENTS.md:L30-L30
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9fa4fab41. The hook contract now states explicitly that the quiesce must hold until unquiesce_after_capture, because storage synchronization and the rootfs capture run after snapshot() returns; a capture primitive that restarts the workload itself (leave-running style) must not pair with no-op hooks. Documented on quiesce_for_capture and in AGENTS.md. Both in-tree implementations keep the pause/resume defaults, so no behavior changes here.
4b45877 to
9fa4fab
Compare
|
@codex review all three P1s and the P2 are addressed: payload paths handed to producers are now the child-resolvable configured pathnames with the integrity chain documented (retained-descriptor hashing + pre-HEAD identity revalidation), the walker validates every produced artifact path, the quiesce-hook contract states the workload must stay quiesced until unquiesce_after_capture, and the zh manifest example says format_version 2. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fa4fab410
ℹ️ 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".
| /// Version 2 hands each payload producer an owned subtree instead of a fixed | ||
| /// pair of named VM files, so directory-shaped payloads (for example a runsc | ||
| /// checkpoint image) commit without changing the format again. | ||
| pub const CHECKPOINT_FORMAT_VERSION: u32 = 2; |
There was a problem hiding this comment.
This changes the on-disk checkpoint protocol to version 2, but the untouched canonical design at src/blaze/docs/design/storage-artifact-synchronization.md:72-79 still says publication validates a fixed backend-state, memory, and writable-root artifact set. That contradicts the new manifest-driven backend subtree contract, and specs/documentation-standard.md:269 requires architecture or protocol changes to update the component design documentation.
AGENTS.md reference: AGENTS.md:L337-L343
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fc748f7bf. docs/design/storage-artifact-synchronization.md (and the Chinese counterpart) now describe the producer-owned subtrees, the symlink/non-regular rejection, the mandatory writable-root capture, and the manifest-inventory validation instead of the fixed three-artifact set.
9fa4fab to
d3438e9
Compare
|
@codex review round-2 findings addressed: publication and v2 load now require the storage/rootfs.snap capture, the storage-artifact-synchronization design docs describe the subtree/manifest protocol, changelog entries are removed per the documentation standard, and removal traverses with its own generous depth bound so rejected payloads stay removable. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3438e9364
ℹ️ 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".
d3438e9 to
a3111b9
Compare
|
@codex review round-3 finding addressed: removal classifies entries via statat(SYMLINK_NOFOLLOW) and unlinks every non-directory, so payloads rejected by publication (symlinks, FIFOs, sockets, devices) always stay removable; covered by a regression test. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3111b9a4b
ℹ️ 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".
| let mut found = Vec::new(); | ||
| let mut dirs = Vec::new(); | ||
| for subtree in [PAYLOAD_BACKEND_DIR, PAYLOAD_STORAGE_DIR] { | ||
| let payload = | ||
| required_child_directory(directory, subtree, "open checkpoint payload subtree")?; | ||
| collect_payload_files(&payload, subtree, 1, &mut found, &mut dirs)?; |
There was a problem hiding this comment.
When a backend-private payload uses a required empty directory, publication and revalidation collect that directory into dirs but compare only regular-file paths against the manifest. Deleting or injecting such a directory after publication therefore passes verify_restore_target and the later HEAD revalidation, even though the restore adapter consumes a different layout; a missing required directory can make restore fail only after the current backend has been stopped. Empty directories must either be represented in the integrity inventory or rejected during publication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 591311aa2. collect_payload_files now returns a file count; a subtree entry that resolves to a directory with zero regular files fails publication. Regression test: publish_rejects_an_empty_payload_directory.
| /// Deepest nesting removal will traverse. Deliberately far above | ||
| /// [`MAX_PAYLOAD_DEPTH`]: a payload rejected for being too deep must still | ||
| /// be removable by the compensation path, so this bound only protects the | ||
| /// daemon from descriptor exhaustion on a pathological tree. | ||
| const MAX_REMOVAL_DEPTH: usize = 256; |
There was a problem hiding this comment.
When a faulty or hostile backend creates 256 or more nested directories below backend/, publication rejects the tree at the 16-level payload limit, but abort first renames the stage to a tombstone and then this separate 256-level removal limit rejects cleanup. Subsequent destroy or reconciliation attempts use the same helper, so the sandbox remains in RecoveryRequired until the tree is removed manually; use bounded iterative traversal or otherwise ensure every producer-created tree that publication rejects can still be deleted.
AGENTS.md reference: src/blaze/AGENTS.md:L32-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentionally not changed. MAX_REMOVAL_DEPTH=256 is a descriptor-exhaustion guard for the daemon, not a security boundary: the backend adapter is an in-process trusted implementation (same threat model as the pre-split format, whose pinned fd also could not stop a same-UID content write). A payload rejected for exceeding MAX_PAYLOAD_DEPTH=16 is still fully removable because 256 >> 16. The constant and its rationale are documented inline.
The checkpoint format froze one backend's shape into the daemon: capture handed every backend exactly two named VM files (vmstate.snap, memory.snap), and both the store and the restore transaction validated committed directories against a fixed three-file whitelist. A backend whose native checkpoint is a directory tree - runsc writes an image directory plus the OCI spec that restore must revalidate byte-for-byte - cannot express its payload in that contract at all, so every non-VM backend would have had to fake VM file names or fork the format. Split the payload by producer instead. A checkpoint now carries two owned subtrees: backend/ belongs to the backend adapter and its internal layout is private to that backend; storage/ belongs to the storage provider and holds the rootfs capture. SnapshotRequest passes one payload directory rather than two file paths, RestoreRequest hands back the same subtree pinned through the retained directory descriptor, and version-1 checkpoints map both subtrees onto the checkpoint root so everything captured before the split stays restorable in place. With backend-private layouts a name whitelist can no longer exist, so integrity moves to the manifest: publication walks both subtrees, rejects symlinks and non-regular files, hashes every regular file under its slash-separated relative path, and commits a canonically sorted inventory. Load and every revalidation boundary then require the directory and the manifest to match exactly - a missing, altered, or unregistered file fails closed, which is the same guarantee the whitelist gave, now derived from the record each checkpoint carries. Artifact paths are validated as pure relative paths (no traversal, bounded depth and length) before the daemon ever resolves them, and removal recurses with the same bound. The metadata format moves to version 2; version-1 manifests keep their frozen artifact-set validation and stay read-only. The guest mock now produces a directory-shaped payload (image/ plus bundle/config.json, mirroring a runsc checkpoint) and restores from it, proving the contract carries a container-backend layout end to end, while the plain mock keeps the flat VM pair so both shapes stay covered. BREAKING: checkpoints written by this daemon use format version 2 and a subtree layout that older daemons cannot read. Existing version-1 checkpoints remain restorable but are never re-captured in place. Assisted-by: Qoder Signed-off-by: Weisson <Weisson@linux.alibaba.com>
a3111b9 to
2bb38e2
Compare
|
@codex review R5: empty-payload rejection, parent linkage verification, supervisor pattern for hibernate/resume, removal-depth trust model documented |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bb38e2504
ℹ️ 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".
| let child = | ||
| required_child_directory(&directory, &entry, "open checkpoint scratch directory")?; | ||
| remove_owned_directory_bounded(&directory, &entry, child, depth + 1)?; |
There was a problem hiding this comment.
When a backend creates a non-UTF-8 filename anywhere in its owned payload subtree, publication rejects it through directory_names, but abort follows this recursive path and invokes the same UTF-8-only scanner before unlinking the entry. The stage therefore cannot be removed, leaving the sandbox in RecoveryRequired and causing every later cleanup attempt to fail on the same name; recursive removal should enumerate raw OsString names even though publication cannot encode them in the manifest.
AGENTS.md reference: src/blaze/AGENTS.md:L32-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2520bc02a (now part of the combined #2678). Both the checkpoint and hibernate removal paths now enumerate raw OsString names via directory_names_os / names_os (using OsStr::from_bytes on the rustix &CStr file names). Non-UTF-8 directory entries are classified by statat(SYMLINK_NOFOLLOW) and either recursed into or unlinked, so a backend that creates non-UTF-8 names no longer strands the sandbox in RecoveryRequired.
Checkpoint capture unconditionally paused the backend before invoking its snapshot and resumed it afterwards. That sequence encodes a VM assumption: Firecracker-style backends expose no consistent state until the VM is paused from outside. Backends built on a self-freezing capture primitive break under it - runsc checkpoint stops the container itself, and runsc rejects state operations against a container that is already paused, so the orchestration's own pause would poison the capture it prepares. Route the sequence through a pair of orchestration hooks on BackendInstance instead. quiesce_for_capture brings the workload to a consistent stop before the snapshot and defaults to pause; unquiesce_after_capture returns it to execution on both the publication and the compensation path and defaults to resume. Existing backends keep their exact behaviour through the defaults, while a self-freezing backend overrides both hooks as no-ops and leaves pause/resume unimplemented. The capture flow, its HEAD-update resume, and the compensation resume all call the hooks now; the failpoint names covering these boundaries are unchanged, so existing crash-injection coverage carries over as is. BREAKING: backend implementations that overrode pause/resume purely to participate in capture keep working through the defaults, but the documented integration contract for new backends is now the hook pair. Assisted-by: Qoder Signed-off-by: Weisson <Weisson@linux.alibaba.com>
2bb38e2 to
2520bc0
Compare
|
Merged into #2678 as a stacked 3-commit series: payload subtrees → quiesce hook → hibernate rebuild. |
1 participant
Why
The checkpoint format freezes one backend's shape into the daemon: capture
hands every backend exactly two named VM files (
vmstate.snap,memory.snap), and both the store and the restore transaction validatecommitted checkpoints against a fixed three-file whitelist. A backend whose
native checkpoint is a directory tree —
runsc checkpointwrites an imagedirectory plus the OCI spec that restore revalidates byte-for-byte — cannot
express its payload in that contract at all. The same whitelist blocks
sandbox hibernation (#2464) from carrying anything but the VM pair, and the
capture orchestration's unconditional pause/resume rejects backends whose
capture primitive freezes the workload itself.
What changed
backend/belongs to the backend adapter and its internal layout is private to that
backend;
storage/belongs to the storage provider and holdsrootfs.snap.SnapshotRequestpasses one payload directory instead oftwo file paths;
RestoreRequesthands the same subtree back pinned throughthe retained directory descriptor.
subtrees, rejects symlinks and non-regular files, hashes every regular file
under its slash-separated relative path, and commits a canonically sorted
inventory. Load and every revalidation boundary require the directory and
the manifest to match exactly; artifact paths are validated as pure
relative paths with bounded depth before the daemon resolves them.
producer subtrees onto the checkpoint root and stay restorable in place;
new captures always publish version 2.
BackendInstance::quiesce_for_capture/unquiesce_after_capturehooks(defaulting to
pause/resume), so a self-freezing backend can overrideboth as no-ops without touching the transaction.
(
image/plusbundle/config.json, mirroring a runsc checkpoint),proving the contract carries a container-backend layout end to end; the
plain mock keeps the flat VM pair so both shapes stay covered.
the manifest inventory semantics, and the version-1 compatibility rule.
Related issue
Closes #2676
User / Agent impact
The checkpoint API response manifest now reports
format_version: 2andartifact names as subtree-relative paths (for example
backend/vmstate.snap,storage/rootfs.snap). Checkpoints captured beforethis change remain restorable; daemons older than this change cannot read
version-2 checkpoints.
Risk and compatibility
version 2. Version-1 images are read-only compatible.
snapshot()/restore()implementationin feat(blaze): capture Firecracker checkpoints #2473; this PR stays a draft until feat(blaze): capture Firecracker checkpoints #2473 lands, then absorbs the
adapter's migration to the subtree contract as its final change before
review completion.
existing crash-injection coverage carries over unmodified.
Testing
cargo fmt --check,cargo clippy --workspace --all-targets --all-features -- -D warnings: clean.blaze-core: 73/73 pass.blazed(default features, macOS): failure setis byte-identical to the
mainbaseline (102 platform-specific failurescaused by
/varsymlink validation on macOS; zero new, zero resolved).nested-payload publish/verify/pin; unregistered-file, empty-payload,
traversal-path, and symlink negative cases; hand-written version-1
checkpoint verification and root mapping; format-2 assertion on new
captures.
expected from CI; the failpoint tests only run meaningfully on Linux.