You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2020/03/09 15:25:40 UTC
Review Request 72216: Added master validation for task resource
limits and shared cgroups.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/1/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 19, 2020, 10:47 a.m., Qian Zhang wrote:
> > src/master/validation.cpp
> > Lines 2072 (patched)
> > <https://reviews.apache.org/r/72216/diff/5/?file=2214199#file2214199line2072>
> >
> > Is it possible that `shareCgroups.isSome()` is false at this point? I think as long as the task cgroup is not empty (at least contains one task), `shareCgroups.isSome()` must be true, right? So maybe we should do `CHECK_SOME(shareCgroups)`?
I think that for a custom executor, LAUNCH_GROUP with no tasks may be a valid use case?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219999
-----------------------------------------------------------
On March 19, 2020, 1:14 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 19, 2020, 1:14 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 19, 2020, 10:47 a.m., Qian Zhang wrote:
> > src/master/validation.cpp
> > Lines 1839-1845 (patched)
> > <https://reviews.apache.org/r/72216/diff/5/?file=2214199#file2214199line1839>
> >
> > So we have this validation only for the tasks within a task group but not for the tasks launched by `LAUNCH` operation? That means for the task whose `share_cgroups` is true, if it is launched by `LAUNCH_GROUP` then it is not allowed to specify resource limits, but if it is launched by `LAUNCH` then it is allowed to specify resource limits, this may confuse users, do we want to make them consistent? Like always allow to specify resource limits for any tasks. If we do this, then I think we may need to update the comments of the `TaskInfo.limits` and `Task.limits` protobuf message fields, since that field may have different meaning depending on the value of `share_cgroups`, like if `share_cgroups==false`, it is the limits of the task itself, otherwise the task may share limits with other tasks launched by the same executor.
I think we talked a lot during the design stage about why it doesn't make sense for resource limits to be set on task groups with share_cgroups==true. In hindsight, I wish we could have restricted this API to work only for task groups, but maybe that's not feasible. I would definitely prefer to still not allow task groups to set limits when share_cgroups==true.
For most users (i.e. users who don't use custom executors), launching task groups is the only time that they will send multiple tasks/task groups to a single executor, so I think that's the case we really care about. I can update the comments accordingly.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219999
-----------------------------------------------------------
On March 19, 2020, 2:10 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 19, 2020, 2:10 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 19, 2020, 10:47 a.m., Qian Zhang wrote:
> > src/master/validation.cpp
> > Lines 2072 (patched)
> > <https://reviews.apache.org/r/72216/diff/5/?file=2214199#file2214199line2072>
> >
> > Is it possible that `shareCgroups.isSome()` is false at this point? I think as long as the task cgroup is not empty (at least contains one task), `shareCgroups.isSome()` must be true, right? So maybe we should do `CHECK_SOME(shareCgroups)`?
>
> Greg Mann wrote:
> I think that for a custom executor, LAUNCH_GROUP with no tasks may be a valid use case?
I'll add master validation for this in a subsequent patch.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219999
-----------------------------------------------------------
On March 19, 2020, 1:14 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 19, 2020, 1:14 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 19, 2020, 10:47 a.m., Qian Zhang wrote:
> > src/master/validation.cpp
> > Lines 2073 (patched)
> > <https://reviews.apache.org/r/72216/diff/5/?file=2214199#file2214199line2073>
> >
> > I think we should not check all tasks of the framework, instead we should only check the tasks launched by the same executor on this specific slave for this specific framework. Maybe we should go through `slave->tasks.at(framework.id())` and check each task launched by the same executor. Or we could do this validation in Mesos agent?
Whoops sorry, I had that check in before and lost it while I was refactoring :facepalm:
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219999
-----------------------------------------------------------
On March 19, 2020, 1:14 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 19, 2020, 1:14 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219999
-----------------------------------------------------------
src/master/validation.cpp
Lines 1839-1841 (patched)
<https://reviews.apache.org/r/72216/#comment308245>
What if the task does not set `ContainerInfo` at all? In that case, I think task's `share_cgroups` is also true (since it is true by default) and we do not want it to have resource limits set as well, right?
src/master/validation.cpp
Lines 1839-1845 (patched)
<https://reviews.apache.org/r/72216/#comment308246>
So we have this validation only for the tasks within a task group but not for the tasks launched by `LAUNCH` operation? That means for the task whose `share_cgroups` is true, if it is launched by `LAUNCH_GROUP` then it is not allowed to specify resource limits, but if it is launched by `LAUNCH` then it is allowed to specify resource limits, this may confuse users, do we want to make them consistent? Like always allow to specify resource limits for any tasks. If we do this, then I think we may need to update the comments of the `TaskInfo.limits` and `Task.limits` protobuf message fields, since that field may have different meaning depending on the value of `share_cgroups`, like if `share_cgroups==false`, it is the limits of the task itself, otherwise the task may share limits with other tasks launched by the same executor.
src/master/validation.cpp
Lines 2030-2036 (patched)
<https://reviews.apache.org/r/72216/#comment308239>
I think we can change this block to:
```
bool taskShareCgroups =
(container.isSome() &&
container->has_linux_info() &&
container->linux_info().has_share_cgroups()) ?
container->linux_info().share_cgroups() :
true;
```
src/master/validation.cpp
Lines 2054-2055 (patched)
<https://reviews.apache.org/r/72216/#comment308240>
A newline between.
src/master/validation.cpp
Lines 2059-2064 (patched)
<https://reviews.apache.org/r/72216/#comment308241>
I see the caller of this method already have a slave pointer `Slave* slave`, so maybe we can just pass `slave->id` into this method instead of getting it from task?
And in another hand, it seems we do not validate a task group must contain at least one task (looks like an existing bug), so if framework launches an empty task group, `CHECK_SOME(slaveId);` will fail.
src/master/validation.cpp
Lines 2072 (patched)
<https://reviews.apache.org/r/72216/#comment308242>
Is it possible that `shareCgroups.isSome()` is false at this point? I think as long as the task cgroup is not empty (at least contains one task), `shareCgroups.isSome()` must be true, right? So maybe we should do `CHECK_SOME(shareCgroups)`?
src/master/validation.cpp
Lines 2073 (patched)
<https://reviews.apache.org/r/72216/#comment308243>
I think we should not check all tasks of the framework, instead we should only check the tasks launched by the same executor on this specific slave for this specific framework. Maybe we should go through `slave->tasks.at(framework.id())` and check each task launched by the same executor. Or we could do this validation in Mesos agent?
- Qian Zhang
On March 19, 2020, 10:10 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 19, 2020, 10:10 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review220034
-----------------------------------------------------------
Ship it!
Ship It!
- Qian Zhang
On March 20, 2020, 2:42 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 20, 2020, 2:42 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/9/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
(Updated March 20, 2020, 6:42 a.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs (updated)
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/9/
Changes: https://reviews.apache.org/r/72216/diff/8-9/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
(Updated March 20, 2020, 3:48 a.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs (updated)
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/8/
Changes: https://reviews.apache.org/r/72216/diff/7-8/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
(Updated March 19, 2020, 5:53 p.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs (updated)
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/7/
Changes: https://reviews.apache.org/r/72216/diff/6-7/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 19, 2020, 2:40 p.m., Qian Zhang wrote:
> > src/master/validation.cpp
> > Lines 2073-2085 (patched)
> > <https://reviews.apache.org/r/72216/diff/6/?file=2214259#file2214259line2073>
> >
> > What if the framework launches multiple executors with the same executor ID on multiple slaves? I think we want to do this validation for the specific slave rather than across slaves.
I'll iterate over tasks in the slave struct instead.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review220008
-----------------------------------------------------------
On March 19, 2020, 1:14 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 19, 2020, 1:14 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 19, 2020, 2:40 p.m., Qian Zhang wrote:
> > src/master/validation.cpp
> > Lines 2073-2085 (patched)
> > <https://reviews.apache.org/r/72216/diff/6/?file=2214259#file2214259line2073>
> >
> > What if the framework launches multiple executors with the same executor ID on multiple slaves? I think we want to do this validation for the specific slave rather than across slaves.
>
> Greg Mann wrote:
> I'll iterate over tasks in the slave struct instead.
I did this for active tasks, but unfortunately unreachable tasks are held in the `Slaves` struct. Since that struct is a member of `Master`, it becomes a little annoying to try to make use of it in this file, so I did not update the loop here over unreachable tasks.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review220008
-----------------------------------------------------------
On March 19, 2020, 5:53 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 19, 2020, 5:53 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/7/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review220008
-----------------------------------------------------------
src/master/validation.cpp
Lines 2015-2017 (patched)
<https://reviews.apache.org/r/72216/#comment308270>
No need this anymore.
src/master/validation.cpp
Lines 2073-2085 (patched)
<https://reviews.apache.org/r/72216/#comment308271>
What if the framework launches multiple executors with the same executor ID on multiple slaves? I think we want to do this validation for the specific slave rather than across slaves.
- Qian Zhang
On March 19, 2020, 9:14 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 19, 2020, 9:14 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
(Updated March 19, 2020, 1:14 p.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs (updated)
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/6/
Changes: https://reviews.apache.org/r/72216/diff/5-6/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
(Updated March 19, 2020, 2:10 a.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs (updated)
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/5/
Changes: https://reviews.apache.org/r/72216/diff/4-5/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Qian Zhang <zh...@gmail.com>.
> On March 15, 2020, 5:04 p.m., Qian Zhang wrote:
> > src/master/validation.cpp
> > Lines 1554-1561 (patched)
> > <https://reviews.apache.org/r/72216/diff/4/?file=2213417#file2213417line1554>
> >
> > What about the task which does not set `share_cgroups` (or even not set `ContainerInfo` at all)? In that case, I think its `share_cgroups` is also true (since it is true by default), right?
> >
> > And I think this validation will affect the command task case because command task must have `share_cgroups` as true and it is allowed to specify resource limits for it, right? So I think even a task's `share_cgroup` is true, we should still allow it to have resource limits specified if it is the first task of the executor, but we should disallow if it is the second task no matter whether the first task has resource limits specified or not because otherwise the second task's resource limits may not be enforced.
> >
> > But it seems hard to find out whether it is the first task of the executor. So another option could be, we just do not do this validation at all, i.e., a task can always specify a resource limits regardless the value of its `share_cgroups`, so task's resource limits will actually have two differen meanings:
> > 1. If task's `share_cgroups` is false, then the resource limits are its own limits.
> > 2. If task's `share_cgroups` is true, its resource limits will contribute into the executor's resource limits, all tasks of an executor will share the same resource limits which is the sum of all task's resource limits.
> >
> > For #2, actually that is what we are doing in our current implemention, e.g., if a customer executor launches multiple tasks, all these tasks will just share the same resource limits, i.e. the customer executor's resource limits (yes, we indeed always set memory hard limit to memory request and set CPU hard limit to CPU request if `--cgroups_enable_cfs` is true in our current implementation). So #2 sounds not that bad since it is backward compatibile?
Or in MVP, we could make an exception here for the command task (i.e. `!task.has_executor()`)? Like it is allowed to specify resource limits for command tasks, but not allowed for any other tasks whose `share_cgroup` is true.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219933
-----------------------------------------------------------
On March 12, 2020, 3:10 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 12, 2020, 3:10 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 15, 2020, 9:04 a.m., Qian Zhang wrote:
> > src/master/validation.cpp
> > Lines 1574-1577 (patched)
> > <https://reviews.apache.org/r/72216/diff/4/?file=2213417#file2213417line1574>
> >
> > Why do we want to return error if `taskCpus` is none? The purpose of this `validateResourceLimits()` method should be validating resource limits rather than resource requests.
This resource request validation is related to the limits: we don't support tasks which specify a resource limit without an associated resource request. I updated the patch to return a separate error message for that case. In the design doc, we list "best effort" tasks as a non-goal; IMO, a best effort task would correspond to a task with a resource limit and no resource request, so I don't think we intend to support that case.
> On March 15, 2020, 9:04 a.m., Qian Zhang wrote:
> > src/master/validation.cpp
> > Lines 1832-1838 (patched)
> > <https://reviews.apache.org/r/72216/diff/4/?file=2213417#file2213417line1832>
> >
> > The new method `validateResourceLimits` that you introduced has already done this validation and that method will be called for both the tasks launched via `LAUNCH` operation and the tasks launched via `LAUNCH_GROUP` operation, so I think we do not need to do this validation again here.
I removed that block from `validateResourceLimits()` after reading your comment on that section, so I'm going to leave this here to catch this case for task groups.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219933
-----------------------------------------------------------
On March 11, 2020, 7:10 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 11, 2020, 7:10 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219933
-----------------------------------------------------------
src/master/validation.cpp
Lines 1554-1561 (patched)
<https://reviews.apache.org/r/72216/#comment308164>
What about the task which does not set `share_cgroups` (or even not set `ContainerInfo` at all)? In that case, I think its `share_cgroups` is also true (since it is true by default), right?
And I think this validation will affect the command task case because command task must have `share_cgroups` as true and it is allowed to specify resource limits for it, right? So I think even a task's `share_cgroup` is true, we should still allow it to have resource limits specified if it is the first task of the executor, but we should disallow if it is the second task no matter whether the first task has resource limits specified or not because otherwise the second task's resource limits may not be enforced.
But it seems hard to find out whether it is the first task of the executor. So another option could be, we just do not do this validation at all, i.e., a task can always specify a resource limits regardless the value of its `share_cgroups`, so task's resource limits will actually have two differen meanings:
1. If task's `share_cgroups` is false, then the resource limits are its own limits.
2. If task's `share_cgroups` is true, its resource limits will contribute into the executor's resource limits, all tasks of an executor will share the same resource limits which is the sum of all task's resource limits.
For #2, actually that is what we are doing in our current implemention, e.g., if a customer executor launches multiple tasks, all these tasks will just share the same resource limits, i.e. the customer executor's resource limits (yes, we indeed always set memory hard limit to memory request and set CPU hard limit to CPU request if `--cgroups_enable_cfs` is true in our current implementation). So #2 sounds not that bad since it is backward compatibile?
src/master/validation.cpp
Lines 1574-1577 (patched)
<https://reviews.apache.org/r/72216/#comment308165>
Why do we want to return error if `taskCpus` is none? The purpose of this `validateResourceLimits()` method should be validating resource limits rather than resource requests.
src/master/validation.cpp
Lines 1582-1587 (patched)
<https://reviews.apache.org/r/72216/#comment308166>
Ditto.
src/master/validation.cpp
Lines 1832-1838 (patched)
<https://reviews.apache.org/r/72216/#comment308167>
The new method `validateResourceLimits` that you introduced has already done this validation and that method will be called for both the tasks launched via `LAUNCH` operation and the tasks launched via `LAUNCH_GROUP` operation, so I think we do not need to do this validation again here.
src/master/validation.cpp
Lines 2008 (patched)
<https://reviews.apache.org/r/72216/#comment308168>
I think just checking the tasks launched by this single `LAUNCH_GROUP` operation is not enough, we should also check the existing tasks launched by this executor before.
- Qian Zhang
On March 12, 2020, 3:10 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 12, 2020, 3:10 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
(Updated March 11, 2020, 7:10 p.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs (updated)
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/4/
Changes: https://reviews.apache.org/r/72216/diff/3-4/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 10, 2020, 1:06 p.m., Andrei Budnik wrote:
> > src/master/validation.cpp
> > Lines 1567 (patched)
> > <https://reviews.apache.org/r/72216/diff/3/?file=2213343#file2213343line1567>
> >
> > `if (limits.size() > cpuCount + memCount)` might be slightly easier to understand.
Good call; done.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219868
-----------------------------------------------------------
On March 11, 2020, 7:10 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 11, 2020, 7:10 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219868
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
src/master/validation.cpp
Lines 1567 (patched)
<https://reviews.apache.org/r/72216/#comment308115>
`if (limits.size() > cpuCount + memCount)` might be slightly easier to understand.
src/master/validation.cpp
Lines 1999 (patched)
<https://reviews.apache.org/r/72216/#comment308114>
Let's declarate `shareCgroups` variable right before the `foreach` loop.
The comment above can be updated as we no longer compare values of `share_cgroups` field between executor container and its task containers.
- Andrei Budnik
On Март 10, 2020, 9:42 д.п., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated Март 10, 2020, 9:42 д.п.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
(Updated March 10, 2020, 9:42 a.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs (updated)
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/3/
Changes: https://reviews.apache.org/r/72216/diff/2-3/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
(Updated March 10, 2020, 9:31 a.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs (updated)
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/2/
Changes: https://reviews.apache.org/r/72216/diff/1-2/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
(Updated March 10, 2020, 8:56 a.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/1/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/
-----------------------------------------------------------
(Updated March 10, 2020, 8:56 a.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10045
https://issues.apache.org/jira/browse/MESOS-10045
Repository: mesos
Description
-------
Added master validation for task resource limits and shared cgroups.
Diffs
-----
src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
Diff: https://reviews.apache.org/r/72216/diff/1/
Testing
-------
Thanks,
Greg Mann
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 9, 2020, 9:35 p.m., Andrei Budnik wrote:
> > src/master/validation.cpp
> > Lines 1991 (patched)
> > <https://reviews.apache.org/r/72216/diff/1/?file=2213311#file2213311line1991>
> >
> > `shareCgroups` can be initialized with the value taken from the first task within a task group. This may simplify the `foreach` loop. Also, this would allow us to move `if (currentShareCgroups && !task.limits().empty` check outside the loop.
I ended up moving the share_cgroups validation to the `validateTask()` function which is used for task group tasks, let me know what you think.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219855
-----------------------------------------------------------
On March 10, 2020, 9:42 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 10, 2020, 9:42 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 9, 2020, 9:35 p.m., Andrei Budnik wrote:
> > src/master/validation.cpp
> > Lines 1567 (patched)
> > <https://reviews.apache.org/r/72216/diff/1/?file=2213311#file2213311line1567>
> >
> > Can a client set a limit for one parameter only (e.g., "cpus")?
Yes they can; AFAICT this validation does not prevent that.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219855
-----------------------------------------------------------
On March 10, 2020, 9:42 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated March 10, 2020, 9:42 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72216: Added master validation for task resource
limits and shared cgroups.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72216/#review219855
-----------------------------------------------------------
src/master/validation.cpp
Lines 1567 (patched)
<https://reviews.apache.org/r/72216/#comment308101>
Can a client set a limit for one parameter only (e.g., "cpus")?
src/master/validation.cpp
Lines 1583 (patched)
<https://reviews.apache.org/r/72216/#comment308100>
Consider using `Bytes::MEGABYTES` instead of `(1024 * 1024)`
src/master/validation.cpp
Lines 1991 (patched)
<https://reviews.apache.org/r/72216/#comment308102>
`shareCgroups` can be initialized with the value taken from the first task within a task group. This may simplify the `foreach` loop. Also, this would allow us to move `if (currentShareCgroups && !task.limits().empty` check outside the loop.
- Andrei Budnik
On Март 9, 2020, 3:25 п.п., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> -----------------------------------------------------------
>
> (Updated Март 9, 2020, 3:25 п.п.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added master validation for task resource limits and shared cgroups.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2
>
>
> Diff: https://reviews.apache.org/r/72216/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>