This is an automated email from the ASF dual-hosted git repository. bbannier pushed a commit to branch 1.7.x in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 47f2a11e6ade71c455efe285898b52c256681e31 Author: Benjamin Bannier <[email protected]> AuthorDate: Thu Jan 23 14:19:57 2020 +0100 Remembered whether an executor was agent-generated. This patch adds code to pass on whether was generated in the agent from the point where the executor is generated to the point where we create an actual `slave::Executor` instance. This allows us to persist this information in the executor state. Review: https://reviews.apache.org/r/72035/ --- src/slave/slave.cpp | 49 +++++++++++++++++++++++++++--------------------- src/slave/slave.hpp | 14 ++++++++++---- src/tests/mock_slave.cpp | 8 +++++--- src/tests/mock_slave.hpp | 8 +++++--- 4 files changed, 48 insertions(+), 31 deletions(-) diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp index d5e5924..5980e90 100644 --- a/src/slave/slave.cpp +++ b/src/slave/slave.cpp @@ -1946,13 +1946,16 @@ void Slave::runTask( const ExecutorInfo executorInfo = getExecutorInfo(frameworkInfo, task); + bool executorGeneratedForCommandTask = !task.has_executor(); + run(frameworkInfo, executorInfo, task, None(), resourceVersionUuids, pid, - launchExecutor); + launchExecutor, + executorGeneratedForCommandTask); } @@ -1963,7 +1966,8 @@ void Slave::run( Option<TaskGroupInfo> taskGroup, const vector<ResourceVersionUUID>& resourceVersionUuids, const UPID& pid, - const Option<bool>& launchExecutor) + const Option<bool>& launchExecutor, + bool executorGeneratedForCommandTask) { CHECK_NE(task.isSome(), taskGroup.isSome()) << "Either task or task group should be set but not both"; @@ -2281,7 +2285,8 @@ void Slave::run( task, taskGroup, resourceVersionUuids, - launchExecutor)) + launchExecutor, + executorGeneratedForCommandTask)) .onFailed(defer(self(), [=](const string& failure) { Framework* _framework = getFramework(frameworkId); if (_framework == nullptr) { @@ -2511,7 +2516,8 @@ void Slave::__run( const Option<TaskInfo>& task, const Option<TaskGroupInfo>& taskGroup, const vector<ResourceVersionUUID>& resourceVersionUuids, - const Option<bool>& launchExecutor) + const Option<bool>& launchExecutor, + bool executorGeneratedForCommandTask) { CHECK_NE(task.isSome(), taskGroup.isSome()) << "Either task or task group should be set but not both"; @@ -2933,7 +2939,8 @@ void Slave::__run( // or we are in the legacy case of launching one if there wasn't // one already. Either way, let's launch executor now. if (executor == nullptr) { - Try<Executor*> added = framework->addExecutor(executorInfo); + Try<Executor*> added = + framework->addExecutor(executorInfo, executorGeneratedForCommandTask); if (added.isError()) { CHECK(framework->getExecutor(executorId) == nullptr); @@ -3598,13 +3605,17 @@ void Slave::runTaskGroup( return; } + // Executors for task groups are injected by the master, not the agent. + constexpr bool executorGeneratedForCommandTask = false; + run(frameworkInfo, executorInfo, None(), taskGroupInfo, resourceVersionUuids, UPID(), - launchExecutor); + launchExecutor, + executorGeneratedForCommandTask); } @@ -8998,7 +9009,9 @@ void Framework::checkpointFramework() const } -Try<Executor*> Framework::addExecutor(const ExecutorInfo& executorInfo) +Try<Executor*> Framework::addExecutor( + const ExecutorInfo& executorInfo, + bool isGeneratedForCommandTask) { // Verify that Resource.AllocationInfo is set, if coming // from a MULTI_ROLE master this will be set, otherwise @@ -9054,7 +9067,8 @@ Try<Executor*> Framework::addExecutor(const ExecutorInfo& executorInfo) containerId, directory.get(), user, - info.checkpoint()); + info.checkpoint(), + isGeneratedForCommandTask); if (executor->checkpoint) { executor->checkpointExecutor(); @@ -9249,7 +9263,8 @@ void Framework::recoverExecutor( latest, directory, info.user(), - info.checkpoint()); + info.checkpoint(), + state.generatedForCommandTask); // Recover the libprocess PID if possible for PID based executors. if (run->http.isSome()) { @@ -9593,7 +9608,8 @@ Executor::Executor( const ContainerID& _containerId, const string& _directory, const Option<string>& _user, - bool _checkpoint) + bool _checkpoint, + bool isGeneratedForCommandTask) : state(REGISTERING), slave(_slave), id(_info.executor_id()), @@ -9604,7 +9620,8 @@ Executor::Executor( user(_user), checkpoint(_checkpoint), http(None()), - pid(None()) + pid(None()), + isGeneratedForCommandTask_(isGeneratedForCommandTask) { CHECK_NOTNULL(slave); @@ -9621,16 +9638,6 @@ Executor::Executor( completedTasks = boost::circular_buffer<shared_ptr<Task>>(MAX_COMPLETED_TASKS_PER_EXECUTOR); - - // TODO(jieyu): The way we determine if an executor is generated for - // a command task (either command or docker executor) is really - // hacky. We rely on the fact that docker executor launch command is - // set in the docker containerizer so that this check is still valid - // in the slave. - // TODO(bbannier): Initialize this field with a value passed to constructor. - isGeneratedForCommandTask_ = - strings::endsWith(info.command().value(), MESOS_EXECUTOR) && - strings::startsWith(info.name(), "Command Executor"); } diff --git a/src/slave/slave.hpp b/src/slave/slave.hpp index c81e2d5..3a75d7b 100644 --- a/src/slave/slave.hpp +++ b/src/slave/slave.hpp @@ -171,7 +171,8 @@ public: Option<TaskGroupInfo> taskGroup, const std::vector<ResourceVersionUUID>& resourceVersionUuids, const process::UPID& pid, - const Option<bool>& launchExecutor); + const Option<bool>& launchExecutor, + bool executorGeneratedForCommandTask); // Made 'virtual' for Slave mocking. // @@ -195,7 +196,8 @@ public: const Option<TaskInfo>& task, const Option<TaskGroupInfo>& taskGroup, const std::vector<ResourceVersionUUID>& resourceVersionUuids, - const Option<bool>& launchExecutor); + const Option<bool>& launchExecutor, + bool executorGeneratedForCommandTask); // This is called when the resource limits of the container have // been updated for the given tasks and task groups. If the update is @@ -896,7 +898,8 @@ public: const ContainerID& containerId, const std::string& directory, const Option<std::string>& user, - bool checkpoint); + bool checkpoint, + bool isGeneratedForCommandTask); ~Executor(); @@ -1093,7 +1096,10 @@ public: const FrameworkID id() const { return info.id(); } - Try<Executor*> addExecutor(const ExecutorInfo& executorInfo); + Try<Executor*> addExecutor( + const ExecutorInfo& executorInfo, + bool isGeneratedForCommandTask); + Executor* getExecutor(const ExecutorID& executorId) const; Executor* getExecutor(const TaskID& taskId) const; diff --git a/src/tests/mock_slave.cpp b/src/tests/mock_slave.cpp index 94a5b0d..57b3bec 100644 --- a/src/tests/mock_slave.cpp +++ b/src/tests/mock_slave.cpp @@ -128,7 +128,7 @@ MockSlave::MockSlave( .WillRepeatedly(Invoke(this, &MockSlave::unmocked_runTask)); EXPECT_CALL(*this, _run(_, _, _, _, _, _)) .WillRepeatedly(Invoke(this, &MockSlave::unmocked__run)); - EXPECT_CALL(*this, __run(_, _, _, _, _, _)) + EXPECT_CALL(*this, __run(_, _, _, _, _, _, _)) .WillRepeatedly(Invoke(this, &MockSlave::unmocked___run)); EXPECT_CALL(*this, runTaskGroup(_, _, _, _, _, _)) .WillRepeatedly(Invoke(this, &MockSlave::unmocked_runTaskGroup)); @@ -213,7 +213,8 @@ void MockSlave::unmocked___run( const Option<TaskInfo>& task, const Option<TaskGroupInfo>& taskGroup, const std::vector<ResourceVersionUUID>& resourceVersionUuids, - const Option<bool>& launchExecutor) + const Option<bool>& launchExecutor, + bool executorGeneratedForCommandTask) { slave::Slave::__run( frameworkInfo, @@ -221,7 +222,8 @@ void MockSlave::unmocked___run( task, taskGroup, resourceVersionUuids, - launchExecutor); + launchExecutor, + executorGeneratedForCommandTask); } diff --git a/src/tests/mock_slave.hpp b/src/tests/mock_slave.hpp index 9a74bf3..817526a 100644 --- a/src/tests/mock_slave.hpp +++ b/src/tests/mock_slave.hpp @@ -151,13 +151,14 @@ public: const std::vector<ResourceVersionUUID>& resourceVersionUuids, const Option<bool>& launchExecutor); - MOCK_METHOD6(__run, void( + MOCK_METHOD7(__run, void( const FrameworkInfo& frameworkInfo, const ExecutorInfo& executorInfo, const Option<TaskInfo>& task, const Option<TaskGroupInfo>& taskGroup, const std::vector<ResourceVersionUUID>& resourceVersionUuids, - const Option<bool>& launchExecutor)); + const Option<bool>& launchExecutor, + bool executorGeneratedForCommandTask)); void unmocked___run( const FrameworkInfo& frameworkInfo, @@ -165,7 +166,8 @@ public: const Option<TaskInfo>& task, const Option<TaskGroupInfo>& taskGroup, const std::vector<ResourceVersionUUID>& resourceVersionUuids, - const Option<bool>& launchExecutor); + const Option<bool>& launchExecutor, + bool executorGeneratedForCommandTask); MOCK_METHOD6(runTaskGroup, void( const process::UPID& from,
