You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2018/02/05 02:49:18 UTC
Review Request 65504: Made master set `launch_executor` in the
RunTask(Group)Message.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65504/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
Bugs: MESOS-1720
https://issues.apache.org/jira/browse/MESOS-1720
Repository: mesos
Description
-------
By setting a new field `launch_executor` in the RunTask(Group)Message,
the master is able to control the executor creation on the agent.
Also refactored `addTask()` logic. Added two new functions:
`isTaskLaunchExecutor()` checks if a task needs to launch executor;
`addExecutor()` adds executor to the framework and slave.
Diffs
-----
src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2
src/master/master.cpp cc2685a6bc14103c639ce776cf1c912361e93381
Diff: https://reviews.apache.org/r/65504/diff/1/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 65504: Made master set `launch_executor` in the
RunTask(Group)Message.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Feb. 8, 2018, 12:03 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 5185-5186 (patched)
> > <https://reviews.apache.org/r/65504/diff/1/?file=1952699#file1952699line5203>
> >
> > this doesn't seem correct?
> >
> > shouldn't this be
> >
> > ```
> > CHECK(_offeredResources.contains(taskResources))
> > << _offeredResources << " does not contain " << taskResources;
> >
> > _offeredResources -= taskResources;
> >
> > ```
> >
> > also, you need to do another check for executor resources before the `foreach` loop
> >
> > ```
> > CHECK(_offeredResources.contains(executorResources))
> > << _offeredResources << " does not contain " << executorResources;
> >
> > _offeredResources -= executorResources;
> >
> > ```
The check is outside of the for loop. `totalResources` is incremented in the loop and we are subtracting it all at once outside of the loop.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65504/#review197099
-----------------------------------------------------------
On Feb. 4, 2018, 6:49 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65504/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2018, 6:49 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
>
>
> Bugs: MESOS-1720
> https://issues.apache.org/jira/browse/MESOS-1720
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By setting a new field `launch_executor` in the RunTask(Group)Message,
> the master is able to control the executor creation on the agent.
>
> Also refactored `addTask()` logic. Added two new functions:
> `isTaskLaunchExecutor()` checks if a task needs to launch executor;
> `addExecutor()` adds executor to the framework and slave.
>
>
> Diffs
> -----
>
> src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2
> src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94
>
>
> Diff: https://reviews.apache.org/r/65504/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 65504: Made master set `launch_executor` in the
RunTask(Group)Message.
Posted by Vinod Kone <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65504/#review197099
-----------------------------------------------------------
src/master/master.cpp
Line 3832 (original), 3832 (patched)
<https://reviews.apache.org/r/65504/#comment277210>
Can you add a comment here that for command tasks, even though an executor is launched by the agent master is oblivious to that fact? Alternatively, this should return true for command tasks.
My proposal:
```
bool islaunchExecutor(
const ExecutorID& executorId,
Framework* framework,
Slave* slave) {
CHECK_NOTNULL(framework);
CHECK_NOTNULL(slave);
if (!slave->hasExecutor(framework->id(), executorId())) {
CHECK(!framework->hasExecutor(slave->id, executorId()))
<< "Executor '" << executorId
<< "' known to the framework " << *framework
<< " but unknown to the agent " << *slave;
return true;
}
}
return false;
}
```
src/master/master.cpp
Lines 4922 (patched)
<https://reviews.apache.org/r/65504/#comment277211>
```
if (launchExecutor && task.has_executor()) {
addExecutor(task.executor(), framework, slave);
consumed += task.executor().resources();
}
```
src/master/master.cpp
Lines 4992 (patched)
<https://reviews.apache.org/r/65504/#comment277212>
s/message.launch_executor()/launchExecutor/
src/master/master.cpp
Lines 5154-5165 (patched)
<https://reviews.apache.org/r/65504/#comment277215>
```
bool launchExecutor = isLaunchExecutor(executor.executor_id(), framework, slave);
if (launchExecutor) {
addExecutor(executor, framework, slave);
executorResources = executor.resources();
totalResources += executorResources;
}
```
src/master/master.cpp
Lines 5185-5186 (patched)
<https://reviews.apache.org/r/65504/#comment277218>
this doesn't seem correct?
shouldn't this be
```
CHECK(_offeredResources.contains(taskResources))
<< _offeredResources << " does not contain " << taskResources;
_offeredResources -= taskResources;
```
also, you need to do another check for executor resources before the `foreach` loop
```
CHECK(_offeredResources.contains(executorResources))
<< _offeredResources << " does not contain " << executorResources;
_offeredResources -= executorResources;
```
src/master/master.cpp
Line 5159 (original), 5201 (patched)
<https://reviews.apache.org/r/65504/#comment277222>
if we do the changes above maybe we can just have `taskGroupResources` variable instead of `totalResources` and use it here?
src/master/master.cpp
Lines 5203 (patched)
<https://reviews.apache.org/r/65504/#comment277219>
s/message.launch_executor()/executor/
- Vinod Kone
On Feb. 5, 2018, 2:49 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65504/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2018, 2:49 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
>
>
> Bugs: MESOS-1720
> https://issues.apache.org/jira/browse/MESOS-1720
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By setting a new field `launch_executor` in the RunTask(Group)Message,
> the master is able to control the executor creation on the agent.
>
> Also refactored `addTask()` logic. Added two new functions:
> `isTaskLaunchExecutor()` checks if a task needs to launch executor;
> `addExecutor()` adds executor to the framework and slave.
>
>
> Diffs
> -----
>
> src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2
> src/master/master.cpp cc2685a6bc14103c639ce776cf1c912361e93381
>
>
> Diff: https://reviews.apache.org/r/65504/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 65504: Made master set `launch_executor` in the
RunTask(Group)Message.
Posted by Vinod Kone <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65504/#review197286
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.cpp
Lines 4920-4924 (original), 4919-4928 (patched)
<https://reviews.apache.org/r/65504/#comment277412>
I think this should be
```
bool launchExecutor = true;
if (task.has_executor()) {
launchExecutor = isLaunchExecutor(task.executor().executor_id(), framework, slave);
if (launchExecutor) {
addExecutor(task.executor(), framework, slave);
consumed += task.executor().resources()
}
}
```
otherwise, the log line below says we are launching a command task on an existing executor which is wrong.
- Vinod Kone
On Feb. 5, 2018, 2:49 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65504/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2018, 2:49 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
>
>
> Bugs: MESOS-1720
> https://issues.apache.org/jira/browse/MESOS-1720
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By setting a new field `launch_executor` in the RunTask(Group)Message,
> the master is able to control the executor creation on the agent.
>
> Also refactored `addTask()` logic. Added two new functions:
> `isTaskLaunchExecutor()` checks if a task needs to launch executor;
> `addExecutor()` adds executor to the framework and slave.
>
>
> Diffs
> -----
>
> src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2
> src/master/master.cpp d7d22866f7a4eb87bd8949efafc97e828e7d4b94
>
>
> Diff: https://reviews.apache.org/r/65504/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>