Repository navigation
[Store] Cut MasterService over to tenant namespaces, the Evictor and the tenant quota policy - #4887
Conversation
Icedcoco
left a comment
Icedcoco
left a comment
There was a problem hiding this comment.
Thanks for the substantial refactor. I like the direction of consolidating metadata, eviction, and quota handling. I had a few questions while reading the changes:
-
For a NoF-only
PutStart(replica_num = 0), could it passGrow(0)and pause during allocation before publishing the object? IfDeletePolicychecks the tenant during that window, could it see zero charged bytes and zero objects, remove the policy, and then have thePutStartpublish successfully? I noticed an existingUSE_NOFtest that seems to expect deletion to wait and returnTENANT_NOT_EMPTY. Is there another synchronization mechanism that prevents this interleaving? -
CreateKeyappears to create the namespace beforeGrow(0)rejects an unregistered tenant, and namespaces seem to remain for the service lifetime. Could repeated requests with different unregistered tenant IDs leave many empty namespaces behind? Would it make sense to check registration before creating the namespace, or is there a reason to keep the current ordering? -
One small eviction behavior detail: soft-pinned group members seem to be deferred individually, and the fallback checks the minimum target between members. Could it stop after evicting one member, leaving the rest of the group in place? If so, is that an acceptable tradeoff, or should the fallback preserve the all-or-none group behavior described in the docs?
-
OnGrowappears to increment the quota-rejection metric, andPutStart/UpsertStartalso increment it when returning the same error. Are those increments intended to count different cases? If not, might the same rejection be counted twice?
|
Thanks for the careful read. All four were worth checking, point by point:
|
github-actions
Bot
added
documentation
Icedcoco
left a comment
Icedcoco
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing the earlier feedback!
One small, non-blocking observation: in the HA path, could durable finalization remove the objects after the eviction pass has already checked for shrinking? If the namespace then stays idle, its route bucket capacity might remain at the previous high-water mark until another shrink is triggered.
My understanding is that this only retains some reusable hash-table capacity, while the objects and their resources are still released normally. I consider this a very minor detail and am happy for the PR to merge as-is. It could be revisited later if it becomes relevant in practice.
Every object now lives in its tenant's namespace (metadata::NamespaceTable, one route per tenant) instead of the 1024 metadata shards. The primitives the previous steps moved MasterService onto keep their shape and change what is behind them: - MetadataAccessorRW and MetadataAccessorRO hold the key's own lock through a route guard instead of the key's shard lock. A plain or ObjectRef accessor holds the key only while it holds an object; a CreateKey accessor holds it either way, so the caller can publish. - The tenant cursors give way to the routes' own cursors: a walk over every tenant is VisitNamespaces followed by ns.objects.ReadCursor(), and the in-flight sweep reads the namespace's in-flight list. - Group membership and the shared group lease live in each route's group index, which Publish and TearDown keep in step, so GroupDomain and the register/unregister calls go. MetadataShard, its accessors, GroupDomain, TenantState and the cursor slices are deleted. So is the per-shard disk object counter, which nothing read since kvcache-ai#2286, and the test-only ClearCandidatesForReload with the tracker's Reset, both orphaned by kvcache-ai#4406. A tenant's quota account is looked up per request instead of being cached on its TenantState; the quota cutover replaces it with the namespace policy. ClearStaleHandles and BatchEvict keep the bucket shrink of kvcache-ai#4103 and whose maps have grown sparse, and ShrinkBucketsIfSparse moves to common/shrink_buckets.h. Until the eviction cutover, BatchEvict's census runs one namespace per thread rather than slices of one namespace. Tests that reached into the shards hold keys through the route instead, and the ones that pinned shard routing or slices are dropped. Co-authored-by: CAICAIIs <3360776475@qq.com>
BatchEvict and EvictTenantMemoryForQuota hand the choice of what to evict to eviction::Evictor, which walks each namespace's lease index, oldest lease first, and holds one key at a time. What used to be the census, the reserve frontier, the two passes and EvictGroupOrObject goes: a group comes out of the index as one entry, and each namespace gives up its own share of the target. MasterService::MemoryEviction holds what the evictor is built from: - the policy: completed memory replicas nobody holds; a soft-pinned object is deferred to make up the minimum when soft pins may be evicted, and skipped otherwise; - the reclaimer: on the spot (cache, quota and dynamic-replication bookkeeping as before), through the OpLog under HA, or behind the offloading reclaimer with offload-on-evict; - the observers: metrics, the per-run log, and the KV removal event when the OpLog is off. BatchEvict keeps its housekeeping (expired writes, deferred releases), the route shrink after a round that evicted and the allocation-pressure signal. Quota eviction now also goes through the OpLog under HA, which it bypassed before. NoF eviction keeps its own pass, since NoF replicas are not in the lease index. The eviction index ranks leases in 100 ms buckets, so the group test puts its ungrouped objects a bucket after the group instead of a nanosecond.
Multi-tenancy is now TenantQuotaPolicy, the NamespacePolicy every namespace is created under; with multi-tenancy off there is no policy at all. MasterService no longer books quota itself: - Admission and growth: a write asks the policy before it allocates (MetadataAccessorRW::Grow, at the five places memory grows: PutStart, UpsertStart, CopyStart, MoveStart and PromotionAllocStart), and a retained-client AddReplica holds the policy's admission token for the whole request instead of taking the manager's policy lock by hand. - Cost: every write lock released on a key settles what the account holds for it towards what the object owes now (TenantQuotaPolicy::MemoryCharge, plus a replaced object's charge while the upsert that replaced it is still being written). UpsertStart tells the policy before it replaces an object; EraseMetadata tells it before a teardown. - Background: restores, capacity changes and each eviction-thread round go to the policy (OnRestored, OnCapacityChanged, OnMaintenance), which owns the per-tenant watermark eviction and asks for it through StoreControl. EvictTenantMemoryForQuota becomes EvictNamespaceMemory. That covers what TenantQuotaLedger tracked as pending, committed and replaced bytes, so the ledger, ObjectMetadata::quota_ledger, the pending_quota_charge_bytes task fields, the charge/release/settle helpers and UpsertStart's replacement-charge transfer with its rollback paths are gone. QuotaEraseMode, now only about KV events, becomes EraseMode. The RPC layer asks the policy directly for the quota admin calls (tenant_quota_policy()); MasterService keeps IsTenantQuotaEnabled() for request tenant resolution. Error codes are unchanged.
A lease cursor copied each bucket it read into one Entry per key, each with its own vector and its own key string, under the index lock. With every eviction going through the index, that was most of a round's time: with a million leases in one 100 ms bucket (a bulk load or a restore), 72% of a BatchEvict that evicted a tenth of them went to those two allocations per key, for keys the round never reached as much as for those it did. The cursor now copies a bucket's keys back to back into one buffer it reuses (LeaseEvictionIndex::Copy), and Entry::keys views it: the keys stay valid until the cursor is asked for the next entry or destroyed, which is how the evictor uses them. batch_evict_bench, 1M objects, target 0.1: 0.44 s -> 0.21 s with every lease in one bucket (main 0.16 s), 0.13 s -> 0.10 s with 10k keys per bucket (main 0.17 s).
ObjectMetadata held its pending session tag and pending soft-pin action inline, 96 and 80 bytes, though each exists only between a write's start and its end. They are now HeapOptional, a pointer while absent, which takes 158 bytes off every object (1M objects: 1245 -> 1087 bytes allocated per object).
Two fixes: - A rejected PutStart or UpsertStart counted toward mooncake_tenant_quota_reject_total twice: TenantQuotaPolicy::OnGrow counts every rejection, and the two writes still counted it again. - The route shrink after an eviction round skipped a namespace the round emptied, since VisitNamespaces only visits namespaces that hold an object. That namespace has the most buckets to give back; the shrink now visits every namespace. And what the cutover left behind: - TenantHasObjects, which lost its last caller with the ledger; the accessor's friend for a rollback that no longer reaches its guard; a friend naming a class that does not exist; the <bitset> includes the shard bitsets needed. - The erase mode and media hint of the durable finalizers, which every caller passed as kFull and nothing. - MasterService members for the offload cap ratio and force-evict flag, read only to build the eviction settings. - The cursor's Clear and Seal around ReadBucket move into ReadBucket, so the copy is sealed by the call that fills it. - One comment on Grow instead of the same one at five call sites; comments, log lines and test names that still talked about metadata shards or the ledger. - Tests: the test peer's ledger-era wrappers (tests call the policy hooks directly), the helpers that searched for group ids hashing to another metadata shard, and QueueLimitRejectsCrossShard, which became the same test as QueueLimitRejectsBeyondCap once there were no shards.
…eviction upkeep - PutStart and UpsertStart took the key, and so created the tenant's namespace, before Grow(0) refused a tenant that is not registered. A namespace stays once created, so every write from an unknown tenant id left one behind, with its 1024 stripes, for every later eviction round to walk. PrepareWriteStart now refuses the tenant first; Grow(0) under the key keeps the last word. - The route shrink after eviction took every stripe lock of every namespace whenever a round released anything, and released replicas alone, with the key kept, were enough. The evictor now shrinks the route of a namespace only once its part of the run dropped an object. An object dropped through the OpLog goes once its record is durable, so a later run's shrink gives its key back, as on main. - A BatchEvict round that only released expired discarded replicas counted as a failure: MetricsObserver saw the evictor's report and nothing else. The pool-wide success and failure counters move back to BatchEvict, which counts that release as progress; the observer keeps the per-tenant bytes. - The namespace policy was declared after the namespaces and so destroyed before them, against the Namespace contract that the policy outlives it. It is now declared first.
- A PutStart with NoF replicas only charges the tenant nothing, and its object counts only once it is published. DeletePolicy looked at those two and nothing else, so it could delete the tenant while such a write was allocating, and the write then published under a tenant that no longer had a policy. On main the metadata scan waited on the key the write held. - The tenant's account now counts the writes in flight: the first growth asked under a write lock begins one, and releasing that lock ends it once the charge is settled and the object, if any, is published. Disabling the policy is refused while one is in flight. The writer and the disable each write one word and then read the other's, sequentially consistent, so a racing pair cannot both miss. - The key marks the begun write in the top bit of the word that holds its charge, which the charge never reaches. - The USE_NOF test now expects DeletePolicy to refuse at once rather than wait for the write.
The fallback came back to each deferred object on its own and asked whether the minimum was met between them, so it could stop after one member of a soft-pinned group and keep the rest, which readers of the group miss on anyway. It now asks only between entries, as main's second pass did between groups, at the cost of evicting more soft-pinned members than the minimum strictly needs.
The route shrink ran after an eviction pass and after ClearStaleHandles, so an object dropped through the OpLog, which goes only once its record is durable, was erased after that pass had already looked. A namespace left idle then kept its stripes' high-water buckets until some later pass happened to shrink it. The route now shrinks a stripe as it drops a slot: SlotHandle::reset already holds the stripe lock exclusively to erase it, and ShrinkBucketsIfSparse costs a bucket count check unless the map has grown sparse. Every erase gives its buckets back on the spot, whichever path did it, so ObjectRoute::ShrinkSparseStripes and the passes that called it are gone.
2772d27 to
7bc70f4
Compare
hongzhi-gao
left a comment
hongzhi-gao
left a comment
There was a problem hiding this comment.
Reviewed commit 7bc70f4d. I reproduced the two regressions below using the existing MasterService test fixtures. Both focused reproductions fail on this head and pass against the available pre-cutover control build (5a9d509c, based on upstream 8c19b728; this is not the exact PR base). Eight existing focused CTest targets and 40 selected HA restore/eviction tests pass on the PR head. Verification used a CPU build and injected HA backends, not live etcd or a NoF build. OpenAI Codex assisted the source review and local reproduction tests.
| kv_events(service), | ||
| log(settings.offload.cap), | ||
| evictor(service.namespaces_, policy, | ||
| settings.through_oplog |
There was a problem hiding this comment.
[P2] Preserve offload-on-evict for HA tenant eviction
When enable_oplog, enable_offload, and offload_on_evict are all enabled, this selects OpLogReclaimer and bypasses OffloadingReclaimer, even with offload_force_evict=false.
This changes the previous tenant-quota eviction behavior: an object with only one MEMORY replica can now be removed without first queuing its LOCAL_DISK copy. PutEnd also skips eager offload in this mode, so there is no earlier offload protecting it.
I reproduced this on 7bc70f4d: evicting a 1,024-byte object reported 1,024 bytes reclaimed, while the offload heartbeat returned zero tasks. The pre-cutover control queued one task and retained the source.
Could we preserve offload gating while using the OpLog for eventual reclamation, and add a regression test for this combination?
There was a problem hiding this comment.
Confirmed, thanks for the reproduction. This predates the cutover: since #2826, memory-pressure eviction skipped the offload whenever the OpLog was on (#4830), while on main tenant-quota eviction offloaded but wrote no OpLog record. Running both through one Evictor took the memory path's choice for both. The fix makes the two choices independent: OffloadingReclaimer now wraps whichever removal applies, the OpLog reclaimer under HA. It is in #4950, with a regression test for HA + offload_on_evict. That also changes memory-pressure eviction under HA, so the PR describes the effect on how much memory a round frees.
| ChargeTenantQuota(accessor.QuotaAccount(), pending_quota_charge); | ||
| if (!quota_result) { | ||
| return tl::make_unexpected(quota_result.error()); | ||
| auto grown = |
There was a problem hiding this comment.
[P2] Credit the retired write’s reservation before replacement admission
When UpsertStart preempts an unfinished primary write with no completed replicas, it erases the old metadata and reaches this allocation path under the same key guard. However, the old reservation remains charged until OnWriteRelease, so Grow() temporarily charges both attempts.
I reproduced this with a 100-byte tenant quota:
PutStart(K, 100), without PutEnd → succeeds; charged = 100
UpsertStart(K, 100) → TENANT_QUOTA_EXCEEDED
After the failed upsert → charged = 0
This unnecessarily rejects the replacement; the pre-cutover control succeeds.
Could we settle or credit the retired attempt’s reservation before admitting the replacement, while retaining in-flight-write protection against concurrent tenant-policy deletion?
There was a problem hiding this comment.
Confirmed. The preemption tears the old object down under the same lock, but the key's charge was settled only when the lock was released, so Grow() charged on top of it. #4950 settles the charge at teardown: a torn-down key owes nothing, while a size-changing upsert, which replaces rather than tears down, still carries the old charge until the replacement settles. Your sequence is the regression test.
Offload admission now hooks the Evictor's OffloadingReclaimer (OffloadQueue::Admit); ace0b07's BatchEvict sort is gone with BatchEvict (kvcache-ai#4887), its tests pass as is. KV snapshot pages walk ObjectRoute stripes through a ranged ReadCursor. MC_STORE_BATCHED_GET now turns on kvcache-ai#4799's per-target grouping for multi-buffer reads.
Description
Steps 15, 16 and 17 of #4557, the last phase: the swap and the two cutovers. One PR of five commits: one per step, in order, then two small follow-ups from profiling the result (4 and 5 below). Steps 1 to 14 are all merged, so nothing here waits on another PR.
1. Serve MasterService metadata from the namespace table (step 15). Every object now lives in its tenant's namespace (
metadata::NamespaceTable, oneroute::ObjectRouteper tenant) instead of the 1024 metadata shards. The primitives steps 9 to 14 moved MasterService onto keep their shape; only what is behind them changes:MetadataAccessorRW/MetadataAccessorROhold the key's own lock through a route guard, not its shard's lock. A plain orObjectRefaccessor holds the key only while it holds an object; aCreateKeyaccessor holds it either way.VisitNamespacesfollowed byns.objects.ReadCursor(), and the in-flight sweep reads the namespace's in-flight list.Publish/TearDown.Deleted:
MetadataShardand its accessors,GroupDomain,TenantState, the cursor slices, and the per-shard disk object counter (write-only since #2286). Also deleted: the test-onlyClearCandidatesForReloadand the tracker'sReset, both orphaned by #4406.The bucket shrink of #4103 and #3576 is kept.
ObjectRoute::ShrinkSparseStripesrehashes a route's sparse stripe maps afterClearStaleHandlesand after an eviction round that evicted something.2. Run memory and quota eviction through the Evictor (step 16).
BatchEvictand the per-tenant quota eviction hand the choice of what to evict toeviction::Evictor(#4811). It walks each namespace's lease index, oldest lease first, one key lock at a time. Gone: the census, the reserve frontier, the two passes andEvictGroupOrObject. A group comes out of the index as one entry, and each namespace gives up its own share of the target.MasterService::MemoryEvictionholds what the evictor is built from:OffloadingReclaimerwith offload-on-evict.NoF eviction keeps its own pass, since NoF replicas are not in the lease index.
3. Attach tenant quotas to the namespaces as their policy (step 17). Multi-tenancy is
TenantQuotaPolicy(#4809), theNamespacePolicyevery namespace is created under. With multi-tenancy off there is no policy at all.MetadataAccessorRW::Grow), at the five places memory grows: PutStart, UpsertStart, CopyStart, MoveStart and PromotionAllocStart.TenantQuotaPolicyderives that from the object.That replaces
TenantQuotaLedger, so the following are deleted:ObjectMetadata::quota_ledger;pending_quota_charge_bytestask fields;QuotaEraseMode, now only about KV events, becomesEraseMode. The quota admin RPCs ask the policy directly viatenant_quota_policy(), and their error codes are unchanged.4. Copy an eviction bucket's keys into one buffer per cursor. Profiling step 16 showed a lease cursor spending most of a round copying each bucket into one
Entryper key, each with its own vector and key string, under the index lock. With a million leases in one 100 ms bucket (a bulk load or restore), 72% of a BatchEvict that evicted a tenth of them went to those two allocations per key, including keys the round never reached. The cursor now copies a bucket's keys back to back into one buffer it reuses (LeaseEvictionIndex::Copy).EvictionIndex::Entry::keysbecomes astd::span<const std::string_view>into that buffer, valid until the cursor is asked for the next entry or destroyed, which is how the evictor uses it.5. Keep pending session tags and soft-pin actions off the object.
ObjectMetadataheldpending_session_tag(96 bytes) andpending_soft_pin_action(80 bytes) inline, though each exists only between a write's start and its end. Both are nowHeapOptional, a pointer while absent. That saves 158 bytes per object and more than offsets what steps 15 to 17 add per key (see the memory numbers below).Behavior changes worth a look
TenantQuotaPolicy::MemoryChargedocuments. Growth beyond what a write asked for is charged rather than rejected as an accounting mismatch.Overlapping work
eviction::OffloadingReclaimer.ObjectRefinstead of a group incarnation and the lease address. Step 16 does not need it; it can go separately.Tracking issue: #4557.
Suggested Merge Priority
Module
mooncake-transfer-engine)mooncake-store)mooncake-conductor)mooncake-reshard)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test commands:
Test results:
tcp_transport_testandtransfer_metadata_testabort without an RDMA/network environment, andordered_oplog_writer_testdoes not link (local libaio). Before the rebase onto c67a88f, each of steps 15, 16 and 17 built and passed the MasterService suites on its own; the rebase changed only the step 16 commit (a conflict with [Store] Decouple NoF data path from SPDK behind neutral interfaces #3424 inmaster_service.cpp, both sides kept).BatchEvict wall time, main (c67a88f) vs this branch at commit 4, default tenant, mean of 2 runs in ABBA order. The bench files every lease within one millisecond, so all of them land in one 100 ms bucket; a local flag spread them to 10k keys per bucket for the second pair of columns, which is what a namespace writing 100k objects per second files.
The evicted counts match main's in every run. What is left of the one-bucket case is the cursor walking and copying the whole bucket when the round needs a tenth of it; reading a bucket in batches would remove it, and is left for later.
Read and write load while eviction runs, master before the series (6e8895c) vs this branch at commit 4: an 8 GiB segment kept at about 97% by 4 writer threads (
BatchPutStart/BatchPutEnd, 5000 keys per batch), 8 reader threads with 30000 keys per batch and no pause, 30 s, mean of 2 runs in ABBA order. Exist used a local--lookup_op=existflag on the bench.BatchGetReplicaListkeys/sBatchGetReplicaListp99 / maxBatchExistKeykeys/sBatchExistKeyp99 / maxMemory per object, 1M objects, heap in use from
mallinfo2with the client still connected:Against the master before the series, the series itself costs about 168 bytes per object: the
ObjectStateevery object carries inline (56, step 1) and what steps 15 to 17 add for theKeySlot(112: its ownstd::shared_mutex, the owner state and two list hooks). Commit 5 takes about 160 bytes off two fields outside the series: #4683's pending session tag and the pending soft-pin action, which predates it.Checklist
./scripts/code_format.shmooncake-store/tests/CMakeLists.txtthat this PR does not touch; those rewrites are left out. Every other hook passes.AI Assistance Disclosure
Claude Code ported the steps from the reference branch onto main, adapted the tests, profiled the eviction path, wrote commits 4 and 5, and ran the builds, tests and benchmarks. I reviewed every changed line.
Co-authored-by: CAICAIIs 3360776475@qq.com (step 15)