----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23771/#review49240 -----------------------------------------------------------
src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86123> CHECK_READY(s.status()); CHECK_SOME(s.status().get()); src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86128> See my comments below. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86146> This has been gone through a few iterations already. Sorry, this is my fault that I haven't thought about it in deep. But I hope we can resolve this before shipping this. A few issues: 1) This is a library, we usually don't have LOG() in the library (otherwise, very likely we'll have double logging). Instead, we usually put error messages in the return value (either Future or Try). 2) Returning 'bool' doesn't seem to be necessary? We always return false on error and return true on success, right? In that case, we can just use Future<Nothing>? So here is what I would suggest (s/success/check/ maybe): Future<Nothing> check(const string& cmd, const Subprocess& s) { return s.status().then(lambda::bind(&_success, cmd, s); } Future<Nothing> _check(const string& cmd, const Subprocess& s) { Option<int> status = s.status().get(); if (status.isNone()) { return Failure("No status found for '" + cmd + "'"); } if (status.get() != 0) { return err(s).then(lambda::bind(&__check, cmd, status, lambda::_1)); } return Nothing(); } Future<Nothing> __check(const string& cmd, int status, const string& err) { return Failure( "Failed to '" + cmd + "': exit status = " + WSTRINGIFY(status) + " stderr = " + err); } // Retrieve the standard error of 's'. Future<string> err(const Subprocess& s) { CHECK_SOME(s.err()); Try<Nothing> nonblock = os::nonblock(s.err().get()); if (nonblock.isError()) { return Failure("Cannot set nonblock for stderr"); } return io::read(s.err().get()); } src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86122> Add a CHECK_READY(s.status()); src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86147> Here, you can just do: return check(cmd, s); src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86166> You should be able to chain remove with check: return check(cmd, s).then(lambda::bind(&__kill, *this, container, lambda::_1)); __kill(docker, container, force) { docker.rm(container, force); } src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86121> This should fit in the above line? src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86168> pass in cmd here src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86167> if (status.isNone()) { return Failure(...); } src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86169> if (status.get() != 0) { return err(s).then(lambda::bind(&__check, cmd, status.get(), lambda::_1)); } src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86182> Ditto. Passing cmd in. src/docker/docker.cpp <https://reviews.apache.org/r/23771/#comment86181> Ditto. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment86185> If 'docker.run' returns Nothing, you won't have a chance to execute this code, but I think it's ok because Self::destroy will do 'docker.kill' in anyway. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment86187> Logging format please. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment86189> Logging format. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/23771/#comment86190> Ditto. src/tests/environment.cpp <https://reviews.apache.org/r/23771/#comment86199> I think this has been fixed. YOu need to do a rebase. src/tests/environment.cpp <https://reviews.apache.org/r/23771/#comment86195> Hum, you switch the order. But that doesn't solve the problem. My point here is that: This function should never return true early, otherwise, you'll miss the following checks. So it should be if (docker.isError()) { return false; } #ifdef __linux__ if (user.get() != "root") { return false; } #endif src/tests/environment.cpp <https://reviews.apache.org/r/23771/#comment86198> This is buggy too (not your fault). Do a rebase, it should be fixed. If not, fix it. - Jie Yu On July 31, 2014, 8:06 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23771/ > ----------------------------------------------------------- > > (Updated July 31, 2014, 8:06 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 > >
