mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 71858: WIP: Set resource limits when launching executor container.
Date Tue, 14 Jan 2020 06:53:54 GMT


> On Jan. 8, 2020, 5:43 a.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 3140-3201 (patched)
> > <https://reviews.apache.org/r/71858/diff/4/?file=2191535#file2191535line3140>
> >
> >     I think maybe this logic could be easier to read if we do something like:
> >     
> >     ```
> >     auto limitsAreSet = [](const vector<TaskInfo>& tasks) {
> >       foreach (const TaskInfo& task, tasks) {
> >         if (!task.limits().empty()) {
> >           return true;
> >         }
> >       }
> >       
> >       return false;
> >     };
> >     
> >     Option<Map<string, Value::Scalar>> executorLimits;
> >     if (limitsAreSet(tasks)) {
> >       executorLimits = Map<string, Value::Scalar>();
> >       foreach (const TaskInfo& task, tasks) {
> >         // Add up task limits/requests here.
> >       }
> >     }
> >     ```
> >     
> >     What do you think?
> 
> Qian Zhang wrote:
>     I am a bit confused how it will simplify the logic here, like: how will you do the
`Add up task limits/requests here`? I guess you still need the code from L3140 to L3201, right?
> 
> Greg Mann wrote:
>     Ah sorry, before I answer your question I have another one: currently, your code
will only set the executor's cpu limit if one or more tasks have a cpu limit set, and will
only set the executor's mem limit if one or more tasks have the memory limit set. However,
I think we also want to set the cpu limit if one or more tasks has a _memory_ limit set, and
we want to set the memory limit if one or more tasks has a _cpu_ limit set, right? This way,
if a single task under an executor sets either a cpu or memory limit, then all tasks will
have both the cpu and memory limit set (and if it wasn't specified for a particular task,
it will be set to the default, which is the value of the request).

So if a framework launches two tasks (t1 and t2) with a same executor, t1 has CPU limit specified
but no memory limit specified, t2 has both of the CPU and memory limits not specified, then
we should not only set CPU hard limit but also the memory hard limit (i.e., set it to t1's
memory request + t2's memory request) in the executor container's cgroups, right? I think
we have already done it because executor's resource requests always includes all task's resource
requests (see L3209 in this patch https://reviews.apache.org/r/71858/diff/6 ), and in the
memory cgroups (`memory.cpp`) we will set the executor container's memory hard limit (`memory.limit_in_bytes`)
to its memory request if its memory limit is not specified (see https://reviews.apache.org/r/71943/
for details).

And similarly if t1 has memory limit specified but no CPU limit specified, in the CPU cgroups
we will set the executor container's CPU hard limit (CFS quota) to the executor's CPU request
if `--cgroups_enable_cfs` is true.


- Qian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71858/#review219154
-----------------------------------------------------------


On Jan. 8, 2020, 10:40 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2020, 10:40 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10046
>     https://issues.apache.org/jira/browse/MESOS-10046
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> WIP: Set resource limits when launching executor container.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb 
>   src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/6/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message