[RFC]: [Store] Split snapshot orchestration and serialization logic from MasterService

[Aionw]

Motivation

Snapshot is not part of the core master data path. Conceptually, it is a persistence and recovery capability that observes, captures, and restores master state, and should be modeled as a separate subsystem rather than intrinsic master request-processing logic.

Keeping snapshot orchestration and serialization inside MasterService blurs this abstraction boundary. It also makes snapshot format evolution difficult, since part of the snapshot serialization logic is implemented as a private nested component of MasterService.

This RFC proposes splitting snapshot orchestration and snapshot state serialization into dedicated components while preserving the current snapshot behavior and runtime semantics.

Proposed Changes

1. Move snapshot orchestration out of MasterService

Introduce a dedicated snapshot manager module, for example:

• ha/snapshot/master_snapshot_manager.h
• ha/snapshot/master_snapshot_manager.cpp

This component will own snapshot-specific orchestration:

• periodic snapshot scheduling
• snapshot ID generation
• snapshot descriptor construction
• child process lifecycle management
• timeout handling
• snapshot payload upload
• catalog publish
• retention cleanup
• snapshot metrics updates

MasterService should only provide the state capture/restore hooks needed by the snapshot manager.

2. Extract snapshot state serialization from MasterService

Move master snapshot state serialization into standalone modules instead of keeping part of the snapshot format inside MasterService.

The current snapshot payload is composed of multiple state files:

• metadata: object metadata shards, discarded replicas, and replica ID state
• segments: mounted memory segments, client-to-segment mappings, allocator ordering, and local disk segment/offloading state
• task_manager: persisted client task manager state
• manifest.txt: snapshot serializer type, version, and snapshot ID

The refactor should model these as one master snapshot state bundle, with dedicated serializers for each sub-state. MasterService should remain responsible for owning live runtime state, while the snapshot serialization layer should own the persisted representation and format compatibility.

3. Define explicit snapshot data structures

Introduce explicit snapshot DTOs for the full master snapshot state, such as:

• metadata snapshot state
• segment manager snapshot state
• task manager snapshot state
• snapshot manifest metadata

These structures should represent the persisted snapshot format independently from the internal layout of MasterService, SegmentManager, and ClientTaskManager.

This will make future snapshot format evolution easier and reduce accidental coupling to private master internals.

4. Keep restore behavior unchanged

Restore should continue to support the current snapshot storage layout and manifest validation behavior.

The proposed refactor should only move restore selection, download, decoding, and application logic into clearer components. The externally visible behavior should remain unchanged.

A possible split is:

• snapshot repository: list, download, upload, publish, cleanup
• snapshot codec: encode/decode snapshot payloads
• master state restorer: apply decoded snapshot data to MasterService

5. Preserve existing configuration and compatibility

Existing snapshot flags and environment variables should continue to work:

• --enable_snapshot
• --snapshot_interval_seconds
• --snapshot_child_timeout_seconds
• --snapshot_retention_count
• --snapshot_object_store_type
• --snapshot_catalog_store_type
• --snapshot_catalog_store_connstring
• --snapshot_backup_dir
• --enable_snapshot_restore
• MOONCAKE_SNAPSHOT_LOCAL_PATH

Existing snapshots should remain restorable unless a later RFC explicitly proposes a format migration.

Expected Benefits

• Clearer abstraction boundary between core master request-processing logic and snapshot persistence/recovery logic
• Smaller and easier-to-review MasterService
• Clear ownership boundary for snapshot code
• Easier snapshot format testing across metadata, segment, and task-manager payloads
• Easier restore testing without constructing a full master lifecycle
• Reduced reliance on private nested classes and test-only friendships
• Lower risk when adding new fields to snapshot format

Non-goals

This RFC does not propose changing the snapshot consistency model.

This RFC does not propose changing the current HA design.

This RFC does not propose changing snapshot storage backends or catalog semantics.

This RFC does not propose changing user-facing configuration.

Testing Plan

Add or update unit tests for:

• full snapshot state encode/decode round trip
• metadata snapshot encode/decode compatibility
• segment snapshot encode/decode compatibility
• task manager snapshot encode/decode compatibility
• snapshot manager descriptor construction
• snapshot manager publish/cleanup behavior
• restore fallback across multiple candidate snapshots
• restore failure handling and state reset

Keep existing snapshot integration tests passing, including:

• master snapshot/restore tests
• SSD snapshot tests
• promotion snapshot tests
• catalog-backed snapshot provider tests
• snapshot child process tests

Migration Plan

1. Introduce standalone snapshot manager and serializer modules.
2. Move code from MasterService into the new modules without behavior changes.
3. Replace direct private serializer usage with explicit state capture/restore hooks.
4. Keep existing tests passing after each step.
5. Remove obsolete private nested serializer definitions and unnecessary test friendships once callers are migrated.

[github-actions]

Thanks for opening this issue, @Aionw!

Field	Value
Issue	#2688
GitHub user ID	41376987
Reporter	@Aionw

A maintainer will triage this when possible. To help us respond faster, please include:

• Mooncake version or commit SHA
• Environment (OS, CUDA/driver, RDMA stack if relevant)
• Steps to reproduce and expected vs. actual behavior

Useful links: Documentation · Contributing guide

This message was posted automatically by the issue bot.

[xiangui33423]

Hi, I’m interested in working on this RFC.

Since this looks like a relatively large refactor, I’d like to avoid changing snapshot semantics in one big PR. My proposed first step is to split out the snapshot orchestration path from MasterService into a dedicated snapshot manager, while keeping the current snapshot format, restore behavior, configuration flags, and tests unchanged.

