Added 'bool' return value to Containerizer::launch. Review: https://reviews.apache.org/r/23461
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/1ae6ade3 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/1ae6ade3 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/1ae6ade3 Branch: refs/heads/master Commit: 1ae6ade3d4c5c52794be11422f731959a581ac9d Parents: 6ddca90 Author: Benjamin Hindman <[email protected]> Authored: Sun Jun 22 17:53:22 2014 -0700 Committer: Benjamin Hindman <[email protected]> Committed: Mon Aug 4 09:15:50 2014 -0700 ---------------------------------------------------------------------- src/slave/containerizer/containerizer.hpp | 12 +++++--- .../containerizer/external_containerizer.cpp | 12 ++++---- .../containerizer/external_containerizer.hpp | 10 +++---- src/slave/containerizer/mesos/containerizer.cpp | 30 ++++++++++++-------- src/slave/containerizer/mesos/containerizer.hpp | 14 ++++----- src/slave/slave.cpp | 10 +++++-- src/slave/slave.hpp | 2 +- src/tests/containerizer.cpp | 6 ++-- src/tests/containerizer.hpp | 6 ++-- src/tests/containerizer_tests.cpp | 8 +++--- src/tests/external_containerizer_test.cpp | 4 +-- src/tests/slave_recovery_tests.cpp | 2 +- src/tests/slave_tests.cpp | 2 +- 13 files changed, 67 insertions(+), 51 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/slave/containerizer/containerizer.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/containerizer.hpp b/src/slave/containerizer/containerizer.hpp index a9f89fc..02754cd 100644 --- a/src/slave/containerizer/containerizer.hpp +++ b/src/slave/containerizer/containerizer.hpp @@ -72,8 +72,10 @@ public: virtual process::Future<Nothing> recover( const Option<state::SlaveState>& state) = 0; - // Launch a containerized executor. - virtual process::Future<Nothing> launch( + // Launch a containerized executor. Returns true if launching this + // ExecutorInfo is supported and it has been launched, otherwise + // false or a failure is something went wrong. + virtual process::Future<bool> launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const std::string& directory, @@ -82,10 +84,12 @@ public: const process::PID<Slave>& slavePid, bool checkpoint) = 0; - // Launch a containerized task. + // Launch a containerized task. Returns true if launching this + // TaskInfo/ExecutorInfo is supported and it has been launched, + // otherwise false or a failure is something went wrong. // TODO(nnielsen): Obsolete the executorInfo argument when the slave // doesn't require executors to run standalone tasks. - virtual process::Future<Nothing> launch( + virtual process::Future<bool> launch( const ContainerID& containerId, const TaskInfo& taskInfo, const ExecutorInfo& executorInfo, http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/slave/containerizer/external_containerizer.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/external_containerizer.cpp b/src/slave/containerizer/external_containerizer.cpp index 3f28d85..03de1fc 100644 --- a/src/slave/containerizer/external_containerizer.cpp +++ b/src/slave/containerizer/external_containerizer.cpp @@ -153,7 +153,7 @@ Future<Nothing> ExternalContainerizer::recover( } -Future<Nothing> ExternalContainerizer::launch( +Future<bool> ExternalContainerizer::launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const string& directory, @@ -175,7 +175,7 @@ Future<Nothing> ExternalContainerizer::launch( } -Future<Nothing> ExternalContainerizer::launch( +Future<bool> ExternalContainerizer::launch( const ContainerID& containerId, const TaskInfo& taskInfo, const ExecutorInfo& executorInfo, @@ -418,7 +418,7 @@ Future<Nothing> ExternalContainerizerProcess::___recover() } -Future<Nothing> ExternalContainerizerProcess::launch( +Future<bool> ExternalContainerizerProcess::launch( const ContainerID& containerId, const Option<TaskInfo>& taskInfo, const ExecutorInfo& executor, @@ -527,7 +527,7 @@ Future<Nothing> ExternalContainerizerProcess::launch( } -Future<Nothing> ExternalContainerizerProcess::_launch( +Future<bool> ExternalContainerizerProcess::_launch( const ContainerID& containerId, const Future<Option<int> >& future) { @@ -546,13 +546,13 @@ Future<Nothing> ExternalContainerizerProcess::_launch( // have gotten chained up. actives[containerId]->launched.set(Nothing()); - return Nothing(); + return true; } void ExternalContainerizerProcess::__launch( const ContainerID& containerId, - const Future<Nothing>& future) + const Future<bool>& future) { VLOG(1) << "Launch confirmation callback triggered on container '" << containerId << "'"; http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/slave/containerizer/external_containerizer.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/external_containerizer.hpp b/src/slave/containerizer/external_containerizer.hpp index 94dffbb..b32efdd 100644 --- a/src/slave/containerizer/external_containerizer.hpp +++ b/src/slave/containerizer/external_containerizer.hpp @@ -81,7 +81,7 @@ public: virtual process::Future<Nothing> recover( const Option<state::SlaveState>& state); - virtual process::Future<Nothing> launch( + virtual process::Future<bool> launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const std::string& directory, @@ -90,7 +90,7 @@ public: const process::PID<Slave>& slavePid, bool checkpoint); - virtual process::Future<Nothing> launch( + virtual process::Future<bool> launch( const ContainerID& containerId, const TaskInfo& task, const ExecutorInfo& executorInfo, @@ -130,7 +130,7 @@ public: process::Future<Nothing> recover(const Option<state::SlaveState>& state); // Start the containerized executor. - process::Future<Nothing> launch( + process::Future<bool> launch( const ContainerID& containerId, const Option<TaskInfo>& taskInfo, const ExecutorInfo& executorInfo, @@ -219,13 +219,13 @@ private: process::Future<Nothing> ___recover(); - process::Future<Nothing> _launch( + process::Future<bool> _launch( const ContainerID& containerId, const process::Future<Option<int> >& future); void __launch( const ContainerID& containerId, - const process::Future<Nothing>& future); + const process::Future<bool>& future); process::Future<containerizer::Termination> _wait( const ContainerID& containerId); http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/slave/containerizer/mesos/containerizer.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp index 2c394e2..694c9d1 100644 --- a/src/slave/containerizer/mesos/containerizer.cpp +++ b/src/slave/containerizer/mesos/containerizer.cpp @@ -124,7 +124,7 @@ Try<MesosContainerizer*> MesosContainerizer::create( LOG(INFO) << "Using isolation: " << isolation; // Create a MesosContainerizerProcess using isolators and a launcher. - hashmap<std::string, Try<Isolator*> (*)(const Flags&)> creators; + hashmap<string, Try<Isolator*> (*)(const Flags&)> creators; creators["posix/cpu"] = &PosixCpuIsolatorProcess::create; creators["posix/mem"] = &PosixMemIsolatorProcess::create; @@ -200,7 +200,7 @@ Future<Nothing> MesosContainerizer::recover( } -Future<Nothing> MesosContainerizer::launch( +Future<bool> MesosContainerizer::launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const string& directory, @@ -221,7 +221,7 @@ Future<Nothing> MesosContainerizer::launch( } -Future<Nothing> MesosContainerizer::launch( +Future<bool> MesosContainerizer::launch( const ContainerID& containerId, const TaskInfo& taskInfo, const ExecutorInfo& executorInfo, @@ -391,7 +391,7 @@ Future<Nothing> MesosContainerizerProcess::__recover( // 4. Exec the executor. The forked child is signalled to continue. It will // first execute any preparation commands from isolators and then exec the // executor. -Future<Nothing> MesosContainerizerProcess::launch( +Future<bool> MesosContainerizerProcess::launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const string& directory, @@ -411,10 +411,10 @@ Future<Nothing> MesosContainerizerProcess::launch( // run. const CommandInfo& command = executorInfo.command(); if (command.has_container()) { - // We return a Failure as this containerizer does not support + // We return false as this containerizer does not support // handling ContainerInfo. Users have to be made aware of this // lack of support to prevent confusion in the task configuration. - return Failure("ContainerInfo is not supported"); + return false; } Owned<Promise<containerizer::Termination> > promise( @@ -445,7 +445,7 @@ Future<Nothing> MesosContainerizerProcess::launch( } -Future<Nothing> MesosContainerizerProcess::launch( +Future<bool> MesosContainerizerProcess::launch( const ContainerID& containerId, const TaskInfo&, const ExecutorInfo& executorInfo, @@ -616,7 +616,7 @@ Future<Nothing> MesosContainerizerProcess::fetch( } -Future<Nothing> MesosContainerizerProcess::_launch( +Future<bool> MesosContainerizerProcess::_launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const string& directory, @@ -735,7 +735,13 @@ Future<Nothing> MesosContainerizerProcess::_launch( } -Future<Nothing> MesosContainerizerProcess::isolate( +static Future<bool> _isolate() +{ + return true; +} + + +Future<bool> MesosContainerizerProcess::isolate( const ContainerID& containerId, pid_t _pid) { @@ -753,11 +759,11 @@ Future<Nothing> MesosContainerizerProcess::isolate( // Wait for all isolators to complete. return collect(futures) - .then(lambda::bind(&_nothing)); + .then(lambda::bind(&_isolate)); } -Future<Nothing> MesosContainerizerProcess::exec( +Future<bool> MesosContainerizerProcess::exec( const ContainerID& containerId, int pipeWrite) { @@ -779,7 +785,7 @@ Future<Nothing> MesosContainerizerProcess::exec( string(strerror(errno))); } - return Nothing(); + return true; } http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/slave/containerizer/mesos/containerizer.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/containerizer.hpp b/src/slave/containerizer/mesos/containerizer.hpp index 8746968..bf246ca 100644 --- a/src/slave/containerizer/mesos/containerizer.hpp +++ b/src/slave/containerizer/mesos/containerizer.hpp @@ -52,7 +52,7 @@ public: virtual process::Future<Nothing> recover( const Option<state::SlaveState>& state); - virtual process::Future<Nothing> launch( + virtual process::Future<bool> launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const std::string& directory, @@ -61,7 +61,7 @@ public: const process::PID<Slave>& slavePid, bool checkpoint); - virtual process::Future<Nothing> launch( + virtual process::Future<bool> launch( const ContainerID& containerId, const TaskInfo& taskInfo, const ExecutorInfo& executorInfo, @@ -109,7 +109,7 @@ public: process::Future<Nothing> recover( const Option<state::SlaveState>& state); - process::Future<Nothing> launch( + process::Future<bool> launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const std::string& directory, @@ -118,7 +118,7 @@ public: const process::PID<Slave>& slavePid, bool checkpoint); - process::Future<Nothing> launch( + process::Future<bool> launch( const ContainerID& containerId, const TaskInfo& taskInfo, const ExecutorInfo& executorInfo, @@ -161,7 +161,7 @@ private: const std::string& directory, const Option<std::string>& user); - process::Future<Nothing> _launch( + process::Future<bool> _launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const std::string& directory, @@ -171,11 +171,11 @@ private: bool checkpoint, const std::list<Option<CommandInfo> >& scripts); - process::Future<Nothing> isolate( + process::Future<bool> isolate( const ContainerID& containerId, pid_t _pid); - process::Future<Nothing> exec( + process::Future<bool> exec( const ContainerID& containerId, int pipeWrite); http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/slave/slave.cpp ---------------------------------------------------------------------- diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp index b4a5a45..df69b75 100644 --- a/src/slave/slave.cpp +++ b/src/slave/slave.cpp @@ -2418,7 +2418,7 @@ void Slave::executorLaunched( const FrameworkID& frameworkId, const ExecutorID& executorId, const ContainerID& containerId, - const Future<Nothing>& future) + const Future<bool>& future) { // Set up callback for executor termination. Note that we do this // regardless of whether or not we have successfully launched the @@ -2444,6 +2444,12 @@ void Slave::executorLaunched( << "' failed to start: " << (future.isFailed() ? future.failure() : " future discarded"); return; + } else if (future.get()) { + LOG(ERROR) << "Container '" << containerId + << "' for executor '" << executorId + << "' of framework '" << frameworkId + << "' failed to start: TaskInfo/ExecutorInfo not supported"; + return; } Framework* framework = getFramework(frameworkId); @@ -3490,7 +3496,7 @@ Executor* Framework::launchExecutor( } // Launch the container. - Future<Nothing> launch; + Future<bool> launch; if (!executor->commandExecutor) { // If the executor is _not_ a command executor, this means that // the task will include the executor to run. The actual task to http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/slave/slave.hpp ---------------------------------------------------------------------- diff --git a/src/slave/slave.hpp b/src/slave/slave.hpp index a896bb6..9ef597e 100644 --- a/src/slave/slave.hpp +++ b/src/slave/slave.hpp @@ -199,7 +199,7 @@ public: const FrameworkID& frameworkId, const ExecutorID& executorId, const ContainerID& containerId, - const process::Future<Nothing>& future); + const process::Future<bool>& future); void executorTerminated( const FrameworkID& frameworkId, http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/tests/containerizer.cpp ---------------------------------------------------------------------- diff --git a/src/tests/containerizer.cpp b/src/tests/containerizer.cpp index 3f11d35..a17e1e0 100644 --- a/src/tests/containerizer.cpp +++ b/src/tests/containerizer.cpp @@ -73,7 +73,7 @@ TestContainerizer::~TestContainerizer() } -Future<Nothing> TestContainerizer::_launch( +Future<bool> TestContainerizer::_launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const string& directory, @@ -137,11 +137,11 @@ Future<Nothing> TestContainerizer::_launch( new Promise<containerizer::Termination>()); promises[containerId] = promise; - return Nothing(); + return true; } -Future<Nothing> TestContainerizer::launch( +Future<bool> TestContainerizer::launch( const ContainerID& containerId, const TaskInfo& taskInfo, const ExecutorInfo& executorInfo, http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/tests/containerizer.hpp ---------------------------------------------------------------------- diff --git a/src/tests/containerizer.hpp b/src/tests/containerizer.hpp index 9325864..24b014f 100644 --- a/src/tests/containerizer.hpp +++ b/src/tests/containerizer.hpp @@ -66,7 +66,7 @@ public: MOCK_METHOD7( launch, - process::Future<Nothing>( + process::Future<bool>( const ContainerID&, const ExecutorInfo&, const std::string&, @@ -75,7 +75,7 @@ public: const process::PID<slave::Slave>&, bool checkpoint)); - virtual process::Future<Nothing> launch( + virtual process::Future<bool> launch( const ContainerID& containerId, const TaskInfo& taskInfo, const ExecutorInfo& executorInfo, @@ -113,7 +113,7 @@ private: void setup(); // Default 'launch' implementation. - process::Future<Nothing> _launch( + process::Future<bool> _launch( const ContainerID& containerId, const ExecutorInfo& executorInfo, const std::string& directory, http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/tests/containerizer_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/containerizer_tests.cpp b/src/tests/containerizer_tests.cpp index 70e1245..2c90d2f 100644 --- a/src/tests/containerizer_tests.cpp +++ b/src/tests/containerizer_tests.cpp @@ -312,7 +312,7 @@ TEST_F(MesosContainerizerIsolatorPreparationTest, ScriptSucceeds) ContainerID containerId; containerId.set_value("test_container"); - process::Future<Nothing> launch = containerizer.get()->launch( + process::Future<bool> launch = containerizer.get()->launch( containerId, CREATE_EXECUTOR_INFO("executor", "exit 0"), directory, @@ -356,7 +356,7 @@ TEST_F(MesosContainerizerIsolatorPreparationTest, ScriptFails) ContainerID containerId; containerId.set_value("test_container"); - Future<Nothing> launch = containerizer.get()->launch( + Future<bool> launch = containerizer.get()->launch( containerId, CREATE_EXECUTOR_INFO("executor", "exit 0"), directory, @@ -409,7 +409,7 @@ TEST_F(MesosContainerizerIsolatorPreparationTest, MultipleScripts) ContainerID containerId; containerId.set_value("test_container"); - Future<Nothing> launch = containerizer.get()->launch( + Future<bool> launch = containerizer.get()->launch( containerId, CREATE_EXECUTOR_INFO("executor", "exit 0"), directory, @@ -462,7 +462,7 @@ TEST_F(MesosContainerizerExecuteTest, IoRedirection) string command = "(echo '" + errMsg + "' 1>&2) && echo '" + outMsg + "'"; - process::Future<Nothing> launch = containerizer.get()->launch( + process::Future<bool> launch = containerizer.get()->launch( containerId, CREATE_EXECUTOR_INFO("executor", command), directory, http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/tests/external_containerizer_test.cpp ---------------------------------------------------------------------- diff --git a/src/tests/external_containerizer_test.cpp b/src/tests/external_containerizer_test.cpp index c26f3c2..2f6aa50 100644 --- a/src/tests/external_containerizer_test.cpp +++ b/src/tests/external_containerizer_test.cpp @@ -77,7 +77,7 @@ class MockExternalContainerizer : public slave::ExternalContainerizer public: MOCK_METHOD8( launch, - process::Future<Nothing>( + process::Future<bool>( const ContainerID&, const TaskInfo&, const ExecutorInfo&, @@ -98,7 +98,7 @@ public: .WillRepeatedly(Invoke(this, &MockExternalContainerizer::_launch)); } - process::Future<Nothing> _launch( + process::Future<bool> _launch( const ContainerID& containerId, const TaskInfo& taskInfo, const ExecutorInfo& executorInfo, http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/tests/slave_recovery_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/slave_recovery_tests.cpp b/src/tests/slave_recovery_tests.cpp index 0b17a8a..9a14b4c 100644 --- a/src/tests/slave_recovery_tests.cpp +++ b/src/tests/slave_recovery_tests.cpp @@ -3209,7 +3209,7 @@ TYPED_TEST(SlaveRecoveryTest, RestartBeforeContainerizerLaunch) Future<Nothing> launch; EXPECT_CALL(*containerizer1, launch(_, _, _, _, _, _, _)) .WillOnce(DoAll(FutureSatisfy(&launch), - Return(Future<Nothing>()))); + Return(Future<bool>()))); // Ensure that wait doesn't complete so that containerizer doesn't // return a failure on 'wait' due to the pending launch. http://git-wip-us.apache.org/repos/asf/mesos/blob/1ae6ade3/src/tests/slave_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp index e45255a..b4f9f30 100644 --- a/src/tests/slave_tests.cpp +++ b/src/tests/slave_tests.cpp @@ -303,7 +303,7 @@ TEST_F(SlaveTest, MesosExecutorWithOverride) Future<Nothing> launch; EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _)) .WillOnce(DoAll(FutureSatisfy(&launch), - Return(Future<Nothing>()))); + Return(Future<bool>()))); // Expect wait after launch is called. wait() will fail if not // intercepted here as the container will never be registered within
