Repository: mesos Updated Branches: refs/heads/master b85c79525 -> 26091f461
Fixed a race condition in hook tests for remove-executor hook. There is currently no good way to synchronize between the test body and the hook code, so we wire a promise (owned by the test code). The technical debt is covered by the following JIRA issue: https://issues.apache.org/jira/browse/MESOS-2641 Review: https://reviews.apache.org/r/29947 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/26091f46 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/26091f46 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/26091f46 Branch: refs/heads/master Commit: 26091f46107af741a81aa7d35020ba0f5e552f62 Parents: b85c795 Author: Kapil Arya <[email protected]> Authored: Tue May 19 08:08:50 2015 -0700 Committer: Niklas Q. Nielsen <[email protected]> Committed: Tue May 19 08:27:52 2015 -0700 ---------------------------------------------------------------------- src/examples/test_hook_module.cpp | 32 +++++++++++++++++++++++--- src/messages/messages.proto | 8 +++++++ src/tests/hook_tests.cpp | 42 ++++++++++++---------------------- 3 files changed, 51 insertions(+), 31 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/26091f46/src/examples/test_hook_module.cpp ---------------------------------------------------------------------- diff --git a/src/examples/test_hook_module.cpp b/src/examples/test_hook_module.cpp index b25830a..d61cd55 100644 --- a/src/examples/test_hook_module.cpp +++ b/src/examples/test_hook_module.cpp @@ -22,18 +22,38 @@ #include <mesos/module/hook.hpp> +#include <process/future.hpp> +#include <process/process.hpp> +#include <process/protobuf.hpp> + #include <stout/foreach.hpp> #include <stout/os.hpp> #include <stout/try.hpp> +#include "messages/messages.hpp" + using namespace mesos; +using process::Future; + // 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* testRemoveLabelKey = "MESOS_Test_Remove_Label"; +class HookProcess : public ProtobufProcess<HookProcess> +{ +public: + Future<Nothing> signal() + { + internal::HookExecuted message; + message.set_module("org_apache_mesos_TestHook"); + send(self(), message); + return Nothing(); + } +}; + class TestHook : public Hook { @@ -117,9 +137,15 @@ public: { LOG(INFO) << "Executing 'slaveRemoveExecutorHook'"; - // TODO(karya): Need to synchronize VerifySlaveLaunchExecutorHook - // test with this hook for validation. The issue is tracked by - // MESOS-2226. + // Send a HookExecuted message to ourself. The hook test + // "VerifySlaveLaunchExecutorHook" will wait for the testing + // infrastructure to intercept this message. The intercepted message + // indicates successful execution of this hook. + HookProcess hookProcess; + process::spawn(&hookProcess); + process::dispatch(hookProcess, &HookProcess::signal).await(); + process::terminate(hookProcess); + process::wait(hookProcess); return Nothing(); } http://git-wip-us.apache.org/repos/asf/mesos/blob/26091f46/src/messages/messages.proto ---------------------------------------------------------------------- diff --git a/src/messages/messages.proto b/src/messages/messages.proto index c946754..39dac72 100644 --- a/src/messages/messages.proto +++ b/src/messages/messages.proto @@ -419,3 +419,11 @@ message TaskHealthStatus { // This will not be populated if task is healthy. optional int32 consecutive_failures = 4; } + + +/** + * Message to signal completion of an event within a module. + */ +message HookExecuted { + optional string module = 1; +} http://git-wip-us.apache.org/repos/asf/mesos/blob/26091f46/src/tests/hook_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/hook_tests.cpp b/src/tests/hook_tests.cpp index a65c0ab..a4740e8 100644 --- a/src/tests/hook_tests.cpp +++ b/src/tests/hook_tests.cpp @@ -28,11 +28,13 @@ #include "hook/manager.hpp" -#include "module/manager.hpp" - #include "master/flags.hpp" #include "master/master.hpp" +#include "messages/messages.hpp" + +#include "module/manager.hpp" + #include "slave/flags.hpp" #include "slave/slave.hpp" @@ -84,7 +86,7 @@ class HookTest : public MesosTest protected: // TODO(karya): Replace constructor/destructor with SetUp/TearDown. // Currently, using SetUp/TearDown causes VerifySlave* test to - // fail with a duplicate slave id message. However, everything + // fail with a duplicate slave id message. However, everything // seems normal when using this construction/destructor combo. HookTest() { @@ -251,9 +253,9 @@ TEST_F(HookTest, VerifySlaveExecutorEnvironmentDecorator) // Test executor environment decorator hook and remove executor hook -// for slave. We expect the environment-decorator hook to create a +// for slave. We expect the environment-decorator hook to create a // temporary file and the remove-executor hook to delete that file. -TEST_F(HookTest, DISABLED_VerifySlaveLaunchExecutorHook) +TEST_F(HookTest, VerifySlaveLaunchExecutorHook) { master::Flags masterFlags = CreateMasterFlags(); @@ -302,38 +304,22 @@ TEST_F(HookTest, DISABLED_VerifySlaveLaunchExecutorHook) EXPECT_CALL(exec, registered(_, _, _, _)) .WillOnce(FutureArg<1>(&executorInfo)); + // On successful completion of the "slaveLaunchExecutorHook", the + // test hook will send a HookExecuted message to itself. We wait + // until that message is intercepted by the testing infrastructure. + Future<HookExecuted> hookFuture = FUTURE_PROTOBUF(HookExecuted(), _, _); + driver.launchTasks(offers.get()[0].id(), tasks); AWAIT_READY(executorInfo); - // At launchTasks, the label decorator hook inside should have been - // executed and we should see the labels now. - // Further, the environment decorator hook should also have been - // executed. In that hook, we create a temp file and set the path - // as the value of the environment variable. - // Here we verify that the environment variable is present and the - // file is present on the disk. - Option<string> path; - foreach (const Environment::Variable& variable, - executorInfo.get().command().environment().variables()) { - if (variable.name() == testEnvironmentVariableName) { - path = variable.value(); - break; - } - } - - EXPECT_SOME(path); - // The file must have been create by the environment decorator hook. - EXPECT_TRUE(os::stat::isfile(path.get())); - driver.stop(); driver.join(); Shutdown(); // Must shutdown before 'containerizer' gets deallocated. - // The removeExecutor hook in the test module deletes the temp file. - // Verify that the file is not present. - EXPECT_FALSE(os::stat::isfile(path.get())); + // Now wait for the hook to finish execution. + AWAIT_READY(hookFuture); }
