Merge pull request #8141 from sfc-gh-jfu/network-disable-bypass

Introduce network option for disabling mvc bypass

bindings/c/CMakeLists.txt

@@ -293,6 +293,22 @@ if(NOT WIN32)
             @LOG_DIR@
             )
 
+  add_fdbclient_test(
+    NAME fdb_c_api_tests_local_only
+    DISABLE_LOG_DUMP
+    COMMAND ${CMAKE_SOURCE_DIR}/bindings/c/test/apitester/run_c_api_tests.py
+            --cluster-file
+            @CLUSTER_FILE@
+            --tester-binary
+            $<TARGET_FILE:fdb_c_api_tester>
+            --test-dir
+            ${CMAKE_SOURCE_DIR}/bindings/c/test/apitester/local_tests
+            --tmp-dir
+            @TMP_DIR@
+            --log-dir
+            @LOG_DIR@
+            )
+
   add_fdbclient_test(
     NAME fdb_c_api_tests_blob_granule
     DISABLE_LOG_DUMP

bindings/c/test/apitester/TesterTestSpec.cpp

@@ -101,6 +101,10 @@ std::unordered_map<std::string, std::function<void(const std::string& value, Tes
 	  [](const std::string& value, TestSpec* spec) { //
 	      processIntOption(value, "maxClients", spec->maxClients, 1, 1000);
 	  } },
+	{ "disableClientBypass",
+	  [](const std::string& value, TestSpec* spec) { //
+	      spec->disableClientBypass = (value == "true");
+	  } },
 	{ "minTenants",
 	  [](const std::string& value, TestSpec* spec) { //
 	      processIntOption(value, "minTenants", spec->minTenants, 1, 1000);

bindings/c/test/apitester/TesterTestSpec.h

@@ -78,6 +78,9 @@ struct TestSpec {
 	int minClients = 1;
 	int maxClients = 10;
 
+	// Disable the ability to bypass the MVC API, for
+	// cases when there are no external clients
+	bool disableClientBypass = false;
 	// Number of tenants (a random number in the [min,max] range)
 	int minTenants = 0;
 	int maxTenants = 0;

bindings/c/test/apitester/fdb_c_api_tester.cpp

@@ -322,6 +322,10 @@ void applyNetworkOptions(TesterOptions& options) {
 		fdb::network::setOption(FDBNetworkOption::FDB_NET_OPTION_CLIENT_BUGGIFY_ENABLE);
 	}
 
+	if (options.testSpec.disableClientBypass && options.apiVersion >= 720) {
+		fdb::network::setOption(FDBNetworkOption::FDB_NET_OPTION_DISABLE_CLIENT_BYPASS);
+	}
+
 	if (options.trace) {
 		fdb::network::setOption(FDBNetworkOption::FDB_NET_OPTION_TRACE_ENABLE, options.traceDir);
 		fdb::network::setOption(FDBNetworkOption::FDB_NET_OPTION_TRACE_FORMAT, options.traceFormat);

bindings/c/test/apitester/local_tests/CApiCorrectnessSingleThr.toml

@@ -0,0 +1,29 @@
+[[test]]
+title = 'API Correctness Single Threaded'
+minClients = 1
+maxClients = 3
+minDatabases = 1
+maxDatabases = 3
+multiThreaded = false
+disableClientBypass = true
+
+    [[test.workload]]
+    name = 'ApiCorrectness'
+    minKeyLength = 1
+    maxKeyLength = 64
+    minValueLength = 1
+    maxValueLength = 1000
+    maxKeysPerTransaction = 50
+    initialSize = 100
+    numRandomOperations = 100
+    readExistingKeysRatio = 0.9
+
+    [[test.workload]]
+    name = 'AtomicOpsCorrectness'
+    initialSize = 0
+    numRandomOperations = 100
+
+    [[test.workload]]
+    name = 'WatchAndWait'
+    initialSize = 0
+    numRandomOperations = 10

bindings/go/src/fdb/generated.go

@@ -266,6 +266,11 @@ func (o NetworkOptions) SetEnableRunLoopProfiling() error {
 	return o.setOpt(71, nil)
 }
 
+// Prevents the multi-version client API from being disabled, even if no external clients are configured. This option is required to use GRV caching.
+func (o NetworkOptions) SetDisableClientBypass() error {
+	return o.setOpt(72, nil)
+}
+
 // Enable client buggify - will make requests randomly fail (intended for client testing)
 func (o NetworkOptions) SetClientBuggifyEnable() error {
 	return o.setOpt(80, nil)
@@ -622,7 +627,7 @@ func (o TransactionOptions) SetBypassUnreadable() error {
 	return o.setOpt(1100, nil)
 }
 
-// Allows this transaction to use cached GRV from the database context. Defaults to off. Upon first usage, starts a background updater to periodically update the cache to avoid stale read versions.
+// Allows this transaction to use cached GRV from the database context. Defaults to off. Upon first usage, starts a background updater to periodically update the cache to avoid stale read versions. The disable_client_bypass option must also be set.
 func (o TransactionOptions) SetUseGrvCache() error {
 	return o.setOpt(1101, nil)
 }

fdbclient/MultiVersionTransaction.actor.cpp

@@ -2248,7 +2248,7 @@ void validateOption(Optional<StringRef> value, bool canBePresent, bool canBeAbse
 
 void MultiVersionApi::disableMultiVersionClientApi() {
 	MutexHolder holder(lock);
-	if (networkStartSetup || localClientDisabled) {
+	if (networkStartSetup || localClientDisabled || disableBypass) {
 		throw invalid_option();
 	}
 
@@ -2455,6 +2455,13 @@ void MultiVersionApi::setNetworkOptionInternal(FDBNetworkOptions::Option option,
 		externalClient = true;
 		bypassMultiClientApi = true;
 		forwardOption = true;
+	} else if (option == FDBNetworkOptions::DISABLE_CLIENT_BYPASS) {
+		MutexHolder holder(lock);
+		ASSERT(!networkStartSetup);
+		if (bypassMultiClientApi) {
+			throw invalid_option();
+		}
+		disableBypass = true;
 	} else if (option == FDBNetworkOptions::CLIENT_THREADS_PER_VERSION) {
 		MutexHolder holder(lock);
 		validateOption(value, true, false, false);
@@ -2553,7 +2560,7 @@ void MultiVersionApi::setupNetwork() {
 
 		networkStartSetup = true;
 
-		if (externalClients.empty()) {
+		if (externalClients.empty() && !disableBypass) {
 			bypassMultiClientApi = true; // SOMEDAY: we won't be able to set this option once it becomes possible to add
 			                             // clients after setupNetwork is called
 		}
@@ -2934,8 +2941,8 @@ void MultiVersionApi::loadEnvironmentVariableNetworkOptions() {
 
 MultiVersionApi::MultiVersionApi()
   : callbackOnMainThread(true), localClientDisabled(false), networkStartSetup(false), networkSetup(false),
-    bypassMultiClientApi(false), externalClient(false), apiVersion(0), threadCount(0), tmpDir("/tmp"),
-    traceShareBaseNameAmongThreads(false), envOptionsLoaded(false) {}
+    disableBypass(false), bypassMultiClientApi(false), externalClient(false), apiVersion(0), threadCount(0),
+    tmpDir("/tmp"), traceShareBaseNameAmongThreads(false), envOptionsLoaded(false) {}
 
 MultiVersionApi* MultiVersionApi::api = new MultiVersionApi();
 

fdbclient/NativeAPI.actor.cpp

@@ -6704,6 +6704,9 @@ void Transaction::setOption(FDBTransactionOptions::Option option, Optional<Strin
 
 	case FDBTransactionOptions::USE_GRV_CACHE:
 		validateOptionValueNotPresent(value);
+		if (apiVersionAtLeast(720) && !trState->cx->sharedStatePtr) {
+			throw invalid_option();
+		}
 		if (trState->numErrors == 0) {
 			trState->options.useGrvCache = true;
 		}

fdbclient/include/fdbclient/MultiVersionTransaction.h

@@ -1104,6 +1104,7 @@ private:
 
 	bool networkStartSetup;
 	volatile bool networkSetup;
+	bool disableBypass;
 	volatile bool bypassMultiClientApi;
 	volatile bool externalClient;
 	ApiVersion apiVersion;

fdbclient/vexillographer/fdb.options

@@ -126,6 +126,8 @@ description is not currently required but encouraged.
             description="Deprecated" />
     <Option name="enable_run_loop_profiling" code="71"
             description="Enables debugging feature to perform run loop profiling. Requires trace logging to be enabled. WARNING: this feature is not recommended for use in production." />
+    <Option name="disable_client_bypass" code="72"
+            description="Prevents the multi-version client API from being disabled, even if no external clients are configured. This option is required to use GRV caching."/>
     <Option name="client_buggify_enable" code="80"
             description="Enable client buggify - will make requests randomly fail (intended for client testing)" />
     <Option name="client_buggify_disable" code="81"
@@ -303,7 +305,7 @@ description is not currently required but encouraged.
     <Option name="bypass_unreadable" code="1100"
                 description="Allows ``get`` operations to read from sections of keyspace that have become unreadable because of versionstamp operations. These reads will view versionstamp operations as if they were set operations that did not fill in the versionstamp." />            
     <Option name="use_grv_cache" code="1101"
-            description="Allows this transaction to use cached GRV from the database context. Defaults to off. Upon first usage, starts a background updater to periodically update the cache to avoid stale read versions." />
+            description="Allows this transaction to use cached GRV from the database context. Defaults to off. Upon first usage, starts a background updater to periodically update the cache to avoid stale read versions. The disable_client_bypass option must also be set." />
     <Option name="skip_grv_cache" code="1102"
             description="Specifically instruct this transaction to NOT use cached GRV. Primarily used for the read version cache's background updater to avoid attempting to read a cached entry in specific situations."
             hidden="true"/>

fdbserver/workloads/SidebandSingle.actor.cpp

@@ -128,6 +128,11 @@ struct SidebandSingleWorkload : TestWorkload {
 	}
 
 	ACTOR Future<Void> checker(SidebandSingleWorkload* self, Database cx) {
+		// Required for GRV Cache to work in simulation.
+		// Normally, MVC would set the shared state and it is verified upon setting the
+		// transaction option for GRV Cache.
+		// In simulation, explicitly initialize it when used for tests.
+		cx->initSharedState();
 		loop {
 			// Pair represents <Key, commitVersion>
 			state std::pair<uint64_t, Version> message = waitNext(self->interf.getFuture());