----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16147/#review33866 -----------------------------------------------------------
Ship it! src/launcher/fetcher.cpp <https://reviews.apache.org/r/16147/#comment63529> s/code/status/ src/launcher/fetcher.cpp <https://reviews.apache.org/r/16147/#comment63530> We use "exit status" to stay consistent with POSIX and libc, i.e., you check the status via WEXITSTATUS, not WEXITCODE. ;) src/launcher/fetcher.cpp <https://reviews.apache.org/r/16147/#comment63533> s/uri/URI/ src/launcher/fetcher.cpp <https://reviews.apache.org/r/16147/#comment63534> s/Fetching uri/Fetching URI/ src/launcher/fetcher.cpp <https://reviews.apache.org/r/16147/#comment63536> I know this one isn't yours but if you see them please capitalize acronyms. src/launcher/fetcher.cpp <https://reviews.apache.org/r/16147/#comment63540> Option<std::string> user = os::hasenv("MESOS_USER") ? Option<std::string>(os::getenv("MESOS_USER")) : None(); Also, just to be clear, 'Some' doesn't work here? src/launcher/fetcher.cpp <https://reviews.apache.org/r/16147/#comment63539> return 0 (this function returns an int so we should return something). src/slave/containerizer/containerizer.hpp <https://reviews.apache.org/r/16147/#comment63597> IIUC, this has moved into mesos-fetcher, please kill. src/slave/containerizer/containerizer.hpp <https://reviews.apache.org/r/16147/#comment63598> IIUC, this has moved into mesos-fetcher, please kill. src/slave/containerizer/containerizer.hpp <https://reviews.apache.org/r/16147/#comment63599> IIUC, this has moved into mesos-fetcher, please kill. src/slave/containerizer/containerizer.hpp <https://reviews.apache.org/r/16147/#comment63600> const & src/slave/containerizer/containerizer.cpp <https://reviews.apache.org/r/16147/#comment63601> Please break the includes into directory "depths", even if it means more '#ifdef __linux__' are needed. src/slave/containerizer/containerizer.cpp <https://reviews.apache.org/r/16147/#comment63602> I don't think this is used anymore. src/slave/containerizer/containerizer.cpp <https://reviews.apache.org/r/16147/#comment63604> Moving it into the Containerizer makes the most sense to me, so feel free to kill these TODOs. src/slave/containerizer/containerizer.cpp <https://reviews.apache.org/r/16147/#comment63596> s/flags.resources.isSome() ? flags.resources.get() : ""/flags.resources.get("")/ src/slave/containerizer/containerizer.cpp <https://reviews.apache.org/r/16147/#comment63606> The 'if' statement way of doing this before was far more readable. src/slave/containerizer/containerizer.cpp <https://reviews.apache.org/r/16147/#comment63621> It's very non-standard to call a function on a libprocess directly (i.e., not dispatch). How about adding factory functions for each Isolator (i.e., Posix*Isolator::create(flags), Cgroups*Isolator::create(flags)) that returns a Try<Isolator*>? This would be an easy cleanup in the isolators. They could still use 'Isolator(Owned<IsolatorProcess>(process))'. src/slave/containerizer/mesos_containerizer.hpp <https://reviews.apache.org/r/16147/#comment63624> virtual src/slave/containerizer/mesos_containerizer.hpp <https://reviews.apache.org/r/16147/#comment63625> Please put all the instance variables below all the functions! src/slave/containerizer/mesos_containerizer.hpp <https://reviews.apache.org/r/16147/#comment63627> This appears to have been killed? src/slave/containerizer/mesos_containerizer.hpp <https://reviews.apache.org/r/16147/#comment63629> const& src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63630> Please put this one after the other "slave/*" with a newline in between. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63633> There are some helpers in stout in fatal.hpp that were subsumed by EXIT. I think we could put an async signal safe 'fatal' there that could be shared by others. How about a TODO? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63634> Do you want to ignore EINTR? src/slave/flags.hpp <https://reviews.apache.org/r/16147/#comment63587> s/be/been/ Also, please put the \n back. src/slave/flags.hpp <https://reviews.apache.org/r/16147/#comment63588> If this is no longer accessed please make it an Option. src/slave/monitor.cpp <https://reviews.apache.org/r/16147/#comment63590> Please indent +4 here. src/slave/monitor.cpp <https://reviews.apache.org/r/16147/#comment63593> This should all fit on one line now. src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment63677> Could you include a comment that explains the semantics around a failed or discarded future here? Are we guaranteed that container has been destroyed? src/slave/containerizer/mesos_containerizer.hpp <https://reviews.apache.org/r/16147/#comment63659> const & src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63639> I think it would be nice to replace stout's fatal with this async safe version since EXIT is a much better replacement in other circumstances. Maybe a TODO for now? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63635> What's with the '----'? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63637> I'm assuming that you're not including the error because the concatenation is not async signal safe? It would be great to comment on that at the top of the function. At the same time, calls like os::chown (and os::su, os::chdir, etc) likely do some potentially dangerous stuff (unless you checked, in which case comment on that too please), so concatenating the error message might not be such a big deal. The tradeoff here is that the process crashes, but it was already exiting anyway! And if I had to choose between the potential crash and some log message showing "Failed to chown work directory: No such user" versus just "Failed to chown work directory" the former would be that would be immensely more useful. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63638> s/Environment_Variable/Environment::Variable/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63640> Why not clean up and return a Failure? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63642> A small comment that you're assuming these rarely fail and therefore doing them in a check would be great. Also doesn't 'execute' above make sure it closes both ends of the pipe no matter what? What's this doing? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63643> If 'prepare' fails, do you clean up 'promises' and 'resources'? I think the semantics here are that the slave is expected to invoke 'destroy' if this future fails but it's not obvious so a comment would be good. Also, is there any reason why you didn't want to just do the destroy preemptively yourself? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63644> s/executor/URIs/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63645> s/executor/URIs/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63646> s/code/status/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63647> s/mesos-launcher/mesos-fetcher/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63648> s/-launcher/-fetcher/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63649> s/executor/URIs/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63650> Not your bug but the indentation here is incorrect. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63651> s|env|/usr/bin/env| src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63652> Subprocess should really take a map of environment variables. ;) src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63653> You're leaking a file descriptor. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63654> You're leaking a file descriptor. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63655> Leaking a file descriptor. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63656> Leaking a file descriptor. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63657> s/out/err/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63658> Should 'statuses' be cleaned up? Perhaps this relates to my cleanup concerns above? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63660> Ignore EINTR? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63661> Include string error of errno? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63662> s/.get()->/->/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63663> Why is this one discarded but the others are failed? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63664> This seems serious enough to have a JIRA ticket to follow up on this. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63668> & (I guess I missed it above.) src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63676> This seems like a scary place to be ... stuff is definitely "leaking" here right? Also, did you want to clean up 'promises', 'statuses', etc.? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63665> s/.get()->/->/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63666> Why "unknown error" instead of "future discarded" or "discarded future"? src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63675> The call to Isolator::watch is always going to require another thread to run 'watch' so even if the limitation is ready it's highly unlikely the future will be ready. I think you're better off only having the limitation message if 'limited' gets invoked below (and saving it until '__destroy' gets invoked). src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63672> s/.get()->/->/ src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment63673> Kill this. ;) - Benjamin Hindman On Feb. 7, 2014, 3:35 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16147/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2014, 3:35 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas > Nielsen, samya, and Jason Dusek. > > > Repository: mesos-git > > > Description > ------- > > The proposed Containerizer interface is to replace the existing Isolator. > > One ContainerizerProcess has been written: > MesosContainerizerProcess - implements containerizeration internally using a > Launcher and one or more Isolators (following review) > > The intent is to also support a generic ExternalContainerizerProcess that can > delegate containerizeration by making external calls. Other Containerizers > could interface with specific external containerization techniques such as > Docker or LXC. > > > Diffs > ----- > > include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8 > src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 > src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8 > src/launcher/fetcher.cpp PRE-CREATION > src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a > src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af > src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf > src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905 > src/slave/cgroups_isolator.hpp 1a66dc630f0857db48b7cfb316fbbb15518fcec2 > src/slave/cgroups_isolator.cpp ef7dd682e5462d0158d6ea6654246d77000e9778 > src/slave/containerizer/containerizer.hpp PRE-CREATION > src/slave/containerizer/containerizer.cpp PRE-CREATION > src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION > src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION > src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd > src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3 > src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608 > src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f > src/slave/main.cpp 6a7131635218181bd85beb7f097c2718f16fa734 > src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f > src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071 > src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc > src/slave/process_isolator.hpp bc52f33d8e83fe026be712c8b15e689eb23dd65c > src/slave/process_isolator.cpp 09cb9968c914a095b5737b70bf5044865fc3a2c4 > src/slave/slave.hpp 891c874397f5add9b70432c40c152f7f19922e34 > src/slave/slave.cpp 1c0502e4b88719f18ecb0caface17b8511e9257b > src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321 > src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93 > src/slave/status_update_manager.hpp > 06ea4659cdd24cb1b82f389f404566ba14a663fb > src/slave/status_update_manager.cpp > 03f5eafefd6ed748bfd4511d654c23c7b460db66 > > Diff: https://reviews.apache.org/r/16147/diff/ > > > Testing > ------- > > > Thanks, > > Ian Downes > >
