> On July 28, 2014, 9:33 p.m., Jie Yu wrote: > > src/slave/containerizer/docker.hpp, line 48 > > <https://reviews.apache.org/r/23771/diff/2/?file=638778#file638778line48> > > > > I don't see the implementation of this function?
Sorry it was used but then removed later, forgot to delete the def. > On July 28, 2014, 9:33 p.m., Jie Yu wrote: > > src/docker/docker.cpp, lines 212-216 > > <https://reviews.apache.org/r/23771/diff/3/?file=643404#file643404line212> > > > > Would you please add a TODO here. Ideally, we should use the argv > > version to avoid escaping and use something similar to 'shlex.split' to > > split the 'command' below. Sounds good, I think we should include a split utils or change our command inputs to a list of args instead. > On July 28, 2014, 9:33 p.m., Jie Yu wrote: > > src/slave/containerizer/docker.cpp, lines 839-840 > > <https://reviews.apache.org/r/23771/diff/3/?file=643413#file643413line839> > > > > Hum, this looks problematic to me. These two static variables will be > > initialized even before 'main' is called, at which time, the mesos > > containerizer (potentially) may not have initialized the cgroups mounts yet. I don't think we're relying on the Mesos Containerizer to initialize the cgroups mount though, as we call this when _update is called it should already been mounted. > On July 28, 2014, 9:33 p.m., Jie Yu wrote: > > src/slave/containerizer/docker.cpp, line 915 > > <https://reviews.apache.org/r/23771/diff/3/?file=643413#file643413line915> > > > > Probably we should comment about how OOM is handled. Does docker use > > the kernel oom killer? Do we need to check oom_control to verify that? > > Should we listen for oom events? I'm going to leave a TODO for OOM. > On July 28, 2014, 9:33 p.m., Jie Yu wrote: > > src/slave/containerizer/docker.cpp, line 1038 > > <https://reviews.apache.org/r/23771/diff/3/?file=643413#file643413line1038> > > > > Seems that we don't trigger any Limitation. I am curious when > > killed=true will be used? In other words, why do we need this additional > > parameter? Worth commenting it. This is set when the process is killed instead of being exited on itself, we keep this flag so in the end when we mark the termination status we can use it to set the killed field in containerizer::Termination > On July 28, 2014, 9:33 p.m., Jie Yu wrote: > > src/tests/docker_containerizer_tests.cpp, line 190 > > <https://reviews.apache.org/r/23771/diff/3/?file=643423#file643423line190> > > > > Is this the only place you want to use validate = false for > > Docker::create? If yes, I would suggest remove this parameter and you can do > > > > Docker::create(tests::flags.docker).get() > > > > here, and the internal check in 'get()' will make sure docker is > > available. Not sure how that change can skip validation, but I was trying to make the test run a bit faster by not validating on each Docker create since it's already been checked before. > On July 28, 2014, 9:33 p.m., Jie Yu wrote: > > src/tests/docker_containerizer_tests.cpp, lines 229-232 > > <https://reviews.apache.org/r/23771/diff/3/?file=643423#file643423line229> > > > > Probably add a comment here saying that test-executor will be > > downloaded. Have you consider making this test self contained? Thought about it, but the binary image itself isn't small and even including the DockerFile since incurrs a download no matter what. So in the end we end up having the smallest executor that we have atm which is a native executor written in go, that is only about 15 mb. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23771/#review48738 ----------------------------------------------------------- On July 28, 2014, 5:41 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23771/ > ----------------------------------------------------------- > > (Updated July 28, 2014, 5:41 a.m.) > > > Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu. > > > Repository: mesos-git > > > Description > ------- > > Docker implementation. > This is all the docker code Ben, I and Yifan worked on excluding the > composing containerizer patches. > > > Diffs > ----- > > src/Makefile.am 45afcd1 > src/common/status_utils.hpp 1980551 > src/docker/docker.hpp PRE-CREATION > src/docker/docker.cpp PRE-CREATION > src/examples/docker_no_executor_framework.cpp PRE-CREATION > src/health-check/main.cpp 707810a > src/launcher/executor.cpp 9c80848 > src/linux/cgroups.hpp decad9d > src/linux/cgroups.cpp 6a73dd7 > src/master/master.cpp 251b699 > src/slave/containerizer/containerizer.cpp 1b71f33 > src/slave/containerizer/docker.hpp PRE-CREATION > src/slave/containerizer/docker.cpp PRE-CREATION > src/slave/containerizer/external_containerizer.cpp 3f28d85 > src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b > src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80 > src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7 > src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35 > src/slave/containerizer/isolators/posix.hpp 17bbd10 > src/slave/flags.hpp 1fe7b7d > src/slave/slave.cpp f42ab60 > src/tests/cgroups_tests.cpp 01cf498 > src/tests/docker_containerizer_tests.cpp PRE-CREATION > src/tests/docker_tests.cpp PRE-CREATION > src/tests/environment.cpp 434b3f7 > src/tests/flags.hpp a003e7f > src/tests/script.cpp 15a6542 > src/usage/usage.hpp 5a76746 > src/usage/usage.cpp 29014d1 > > Diff: https://reviews.apache.org/r/23771/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
