[Store] feat: implements strict multi-tenant quota admission#2612
[Store] feat: implements strict multi-tenant quota admission#2612Lin-z-w wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the tenant quota management system to support a strict multi-tenant mode with connector-managed policies (e.g., YAML files), removing default fallback quotas and requiring explicit tenant registration. Key improvements include the introduction of a TenantQuotaPolicyStore for persistent policy management, updated admin APIs, and modified snapshot restore behavior. The review feedback highlights several optimization opportunities in MasterService, including pre-grouping tenant quotas by shard in ApplyTenantQuotaPolicies to reduce complexity, performing the TenantHasObjects check earlier in DeleteTenantQuotaPolicy to simplify rollback logic, and consolidating loops in RecomputeTenantEffectiveQuotas to avoid redundant double-locking of shard mutexes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
6480029 to
1df4fd2
Compare
e3ad819 to
0bc3b0a
Compare
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
|
||
| const uint64_t reserved_quota_charge = | ||
| RequestedMemoryQuotaCharge(value_length, config); | ||
| auto quota_result = ReserveTenantQuota(tenant_id, reserved_quota_charge); |
There was a problem hiding this comment.
This leaves a race for zero-charge writes. When replica_num == 0 && nof_replica_num > 0, reserved_quota_charge is 0, so ReserveTenantQuota() only validates registration and does not leave any reserved state behind. A concurrent DeleteTenantQuotaPolicy() can then observe the tenant as empty, delete the policy, and this PutStart can still create metadata afterward at line 2760. That violates the strict registration guarantee and can create a new orphan object after the tenant was deleted.
Can we either keep the tenant policy lock held until metadata_object_count is incremented for zero-charge creates, or add a pending write/metadata marker that makes delete see the tenant as non-empty?
There was a problem hiding this comment.
addressed in the latest revision. The three functions now normalize the tenant before taking snapshot_mutex_, so they no longer acquire tenant_quota_policy_mutex_ while holding snapshot_mutex_.
| @@ -4631,7 +5021,11 @@ | |||
| uint64_t size, const std::vector<std::string>& preferred_segments) | |||
| -> tl::expected<PromotionAllocStartResponse, ErrorCode> { | |||
| std::shared_lock<std::shared_mutex> shared_lock(snapshot_mutex_); | |||
There was a problem hiding this comment.
This introduces an inconsistent lock order. AddReplica() takes tenant_quota_policy_mutex_ before snapshot_mutex_, but this path takes snapshot_mutex_ first and then calls NormalizeTenantIdForWrite(), which takes tenant_quota_policy_mutex_. The same pattern also appears in CreateCopyTask() and CreateMoveTask().
With a pending snapshot writer, this can deadlock on writer-preferring shared_mutex implementations: one thread holds the policy mutex and waits for a snapshot shared lock, while this thread holds a snapshot shared lock and waits for the policy mutex. Can we move tenant normalization before acquiring snapshot_mutex_, as PutStart, CopyStart, and MoveStart already do?
There was a problem hiding this comment.
Agreed. I kept the zero-charge path protected by holding tenant_quota_policy_mutex_ only around the metadata-create admission window, and used metadata_object_count to make DeleteTenantQuotaPolicy observe zero-charge objects. This avoids holding the policy mutex through quota accounting itself while still preventing the tenant-delete race.
Description
Part of #2153. Implements strict multi-tenant quota admission for Mooncake Store.
This PR replaces the inherited/default tenant quota model with a connector-backed policy source:
enable_multi_tenants,tenant_quota_connector_type, andtenant_quota_connector_uri.TenantQuotaPolicyStoreand a v1 YAML file connector with atomic writes.Module
mooncake-transfer-engine)mooncake-store)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:
cmake --build build --target tenant_quota_test master_service_tenant_quota_test ctest --test-dir build -R "tenant_quota_test|master_service_tenant_quota_test" --output-on-failureTest results:
tenant_quota_testandmaster_service_tenant_quota_testpassed locally.Checklist
./scripts/code_format.shpre-commit run --all-filesand all hooks passAI Assistance Disclosure
Codex was used to help implement, review, test, and document this change. I reviewed and edited the generated code before submission.