Ok, any plan to add TODOs with this context? On Wed, Sep 23, 2015 at 4:11 PM, Niklas Nielsen <nik...@mesosphere.io> wrote:
> 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 > > > > > > > > >
