Repository: mesos Updated Branches: refs/heads/master d5abaccf7 -> 6b675dd1b
Updated Hooks to fix failure during master failover. The ExecutorInfo is immutable, and so the environment decoration is deferred until containerizer launch. The new slaveExecutorHook simply notifies the hook module of a new executor being launched. The environment decorator hook for containerizer launch update it's environment variables as gathered from the hook modules. Review: https://reviews.apache.org/r/31889 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/9a15b695 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/9a15b695 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/9a15b695 Branch: refs/heads/master Commit: 9a15b6955ffa6d99dee7776a1cdbe73617707ce0 Parents: d5abacc Author: Kapil Arya <[email protected]> Authored: Wed Mar 11 16:55:50 2015 -0700 Committer: Niklas Q. Nielsen <[email protected]> Committed: Wed Mar 11 16:55:50 2015 -0700 ---------------------------------------------------------------------- include/mesos/hook.hpp | 17 +++--- src/examples/test_hook_module.cpp | 58 +++++-------------- src/hook/manager.cpp | 16 +++--- src/hook/manager.hpp | 5 +- src/slave/containerizer/containerizer.cpp | 19 ++++++ src/slave/slave.cpp | 22 +------ src/tests/hook_tests.cpp | 80 +++++++++++++++++++++----- 7 files changed, 119 insertions(+), 98 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/9a15b695/include/mesos/hook.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/hook.hpp b/include/mesos/hook.hpp index d83ace5..9ae8b94 100644 --- a/include/mesos/hook.hpp +++ b/include/mesos/hook.hpp @@ -45,14 +45,15 @@ public: return None(); } - // 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 variables are then merged into the executorInfo - // and become part of the executor's environment. - virtual Result<Environment> slaveLaunchExecutorEnvironmentDecorator( - const ExecutorInfo& executorInfo, - const TaskInfo& taskInfo) + // 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 + // variables then become part of the executor's environment. + // Ideally, a hook module will also look at the exiting environment + // variables in executorInfo and extend the values as needed in case + // of a conflict. + virtual Result<Environment> slaveExecutorEnvironmentDecorator( + const ExecutorInfo& executorInfo) { return None(); } http://git-wip-us.apache.org/repos/asf/mesos/blob/9a15b695/src/examples/test_hook_module.cpp ---------------------------------------------------------------------- diff --git a/src/examples/test_hook_module.cpp b/src/examples/test_hook_module.cpp index 2221226..47409cd 100644 --- a/src/examples/test_hook_module.cpp +++ b/src/examples/test_hook_module.cpp @@ -16,8 +16,6 @@ * limitations under the License. */ -#include <string> - #include <mesos/hook.hpp> #include <mesos/mesos.hpp> #include <mesos/module.hpp> @@ -28,15 +26,13 @@ #include <stout/os.hpp> #include <stout/try.hpp> -using std::string; - using namespace mesos; // Must be kept in sync with variables of the same name in // tests/hook_tests.cpp. const char* testLabelKey = "MESOS_Test_Label"; const char* testLabelValue = "ApacheMesos"; -const char* testEnvironmentVariableName = "MESOS_TEST_ENVIRONMENT_VARIABLE"; + class TestHook : public Hook { @@ -57,37 +53,17 @@ public: } - // In this hook, we create a temporary file and add its path to an - // environment variable. Later on, this environment variable is - // looked up by the removeExecutorHook to locate and delete this - // file. - virtual Result<Environment> slaveLaunchExecutorEnvironmentDecorator( - const ExecutorInfo& executorInfo, - const TaskInfo& taskInfo) + // In this hook, we create a new environment variable "FOO" and set + // it's value to "bar". + virtual Result<Environment> slaveExecutorEnvironmentDecorator( + const ExecutorInfo& executorInfo) { - LOG(INFO) << "Executing 'slaveLaunchExecutorEnvironmentDecorator' hook"; - - // Find the label value for the label that was created in the - // label decorator hook above. - Option<string> labelValue; - foreach (const Label& label, taskInfo.labels().labels()) { - if (label.key() == testLabelKey) { - labelValue = label.value(); - CHECK_EQ(labelValue.get(), testLabelValue); - } - } - CHECK_SOME(labelValue); - - // Create a temporary file. - Try<string> file = os::mktemp(); - CHECK_SOME(file); - CHECK_SOME(os::write(file.get(), labelValue.get())); - - // Inject file path into command environment. + LOG(INFO) << "Executing 'slaveExecutorEnvironmentDecorator' hook"; + Environment environment; Environment::Variable* variable = environment.add_variables(); - variable->set_name(testEnvironmentVariableName); - variable->set_value(file.get()); + variable->set_name("FOO"); + variable->set_value("bar"); return environment; } @@ -101,18 +77,10 @@ public: { LOG(INFO) << "Executing 'slaveRemoveExecutorHook'"; - foreach (const Environment::Variable& variable, - executorInfo.command().environment().variables()) { - if (variable.name() == testEnvironmentVariableName) { - string path = variable.value(); - // The removeExecutor hook may be called multiple times; thus - // we ignore the subsequent calls. - if (os::stat::isfile(path)) { - CHECK_SOME(os::rm(path)); - } - break; - } - } + // TODO(karya): Need to synchronize VerifySlaveLaunchExecutorHook + // test with this hook for validation. The issue is tracked by + // MESOS-2226. + return Nothing(); } }; http://git-wip-us.apache.org/repos/asf/mesos/blob/9a15b695/src/hook/manager.cpp ---------------------------------------------------------------------- diff --git a/src/hook/manager.cpp b/src/hook/manager.cpp index 3fd9d5e..7a4cb09 100644 --- a/src/hook/manager.cpp +++ b/src/hook/manager.cpp @@ -113,25 +113,27 @@ Labels HookManager::masterLaunchTaskLabelDecorator( } -Environment HookManager::slaveLaunchExecutorEnvironmentDecorator( - const ExecutorInfo& executorInfo, - const TaskInfo& taskInfo) +Environment HookManager::slaveExecutorEnvironmentDecorator( + ExecutorInfo executorInfo) { Lock lock(&mutex); - Environment environment; foreachpair (const string& name, Hook* hook, availableHooks) { const Result<Environment>& result = - hook->slaveLaunchExecutorEnvironmentDecorator(executorInfo, taskInfo); + hook->slaveExecutorEnvironmentDecorator(executorInfo); if (result.isSome()) { - environment.MergeFrom(result.get()); + // Update executorInfo to include newer environment variables + // so that the next hook module can extend the environment + // variables instead of simply overwriting them. + executorInfo.mutable_command()->mutable_environment()->MergeFrom( + result.get()); } else if (result.isError()) { LOG(WARNING) << "Slave environment decorator hook failed for module '" << name << "': " << result.error(); } } - return environment; + return executorInfo.command().environment(); } http://git-wip-us.apache.org/repos/asf/mesos/blob/9a15b695/src/hook/manager.hpp ---------------------------------------------------------------------- diff --git a/src/hook/manager.hpp b/src/hook/manager.hpp index d3729ea..da81349 100644 --- a/src/hook/manager.hpp +++ b/src/hook/manager.hpp @@ -44,9 +44,8 @@ public: const FrameworkInfo& frameworkInfo, const SlaveInfo& slaveInfo); - static Environment slaveLaunchExecutorEnvironmentDecorator( - const ExecutorInfo& executorInfo, - const TaskInfo& taskInfo); + static Environment slaveExecutorEnvironmentDecorator( + ExecutorInfo executorInfo); static void slaveRemoveExecutorHook( const FrameworkInfo& frameworkInfo, http://git-wip-us.apache.org/repos/asf/mesos/blob/9a15b695/src/slave/containerizer/containerizer.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/containerizer.cpp b/src/slave/containerizer/containerizer.cpp index 33f8738..4d66e76 100644 --- a/src/slave/containerizer/containerizer.cpp +++ b/src/slave/containerizer/containerizer.cpp @@ -28,6 +28,8 @@ #include <stout/strings.hpp> #include <stout/uuid.hpp> +#include "hook/manager.hpp" + #include "slave/flags.hpp" #include "slave/slave.hpp" @@ -288,6 +290,23 @@ map<string, string> executorEnvironment( env["MESOS_RECOVERY_TIMEOUT"] = stringify(recoveryTimeout); } + // Include any environment variables from Hooks. + // TODO(karya): Call environment decorator hook _after_ putting all + // variables from executorInfo into 'env'. This would prevent the + // ones provided by hooks from being overwritten by the ones in + // executorInfo in case of a conflict. The overwriting takes places + // at the callsites of executorEnvironment (e.g., ___launch function + // in src/slave/containerizer/docker.cpp) + // TODO(karya): Provide a mechanism to pass the new environment + // variables created above (MESOS_*) on to the hook modules. + const Environment& hooksEnvironment = + HookManager::slaveExecutorEnvironmentDecorator(executorInfo); + + foreach (const Environment::Variable& variable, + hooksEnvironment.variables()) { + env[variable.name()] = variable.value(); + } + return env; } http://git-wip-us.apache.org/repos/asf/mesos/blob/9a15b695/src/slave/slave.cpp ---------------------------------------------------------------------- diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp index 71ae84b..0f99e4e 100644 --- a/src/slave/slave.cpp +++ b/src/slave/slave.cpp @@ -4114,31 +4114,11 @@ Framework::~Framework() } -// Environment decorator for executor. -static ExecutorInfo decorateExecutorEnvironment( - ExecutorInfo executorInfo, - const TaskInfo& taskInfo) -{ - // Merge environment variables retrieved from label-decorator hooks. - executorInfo.mutable_command()->mutable_environment()->MergeFrom( - HookManager::slaveLaunchExecutorEnvironmentDecorator( - executorInfo, - taskInfo)); - - return executorInfo; -} - - // Create and launch an executor. Executor* Framework::launchExecutor( - const ExecutorInfo& executorInfo__, + const ExecutorInfo& executorInfo, const TaskInfo& taskInfo) { - // Merge environment variables retrieved from environment-decorator - // hooks. - const ExecutorInfo& executorInfo = - decorateExecutorEnvironment(executorInfo__, taskInfo); - // Generate an ID for the executor's container. // TODO(idownes) This should be done by the containerizer but we need the // ContainerID to create the executor's directory and to set up monitoring. http://git-wip-us.apache.org/repos/asf/mesos/blob/9a15b695/src/tests/hook_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/hook_tests.cpp b/src/tests/hook_tests.cpp index b5d776a..bb9de25 100644 --- a/src/tests/hook_tests.cpp +++ b/src/tests/hook_tests.cpp @@ -27,12 +27,19 @@ #include <stout/try.hpp> #include "hook/manager.hpp" + #include "module/manager.hpp" + #include "master/flags.hpp" #include "master/master.hpp" + #include "slave/flags.hpp" #include "slave/slave.hpp" +#include "slave/containerizer/fetcher.hpp" + +#include "slave/containerizer/mesos/containerizer.hpp" + #include "tests/containerizer.hpp" #include "tests/flags.hpp" #include "tests/mesos.hpp" @@ -42,6 +49,9 @@ using std::string; using namespace mesos::modules; using mesos::internal::master::Master; + +using mesos::internal::slave::Fetcher; +using mesos::internal::slave::MesosContainerizer; using mesos::internal::slave::Slave; using process::Future; @@ -113,7 +123,7 @@ TEST_F(HookTest, HookLoading) // taskinfo message during master launch task. TEST_F(HookTest, VerifyMasterLaunchTaskHook) { - Try<PID<Master> > master = StartMaster(CreateMasterFlags()); + Try<PID<Master>> master = StartMaster(CreateMasterFlags()); ASSERT_SOME(master); TestContainerizer containerizer; @@ -131,7 +141,7 @@ TEST_F(HookTest, VerifyMasterLaunchTaskHook) EXPECT_CALL(sched, registered(&driver, _, _)) .Times(1); - Future<vector<Offer> > offers; + Future<vector<Offer>> offers; EXPECT_CALL(sched, resourceOffers(&driver, _)) .WillOnce(FutureArg<1>(&offers)) .WillRepeatedly(Return()); // Ignore subsequent offers. @@ -148,9 +158,9 @@ TEST_F(HookTest, VerifyMasterLaunchTaskHook) TaskInfo task; task.set_name(""); task.mutable_task_id()->set_value("1"); - task.mutable_slave_id()->MergeFrom(offers.get()[0].slave_id()); - task.mutable_resources()->MergeFrom(offers.get()[0].resources()); - task.mutable_command()->MergeFrom(command); + task.mutable_slave_id()->CopyFrom(offers.get()[0].slave_id()); + task.mutable_resources()->CopyFrom(offers.get()[0].resources()); + task.mutable_command()->CopyFrom(command); vector<TaskInfo> tasks; tasks.push_back(task); @@ -184,6 +194,50 @@ TEST_F(HookTest, VerifyMasterLaunchTaskHook) } +// 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 +// with a value "bar". We validate the hook by verifying the value +// of this environment variable. +TEST_F(HookTest, VerifySlaveExecutorEnvironmentDecorator) +{ + const string& directory = os::getcwd(); // We're inside a temporary sandbox. + Fetcher fetcher; + + Try<MesosContainerizer*> containerizer = + MesosContainerizer::create(CreateSlaveFlags(), false, &fetcher); + ASSERT_SOME(containerizer); + + ContainerID containerId; + containerId.set_value("test_container"); + + // Test hook adds a new environment variable "FOO" to the executor + // with a value "bar". A '0' (success) exit status for the following + // command validates the hook. + process::Future<bool> launch = containerizer.get()->launch( + containerId, + CREATE_EXECUTOR_INFO("executor", "test $FOO = 'bar'"), + directory, + None(), + SlaveID(), + process::PID<Slave>(), + false); + AWAIT_READY(launch); + ASSERT_TRUE(launch.get()); + + // Wait on the container. + process::Future<containerizer::Termination> wait = + containerizer.get()->wait(containerId); + AWAIT_READY(wait); + + // Check the executor exited correctly. + EXPECT_TRUE(wait.get().has_status()); + EXPECT_EQ(0, wait.get().status()); + + delete containerizer.get(); +} + + // Test executor environment decorator hook and remove executor hook // for slave. We expect the environment-decorator hook to create a // temporary file and the remove-executor hook to delete that file. @@ -191,7 +245,7 @@ TEST_F(HookTest, DISABLED_VerifySlaveLaunchExecutorHook) { master::Flags masterFlags = CreateMasterFlags(); - Try<PID<Master> > master = StartMaster(masterFlags); + Try<PID<Master>> master = StartMaster(masterFlags); ASSERT_SOME(master); slave::Flags slaveFlags = CreateSlaveFlags(); @@ -200,17 +254,16 @@ TEST_F(HookTest, DISABLED_VerifySlaveLaunchExecutorHook) TestContainerizer containerizer(&exec); - Try<PID<Slave> > slave = StartSlave(&containerizer); + Try<PID<Slave>> slave = StartSlave(&containerizer); ASSERT_SOME(slave); MockScheduler sched; MesosSchedulerDriver driver( &sched, DEFAULT_FRAMEWORK_INFO, master.get(), DEFAULT_CREDENTIAL); - EXPECT_CALL(sched, registered(&driver, _, _)) - .Times(1); + EXPECT_CALL(sched, registered(&driver, _, _)); - Future<vector<Offer> > offers; + Future<vector<Offer>> offers; EXPECT_CALL(sched, resourceOffers(&driver, _)) .WillOnce(FutureArg<1>(&offers)) .WillRepeatedly(Return()); // Ignore subsequent offers. @@ -224,9 +277,9 @@ TEST_F(HookTest, DISABLED_VerifySlaveLaunchExecutorHook) TaskInfo task; task.set_name(""); task.mutable_task_id()->set_value("1"); - task.mutable_slave_id()->MergeFrom(offers.get()[0].slave_id()); - task.mutable_resources()->MergeFrom(offers.get()[0].resources()); - task.mutable_executor()->MergeFrom(DEFAULT_EXECUTOR_INFO); + task.mutable_slave_id()->CopyFrom(offers.get()[0].slave_id()); + task.mutable_resources()->CopyFrom(offers.get()[0].resources()); + task.mutable_executor()->CopyFrom(DEFAULT_EXECUTOR_INFO); vector<TaskInfo> tasks; tasks.push_back(task); @@ -235,7 +288,6 @@ TEST_F(HookTest, DISABLED_VerifySlaveLaunchExecutorHook) Future<ExecutorInfo> executorInfo; EXPECT_CALL(exec, registered(_, _, _, _)) - .Times(1) .WillOnce(FutureArg<1>(&executorInfo)); driver.launchTasks(offers.get()[0].id(), tasks);
