Add error injection tests to test logging in CouchKVStore

This patch adds injection of FileOps and a no buffering flag in order to allow injection of couchstore's MockOps. This is used in conjunction with a new MockLogger to allow verification of CouchKVStore's logging output. Change-Id: I489760e74c88acab594d1fd6afd422bb3f6beeb8 Reviewed-on: http://review.couchbase.org/62492 Reviewed-by: Dave Rigby <daver@couchbase.com> Tested-by: buildbot <build@couchbase.com>
couchbase · May 4, 2016 · b7fa139 · b7fa139
1 parent 4031d7c
commit b7fa139
Show file tree

Hide file tree

Showing 9 changed files with 864 additions and 37 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -26,7 +26,8 @@ INCLUDE_DIRECTORIES(BEFORE ${CMAKE_INSTALL_PREFIX}/include
                            ${CMAKE_CURRENT_BINARY_DIR})
 
 INCLUDE_DIRECTORIES(AFTER
-                    ${gtest_SOURCE_DIR}/include)
+                    ${gtest_SOURCE_DIR}/include
+                    ${gmock_SOURCE_DIR}/include)
 
 CHECK_INCLUDE_FILES("alloca.h" HAVE_ALLOCA_H)
 CHECK_INCLUDE_FILES("arpa/inet.h" HAVE_ARPA_INET_H)
