Replaced slaveTaskStatusLabelDecorator hook with slaveTaskStatusDecorator hook.
This allows the hook to not only update TaskStatus::labels, but also TaskStatus::container_status. Enhanced example hook module and relevant test to reflect the change. Review: https://reviews.apache.org/r/38368 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/ddfcec81 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/ddfcec81 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/ddfcec81 Branch: refs/heads/master Commit: ddfcec81820878222cf34b806985eabdec03b4ac Parents: f7b470e Author: Kapil Arya <[email protected]> Authored: Wed Sep 16 17:02:23 2015 -0700 Committer: Niklas Q. Nielsen <[email protected]> Committed: Wed Sep 16 18:16:10 2015 -0700 ---------------------------------------------------------------------- include/mesos/hook.hpp | 11 ++++++----- src/examples/test_hook_module.cpp | 21 ++++++++++++++++++--- src/hook/manager.cpp | 22 +++++++++++++++------- src/hook/manager.hpp | 2 +- src/slave/slave.cpp | 18 ++++++++++++++---- src/tests/hook_tests.cpp | 29 +++++++++++++++++++++++++++-- 6 files changed, 81 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/ddfcec81/include/mesos/hook.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/hook.hpp b/include/mesos/hook.hpp index d90bacc..2353602 100644 --- a/include/mesos/hook.hpp +++ b/include/mesos/hook.hpp @@ -102,11 +102,12 @@ public: return Nothing(); } - // This hook is called from within slave when it receives a status - // update from the executor. A module implementing the hook creates - // and returns a set of labels. These labels overwrite the existing - // labels on the TaskStatus. - virtual Result<Labels> slaveTaskStatusLabelDecorator( + // This hook is called from within slave when it receives a status update from + // the executor. A module implementing the hook creates and returns a + // TaskStatus with a set of labels and container_status. These labels and + // container status overwrite the existing labels on the TaskStatus. Remaining + // fields from the returned TaskStatus are discarded. + virtual Result<TaskStatus> slaveTaskStatusDecorator( const FrameworkID& frameworkId, const TaskStatus& status) { http://git-wip-us.apache.org/repos/asf/mesos/blob/ddfcec81/src/examples/test_hook_module.cpp ---------------------------------------------------------------------- diff --git a/src/examples/test_hook_module.cpp b/src/examples/test_hook_module.cpp index bc13a8a..0dc74d6 100644 --- a/src/examples/test_hook_module.cpp +++ b/src/examples/test_hook_module.cpp @@ -171,11 +171,11 @@ public: } - virtual Result<Labels> slaveTaskStatusLabelDecorator( + virtual Result<TaskStatus> slaveTaskStatusDecorator( const FrameworkID& frameworkId, const TaskStatus& status) { - LOG(INFO) << "Executing 'slaveTaskStatusLabelDecorator' hook"; + LOG(INFO) << "Executing 'slaveTaskStatusDecorator' hook"; Labels labels; @@ -191,7 +191,22 @@ public: } } - return labels; + TaskStatus result; + result.mutable_labels()->CopyFrom(labels); + + // Set an IP address, a network isolation group, and a known label + // in network info. This data is later validated by the + // 'HookTest.VerifySlaveTaskStatusDecorator' test. + NetworkInfo* networkInfo = + result.mutable_container_status()->add_network_infos(); + networkInfo->set_ip_address("4.3.2.1"); + networkInfo->add_groups("public"); + + Label* networkInfoLabel = networkInfo->mutable_labels()->add_labels(); + networkInfoLabel->set_key("net_foo"); + networkInfoLabel->set_value("net_bar"); + + return result; } }; http://git-wip-us.apache.org/repos/asf/mesos/blob/ddfcec81/src/hook/manager.cpp ---------------------------------------------------------------------- diff --git a/src/hook/manager.cpp b/src/hook/manager.cpp index 754c238..691976e 100644 --- a/src/hook/manager.cpp +++ b/src/hook/manager.cpp @@ -231,25 +231,33 @@ void HookManager::slaveRemoveExecutorHook( } -Labels HookManager::slaveTaskStatusLabelDecorator( +TaskStatus HookManager::slaveTaskStatusDecorator( const FrameworkID& frameworkId, TaskStatus status) { synchronized (mutex) { foreachpair (const string& name, Hook* hook, availableHooks) { - const Result<Labels> result = - hook->slaveTaskStatusLabelDecorator(frameworkId, status); + const Result<TaskStatus> result = + hook->slaveTaskStatusDecorator(frameworkId, status); - // NOTE: Labels remain unchanged if the hook returns None(). + // NOTE: Labels/ContainerStatus remain unchanged if the hook returns + // None(). if (result.isSome()) { - status.mutable_labels()->CopyFrom(result.get()); + if (result.get().has_labels()) { + status.mutable_labels()->CopyFrom(result.get().labels()); + } + + if (result.get().has_container_status()) { + status.mutable_container_status()->CopyFrom( + result.get().container_status()); + } } else if (result.isError()) { - LOG(WARNING) << "Slave TaskStatus label decorator hook failed for " + LOG(WARNING) << "Slave TaskStatus decorator hook failed for " << "module '" << name << "': " << result.error(); } } - return status.labels(); + return status; } } http://git-wip-us.apache.org/repos/asf/mesos/blob/ddfcec81/src/hook/manager.hpp ---------------------------------------------------------------------- diff --git a/src/hook/manager.hpp b/src/hook/manager.hpp index 30d8321..a517c05 100644 --- a/src/hook/manager.hpp +++ b/src/hook/manager.hpp @@ -69,7 +69,7 @@ public: const FrameworkInfo& frameworkInfo, const ExecutorInfo& executorInfo); - static Labels slaveTaskStatusLabelDecorator( + static TaskStatus slaveTaskStatusDecorator( const FrameworkID& frameworkId, TaskStatus status); }; http://git-wip-us.apache.org/repos/asf/mesos/blob/ddfcec81/src/slave/slave.cpp ---------------------------------------------------------------------- diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp index 43fc737..93353a1 100644 --- a/src/slave/slave.cpp +++ b/src/slave/slave.cpp @@ -2759,10 +2759,20 @@ void Slave::statusUpdate(StatusUpdate update, const UPID& pid) } if (HookManager::hooksAvailable()) { - // Set TaskStatus labels from run task label decorator. - update.mutable_status()->mutable_labels()->CopyFrom( - HookManager::slaveTaskStatusLabelDecorator( - update.framework_id(), update.status())); + // Even though the hook(s) return a TaskStatus, we only use two fields: + // container_status and labels. Remaining fields are discarded. + TaskStatus statusFromHooks = + HookManager::slaveTaskStatusDecorator( + update.framework_id(), update.status()); + if (statusFromHooks.has_labels()) { + update.mutable_status()->mutable_labels()->CopyFrom( + statusFromHooks.labels()); + } + + if (statusFromHooks.has_container_status()) { + update.mutable_status()->mutable_container_status()->CopyFrom( + statusFromHooks.container_status()); + } } // Fill in the container IP address with the IP from the agent PID, if not http://git-wip-us.apache.org/repos/asf/mesos/blob/ddfcec81/src/tests/hook_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/hook_tests.cpp b/src/tests/hook_tests.cpp index deb4343..b23a587 100644 --- a/src/tests/hook_tests.cpp +++ b/src/tests/hook_tests.cpp @@ -428,7 +428,7 @@ TEST_F(HookTest, VerifySlaveRunTaskHook) // "bar":"baz") is sent from the executor. The labels get modified by // the slave hook to strip the "foo":"bar" pair and/ add a new // "baz":"qux" pair. -TEST_F(HookTest, VerifySlaveTaskStatusLabelDecorator) +TEST_F(HookTest, VerifySlaveTaskStatusDecorator) { Try<PID<Master>> master = StartMaster(); ASSERT_SOME(master); @@ -495,7 +495,7 @@ TEST_F(HookTest, VerifySlaveTaskStatusLabelDecorator) AWAIT_READY(status); - // The master hook will hang an extra label off. + // The hook will hang an extra label off. const Labels& labels_ = status.get().labels(); EXPECT_EQ(2, labels_.labels_size()); @@ -510,6 +510,31 @@ TEST_F(HookTest, VerifySlaveTaskStatusLabelDecorator) EXPECT_EQ("bar", labels_.labels(1).key()); EXPECT_EQ("baz", labels_.labels(1).value()); + // Now validate TaskInfo.container_status. We must have received a + // container_status with one network_info set by the test hook module. + EXPECT_TRUE(status.get().has_container_status()); + EXPECT_EQ(1, status.get().container_status().network_infos().size()); + + const NetworkInfo networkInfo = + status.get().container_status().network_infos(0); + + // The hook module sets up '4.3.2.1' as the IP address and 'public' as the + // network isolation group. + EXPECT_TRUE(networkInfo.has_ip_address()); + EXPECT_EQ("4.3.2.1", networkInfo.ip_address()); + + EXPECT_EQ(1, networkInfo.groups().size()); + EXPECT_EQ("public", networkInfo.groups(0)); + + EXPECT_TRUE(networkInfo.has_labels()); + EXPECT_EQ(1, networkInfo.labels().labels().size()); + + const Label networkInfoLabel = networkInfo.labels().labels(0); + + // Finally, the labels set inside NetworkInfo by the hook module. + EXPECT_EQ("net_foo", networkInfoLabel.key()); + EXPECT_EQ("net_bar", networkInfoLabel.value()); + driver.stop(); driver.join();
