----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23771/#review48738 -----------------------------------------------------------
src/slave/containerizer/docker.hpp <https://reviews.apache.org/r/23771/#comment85484> I don't see the implementation of this function? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85486> This CHECK seems to be redundant as you just have 'if (!run.get().forkedPid.isSome())' above. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85487> This log seems to be redundant as the caller should print the error. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85488> Use a helper function to construct the docker container name. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85489> Ditto. Remove this redundant log line. src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment85600> Move ')' above. Also, use const ref for parameters. Finally, remove ';' at the end. Container( const std::string& _id, const std::string& _name, const Option<pid_t> _pid) : id(_id), name(_name), pid(_pid) {} src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85601> Add a new line above. We add a new line if two headers are not in the same directory. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85602> Instead of putting all the validation code in to a giant if block, how about this: if (!validate) { return Docker(path); } // Validation code. return Docker(path); src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85604> // TODO(yifan): Reload ... above. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85607> Kill this empty line. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85610> 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. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85615> why not use 'cmd' here? src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85619> Remove const for primitive types. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85623> Move this to the bottom (or top) so that inspect and _inspect are close together for better readability. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85612> if (status.isNone()) { return false; } if (status.get() != 0) { LOG(WARNING) << cmd << " failed (" << WSTRINGIFY(status.get()) << ")"; return false; } return true; src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85626> Should we also have a warning or error message if status is None? src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85625> Kill the empty line here. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85620> Remove const. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85627> Ditto. Should we handle status.isNone()? src/examples/docker_no_executor_framework.cpp <https://reviews.apache.org/r/23771/#comment85628> Docker No Executor Framework (C++) ? src/linux/cgroups.cpp <https://reviews.apache.org/r/23771/#comment85630> Kill the empty line here. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85632> Does 'parse' need to be a member function? What about making them a static function in the cpp file, and do not declare them in the header? Also, consider renaming them: static Option<ContainerID> containerID(...); static string containerName(..); src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85635> This check seems to be redundant because you just have a if check above. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85646> Remove this log line as it should be logged by the caller. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85647> Ditto. Remove this redundant log line. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85656> IIUC, !taskInfo.has_command() shouldn't happen, because otherwise it would use the other version of the launch, right? If that's the case, could you please add a warning here. If that's not the case, it would be better to add a comment to explain in what situation this could have happened. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85654> Any reason to use bash here. Seems that we use /bin/sh everywhere else (e.g., in subprocess). src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85657> The errno might be reset by os::close. You probably want to save it before calling os::close. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85658> Wrap the comments with 70 char width. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85660> No need to call 'value()': LOG(WARNING) << "Ignore..." << containerId; src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85662> 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. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85663> We usually have the following format for logging: LOG(WARNING) << "..." << "..."; src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85664> Ditto. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85665> Ditto. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85668> 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? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85666> Ditto. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85667> Copy the TODO from cgroups/mem.cpp src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85671> 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. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85672> Should we set message for the Termination as well? src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/23771/#comment85673> Rename it to 'exists'? src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/23771/#comment85661> Use stringify(containerId) instead. src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/23771/#comment85684> 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. src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/23771/#comment85688> Probably add a comment here saying that test-executor will be downloaded. Have you consider making this test self contained? src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/23771/#comment85690> Hum, in this test, you constructed the RunState manually. Is it possible to use docker containerizer to launch tasks and then recover it? Similar to what we did in MesosContainerizerSlaveRecoveryTest? src/tests/docker_tests.cpp <https://reviews.apache.org/r/23771/#comment85681> We usually use: using namespace process; for tests. src/tests/docker_tests.cpp <https://reviews.apache.org/r/23771/#comment85682> Wrap the comments using 70 char width. Here and everywhere else. src/tests/environment.cpp <https://reviews.apache.org/r/23771/#comment85680> Kill this include? src/tests/environment.cpp <https://reviews.apache.org/r/23771/#comment85679> Hum, it's not your fault. What if the user define a test: DOCKER_BENCHMARK_... Are we gonna run the test even if flags.benchmark = false? - Jie Yu 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 > >
