----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25865/#review54637 -----------------------------------------------------------
src/slave/containerizer/isolators/namespaces/pid.hpp <https://reviews.apache.org/r/25865/#comment94869> s/NamespacesPid/PidNamespace/ ? src/slave/containerizer/isolators/namespaces/pid.hpp <https://reviews.apache.org/r/25865/#comment94870> kill new line. src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment94871> Comment? src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment94873> As mentioned in the previous review, instead of requiring users/operators to know this dependency, we should just automatically use fileystem/shared isoator when using pid or network isolation. src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment94877> Who is calling this method? src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment94882> // Cleanup orphans. ? src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment94880> if you use hashset, you can just do !containerers.contain(). src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment94883> Why not just call cleanup() here? src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment94881> Can you also say why in the comment? Presumably because you dont want containers to see other containers runninng in the system? src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment94884> Add a comment that you are doing this for the ability to cleanup orphans during recovery? Also, what is the need for manual cleanup or orphans? src/slave/containerizer/linux_launcher.cpp <https://reviews.apache.org/r/25865/#comment94867> why is this pulled out? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25865/#comment94885> s/NamespacesPidIsolatorTest/PidNamespaceIsolatorTest/ src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25865/#comment94886> you are writing to files, not stdout and stderr right? - Vinod Kone On Sept. 23, 2014, 11:39 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25865/ > ----------------------------------------------------------- > > (Updated Sept. 23, 2014, 11:39 p.m.) > > > Review request for mesos, Jie Yu and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > Add namespaces/pid to --isolation slave flag. Places executor into a pid > namespace so it and all descendants will be contained in the namespace. > Requires the filesystem/shared isolator so /proc and /sys are remounted to > reflect the different namespace. > > > Diffs > ----- > > src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 > src/slave/containerizer/isolators/filesystem/shared.cpp PRE-CREATION > src/slave/containerizer/isolators/namespaces/pid.hpp PRE-CREATION > src/slave/containerizer/isolators/namespaces/pid.cpp PRE-CREATION > src/slave/containerizer/linux_launcher.cpp > f7bc894830a7ca3f55465dacc7b653cdc2d7758b > src/slave/containerizer/mesos/containerizer.cpp > 9d083294caa5c5a47ba3ceaa1b57346144cb795c > src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 > > Diff: https://reviews.apache.org/r/25865/diff/ > > > Testing > ------- > > Added test that command in pid namespaced container is in a different > namespace and that the command is 'init' (verifies remount of /proc). > > > Thanks, > > Ian Downes > >
