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 72216: Added master validation for task resource limits and shared cgroups.
Date Thu, 19 Mar 2020 10:47:21 GMT

-----------------------------------------------------------
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
> 
>


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