I apologize; mix of terminology. Things we call XYZ*Hook*() only returns Future<Nothing> and can't mutate anything; those are the ones I meant. The others are all annotated with XYZ*Decorator*() and can change the object in flight, and yes - I don't think these can be done with event subscribers, unless we have something like event filters :)
Don't get me wrong; I love the idea of having this be done much more transparent. It's just not clear what form they will have or how module writers can interact/use them; that's why I am hesitant about hinting them coming up in the code. Niklas On 23 September 2015 at 17:10, Benjamin Mahler <bmah...@twitter.com.invalid> wrote: > My understanding was that it doesn't apply to all hooks because some hooks > mutate data (e.g. add labels), whereas some hooks are "read-only" > event-like hooks. > > On Wed, Sep 23, 2015 at 4:37 PM, Niklas Nielsen <nik...@mesosphere.io> > wrote: > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > >
