On 23 September 2015 at 15:47, Benjamin Mahler <benjamin.mah...@gmail.com> wrote:
> +dev > > Just so the rest of the dev community understands, are these kinds of event > based hooks going to be subsumed by a mechanism to stream out cluster > events? Or will these hooks co-exist alongside cluster events? > Could definitely be. All modules have to be (re)built for new Mesos releases; if they can listen to a SlaveLost event, then we can obsolete the explicit hooks. Niklas > > On Wed, Sep 23, 2015 at 3:38 PM, <nniel...@apache.org> wrote: > > > Added masterSlaveLostHook. > > > > This patch adds a new masterSlaveLost hook to enable modules to clean up > > after lost slaves events (as in networking modules, where we want to > > avoid leaking IPs). > > > > Review: https://reviews.apache.org/r/38575 > > > > > > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo > > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/1d86932c > > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/1d86932c > > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/1d86932c > > > > Branch: refs/heads/master > > Commit: 1d86932ce424f3e7b921cc6a436051a45f704dd0 > > Parents: f6706e8 > > Author: Niklas Nielsen <n...@qni.dk> > > Authored: Wed Sep 23 15:37:21 2015 -0700 > > Committer: Niklas Q. Nielsen <nik...@mesosphere.io> > > Committed: Wed Sep 23 15:37:24 2015 -0700 > > > > ---------------------------------------------------------------------- > > include/mesos/hook.hpp | 10 +++++++ > > src/examples/test_hook_module.cpp | 26 +++++++++++++++++ > > src/hook/manager.cpp | 12 ++++++++ > > src/hook/manager.hpp | 2 ++ > > src/master/master.cpp | 5 ++++ > > src/tests/hook_tests.cpp | 53 > ++++++++++++++++++++++++++++++++++ > > 6 files changed, 108 insertions(+) > > ---------------------------------------------------------------------- > > > > > > > > > http://git-wip-us.apache.org/repos/asf/mesos/blob/1d86932c/include/mesos/hook.hpp > > ---------------------------------------------------------------------- > > diff --git a/include/mesos/hook.hpp b/include/mesos/hook.hpp > > index 2353602..2fe060e 100644 > > --- a/include/mesos/hook.hpp > > +++ b/include/mesos/hook.hpp > > @@ -62,6 +62,16 @@ public: > > return None(); > > } > > > > + > > + // This hook is called when an Agent is removed i.e. deemed lost by > the > > + // master. The hook is invoked after all frameworks have been informed > > about > > + // the loss. > > + virtual Try<Nothing> masterSlaveLostHook(const SlaveInfo& slaveInfo) > > + { > > + return Nothing(); > > + } > > + > > + > > // This environment decorator hook is called from within slave when > > // launching a new executor. A module implementing the hook creates > > // and returns a set of environment variables. These environment > > > > > > > http://git-wip-us.apache.org/repos/asf/mesos/blob/1d86932c/src/examples/test_hook_module.cpp > > ---------------------------------------------------------------------- > > diff --git a/src/examples/test_hook_module.cpp > > b/src/examples/test_hook_module.cpp > > index 5c4d71a..c09d7dd 100644 > > --- a/src/examples/test_hook_module.cpp > > +++ b/src/examples/test_hook_module.cpp > > @@ -108,6 +108,32 @@ public: > > return labels; > > } > > > > + virtual Try<Nothing> masterSlaveLostHook(const SlaveInfo& slaveInfo) > > + { > > + LOG(INFO) << "Executing 'masterSlaveLostHook' in slave '" > > + << slaveInfo.id() << "'"; > > + > > + // TODO(nnielsen): Add argument to signal(), so we can filter > > messages from > > + // the `masterSlaveLostHook` from `slaveRemoveExecutorHook`. > > + // NOTE: Will not be a problem **as long as** the test doesn't start > > any > > + // tasks. > > + HookProcess hookProcess; > > + process::spawn(&hookProcess); > > + Future<Nothing> future = > > + process::dispatch(hookProcess, &HookProcess::await); > > + > > + process::dispatch(hookProcess, &HookProcess::signal); > > + > > + // Make sure we don't terminate the process before the message > > self-send has > > + // completed. > > + future.await(); > > + > > + process::terminate(hookProcess); > > + process::wait(hookProcess); > > + > > + return Nothing(); > > + } > > + > > // TODO(nnielsen): Split hook tests into multiple modules to avoid > > // interference. > > virtual Result<Labels> slaveRunTaskLabelDecorator( > > > > > > > http://git-wip-us.apache.org/repos/asf/mesos/blob/1d86932c/src/hook/manager.cpp > > ---------------------------------------------------------------------- > > diff --git a/src/hook/manager.cpp b/src/hook/manager.cpp > > index 691976e..52d53f0 100644 > > --- a/src/hook/manager.cpp > > +++ b/src/hook/manager.cpp > > @@ -133,6 +133,18 @@ Labels HookManager::masterLaunchTaskLabelDecorator( > > } > > > > > > +void HookManager::masterSlaveLostHook(const SlaveInfo& slaveInfo) > > +{ > > + foreachpair (const string& name, Hook* hook, availableHooks) { > > + Try<Nothing> result = hook->masterSlaveLostHook(slaveInfo); > > + if (result.isError()) { > > + LOG(WARNING) << "Master slave-lost hook failed for module '" > > + << name << "': " << result.error(); > > + } > > + } > > +} > > + > > + > > Labels HookManager::slaveRunTaskLabelDecorator( > > const TaskInfo& taskInfo, > > const ExecutorInfo& executorInfo, > > > > > > > http://git-wip-us.apache.org/repos/asf/mesos/blob/1d86932c/src/hook/manager.hpp > > ---------------------------------------------------------------------- > > diff --git a/src/hook/manager.hpp b/src/hook/manager.hpp > > index a517c05..d35a762 100644 > > --- a/src/hook/manager.hpp > > +++ b/src/hook/manager.hpp > > @@ -45,6 +45,8 @@ public: > > const FrameworkInfo& frameworkInfo, > > const SlaveInfo& slaveInfo); > > > > + static void masterSlaveLostHook(const SlaveInfo& slaveInfo); > > + > > static Labels slaveRunTaskLabelDecorator( > > const TaskInfo& taskInfo, > > const ExecutorInfo& executorInfo, > > > > > > > http://git-wip-us.apache.org/repos/asf/mesos/blob/1d86932c/src/master/master.cpp > > ---------------------------------------------------------------------- > > diff --git a/src/master/master.cpp b/src/master/master.cpp > > index 5ca1941..0a40bc3 100644 > > --- a/src/master/master.cpp > > +++ b/src/master/master.cpp > > @@ -5998,6 +5998,11 @@ void Master::_removeSlave( > > message.mutable_slave_id()->MergeFrom(slaveInfo.id()); > > framework->send(message); > > } > > + > > + // Finally, notify the `SlaveLost` hooks. > > + if (HookManager::hooksAvailable()) { > > + HookManager::masterSlaveLostHook(slaveInfo); > > + } > > } > > > > > > > > > > > http://git-wip-us.apache.org/repos/asf/mesos/blob/1d86932c/src/tests/hook_tests.cpp > > ---------------------------------------------------------------------- > > diff --git a/src/tests/hook_tests.cpp b/src/tests/hook_tests.cpp > > index f85d917..c9d35fb 100644 > > --- a/src/tests/hook_tests.cpp > > +++ b/src/tests/hook_tests.cpp > > @@ -18,6 +18,7 @@ > > > > #include <mesos/module.hpp> > > > > +#include <process/clock.hpp> > > #include <process/future.hpp> > > #include <process/gmock.hpp> > > #include <process/pid.hpp> > > @@ -61,6 +62,7 @@ using mesos::internal::slave::Fetcher; > > using mesos::internal::slave::MesosContainerizer; > > using mesos::internal::slave::Slave; > > > > +using process::Clock; > > using process::Future; > > using process::PID; > > using process::Shared; > > @@ -71,6 +73,7 @@ using std::vector; > > > > using testing::_; > > using testing::DoAll; > > +using testing::Eq; > > using testing::Return; > > using testing::SaveArg; > > > > @@ -212,6 +215,56 @@ TEST_F(HookTest, VerifyMasterLaunchTaskHook) > > } > > > > > > +// This test forces a `SlaveLost` event. When this happens, we expect > the > > +// `masterSlaveLostHook` to be invoked and await an internal libprocess > > event > > +// to trigger in the module code. > > +TEST_F(HookTest, MasterSlaveLostHookTest) > > +{ > > + Future<HookExecuted> hookFuture = FUTURE_PROTOBUF(HookExecuted(), _, > _); > > + > > + DROP_MESSAGES(Eq(PingSlaveMessage().GetTypeName()), _, _); > > + > > + master::Flags masterFlags = CreateMasterFlags(); > > + > > + // Speed up timeout cycles. > > + masterFlags.slave_ping_timeout = Seconds(1); > > + masterFlags.max_slave_ping_timeouts = 1; > > + > > + Try<PID<Master>> master = StartMaster(masterFlags); > > + ASSERT_SOME(master); > > + > > + MockExecutor exec(DEFAULT_EXECUTOR_ID); > > + > > + TestContainerizer containerizer(&exec); > > + > > + Future<SlaveRegisteredMessage> slaveRegisteredMessage = > > + FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _); > > + > > + // Start a mock Agent since we aren't testing the slave hooks. > > + Try<PID<Slave>> slave = StartSlave(&containerizer); > > + ASSERT_SOME(slave); > > + > > + // Make sure Agent is up and running. > > + AWAIT_READY(slaveRegisteredMessage); > > + > > + // Forward clock slave timeout. > > + Duration totalTimeout = > > + masterFlags.slave_ping_timeout * > masterFlags.max_slave_ping_timeouts; > > + > > + Clock::pause(); > > + Clock::advance(totalTimeout); > > + Clock::settle(); > > + Clock::resume(); > > + > > + // `masterSlaveLostHook()` should be called from within module code. > > + AWAIT_READY(hookFuture); > > + > > + // TODO(nnielsen): Verify hook signal type. > > + > > + Shutdown(); // Must shutdown before 'containerizer' gets deallocated. > > +} > > + > > + > > // Test that the environment decorator hook adds a new environment > > // variable to the executor runtime. > > // Test hook adds a new environment variable "FOO" to the executor > > > > >
