----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23771/#review48287 -----------------------------------------------------------
This is the first half of the review from Jie and Ian. We will continue tomorrow. src/common/status_utils.hpp <https://reviews.apache.org/r/23771/#comment85012> Fix include <> src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment84732> Is there any check on compatibility with the Docker version? What about having a generic Docker abstraction with DockerCLI as one implementation? src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment85014> Better if this was: Try<Docker*> DockerCLI::create(const string& path) Try<Docker*> DockerREST::create(const string& sockFile) Because you've created the instance and then later you validate it. The create function can do whatever validation is necessary. src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment85015> For consistency the argument should be _json. src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment84727> s/Pid/pid/ src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment85016> Make private once using ::create. src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment85017> Remove const for the general Docker interface (and for the DockerCLI too). Other implementors may want to change the object. Return the Subprocess status is only really for the DockerCLI and it isn't actually used. Better to return a Future<bool> or some Docker specific error type. The error structure could have the error code and error string. src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment85018> Ditto remove const for this and other methods. Ditto for return value of other methods. Check with the REST API and return something relevant to both CLI and REST. src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment84781> no need for const on basic types. src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment85021> s/Depreciate/Deprecate/ s/the// If you want to support "rm --kill" then just support this as a flag to kill now and clean up comments. src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment84782> ditto src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment84783> separate with newlines. src/docker/docker.hpp <https://reviews.apache.org/r/23771/#comment85032> Can you make this into a libprocess object and then defer to these non-static methods? The lifecycle of the object you pass in the lambda::binds is not clear. Any you can make delayed calls later if necessary. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84730> kill newline and re-order. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84731> s/Docker &docker/Docker& docker/ src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84758> Why is this necessary. Shouldn't we let Docker decide if it can run containers? All validation should be moved into the Docker::create function. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85038> Docker::info can be moved into Docker::create since it's only used here. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85039> Can the json parsing be done in a single place, say into a Try<Docker::Container> Docker::Container::create(json) rather than parsing every time. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84761> Should these actually be CHECKs - can you not make these Try<>s and return Errors? src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84777> consistency, newline before return? src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84778> Two lines between functions. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84779> ditto src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84733> s/yifan)/yifan):/ src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84776> ditto, newline. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84785> ditto src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84768> kill newline src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84770> why the leading whitespace here? what about starting with path + " run -d" here and appending the arguments? src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84772> You could clean this up a little with the min/max using Option(s) provided by stout, e.g.: template <typename T> Option<T> max(const T& left, const Option<T>& right) src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85044> Instead of escaping character(s) you should use the execve-style version of subprocess which takes a vector<string> for argv. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85042> Any specific reason why you use --net=host here? Should this be exposed so bridge can be used too? src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84773> Why are you using pipes for std* here? You don't read/write to them and there's the possibility of blocking the subprocess if you fill a buffer. If they aren't needed then redirect to /dev/null. Subprocess::PATH("/dev/null") src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84775> ditto newline. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84774> ditto. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85046> ditto src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85047> This should be moved into kill and enabled with optional flag. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84786> Do you want to log this or is it not unexpected? src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84780> newline src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84787> Ditto, does anyone do anything with these pipes? src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84798> Can this replace the existing stout os::read()? It should go into stout regardless. If you're reading asynchronously using io::read(fd) then you don't need this function. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment84799> This should definitely be done, otherwise the subprocess could be blocked! src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85050> kill newline src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85051> If it doesn't exist then docker inspect should exit 1 so it won't even reach here. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85052> Redundant because this is from .then() src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85054> This seems very fragile.... Perhaps add a comment warning about this! src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment85055> This should get moved to Docker::create() src/linux/cgroups.hpp <https://reviews.apache.org/r/23771/#comment84725> Every pid is at least a member of the root cgroup for any attached subsystem - drop the last part of this sentence. src/linux/cgroups.hpp <https://reviews.apache.org/r/23771/#comment84724> ditto src/linux/cgroups.cpp <https://reviews.apache.org/r/23771/#comment84722> Default constructor is none. src/linux/cgroups.cpp <https://reviews.apache.org/r/23771/#comment84691> Don't you mean memory? src/linux/cgroups.cpp <https://reviews.apache.org/r/23771/#comment84692> Ditto - you should be finding the memory cgroup. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment84800> Clean up parts of the commenting that don't apply now, e.g., this code doesn't use the launcher. src/slave/flags.hpp <https://reviews.apache.org/r/23771/#comment84726> I assume that this must be the absolute path? is $PATH searched? Can I specify a relative path...? src/linux/cgroups.hpp <https://reviews.apache.org/r/23771/#comment85060> Add test for this and other new cgroups functions. src/linux/cgroups.hpp <https://reviews.apache.org/r/23771/#comment85059> s/subsytem/subsystem/ src/linux/cgroups.hpp <https://reviews.apache.org/r/23771/#comment85062> if the subsystem is mounted, every pid is in either the root cgroup or another cgroup. src/linux/cgroups.hpp <https://reviews.apache.org/r/23771/#comment85061> s/subsytem/subsystem/ src/slave/containerizer/docker.hpp <https://reviews.apache.org/r/23771/#comment85064> Fits on a single line? src/slave/containerizer/docker.hpp <https://reviews.apache.org/r/23771/#comment85065> This can be private. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85066> Consistency, name arguments _flags and _docker. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85068> kill extra newline src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85069> no need for const reference src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85070> ditto src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85071> consistency, just call variable validate src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85072> pull this out of the ifdef so the limits are always known? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment85073> write a helper function which returns the docker container name. - Ian Downes On July 22, 2014, 11:12 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23771/ > ----------------------------------------------------------- > > (Updated July 22, 2014, 11:12 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/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/mesos_test_executor_docker_image/Dockerfile PRE-CREATION > src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION > 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 > >
