Require 'dbname' is specified for all tests

As part of the changes for MB-48399, we create the per-bucket data
directory inside EPEngine::initialize() - see "MB-48399: Move data
directory creation to engine init" (e5bb822).

This highlighted some latent issues in tests which _don't_ set a
dbname value, instead they end up using the default value in
configuration.json of "test". This can cause problems when running
tests in parallel, as two different tests can end up using the same
data directory and fail / hang in wierd and wonderful ways.

To try to avoid this kind of problem, make dbname a required
configuration param - ep-engine now fails to create a bucket if an
explicit value for dbname is not specified.

This is achieved by:

1. Changing the default value of dbname in configuration.json to the
   empty string "".
2. Checking that config.getDbname() returns a non-empty value inside
   EPEngine::initialize (and the test equivalent
   SynchronousEPEngine::build), and failing initializing if it is empty.
3. With 1 & 2 in place, running all tests and fixing any which are not
   already setting an explicit dbname.

Note that dbname is already always passed by ns_server when we are
running in the full production setup, so there's no production impact
here.

Change-Id: Ie3289cea8c9887d8daaad080af6a4ea1e900a0b4
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/163484
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@couchbase.com>

engines/ep/configuration.json

@@ -343,7 +343,7 @@
             "type": "bool"
         },
         "dbname": {
-            "default": "./test",
+            "default": "",
             "descr": "Path to on-disk storage.",
             "dynamic": false,
             "type": "std::string"

engines/ep/src/ep_engine.cc

@@ -2049,25 +2049,30 @@ cb::engine_errc EventuallyPersistentEngine::initialize(const char* config) {
             return cb::engine_errc::failed;
         }
     }
-
-    // Create the bucket data directory, ns_server should have created the
-    // process level one but they expect us to create the bucket level one.
-    try {
-        cb::io::mkdirp(configuration.getDbname());
-    } catch (const std::system_error& error) {
-        throw std::runtime_error(
-                fmt::format("Failed to create data directory [{}]:{}",
-                            configuration.getDbname(),
-                            error.code().message()));
-    }
-
     name = configuration.getCouchBucket();
 
     if (config != nullptr) {
         EP_LOG_INFO(R"(EPEngine::initialize: using configuration:"{}")",
                     config);
     }
 
+    // Create the bucket data directory, ns_server should have created the
+    // process level one but they expect us to create the bucket level one.
+    const auto dbName = configuration.getDbname();
+    if (dbName.empty()) {
+        EP_LOG_WARN_RAW(
+                "Invalid configuration: dbname must be a non-empty value");
+        return cb::engine_errc::failed;
+    }
+    try {
+        cb::io::mkdirp(dbName);
+    } catch (const std::system_error& error) {
+        EP_LOG_WARN("Failed to create data directory [{}]:{}",
+                    dbName,
+                    error.code().message());
+        return cb::engine_errc::failed;
+    }
+
     auto& env = Environment::get();
     env.engineFileDescriptors = serverApi->core->getMaxEngineFileDescriptors();
 

engines/ep/src/kvstore/couch-kvstore/couch-kvstore.cc

@@ -4388,4 +4388,4 @@ std::unique_ptr<TransactionContext> CouchKVStore::begin(
 
     return std::make_unique<CouchKVStoreTransactionContext>(
             *this, vbid, std::move(pcb));
-}
+}

engines/ep/src/kvstore/kvstore.cc

@@ -181,6 +181,14 @@ void KVStoreStats::reset() {
 }
 
 std::unique_ptr<KVStoreIface> KVStoreFactory::create(KVStoreConfig& config) {
+    // Directory for kvstore files should exist already (see
+    // EventuallyPersistentEngine::initialize).
+    if (!cb::io::isDirectory(config.getDBName())) {
+        throw std::runtime_error(fmt::format(
+                "KVStoreFactory ctor: Specified dbname '{}' is not a directory",
+                config.getDBName()));
+    }
+
     std::string backend = config.getBackend();
     if (backend == "couchdb") {
         auto rw = std::make_unique<CouchKVStore>(

engines/ep/src/kvstore/kvstore_config.cc

@@ -153,4 +153,4 @@ std::unique_ptr<KVStoreConfig> KVStoreConfig::createKVStoreConfig(
                 std::string(backend) + "'");
     }
     return kvConfig;
-}
+}

