Skip to content

feat(snapshot): add tmpfs PageBroker daemon - #12958

Closed
dfeigin-nv wants to merge 1 commit into
pagebroker-wirefrom
pagebroker-daemon
Closed

feat(snapshot): add tmpfs PageBroker daemon#12958
dfeigin-nv wants to merge 1 commit into
pagebroker-wirefrom
pagebroker-daemon

Conversation

@dfeigin-nv

@dfeigin-nv dfeigin-nv commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

What

A local PageBroker daemon for the v1 filesystem-storage / POSIX-copy plan.

  • Restore materializes the requested image directory in broker-owned tmpfs.
  • Checkpoint prepare allocates a broker-owned tmpfs output directory.
  • Commit publishes checkpoint output with copy then rename, or removes restore staging.
  • Abort removes broker-owned staging for either transaction.
  • Commit and Abort retain terminal state, so retrying either after a lost response returns the same completion.

The Snapshot Agent sends Commit after CRIU returns.

Scope

No S3, NIXL, GPU path, CRIU provider integration, or direct restore implementation.

Validation

Regenerated the C++ protobuf binding and compiled broker.cpp with -std=c++20 -Wall -Werror. The stacked test PR runs the GoogleTest lifecycle suite.

@dfeigin-nv
dfeigin-nv requested a review from a team as a code owner August 10, 2026 18:33
@github-actions github-actions Bot added the feat label Aug 10, 2026
@dfeigin-nv
dfeigin-nv marked this pull request as draft August 10, 2026 18:33

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 5 potential issues.

View 2 additional findings in Devin Review.

Open in Devin Review

Comment thread deploy/snapshot/pagebroker/daemon.cpp Outdated
Comment thread deploy/snapshot/pagebroker/daemon.cpp Outdated
Comment thread deploy/snapshot/pagebroker/daemon.cpp Outdated
Comment thread deploy/snapshot/pagebroker/daemon.cpp Outdated
Comment thread deploy/snapshot/pagebroker/daemon.cpp Outdated
@datadog-official

datadog-official Bot commented Aug 10, 2026

Copy link
Copy Markdown

Pipelines  Tests

⚠️ Warnings

🚦 7 Pipeline jobs failed

Pre Merge | pre-commit   View in Datadog   GitHub Actions

🔧 Fix in code. This looks caused by changes in this PR. Pre-commit hook found formatting issues in multiple files.

Pre Merge | snapshot   View in Datadog   GitHub Actions

🔧 Fix in code. This looks caused by changes in this PR. 2 tests failed. Error: Lease checkpoint-lease-abc must exist in namespace inference.

PR | trtllm Deploy Test / agg   View in Datadog   GitHub Actions

🔄 Retry job. This looks flaky and may succeed on retry. Helm upgrade failed: Release 'ci-31950891421' does not exist. Error: release already exists.

View all 7 failed jobs.

📋 Copy prompt for your agent
CI on my pull request is failing. Help me find and fix the root cause of each failing job below — they were flagged as caused by changes in this PR, so focus on the diff. For each job, explain the failure and propose a fix.

Branch: pagebroker-daemon

Pre Merge | pre-commit
Commit: 8b32169aac7e4d7e1aeb8542102295ac49aa0b8e
Error (code / quality):
Pre-commit hook found formatting issues in multiple files.
CI job: https://github.com/ai-dynamo/dynamo/actions/runs/31950892048/job/95174071035

Pre Merge | snapshot
Commit: 8b32169aac7e4d7e1aeb8542102295ac49aa0b8e
Error (code / test):
2 tests failed. Error: Lease checkpoint-lease-abc must exist in namespace inference.
CI job: https://github.com/ai-dynamo/dynamo/actions/runs/31950892048/job/95174093464

ℹ️ Info

🔄 Datadog auto-retried 3 jobs - 1 passed on retry View in Datadog

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 51.27% (+7.95%)

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 8b32169 | Docs | Datadog PR Page | Give us feedback!

@dfeigin-nv
dfeigin-nv force-pushed the pagebroker-daemon branch 4 times, most recently from b14c053 to 90d0fe9 Compare August 11, 2026 06:09
@dfeigin-nv dfeigin-nv changed the title feat(snapshot): add PageBroker transaction daemon feat(snapshot): add tmpfs PageBroker daemon Aug 11, 2026
@dfeigin-nv
dfeigin-nv marked this pull request as ready for review August 11, 2026 17:37
@dfeigin-nv
dfeigin-nv force-pushed the pagebroker-daemon branch 2 times, most recently from 07f2f96 to f38502c Compare August 13, 2026 07:35

@hhzhang16 hhzhang16 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the narrow scope you have in mind, but I’d like restart recovery and the copy/remove/rename crash semantics defined here rather than repaired in #13172

Comment on lines +94 to +99
Broker::Broker(Path staging_root) : staging_root_(fs::weakly_canonical(std::move(staging_root)))
{
io_engines_.push_back(std::make_unique<PosixCopyEngine>());
fs::create_directories(staging_root_ / "restore");
fs::create_directories(staging_root_ / "checkpoint");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit/Abort retryability currently survives only while this Broker process survives. A sidecar restart reconstructs an empty transaction map while the Pod’s shared staging volume and any .pagebroker-partial on checkpoint storage can remain. The next Commit returns TRANSACTION_NOT_FOUND, and a stale partial can block publication indefinitely. Since this MR defines broker-owned staging and retryable terminal operations, please make startup reconcile persisted staging/partial state or persist enough transaction state to determine the outcome after restart.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be a follow-up. What do you think?

Comment on lines +246 to +259
try {
fs::create_directories(published_directory.parent_path());
Engine(transaction.engine_type()).CopyDirectory(staging_directory, partial);
fs::remove_all(published_directory);
fs::rename(partial, published_directory);
checkpoint_transactions_.erase(request.transaction_id());
transaction_states_.at(request.transaction_id()) = TransactionState::COMMITTED;
std::error_code cleanup_error;
fs::remove_all(staging_directory, cleanup_error);
}
catch (...) {
fs::remove_all(partial);
throw;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please define and test the outcome at each crash boundary here: after partial copy, after removing the previous destination, after rename, and after commit succeeds but before the reply reaches the client. In particular, a crash between remove_all(published_directory) and rename makes the previously valid checkpoint disappear.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be a follow-up. What do you think?

@dfeigin-nv
dfeigin-nv force-pushed the pagebroker-daemon branch 5 times, most recently from bc96716 to 8ae556a Compare August 16, 2026 13:29
@dfeigin-nv

Copy link
Copy Markdown
Contributor Author

Moved to ai-dynamo/snapshot#84.

@dfeigin-nv dfeigin-nv closed this Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants