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