From 8b3ef8725ca935c2c2d18d7facc9ca5e17e30e81 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 17 May 2016 16:00:54 -0400 Subject: [PATCH] In order to minimize contention for the mongodb global lock, use one database per process in tests --- .../lib/xmodule/xmodule/contentstore/mongo.py | 25 ++++++++++-- .../xmodule/xmodule/modulestore/__init__.py | 25 +++++++++--- .../lib/xmodule/xmodule/modulestore/mixed.py | 13 ++++-- .../xmodule/xmodule/modulestore/mongo/base.py | 23 +++++++++-- .../split_mongo/mongo_connection.py | 40 +++++++++++++++++++ .../xmodule/modulestore/split_mongo/split.py | 20 ++++++---- .../xmodule/modulestore/tests/django_utils.py | 9 +++-- .../tests/test_split_modulestore.py | 4 +- .../tests/test_split_w_old_mongo.py | 22 +--------- setup.cfg | 1 - 10 files changed, 130 insertions(+), 52 deletions(-) diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index f2cb96497a..8fc505e121 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -45,6 +45,7 @@ class MongoContentStore(ContentStore): self.fs = gridfs.GridFS(mongo_db, bucket) # pylint: disable=invalid-name self.fs_files = mongo_db[bucket + ".files"] # the underlying collection GridFS uses + self.chunks = mongo_db[bucket + ".chunks"] def close_connections(self): """ @@ -52,13 +53,31 @@ class MongoContentStore(ContentStore): """ self.fs_files.database.connection.close() - def _drop_database(self): + def _drop_database(self, database=True, collections=True, connections=True): """ A destructive operation to drop the underlying database and close all connections. Intended to be used by test code for cleanup. + + If database is True, then this should drop the entire database. + Otherwise, if collections is True, then this should drop all of the collections used + by this modulestore. + Otherwise, the modulestore should remove all data from the collections. + + If connections is True, then close the connection to the database as well. """ - self.close_connections() - self.fs_files.database.connection.drop_database(self.fs_files.database) + connection = self.fs_files.database.connection + + if database: + connection.drop_database(self.fs_files.database) + elif collections: + self.fs_files.drop() + self.chunks.drop() + else: + self.fs_files.remove({}) + self.chunks.remove({}) + + if connections: + self.close_connections() def save(self, content): content_id, content_son = self.asset_db_key(content.location) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 6e0e8671e6..34a8575ab7 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -10,7 +10,6 @@ import datetime from pytz import UTC from collections import defaultdict -import collections from contextlib import contextmanager import threading from operator import itemgetter @@ -1144,10 +1143,17 @@ class ModuleStoreWrite(ModuleStoreRead, ModuleStoreAssetWriteInterface): pass @abstractmethod - def _drop_database(self): + def _drop_database(self, database=True, collections=True, connections=True): """ A destructive operation to drop the underlying database and close all connections. Intended to be used by test code for cleanup. + + If database is True, then this should drop the entire database. + Otherwise, if collections is True, then this should drop all of the collections used + by this modulestore. + Otherwise, the modulestore should remove all data from the collections. + + If connections is True, then close the connection to the database as well. """ pass @@ -1291,7 +1297,7 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): :param category: the xblock category :param fields: the dictionary of {fieldname: value} """ - result = collections.defaultdict(dict) + result = defaultdict(dict) if fields is None: return result cls = self.mixologist.mix(XBlock.load_class(category, select=prefer_xmodules)) @@ -1342,14 +1348,21 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): self.contentstore.delete_all_course_assets(course_key) super(ModuleStoreWriteBase, self).delete_course(course_key, user_id) - def _drop_database(self): + def _drop_database(self, database=True, collections=True, connections=True): """ A destructive operation to drop the underlying database and close all connections. Intended to be used by test code for cleanup. + + If database is True, then this should drop the entire database. + Otherwise, if collections is True, then this should drop all of the collections used + by this modulestore. + Otherwise, the modulestore should remove all data from the collections. + + If connections is True, then close the connection to the database as well. """ if self.contentstore: - self.contentstore._drop_database() # pylint: disable=protected-access - super(ModuleStoreWriteBase, self)._drop_database() + self.contentstore._drop_database(database, collections, connections) # pylint: disable=protected-access + super(ModuleStoreWriteBase, self)._drop_database(database, collections, connections) def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, **kwargs): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 78eb0fd36c..0f726bbaf0 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -810,15 +810,22 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): for modulestore in self.modulestores: modulestore.close_connections() - def _drop_database(self): + def _drop_database(self, database=True, collections=True, connections=True): """ - A destructive operation to drop all databases and close all db connections. + A destructive operation to drop the underlying database and close all connections. Intended to be used by test code for cleanup. + + If database is True, then this should drop the entire database. + Otherwise, if collections is True, then this should drop all of the collections used + by this modulestore. + Otherwise, the modulestore should remove all data from the collections. + + If connections is True, then close the connection to the database as well. """ for modulestore in self.modulestores: # drop database if the store supports it (read-only stores do not) if hasattr(modulestore, '_drop_database'): - modulestore._drop_database() # pylint: disable=protected-access + modulestore._drop_database(database, collections, connections) # pylint: disable=protected-access @strip_key def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 5cf793325b..7000e1e777 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -611,17 +611,32 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo self.database.connection._ensure_connected() return self.database.connection.max_wire_version - def _drop_database(self): + def _drop_database(self, database=True, collections=True, connections=True): """ A destructive operation to drop the underlying database and close all connections. Intended to be used by test code for cleanup. + + If database is True, then this should drop the entire database. + Otherwise, if collections is True, then this should drop all of the collections used + by this modulestore. + Otherwise, the modulestore should remove all data from the collections. + + If connections is True, then close the connection to the database as well. """ # drop the assets - super(MongoModuleStore, self)._drop_database() + super(MongoModuleStore, self)._drop_database(database, collections, connections) connection = self.collection.database.connection - connection.drop_database(self.collection.database.proxied_object) - connection.close() + + if database: + connection.drop_database(self.collection.database.proxied_object) + elif collections: + self.collection.drop() + else: + self.collection.remove({}) + + if connections: + connection.close() @autoretry_read() def fill_in_run(self, course_key): diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index 4bf908fc93..dd34cb5f6c 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -556,3 +556,43 @@ class MongoConnection(object): unique=True, background=True ) + + def close_connections(self): + """ + Closes any open connections to the underlying databases + """ + self.database.connection.close() + + def mongo_wire_version(self): + """ + Returns the wire version for mongo. Only used to unit tests which instrument the connection. + """ + return self.database.connection.max_wire_version + + def _drop_database(self, database=True, collections=True, connections=True): + """ + A destructive operation to drop the underlying database and close all connections. + Intended to be used by test code for cleanup. + + If database is True, then this should drop the entire database. + Otherwise, if collections is True, then this should drop all of the collections used + by this modulestore. + Otherwise, the modulestore should remove all data from the collections. + + If connections is True, then close the connection to the database as well. + """ + connection = self.database.connection + + if database: + connection.drop_database(self.database.name) + elif collections: + self.course_index.drop() + self.structures.drop() + self.definitions.drop() + else: + self.course_index.remove({}) + self.structures.remove({}) + self.definitions.remove({}) + + if connections: + connection.close() diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 72b65596d2..bb2c0cbe66 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -663,7 +663,6 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): super(SplitMongoModuleStore, self).__init__(contentstore, **kwargs) self.db_connection = MongoConnection(**doc_store_config) - self.db = self.db_connection.database if default_class is not None: module_path, __, class_name = default_class.rpartition('.') @@ -693,25 +692,30 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): """ Closes any open connections to the underlying databases """ - self.db.connection.close() + self.db_connection.close_connections() def mongo_wire_version(self): """ Returns the wire version for mongo. Only used to unit tests which instrument the connection. """ - return self.db.connection.max_wire_version + return self.db_connection.mongo_wire_version - def _drop_database(self): + def _drop_database(self, database=True, collections=True, connections=True): """ A destructive operation to drop the underlying database and close all connections. Intended to be used by test code for cleanup. + + If database is True, then this should drop the entire database. + Otherwise, if collections is True, then this should drop all of the collections used + by this modulestore. + Otherwise, the modulestore should remove all data from the collections. + + If connections is True, then close the connection to the database as well. """ # drop the assets - super(SplitMongoModuleStore, self)._drop_database() + super(SplitMongoModuleStore, self)._drop_database(database, collections, connections) - connection = self.db.connection - connection.drop_database(self.db.name) - connection.close() + self.db_connection._drop_database(database, collections, connections) # pylint: disable=protected-access def cache_items(self, system, base_block_ids, course_key, depth=0, lazy=True): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 147563a967..8fd5b99485 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -4,6 +4,7 @@ Modulestore configuration for test cases. """ import copy import functools +import os from uuid import uuid4 from contextlib import contextmanager @@ -93,7 +94,7 @@ def draft_mongo_store_config(data_dir): 'DOC_STORE_CONFIG': { 'host': MONGO_HOST, 'port': MONGO_PORT_NUM, - 'db': 'test_xmodule_{}'.format(uuid4().hex), + 'db': 'test_xmodule_{}'.format(os.getpid()), 'collection': 'modulestore', }, 'OPTIONS': modulestore_options @@ -120,7 +121,7 @@ def split_mongo_store_config(data_dir): 'DOC_STORE_CONFIG': { 'host': MONGO_HOST, 'port': MONGO_PORT_NUM, - 'db': 'test_xmodule_{}'.format(uuid4().hex), + 'db': 'test_xmodule_{}'.format(os.getpid()), 'collection': 'modulestore', }, 'OPTIONS': modulestore_options @@ -139,7 +140,7 @@ def contentstore_config(): 'ENGINE': 'xmodule.contentstore.mongo.MongoContentStore', 'DOC_STORE_CONFIG': { 'host': MONGO_HOST, - 'db': 'test_xcontent_{}'.format(uuid4().hex), + 'db': 'test_xcontent_{}'.format(os.getpid()), 'port': MONGO_PORT_NUM, }, # allow for additional options that can be keyed on a name, e.g. 'trashcan' @@ -161,7 +162,7 @@ def drop_mongo_collections(mock_create): module_store = modulestore() if hasattr(module_store, '_drop_database'): - module_store._drop_database() # pylint: disable=protected-access + module_store._drop_database(database=False) # pylint: disable=protected-access _CONTENTSTORE.clear() if hasattr(module_store, 'close_connections'): module_store.close_connections() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 4a91344a47..e3a87f0c52 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -543,10 +543,8 @@ class SplitModuleTest(unittest.TestCase): """ Clear persistence between each test. """ - collection_prefix = SplitModuleTest.MODULESTORE['DOC_STORE_CONFIG']['collection'] + '.' if SplitModuleTest.modulestore: - for collection in ('active_versions', 'structures', 'definitions'): - modulestore().db.drop_collection(collection_prefix + collection) + modulestore()._drop_database(database=False, connections=False) # pylint: disable=protected-access # drop the modulestore to force re init SplitModuleTest.modulestore = None super(SplitModuleTest, self).tearDown() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py index 5ef3ddbcd2..a9e92b1211 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py @@ -57,35 +57,17 @@ class SplitWMongoCourseBootstrapper(unittest.TestCase): self.db_config, **self.modulestore_options ) - self.addCleanup(self.split_mongo.db.connection.close) - self.addCleanup(self.tear_down_split) + self.addCleanup(self.split_mongo._drop_database) # pylint: disable=protected-access self.draft_mongo = DraftMongoModuleStore( None, self.db_config, branch_setting_func=lambda: ModuleStoreEnum.Branch.draft_preferred, metadata_inheritance_cache_subsystem=MemoryCache(), **self.modulestore_options ) - self.addCleanup(self.tear_down_mongo) + self.addCleanup(self.draft_mongo._drop_database) # pylint: disable=protected-access self.old_course_key = None self.runtime = None self._create_course() - def tear_down_split(self): - """ - Remove the test collections, close the db connection - """ - split_db = self.split_mongo.db - split_db.drop_collection(split_db.course_index.proxied_object) - split_db.drop_collection(split_db.structures.proxied_object) - split_db.drop_collection(split_db.definitions.proxied_object) - - def tear_down_mongo(self): - """ - Remove the test collections, close the db connection - """ - split_db = self.split_mongo.db - # old_mongo doesn't give a db attr, but all of the dbs are the same - split_db.drop_collection(self.draft_mongo.collection.proxied_object) - def _create_item(self, category, name, data, metadata, parent_category, parent_name, draft=True, split=True): """ Create the item of the given category and block id in split and old mongo, add it to the optional diff --git a/setup.cfg b/setup.cfg index f302088d4d..97aed7995b 100644 --- a/setup.cfg +++ b/setup.cfg @@ -9,7 +9,6 @@ exclude-dir=lms/envs # which shadows the xblock library (among other things) no-path-adjustment=1 -process-restartworker=1 process-timeout=300 # Uncomment the following lines to open pdb when a test fails