@@ -349,9 +350,13 @@ ADD_EXECUTABLE(ep-engine_kvstore_test
   src/logger.cc
   src/testlogger.cc
   ${OBJECTREGISTRY_SOURCE} ${KVSTORE_SOURCE} ${COUCH_KVSTORE_SOURCE}
-  ${FOREST_KVSTORE_SOURCE} ${CONFIG_SOURCE})
+  ${FOREST_KVSTORE_SOURCE} ${CONFIG_SOURCE}
+  $<TARGET_OBJECTS:couchstore_test_fileops>)
 TARGET_LINK_LIBRARIES(ep-engine_kvstore_test
-                      cJSON JSON_checker couchstore dirutils forestdb gtest platform)
+                      cJSON JSON_checker couchstore dirutils forestdb gmock gtest platform)
+TARGET_INCLUDE_DIRECTORIES(ep-engine_kvstore_test
+        PUBLIC
+        ${Couchstore_SOURCE_DIR})
 
 ADD_EXECUTABLE(ep-engine_atomic_unordered_map_test
   tests/module_tests/atomic_unordered_map_test.cc

diff --git a/src/couch-kvstore/couch-fs-stats.cc b/src/couch-kvstore/couch-fs-stats.cc
@@ -21,8 +21,9 @@
 #include "couch-kvstore/couch-fs-stats.h"
 #include <platform/histogram.h>
 
-std::unique_ptr<FileOpsInterface> getCouchstoreStatsOps(CouchstoreStats& stats) {
-    return std::unique_ptr<FileOpsInterface>(new StatsOps(stats));
+std::unique_ptr<FileOpsInterface> getCouchstoreStatsOps(
+    CouchstoreStats& stats, FileOpsInterface& base_ops) {
+    return std::unique_ptr<FileOpsInterface>(new StatsOps(stats, base_ops));
 }
 
 StatsOps::StatFile::StatFile(FileOpsInterface* _orig_ops,
@@ -34,7 +35,7 @@ StatsOps::StatFile::StatFile(FileOpsInterface* _orig_ops,
 }
 
 couch_file_handle StatsOps::constructor(couchstore_error_info_t *errinfo) {
-    FileOpsInterface* orig_ops = couchstore_get_default_file_ops();
+    FileOpsInterface* orig_ops = &wrapped_ops;
     StatFile* sf = new StatFile(orig_ops,
                                 orig_ops->constructor(errinfo),
                                 0);

diff --git a/src/couch-kvstore/couch-fs-stats.h b/src/couch-kvstore/couch-fs-stats.h
@@ -65,16 +65,22 @@ struct CouchstoreStats {
     }
 };
 
-std::unique_ptr<FileOpsInterface> getCouchstoreStatsOps(CouchstoreStats& stats);
+/**
+ * Returns an instance of StatsOps from a CouchstoreStats reference and
+ * a reference to a base FileOps implementation to wrap
+ */
+std::unique_ptr<FileOpsInterface> getCouchstoreStatsOps(
+    CouchstoreStats& stats, FileOpsInterface& base_ops);
 
 /**
  * FileOpsInterface implementation which records various statistics
  * about OS-level file operations performed by Couchstore.
  */
 class StatsOps : public FileOpsInterface {
 public:
-    StatsOps(CouchstoreStats& _stats)
-        : stats(_stats) {}
+    StatsOps(CouchstoreStats& _stats, FileOpsInterface& ops)
+        : stats(_stats),
+          wrapped_ops(ops) {}
 
     couch_file_handle constructor(couchstore_error_info_t* errinfo) override ;
     couchstore_error_t open(couchstore_error_info_t* errinfo,
@@ -100,6 +106,8 @@ class StatsOps : public FileOpsInterface {
 
 protected:
     CouchstoreStats& stats;
+    FileOpsInterface& wrapped_ops;
+
     struct StatFile {
         StatFile(FileOpsInterface* _orig_ops,
                  couch_file_handle _orig_handle,

diff --git a/src/couch-kvstore/couch-kvstore.cc b/src/couch-kvstore/couch-kvstore.cc
@@ -243,13 +243,24 @@ CouchRequest::CouchRequest(const Item &it, uint64_t rev,
     }
 }
 
-CouchKVStore::CouchKVStore(KVStoreConfig &config, bool read_only) :
-    KVStore(config, read_only), dbname(config.getDBName()),
-    intransaction(false), backfillCounter(0), logger(config.getLogger())
+CouchKVStore::CouchKVStore(KVStoreConfig &config, bool read_only)
+    : CouchKVStore(config, *couchstore_get_default_file_ops(), read_only) {
+
+}
+
+CouchKVStore::CouchKVStore(KVStoreConfig &config, FileOpsInterface& ops,
+                           bool read_only)
+    : KVStore(config, read_only),
+      dbname(config.getDBName()),
+      intransaction(false),
+      backfillCounter(0),
+      logger(config.getLogger()),
+      base_ops(ops)
 {
     createDataDir(dbname);
-    statCollectingFileOps = getCouchstoreStatsOps(st.fsStats);
-    statCollectingFileOpsCompaction = getCouchstoreStatsOps(st.fsStatsCompaction);
+    statCollectingFileOps = getCouchstoreStatsOps(st.fsStats, base_ops);
+    statCollectingFileOpsCompaction = getCouchstoreStatsOps(
+        st.fsStatsCompaction, base_ops);
 
     // init db file map with default revision number, 1
     numDbFiles = configuration.getMaxVBuckets();
@@ -267,13 +278,19 @@ CouchKVStore::CouchKVStore(KVStoreConfig &config, bool read_only) :
     initialize();
 }
 
-CouchKVStore::CouchKVStore(const CouchKVStore &copyFrom) :
-    KVStore(copyFrom), dbname(copyFrom.dbname),
-    dbFileRevMap(copyFrom.dbFileRevMap), numDbFiles(copyFrom.numDbFiles),
-    intransaction(false), logger(copyFrom.logger)
+CouchKVStore::CouchKVStore(const CouchKVStore &copyFrom)
+    : KVStore(copyFrom),
+      dbname(copyFrom.dbname),
+      dbFileRevMap(copyFrom.dbFileRevMap),
+      numDbFiles(copyFrom.numDbFiles),
+      intransaction(false),
+      logger(copyFrom.logger),
+      base_ops(copyFrom.base_ops)
 {
     createDataDir(dbname);
-    statCollectingFileOps = getCouchstoreStatsOps(st.fsStats);
+    statCollectingFileOps = getCouchstoreStatsOps(st.fsStats, base_ops);
+    statCollectingFileOpsCompaction = getCouchstoreStatsOps(
+        st.fsStatsCompaction, base_ops);
 }
 
 void CouchKVStore::initialize() {
@@ -827,9 +844,20 @@ bool CouchKVStore::compactDB(compaction_ctx *hook_ctx) {
     dbfile       = getDBFileName(dbname, vbid, fileRev);
     compact_file = dbfile + ".compact";
 
+    couchstore_open_flags flags(COUCHSTORE_COMPACT_FLAG_UPGRADE_DB);
+
+    /**
+     * This flag disables IO buffering in couchstore which means
+     * file operations will trigger syscalls immediately. This has
+     * a detrimental impact on performance and is only intended
+     * for testing.
+     */
+    if(!configuration.getBuffered()) {
+        flags |= COUCHSTORE_OPEN_FLAG_UNBUFFERED;
+    }
+
     // Perform COMPACTION of vbucket.couch.rev into vbucket.couch.rev.compact
-    errCode = couchstore_compact_db_ex(compactdb, compact_file.c_str(),
-                                       COUCHSTORE_COMPACT_FLAG_UPGRADE_DB,
+    errCode = couchstore_compact_db_ex(compactdb, compact_file.c_str(), flags,
                                        hook, dhook, hook_ctx, def_iops);
     if (errCode != COUCHSTORE_SUCCESS) {
         logger.log(EXTENSION_LOG_WARNING,
@@ -1373,6 +1401,16 @@ couchstore_error_t CouchKVStore::openDB(uint16_t vbucketId,
     uint64_t newRevNum = fileRev;
     couchstore_error_t errorCode = COUCHSTORE_SUCCESS;
 
+    /**
+     * This flag disables IO buffering in couchstore which means
+     * file operations will trigger syscalls immediately. This has
+     * a detrimental impact on performance and is only intended
+     * for testing.
+     */
+    if(!configuration.getBuffered()) {
+        options |= COUCHSTORE_OPEN_FLAG_UNBUFFERED;
+    }
+
     if (reset) {
         errorCode = couchstore_open_db_ex(dbFileName.c_str(), options,
                                           ops, db);
@@ -1389,7 +1427,7 @@ couchstore_error_t CouchKVStore::openDB(uint16_t vbucketId,
                        fileRev, couchstore_strerror(errorCode));
         }
     } else {
-        if (options == COUCHSTORE_OPEN_FLAG_CREATE) {
+        if (options & COUCHSTORE_OPEN_FLAG_CREATE) {
             // first try to open the requested file without the
             // create option in case it does already exist
             errorCode = couchstore_open_db_ex(dbFileName.c_str(), 0, ops, db);

diff --git a/src/couch-kvstore/couch-kvstore.h b/src/couch-kvstore/couch-kvstore.h
@@ -217,12 +217,21 @@ class CouchKVStore : public KVStore
     /**
      * Constructor
      *
-     * @param stats     Engine stats
      * @param config    Configuration information
      * @param read_only flag indicating if this kvstore instance is for read-only operations
      */
     CouchKVStore(KVStoreConfig &config, bool read_only = false);
 
+    /**
+     * Alternate constructor for injecting base FileOps
+     *
+     * @param config    Configuration information
+     * @param ops       Couchstore FileOps implementation to be used
+     * @param read_only flag indicating if this kvstore instance is for read-only operations
+     */
+    CouchKVStore(KVStoreConfig &config, FileOpsInterface& ops,
+                 bool read_only = false);
+
     /**
      * Copy constructor
      *
@@ -603,6 +612,11 @@ class CouchKVStore : public KVStore
     Mutex backfillLock;
 
     Logger& logger;
+
+    /**
+     * Base fileops implementation to be wrapped by stat collecting fileops
+     */
+    FileOpsInterface& base_ops;
 };
 
 #endif  // SRC_COUCH_KVSTORE_COUCH_KVSTORE_H_
diff --git a/src/kvstore.cc b/src/kvstore.cc
@@ -33,25 +33,39 @@
 using namespace CouchbaseDirectoryUtilities;
 
 KVStoreConfig::KVStoreConfig(Configuration& config, uint16_t shardid)
-    : maxVBuckets(config.getMaxVbuckets()), maxShards(config.getMaxNumShards()),
-      dbname(config.getDbname()), backend(config.getBackend()), shardId(shardid),
-      logger(&global_logger) {
+    : KVStoreConfig(config.getMaxVbuckets(),
+                    config.getMaxNumShards(),
+                    config.getDbname(),
+                    config.getBackend(),
+                    shardid) {
 
 }
-KVStoreConfig::KVStoreConfig(uint16_t _maxVBuckets, uint16_t _maxShards,
+
+KVStoreConfig::KVStoreConfig(uint16_t _maxVBuckets,
+                             uint16_t _maxShards,
                              const std::string& _dbname,
                              const std::string& _backend,
                              uint16_t _shardId)
-    : maxVBuckets(_maxVBuckets), maxShards(_maxShards), dbname(_dbname),
-      backend(_backend), shardId(_shardId), logger(&global_logger) {
+    : maxVBuckets(_maxVBuckets),
+      maxShards(_maxShards),
+      dbname(_dbname),
+      backend(_backend),
+      shardId(_shardId),
+      logger(&global_logger),
+      buffered(true) {
 
 }
 
-KVStoreConfig KVStoreConfig::setLogger(Logger& _logger) {
+KVStoreConfig& KVStoreConfig::setLogger(Logger& _logger) {
     logger = &_logger;
     return *this;
 }
 
+KVStoreConfig& KVStoreConfig::setBuffered(bool _buffered) {
+    buffered = _buffered;
+    return *this;
+}
+
 KVStore *KVStoreFactory::create(KVStoreConfig &config, bool read_only) {
     KVStore *ret = NULL;
     std::string backend = config.getBackend();

diff --git a/src/kvstore.h b/src/kvstore.h
@@ -374,9 +374,19 @@ class Configuration;
 
 class KVStoreConfig {
 public:
+    /**
+     * This constructor intialises the object from a central
+     * ep-engine Configuration instance.
+     */
     KVStoreConfig(Configuration& config, uint16_t shardId);
 
-    KVStoreConfig(uint16_t _maxVBuckets, uint16_t _maxShards,
+    /**
+     * This constructor sets the mandatory config options
+     *
+     * Optional config options are set using a separate method
+     */
+    KVStoreConfig(uint16_t _maxVBuckets,
+                  uint16_t _maxShards,
                   const std::string& _dbname,
                   const std::string& _backend,
                   uint16_t _shardId);
@@ -405,10 +415,27 @@ class KVStoreConfig {
         return *logger;
     }
 
+    /**
+     * Indicates whether or not underlying file operations will be
+     * buffered by the storage engine used.
+     *
+     * Only recognised by CouchKVStore
+     */
+    bool getBuffered() {
+        return buffered;
+    }
+
     /**
      * Used to override the default logger object
      */
-    KVStoreConfig setLogger(Logger& _logger);
+    KVStoreConfig& setLogger(Logger& _logger);
+
+    /**
+     * Used to override the default buffering behaviour.
+     *
+     * Only recognised by CouchKVStore
+     */
+    KVStoreConfig& setBuffered(bool _buffered);
 
 private:
     uint16_t maxVBuckets;
@@ -417,6 +444,7 @@ class KVStoreConfig {
     std::string backend;
     uint16_t shardId;
     Logger* logger;
+    bool buffered;
 };
 
 class IORequest {

diff --git a/tests/module_tests/couch-fs-stats_test.cc b/tests/module_tests/couch-fs-stats_test.cc
@@ -21,7 +21,7 @@
 class TestStatsOps : public StatsOps {
 public:
     TestStatsOps(FileOpsInterface* ops)
-        : StatsOps(_stats),
+        : StatsOps(_stats, *ops),
           wrapped_ops(ops) {}
 
     couch_file_handle constructor(couchstore_error_info_t *errinfo) override {