engines/ep/tests/mock/mock_synchronous_ep_engine.cc

@@ -24,6 +24,7 @@
 #include "objectregistry.h"
 #include <platform/cb_arena_malloc.h>
 #include <platform/cbassert.h>
+#include <platform/dirutils.h>
 #include <programs/engine_testapp/mock_server.h>
 #include <string>
 
@@ -132,6 +133,23 @@ SynchronousEPEngineUniquePtr SynchronousEPEngine::build(
     // are accounted in it's mem_used.
     ObjectRegistry::onSwitchThread(engine.get());
 
+    // Similarly to EPEngine::initialize, create the data directory.
+    const auto dbName = engine->getConfiguration().getDbname();
+    if (dbName.empty()) {
+        throw std::runtime_error(
+                fmt::format("SynchronousEPEngine::build(): Invalid "
+                            "configuration: dbname must be a non-empty value"));
+    }
+    try {
+        cb::io::mkdirp(dbName);
+    } catch (const std::system_error& error) {
+        throw std::runtime_error(
+                fmt::format("SynchronousEPEngine::build(): Failed to create "
+                            "data directory [{}]:{}",
+                            dbName,
+                            error.code().message()));
+    }
+
     engine->setKVBucket(
             engine->public_makeMockBucket(engine->getConfiguration()));
 

engines/ep/tests/mock/mock_synchronous_ep_engine.h

@@ -36,8 +36,14 @@ class SynchronousEPEngine : public EventuallyPersistentEngine {
     void setKVBucket(std::unique_ptr<KVBucket> store);
     void setDcpConnMap(std::unique_ptr<DcpConnMap> dcpConnMap);
 
-    /// Constructs a SynchronousEPEngine instance, along with the necessary
-    /// sub-components.
+    /**
+     * Constructs a SynchronousEPEngine instance, along with the necessary
+     * sub-components.
+     * Similar in scope as EventuallyPersistentEngine ctor +
+     * EventuallyPersistentEngine::initialize(), with the main difference
+     * being none of the asynchronous parts of ::initialize() are performed
+     * (background tasks, etc...)
+     */
     static SynchronousEPEngineUniquePtr build(const std::string& config);
 
     /* Allow us to call normally protected methods */

engines/ep/tests/module_tests/dcp_reflection_test.cc

@@ -296,19 +296,6 @@ class DCPLoopbackTestHelper : virtual public SingleThreadedKVBucketTest {
             }
         }
 
-        // We don't call engine intialize which normally creates the data
-        // directory for us in these tests, manually create the data directory.
-        try {
-            cb::io::mkdirp(test_dbname + "-node_1");
-            cb::io::mkdirp(test_dbname + "-node_2");
-            cb::io::mkdirp(test_dbname + "-node_3");
-        } catch (const std::system_error& error) {
-            throw std::runtime_error(
-                    fmt::format("Failed to create data directory [{}]: {}",
-                                test_dbname,
-                                error.code().message()));
-        }
-
         auto meta = nlohmann::json{
                 {"topology", nlohmann::json::array({{"active", "replica"}})}};
         ASSERT_EQ(cb::engine_errc::success,

engines/ep/tests/module_tests/executorpool_test.cc

@@ -1271,12 +1271,13 @@ TEST_F(SingleThreadedExecutorPoolTest, ignore_duplicate_schedule) {
 
 template <>
 ExecutorPoolEpEngineTest<TestExecutorPool>::ExecutorPoolEpEngineTest() {
-    config = "executor_pool_backend=cb3";
+    config = "executor_pool_backend=cb3;dbname=" + dbnameFromCurrentGTestInfo();
 }
 
 template <>
 ExecutorPoolEpEngineTest<FollyExecutorPool>::ExecutorPoolEpEngineTest() {
-    config = "executor_pool_backend=folly";
+    config = "executor_pool_backend=folly;dbname=" +
+             dbnameFromCurrentGTestInfo();
 }
 
 template <typename T>

engines/ep/tests/module_tests/flusher_test.cc

@@ -21,14 +21,17 @@
 #include <folly/portability/GTest.h>
 #include <programs/engine_testapp/mock_server.h>
 
+using namespace std::string_literals;
+
 class FlusherTest : public ::testing::Test {
 protected:
     void SetUp() override {
         {
             NonBucketAllocationGuard guard;
             ExecutorPool::create(ExecutorPool::Backend::Fake);
         }
-        engine = SynchronousEPEngine::build({});
+        const auto config = "dbname="s + dbnameFromCurrentGTestInfo();
+        engine = SynchronousEPEngine::build(config);
         task_executor = reinterpret_cast<SingleThreadedExecutorPool*>(
                 ExecutorPool::get());
 
@@ -155,4 +158,4 @@ TEST_F(FlusherTest, GetToLowPrioWhenSomeHighPriIsPending) {
     // Run the FLusher again, should drain the low-priority queue
     task_executor->runNextTask(WRITER_TASK_IDX, flusherName);
     ASSERT_EQ(0, flusher->getLPQueueSize());
-}
+}

engines/ep/tests/module_tests/kv_bucket_test.cc

@@ -75,17 +75,6 @@ void KVBucketTest::SetUp() {
         }
     }
 
-    // These tests don't init the engine normally so we need to create the data
-    // directory manually.
-    try {
-        cb::io::mkdirp(test_dbname);
-    } catch (const std::system_error& error) {
-        throw std::runtime_error(
-                fmt::format("Failed to create data directory [{}]:{}",
-                            test_dbname,
-                            error.code().message()));
-    }
-
     if (!ExecutorPool::exists()) {
         ExecutorPool::create();
     }

tests/testapp/testapp_environment.cc

@@ -572,6 +572,10 @@ class McdEnvironmentImpl : public McdEnvironment {
         return *testBucket;
     }
 
+    std::string getTestDir() const override {
+        return test_directory.generic_string();
+    }
+
     std::string getDbPath() const override {
         return static_cast<EpBucketImpl*>(testBucket.get())
                 ->dbPath.generic_string();

tests/testapp/testapp_environment.h

@@ -184,6 +184,9 @@ class McdEnvironment {
      */
     virtual TestBucketImpl& getTestBucket() = 0;
 
+    /// @returns the base directory this test uses.
+    virtual std::string getTestDir() const = 0;
+
     /**
      * @return The dbPath of a persistent bucket (throws if not persistent)
      */

tests/testapp/testapp_misc.cc

@@ -193,7 +193,7 @@ TEST_P(MiscTest, Version) {
 
 TEST_F(TestappTest, CollectionsSelectBucket) {
     // Create and select a bucket on which we will be able to hello collections
-    adminConnection->createBucket("collections", "", BucketType::Couchbase);
+    mcd_env->getTestBucket().createBucket("collections", "", *adminConnection);
 
     auto& conn = getAdminConnection();
     conn.selectBucket("collections");

tests/testapp/testapp_sasl.cc

@@ -12,9 +12,12 @@
 #include "testapp.h"
 #include "testapp_client_test.h"
 
+#include <boost/filesystem.hpp>
 #include <cbcrypto/cbcrypto.h>
 #include <algorithm>
 
+using namespace std::string_literals;
+
 class SaslTest : public TestappClientTest {
 public:
     /**
@@ -39,7 +42,10 @@ class SaslTest : public TestappClientTest {
     void SetUp() override {
         adminConnection->createBucket(bucket1, "", BucketType::Memcached);
         adminConnection->createBucket(bucket2, "", BucketType::Memcached);
-        adminConnection->createBucket(bucket3, "", BucketType::Couchbase);
+        const auto dbname =
+                boost::filesystem::path{mcd_env->getTestDir()} / bucket3;
+        const auto config = "dbname="s + dbname.generic_string();
+        adminConnection->createBucket(bucket3, config, BucketType::Couchbase);
     }
 
     void TearDown() override {