It applies to all hooks; so I don't think a TODO fit well around the mastSlaveLostHook() as a one-off. Do you want it for each hook or in the module docs? As long as the plans around eventing and subscribing to events from modules haven't been solidified more, I'd suggest we post-pone adding it as it isn't an alternative (yet).
Niklas On 23 September 2015 at 16:29, Benjamin Mahler <bmah...@twitter.com.invalid> wrote: > 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 > > > > > > > > > > > > > >
