You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ka...@apache.org on 2016/03/11 11:27:51 UTC
mesos git commit: Enabled multiple calls to ModuleManager::load().
Repository: mesos
Updated Branches:
refs/heads/master 3b52ef106 -> 5ea02aea2
Enabled multiple calls to ModuleManager::load().
As long as the module manifest(s) being loaded don't conflict with the
already loaded manifests, the module manager will allow multiple calls
to `load()`.
Review: https://reviews.apache.org/r/44694
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/5ea02aea
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/5ea02aea
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/5ea02aea
Branch: refs/heads/master
Commit: 5ea02aea22122a833fb133f0a6e7c085fcd5c01c
Parents: 3b52ef1
Author: Kapil Arya <ka...@mesosphere.io>
Authored: Thu Mar 10 21:50:21 2016 -0500
Committer: Kapil Arya <ka...@mesosphere.io>
Committed: Fri Mar 11 05:24:57 2016 -0500
----------------------------------------------------------------------
src/module/manager.cpp | 101 +++++++++++++++++++++++++++++++++++++---
src/module/manager.hpp | 11 +++++
src/tests/module_tests.cpp | 48 ++++++++++++++++---
3 files changed, 147 insertions(+), 13 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/5ea02aea/src/module/manager.cpp
----------------------------------------------------------------------
diff --git a/src/module/manager.cpp b/src/module/manager.cpp
index 6ae9950..8c9aaf7 100644
--- a/src/module/manager.cpp
+++ b/src/module/manager.cpp
@@ -44,10 +44,15 @@ using namespace mesos::internal;
using namespace mesos::modules;
std::mutex ModuleManager::mutex;
+
+// TODO(karya): MESOS-4917: Replace the following non-pod static variables with
+// pod equivalents. Cleanup further by introducing additional data structures to
+// avoid keeping multiple mappings from module names.
hashmap<string, string> ModuleManager::kindToVersion;
hashmap<string, ModuleBase*> ModuleManager::moduleBases;
-hashmap<string, Owned<DynamicLibrary>> ModuleManager::dynamicLibraries;
hashmap<string, Parameters> ModuleManager::moduleParameters;
+hashmap<string, string> ModuleManager::moduleLibraries;
+hashmap<string, Owned<DynamicLibrary>> ModuleManager::dynamicLibraries;
void ModuleManager::initialize()
@@ -188,6 +193,74 @@ Try<Nothing> ModuleManager::verifyModule(
}
+// This check is to ensure that the two modules, with the same module name, are
+// indeed identical. We verify that they belong to the same module library and
+// have identical attributes such as parameters, kind, version, etc.
+//
+// Notice that this check doesn't prevent one from having multiple instances of
+// the same module. In the current implementation, it is up to the module
+// developer to return an error if the module in question doesn't support
+// multiple instances.
+//
+// TODO(karya): MESOS-4960: Enhance the module API to allow module developers to
+// express whether the modules are multi-instantiable and thread-safe.
+Try<Nothing> ModuleManager::verifyIdenticalModule(
+ const string& libraryName,
+ const Modules::Library::Module& module,
+ const ModuleBase* base)
+{
+ const string moduleName = module.name();
+
+ // Verify that the two modules come from the same module library.
+ CHECK(moduleLibraries.contains(moduleName));
+ if (libraryName != moduleLibraries[moduleName]) {
+ return Error(
+ "The same module appears in two different module libraries - "
+ "'" + libraryName + "' and '" + moduleLibraries[moduleName] + "'");
+ }
+
+ // Verify that the two modules contain the same set of parameters that appear
+ // in the same order.
+ CHECK(moduleParameters.contains(moduleName));
+ const Parameters& parameters = moduleParameters[moduleName];
+ bool parameterError =
+ module.parameters().size() != parameters.parameter().size();
+
+ for (int i = 0; i < module.parameters().size() && !parameterError; i++) {
+ const Parameter& lhs = parameters.parameter().Get(i);
+ const Parameter& rhs = module.parameters().Get(i);
+ if (lhs.key() != rhs.key() || lhs.value() != rhs.value()) {
+ parameterError = true;
+ }
+ }
+
+ if (parameterError) {
+ return Error(
+ "A module with same name but different parameters already exists");
+ }
+
+ // Verify that the two `ModuleBase` definitions match.
+ CHECK_NOTNULL(base);
+ CHECK(moduleBases.contains(moduleName));
+ ModuleBase* duplicateBase = moduleBases[moduleName];
+
+ // TODO(karya): MESOS-4918: Cache module manifests to avoid potential
+ // overwrite of `ModuleBase` fields by the module itself.
+ if (strcmp(base->moduleApiVersion, duplicateBase->moduleApiVersion) != 0 ||
+ strcmp(base->mesosVersion, duplicateBase->mesosVersion) != 0 ||
+ strcmp(base->kind, duplicateBase->kind) != 0 ||
+ strcmp(base->authorName, duplicateBase->authorName) != 0 ||
+ strcmp(base->authorEmail, duplicateBase->authorEmail) != 0 ||
+ strcmp(base->description, duplicateBase->description) != 0 ||
+ base->compatible != duplicateBase->compatible) {
+ return Error(
+ "A module with same name but different module manifest already exists");
+ }
+
+ return Nothing();
+}
+
+
Try<Nothing> ModuleManager::load(const Modules& modules)
{
synchronized (mutex) {
@@ -223,11 +296,7 @@ Try<Nothing> ModuleManager::load(const Modules& modules)
"'");
}
- // Check for possible duplicate module names.
const string moduleName = module.name();
- if (moduleBases.contains(moduleName)) {
- return Error("Error loading duplicate module '" + moduleName + "'");
- }
// Load ModuleBase.
Try<void*> symbol =
@@ -236,6 +305,7 @@ Try<Nothing> ModuleManager::load(const Modules& modules)
return Error(
"Error loading module '" + moduleName + "': " + symbol.error());
}
+
ModuleBase* moduleBase = (ModuleBase*) symbol.get();
// Verify module compatibility including version, etc.
@@ -245,7 +315,26 @@ Try<Nothing> ModuleManager::load(const Modules& modules)
"Error verifying module '" + moduleName + "': " + result.error());
}
- moduleBases[moduleName] = (ModuleBase*) symbol.get();
+ // We verify module compatibilty before checking for identical modules
+ // to ensure that all the fields in the module manifest are valid before
+ // we start comparing them with an already loaded manifest with the same
+ // module name.
+ if (moduleBases.contains(moduleName)) {
+ Try<Nothing> result =
+ verifyIdenticalModule(libraryName, module, moduleBase);
+
+ if (result.isError()) {
+ return Error(
+ "Error loading module '" + moduleName + "'; this is "
+ " potenatially due to duplicate module names; " +
+ result.error());
+ }
+
+ continue;
+ }
+
+ moduleBases[moduleName] = moduleBase;
+ moduleLibraries[moduleName] = libraryName;
// Now copy the supplied module-specific parameters.
moduleParameters[moduleName].mutable_parameter()->CopyFrom(
http://git-wip-us.apache.org/repos/asf/mesos/blob/5ea02aea/src/module/manager.hpp
----------------------------------------------------------------------
diff --git a/src/module/manager.hpp b/src/module/manager.hpp
index 89dd060..9944af0 100644
--- a/src/module/manager.hpp
+++ b/src/module/manager.hpp
@@ -144,6 +144,14 @@ private:
const std::string& moduleName,
const ModuleBase* moduleBase);
+ // Allow multiple calls to `load()` by verifying that the modules with same
+ // name are indeed identical. Thus, multiple loadings of a module manifest
+ // are harmless.
+ static Try<Nothing> verifyIdenticalModule(
+ const std::string& libraryName,
+ const Modules::Library::Module& module,
+ const ModuleBase* base);
+
static std::mutex mutex;
static hashmap<std::string, std::string> kindToVersion;
@@ -160,6 +168,9 @@ private:
// destructed. Destroying the DynamicLibrary object could result in
// unloading the library from the process memory.
static hashmap<std::string, process::Owned<DynamicLibrary>> dynamicLibraries;
+
+ // Module to library name mapping.
+ static hashmap<std::string, std::string> moduleLibraries;
};
} // namespace modules {
http://git-wip-us.apache.org/repos/asf/mesos/blob/5ea02aea/src/tests/module_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/module_tests.cpp b/src/tests/module_tests.cpp
index 497ed87..eddb0e5 100644
--- a/src/tests/module_tests.cpp
+++ b/src/tests/module_tests.cpp
@@ -444,16 +444,50 @@ TEST_F(ModuleTest, NonModuleLibrary)
}
+// Test that loading the same module twice works.
+TEST_F(ModuleTest, LoadSameModuleTwice)
+{
+ // First load the default modules.
+ EXPECT_SOME(ModuleManager::load(defaultModules));
+
+ // Try to load the same module once again.
+ EXPECT_SOME(ModuleManager::load(defaultModules));
+}
+
+
+// Test that loading the same module twice with different parameters fails.
+TEST_F(ModuleTest, DuplicateModulesWithDifferentParameters)
+{
+ // First load the default modules.
+ EXPECT_SOME(ModuleManager::load(defaultModules));
+
+ // Create a duplicate modules object with some parameters.
+ Modules duplicateModules = getModules(
+ DEFAULT_MODULE_LIBRARY_NAME,
+ DEFAULT_MODULE_NAME,
+ "operation",
+ "X");
+
+ EXPECT_ERROR(ModuleManager::load(duplicateModules));
+}
+
+
// Test that loading a duplicate module fails.
-TEST_F(ModuleTest, DuplicateModule)
+TEST_F(ModuleTest, DuplicateModuleInDifferentLibraries)
{
- // Add duplicate module.
- Modules::Library* library = defaultModules.add_libraries();
- library->set_name(DEFAULT_MODULE_LIBRARY_NAME);
- Modules::Library::Module* module = library->add_modules();
- module->set_name(DEFAULT_MODULE_NAME);
+ // First load the default modules.
+ EXPECT_SOME(ModuleManager::load(defaultModules));
- EXPECT_ERROR(ModuleManager::load(defaultModules));
+ // Create a duplicate modules object with different library name.
+ // We use the full name for library (i.e., examplemodule-X.Y.Z) to simulate
+ // the case of two libraries with the same module.
+ string library =
+ string(DEFAULT_MODULE_LIBRARY_NAME).append("-").append(MESOS_VERSION);
+
+ // Create a duplicate modules object with some parameters.
+ Modules duplicateModules = getModules(library, DEFAULT_MODULE_NAME);
+
+ EXPECT_ERROR(ModuleManager::load(duplicateModules));
}