Externalized creation of resource provider manager backing storage. This patch changes the way the storage backing an agent's resource provider registrar is created: while before we created it implicitly when constructing the registrar, we now consume storage passed on construction.
Being able to explicitly inject the used storage simplifies testing. Review: https://reviews.apache.org/r/66309/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/3f0cd112 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/3f0cd112 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/3f0cd112 Branch: refs/heads/master Commit: 3f0cd112cf9645ee28b366e6081069662c619e70 Parents: 6850353 Author: Benjamin Bannier <benjamin.bann...@mesosphere.io> Authored: Tue May 1 13:09:03 2018 -0700 Committer: Chun-Hung Hsiao <chhs...@mesosphere.io> Committed: Tue May 1 13:09:03 2018 -0700 ---------------------------------------------------------------------- src/resource_provider/registrar.cpp | 49 ++++------------------ src/resource_provider/registrar.hpp | 15 ++++--- src/tests/resource_provider_manager_tests.cpp | 27 +----------- 3 files changed, 16 insertions(+), 75 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/3f0cd112/src/resource_provider/registrar.cpp ---------------------------------------------------------------------- diff --git a/src/resource_provider/registrar.cpp b/src/resource_provider/registrar.cpp index 92ef9ae..dbb55dd 100644 --- a/src/resource_provider/registrar.cpp +++ b/src/resource_provider/registrar.cpp @@ -27,10 +27,6 @@ #include <mesos/state/in_memory.hpp> -#ifndef __WINDOWS__ -#include <mesos/state/leveldb.hpp> -#endif // __WINDOWS__ - #include <mesos/state/protobuf.hpp> #include <process/defer.hpp> @@ -53,12 +49,6 @@ using std::string; using mesos::resource_provider::registry::Registry; using mesos::resource_provider::registry::ResourceProvider; -using mesos::state::InMemoryStorage; - -#ifndef __WINDOWS__ -using mesos::state::LevelDBStorage; -#endif // __WINDOWS__ - using mesos::state::Storage; using mesos::state::protobuf::Variable; @@ -96,11 +86,9 @@ bool Registrar::Operation::set() } -Try<Owned<Registrar>> Registrar::create( - const slave::Flags& slaveFlags, - const SlaveID& slaveId) +Try<Owned<Registrar>> Registrar::create(Owned<Storage> storage) { - return new AgentRegistrar(slaveFlags, slaveId); + return new AgentRegistrar(std::move(storage)); } @@ -160,7 +148,7 @@ Try<bool> RemoveResourceProvider::perform(Registry* registry) class AgentRegistrarProcess : public Process<AgentRegistrarProcess> { public: - AgentRegistrarProcess(const slave::Flags& flags, const SlaveID& slaveId); + AgentRegistrarProcess(Owned<Storage> storage); Future<Nothing> recover(); @@ -191,33 +179,12 @@ private: deque<Owned<Registrar::Operation>> operations; bool updating = false; - - static Owned<Storage> createStorage(const std::string& path); }; -Owned<Storage> AgentRegistrarProcess::createStorage(const std::string& path) -{ - // The registrar uses LevelDB as underlying storage. Since LevelDB - // is currently not supported on Windows (see MESOS-5932), we fall - // back to in-memory storage there. - // - // TODO(bbannier): Remove this Windows workaround once MESOS-5932 is fixed. -#ifndef __WINDOWS__ - return Owned<Storage>(new LevelDBStorage(path)); -#else - LOG(WARNING) - << "Persisting resource provider manager state is not supported on Windows"; - return Owned<Storage>(new InMemoryStorage()); -#endif // __WINDOWS__ -} - - -AgentRegistrarProcess::AgentRegistrarProcess( - const slave::Flags& flags, const SlaveID& slaveId) +AgentRegistrarProcess::AgentRegistrarProcess(Owned<Storage> _storage) : ProcessBase(process::ID::generate("resource-provider-agent-registrar")), - storage(createStorage(slave::paths::getResourceProviderRegistryPath( - flags.work_dir, slaveId))), + storage(std::move(_storage)), state(storage.get()) {} @@ -355,10 +322,8 @@ void AgentRegistrarProcess::_update( } -AgentRegistrar::AgentRegistrar( - const slave::Flags& slaveFlags, - const SlaveID& slaveId) - : process(new AgentRegistrarProcess(slaveFlags, slaveId)) +AgentRegistrar::AgentRegistrar(Owned<Storage> storage) + : process(new AgentRegistrarProcess(std::move(storage))) { process::spawn(process.get(), false); } http://git-wip-us.apache.org/repos/asf/mesos/blob/3f0cd112/src/resource_provider/registrar.hpp ---------------------------------------------------------------------- diff --git a/src/resource_provider/registrar.hpp b/src/resource_provider/registrar.hpp index 39f45b0..34cb166 100644 --- a/src/resource_provider/registrar.hpp +++ b/src/resource_provider/registrar.hpp @@ -19,6 +19,8 @@ #include <memory> +#include <mesos/state/storage.hpp> + #include <process/future.hpp> #include <process/owned.hpp> @@ -64,14 +66,13 @@ public: bool success = false; }; - // Create a registry on top of a master's persistent state. + // Create a registry on top of generic storage. static Try<process::Owned<Registrar>> create( - mesos::internal::master::Registrar* registrar); + process::Owned<state::Storage> storage); - // Create a registry on top of an agent's persistent state. + // Create a registry on top of a master's persistent state. static Try<process::Owned<Registrar>> create( - const mesos::internal::slave::Flags& slaveFlags, - const SlaveID& slaveId); + mesos::internal::master::Registrar* registrar); virtual ~Registrar() = default; @@ -110,9 +111,7 @@ class AgentRegistrarProcess; class AgentRegistrar : public Registrar { public: - AgentRegistrar( - const mesos::internal::slave::Flags& slaveFlags, - const SlaveID& slaveId); + AgentRegistrar(process::Owned<state::Storage> storage); ~AgentRegistrar() override; http://git-wip-us.apache.org/repos/asf/mesos/blob/3f0cd112/src/tests/resource_provider_manager_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/resource_provider_manager_tests.cpp b/src/tests/resource_provider_manager_tests.cpp index 0de4e79..72e8122 100644 --- a/src/tests/resource_provider_manager_tests.cpp +++ b/src/tests/resource_provider_manager_tests.cpp @@ -825,31 +825,8 @@ TEST_F(ResourceProviderRegistrarTest, AgentRegistrar) ResourceProviderID resourceProviderId; resourceProviderId.set_value("foo"); - Try<Owned<cluster::Master>> master = StartMaster(); - ASSERT_SOME(master); - - Owned<MasterDetector> detector = master.get()->createDetector(); - - const slave::Flags flags = CreateSlaveFlags(); - - Future<SlaveRegisteredMessage> slaveRegisteredMessage = - FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get()->pid, _); - - Future<UpdateSlaveMessage> updateSlaveMessage = - FUTURE_PROTOBUF(UpdateSlaveMessage(), _, _); - - Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), flags); - ASSERT_SOME(slave); - - AWAIT_READY(slaveRegisteredMessage); - - // The agent will send `UpdateSlaveMessage` after it has created its - // meta directories. Await the message to make sure the agent - // registrar can create its store in the meta hierarchy. - AWAIT_READY(updateSlaveMessage); - - Try<Owned<Registrar>> registrar = - Registrar::create(flags, slaveRegisteredMessage->slave_id()); + Owned<mesos::state::Storage> storage(new mesos::state::InMemoryStorage()); + Try<Owned<Registrar>> registrar = Registrar::create(std::move(storage)); ASSERT_SOME(registrar); ASSERT_NE(nullptr, registrar->get());
