Skip to content

[Store] Persist NoF registrations across HA failover - #4192

Open
cage-goat wants to merge 1 commit into
kvcache-ai:mainfrom
cage-goat:codex/nof-ha-registration-4037
Open

cage-goat wants to merge 1 commit into
kvcache-ai:mainfrom
cage-goat:codex/nof-ha-registration-4037

Conversation

@cage-goat

Copy link
Copy Markdown

Description

Fixes #4037.

NoF storage pool registrations currently need to be recreated after HA recovery. This PR persists these registrations so that a new master can restore the segment ID, owner, name, endpoint, base offset, and capacity without rerunning the registration tool.

  • Record successful NoF mounts, remounts, explicit unmounts, and heartbeat-triggered unmounts in OpLog. Preserve logs for successful items when a later remount item fails, and avoid extra logs for duplicate mounts.
  • Maintain the NoF registration list while the standby replays OpLog, and restore it when the standby becomes the new master.
  • Save NoF registrations in a separate nof_segments.bin file in batch OpLog snapshots. Load these registrations before replaying subsequent logs. Older snapshots without this information remain readable, with an empty NoF registration list.
  • Include the NoF registration file in snapshot restore, log pruning, and GC validation.

This applies to HA with OpLog enabled. Registration logs remain asynchronous: a successful registration RPC does not guarantee that its log entry is durable yet.

Scope: Rebuilding the allocator's occupied ranges is outside this PR. The existing behavior where allocations after HA recovery can reuse offsets referenced by restored replicas is tracked separately in #3826; the related quarantine proposal is in #3858. This PR does not fix that issue.

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Mooncake Conductor (mooncake-conductor)
  • Reshard (mooncake-reshard)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

Built and tested with both USE_NOF=OFF and USE_NOF=ON. All newly added NoF tests passed.

Test commands:

# Master HA suite, run in both build configurations:
cmake --build "$BUILD_DIR" --target master_service_ha_test -j 2
ctest --test-dir "$BUILD_DIR" --output-on-failure -R '^master_service_ha_test$'

Additional validation covered OpLog replay, snapshot encoding and restoration, pruning, GC, segment management, and NoF heartbeat behavior.

Configuration Related test executables Master HA test cases
USE_NOF=OFF 16/16 passed 101/101 passed
USE_NOF=ON 16/17 passed 105/106 passed

The only failure was the pre-existing NoFBatchEvictReleasesNoFSpaceAfterDurable, with the same assertions failing before these changes. Four cases were skipped according to the build configuration.

Manual validation used three HA master processes on one host, a dedicated etcd 3.5.21 instance, and a 64 MiB SPDK malloc bdev over NVMe/TCP. The client used one NoF replica and no memory replicas.

Both OpLog-only recovery and snapshot-plus-OpLog recovery were tested by killing the active master and by killing and restarting all masters. Each scenario registered the pool only once. The registration fields and owner were preserved, and recovery did not write additional mount logs. The snapshot scenario also pruned old logs to verify recovery from the saved registration file.

Test results:

  • Unit tests pass
  • Integration tests pass (if applicable)
  • Manual testing done (described above)

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
  • [] I have run pre-commit on the files changed in this PR and all hooks pass
  • I have updated the documentation (if applicable)
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue

AI Assistance Disclosure

  • No AI tools were used
  • AI tools were used (specify below)

Codex assisted with implementation, test refinement, rebase integration, and validation.

Record NoF mount and unmount events in OpLog, including successful
registrations from partially failed remounts. Restore segment metadata
and ownership during standby promotion.

Include NoF registrations in batch OpLog snapshots and validate the
optional artifact during restore, pruning and GC. Preserve compatibility
with snapshots that omit NoF metadata.

Signed-off-by: Cage Chen <cage@arcfra.com>

@he-yufeng he-yufeng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mechanism read through the record path, the replay path, and the snapshot compat story. The lifecycle accounting looks right at every site I can attack it from: mount only logs after OK, remount logs exactly the successful indices with the NoF lock held across the queueing so a concurrent unmount cannot overtake the mount events, explicit and heartbeat unmounts log only after commit/OK, replay validates payload and non-empty endpoint before touching the registry, and the old-snapshot story degrades exactly the way the description claims (legacy baseline loads an empty NoF registry, which is pre-PR behavior).

One design question before this lands, on the registry key. StandbyNoFSegmentRegistry is keyed by transport_endpoint alone: OnSegmentMount overwrites segments_by_endpoint_[endpoint] and OnSegmentUnmount erases by endpoint. The master-side indexes do not impose endpoint uniqueness (mounted_segments_ is by segment id, client_by_name_ by name), so nothing stops one client from registering two distinct NoF segments with different names through the same transport endpoint. If that configuration is possible, the standby registry collapses the two: the second mount overwrites the first, and an unmount of either erases both, so a failover would restore neither (or restore a stale one while the live one keeps running). Is segment-per-endpoint uniqueness enforced somewhere upstream (validation at mount, or a NoF domain invariant), or should the registry key on segment id / (endpoint, name) instead?

Also flagging an interaction for sequencing, not as an objection: this changes the RestoreFromStandbyState signature and the standby restore path, which #3806 also reworks (tolerant index restore + ambiguous-overlap discard). Whichever lands second needs a rebase pass on the other; the semantics compose fine (this adds the NoF channel, #3806 hardens the object index channel), but the textual overlap in master_service.h/master_service.cpp and master_service_ha_test.cpp is nontrivial.

Reviewed at the mechanism level only; I did not run this branch's test suite locally.

@cage-goat

Copy link
Copy Markdown
Author

@he-yufeng

Thanks for the review. The mount path already enforces endpoint uniqueness for segments in the OK state, through this check in ScopedNoFSegmentAccess::MountSegment():

if (existing_segment.status == SegmentStatus::OK &&
    existing_segment.segment.te_endpoint == segment.te_endpoint) {
    return ErrorCode::SEGMENT_ALREADY_EXISTS;
}

As long as the existing segment is OK, a second registration with the same endpoint is rejected internally, even if its segment ID and name differ. The master treats this as an idempotent success without adding another registration or writing another mount log. Remount uses the same check.

However, this check only covers OK segments. Once an existing segment enters UNMOUNTING, a new registration for the same endpoint can be accepted before the old unmount finishes. If the registration reuses the same name, the old unmount's completion can clear name-based state installed by the new registration, leaving the master's in-memory state inconsistent.

I am already working on this race and will open a separate issue and follow-up PR for it. This PR remains scoped to persisting and restoring NoF registrations.

I have also noted the overlap with #3806. If this PR lands second, I will rebase onto it and check the integration between the restore interfaces, NoF registration recovery, and object-index recovery.

@he-yufeng

Copy link
Copy Markdown
Collaborator

Thanks, the OK-state idempotent rejection resolves the keying question for me, and the UNMOUNTING race as its own issue+PR is the right split. On the overlap: whichever lands second, the restore-interface rebase should be mechanical since the NoF channel and the object-index channel compose through the same RestoreFromStandbyState call sites.

@cage-goat

Copy link
Copy Markdown
Author

@he-yufeng I've opened #4212 with a fix for the UNMOUNTING race we discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation run-ci Store

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request]: Persist NoF storage pool registrations across master restarts

2 participants