----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70757/#review216599 -----------------------------------------------------------
This looks like it is in good shape, just some final nits. Looking forward to a later patch to add documentation and changelog updates :) Please rebase and check the commit series. The review desn't apply because the nnp isolator test are a patch rather than a new file: ``` $ ./support/apply-reviews.py -r 70757 2019-07-14 17:03:13 URL:https://reviews.apache.org/r/70757/diff/raw/ [12172/12172] -> "70757.patch" [1] error: patch failed: src/slave/containerizer/mesos/launch.cpp:57 error: src/slave/containerizer/mesos/launch.cpp: patch does not apply error: src/tests/containerizer/linux_nnp_isolator_tests.cpp: does not exist in index ``` I think that this is also why whic review ends up depending on itself when you update it. src/slave/containerizer/mesos/isolators/linux/nnp.hpp Lines 46 (patched) <https://reviews.apache.org/r/70757/#comment303799> We don't need to store a copy of the flags, which also means you can remove the constructor. src/slave/containerizer/mesos/isolators/linux/nnp.cpp Lines 22 (patched) <https://reviews.apache.org/r/70757/#comment303800> Just 1 empty line here. src/slave/containerizer/mesos/launch.cpp Lines 1162 (patched) <https://reviews.apache.org/r/70757/#comment303801> As per Andrei, if the message field is present, toggle the NNP flag depending on its value. e.g. ``` int value = launchInfo.no_new_privileges() ? 1 : 0; prctl(PR_SET_NO_NEW_PRIVS, value, 0, 0, 0) ``` src/slave/containerizer/mesos/launch.cpp Lines 1163 (patched) <https://reviews.apache.org/r/70757/#comment303798> We should include `os::strerror(errno)` here: ``` cerr << "foo foo foo: " << os::strerror(errno) << endl; ``` - James Peach On July 11, 2019, 11:14 p.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70757/ > ----------------------------------------------------------- > > (Updated July 11, 2019, 11:14 p.m.) > > > Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James > Peach. > > > Bugs: MESOS-9770 > https://issues.apache.org/jira/browse/MESOS-9770 > > > Repository: mesos > > > Description > ------- > > This patch adds the isolation capability of flipping the > NO_NEW_PRIVILEGES bit for process control. > > > Diffs > ----- > > include/mesos/slave/containerizer.proto > 2d04f3c182a4274dda527a3da56c894c3c892a12 > src/CMakeLists.txt eb4e2ac699121ac16e2ac58e4606b26aae906ad8 > src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 > src/slave/containerizer/mesos/containerizer.cpp > c9a369bcdbfc1676912430c3e84fa567bbd7f766 > src/slave/containerizer/mesos/isolators/linux/nnp.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/linux/nnp.cpp PRE-CREATION > src/slave/containerizer/mesos/launch.cpp > 0419e8e0fbf154396ab5fb5026d77b94cc9fca5b > src/tests/CMakeLists.txt 5eb9c65f30d9d6ade289f9d47b18d73908b1f1db > src/tests/containerizer/linux_nnp_isolator_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70757/diff/7/ > > > Testing > ------- > > > Thanks, > > Jacob Janco > >