After that lands, the snapshot state serialization / DTO split can be handled in a separate follow-up PR.

Does this staged direction sound acceptable?

[Aionw]

Thanks! That’d be awesome. Before diving into the implementation, it’d be great if we could align on the high-level design in the RFC first. Once we’re on the same page, feel free to open a PR.

[xiangui33423]

Thanks for the guidance! I agree it makes sense to align on the design before starting the implementation.

Here is my current thinking on the split.

Snapshot orchestration boundary

I think the first PR should move the full snapshot orchestration path out of MasterService and into a dedicated MasterSnapshotManager.

That would include:

• periodic scheduling;
• snapshot descriptor construction;
• fork / child process handling;
• timeout and log forwarding;
• upload and catalog publishing;
• retention cleanup.

In this model, MasterService would only be responsible for wiring the component during initialization, for example by creating the manager and calling Start() / Stop(). The existing SnapshotThreadFunc() logic would effectively become part of MasterSnapshotManager.

This seems cleaner than keeping the scheduling loop in MasterService and only delegating the actual snapshot operation, since otherwise the snapshot lifecycle would still be split across two places.

State serialization boundary

For the first step, I would avoid redesigning the snapshot payload format. My preference is to keep the existing serialization behavior mostly unchanged, but move the serialization code behind a cleaner interface.

A possible first step is:

• move MetadataSerializer out of MasterService into a standalone snapshot-related class;
• pass the required state explicitly, preferably as const references where possible;
• keep the current encoded snapshot format unchanged.

Longer term, we can introduce explicit DTOs such as MetadataShardSnapshot, SegmentSnapshot, or TaskSnapshot, but I think doing that in the first PR may make the refactor too large and harder to review. So I would treat the DTO split as a follow-up after the orchestration boundary is settled.

Fork safety and locking

For locking, I would keep the current behavior unchanged in the first PR.

My understanding is that snapshot_mutex_ currently protects the fork-based snapshot path and participates in the existing lock order:

tenant_quota_policy_mutex_ -> snapshot_mutex_ -> metadata_shards_[i].mutex

To avoid changing semantics, I think snapshot_mutex_ should remain shared between MasterService and MasterSnapshotManager, likely passed into the manager during construction together with the snapshot config. That keeps the current lock ordering intact and avoids introducing a new locking model as part of this refactor.

I would not try to replace the current fork-under-lock behavior with COW snapshots in this PR, since that feels like a separate design change.

Restore path

For restore, I slightly prefer keeping MasterService in control of applying restored state.

So the split could be:

• MasterSnapshotManager / repository layer: find and load the latest snapshot;
• snapshot codec: decode the snapshot payload;
• MasterService: apply the decoded state during initialization.

That keeps state mutation local to MasterService, while still moving snapshot storage / loading / decoding logic out of the main service class.

Proposed PR sequence

To keep the rollout reviewable, I would suggest something like:

1. Extract MasterSnapshotManager and move snapshot orchestration into it, with no behavior or format changes.
2. Move MetadataSerializer out of MasterService behind a standalone snapshot codec interface, still preserving the current format.
3. Refactor the restore path into repository / codec / apply-state pieces.
4. Add more focused compatibility tests around snapshot encode/decode and restore.

I would avoid adding a feature flag unless maintainers think it is necessary. Since the first step should be behavior-preserving, a direct cutover with compatibility tests may be simpler.

Overall, my proposed first PR would focus only on the orchestration split and keep the snapshot format, restore semantics, config flags, and locking behavior unchanged.

Does this direction match what you had in mind for the RFC?

[Aionw]

Overall, this looks good to me. Feel free to invite me to review it once your PR is ready.

[xiangui33423]

Hi, I’ve opened the first PR #2805 for this RFC.

This PR focuses only on the first step: extracting snapshot orchestration from MasterService into a dedicated MasterSnapshotManager, while keeping the existing behavior unchanged.

In particular, it does not change the snapshot format, restore path, storage layout, config flags, or locking semantics. The goal is to make this step easier to review and keep the refactor bisectable.

If this boundary looks good, I’ll continue with the next step in a follow-up PR, likely extracting the snapshot repository / storage-related operations.

[xiangui33423]

Hi reviewers, this PR #2831 is ready for review.

This is the third step of the snapshot refactor series. The main goal here is to extract the master snapshot encode/decode logic into ha::MasterSnapshotCodec, while keeping the existing snapshot format and behavior unchanged.

A few notes on the scope:

• This PR owns the snapshot payload format boundary: metadata, segment manager state, task manager state, discarded replicas, and the manifest descriptor.
• It does not change the on-disk format, restore semantics, storage layout, or snapshot scheduling behavior.
• MasterSnapshotManager now delegates encode/decode to the codec, but the higher-level restore flow is intentionally left for the next PR.
• I addressed the automated review comments by removing the internal const_cast usage and replacing the string-keyed payload map with a typed MasterSnapshotPayloads struct.

The main thing I’d like feedback on is whether this codec boundary looks reasonable before I continue with PR 4 for the restore path split.

[xiangui33423]

Update on the snapshot refactor series:

I’ve opened PR #2879 as the fourth step of this RFC.

This PR focuses on splitting the snapshot restore path into a layered flow:

• repository layer: load restore candidates and download snapshot payloads;
• codec layer: decode snapshot payloads into in-memory state;
• MasterService: apply the decoded state, clean up expired metadata, and rebuild metrics.

The intent is to keep this step focused on the restore-path boundary only. It does not change the snapshot on-disk format, storage layout, or restore semantics.

After this, the remaining follow-up should mainly be cleanup of obsolete snapshot code left from the refactor.