From d39c0b773afe1a646a8ead956ae1e59ed5cfff7b Mon Sep 17 00:00:00 2001 From: "A.J. Beamon" Date: Mon, 25 Jul 2022 09:53:56 -0700 Subject: [PATCH] Add a limit to the number of tenants that can be created in a cluster --- fdbclient/ClientKnobs.cpp | 4 ++ fdbclient/include/fdbclient/ClientKnobs.h | 3 + fdbclient/include/fdbclient/Tenant.h | 4 +- .../fdbclient/TenantManagement.actor.h | 14 +++++ .../fdbclient/TenantSpecialKeys.actor.h | 25 +++++++-- fdbserver/BlobManager.actor.cpp | 19 ++----- fdbserver/BlobWorker.actor.cpp | 17 ++---- fdbserver/TenantCache.actor.cpp | 4 +- fdbserver/storageserver.actor.cpp | 3 +- .../TenantManagementWorkload.actor.cpp | 56 ++++++++++++++++++- flow/include/flow/error_definitions.h | 1 + 11 files changed, 115 insertions(+), 35 deletions(-) diff --git a/fdbclient/ClientKnobs.cpp b/fdbclient/ClientKnobs.cpp index 0ad5501629..29316ed2b8 100644 --- a/fdbclient/ClientKnobs.cpp +++ b/fdbclient/ClientKnobs.cpp @@ -22,6 +22,7 @@ #include "fdbclient/FDBTypes.h" #include "fdbclient/SystemData.h" #include "fdbclient/Tenant.h" +#include "flow/IRandom.h" #include "flow/UnitTest.h" #define init(...) KNOB_FN(__VA_ARGS__, INIT_ATOMIC_KNOB, INIT_KNOB)(__VA_ARGS__) @@ -289,6 +290,9 @@ void ClientKnobs::initialize(Randomize randomize) { init( CHANGE_QUORUM_BAD_STATE_RETRY_TIMES, 3 ); init( CHANGE_QUORUM_BAD_STATE_RETRY_DELAY, 2.0 ); + // Tenants and Metacluster + init( MAX_TENANTS_PER_CLUSTER, 1e6 ); if ( randomize && BUGGIFY ) MAX_TENANTS_PER_CLUSTER = deterministicRandom()->randomInt(20, 100); + // clang-format on } diff --git a/fdbclient/include/fdbclient/ClientKnobs.h b/fdbclient/include/fdbclient/ClientKnobs.h index 9280cda629..66b8bbc873 100644 --- a/fdbclient/include/fdbclient/ClientKnobs.h +++ b/fdbclient/include/fdbclient/ClientKnobs.h @@ -284,6 +284,9 @@ public: int CHANGE_QUORUM_BAD_STATE_RETRY_TIMES; double CHANGE_QUORUM_BAD_STATE_RETRY_DELAY; + // Tenants and Metacluster + int MAX_TENANTS_PER_CLUSTER; + ClientKnobs(Randomize randomize); void initialize(Randomize randomize); }; diff --git a/fdbclient/include/fdbclient/Tenant.h b/fdbclient/include/fdbclient/Tenant.h index 089eb5365b..cdcc1d0df4 100644 --- a/fdbclient/include/fdbclient/Tenant.h +++ b/fdbclient/include/fdbclient/Tenant.h @@ -99,12 +99,13 @@ struct TenantMetadataSpecification { KeyBackedObjectMap tenantMap; KeyBackedProperty lastTenantId; + KeyBackedBinaryValue tenantCount; KeyBackedSet tenantGroupTenantIndex; KeyBackedObjectMap tenantGroupMap; TenantMetadataSpecification(KeyRef subspace) : tenantMap(subspace.withSuffix("tenant/map/"_sr), IncludeVersion()), - lastTenantId(subspace.withSuffix("tenant/lastId"_sr)), + lastTenantId(subspace.withSuffix("tenant/lastId"_sr)), tenantCount(subspace.withSuffix("tenant/count"_sr)), tenantGroupTenantIndex(subspace.withSuffix("tenant/tenantGroup/tenantIndex/"_sr)), tenantGroupMap(subspace.withSuffix("tenant/tenantGroup/map/"_sr), IncludeVersion()) {} }; @@ -116,6 +117,7 @@ private: public: static inline auto& tenantMap = instance.tenantMap; static inline auto& lastTenantId = instance.lastTenantId; + static inline auto& tenantCount = instance.tenantCount; static inline auto& tenantGroupTenantIndex = instance.tenantGroupTenantIndex; static inline auto& tenantGroupMap = instance.tenantGroupMap; diff --git a/fdbclient/include/fdbclient/TenantManagement.actor.h b/fdbclient/include/fdbclient/TenantManagement.actor.h index db32f1130a..7838db39b5 100644 --- a/fdbclient/include/fdbclient/TenantManagement.actor.h +++ b/fdbclient/include/fdbclient/TenantManagement.actor.h @@ -19,6 +19,7 @@ */ #pragma once +#include "fdbclient/ClientBooleanParams.h" #include "flow/IRandom.h" #if defined(NO_INTELLISENSE) && !defined(FDBCLIENT_TENANT_MANAGEMENT_ACTOR_G_H) #define FDBCLIENT_TENANT_MANAGEMENT_ACTOR_G_H @@ -140,6 +141,16 @@ Future, bool>> createTenantTransaction(Transa } } + // This is idempotent because we only add an entry to the tenant map if it isn't already there + TenantMetadata::tenantCount.atomicOp(tr, 1, MutationRef::AddValue); + + // Read the tenant count after incrementing the counter so that simultaneous attempts to create + // tenants in the same transaction are properly reflected. + int64_t tenantCount = wait(TenantMetadata::tenantCount.getD(tr, Snapshot::False, 0)); + if (tenantCount > CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER) { + throw cluster_no_capacity(); + } + return std::make_pair(tenantEntry, true); } @@ -232,7 +243,10 @@ Future deleteTenantTransaction(Transaction tr, throw tenant_not_empty(); } + // This is idempotent because we only erase an entry from the tenant map if it is present TenantMetadata::tenantMap.erase(tr, name); + TenantMetadata::tenantCount.atomicOp(tr, -1, MutationRef::AddValue); + if (tenantEntry.get().tenantGroup.present()) { TenantMetadata::tenantGroupTenantIndex.erase(tr, Tuple::makeTuple(tenantEntry.get().tenantGroup.get(), name)); diff --git a/fdbclient/include/fdbclient/TenantSpecialKeys.actor.h b/fdbclient/include/fdbclient/TenantSpecialKeys.actor.h index edefe22020..ece4f6ec6c 100644 --- a/fdbclient/include/fdbclient/TenantSpecialKeys.actor.h +++ b/fdbclient/include/fdbclient/TenantSpecialKeys.actor.h @@ -107,7 +107,8 @@ private: return results; } - ACTOR static Future createTenant( + // Returns true if the tenant was created, false if it already existed + ACTOR static Future createTenant( ReadYourWritesTransaction* ryw, TenantNameRef tenantName, std::vector, Optional>> configMutations, @@ -127,23 +128,39 @@ private: std::pair, bool> entry = wait(TenantAPI::createTenantTransaction(&ryw->getTransaction(), tenantName, tenantEntry)); - return Void(); + return entry.second; } ACTOR static Future createTenants( ReadYourWritesTransaction* ryw, std::map, Optional>>> tenants, std::map* tenantGroupNetTenantDelta) { + state Future tenantCountFuture = + TenantMetadata::tenantCount.getD(&ryw->getTransaction(), Snapshot::False, 0); int64_t _nextId = wait(TenantAPI::getNextTenantId(&ryw->getTransaction())); - int64_t nextId = _nextId; + state int64_t nextId = _nextId; - std::vector> createFutures; + state std::vector> createFutures; for (auto const& [tenant, config] : tenants) { createFutures.push_back(createTenant(ryw, tenant, config, nextId++, tenantGroupNetTenantDelta)); } TenantMetadata::lastTenantId.set(&ryw->getTransaction(), nextId - 1); wait(waitForAll(createFutures)); + + state int numCreatedTenants = 0; + for (auto f : createFutures) { + if (f.get()) { + ++numCreatedTenants; + } + } + + // Check the tenant count here rather than rely on the createTenantTransaction check because we don't have RYW + int64_t tenantCount = wait(tenantCountFuture); + if (tenantCount + numCreatedTenants > CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER) { + throw cluster_no_capacity(); + } + return Void(); } diff --git a/fdbserver/BlobManager.actor.cpp b/fdbserver/BlobManager.actor.cpp index 446ec2e390..c8f8431682 100644 --- a/fdbserver/BlobManager.actor.cpp +++ b/fdbserver/BlobManager.actor.cpp @@ -967,19 +967,12 @@ ACTOR Future monitorClientRanges(Reference bmData) { } else { state KeyBackedRangeResult> tenantResults; wait(store(tenantResults, - TenantMetadata::tenantMap.getRange( - tr, Optional(), Optional(), CLIENT_KNOBS->TOO_MANY))); - ASSERT_WE_THINK(!tenantResults.more && tenantResults.results.size() < CLIENT_KNOBS->TOO_MANY); - if (tenantResults.more || tenantResults.results.size() >= CLIENT_KNOBS->TOO_MANY) { - TraceEvent(SevError, "BlobManagerTooManyTenants", bmData->id) - .detail("Epoch", bmData->epoch) - .detail("TenantCount", tenantResults.results.size()); - wait(delay(600)); - if (bmData->iAmReplaced.canBeSet()) { - bmData->iAmReplaced.sendError(internal_error()); - } - throw internal_error(); - } + TenantMetadata::tenantMap.getRange(tr, + Optional(), + Optional(), + CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER + 1))); + ASSERT(tenantResults.results.size() <= CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER && + !tenantResults.more); std::vector prefixes; for (auto& it : tenantResults.results) { diff --git a/fdbserver/BlobWorker.actor.cpp b/fdbserver/BlobWorker.actor.cpp index 568c12ebef..533dac0aaa 100644 --- a/fdbserver/BlobWorker.actor.cpp +++ b/fdbserver/BlobWorker.actor.cpp @@ -3999,18 +3999,11 @@ ACTOR Future monitorTenants(Reference bwData) { tr->setOption(FDBTransactionOptions::PRIORITY_SYSTEM_IMMEDIATE); state KeyBackedRangeResult> tenantResults; wait(store(tenantResults, - TenantMetadata::tenantMap.getRange( - tr, Optional(), Optional(), CLIENT_KNOBS->TOO_MANY))); - ASSERT_WE_THINK(!tenantResults.more && tenantResults.results.size() < CLIENT_KNOBS->TOO_MANY); - if (tenantResults.more || tenantResults.results.size() >= CLIENT_KNOBS->TOO_MANY) { - TraceEvent(SevError, "BlobWorkerTooManyTenants", bwData->id) - .detail("TenantCount", tenantResults.results.size()); - wait(delay(600)); - if (bwData->fatalError.canBeSet()) { - bwData->fatalError.sendError(internal_error()); - } - throw internal_error(); - } + TenantMetadata::tenantMap.getRange(tr, + Optional(), + Optional(), + CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER + 1))); + ASSERT(tenantResults.results.size() <= CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER && !tenantResults.more); std::vector> tenants; for (auto& it : tenantResults.results) { diff --git a/fdbserver/TenantCache.actor.cpp b/fdbserver/TenantCache.actor.cpp index dffda39e4e..2d3cb5cde0 100644 --- a/fdbserver/TenantCache.actor.cpp +++ b/fdbserver/TenantCache.actor.cpp @@ -33,8 +33,8 @@ class TenantCacheImpl { KeyBackedRangeResult> tenantList = wait(TenantMetadata::tenantMap.getRange( - tr, Optional(), Optional(), CLIENT_KNOBS->TOO_MANY)); - ASSERT(!tenantList.more && tenantList.results.size() < CLIENT_KNOBS->TOO_MANY); + tr, Optional(), Optional(), CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER + 1)); + ASSERT(tenantList.results.size() <= CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER && !tenantList.more); return tenantList.results; } diff --git a/fdbserver/storageserver.actor.cpp b/fdbserver/storageserver.actor.cpp index 84e6771e74..a9aac7af82 100644 --- a/fdbserver/storageserver.actor.cpp +++ b/fdbserver/storageserver.actor.cpp @@ -10127,7 +10127,8 @@ ACTOR Future initTenantMap(StorageServer* self) { // when SSs store only the local tenants KeyBackedRangeResult> entries = wait(TenantMetadata::tenantMap.getRange( - tr, Optional(), Optional(), CLIENT_KNOBS->TOO_MANY)); + tr, Optional(), Optional(), CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER + 1)); + ASSERT(entries.results.size() <= CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER && !entries.more); TraceEvent("InitTenantMap", self->thisServerID) .detail("Version", version) diff --git a/fdbserver/workloads/TenantManagementWorkload.actor.cpp b/fdbserver/workloads/TenantManagementWorkload.actor.cpp index f09c2daea9..49c23205ff 100644 --- a/fdbserver/workloads/TenantManagementWorkload.actor.cpp +++ b/fdbserver/workloads/TenantManagementWorkload.actor.cpp @@ -20,7 +20,9 @@ #include #include +#include "fdbclient/ClientBooleanParams.h" #include "fdbclient/FDBOptions.g.h" +#include "fdbclient/ReadYourWrites.h" #include "fdbclient/RunTransaction.actor.h" #include "fdbclient/TenantManagement.actor.h" #include "fdbclient/TenantSpecialKeys.actor.h" @@ -194,6 +196,7 @@ struct TenantManagementWorkload : TestWorkload { // True if any tenant group name starts with \xff state bool hasSystemTenantGroup = false; + state int newTenants = 0; state std::map tenantsToCreate; for (int i = 0; i < numTenants; ++i) { TenantName tenant = self->chooseTenantName(true); @@ -203,21 +206,39 @@ struct TenantManagementWorkload : TestWorkload { TenantMapEntry entry; entry.tenantGroup = self->chooseTenantGroup(true); - tenantsToCreate[tenant] = entry; - alreadyExists = alreadyExists || self->createdTenants.count(tenant); + if (self->createdTenants.count(tenant)) { + alreadyExists = true; + } else if (!tenantsToCreate.count(tenant)) { + ++newTenants; + } + + tenantsToCreate[tenant] = entry; hasSystemTenant = hasSystemTenant || tenant.startsWith("\xff"_sr); hasSystemTenantGroup = hasSystemTenantGroup || entry.tenantGroup.orDefault(""_sr).startsWith("\xff"_sr); } state Reference tr = makeReference(cx); + state int64_t minTenantCount = std::numeric_limits::max(); + state int64_t finalTenantCount = 0; loop { try { + if (operationType != OperationType::MANAGEMENT_DATABASE) { + tr->setOption(FDBTransactionOptions::READ_SYSTEM_KEYS); + wait(store(finalTenantCount, TenantMetadata::tenantCount.getD(tr, Snapshot::False, 0))); + minTenantCount = std::min(finalTenantCount, minTenantCount); + } + wait(createImpl(cx, tr, tenantsToCreate, operationType, self)); if (operationType == OperationType::MANAGEMENT_DATABASE) { ASSERT(!alreadyExists); + } else { + // Make sure that we had capacity to create the tenants. This cannot be validated for database + // operations because we cannot determine the tenant count in the same transaction that the tenant + // is created + ASSERT(minTenantCount + newTenants <= CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER); } // It is not legal to create a tenant or tenant group starting with \xff @@ -294,6 +315,13 @@ struct TenantManagementWorkload : TestWorkload { } else if (e.code() == error_code_invalid_tenant_group_name) { ASSERT(hasSystemTenantGroup); return Void(); + } else if (e.code() == error_code_cluster_no_capacity) { + // Confirm that we overshot our capacity. This check cannot be done for database operations + // because we cannot transactionally get the tenant count with the creation. + if (operationType != OperationType::MANAGEMENT_DATABASE) { + ASSERT(finalTenantCount + newTenants > CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER); + } + return Void(); } // Database-based operations should not need to be retried @@ -972,6 +1000,26 @@ struct TenantManagementWorkload : TestWorkload { return Void(); } + // Verify that the tenant count matches the actual number of tenants in the cluster and that we haven't created too + // many + ACTOR static Future checkTenantCount(Database cx) { + state Reference tr = cx->createTransaction(); + loop { + try { + tr->setOption(FDBTransactionOptions::READ_SYSTEM_KEYS); + state int64_t tenantCount = wait(TenantMetadata::tenantCount.getD(tr, Snapshot::False, 0)); + KeyBackedRangeResult> tenants = + wait(TenantMetadata::tenantMap.getRange(tr, {}, {}, CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER + 1)); + + ASSERT(tenants.results.size() == tenantCount && !tenants.more); + ASSERT(tenantCount <= CLIENT_KNOBS->MAX_TENANTS_PER_CLUSTER); + return Void(); + } catch (Error& e) { + wait(tr->onError(e)); + } + } + } + // Verify that the set of tenants in the database matches our local state ACTOR static Future compareTenants(Database cx, TenantManagementWorkload* self) { state std::map::iterator localItr = self->createdTenants.begin(); @@ -1094,6 +1142,10 @@ struct TenantManagementWorkload : TestWorkload { } } + if (self->clientId == 0) { + wait(checkTenantCount(cx)); + } + wait(compareTenants(cx, self) && compareTenantGroups(cx, self)); return true; } diff --git a/flow/include/flow/error_definitions.h b/flow/include/flow/error_definitions.h index 56520c8c15..bc75885925 100755 --- a/flow/include/flow/error_definitions.h +++ b/flow/include/flow/error_definitions.h @@ -235,6 +235,7 @@ ERROR( unknown_tenant, 2137, "Tenant is not available from this server" ) ERROR( illegal_tenant_access, 2138, "Illegal tenant access" ) ERROR( invalid_tenant_group_name, 2139, "Tenant group name cannot begin with \\xff" ) ERROR( invalid_tenant_configuration, 2140, "Tenant configuration is invalid" ) +ERROR( cluster_no_capacity, 2141, "Cluster does not have capacity to perform the specified operation" ) // 2200 - errors from bindings and official APIs ERROR( api_version_unset, 2200, "API version is not set" )