From 6a44778b45e532e102a0dfbfcb393304bf5e0956 Mon Sep 17 00:00:00 2001 From: Josh Slocum Date: Fri, 7 Oct 2022 12:28:06 -0500 Subject: [PATCH 1/8] Fixed spacing in fdbcli status so dashes are aligned --- fdbcli/StatusCommand.actor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fdbcli/StatusCommand.actor.cpp b/fdbcli/StatusCommand.actor.cpp index 9da5e768a3..494da41c38 100644 --- a/fdbcli/StatusCommand.actor.cpp +++ b/fdbcli/StatusCommand.actor.cpp @@ -442,7 +442,7 @@ void printStatus(StatusObjectReader statusObj, outputString += "\n Blob granules - enabled"; } - outputString += "\n Encryption at-rest - "; + outputString += "\n Encryption at-rest - "; if (statusObjConfig.get("encryption_at_rest_mode", strVal)) { outputString += strVal; } else { From 4ebe341e2c6f5b34976f898dac4820e25992956f Mon Sep 17 00:00:00 2001 From: Josh Slocum Date: Fri, 7 Oct 2022 16:12:39 -0500 Subject: [PATCH 2/8] adding --disable_client_bypass to mako --- bindings/c/test/mako/mako.cpp | 15 +++++++++++++++ bindings/c/test/mako/mako.hpp | 2 ++ 2 files changed, 17 insertions(+) diff --git a/bindings/c/test/mako/mako.cpp b/bindings/c/test/mako/mako.cpp index e7ce98b198..0cc82e3d8a 100644 --- a/bindings/c/test/mako/mako.cpp +++ b/bindings/c/test/mako/mako.cpp @@ -875,6 +875,16 @@ int workerProcessMain(Arguments const& args, int worker_id, shared_memory::Acces } } + if (args.disable_client_bypass > 0) { + err = network::setOptionNothrow(FDB_NET_OPTION_DISABLE_CLIENT_BYPASS, args.disable_client_bypass); + if (err) { + logr.error("network::setOption (FDB_NET_OPTION_DISABLE_CLIENT_BYPASS) ({}): {}", + args.disable_client_bypass, + err.what()); + return -1; + } + } + /* Network thread must be setup before doing anything */ logr.debug("network::setup()"); network::setup(); @@ -1005,6 +1015,7 @@ int initArguments(Arguments& args) { args.txnspec.ops[i][OP_COUNT] = 0; } args.client_threads_per_version = 0; + args.disable_client_bypass = 0; args.disable_ryw = 0; args.json_output_path[0] = '\0'; args.stats_export_path[0] = '\0'; @@ -1248,6 +1259,7 @@ int parseArguments(int argc, char* argv[], Arguments& args) { { "txntagging_prefix", required_argument, NULL, ARG_TXNTAGGINGPREFIX }, { "version", no_argument, NULL, ARG_VERSION }, { "client_threads_per_version", required_argument, NULL, ARG_CLIENT_THREADS_PER_VERSION }, + { "disable_client_bypass", no_argument, NULL, ARG_DISABLE_CLIENT_BYPASS }, { "disable_ryw", no_argument, NULL, ARG_DISABLE_RYW }, { "json_report", optional_argument, NULL, ARG_JSON_REPORT }, { "bg_file_path", required_argument, NULL, ARG_BG_FILE_PATH }, @@ -1446,6 +1458,9 @@ int parseArguments(int argc, char* argv[], Arguments& args) { case ARG_CLIENT_THREADS_PER_VERSION: args.client_threads_per_version = atoi(optarg); break; + case ARG_DISABLE_CLIENT_BYPASS: + args.disable_client_bypass = 1; + break; case ARG_DISABLE_RYW: args.disable_ryw = 1; break; diff --git a/bindings/c/test/mako/mako.hpp b/bindings/c/test/mako/mako.hpp index 4d1df16bec..85bb38ec19 100644 --- a/bindings/c/test/mako/mako.hpp +++ b/bindings/c/test/mako/mako.hpp @@ -75,6 +75,7 @@ enum ArgKind { ARG_STREAMING_MODE, ARG_DISABLE_RYW, ARG_CLIENT_THREADS_PER_VERSION, + ARG_DISABLE_CLIENT_BYPASS, ARG_JSON_REPORT, ARG_BG_FILE_PATH, // if blob granule files are stored locally, mako will read and materialize them if this is set ARG_EXPORT_PATH, @@ -169,6 +170,7 @@ struct Arguments { char txntagging_prefix[TAGPREFIXLENGTH_MAX]; FDBStreamingMode streaming_mode; int64_t client_threads_per_version; + int disable_client_bypass; int disable_ryw; char json_output_path[PATH_MAX]; bool bg_materialize_files; From b7043154655df3af2914aa6f303dadab4d7710e3 Mon Sep 17 00:00:00 2001 From: Josh Slocum Date: Mon, 10 Oct 2022 09:30:16 -0500 Subject: [PATCH 3/8] fixing parameter to be bool --- bindings/c/test/mako/mako.cpp | 10 +++++----- bindings/c/test/mako/mako.hpp | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/bindings/c/test/mako/mako.cpp b/bindings/c/test/mako/mako.cpp index 0cc82e3d8a..becf107fc7 100644 --- a/bindings/c/test/mako/mako.cpp +++ b/bindings/c/test/mako/mako.cpp @@ -875,10 +875,10 @@ int workerProcessMain(Arguments const& args, int worker_id, shared_memory::Acces } } - if (args.disable_client_bypass > 0) { - err = network::setOptionNothrow(FDB_NET_OPTION_DISABLE_CLIENT_BYPASS, args.disable_client_bypass); + if (args.disable_client_bypass) { + err = network::setOptionNothrow(FDB_NET_OPTION_DISABLE_CLIENT_BYPASS); if (err) { - logr.error("network::setOption (FDB_NET_OPTION_DISABLE_CLIENT_BYPASS) ({}): {}", + logr.error("network::setOption (FDB_NET_OPTION_DISABLE_CLIENT_BYPASS): {}", args.disable_client_bypass, err.what()); return -1; @@ -1015,7 +1015,7 @@ int initArguments(Arguments& args) { args.txnspec.ops[i][OP_COUNT] = 0; } args.client_threads_per_version = 0; - args.disable_client_bypass = 0; + args.disable_client_bypass = false; args.disable_ryw = 0; args.json_output_path[0] = '\0'; args.stats_export_path[0] = '\0'; @@ -1459,7 +1459,7 @@ int parseArguments(int argc, char* argv[], Arguments& args) { args.client_threads_per_version = atoi(optarg); break; case ARG_DISABLE_CLIENT_BYPASS: - args.disable_client_bypass = 1; + args.disable_client_bypass = true; break; case ARG_DISABLE_RYW: args.disable_ryw = 1; diff --git a/bindings/c/test/mako/mako.hpp b/bindings/c/test/mako/mako.hpp index 85bb38ec19..bafe65b546 100644 --- a/bindings/c/test/mako/mako.hpp +++ b/bindings/c/test/mako/mako.hpp @@ -170,7 +170,7 @@ struct Arguments { char txntagging_prefix[TAGPREFIXLENGTH_MAX]; FDBStreamingMode streaming_mode; int64_t client_threads_per_version; - int disable_client_bypass; + bool disable_client_bypass; int disable_ryw; char json_output_path[PATH_MAX]; bool bg_materialize_files; From 55f091a139ef7cbb4731399295ab46f2e9ba94f8 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Mon, 10 Oct 2022 16:43:23 -0700 Subject: [PATCH 4/8] Disable shard_encode_location_metadata for downgrade to 7.1 Using the feature to set knobs from test spec files appears to require using toml, so also rewrite CycleTestRestart-{1,2}.txt to CycleTestRestart-{1,2}.toml --- tests/CMakeLists.txt | 4 +- ...onfigureStorageMigrationTestRestart-1.toml | 4 ++ .../to_7.1.0/CycleTestRestart-1.toml | 49 +++++++++++++++++++ .../to_7.1.0/CycleTestRestart-1.txt | 36 -------------- .../to_7.1.0/CycleTestRestart-2.toml | 37 ++++++++++++++ .../to_7.1.0/CycleTestRestart-2.txt | 28 ----------- 6 files changed, 92 insertions(+), 66 deletions(-) create mode 100644 tests/restarting/to_7.1.0/CycleTestRestart-1.toml delete mode 100644 tests/restarting/to_7.1.0/CycleTestRestart-1.txt create mode 100644 tests/restarting/to_7.1.0/CycleTestRestart-2.toml delete mode 100644 tests/restarting/to_7.1.0/CycleTestRestart-2.txt diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a2bbcb8a4e..e09cd6af9c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -277,8 +277,8 @@ if(WITH_PYTHON) TEST_FILES restarting/from_7.0.0/UpgradeAndBackupRestore-1.toml restarting/from_7.0.0/UpgradeAndBackupRestore-2.toml) add_fdb_test( - TEST_FILES restarting/to_7.1.0/CycleTestRestart-1.txt - restarting/to_7.1.0/CycleTestRestart-2.txt) + TEST_FILES restarting/to_7.1.0/CycleTestRestart-1.toml + restarting/to_7.1.0/CycleTestRestart-2.toml) add_fdb_test( TEST_FILES restarting/from_7.1.0/SnapTestAttrition-1.txt restarting/from_7.1.0/SnapTestAttrition-2.txt) diff --git a/tests/restarting/to_7.1.0/ConfigureStorageMigrationTestRestart-1.toml b/tests/restarting/to_7.1.0/ConfigureStorageMigrationTestRestart-1.toml index 07640495a0..ec0fc7c6e8 100644 --- a/tests/restarting/to_7.1.0/ConfigureStorageMigrationTestRestart-1.toml +++ b/tests/restarting/to_7.1.0/ConfigureStorageMigrationTestRestart-1.toml @@ -5,6 +5,10 @@ disableHostname=true disableEncryption=true storageEngineExcludeTypes=[3,4] +[[knobs]] +# This can be removed once the lower bound of this downgrade test is a version that understands the new protocol +shard_encode_location_metadata = false + [[test]] testTitle = 'CloggedConfigureDatabaseTest' clearAfterTest = false diff --git a/tests/restarting/to_7.1.0/CycleTestRestart-1.toml b/tests/restarting/to_7.1.0/CycleTestRestart-1.toml new file mode 100644 index 0000000000..e4e5be232c --- /dev/null +++ b/tests/restarting/to_7.1.0/CycleTestRestart-1.toml @@ -0,0 +1,49 @@ +[configuration] +storageEngineExcludeTypes = [-1,-2,3] +maxTLogVersion = 6 +disableTss = true +disableHostname = true +disableEncryption = true + +[[knobs]] +# This can be removed once the lower bound of this downgrade test is a version that understands the new protocol +shard_encode_location_metadata = false + +[[test]] +testTitle = 'Clogged' +clearAfterTest = false + + [[test.workload]] + testName = 'Cycle' + transactionsPerSecond = 500.0 + nodeCount = 2500 + testDuration = 10.0 + expectedRate = 0 + + [[test.workload]] + testName = 'RandomClogging' + testDuration = 10.0 + + [[test.workload]] + testName = 'Rollback' + meanDelay = 10.0 + testDuration = 10.0 + + [[test.workload]] + testName = 'Attrition' + machinesToKill = 10 + machinesToLeave = 3 + reboot = true + testDuration = 10.0 + + [[test.workload]] + testName = 'Attrition' + machinesToKill = 10 + machinesToLeave = 3 + reboot = true + testDuration = 10.0 + + [[test.workload]] + testName = 'SaveAndKill' + restartInfoLocation = 'simfdb/restartInfo.ini' + testDuration = 10.0 diff --git a/tests/restarting/to_7.1.0/CycleTestRestart-1.txt b/tests/restarting/to_7.1.0/CycleTestRestart-1.txt deleted file mode 100644 index e2aeaa4291..0000000000 --- a/tests/restarting/to_7.1.0/CycleTestRestart-1.txt +++ /dev/null @@ -1,36 +0,0 @@ -storageEngineExcludeTypes=-1,-2,3 -maxTLogVersion=6 -disableTss=true -disableHostname=true -disableEncryption=true - -testTitle=Clogged - clearAfterTest=false - testName=Cycle - transactionsPerSecond=500.0 - nodeCount=2500 - testDuration=10.0 - expectedRate=0 - - testName=RandomClogging - testDuration=10.0 - - testName=Rollback - meanDelay=10.0 - testDuration=10.0 - - testName=Attrition - machinesToKill=10 - machinesToLeave=3 - reboot=true - testDuration=10.0 - - testName=Attrition - machinesToKill=10 - machinesToLeave=3 - reboot=true - testDuration=10.0 - - testName=SaveAndKill - restartInfoLocation=simfdb/restartInfo.ini - testDuration=10.0 diff --git a/tests/restarting/to_7.1.0/CycleTestRestart-2.toml b/tests/restarting/to_7.1.0/CycleTestRestart-2.toml new file mode 100644 index 0000000000..51c6422304 --- /dev/null +++ b/tests/restarting/to_7.1.0/CycleTestRestart-2.toml @@ -0,0 +1,37 @@ +[configuration] +storageEngineExcludeTypes = [-1,-2] +maxTLogVersion = 6 +disableTss = true + +[[test]] +testTitle = 'Clogged' +runSetup = false + + [[test.workload]] + testName = 'Cycle' + transactionsPerSecond = 2500.0 + nodeCount = 2500 + testDuration = 10.0 + expectedRate = 0 + + [[test.workload]] + testName = 'RandomClogging' + testDuration = 10.0 + + [[test.workload]] + testName = 'Rollback' + meanDelay = 10.0 + testDuration = 10.0 + + [[test.workload]] + testName = 'Attrition' + machinesToKill = 10 + machinesToLeave = 3 + reboot = true + testDuration = 10.0 + + [[test.workload]] + testName = 'Attrition' + machinesToKill = 10 + machinesToLeave = 3 + reboot = true diff --git a/tests/restarting/to_7.1.0/CycleTestRestart-2.txt b/tests/restarting/to_7.1.0/CycleTestRestart-2.txt deleted file mode 100644 index f11cf32d76..0000000000 --- a/tests/restarting/to_7.1.0/CycleTestRestart-2.txt +++ /dev/null @@ -1,28 +0,0 @@ -storageEngineExcludeTypes=-1,-2 -maxTLogVersion=6 -disableTss=true -testTitle=Clogged - runSetup=false - testName=Cycle - transactionsPerSecond=2500.0 - nodeCount=2500 - testDuration=10.0 - expectedRate=0 - - testName=RandomClogging - testDuration=10.0 - - testName=Rollback - meanDelay=10.0 - testDuration=10.0 - - testName=Attrition - machinesToKill=10 - machinesToLeave=3 - reboot=true - testDuration=10.0 - - testName=Attrition - machinesToKill=10 - machinesToLeave=3 - reboot=true \ No newline at end of file From 693f015d69b8f3a2339fac05ab5891010602ae12 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Tue, 11 Oct 2022 08:42:49 -0700 Subject: [PATCH 5/8] Remove bogus storage engine exclude types Originally, storageEngineExcludeTypes was storageEngineExcludeType, and -1 signified "don't ignore any storage engines". This is no longer meaningful now that it's a list. --- tests/restarting/to_7.1.0/CycleTestRestart-1.toml | 2 +- tests/restarting/to_7.1.0/CycleTestRestart-2.toml | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/restarting/to_7.1.0/CycleTestRestart-1.toml b/tests/restarting/to_7.1.0/CycleTestRestart-1.toml index e4e5be232c..d74ea9ab5d 100644 --- a/tests/restarting/to_7.1.0/CycleTestRestart-1.toml +++ b/tests/restarting/to_7.1.0/CycleTestRestart-1.toml @@ -1,5 +1,5 @@ [configuration] -storageEngineExcludeTypes = [-1,-2,3] +storageEngineExcludeTypes = [3] maxTLogVersion = 6 disableTss = true disableHostname = true diff --git a/tests/restarting/to_7.1.0/CycleTestRestart-2.toml b/tests/restarting/to_7.1.0/CycleTestRestart-2.toml index 51c6422304..05571e0606 100644 --- a/tests/restarting/to_7.1.0/CycleTestRestart-2.toml +++ b/tests/restarting/to_7.1.0/CycleTestRestart-2.toml @@ -1,5 +1,4 @@ [configuration] -storageEngineExcludeTypes = [-1,-2] maxTLogVersion = 6 disableTss = true From 5411b687a60771f00d51779b7486858eb26a2479 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Tue, 11 Oct 2022 09:21:55 -0700 Subject: [PATCH 6/8] We need try_run to work even when linking libc++ statically --- cmake/ConfigureCompiler.cmake | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/cmake/ConfigureCompiler.cmake b/cmake/ConfigureCompiler.cmake index e38f333b58..cb442604d5 100644 --- a/cmake/ConfigureCompiler.cmake +++ b/cmake/ConfigureCompiler.cmake @@ -291,6 +291,19 @@ else() # for more information. #add_compile_options(-fno-builtin-memcpy) + if (USE_LIBCXX) + # Make sure that libc++ can be found be the platform's loader, so that thing's like cmake's "try_run" work. + find_library(LIBCXX_SO_PATH c++ /usr/local/lib) + if (LIBCXX_SO_PATH) + get_filename_component(LIBCXX_SO_DIR ${LIBCXX_SO_PATH} DIRECTORY) + if (APPLE) + set(ENV{DYLD_LIBRARY_PATH} "$ENV{DYLD_LIBRARY_PATH}:${LIBCXX_SO_DIR}") + else() + set(ENV{LD_LIBRARY_PATH} "$ENV{LD_LIBRARY_PATH}:${LIBCXX_SO_DIR}") + endif() + endif() + endif() + if (CLANG OR ICX) if (APPLE OR USE_LIBCXX) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libc++") @@ -298,19 +311,6 @@ else() if (STATIC_LINK_LIBCXX) set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -static-libgcc -nostdlib++ -Wl,-Bstatic -lc++ -lc++abi -Wl,-Bdynamic") set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -static-libgcc -nostdlib++ -Wl,-Bstatic -lc++ -lc++abi -Wl,-Bdynamic") - else() - # Make sure that libc++ can be found be the platform's loader, so that thing's like cmake's "try_run" work. - find_library(LIBCXX_SO_PATH c++ /usr/local/lib) - if (LIBCXX_SO_PATH) - get_filename_component(LIBCXX_SO_DIR ${LIBCXX_SO_PATH} DIRECTORY) - if (APPLE) - set(ENV{DYLD_LIBRARY_PATH} "$ENV{DYLD_LIBRARY_PATH}:${LIBCXX_SO_DIR}") - elseif(WIN32) - set(ENV{PATH} "$ENV{PATH};${LIBCXX_SO_DIR}") - else() - set(ENV{LD_LIBRARY_PATH} "$ENV{LD_LIBRARY_PATH}:${LIBCXX_SO_DIR}") - endif() - endif() endif() set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -stdlib=libc++ -Wl,-build-id=sha1") set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -stdlib=libc++ -Wl,-build-id=sha1") From 86c732fc0e8bd72333c558951a5f8f1dd3abc3b5 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Tue, 11 Oct 2022 11:19:28 -0700 Subject: [PATCH 7/8] Prevent changing the value of ProcessClass enums ConsistencyScanClass was added in the middle of the enum, which changed the values for all subsequent enums. Correct this and prevent it from happening again. --- fdbrpc/include/fdbrpc/Locality.h | 34 +++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/fdbrpc/include/fdbrpc/Locality.h b/fdbrpc/include/fdbrpc/Locality.h index 20269b6d05..8c98db88f0 100644 --- a/fdbrpc/include/fdbrpc/Locality.h +++ b/fdbrpc/include/fdbrpc/Locality.h @@ -43,16 +43,42 @@ struct ProcessClass { DataDistributorClass, CoordinatorClass, RatekeeperClass, - ConsistencyScanClass, StorageCacheClass, BackupClass, GrvProxyClass, BlobManagerClass, BlobWorkerClass, EncryptKeyProxyClass, + ConsistencyScanClass, InvalidClass = -1 }; + // class is serialized by enum value, so it's important not to change the + // enum value of a class. New classes should only be added to the end. + static_assert(ProcessClass::UnsetClass == 0); + static_assert(ProcessClass::StorageClass == 1); + static_assert(ProcessClass::TransactionClass == 2); + static_assert(ProcessClass::ResolutionClass == 3); + static_assert(ProcessClass::TesterClass == 4); + static_assert(ProcessClass::CommitProxyClass == 5); + static_assert(ProcessClass::MasterClass == 6); + static_assert(ProcessClass::StatelessClass == 7); + static_assert(ProcessClass::LogClass == 8); + static_assert(ProcessClass::ClusterControllerClass == 9); + static_assert(ProcessClass::LogRouterClass == 10); + static_assert(ProcessClass::FastRestoreClass == 11); + static_assert(ProcessClass::DataDistributorClass == 12); + static_assert(ProcessClass::CoordinatorClass == 13); + static_assert(ProcessClass::RatekeeperClass == 14); + static_assert(ProcessClass::StorageCacheClass == 15); + static_assert(ProcessClass::BackupClass == 16); + static_assert(ProcessClass::GrvProxyClass == 17); + static_assert(ProcessClass::BlobManagerClass == 18); + static_assert(ProcessClass::BlobWorkerClass == 19); + static_assert(ProcessClass::EncryptKeyProxyClass == 20); + static_assert(ProcessClass::ConsistencyScanClass == 21); + static_assert(ProcessClass::InvalidClass == -1); + enum Fitness { BestFit, GoodFit, @@ -86,6 +112,12 @@ struct ProcessClass { int16_t _class; int16_t _source; + // source is serialized by enum value, so it's important not to change the + // enum value of a source. New sources should only be added to the end. + static_assert(ProcessClass::CommandLineSource == 0); + static_assert(ProcessClass::AutoSource == 1); + static_assert(ProcessClass::DBSource == 2); + public: ProcessClass() : _class(UnsetClass), _source(CommandLineSource) {} ProcessClass(ClassType type, ClassSource source) : _class(type), _source(source) {} From 06d9ebd6206609874d24ca2629cea4175310faef Mon Sep 17 00:00:00 2001 From: Josh Slocum Date: Tue, 11 Oct 2022 14:31:14 -0500 Subject: [PATCH 8/8] Adding domain name to blob metadata requests (#8415) --- .../include/fdbclient/BlobMetadataUtils.h | 15 +++++-- .../fdbclient/EncryptKeyProxyInterface.h | 8 ++-- .../fdbclient/GetEncryptCipherKeys.actor.h | 2 +- fdbserver/BlobGranuleServerCommon.actor.cpp | 21 +++++----- fdbserver/EncryptKeyProxy.actor.cpp | 39 +++++++++---------- fdbserver/SimKmsConnector.actor.cpp | 17 +++++--- .../include/fdbserver/KmsConnectorInterface.h | 6 +-- .../workloads/EncryptKeyProxyTest.actor.cpp | 8 ++-- 8 files changed, 63 insertions(+), 53 deletions(-) diff --git a/fdbclient/include/fdbclient/BlobMetadataUtils.h b/fdbclient/include/fdbclient/BlobMetadataUtils.h index d9ed6dd897..4df5c7bfdb 100644 --- a/fdbclient/include/fdbclient/BlobMetadataUtils.h +++ b/fdbclient/include/fdbclient/BlobMetadataUtils.h @@ -25,6 +25,8 @@ #include "flow/FileIdentifier.h" using BlobMetadataDomainId = int64_t; +using BlobMetadataDomainNameRef = StringRef; +using BlobMetadataDomainName = Standalone; /* * There are 3 cases for blob metadata. @@ -38,26 +40,31 @@ using BlobMetadataDomainId = int64_t; struct BlobMetadataDetailsRef { constexpr static FileIdentifier file_identifier = 6685526; BlobMetadataDomainId domainId; + BlobMetadataDomainNameRef domainName; Optional base; VectorRef partitions; BlobMetadataDetailsRef() {} BlobMetadataDetailsRef(Arena& arena, const BlobMetadataDetailsRef& from) - : domainId(from.domainId), partitions(arena, from.partitions) { + : domainId(from.domainId), domainName(arena, from.domainName), partitions(arena, from.partitions) { if (from.base.present()) { base = StringRef(arena, from.base.get()); } } explicit BlobMetadataDetailsRef(BlobMetadataDomainId domainId, + BlobMetadataDomainNameRef domainName, Optional base, VectorRef partitions) - : domainId(domainId), base(base), partitions(partitions) {} + : domainId(domainId), domainName(domainName), base(base), partitions(partitions) {} - int expectedSize() const { return sizeof(BlobMetadataDetailsRef) + partitions.expectedSize(); } + int expectedSize() const { + return sizeof(BlobMetadataDetailsRef) + domainName.size() + (base.present() ? base.get().size() : 0) + + partitions.expectedSize(); + } template void serialize(Ar& ar) { - serializer(ar, domainId, base, partitions); + serializer(ar, domainId, domainName, base, partitions); } }; diff --git a/fdbclient/include/fdbclient/EncryptKeyProxyInterface.h b/fdbclient/include/fdbclient/EncryptKeyProxyInterface.h index 5f4d56eb96..e0c88649c0 100644 --- a/fdbclient/include/fdbclient/EncryptKeyProxyInterface.h +++ b/fdbclient/include/fdbclient/EncryptKeyProxyInterface.h @@ -197,6 +197,7 @@ struct EKPGetLatestBaseCipherKeysReply { } }; +// TODO: also used for blob metadata, fix name struct EKPGetLatestCipherKeysRequestInfo { constexpr static FileIdentifier file_identifier = 2180516; // Encryption domain identifier @@ -206,7 +207,7 @@ struct EKPGetLatestCipherKeysRequestInfo { EncryptCipherDomainNameRef domainName; EKPGetLatestCipherKeysRequestInfo() : domainId(INVALID_ENCRYPT_DOMAIN_ID) {} - EKPGetLatestCipherKeysRequestInfo(const EncryptCipherDomainId dId, StringRef name, Arena& arena) + explicit EKPGetLatestCipherKeysRequestInfo(Arena& arena, const EncryptCipherDomainId dId, StringRef name) : domainId(dId), domainName(StringRef(arena, name)) {} bool operator==(const EKPGetLatestCipherKeysRequestInfo& info) const { @@ -261,16 +262,15 @@ struct EKPGetLatestBlobMetadataReply { struct EKPGetLatestBlobMetadataRequest { constexpr static FileIdentifier file_identifier = 3821549; - std::vector domainIds; + Standalone> domainInfos; Optional debugId; ReplyPromise reply; EKPGetLatestBlobMetadataRequest() {} - explicit EKPGetLatestBlobMetadataRequest(const std::vector& ids) : domainIds(ids) {} template void serialize(Ar& ar) { - serializer(ar, domainIds, debugId, reply); + serializer(ar, domainInfos, debugId, reply); } }; diff --git a/fdbclient/include/fdbclient/GetEncryptCipherKeys.actor.h b/fdbclient/include/fdbclient/GetEncryptCipherKeys.actor.h index 0f93675a6a..bd5b252030 100644 --- a/fdbclient/include/fdbclient/GetEncryptCipherKeys.actor.h +++ b/fdbclient/include/fdbclient/GetEncryptCipherKeys.actor.h @@ -108,7 +108,7 @@ Future>> getL cipherKeys[domain.first] = cachedCipherKey; } else { request.encryptDomainInfos.emplace_back( - domain.first /*domainId*/, domain.second /*domainName*/, request.arena); + request.arena, domain.first /*domainId*/, domain.second /*domainName*/); } } if (request.encryptDomainInfos.empty()) { diff --git a/fdbserver/BlobGranuleServerCommon.actor.cpp b/fdbserver/BlobGranuleServerCommon.actor.cpp index 750eb1592d..d930f294fe 100644 --- a/fdbserver/BlobGranuleServerCommon.actor.cpp +++ b/fdbserver/BlobGranuleServerCommon.actor.cpp @@ -451,12 +451,14 @@ TEST_CASE("/blobgranule/server/common/granulesummary") { } // FIXME: if credentials can expire, refresh periodically -ACTOR Future loadBlobMetadataForTenants(BGTenantMap* self, std::vector tenantMapEntries) { +ACTOR Future loadBlobMetadataForTenants( + BGTenantMap* self, + std::vector> tenantsToLoad) { ASSERT(SERVER_KNOBS->BG_METADATA_SOURCE == "tenant"); - ASSERT(!tenantMapEntries.empty()); - state std::vector domainIds; - for (auto& entry : tenantMapEntries) { - domainIds.push_back(entry.id); + ASSERT(!tenantsToLoad.empty()); + state EKPGetLatestBlobMetadataRequest req; + for (auto& tenant : tenantsToLoad) { + req.domainInfos.emplace_back_deep(req.domainInfos.arena(), tenant.first, StringRef(tenant.second)); } // FIXME: if one tenant gets an error, don't kill whole process @@ -464,8 +466,7 @@ ACTOR Future loadBlobMetadataForTenants(BGTenantMap* self, std::vector requestFuture; if (self->dbInfo.isValid() && self->dbInfo->get().encryptKeyProxy.present()) { - EKPGetLatestBlobMetadataRequest req; - req.domainIds = domainIds; + req.reply.reset(); requestFuture = brokenPromiseToNever(self->dbInfo->get().encryptKeyProxy.get().getLatestBlobMetadata.getReply(req)); } else { @@ -473,7 +474,7 @@ ACTOR Future loadBlobMetadataForTenants(BGTenantMap* self, std::vectortenantInfoById.find(metadata.domainId); @@ -493,7 +494,7 @@ ACTOR Future loadBlobMetadataForTenants(BGTenantMap* self, std::vector> tenants) { - std::vector tenantsToLoad; + std::vector> tenantsToLoad; for (auto entry : tenants) { if (tenantInfoById.insert({ entry.second.id, entry.second }).second) { auto r = makeReference(entry.first, entry.second); @@ -501,7 +502,7 @@ void BGTenantMap::addTenants(std::vector> if (SERVER_KNOBS->BG_METADATA_SOURCE != "tenant") { r->bstoreLoaded.send(Void()); } else { - tenantsToLoad.push_back(entry.second); + tenantsToLoad.push_back({ entry.second.id, entry.first }); } } } diff --git a/fdbserver/EncryptKeyProxy.actor.cpp b/fdbserver/EncryptKeyProxy.actor.cpp index c1ed8bf2e2..f1e1fa1d19 100644 --- a/fdbserver/EncryptKeyProxy.actor.cpp +++ b/fdbserver/EncryptKeyProxy.actor.cpp @@ -690,44 +690,43 @@ ACTOR Future getLatestBlobMetadata(Reference ekpProxy } // Dedup the requested domainIds. - std::unordered_set dedupedDomainIds; - for (auto id : req.domainIds) { - dedupedDomainIds.emplace(id); + std::unordered_map dedupedDomainInfos; + for (auto info : req.domainInfos) { + dedupedDomainInfos.insert({ info.domainId, info.domainName }); } if (dbgTrace.present()) { - dbgTrace.get().detail("NKeys", dedupedDomainIds.size()); - for (BlobMetadataDomainId id : dedupedDomainIds) { + dbgTrace.get().detail("NKeys", dedupedDomainInfos.size()); + for (auto& info : dedupedDomainInfos) { // log domainids queried - dbgTrace.get().detail("BMQ" + std::to_string(id), ""); + dbgTrace.get().detail("BMQ" + std::to_string(info.first), ""); } } // First, check if the requested information is already cached by the server. // Ensure the cached information is within SERVER_KNOBS->BLOB_METADATA_CACHE_TTL time window. - std::vector lookupDomains; - for (BlobMetadataDomainId id : dedupedDomainIds) { - const auto itr = ekpProxyData->blobMetadataDomainIdCache.find(id); + state KmsConnBlobMetadataReq kmsReq; + kmsReq.debugId = req.debugId; + + for (auto& info : dedupedDomainInfos) { + const auto itr = ekpProxyData->blobMetadataDomainIdCache.find(info.first); if (itr != ekpProxyData->blobMetadataDomainIdCache.end() && itr->second.isValid()) { metadataDetails.arena().dependsOn(itr->second.metadataDetails.arena()); metadataDetails.push_back(metadataDetails.arena(), itr->second.metadataDetails); if (dbgTrace.present()) { - dbgTrace.get().detail("BMC" + std::to_string(id), ""); + dbgTrace.get().detail("BMC" + std::to_string(info.first), ""); } - ++ekpProxyData->blobMetadataCacheHits; } else { - lookupDomains.emplace_back(id); - ++ekpProxyData->blobMetadataCacheMisses; + kmsReq.domainInfos.emplace_back(kmsReq.domainInfos.arena(), info.first, info.second); } } - ekpProxyData->baseCipherDomainIdCacheHits += metadataDetails.size(); - ekpProxyData->baseCipherDomainIdCacheMisses += lookupDomains.size(); + ekpProxyData->blobMetadataCacheHits += metadataDetails.size(); - if (!lookupDomains.empty()) { + if (!kmsReq.domainInfos.empty()) { + ekpProxyData->blobMetadataCacheMisses += kmsReq.domainInfos.size(); try { - KmsConnBlobMetadataReq kmsReq(lookupDomains, req.debugId); state double startTime = now(); KmsConnBlobMetadataRep kmsRep = wait(kmsConnectorInf.blobMetadataReq.getReply(kmsReq)); ekpProxyData->kmsBlobMetadataReqLatency.addMeasurement(now() - startTime); @@ -755,7 +754,6 @@ ACTOR Future getLatestBlobMetadata(Reference ekpProxy } req.reply.send(EKPGetLatestBlobMetadataReply(metadataDetails)); - return Void(); } @@ -771,10 +769,11 @@ ACTOR Future refreshBlobMetadataCore(Reference ekpPro try { KmsConnBlobMetadataReq req; req.debugId = debugId; - req.domainIds.reserve(ekpProxyData->blobMetadataDomainIdCache.size()); + req.domainInfos.reserve(req.domainInfos.arena(), ekpProxyData->blobMetadataDomainIdCache.size()); + // TODO add refresh + expire timestamp and filter to only ones that need refreshing for (auto& item : ekpProxyData->blobMetadataDomainIdCache) { - req.domainIds.emplace_back(item.first); + req.domainInfos.emplace_back(req.domainInfos.arena(), item.first, item.second.metadataDetails.domainName); } state double startTime = now(); KmsConnBlobMetadataRep rep = wait(kmsConnectorInf.blobMetadataReq.getReply(req)); diff --git a/fdbserver/SimKmsConnector.actor.cpp b/fdbserver/SimKmsConnector.actor.cpp index da903870bd..dd84c0aeea 100644 --- a/fdbserver/SimKmsConnector.actor.cpp +++ b/fdbserver/SimKmsConnector.actor.cpp @@ -185,10 +185,13 @@ ACTOR Future ekLookupByDomainIds(Reference ctx, return Void(); } - -static Standalone createBlobMetadata(BlobMetadataDomainId domainId) { +// TODO: switch this to use bg_url instead of hardcoding file://fdbblob, so it works as FDBPerfKmsConnector +// FIXME: make this (more) deterministic outside of simulation for FDBPerfKmsConnector +static Standalone createBlobMetadata(BlobMetadataDomainId domainId, + BlobMetadataDomainName domainName) { Standalone metadata; metadata.domainId = domainId; + metadata.domainName = domainName; // 0 == no partition, 1 == suffix partitioned, 2 == storage location partitioned int type = deterministicRandom()->randomInt(0, 3); int partitionCount = (type == 0) ? 0 : deterministicRandom()->randomInt(2, 12); @@ -234,17 +237,19 @@ ACTOR Future blobMetadataLookup(KmsConnectorInterface interf, KmsConnBlobM dbgDIdTrace.get().detail("DbgId", req.debugId.get()); } - for (BlobMetadataDomainId domainId : req.domainIds) { - auto it = simBlobMetadataStore.find(domainId); + for (auto const& domainInfo : req.domainInfos) { + auto it = simBlobMetadataStore.find(domainInfo.domainId); if (it == simBlobMetadataStore.end()) { // construct new blob metadata - it = simBlobMetadataStore.insert({ domainId, createBlobMetadata(domainId) }).first; + it = simBlobMetadataStore + .insert({ domainInfo.domainId, createBlobMetadata(domainInfo.domainId, domainInfo.domainName) }) + .first; } rep.metadataDetails.arena().dependsOn(it->second.arena()); rep.metadataDetails.push_back(rep.metadataDetails.arena(), it->second); } - wait(delayJittered(1.0)); // simulate network delay + wait(delay(deterministicRandom()->random01())); // simulate network delay req.reply.send(rep); diff --git a/fdbserver/include/fdbserver/KmsConnectorInterface.h b/fdbserver/include/fdbserver/KmsConnectorInterface.h index ac31bc5d37..b418eb99ba 100644 --- a/fdbserver/include/fdbserver/KmsConnectorInterface.h +++ b/fdbserver/include/fdbserver/KmsConnectorInterface.h @@ -232,17 +232,15 @@ struct KmsConnBlobMetadataRep { struct KmsConnBlobMetadataReq { constexpr static FileIdentifier file_identifier = 3913147; - std::vector domainIds; + Standalone> domainInfos; Optional debugId; ReplyPromise reply; KmsConnBlobMetadataReq() {} - explicit KmsConnBlobMetadataReq(const std::vector& ids, Optional dbgId) - : domainIds(ids), debugId(dbgId) {} template void serialize(Ar& ar) { - serializer(ar, domainIds, debugId, reply); + serializer(ar, domainInfos, debugId, reply); } }; diff --git a/fdbserver/workloads/EncryptKeyProxyTest.actor.cpp b/fdbserver/workloads/EncryptKeyProxyTest.actor.cpp index c92a9c9c34..970b3162bf 100644 --- a/fdbserver/workloads/EncryptKeyProxyTest.actor.cpp +++ b/fdbserver/workloads/EncryptKeyProxyTest.actor.cpp @@ -71,7 +71,7 @@ struct EncryptKeyProxyTestWorkload : TestWorkload { for (int i = 0; i < self->numDomains / 2; i++) { const EncryptCipherDomainId domainId = self->minDomainId + i; self->domainInfos.emplace_back( - EKPGetLatestCipherKeysRequestInfo(domainId, StringRef(std::to_string(domainId)), self->arena)); + EKPGetLatestCipherKeysRequestInfo(self->arena, domainId, StringRef(std::to_string(domainId)))); } state int nAttempts = 0; @@ -127,14 +127,14 @@ struct EncryptKeyProxyTestWorkload : TestWorkload { for (int i = 0; i < expectedHits; i++) { const EncryptCipherDomainId domainId = self->minDomainId + i; self->domainInfos.emplace_back( - EKPGetLatestCipherKeysRequestInfo(domainId, StringRef(std::to_string(domainId)), self->arena)); + EKPGetLatestCipherKeysRequestInfo(self->arena, domainId, StringRef(std::to_string(domainId)))); } expectedMisses = deterministicRandom()->randomInt(1, self->numDomains / 2); for (int i = 0; i < expectedMisses; i++) { const EncryptCipherDomainId domainId = self->minDomainId + i + self->numDomains / 2 + 1; self->domainInfos.emplace_back( - EKPGetLatestCipherKeysRequestInfo(domainId, StringRef(std::to_string(domainId)), self->arena)); + EKPGetLatestCipherKeysRequestInfo(self->arena, domainId, StringRef(std::to_string(domainId)))); } state int nAttempts = 0; @@ -191,7 +191,7 @@ struct EncryptKeyProxyTestWorkload : TestWorkload { for (int i = 0; i < self->numDomains; i++) { const EncryptCipherDomainId domainId = self->minDomainId + i; self->domainInfos.emplace_back( - EKPGetLatestCipherKeysRequestInfo(domainId, StringRef(std::to_string(domainId)), self->arena)); + EKPGetLatestCipherKeysRequestInfo(self->arena, domainId, StringRef(std::to_string(domainId)))); } EKPGetLatestBaseCipherKeysRequest req;