mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 53062: Add rlimit support to Mesos containerizer.
Date Thu, 20 Oct 2016 22:39:09 GMT


> On Oct. 20, 2016, 9:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, lines 592-603
> > <https://reviews.apache.org/r/53062/diff/1/?file=1542360#file1542360line592>
> >
> >     This won't work because data services explicitly want to get limit above what's
configured.

Wouldn't we poke a giant hole into the system if we allowed unpriviledged tasks to set arbitrary
rlimits from potentially `root` the agent might be running as without any checks on the agent
side? AFAICT above code allows non-priviledged tasks to only lower limits, while priviledged
tasks can still set any limits, which should be safe and enables rlimits for a large class
of frameworks.

Note that we set rlimits before we potentially drop capabilities like `CAP_SYS_RESOURCE`.
I now mention this fact explicitly in the comment.

Once we implement agent functionality to check against limiting rlimits we might be able to
open up above restriction.


> On Oct. 20, 2016, 9:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 105
> > <https://reviews.apache.org/r/53062/diff/1/?file=1542360#file1542360line105>
> >
> >     That makes me think if we should make 'unlimited' be more explicit. With the
current scheme, unlimited of RLIMIT_FSIZE will be represented as:
> >     ```
> >     '{"rlimits":[{"type":"RLMT_FSIZE"}]}'
> >     ```
> >     
> >     I am wondering if we should introduce an optional `unlimited` field:
> >     ```
> >     message RLimit {
> >       optional Type type = 1;
> >       optional uint64_t soft = 2;
> >       optional uint64_t hard = 3;
> >       optional bool unlimited = 4;
> >     }
> >     
> >     '{"rlimits":[{"type":"RLMT_FSIZE","unlimited":true}]}'
> >     ```

I agree that the serialized form of this message reads better with `unlimited`.

At the same time this format leads to a potential explosion of complexity, e.g., combinations
without `unlimited` would be

* both `soft` and `hard` are set (1 case, valid)
* neither `soft` and `hard` are set (1 case, valid)
* one of `{soft, hard}` is set, the other one unset (2 cases, invalid)

while with `unlimited` we could have

* both `soft` and `hard` are set, `unlimited` unset (1 case, valid)
* neither `soft` and `hard` are set, `unlimited` set to `true` (1 case, valid)
* one of `{soft, hard}` is set, the other one unset, any state for `unlimited` (4 cases, invalid)
* `unlimited` is `false`, any states for `soft` or `hard` (5 cases, invalid)

It seems it might be more straight-forward to document above string serialization when `RLimit`s
are used on e.g., CLI interfaces, than to expose users to the added complexity with `unlimited`.


- Benjamin


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


On Oct. 21, 2016, 12:38 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53062/
> -----------------------------------------------------------
> 
> (Updated Oct. 21, 2016, 12:38 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6426
>     https://issues.apache.org/jira/browse/MESOS-6426
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit adds a new launch flag `--rlimits` which can be used to
> specify POSIX resource limits for the container. The resource limits
> are set as the user, so to increase resource limits beyond configured
> system limits additional priviledges might be needed.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.hpp f8bac0650965a49562b9910bf6140ded8dbb69ac 
>   src/slave/containerizer/mesos/launch.cpp 4a41aaf103f5a9bc6f7a798f63f491fc7cf11f7e 
> 
> Diff: https://reviews.apache.org/r/53062/diff/
> 
> 
> Testing
> -------
> 
> Tested as part of https://reviews.apache.org/r/53078/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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