Return-Path: X-Original-To: apmail-mesos-dev-archive@www.apache.org Delivered-To: apmail-mesos-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id F1625106A9 for ; Fri, 24 Jan 2014 01:59:07 +0000 (UTC) Received: (qmail 8217 invoked by uid 500); 24 Jan 2014 01:59:07 -0000 Delivered-To: apmail-mesos-dev-archive@mesos.apache.org Received: (qmail 8177 invoked by uid 500); 24 Jan 2014 01:59:07 -0000 Mailing-List: contact dev-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@mesos.apache.org Delivered-To: mailing list dev@mesos.apache.org Received: (qmail 8160 invoked by uid 99); 24 Jan 2014 01:59:06 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 24 Jan 2014 01:59:06 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 898A51D45D5; Fri, 24 Jan 2014 01:59:05 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============0569890171646576838==" MIME-Version: 1.0 Subject: Re: Review Request 16147: Containerizer (part 1) From: "Adam B" To: "Benjamin Hindman" , "Chi Zhang" , samyad@gmail.com, "Niklas Nielsen" , "Ben Mahler" , "Jason Dusek" Cc: "Adam B" , "mesos" , "Ian Downes" Date: Fri, 24 Jan 2014 01:59:05 -0000 Message-ID: <20140124015905.3932.76788@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Adam B" X-ReviewGroup: mesos X-ReviewRequest-URL: https://reviews.apache.org/r/16147/ X-Sender: "Adam B" References: <20140121193610.11320.84440@reviews.apache.org> In-Reply-To: <20140121193610.11320.84440@reviews.apache.org> Reply-To: "Adam B" X-ReviewRequest-Repository: mesos-git --===============0569890171646576838== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16147/#review32670 ----------------------------------------------------------- Finally got around to reviewing the new containerizer files. Looks great, only some minor comments. src/slave/container/containerizer.hpp Comment what the map return value represents, please? Looks like environment variables mapped to their values. src/slave/container/containerizer.cpp Do you need to use "process::" since you're "using namespace process" above? src/slave/container/containerizer.cpp Lost some TODOs in the move that might be useful: // TODO(benh): Move this computation into Flags as the "default". // TODO(vinod): Move some of this computation into Resources. src/slave/container/containerizer.cpp If resources = parsed.get(), and !resources.cpus().isSome(), then how can parsed.get().cpus().isSome() be true? Maybe I'm missing something here about Resources and Try<>, but it looks like unreachable code to me. Ditto for mem. src/slave/container/containerizer.cpp Lost the old comment, including BenH's TODO: // Leave 1 GB free if we have more than 1 GB, otherwise, use all! // TODO(benh): Have better default scheme (e.g., % of mem not // greater than 1 GB?) Shouldn't we put it back in? src/slave/container/containerizer.cpp resources += Resources::parse(...? Like you did for cpu/mem. Same applies to 'ports' below. I'd like to see the style consistent for each of these four resources. src/slave/container/containerizer.cpp Could include the default translation (e.g. process -> "posix/cpu,posix/mem") in log/warn message, so a user knows what to replace their deprecated isolation options with. src/slave/container/containerizer.cpp Will isolator.error() include type in its string? Seems useful. src/slave/container/containerizer.cpp PosixLauncher can be used on Linux too, right? src/slave/container/containerizer.cpp Might want to give a different error message from below, to indicate an unrecognized-file-extension error as opposed to the command failing. src/slave/container/containerizer.cpp s/htfp/hftp/ src/slave/container/containerizer.cpp from to ? src/slave/container/containerizer.cpp Shouldn't path::join be "making it: " and not uri (the original relative path)? src/slave/container/containerizer.cpp More specific error message? src/slave/container/containerizer.cpp s/uri/local/ ? src/slave/container/mesos_containerizer.hpp And if state.isNone(), it terminates/cleans-up all executors? Or recovers all? src/slave/container/mesos_containerizer.hpp There were talks (with BenH) of moving the Reaper into the launcher, but that can be a later review/change - Adam B On Jan. 21, 2014, 11:36 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16147/ > ----------------------------------------------------------- > > (Updated Jan. 21, 2014, 11:36 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 655f86757487ddbe551fdcf53eb793e773ecdd34 > src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454 > src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1 > 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 e86062e9abaaa263c32c55e9dbfefd700f605886 > src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735 > src/slave/container/containerizer.hpp PRE-CREATION > src/slave/container/containerizer.cpp PRE-CREATION > src/slave/container/external_containerizer.hpp PRE-CREATION > src/slave/container/mesos_containerizer.hpp PRE-CREATION > src/slave/container/mesos_containerizer.cpp PRE-CREATION > src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd > src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc > src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608 > src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f > src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37 > src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578 > src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa > src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf > src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b > src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01 > src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4 > src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84 > src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321 > src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1 > 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 > > --===============0569890171646576838==--