----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50271/#review149587 -----------------------------------------------------------
Haven't looked at the tests yet. src/Makefile.am (line 833) <https://reviews.apache.org/r/50271/#comment217232> This should belong linux files below? src/Makefile.am (line 958) <https://reviews.apache.org/r/50271/#comment217233> Ditto. src/slave/containerizer/mesos/containerizer.cpp (line 73) <https://reviews.apache.org/r/50271/#comment217234> Hum, the headers here is crazy (too many ifdef linux here). The order of this is not correct. I suggest we group all linux related headers together. src/slave/containerizer/mesos/containerizer.cpp (line 99) <https://reviews.apache.org/r/50271/#comment217235> Not yours, but why this is not wrapped with ifdef linux? src/slave/containerizer/mesos/containerizer.cpp (lines 1242 - 1251) <https://reviews.apache.org/r/50271/#comment217238> I understand why you want to do this check, but we usually do not do such checks in containerizer. We rely on operator to properly configure the agent. The correct solution I believe is to advertise agent isolation capabilities to the master and let master reject those tasks. I would simply remove this check here for now. src/slave/containerizer/mesos/isolators/linux/capabilities.hpp (line 17) <https://reviews.apache.org/r/50271/#comment217239> LINUX_CAPABILITIES src/slave/containerizer/mesos/isolators/linux/capabilities.cpp (line 40) <https://reviews.apache.org/r/50271/#comment217240> Given that we don't need to do the check in the agent, please just inline this method. src/slave/containerizer/mesos/isolators/linux/capabilities.cpp (line 94) <https://reviews.apache.org/r/50271/#comment217252> I don't follow the if/else here. Should we simply calculate "what will be the capabilities for the task/executor, depending on what is inside containerConfig.container_info() and flags.allowed_capabilities" first, and then, depending on if the container is a command task (w or w/ rootfs) or custom executor, setting launchInfo accordingly? src/slave/containerizer/mesos/isolators/linux/capabilities.cpp (lines 103 - 104) <https://reviews.apache.org/r/50271/#comment217251> Hum, if i specify `--allowed_capabilities` and the framework does not specify capabilities for TaskInfo.container. Will this work? I think you need to consider three cases here: 1) For command task that has rootfs. In that case, we need to make sure when we launch the executor, we don't specify capabilities (keep it None() in launchInfo, indicating that we don't want to limit capabilities). This is because the executor will perform priviledged operations like pivot_root and setuid, we want to make sure it is not restricted. 2) For command task that has no rootfs. In that case, simply set launchInfo.capabilities, meaning that when launching the executor, we limit the capabilities of it. So the task will inherit the capabilities when the executor fork-exec the task. 3) For custom executor case, simply set launchInfo.capabilities. To check if it is a command task is by checking containerConfig.has_task_info(). - Jie Yu On Sept. 19, 2016, 2:21 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50271/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2016, 2:21 p.m.) > > > Review request for mesos, Jay Guo and Jie Yu. > > > Bugs: MESOS-5275 > https://issues.apache.org/jira/browse/MESOS-5275 > > > Repository: mesos > > > Description > ------- > > This isolator evaluates agent allowed capabilities and passes > net capabilities on to `mesos-containerizer` which enforces the > capabilities. > > Capability information is passed from the isolator via a new > capability field in `ContainerLaunchInfo`. > > > Diffs > ----- > > include/mesos/slave/containerizer.proto > 20db010ea158a813034b411111ce9cddac7d8317 > src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 > src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd > src/slave/containerizer/mesos/containerizer.cpp > e54169ba00f6e0cdd7043075b4cdd12d714c99e3 > src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION > src/slave/containerizer/mesos/launch.cpp > fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 > src/tests/containerizer/isolator_tests.cpp > 93ce75180520d382f63ce7323be844fe43c6594e > > Diff: https://reviews.apache.org/r/50271/diff/ > > > Testing > ------- > > `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o > optimizations) > > > Thanks, > > Benjamin Bannier > >
