mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Niklas Nielsen <nik...@mesosphere.io>
Subject Re: [2/2] mesos git commit: Added masterSlaveLostHook.
Date Thu, 24 Sep 2015 00:15:22 GMT
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 <bmahler@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 <niklas@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
> <bmahler@twitter.com.invalid
> > >
> > wrote:
> >
> > > Ok, any plan to add TODOs with this context?
> > >
> > > On Wed, Sep 23, 2015 at 4:11 PM, Niklas Nielsen <niklas@mesosphere.io>
> > > wrote:
> > >
> > > > On 23 September 2015 at 15:47, Benjamin Mahler <
> > > benjamin.mahler@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, <nnielsen@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 <nik@qni.dk>
> > > > > > Authored: Wed Sep 23 15:37:21 2015 -0700
> > > > > > Committer: Niklas Q. Nielsen <niklas@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
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message