From ad03a4787a5ed85e8f8c0459ddd0461fe7db3f2a Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Fri, 9 Jul 2021 21:06:15 -0700 Subject: [PATCH 01/10] Fix non-TLS build --- fdbclient/BackupContainerAzureBlobStore.actor.cpp | 4 ++++ fdbclient/BackupContainerFileSystem.actor.cpp | 10 ++++++++++ fdbclient/BackupContainerS3BlobStore.actor.cpp | 4 ++++ fdbrpc/AsyncFileEncrypted.h | 4 ++++ fdbrpc/CMakeLists.txt | 2 +- fdbrpc/Net2FileSystem.cpp | 2 -- fdbrpc/sim2.actor.cpp | 2 -- flow/CMakeLists.txt | 4 ++-- flow/StreamCipher.h | 4 ++++ 9 files changed, 29 insertions(+), 7 deletions(-) diff --git a/fdbclient/BackupContainerAzureBlobStore.actor.cpp b/fdbclient/BackupContainerAzureBlobStore.actor.cpp index 4ee3a7ebf5..39c001a198 100644 --- a/fdbclient/BackupContainerAzureBlobStore.actor.cpp +++ b/fdbclient/BackupContainerAzureBlobStore.actor.cpp @@ -170,9 +170,11 @@ public: } Reference f = makeReference(self->asyncTaskThread, self->containerName, fileName, self->client.get()); +#if (!defined(TLS_DISABLED) && !defined(_WIN32)) if (self->usesEncryption()) { f = makeReference(f, false); } +#endif return f; } @@ -183,9 +185,11 @@ public: return Void(); })); auto f = makeReference(self->asyncTaskThread, self->containerName, fileName, self->client.get()); +#if (!defined(TLS_DISABLED) && !defined(_WIN32)) if (self->usesEncryption()) { f = makeReference(f, true); } +#endif return makeReference(fileName, f); } diff --git a/fdbclient/BackupContainerFileSystem.actor.cpp b/fdbclient/BackupContainerFileSystem.actor.cpp index a4eb1f6e34..051d6ec2ea 100644 --- a/fdbclient/BackupContainerFileSystem.actor.cpp +++ b/fdbclient/BackupContainerFileSystem.actor.cpp @@ -1127,6 +1127,7 @@ public: return false; } +#if (!defined(TLS_DISABLED) && !defined(_WIN32)) ACTOR static Future createTestEncryptionKeyFile(std::string filename) { state Reference keyFile = wait(IAsyncFileSystem::filesystem()->open( filename, @@ -1163,6 +1164,7 @@ public: StreamCipher::Key::initializeKey(std::move(key)); return Void(); } +#endif // encryption enabled }; // class BackupContainerFileSystemImpl Future> BackupContainerFileSystem::writeLogFile(Version beginVersion, @@ -1477,11 +1479,19 @@ Future BackupContainerFileSystem::encryptionSetupComplete() const { } void BackupContainerFileSystem::setEncryptionKey(Optional const& encryptionKeyFileName) { if (encryptionKeyFileName.present()) { +#if (!defined(TLS_DISABLED) && !defined(_WIN32)) encryptionSetupFuture = BackupContainerFileSystemImpl::readEncryptionKey(encryptionKeyFileName.get()); +#else + encryptionSetupFuture = Void(); +#endif } } Future BackupContainerFileSystem::createTestEncryptionKeyFile(std::string const &filename) { +#if (!defined(TLS_DISABLED) && !defined(_WIN32)) return BackupContainerFileSystemImpl::createTestEncryptionKeyFile(filename); +#else + return Void(); +#endif } namespace backup_test { diff --git a/fdbclient/BackupContainerS3BlobStore.actor.cpp b/fdbclient/BackupContainerS3BlobStore.actor.cpp index 02112c2f58..0dc526e953 100644 --- a/fdbclient/BackupContainerS3BlobStore.actor.cpp +++ b/fdbclient/BackupContainerS3BlobStore.actor.cpp @@ -171,9 +171,11 @@ std::string BackupContainerS3BlobStore::getURLFormat() { Future> BackupContainerS3BlobStore::readFile(const std::string& path) { Reference f = makeReference(m_bstore, m_bucket, dataPath(path)); +#if (!defined(TLS_DISABLED) && !defined(_WIN32)) if (usesEncryption()) { f = makeReference(f, AsyncFileEncrypted::Mode::READ_ONLY); } +#endif f = makeReference(f, m_bstore->knobs.read_block_size, m_bstore->knobs.read_ahead_blocks, @@ -189,9 +191,11 @@ Future> BackupContainerS3BlobStore::listURLs(Reference< Future> BackupContainerS3BlobStore::writeFile(const std::string& path) { Reference f = makeReference(m_bstore, m_bucket, dataPath(path)); +#if (!defined(TLS_DISABLED) && !defined(_WIN32)) if (usesEncryption()) { f = makeReference(f, AsyncFileEncrypted::Mode::APPEND_ONLY); } +#endif return Future>(makeReference(path, f)); } diff --git a/fdbrpc/AsyncFileEncrypted.h b/fdbrpc/AsyncFileEncrypted.h index ed5693de29..76d6d53222 100644 --- a/fdbrpc/AsyncFileEncrypted.h +++ b/fdbrpc/AsyncFileEncrypted.h @@ -20,6 +20,8 @@ #pragma once +#if (!defined(TLS_DISABLED) && !defined(_WIN32)) + #include "fdbrpc/IAsyncFile.h" #include "flow/FastRef.h" #include "flow/flow.h" @@ -79,3 +81,5 @@ public: void releaseZeroCopy(void* data, int length, int64_t offset) override; int64_t debugFD() const override; }; + +#endif // Encryption enabled diff --git a/fdbrpc/CMakeLists.txt b/fdbrpc/CMakeLists.txt index 026ca36972..bfecc781f1 100644 --- a/fdbrpc/CMakeLists.txt +++ b/fdbrpc/CMakeLists.txt @@ -1,6 +1,7 @@ set(FDBRPC_SRCS AsyncFileCached.actor.h AsyncFileEIO.actor.h + AsyncFileEncrypted.h AsyncFileKAIO.actor.h AsyncFileNonDurable.actor.h AsyncFileReadAhead.actor.h @@ -36,7 +37,6 @@ set(FDBRPC_SRCS if(WITH_TLS AND NOT WIN32) set(FDBRPC_SRCS ${FDBRPC_SRCS} - AsyncFileEncrypted.h AsyncFileEncrypted.actor.cpp) endif() diff --git a/fdbrpc/Net2FileSystem.cpp b/fdbrpc/Net2FileSystem.cpp index a2a8874bed..908eb072be 100644 --- a/fdbrpc/Net2FileSystem.cpp +++ b/fdbrpc/Net2FileSystem.cpp @@ -32,9 +32,7 @@ #include "fdbrpc/AsyncFileCached.actor.h" #include "fdbrpc/AsyncFileEIO.actor.h" -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) #include "fdbrpc/AsyncFileEncrypted.h" -#endif #include "fdbrpc/AsyncFileWinASIO.actor.h" #include "fdbrpc/AsyncFileKAIO.actor.h" #include "flow/AsioReactor.h" diff --git a/fdbrpc/sim2.actor.cpp b/fdbrpc/sim2.actor.cpp index 9243bda4a1..40e576faab 100644 --- a/fdbrpc/sim2.actor.cpp +++ b/fdbrpc/sim2.actor.cpp @@ -33,9 +33,7 @@ #include "flow/Util.h" #include "fdbrpc/IAsyncFile.h" #include "fdbrpc/AsyncFileCached.actor.h" -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) #include "fdbrpc/AsyncFileEncrypted.h" -#endif #include "fdbrpc/AsyncFileNonDurable.actor.h" #include "flow/crc32c.h" #include "fdbrpc/TraceFileIO.h" diff --git a/flow/CMakeLists.txt b/flow/CMakeLists.txt index bc8763f35f..78d097517f 100644 --- a/flow/CMakeLists.txt +++ b/flow/CMakeLists.txt @@ -53,6 +53,7 @@ set(FLOW_SRCS SignalSafeUnwind.cpp SignalSafeUnwind.h SimpleOpt.h + StreamCipher.h SystemMonitor.cpp SystemMonitor.h TDMetric.actor.h @@ -100,8 +101,7 @@ set(FLOW_SRCS if(WITH_TLS AND NOT WIN32) set(FLOW_SRCS ${FLOW_SRCS} - StreamCipher.cpp - StreamCipher.h) + StreamCipher.cpp) endif() add_library(stacktrace stacktrace.amalgamation.cpp stacktrace.h) diff --git a/flow/StreamCipher.h b/flow/StreamCipher.h index 57c2e0e436..ed85c0bc07 100644 --- a/flow/StreamCipher.h +++ b/flow/StreamCipher.h @@ -20,6 +20,8 @@ #pragma once +#if (!defined(TLS_DISABLED) && !defined(_WIN32)) + #include "flow/Arena.h" #include "flow/FastRef.h" #include "flow/flow.h" @@ -78,3 +80,5 @@ public: StringRef decrypt(unsigned char const* ciphertext, int len, Arena&); StringRef finish(Arena&); }; + +#endif // encryption enabled From 41b4ace19a70bf9a33fc7a81e19e47a9fac9dec2 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Fri, 9 Jul 2021 21:20:40 -0700 Subject: [PATCH 02/10] Added ENCRYPTION_ENABLED macro --- fdbclient/BackupContainerAzureBlobStore.actor.cpp | 4 ++-- fdbclient/BackupContainerFileSystem.actor.cpp | 9 +++++---- fdbclient/BackupContainerS3BlobStore.actor.cpp | 4 ++-- fdbrpc/AsyncFileEncrypted.h | 4 ++-- flow/StreamCipher.h | 8 +++++++- 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/fdbclient/BackupContainerAzureBlobStore.actor.cpp b/fdbclient/BackupContainerAzureBlobStore.actor.cpp index 39c001a198..5b32d9a9f7 100644 --- a/fdbclient/BackupContainerAzureBlobStore.actor.cpp +++ b/fdbclient/BackupContainerAzureBlobStore.actor.cpp @@ -170,7 +170,7 @@ public: } Reference f = makeReference(self->asyncTaskThread, self->containerName, fileName, self->client.get()); -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#if ENCRYPTION_ENABLED if (self->usesEncryption()) { f = makeReference(f, false); } @@ -185,7 +185,7 @@ public: return Void(); })); auto f = makeReference(self->asyncTaskThread, self->containerName, fileName, self->client.get()); -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#if ENCRYPTED_ENABLED if (self->usesEncryption()) { f = makeReference(f, true); } diff --git a/fdbclient/BackupContainerFileSystem.actor.cpp b/fdbclient/BackupContainerFileSystem.actor.cpp index 051d6ec2ea..5c6783201e 100644 --- a/fdbclient/BackupContainerFileSystem.actor.cpp +++ b/fdbclient/BackupContainerFileSystem.actor.cpp @@ -1127,7 +1127,7 @@ public: return false; } -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#if ENCRYPTION_ENABLED ACTOR static Future createTestEncryptionKeyFile(std::string filename) { state Reference keyFile = wait(IAsyncFileSystem::filesystem()->open( filename, @@ -1164,7 +1164,8 @@ public: StreamCipher::Key::initializeKey(std::move(key)); return Void(); } -#endif // encryption enabled +#endif // ENCRYPTION_ENABLED + }; // class BackupContainerFileSystemImpl Future> BackupContainerFileSystem::writeLogFile(Version beginVersion, @@ -1479,7 +1480,7 @@ Future BackupContainerFileSystem::encryptionSetupComplete() const { } void BackupContainerFileSystem::setEncryptionKey(Optional const& encryptionKeyFileName) { if (encryptionKeyFileName.present()) { -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#if ENCRYPTION_ENABLED encryptionSetupFuture = BackupContainerFileSystemImpl::readEncryptionKey(encryptionKeyFileName.get()); #else encryptionSetupFuture = Void(); @@ -1487,7 +1488,7 @@ void BackupContainerFileSystem::setEncryptionKey(Optional const& en } } Future BackupContainerFileSystem::createTestEncryptionKeyFile(std::string const &filename) { -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#if ENCRYPTION_ENABLED return BackupContainerFileSystemImpl::createTestEncryptionKeyFile(filename); #else return Void(); diff --git a/fdbclient/BackupContainerS3BlobStore.actor.cpp b/fdbclient/BackupContainerS3BlobStore.actor.cpp index 0dc526e953..f95e683768 100644 --- a/fdbclient/BackupContainerS3BlobStore.actor.cpp +++ b/fdbclient/BackupContainerS3BlobStore.actor.cpp @@ -171,7 +171,7 @@ std::string BackupContainerS3BlobStore::getURLFormat() { Future> BackupContainerS3BlobStore::readFile(const std::string& path) { Reference f = makeReference(m_bstore, m_bucket, dataPath(path)); -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#if ENCRYPTED_ENABLED if (usesEncryption()) { f = makeReference(f, AsyncFileEncrypted::Mode::READ_ONLY); } @@ -191,7 +191,7 @@ Future> BackupContainerS3BlobStore::listURLs(Reference< Future> BackupContainerS3BlobStore::writeFile(const std::string& path) { Reference f = makeReference(m_bstore, m_bucket, dataPath(path)); -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#if ENCRYPTION_ENABLED if (usesEncryption()) { f = makeReference(f, AsyncFileEncrypted::Mode::APPEND_ONLY); } diff --git a/fdbrpc/AsyncFileEncrypted.h b/fdbrpc/AsyncFileEncrypted.h index 76d6d53222..a31673b829 100644 --- a/fdbrpc/AsyncFileEncrypted.h +++ b/fdbrpc/AsyncFileEncrypted.h @@ -20,7 +20,7 @@ #pragma once -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#if ENCRYPTION_ENABLED #include "fdbrpc/IAsyncFile.h" #include "flow/FastRef.h" @@ -82,4 +82,4 @@ public: int64_t debugFD() const override; }; -#endif // Encryption enabled +#endif // ENCRYPTION_ENABLED diff --git a/flow/StreamCipher.h b/flow/StreamCipher.h index ed85c0bc07..8c35d99da5 100644 --- a/flow/StreamCipher.h +++ b/flow/StreamCipher.h @@ -21,6 +21,12 @@ #pragma once #if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#define ENCRYPTION_ENABLED 1 +#else +#define ENCRYPTION_ENABLED 0 +#endif + +#if ENCRYPTION_ENABLED #include "flow/Arena.h" #include "flow/FastRef.h" @@ -81,4 +87,4 @@ public: StringRef finish(Arena&); }; -#endif // encryption enabled +#endif // ENCRYPTION_ENABLED From 57689e83d3c6fec0e38abaa3ff117c4879fe9f53 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Fri, 9 Jul 2021 21:33:33 -0700 Subject: [PATCH 03/10] Move ENCRYPTION_ENABLED guard in AsyncFileEncrypted.h --- fdbrpc/AsyncFileEncrypted.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fdbrpc/AsyncFileEncrypted.h b/fdbrpc/AsyncFileEncrypted.h index a31673b829..0d1d407a3d 100644 --- a/fdbrpc/AsyncFileEncrypted.h +++ b/fdbrpc/AsyncFileEncrypted.h @@ -20,14 +20,14 @@ #pragma once -#if ENCRYPTION_ENABLED - #include "fdbrpc/IAsyncFile.h" #include "flow/FastRef.h" #include "flow/flow.h" #include "flow/IRandom.h" #include "flow/StreamCipher.h" +#if ENCRYPTION_ENABLED + #include /* From 17fce0596cc3257d28b02f2bd120e8761e8175d0 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Fri, 9 Jul 2021 21:42:42 -0700 Subject: [PATCH 04/10] Expand use of ENCRYPTION_ENABLED macro --- fdbrpc/Net2FileSystem.cpp | 4 ++-- fdbrpc/sim2.actor.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/fdbrpc/Net2FileSystem.cpp b/fdbrpc/Net2FileSystem.cpp index 908eb072be..56eb336cd6 100644 --- a/fdbrpc/Net2FileSystem.cpp +++ b/fdbrpc/Net2FileSystem.cpp @@ -77,14 +77,14 @@ Future> Net2FileSystem::open(const std::string& file static_cast((void*)g_network->global(INetwork::enASIOService))); if (FLOW_KNOBS->PAGE_WRITE_CHECKSUM_HISTORY > 0) f = map(f, [=](Reference r) { return Reference(new AsyncFileWriteChecker(r)); }); -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#if ENCRYPTION_ENABLED if (flags & IAsyncFile::OPEN_ENCRYPTED) f = map(f, [flags](Reference r) { auto mode = flags & IAsyncFile::OPEN_READWRITE ? AsyncFileEncrypted::Mode::APPEND_ONLY : AsyncFileEncrypted::Mode::READ_ONLY; return Reference(new AsyncFileEncrypted(r, mode)); }); -#endif +#endif // ENCRYPTION_ENABLED return f; } diff --git a/fdbrpc/sim2.actor.cpp b/fdbrpc/sim2.actor.cpp index 40e576faab..10894c8e86 100644 --- a/fdbrpc/sim2.actor.cpp +++ b/fdbrpc/sim2.actor.cpp @@ -2474,14 +2474,14 @@ Future> Sim2FileSystem::open(const std::string& file f = AsyncFileDetachable::open(f); if (FLOW_KNOBS->PAGE_WRITE_CHECKSUM_HISTORY > 0) f = map(f, [=](Reference r) { return Reference(new AsyncFileWriteChecker(r)); }); -#if (!defined(TLS_DISABLED) && !defined(_WIN32)) +#if ENCRYPTION_ENABLED if (flags & IAsyncFile::OPEN_ENCRYPTED) f = map(f, [flags](Reference r) { auto mode = flags & IAsyncFile::OPEN_READWRITE ? AsyncFileEncrypted::Mode::APPEND_ONLY : AsyncFileEncrypted::Mode::READ_ONLY; return Reference(new AsyncFileEncrypted(r, mode)); }); -#endif +#endif // ENCRYPTION_ENABLED return f; } else return AsyncFileCached::open(filename, flags, mode); From 34e7a7b024f20356ceb94ad944bdfdeabdd85f41 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Sun, 11 Jul 2021 10:30:40 -0700 Subject: [PATCH 05/10] Fix ENCRYPTION_ENABLED typos --- fdbclient/BackupContainerAzureBlobStore.actor.cpp | 2 +- fdbclient/BackupContainerS3BlobStore.actor.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fdbclient/BackupContainerAzureBlobStore.actor.cpp b/fdbclient/BackupContainerAzureBlobStore.actor.cpp index 5b32d9a9f7..6018c4d5cb 100644 --- a/fdbclient/BackupContainerAzureBlobStore.actor.cpp +++ b/fdbclient/BackupContainerAzureBlobStore.actor.cpp @@ -185,7 +185,7 @@ public: return Void(); })); auto f = makeReference(self->asyncTaskThread, self->containerName, fileName, self->client.get()); -#if ENCRYPTED_ENABLED +#if ENCRYPTION_ENABLED if (self->usesEncryption()) { f = makeReference(f, true); } diff --git a/fdbclient/BackupContainerS3BlobStore.actor.cpp b/fdbclient/BackupContainerS3BlobStore.actor.cpp index f95e683768..232154c7a2 100644 --- a/fdbclient/BackupContainerS3BlobStore.actor.cpp +++ b/fdbclient/BackupContainerS3BlobStore.actor.cpp @@ -171,7 +171,7 @@ std::string BackupContainerS3BlobStore::getURLFormat() { Future> BackupContainerS3BlobStore::readFile(const std::string& path) { Reference f = makeReference(m_bstore, m_bucket, dataPath(path)); -#if ENCRYPTED_ENABLED +#if ENCRYPTION_ENABLED if (usesEncryption()) { f = makeReference(f, AsyncFileEncrypted::Mode::READ_ONLY); } From 0f99d449fc8eb641be860ea38695e2392cd2487e Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Fri, 16 Jul 2021 12:01:01 -0400 Subject: [PATCH 06/10] Revert "Merge pull request #5194 from apple/revert-5144-rocksdb-in-simulation" This reverts commit 8ecff8c7304f71be30f3ed6f9b51263c7c2dcc4a, reversing changes made to c8e18f99f080d0bfbbb18139517c38958f64bbf6. --- fdbclient/ServerKnobs.cpp | 4 +++- fdbserver/KeyValueStoreRocksDB.actor.cpp | 24 ++++++++++++++++--- fdbserver/SimulatedCluster.actor.cpp | 15 ++++++++++-- .../from_6.2.33/SnapCycleRestart-1.txt | 2 ++ .../from_6.2.33/SnapCycleRestart-2.txt | 1 + .../from_6.2.33/SnapTestAttrition-1.txt | 2 ++ .../from_6.2.33/SnapTestAttrition-2.txt | 1 + .../from_6.2.33/SnapTestRestart-1.txt | 2 ++ .../from_6.2.33/SnapTestRestart-2.txt | 1 + .../from_6.2.33/SnapTestSimpleRestart-1.txt | 2 ++ .../from_6.2.33/SnapTestSimpleRestart-2.txt | 1 + .../from_7.0.0/SnapIncrementalRestore-1.toml | 3 ++- .../from_7.0.0/SnapIncrementalRestore-2.toml | 3 +++ 13 files changed, 54 insertions(+), 7 deletions(-) diff --git a/fdbclient/ServerKnobs.cpp b/fdbclient/ServerKnobs.cpp index 994f367292..6d74c2f67a 100644 --- a/fdbclient/ServerKnobs.cpp +++ b/fdbclient/ServerKnobs.cpp @@ -335,7 +335,9 @@ void ServerKnobs::initialize(Randomize randomize, ClientKnobs* clientKnobs, IsSi // KeyValueStoreRocksDB init( ROCKSDB_BACKGROUND_PARALLELISM, 0 ); init( ROCKSDB_READ_PARALLELISM, 4 ); - init( ROCKSDB_MEMTABLE_BYTES, 512 * 1024 * 1024 ); + // Use a smaller memtable in simulation to avoid OOMs. + int64_t memtableBytes = isSimulated ? 32 * 1024 : 512 * 1024 * 1024; + init( ROCKSDB_MEMTABLE_BYTES, memtableBytes ); init( ROCKSDB_UNSAFE_AUTO_FSYNC, false ); init( ROCKSDB_PERIODIC_COMPACTION_SECONDS, 0 ); init( ROCKSDB_PREFIX_LEN, 0 ); diff --git a/fdbserver/KeyValueStoreRocksDB.actor.cpp b/fdbserver/KeyValueStoreRocksDB.actor.cpp index 99278a7862..08dee91f04 100644 --- a/fdbserver/KeyValueStoreRocksDB.actor.cpp +++ b/fdbserver/KeyValueStoreRocksDB.actor.cpp @@ -7,6 +7,7 @@ #include #include #include +#include "fdbserver/CoroFlow.h" #include "flow/flow.h" #include "flow/IThreadPool.h" @@ -283,7 +284,9 @@ struct RocksDBKeyValueStore : IKeyValueStore { std::min(value.size(), size_t(a.maxLength))))); } else { if (!s.IsNotFound()) { - TraceEvent(SevError, "RocksDBError").detail("Error", s.ToString()).detail("Method", "ReadValuePrefix"); + TraceEvent(SevError, "RocksDBError") + .detail("Error", s.ToString()) + .detail("Method", "ReadValuePrefix"); } a.result.send(Optional()); } @@ -367,8 +370,23 @@ struct RocksDBKeyValueStore : IKeyValueStore { std::unique_ptr writeBatch; explicit RocksDBKeyValueStore(const std::string& path, UID id) : path(path), id(id) { - writeThread = createGenericThreadPool(); - readThreads = createGenericThreadPool(); + // In simluation, run the reader/writer threads as Coro threads (i.e. in the network thread. The storage engine + // is still multi-threaded as background compaction threads are still present. Reads/writes to disk will also + // block the network thread in a way that would be unacceptable in production but is a necessary evil here. When + // performing the reads in background threads in simulation, the event loop thinks there is no work to do and + // advances time faster than 1 sec/sec. By the time the blocking read actually finishes, simulation has advanced + // time by more than 5 seconds, so every read fails with a transaction_too_old error. Doing blocking IO on the + // main thread solves this issue. There are almost certainly better fixes, but my goal was to get a less + // invasive change merged first and work on a more realistic version if/when we think that would provide + // substantially more confidence in the correctness. + // TODO: Adapt the simulation framework to not advance time quickly when background reads/writes are occurring. + if (g_network->isSimulated()) { + writeThread = CoroThreadPool::createThreadPool(); + readThreads = CoroThreadPool::createThreadPool(); + } else { + writeThread = createGenericThreadPool(); + readThreads = createGenericThreadPool(); + } writeThread->addThread(new Writer(db, id), "fdb-rocksdb-wr"); for (unsigned i = 0; i < SERVER_KNOBS->ROCKSDB_READ_PARALLELISM; ++i) { readThreads->addThread(new Reader(db), "fdb-rocksdb-re"); diff --git a/fdbserver/SimulatedCluster.actor.cpp b/fdbserver/SimulatedCluster.actor.cpp index a8a152f141..42a9642d24 100644 --- a/fdbserver/SimulatedCluster.actor.cpp +++ b/fdbserver/SimulatedCluster.actor.cpp @@ -232,6 +232,7 @@ public: // 1 = "memory" // 2 = "memory-radixtree-beta" // 3 = "ssd-redwood-experimental" + // 4 = "ssd-rocksdb-experimental" // Requires a comma-separated list of numbers WITHOUT whitespaces std::vector storageEngineExcludeTypes; // Set the maximum TLog version that can be selected for a test @@ -1252,7 +1253,7 @@ void SimulationConfig::setDatacenters(const TestConfig& testConfig) { // Sets storage engine based on testConfig details void SimulationConfig::setStorageEngine(const TestConfig& testConfig) { - int storage_engine_type = deterministicRandom()->randomInt(0, 4); + int storage_engine_type = deterministicRandom()->randomInt(0, 5); if (testConfig.storageEngineType.present()) { storage_engine_type = testConfig.storageEngineType.get(); } else { @@ -1260,7 +1261,7 @@ void SimulationConfig::setStorageEngine(const TestConfig& testConfig) { while (std::find(testConfig.storageEngineExcludeTypes.begin(), testConfig.storageEngineExcludeTypes.end(), storage_engine_type) != testConfig.storageEngineExcludeTypes.end()) { - storage_engine_type = deterministicRandom()->randomInt(0, 4); + storage_engine_type = deterministicRandom()->randomInt(0, 5); } } @@ -1285,6 +1286,16 @@ void SimulationConfig::setStorageEngine(const TestConfig& testConfig) { set_config("ssd-redwood-experimental"); break; } + case 4: { + TEST(true); // Simulated cluster using RocksDB storage engine + set_config("ssd-rocksdb-experimental"); + // Tests using the RocksDB engine are necessarily non-deterministic because of RocksDB + // background threads. + TraceEvent(SevWarn, "RocksDBNonDeterminism") + .detail("Explanation", "The RocksDB storage engine is threaded and non-deterministic"); + noUnseed = true; + break; + } default: ASSERT(false); // Programmer forgot to adjust cases. } diff --git a/tests/restarting/from_6.2.33/SnapCycleRestart-1.txt b/tests/restarting/from_6.2.33/SnapCycleRestart-1.txt index e56bda34f9..3a3a678b92 100644 --- a/tests/restarting/from_6.2.33/SnapCycleRestart-1.txt +++ b/tests/restarting/from_6.2.33/SnapCycleRestart-1.txt @@ -1,3 +1,5 @@ +storageEngineExcludeTypes=4 + ;Take snap and do cycle test testTitle=SnapCyclePre clearAfterTest=false diff --git a/tests/restarting/from_6.2.33/SnapCycleRestart-2.txt b/tests/restarting/from_6.2.33/SnapCycleRestart-2.txt index 2fae7418d4..a2b7f05a0b 100644 --- a/tests/restarting/from_6.2.33/SnapCycleRestart-2.txt +++ b/tests/restarting/from_6.2.33/SnapCycleRestart-2.txt @@ -1,4 +1,5 @@ buggify=off +storageEngineExcludeTypes=4 testTitle=SnapCycleRestore runSetup=false diff --git a/tests/restarting/from_6.2.33/SnapTestAttrition-1.txt b/tests/restarting/from_6.2.33/SnapTestAttrition-1.txt index 401a075c0d..f389400c98 100644 --- a/tests/restarting/from_6.2.33/SnapTestAttrition-1.txt +++ b/tests/restarting/from_6.2.33/SnapTestAttrition-1.txt @@ -1,3 +1,5 @@ +storageEngineExcludeTypes=4 + ;write 1000 Keys ending with even numbers testTitle=SnapTestPre clearAfterTest=false diff --git a/tests/restarting/from_6.2.33/SnapTestAttrition-2.txt b/tests/restarting/from_6.2.33/SnapTestAttrition-2.txt index d7d9131549..d4a37486cb 100644 --- a/tests/restarting/from_6.2.33/SnapTestAttrition-2.txt +++ b/tests/restarting/from_6.2.33/SnapTestAttrition-2.txt @@ -1,4 +1,5 @@ buggify=off +storageEngineExcludeTypes=4 ; verify all keys are even numbered testTitle=SnapTestVerify diff --git a/tests/restarting/from_6.2.33/SnapTestRestart-1.txt b/tests/restarting/from_6.2.33/SnapTestRestart-1.txt index 319a617628..c639381244 100644 --- a/tests/restarting/from_6.2.33/SnapTestRestart-1.txt +++ b/tests/restarting/from_6.2.33/SnapTestRestart-1.txt @@ -1,3 +1,5 @@ +storageEngineExcludeTypes=4 + ;write 1000 Keys ending with even numbers testTitle=SnapTestPre clearAfterTest=false diff --git a/tests/restarting/from_6.2.33/SnapTestRestart-2.txt b/tests/restarting/from_6.2.33/SnapTestRestart-2.txt index 0d4aae1d16..f4154c9754 100644 --- a/tests/restarting/from_6.2.33/SnapTestRestart-2.txt +++ b/tests/restarting/from_6.2.33/SnapTestRestart-2.txt @@ -1,4 +1,5 @@ buggify=off +storageEngineExcludeTypes=4 ; verify all keys are even numbered testTitle=SnapTestVerify diff --git a/tests/restarting/from_6.2.33/SnapTestSimpleRestart-1.txt b/tests/restarting/from_6.2.33/SnapTestSimpleRestart-1.txt index 2416f27d29..00f3bb6c3e 100644 --- a/tests/restarting/from_6.2.33/SnapTestSimpleRestart-1.txt +++ b/tests/restarting/from_6.2.33/SnapTestSimpleRestart-1.txt @@ -1,3 +1,5 @@ +storageEngineExcludeTypes=4 + ;write 1000 Keys ending with even number testTitle=SnapSimplePre clearAfterTest=false diff --git a/tests/restarting/from_6.2.33/SnapTestSimpleRestart-2.txt b/tests/restarting/from_6.2.33/SnapTestSimpleRestart-2.txt index eeaa599184..3ffe0a8fcc 100644 --- a/tests/restarting/from_6.2.33/SnapTestSimpleRestart-2.txt +++ b/tests/restarting/from_6.2.33/SnapTestSimpleRestart-2.txt @@ -1,4 +1,5 @@ buggify=off +storageEngineExcludeTypes=4 ; verify all keys are even numbered testTitle=SnapSimpleVerify diff --git a/tests/restarting/from_7.0.0/SnapIncrementalRestore-1.toml b/tests/restarting/from_7.0.0/SnapIncrementalRestore-1.toml index 6321090c4e..3cde35a07c 100644 --- a/tests/restarting/from_7.0.0/SnapIncrementalRestore-1.toml +++ b/tests/restarting/from_7.0.0/SnapIncrementalRestore-1.toml @@ -1,7 +1,8 @@ [configuration] logAntiQuorum = 0 +storageEngineExcludeTypes = [4] -[[test]] +[[test]] testTitle = 'SubmitBackup' simBackupAgents = 'BackupToFile' clearAfterTest = false diff --git a/tests/restarting/from_7.0.0/SnapIncrementalRestore-2.toml b/tests/restarting/from_7.0.0/SnapIncrementalRestore-2.toml index f99d46b344..8eaedcf6fd 100644 --- a/tests/restarting/from_7.0.0/SnapIncrementalRestore-2.toml +++ b/tests/restarting/from_7.0.0/SnapIncrementalRestore-2.toml @@ -1,3 +1,6 @@ +[configuration] +storageEngineExcludeTypes = [4] + [[test]] testTitle = 'RestoreBackup' simBackupAgents = 'BackupToFile' From 0dbf94db7a469188ff7266d28f5740e273da66bc Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Fri, 16 Jul 2021 12:02:20 -0400 Subject: [PATCH 07/10] Don't use storageEngineExcludeTypes in restarting tests --- tests/restarting/from_6.2.33/SnapCycleRestart-1.txt | 2 -- tests/restarting/from_6.2.33/SnapCycleRestart-2.txt | 1 - tests/restarting/from_6.2.33/SnapTestAttrition-1.txt | 2 -- tests/restarting/from_6.2.33/SnapTestAttrition-2.txt | 1 - tests/restarting/from_6.2.33/SnapTestRestart-1.txt | 2 -- tests/restarting/from_6.2.33/SnapTestRestart-2.txt | 1 - tests/restarting/from_6.2.33/SnapTestSimpleRestart-1.txt | 2 -- tests/restarting/from_6.2.33/SnapTestSimpleRestart-2.txt | 1 - tests/restarting/from_7.0.0/SnapIncrementalRestore-1.toml | 3 +-- tests/restarting/from_7.0.0/SnapIncrementalRestore-2.toml | 3 --- 10 files changed, 1 insertion(+), 17 deletions(-) diff --git a/tests/restarting/from_6.2.33/SnapCycleRestart-1.txt b/tests/restarting/from_6.2.33/SnapCycleRestart-1.txt index 3a3a678b92..e56bda34f9 100644 --- a/tests/restarting/from_6.2.33/SnapCycleRestart-1.txt +++ b/tests/restarting/from_6.2.33/SnapCycleRestart-1.txt @@ -1,5 +1,3 @@ -storageEngineExcludeTypes=4 - ;Take snap and do cycle test testTitle=SnapCyclePre clearAfterTest=false diff --git a/tests/restarting/from_6.2.33/SnapCycleRestart-2.txt b/tests/restarting/from_6.2.33/SnapCycleRestart-2.txt index a2b7f05a0b..2fae7418d4 100644 --- a/tests/restarting/from_6.2.33/SnapCycleRestart-2.txt +++ b/tests/restarting/from_6.2.33/SnapCycleRestart-2.txt @@ -1,5 +1,4 @@ buggify=off -storageEngineExcludeTypes=4 testTitle=SnapCycleRestore runSetup=false diff --git a/tests/restarting/from_6.2.33/SnapTestAttrition-1.txt b/tests/restarting/from_6.2.33/SnapTestAttrition-1.txt index f389400c98..401a075c0d 100644 --- a/tests/restarting/from_6.2.33/SnapTestAttrition-1.txt +++ b/tests/restarting/from_6.2.33/SnapTestAttrition-1.txt @@ -1,5 +1,3 @@ -storageEngineExcludeTypes=4 - ;write 1000 Keys ending with even numbers testTitle=SnapTestPre clearAfterTest=false diff --git a/tests/restarting/from_6.2.33/SnapTestAttrition-2.txt b/tests/restarting/from_6.2.33/SnapTestAttrition-2.txt index d4a37486cb..d7d9131549 100644 --- a/tests/restarting/from_6.2.33/SnapTestAttrition-2.txt +++ b/tests/restarting/from_6.2.33/SnapTestAttrition-2.txt @@ -1,5 +1,4 @@ buggify=off -storageEngineExcludeTypes=4 ; verify all keys are even numbered testTitle=SnapTestVerify diff --git a/tests/restarting/from_6.2.33/SnapTestRestart-1.txt b/tests/restarting/from_6.2.33/SnapTestRestart-1.txt index c639381244..319a617628 100644 --- a/tests/restarting/from_6.2.33/SnapTestRestart-1.txt +++ b/tests/restarting/from_6.2.33/SnapTestRestart-1.txt @@ -1,5 +1,3 @@ -storageEngineExcludeTypes=4 - ;write 1000 Keys ending with even numbers testTitle=SnapTestPre clearAfterTest=false diff --git a/tests/restarting/from_6.2.33/SnapTestRestart-2.txt b/tests/restarting/from_6.2.33/SnapTestRestart-2.txt index f4154c9754..0d4aae1d16 100644 --- a/tests/restarting/from_6.2.33/SnapTestRestart-2.txt +++ b/tests/restarting/from_6.2.33/SnapTestRestart-2.txt @@ -1,5 +1,4 @@ buggify=off -storageEngineExcludeTypes=4 ; verify all keys are even numbered testTitle=SnapTestVerify diff --git a/tests/restarting/from_6.2.33/SnapTestSimpleRestart-1.txt b/tests/restarting/from_6.2.33/SnapTestSimpleRestart-1.txt index 00f3bb6c3e..2416f27d29 100644 --- a/tests/restarting/from_6.2.33/SnapTestSimpleRestart-1.txt +++ b/tests/restarting/from_6.2.33/SnapTestSimpleRestart-1.txt @@ -1,5 +1,3 @@ -storageEngineExcludeTypes=4 - ;write 1000 Keys ending with even number testTitle=SnapSimplePre clearAfterTest=false diff --git a/tests/restarting/from_6.2.33/SnapTestSimpleRestart-2.txt b/tests/restarting/from_6.2.33/SnapTestSimpleRestart-2.txt index 3ffe0a8fcc..eeaa599184 100644 --- a/tests/restarting/from_6.2.33/SnapTestSimpleRestart-2.txt +++ b/tests/restarting/from_6.2.33/SnapTestSimpleRestart-2.txt @@ -1,5 +1,4 @@ buggify=off -storageEngineExcludeTypes=4 ; verify all keys are even numbered testTitle=SnapSimpleVerify diff --git a/tests/restarting/from_7.0.0/SnapIncrementalRestore-1.toml b/tests/restarting/from_7.0.0/SnapIncrementalRestore-1.toml index 3cde35a07c..6321090c4e 100644 --- a/tests/restarting/from_7.0.0/SnapIncrementalRestore-1.toml +++ b/tests/restarting/from_7.0.0/SnapIncrementalRestore-1.toml @@ -1,8 +1,7 @@ [configuration] logAntiQuorum = 0 -storageEngineExcludeTypes = [4] -[[test]] +[[test]] testTitle = 'SubmitBackup' simBackupAgents = 'BackupToFile' clearAfterTest = false diff --git a/tests/restarting/from_7.0.0/SnapIncrementalRestore-2.toml b/tests/restarting/from_7.0.0/SnapIncrementalRestore-2.toml index 8eaedcf6fd..f99d46b344 100644 --- a/tests/restarting/from_7.0.0/SnapIncrementalRestore-2.toml +++ b/tests/restarting/from_7.0.0/SnapIncrementalRestore-2.toml @@ -1,6 +1,3 @@ -[configuration] -storageEngineExcludeTypes = [4] - [[test]] testTitle = 'RestoreBackup' simBackupAgents = 'BackupToFile' From 66883e5f1ca84cba82ec71424f031b15c8478b38 Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Fri, 16 Jul 2021 12:51:38 -0400 Subject: [PATCH 08/10] Disable the RocksDB engine in simulation in restarting tests or when it isn't built --- fdbserver/SimulatedCluster.actor.cpp | 45 +++++++++++++++++++++------- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/fdbserver/SimulatedCluster.actor.cpp b/fdbserver/SimulatedCluster.actor.cpp index 42a9642d24..5f656f13f1 100644 --- a/fdbserver/SimulatedCluster.actor.cpp +++ b/fdbserver/SimulatedCluster.actor.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include "fdbrpc/Locality.h" #include "fdbrpc/simulator.h" @@ -50,6 +51,19 @@ extern const char* getSourceVersion(); using namespace std::literals; +// TODO: Defining these here is just asking for ODR violations. +template <> +std::string describe(bool const& val) { + return val ? "true" : "false"; +} + +template <> +std::string describe(int const& val) { + return format("%d", val); +} + +namespace { + const int MACHINE_REBOOT_TIME = 10; bool destructed = false; @@ -630,16 +644,6 @@ ACTOR Future simulatedFDBDRebooter(Reference -std::string describe(bool const& val) { - return val ? "true" : "false"; -} - -template <> -std::string describe(int const& val) { - return format("%d", val); -} - // Since a datacenter kill is considered to be the same as killing a machine, files cannot be swapped across datacenters std::map>, std::vector>> availableFolders; // process count is no longer needed because it is now the length of the vector of ip's, because it was one ip per @@ -2092,9 +2096,17 @@ void setupSimulatedSystem(vector>* systemActors, using namespace std::literals; +#ifdef SSD_ROCKSDB_EXPERIMENTAL +bool rocksDBEnabled = true; +#else +bool rocksDBEnabled = false; +#endif + // Populates the TestConfig fields according to what is found in the test file. void checkTestConf(const char* testFile, TestConfig* testConfig) {} +} // namespace + ACTOR void setupAndRun(std::string dataFolder, const char* testFile, bool rebooting, @@ -2109,6 +2121,19 @@ ACTOR void setupAndRun(std::string dataFolder, g_simulator.hasDiffProtocolProcess = testConfig.startIncompatibleProcess; g_simulator.setDiffProtocol = false; + // The RocksDB storage engine does not support the restarting tests because you cannot consistently get a clean + // snapshot of the storage engine without a snapshotting file system. + // https://github.com/apple/foundationdb/issues/5155 + if (std::string_view(testFile).find("restarting") != std::string_view::npos) { + testConfig.storageEngineExcludeTypes.push_back(4); + } + + // The RocksDB engine is not always built with the rest of fdbserver. Don't try to use it if it is not included + // in the build. + if (!rocksDBEnabled) { + testConfig.storageEngineExcludeTypes.push_back(4); + } + state ProtocolVersion protocolVersion = currentProtocolVersion; if (testConfig.startIncompatibleProcess) { // isolates right most 1 bit of compatibleProtocolVersionMask to make this protocolVersion incompatible From ce023af47ae061e707cf9394b182d2d577caf0b6 Mon Sep 17 00:00:00 2001 From: Steve Atherton Date: Tue, 20 Jul 2021 13:29:39 -0700 Subject: [PATCH 09/10] Add VSCode workspace file pattern to .gitignore. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 01bf5d30ee..4edd5690c3 100644 --- a/.gitignore +++ b/.gitignore @@ -84,6 +84,7 @@ ipch/ compile_commands.json flow/actorcompiler/obj flow/coveragetool/obj +*.code-workspace # IDE indexing (commonly used tools) /compile_commands.json From f48a2b52f1d8da6354b5fb22112f5c6617e733cf Mon Sep 17 00:00:00 2001 From: Chaoguang Lin Date: Tue, 20 Jul 2021 20:44:46 +0000 Subject: [PATCH 10/10] Disable test for exclude for now which can time out sometime --- bindings/python/tests/fdbcli_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bindings/python/tests/fdbcli_tests.py b/bindings/python/tests/fdbcli_tests.py index 810257be64..49fd45c632 100755 --- a/bindings/python/tests/fdbcli_tests.py +++ b/bindings/python/tests/fdbcli_tests.py @@ -416,6 +416,6 @@ if __name__ == '__main__': else: assert process_number > 1, "Process number should be positive" coordinators() - exclude() + # exclude()